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 C9E06A050B; Wed, 13 Apr 2022 14:13:34 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4198A40694; Wed, 13 Apr 2022 14:13:34 +0200 (CEST) Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by mails.dpdk.org (Postfix) with ESMTP id 387BF4068B for ; Wed, 13 Apr 2022 14:13:32 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id D73AA5C0224; Wed, 13 Apr 2022 08:13:31 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Wed, 13 Apr 2022 08:13:31 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm1; t=1649852011; x= 1649938411; bh=1iuujvnBjZEgZj50rFULF5rUd3CSjJa6tbAdw/oVzUg=; b=v hlSGWbl5Y/EaBsxyMkcfnfVsHC9yOGUjy+6vPFNOJiQvG87FGM91MNbL0g8WprqR sdvILcy0QmHZALCi3wkdwhj/hJ9BgR6B0gNEF2bavmsB0a7h5WmIicX7YYiBkzwo 2rlbvJf8Iswpo/fPdK8wPEXd6bgNu+TQehQZAlaP0xVBbpFZP6Mh5k0UhtScUaKY r6Hdm48uw8lFaeMcG08MNc7uYAcMk2R0euq+FPRbSuSNrrttpnMD3jYTkyuU9Dwx 1Jmw2OY1m2ns16we2NFJVNkqh+mRtOZzSTPHyZX4txeoZGhKgEqm0OFHA+iGXKIU oXgj+y6jQw5o1B5UFHtrg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; t=1649852011; x=1649938411; bh=1iuujvnBjZEgZ j50rFULF5rUd3CSjJa6tbAdw/oVzUg=; b=Ak5V3HnrcMZW/uqW91yYSAnGFUHqK WP+1qH5fP2afoekkbkUBKJEi8ZUb8ChfTl8oX8MdJGqLmdR+h42fHaghsfSxQVhh m0cDHcozdduTmA/SRUAUkQQdikYN6wYZeLvCRXGJoUqs9/kSRKAWRczdVlNpGiZ+ XTodqgIUVHlWjdrbI7lR0IF4DHM650GCv4jtnZXHUGCFj4yCl3ydx6uFkXuLvf60 nHr/0yh2/shuQ7lxtmY6+wIPJtszvFhGCR/U7sn8P0OfXLDl1s3pSSDbPvwuPNWv VqlW7RedFc56RITOyBWwoe7K3GFMttkerulnw0R6cMoAVkkVRZyV2aHeg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvvddrudeluddgvdefucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepudeggfdvfeduffdtfeeglefghfeukefgfffhueejtdetuedtjeeu ieeivdffgeehnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 13 Apr 2022 08:13:30 -0400 (EDT) From: Thomas Monjalon To: "Zhang, RobinX" , Bruce Richardson Cc: "dev@dpdk.org" , "Yang, Qiming" , "Zhang, Qi Z" , "Yang, SteveX" , david.marchand@redhat.com, andrew.rybchenko@oktetlabs.ru Subject: Re: [PATCH v2] common/sff_module: add telemetry command to dump module EEPROM Date: Wed, 13 Apr 2022 14:13:29 +0200 Message-ID: <3156254.N7aMVyhfb1@thomas> In-Reply-To: References: <20220215101853.919735-1-robinx.zhang@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 11/04/2022 11:13, Bruce Richardson: > On Mon, Apr 11, 2022 at 09:13:47AM +0100, Zhang, RobinX wrote: > > Hi Bruce, > > > > > -----Original Message----- > > > From: Richardson, Bruce > > > Sent: Friday, April 8, 2022 7:27 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 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. > > > > > > > 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 for the additional explanation. Adding more folks on CC who may have > more thoughts on the best way to handle this. That's ethdev and/or net driver code. If I understand well, SFF are standards and no tight to any HW, right? In this case, I think the common code can be in ethdev library.