[PATCH v6 0/3] PCI/ASPM: Mask ASPM states based on Devicetree properties

[PATCH v6 0/3] PCI/ASPM: Mask ASPM states based on Devicetree properties

[Krishna Chaitanya Chundru]

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
Changes in v6:
- Disable/enable each function L0s/L1. (sashiko)
- Move from pcie_capability_write_word to
  pcie_capability_clear_and_set_word (Sashiko)
- Link to v5: https://patch.msgid.link/20260721-aspm-v5-1-58860c48d0c7@oss.qualcomm.com

Changes in v5:
- update all the child devices when enabling the ASPM back (sashiko).
- Link to v4: https://patch.msgid.link/20260707-aspm-v4-1-d66f85b728b0@oss.qualcomm.com

Changes in v4:
- Clear link->aspm_enabled alongside aspm_support/aspm_default when an
  ASPM state is masked, so it no longer goes stale relative to hardware (sashiko).
- Mask the LNKCTL restore snapshot against the post-override
  aspm_support so the restore step can no longer re-enable a state the
  Devicetree override just disabled. (sashiko)
- Link to v3: https://patch.msgid.link/20260704-aspm-v3-1-157217aff76f@oss.qualcomm.com

Changes in v3:
- Move pcie_aspm_override_default_link_state() call in pcie_aspm_cap_init() to
  before the "Restore L0s/L1" block. pcie_aspm_cap_init() disables L1 in
  hardware prior to aspm_l1ss_init() and re-enables it only in the
  restore block. Calling pcie_config_aspm_l1ss() while L1 is already
  disabled satisfies its precondition ("Caller must disable L1 first"),
  whereas the previous placement after the restore violated it (sashiko).
- Link to v2: https://patch.msgid.link/20260624-aspm-v2-1-800a4151ba3a@oss.qualcomm.com

Changes in v2:
- Disable L1ss when L1 is disabled as pointed by sashiko.
- Disable L1ss if bootloader enables them but we are disabling via
  devicetree pointed by sashiko.
- Link to v1: https://patch.msgid.link/20260511-aspm-v1-1-b4a9fe955cf9@oss.qualcomm.com

---
Krishna Chaitanya Chundru (3):
      PCI/ASPM: Use pcie_capability_clear_and_set_word() for ASPM disable/restore
      PCI/ASPM: Disable/restore ASPM on every function for multi-function devices
      PCI/ASPM: Mask ASPM states based on Devicetree properties

 drivers/pci/pcie/aspm.c | 149 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 102 insertions(+), 47 deletions(-)
---
base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
change-id: 20260429-aspm-da55a54279e4

Best regards,
--  
Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

[PATCH v6 1/3] PCI/ASPM: Use pcie_capability_clear_and_set_word() for ASPM disable/restore

[Krishna Chaitanya Chundru]

pcie_aspm_cap_init() disables ASPM L0s/L1 on both ends of the Link
before touching L1SS config, then later restores the LNKCTL state
that was in effect beforehand. Both steps use raw
pcie_capability_write_word() calls: the disable step computes the
new value by hand from a snapshot taken earlier in the function, and
the restore step writes that same snapshot straight back.

Switch both steps to pcie_capability_clear_and_set_word(), masked to
PCI_EXP_LNKCTL_ASPMC, matching the accessor pcie_config_aspm_dev()
already uses elsewhere in this file for the exact same register. This
does a live read-modify-write of just the ASPM Control bits instead of
relying on a stale snapshot for the rest of the word, and is
consistent with how the rest of the file already touches this
register. No functional change.

Fixes: 7447990137bf ("PCI/ASPM: Disable L1 before disabling L1 PM Substates")
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 172783e7f519..3f9c0f9a1cc7 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -894,10 +894,10 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	/* Disable L0s/L1 before updating L1SS config */
 	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
 	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
-		pcie_capability_write_word(child, PCI_EXP_LNKCTL,
-					   child_lnkctl & ~PCI_EXP_LNKCTL_ASPMC);
-		pcie_capability_write_word(parent, PCI_EXP_LNKCTL,
-					   parent_lnkctl & ~PCI_EXP_LNKCTL_ASPMC);
+		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
+						   PCI_EXP_LNKCTL_ASPMC, 0);
+		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
+						   PCI_EXP_LNKCTL_ASPMC, 0);
 	}
 
 	/*
@@ -927,8 +927,12 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	/* Restore L0s/L1 if they were enabled */
 	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
 	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
