Re: [PATCH v2] 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 ASPM L1 PM Substates without first disabling ASPM L1 violates PCIe specifications.
- [Medium] The 'aspm-no-l1' override removes L1SS software support but fails to clear firmware-enabled L1SS hardware state.
--

commit 0343a07bd629cd03e625683fa27781cd434b46f3
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

PCI/ASPM: Mask ASPM states based on Devicetree properties

This commit updates ASPM link initialization to check for Devicetree
properties like 'aspm-no-l0s', 'aspm-no-l1', and 'aspm-no-l1ss'. It masks
the corresponding ASPM states from the link support mask before applying
the default ASPM policy.

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 925373b98dff0..a79426b5bff0f 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c

[ ... ]

> @@ -806,6 +815,24 @@ 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()) {
> +		if (pcie_link_has_aspm_override(link, "aspm-no-l0s"))
> +			link->aspm_support &= ~PCIE_LINK_STATE_L0S;
> +
> +		if (pcie_link_has_aspm_override(link, "aspm-no-l1"))
> +			link->aspm_support &= ~(PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1SS);

[Severity: Medium]
If the devicetree has 'aspm-no-l1' but not 'aspm-no-l1ss', this clears
PCIE_LINK_STATE_L1SS from the link's aspm_support. Will this cause
pcie_config_aspm_link() to skip updating the L1SS registers because
aspm_capable lacks L1SS?

Should this branch also explicitly clear the hardware state via
pcie_config_aspm_l1ss(link, 0) in the same way the 'aspm-no-l1ss' branch
does, to prevent leaving a firmware-enabled L1SS configuration active in
hardware?

> +
> +		if (pcie_link_has_aspm_override(link, "aspm-no-l1ss")) {
> +			/*
> +			 * 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.
> +			 */
> +			if (link->aspm_enabled & PCIE_LINK_STATE_L1SS)
> +				pcie_config_aspm_l1ss(link, 0);

[Severity: High]
Does modifying the L1SS registers here violate PCIe specifications by
changing L1 PM Substates while ASPM L1 is still enabled?

Looking at the caller pcie_aspm_cap_init(), the original PCI_EXP_LNKCTL
values are restored right before calling pcie_aspm_override_default_link_state(),
which can re-enable ASPM L1 if the firmware had it enabled:

pcie_aspm_cap_init() {
    ...
    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_aspm_override_default_link_state(link);
}

Since PCIe r6.2 sec 5.5.4 requires disabling ASPM L1 first, is there
a risk of undefined hardware behavior or link drops here?

> +			link->aspm_support &= ~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)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-aspm-v2-1-800a4151ba3a@oss.qualcomm.com?part=1