Re: [PATCH] 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_support` bypasses `pcie_config_aspm_l1ss()`, failing to disable ASPM L1 Substates in hardware if the bootloader previously enabled them.
- [Medium] The `aspm-no-l1` override only masks `PCIE_LINK_STATE_L1` from `link->aspm_support`, failing to mask the functionally dependent L1 PM Substates (`PCIE_LINK_STATE_L1SS`), leading to sysfs inconsistency.
--

commit 3739505256d4ced724de1faf6f3f111a9994fa59
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 aspm-no-l0s,
aspm-no-l1, and aspm-no-l1ss Devicetree properties. It masks the
corresponding ASPM states from link->aspm_support before applying the
default ASPM policy.

Link: https://github.com/devicetree-org/dt-schema/pull/188
Link: 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..3111ef7fd226e 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c

[ ... ]

> @@ -806,6 +813,15 @@ 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;

Should this also clear PCIE_LINK_STATE_L1SS?

If we clear only PCIE_LINK_STATE_L1, link->aspm_capable will retain the
L1SS bits while dropping the L1 bit. The sysfs visibility check in
aspm_ctrl_attrs_are_visible() relies on aspm_capable, which means the main
L1 control attribute will be hidden, but the L1.1 and L1.2 substate
attributes will be incorrectly exposed.

Any userspace attempts to enable these exposed L1SS attributes via sysfs
will silently fail, as pcie_config_aspm_link() enforces:

if (!(state & PCIE_LINK_STATE_L1))
        state &= ~PCIE_LINK_STATE_L1SS;

> +
> +		if (pcie_link_has_aspm_override(link, "aspm-no-l1ss"))
> +			link->aspm_support &= ~PCIE_LINK_STATE_L1SS;

Will modifying link->aspm_support here bypass disabling ASPM L1 Substates
in hardware if the bootloader previously enabled them?

By clearing bits from link->aspm_support, the changes propagate to
link->aspm_capable. Later, when the kernel applies the ASPM policy in
pcie_config_aspm_link(), the hardware update for L1SS is guarded by:

if (link->aspm_capable & PCIE_LINK_STATE_L1SS)

Because the capability bit was cleared by the DT override, the kernel skips
calling pcie_config_aspm_l1ss() entirely, leaving the hardware actively
using L1SS if the firmware or bootloader had enabled it.

Would setting bits in link->aspm_disable instead (similar to
pci_disable_link_state()) correctly disable these states in hardware
while keeping aspm_capable intact?

> +
>  		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/20260511-aspm-v1-1-b4a9fe955cf9@oss.qualcomm.com?part=1