-		pcie_capability_write_word(parent, PCI_EXP_LNKCTL, parent_lnkctl);
-		pcie_capability_write_word(child, PCI_EXP_LNKCTL, child_lnkctl);
+		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
+						   PCI_EXP_LNKCTL_ASPMC,
+						   parent_lnkctl & PCI_EXP_LNKCTL_ASPMC);
+		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
+						   PCI_EXP_LNKCTL_ASPMC,
+						   child_lnkctl & PCI_EXP_LNKCTL_ASPMC);
 	}
 
 	/* Save default state */

-- 
2.34.1

Re: [PATCH v6 1/3] PCI/ASPM: Use pcie_capability_clear_and_set_word() for ASPM disable/restore

[sashiko-bot]

> pcie_aspm_cap_init() disables ASPM L0s/L1 on both ends of the Link
> before touching L1SS config, then later restores the LNKCTL state
> that was in effect beforehand. Both steps use raw
> pcie_capability_write_word() calls: the disable step computes the
> new value by hand from a snapshot taken earlier in the function, and
> the restore step writes that same snapshot straight back.
> 
> Switch both steps to pcie_capability_clear_and_set_word(), masked to
> PCI_EXP_LNKCTL_ASPMC, matching the accessor pcie_config_aspm_dev()
> already uses elsewhere in this file for the exact same register. This
> does a live read-modify-write of just the ASPM Control bits instead of
> relying on a stale snapshot for the rest of the word, and is
> consistent with how the rest of the file already touches this
> register. No functional change.
> 
> Fixes: 7447990137bf ("PCI/ASPM: Disable L1 before disabling L1 PM Substates")
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-aspm-v6-0-2ebb3ee7ef71@oss.qualcomm.com?part=1

[PATCH v6 2/3] PCI/ASPM: Disable/restore ASPM on every function for multi-function devices

[Krishna Chaitanya Chundru]

pcie_aspm_cap_init() disables ASPM L0s/L1 before touching L1SS config,
then restores the pre-existing state afterward. Both steps only ever
touched link->downstream, i.e. function 0 of the downstream component,
leaving sibling functions (>0) on a multi-function device untouched.

This means the "disable" step does not actually disable ASPM link-wide
on a multi-function device: a sibling function can still have L1
enabled even after this step runs. PCIe r6.2 sec 7.5.3.7 recommends
programming the same ASPM Control value for all functions of a
multi-function device, and pcie_config_aspm_link() already loops over
every function on the bus for exactly this reason.

Loop over every function on linkbus->devices for both the disable and
restore steps, keeping the existing PCIe r6.2 sec 7.5.3.7 ordering
(disable downstream functions before upstream, restore upstream before
downstream functions). The masked pcie_capability_clear_and_set_word()
accessor from the previous commit makes this safe: it only ever
touches the ASPM Control bits, so function-specific bits elsewhere in
LNKCTL (e.g. Read Completion Boundary, CLKREQ Enable) on sibling
functions are left untouched.

Fixes: 7447990137bf ("PCI/ASPM: Disable L1 before disabling L1 PM Substates")
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 3f9c0f9a1cc7..544f35bb2555 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -861,6 +861,7 @@ 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;
+	struct pci_dev *fn;
 	u16 parent_lnkctl, child_lnkctl;
 	struct pci_bus *linkbus = parent->subordinate;
 
@@ -894,8 +895,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	/* Disable L0s/L1 before updating L1SS config */
 	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
 	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
-		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
-						   PCI_EXP_LNKCTL_ASPMC, 0);
+		list_for_each_entry(fn, &linkbus->devices, bus_list)
+			pcie_capability_clear_and_set_word(fn, PCI_EXP_LNKCTL,
+							   PCI_EXP_LNKCTL_ASPMC, 0);
 		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
 						   PCI_EXP_LNKCTL_ASPMC, 0);
 	}
@@ -930,9 +932,10 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
 						   PCI_EXP_LNKCTL_ASPMC,
 						   parent_lnkctl & PCI_EXP_LNKCTL_ASPMC);
-		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
-						   PCI_EXP_LNKCTL_ASPMC,
-						   child_lnkctl & PCI_EXP_LNKCTL_ASPMC);
+		list_for_each_entry(fn, &linkbus->devices, bus_list)
+			pcie_capability_clear_and_set_word(fn, PCI_EXP_LNKCTL,
+							   PCI_EXP_LNKCTL_ASPMC,
+							   child_lnkctl & PCI_EXP_LNKCTL_ASPMC);
 	}
 
 	/* Save default state */

