Re: [PATCH net-next v8 2/2] net: sfp: extend SMBus support

[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: extend SMBus support

Extend SFP SMBus read/write helpers to use SMBus word access and I2C block
access when advertised by the adapter, picking the largest supported transfer
per chunk.

> 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
> @@ -756,50 +757,110 @@ static int sfp_i2c_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
> [ ... ]
> -static int sfp_smbus_byte_read(struct sfp *sfp, bool a2, u8 dev_addr,
> -			       void *buf, size_t len)
> +static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
> +			  size_t len)
>  {
> -	union i2c_smbus_data smbus_data;
> +	union i2c_smbus_data smbus_data = {0};
>  	u8 bus_addr = a2 ? 0x51 : 0x50;
> +	size_t this_len, transferred;
> +	u32 functionality;
>  	u8 *data = buf;
>  	int ret;
>  
> -	while (len) {
> -		ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> -				     I2C_SMBUS_READ, dev_addr,
> -				     I2C_SMBUS_BYTE_DATA, &smbus_data);
> -		if (ret < 0)
> -			return ret;
> +	functionality = i2c_get_functionality(sfp->i2c);
>  
> -		*data = smbus_data.byte;
> +	while (len) {
> +		this_len = min(len, sfp->i2c_block_size);
> +

Does this introduce an -EINVAL regression for ethtool -m during the initial
300ms module insertion window?

When an SFP module is inserted, sfp_check_state() sets the SFP_F_PRESENT
flag and schedules sfp_sm_mod_probe() to run after a 300ms delay.
Because sfp->i2c_block_size isn't initialized until sfp_sm_mod_probe()
runs, it remains 0 during this window.

If ethtool -m is run during this 300ms window, sfp_module_eeprom() proceeds
because SFP_F_PRESENT is set. This means this_len will evaluate to 0.
For adapters supporting I2C_FUNC_SMBUS_READ_I2C_BLOCK, the code attempts
a 0-length I2C block read, which the I2C subsystem core explicitly rejects
with -EINVAL.

Prior to this patch, sfp_smbus_byte_read() bypassed the block size entirely
and safely executed 1-byte transfers during the 300ms window.

This also seems to expose a pre-existing deadlock in the pure I2C path.
For pure I2C controllers, sfp_i2c_read() is used. If this_len evaluates
to 0, i2c_transfer() is called with a 0-length read message. If the I2C
adapter returns success, the loop variables remain unchanged, causing an
infinite loop while holding rtnl_lock, which deadlocks the networking stack.

Is there a way to prevent reads before sfp->i2c_block_size is properly
initialized, or fall back to 1-byte reads when the block size is 0?

> +		if (functionality & I2C_FUNC_SMBUS_READ_I2C_BLOCK) {
> +			smbus_data.block[0] = this_len;
> +			ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> +					     I2C_SMBUS_READ, dev_addr,
> +					     I2C_SMBUS_I2C_BLOCK_DATA, &smbus_data);
> +			if (ret < 0)
> +				return ret;
> +
> +			memcpy(data, &smbus_data.block[1], this_len);
> +			transferred = this_len;