* Re: [PATCH v7 4/5] PCI/ASPM: Add sysfs attributes for controlling ASPM link states [not found] <b1c83f8a-9bf6-eac5-82d0-cf5b90128fbf@gmail.com> @ 2019-11-21 20:49 ` Bjorn Helgaas 2019-11-21 21:03 ` Rajat Jain 0 siblings, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2019-11-21 20:49 UTC (permalink / raw) To: Heiner Kallweit Cc: Frederick Lawler, Greg KH, Rajat Jain, linux-pci, Rafael J. Wysocki, Mika Westerberg, Wong Vee Khee, Hui Chun Ong, Keith Busch, linux-kernel [+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@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. 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 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@linux.intel.com [2] https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com [3] https://lore.kernel.org/r/20191030221436.GA261632@google.com/ [4] https://lore.kernel.org/r/20190827222145.32642-2-rajatja@google.com [5] https://lore.kernel.org/r/b1c83f8a-9bf6-eac5-82d0-cf5b90128fbf@gmail.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v7 4/5] PCI/ASPM: Add sysfs attributes for controlling ASPM link states 2019-11-21 20:49 ` [PATCH v7 4/5] PCI/ASPM: Add sysfs attributes for controlling ASPM link states Bjorn Helgaas @ 2019-11-21 21:03 ` Rajat Jain 2019-11-21 21:10 ` Greg KH 0 siblings, 1 reply; 5+ messages in thread From: Rajat Jain @ 2019-11-21 21:03 UTC (permalink / raw) To: Bjorn Helgaas Cc: Heiner Kallweit, Frederick Lawler, Greg KH, linux-pci, Rafael J. Wysocki, Mika Westerberg, Wong Vee Khee, Hui Chun Ong, Keith Busch, Linux Kernel Mailing List Hi, On Thu, Nov 21, 2019 at 12:49 PM Bjorn Helgaas <helgaas@kernel.org> 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@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)? 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@linux.intel.com > [2] https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com > [3] https://lore.kernel.org/r/20191030221436.GA261632@google.com/ > [4] https://lore.kernel.org/r/20190827222145.32642-2-rajatja@google.com > [5] https://lore.kernel.org/r/b1c83f8a-9bf6-eac5-82d0-cf5b90128fbf@gmail.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v7 4/5] PCI/ASPM: Add sysfs attributes for controlling ASPM link states 2019-11-21 21:03 ` Rajat Jain @ 2019-11-21 21:10 ` Greg KH 2019-11-21 23:04 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: Greg KH @ 2019-11-21 21:10 UTC (permalink / raw) To: Rajat Jain Cc: Bjorn Helgaas, Heiner Kallweit, Frederick Lawler, linux-pci, Rafael J. Wysocki, Mika Westerberg, Wong Vee Khee, Hui Chun Ong, Keith Busch, Linux Kernel Mailing List 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@kernel.org> 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@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. thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v7 4/5] PCI/ASPM: Add sysfs attributes for controlling ASPM link states 2019-11-21 21:10 ` Greg KH @ 2019-11-21 23:04 ` Bjorn Helgaas 2019-11-24 17:02 ` Greg KH 0 siblings, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2019-11-21 23:04 UTC (permalink / raw) To: Greg KH Cc: Rajat Jain, Heiner Kallweit, Frederick Lawler, linux-pci, Rafael J. Wysocki, Mika Westerberg, Keith Busch, Linux Kernel Mailing List [-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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v7 4/5] PCI/ASPM: Add sysfs attributes for controlling ASPM link states 2019-11-21 23:04 ` Bjorn Helgaas @ 2019-11-24 17:02 ` Greg KH 0 siblings, 0 replies; 5+ messages in thread From: Greg KH @ 2019-11-24 17:02 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rajat Jain, Heiner Kallweit, Frederick Lawler, linux-pci, Rafael J. Wysocki, Mika Westerberg, Keith Busch, Linux Kernel Mailing List On Thu, Nov 21, 2019 at 05:04:11PM -0600, Bjorn Helgaas wrote: > 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. Any normal tool that looks at sysfs attributes, like udev and friends. They will "miss" the uevent for the subdirs and not know how to associate anything with the "parent" struct device. > 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> Either is fine, but yes, the second one is nicer. thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-11-24 17:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <b1c83f8a-9bf6-eac5-82d0-cf5b90128fbf@gmail.com>
2019-11-21 20:49 ` [PATCH v7 4/5] PCI/ASPM: Add sysfs attributes for controlling ASPM link states Bjorn Helgaas
2019-11-21 21:03 ` Rajat Jain
2019-11-21 21:10 ` Greg KH
2019-11-21 23:04 ` Bjorn Helgaas
2019-11-24 17:02 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).