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

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

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?
2. For the driver approach you previously took, how was the presence of
   hardware detected to load the driver? 
3. Does this work on SFPs need to interact with the NIC drivers in any way?

Thanks,
/Bruce