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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!