[PATCH net-next v2 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[PATCH net-next v2 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Maxime Chevallier]

Hello everyone,

This is V2 for the single-byte SMBus support for SFP cages as well as
embedded PHYs accessed over mdio-i2c.

Discussions in V1 [1] with Russell and Andrew showed that we should be mor
conservative with SMBus access, as it is either stated as non-compliant
with SFF-8472 for diag data, or simply blurry as to how well this will work
with Copper modules.

Tests on a variety of modules show that it looks OK however this is
really not enough to guarantee that it will work with all modules, so in
this V2 we :
 - Disable hwmon
 - Print a big warning indicating that it may not work as expected, but
   more importantly that the kernel isn't to blame, but rather the HW
   design.

I've added Sean's tested-by tags, I hope that's OK given I've added the
new flag to disable hwmon.

V1 ([1]): https://lore.kernel.org/netdev/20250223172848.1098621-1-maxime.chevallier@bootlin.com/#t

Maxime Chevallier (2):
  net: phy: sfp: Add support for SMBus module access
  net: mdio: mdio-i2c: Add support for single-byte SMBus operations

 drivers/net/mdio/mdio-i2c.c | 79 ++++++++++++++++++++++++++++++++++++-
 drivers/net/phy/sfp.c       | 79 ++++++++++++++++++++++++++++++++++---
 2 files changed, 151 insertions(+), 7 deletions(-)

-- 
2.48.1

[PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Maxime Chevallier]

The SFP module's eeprom and internals are accessible through an i2c bus.
However, all the i2c transfers that are performed are SMBus-style
transfers for read and write operations.

It is possible that the SFP might be connected to an SMBus-only
controller, such as the one found in some PHY devices in the VSC85xx
family.

Introduce a set of sfp read/write ops that are going to be used if the
i2c bus is only capable of doing smbus byte accesses.

As Single-byte SMBus transaction go against SFF-8472 and breaks the
atomicity for diagnostics data access, hwmon is disabled in the case
of SMBus access.

Moreover, as this may cause other instabilities, print a warning at
probe time to indicate that the setup may be unreliable because of the
hardware design.

Tested-by: Sean Anderson <sean.anderson@linux.dev>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---

V2: - Added Sean's tested-by
    - Added a warning indicating that operations won't be reliable, from
      Russell and Andrew's reviews
    - Also added a flag saying we're under a single-byte-access bus, to
      both print the warning and disable hwmon.

 drivers/net/phy/sfp.c | 79 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 73 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 9369f5297769..6e9d3d95eb95 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -282,6 +282,7 @@ struct sfp {
 	unsigned int rs_state_mask;
 
 	bool have_a2;
+	bool single_byte_access;
 
 	const struct sfp_quirk *quirk;
 
@@ -690,14 +691,70 @@ 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_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
+static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
+			  size_t len)
 {
-	if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
-		return -EINVAL;
+	u8 bus_addr = a2 ? 0x51 : 0x50;
+	union i2c_smbus_data smbus_data;
+	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;
+
+		*data = smbus_data.byte;
+
+		len--;
+		data++;
+		dev_addr++;
+	}
+
+	return data - (u8 *)buf;
+}
+
+static int sfp_smbus_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
+			   size_t len)
+{
+	u8 bus_addr = a2 ? 0x51 : 0x50;
+	union i2c_smbus_data smbus_data;
+	u8 *data = buf;
+	int ret;
 
+	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;
+
+		len--;
+		data++;
+		dev_addr++;
+	}
+
+	return 0;
+}
+
+static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
+{
 	sfp->i2c = i2c;
-	sfp->read = sfp_i2c_read;
-	sfp->write = sfp_i2c_write;
+
+	if (i2c_check_functionality(i2c, I2C_FUNC_I2C)) {
+		sfp->read = sfp_i2c_read;
+		sfp->write = sfp_i2c_write;
+	} else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		sfp->read = sfp_smbus_read;
+		sfp->write = sfp_smbus_write;
+		sfp->single_byte_access = true;
+	} else {
+		sfp->i2c = NULL;
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -1628,7 +1685,8 @@ static void sfp_hwmon_probe(struct work_struct *work)
 
 static int sfp_hwmon_insert(struct sfp *sfp)
 {
-	if (sfp->have_a2 && sfp->id.ext.diagmon & SFP_DIAGMON_DDM) {
+	if (sfp->have_a2 && sfp->id.ext.diagmon & SFP_DIAGMON_DDM &&
+	    !sfp->single_byte_access) {
 		mod_delayed_work(system_wq, &sfp->hwmon_probe, 1);
 		sfp->hwmon_tries = R_PROBE_RETRY_SLOW;
 	}
@@ -3114,6 +3172,15 @@ static int sfp_probe(struct platform_device *pdev)
 	if (!sfp->sfp_bus)
 		return -ENOMEM;
 
+	if (sfp->single_byte_access)
+		dev_warn(sfp->dev,
+			 "Please note:\n"
+			 "This SFP cage is accessed via an SMBus only capable of single byte\n"
+			 "transactions. Some features are disabled, other may be unreliable or\n"
+			 "sporadically fail. Use with caution. There is nothing that the kernel\n"
+			 "or community can do to fix it, the kernel will try best efforts. Please\n"
+			 "verify any problems on hardware that supports multi-byte I2C transactions.\n");
+
 	sfp_debugfs_init(sfp);
 
 	return 0;
-- 
2.48.1

[PATCH net-next v2 2/2] net: mdio: mdio-i2c: Add support for single-byte SMBus operations

[Maxime Chevallier]

PHYs that are within copper SFP modules have their MDIO bus accessible
through address 0x56 (usually) on the i2c bus. The MDIO-I2C bridge is
desgned for 16 bits accesses, but we can also perform 8bits accesses by
reading/writing the high and low bytes sequentially.

This commit adds support for this type of accesses, thus supporting
smbus controllers such as the one in the VSC8552.

This was only tested on Copper SFP modules that embed a Marvell 88e1111
PHY.

Tested-by: Sean Anderson <sean.anderson@linux.dev>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---

V2: No changes

 drivers/net/mdio/mdio-i2c.c | 79 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mdio/mdio-i2c.c b/drivers/net/mdio/mdio-i2c.c
index da2001ea1f99..202f486e71f1 100644
--- a/drivers/net/mdio/mdio-i2c.c
+++ b/drivers/net/mdio/mdio-i2c.c
@@ -106,6 +106,62 @@ static int i2c_mii_write_default_c22(struct mii_bus *bus, int phy_id, int reg,
 	return i2c_mii_write_default_c45(bus, phy_id, -1, reg, val);
 }
 
+static int smbus_byte_mii_read_default_c22(struct mii_bus *bus, int phy_id,
+					   int reg)
+{
+	struct i2c_adapter *i2c = bus->priv;
+	union i2c_smbus_data smbus_data;
+	int val = 0, ret;
+
+	if (!i2c_mii_valid_phy_id(phy_id))
+		return 0;
+
+	ret = i2c_smbus_xfer(i2c, i2c_mii_phy_addr(phy_id), 0,
+			     I2C_SMBUS_READ, reg,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (ret < 0)
+		return ret;
+
+	val = ((smbus_data.byte & 0xff) << 8);
+
+	ret = i2c_smbus_xfer(i2c, i2c_mii_phy_addr(phy_id), 0,
+			     I2C_SMBUS_READ, reg,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (ret < 0)
+		return ret;
+
+	val |= (smbus_data.byte & 0xff);
+
+	return val;
+}
+
+static int smbus_byte_mii_write_default_c22(struct mii_bus *bus, int phy_id,
+					    int reg, u16 val)
+{
+	struct i2c_adapter *i2c = bus->priv;
+	union i2c_smbus_data smbus_data;
+	int ret;
+
+	if (!i2c_mii_valid_phy_id(phy_id))
+		return 0;
+
+	smbus_data.byte = ((val & 0xff00) >> 8);
+
+	ret = i2c_smbus_xfer(i2c, i2c_mii_phy_addr(phy_id), 0,
+			     I2C_SMBUS_WRITE, reg,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (ret < 0)
+		return ret;
+
+	smbus_data.byte = val & 0xff;
+
+	ret = i2c_smbus_xfer(i2c, i2c_mii_phy_addr(phy_id), 0,
+			     I2C_SMBUS_WRITE, reg,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+
+	return ret < 0 ? ret : 0;
+}
+
 /* RollBall SFPs do not access internal PHY via I2C address 0x56, but
  * instead via address 0x51, when SFP page is set to 0x03 and password to
  * 0xffffffff.
@@ -378,13 +434,26 @@ static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
 		return 0;
 }
 
+static bool mdio_i2c_check_functionality(struct i2c_adapter *i2c,
+					 enum mdio_i2c_proto protocol)
+{
+	if (i2c_check_functionality(i2c, I2C_FUNC_I2C))
+		return true;
+
+	if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA) &&
+	    protocol == MDIO_I2C_MARVELL_C22)
+		return true;
+
+	return false;
+}
+
 struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
 			       enum mdio_i2c_proto protocol)
 {
 	struct mii_bus *mii;
 	int ret;
 
-	if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
+	if (!mdio_i2c_check_functionality(i2c, protocol))
 		return ERR_PTR(-EINVAL);
 
 	mii = mdiobus_alloc();
@@ -395,6 +464,14 @@ struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
 	mii->parent = parent;
 	mii->priv = i2c;
 
+	/* Only use SMBus if we have no other choice */
+	if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA) &&
+	    !i2c_check_functionality(i2c, I2C_FUNC_I2C)) {
+		mii->read = smbus_byte_mii_read_default_c22;
+		mii->write = smbus_byte_mii_write_default_c22;
+		return mii;
+	}
+
 	switch (protocol) {
 	case MDIO_I2C_ROLLBALL:
 		ret = i2c_mii_init_rollball(i2c);
-- 
2.48.1

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Andrew Lunn]

On Tue, Feb 25, 2025 at 12:20:39PM +0100, Maxime Chevallier wrote:
> The SFP module's eeprom and internals are accessible through an i2c bus.
> However, all the i2c transfers that are performed are SMBus-style
> transfers for read and write operations.
> 
> It is possible that the SFP might be connected to an SMBus-only
> controller, such as the one found in some PHY devices in the VSC85xx
> family.
> 
> Introduce a set of sfp read/write ops that are going to be used if the
> i2c bus is only capable of doing smbus byte accesses.
> 
> As Single-byte SMBus transaction go against SFF-8472 and breaks the
> atomicity for diagnostics data access, hwmon is disabled in the case
> of SMBus access.
> 
> Moreover, as this may cause other instabilities, print a warning at
> probe time to indicate that the setup may be unreliable because of the
> hardware design.
> 
> Tested-by: Sean Anderson <sean.anderson@linux.dev>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> 
> V2: - Added Sean's tested-by
>     - Added a warning indicating that operations won't be reliable, from
>       Russell and Andrew's reviews
>     - Also added a flag saying we're under a single-byte-access bus, to
>       both print the warning and disable hwmon.
> 
>  drivers/net/phy/sfp.c | 79 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 9369f5297769..6e9d3d95eb95 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -282,6 +282,7 @@ struct sfp {
>  	unsigned int rs_state_mask;
>  
>  	bool have_a2;
> +	bool single_byte_access;
>  
>  	const struct sfp_quirk *quirk;
>  
> @@ -690,14 +691,70 @@ 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_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
> +static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,

Maybe call this sfp_smbus_byte_read(), leaving space for
sfp_smbus_word_read() in the future.

> +			  size_t len)
>  {
> -	if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
> -		return -EINVAL;
> +	u8 bus_addr = a2 ? 0x51 : 0x50;
> +	union i2c_smbus_data smbus_data;
> +	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;

Isn't this the wrong order? You should do the upper byte first, then
the lower?


    Andrew

---
pw-bot: cr

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Maxime Chevallier]

Hi Andrew,

> > -static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
> > +static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,  
> 
> Maybe call this sfp_smbus_byte_read(), leaving space for
> sfp_smbus_word_read() in the future.

Good idea, I'll do that :)

> > +			  size_t len)
> >  {
> > -	if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
> > -		return -EINVAL;
> > +	u8 bus_addr = a2 ? 0x51 : 0x50;
> > +	union i2c_smbus_data smbus_data;
> > +	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;  
> 
> Isn't this the wrong order? You should do the upper byte first, then
> the lower?

You might be correct. As I have been running that code out-of-tree for
a while, I was thinking that surely I'd have noticed if this was
wrong, however there are only a few cases where we actually write to
SFP :

 - sfp_modify_u8(...) => one-byte write
 - in sfp_cotsworks_fixup_check(...) there are 2 writes : one 1-byte
write and a 3-bytes write.

As I don't have any cotsworks SFP, then it looks like having the writes
mis-ordered would have stayed un-noticed on my side as I only
stressed the 1 byte write path...

So, good catch :) Let me triple-check and see if I can find any
conceivable way of testing that...

Thanks,

Maxime

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Andrew Lunn]

> You might be correct. As I have been running that code out-of-tree for
> a while, I was thinking that surely I'd have noticed if this was
> wrong, however there are only a few cases where we actually write to
> SFP :
> 
>  - sfp_modify_u8(...) => one-byte write
>  - in sfp_cotsworks_fixup_check(...) there are 2 writes : one 1-byte
> write and a 3-bytes write.
> 
> As I don't have any cotsworks SFP, then it looks like having the writes
> mis-ordered would have stayed un-noticed on my side as I only
> stressed the 1 byte write path...
> 
> So, good catch :) Let me triple-check and see if I can find any
> conceivable way of testing that...

Read might be more important than write. This is particularly
important for the second page containing the diagnostics, and dumped
by ethtool -m. It could be the sensor values latch when you read the
higher byte, so you can read the lower byte without worrying about it
changing. This is why we don't want HWMON, if you can only do byte
access. You might be able to test this with the temperature
sensor. The value is in 1/256 degrees. So if you can get is going from
21 255/256C to 22 0/256C and see if you ever read 21 0/256 or 22
255/256C.

	Andrew

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Russell King (Oracle)]

On Tue, Feb 25, 2025 at 12:20:39PM +0100, Maxime Chevallier wrote:
> The SFP module's eeprom and internals are accessible through an i2c bus.
> However, all the i2c transfers that are performed are SMBus-style
> transfers for read and write operations.

Note that there are SFPs that fail if you access them by byte - the
3FE46541AA locks the bus if you byte access the emulated EEPROM at
0x50, address 0x51. This is documented in sfp_sm_mod_probe().

So there's a very real reason for adding the warning - this module
will not work!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Russell King (Oracle)]

