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

["Zhang, RobinX" <robinx.zhang@intel.com>]

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.

> Thanks,
> /Bruce