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

From: "Zhang, RobinX" <robinx.zhang@intel.com>
To: "Richardson, Bruce" <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <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
Date: Fri, 8 Apr 2022 10:55:07 +0000	[thread overview]
Message-ID: <BN9PR11MB5529D645A0C6FF4C670F25C09BE99@BN9PR11MB5529.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YlAPZwaj7OdxBjsx@bricha3-MOBL.ger.corp.intel.com>

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.

> /Bruce