Hi Bruce,
> -----Original Message-----
> From: Richardson, Bruce <bruce.richard...@intel.com>
> Sent: Friday, April 8, 2022 7:27 PM
> To: Zhang, RobinX <robinx.zh...@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.y...@intel.com>; Zhang, Qi Z
> <qi.z.zh...@intel.com>; Yang, SteveX <stevex.y...@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.richard...@intel.com>
> > > Sent: Friday, April 8, 2022 7:01 PM
> > > To: Zhang, RobinX <robinx.zh...@intel.com>
> > > Cc: dev@dpdk.org; Yang, Qiming <qiming.y...@intel.com>; Zhang, Qi Z
> > > <qi.z.zh...@intel.com>; Yang, SteveX <stevex.y...@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.richard...@intel.com>
> > > > > Sent: Friday, April 8, 2022 6:33 PM
> > > > > To: Zhang, RobinX <robinx.zh...@intel.com>
> > > > > Cc: dev@dpdk.org; Yang, Qiming <qiming.y...@intel.com>; Zhang,
> > > > > Qi Z <qi.z.zh...@intel.com>; Yang, SteveX
> > > > > <stevex.y...@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.zh...@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,
> /Bruce
