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

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Aug 20, 2019 at 12:05:01PM -0700, Rajat Jain wrote:
> On Tue, Aug 20, 2019 at 3:34 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > 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
> 
> 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.

Don't worry, I cc'd you mostly as FYI because you're a recent
contributor to aspm.c in this area, though I wouldn't complain about
AER stats sysfs patches either :)

Bjorn