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 <tho...@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.
