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: <8e1d127e-0723-4b90-984a-59fb8de1e43c@nvidia.com>
Date: Wed, 30 Jul 2025 15:08:15 +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] ethtool: add FEC bins histogramm report



On 29/07/2025 13:23, Vadim Fedorenko wrote:
> IEEE 802.3ck-2022 defines counters for FEC bins and 802.3df-2024
> clarifies it a bit further. Implement reporting interface through as
> addition to FEC stats available in ethtool.
> 
> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
> ---
> 
> With this RFC I would like to get early feedback from the community
> about implementing FEC histogram statistics while we are waiting for
> some vendors to actually implement it in their drivers. I implemented
> the simplest solution in netdevsim driver to show how API looks like.
> The example query is the same as usual FEC statistics while the answer
> is a bit more verbose:
> 

Hi Vadim. thanks for the RFC.> $ ./tools/net/ynl/pyynl/cli.py --spec \
>   Documentation/netlink/specs/ethtool.yaml --do fec-get \
>   --json '{"header":{"dev-index": 10, "flags": 4}}'
> 
> {'auto': 0,
>   'header': {'dev-index': 10, 'dev-name': 'eni10np1'},
>   'modes': {'bits': {}, 'nomask': True, 'size': 121},
>   'stats': {'corr-bits': [],
>             'corrected': [123],
>             'hist': [{'fec-hist-bin-high': 0,
>                       'fec-hist-bin-low': 0,
>                       'fec-hist-bin-val': 345},
>                      {'fec-hist-bin-high': 3,
>                       'fec-hist-bin-low': 1,
>                       'fec-hist-bin-val': 12},
>                      {'fec-hist-bin-high': 7,
>                       'fec-hist-bin-low': 4,
>                       'fec-hist-bin-val': 2}],
>             'uncorr': [4]}}> ----
>   Documentation/netlink/specs/ethtool.yaml      | 24 +++++++++
>   Documentation/networking/ethtool-netlink.rst  |  5 ++
>   .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  3 +-
>   .../ethernet/fungible/funeth/funeth_ethtool.c |  3 +-
>   .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  3 +-
>   drivers/net/ethernet/intel/ice/ice_ethtool.c  |  3 +-
>   .../marvell/octeontx2/nic/otx2_ethtool.c      |  3 +-
>   .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  3 +-
>   drivers/net/ethernet/sfc/ethtool.c            |  3 +-
>   drivers/net/ethernet/sfc/siena/ethtool.c      |  3 +-
>   drivers/net/netdevsim/ethtool.c               | 15 +++++-
>   include/linux/ethtool.h                       | 15 +++++-
>   .../uapi/linux/ethtool_netlink_generated.h    |  4 ++
>   net/ethtool/fec.c                             | 53 ++++++++++++++++++-
>   14 files changed, 128 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index 1063d5d32fea2..3781ced722fee 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -1239,6 +1239,30 @@ attribute-sets:
>           name: corr-bits
>           type: binary
>           sub-type: u64
> +      -
> +        name: hist
> +        type: nest
> +        multi-attr: True
> +        nested-attributes: fec-hist
> +      -
> +        name: fec-hist-bin-low
> +        type: s32
> +      -
> +        name: fec-hist-bin-high
> +        type: s32
> +      -
> +        name: fec-hist-bin-val
> +        type: u64

