Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change() - Kai-Heng Feng

From: Kai-Heng Feng <kai.heng.feng@canonical.com>
To: bhelgaas@google.com
Cc: "Saheed O. Bolarinwa" <refactormyself@gmail.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Logan Gunthorpe" <logang@deltatee.com>,
	"Rajat Jain" <rajatja@google.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()
Date: Tue, 21 Jun 2022 10:27:56 +0800	[thread overview]
Message-ID: <CAAd53p4NZ1Pd0TteMm0=Pcd2s-F+f7--tkiUNQqMRm+NgZtuSQ@mail.gmail.com> (raw)
In-Reply-To: <20220509073639.2048236-1-kai.heng.feng@canonical.com>

On Mon, May 9, 2022 at 3:37 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> 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@nvidia.com/
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

A gentle ping...

> ---
>  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
>