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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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.