[RFC PATCH] ethtool: add FEC bins histogramm report

[RFC PATCH] ethtool: add FEC bins histogramm report

[Vadim Fedorenko]

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@linux.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:

$ ./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
+  -
+    name: fec-hist
+    subset-of: fec-stat
+    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;
+
+		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;
 
-- 
2.47.3

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Andrew Lunn]

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

	Andrew

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Vadim Fedorenko]

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, so I believe it's ok to keep smaller
memory footprint 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.

Does it look OK?


> 
> 	Andrew

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Andrew Lunn]

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.

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

	Andrew

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Vadim Fedorenko]

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

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Andrew Lunn]

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

Can any of these NICs send runt packets? Can any send packets without
an ethernet header and FCS?

Seems to me, the bin (0,0) is meaningless, so can could be considered
the end marker. You then have unsigned everywhere, keeping it KISS.

	Andrew

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Vadim Fedorenko]

On 29/07/2025 18:31, Andrew Lunn wrote:
>> 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.
> 
> Can any of these NICs send runt packets? Can any send packets without
> an ethernet header and FCS?
> 
> Seems to me, the bin (0,0) is meaningless, so can could be considered
> the end marker. You then have unsigned everywhere, keeping it KISS.

I had to revisit the 802.3df-2024, and it looks like you are right:
"FEC_codeword_error_bin_i, where i=1 to 15, are optional 32-bit
counters. While align_status is true, for each codeword received with
exactly i correctable 10-bit symbols"

That means bin (0,0) doesn't exist according to standard, so we can use
it as a marker even though some vendors provide this bin as part of
histogram.

Thanks for the feedback!

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Jakub Kicinski]

On Tue, 29 Jul 2025 03:23:54 -0700 Vadim Fedorenko wrote:
> 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
> +  -
> +    name: fec-hist
> +    subset-of: fec-stat

no need to make this a subset, better to make it its own attr set

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

> +static const struct ethtool_fec_hist_range netdevsim_fec_ranges[] = {
> +	{  0,  0},
> +	{  1,  3},
> +	{  4,  7},
> +	{ -1, -1}
> +};

Let's add a define for the terminating element?

> +/**
> + * struct ethtool_fec_hist_range - byte range for FEC bins histogram statistics

byte range? thought these are bit errors..

> + * 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)
> + */

> +		len += nla_total_size_64bit(sizeof(u64) * ETHTOOL_FEC_HIST_MAX);

I don't think it's right, each attr is its own nla_total_size().
Add a nla_total_size(8) to the calculation below

