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

["Falcon, Thomas" <thomas.falcon@intel.com>]

On Thu, 2026-04-30 at 13:17 +0300, Ilpo Järvinen wrote:
> 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@linux.intel.com>
> > Signed-off-by: Thomas Falcon <thomas.falcon@intel.com>
> > ---
> >  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).

Thanks, I will fix this in the next version.

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

Oops, I will fix in v2.

> 
> > +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?

Ok, might be able to use pcie_aspm_legacy_config_check from the
previous patch here. I will fix in v2.

Thanks,
Tom

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