From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 19A57A0501; Fri, 8 Apr 2022 13:27:08 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BA8D04067E; Fri, 8 Apr 2022 13:27:07 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mails.dpdk.org (Postfix) with ESMTP id D598B4003F for ; Fri, 8 Apr 2022 13:27:04 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1649417225; x=1680953225; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=CaleJHgcVGZ6gXTaMM1BN49BgxhKnhJdRV7nJnb1U5M=; b=izWtytVLcWRTx4VTWC/rHvj7EADaz/mXn3gXjGHxzDPLbTZey2/MLGHm NHidZMb49b9v3dCdXKxfJaffSwPBdnQN64LZB+EXKl6fiaRZakQiITipQ JkiKysSSqQqNZlEb2O3wkgj+3RVHBZBM5exIIjEiP+oJ6RB+KWkyy/JZ/ XVHemp0Z61ZF7IBViyIoHAncpC3RuY0xAUAWIsS1l17WhvqPM8trb7JwP crnpOFcXL2V2q9HBFQIBcoyezmrcjvH+3CV5Ita2/dxjkxRxGVaTJ55Yr n2u0QMKRzI+yqziCN+uIDQoLWz+6AHXHulZYJZLFhQxw0kUMcOuiZrs/q g==; X-IronPort-AV: E=McAfee;i="6400,9594,10310"; a="249101008" X-IronPort-AV: E=Sophos;i="5.90,244,1643702400"; d="scan'208";a="249101008" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Apr 2022 04:27:04 -0700 X-IronPort-AV: E=Sophos;i="5.90,244,1643702400"; d="scan'208";a="571459457" Received: from bricha3-mobl.ger.corp.intel.com ([10.213.224.53]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 08 Apr 2022 04:27:02 -0700 Date: Fri, 8 Apr 2022 12:26:59 +0100 From: Bruce Richardson To: "Zhang, RobinX" Cc: "dev@dpdk.org" , "Yang, Qiming" , "Zhang, Qi Z" , "Yang, SteveX" Subject: Re: [PATCH v2] common/sff_module: add telemetry command to dump module EEPROM Message-ID: References: <20220215101853.919735-1-robinx.zhang@intel.com> <20220408102330.774749-1-robinx.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, Apr 08, 2022 at 12:20:23PM +0100, Zhang, RobinX wrote: > Hi Bruce > > > -----Original Message----- > > From: Richardson, Bruce > > Sent: Friday, April 8, 2022 7:01 PM > > To: Zhang, RobinX > > Cc: dev@dpdk.org; Yang, Qiming ; Zhang, Qi Z > > ; Yang, SteveX > > 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 > > > > Sent: Friday, April 8, 2022 6:33 PM > > > > To: Zhang, RobinX > > > > Cc: dev@dpdk.org; Yang, Qiming ; Zhang, Qi Z > > > > ; Yang, SteveX > > > > 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 > > > > > --- > > > > > > > > > > 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, > > > > > > > > > > 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