Message-ID: <ng67s7imjpj7i5ym7unvmewzhyk4ybgpkgw5aizicfs423vsxh@hvpfmk32ooe4>
Date: Fri, 29 Aug 2025 12:54:20 -0700
From: David Box <david.e.box@...ux.intel.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: rafael@...nel.org, bhelgaas@...gle.com, vicamo.yang@...onical.com, 
	kenny@...ix.com, ilpo.jarvinen@...ux.intel.com, nirmal.patel@...ux.intel.com, 
	mani@...nel.org, linux-pm@...r.kernel.org, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 1/2] PCI/ASPM: Add host-bridge API to override default
 ASPM/CLKPM link state

On Thu, Aug 28, 2025 at 03:43:45PM -0500, Bjorn Helgaas wrote:
> On Mon, Aug 25, 2025 at 01:35:22PM -0700, David E. Box wrote:
> > Synthetic PCIe hierarchies, such as those created by Intel VMD, are not
> > enumerated by firmware and do not receive BIOS-provided ASPM or CLKPM
> > defaults. Devices in such domains may therefore run without the intended
> > power management.
> > 
> > Add a host-bridge mechanism that lets controller drivers supply their own
> > defaults. A new aspm_default_link_state field in struct pci_host_bridge is
> > set via pci_host_set_default_pcie_link_state(). During link initialization,
> > if this field is non-zero, ASPM and CLKPM defaults come from it instead of
> > BIOS.
> > 
> > This enables drivers like VMD to align link power management with platform
> > expectations and avoids embedding controller-specific quirks in ASPM core
> > logic.
> 
> I think this kind of sidesteps the real issue.  Drivers for host
> controllers or PCI devices should tell us about *broken* things, but
> not about things advertised by the hardware and available for use.

I agree with the principle. The intent isn’t for VMD (or any controller) to
override valid platform policy. It’s to handle synthetic domains where the
platform doesn’t provide any policy path (no effective _OSC/FADT for the child
hierarchy). In those cases, the controller is the only agent that knows the
topology and can supply sane defaults.

I’m happy to tighten the patch to explicitly cover synthetic domains only.
Instead of an API, we could have a boolean flag 'aspm_synthetic_domain'. When
set by the controller, we can do:

    if (host_bridge->aspm_synthetic_domain)
            link->aspm_default = PCIE_LINK_STATE_ALL;

This at least addresses your concern about policy decision, leaving it to the
core to determine how these domains are handled rather than an ABI that lets
domains set policy.

> 
> The only documented policy controls I'm aware of for ASPM are:
> 
>   - FADT "PCIe ASPM Controls" bit ("if set, OS must not enable ASPM
>     control on this platform")
> 
>   - _OSC negotiation for control of the PCIe Capability (OS is only
>     allowed to write PCI_EXP_LNKCTL if platform has granted control to
>     the OS)
> 
> I think what we *should* be doing is enabling ASPM when it's
> advertised, subject to those platform policy controls and user choices
> like CONFIG_PCIEASPM_PERFORMANCE/POWERSAVE/etc and sysfs attributes.
> 
> So basically I think link->aspm_default should be PCIE_LINK_STATE_ALL
> without drivers doing anything at all.  Maybe we have to carve out
> exceptions, e.g., "VMD hierarchies are exempt from _OSC," or "devices
> on x86 systems before 2026 can't enable more ASPM than BIOS did," or
> whatever.  Is there any baby step we can make in that direction?
> 
> This feels a little scary, so feel free to convince me it can't be
> done :)

I understand your direction of enabling all advertised states by default
(subject to FADT/_OSC and user settings). To explore that, I’ll send an RFC in
parallel with this patch that proposes a baby step, e.g.  add instrumentation so
we can see where BIOS left capabilities unused, and make it opt-in via a boot
param so we can evaluate impact safely.

So this series will handle the VMD gap directly, and the RFC can kick off the
wider discussion about defaults on ACPI-managed hosts. Does that sound like a
reasonable approach and split?

David

