| 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
| ||
|
Message-ID: <e85af6d4-45f9-4011-a6e4-f06aa6662dad@linux.dev>
Date: Tue, 5 Aug 2025 10:28:50 +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 05/08/2025 10:03, Carolina Jubran wrote:
>
>
> On 04/08/2025 11:31, Vadim Fedorenko wrote:
>> 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?
>>
>
> That’s true, we have known FEC modes, but we don’t always know the
> actual bin layout, it can vary per device. So the driver still needs to
> query the device to get the correct ranges, even if the FEC type is fixed.
Correct me if I'm wrong, but I believe it's not per device but rather
per device generation. Cutting out old devices which don't support
reporting FEC histogram at all, and CX7 and CX8 supporting bins layout
as per standard, I would say there are 4 different variants, plus
RS(272, 257) for Infiniband. Not much to make it constant and avoid any
allocations and memory management, and keep maintenance easy for both
parties...
On the other side, if you want to dynamically fill in this data, it will
take proper amount of requests over PCIe: 1 (the amount of bins) + 2 *
16 (low and high boundary per bin for RS(544,514)) + 2 * 16 (64 bits
value per bin) = 65. In case of continues monitoring may be potentially
disruptive for high speed low latency traffic, but I believe we can
avoid at least half of these transactions.
>>>
>>> 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.
>>
> I see your point, yeah I’m good with the WARN_ON_ONCE()
>
>>>
>>>
>>> Thanks,
>>> Carolina
>>
>
Powered by blists - more mailing lists