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

[Simon Horman <horms@kernel.org>]

On Thu, May 14, 2026 at 04:33:47PM +0200, Jonas Jelonek wrote:
> Hi,
> 
> On 10.05.26 18:47, Simon Horman wrote:
> > [ ... ]
> >> +	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.
> >
> 
> The concern is structurally valid - the single i2c_block_size does
> conflate the two direction caps, and on an adapter with
> max_write_len < max_read_len the read path would be throttled while
> sfp_i2c_write (which doesn't chunk) wouldn't actually be protected by
> the write clamp.
> 
> However, a sweep of drivers/i2c/busses/ shows no in-tree adapter
> currently sets max_write_len < max_read_len; the only asymmetric
> quirks go the other way (read-only caps, or read ≤ write). In every
> existing case min(max_read_len, max_write_len) collapses to the read
> cap, so the clamp behaves identically to the situation without this
> change.
> 
> Given that this only has theoretical implications and there won't be a
> real-world impact right now, I'd keep this as-is. But if handling this
> properly is desired, I can add another patch splitting the size caps
> per direction and adding proper handling to sfp_i2c_write.

Thanks, I'm also unsure if it's worth fixing if it doesn't effect anything
in practice.

> 
> >> +
> >> +	sfp->i2c_max_block_size = max_block_size;
> >>  	return 0;
> >>  }
> 
> Regards,
> Jonas Jelonek