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

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

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.

> 
>>   
>>   static void
>> -nsim_get_fec_stats(struct net_device *dev, struct ethtool_fec_stats *fec_stats)
>> +nsim_get_fec_stats(struct net_device *dev, struct ethtool_fec_stats *fec_stats,
>> +		   const struct ethtool_fec_hist_range **ranges)
>>   {
>> +	*ranges = netdevsim_fec_ranges;
>> +
>>   	fec_stats->corrected_blocks.total = 123;
>>   	fec_stats->uncorrectable_blocks.total = 4;
>> +
>> +	fec_stats->hist[0] = 345;
>> +	fec_stats->hist[1] = 12;
>> +	fec_stats->hist[2] = 2;
>>   }
>>   
>>   static int nsim_get_ts_info(struct net_device *dev,
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index de5bd76a400ca..9421a5e31af21 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -492,6 +492,17 @@ struct ethtool_pause_stats {
>>   };
>>   
>>   #define ETHTOOL_MAX_LANES	8
>> +#define ETHTOOL_FEC_HIST_MAX	18
> 
> I suspect we might need to increase this value in the future, so I like
> the fact that it's not hardcoded anywhere in the uapi.

Yep, that's the plan