On Tue, Feb 25, 2025 at 02:56:17PM +0100, Maxime Chevallier wrote:
> > > +	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;  
> > 
> > Isn't this the wrong order? You should do the upper byte first, then
> > the lower?
> 
> You might be correct. As I have been running that code out-of-tree for
> a while, I was thinking that surely I'd have noticed if this was
> wrong, however there are only a few cases where we actually write to
> SFP :
> 
>  - sfp_modify_u8(...) => one-byte write
>  - in sfp_cotsworks_fixup_check(...) there are 2 writes : one 1-byte
> write and a 3-bytes write.
> 
> As I don't have any cotsworks SFP, then it looks like having the writes
> mis-ordered would have stayed un-noticed on my side as I only
> stressed the 1 byte write path...

This Cotsworks module is not a SFP. It's a solder-on SFF module.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Maxime Chevallier]

On Tue, 25 Feb 2025 18:04:29 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Tue, Feb 25, 2025 at 12:20:39PM +0100, Maxime Chevallier wrote:
> > The SFP module's eeprom and internals are accessible through an i2c bus.
> > However, all the i2c transfers that are performed are SMBus-style
> > transfers for read and write operations.  
> 
> Note that there are SFPs that fail if you access them by byte - the
> 3FE46541AA locks the bus if you byte access the emulated EEPROM at
> 0x50, address 0x51. This is documented in sfp_sm_mod_probe().
> 
> So there's a very real reason for adding the warning - this module
> will not work!

