linux-kernel - Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220711231117.GA706272@bhelgaas>
Date:   Mon, 11 Jul 2022 18:11:17 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>
Cc:     bhelgaas@...gle.com,
        "Saheed O. Bolarinwa" <refactormyself@...il.com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Logan Gunthorpe <logang@...tatee.com>,
        Rajat Jain <rajatja@...gle.com>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

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@nvidia.com/
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>

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
>