netdev - RE: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled via ETHTOOL

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: <CO1PR11MB508971C03652EF412A43DD80D6739@CO1PR11MB5089.namprd11.prod.outlook.com>
Date:   Wed, 24 Aug 2022 21:29:31 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Greenwalt, Paul" <paul.greenwalt@...el.com>
Subject: RE: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC
 disabled via ETHTOOL_SFECPARAM



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Tuesday, August 23, 2022 3:18 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: netdev@...r.kernel.org; Greenwalt, Paul <paul.greenwalt@...el.com>
> Subject: Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled
> via ETHTOOL_SFECPARAM
> 
> On Tue, 23 Aug 2022 08:04:38 -0700 Jacob Keller wrote:
> > The default Link Establishment State Machine (LESM) behavior does not
> 
> LESM is the algo as specified by the IEEE standard? If so could you add
> the citation (section of the spec where it's defined)?
> 
> Is disabling the only customization we may want?
> 

Ok I got information from the other folks here. LESM is not a standard its just the name we used internally for how the firmware establishes link. I'll rephrase this whole section and clarify it.

I also clarified whats going on here in

https://lore.kernel.org/netdev/CO1PR11MB50895C42C04A408CF6151023D6739@CO1PR11MB5089.namprd11.prod.outlook.com/

Basically: the firmware has a process for automatically selecting FEC mode. On older firmware, this process didn't include the possibility of selecting "No FEC", or in other words of disabling FEC. The process firmware uses is a state machine that goes through the FEC modes known to be supported by the media type.

Some of our customers were confused about this and have asked if it was possible to allow disabling FEC. This is distinct from manually setting FEC_OFF, because it lets the firmwares existing state machine determine of the disabled mode is suitable or not. I understand it as the goal of being able to say "automatically select FEC for me, but if No FEC is suitable allow that".

The new firmware requires manually opting in with a new bit, and we don't want to change existing behavior, hence the new approach to using both AUTO and OFF together. If we instead go with "use the new mode when its available" then there is no way for users to know this easily since it would just depend on the firmware version.

I've got a proposed reword to the commit message as follows:

    Users can request automatic selection of FEC mode via the
    ETHTOOL_MSG_FEC_SET netlink message (or ETHTOOL_SFECPARAM ioctl). The ice
    driver implements this by asking firmware to select a suitable mode.

    The firmware selects a FEC mode automatically based on a table of supported
    FEC modes for the media type. Older versions of firmware will never select
    "No FEC" (i.e. disabling FEC).

    Newer versions of firmware support an automatic mode which also allows
    selecting the "No FEC" mode.

    To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a request
    to automatically select an appropriate FEC mode including potentially
    disabling FEC.

    This is done so that the existing behavior of ETHTOOL_FEC_AUTO remains
    unchanged, and users must actively select the new behavior. This is
    important since we do not want to change the behavior purely based on the
    firmware version. Additionally, this allows reporting an error if the mode
    is requested on a device still operating the older firmware version.

    This is distinct from ETHTOOL_FEC_OFF because that selection will always
    simply disable FEC without going through the firmware automatic selection
    state machine.

    This *does* mean that ice is now accepting one "bitwise OR" set for FEC
    configuration, which is somewhat against the recommendations made in
    6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not
    sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make any
    sense here.

Is that a better explanation of the reasoning?

Thanks,
Jake

> > allow the use of FEC disabled if the media does not support FEC
> > disabled. However users may want to override this behavior.
> >
> > To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a
> request
> > to automatically select an appropriate FEC mode including potentially
> > disabling FEC.
> >
> > This is distinct from ETHTOOL_FEC_AUTO because that will not allow the LESM
> > to select FEC disabled. It is distinct from ETHTOOL_FEC_OFF because
> > FEC_OFF will always disable FEC without any LESM automatic selection.
> >
> > This *does* mean that ice is now accepting one "bitwise OR" set for FEC
> > configuration, which is somewhat against the recommendations made in
> > 6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not
> > sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make
> any
> > sense here.
> >
> > With this change, users can opt to allow automatic FEC disable via
> >
> >   ethtool --set-fec ethX encoding auto off