Re: [PATCH net-next v8 1/2] net: sfp: apply I2C adapter quirks to limit block size

[Jonas Jelonek <jelonek.jonas@gmail.com>]

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: apply I2C adapter quirks to limit block size
>
> This patch modifies the SFP driver to respect I2C adapter quirks when
> configuring the I2C block size. It restricts the maximum block size
> based on the adapter's max_read_len and max_write_len fields.
>
>> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
>> index bd970f753beb6..e58e29a1e8d27 100644
>> --- a/drivers/net/phy/sfp.c
>> +++ b/drivers/net/phy/sfp.c
>> @@ -807,21 +807,29 @@ static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
>>  
>>  static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
>>  {
> [ ... ]
>
>>  	} else {
>>  		sfp->i2c = NULL;
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (i2c->quirks && i2c->quirks->max_read_len)
>> +		max_block_size = min(max_block_size, i2c->quirks->max_read_len);
>> +	if (i2c->quirks && i2c->quirks->max_write_len)
>> +		max_block_size = min(max_block_size, i2c->quirks->max_write_len);
> Does this limit the read block size incorrectly based on the write limit?
>
> The write portion of an I2C read transfer is always 1 byte for the EEPROM
> offset, regardless of the read block size.
>
> If an adapter has a max_write_len of 1, max_block_size will drop to 1.
> Since sfp_hwmon_probe() checks if sfp->i2c_max_block_size < 2 to ensure
> atomic 16-bit reads, does this inadvertently disable hardware monitoring for
> adapters that have a small write limit but sufficient read capabilities?
>
> Also, for actual write transfers, sfp_i2c_write() prepends a 1-byte register
> address to the payload. If max_block_size is set exactly to max_write_len,
> won't the resulting message length be 1 + max_write_len, which still exceeds
> the adapter's quirk limit?
>
> Additionally, looking at sfp_i2c_write(), it does not appear to loop or
> chunk the buffer using sfp->i2c_max_block_size. Does this mean the new
> limit will be ignored during writes, and the function will still attempt
> to transfer the entire buffer in a single operation?

Valid concern but it seems like this doesn't have a practical impact right
now. Having a closer look at in-tree drivers shows that

(a) i2c-tegra (read-cap: 4096 | write-cap: 4084)
(b) i2c-tegra194 (read-cap: no | write-cap: 65524)
(c) i2c-mlxbf (read-cap: 128 | write-cap: 127)
(d) i2c-qcom-cci (read-cap: 12 | write-cap: 10/11)

seem to have an asymmetry between read cap and write cap. However,
the actual limits for (1), (2) and (3) are far beyond the 16 bytes the SFP
driver uses. For (d) it is below but would trigger the hwmon disable.
Moreover, qcom-cci seems to be a driver running camera sensors,
nothing that will ever be used for SFP cages. Thus I would acknowledge
this but not add a fix for that.

On other opinions, please raise your hand. Then I might provide
another patch implementing per-direction caps, or any other
suggested solution.

>> +
>> +	sfp->i2c_max_block_size = max_block_size;
>>  	return 0;
>>  }

Regards,
Jonas