[PATCH v2] common/sff_module: add telemetry command to dump module EEPROM

[Zhang, RobinX]

Hi Bruce,

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson at intel.com>
> Sent: Friday, April 8, 2022 7:27 PM
> To: Zhang, RobinX <robinx.zhang at intel.com>
> Cc: dev at dpdk.org; Yang, Qiming <qiming.yang at intel.com>; Zhang, Qi Z
> <qi.z.zhang at intel.com>; Yang, SteveX <stevex.yang at intel.com>
> Subject: Re: [PATCH v2] common/sff_module: add telemetry command to
> dump module EEPROM
> 
> On Fri, Apr 08, 2022 at 12:20:23PM +0100, Zhang, RobinX wrote:
> > Hi Bruce
> >
> > > -----Original Message-----
> > > From: Richardson, Bruce <bruce.richardson at intel.com>
> > > Sent: Friday, April 8, 2022 7:01 PM
> > > To: Zhang, RobinX <robinx.zhang at intel.com>
> > > Cc: dev at dpdk.org; Yang, Qiming <qiming.yang at intel.com>; Zhang, Qi Z
> > > <qi.z.zhang at intel.com>; Yang, SteveX <stevex.yang at intel.com>
> > > Subject: Re: [PATCH v2] common/sff_module: add telemetry command
> to
> > > dump module EEPROM
> > >
> > > On Fri, Apr 08, 2022 at 11:55:07AM +0100, Zhang, RobinX wrote:
> > > > Hi Bruce,
> > > >
> > > > > -----Original Message-----
> > > > > From: Richardson, Bruce <bruce.richardson at intel.com>
> > > > > Sent: Friday, April 8, 2022 6:33 PM
> > > > > To: Zhang, RobinX <robinx.zhang at intel.com>
> > > > > Cc: dev at dpdk.org; Yang, Qiming <qiming.yang at intel.com>; Zhang,
> > > > > Qi Z <qi.z.zhang at intel.com>; Yang, SteveX
> > > > > <stevex.yang at intel.com>
> > > > > Subject: Re: [PATCH v2] common/sff_module: add telemetry
> command
> > > to
> > > > > dump module EEPROM
> > > > >
> > > > > On Fri, Apr 08, 2022 at 10:23:30AM +0000, Robin Zhang wrote:
> > > > > > This patch introduce a new telemetry command '/sff_module/info'
> > > > > > to dump format module EEPROM information.
> > > > > >
> > > > > > The format support for SFP(Small Formfactor Pluggable)/SFP+
> > > > > > /QSFP+(Quad Small Formfactor Pluggable)/QSFP28 modules based
> > > > > > on SFF(Small Form Factor) Committee specifications
> > > > > > SFF-8079/SFF-8472/SFF-8024/SFF-8636.
> > > > > >
> > > > > > Signed-off-by: Robin Zhang <robinx.zhang at intel.com>
> > > > > > ---
> > > > > >
> > > > > > v2:
> > > > > > - Redesign the dump function as a telemetry command, so that
> > > > > > the
> > > > > EEPROM
> > > > > >   information can be used by other app.
> > > > > >
> > > > > > - The usage like this:
> > > > > >
> > > > > >   Launch the primary application with telemetry:
> > > > > >   Take testpmd as example: ./app/dpdk-testpmd
> > > > > >
> > > > > >   Then launch the telemetry client script:
> > > > > >   ./usertools/dpdk-telemetry.py
> > > > > >
> > > > > >   In telemetry client run command:
> > > > > >   --> /sff_module/info,<port number>
> > > > > >
> > > > > >   Both primary application and telemetry client will show the
> formated
> > > > > >   module EEPROM information.
> > > > > >
> > > > > >  drivers/common/meson.build                |    1 +
> > > > > >  drivers/common/sff_module/meson.build     |   16 +
> > > > > >  drivers/common/sff_module/sff_8079.c      |  672
> ++++++++++++++
> > > > > >  drivers/common/sff_module/sff_8472.c      |  301 ++++++
> > > > > >  drivers/common/sff_module/sff_8636.c      | 1004
> > > > > +++++++++++++++++++++
> > > > > >  drivers/common/sff_module/sff_8636.h      |  592 ++++++++++++
> > > > > >  drivers/common/sff_module/sff_common.c    |  415 +++++++++
> > > > > >  drivers/common/sff_module/sff_common.h    |  192 ++++
> > > > > >  drivers/common/sff_module/sff_telemetry.c |  142 +++
> > > > > >  drivers/common/sff_module/sff_telemetry.h |   41 +
> > > > > >  drivers/common/sff_module/version.map     |    9 +
> > > > > >  11 files changed, 3385 insertions(+)  create mode 100644
> > > > > > drivers/common/sff_module/meson.build
> > > > > >  create mode 100644 drivers/common/sff_module/sff_8079.c
> > > > > >  create mode 100644 drivers/common/sff_module/sff_8472.c
> > > > > >  create mode 100644 drivers/common/sff_module/sff_8636.c
> > > > > >  create mode 100644 drivers/common/sff_module/sff_8636.h
> > > > > >  create mode 100644 drivers/common/sff_module/sff_common.c
> > > > > >  create mode 100644 drivers/common/sff_module/sff_common.h
> > > > > >  create mode 100644 drivers/common/sff_module/sff_telemetry.c
> > > > > >  create mode 100644 drivers/common/sff_module/sff_telemetry.h
> > > > > >  create mode 100644 drivers/common/sff_module/version.map
> > > > > >
> > > > > Is this is whole new driver just to provide telemetry dumps of
> > > > > SFP information? I can understand the problem somewhat - though
> > > > > I am in some doubt that telemetry is the best way to expose this
> > > > > information
> > > > > - but creating a new driver seems the wrong approach here. SFPs
> > > > > are for NIC devices, so why isn't this available in a common API
> > > > > such as
> > > ethdev?
> > > > >
> > > >
> > > > I have considered add this function as a new telemetry command of
> > > ethdev (like '/ethdev/sff_module_info') to dump these SFP information.
> > > > But I'm not sure if it's acceptable to add all these production
> > > > code
> > > (sff_8xxx.c) into lib/ethdev?
> > > > If it's OK, I can make V3 patches to change it as a telemetry
> > > > command of
> > > ethdev.
> > > >
> > >
> > > Hi,
> > >
> > > I think some discussion is needed before you go preparing a new
> > > version of this patchset.
> > >
> > > Some initial questions:
> > >
> > > 1. Does SFF code apply only to Intel products/NICs or is it multi-vendor?
> > The SFF code apply to multi-vendor.
> > In fact, it's applied to all the NIC driver which implemented dev_ops-
> >get_module_eeprom.
> >
> > > 2. For the driver approach you previously took, how was the presence of
> > >    hardware detected to load the driver?
> > The purpose of put these production code into drivers/common is want to
> treat it as a common function for NIC drivers.
> > It will not related to any presence of hardware.
> >
> > > 3. Does this work on SFPs need to interact with the NIC drivers in any way?
> > >
> > Yes, just like my answer in question 1, the module EEPROM raw data is get
> from dev_ops->get_module_eeprom.
> > So need the NIC drivers to implement dev_ops->get_module_eeprom.
> >
> 
> So is the intent that individual NIC drivers would add a get_module_eeprom
> function to their drivers pointing at this driver? If so, this approach of putting
> the code in drivers/common does make sense. However, this needs to be
> better explained in the patch description, and maybe include with the driver
> patch (which should probably be split up into easier reviewed sections),
> additional patches to add the get_eeprom function to some drivers to show
> use.
> 

Let me explain in more detail.

This patch actually include two parts:
1. Module EEPROM raw data parser code
    Files: sff_common.h, sff_common.c, sff_8xxx.*
2. Add new telemetry command
    Files: sff_telemetry.h, sff_telemetry.c

Part 1 will only parsing the module EEPROM raw data base on different module type.
Now DPDK support 4 types that defined in rte_dev_info.h with macro RTE_ETH_MODULE_SFF_8xxx.

Part 2 will call rte_eth_dev_get_module_info and rte_eth_dev_get_module_eeprom to get the module EEPROM raw data, then pass the raw data to Part 1 parser code. Finally, Part 1 parser code will print formatted information.

So, these codes are more likely a common tool than a common driver, because it will only read the module EEPROM raw data from NIC PMD driver.
For those NIC drivers who has not implemented get_module_info and get_module_eeprom dev_ops, we will simply return not support.

> Thanks,
> /Bruce