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

[Jonas Jelonek]

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.

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

Regards,
Jonas Jelonek