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

[Ilpo Järvinen]

On Wed, 29 Apr 2026, Thomas Falcon wrote:

> For systems with a BIOS release date starting in 2025, default
> ASPM policy to powersupersave if supported in the ACPI FADT.
> Provide a flag, aspm_user_policy, tracking whether a user has
> requested a specific power state to give those precedence.
> Do not enable all states if user has chosen a specific policy
> or disabled ASPM using the pcie_aspm module parameter.
> 
> Suggested-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> Signed-off-by: Thomas Falcon <thomas.falcon@xxxxxxxxx>
> ---
>  drivers/pci/pci-acpi.c  |  4 +++-
>  drivers/pci/pcie/aspm.c | 17 +++++++++++++++++
>  include/linux/pci.h     |  1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 4d0f2cb6c695..d849bc6d0c0c 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1523,7 +1523,9 @@ static int __init acpi_pci_init(void)
>  	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
>  		pr_info("ACPI FADT declares the system doesn't support PCIe ASPM, so disable it\n");
>  		pcie_no_aspm();
> -	}
> +	} else
> +		/* If ASPM is supported, configure the default policy here. */
> +		pcie_aspm_policy_config_init();

Please balance braces (and with comment this is multiline block anyway 
so you should use braces even because of that).

>  
>  	if (acpi_pci_disabled)
>  		return 0;
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 84d49aa8a5ba..1c81e2f2e589 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -267,6 +267,8 @@ static int aspm_policy = POLICY_POWER_SUPERSAVE;
>  #else
>  static int aspm_policy;
>  #endif
> +static int aspm_default_policy = POLICY_POWER_SUPERSAVE;
> +static bool aspm_user_policy;
>  
>  static const char *policy_str[] = {
>  	[POLICY_DEFAULT] = "default",
> @@ -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;
>  	list_for_each_entry(link, &link_list, sibling) {
>  		pcie_config_aspm_link(link, policy_to_aspm_state(link));
>  		pcie_set_clkpm(link, policy_to_clkpm_state(link));
> @@ -1810,6 +1813,20 @@ static int __init pcie_aspm_disable(char *str)
>  
>  __setup("pcie_aspm=", pcie_aspm_disable);
>  
> +
> +

Extra empty lines.

> +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 && dmi_get_bios_year() >= 2025)

Is it good to have this 2025 check in two places as literals, should there 
be only one function which is called by both places?

> +		aspm_policy = aspm_default_policy;
> +}
> +
>  void pcie_no_aspm(void)
>  {
>  	/*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2c4454583c11..36fa5579709c 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);
>  bool pcie_aspm_support_enabled(void);
>  bool pcie_aspm_enabled(struct pci_dev *pdev);
>  #else
> 

-- 
 i.