These attributes are already defined inside the fec-hist nested 
attribute set. The top-level definitions can be removed.> +  -
> +    name: fec-hist
> +    subset-of: fec-stat
You can use here name-prefix as fec-hist-bin- > +    attributes:
> +      -
> +        name: fec-hist-bin-low
> +      -
> +        name: fec-hist-bin-high
> +      -
> +        name: fec-hist-bin-val
>     -
>       name: fec
>       attr-cnt-name: __ethtool-a-fec-cnt
> 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.
> +
>   FEC_SET
>   =======
>   
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 1b37612b1c01f..ff8c0977a1f4a 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -3185,7 +3185,8 @@ static int bnxt_get_fecparam(struct net_device *dev,
>   }
>   
>   static void bnxt_get_fec_stats(struct net_device *dev,
> -			       struct ethtool_fec_stats *fec_stats)
> +			       struct ethtool_fec_stats *fec_stats,
> +			       const struct ethtool_fec_hist_range **ranges)
>   {
>   	struct bnxt *bp = netdev_priv(dev);
>   	u64 *rx;
> diff --git a/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c b/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
> index ba83dbf4ed222..42027ce2b013d 100644
> --- a/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
> +++ b/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
> @@ -930,7 +930,8 @@ static void fun_get_rmon_stats(struct net_device *netdev,
>   }
>   
>   static void fun_get_fec_stats(struct net_device *netdev,
> -			      struct ethtool_fec_stats *stats)
> +			      struct ethtool_fec_stats *stats,
> +			      const struct ethtool_fec_hist_range **ranges)
>   {
>   	const struct funeth_priv *fp = netdev_priv(netdev);
>   
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index d5454e126c856..c5af42706c179 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> @@ -1659,7 +1659,8 @@ static void hns3_set_msglevel(struct net_device *netdev, u32 msg_level)
>   }
>   
>   static void hns3_get_fec_stats(struct net_device *netdev,
> -			       struct ethtool_fec_stats *fec_stats)
> +			       struct ethtool_fec_stats *fec_stats,
> +			       const struct ethtool_fec_hist_range **ranges)
>   {
>   	struct hnae3_handle *handle = hns3_get_handle(netdev);
>   	struct hnae3_ae_dev *ae_dev = hns3_get_ae_dev(handle);
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index 55e0f2c6af9e5..321704c53a0c2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -4623,7 +4623,8 @@ static int ice_get_port_fec_stats(struct ice_hw *hw, u16 pcs_quad, u16 pcs_port,
>    *
>    */
>   static void ice_get_fec_stats(struct net_device *netdev,
> -			      struct ethtool_fec_stats *fec_stats)
> +			      struct ethtool_fec_stats *fec_stats,
> +			      const struct ethtool_fec_hist_range **ranges)
>   {
>   	struct ice_netdev_priv *np = netdev_priv(netdev);
>   	struct ice_port_topology port_topology;
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> index 998c734ff8399..7c6643aa24bfa 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> @@ -1283,7 +1283,8 @@ static int otx2_set_link_ksettings(struct net_device *netdev,
>   }
>   
>   static void otx2_get_fec_stats(struct net_device *netdev,
> -			       struct ethtool_fec_stats *fec_stats)
> +			       struct ethtool_fec_stats *fec_stats,
> +			       const struct ethtool_fec_hist_range **ranges)
>   {
>   	struct otx2_nic *pfvf = netdev_priv(netdev);
>   	struct cgx_fw_data *rsp;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index d507366d773e1..9ad43b40d4ca5 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -1927,7 +1927,8 @@ static int mlx5e_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
>   }
>   
>   static void mlx5e_get_fec_stats(struct net_device *netdev,
> -				struct ethtool_fec_stats *fec_stats)
> +				struct ethtool_fec_stats *fec_stats,
> +				const struct ethtool_fec_hist_range **ranges)
>   {
>   	struct mlx5e_priv *priv = netdev_priv(netdev);
>   
> diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
> index 23c6a7df78d03..20de19d6a4d70 100644
> --- a/drivers/net/ethernet/sfc/ethtool.c
> +++ b/drivers/net/ethernet/sfc/ethtool.c
> @@ -217,7 +217,8 @@ static int efx_ethtool_set_wol(struct net_device *net_dev,
>   }
>   
>   static void efx_ethtool_get_fec_stats(struct net_device *net_dev,
> -				      struct ethtool_fec_stats *fec_stats)
> +				      struct ethtool_fec_stats *fec_stats,
> +				      const struct ethtool_fec_hist_range **ranges)
>   {
>   	struct efx_nic *efx = efx_netdev_priv(net_dev);
>   
> diff --git a/drivers/net/ethernet/sfc/siena/ethtool.c b/drivers/net/ethernet/sfc/siena/ethtool.c
> index 994909789bfea..b98271c546738 100644
> --- a/drivers/net/ethernet/sfc/siena/ethtool.c
> +++ b/drivers/net/ethernet/sfc/siena/ethtool.c
> @@ -217,7 +217,8 @@ static int efx_ethtool_set_wol(struct net_device *net_dev,
>   }
>   
>   static void efx_ethtool_get_fec_stats(struct net_device *net_dev,
> -				      struct ethtool_fec_stats *fec_stats)
> +				      struct ethtool_fec_stats *fec_stats,
> +				      const struct ethtool_fec_hist_range **ranges)
>   {
>   	struct efx_nic *efx = netdev_priv(net_dev);
>   
> diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
> index f631d90c428ac..7257de9ea2f44 100644
> --- a/drivers/net/netdevsim/ethtool.c
> +++ b/drivers/net/netdevsim/ethtool.c
> @@ -164,12 +164,25 @@ 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},
> +	{ -1, -1}
> +};
>   
>   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] = 345;
> +	fec_stats->hist[1] = 12;
> +	fec_stats->hist[2] = 2;
>   }
>   
>   static int nsim_get_ts_info(struct net_device *dev,
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index de5bd76a400ca..9421a5e31af21 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -492,6 +492,17 @@ struct ethtool_pause_stats {
>   };
>   
>   #define ETHTOOL_MAX_LANES	8
> +#define ETHTOOL_FEC_HIST_MAX	18
> +/**
> + * struct ethtool_fec_hist_range - byte range for FEC bins histogram statistics
> + * sentinel value of { -1, -1 } is used as marker for end of bins array
> + * @low: low bound of the bin (inclusive)
> + * @high: high bound of the bin (inclusive)
> + */
> +struct ethtool_fec_hist_range {
> +	s16 low;
> +	s16 high;
> +};
>   
>   /**
>    * struct ethtool_fec_stats - statistics for IEEE 802.3 FEC
> @@ -524,6 +535,7 @@ struct ethtool_fec_stats {
>   		u64 total;
>   		u64 lanes[ETHTOOL_MAX_LANES];
>   	} corrected_blocks, uncorrectable_blocks, corrected_bits;
> +	u64 hist[ETHTOOL_FEC_HIST_MAX];
>   };
>   
>   /**
> @@ -1212,7 +1224,8 @@ struct ethtool_ops {
>   	int	(*set_link_ksettings)(struct net_device *,
>   				      const struct ethtool_link_ksettings *);
>   	void	(*get_fec_stats)(struct net_device *dev,
> -				 struct ethtool_fec_stats *fec_stats);
> +				 struct ethtool_fec_stats *fec_stats,
> +				 const struct ethtool_fec_hist_range **ranges);
>   	int	(*get_fecparam)(struct net_device *,
>   				      struct ethtool_fecparam *);
>   	int	(*set_fecparam)(struct net_device *,
> diff --git a/include/uapi/linux/ethtool_netlink_generated.h b/include/uapi/linux/ethtool_netlink_generated.h
> index e3b8813465d73..f9babbd2e76f9 100644
> --- a/include/uapi/linux/ethtool_netlink_generated.h
> +++ b/include/uapi/linux/ethtool_netlink_generated.h
> @@ -567,6 +567,10 @@ enum {
>   	ETHTOOL_A_FEC_STAT_CORRECTED,
>   	ETHTOOL_A_FEC_STAT_UNCORR,
>   	ETHTOOL_A_FEC_STAT_CORR_BITS,
> +	ETHTOOL_A_FEC_STAT_HIST,
> +	ETHTOOL_A_FEC_STAT_FEC_HIST_BIN_LOW,
> +	ETHTOOL_A_FEC_STAT_FEC_HIST_BIN_HIGH,
> +	ETHTOOL_A_FEC_STAT_FEC_HIST_BIN_VAL,
>   
>   	__ETHTOOL_A_FEC_STAT_CNT,
>   	ETHTOOL_A_FEC_STAT_MAX = (__ETHTOOL_A_FEC_STAT_CNT - 1)
> diff --git a/net/ethtool/fec.c b/net/ethtool/fec.c
> index e7d3f2c352a34..b20a1e40dc45e 100644
> --- a/net/ethtool/fec.c
> +++ b/net/ethtool/fec.c
> @@ -17,6 +17,8 @@ struct fec_reply_data {
>   		u64 stats[1 + ETHTOOL_MAX_LANES];
>   		u8 cnt;
>   	} corr, uncorr, corr_bits;
> +	u64 hist[ETHTOOL_FEC_HIST_MAX];
> +	const struct ethtool_fec_hist_range *fec_ranges;
>   };
>   
>   #define FEC_REPDATA(__reply_base) \
> @@ -113,11 +115,13 @@ static int fec_prepare_data(const struct ethnl_req_info *req_base,
>   		struct ethtool_fec_stats stats;
>   
>   		ethtool_stats_init((u64 *)&stats, sizeof(stats) / 8);
> -		dev->ethtool_ops->get_fec_stats(dev, &stats);
> +		dev->ethtool_ops->get_fec_stats(dev, &stats, &data->fec_ranges);
>   
>   		fec_stats_recalc(&data->corr, &stats.corrected_blocks);
>   		fec_stats_recalc(&data->uncorr, &stats.uncorrectable_blocks);
>   		fec_stats_recalc(&data->corr_bits, &stats.corrected_bits);
> +		if (data->fec_ranges)
> +			memcpy(data->hist, stats.hist, sizeof(stats.hist));
>   	}
>   
>   	WARN_ON_ONCE(fec.reserved);
> @@ -157,13 +161,55 @@ static int fec_reply_size(const struct ethnl_req_info *req_base,
>   	len += nla_total_size(sizeof(u8)) +	/* _FEC_AUTO */
>   	       nla_total_size(sizeof(u32));	/* _FEC_ACTIVE */
>   
> -	if (req_base->flags & ETHTOOL_FLAG_STATS)
> +	if (req_base->flags & ETHTOOL_FLAG_STATS) {
>   		len += 3 * nla_total_size_64bit(sizeof(u64) *
>   						(1 + ETHTOOL_MAX_LANES));
> +		len += nla_total_size_64bit(sizeof(u64) * ETHTOOL_FEC_HIST_MAX);
> +		/* add FEC bins information */
> +		len += (nla_total_size(0) +  /* _A_FEC_HIST */
> +			nla_total_size(4) +  /* _A_FEC_HIST_BIN_LOW */
> +			nla_total_size(4)) * /* _A_FEC_HIST_BIN_HI */
> +			ETHTOOL_FEC_HIST_MAX;
> +	}
>   
>   	return len;
>   }
>   
> +static int fec_put_hist(struct sk_buff *skb, const u64 *hist,
> +			const struct ethtool_fec_hist_range *ranges)
> +{
> +	struct nlattr *nest;
> +	int i;
> +
> +	if (!ranges)
> +		return 0;
> +
> +	for (i = 0; i < ETHTOOL_FEC_HIST_MAX; i++) {
> +		if (ranges[i].low == -1 && ranges[i].high == -1)
> +			break;
> +
To avoid relying on { -1, -1 } as an end marker, first check
if (i && !ranges[i].low && !ranges[i].high) break;
to treat a second {0, 0} as the end of the histogram, while still 
allowing {0, 0} to be used as a valid first bin. Then check
if (hist[i] == ETHTOOL_STAT_NOT_SET) continue;
to skip over any unused or uninitialized bins. This should work safely 
even if the driver provides fewer bins than expected.> +		nest = 
nla_nest_start(skb, ETHTOOL_A_FEC_STAT_HIST);
> +		if (!nest)
> +			return -EMSGSIZE;
> +
> +		if (nla_put_u32(skb, ETHTOOL_A_FEC_STAT_FEC_HIST_BIN_LOW,
> +				ranges[i].low) ||
> +		    nla_put_u32(skb, ETHTOOL_A_FEC_STAT_FEC_HIST_BIN_HIGH,
> +				ranges[i].high) ||
> +		    nla_put_u64_64bit(skb, ETHTOOL_A_FEC_STAT_FEC_HIST_BIN_VAL,
> +				      hist[i], ETHTOOL_A_FEC_STAT_PAD))
> +			goto err_cancel_hist;
> +
> +		nla_nest_end(skb, nest);
> +	}
> +
> +	return 0;
> +
> +err_cancel_hist:
> +	nla_nest_cancel(skb, nest);
> +	return -EMSGSIZE;
> +}
> +
>   static int fec_put_stats(struct sk_buff *skb, const struct fec_reply_data *data)
>   {
>   	struct nlattr *nest;
> @@ -183,6 +229,9 @@ static int fec_put_stats(struct sk_buff *skb, const struct fec_reply_data *data)
>   			  data->corr_bits.stats, ETHTOOL_A_FEC_STAT_PAD))
>   		goto err_cancel;
>   
> +	if (fec_put_hist(skb, data->hist, data->fec_ranges))
> +		goto err_cancel;
> +
>   	nla_nest_end(skb, nest);
>   	return 0;
>   
Thanks,
Carolina


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