[PATCH net] ethtool: Embed FEC hist ranges as buffer in struct

[PATCH net] ethtool: Embed FEC hist ranges as buffer in struct

[Eric Joyner]

When a driver's .get_fec_stats() handler is called and the driver
supports FEC histogram stats, the driver supplies the histogram bin
ranges via a pointer.  This pointer is assigned while under the netdev
ops lock in fec_prepare_data(), but the actual data is only read after
the lock is released; so this allows the driver to change the ranges
(e.g. from another .get_fec_stats() call) while the current call chain
is reading them in fec_fill_reply().

Fix this by embedding a buffer for the driver-supplied ranges in struct
ethtool_fec_hist instead of using a pointer; this ensures there's an
ethtool core-owned consistent copy that can be used after the netdev ops
lock is dropped and later in fec_fill_reply(). While some drivers like
bnxt use a constant struct for their ranges and won't be affected by
this issue, others like mlx5 (and eventually ionic) will use a
dynamically constructed range struct and could potentially run into an
issue.

Since the kernel API changed here, change the in-tree drivers that
report FEC histogram stats to copy their ranges instead of just
supplying a pointer.

Fixes: cc2f08129925 ("ethtool: add FEC bins histogram report")
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---

This is a fix for the issue described by Jakub in:
https://lore.kernel.org/netdev/20260615182732.7d28e31a@kernel.org/

 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  3 ++-
 drivers/net/ethernet/mellanox/mlx5/core/en.h      |  1 -
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c |  7 -------
 .../net/ethernet/mellanox/mlx5/core/en_stats.c    | 15 ++++++---------
 drivers/net/netdevsim/ethtool.c                   |  3 ++-
 include/linux/ethtool.h                           |  2 +-
 net/ethtool/fec.c                                 |  3 ++-
 7 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 62bc9cae613c..2e0ec8543f7f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -3291,7 +3291,8 @@ static void bnxt_hwrm_port_phy_fdrstat(struct bnxt *bp,
 	resp = hwrm_req_hold(bp, req);
 	rc = hwrm_req_send(bp, req);
 	if (!rc) {
-		hist->ranges = bnxt_fec_ranges;
+		memcpy(hist->ranges, bnxt_fec_ranges,
+		       ETHTOOL_FEC_HIST_MAX * sizeof(*hist->ranges));
 		for (i = 0; i <= 15; i++) {
 			__le64 sum = resp->accumulated_codewords_err_s[i];
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index d507289096c2..6867a5aed42c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -984,7 +984,6 @@ 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;
 };
 
 static inline u16 mlx5e_stats_nch_read(const struct mlx5e_priv *priv)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index aa8610cedaa8..8db235ac9ae4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -6415,14 +6415,8 @@ int mlx5e_priv_init(struct mlx5e_priv *priv,
 	if (!priv->channel_stats)
 		goto err_free_tx_rates;
 
-	priv->fec_ranges = kzalloc_objs(*priv->fec_ranges, ETHTOOL_FEC_HIST_MAX);
-	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:
@@ -6447,7 +6441,6 @@ 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 de38b60806c2..5f45e3c77cf1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -1550,7 +1550,7 @@ static bool fec_rs_validate_hist_type(int mode, int hist_type)
 
 static u8
 fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv, int mode,
-			     const struct ethtool_fec_hist_range **ranges)
+			     struct ethtool_fec_hist_range *ranges)
 {
 	struct mlx5_core_dev *mdev = priv->mdev;
 	u32 out[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
@@ -1558,8 +1558,6 @@ fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv, int mode,
 	int sz = MLX5_ST_SZ_BYTES(pphcr_reg);
 	u8 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;
@@ -1575,12 +1573,11 @@ fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv, int mode,
 	for (int 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[i].high = MLX5_GET(bin_range_layout, bin_range,
+					  high_val);
+		ranges[i].low = MLX5_GET(bin_range_layout, bin_range,
+					 low_val);
 	}
-	*ranges = priv->fec_ranges;
 
 	return num_of_bins;
 }
@@ -1622,7 +1619,7 @@ static void fec_set_histograms_stats(struct mlx5e_priv *priv, int mode,
 	case MLX5E_FEC_LLRS_272_257_1:
 	case MLX5E_FEC_RS_544_514_INTERLEAVED_QUAD:
 		num_of_bins =
-			fec_rs_histogram_fill_ranges(priv, mode, &hist->ranges);
+			fec_rs_histogram_fill_ranges(priv, mode, hist->ranges);
 		if (num_of_bins)
 			return fec_rs_histogram_fill_stats(priv, num_of_bins,
 							   hist);
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 025ea79879f3..69849e4192d4 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -178,7 +178,8 @@ nsim_get_fec_stats(struct net_device *dev, struct ethtool_fec_stats *fec_stats,
 {
 	struct ethtool_fec_hist_value *values = hist->values;
 
-	hist->ranges = netdevsim_fec_ranges;
+	memcpy(hist->ranges, netdevsim_fec_ranges,
+	       ARRAY_SIZE(netdevsim_fec_ranges) * sizeof(*hist->ranges));
 
 	fec_stats->corrected_blocks.total = 123;
 	fec_stats->uncorrectable_blocks.total = 4;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 5d491a98265e..d3bf1b2ddecc 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -561,7 +561,7 @@ struct ethtool_fec_hist {
 		u64 sum;
 		u64 per_lane[ETHTOOL_MAX_LANES];
 	} values[ETHTOOL_FEC_HIST_MAX];
-	const struct ethtool_fec_hist_range *ranges;
+	struct ethtool_fec_hist_range ranges[ETHTOOL_FEC_HIST_MAX];
 };
 /**
  * struct ethtool_fec_stats - statistics for IEEE 802.3 FEC
diff --git a/net/ethtool/fec.c b/net/ethtool/fec.c
index e2d539271060..28373e16dd51 100644
--- a/net/ethtool/fec.c
+++ b/net/ethtool/fec.c
@@ -186,7 +186,8 @@ static int fec_put_hist(struct sk_buff *skb,
 	int i, j;
 	u64 sum;
 
-	if (!ranges)
+	if (values[0].sum == ETHTOOL_STAT_NOT_SET &&
+	    values[0].per_lane[0] == ETHTOOL_STAT_NOT_SET)
 		return 0;
 
 	for (i = 0; i < ETHTOOL_FEC_HIST_MAX; i++) {
-- 
2.17.1

Re: [PATCH net] ethtool: Embed FEC hist ranges as buffer in struct

[Vadim Fedorenko]

On 11/07/2026 00:00, Eric Joyner wrote:
> When a driver's .get_fec_stats() handler is called and the driver
> supports FEC histogram stats, the driver supplies the histogram bin
> ranges via a pointer.  This pointer is assigned while under the netdev
> ops lock in fec_prepare_data(), but the actual data is only read after
> the lock is released; so this allows the driver to change the ranges
> (e.g. from another .get_fec_stats() call) while the current call chain
> is reading them in fec_fill_reply().
> 
> Fix this by embedding a buffer for the driver-supplied ranges in struct
> ethtool_fec_hist instead of using a pointer; this ensures there's an
> ethtool core-owned consistent copy that can be used after the netdev ops
> lock is dropped and later in fec_fill_reply(). While some drivers like
> bnxt use a constant struct for their ranges and won't be affected by
> this issue, others like mlx5 (and eventually ionic) will use a
> dynamically constructed range struct and could potentially run into an
> issue.

I didn't like the idea of dynamic range, FEC is not changing while the
link is UP, I don't see a reason to dynamically reconstruct histogram
bins every single call. And the histogram itself is stable per HW per
FEC, can be constant pre-defined struct in a driver, like in bnxt.

But if dynamic allocation is the only option, then yes, we have to
change this ABI.

> Since the kernel API changed here, change the in-tree drivers that
> report FEC histogram stats to copy their ranges instead of just
> supplying a pointer.
> 
> Fixes: cc2f08129925 ("ethtool: add FEC bins histogram report")
> Signed-off-by: Eric Joyner <eric.joyner@amd.com>

Re: [PATCH net] ethtool: Embed FEC hist ranges as buffer in struct

[Eric Joyner]

On 7/11/2026 2:03 PM, Vadim Fedorenko wrote:
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> On 11/07/2026 00:00, Eric Joyner wrote:
>> When a driver's .get_fec_stats() handler is called and the driver
>> supports FEC histogram stats, the driver supplies the histogram bin
>> ranges via a pointer.  This pointer is assigned while under the netdev
>> ops lock in fec_prepare_data(), but the actual data is only read after
>> the lock is released; so this allows the driver to change the ranges
>> (e.g. from another .get_fec_stats() call) while the current call chain
>> is reading them in fec_fill_reply().
>>
>> Fix this by embedding a buffer for the driver-supplied ranges in struct
>> ethtool_fec_hist instead of using a pointer; this ensures there's an
>> ethtool core-owned consistent copy that can be used after the netdev ops
>> lock is dropped and later in fec_fill_reply(). While some drivers like
>> bnxt use a constant struct for their ranges and won't be affected by
>> this issue, others like mlx5 (and eventually ionic) will use a
>> dynamically constructed range struct and could potentially run into an
>> issue.
> 
> I didn't like the idea of dynamic range, FEC is not changing while the
> link is UP, I don't see a reason to dynamically reconstruct histogram
> bins every single call. And the histogram itself is stable per HW per
> FEC, can be constant pre-defined struct in a driver, like in bnxt.
> 
> But if dynamic allocation is the only option, then yes, we have to
> change this ABI.

We can discuss this more.

I think overall drivers aren't going to need to dynamically allocate a range; I
mention ionic but at the moment I think there's only going to be two possible
FEC ranges; the sixteen bin one for RS(544,514) and I think what should be a
reduced size eight bin one for low latency RS-FEC RS(272,258) (unlike the 802.3
spec the Ethernet Consortium Spec for LL RS-FEC doesn't talk about a histogram,
but FEC math says those parameters can only correct up to 7-bit errors).

So one option could be to have the pointer be required to point to static
memory; or possibly a pre-defined histogram range entry in the kernel? I don't
see any other drivers currently combining multiple bit-error counts into one bin
and I wasn't sure if that's something the mlx5 driver actually uses, too.

OTOH, doing this dynamic range calculation should be computationally pretty
cheap overall, and provides flexibility without keeping or adding new
concurrency problems (which is an important concern!), so I don't mind the
current approach even if it does look wasteful.

- Eric

> 
>> Since the kernel API changed here, change the in-tree drivers that
>> report FEC histogram stats to copy their ranges instead of just
>> supplying a pointer.
>>
>> Fixes: cc2f08129925 ("ethtool: add FEC bins histogram report")
>> Signed-off-by: Eric Joyner <eric.joyner@amd.com>

Re: [PATCH net] ethtool: Embed FEC hist ranges as buffer in struct

[Vadim Fedorenko]

On 13.07.2026 23:37, Eric Joyner wrote:
> On 7/11/2026 2:03 PM, Vadim Fedorenko wrote:
>> Caution: This message originated from an External Source. Use proper caution
>> when opening attachments, clicking links, or responding.
>>
>>
>> On 11/07/2026 00:00, Eric Joyner wrote:
>>> When a driver's .get_fec_stats() handler is called and the driver
>>> supports FEC histogram stats, the driver supplies the histogram bin
>>> ranges via a pointer.  This pointer is assigned while under the netdev
>>> ops lock in fec_prepare_data(), but the actual data is only read after
>>> the lock is released; so this allows the driver to change the ranges
>>> (e.g. from another .get_fec_stats() call) while the current call chain
>>> is reading them in fec_fill_reply().
>>>
>>> Fix this by embedding a buffer for the driver-supplied ranges in struct
>>> ethtool_fec_hist instead of using a pointer; this ensures there's an
>>> ethtool core-owned consistent copy that can be used after the netdev ops
>>> lock is dropped and later in fec_fill_reply(). While some drivers like
>>> bnxt use a constant struct for their ranges and won't be affected by
>>> this issue, others like mlx5 (and eventually ionic) will use a
>>> dynamically constructed range struct and could potentially run into an
>>> issue.
>>
>> I didn't like the idea of dynamic range, FEC is not changing while the
>> link is UP, I don't see a reason to dynamically reconstruct histogram
>> bins every single call. And the histogram itself is stable per HW per
>> FEC, can be constant pre-defined struct in a driver, like in bnxt.
>>
>> But if dynamic allocation is the only option, then yes, we have to
>> change this ABI.
> 
> We can discuss this more.
> 
> I think overall drivers aren't going to need to dynamically allocate a range; I
> mention ionic but at the moment I think there's only going to be two possible
> FEC ranges; the sixteen bin one for RS(544,514) and I think what should be a
> reduced size eight bin one for low latency RS-FEC RS(272,258) (unlike the 802.3
> spec the Ethernet Consortium Spec for LL RS-FEC doesn't talk about a histogram,
> but FEC math says those parameters can only correct up to 7-bit errors).
> 
> So one option could be to have the pointer be required to point to static
> memory; or possibly a pre-defined histogram range entry in the kernel? I don't
> see any other drivers currently combining multiple bit-error counts into one bin
> and I wasn't sure if that's something the mlx5 driver actually uses, too.

The problem of mlx5 driver is that for RS(528,514) CX6 firmware returns only 4
bins with merged ranges, while newer generations have 7 bins for the same FEC.
That means core kernel won't have all possible options and the drivers have to
have their own static lists of ranges. But it's still stable list per HW per FEC
as I stated before.

> OTOH, doing this dynamic range calculation should be computationally pretty
> cheap overall, and provides flexibility without keeping or adding new
> concurrency problems (which is an important concern!), so I don't mind the
> current approach even if it does look wasteful.
> 
> - Eric
> 
>>
>>> Since the kernel API changed here, change the in-tree drivers that
>>> report FEC histogram stats to copy their ranges instead of just
>>> supplying a pointer.
>>>
>>> Fixes: cc2f08129925 ("ethtool: add FEC bins histogram report")
>>> Signed-off-by: Eric Joyner <eric.joyner@amd.com>
>