Re: [net-next,v5] 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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: sfp: extend SMBus support

This patch extends SMBus support in the SFP driver from single-byte
accesses to also support SMBus I2C block and word access, enabling more
SMBus controllers to work with SFP modules.

> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 84bef5099dda..8f0b34a93ae8 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c

[ ... ]

> @@ -759,31 +773,55 @@ static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
> +static int sfp_smbus_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
> +			   size_t len)
>  {
>  	union i2c_smbus_data smbus_data;
>  	u8 bus_addr = a2 ? 0x51 : 0x50;
> +	size_t this_len, transferred;
> +	u32 functionality;
>  	u8 *data = buf;
>  	int ret;
>
> +	functionality = i2c_get_functionality(sfp->i2c);
> +
>  	while (len) {
> -		smbus_data.byte = *data;
> -		ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> -				     I2C_SMBUS_WRITE, dev_addr,
> -				     I2C_SMBUS_BYTE_DATA, &smbus_data);
> -		if (ret)
> +		this_len = min(len, sfp->i2c_max_block_size);
> +
> +		if (this_len > 2 &&
> +		    functionality & I2C_FUNC_SMBUS_WRITE_I2C_BLOCK) {
> +			smbus_data.block[0] = this_len;
> +			memcpy(&smbus_data.block[1], data, this_len);
> +
> +			ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> +					     I2C_SMBUS_WRITE, dev_addr,
> +					     I2C_SMBUS_WORD_DATA, &smbus_data);
                                             ^^^^^^^^^^^^^^^^^

Should this be I2C_SMBUS_I2C_BLOCK_DATA instead of I2C_SMBUS_WORD_DATA?

The code checks for I2C_FUNC_SMBUS_WRITE_I2C_BLOCK support and sets up
smbus_data.block[] with the length byte and data for a block transfer,
but then calls i2c_smbus_xfer() with I2C_SMBUS_WORD_DATA protocol.

For comparison, the read function correctly uses I2C_SMBUS_I2C_BLOCK_DATA
when I2C_FUNC_SMBUS_READ_I2C_BLOCK is supported:

    if (this_len > 2 &&
        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);

With I2C_SMBUS_WORD_DATA, only 2 bytes from smbus_data.word would be
written instead of the this_len bytes set up in smbus_data.block[].
The pointer then advances by transferred (this_len) bytes, effectively
skipping data that was never written to the device.

> +			transferred = this_len;

[ ... ]
-- 
pw-bot: cr