[PATCH net-next v3 0/4] add FEC bins histogram report via ethtool

[PATCH net-next v3 0/4] add FEC bins histogram report via ethtool

[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. NetDevSim driver has simple
implementation as an example while mlx5 has much more complex solution.

The example query is the same as usual FEC statistics while the answer
is a bit more verbose:

[vmuser@archvm9 linux]$ ./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': [{'bin-high': 0,
                     'bin-low': 0,
                     'bin-val': 445,
                     'bin-val-per-lane': [125, 120, 100, 100]},
                    {'bin-high': 3, 'bin-low': 1, 'bin-val': 12},
                    {'bin-high': 7, 'bin-low': 4, 'bin-val': 2}],
           'uncorr': [4]}}

v2 -> v3:
* fix yaml spec to use binary array for histogram per-lane values
* fix spelling
v1 -> v2:
* fix memset size of FEC histogram bins in mlx5
* adjust fbnic driver FEC stats callback

Links to RFC discussions:
v1 - https://lore.kernel.org/netdev/20250729102354.771859-1-vadfed@meta.com/
v2 - https://lore.kernel.org/netdev/20250731231019.1809172-1-vadfed@meta.com/
v3 - https://lore.kernel.org/netdev/20250802063024.2423022-1-vadfed@meta.com/
v4 - https://lore.kernel.org/netdev/20250807155924.2272507-1-vadfed@meta.com/
v5 - https://lore.kernel.org/netdev/20250815132729.2251597-1-vadfed@meta.com/

Carolina Jubran (3):
  net/mlx5e: Don't query FEC statistics when FEC is disabled
  net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR
  net/mlx5e: Report RS-FEC histogram statistics via ethtool

Vadim Fedorenko (1):
  ethtool: add FEC bins histogram report

 Documentation/netlink/specs/ethtool.yaml      |  22 ++++
 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  |   4 +-
 .../marvell/octeontx2/nic/otx2_ethtool.c      |   3 +-
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |   1 +
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |   5 +-
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  10 ++
 .../ethernet/mellanox/mlx5/core/en_stats.c    | 102 ++++++++++++++++--
 .../ethernet/mellanox/mlx5/core/en_stats.h    |   4 +-
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |   3 +-
 drivers/net/ethernet/sfc/ethtool.c            |   3 +-
 drivers/net/ethernet/sfc/siena/ethtool.c      |   3 +-
 drivers/net/netdevsim/ethtool.c               |  22 +++-
 include/linux/ethtool.h                       |  25 ++++-
 .../uapi/linux/ethtool_netlink_generated.h    |  11 ++
 net/ethtool/fec.c                             |  69 +++++++++++-
 19 files changed, 278 insertions(+), 23 deletions(-)

-- 
2.47.3

[PATCH net-next v3 1/4] ethtool: add FEC bins histogram 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>
---
 Documentation/netlink/specs/ethtool.yaml      | 22 ++++++
 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  |  4 +-
 .../marvell/octeontx2/nic/otx2_ethtool.c      |  3 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  3 +-
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  3 +-
 drivers/net/ethernet/sfc/ethtool.c            |  3 +-
 drivers/net/ethernet/sfc/siena/ethtool.c      |  3 +-
 drivers/net/netdevsim/ethtool.c               | 22 +++++-
 include/linux/ethtool.h                       | 25 ++++++-
 .../uapi/linux/ethtool_netlink_generated.h    | 11 +++
 net/ethtool/fec.c                             | 69 ++++++++++++++++++-
 15 files changed, 169 insertions(+), 13 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 7a7594713f1f..de5008266884 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1219,6 +1219,23 @@ attribute-sets:
         name: udp-ports
         type: nest
         nested-attributes: tunnel-udp
+  -
+    name: fec-hist
+    attr-cnt-name: __ethtool-a-fec-hist-cnt
+    attributes:
+      -
+        name: bin-low
+        type: u32
+      -
+        name: bin-high
+        type: u32
+      -
+        name: bin-val
+        type: uint
+      -
+        name: bin-val-per-lane
+        type: binary
+        sub-type: u64
   -
     name: fec-stat
     attr-cnt-name: __ethtool-a-fec-stat-cnt
@@ -1242,6 +1259,11 @@ attribute-sets:
         name: corr-bits
         type: binary
         sub-type: u64
+      -
+        name: hist
+        type: nest
+        multi-attr: True
+        nested-attributes: fec-hist
   -
     name: fec
     attr-cnt-name: __ethtool-a-fec-cnt
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index ab20c644af24..b270886c5f5d 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 2830a2b17a27..e3dab169c865 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -3208,7 +3208,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,
+			       struct ethtool_fec_hist *hist)
 {
 	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 ba83dbf4ed22..1966dba512f8 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,
+			      struct ethtool_fec_hist *hist)
 {
 	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 a752d0e3db3a..a5eefa28454c 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,
+			       struct ethtool_fec_hist *hist)
 {
 	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 55e0f2c6af9e..62d3cfca350c 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -4620,10 +4620,12 @@ static int ice_get_port_fec_stats(struct ice_hw *hw, u16 pcs_quad, u16 pcs_port,
  * ice_get_fec_stats - returns FEC correctable, uncorrectable stats per netdev
  * @netdev: network interface device structure
  * @fec_stats: buffer to hold FEC statistics for given port
+ * @hist: buffer to put FEC histogram statistics for given port
  *
  */
 static void ice_get_fec_stats(struct net_device *netdev,
-			      struct ethtool_fec_stats *fec_stats)
+			      struct ethtool_fec_stats *fec_stats,
+			      struct ethtool_fec_hist *hist)
 {
 	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 998c734ff839..b90e23dc49de 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,
+			       struct ethtool_fec_hist *hist)
 {
 	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 d507366d773e..bcc3bbb78cc9 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,
+				struct ethtool_fec_hist *hist)
 {
 	struct mlx5e_priv *priv = netdev_priv(netdev);
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index b4ff98ee2051..b6e5bdd509f1 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -1659,7 +1659,8 @@ fbnic_get_pause_stats(struct net_device *netdev,
 
 static void
 fbnic_get_fec_stats(struct net_device *netdev,
-		    struct ethtool_fec_stats *fec_stats)
+		    struct ethtool_fec_stats *fec_stats,
+		    struct ethtool_fec_hist *hist)
 {
 	struct fbnic_net *fbn = netdev_priv(netdev);
 	struct fbnic_phy_stats *phy_stats;
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 23c6a7df78d0..18fe5850a978 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,
+				      struct ethtool_fec_hist *hist)
 {
 	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 994909789bfe..8c3ebd0617fb 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,
+				      struct ethtool_fec_hist *hist)
 {
 	struct efx_nic *efx = netdev_priv(net_dev);
 
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index f631d90c428a..6ef163847d13 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -165,11 +165,31 @@ nsim_set_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
 	return 0;
 }
 
+static const struct ethtool_fec_hist_range netdevsim_fec_ranges[] = {
+	{ 0, 0},
+	{ 1, 3},
+	{ 4, 7},
+	{ 0, 0}
+};
+
 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,
+		   struct ethtool_fec_hist *hist)
 {
+	struct ethtool_fec_hist_value *values = hist->values;
+
+	hist->ranges = netdevsim_fec_ranges;
+
 	fec_stats->corrected_blocks.total = 123;
 	fec_stats->uncorrectable_blocks.total = 4;
+
+	values[0].bin_value = 445;
+	values[1].bin_value = 12;
+	values[2].bin_value = 2;
+	values[0].bin_value_per_lane[0] = 125;
+	values[0].bin_value_per_lane[1] = 120;
+	values[0].bin_value_per_lane[2] = 100;
+	values[0].bin_value_per_lane[3] = 100;
 }
 
 static int nsim_get_ts_info(struct net_device *dev,
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index d7d757e72554..9d357b68f55b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -492,7 +492,29 @@ struct ethtool_pause_stats {
 };
 
 #define ETHTOOL_MAX_LANES	8
+/**
+ * IEEE 802.3ck/df defines 16 bins for FEC histogram plus one more for
+ * the end-of-list marker, total 17 items
+ */
+#define ETHTOOL_FEC_HIST_MAX	17
+/**
+ * struct ethtool_fec_hist_range - error bits range for FEC bins histogram
+ * statistics
+ * @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_hist {
+	struct ethtool_fec_hist_value {
+		u64 bin_value;
+		u64 bin_value_per_lane[ETHTOOL_MAX_LANES];
+	} values[ETHTOOL_FEC_HIST_MAX];
+	const struct ethtool_fec_hist_range *ranges;
+};
 /**
  * struct ethtool_fec_stats - statistics for IEEE 802.3 FEC
  * @corrected_blocks: number of received blocks corrected by FEC
@@ -1212,7 +1234,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,
+				 struct ethtool_fec_hist *hist);
 	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 e3b8813465d7..fd70f15666c1 100644
--- a/include/uapi/linux/ethtool_netlink_generated.h
+++ b/include/uapi/linux/ethtool_netlink_generated.h
@@ -561,12 +561,23 @@ enum {
 	ETHTOOL_A_TUNNEL_INFO_MAX = (__ETHTOOL_A_TUNNEL_INFO_CNT - 1)
 };
 
+enum {
+	ETHTOOL_A_FEC_HIST_BIN_LOW = 1,
+	ETHTOOL_A_FEC_HIST_BIN_HIGH,
+	ETHTOOL_A_FEC_HIST_BIN_VAL,
+	ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE,
+
+	__ETHTOOL_A_FEC_HIST_CNT,
+	ETHTOOL_A_FEC_HIST_MAX = (__ETHTOOL_A_FEC_HIST_CNT - 1)
+};
+
 enum {
 	ETHTOOL_A_FEC_STAT_UNSPEC,
 	ETHTOOL_A_FEC_STAT_PAD,
 	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_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 e7d3f2c352a3..536b19f4f1cf 100644
--- a/net/ethtool/fec.c
+++ b/net/ethtool/fec.c
@@ -17,6 +17,7 @@ struct fec_reply_data {
 		u64 stats[1 + ETHTOOL_MAX_LANES];
 		u8 cnt;
 	} corr, uncorr, corr_bits;
+	struct ethtool_fec_hist fec_stat_hist;
 };
 
 #define FEC_REPDATA(__reply_base) \
@@ -113,7 +114,11 @@ 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);
+		ethtool_stats_init((u64 *)data->fec_stat_hist.values,
+				   ETHTOOL_FEC_HIST_MAX *
+				   sizeof(struct ethtool_fec_hist_value) / 8);
+		dev->ethtool_ops->get_fec_stats(dev, &stats,
+						&data->fec_stat_hist);
 
 		fec_stats_recalc(&data->corr, &stats.corrected_blocks);
 		fec_stats_recalc(&data->uncorr, &stats.uncorrectable_blocks);
@@ -157,13 +162,70 @@ 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));
+		/* 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 */
+			/* _A_FEC_HIST_BIN_VAL + per-lane values */
+			nla_total_size_64bit(sizeof(u64)) +
+			nla_total_size_64bit(sizeof(u64) * ETHTOOL_MAX_LANES)) *
+			ETHTOOL_FEC_HIST_MAX;
+	}
 
 	return len;
 }
 
