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

[Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>]

On 5/12/2026 6:16 AM, sashiko-bot@kernel.org 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 <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;
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.

- 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)