> 
> > Link: https://patchwork.ozlabs.org/project/linux-pci/patch/20250720190140.2639200-1-david.e.box%40linux.intel.com/
> > Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> > Tested-by: Manivannan Sadhasivam <mani@...nel.org>
> > Reviewed-by: Rafael J. Wysocki (Intel) <rafael@...nel.org>
> > ---
> > Changes in V3:
> >   -- Changed pci_host_get_default_pcie_link_state() argument name from
> >      parent to dev.
> >   -- Applied changelog tags
> > 
> > Changes in V2:
> > 
> >   -- Host field name changed to aspm_default_link_state.
> >   -- Added get/set functions for aspm_default_link_state. Only the
> >      setter is exported. Added a kernel-doc describing usage and
> >      particulars around meaning of 0.
> > 
> > Changes in V1 from RFC:
> > 
> >   -- Rename field to aspm_dflt_link_state since it stores
> >      PCIE_LINK_STATE_XXX flags, not a policy enum.
> >   -- Move the field to struct pci_host_bridge since it's being applied to
> >      the entire host bridge per Mani's suggestion.
> >   -- During testing noticed that clkpm remained disabled and this was
> >      also handled by the formerly used pci_enable_link_state(). Add a
> >      check in pcie_clkpm_cap_init() as well to enable clkpm during init.
> > 
> >  drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++--
> >  include/linux/pci.h     |  9 +++++++++
> >  2 files changed, 49 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 919a05b97647..851ca3d68e55 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -373,6 +373,39 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> >  	pcie_set_clkpm_nocheck(link, enable);
> >  }
> >  
> > +/**
> > + * pci_host_set_default_pcie_link_state - set controller-provided default ASPM/CLKPM mask
> > + * @host: host bridge on which to apply the defaults
> > + * @state: PCIE_LINK_STATE_XXX flags
> > + *
> > + * Allows a PCIe controller driver to specify the default ASPM and/or
> > + * Clock Power Management (CLKPM) link state mask that will be used
> > + * for links under this host bridge during ASPM/CLKPM capability init.
> > + *
> > + * The value is consumed in pcie_aspm_cap_init() and pcie_clkpm_cap_init()
> > + * to override the firmware-discovered defaults.
> > + *
> > + * Interpretation of aspm_default_link_state:
> > + *   - Nonzero: bitmask of PCIE_LINK_STATE_* values to be used as defaults
> > + *   - Zero:    no override provided; ASPM/CLKPM defaults fall back to
> > + *              values discovered in hardware/firmware
> > + *
> > + * Note: zero is always treated as "unset", not as "force ASPM/CLKPM off".
> > + */
> > +void pci_host_set_default_pcie_link_state(struct pci_host_bridge *host,
> > +					  unsigned int state)
> > +{
> > +	host->aspm_default_link_state = state;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_host_set_default_pcie_link_state);
> > +
> > +static u32 pci_host_get_default_pcie_link_state(struct pci_dev *dev)
> > +{
> > +	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> > +
> > +	return host ? host->aspm_default_link_state : 0;
> > +}
> > +
> >  static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> >  {
> >  	int capable = 1, enabled = 1;
> > @@ -394,7 +427,10 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> >  			enabled = 0;
> >  	}
> >  	link->clkpm_enabled = enabled;
> > -	link->clkpm_default = enabled;
> > +	if (pci_host_get_default_pcie_link_state(link->pdev) & PCIE_LINK_STATE_CLKPM)
> > +		link->clkpm_default = 1;
> > +	else
> > +		link->clkpm_default = enabled;
> >  	link->clkpm_capable = capable;
> >  	link->clkpm_disable = blacklist ? 1 : 0;
> >  }
> > @@ -866,7 +902,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> >  	}
> >  
> >  	/* Save default state */
> > -	link->aspm_default = link->aspm_enabled;
> > +	link->aspm_default = pci_host_get_default_pcie_link_state(parent);
> > +	if (!link->aspm_default)
> > +		link->aspm_default = link->aspm_enabled;
> >  
> >  	/* Setup initial capable state. Will be updated later */
> >  	link->aspm_capable = link->aspm_support;
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 59876de13860..8947cbaf9fa6 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -620,6 +620,10 @@ struct pci_host_bridge {
> >  	unsigned int	size_windows:1;		/* Enable root bus sizing */
> >  	unsigned int	msi_domain:1;		/* Bridge wants MSI domain */
> >  
> > +#ifdef CONFIG_PCIEASPM
> > +	unsigned int	aspm_default_link_state;	/* Controller-provided default */
> > +#endif
> > +
> >  	/* Resource alignment requirements */
> >  	resource_size_t (*align_resource)(struct pci_dev *dev,
> >  			const struct resource *res,
> > @@ -1849,6 +1853,8 @@ int pci_disable_link_state(struct pci_dev *pdev, int state);
> >  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 pci_host_set_default_pcie_link_state(struct pci_host_bridge *host,
> > +					  unsigned int state);
> >  void pcie_no_aspm(void);
> >  bool pcie_aspm_support_enabled(void);
> >  bool pcie_aspm_enabled(struct pci_dev *pdev);
> > @@ -1861,6 +1867,9 @@ static inline int pci_enable_link_state(struct pci_dev *pdev, int state)
> >  { return 0; }
> >  static inline int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
> >  { return 0; }
> > +static inline void
> > +pci_host_set_default_pcie_link_state(struct pci_host_bridge *host,
> > +				     unsigned int state) { }
> >  static inline void pcie_no_aspm(void) { }
> >  static inline bool pcie_aspm_support_enabled(void) { return false; }
> >  static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
> > 
> > base-commit: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
> > -- 
> > 2.43.0
> > 

Powered by blists - more mailing lists