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 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.

Thanks,
/Bruce