On Mon, Nov 30, 2020 at 1:32 PM Tony Nguyen <anthony.l.ngu...@intel.com> wrote:
>
> From: Mario Limonciello <mario.limoncie...@dell.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.limoncie...@dell.com>
> Tested-by: Aaron Brown <aaron.f.br...@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.ngu...@intel.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.
