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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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

After thinking a bit more, I don't like the idea of copying static constants
over and over again in cases where we have pre-defined ranges. What about adding
an array to the pointer, so drivers with dynamic range calculation can fill up
array per query while others can simply put a static const pointer and don't do
memcpy?

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