Re: [PATCH net-next v8 2/2] net: sfp: extend SMBus support

[Jonas Jelonek]

Hi,

On 21.05.26 01:42, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: sfp: extend SMBus support
>
> Extend SFP SMBus read/write helpers to use SMBus word access and I2C block
> access when advertised by the adapter, picking the largest supported transfer
> per chunk.
>
>> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
>> --- a/drivers/net/phy/sfp.c
>> +++ b/drivers/net/phy/sfp.c
>> @@ -756,50 +757,110 @@ static int sfp_i2c_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
>> [ ... ]
>> -static int sfp_smbus_byte_read(struct sfp *sfp, bool a2, u8 dev_addr,
>> -			       void *buf, size_t len)
>> +static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
>> +			  size_t len)
>>  {
>> -	union i2c_smbus_data smbus_data;
>> +	union i2c_smbus_data smbus_data = {0};
>>  	u8 bus_addr = a2 ? 0x51 : 0x50;
>> +	size_t this_len, transferred;
>> +	u32 functionality;
>>  	u8 *data = buf;
>>  	int ret;
>>  
>> -	while (len) {
>> -		ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
>> -				     I2C_SMBUS_READ, dev_addr,
>> -				     I2C_SMBUS_BYTE_DATA, &smbus_data);
>> -		if (ret < 0)
>> -			return ret;
>> +	functionality = i2c_get_functionality(sfp->i2c);
>>  
>> -		*data = smbus_data.byte;
>> +	while (len) {
>> +		this_len = min(len, sfp->i2c_block_size);
>> +
> Does this introduce an -EINVAL regression for ethtool -m during the initial
> 300ms module insertion window?
>
> When an SFP module is inserted, sfp_check_state() sets the SFP_F_PRESENT
> flag and schedules sfp_sm_mod_probe() to run after a 300ms delay.
> Because sfp->i2c_block_size isn't initialized until sfp_sm_mod_probe()
> runs, it remains 0 during this window.
>
> If ethtool -m is run during this 300ms window, sfp_module_eeprom() proceeds
> because SFP_F_PRESENT is set. This means this_len will evaluate to 0.
> For adapters supporting I2C_FUNC_SMBUS_READ_I2C_BLOCK, the code attempts
> a 0-length I2C block read, which the I2C subsystem core explicitly rejects
> with -EINVAL.
>
> Prior to this patch, sfp_smbus_byte_read() bypassed the block size entirely
> and safely executed 1-byte transfers during the 300ms window.
>
> This also seems to expose a pre-existing deadlock in the pure I2C path.
> For pure I2C controllers, sfp_i2c_read() is used. If this_len evaluates
> to 0, i2c_transfer() is called with a 0-length read message. If the I2C
> adapter returns success, the loop variables remain unchanged, causing an
> infinite loop while holding rtnl_lock, which deadlocks the networking stack.
>
> Is there a way to prevent reads before sfp->i2c_block_size is properly
> initialized, or fall back to 1-byte reads when the block size is 0?

Valid finding. I would fix this by setting sfp->i2c_block_size in
sfp_i2c_configure to the known adapter maximum size at that point.
This way, it is filled non-zero and the issue cannot come up.

If there's another suggestion, please raise your hand.

>> +		if (functionality & I2C_FUNC_SMBUS_READ_I2C_BLOCK) {
>> +			smbus_data.block[0] = this_len;
>> +			ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
>> +					     I2C_SMBUS_READ, dev_addr,
>> +					     I2C_SMBUS_I2C_BLOCK_DATA, &smbus_data);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			memcpy(data, &smbus_data.block[1], this_len);
>> +			transferred = this_len;

Regards,
Jonas