That's indeed pretty serious and since we can't even read the
eeprom, there's not even a chance to have, say, a list of modules that
will break with 1-byte io. So indeed the warning is needed...

Thanks

Maxime

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Russell King (Oracle)]

On Tue, Feb 25, 2025 at 03:58:31PM +0100, Andrew Lunn wrote:
> > You might be correct. As I have been running that code out-of-tree for
> > a while, I was thinking that surely I'd have noticed if this was
> > wrong, however there are only a few cases where we actually write to
> > SFP :
> > 
> >  - sfp_modify_u8(...) => one-byte write
> >  - in sfp_cotsworks_fixup_check(...) there are 2 writes : one 1-byte
> > write and a 3-bytes write.
> > 
> > As I don't have any cotsworks SFP, then it looks like having the writes
> > mis-ordered would have stayed un-noticed on my side as I only
> > stressed the 1 byte write path...
> > 
> > So, good catch :) Let me triple-check and see if I can find any
> > conceivable way of testing that...
> 
> Read might be more important than write. This is particularly
> important for the second page containing the diagnostics, and dumped
> by ethtool -m. It could be the sensor values latch when you read the
> higher byte, so you can read the lower byte without worrying about it
> changing. This is why we don't want HWMON, if you can only do byte
> access. You might be able to test this with the temperature
> sensor. The value is in 1/256 degrees. So if you can get is going from
> 21 255/256C to 22 0/256C and see if you ever read 21 0/256 or 22
> 255/256C.

