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

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

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
Enable all ClockPM and ASPM states for devicetree platforms") enabled ASPM
L0s, L1, and (if advertised) L1 PM Substates.

df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
(v6.18-rc3) backed off and omitted Clock PM and L1 Substates because we
don't have good infrastructure to discover CLKREQ# support, and L1
Substates may require device-specific configuration.

L0s and L1 are generically discoverable and should not require
device-specific support, but some devices advertise them even though they
don't work correctly.  This series is a way to add quirks avoid L0s and L1
in this case.


Bjorn Helgaas (4):
  PCI/ASPM: Cache L0s/L1 Supported so advertised link states can be
    overridden
  PCI/ASPM: Add pcie_aspm_remove_cap() to override advertised link
    states
  PCI/ASPM: Convert quirks to override advertised link states
  PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports

 drivers/pci/pci.h       |  2 ++
 drivers/pci/pcie/aspm.c | 25 +++++++++++++++++--------
 drivers/pci/probe.c     |  7 +++++++
 drivers/pci/quirks.c    | 38 +++++++++++++++++++-------------------
 include/linux/pci.h     |  2 ++
 5 files changed, 47 insertions(+), 27 deletions(-)

-- 

v1: https://lore.kernel.org/r/20251106183643.1963801-1-helgaas@kernel.org

Changes between v1 and v2:
- Cache just the two bits for L0s and L1 support, not the entire Link
  Capabilities (Lukas)
- Add pcie_aspm_remove_cap() to override the ASPM Support bits in Link
  Capabilities (Lukas)
- Convert existing quirks to use pcie_aspm_remove_cap() instead of
  pci_disable_link_state(), and from FINAL to HEADER (Mani)

[PATCH v2 1/4] PCI/ASPM: Cache L0s/L1 Supported so advertised link states can be overridden

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Defective devices sometimes advertise support for ASPM L0s or L1 states
even if they don't work correctly.

Cache the L0s Supported and L1 Supported bits early in enumeration so
HEADER quirks can override the ASPM states advertised in Link Capabilities
before pcie_aspm_cap_init() enables ASPM.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 12 ++++--------
 drivers/pci/probe.c     |  7 +++++++
 include/linux/pci.h     |  2 ++
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 7cc8281e7011..15d50c089070 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -830,7 +830,6 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
-	u32 parent_lnkcap, child_lnkcap;
 	u16 parent_lnkctl, child_lnkctl;
 	struct pci_bus *linkbus = parent->subordinate;
 
@@ -845,9 +844,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * If ASPM not supported, don't mess with the clocks and link,
 	 * bail out now.
 	 */
-	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
-	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
-	if (!(parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPMS))
+	if (!(parent->aspm_l0s_support && child->aspm_l0s_support) &&
+	    !(parent->aspm_l1_support && child->aspm_l1_support))
 		return;
 
 	/* Configure common clock before checking latencies */
@@ -859,8 +857,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * read-only Link Capabilities may change depending on common clock
 	 * configuration (PCIe r5.0, sec 7.5.3.6).
 	 */
-	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
-	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
 	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &parent_lnkctl);
 	pcie_capability_read_word(child, PCI_EXP_LNKCTL, &child_lnkctl);
 
@@ -880,7 +876,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * given link unless components on both sides of the link each
 	 * support L0s.
 	 */
