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

On 5/12/2026 3:56 PM, Krishna Chaitanya Chundru wrote:
>
> On 5/12/2026 6:16 AM, [email protected] wrote:
>> 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 <[email protected]>
>>
>> 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;
> ack
>>> +
>>> +		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?
> we can't keep aspm_capable intact as this can expose l1ss capability to sysfs
> as pointed
> by you in the above, I will do like below, 
>  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_support & PCIE_LINK_STATE_L1SS)
>           pcie_config_aspm_l1ss(link, 0);
>       link->aspm_support &= ~PCIE_LINK_STATE_L1SS;
>   }
>
> Bjorn,
> can you share your taught's on this.
Hi Bjorn,
Can you please share your opinion on this.

- Krishna Chaitanya.
>
> - Krishna Chaitanya.
>>> +
>>>  		if (link->aspm_support & PCIE_LINK_STATE_L0S)
>>>  			link->aspm_default |= PCIE_LINK_STATE_L0S;
>>>  		if (link->aspm_support & PCIE_LINK_STATE_L1)