Re: [PATCH 3/3] PCI/ASPM: add sysfs attribute for controlling ASPM

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Greg, Rajat]

On Thu, May 23, 2019 at 10:05:35PM +0200, Heiner Kallweit wrote:
> Background of this extension is a problem with the r8169 network driver.
> Several combinations of board chipsets and network chip versions have
> problems if ASPM is enabled, therefore we have to disable ASPM per default.
> However especially on notebooks ASPM can provide significant power-saving,
> therefore we want to give users the option to enable ASPM. With the new sysfs
> attribute users can control which ASPM link-states are enabled/disabled.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |  13 ++
>  drivers/pci/pci.h                       |   8 +-
>  drivers/pci/pcie/aspm.c                 | 180 +++++++++++++++++++++++-
>  3 files changed, 193 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 8bfee557e..38fe358de 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -347,3 +347,16 @@ Description:
>  		If the device has any Peer-to-Peer memory registered, this
>  	        file contains a '1' if the memory has been published for
>  		use outside the driver that owns the device.
> +
> +What:		/sys/bus/pci/devices/.../power/aspm_link_states
> +Date:		May 2019
> +Contact:	Heiner Kallweit <hkallweit1@gmail.com>
> +Description:
> +		If ASPM is supported for an endpoint, then this file can be
> +		used to enable / disable link states. A link state
> +		displayed in brackets is enabled, otherwise it's disabled.
> +		To control link states (case insensitive):
> +		+state : enables a supported state
> +		-state : disables a state
> +		none : disables all link states
> +		all : enables all supported link states

IIUC this "aspm_link_states" file will contain things like this:

  L0S L1 L1.1 L1.2                 # All states supported, all disabled
  [L0S] L1                         # L0s enabled, L1 supported but disabled
  [L0S] [L1]                       # L0s and L1 enabled
  ...

and the control is by writing things like this to it:

  +L1                              # enables L1
  +L1.1                            # enables L1.1
  -L0S                             # disables L0s

I know this file structure is similar to protocol handling in
drivers/media/rc/rc-main.c, but Documentation/filesystems/sysfs.txt
suggests single values in a file, and Greg recently pointed out that
we screwed up some PCI AER stats [1].

So I'm thinking maybe we should split this into several files, e.g.,

  /sys/devices/pci*/.../power/aspm_l0s
  /sys/devices/pci*/.../power/aspm_l1
  /sys/devices/pci*/.../power/aspm_l1.1
  /sys/devices/pci*/.../power/aspm_l1.2

which would contain just 1/0 values, and we'd write 1/0 to
enable/disable things.

Since the L1 PM Substates control register has separate enable bits
for PCI-PM L1.1 and L1.2, we might also want a way to manage those.

Bjorn

[1] https://lore.kernel.org/r/20190621072911.GA21600@kroah.com