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: <d3bb8295-bb4f-4817-a2dd-017332c489d4@nvidia.com>
Date: Sun, 3 Aug 2025 14:24:30 +0300
From: Carolina Jubran <cjubran@...dia.com>
To: Vadim Fedorenko <vadfed@...a.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, Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Subject: Re: [RFC PATCH v3] ethtool: add FEC bins histogramm report



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.

>   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.

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.

> +	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.


Thanks,
Carolina