netdev - Re: [RFC PATCH] ethtool: add FEC bins histogramm report

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2dc1fc35-a906-461b-b8c1-857c240604a3@nvidia.com>
Date: Wed, 30 Jul 2025 16:47:51 +0300
From: Gal Pressman <gal@...dia.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, Andrew Lunn
 <andrew@...n.ch>, Michael Chan <michael.chan@...adcom.com>,
 Pavan Chebbi <pavan.chebbi@...adcom.com>, Tariq Toukan <tariqt@...dia.com>,
 intel-wired-lan@...ts.osuosl.org, Donald Hunter <donald.hunter@...il.com>,
 Jakub Kicinski <kuba@...nel.org>
Cc: Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 netdev@...r.kernel.org
Subject: Re: [RFC PATCH] ethtool: add FEC bins histogramm report

On 30/07/2025 14:32, Vadim Fedorenko wrote:
> On 30/07/2025 11:42, Gal Pressman wrote:
>> On 30/07/2025 12:29, Vadim Fedorenko wrote:
>>> On 30/07/2025 06:54, Gal Pressman wrote:
>>>> On 29/07/2025 13:23, Vadim Fedorenko wrote:
>>>>> diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/
>>>>> ethtool.c
>>>>> index f631d90c428ac..7257de9ea2f44 100644
>>>>> --- a/drivers/net/netdevsim/ethtool.c
>>>>> +++ b/drivers/net/netdevsim/ethtool.c
>>>>> @@ -164,12 +164,25 @@ nsim_set_fecparam(struct net_device *dev,
>>>>> struct ethtool_fecparam *fecparam)
>>>>>        ns->ethtool.fec.active_fec = 1 << (fls(fec) - 1);
>>>>>        return 0;
>>>>>    }
>>>>> +static const struct ethtool_fec_hist_range netdevsim_fec_ranges[] = {
>>>>> +    {  0,  0},
>>>>> +    {  1,  3},
>>>>> +    {  4,  7},
>>>>> +    { -1, -1}
>>>>> +};
>>>>
>>>> The driver-facing API works nicely when the ranges are allocated as
>>>> static arrays, but I expect most drivers will need to allocate it
>>>> dynamically as the ranges will be queried from the device.
>>>> In that case, we need to define who is responsible of freeing the
>>>> ranges
>>>> array.
>>>
>>> Well, the ranges will not change during link operation, unless the type
>>> of FEC is changed. You may either have static array of FEC ranges per
>>> supported FEC types. Or query it on link-up event and reuse it on every
>>> call for FEC stats. In this case it's pure driver's responsibility to
>>> manage memory allocations. There is definitely no need to re-query
>>> ranges on every single call for stats.
>>
>> This is just adding unnecessary complexity to the drivers, trying to
>> figure out the right lifetime for this array.
>> It will be much simpler if the core passes an array for the drivers to
>> fill. That way both static and dynamic ranges would work the same.
> 
> There is no need to pass huge pre-allocated array for drivers which
> doesn't have support for the histogram. The core doesn't have this info
> about the driver. And again - there is no need to refill ranges array
> every time as it will not change. I believe it's pure driver area of
> knowledge and responsibility.

It's not huge at all, it's an 18 elements array, and I expect most
drivers will need the dynamic ranges.

This is a control path operation, the "price" of allocating and filling
is small, and will reduce the number of driver bugs which get the array
lifetime wrong.

Or just let the driver fill the netlink attributes directly? Not sure if
we have precedent for that.