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

[sashiko-bot@kernel.org]

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