Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

From: Bjorn Helgaas
Date: Mon Jul 11 2022 - 19:11:29 EST


On Mon, May 09, 2022 at 03:36:37PM +0800, Kai-Heng Feng wrote:
> pcie_aspm_pm_state_change() was introduced at the inception of PCIe
> ASPM code.
>
> However, it can cause some issues. For instance, when ASPM config is
> changed via sysfs, those changes won't persist across power state change
> because pcie_aspm_pm_state_change() overwrites them.
>
> In addition to that, if the driver is to restore L1ss [1] after system
> resume, the restored states will also be overwritten by
> pcie_aspm_pm_state_change().
>
> So remove pcie_aspm_pm_state_change() for now, if there's any hardware
> really needs it to function, a quirk can be used instead.
>
> [1] https://lore.kernel.org/linux-pci/20220201123536.12962-1-vidyas@xxxxxxxxxx/
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>

Applied to pci/aspm for v5.20, thanks!

> ---
> drivers/pci/pci.c | 3 ---
> drivers/pci/pci.h | 2 --
> drivers/pci/pcie/aspm.c | 19 -------------------
> 3 files changed, 24 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f1..d09f7b60ee4dc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1181,9 +1181,6 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
> if (need_restore)
> pci_restore_bars(dev);
>
> - if (dev->bus->self)
> - pcie_aspm_pm_state_change(dev->bus->self);
> -
> return 0;
> }
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3d60cabde1a15..86a19f293d4ad 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -560,12 +560,10 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
> #ifdef CONFIG_PCIEASPM
> void pcie_aspm_init_link_state(struct pci_dev *pdev);
> void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> -void pcie_aspm_pm_state_change(struct pci_dev *pdev);
> void pcie_aspm_powersave_config_link(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
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b7424c9bc8..7f76a5875feb4 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1012,25 +1012,6 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
> up_read(&pci_bus_sem);
> }
>
> -/* @pdev: the root port or switch downstream port */
> -void pcie_aspm_pm_state_change(struct pci_dev *pdev)
> -{
> - struct pcie_link_state *link = pdev->link_state;
> -
> - if (aspm_disabled || !link)
> - return;
> - /*
> - * Devices changed PM state, we should recheck if latency
> - * meets all functions' requirement
> - */
> - down_read(&pci_bus_sem);
> - mutex_lock(&aspm_lock);
> - pcie_update_aspm_capable(link->root);
> - pcie_config_aspm_path(link);
> - mutex_unlock(&aspm_lock);
> - up_read(&pci_bus_sem);
> -}
> -
> void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
> {
> struct pcie_link_state *link = pdev->link_state;
> --
> 2.34.1
>