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

[Jakub Kicinski <kuba@kernel.org>]

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?

> +
> +	sfp->i2c_max_block_size = max_block_size;
>  	return 0;
>  }
-- 
pw-bot: cr