Re: [RFC PATCH v2 4/4] pcie/aspm: Remove CONFIG_PCIEASPM_* policy definitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The command-line parameter `pcie_aspm.policy=default` is silently ignored on modern systems due to an early return in the parameter parsing logic.
- [High] Removal of compile-time ASPM policy causes non-ACPI and non-DMI platforms to silently default to enabling ASPM, circumventing hardware quirk protections.
--

commit 374fa4f0c33fee54c3758b88f99192ede2552a40
Author: Thomas Falcon <thomas.falcon@intel.com>

pcie/aspm: Remove CONFIG_PCIEASPM_* policy definitions

This commit removes compile-time Kconfig options for PCIe ASPM policy.
It dynamically defaults to POLICY_POWER_SUPERSAVE for new systems and
POLICY_DEFAULT for legacy systems, while keeping the pcie_aspm module
parameter for user overrides.

> diff --git a/arch/mips/configs/loongson2k_defconfig b/arch/mips/configs/loongson2k_defconfig
> index ca534a6b66deb..0a2c123eee69b 100644
> --- a/arch/mips/configs/loongson2k_defconfig
> +++ b/arch/mips/configs/loongson2k_defconfig
> @@ -88,7 +88,6 @@ CONFIG_RFKILL=m
>  CONFIG_RFKILL_INPUT=y
>  CONFIG_PCIEPORTBUS=y
>  CONFIG_HOTPLUG_PCI_PCIE=y
> -CONFIG_PCIEASPM_PERFORMANCE=y

Could this removal introduce a regression on non-ACPI platforms like
loongson2k by circumventing hardware quirk protections?

The new dynamic assignment in pcie_aspm_policy_config_init() is exclusively
called from drivers/pci/pci-acpi.c. This means it is never executed on
non-ACPI platforms.

Even if it were called, dmi_get_bios_year() returns a negative error code
(-ENXIO) on non-DMI platforms, causing the evaluation to incorrectly flag
them as legacy and skip setting aspm_policy to POLICY_POWER_SUPERSAVE.

Consequently, aspm_policy remains permanently set to 0 (POLICY_DEFAULT)
on these platforms. On Device Tree platforms, POLICY_DEFAULT uses
link->aspm_default, where pcie_aspm_override_default_link_state()
explicitly adds PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 to the
default state.

Does this silently flip the hardware link state from safely disabled to
unconditionally enabled for platforms that previously relied on
CONFIG_PCIEASPM_PERFORMANCE=y to explicitly disable ASPM due to hardware bugs?

[ ... ]

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 063ebe81a9cdf..4502f56f2314c 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -258,15 +258,7 @@ static LIST_HEAD(link_list);
>  #define POLICY_POWERSAVE 2	/* high power saving */
>  #define POLICY_POWER_SUPERSAVE 3 /* possibly even more power saving */
>  
> -#ifdef CONFIG_PCIEASPM_PERFORMANCE
> -static int aspm_policy = POLICY_PERFORMANCE;
> -#elif defined CONFIG_PCIEASPM_POWERSAVE
> -static int aspm_policy = POLICY_POWERSAVE;
> -#elif defined CONFIG_PCIEASPM_POWER_SUPERSAVE
> -static int aspm_policy = POLICY_POWER_SUPERSAVE;
> -#else
>  static int aspm_policy;

Since aspm_policy is now statically initialized to 0 (POLICY_DEFAULT),
is the command-line parameter pcie_aspm.policy=default silently ignored
on modern systems?

When a user passes pcie_aspm.policy=default during early boot,
sysfs_match_string() resolves "default" to 0. The parsing logic exits
early because the requested policy matches the initial value:

drivers/pci/pcie/aspm.c:pcie_aspm_set_policy() {
    ...
    i = sysfs_match_string(policy_str, val);
    if (i < 0)
        return i;
    if (i == aspm_policy)
        return 0;
    ...
    aspm_user_policy = true;
    ...
}

This early return prevents aspm_user_policy from being set to true.

Later in the boot process, pcie_aspm_policy_config_init() runs. Because
aspm_user_policy is still false, on modern systems the kernel unconditionally
overwrites aspm_policy:

drivers/pci/pcie/aspm.c:pcie_aspm_policy_config_init() {
    ...
    if (!aspm_disabled && !aspm_user_policy &&
        !pcie_aspm_legacy_config_check())
        aspm_policy = aspm_default_policy;
}

Does this sequence introduce a regression where the user's explicit request
to force BIOS defaults via the command line is swallowed, potentially causing
hardware hangs on boards with broken ASPM implementations?

> -#endif
>  static int aspm_default_policy = POLICY_POWER_SUPERSAVE;
>  static bool aspm_user_policy;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511210936.562622-1-thomas.falcon@intel.com?part=4