netdev - Re: [PATCH net-next 0/2] ice: support FEC automatic disable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5d9c6b31-cdf2-1285-6d4b-2368bae8b6f4@nvidia.com>
Date:   Thu, 25 Aug 2022 10:08:05 +0300
From:   Gal Pressman <gal@...dia.com>
To:     "Keller, Jacob E" <jacob.e.keller@...el.com>
Cc:     Saeed Mahameed <saeedm@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable

On 24/08/2022 20:40, Keller, Jacob E wrote:
>
>> -----Original Message-----
>> From: Gal Pressman <gal@...dia.com>
>> Sent: Wednesday, August 24, 2022 6:36 AM
>> To: Keller, Jacob E <jacob.e.keller@...el.com>
>> Cc: Saeed Mahameed <saeedm@...dia.com>; netdev@...r.kernel.org
>> Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable
>>
>> On 23/08/2022 18:04, Jacob Keller wrote:
>>> 2) always treat ETHTOOL_FEC_AUTO as "automatic + allow disable"
>>>
>>>   This could work, but it means that behavior will differ depending on the
>>>   firmware version. Users have no way to know that and might be surprised to
>>>   find the behavior differ across devices which have different firmware
>>>   which do or don't support this variation of automatic selection.
>> Hi Jacob,
>> This is exactly how it's already implemented in mlx5, and I don't really
>> understand how firmware version is related? Is it specific to your
>> device firmware?
>> Maybe you can workaround that in the driver?
> For ice, the original "auto" implementation (which is handled by firmware) will automatically select an FEC mode based on the media type and using a state machine to go through options until it finds a valid link.
>
> This implementation would never select "No FEC" (i.e. FEC_OFF) for certain module types which do not list "No FEC" as part of their auto negotiation supported list. (Despite this not actually being auto negotiation). Some of our customers were surprised by this and asked if we could change it, so new firmware has an option to allow choosing "No FEC". This is an "opt-in" that the driver must tell firmware when setting up FEC mode. This obviously is only available on newer firmware. Going with option 2 would result in differing behavior depending on what firmware and driver you're using.
>
> I thought that was a bit confusing since userspace/users would not know which variant is in use, and my understanding from our customer engineers is that we don't want to change the behavior without an explicit request of some kind. That's where the original private flag came from.
>
> As for working around this in the driver, I am not sure how feasible that is.
>
>> I feel like we're going the wrong way here having different flags
>> interpretations by different drivers.
> I would prefer to avoid that as well

Then maybe adding a new flag is the right thing to do here.

That way the existing auto mode will keep its current meaning (all modes
including off), which you'll be able to support on newer firmware
versions, and the new auto flag (all modes excluding off) will be
supported on all firmware versions.
Then maybe we can even add support for the new flag in mlx5 (I need to
check whether that's feasible with our hardware).