netdev - Re: [net-next 1/4] e1000e: allow turning s0ix flows on for systems with ME

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: <CAKgT0UedAJbhh5dA5V+otzXe2Hn3VZ44+=DGEtNWjA5R3sDBug@mail.gmail.com>
Date:   Mon, 30 Nov 2020 14:25:52 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     "Limonciello, Mario" <Mario.Limonciello@...l.com>
Cc:     Tony Nguyen <anthony.l.nguyen@...el.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Netdev <netdev@...r.kernel.org>,
        Stefan Assmann <sassmann@...hat.com>,
        "Neftin, Sasha" <sasha.neftin@...el.com>,
        Aaron Brown <aaron.f.brown@...el.com>
Subject: Re: [net-next 1/4] e1000e: allow turning s0ix flows on for systems
 with ME

On Mon, Nov 30, 2020 at 2:16 PM Limonciello, Mario
<Mario.Limonciello@...l.com> wrote:
>
> >
> > Generally the use of module parameters and sysfs usage are frowned
> > upon.
>
> I was trying to build on the existing module parameters that existed
> already for e1000e.  So I guess I would ask, why are those not done in
> ethtool?  Should those parameters go away and they migrate to ethtool
> for the same reasons as this?

What it comes down to is that the existing module parameters are
grandfathered in and we should not break things by removing them. New
drivers aren't allowed to add them, and we are not supposed to add to
them.

> > Based on the configuration isn't this something that could just
> > be controlled via an ethtool priv flag? Couldn't you just have this
> > default to whatever the heuristics decide at probe on and then support
> > enabling/disabling it via the priv flag? You could look at
> > igb_get_priv_flags/igb_set_priv_flags for an example of how to do what
> > I am proposing.
>
> I don't disagree this solution would work, but it adds a new dependency
> on having ethtool and the kernel move together to enable it.

Actually ethtool wouldn't have to change. The priv-flags are passed as
strings to ethtool from the driver and set as a u32 bit flag array if
I recall correctly.

> One advantage of the way this is done it allows shipping a system with an
> older Linux kernel that isn't yet recognized in the kernel heuristics to
> turn on by default with a small udev rule or kernel command line change.
>
> For example systems that aren't yet released could have this documented on
> RHEL certification pages at release time for older RHEL releases before a
> patch to add to the heuristics has been backported.

I suggest taking a look at the priv-flags interface. I am not
suggesting adding a new interface to ethtool. It is an existing
interface designed to allow for one-off features to be
enabled/disabled on a given port.

> >
> > I think it would simplify this quite a bit since you wouldn't have to
> > implement sysfs show/store operations for the value and would instead
> > be allowing for reading/setting via the get_priv_flags/set_priv_flags
> > operations. In addition you could leave the code for checking what
> > supports this in place and have it set a flag that can be read or
> > overwritten.
>
> If the consensus is to move in this direction, yes I'll redo the patch
> series and modify ethtool as well.

No changes needed to ethtool. The flags are driver specific which is
why this would work, or are you saying this change will be needed for
other drivers? If so then yes I would recommend coming up with a
standard interface we can use for those drivers as well.