+static int fec_put_hist(struct sk_buff *skb, const struct ethtool_fec_hist *hist)
+{
+	const struct ethtool_fec_hist_range *ranges = hist->ranges;
+	const struct ethtool_fec_hist_value *values = hist->values;
+	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;
+
+		if (WARN_ON_ONCE(values[i].bin_value == ETHTOOL_STAT_NOT_SET))
+			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,
+				 values[i].bin_value))
+			goto err_cancel_hist;
+		for (j = 0; j < ETHTOOL_MAX_LANES; j++) {
+			if (values[i].bin_value_per_lane[j] == ETHTOOL_STAT_NOT_SET)
+				break;
+		}
+		if (j && nla_put_64bit(skb, ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE,
+				       sizeof(u64) * j,
+				       values[i].bin_value_per_lane,
+				       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 +245,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->fec_stat_hist))
+		goto err_cancel;
+
 	nla_nest_end(skb, nest);
 	return 0;
 
-- 
2.47.3

[PATCH net-next v3 2/4] net/mlx5e: Don't query FEC statistics when FEC is disabled

[Vadim Fedorenko]

From: Carolina Jubran <cjubran@nvidia.com>

Update mlx5e_stats_fec_get() to check the active FEC mode and skip
statistics collection when FEC is disabled.

Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Yael Chemla <ychemla@nvidia.com>
Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index 87536f158d07..aae0022e8736 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -1446,16 +1446,13 @@ static void fec_set_rs_stats(struct ethtool_fec_stats *fec_stats, u32 *ppcnt)
 }
 
 static void fec_set_block_stats(struct mlx5e_priv *priv,
+				int mode,
 				struct ethtool_fec_stats *fec_stats)
 {
 	struct mlx5_core_dev *mdev = priv->mdev;
 	u32 out[MLX5_ST_SZ_DW(ppcnt_reg)] = {};
 	u32 in[MLX5_ST_SZ_DW(ppcnt_reg)] = {};
 	int sz = MLX5_ST_SZ_BYTES(ppcnt_reg);
-	int mode = fec_active_mode(mdev);
-
-	if (mode == MLX5E_FEC_NOFEC)
-		return;
 
 	MLX5_SET(ppcnt_reg, in, local_port, 1);
 	MLX5_SET(ppcnt_reg, in, grp, MLX5_PHYSICAL_LAYER_COUNTERS_GROUP);
@@ -1496,11 +1493,14 @@ static void fec_set_corrected_bits_total(struct mlx5e_priv *priv,
 void mlx5e_stats_fec_get(struct mlx5e_priv *priv,
 			 struct ethtool_fec_stats *fec_stats)
 {
-	if (!MLX5_CAP_PCAM_FEATURE(priv->mdev, ppcnt_statistical_group))
+	int mode = fec_active_mode(priv->mdev);
+
+	if (mode == MLX5E_FEC_NOFEC ||
+	    !MLX5_CAP_PCAM_FEATURE(priv->mdev, ppcnt_statistical_group))
 		return;
 
 	fec_set_corrected_bits_total(priv, fec_stats);
-	fec_set_block_stats(priv, fec_stats);
+	fec_set_block_stats(priv, mode, fec_stats);
 }
 
 #define PPORT_ETH_EXT_OFF(c) \
-- 
2.47.3

[PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Vadim Fedorenko]

From: Carolina Jubran <cjubran@nvidia.com>

Introduce support for querying the Ports Phy Histogram Configuration
Register (PPHCR) to retrieve RS-FEC histogram bin ranges. The ranges
are stored in a static array and will be used to map histogram counters
to error levels.

The actual RS-FEC histogram statistics are not yet reported in this
commit and will be handled in a downstream patch.

Co-developed-by: Yael Chemla <ychemla@nvidia.com>
Signed-off-by: Yael Chemla <ychemla@nvidia.com>
Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Yael Chemla <ychemla@nvidia.com>
Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
---
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |  1 +
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  2 +-
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 10 ++++
 .../ethernet/mellanox/mlx5/core/en_stats.c    | 59 ++++++++++++++++++-
 .../ethernet/mellanox/mlx5/core/en_stats.h    |  4 +-
 5 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index 0dd3bc0f4caa..8b2e81170e6b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -950,6 +950,7 @@ struct mlx5e_priv {
 	struct mlx5e_mqprio_rl    *mqprio_rl;
 	struct dentry             *dfs_root;
 	struct mlx5_devcom_comp_dev *devcom;
+	struct ethtool_fec_hist_range *fec_ranges;
 };
 
 struct mlx5e_dev {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index bcc3bbb78cc9..fd45384a855b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1932,7 +1932,7 @@ static void mlx5e_get_fec_stats(struct net_device *netdev,
 {
 	struct mlx5e_priv *priv = netdev_priv(netdev);
 
-	mlx5e_stats_fec_get(priv, fec_stats);
+	mlx5e_stats_fec_get(priv, fec_stats, hist);
 }
 
 static int mlx5e_get_fecparam(struct net_device *netdev,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 714cce595692..1e516b354485 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -6246,8 +6246,17 @@ int mlx5e_priv_init(struct mlx5e_priv *priv,
 	if (!priv->channel_stats)
 		goto err_free_tx_rates;
 
+	priv->fec_ranges = kcalloc_node(ETHTOOL_FEC_HIST_MAX,
+					sizeof(*priv->fec_ranges),
+					GFP_KERNEL,
+					node);
+	if (!priv->fec_ranges)
+		goto err_free_channel_stats;
+
 	return 0;
 
+err_free_channel_stats:
+	kfree(priv->channel_stats);
 err_free_tx_rates:
 	kfree(priv->tx_rates);
 err_free_txq2sq_stats:
@@ -6271,6 +6280,7 @@ void mlx5e_priv_cleanup(struct mlx5e_priv *priv)
 	if (!priv->mdev)
 		return;
 
+	kfree(priv->fec_ranges);
 	for (i = 0; i < priv->stats_nch; i++)
 		kvfree(priv->channel_stats[i]);
 	kfree(priv->channel_stats);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index aae0022e8736..476689cb0c1f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -1490,8 +1490,63 @@ static void fec_set_corrected_bits_total(struct mlx5e_priv *priv,
 				      phy_corrected_bits);
 }
 
+#define MLX5E_FEC_RS_HIST_MAX 16
+
+static u8
+fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv,
+			     const struct ethtool_fec_hist_range **ranges)
+{
+	struct mlx5_core_dev *mdev = priv->mdev;
+	u32 out[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
+	u32 in[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
+	int sz = MLX5_ST_SZ_BYTES(pphcr_reg);
+	u8 active_hist_type, num_of_bins;
+
+	memset(priv->fec_ranges, 0,
+	       ETHTOOL_FEC_HIST_MAX * sizeof(*priv->fec_ranges));
+	MLX5_SET(pphcr_reg, in, local_port, 1);
+	if (mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPHCR, 0, 0))
+		return 0;
+
+	active_hist_type = MLX5_GET(pphcr_reg, out, active_hist_type);
+	if (!active_hist_type)
+		return 0;
+
+	num_of_bins = MLX5_GET(pphcr_reg, out, num_of_bins);
+	if (WARN_ON_ONCE(num_of_bins > MLX5E_FEC_RS_HIST_MAX))
+		return 0;
+
+	for (u8 i = 0; i < num_of_bins; i++) {
+		void *bin_range = MLX5_ADDR_OF(pphcr_reg, out, bin_range[i]);
+
+		priv->fec_ranges[i].high = MLX5_GET(bin_range_layout, bin_range,
+						    high_val);
+		priv->fec_ranges[i].low = MLX5_GET(bin_range_layout, bin_range,
+						   low_val);
+	}
+	*ranges = priv->fec_ranges;
+
+	return num_of_bins;
+}
+
+static void fec_set_histograms_stats(struct mlx5e_priv *priv, int mode,
+				     struct ethtool_fec_hist *hist)
+{
+	switch (mode) {
+	case MLX5E_FEC_RS_528_514:
+	case MLX5E_FEC_RS_544_514:
+	case MLX5E_FEC_LLRS_272_257_1:
+	case MLX5E_FEC_RS_544_514_INTERLEAVED_QUAD:
+		fec_rs_histogram_fill_ranges(priv, &hist->ranges);
+		break;
+	default:
+		return;
+	}
+}
+
 void mlx5e_stats_fec_get(struct mlx5e_priv *priv,
-			 struct ethtool_fec_stats *fec_stats)
+			 struct ethtool_fec_stats *fec_stats,
+			 struct ethtool_fec_hist *hist)
 {
 	int mode = fec_active_mode(priv->mdev);
 
@@ -1501,6 +1556,7 @@ void mlx5e_stats_fec_get(struct mlx5e_priv *priv,
 
 	fec_set_corrected_bits_total(priv, fec_stats);
 	fec_set_block_stats(priv, mode, fec_stats);
+	fec_set_histograms_stats(priv, mode, hist);
 }
 
 #define PPORT_ETH_EXT_OFF(c) \
@@ -2619,3 +2675,4 @@ unsigned int mlx5e_nic_stats_grps_num(struct mlx5e_priv *priv)
 {
 	return ARRAY_SIZE(mlx5e_nic_stats_grps);
 }
+
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
index 72dbcc1928ef..6019f47308fc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
@@ -117,8 +117,8 @@ void mlx5e_stats_update_ndo_stats(struct mlx5e_priv *priv);
 void mlx5e_stats_pause_get(struct mlx5e_priv *priv,
 			   struct ethtool_pause_stats *pause_stats);
 void mlx5e_stats_fec_get(struct mlx5e_priv *priv,
-			 struct ethtool_fec_stats *fec_stats);
-
+			 struct ethtool_fec_stats *fec_stats,
+			 struct ethtool_fec_hist *hist);
 void mlx5e_stats_eth_phy_get(struct mlx5e_priv *priv,
 			     struct ethtool_eth_phy_stats *phy_stats);
 void mlx5e_stats_eth_mac_get(struct mlx5e_priv *priv,
-- 
2.47.3

[PATCH net-next v3 4/4] net/mlx5e: Report RS-FEC histogram statistics via ethtool

[Vadim Fedorenko]

From: Carolina Jubran <cjubran@nvidia.com>

Add support for reporting RS-FEC histogram counters by reading them
from the RS_FEC_HISTOGRAM_GROUP in the PPCNT register.

Co-developed-by: Yael Chemla <ychemla@nvidia.com>
Signed-off-by: Yael Chemla <ychemla@nvidia.com>
Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Yael Chemla <ychemla@nvidia.com>
Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
---
 .../ethernet/mellanox/mlx5/core/en_stats.c    | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index 476689cb0c1f..1da439dda323 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -1529,15 +1529,46 @@ fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv,
 	return num_of_bins;
 }
 
+static void fec_rs_histogram_fill_stats(struct mlx5e_priv *priv,
+					u8 num_of_bins,
+					struct ethtool_fec_hist *hist)
+{
+	struct mlx5_core_dev *mdev = priv->mdev;
+	u32 out[MLX5_ST_SZ_DW(ppcnt_reg)] = {0};
+	u32 in[MLX5_ST_SZ_DW(ppcnt_reg)] = {0};
+	int sz = MLX5_ST_SZ_BYTES(ppcnt_reg);
+	void *rs_histogram_cntrs;
+
+	MLX5_SET(ppcnt_reg, in, local_port, 1);
+	MLX5_SET(ppcnt_reg, in, grp, MLX5_RS_FEC_HISTOGRAM_GROUP);
+	if (mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPCNT, 0, 0))
+		return;
+
+	rs_histogram_cntrs = MLX5_ADDR_OF(ppcnt_reg, out,
+					  counter_set.rs_histogram_cntrs);
+	/* Guaranteed that num_of_bins is less than MLX5E_FEC_RS_HIST_MAX
+	 * by fec_rs_histogram_fill_ranges().
+	 */
+	for (int i = 0; i < num_of_bins; i++) {
+		hist->values[i].bin_value = MLX5_GET64(rs_histogram_cntrs,
+						       rs_histogram_cntrs,
+						       hist[i]);
+	}
+}
+
 static void fec_set_histograms_stats(struct mlx5e_priv *priv, int mode,
 				     struct ethtool_fec_hist *hist)
 {
+	u8 num_of_bins;
+
 	switch (mode) {
 	case MLX5E_FEC_RS_528_514:
 	case MLX5E_FEC_RS_544_514:
 	case MLX5E_FEC_LLRS_272_257_1:
 	case MLX5E_FEC_RS_544_514_INTERLEAVED_QUAD:
-		fec_rs_histogram_fill_ranges(priv, &hist->ranges);
+		num_of_bins = fec_rs_histogram_fill_ranges(priv, &hist->ranges);
+		if (num_of_bins)
+			fec_rs_histogram_fill_stats(priv, num_of_bins, hist);
 		break;
 	default:
 		return;
-- 
2.47.3

RE: [Intel-wired-lan] [PATCH net-next v3 4/4] net/mlx5e: Report RS-FEC histogram statistics via ethtool

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Vadim Fedorenko
> Sent: Tuesday, September 16, 2025 9:13 PM
> To: Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> Michael Chan <michael.chan@broadcom.com>; Pavan Chebbi
> <pavan.chebbi@broadcom.com>; Tariq Toukan <tariqt@nvidia.com>; Gal
> Pressman <gal@nvidia.com>; intel-wired-lan@lists.osuosl.org; Donald
> Hunter <donald.hunter@gmail.com>; Carolina Jubran
> <cjubran@nvidia.com>; Vadim Fedorenko <vadim.fedorenko@linux.dev>
> Cc: Paolo Abeni <pabeni@redhat.com>; Simon Horman <horms@kernel.org>;
> netdev@vger.kernel.org; Yael Chemla <ychemla@nvidia.com>; Dragos
> Tatulea <dtatulea@nvidia.com>
> Subject: [Intel-wired-lan] [PATCH net-next v3 4/4] net/mlx5e: Report
> RS-FEC histogram statistics via ethtool
> 
> From: Carolina Jubran <cjubran@nvidia.com>
> 
> Add support for reporting RS-FEC histogram counters by reading them
> from the RS_FEC_HISTOGRAM_GROUP in the PPCNT register.
> 
> Co-developed-by: Yael Chemla <ychemla@nvidia.com>
> Signed-off-by: Yael Chemla <ychemla@nvidia.com>
> Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
> Reviewed-by: Yael Chemla <ychemla@nvidia.com>
> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> ---
>  .../ethernet/mellanox/mlx5/core/en_stats.c    | 33
> ++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index 476689cb0c1f..1da439dda323 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -1529,15 +1529,46 @@ fec_rs_histogram_fill_ranges(struct mlx5e_priv
> *priv,
>  	return num_of_bins;
>  }
> 
> +static void fec_rs_histogram_fill_stats(struct mlx5e_priv *priv,
> +					u8 num_of_bins,
> +					struct ethtool_fec_hist *hist)
> +{
> +	struct mlx5_core_dev *mdev = priv->mdev;
> +	u32 out[MLX5_ST_SZ_DW(ppcnt_reg)] = {0};
> +	u32 in[MLX5_ST_SZ_DW(ppcnt_reg)] = {0};
> +	int sz = MLX5_ST_SZ_BYTES(ppcnt_reg);
> +	void *rs_histogram_cntrs;
> +
> +	MLX5_SET(ppcnt_reg, in, local_port, 1);
> +	MLX5_SET(ppcnt_reg, in, grp, MLX5_RS_FEC_HISTOGRAM_GROUP);
> +	if (mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPCNT,
> 0, 0))
> +		return;
> +
> +	rs_histogram_cntrs = MLX5_ADDR_OF(ppcnt_reg, out,
> +					  counter_set.rs_histogram_cntrs);
> +	/* Guaranteed that num_of_bins is less than
> MLX5E_FEC_RS_HIST_MAX
> +	 * by fec_rs_histogram_fill_ranges().
> +	 */
> +	for (int i = 0; i < num_of_bins; i++) {
> +		hist->values[i].bin_value =
> MLX5_GET64(rs_histogram_cntrs,
> +						       rs_histogram_cntrs,
> +						       hist[i]);
> +	}
> +}
> +
>  static void fec_set_histograms_stats(struct mlx5e_priv *priv, int
> mode,
>  				     struct ethtool_fec_hist *hist)  {
> +	u8 num_of_bins;
> +
>  	switch (mode) {
>  	case MLX5E_FEC_RS_528_514:
>  	case MLX5E_FEC_RS_544_514:
>  	case MLX5E_FEC_LLRS_272_257_1:
>  	case MLX5E_FEC_RS_544_514_INTERLEAVED_QUAD:
> -		fec_rs_histogram_fill_ranges(priv, &hist->ranges);
> +		num_of_bins = fec_rs_histogram_fill_ranges(priv, &hist-
> >ranges);
> +		if (num_of_bins)
> +			fec_rs_histogram_fill_stats(priv, num_of_bins,
> hist);
>  		break;
>  	default:
>  		return;
> --
> 2.47.3


Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

RE: [Intel-wired-lan] [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Vadim Fedorenko
> Sent: Tuesday, September 16, 2025 9:13 PM
> To: Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> Michael Chan <michael.chan@broadcom.com>; Pavan Chebbi
> <pavan.chebbi@broadcom.com>; Tariq Toukan <tariqt@nvidia.com>; Gal
> Pressman <gal@nvidia.com>; intel-wired-lan@lists.osuosl.org; Donald
> Hunter <donald.hunter@gmail.com>; Carolina Jubran
> <cjubran@nvidia.com>; Vadim Fedorenko <vadim.fedorenko@linux.dev>
> Cc: Paolo Abeni <pabeni@redhat.com>; Simon Horman <horms@kernel.org>;
> netdev@vger.kernel.org; Yael Chemla <ychemla@nvidia.com>; Dragos
> Tatulea <dtatulea@nvidia.com>
> Subject: [Intel-wired-lan] [PATCH net-next v3 3/4] net/mlx5e: Add
> logic to read RS-FEC histogram bin ranges from PPHCR
> 
> From: Carolina Jubran <cjubran@nvidia.com>
> 
> Introduce support for querying the Ports Phy Histogram Configuration
> Register (PPHCR) to retrieve RS-FEC histogram bin ranges. The ranges
> are stored in a static array and will be used to map histogram
> counters to error levels.
> 
> The actual RS-FEC histogram statistics are not yet reported in this
> commit and will be handled in a downstream patch.
> 
> Co-developed-by: Yael Chemla <ychemla@nvidia.com>
> Signed-off-by: Yael Chemla <ychemla@nvidia.com>
> Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
> Reviewed-by: Yael Chemla <ychemla@nvidia.com>
> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

> ---
...
> --
> 2.47.3

RE: [Intel-wired-lan] [PATCH net-next v3 2/4] net/mlx5e: Don't query FEC statistics when FEC is disabled

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Vadim Fedorenko
> Sent: Tuesday, September 16, 2025 9:13 PM
> To: Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> Michael Chan <michael.chan@broadcom.com>; Pavan Chebbi
> <pavan.chebbi@broadcom.com>; Tariq Toukan <tariqt@nvidia.com>; Gal
> Pressman <gal@nvidia.com>; intel-wired-lan@lists.osuosl.org; Donald
> Hunter <donald.hunter@gmail.com>; Carolina Jubran
> <cjubran@nvidia.com>; Vadim Fedorenko <vadim.fedorenko@linux.dev>
> Cc: Paolo Abeni <pabeni@redhat.com>; Simon Horman <horms@kernel.org>;
> netdev@vger.kernel.org; Dragos Tatulea <dtatulea@nvidia.com>; Yael
> Chemla <ychemla@nvidia.com>
> Subject: [Intel-wired-lan] [PATCH net-next v3 2/4] net/mlx5e: Don't
> query FEC statistics when FEC is disabled
> 
> From: Carolina Jubran <cjubran@nvidia.com>
> 
> Update mlx5e_stats_fec_get() to check the active FEC mode and skip
> statistics collection when FEC is disabled.
> 
> Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
> Reviewed-by: Yael Chemla <ychemla@nvidia.com>
> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index 87536f158d07..aae0022e8736 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -1446,16 +1446,13 @@ static void fec_set_rs_stats(struct
> ethtool_fec_stats *fec_stats, u32 *ppcnt)  }
> 
>  static void fec_set_block_stats(struct mlx5e_priv *priv,
> +				int mode,
>  				struct ethtool_fec_stats *fec_stats)  {
>  	struct mlx5_core_dev *mdev = priv->mdev;
>  	u32 out[MLX5_ST_SZ_DW(ppcnt_reg)] = {};
>  	u32 in[MLX5_ST_SZ_DW(ppcnt_reg)] = {};
>  	int sz = MLX5_ST_SZ_BYTES(ppcnt_reg);
> -	int mode = fec_active_mode(mdev);
> -
> -	if (mode == MLX5E_FEC_NOFEC)
> -		return;
> 
>  	MLX5_SET(ppcnt_reg, in, local_port, 1);
>  	MLX5_SET(ppcnt_reg, in, grp,
> MLX5_PHYSICAL_LAYER_COUNTERS_GROUP);
> @@ -1496,11 +1493,14 @@ static void
> fec_set_corrected_bits_total(struct mlx5e_priv *priv,  void
> mlx5e_stats_fec_get(struct mlx5e_priv *priv,
>  			 struct ethtool_fec_stats *fec_stats)  {
> -	if (!MLX5_CAP_PCAM_FEATURE(priv->mdev,
> ppcnt_statistical_group))
> +	int mode = fec_active_mode(priv->mdev);
> +
> +	if (mode == MLX5E_FEC_NOFEC ||
> +	    !MLX5_CAP_PCAM_FEATURE(priv->mdev,
> ppcnt_statistical_group))
>  		return;
> 
>  	fec_set_corrected_bits_total(priv, fec_stats);
> -	fec_set_block_stats(priv, fec_stats);
> +	fec_set_block_stats(priv, mode, fec_stats);
>  }
> 
>  #define PPORT_ETH_EXT_OFF(c) \
> --
> 2.47.3

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

RE: [Intel-wired-lan] [PATCH net-next v3 1/4] ethtool: add FEC bins histogram report

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Vadim Fedorenko
> Sent: Tuesday, September 16, 2025 9:13 PM
> To: Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> Michael Chan <michael.chan@broadcom.com>; Pavan Chebbi
> <pavan.chebbi@broadcom.com>; Tariq Toukan <tariqt@nvidia.com>; Gal
> Pressman <gal@nvidia.com>; intel-wired-lan@lists.osuosl.org; Donald
> Hunter <donald.hunter@gmail.com>; Carolina Jubran
> <cjubran@nvidia.com>; Vadim Fedorenko <vadim.fedorenko@linux.dev>
> Cc: Paolo Abeni <pabeni@redhat.com>; Simon Horman <horms@kernel.org>;
> netdev@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH net-next v3 1/4] ethtool: add FEC
> bins histogram report
> 
> 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>

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

> ---
... 
> --
> 2.47.3

Re: [PATCH net-next v3 1/4] ethtool: add FEC bins histogram report

[Jakub Kicinski]

On Tue, 16 Sep 2025 19:12:54 +0000 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.
> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index 7a7594713f1f..de5008266884 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -1219,6 +1219,23 @@ attribute-sets:
>          name: udp-ports
>          type: nest
>          nested-attributes: tunnel-udp
> +  -
> +    name: fec-hist
> +    attr-cnt-name: __ethtool-a-fec-hist-cnt

s/__/--/

> +    attributes:
> +      -
> +        name: bin-low
> +        type: u32
> +      -
> +        name: bin-high
> +        type: u32

We should add some doc: strings here so that the important info like
which one is inclusive is rendered right in the API reference

> +      -
> +        name: bin-val
> +        type: uint
> +      -
> +        name: bin-val-per-lane
> +        type: binary

probably good to doc this too

> +        sub-type: u64
>    -
>      name: fec-stat
>      attr-cnt-name: __ethtool-a-fec-stat-cnt
> @@ -1242,6 +1259,11 @@ attribute-sets:
>          name: corr-bits
>          type: binary
>          sub-type: u64
> +      -
> +        name: hist
> +        type: nest
> +        multi-attr: True
> +        nested-attributes: fec-hist
>    -
>      name: fec
>      attr-cnt-name: __ethtool-a-fec-cnt
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index ab20c644af24..b270886c5f5d 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.

Does this sound better?

  Optional ``ETHTOOL_A_FEC_STAT_HIST`` attributes form a FEC error
  histogram, as defined in IEEE 802.3ck-2022 and 802.3df-2024
  (histogram of number of errors within a single FEC block). 
  Each ``ETHTOOL_A_FEC_STAT_HIST`` entry contains error count
  (optionally also broken down by SerDes lane) as well as metadata
  about the bin. Bin range (low, high) is inclusive.

>  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,
> +		   struct ethtool_fec_hist *hist)
>  {
> +	struct ethtool_fec_hist_value *values = hist->values;
> +
> +	hist->ranges = netdevsim_fec_ranges;
> +
>  	fec_stats->corrected_blocks.total = 123;
>  	fec_stats->uncorrectable_blocks.total = 4;
> +
> +	values[0].bin_value = 445;

Bin 0 had per lane breakdown, can't core add up the lanes for the
driver?

> +	values[1].bin_value = 12;
> +	values[2].bin_value = 2;
> +	values[0].bin_value_per_lane[0] = 125;
> +	values[0].bin_value_per_lane[1] = 120;
> +	values[0].bin_value_per_lane[2] = 100;
> +	values[0].bin_value_per_lane[3] = 100;
>  }

> +/**
> + * struct ethtool_fec_hist_range - error bits range for FEC bins histogram

Don't say "FEC bin histogram" I think the word histogram implies that
the data is bin'ed up.

> + * statistics
> + * @low: low bound of the bin (inclusive)
> + * @high: high bound of the bin (inclusive)
> + */

> @@ -113,7 +114,11 @@ 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);
> +		ethtool_stats_init((u64 *)data->fec_stat_hist.values,
> +				   ETHTOOL_FEC_HIST_MAX *
> +				   sizeof(struct ethtool_fec_hist_value) / 8);

sizeof(data->fec_stat_hist.values) / 8

would save you the multiplication?

> +		dev->ethtool_ops->get_fec_stats(dev, &stats,
> +						&data->fec_stat_hist);
>  
>  		fec_stats_recalc(&data->corr, &stats.corrected_blocks);
>  		fec_stats_recalc(&data->uncorr, &stats.uncorrectable_blocks);

> +static int fec_put_hist(struct sk_buff *skb, const struct ethtool_fec_hist *hist)

over 80 chars, please wrap (checkpatch --max-line-length=80)

> +{
> +	const struct ethtool_fec_hist_range *ranges = hist->ranges;
> +	const struct ethtool_fec_hist_value *values = hist->values;
> +	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)

low and high should probably be unsigned now

> +			break;
> +
> +		if (WARN_ON_ONCE(values[i].bin_value == ETHTOOL_STAT_NOT_SET))
> +			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,
> +				 values[i].bin_value))
> +			goto err_cancel_hist;
> +		for (j = 0; j < ETHTOOL_MAX_LANES; j++) {
> +			if (values[i].bin_value_per_lane[j] == ETHTOOL_STAT_NOT_SET)

You know, bin_value could be 'sum', and bin_value_per_lane could be
simply 'per_lane'.

> +				break;
> +		}

{} brackets unnecessary

> +		if (j && nla_put_64bit(skb, ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE,
> +				       sizeof(u64) * j,
> +				       values[i].bin_value_per_lane,
> +				       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;
> +}

We need a selftest. Add a case to stats.py and do basic sanity checking
on what the kernel spits out? Maybe 2 test cases - one for overall and
one for per-lane, cause mlx5 doesn't have per lane and we'd like the
test to pass there.

Re: [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Jakub Kicinski]

On Tue, 16 Sep 2025 19:12:56 +0000 Vadim Fedorenko wrote:
> From: Carolina Jubran <cjubran@nvidia.com>
> 
> Introduce support for querying the Ports Phy Histogram Configuration
> Register (PPHCR) to retrieve RS-FEC histogram bin ranges. The ranges
> are stored in a static array and will be used to map histogram counters
> to error levels.
> 
> The actual RS-FEC histogram statistics are not yet reported in this
> commit and will be handled in a downstream patch.

> @@ -6246,8 +6246,17 @@ int mlx5e_priv_init(struct mlx5e_priv *priv,
>  	if (!priv->channel_stats)
>  		goto err_free_tx_rates;
>  
> +	priv->fec_ranges = kcalloc_node(ETHTOOL_FEC_HIST_MAX,
> +					sizeof(*priv->fec_ranges),
> +					GFP_KERNEL,
> +					node);

Why bother allocating his on the device node? We have no reason to
believe user will pin eth process that reads these stats to the node
where the device is :\

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index aae0022e8736..476689cb0c1f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -1490,8 +1490,63 @@ static void fec_set_corrected_bits_total(struct mlx5e_priv *priv,
>  				      phy_corrected_bits);
>  }
>  
> +#define MLX5E_FEC_RS_HIST_MAX 16
> +
> +static u8
> +fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv,
> +			     const struct ethtool_fec_hist_range **ranges)
> +{
> +	struct mlx5_core_dev *mdev = priv->mdev;
> +	u32 out[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
> +	u32 in[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
> +	int sz = MLX5_ST_SZ_BYTES(pphcr_reg);
> +	u8 active_hist_type, num_of_bins;
> +
> +	memset(priv->fec_ranges, 0,
> +	       ETHTOOL_FEC_HIST_MAX * sizeof(*priv->fec_ranges));
> +	MLX5_SET(pphcr_reg, in, local_port, 1);
> +	if (mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPHCR, 0, 0))
> +		return 0;
> +
> +	active_hist_type = MLX5_GET(pphcr_reg, out, active_hist_type);
> +	if (!active_hist_type)
> +		return 0;
> +
> +	num_of_bins = MLX5_GET(pphcr_reg, out, num_of_bins);
> +	if (WARN_ON_ONCE(num_of_bins > MLX5E_FEC_RS_HIST_MAX))

why does MLX5E_FEC_RS_HIST_MAX exist?
We care that bins_cnt <= ETHTOOL_FEC_HIST_MAX - 1
or is there something in the interface that hardcodes 16?

> +		return 0;

> @@ -2619,3 +2675,4 @@ unsigned int mlx5e_nic_stats_grps_num(struct mlx5e_priv *priv)
>  {
>  	return ARRAY_SIZE(mlx5e_nic_stats_grps);
>  }
> +

spurious change

Re: [PATCH net-next v3 4/4] net/mlx5e: Report RS-FEC histogram statistics via ethtool

[Jakub Kicinski]

On Tue, 16 Sep 2025 19:12:57 +0000 Vadim Fedorenko wrote:
> +	for (int i = 0; i < num_of_bins; i++) {

brackets unnecessary

in the other patch you picked u8 for i, good to be consistent
(int is better)

> +		hist->values[i].bin_value = MLX5_GET64(rs_histogram_cntrs,
> +						       rs_histogram_cntrs,
> +						       hist[i]);

could also be written as:

		hist->values[i].bin_value =
			MLX5_GET64(rs_histogram_cntrs, rs_histogram_cntrs, hist[i]);

Re: [PATCH net-next v3 1/4] ethtool: add FEC bins histogram report

[Vadim Fedorenko]

On 18/09/2025 01:41, Jakub Kicinski wrote:
> On Tue, 16 Sep 2025 19:12:54 +0000 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.
>> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
>> index 7a7594713f1f..de5008266884 100644
>> --- a/Documentation/netlink/specs/ethtool.yaml
>> +++ b/Documentation/netlink/specs/ethtool.yaml
>> @@ -1219,6 +1219,23 @@ attribute-sets:
>>           name: udp-ports
>>           type: nest
>>           nested-attributes: tunnel-udp
>> +  -
>> +    name: fec-hist
>> +    attr-cnt-name: __ethtool-a-fec-hist-cnt
> 
> s/__/--/

That will bring strong inconsistency in schema. All other attributes
have counter attribute with __ in the beginning:

     name: fec-stat
     attr-cnt-name: __ethtool-a-fec-stat-cnt

     name: stats-grp
     attr-cnt-name: __ethtool-a-stats-grp-cnt

     name: stats
     attr-cnt-name: __ethtool-a-stats-cnt

> 
>> +    attributes:
>> +      -
>> +        name: bin-low
>> +        type: u32
>> +      -
>> +        name: bin-high
>> +        type: u32
> 
> We should add some doc: strings here so that the important info like
> which one is inclusive is rendered right in the API reference

Yep, will add some doc

> 
>> +      -
>> +        name: bin-val
>> +        type: uint
>> +      -
>> +        name: bin-val-per-lane
>> +        type: binary
> 
> probably good to doc this too

ack

> 
>> +        sub-type: u64
>>     -
>>       name: fec-stat
>>       attr-cnt-name: __ethtool-a-fec-stat-cnt
>> @@ -1242,6 +1259,11 @@ attribute-sets:
>>           name: corr-bits
>>           type: binary
>>           sub-type: u64
>> +      -
>> +        name: hist
>> +        type: nest
>> +        multi-attr: True
>> +        nested-attributes: fec-hist
>>     -
>>       name: fec
>>       attr-cnt-name: __ethtool-a-fec-cnt
>> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
>> index ab20c644af24..b270886c5f5d 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.
> 
> Does this sound better?
> 
>    Optional ``ETHTOOL_A_FEC_STAT_HIST`` attributes form a FEC error
>    histogram, as defined in IEEE 802.3ck-2022 and 802.3df-2024
>    (histogram of number of errors within a single FEC block).
>    Each ``ETHTOOL_A_FEC_STAT_HIST`` entry contains error count
>    (optionally also broken down by SerDes lane) as well as metadata
>    about the bin. Bin range (low, high) is inclusive.

Ack

> 
>>   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,
>> +		   struct ethtool_fec_hist *hist)
>>   {
>> +	struct ethtool_fec_hist_value *values = hist->values;
>> +
>> +	hist->ranges = netdevsim_fec_ranges;
>> +
>>   	fec_stats->corrected_blocks.total = 123;
>>   	fec_stats->uncorrectable_blocks.total = 4;
>> +
>> +	values[0].bin_value = 445;
> 
> Bin 0 had per lane breakdown, can't core add up the lanes for the
> driver?

Like it's done for blocks counter? Should we force drivers to keep 'sum'
value equal to ETHTOOL_STAT_NOT_SET when they provide per-lane values?

> 
>> +	values[1].bin_value = 12;
>> +	values[2].bin_value = 2;
>> +	values[0].bin_value_per_lane[0] = 125;
>> +	values[0].bin_value_per_lane[1] = 120;
>> +	values[0].bin_value_per_lane[2] = 100;
>> +	values[0].bin_value_per_lane[3] = 100;
>>   }
> 
>> +/**
>> + * struct ethtool_fec_hist_range - error bits range for FEC bins histogram
> 
> Don't say "FEC bin histogram" I think the word histogram implies that
> the data is bin'ed up.

Ack

> 
>> + * statistics
>> + * @low: low bound of the bin (inclusive)
>> + * @high: high bound of the bin (inclusive)
>> + */
> 
>> @@ -113,7 +114,11 @@ 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);
>> +		ethtool_stats_init((u64 *)data->fec_stat_hist.values,
>> +				   ETHTOOL_FEC_HIST_MAX *
>> +				   sizeof(struct ethtool_fec_hist_value) / 8);
> 
> sizeof(data->fec_stat_hist.values) / 8
> 
> would save you the multiplication?

yeah...

> 
>> +		dev->ethtool_ops->get_fec_stats(dev, &stats,
>> +						&data->fec_stat_hist);
>>   
>>   		fec_stats_recalc(&data->corr, &stats.corrected_blocks);
>>   		fec_stats_recalc(&data->uncorr, &stats.uncorrectable_blocks);
> 
>> +static int fec_put_hist(struct sk_buff *skb, const struct ethtool_fec_hist *hist)
> 
> over 80 chars, please wrap (checkpatch --max-line-length=80)

ouch, that's proper strict check! :)

> 
>> +{
>> +	const struct ethtool_fec_hist_range *ranges = hist->ranges;
>> +	const struct ethtool_fec_hist_value *values = hist->values;
>> +	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)
> 
> low and high should probably be unsigned now

ack

> 
>> +			break;
>> +
>> +		if (WARN_ON_ONCE(values[i].bin_value == ETHTOOL_STAT_NOT_SET))
>> +			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,
>> +				 values[i].bin_value))
>> +			goto err_cancel_hist;
>> +		for (j = 0; j < ETHTOOL_MAX_LANES; j++) {
>> +			if (values[i].bin_value_per_lane[j] == ETHTOOL_STAT_NOT_SET)
> 
> You know, bin_value could be 'sum', and bin_value_per_lane could be
> simply 'per_lane'.

SG, I'll change it

> 
>> +				break;
>> +		}
> 
> {} brackets unnecessary
> 
>> +		if (j && nla_put_64bit(skb, ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE,
>> +				       sizeof(u64) * j,
>> +				       values[i].bin_value_per_lane,
>> +				       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;
>> +}
> 
> We need a selftest. Add a case to stats.py and do basic sanity checking
> on what the kernel spits out? Maybe 2 test cases - one for overall and
> one for per-lane, cause mlx5 doesn't have per lane and we'd like the
> test to pass there.
Yeah, sure, I'll add them as another patch to this series

Re: [Intel-wired-lan] [PATCH net-next v3 1/4] ethtool: add FEC bins histogram report

[Vadim Fedorenko]

On 17/09/2025 12:27, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
>> Of Vadim Fedorenko
>> Sent: Tuesday, September 16, 2025 9:13 PM
>> To: Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
>> Michael Chan <michael.chan@broadcom.com>; Pavan Chebbi
>> <pavan.chebbi@broadcom.com>; Tariq Toukan <tariqt@nvidia.com>; Gal
>> Pressman <gal@nvidia.com>; intel-wired-lan@lists.osuosl.org; Donald
>> Hunter <donald.hunter@gmail.com>; Carolina Jubran
>> <cjubran@nvidia.com>; Vadim Fedorenko <vadim.fedorenko@linux.dev>
>> Cc: Paolo Abeni <pabeni@redhat.com>; Simon Horman <horms@kernel.org>;
>> netdev@vger.kernel.org
>> Subject: [Intel-wired-lan] [PATCH net-next v3 1/4] ethtool: add FEC
>> bins histogram report
>>
>> 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>
> 
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> 

Thanks for the review!
BTW, do you know if Intel's E8xx series can provide such kind of statistics?

CC: Tony and Przemek as maintainers of ice driver

Re: [PATCH net-next v3 1/4] ethtool: add FEC bins histogram report

[Jakub Kicinski]

On Thu, 18 Sep 2025 11:53:53 +0100 Vadim Fedorenko wrote:
> On 18/09/2025 01:41, Jakub Kicinski wrote:
> > On Tue, 16 Sep 2025 19:12:54 +0000 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.
> >> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> >> index 7a7594713f1f..de5008266884 100644
> >> --- a/Documentation/netlink/specs/ethtool.yaml
> >> +++ b/Documentation/netlink/specs/ethtool.yaml
> >> @@ -1219,6 +1219,23 @@ attribute-sets:
> >>           name: udp-ports
> >>           type: nest
> >>           nested-attributes: tunnel-udp
> >> +  -
> >> +    name: fec-hist
> >> +    attr-cnt-name: __ethtool-a-fec-hist-cnt  
> > 
> > s/__/--/  
> 
> That will bring strong inconsistency in schema. All other attributes
> have counter attribute with __ in the beginning:
> 
>      name: fec-stat
>      attr-cnt-name: __ethtool-a-fec-stat-cnt
> 
>      name: stats-grp
>      attr-cnt-name: __ethtool-a-stats-grp-cnt
> 
>      name: stats
>      attr-cnt-name: __ethtool-a-stats-cnt

I know.

> >>   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,
> >> +		   struct ethtool_fec_hist *hist)
> >>   {
> >> +	struct ethtool_fec_hist_value *values = hist->values;
> >> +
> >> +	hist->ranges = netdevsim_fec_ranges;
> >> +
> >>   	fec_stats->corrected_blocks.total = 123;
> >>   	fec_stats->uncorrectable_blocks.total = 4;
> >> +
> >> +	values[0].bin_value = 445;  
> > 
> > Bin 0 had per lane breakdown, can't core add up the lanes for the
> > driver?  
> 
> Like it's done for blocks counter? Should we force drivers to keep 'sum'
> value equal to ETHTOOL_STAT_NOT_SET when they provide per-lane values?

No preference, but if it is NOT_SET we should add it up.

Re: [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Carolina Jubran]

On 18/09/2025 3:46, Jakub Kicinski wrote:
> On Tue, 16 Sep 2025 19:12:56 +0000 Vadim Fedorenko wrote:
>> From: Carolina Jubran <cjubran@nvidia.com>
>>
>> Introduce support for querying the Ports Phy Histogram Configuration
>> Register (PPHCR) to retrieve RS-FEC histogram bin ranges. The ranges
>> are stored in a static array and will be used to map histogram counters
>> to error levels.
>>
>> The actual RS-FEC histogram statistics are not yet reported in this
>> commit and will be handled in a downstream patch.
>> @@ -6246,8 +6246,17 @@ int mlx5e_priv_init(struct mlx5e_priv *priv,
>>   	if (!priv->channel_stats)
>>   		goto err_free_tx_rates;
>>   
>> +	priv->fec_ranges = kcalloc_node(ETHTOOL_FEC_HIST_MAX,
>> +					sizeof(*priv->fec_ranges),
>> +					GFP_KERNEL,
>> +					node);
> Why bother allocating his on the device node? We have no reason to
> believe user will pin eth process that reads these stats to the node
> where the device is :\


You are right. will change.

>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
>> index aae0022e8736..476689cb0c1f 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
>> @@ -1490,8 +1490,63 @@ static void fec_set_corrected_bits_total(struct mlx5e_priv *priv,
>>   				      phy_corrected_bits);
>>   }
>>   
>> +#define MLX5E_FEC_RS_HIST_MAX 16
>> +
>> +static u8
>> +fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv,
>> +			     const struct ethtool_fec_hist_range **ranges)
>> +{
>> +	struct mlx5_core_dev *mdev = priv->mdev;
>> +	u32 out[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
>> +	u32 in[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
>> +	int sz = MLX5_ST_SZ_BYTES(pphcr_reg);
>> +	u8 active_hist_type, num_of_bins;
>> +
>> +	memset(priv->fec_ranges, 0,
>> +	       ETHTOOL_FEC_HIST_MAX * sizeof(*priv->fec_ranges));
>> +	MLX5_SET(pphcr_reg, in, local_port, 1);
>> +	if (mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPHCR, 0, 0))
>> +		return 0;
>> +
>> +	active_hist_type = MLX5_GET(pphcr_reg, out, active_hist_type);
>> +	if (!active_hist_type)
>> +		return 0;
>> +
>> +	num_of_bins = MLX5_GET(pphcr_reg, out, num_of_bins);
>> +	if (WARN_ON_ONCE(num_of_bins > MLX5E_FEC_RS_HIST_MAX))
> why does MLX5E_FEC_RS_HIST_MAX exist?
> We care that bins_cnt <= ETHTOOL_FEC_HIST_MAX - 1
> or is there something in the interface that hardcodes 16?

My intention was to keep mlx5 capped at 16 even if ethtool raises its max.
We’ll only increase this once we know the device should expose more than 16.
Since our HW has internal modes that can report more than 16 bins, this 
ensures
we don’t accidentally expose them if ethtool increases its max.

>> @@ -2619,3 +2675,4 @@ unsigned int mlx5e_nic_stats_grps_num(struct mlx5e_priv *priv)
>>   {
>>   	return ARRAY_SIZE(mlx5e_nic_stats_grps);
>>   }
>> +
> spurious change

Ack! Thanks! Carolina

Re: [PATCH net-next v3 4/4] net/mlx5e: Report RS-FEC histogram statistics via ethtool

[Vadim Fedorenko]

On 18/09/2025 01:48, Jakub Kicinski wrote:
> On Tue, 16 Sep 2025 19:12:57 +0000 Vadim Fedorenko wrote:
>> +	for (int i = 0; i < num_of_bins; i++) {
> 
> brackets unnecessary
> 
> in the other patch you picked u8 for i, good to be consistent
> (int is better)
> 
>> +		hist->values[i].bin_value = MLX5_GET64(rs_histogram_cntrs,
>> +						       rs_histogram_cntrs,
>> +						       hist[i]);
> 
> could also be written as:
> 
> 		hist->values[i].bin_value =
> 			MLX5_GET64(rs_histogram_cntrs, rs_histogram_cntrs, hist[i]);

this doesn't actually fit into 80 chars (84 chars long)... unless we are
not too strict in the drivers..

Re: [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Jakub Kicinski]

On Thu, 18 Sep 2025 17:25:40 +0300 Carolina Jubran wrote:
> > why does MLX5E_FEC_RS_HIST_MAX exist?
> > We care that bins_cnt <= ETHTOOL_FEC_HIST_MAX - 1
> > or is there something in the interface that hardcodes 16?  
> 
> My intention was to keep mlx5 capped at 16 even if ethtool raises its max.
> We’ll only increase this once we know the device should expose more than 16.
> Since our HW has internal modes that can report more than 16 bins, this 
> ensures we don’t accidentally expose them if ethtool increases its max.

But why?

Re: [PATCH net-next v3 4/4] net/mlx5e: Report RS-FEC histogram statistics via ethtool

[Jakub Kicinski]

On Thu, 18 Sep 2025 15:32:51 +0100 Vadim Fedorenko wrote:
> On 18/09/2025 01:48, Jakub Kicinski wrote:
> > On Tue, 16 Sep 2025 19:12:57 +0000 Vadim Fedorenko wrote:  
> >> +	for (int i = 0; i < num_of_bins; i++) {  
> > 
> > brackets unnecessary
> > 
> > in the other patch you picked u8 for i, good to be consistent
> > (int is better)
> >   
> >> +		hist->values[i].bin_value = MLX5_GET64(rs_histogram_cntrs,
> >> +						       rs_histogram_cntrs,
> >> +						       hist[i]);  
> > 
> > could also be written as:
> > 
> > 		hist->values[i].bin_value =
> > 			MLX5_GET64(rs_histogram_cntrs, rs_histogram_cntrs, hist[i]);  
> 
> this doesn't actually fit into 80 chars (84 chars long)... unless we are
> not too strict in the drivers..

Thought it did, ignore the suggestion, then

Re: [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Carolina Jubran]

On 18/09/2025 17:35, Jakub Kicinski wrote:
> On Thu, 18 Sep 2025 17:25:40 +0300 Carolina Jubran wrote:
>>> why does MLX5E_FEC_RS_HIST_MAX exist?
>>> We care that bins_cnt <= ETHTOOL_FEC_HIST_MAX - 1
>>> or is there something in the interface that hardcodes 16?
>> My intention was to keep mlx5 capped at 16 even if ethtool raises its max.
>> We’ll only increase this once we know the device should expose more than 16.
>> Since our HW has internal modes that can report more than 16 bins, this
>> ensures we don’t accidentally expose them if ethtool increases its max.
> But why?

Because currently those modes shouldn't be exposed for ethernet.

Re: [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Jakub Kicinski]

On Thu, 18 Sep 2025 18:16:14 +0300 Carolina Jubran wrote:
> On 18/09/2025 17:35, Jakub Kicinski wrote:
> > On Thu, 18 Sep 2025 17:25:40 +0300 Carolina Jubran wrote:  
> >>> why does MLX5E_FEC_RS_HIST_MAX exist?
> >>> We care that bins_cnt <= ETHTOOL_FEC_HIST_MAX - 1
> >>> or is there something in the interface that hardcodes 16?  
> >> My intention was to keep mlx5 capped at 16 even if ethtool raises its max.
> >> We’ll only increase this once we know the device should expose more than 16.
> >> Since our HW has internal modes that can report more than 16 bins, this
> >> ensures we don’t accidentally expose them if ethtool increases its max.  
> > But why?  
> 
> Because currently those modes shouldn't be exposed for ethernet.

I understand that the modes should not be exposed.
I don't get why this has anything to do with the number of bins.
Does the FW hardcode that the non-Ethernet modes use bins >=16?
When you say "internal modes that can report more than 16 bins"
it sounds like it uses bins starting from 0, e.g. 0..31.

Re: [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Carolina Jubran]

On 18/09/2025 18:40, Jakub Kicinski wrote:
> I understand that the modes should not be exposed.
> I don't get why this has anything to do with the number of bins.
> Does the FW hardcode that the non-Ethernet modes use bins >=16?
> When you say "internal modes that can report more than 16 bins"
> it sounds like it uses bins starting from 0, e.g. 0..31.

The FW hardcodes that Ethernet modes report up to 16 bins,
while non-Ethernet modes may report up to 19.
And yes, those modes use bins starting from 0, e.g. 0..18.

Re: [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Jakub Kicinski]

On Thu, 18 Sep 2025 22:41:38 +0300 Carolina Jubran wrote:
> On 18/09/2025 18:40, Jakub Kicinski wrote:
> > I understand that the modes should not be exposed.
> > I don't get why this has anything to do with the number of bins.
> > Does the FW hardcode that the non-Ethernet modes use bins >=16?
> > When you say "internal modes that can report more than 16 bins"
> > it sounds like it uses bins starting from 0, e.g. 0..31.  
> 
> The FW hardcodes that Ethernet modes report up to 16 bins,
> while non-Ethernet modes may report up to 19.
> And yes, those modes use bins starting from 0, e.g. 0..18.

Which means that the number of bins doesn't really matter.
You're purely using the bin count as a second order check
to catch the device being in the wrong mode (and I presume
you think that device in the wrong mode should never enter 
the function given the WARN_ON_ONCE()).

Please check the mode directly or remove the check completely.

Re: [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Carolina Jubran]

On 19/09/2025 1:18, Jakub Kicinski wrote:
> On Thu, 18 Sep 2025 22:41:38 +0300 Carolina Jubran wrote:
>> On 18/09/2025 18:40, Jakub Kicinski wrote:
>>> I understand that the modes should not be exposed.
>>> I don't get why this has anything to do with the number of bins.
>>> Does the FW hardcode that the non-Ethernet modes use bins >=16?
>>> When you say "internal modes that can report more than 16 bins"
>>> it sounds like it uses bins starting from 0, e.g. 0..31.
>> The FW hardcodes that Ethernet modes report up to 16 bins,
>> while non-Ethernet modes may report up to 19.
>> And yes, those modes use bins starting from 0, e.g. 0..18.
> Which means that the number of bins doesn't really matter.
> You're purely using the bin count as a second order check
> to catch the device being in the wrong mode (and I presume
> you think that device in the wrong mode should never enter
> the function given the WARN_ON_ONCE()).
>
> Please check the mode directly or remove the check completely.

You are right, the check does look like it's combining two different
things. I will add explicit checking for the mode and keep the
WARN_ON_ONCE() to guard against FW changes or potential bugs.

Re: [Intel-wired-lan] [PATCH net-next v3 1/4] ethtool: add FEC bins histogram report

[Tony Nguyen]

On 9/18/2025 3:58 AM, Vadim Fedorenko wrote:
> On 17/09/2025 12:27, Loktionov, Aleksandr wrote:
>>
>>
>>> -----Original Message-----
>>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
>>> Of Vadim Fedorenko
>>> Sent: Tuesday, September 16, 2025 9:13 PM
>>> To: Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
>>> Michael Chan <michael.chan@broadcom.com>; Pavan Chebbi
>>> <pavan.chebbi@broadcom.com>; Tariq Toukan <tariqt@nvidia.com>; Gal
>>> Pressman <gal@nvidia.com>; intel-wired-lan@lists.osuosl.org; Donald
>>> Hunter <donald.hunter@gmail.com>; Carolina Jubran
>>> <cjubran@nvidia.com>; Vadim Fedorenko <vadim.fedorenko@linux.dev>
>>> Cc: Paolo Abeni <pabeni@redhat.com>; Simon Horman <horms@kernel.org>;
>>> netdev@vger.kernel.org
>>> Subject: [Intel-wired-lan] [PATCH net-next v3 1/4] ethtool: add FEC
>>> bins histogram report
>>>
>>> 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>
>>
>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>>
> 
> Thanks for the review!
> BTW, do you know if Intel's E8xx series can provide such kind of 
> statistics?

Hi Vadim,

I'm not finding anything that suggests we have this information 
available/accessible; I am checking internally to confirm though.

Thanks,
Tony

> CC: Tony and Przemek as maintainers of ice driver