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: <4270ff14-06cd-4a78-afe7-1aa5f254ebb6@linux.dev>
Date: Tue, 29 Jul 2025 17:36:50 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Andrew Lunn <andrew@...n.ch>
Cc: Michael Chan <michael.chan@...adcom.com>,
 Pavan Chebbi <pavan.chebbi@...adcom.com>, Tariq Toukan <tariqt@...dia.com>,
 Gal Pressman <gal@...dia.com>, intel-wired-lan@...ts.osuosl.org,
 Donald Hunter <donald.hunter@...il.com>, Jakub Kicinski <kuba@...nel.org>,
 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 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