<frustrated>

Why don't we read SFF-8472 instead of testing module specific behaviour?
Section 9.1 (Diagnostics overview) paragraphs 4 and 5 cover this.

No, it's not latched when you read the high byte. Paragraph 4 states
that multi-byte fields must be read using "a single two-byte read
sequence across the 2-wire interface".

Paragraph 5 states that "the transceiver shall not update a multi-byte
field within the structure during the transfer of that multi-byte field
to the host, such that partially updated data would be transferred to
the host."

In other words, while reading the the individual bytes of a multi-byte
field, the value will remain stable _while the bus transaction which
is required to be a multi-byte read is in progress_.

So, when the STOP condition is signalled on the bus, the transceiver
is then free to change the values. So accessing the high byte and
low byte seperately does not guarantee to be coherent.

It *might* work with some modules. It may not work with others. It
might crash or lock the I2C bus with other modules. (I already know
that at least one GPON module locks the bus with byte reads of
0xA0 EEPROM offset 0x51.)

We've had this before. We have a byte-mode fallback in the SFP code,
and we've had to be *very* careful when enabling this only for
modules that need it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Sean Anderson]

On 2/25/25 13:04, Russell King (Oracle) wrote:
> On Tue, Feb 25, 2025 at 12:20:39PM +0100, Maxime Chevallier wrote:
>> The SFP module's eeprom and internals are accessible through an i2c bus.
>> However, all the i2c transfers that are performed are SMBus-style
>> transfers for read and write operations.
> 
> Note that there are SFPs that fail if you access them by byte - the
> 3FE46541AA locks the bus if you byte access the emulated EEPROM at
> 0x50, address 0x51. This is documented in sfp_sm_mod_probe().
> 
> So there's a very real reason for adding the warning - this module
> will not work!
> 

