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

[Bjorn Helgaas <helgaas@kernel.org>]

On Sat, Apr 13, 2019 at 11:12:41AM +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 disabled.

> +void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
> +void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
>  #else
>  static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
> -#endif
> -
> -#ifdef CONFIG_PCIEASPM_DEBUG
> -void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
> -void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
> -#else

I like the idea of exposing these sysfs control files all the time,
instead of only when CONFIG_PCIEASPM_DEBUG=y, but I think when we do
that, we should put the files at the downstream end of the link (e.g.,
an endpoint) instead of at the upstream end (e.g., a root port or
switch downstream port).  We had some conversation about this here:

https://lore.kernel.org/lkml/20180727202619.GD173328@bhelgaas-glaptop.roam.corp.google.com

Doing it at the downstream end would require more changes, of course,
and probably raises some locking issues, but I think we have a small
window of opportunity here where we can tweak the sysfs structure
before we're committed to supporting something forever.

Bjorn