-- 
2.34.1

Re: [PATCH v6 2/3] PCI/ASPM: Disable/restore ASPM on every function for multi-function devices

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The conditional check guarding the ASPM disable/restore sequence evaluates only function 0, bypassing the new multi-function loop if only a sibling function has ASPM enabled.
- [High] L1 PM Substates (L1SS) configuration is only applied to function 0, violating PCIe specifications for multi-function devices.
- [High] NULL pointer dereference in `pcie_aspm_cap_init` if a PCIe link has downstream devices but no function 0.
- [High] Spec violation in `pci_restore_aspm_l1ss_state()` where ASPMC is temporarily disabled only on the parent and the active function, ignoring sibling functions.
--

--- Patch [2]: [PATCH v6 2/3] PCI/ASPM: Disable/restore ASPM on every function for multi-function devices ---
commit b996c9e712ad7161b9278ed4aa901d8d723bf24a
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

PCI/ASPM: Disable/restore ASPM on every function for multi-function devices

This patch modifies pcie_aspm_cap_init() to disable and restore ASPM L0s/L1
states across all functions of a multi-function device. It replaces the
direct configuration of only the downstream function 0 with a loop over
linkbus->devices.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 3f9c0f9a1cc7d..544f35bb25554 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -861,6 +861,7 @@ 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;
> +	struct pci_dev *fn;

[Severity: High]
This is a pre-existing issue, but is it possible for child to be NULL here and
dereferenced later in this function?

If a PCIe bus has downstream devices but lacks function 0 (for example, due
to dynamic sysfs removal, SR-IOV configurations, or device passthrough),
pci_function_0() returns NULL. This causes alloc_pcie_link_state() to set
link->downstream to NULL.

Later in pcie_aspm_cap_init(), child is unconditionally dereferenced:

pcie_aspm_cap_init() {
    ...
    if (!(parent->aspm_l0s_support && child->aspm_l0s_support) &&
        !(parent->aspm_l1_support && child->aspm_l1_support))
        return;
    ...
}

Can this lead to a NULL pointer dereference panic?

[ ... ]

> @@ -894,8 +895,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	/* Disable L0s/L1 before updating L1SS config */
>  	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
>  	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
> -		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
> -						   PCI_EXP_LNKCTL_ASPMC, 0);
> +		list_for_each_entry(fn, &linkbus->devices, bus_list)
> +			pcie_capability_clear_and_set_word(fn, PCI_EXP_LNKCTL,
> +							   PCI_EXP_LNKCTL_ASPMC, 0);

[Severity: High]
This is a pre-existing issue, but does this conditional check bypass the new
multi-function loop if only a sibling function has ASPM enabled?

The if condition evaluates only child_lnkctl (function 0) and parent_lnkctl.
If a multi-function device is initialized where function 0 and the parent port
have ASPM L0s/L1 disabled, but a sibling function has it enabled, it appears
this check evaluates to false.

This would completely bypass the newly added loop, allowing L1SS configuration
to proceed while a sibling function still has ASPM L1 enabled, violating the
PCIe spec.

>  		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
>  						   PCI_EXP_LNKCTL_ASPMC, 0);
>  	}

[ ... ]


[Severity: High]
This is a pre-existing issue, but does pcie_config_aspm_l1ss() need a similar
multi-function loop?

Looking at pcie_config_aspm_l1ss() in drivers/pci/pcie/aspm.c:

pcie_config_aspm_l1ss() {
    ...
    /* Disable all L1 substates */
    pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
                                   PCI_L1SS_CTL1_L1SS_MASK, 0);
    pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
                                   PCI_L1SS_CTL1_L1SS_MASK, 0);
    ...
}

It appears L1SS configuration is only applied to child (function 0) and the
parent. Does this leave L1 PM Substates out of sync across sibling functions,
violating the PCIe r6.2 sec 7.8.3.3 mandate for identical L1SS programming
across all functions of a multi-function device?


[Severity: High]
This is a pre-existing issue, but is there a similar multi-function ordering
violation in pci_restore_aspm_l1ss_state()?

Looking at pci_restore_aspm_l1ss_state() in drivers/pci/pcie/aspm.c:

pci_restore_aspm_l1ss_state() {
    ...
    if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, clnkctl) ||
        FIELD_GET(PCI_EXP_LNKCTL_ASPMC, plnkctl)) {
        pcie_capability_write_word(pdev, PCI_EXP_LNKCTL,
                                   clnkctl & ~PCI_EXP_LNKCTL_ASPMC);
        pcie_capability_write_word(parent, PCI_EXP_LNKCTL,
                                   plnkctl & ~PCI_EXP_LNKCTL_ASPMC);
    }
    ...
}

