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 E730F43BCF; Fri, 1 Mar 2024 14:37:07 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C006843368; Fri, 1 Mar 2024 14:37:07 +0100 (CET) Received: from fout8-smtp.messagingengine.com (fout8-smtp.messagingengine.com [103.168.172.151]) by mails.dpdk.org (Postfix) with ESMTP id 6C6A44026C for ; Fri, 1 Mar 2024 14:37:06 +0100 (CET) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailfout.nyi.internal (Postfix) with ESMTP id CFA2913800B6; Fri, 1 Mar 2024 08:37:05 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Fri, 01 Mar 2024 08:37:05 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm1; t=1709300225; x=1709386625; bh=ok+jD8JSGPCaxmGEu53GR76pF5xXt10RAKEdj/+KB4Q=; b= oFMWLBuSl7a8OLKjI1pR4dcibeKeKvHMUSNDJvyJdl5rwJvBXXC5HQe3kZVNVApB xXUJ92rBgHjwe8DwwchOtMMB9hSnJRS6QPQj8tPGEoMexm0R3bLxGNtbYGZOIM5K pmRRnVzATvqOw/YdOzFq2NpY3ZH3Ec8THyWwSr/sfYEwp0UIfG4OpaCMnr5RxbaP 2UIgZ0e4xed8NpTnk8ivTYMR6Dp/vpWH931WMJ/S6Hg0syubHKYh7bz8OP4wGDRm X0s/n8tt84BrJkXkbzXtscuwBtsTfCF+/Nwl60Jt0rdZY8LWa4q7H9OlTR2VIR3+ saOQOBj7o4l5kNgl618v6Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1709300225; x= 1709386625; bh=ok+jD8JSGPCaxmGEu53GR76pF5xXt10RAKEdj/+KB4Q=; b=P L2IwsBvOBKa0Y5XdCMGKpF5+h8r6ICmamLrV+RxOUDrBC1bVFEDTgRSAvBRxyzWj LU36/t7sAsN1CzHFxVcbZ+IHlENXF8nfSMXHEGLGnLTGFLcHPBzx9uEdC1sxvOmp ntwfVMI/5YahThOGjqYRINRn7nOQj/RiVOZe7Laz0yvKtG1diHYjXSwefq6SMoNm oflJaaDY0um8eXZftLkkjn16tEaGaZC602Xya3iHeNjSxWoljDqVR0izAdLKoBnq ZhtKltjPkJ/2SS+QCHpvzvy5KGMZxviKQ8mLhpC+01ihxZK5Vp+FIE7GS2kq9jaJ Kz5GKH87KiSgOcH/9thig== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvledrhedugdehfecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvfevufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnheptdejieeifeehtdffgfdvleetueeffeehueejgfeuteeftddtieek gfekudehtdfgnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 1 Mar 2024 08:37:04 -0500 (EST) From: Thomas Monjalon To: Ferruh Yigit Cc: dev@dpdk.org, Andrew Rybchenko Subject: Re: [PATCH v2] ethdev: add Linux ethtool link mode conversion Date: Fri, 01 Mar 2024 14:37:01 +0100 Message-ID: <3271586.N7aMVyhfb1@thomas> In-Reply-To: <14dc648a-dc45-4308-b096-cd1a189a2e80@amd.com> References: <20240229123653.1379466-1-thomas@monjalon.net> <20240229154343.1752555-1-thomas@monjalon.net> <14dc648a-dc45-4308-b096-cd1a189a2e80@amd.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 01/03/2024 14:12, Ferruh Yigit: > On 2/29/2024 3:42 PM, Thomas Monjalon wrote: > > Speed capabilities of a NIC may be discovered through its Linux > > kernel driver. It is especially useful for bifurcated drivers, > > so they don't have to duplicate the same logic in the DPDK driver. > > > > Parsing ethtool speed capabilities is made easy thanks to > > the functions added in ethdev for internal usage only. > > Of course these functions work only on Linux, > > so they are not compiled in other environments. > > > > In order to ease parsing, the ethtool macro names are parsed > > externally in a shell command which generates a C array > > included in this patch. > > It also avoids to depend on a kernel version. > > This C array should be updated in future to get latest ethtool bits. > > Note it is easier to update this array than adding new cases > > in a parsing code. > > > > The types in the functions are following the ethtool type: > > uint32_t for bitmaps, and int8_t for the number of 32-bitmaps. > > > > Signed-off-by: Thomas Monjalon > > --- > > > > A follow-up patch will be sent to use these functions in mlx5. > > I suspect mana could use this parsing as well. > > > > Is the usecase driver get link info via ibverbs and convert it to DPDK > link info? The use case is to get capabilities from the kernel driver via ethtool ioctl. > How complex or duplicated effort to get link info directly via DPDK > functions? This is done by the driver. This is how mlx5 driver is getting speed capabilities. > Because this approach is can be applied to only limited devices in DPDK > and solving an issue DPDK already has a solution, does it worth to the > code it adds? It is going to replace code in mlx5 driver. I could add this code in mlx5 driver, but it could help other drivers in future like mana. > > + speed = link_modes[bit]; > > + if (speed == 0) > > + return RTE_ETH_LINK_SPEED_AUTONEG; > > + RTE_BUILD_BUG_ON(RTE_ETH_LINK_SPEED_AUTONEG != 0); > > > > I think for above two checks, we can't really get the speed from > provided ethtool enum, and intention is to return something ineffective, > intention is not really return AUTONEG, right? If so why not directly > return 0? Yes it could return 0 directly, but the namespace of the returned value is RTE_ETH_LINK_SPEED_. Also it is semantically correct: if no other capability found, there is no other choice than autoneg. > > + > > + /* duplex is LSB */ > > + duplex = (speed & 1) ? > > + RTE_ETH_LINK_HALF_DUPLEX : > > + RTE_ETH_LINK_FULL_DUPLEX; > > + speed &= RTE_GENMASK32(31, 1); > > As trying to zero the LSB, following also work, > > speed &= ~UINT32_C(1) Indeed, this is what RTE_GENMASK32 is doing. But I think using RTE_GENMASK32 better convey the intent. [...] > > + for (word = 0; word < nwords; word++) { > > + for (bit = 0; bit < 32; bit++) { > > May be (sizeof(bitmap) * CHAR_BIT) instead of hardcoded 32, although not > sure if it is required. Anyway we are using RTE_BIT32 below, so we must know it is 32 bits. > > + if ((bitmap[word] & RTE_BIT32(bit)) == 0) > > + continue; > > + ethdev_bitmap |= rte_eth_link_speed_ethtool(word * 32 + bit); [...] > > --- a/lib/ethdev/meson.build > > +++ b/lib/ethdev/meson.build > > +if is_linux > > + driver_sdk_headers += files( > > + 'ethdev_linux_ethtool.h', > > + ) > > + sources += files( > > + 'ethdev_linux_ethtool.c', > > + ) > > +endif > > Should meson check if 'linux/ethtool.h' exists, for anycase? It is an old API header file. Why would not be there? If we make it conditional here, we'll need to make it conditional in the caller.