-	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
+	if (parent->aspm_l0s_support && child->aspm_l0s_support)
 		link->aspm_support |= PCIE_LINK_STATE_L0S;
 
 	if (child_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
@@ -889,7 +885,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		link->aspm_enabled |= PCIE_LINK_STATE_L0S_DW;
 
 	/* Setup L1 state */
-	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
+	if (parent->aspm_l1_support && child->aspm_l1_support)
 		link->aspm_support |= PCIE_LINK_STATE_L1;
 
 	if (parent_lnkctl & child_lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c83e75a0ec12..de72ceaea285 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1663,6 +1663,13 @@ void set_pcie_port_type(struct pci_dev *pdev)
 	if (reg32 & PCI_EXP_LNKCAP_DLLLARC)
 		pdev->link_active_reporting = 1;
 
+#ifdef CONFIG_PCIEASPM
+	if (reg32 & PCI_EXP_LNKCAP_ASPM_L0S)
+		pdev->aspm_l0s_support = 1;
+	if (reg32 & PCI_EXP_LNKCAP_ASPM_L1)
+		pdev->aspm_l1_support = 1;
+#endif
+
 	parent = pci_upstream_bridge(pdev);
 	if (!parent)
 		return;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d1fdf81fbe1e..bf97d49c23cf 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -412,6 +412,8 @@ struct pci_dev {
 	u16		l1ss;		/* L1SS Capability pointer */
 #ifdef CONFIG_PCIEASPM
 	struct pcie_link_state	*link_state;	/* ASPM link state */
+	unsigned int	aspm_l0s_support:1;	/* ASPM L0s support */
+	unsigned int	aspm_l1_support:1;	/* ASPM L1 support */
 	unsigned int	ltr_path:1;	/* Latency Tolerance Reporting
 					   supported from root to here */
 #endif
-- 
2.43.0

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

[Bjorn Helgaas]

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

[PATCH v2 3/4] PCI/ASPM: Convert quirks to override advertised link states

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Existing quirks to disable ASPM L0s and L1 use pci_disable_link_state(),
which disables ASPM states and prevents their use in the future.  But since
they are FINAL quirks, they happen after ASPM has already been enabled.
Here's a typical call path:

  pci_host_probe
    pci_scan_root_bus_bridge
      pci_scan_child_bus
        pci_scan_slot
          pci_scan_single_device
            pci_device_add
              pci_fixup_device(pci_fixup_header)  # HEADER quirks
          pcie_aspm_init_link_state
            pcie_config_aspm_path
              pcie_config_aspm_link
                pcie_config_aspm_dev              # ASPM may be enabled
    pci_bus_add_devices
      pci_bus_add_devices
        pci_fixup_device(pci_fixup_final)         # FINAL quirks
          quirk_disable_aspm_l0s
            pci_disable_link_state(dev, PCIE_LINK_STATE_L0S)

Sometimes enabling ASPM can make the link non-functional, so if we know
ASPM is broken on a device, we shouldn't enable it at all, even
temporarily.

Convert the existing quirks to use pcie_aspm_remove_cap() instead, which
overrides the ASPM Support advertised in PCIe Link Capabilities, and make
them HEADER quirks so they run before pcie_aspm_init_link_state() has a
chance to enable ASPM.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/quirks.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 214ed060ca1b..922c77c627a1 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2494,28 +2494,27 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
  */
 static void quirk_disable_aspm_l0s(struct pci_dev *dev)
 {
-	pci_info(dev, "Disabling L0s\n");
-	pci_disable_link_state(dev, PCIE_LINK_STATE_L0S);
+	pcie_aspm_remove_cap(dev, PCI_EXP_LNKCAP_ASPM_L0S);
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10a7, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10a9, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10b6, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10c6, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10c7, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10c8, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10d6, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10db, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10dd, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10e1, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10ec, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f1, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f4, quirk_disable_aspm_l0s);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1508, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10a7, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10a9, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10b6, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10c6, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10c7, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10c8, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10d6, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10db, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10dd, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10e1, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10ec, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10f1, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x10f4, quirk_disable_aspm_l0s);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1508, quirk_disable_aspm_l0s);
 
 static void quirk_disable_aspm_l0s_l1(struct pci_dev *dev)
 {
-	pci_info(dev, "Disabling ASPM L0s/L1\n");
-	pci_disable_link_state(dev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
+	pcie_aspm_remove_cap(dev,
+			     PCI_EXP_LNKCAP_ASPM_L0S | PCI_EXP_LNKCAP_ASPM_L1);
 }
 
 /*
@@ -2523,7 +2522,7 @@ static void quirk_disable_aspm_l0s_l1(struct pci_dev *dev)
  * upstream PCIe root port when ASPM is enabled. At least L0s mode is affected;
  * disable both L0s and L1 for now to be safe.
  */
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ASMEDIA, 0x1080, quirk_disable_aspm_l0s_l1);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ASMEDIA, 0x1080, quirk_disable_aspm_l0s_l1);
 
 /*
  * Some Pericom PCIe-to-PCI bridges in reverse mode need the PCIe Retrain
-- 
2.43.0

[PATCH v2 4/4] PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Christian reported that f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and
ASPM states for devicetree platforms") broke booting on the A-EON X5000.

Override the L0s and L1 Support advertised in Link Capabilities by the
X5000 Root Ports ([1957:0451]) so we don't try to enable those states.

Fixes: f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree platforms")
Fixes: df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Link: https://lore.kernel.org/r/db5c95a1-cf3e-46f9-8045-a1b04908051a@xenosoft.de
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/quirks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 922c77c627a1..b94264cd3833 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2523,6 +2523,7 @@ static void quirk_disable_aspm_l0s_l1(struct pci_dev *dev)
  * disable both L0s and L1 for now to be safe.
  */
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ASMEDIA, 0x1080, quirk_disable_aspm_l0s_l1);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, 0x0451, quirk_disable_aspm_l0s_l1);
 
 /*
  * Some Pericom PCIe-to-PCI bridges in reverse mode need the PCIe Retrain
-- 
2.43.0

Re: [PATCH v2 0/4] PCI/ASPM: Allow quirks to avoid L0s and L1

[Shawn Lin]

在 2025/11/11 星期二 6:22, Bjorn Helgaas 写道:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
> Enable all ClockPM and ASPM states for devicetree platforms") enabled ASPM
> L0s, L1, and (if advertised) L1 PM Substates.
> 
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> (v6.18-rc3) backed off and omitted Clock PM and L1 Substates because we
> don't have good infrastructure to discover CLKREQ# support, and L1
> Substates may require device-specific configuration.
> 
> L0s and L1 are generically discoverable and should not require
> device-specific support, but some devices advertise them even though they
> don't work correctly.  This series is a way to add quirks avoid L0s and L1
> in this case.
> 

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>

> 
> Bjorn Helgaas (4):
>    PCI/ASPM: Cache L0s/L1 Supported so advertised link states can be
>      overridden
>    PCI/ASPM: Add pcie_aspm_remove_cap() to override advertised link
>      states
>    PCI/ASPM: Convert quirks to override advertised link states
>    PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports
> 
>   drivers/pci/pci.h       |  2 ++
>   drivers/pci/pcie/aspm.c | 25 +++++++++++++++++--------
>   drivers/pci/probe.c     |  7 +++++++
>   drivers/pci/quirks.c    | 38 +++++++++++++++++++-------------------
>   include/linux/pci.h     |  2 ++
>   5 files changed, 47 insertions(+), 27 deletions(-)
> 

Re: [PATCH v2 0/4] PCI/ASPM: Allow quirks to avoid L0s and L1

[Lukas Wunner]

On Mon, Nov 10, 2025 at 04:22:24PM -0600, Bjorn Helgaas wrote:
> We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
> Enable all ClockPM and ASPM states for devicetree platforms") enabled ASPM
> L0s, L1, and (if advertised) L1 PM Substates.
> 
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> (v6.18-rc3) backed off and omitted Clock PM and L1 Substates because we
> don't have good infrastructure to discover CLKREQ# support, and L1
> Substates may require device-specific configuration.
> 
> L0s and L1 are generically discoverable and should not require
> device-specific support, but some devices advertise them even though they
> don't work correctly.  This series is a way to add quirks avoid L0s and L1
> in this case.

Reviewed-by: Lukas Wunner <lukas@wunner.de>

I note that a number of drivers call pci_disable_link_state() or
pci_disable_link_state_locked() to disable ASPM on probe.
Can we convert (all of) these to quirks which use the new helper
introduced here?

I think that would be useful because it would disable ASPM even if
the driver isn't available and thus avoid e.g. AER messages caused
by ASPM issues.

pcie_aspm_init_link_state() also contains the following code comment:

	/*
	 * At this stage drivers haven't had an opportunity to change the
	 * link policy setting. Enabling ASPM on broken hardware can cripple
	 * it even before the driver has had a chance to disable ASPM, so
	 * default to a safe level right now. If we're enabling ASPM beyond
	 * the BIOS's expectation, we'll do so once pci_enable_device() is
	 * called.
	 */

