drivers/net/phy/sfp.c | 77 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 2 deletions(-)
Commit 7662abf4db94 ("net: phy: sfp: Add support for SMBus module access")
added support for SMBus-only controllers for module access. However,
this is restricted to single-byte accesses and has the implication that
hwmon is disabled (due to missing atomicity of 16-bit accesses) and
warnings are printed.
There are probably a lot of SMBus-only I2C controllers out in the wild
which support block reads. Right now, they don't work with SFP modules.
This applies - amongst others - to I2C/SMBus-only controllers in Realtek
longan and mango SoCs.
Downstream in OpenWrt, a patch similar to the abovementioned patch is
used for current LTS kernel 6.12. However, this uses byte-access for all
kinds of access and thus disregards the atomicity for wider access.
Introduce read/write SMBus I2C block operations to support SMBus-only
controllers with appropriate support for block read/write. Those
operations are used for all accesses if supported, otherwise the
single-byte operations will be used. With block reads, atomicity for
16-bit reads as required by hwmon is preserved and thus, hwmon can be
used.
The implementation requires the I2C_FUNC_SMBUS_I2C_BLOCK to be
supported as it relies on reading a pre-defined amount of bytes.
This isn't intended by the official SMBus Block Read but supported by
several I2C controllers/drivers.
Support for word access is not implemented due to issues regarding
endianness.
Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
v2: return number of written bytes in sfp_smbus_block_write
v1: https://lore.kernel.org/netdev/20251228213331.472887-1-jelonek.jonas@gmail.com/
---
drivers/net/phy/sfp.c | 77 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 75 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 6166e9196364..4f2175397534 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -744,6 +744,35 @@ static int sfp_smbus_byte_read(struct sfp *sfp, bool a2, u8 dev_addr,
return data - (u8 *)buf;
}
+static int sfp_smbus_block_read(struct sfp *sfp, bool a2, u8 dev_addr,
+ void *buf, size_t len)
+{
+ size_t block_size = sfp->i2c_block_size;
+ union i2c_smbus_data smbus_data;
+ u8 bus_addr = a2 ? 0x51 : 0x50;
+ u8 *data = buf;
+ u8 this_len;
+ int ret;
+
+ while (len) {
+ this_len = min(len, block_size);
+
+ 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);
+ len -= this_len;
+ data += this_len;
+ dev_addr += this_len;
+ }
+
+ return data - (u8 *)buf;
+}
+
static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
void *buf, size_t len)
{
@@ -765,26 +794,70 @@ static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
dev_addr++;
}
+ return data - (u8 *)buf;
+}
+
+static int sfp_smbus_block_write(struct sfp *sfp, bool a2, u8 dev_addr,
+ void *buf, size_t len)
+{
+ size_t block_size = sfp->i2c_block_size;
+ union i2c_smbus_data smbus_data;
+ u8 bus_addr = a2 ? 0x51 : 0x50;
+ u8 *data = buf;
+ u8 this_len;
+ int ret;
+
+ while (len) {
+ this_len = min(len, block_size);
+
+ 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)
+ return ret;
+
+ len -= this_len;
+ data += this_len;
+ dev_addr += this_len;
+ }
+
return 0;
}
static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
{
+ size_t max_block_size;
+
sfp->i2c = i2c;
if (i2c_check_functionality(i2c, I2C_FUNC_I2C)) {
sfp->read = sfp_i2c_read;
sfp->write = sfp_i2c_write;
- sfp->i2c_max_block_size = SFP_EEPROM_BLOCK_SIZE;
+ max_block_size = SFP_EEPROM_BLOCK_SIZE;
+ } else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_I2C_BLOCK)) {
+ sfp->read = sfp_smbus_block_read;
+ sfp->write = sfp_smbus_block_write;
+
+ max_block_size = SFP_EEPROM_BLOCK_SIZE;
+ 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);
+
} else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA)) {
sfp->read = sfp_smbus_byte_read;
sfp->write = sfp_smbus_byte_write;
- sfp->i2c_max_block_size = 1;
+ max_block_size = 1;
} else {
sfp->i2c = NULL;
return -EINVAL;
}
+ sfp->i2c_max_block_size = max_block_size;
return 0;
}
base-commit: c303e8b86d9dbd6868f5216272973292f7f3b7f1
--
2.48.1On Mon, Jan 05, 2026 at 03:46:53PM +0000, Jonas Jelonek wrote:
> @@ -765,26 +794,70 @@ static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
> dev_addr++;
> }
>
> + return data - (u8 *)buf;
A separate fix is being submitted for this.
> +}
> +
> +static int sfp_smbus_block_write(struct sfp *sfp, bool a2, u8 dev_addr,
> + void *buf, size_t len)
> +{
> + size_t block_size = sfp->i2c_block_size;
> + union i2c_smbus_data smbus_data;
> + u8 bus_addr = a2 ? 0x51 : 0x50;
> + u8 *data = buf;
> + u8 this_len;
> + int ret;
> +
> + while (len) {
> + this_len = min(len, block_size);
> +
> + 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)
> + return ret;
> +
> + len -= this_len;
> + data += this_len;
> + dev_addr += this_len;
> + }
> +
> return 0;
This is wrong. As already said, the I2C accessors return the number of
bytes successfully transferred. Zero means no bytes were transferred,
which is an error.
All callers to sfp_write() validate that the expected number of bytes
were written. Thus, returning zero will cause failures.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Russell,
On 05.01.26 17:16, Russell King (Oracle) wrote:
> On Mon, Jan 05, 2026 at 03:46:53PM +0000, Jonas Jelonek wrote:
>> @@ -765,26 +794,70 @@ static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
>> dev_addr++;
>> }
>>
>> + return data - (u8 *)buf;
> A separate fix is being submitted for this.
>
>> +}
>> +
>> +static int sfp_smbus_block_write(struct sfp *sfp, bool a2, u8 dev_addr,
>> + void *buf, size_t len)
>> +{
>> + size_t block_size = sfp->i2c_block_size;
>> + union i2c_smbus_data smbus_data;
>> + u8 bus_addr = a2 ? 0x51 : 0x50;
>> + u8 *data = buf;
>> + u8 this_len;
>> + int ret;
>> +
>> + while (len) {
>> + this_len = min(len, block_size);
>> +
>> + 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)
>> + return ret;
>> +
>> + len -= this_len;
>> + data += this_len;
>> + dev_addr += this_len;
>> + }
>> +
>> return 0;
> This is wrong. As already said, the I2C accessors return the number of
> bytes successfully transferred. Zero means no bytes were transferred,
> which is an error.
>
> All callers to sfp_write() validate that the expected number of bytes
> were written. Thus, returning zero will cause failures.
>
yes. I totally messed up v2 but already pushed out a v3 that fixes that, and
has the separate fix as a prerequisite.
Sorry for the confusion.
Best,
Jonas
Sorry for the noise, messed that version up.
Will send a v3 quickly.
Best regards,
Jonas Jelonek
On 05.01.26 16:46, Jonas Jelonek wrote:
> Commit 7662abf4db94 ("net: phy: sfp: Add support for SMBus module access")
> added support for SMBus-only controllers for module access. However,
> this is restricted to single-byte accesses and has the implication that
> hwmon is disabled (due to missing atomicity of 16-bit accesses) and
> warnings are printed.
>
> There are probably a lot of SMBus-only I2C controllers out in the wild
> which support block reads. Right now, they don't work with SFP modules.
> This applies - amongst others - to I2C/SMBus-only controllers in Realtek
> longan and mango SoCs.
>
> Downstream in OpenWrt, a patch similar to the abovementioned patch is
> used for current LTS kernel 6.12. However, this uses byte-access for all
> kinds of access and thus disregards the atomicity for wider access.
>
> Introduce read/write SMBus I2C block operations to support SMBus-only
> controllers with appropriate support for block read/write. Those
> operations are used for all accesses if supported, otherwise the
> single-byte operations will be used. With block reads, atomicity for
> 16-bit reads as required by hwmon is preserved and thus, hwmon can be
> used.
>
> The implementation requires the I2C_FUNC_SMBUS_I2C_BLOCK to be
> supported as it relies on reading a pre-defined amount of bytes.
> This isn't intended by the official SMBus Block Read but supported by
> several I2C controllers/drivers.
>
> Support for word access is not implemented due to issues regarding
> endianness.
>
> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
>
> ---
> v2: return number of written bytes in sfp_smbus_block_write
> v1: https://lore.kernel.org/netdev/20251228213331.472887-1-jelonek.jonas@gmail.com/
> ---
> drivers/net/phy/sfp.c | 77 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 75 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 6166e9196364..4f2175397534 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -744,6 +744,35 @@ static int sfp_smbus_byte_read(struct sfp *sfp, bool a2, u8 dev_addr,
> return data - (u8 *)buf;
> }
>
> +static int sfp_smbus_block_read(struct sfp *sfp, bool a2, u8 dev_addr,
> + void *buf, size_t len)
> +{
> + size_t block_size = sfp->i2c_block_size;
> + union i2c_smbus_data smbus_data;
> + u8 bus_addr = a2 ? 0x51 : 0x50;
> + u8 *data = buf;
> + u8 this_len;
> + int ret;
> +
> + while (len) {
> + this_len = min(len, block_size);
> +
> + 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);
> + len -= this_len;
> + data += this_len;
> + dev_addr += this_len;
> + }
> +
> + return data - (u8 *)buf;
> +}
> +
> static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
> void *buf, size_t len)
> {
> @@ -765,26 +794,70 @@ static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
> dev_addr++;
> }
>
> + return data - (u8 *)buf;
> +}
> +
> +static int sfp_smbus_block_write(struct sfp *sfp, bool a2, u8 dev_addr,
> + void *buf, size_t len)
> +{
> + size_t block_size = sfp->i2c_block_size;
> + union i2c_smbus_data smbus_data;
> + u8 bus_addr = a2 ? 0x51 : 0x50;
> + u8 *data = buf;
> + u8 this_len;
> + int ret;
> +
> + while (len) {
> + this_len = min(len, block_size);
> +
> + 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)
> + return ret;
> +
> + len -= this_len;
> + data += this_len;
> + dev_addr += this_len;
> + }
> +
> return 0;
> }
>
> static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
> {
> + size_t max_block_size;
> +
> sfp->i2c = i2c;
>
> if (i2c_check_functionality(i2c, I2C_FUNC_I2C)) {
> sfp->read = sfp_i2c_read;
> sfp->write = sfp_i2c_write;
> - sfp->i2c_max_block_size = SFP_EEPROM_BLOCK_SIZE;
> + max_block_size = SFP_EEPROM_BLOCK_SIZE;
> + } else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_I2C_BLOCK)) {
> + sfp->read = sfp_smbus_block_read;
> + sfp->write = sfp_smbus_block_write;
> +
> + max_block_size = SFP_EEPROM_BLOCK_SIZE;
> + 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);
> +
> } else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA)) {
> sfp->read = sfp_smbus_byte_read;
> sfp->write = sfp_smbus_byte_write;
> - sfp->i2c_max_block_size = 1;
> + max_block_size = 1;
> } else {
> sfp->i2c = NULL;
> return -EINVAL;
> }
>
> + sfp->i2c_max_block_size = max_block_size;
> return 0;
> }
>
>
> base-commit: c303e8b86d9dbd6868f5216272973292f7f3b7f1
© 2016 - 2026 Red Hat, Inc.