> +		/* 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;

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Jakub Kicinski]

On Tue, 29 Jul 2025 03:23:54 -0700 Vadim Fedorenko wrote:
>  /**
>   * 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];
>  };

Some devices may want these per lane, let's make sure uAPI supports it.

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Jakub Kicinski]

On Tue, 29 Jul 2025 19:07:59 +0100 Vadim Fedorenko wrote:
> On 29/07/2025 18:31, Andrew Lunn wrote:
> >> 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.  
> > 
> > Can any of these NICs send runt packets? Can any send packets without
> > an ethernet header and FCS?
> > 
> > Seems to me, the bin (0,0) is meaningless, so can could be considered
> > the end marker. You then have unsigned everywhere, keeping it KISS.  
> 
> I had to revisit the 802.3df-2024, and it looks like you are right:
> "FEC_codeword_error_bin_i, where i=1 to 15, are optional 32-bit
> counters. While align_status is true, for each codeword received with
> exactly i correctable 10-bit symbols"
> 
> That means bin (0,0) doesn't exist according to standard, so we can use
> it as a marker even though some vendors provide this bin as part of
> histogram.

IDK, 0,0 means all symbols were completely correct.
It may be useful for calculating bit error rate?

A workaround for having the {-1, -1} sentinel could also be to skip 
the first entry:

	if (i && !ranges[i].low && !ranges[i].high)
		break;

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Gal Pressman]

On 30/07/2025 4:51, Jakub Kicinski wrote:
> On Tue, 29 Jul 2025 19:07:59 +0100 Vadim Fedorenko wrote:
>> On 29/07/2025 18:31, Andrew Lunn wrote:
>>>> 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.  
>>>
>>> Can any of these NICs send runt packets? Can any send packets without
>>> an ethernet header and FCS?
>>>
>>> Seems to me, the bin (0,0) is meaningless, so can could be considered
>>> the end marker. You then have unsigned everywhere, keeping it KISS.  
>>
>> I had to revisit the 802.3df-2024, and it looks like you are right:
>> "FEC_codeword_error_bin_i, where i=1 to 15, are optional 32-bit
>> counters. While align_status is true, for each codeword received with
>> exactly i correctable 10-bit symbols"
>>
>> That means bin (0,0) doesn't exist according to standard, so we can use
>> it as a marker even though some vendors provide this bin as part of
>> histogram.
> 
> IDK, 0,0 means all symbols were completely correct.
> It may be useful for calculating bit error rate?

Exactly. mlx5 will use (0, 0) for sure.

> 
> A workaround for having the {-1, -1} sentinel could also be to skip 
> the first entry:
> 
> 	if (i && !ranges[i].low && !ranges[i].high)
> 		break;

Yes, that is a valid approach.

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Gal Pressman]

On 29/07/2025 13:23, Vadim Fedorenko wrote:
> 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}
> +};

The driver-facing API works nicely when the ranges are allocated as
static arrays, but I expect most drivers will need to allocate it
dynamically as the ranges will be queried from the device.
In that case, we need to define who is responsible of freeing the ranges
array.

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

I suspect we might need to increase this value in the future, so I like
the fact that it's not hardcoded anywhere in the uapi.

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Vadim Fedorenko]

On 30/07/2025 02:51, Jakub Kicinski wrote:
> On Tue, 29 Jul 2025 19:07:59 +0100 Vadim Fedorenko wrote:
>> On 29/07/2025 18:31, Andrew Lunn wrote:
>>>> 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.
>>>
>>> Can any of these NICs send runt packets? Can any send packets without
>>> an ethernet header and FCS?
>>>
>>> Seems to me, the bin (0,0) is meaningless, so can could be considered
>>> the end marker. You then have unsigned everywhere, keeping it KISS.
>>
>> I had to revisit the 802.3df-2024, and it looks like you are right:
>> "FEC_codeword_error_bin_i, where i=1 to 15, are optional 32-bit
>> counters. While align_status is true, for each codeword received with
>> exactly i correctable 10-bit symbols"
>>
>> That means bin (0,0) doesn't exist according to standard, so we can use
>> it as a marker even though some vendors provide this bin as part of
>> histogram.
> 
> IDK, 0,0 means all symbols were completely correct.
> It may be useful for calculating bit error rate?

The standard doesn't have this bin, its value can be potentially
deducted from all packets counter.

> 
> A workaround for having the {-1, -1} sentinel could also be to skip
> the first entry:
> 
> 	if (i && !ranges[i].low && !ranges[i].high)
> 		break;

I was thinking of this way, the problem is that in the core we rely on
the driver to provide at least 2 bins and we cannot add any compile-time
checks because it's all dynamic.

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Vadim Fedorenko]

On 30/07/2025 02:45, Jakub Kicinski wrote:
> On Tue, 29 Jul 2025 03:23:54 -0700 Vadim Fedorenko wrote:
>> 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
>> +  -
>> +    name: fec-hist
>> +    subset-of: fec-stat
> 
> no need to make this a subset, better to make it its own attr set

like a set for general histogram? or still fec-specific?

> 
>> +    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
> 
>> +static const struct ethtool_fec_hist_range netdevsim_fec_ranges[] = {
>> +	{  0,  0},
>> +	{  1,  3},
>> +	{  4,  7},
>> +	{ -1, -1}
>> +};
> 
> Let's add a define for the terminating element?

I believe it's about (-1, -1) case. If we end up using (0, 0) then there
is no need to define anything, right?

> 
>> +/**
>> + * struct ethtool_fec_hist_range - byte range for FEC bins histogram statistics
> 
> byte range? thought these are bit errors..
> 
>> + * 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)
>> + */
> 
>> +		len += nla_total_size_64bit(sizeof(u64) * ETHTOOL_FEC_HIST_MAX);
> 
> I don't think it's right, each attr is its own nla_total_size().
> Add a nla_total_size(8) to the calculation below

got it

