RE: [PATCH net 0/3] Add I2C bus lock for Wangxun

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Thu, Aug 29, 2024 11:28 PM, Andrew Lunn wrote:
> > > O.K, that is bad. The SFP is totally unreliable...
> > >
> > > You however have still not answered my question. What is the firmware
> > > accessing? How does it handle pages?
> > >
> > > The hack you have put in place is per i2c transaction. But accessing
> > > pages is likely to be multiple transactions. One to change the page,
> > > followed by a few reads/writes in the new page, then maybe followed by
> > > a transactions to return to page 0.
> >
> > Do you mean the bus address A0 or A2? Firmware accesses I2C just like driver,
> > but it only change the page once per full transaction, during a possession of
> > the semaphore.  What you fear seems unlikely to happen.
> 
> What sort of SFP is this? QSFP byte 127 selects the page for addresses
> 128-255. Paged 0 and 3 are mandatory, pages 1 and 2 are optional.
>
> SFP+ also uses byte 127 in the same way:
> 
> 10.3 Optional Page Select Byte [Address A2h, Byte 127]
> 
> In order to provide memory space for DWDM and CDR control functions
> and for other potential extensions, multiple Pages can be defined for
> the upper half of the A2h address space. At startup the value of byte
> 127 defaults to 00h, which points to the User EEPROM. This ensures
> backward compatibility for transceivers that do not implement the
> optional Page structure. When a Page value is written to byte 127,
> subsequent reads and writes to bytes 128-255 are made to the relevant
> Page.
> 
> This specification defines functions in Pages 00h-02h. Pages 03-7Fh
> are reserved for future use. Writing the value of a non-supported Page
> shall not be accepted by the transceiver. The Page Select byte shall
> revert to 0 and read / write operations shall be to the unpaged A2h
> memory map.
> 
> ethtool allows you to access more than page 0.
> 
> ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off]
>         [hex on|off] [offset N] [length N] [page N] [bank N] [i2c N]
> 
> > > I think your best solution is to simply take the mutex and never
> > > release it. Block your firmware from accessing the SFP.
> >
> > Firmware accesses the SFP in order to provide information to the BMC.
> > So it cannot simply be blocked.
> 
> Then you have a design problem. And i don't think locking the I2C bus
> per transaction is sufficient.

SFP+ is used on our device.

But I don't quite understand why this lock is not sufficient. The entire
transaction is locked, include setting the bus address and selecting pages,
and all subsequent reads and writes on this page. Also, firmware uses this
lock (hardware semaphore) in the same way. Neither driver nor firmware
switches pages whiling the other is reading / writing ?