When called on a multi-function device for function 1, it temporarily disables
ASPMC on function 1 and immediately on the parent. If function 0 still has
ASPMC enabled, the upstream parent is disabled before the downstream function 0.

Does this violate the PCIe spec requirement to disable downstream functions
before upstream functions, potentially causing link drops during device resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-aspm-v6-0-2ebb3ee7ef71@oss.qualcomm.com?part=2

[PATCH v6 3/3] PCI/ASPM: Mask ASPM states based on Devicetree properties

[Krishna Chaitanya Chundru]

Some platforms require selectively disabling specific ASPM states on a
given PCIe link to avoid link instability or functional failures caused
by board-level connectivity constraints such as PCB routing, connectors,
slots, or external cabling.

Devicetree supports disabling ASPM L0s, L1, and L1 PM Substates via the
'aspm-no-l0s', 'aspm-no-l1' [1], and 'aspm-no-l1ss' [2] properties.
However, the ASPM driver does not currently honor these properties when
initializing the default link state.

When firmware enables L1 PM Substates before the kernel takes over,
masking aspm_support alone is insufficient to disable them in hardware.
pcie_config_aspm_link() guards L1SS configuration behind a check on
aspm_capable, which is derived from aspm_support. Once aspm_support is
masked, pcie_config_aspm_l1ss() is never called, leaving
firmware-enabled L1SS substates active in hardware.

Fix this by introducing pcie_link_has_aspm_override() to check for DT
override properties on either endpoint of the link. In
pcie_aspm_override_default_link_state(), use it to:

 - Mask aspm_support, aspm_default, and aspm_enabled for any disabled
   state, so software's view of the link stays in sync with what is
   actually programmed in hardware. Leaving aspm_enabled stale would
   make pcie_aspm_enabled() and the aspm sysfs attributes report a
   state as active even after it has been masked, and could cause
   pcie_config_aspm_link()'s "already in requested state" check to
   skip reprogramming hardware to match.
 - Explicitly call pcie_config_aspm_l1ss(link, 0) before masking
   aspm_support when firmware has L1SS active and DT requests disabling
   L1 or L1SS, since pcie_config_aspm_link() will no longer do so once
   aspm_capable is derived from the masked aspm_support.

Move the aspm_default initialization and
pcie_aspm_override_default_link_state() call in pcie_aspm_cap_init() to
before the "Restore L0s/L1" block. pcie_aspm_cap_init() disables L1 in
hardware prior to aspm_l1ss_init() and re-enables it only in the
restore block. Calling pcie_config_aspm_l1ss() while L1 is already
disabled satisfies its precondition ("Caller must disable L1 first"),
whereas the previous placement after the restore violated it.

Since the restore block writes back the parent_lnkctl/child_lnkctl
snapshot taken from hardware before the DT override ran, mask the L0s
and L1 enable bits out of that snapshot for any state the override has
just disabled in aspm_support. Otherwise the restore step would
unconditionally reprogram the link back to firmware's original L0s/L1
configuration, defeating the Devicetree override it is meant to
enforce.

Move pcie_config_aspm_l1ss() earlier in the file so it can be called
from pcie_aspm_override_default_link_state().

Link [1]: https://github.com/devicetree-org/dt-schema/pull/188
Link [2]: https://github.com/devicetree-org/dt-schema/pull/190
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 132 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 90 insertions(+), 42 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 544f35bb2555..0a671825fac8 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -839,6 +839,49 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 
 #define FLAG(x, y, d)	(((x) & (PCIE_LINK_STATE_##y)) ? d : "")
 
