Re: [RFC PATCH v2 2/4] pcie/aspm: Enable all power-saving states during link state initialization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data race and missing memory barriers in lazy initialization of `pcie_aspm_legacy_config_check()`
- [Medium] Implicit reliance on `dmi_get_bios_year()` error code evaluating as a chronological year
- [Low] Misleading log message claiming ASPM configuration occurs at 'build' time, and stale code comments
--

commit 5d1b20da84d0615c569d56fd7efaa7c241b6e329
Author: Thomas Falcon <thomas.falcon@intel.com>

pcie/aspm: Enable all power-saving states during link state initialization

This commit enables setting powersave and powersupersave states during
ASPM link state initialization. It restricts this behavior to systems
with a BIOS release during or after 2025 to simplify the enabling flow.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index cd23c1462502f..e93b724945341 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c

[ ... ]

> @@ -1057,6 +1058,23 @@ static void free_link_state(struct pcie_link_state *link)
>  	kfree(link);
>  }
>  
> +static bool pcie_aspm_legacy_config_check(void)
> +{
> +	static bool legacy_aspm_config;
> +	static bool checked;
> +
> +	if (checked)
> +		return legacy_aspm_config;
> +	if (dmi_get_bios_year() < 2025)
> +		legacy_aspm_config = true;

When dmi_get_bios_year() returns -ENXIO (-6) on systems without DMI support
(like Device Tree based ARM64 or PowerPC), won't it evaluate as less than
2025?

Does this unintentionally classify all non-DMI platforms as legacy systems
regardless of their actual age?

> +
> +	pr_info_once("ASPM configuration is determined at %s time\n",
> +		     legacy_aspm_config ? "build" : "boot");
> +	checked = true;

Are we missing memory barriers here?

During concurrent PCIe device enumeration, if one thread sets
legacy_aspm_config = true and then checked = true:

Thread A:
    legacy_aspm_config = true;
    checked = true;

Could weak memory ordering cause Thread B to observe checked == true but
read a stale uninitialized legacy_aspm_config value of false, leading to
a regression where unsupported systems bypass the legacy ASPM delay?

Also, this isn't a bug, but does ASPM configuration really occur at "build"
time on legacy systems, or is it deferred until runtime when the device
driver calls pci_enable_device()?

The log message might be misleading for system administrators.

> +
> +	return legacy_aspm_config;
> +}
> +
>  static int pcie_aspm_sanity_check(struct pci_dev *pdev)
>  {
>  	struct pci_dev *child;

[ ... ]

> @@ -1196,8 +1214,9 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
>  	 * the BIOS's expectation, we'll do so once pci_enable_device() is
>  	 * called.
>  	 */

This isn't a bug, but should the comment block above be updated to reflect
that POLICY_POWERSAVE configurations are now applied immediately on modern
systems instead of being deferred?

> -	if (aspm_policy != POLICY_POWERSAVE &&
> -	    aspm_policy != POLICY_POWER_SUPERSAVE) {
> +	if (!pcie_aspm_legacy_config_check() ||
> +	    (aspm_policy != POLICY_POWERSAVE &&
> +	     aspm_policy != POLICY_POWER_SUPERSAVE)) {
>  		pcie_config_aspm_path(link);
>  		pcie_set_clkpm(link, policy_to_clkpm_state(link));
>  	}

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