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]

Date:   Mon, 30 Nov 2020 14:00:14 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Mario Limonciello <mario.limonciello@...l.com>,
        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 1:32 PM Tony Nguyen <anthony.l.nguyen@...el.com> wrote:
>
> From: Mario Limonciello <mario.limonciello@...l.com>
>
> S0ix for GBE flows are needed for allowing the system to get into deepest
> power state, but these require coordination of components outside of
> control of Linux kernel.  For systems that have confirmed to coordinate
> this properly, allow turning on the s0ix flows at load time or runtime.
>
> Fixes: e086ba2fccda ("e1000e: disable s0ix entry and exit flows for ME systems")
> Signed-off-by: Mario Limonciello <mario.limonciello@...l.com>
> Tested-by: Aaron Brown <aaron.f.brown@...el.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> ---
>  .../device_drivers/ethernet/intel/e1000e.rst  |  23 ++++
>  drivers/net/ethernet/intel/e1000e/e1000.h     |   7 ++
>  drivers/net/ethernet/intel/e1000e/netdev.c    |  64 +++++-----
>  drivers/net/ethernet/intel/e1000e/param.c     | 110 ++++++++++++++++++
>  4 files changed, 166 insertions(+), 38 deletions(-)
>
> diff --git a/Documentation/networking/device_drivers/ethernet/intel/e1000e.rst b/Documentation/networking/device_drivers/ethernet/intel/e1000e.rst
> index f49cd370e7bf..da029b703573 100644
> --- a/Documentation/networking/device_drivers/ethernet/intel/e1000e.rst
> +++ b/Documentation/networking/device_drivers/ethernet/intel/e1000e.rst
> @@ -249,6 +249,29 @@ Debug
>
>  This parameter adjusts the level of debug messages displayed in the system logs.
>
> +EnableS0ix
> +----------
> +:Valid Range: 0, 1, 2
> +:Default Value: 1 (Use Heuristics)
> +
> +   +-------+----------------+
> +   | Value |    EnableS0ix  |
> +   +=======+================+
> +   |   0   |    Disabled    |
> +   +-------+----------------+
> +   |   1   | Use Heuristics |
> +   +-------+----------------+
> +   |   2   |    Enabled     |
> +   +-------+----------------+
> +
> +Controls whether the e1000e driver will attempt s0ix flows.  S0ix flows require
> +correct platform configuration. By default the e1000e driver will use some heuristics
> +to decide whether to enable s0ix.  This parameter can be used to override heuristics.
> +
> +Additionally a sysfs file "enable_s0ix" will be present that can change the value at
> +runtime.
> +
> +Note: This option is only effective on Cannon Point or later hardware.
>
>  Additional Features and Configurations
>  ======================================

Generally the use of module parameters and sysfs usage are frowned
upon. 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 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.