drivers/net/phy/sfp.c | 124 +++++++++++++++++++++++++++++++++--------- 1 file changed, 98 insertions(+), 26 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 more than just byte access. And it also seems that in
several devices, SFP slots are attached to these SMBus controllers
instead of full-featured I2C controllers. Right now, they don't work
with SFP modules. This applies - amongst others - to I2C/SMBus-only
controllers in Realtek longan and mango SoCs. They also support word
access and I2C block reads.
Extend the current read/write SMBus operations to support SMBus I2C
block and SMBus word access. To avoid having dedicated operations for
each kind of transfer, provide generic read and write operations that
covers all kinds of access depending on whats supported.
For block access, this requires 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.
Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
changes since v4:
- made a more general approach, also covering word access
v4: https://lore.kernel.org/netdev/20260109101321.2804-1-jelonek.jonas@gmail.com/
changes since v3:
- fix formal issues
v3: https://lore.kernel.org/netdev/20260105161242.578487-1-jelonek.jonas@gmail.com/
changes since v2:
- fix previous attempt of v2 to fix return value
v2: https://lore.kernel.org/netdev/20260105154653.575397-1-jelonek.jonas@gmail.com/
changes since v1:
- return number of written bytes instead of zero
v1: https://lore.kernel.org/netdev/20251228213331.472887-1-jelonek.jonas@gmail.com/
---
drivers/net/phy/sfp.c | 124 +++++++++++++++++++++++++++++++++---------
1 file changed, 98 insertions(+), 26 deletions(-)
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
@@ -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"
@@ -719,50 +720,103 @@ 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;
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) {
- ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
- I2C_SMBUS_READ, dev_addr,
- I2C_SMBUS_BYTE_DATA, &smbus_data);
+ this_len = min(len, sfp->i2c_max_block_size);
+
+ 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);
+
+ 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);
+
+ 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);
+
+ *data = smbus_data.byte;
+ transferred = 1;
+ }
+
if (ret < 0)
return ret;
- *data = smbus_data.byte;
-
- 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)
+ 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);
+ 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);
+ 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);
+ transferred = 1;
+ }
+
+ if (ret < 0)
return ret;
- len--;
- data++;
- dev_addr++;
+ data += transferred;
+ len -= transferred;
+ dev_addr += transferred;
}
return data - (u8 *)buf;
@@ -770,21 +824,39 @@ static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr,
static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
{
+ size_t max_block_size;
+ u32 functionality;
+
sfp->i2c = i2c;
+ functionality = i2c_get_functionality(i2c);
- if (i2c_check_functionality(i2c, I2C_FUNC_I2C)) {
+ if (functionality & I2C_FUNC_I2C) {
sfp->read = sfp_i2c_read;
sfp->write = sfp_i2c_write;
- sfp->i2c_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;
- sfp->i2c_max_block_size = 1;
+ max_block_size = SFP_EEPROM_BLOCK_SIZE;
+ } else if (functionality & I2C_FUNC_SMBUS_BYTE_DATA) {
+ sfp->read = sfp_smbus_read;
+ sfp->write = sfp_smbus_write;
+
+ if (functionality & I2C_FUNC_SMBUS_I2C_BLOCK)
+ max_block_size = SFP_EEPROM_BLOCK_SIZE;
+ else if (functionality & I2C_FUNC_SMBUS_WORD_DATA)
+ max_block_size = 2;
+ else
+ max_block_size = 1;
+
+ 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 {
sfp->i2c = NULL;
return -EINVAL;
}
+ sfp->i2c_max_block_size = max_block_size;
return 0;
}
base-commit: 74ecff77dace0f9aead6aac852b57af5d4ad3b85
--
2.48.1This 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
On Fri, Jan 16, 2026 at 11:31:05AM +0000, Jonas Jelonek wrote:
> + max_block_size = SFP_EEPROM_BLOCK_SIZE;
> + } else if (functionality & I2C_FUNC_SMBUS_BYTE_DATA) {
If we want to be fully flexible, then:
} else if (functionality & (I2C_FUNC_SMBUS_BYTE_DATA |
I2C_FUNC_SMBUS_I2C_BLOCK)) {
since if we only have SMBus I2C block, then everything is fine.
However, I suggest asking I2C people whether it's possible to have a
SMBUS that supports I2C block and/or word access but not byte access.
Is there a heirarchy to the SMBus capabilities.
Also, is it possible for SMBus to support different read and write
capabilities (since there are separate bits for read and write of
each size.)
So, maybe it needs to be:
} else if ((functionality & I2C_FUNC_SMBUS_BYTE_DATA) == I2C_FUNC_SMBUS_BYTE_DATA ||
(functionality & I2C_FUNC_SMBUS_I2C_BLOCK) == I2C_FUNC_SMBUS_I2C_BLOCK) {
to check that both read and write capabilities for each are set.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Jonas,
On 16/01/2026 12:31, 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 more than just byte access. And it also seems that in
> several devices, SFP slots are attached to these SMBus controllers
> instead of full-featured I2C controllers. Right now, they don't work
> with SFP modules. This applies - amongst others - to I2C/SMBus-only
> controllers in Realtek longan and mango SoCs. They also support word
> access and I2C block reads.
>
> Extend the current read/write SMBus operations to support SMBus I2C
> block and SMBus word access. To avoid having dedicated operations for
> each kind of transfer, provide generic read and write operations that
> covers all kinds of access depending on whats supported.
>
> For block access, this requires 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.
First of all, thanks for this new version :)
[...]
> +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);
> + 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);
> + 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);
> + transferred = 1;
> + }
> +
> + if (ret < 0)
> return ret;
>
> - len--;
> - data++;
> - dev_addr++;
> + data += transferred;
> + len -= transferred;
> + dev_addr += transferred;
> }
I think Russell pointed it out, but I was also wondering the same.
How do we deal with controllers that cannot do neither block nor
single-byte, i.e. that can only do word access ?
We can't do transfers that have an odd length. And there are some,
see sfp_cotsworks_fixup_check() for example.
Maybe these smbus controller don't even exist, but I think we should
anyway have some log saying that this doesn't work, either at SFP
access time, or at init time.
Maxime
Hi, On 16.01.26 14:23, Maxime Chevallier wrote: > I think Russell pointed it out, but I was also wondering the same. > How do we deal with controllers that cannot do neither block nor > single-byte, i.e. that can only do word access ? > > We can't do transfers that have an odd length. And there are some, > see sfp_cotsworks_fixup_check() for example. > > Maybe these smbus controller don't even exist, but I think we should > anyway have some log saying that this doesn't work, either at SFP > access time, or at init time. I tried to guard that in the sfp_i2c_configure() right now. The whole path to allow SMBus transfers is only allowed if there's at least byte access. For exactly the reason that we need byte access in case of odd lengths. Then, it can upgrade to word or block access if available. Or did I miss anything in the conditions? This of course rules out any controllers which just can do word access. I guess covering this case increases the complexity. But I'd be fine having a log or something similar handling this condition. Kind regards, Jonas
On Fri, Jan 16, 2026 at 02:43:47PM +0100, Jonas Jelonek wrote: > Hi, > > On 16.01.26 14:23, Maxime Chevallier wrote: > > I think Russell pointed it out, but I was also wondering the same. > > How do we deal with controllers that cannot do neither block nor > > single-byte, i.e. that can only do word access ? > > > > We can't do transfers that have an odd length. And there are some, > > see sfp_cotsworks_fixup_check() for example. > > > > Maybe these smbus controller don't even exist, but I think we should > > anyway have some log saying that this doesn't work, either at SFP > > access time, or at init time. > > I tried to guard that in the sfp_i2c_configure() right now. The whole path > to allow SMBus transfers is only allowed if there's at least byte access. For > exactly the reason that we need byte access in case of odd lengths. Is there a use case for odd lengths? Apart from 1. > This of course rules out any controllers which just can do word access. There are some PHYs embedded within SFPs which kill the bus if you do anything but 1 byte access. There is a quirk for it. We should refuse to drive the SFP if we have such an SFP and an I2C bus that can only do words. Andrew
On 16/01/2026 15:07, Andrew Lunn wrote: > On Fri, Jan 16, 2026 at 02:43:47PM +0100, Jonas Jelonek wrote: >> Hi, >> >> On 16.01.26 14:23, Maxime Chevallier wrote: >>> I think Russell pointed it out, but I was also wondering the same. >>> How do we deal with controllers that cannot do neither block nor >>> single-byte, i.e. that can only do word access ? >>> >>> We can't do transfers that have an odd length. And there are some, >>> see sfp_cotsworks_fixup_check() for example. >>> >>> Maybe these smbus controller don't even exist, but I think we should >>> anyway have some log saying that this doesn't work, either at SFP >>> access time, or at init time. >> >> I tried to guard that in the sfp_i2c_configure() right now. The whole path >> to allow SMBus transfers is only allowed if there's at least byte access. For >> exactly the reason that we need byte access in case of odd lengths. > > Is there a use case for odd lengths? Apart from 1. There's sfp_cotsworks_fixup_check() that does a 3-byte access : id->base.phys_id = SFF8024_ID_SFF_8472; id->base.phys_ext_id = SFP_PHYS_EXT_ID_SFP; id->base.connector = SFF8024_CONNECTOR_LC; sfp_write(sfp, false, SFP_PHYS_ID, &id->base, 3); It may be possible to turn that into 2 2-byte accesses if we write id->base.phys_id = SFF8024_ID_SFF_8472; id->base.phys_ext_id = SFP_PHYS_EXT_ID_SFP; and then id->base.phys_ext_id = SFP_PHYS_EXT_ID_SFP; id->base.connector = SFF8024_CONNECTOR_LC; But let's first figure-out if word-only smbus are really a thing > >> This of course rules out any controllers which just can do word access. > > There are some PHYs embedded within SFPs which kill the bus if you do > anything but 1 byte access. There is a quirk for it. We should refuse > to drive the SFP if we have such an SFP and an I2C bus that can only > do words. Heh true, same for the weird SGMII <-> 100FX from Prolabs Maxime
> > Is there a use case for odd lengths? Apart from 1.
>
> There's sfp_cotsworks_fixup_check() that does a 3-byte access :
>
> id->base.phys_id = SFF8024_ID_SFF_8472;
> id->base.phys_ext_id = SFP_PHYS_EXT_ID_SFP;
> id->base.connector = SFF8024_CONNECTOR_LC;
> sfp_write(sfp, false, SFP_PHYS_ID, &id->base, 3);
Ah, fixing those broken cotsworks PHYs.
> It may be possible to turn that into 2 2-byte accesses if we write
>
> id->base.phys_id = SFF8024_ID_SFF_8472;
> id->base.phys_ext_id = SFP_PHYS_EXT_ID_SFP;
>
> and then
>
> id->base.phys_ext_id = SFP_PHYS_EXT_ID_SFP;
> id->base.connector = SFF8024_CONNECTOR_LC;
Or just don't bother fixing the EEPROM, leave it broken, but use the
corrected values internally.
> But let's first figure-out if word-only smbus are really a thing
Some grep foo on /drivers/i2c/busses might answer that.
Andrew
Hi, On 16.01.26 15:25, Andrew Lunn wrote: >> But let's first figure-out if word-only smbus are really a thing > Some grep foo on /drivers/i2c/busses might answer that. Did that and haven't found any driver in mainline which is word-only. All drivers with word access capability have byte access too. FWIW, I briefly looked at some specifications. SMBus doesn't seem to require any hierarchy or bare minimum operations, so it's up to the vendor which operations are implemented. Though one could argue, byte access is probably simpler to implement and if a vendor implements word access, byte access is usually implemented too. Looking at the SFP MSA [1], some sentences sound like one could assume byte access is needed at least for SFP. In Section B4, there are statements like: - "The memories are organized as a series of 8-bit data words that can be addressed individually..." - "...provides sequential or random access to 8 bit parameters..." - "The protocol ... sequentially transmits one or more 8-bit bytes..." But that may be too vague and I can't judge if that's a valid argument to not care about word-only here. Kind regards, Jonas [1] https://members.snia.org/document/dl/26184
On Sun, Jan 18, 2026 at 10:43:12AM +0100, Jonas Jelonek wrote: > Hi, > > On 16.01.26 15:25, Andrew Lunn wrote: > >> But let's first figure-out if word-only smbus are really a thing > > Some grep foo on /drivers/i2c/busses might answer that. > > Did that and haven't found any driver in mainline which is word-only. > All drivers with word access capability have byte access too. So for the moment, maybe add a WARN_ON() for an I2C bus that only supports word access, and we can deal with it only if we ever get a report of it firing. Andrew
Hi Andrew, On 18.01.26 16:39, Andrew Lunn wrote: > On Sun, Jan 18, 2026 at 10:43:12AM +0100, Jonas Jelonek wrote: >> Hi, >> >> On 16.01.26 15:25, Andrew Lunn wrote: >>>> But let's first figure-out if word-only smbus are really a thing >>> Some grep foo on /drivers/i2c/busses might answer that. >> Did that and haven't found any driver in mainline which is word-only. >> All drivers with word access capability have byte access too. > So for the moment, maybe add a WARN_ON() for an I2C bus that only > supports word access, and we can deal with it only if we ever get a > report of it firing. Should it just have a WARN_ON and continue (so it may work in some cases) or fail at that point? Kind regards, Jonas
On Thu, Jan 22, 2026 at 11:14:03AM +0100, Jonas Jelonek wrote: > Hi Andrew, > > On 18.01.26 16:39, Andrew Lunn wrote: > > On Sun, Jan 18, 2026 at 10:43:12AM +0100, Jonas Jelonek wrote: > >> Hi, > >> > >> On 16.01.26 15:25, Andrew Lunn wrote: > >>>> But let's first figure-out if word-only smbus are really a thing > >>> Some grep foo on /drivers/i2c/busses might answer that. > >> Did that and haven't found any driver in mainline which is word-only. > >> All drivers with word access capability have byte access too. > > So for the moment, maybe add a WARN_ON() for an I2C bus that only > > supports word access, and we can deal with it only if we ever get a > > report of it firing. > > Should it just have a WARN_ON and continue (so it may work in some cases) > or fail at that point? I guess doing a word access when a byte access is wanted will either work, or immediately kill the I2C bus because the SFP is broken and only supports byte access, and the dead I2C bus will cause a cascade of errors. So continue seems reasonable and hope for the best. Andrew
On Thu, Jan 22, 2026 at 05:04:29PM +0100, Andrew Lunn wrote: > On Thu, Jan 22, 2026 at 11:14:03AM +0100, Jonas Jelonek wrote: > > Hi Andrew, > > > > On 18.01.26 16:39, Andrew Lunn wrote: > > > On Sun, Jan 18, 2026 at 10:43:12AM +0100, Jonas Jelonek wrote: > > >> Hi, > > >> > > >> On 16.01.26 15:25, Andrew Lunn wrote: > > >>>> But let's first figure-out if word-only smbus are really a thing > > >>> Some grep foo on /drivers/i2c/busses might answer that. > > >> Did that and haven't found any driver in mainline which is word-only. > > >> All drivers with word access capability have byte access too. > > > So for the moment, maybe add a WARN_ON() for an I2C bus that only > > > supports word access, and we can deal with it only if we ever get a > > > report of it firing. > > > > Should it just have a WARN_ON and continue (so it may work in some cases) > > or fail at that point? > > I guess doing a word access when a byte access is wanted will either > work, or immediately kill the I2C bus because the SFP is broken and > only supports byte access, and the dead I2C bus will cause a cascade > of errors. RTL8672 / RTL9601C based xPON SFPs are an example, they need single byte access. Conversely, we have the Nokia 3FE46541AA GPON module which locks up the I2C bus if one reads the EEPROM (i2c 0x50) offset 0x51 using a single byte read. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Jan 18, 2026 at 10:43:12AM +0100, Jonas Jelonek wrote: > Looking at the SFP MSA [1], some sentences sound like one could assume > byte access is needed at least for SFP. In Section B4, there are statements > like: > - "The memories are organized as a series of 8-bit data words that can be > addressed individually..." > - "...provides sequential or random access to 8 bit parameters..." > - "The protocol ... sequentially transmits one or more 8-bit bytes..." > > But that may be too vague and I can't judge if that's a valid argument to not > care about word-only here. There's a whole bunch of documents. You also need to look at SFF-8472. This contains the following paragraph: To guarantee coherency of the diagnostic monitoring data, the host is required to retrieve any multi-byte fields from the diagnostic monitoring data structure (e.g. Rx Power MSB - byte 104 in A2h, Rx Power LSB - byte 105 in A2h) by the use of a single two-byte read sequence across the 2-wire interface. Hence why we don't allow hwmon when only byte accesses are available. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Jan 18, 2026 at 10:08:12AM +0000, Russell King (Oracle) wrote:
> On Sun, Jan 18, 2026 at 10:43:12AM +0100, Jonas Jelonek wrote:
> > Looking at the SFP MSA [1], some sentences sound like one could assume
> > byte access is needed at least for SFP. In Section B4, there are statements
> > like:
> > - "The memories are organized as a series of 8-bit data words that can be
> > addressed individually..."
> > - "...provides sequential or random access to 8 bit parameters..."
> > - "The protocol ... sequentially transmits one or more 8-bit bytes..."
> >
> > But that may be too vague and I can't judge if that's a valid argument to not
> > care about word-only here.
>
> There's a whole bunch of documents. You also need to look at SFF-8472.
> This contains the following paragraph:
>
> To guarantee coherency of the diagnostic monitoring data, the host is
> required to retrieve any multi-byte fields from the diagnostic
> monitoring data structure (e.g. Rx Power MSB - byte 104 in A2h, Rx
> Power LSB - byte 105 in A2h) by the use of a single two-byte read
> sequence across the 2-wire interface.
>
> Hence why we don't allow hwmon when only byte accesses are available.
I would also add, SFP vendors like to ignore the MSA and do random
things. Look at the number of quirks we have for dealing with SFPs
which break what these documents say.
Andrew
On 16/01/2026 14:43, Jonas Jelonek wrote: > Hi, > > On 16.01.26 14:23, Maxime Chevallier wrote: >> I think Russell pointed it out, but I was also wondering the same. >> How do we deal with controllers that cannot do neither block nor >> single-byte, i.e. that can only do word access ? >> >> We can't do transfers that have an odd length. And there are some, >> see sfp_cotsworks_fixup_check() for example. >> >> Maybe these smbus controller don't even exist, but I think we should >> anyway have some log saying that this doesn't work, either at SFP >> access time, or at init time. > > I tried to guard that in the sfp_i2c_configure() right now. The whole path > to allow SMBus transfers is only allowed if there's at least byte access. For > exactly the reason that we need byte access in case of odd lengths. Then, > it can upgrade to word or block access if available. Or did I miss anything in > the conditions? Ah true, true. Should you need to respin, maybe add a comment for clueless people like myself :) I'll see if I can give this a test today then Thanks Maxime
© 2016 - 2026 Red Hat, Inc.