netdev - Re: [RFC PATCH v3] 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: <25ab441c-84e8-4c47-8d13-1b88d78ed4c6@linux.dev>
Date: Mon, 4 Aug 2025 09:31:06 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Carolina Jubran <cjubran@...dia.com>, Andrew Lunn <andrew@...n.ch>,
 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>
Cc: Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 netdev@...r.kernel.org
Subject: Re: [RFC PATCH v3] ethtool: add FEC bins histogramm report

On 03/08/2025 12:24, Carolina Jubran wrote:
> 
> 
> On 02/08/2025 9:30, Vadim Fedorenko wrote:
>> diff --git a/Documentation/networking/ethtool-netlink.rst b/ 
>> Documentation/networking/ethtool-netlink.rst
>> index ab20c644af248..b270886c5f5d5 100644
>> --- a/Documentation/networking/ethtool-netlink.rst
>> +++ b/Documentation/networking/ethtool-netlink.rst
>> @@ -1541,6 +1541,11 @@ Drivers fill in the statistics in the following 
>> structure:
>>   .. kernel-doc:: include/linux/ethtool.h
>>       :identifiers: ethtool_fec_stats
>> +Statistics may have FEC bins histogram attribute 
>> ``ETHTOOL_A_FEC_STAT_HIST``
>> +as defined in IEEE 802.3ck-2022 and 802.3df-2024. Nested attributes 
>> will have
>> +the range of FEC errors in the bin (inclusive) and the amount of 
>> error events
>> +in the bin.
>> +
> 
> Maybe worth mentioning per-lane histograms here.

Yep, will do it

> 
>>   FEC_SET
>>   =======
>> diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ 
>> ethtool.c
>> index f631d90c428ac..1dc9a6c126b24 100644
>> --- a/drivers/net/netdevsim/ethtool.c
>> +++ b/drivers/net/netdevsim/ethtool.c
>> @@ -164,12 +164,29 @@ 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},
>> +    { 0, 0}
>> +};
>>
> 
> Following up on the discussion from v1, I agree with Gal's concern about 
> pushing array management into the driver. It adds complexity especially 
> when ranges depend on FEC mode.

Still don't really get the reason. You have finite amount of FEC bin
configurations, per hardware per FEC type, you know current FEC type
value and can choose static range based on this knowledge. Why do you
want to query device over PCIe multiple times to figure out the same
configuration every time?

> 
> The approach Andrew suggested makes sense to me. A simple helper to add 
> a bin would support both static and dynamic cases.
> 
>>   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].bin_value = 345;
> 
> bin_value is 345 but the per-lane sum is 445.

ahh.. yeah, will fix it

>> +    fec_stats->hist[1].bin_value = 12;
>> +    fec_stats->hist[2].bin_value = 2;
>> +    fec_stats->hist[0].bin_value_per_lane[0] = 125;
>> +    fec_stats->hist[0].bin_value_per_lane[1] = 120;
>> +    fec_stats->hist[0].bin_value_per_lane[2] = 100;
>> +    fec_stats->hist[0].bin_value_per_lane[3] = 100;
>>   }
>> +static int fec_put_hist(struct sk_buff *skb, const struct 
>> fec_stat_hist *hist,
>> +            const struct ethtool_fec_hist_range *ranges)
>> +{
>> +    struct nlattr *nest;
>> +    int i, j;
>> +
>> +    if (!ranges)
>> +        return 0;
>> +
>> +    for (i = 0; i < ETHTOOL_FEC_HIST_MAX; i++) {
>> +        if (i && !ranges[i].low && !ranges[i].high)
>> +            break;
>> +
>> +        nest = nla_nest_start(skb, ETHTOOL_A_FEC_STAT_HIST);
>> +        if (!nest)
>> +            return -EMSGSIZE;
>> +
>> +        if (nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_LOW,
>> +                 ranges[i].low) ||
>> +            nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_HIGH,
>> +                 ranges[i].high) ||
>> +            nla_put_uint(skb, ETHTOOL_A_FEC_HIST_BIN_VAL,
>> +                     hist[i].bin_value))
> 
> Should skip bins where hist[i].bin_value isn’t set.

I'm kinda disagree. If the bin is configured, then the HW must provide a
value for it. Otherwise we will have inconsistency in user's output.

I was thinking of adding WARN_ON_ONCE() for such cases actually.

> 
> 
> Thanks,
> Carolina