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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying `link->aspm_enabled` prematurely prevents actual hardware reprogramming on multi-function devices.
- [High] Clearing `link->aspm_enabled` in the DT override bypasses `pcie_config_aspm_link()`'s state update, leaving the PCI saved state stale.
--

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

PCI/ASPM: Mask ASPM states based on Devicetree properties

Some platforms require selectively disabling specific ASPM states on a given
PCIe link to avoid link instability. This patch introduces support for
devicetree properties like aspm-no-l0s, aspm-no-l1, and aspm-no-l1ss
to mask out unsupported ASPM states during default link state initialization.

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 172783e7f519a..19f5582a910dc 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c

[ ... ]

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

[Severity: High]
Does prematurely modifying link->aspm_enabled prevent actual hardware
reprogramming on multi-function devices?

If link->aspm_enabled is updated here, later when pcie_config_aspm_link() is
called, it might see that (link->aspm_enabled == state) and return early:

drivers/pci/pcie/aspm.c:pcie_config_aspm_link() {
    if (link->aspm_enabled == state)
        return;
    ...
    list_for_each_entry(child, &linkbus->devices, bus_list)
        pcie_config_aspm_dev(child, dwstream);
}

Will this early return skip the loop that programs the LNKCTL register for PCI
functions greater than 0, leaving them with firmware-enabled ASPM states that
violate the Devicetree override?

> +		}
> +
> +		/*
> +		 * 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);

[Severity: High]
Does returning early from pcie_config_aspm_link() also leave the PCI saved
state stale?

Because pcie_config_aspm_link() returns early when link->aspm_enabled matches
the requested state, it skips the saved state updates:

drivers/pci/pcie/aspm.c:pcie_config_aspm_link() {
    if (link->aspm_enabled == state)
        return;
    ...
    pci_save_aspm_l1ss_state(link->downstream);
    pci_update_aspm_saved_state(link->downstream);
    pci_save_aspm_l1ss_state(parent);
    pci_update_aspm_saved_state(parent);
}

If the PCI saved state retains the firmware-enabled ASPM configuration, won't
pci_restore_state() write those stale states back to hardware upon resume from
suspend, FLR, or AER recovery, re-enabling the ASPM states that the
Devicetree override intended to disable?

> +		} 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;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-aspm-v4-1-d66f85b728b0@oss.qualcomm.com?part=1