+/* Configure the ASPM L1 substates. Caller must disable L1 first. */
+static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
+{
+	u32 val = 0;
+	struct pci_dev *child = link->downstream, *parent = link->pdev;
+
+	if (state & PCIE_LINK_STATE_L1_1)
+		val |= PCI_L1SS_CTL1_ASPM_L1_1;
+	if (state & PCIE_LINK_STATE_L1_2)
+		val |= PCI_L1SS_CTL1_ASPM_L1_2;
+	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
+		val |= PCI_L1SS_CTL1_PCIPM_L1_1;
+	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
+		val |= PCI_L1SS_CTL1_PCIPM_L1_2;
+
+	/*
+	 * PCIe r6.2, sec 5.5.4, rules for enabling L1 PM Substates:
+	 * - Clear L1.x enable bits at child first, then at parent
+	 * - Set L1.x enable bits at parent first, then at child
+	 * - ASPM/PCIPM L1.2 must be disabled while programming timing
+	 *   parameters
+	 */
+
+	/* Disable all L1 substates */
+	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
+				       PCI_L1SS_CTL1_L1SS_MASK, 0);
+	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+				       PCI_L1SS_CTL1_L1SS_MASK, 0);
+
+	/* Enable what we need to enable */
+	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+				       PCI_L1SS_CTL1_L1SS_MASK, val);
+	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
+				       PCI_L1SS_CTL1_L1SS_MASK, val);
+}
+
+static bool pcie_link_has_aspm_override(const struct pcie_link_state *link,
+					const char *aspm)
+{
+	return (device_property_present(&link->pdev->dev, aspm) ||
+		device_property_present(&link->downstream->dev, aspm));
+}
+
 static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
 {
 	struct pci_dev *pdev = link->downstream;
@@ -846,6 +889,36 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
 
 	/* For devicetree platforms, enable L0s and L1 by default */
 	if (of_have_populated_dt()) {
+		bool no_l0s = pcie_link_has_aspm_override(link, "aspm-no-l0s");
+		bool no_l1 = pcie_link_has_aspm_override(link, "aspm-no-l1");
+		bool no_l1ss = pcie_link_has_aspm_override(link, "aspm-no-l1ss");
+
+		if (no_l0s) {
+			link->aspm_support &= ~PCIE_LINK_STATE_L0S;
+			link->aspm_default &= ~PCIE_LINK_STATE_L0S;
+			link->aspm_enabled &= ~PCIE_LINK_STATE_L0S;
+		}
+
+		/*
+		 * Clear L1SS in hardware before updating aspm_support. Once
+		 * aspm_capable is derived from aspm_support, pcie_config_aspm_link()
+		 * skips pcie_config_aspm_l1ss() entirely via the aspm_capable guard,
+		 * leaving firmware-enabled L1SS substates active in hardware.
+		 * This applies equally when disabling L1 (which implies L1SS).
+		 */
+		if ((no_l1 || no_l1ss) && (link->aspm_enabled & PCIE_LINK_STATE_L1SS))
+			pcie_config_aspm_l1ss(link, 0);
+
+		if (no_l1) {
+			link->aspm_support &= ~(PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1SS);
+			link->aspm_default &= ~(PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1SS);
+			link->aspm_enabled &= ~(PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1SS);
+		} else if (no_l1ss) {
+			link->aspm_support &= ~PCIE_LINK_STATE_L1SS;
+			link->aspm_default &= ~PCIE_LINK_STATE_L1SS;
+			link->aspm_enabled &= ~PCIE_LINK_STATE_L1SS;
+		}
+
 		if (link->aspm_support & PCIE_LINK_STATE_L0S)
 			link->aspm_default |= PCIE_LINK_STATE_L0S;
 		if (link->aspm_support & PCIE_LINK_STATE_L1)
@@ -926,9 +999,25 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 
 	aspm_l1ss_init(link);
 
-	/* Restore L0s/L1 if they were enabled */
+	/* Save default state */
+	link->aspm_default = link->aspm_enabled;
+
+	pcie_aspm_override_default_link_state(link);
+
+	/*
+	 * Restore L0s/L1 if they were enabled, but don't restore any
+	 * state a Devicetree override just disabled in aspm_support above.
+	 */
 	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
 	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
+		if (!(link->aspm_support & PCIE_LINK_STATE_L0S)) {
+			child_lnkctl &= ~PCI_EXP_LNKCTL_ASPM_L0S;
+			parent_lnkctl &= ~PCI_EXP_LNKCTL_ASPM_L0S;
+		}
+		if (!(link->aspm_support & PCIE_LINK_STATE_L1)) {
+			child_lnkctl &= ~PCI_EXP_LNKCTL_ASPM_L1;
+			parent_lnkctl &= ~PCI_EXP_LNKCTL_ASPM_L1;
+		}
 		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
 						   PCI_EXP_LNKCTL_ASPMC,
 						   parent_lnkctl & PCI_EXP_LNKCTL_ASPMC);
@@ -938,11 +1027,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 							   child_lnkctl & PCI_EXP_LNKCTL_ASPMC);
 	}
 
-	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
-
-	pcie_aspm_override_default_link_state(link);
-
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
 
@@ -956,42 +1040,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 }
 
