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

[Rajat Jain]

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)?
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.

Overall the proposal looks like a step in the right direction to me.

Thanks & Best Regards,

Rajat

>
> There are some AER-related things we don't have at all today that
> could go here:
>
>   /sys/.../0000:00:1c.0/errors/aer/ecrc_gen
>   /sys/.../0000:00:1c.0/errors/aer/ecrc_check
>   /sys/.../0000:00:1c.0/errors/aer/unc_err_status
>   /sys/.../0000:00:1c.0/errors/aer/unc_err_mask
>   /sys/.../0000:00:1c.0/errors/aer/unc_err_sev
>
> And we might someday want DPC knobs like this:
>
>   /sys/.../0000:00:1c.0/errors/dpc/status
>   /sys/.../0000:00:1c.0/errors/dpc/error_source
>
> Any thoughts?
>
> Bjorn
>
> [1] https://lore.kernel.org/r/20190529104942.74991-1-mika.westerberg@xxxxxxxxxxxxxxx
> [2] https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@xxxxxxxxxxxxxxx
> [3] https://lore.kernel.org/r/20191030221436.GA261632@xxxxxxxxxx/
> [4] https://lore.kernel.org/r/20190827222145.32642-2-rajatja@xxxxxxxxxx
> [5] https://lore.kernel.org/r/b1c83f8a-9bf6-eac5-82d0-cf5b90128fbf@xxxxxxxxx