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

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

On 29/07/2025 17:17, Andrew Lunn wrote:
> On Tue, Jul 29, 2025 at 05:01:06PM +0100, Vadim Fedorenko wrote:
>> On 29/07/2025 14:48, Andrew Lunn wrote:
>>>> +        name: fec-hist-bin-low
>>>> +        type: s32
>>>
>>> Signed 32 bit
>>>
>>>> +struct ethtool_fec_hist_range {
>>>> +	s16 low;
>>>
>>> Signed 16 bit.
>>>
>>>> +		if (nla_put_u32(skb, ETHTOOL_A_FEC_STAT_FEC_HIST_BIN_LOW,
>>>> +				ranges[i].low) ||
>>>
>>> Unsigned 32 bit.
>>>
>>> Could we have some consistency with the types.
>>
>> Yeah, it looks a bit messy. AFAIK, any type of integer less than 32 bits
>> will be extended to 32 bits anyway,
> 
> sign extended, not just extended. That makes things more fun.
> 
>> so I believe it's ok to keep smaller
>> memory footprint
> 
>   .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c
>   .../ethernet/fungible/funeth/funeth_ethtool.c
>   .../ethernet/hisilicon/hns3/hns3_ethtool.c
>   drivers/net/ethernet/intel/ice/ice_ethtool.c
>   .../marvell/octeontx2/nic/otx2_ethtool.c
>   .../ethernet/mellanox/mlx5/core/en_ethtool.c
>   drivers/net/ethernet/sfc/ethtool.c
>   drivers/net/ethernet/sfc/siena/ethtool.c
> 
> These are all huge drivers, with extensive memory footprint.  How many
> bins are we talking about? 5? One per PCS? I suspect the size
> difference it deep in the noise.

Well, it's currently up to 18 according to different possible FEC algos,
but I agree, it's not that much.

> 
>> for the histogram definition in the driver but still use
>> s32 as netlink attr type. I'll change the code to use nla_put_s32()
>> to keep sign info.
> 
> So bins can have negative low/high values?

The only one bin will have negative value is the one to signal the end
of the list of the bins, which is not actually put into netlink message.
It actually better to change spec to have unsigned values, I believe.

> 
> 	Andrew