I had a look at sfp_sm_mod_probe, and from what I can tell the SFP that
I was having issues with should have been fixed by commit 426c6cbc409c
("net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips"). I
re-tested without this series applied, and the SFP still worked. So I
guess I don't have an SFP module with the issue this series is trying to
address after all.

--Sean

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Maxime Chevallier]

Hi Sean,

On Tue, 25 Feb 2025 13:41:57 -0500
Sean Anderson <sean.anderson@linux.dev> wrote:

> On 2/25/25 13:04, Russell King (Oracle) wrote:
> > On Tue, Feb 25, 2025 at 12:20:39PM +0100, Maxime Chevallier wrote:  
> >> The SFP module's eeprom and internals are accessible through an i2c bus.
> >> However, all the i2c transfers that are performed are SMBus-style
> >> transfers for read and write operations.  
> > 
> > Note that there are SFPs that fail if you access them by byte - the
> > 3FE46541AA locks the bus if you byte access the emulated EEPROM at
> > 0x50, address 0x51. This is documented in sfp_sm_mod_probe().
> > 
> > So there's a very real reason for adding the warning - this module
> > will not work!
> >   
> 
> I had a look at sfp_sm_mod_probe, and from what I can tell the SFP that
> I was having issues with should have been fixed by commit 426c6cbc409c
> ("net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips"). I
> re-tested without this series applied, and the SFP still worked. So I
> guess I don't have an SFP module with the issue this series is trying to
> address after all.

