Re: [RFC PATCH v2 3/4] pcie/aspm: Enable all hardware power-saving states by default

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Explicit user command-line policy requests via pcie_aspm.policy are incorrectly ignored and overridden if they match the initial compile-time default.
- [High] Data race and missing memory barriers in lockless initialization of pcie_aspm_legacy_config_check().
- [Low] Missing static inline stub for pcie_aspm_policy_config_init() causes build failure when CONFIG_PCIEASPM is disabled.
--

commit 8a98442b198d4058ab31172e6d27a97c7e0f4b0f
Author: Thomas Falcon <thomas.falcon@intel.com>

pcie/aspm: Enable all hardware power-saving states by default

This commit defaults ASPM policy to powersupersave for systems with BIOS release
dates starting in 2025. It provides a flag to track user-requested policies to
give them precedence over the new default.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index e93b724945341..063ebe81a9cdf 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
[ ... ]
> @@ -1609,6 +1611,7 @@ static int pcie_aspm_set_policy(const char *val,
>  	down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);
>  	aspm_policy = i;
> +	aspm_user_policy = true;

Are explicit user command-line policy requests via pcie_aspm.policy incorrectly
ignored and overridden if they match the initial compile-time default?

If a user explicitly boots with pcie_aspm.policy=default,
pcie_aspm_set_policy() is invoked to parse the boot parameter before initcalls
run.

If the requested policy matches the initial compiled-in aspm_policy value
(e.g., POLICY_DEFAULT), the early check if (i == aspm_policy) is met, causing
the function to return 0.

Because of this early return, the aspm_user_policy = true flag is never set.

Later during boot, acpi_pci_init() unconditionally calls
pcie_aspm_policy_config_init(). Since aspm_user_policy is false, it incorrectly
assumes the user didn't specify a preference and unilaterally forces the policy
to POLICY_POWER_SUPERSAVE (on BIOS >= 2025 systems).

This logic error completely contradicts the user's explicit command-line
override and forces unsupported power states.

[ ... ]
> @@ -1810,6 +1813,19 @@ static int __init pcie_aspm_disable(char *str)
>  
>  __setup("pcie_aspm=", pcie_aspm_disable);
>  
> +void __init pcie_aspm_policy_config_init(void)
> +{
> +	/*
> +	 * Set ASPM policy here, enabling all power-saving states
> +	 * unless ASPM has been disabled or the user has already
> +	 * requested a policy or the systems BIOS release date
> +	 * is before the year 2025. Otherwise use BIOS defaults.
> +	 */
> +	if (!aspm_disabled && !aspm_user_policy &&
> +	    !pcie_aspm_legacy_config_check())

This problem wasn't introduced by this patch, but exists in
pcie_aspm_legacy_config_check(), introduced earlier in the series.

Does the lockless initialization pattern in pcie_aspm_legacy_config_check()
contain a data race?

The function implements a lockless double-checked initialization pattern using
the static variables checked and legacy_aspm_config without any memory barriers
(e.g., smp_store_release() and smp_load_acquire()).

It is called from pcie_aspm_init_link_state(), which can be executed
concurrently across multiple CPUs during PCI device enumeration (e.g., parallel
hotplug events on different root ports).

Due to compiler or CPU reordering, a CPU can observe checked == true but read
a stale false value for legacy_aspm_config.

This data race causes the function to incorrectly return false on legacy
hardware, causing the modern ASPM link configuration to be misapplied to older
hardware.

> +		aspm_policy = aspm_default_policy;
> +}
> +
>  void pcie_no_aspm(void)
>  {
>  	/*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2c4454583c115..36fa5579709c5 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1915,6 +1915,7 @@ int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
>  int pci_enable_link_state(struct pci_dev *pdev, int state);
>  int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
>  void pcie_no_aspm(void);
> +void pcie_aspm_policy_config_init(void);

Does this missing static inline stub for pcie_aspm_policy_config_init() cause
a build failure when CONFIG_PCIEASPM is disabled?

The patch adds the function declaration void pcie_aspm_policy_config_init(void);
inside the #ifdef CONFIG_PCIEASPM block in include/linux/pci.h.

However, the patch forgets to provide an empty static inline stub in the
corresponding #else block.

Because drivers/pci/pci-acpi.c calls this function unconditionally and is
compiled whenever CONFIG_ACPI=y is set, building a kernel with CONFIG_ACPI=y
and CONFIG_PCIEASPM=n will result in implicit declaration compiler errors and
undefined reference linker errors.

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