et1011c_read_status() stores the return value of phy_read() in a u32 and does not check it for an error. If the read of the configuration register fails, the negative error code goes through the read-modify-write and its low 16 bits are written back to the PHY, for example 0xdffb after -EIO. That sets 14 of the 16 bits of the register. The interface field (ET1011C_INTERFACE_MASK), for one, ends up as 3 instead of ET1011C_GMII_INTERFACE. The function still returns the result of genphy_read_status(), normally 0, so nothing reports the failure. If the read of the status register fails, the error code is compared with the speed bits. For the usual error codes this does not look like 1000 Mbit/s, and the update of the configuration register is silently skipped. Make val an int and return the error of either read, like other PHY drivers do. phylib then handles it like any other failed read_status(): it triggers a WARN and puts the PHY into the error state, with the link down until the interface is brought down and up again. Discovered by an AI agent (LLM) inspecting the code. The issue was not seen on a real system. It was confirmed with a KUnit test, which is not part of this patch and uses error injection: it binds the driver to a PHY on a fake MDIO bus, brings the link up at 1000 Mbit/s and makes one phy_read() fail. After -EIO it sees the value above written to the fake bus (UML on x86-64). With this patch the same test sees no write and gets the error back. Not tested on hardware (no ET1011C available). Kernel selftests were not run (none mentions this driver). Fixes: dbb7a95d810a ("phy: Add LSI ET1011C PHY driver") Signed-off-by: Elias Pukari --- Notes: How this was found and tested, since no hardware was involved: The problem was found by an LLM (Claude) reading the driver. The patch and the test were drafted with Claude as well. I have read both and run the test myself. The test is a KUnit test with a fake MDIO bus. It binds the real driver, brings the link up at 1000 Mbit/s and makes one phy_read() fail. On net-next d1f4b7c6b997 and on v7.3 (UML on x86-64, with and without KASAN): before config read fails with -EIO 0xdffb written, returns 0 config read fails with -ETIMEDOUT 0xdf92 written, returns 0 status read fails with -EIO nothing written, returns 0 after nothing is written and the error is returned, in all cases The second value depends on the architecture: -ETIMEDOUT is -110 here, but not on mips or parisc, for example. The test is not part of this patch. I can post it if it is useful. Also built with W=1 for x86-64 (gcc and clang) and for 32-bit x86, allmodconfig. I do not have an ET1011C, so this is not tested on hardware. For that reason it is sent for net-next and without Cc: stable. I can resend it for net if you prefer. Not changed here, to keep the fix small: - "speed" is a static variable, shared by all ET1011C PHYs in the system, and it is updated before the registers are read; - the return value of phy_write() is ignored, here and in et1011c_config_aneg(). drivers/net/phy/et1011c.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/net/phy/et1011c.c b/drivers/net/phy/et1011c.c index 6cd8d7758..603fe5a1e 100644 --- a/drivers/net/phy/et1011c.c +++ b/drivers/net/phy/et1011c.c @@ -62,16 +62,22 @@ static int et1011c_read_status(struct phy_device *phydev) { static int speed; int ret; - u32 val; + int val; ret = genphy_read_status(phydev); if (speed != phydev->speed) { speed = phydev->speed; val = phy_read(phydev, ET1011C_STATUS_REG); + if (val < 0) + return val; + if ((val & ET1011C_SPEED_MASK) == ET1011C_GIGABIT_SPEED) { val = phy_read(phydev, ET1011C_CONFIG_REG); + if (val < 0) + return val; + val &= ~ET1011C_TX_FIFO_MASK; phy_write(phydev, ET1011C_CONFIG_REG, val | ET1011C_GMII_INTERFACE | base-commit: d1f4b7c6b997c29de67ad7178ababac2328cb1b4 -- 2.56.1