RE: [PATCH v2] eeprom/optoe: driver to read/write SFP/QSFP/CMIS EEPROMS

On Fri, Feb 26, 2021 at 6:46 PM -0800, Don Bollinger wrote:

On Fri, Feb 26, 2021 at 2:35 PM -0800, Andrew Lunn wrote:

quoted

On Mon, Feb 15, 2021 at 11:38:21AM -0800, Don Bollinger wrote:

quoted

optoe is an i2c based driver that supports read/write access to all
the pages (tables) of MSA standard SFP and similar devices
(conforming to the SFF-8472 spec), MSA standard QSFP and similar
devices (conforming to the SFF-8636 spec) and CMIS and similar
devices (conforming to the Common Management Interface

Specfication).

quoted

Hi Don

And we have seen this code before, and the netdev Maintainers have
argued against it before.

Yes, though I have tried to make it more acceptable, and also more useful

to

you...

quoted

quoted

These devices provide identification, operational status and control
registers via an EEPROM model.  These devices support one or 3 fixed
pages
(128 bytes) of data, and one page that is selected via a page
register on the first fixed page.  Thus the driver's main task is to
map these pages onto a simple linear address space for user space
management

applications.

quoted

See the driver code for a detailed layout.

I assume you have seen the work NVIDIA submitted last week? This idea
of linear pages is really restrictive and we are moving away from it.

Yes, I see at least most of it in your response in the netdev archive.  The
linear pages are really a very simple mapping, but they also provide a very
simple access model.  I can readily accommodate Moshe's addressing (i2c
addr, page, bank, offset, len).  Details below...

quoted

quoted

The EEPROM data is accessible to user space and kernel consumers via
the nvmem interface.

ethtool -m ?

This is actually below ethtool.  Ethtool has to fetch the data from the

eeprom

using an appropriate i2c interface (these are defined to be i2c devices).

And,

to fetch the data from any but the first 256 bytes the code ethtool calls

must

deal with the page register on the device.  This driver handles the page
register, for both SFP and QSFP/CMIS devices.  This driver would be a

useful

path for ethtool to use when someone decides to make that data available.
Note for example, CMIS devices put the DOM data (per-channel Tx power,
Rx Power, laser bias) on page 0x11.  When you want that data, you'll have

to

go past 256 bytes and deal with the page register.
optoe will do that for you.

To be more specific, optoe is only replacing the functionality of
drivers/net/phy/sfp.c, the functions of sfp_i2c_read() and sfp_i2c_write().
These are the routines at the very bottom of the ethtool stack that actually
execute the i2c calls to get the data.  The existing routines are very
limited, in that they don't handle pages at all.  Hence they can only reach
256 bytes of QSFP EEPROM data and 512 bytes of SFP EEPROM data.  I can
propose a shorter cleaner replacement for each of those routines which will
provide access to the rest of the data on those devices.

ALL of the ethtool stack remains unchanged, works exactly the same, and will
now provide access to more data on the EEPROMs.  We may have to remove some
range checks to allow requests to pages beyond page 0.

Note that Moshe's RFC does not include the actual access routines, the
equivalent of sfp_i2c_read/write.  That will in fact require managing pages.
Using optoe, those routines are a few lines of code to map his
addr/page/bank/offset and a call to the new sfp_i2c_read/write calls.

And, all of the i2c/EEPROM accesses go through the same code that routinely
handles the rest of the EEPROMs in the system, with all of the architectural
consolidation, i2c anomaly handling, and simplified support inherent in
sharing common code.
 

quoted

In the past, this code has been NACKed because it does not integrate
into the networking stack. Is this attempt any different?

Yes.  I have updated the driver with all the latest changes in at24.c, the
primary eeprom driver.  That update includes use of the nvmem interface,
which means this driver can now be called by kernel code.  I believe it

would

be useful and easy to replace the sfp.c code that accesses the eeprom with
nvmem calls to this driver.  By doing so, you will be able to access the
additional pages of data on those devices.  You would also get the benefit

of

sharing common code the other eeprom drivers.  As part of that cleanup,

the

explicit sysfs creation of an eeprom device has been removed.
Full disclosure, the nvmem interface creates that device now.

quoted

Thanks
	Andrew

One more point, I have been requested by the SONiC team to upstream this
driver.  It is on their short list of kernel code that is not upstream,

and they

want to fix that.  They are not consumers of the netdev interface, but

they

(and other NOS providers) have a need for a driver to access the eeprom
data.  Greg KH was involved in that request, and I related your concerns

to

him, as well as my position that this is an eeprom driver with partners

that

need it independent of netdev.  His response:

GKH> I can't remember the actual patch anymore, but you are right, it's
"just"
GKH> poking around in the EEPROM for the device, and doing what you want
GKH> in userspace, which really should be fine.  Submit it and I'll be
GKH> glad to
review it
GKH> under that type of functionality being added.

He didn't say he would override your position, but he suggested it was
appropriate to submit it.  So, here it is.

Thus:
1.  There are existing NOS platforms that need and use this functionality,
they want it in the upstream kernel.
2.  This driver is better than sfp.c, and could easily be plugged in to

improve

netdev and ethtool access to SFP/QSFP/CMIS EEPROM data.

Don