Re: [PATCH] net: usb: lan78xx: Fix lost EEPROM write timeout error(-ETIMEDOUT) in lan78xx_write_raw_eeprom

[Bhanu Seshu Kumar Valluri]

On 07/10/25 00:30, Khalid Aziz wrote:
> On 10/3/25 10:07 PM, Bhanu Seshu Kumar Valluri wrote:
>> The function lan78xx_write_raw_eeprom failed to properly propagate EEPROM
>> write timeout errors (-ETIMEDOUT). In the timeout  fallthrough path, it first
>> attempted to restore the pin configuration for LED outputs and then
>> returned only the status of that restore operation, discarding the
>> original timeout error saved in ret.
>>
>> As a result, callers could mistakenly treat EEPROM write operation as
>> successful even though the EEPROM write had actually timed out with no
>> or partial data write.
>>
>> To fix this, handle errors in restoring the LED pin configuration separately.
>> If the restore succeeds, return any prior EEPROM write timeout error saved
>> in ret to the caller.
>>
>> Suggested-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
>> Fixes: 8b1b2ca83b20 ("net: usb: lan78xx: Improve error handling in EEPROM and OTP operations")
>> Signed-off-by: Bhanu Seshu Kumar Valluri <bhanuseshukumar@xxxxxxxxx>
>> ---
>>   Note:
>>   The patch is compiled and tested.
>>   The patch was suggested by Oleksij Rempel while reviewing a fix to a bug
>>   found by syzbot earlier.
>>   The review mail chain where this fix was suggested is given below.
>>   https://lore.kernel.org/all/aNzojoXK-m1Tn6Lc@xxxxxxxxxxxxxx/
>>
>>   drivers/net/usb/lan78xx.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>> index d75502ebbc0d..5ccbe6ae2ebe 100644
>> --- a/drivers/net/usb/lan78xx.c
>> +++ b/drivers/net/usb/lan78xx.c
>> @@ -1174,10 +1174,13 @@ static int lan78xx_write_raw_eeprom(struct lan78xx_net *dev, u32 offset,
>>       }
>>     write_raw_eeprom_done:
>> -    if (dev->chipid == ID_REV_CHIP_ID_7800_)
>> -        return lan78xx_write_reg(dev, HW_CFG, saved);
>> -
>> -    return 0;
>> +    if (dev->chipid == ID_REV_CHIP_ID_7800_) {
>> +        int rc = lan78xx_write_reg(dev, HW_CFG, saved);
>> +        /* If USB fails, there is nothing to do */
>> +        if (rc < 0)
>> +            return rc;
>> +    }
>> +    return ret;
>>   }
>>     static int lan78xx_read_raw_otp(struct lan78xx_net *dev, u32 offset,
> 
> You were able to test the change to read eeprom code by forcing a timeout while doing probe on EVB-LAN7800LC. Were you able to test this code change the same way just to make sure callers of the write function handle the new ETIMEDOUT return value correctly?
> 
> Thanks,

Hi Khalid,

This function is only invoked from user's ethtool operations.  The ethtool handles errors returned from the driver callback functions. 
I tested it with ethtool -E option by forcing a ETIMEDOUT error early in the lan78xx_write_raw_eeprom temporarily. The ethtool 
reported error with "Cannot set EEPROM data" message. The ethtool version used is 5.16.

Regards,
Bhanu Seshu Kumar Valluri