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: <8b22e9d3-e4d2-4726-9622-28881b2cd406@linux.dev>
Date: Wed, 30 Jul 2025 12:32:43 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Gal Pressman <gal@...dia.com>, 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 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.