If we'd mask out incorrect or non-working L0s/L1 capabilities for all
devices early during enumeration via quirks, we wouldn't have to go
through these contortions of setting up deeper ASPM states only at
device enable time.

Thanks,

Lukas

Re: [PATCH v2 0/4] PCI/ASPM: Allow quirks to avoid L0s and L1

[Bjorn Helgaas]

On Tue, Nov 11, 2025 at 10:33:43AM +0100, Lukas Wunner wrote:
> On Mon, Nov 10, 2025 at 04:22:24PM -0600, Bjorn Helgaas wrote:
> > We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
> > Enable all ClockPM and ASPM states for devicetree platforms") enabled ASPM
> > L0s, L1, and (if advertised) L1 PM Substates.
> > 
> > df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> > (v6.18-rc3) backed off and omitted Clock PM and L1 Substates because we
> > don't have good infrastructure to discover CLKREQ# support, and L1
> > Substates may require device-specific configuration.
> > 
> > L0s and L1 are generically discoverable and should not require
> > device-specific support, but some devices advertise them even though they
> > don't work correctly.  This series is a way to add quirks avoid L0s and L1
> > in this case.
> 
> Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thanks!

> I note that a number of drivers call pci_disable_link_state() or
> pci_disable_link_state_locked() to disable ASPM on probe.
> Can we convert (all of) these to quirks which use the new helper
> introduced here?
> 
> I think that would be useful because it would disable ASPM even if
> the driver isn't available and thus avoid e.g. AER messages caused
> by ASPM issues.
> 
> pcie_aspm_init_link_state() also contains the following code comment:
> 
> 	/*
> 	 * At this stage drivers haven't had an opportunity to change the
> 	 * link policy setting. Enabling ASPM on broken hardware can cripple
> 	 * it even before the driver has had a chance to disable ASPM, so
> 	 * default to a safe level right now. If we're enabling ASPM beyond
> 	 * the BIOS's expectation, we'll do so once pci_enable_device() is
> 	 * called.
> 	 */
> 
> If we'd mask out incorrect or non-working L0s/L1 capabilities for all
> devices early during enumeration via quirks, we wouldn't have to go
> through these contortions of setting up deeper ASPM states only at
> device enable time.

