Re: [PATCH v3] 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:

New issues:
- [Medium] Software ASPM state (`link->aspm_enabled`) is not updated after L1 PM Substates are explicitly disabled in hardware, causing sysfs to report incorrect state.

Pre-existing issues:
- [High] The 'aspm-no-l0s' and 'aspm-no-l1' DT properties fail to prevent the firmware's L0s and L1 states from being temporarily or permanently re-enabled, leaving broken link states active.
--

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

PCI/ASPM: Mask ASPM states based on Devicetree properties

This patch introduces pcie_link_has_aspm_override to check Devicetree
properties and mask ASPM support accordingly. It explicitly disables
L1SS if requested and moves the initialization sequence before the
restore block.

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
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -846,6 +889,33 @@ 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");

[ ... ]

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

[Severity: Medium]
Does this explicit disable step leave software's tracking of enabled ASPM
states out of sync with the actual hardware state?

By directly calling pcie_config_aspm_l1ss with a state of 0 without also
clearing the corresponding PCIE_LINK_STATE_L1SS bits in link->aspm_enabled,
could sysfs attributes and kernel APIs like pcie_aspm_enabled incorrectly
report L1SS as enabled until pcie_config_aspm_link is later invoked to
reconfigure the link?

> @@ -924,6 +994,11 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  
>  	aspm_l1ss_init(link);
>  
> +	/* Save default state */
> +	link->aspm_default = link->aspm_enabled;
> +
> +	pcie_aspm_override_default_link_state(link);
> +
>  	/* Restore L0s/L1 if they were enabled */
>  	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
>  	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
> @@ -931,11 +1006,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  		pcie_capability_write_word(child, PCI_EXP_LNKCTL, child_lnkctl);
>  	}

[Severity: High]
This is a pre-existing issue, but does moving the override logic above the
restore block still fail to prevent the firmware's L0s and L1 states from
being temporarily or permanently re-enabled?

Because the local variables child_lnkctl and parent_lnkctl hold the original
firmware state and are not masked by the Devicetree overrides processed in
pcie_aspm_override_default_link_state, will this restore block
unconditionally write the original state back to the hardware and re-enable
L0s/L1 despite the Devicetree prohibition?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704-aspm-v3-1-157217aff76f@oss.qualcomm.com?part=1