Re: [PATCH v7 4/5] PCI/ASPM: Add sysfs attributes for controlling ASPM link states

[Bjorn Helgaas <helgaas@kernel.org>]

[-cc Wong, Hui]

On Thu, Nov 21, 2019 at 10:10:17PM +0100, Greg KH wrote:
> On Thu, Nov 21, 2019 at 01:03:06PM -0800, Rajat Jain wrote:
> > On Thu, Nov 21, 2019 at 12:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Sat, Oct 05, 2019 at 02:07:56PM +0200, Heiner Kallweit wrote:
> > >
> > > > +What:                /sys/bus/pci/devices/.../link_pm/clkpm
> > > > +             /sys/bus/pci/devices/.../link_pm/l0s_aspm
> > > > +             /sys/bus/pci/devices/.../link_pm/l1_aspm
> > > > +             /sys/bus/pci/devices/.../link_pm/l1_1_aspm
> > > > +             /sys/bus/pci/devices/.../link_pm/l1_2_aspm
> > > > +             /sys/bus/pci/devices/.../link_pm/l1_1_pcipm
> > > > +             /sys/bus/pci/devices/.../link_pm/l1_2_pcipm
> > > > +Date:                October 2019
> > > > +Contact:     Heiner Kallweit <hkallweit1@gmail.com>
> > > > +Description: If ASPM is supported for an endpoint, then these files
> > > > +             can be used to disable or enable the individual
> > > > +             power management states. Write y/1/on to enable,
> > > > +             n/0/off to disable.
> > >
> > > This is queued up for the v5.5 merge window, so if we want to tweak
> > > anything (path names or otherwise), now is the time.
> > >
> > > I think I might be inclined to change the directory from "link_pm" to
> > > "link", e.g.,
> > >
> > >   - /sys/bus/pci/devices/0000:00:1c.0/link_pm/clkpm
> > >   + /sys/bus/pci/devices/0000:00:1c.0/link/clkpm
> > >
> > > because there are other things that haven't been merged yet that could
> > > go in link/ as well:
> > >
> > >   * Mika's "link disable" control [1]
> > >   * Dilip's link width/speed controls [2,3]
> > >
> > > The max_link_speed, max_link_width, current_link_speed,
> > > current_link_width files could also logically be in link/, although
> > > they've already been merged at the top level.
> > >
> > > Rajat's AER statistics change [4] is also coming.  Those stats aren't
> > > link-related, so they wouldn't go in link/.  The current strawman is
> > > an "aer_stats" directory, but I wonder if we should make a more
> > > generic directory like "errors" that could be used for both AER and
> > > DPC and potentially other error-related things.
> > 
> > Sorry, I haven't been able to find time for it for some time. I doubt
> > if I'll be able to make it to 5.6 timeframe. Nevertheless...
> > 
> > > For example, we could have these link-related things:
> > >
> > >   /sys/.../0000:00:1c.0/link/clkpm            # RW ASPM stuff
> > >   /sys/.../0000:00:1c.0/link/l0s_aspm
> > >   /sys/.../0000:00:1c.0/link/...
> > >   /sys/.../0000:00:1c.0/link/disable          # RW Mika
> > >   /sys/.../0000:00:1c.0/link/speed            # RW Dilip's control
> > >   /sys/.../0000:00:1c.0/link/width            # RW Dilip's control
> > >   /sys/.../0000:00:1c.0/link/max_speed        # RO possible rework
> > >   /sys/.../0000:00:1c.0/link/max_width        # RO possible rework
> > >
> > > With these backwards compatibility symlinks:
> > >
> > >   /sys/.../0000:00:1c.0/max_link_speed     -> link/max_speed
> > >   /sys/.../0000:00:1c.0/current_link_speed -> link/speed
> > >
> > > Rajat's current patch puts the AER stats here at the top level:
> > >
> > >   /sys/.../0000:00:1c.0/aer_stats/fatal_bit4_DLP
> > >
> > > But maybe we could push them down like this:
> > >
> > >   /sys/.../0000:00:1c.0/errors/aer/stats/unc_04_dlp
> > >   /sys/.../0000:00:1c.0/errors/aer/stats/unc_26_poison_tlb_blocked
> > >   /sys/.../0000:00:1c.0/errors/aer/stats/cor_00_rx_err
> > >   /sys/.../0000:00:1c.0/errors/aer/stats/cor_15_hdr_log_overflow
> > 
> > How do we create sub-sub-sub directories in sysfs (errors/aer/stats)?
> 
> You should not.
> 
> > My understanding is that we can only create 1 subdirectory by using a
> > "named" attribute group. If we want more hierarchy, the "errors" and
> > the "aer" will need to be backed up by a kobject. Doable, but just
> > mentioning.
> 
> Not doable, you break userspace tools as they will not "see" those
> directories or attributes.
>
> Keep it only 1 deep if at all possible please.

Oh, that's good to know, thanks!  I guess we'll have to think more
about the error stuff.

What sort of tools would this break?  There are no AER tools since the
AER stats sysfs files don't exist yet, so I assume there are some
generic sysfs tools or libraries.

Incidentally,
https://www.kernel.org/doc/html/latest/admin-guide/sysfs-rules.html
suggests that maybe we should be documenting these files with
/sys/devices paths instead of the symlinks in /sys/bus/pci/devices/,
e.g.,

  diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
  -What:		/sys/bus/pci/devices/.../msi_bus
  -What:		/sys/bus/pci/devices/.../msi_irqs/
  -What:		/sys/bus/pci/devices/.../msi_irqs/<N>
  +What:		/sys/devices/pci*/.../msi_bus
  +What:		/sys/devices/pci*/.../msi_irqs/
  +What:		/sys/devices/pci*/.../msi_irqs/<N>