I definitely agree.  I forgot to follow up on all of those cases.
There aren't that many of them, but it looks like probably too many to
address for v6.18, and I *think* it's safe to wait and deal with them
for v6.19.

Bjorn

Re: [PATCH v2 0/4] PCI/ASPM: Allow quirks to avoid L0s and L1

[Bjorn Helgaas]

On Mon, Nov 10, 2025 at 04:22:24PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
> Enable all ClockPM and ASPM states for devicetree platforms") enabled ASPM
> L0s, L1, and (if advertised) L1 PM Substates.
> 
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> (v6.18-rc3) backed off and omitted Clock PM and L1 Substates because we
> don't have good infrastructure to discover CLKREQ# support, and L1
> Substates may require device-specific configuration.
> 
> L0s and L1 are generically discoverable and should not require
> device-specific support, but some devices advertise them even though they
> don't work correctly.  This series is a way to add quirks avoid L0s and L1
> in this case.
> 
> 
> Bjorn Helgaas (4):
>   PCI/ASPM: Cache L0s/L1 Supported so advertised link states can be
>     overridden
>   PCI/ASPM: Add pcie_aspm_remove_cap() to override advertised link
>     states
>   PCI/ASPM: Convert quirks to override advertised link states
>   PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports
> 
>  drivers/pci/pci.h       |  2 ++
>  drivers/pci/pcie/aspm.c | 25 +++++++++++++++++--------
>  drivers/pci/probe.c     |  7 +++++++
>  drivers/pci/quirks.c    | 38 +++++++++++++++++++-------------------
>  include/linux/pci.h     |  2 ++
>  5 files changed, 47 insertions(+), 27 deletions(-)

Applied to pci/for-linus, hoping for v6.18.  Thanks Shawn and Lukas
for testing and reviewing.  Any other comments and testing would be
very welcome.

