Re: [PATCH v2] ethdev: add Linux ethtool link mode conversion

[Thomas Monjalon <thomas@monjalon.net>]

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.