I see, this series actually wasn't supposed to solve that at all (but
it's true that the solution was to fallback to 1-byte access, as
Russell explains on that thread) :)

The use-case for that series is to deal with situations where the Host
(i2c master) is only capable of 1-byte transactions (it's not a true
i2c controller, but rather a very limited smbus controller) :)

Maxime

Re: [PATCH net-next v2 1/2] net: phy: sfp: Add support for SMBus module access

[Maxime Chevallier]

Hi,

On Tue, 25 Feb 2025 12:20:39 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> The SFP module's eeprom and internals are accessible through an i2c bus.
> However, all the i2c transfers that are performed are SMBus-style
> transfers for read and write operations.
> 
> It is possible that the SFP might be connected to an SMBus-only
> controller, such as the one found in some PHY devices in the VSC85xx
> family.
> 
> Introduce a set of sfp read/write ops that are going to be used if the
> i2c bus is only capable of doing smbus byte accesses.
> 
> As Single-byte SMBus transaction go against SFF-8472 and breaks the
> atomicity for diagnostics data access, hwmon is disabled in the case
> of SMBus access.
> 
> Moreover, as this may cause other instabilities, print a warning at
> probe time to indicate that the setup may be unreliable because of the
> hardware design.
> 
> Tested-by: Sean Anderson <sean.anderson@linux.dev>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> 
> V2: - Added Sean's tested-by
>     - Added a warning indicating that operations won't be reliable, from
>       Russell and Andrew's reviews
>     - Also added a flag saying we're under a single-byte-access bus, to
>       both print the warning and disable hwmon.
> 
>  drivers/net/phy/sfp.c | 79 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 9369f5297769..6e9d3d95eb95 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -282,6 +282,7 @@ struct sfp {
>  	unsigned int rs_state_mask;
>  
>  	bool have_a2;
> +	bool single_byte_access;

Looking back at that code and our discussions, struct sfp already has an
"i2c_block_size", that is set to 1 for modules with broken emulated
eeprom, and there's already some logging and all the logic to disable
hwmon in such case.

So I think V3 will ditch that "single_byte_access" bool, and rather add
a "i2c_max_block_size" member, set depending on the bus capabilities,
that we'll use to clamp the i2c_block_size.

Of course the big warning to say that the design is inherently broken
because we're on a bus that's limited will stay, but that should make
our life easier for proper non-single-byte smbus support, and also
keep the code flow cleaner.

Let me know what you think,

Maxime