[v2] PCI/ASPM: Allow quirks to avoid L0s and L1

[PATCH v2 2/4] PCI/ASPM: Add pcie_aspm_remove_cap() to override advertised link states

Posted by Bjorn Helgaas 1 month, 1 week ago

From: Bjorn Helgaas <bhelgaas@google.com>

Add pcie_aspm_remove_cap().  A quirk can use this to prevent use of ASPM
L0s or L1 link states, even if the device advertised support for them.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.h       |  2 ++
 drivers/pci/pcie/aspm.c | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4492b809094b..36f8c0985430 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -958,6 +958,7 @@ void pci_save_aspm_l1ss_state(struct pci_dev *dev);
 void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
 
 #ifdef CONFIG_PCIEASPM
+void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap);
 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, bool locked);
@@ -965,6 +966,7 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
 void pci_configure_ltr(struct pci_dev *pdev);
 void pci_bridge_reconfigure_ltr(struct pci_dev *pdev);
 #else
+static inline void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap) { }
 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, bool locked) { }
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 15d50c089070..bc3cb8bc7018 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1542,6 +1542,19 @@ int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
 }
 EXPORT_SYMBOL(pci_enable_link_state_locked);
 
+void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap)
+{
+	if (lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
+		pdev->aspm_l0s_support = 0;
+	if (lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
+		pdev->aspm_l1_support = 0;
+
+	pci_info(pdev, "ASPM:%s%s removed from Link Capabilities to avoid device defect\n",
+		 lnkcap & PCI_EXP_LNKCAP_ASPM_L0S ? " L0s" : "",
+		 lnkcap & PCI_EXP_LNKCAP_ASPM_L1 ? " L1" : "");
+
+}
+
 static int pcie_aspm_set_policy(const char *val,
 				const struct kernel_param *kp)
 {
-- 
2.43.0

Re: [PATCH v2 2/4] PCI/ASPM: Add pcie_aspm_remove_cap() to override advertised link states

Posted by Manivannan Sadhasivam 1 month ago

On Mon, Nov 10, 2025 at 04:22:26PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Add pcie_aspm_remove_cap().  A quirk can use this to prevent use of ASPM
> L0s or L1 link states, even if the device advertised support for them.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci.h       |  2 ++
>  drivers/pci/pcie/aspm.c | 13 +++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 4492b809094b..36f8c0985430 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -958,6 +958,7 @@ void pci_save_aspm_l1ss_state(struct pci_dev *dev);
>  void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
>  
>  #ifdef CONFIG_PCIEASPM
> +void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap);
>  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, bool locked);
> @@ -965,6 +966,7 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
>  void pci_configure_ltr(struct pci_dev *pdev);
>  void pci_bridge_reconfigure_ltr(struct pci_dev *pdev);
>  #else
> +static inline void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap) { }
>  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, bool locked) { }
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 15d50c089070..bc3cb8bc7018 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1542,6 +1542,19 @@ int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
>  }
>  EXPORT_SYMBOL(pci_enable_link_state_locked);
>  
> +void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap)
> +{
> +	if (lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
> +		pdev->aspm_l0s_support = 0;
> +	if (lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
> +		pdev->aspm_l1_support = 0;
> +
> +	pci_info(pdev, "ASPM:%s%s removed from Link Capabilities to avoid device defect\n",
> +		 lnkcap & PCI_EXP_LNKCAP_ASPM_L0S ? " L0s" : "",
> +		 lnkcap & PCI_EXP_LNKCAP_ASPM_L1 ? " L1" : "");

I think this gives a false impression that the ASPM CAPs are being removed from
the LnkCap register. This function is just removing it from the internal cache
and the LnkCap register is left unchanged.

IMO, either we need to disable relevant CAPs in LnkCap register also or change
the log in this and quirks patches.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 2/4] PCI/ASPM: Add pcie_aspm_remove_cap() to override advertised link states

Posted by Bjorn Helgaas 1 month ago

[-cc Roland]

On Wed, Nov 12, 2025 at 10:57:07PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Nov 10, 2025 at 04:22:26PM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Add pcie_aspm_remove_cap().  A quirk can use this to prevent use of ASPM
> > L0s or L1 link states, even if the device advertised support for them.

> > +void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap)
> > +{
> > +	if (lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
> > +		pdev->aspm_l0s_support = 0;
> > +	if (lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
> > +		pdev->aspm_l1_support = 0;
> > +
> > +	pci_info(pdev, "ASPM:%s%s removed from Link Capabilities to avoid device defect\n",
> > +		 lnkcap & PCI_EXP_LNKCAP_ASPM_L0S ? " L0s" : "",
> > +		 lnkcap & PCI_EXP_LNKCAP_ASPM_L1 ? " L1" : "");
> 
> I think this gives a false impression that the ASPM CAPs are being
> removed from the LnkCap register. This function is just removing it
> from the internal cache and the LnkCap register is left unchanged.

Very true, this is confusing since we're not actually changing the
LnkCap register, so lspci etc will still show these states as
supported.  The quirk needs to work for arbitrary devices, and there's
no generic way to change LnkCap, so the quirk can't do that.

Any ideas for better wording?  I don't like "disable" because that
suggests that we're clearing bits in LnkCtl.

"L0s L1 in Link Capabilities will be ignored ..."?

"ignoring Link Capabilities L0s L1 ..."?

"L0s L1 treated as unsupported ..."?

Re: [PATCH v2 2/4] PCI/ASPM: Add pcie_aspm_remove_cap() to override advertised link states

Posted by Maciej W. Rozycki 1 month ago

On Wed, 12 Nov 2025, Bjorn Helgaas wrote:

> > > +	pci_info(pdev, "ASPM:%s%s removed from Link Capabilities to avoid device defect\n",
> > > +		 lnkcap & PCI_EXP_LNKCAP_ASPM_L0S ? " L0s" : "",
> > > +		 lnkcap & PCI_EXP_LNKCAP_ASPM_L1 ? " L1" : "");
> > 
> > I think this gives a false impression that the ASPM CAPs are being
> > removed from the LnkCap register. This function is just removing it
> > from the internal cache and the LnkCap register is left unchanged.
> 
> Very true, this is confusing since we're not actually changing the
> LnkCap register, so lspci etc will still show these states as
> supported.  The quirk needs to work for arbitrary devices, and there's
> no generic way to change LnkCap, so the quirk can't do that.

 There's no way to poke at hw, but that is only relevant for x86 I believe 
and not the default access method for `lspci' anyway.  For sysfs we do it 
already for things such as fixing the device class; cf. `quirk_isa_bridge'
(arch/alpha/kernel/pci.c), so why is it a problem here?  Unless we want to 
keep it for `lspci' actually.

  Maciej