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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Jonas,

On 5/16/26 15:54, Jonas Jelonek wrote:
> Commit 7662abf4db94 ("net: phy: sfp: Add support for SMBus module access")
> added SMBus access for SFP modules, but limited it to single-byte
> transfers. As a side effect, hwmon is disabled (16-bit reads cannot be
> guaranteed atomic) and a warning is printed.
> 
> Many SMBus-only I2C controllers in the wild support more than just
> byte access, and SFP cages are often wired to such controllers
> rather than to a full-featured I2C controller -- e.g. the SMBus
> controllers in the Realtek longan and mango SoCs, which advertise
> word access and I2C block reads. Today, they cannot drive an SFP at
> all without falling back to the byte-only path.
> 
> Extend sfp_smbus_read()/sfp_smbus_write() so that, in addition to
> the existing byte access, they also use SMBus word access and SMBus
> I2C block access whenever the adapter advertises them. Both
> directions are handled in a single read and a single write helper
> that pick the largest supported transfer per chunk and fall back as
> needed.
> 
> I2C-block is preferred unconditionally when available: the protocol
> carries any length 1..32, so it can serve every chunk -- including
> the 1- and 2-byte tails -- without help from word or byte access.
> Note that this requires I2C_FUNC_SMBUS_I2C_BLOCK, which reads a
> caller-specified number of bytes. This deviates from the official
> SMBus Block Read (length is supplied by the slave) but is widely
> supported by Linux I2C controllers/drivers.
> 
> Capability matrix this implementation supports:
> 
>    - BYTE only:                  works (unchanged behaviour); 1-byte
>                                  xfers, hwmon disabled.
>    - BYTE + WORD:                word for >=2-byte chunks, byte for
>                                  trailing odd byte.
>    - I2C_BLOCK present (with or
>      without BYTE/WORD):         block as the universal transport for
>                                  every chunk.
>    - WORD only (no BYTE/BLOCK):  accepted with WARN_ONCE. Even-length
>                                  transfers work; odd-length transfers
>                                  (e.g. the 3-byte cotsworks fixup
>                                  write) hit the BYTE branch which the
>                                  adapter does not implement, so the
>                                  xfer returns an error and the
>                                  operation is aborted. No mainline
>                                  I2C driver was found to advertise
>                                  WORD without BYTE; the warning lets
>                                  us learn about it if it ever shows
>                                  up.
> 
> Adapters with asymmetric R/W capabilities (e.g. only READ_I2C_BLOCK
> but not WRITE_I2C_BLOCK) remain functionally correct -- the
> per-iteration fallback uses the direction-specific bits -- but the
> shared i2c_max_block_size is sized by the all-bits-set check, so a
> transfer in the better-supported direction is not upgraded. None of
> the mainline I2C bus drivers surveyed during review advertise such
> asymmetry; promoting i2c_max_block_size to per-direction sizes can
> be revisited if needed.
> 
> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks !

Maxime

> ---
>   drivers/net/phy/sfp.c | 136 +++++++++++++++++++++++++++++++++---------
>   1 file changed, 108 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index e58e29a1e8d2..85eb13cf5df2 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -14,6 +14,7 @@
>   #include <linux/platform_device.h>
>   #include <linux/rtnetlink.h>
>   #include <linux/slab.h>
> +#include <linux/unaligned.h>
>   #include <linux/workqueue.h>
>   
>   #include "sfp.h"
> @@ -756,50 +757,110 @@ static int sfp_i2c_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
>   	return ret == ARRAY_SIZE(msgs) ? len : 0;
>   }
>   
> -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);
> +
> +		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;
> +		} else if (this_len >= 2 &&
> +			   (functionality & I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> +			ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> +					     I2C_SMBUS_READ, dev_addr,
> +					     I2C_SMBUS_WORD_DATA, &smbus_data);
> +			if (ret < 0)
> +				return ret;
> +
> +			put_unaligned_le16(smbus_data.word, data);
> +			transferred = 2;
> +		} else {
> +			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;
> +
> +			*data = smbus_data.byte;
> +			transferred = 1;
> +		}
>   
> -		len--;
> -		data++;
> -		dev_addr++;
> +		data += transferred;
> +		len -= transferred;
> +		dev_addr += transferred;
>   	}
>   
>   	return data - (u8 *)buf;
>   }
>   
> -static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
> -				void *buf, size_t len)
> +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)
> -			return ret;
> +		this_len = min(len, sfp->i2c_block_size);
> +
> +		if (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_I2C_BLOCK_DATA, &smbus_data);
> +			if (ret < 0)
> +				return ret;
> +
> +			transferred = this_len;
> +		} else if (this_len >= 2 &&
> +			   (functionality & I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> +			smbus_data.word = get_unaligned_le16(data);
> +			ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> +					     I2C_SMBUS_WRITE, dev_addr,
> +					     I2C_SMBUS_WORD_DATA, &smbus_data);
> +			if (ret < 0)
> +				return ret;
> +
> +			transferred = 2;
> +		} else {
> +			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 < 0)
> +				return ret;
> +
> +			transferred = 1;
> +		}
>   
> -		len--;
> -		data++;
> -		dev_addr++;
> +		data += transferred;
> +		len -= transferred;
> +		dev_addr += transferred;
>   	}
>   
>   	return data - (u8 *)buf;
> @@ -815,10 +876,29 @@ static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
>   		sfp->read = sfp_i2c_read;
>   		sfp->write = sfp_i2c_write;
>   		max_block_size = SFP_EEPROM_BLOCK_SIZE;
> -	} else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA)) {
> -		sfp->read = sfp_smbus_byte_read;
> -		sfp->write = sfp_smbus_byte_write;
> -		max_block_size = 1;
> +	} else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA) ||
> +		   i2c_check_functionality(i2c, I2C_FUNC_SMBUS_I2C_BLOCK)) {
> +		/* Either protocol alone covers any length: I2C-block carries
> +		 * 1..32 bytes per xfer, byte iterates one byte at a time.
> +		 */
> +		sfp->read = sfp_smbus_read;
> +		sfp->write = sfp_smbus_write;
> +
> +		if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_I2C_BLOCK))
> +			max_block_size = SFP_EEPROM_BLOCK_SIZE;
> +		else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_WORD_DATA))
> +			max_block_size = 2;
> +		else
> +			max_block_size = 1;
> +	} else if (WARN_ONCE(i2c_check_functionality(i2c, I2C_FUNC_SMBUS_WORD_DATA),
> +			     "SMBus word-only adapter; odd-length transfers will fail\n")) {
> +		/* Word-only: even-length xfers work; odd-length xfers fall
> +		 * to BYTE, which the adapter does not advertise and will
> +		 * likely fail.
> +		 */
> +		sfp->read = sfp_smbus_read;
> +		sfp->write = sfp_smbus_write;
> +		max_block_size = 2;
>   	} else {
>   		sfp->i2c = NULL;
>   		return -EINVAL;