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:

quoted

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

quoted

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