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

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: sfp: apply I2C adapter quirks to limit block size

This commit limits the SFP I2C block size based on the I2C adapter's
max_read_len and max_write_len quirks.

> 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
[ ... ]
> @@ -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)
>  {
[ ... ]
> +	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 applying max_write_len to max_block_size inadvertently throttle reads
and break hardware monitoring?

The max_block_size dictates sfp->i2c_max_block_size, which subsequently sets
sfp->i2c_block_size. This block size is exclusively used by sfp_i2c_read()
to chunk read transfers. The write message in a chunked sfp_i2c_read()
operation is always exactly 1 byte (the register address), irrespective of
the read payload size. Restricting the read chunk size based on
max_write_len appears to cause unintended side effects.

If an adapter has a max_write_len of 1, the i2c_block_size becomes 1. This
forces 1-byte read chunking and explicitly skips hwmon device registration
since sfp_hwmon_probe() requires an i2c_block_size >= 2.

Will this change actually protect writes against the max_write_len quirk?

Looking at sfp_i2c_write(), it ignores sfp->i2c_block_size and does not
implement chunking. It unconditionally allocates 1 + len bytes and submits
a single i2c_msg. Any write where 1 + len > max_write_len will still be
rejected by the I2C core.

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