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

[Greg KH]

On Thu, Nov 21, 2019 at 01:03:06PM -0800, Rajat Jain wrote:
> Hi,
> 
> On Thu, Nov 21, 2019 at 12:49 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> >
> > [+cc Rafael, Mika, Wong, Hui, Rajat, Keith, LKML, original patch at [5]]
> >
> > 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@xxxxxxxxx>
> > > +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.

thanks,

greg k-h