I think we'll need to add a similar quirk for Christian's X1000
(https://lore.kernel.org/r/a41d2ca1-fcd9-c416-b111-a958e92e94bf@xenosoft.de),
but I don't know the device ID for it yet.

> -- 
> 
> v1: https://lore.kernel.org/r/20251106183643.1963801-1-helgaas@kernel.org
> 
> Changes between v1 and v2:
> - Cache just the two bits for L0s and L1 support, not the entire Link
>   Capabilities (Lukas)
> - Add pcie_aspm_remove_cap() to override the ASPM Support bits in Link
>   Capabilities (Lukas)
> - Convert existing quirks to use pcie_aspm_remove_cap() instead of
>   pci_disable_link_state(), and from FINAL to HEADER (Mani)

Re: PCI/ASPM: Allow quirks to avoid L0s and L1

[Roland]

Hello all!

I started to get to my email these discussions about Linux a few days ago. I have not subscribed any mailing list, or given anyone permission to include my name on such list!

So, if there is a moderator on this list, could you PLEASE remove immediately my email (rol7and@gmx.com) from the recipients/cc list? Thank you!

If there is no moderator, could the participants then please take care to remove my email from their messages? Thank you!

Regards,

Roland



> On Mon, Nov 10, 2025 at 04:22:24PM -0600, Bjorn Helgaas wrote:
>> From: Bjorn Helgaas <bhelgaas@google.com>
>> 
>> We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
>> Enable all ClockPM and ASPM states for devicetree platforms") enabled
>> ASPM L0s, L1, and (if advertised) L1 PM Substates.
>> 
>> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree
>> platforms") (v6.18-rc3) backed off and omitted Clock PM and L1 Substates
>> because we don't have good infrastructure to discover CLKREQ# support,
>> and L1 Substates may require device-specific configuration.
>> 
>> L0s and L1 are generically discoverable and should not require
>> device-specific support, but some devices advertise them even though
>> they don't work correctly.  This series is a way to add quirks avoid L0s
>> and L1 in this case.
>> 
>> 
>> Bjorn Helgaas (4):
>>   PCI/ASPM: Cache L0s/L1 Supported so advertised link states can be
>>     overridden
>>   PCI/ASPM: Add pcie_aspm_remove_cap() to override advertised link
>>     states
>>   PCI/ASPM: Convert quirks to override advertised link states
>>   PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports
>> 
>>  drivers/pci/pci.h       |  2 ++
>>  drivers/pci/pcie/aspm.c | 25 +++++++++++++++++--------
>>  drivers/pci/probe.c     |  7 +++++++
>>  drivers/pci/quirks.c    | 38 +++++++++++++++++++-------------------
>>  include/linux/pci.h     |  2 ++
>>  5 files changed, 47 insertions(+), 27 deletions(-)
>
> Applied to pci/for-linus, hoping for v6.18.  Thanks Shawn and Lukas
> for testing and reviewing.  Any other comments and testing would be
> very welcome.
>
> I think we'll need to add a similar quirk for Christian's X1000
> (https://lore.kernel.org/r/a41d2ca1-fcd9-c416-b111-a958e92e94bf@xenosoft.de),
> but I don't know the device ID for it yet.
>
>> -- 
>> 
>> v1:
>> https://lore.kernel.org/r/20251106183643.1963801-1-helgaas@kernel.org
>
>> Changes between v1 and v2:
>> - Cache just the two bits for L0s and L1 support, not the entire Link
>>   Capabilities (Lukas)
>> - Add pcie_aspm_remove_cap() to override the ASPM Support bits in Link
>>   Capabilities (Lukas)
>> - Convert existing quirks to use pcie_aspm_remove_cap() instead of
>>   pci_disable_link_state(), and from FINAL to HEADER (Mani)
Regards

Re: [PATCH v2 0/4] PCI/ASPM: Allow quirks to avoid L0s and L1

[Lukas Wunner]

On Tue, Nov 11, 2025 at 09:44:45AM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 11, 2025 at 10:33:43AM +0100, Lukas Wunner wrote:
> > I note that a number of drivers call pci_disable_link_state() or
> > pci_disable_link_state_locked() to disable ASPM on probe.
> > Can we convert (all of) these to quirks which use the new helper
> > introduced here?
[...]
> I definitely agree.  I forgot to follow up on all of those cases.
> There aren't that many of them, but it looks like probably too many to
> address for v6.18, and I *think* it's safe to wait and deal with them
> for v6.19.

Yes I agree this isn't necessary for v6.18.

It may even be too late for v6.19 given the amount of time to come up
with patches, get them reviewed and allow sufficient time to soak in
linux-next.  And this would be a cleanup, so not really urgent.

I note that a lot of material has queued up in patchwork and only few
new features have been applied to pci.git this cycle, which I guess
has been caused by the unusual number of regressions introduced
during the merge window.  Given that, I'd down-prioritize conversion
of pci_disable_link_state() calls.

One feature I'd be keen to get in for v6.19 (if time permits) is this one:

https://lore.kernel.org/all/cover.1760274044.git.lukas@wunner.de/

I'm worried that the "pci_save_state() after pci_restore_state()"
anti-pattern gets cargo-culted to more drivers if it's not removed soon.
I can split patch [2/2] in that series into smaller patches and funnel
them through the individual subsystem trees if you'd rather not apply
them wholesale in one large treewide patch.

Thanks,

Lukas

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

[Manivannan Sadhasivam]

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

[Bjorn Helgaas]

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

[Maciej W. Rozycki]

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