-/* Configure the ASPM L1 substates. Caller must disable L1 first. */
-static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
-{
-	u32 val = 0;
-	struct pci_dev *child = link->downstream, *parent = link->pdev;
-
-	if (state & PCIE_LINK_STATE_L1_1)
-		val |= PCI_L1SS_CTL1_ASPM_L1_1;
-	if (state & PCIE_LINK_STATE_L1_2)
-		val |= PCI_L1SS_CTL1_ASPM_L1_2;
-	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
-		val |= PCI_L1SS_CTL1_PCIPM_L1_1;
-	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
-		val |= PCI_L1SS_CTL1_PCIPM_L1_2;
-
-	/*
-	 * PCIe r6.2, sec 5.5.4, rules for enabling L1 PM Substates:
-	 * - Clear L1.x enable bits at child first, then at parent
-	 * - Set L1.x enable bits at parent first, then at child
-	 * - ASPM/PCIPM L1.2 must be disabled while programming timing
-	 *   parameters
-	 */
-
-	/* Disable all L1 substates */
-	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
-				       PCI_L1SS_CTL1_L1SS_MASK, 0);
-	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-				       PCI_L1SS_CTL1_L1SS_MASK, 0);
-
-	/* Enable what we need to enable */
-	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-				       PCI_L1SS_CTL1_L1SS_MASK, val);
-	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
-				       PCI_L1SS_CTL1_L1SS_MASK, val);
-}
-
 static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
 {
 	pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,

-- 
2.34.1

Re: [PATCH v6 3/3] PCI/ASPM: Mask ASPM states based on Devicetree properties

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardware ASPM state becomes desynchronized from the software-tracked `link->aspm_enabled` state on links where firmware forced ASPM L0s/L1 on despite missing hardware capabilities, breaking sysfs reporting.
--

--- Patch [3]: [PATCH v6 3/3] PCI/ASPM: Mask ASPM states based on Devicetree properties ---
commit d3ca900c30e202e8adf6acdf7daf494224ae2179
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

PCI/ASPM: Mask ASPM states based on Devicetree properties

This commit introduces Devicetree property checks to selectively disable ASPM
states based on hardware constraints. It moves the default state override
evaluation earlier in capability initialization and ensures that states disabled
by Devicetree are explicitly cleared from the hardware configuration during the
L0s/L1 restore phase.

Link [1]: https://github.com/devicetree-org/dt-schema/pull/188
Link [2]: https://github.com/devicetree-org/dt-schema/pull/190

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 544f35bb25554..0a671825fac8d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c

[ ... ]

> @@ -926,9 +999,25 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  
>  	aspm_l1ss_init(link);
>  
> -	/* Restore L0s/L1 if they were enabled */
> +	/* Save default state */
> +	link->aspm_default = link->aspm_enabled;
> +
> +	pcie_aspm_override_default_link_state(link);
> +
> +	/*
> +	 * Restore L0s/L1 if they were enabled, but don't restore any
> +	 * state a Devicetree override just disabled in aspm_support above.
> +	 */
>  	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
>  	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
> +		if (!(link->aspm_support & PCIE_LINK_STATE_L0S)) {
> +			child_lnkctl &= ~PCI_EXP_LNKCTL_ASPM_L0S;
> +			parent_lnkctl &= ~PCI_EXP_LNKCTL_ASPM_L0S;
> +		}
> +		if (!(link->aspm_support & PCIE_LINK_STATE_L1)) {
> +			child_lnkctl &= ~PCI_EXP_LNKCTL_ASPM_L1;
> +			parent_lnkctl &= ~PCI_EXP_LNKCTL_ASPM_L1;
> +		}

[Severity: Medium]
If firmware forced ASPM L0s or L1 on despite missing hardware capabilities,
this block will correctly strip those unsupported bits before writing back to
hardware. However, does this leave the software state desynchronized?

Earlier during initialization, link->aspm_enabled captures the original
firmware settings. Since this restore block now selectively clears unsupported
bits from the hardware write but doesn't synchronize those changes back to
link->aspm_enabled, won't sysfs attributes and pcie_aspm_enabled() incorrectly
report these states as active?

Could this lead to incorrect sysfs reporting until pci_enable_device()
eventually triggers a heal of the state via pcie_config_aspm_link()?

>  		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
>  						   PCI_EXP_LNKCTL_ASPMC,
>  						   parent_lnkctl & PCI_EXP_LNKCTL_ASPMC);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-aspm-v6-0-2ebb3ee7ef71@oss.qualcomm.com?part=3