From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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: <xms:AdrhZVeoqJJvlb0US0n6wuOH_WP3gYM2fEY5kCcVnblE4EMMSJ1SIQ>
<xme:AdrhZTPzN-MPcAHT4tX4F0K30wGPMddrgGF9uZVz-5rUhGMUjVnsugATjYMuN8BDi
iOvdE9O6iuTfb4d0g>
X-ME-Received: <xmr:AdrhZei2tmfg-nmNW2OE_eACcpPHGIfpCRbcgvAKEXM81rIAJApaRYzpehZNmBJYMqRiEzw9sOfjF8Z8lhD-o-F2Vw>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvledrhedugdehfecutefuodetggdotefrodftvf
curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu
uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc
fjughrpefhvfevufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr
shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg
ftrfgrthhtvghrnheptdejieeifeehtdffgfdvleetueeffeehueejgfeuteeftddtieek
gfekudehtdfgnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh
homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth
X-ME-Proxy: <xmx:AdrhZe-842PodYjcQ9snUPiDafIf20WCS7k1nxQKP7OYtVN1b9XwdA>
<xmx:AdrhZRvwTc6fzxENVOfVlDz2yD4yQJEyfEffExxZXdog3AHxWYLzzQ>
<xmx:AdrhZdEZsiDWX4V0IFa6pmSsF7UWmeOG_Itjook7U52a8OAXuZLwfQ>
<xmx:AdrhZZKI20S_e0SAnBBR0u1gXFQHv6hmSP4O_UekMGR5i0UnRIDwaA>
Feedback-ID: i47234305:Fastmail
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri,
1 Mar 2024 08:37:04 -0500 (EST)
From: Thomas Monjalon <thomas@monjalon.net>
To: Ferruh Yigit <ferruh.yigit@amd.com>
Cc: dev@dpdk.org, Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <thomas@monjalon.net>
> > ---
> >
> > 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.