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

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Apr 11, 2022 at 09:13:47AM +0100, Zhang, RobinX wrote:
> Hi Bruce,
> 
> > -----Original Message-----
> > From: Richardson, Bruce <bruce.richardson@intel.com>
> > Sent: Friday, April 8, 2022 7:27 PM
> > To: Zhang, RobinX <robinx.zhang@intel.com>
> > Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Yang, SteveX <stevex.yang@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@intel.com>
> > > > Sent: Friday, April 8, 2022 7:01 PM
> > > > To: Zhang, RobinX <robinx.zhang@intel.com>
> > > > Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>; Yang, SteveX <stevex.yang@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@intel.com>
> > > > > > Sent: Friday, April 8, 2022 6:33 PM
> > > > > > To: Zhang, RobinX <robinx.zhang@intel.com>
> > > > > > Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang,
> > > > > > Qi Z <qi.z.zhang@intel.com>; Yang, SteveX
> > > > > > <stevex.yang@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@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 for the additional explanation. Adding more folks on CC who may have
more thoughts on the best way to handle this.

/Bruce