> 
>> +		/* 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;

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Vadim Fedorenko]

On 30/07/2025 06:54, Gal Pressman wrote:
> On 29/07/2025 13:23, Vadim Fedorenko wrote:
>> 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}
>> +};
> 
> The driver-facing API works nicely when the ranges are allocated as
> static arrays, but I expect most drivers will need to allocate it
> dynamically as the ranges will be queried from the device.
> In that case, we need to define who is responsible of freeing the ranges
> array.

Well, the ranges will not change during link operation, unless the type
of FEC is changed. You may either have static array of FEC ranges per
supported FEC types. Or query it on link-up event and reuse it on every
call for FEC stats. In this case it's pure driver's responsibility to
manage memory allocations. There is definitely no need to re-query
ranges on every single call for stats.

> 
>>   
>>   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
> 
> I suspect we might need to increase this value in the future, so I like
> the fact that it's not hardcoded anywhere in the uapi.

Yep, that's the plan

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Gal Pressman]

On 30/07/2025 12:29, Vadim Fedorenko wrote:
> On 30/07/2025 06:54, Gal Pressman wrote:
>> On 29/07/2025 13:23, Vadim Fedorenko wrote:
>>> 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}
>>> +};
>>
>> The driver-facing API works nicely when the ranges are allocated as
>> static arrays, but I expect most drivers will need to allocate it
>> dynamically as the ranges will be queried from the device.
>> In that case, we need to define who is responsible of freeing the ranges
>> array.
> 
> Well, the ranges will not change during link operation, unless the type
> of FEC is changed. You may either have static array of FEC ranges per
> supported FEC types. Or query it on link-up event and reuse it on every
> call for FEC stats. In this case it's pure driver's responsibility to
> manage memory allocations. There is definitely no need to re-query
> ranges on every single call for stats.

This is just adding unnecessary complexity to the drivers, trying to
figure out the right lifetime for this array.
It will be much simpler if the core passes an array for the drivers to
fill. That way both static and dynamic ranges would work the same.

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Vadim Fedorenko]

On 30/07/2025 11:42, Gal Pressman wrote:
> On 30/07/2025 12:29, Vadim Fedorenko wrote:
>> On 30/07/2025 06:54, Gal Pressman wrote:
>>> On 29/07/2025 13:23, Vadim Fedorenko wrote:
>>>> 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}
>>>> +};
>>>
>>> The driver-facing API works nicely when the ranges are allocated as
>>> static arrays, but I expect most drivers will need to allocate it
>>> dynamically as the ranges will be queried from the device.
>>> In that case, we need to define who is responsible of freeing the ranges
>>> array.
>>
>> Well, the ranges will not change during link operation, unless the type
>> of FEC is changed. You may either have static array of FEC ranges per
>> supported FEC types. Or query it on link-up event and reuse it on every
>> call for FEC stats. In this case it's pure driver's responsibility to
>> manage memory allocations. There is definitely no need to re-query
>> ranges on every single call for stats.
> 
> This is just adding unnecessary complexity to the drivers, trying to
> figure out the right lifetime for this array.
> It will be much simpler if the core passes an array for the drivers to
> fill. That way both static and dynamic ranges would work the same.

There is no need to pass huge pre-allocated array for drivers which
doesn't have support for the histogram. The core doesn't have this info
about the driver. And again - there is no need to refill ranges array
every time as it will not change. I believe it's pure driver area of
knowledge and responsibility.

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Carolina Jubran]

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

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Andrew Lunn]

On Wed, Jul 30, 2025 at 08:39:25AM +0300, Gal Pressman wrote:
> On 30/07/2025 4:51, Jakub Kicinski wrote:
> > On Tue, 29 Jul 2025 19:07:59 +0100 Vadim Fedorenko wrote:
> >> On 29/07/2025 18:31, Andrew Lunn wrote:
> >>>> 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.  
> >>>
> >>> Can any of these NICs send runt packets? Can any send packets without
> >>> an ethernet header and FCS?
> >>>
> >>> Seems to me, the bin (0,0) is meaningless, so can could be considered
> >>> the end marker. You then have unsigned everywhere, keeping it KISS.  
> >>
> >> I had to revisit the 802.3df-2024, and it looks like you are right:
> >> "FEC_codeword_error_bin_i, where i=1 to 15, are optional 32-bit
> >> counters. While align_status is true, for each codeword received with
> >> exactly i correctable 10-bit symbols"
> >>
> >> That means bin (0,0) doesn't exist according to standard, so we can use
> >> it as a marker even though some vendors provide this bin as part of
> >> histogram.
> > 
> > IDK, 0,0 means all symbols were completely correct.
> > It may be useful for calculating bit error rate?
> 
> Exactly. mlx5 will use (0, 0) for sure.

Sorry, i did not spend time to read the standard and issued this was
related to frame length somehow, like the RMON statistics which have
bins for packet length counts.

	Andrew

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Jakub Kicinski]

On Wed, 30 Jul 2025 10:18:46 +0100 Vadim Fedorenko wrote:
> > IDK, 0,0 means all symbols were completely correct.
> > It may be useful for calculating bit error rate?  
> 
> The standard doesn't have this bin, its value can be potentially
> deducted from all packets counter.

We have a number of counters outside of the standard. Here the
extension is pretty trivial, so I don't see why we'd deprive
the user of the information HW collects. The translation between
bytes and symbols is not exact. Not sure we care about exactness
but, again, trivial to keep the 0,0 bin.

> > A workaround for having the {-1, -1} sentinel could also be to skip
> > the first entry:
> > 
> > 	if (i && !ranges[i].low && !ranges[i].high)
> > 		break;  
> 
> I was thinking of this way, the problem is that in the core we rely on
> the driver to provide at least 2 bins and we cannot add any compile-time
> checks because it's all dynamic.

1 bin is no binning, its not a legit use of the histogram API. 
We have a counter for corrected symbols already, that's the "1 bin".

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Jakub Kicinski]

On Wed, 30 Jul 2025 10:22:36 +0100 Vadim Fedorenko wrote:
> >> +    name: fec-hist
> >> +    subset-of: fec-stat  
> > 
> > no need to make this a subset, better to make it its own attr set  
> 
> like a set for general histogram? or still fec-specific?

You can make it a general histogram, I guess 🤔️
No strong preference.

> >> +    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  
> >   
> >> +static const struct ethtool_fec_hist_range netdevsim_fec_ranges[] = {
> >> +	{  0,  0},
> >> +	{  1,  3},
> >> +	{  4,  7},
> >> +	{ -1, -1}
> >> +};  
> > 
> > Let's add a define for the terminating element?  
> 
> I believe it's about (-1, -1) case. If we end up using (0, 0) then there
> is no need to define anything, right?

Yup, 0,0 is better written as {} so no need for a define.

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Gal Pressman]

On 30/07/2025 14:32, Vadim Fedorenko wrote:
> On 30/07/2025 11:42, Gal Pressman wrote:
>> On 30/07/2025 12:29, Vadim Fedorenko wrote:
>>> On 30/07/2025 06:54, Gal Pressman wrote:
>>>> On 29/07/2025 13:23, Vadim Fedorenko wrote:
>>>>> 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}
>>>>> +};
>>>>
>>>> The driver-facing API works nicely when the ranges are allocated as
>>>> static arrays, but I expect most drivers will need to allocate it
>>>> dynamically as the ranges will be queried from the device.
>>>> In that case, we need to define who is responsible of freeing the
>>>> ranges
>>>> array.
>>>
>>> Well, the ranges will not change during link operation, unless the type
>>> of FEC is changed. You may either have static array of FEC ranges per
>>> supported FEC types. Or query it on link-up event and reuse it on every
>>> call for FEC stats. In this case it's pure driver's responsibility to
>>> manage memory allocations. There is definitely no need to re-query
>>> ranges on every single call for stats.
>>
>> This is just adding unnecessary complexity to the drivers, trying to
>> figure out the right lifetime for this array.
>> It will be much simpler if the core passes an array for the drivers to
>> fill. That way both static and dynamic ranges would work the same.
> 
> There is no need to pass huge pre-allocated array for drivers which
> doesn't have support for the histogram. The core doesn't have this info
> about the driver. And again - there is no need to refill ranges array
> every time as it will not change. I believe it's pure driver area of
> knowledge and responsibility.

It's not huge at all, it's an 18 elements array, and I expect most
drivers will need the dynamic ranges.

This is a control path operation, the "price" of allocating and filling
is small, and will reduce the number of driver bugs which get the array
lifetime wrong.

Or just let the driver fill the netlink attributes directly? Not sure if
we have precedent for that.

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Gal Pressman]

On 30/07/2025 15:59, Andrew Lunn wrote:
> On Wed, Jul 30, 2025 at 08:39:25AM +0300, Gal Pressman wrote:
>> On 30/07/2025 4:51, Jakub Kicinski wrote:
>>> On Tue, 29 Jul 2025 19:07:59 +0100 Vadim Fedorenko wrote:
>>>> On 29/07/2025 18:31, Andrew Lunn wrote:
>>>>>> 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.  
>>>>>
>>>>> Can any of these NICs send runt packets? Can any send packets without
>>>>> an ethernet header and FCS?
>>>>>
>>>>> Seems to me, the bin (0,0) is meaningless, so can could be considered
>>>>> the end marker. You then have unsigned everywhere, keeping it KISS.  
>>>>
>>>> I had to revisit the 802.3df-2024, and it looks like you are right:
>>>> "FEC_codeword_error_bin_i, where i=1 to 15, are optional 32-bit
>>>> counters. While align_status is true, for each codeword received with
>>>> exactly i correctable 10-bit symbols"
>>>>
>>>> That means bin (0,0) doesn't exist according to standard, so we can use
>>>> it as a marker even though some vendors provide this bin as part of
>>>> histogram.
>>>
>>> IDK, 0,0 means all symbols were completely correct.
>>> It may be useful for calculating bit error rate?
>>
>> Exactly. mlx5 will use (0, 0) for sure.
> 
> Sorry, i did not spend time to read the standard and issued this was
> related to frame length somehow, like the RMON statistics which have
> bins for packet length counts.

I'm not sure I'm using the right terminology, but the ranges are # of
symbols that had FEC errors. So (0, 0) means zero symbol errors.

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Andrew Lunn]

> Or just let the driver fill the netlink attributes directly? Not sure if
> we have precedent for that.

A variant on that exists, for cable testing. For BaseT, you can have
1, 2 or 4 pairs. Hence you need 1, 2 or 4 test results. There are
helpers which a driver can use to add test results. You can call them
as many times as you want.

The thing about cable testing is that it is not standardised in any
way, so vendors get to use there imagination. However, there are only
a limited number of ways a cable can be broken, and you can measure
the distance to the break, but not too much more. So i provided a set
of helpers to add well defined nested attributes, and it is up to user
space to handle whatever it receives, which might or might not include
all pairs, might have the length attribute or not, etc.

However, it seems like this is well defined in the standard, so i
don't think you need such flexibility. A single helper is all that
should be needed to add a bin.

So it does exist, but this does go against the general pattern.

	Andrew

Re: [RFC PATCH] ethtool: add FEC bins histogramm report

[Vadim Fedorenko]

On 30/07/2025 14:44, Jakub Kicinski wrote:
> On Wed, 30 Jul 2025 10:18:46 +0100 Vadim Fedorenko wrote:
>>> IDK, 0,0 means all symbols were completely correct.
>>> It may be useful for calculating bit error rate?
>>
>> The standard doesn't have this bin, its value can be potentially
>> deducted from all packets counter.
> 
> We have a number of counters outside of the standard. Here the
> extension is pretty trivial, so I don't see why we'd deprive
> the user of the information HW collects. The translation between
> bytes and symbols is not exact. Not sure we care about exactness
> but, again, trivial to keep the 0,0 bin.
> 
>>> A workaround for having the {-1, -1} sentinel could also be to skip
>>> the first entry:
>>>
>>> 	if (i && !ranges[i].low && !ranges[i].high)
>>> 		break;
>>
>> I was thinking of this way, the problem is that in the core we rely on
>> the driver to provide at least 2 bins and we cannot add any compile-time
>> checks because it's all dynamic.
> 
> 1 bin is no binning, its not a legit use of the histogram API.
> We have a counter for corrected symbols already, that's the "1 bin".

Got it. Ok, I agree, we can keep bin (0,0) as the very first one, I'll
implement it in the way you suggested above