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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 20, 2019 at 3:34 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> [+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@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxx>
> > +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@xxxxxxxxx


Sorry, this has been sitting on my plate for some time now. I'll work
on it and send out a patch later in this week.

Thanks,

Rajat



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux