Message-ID: <c6757u3xdyxxuodcjsbpdje7m4qiq26tug5lfxvpbs5wm7r56l@ksy4yge7kg35>
Date: Mon, 21 Jul 2025 13:53:57 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: "David E. Box" <david.e.box@...ux.intel.com>
Cc: rafael@...nel.org, bhelgaas@...gle.com, vicamo.yang@...onical.com, 
	kenny@...ix.com, ilpo.jarvinen@...ux.intel.com, nirmal.patel@...ux.intel.com, 
	linux-pm@...r.kernel.org, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI/ASPM: Allow controller drivers to override default
 ASPM and CLKPM link state

On Sun, Jul 20, 2025 at 12:01:37PM GMT, David E. Box wrote:
> Synthetic PCIe hierarchies, such as those created by Intel VMD, are not
> visible to firmware and do not receive BIOS-provided default ASPM and CLKPM
> configuration. As a result, devices behind such domains operate without
> proper power management, regardless of platform intent.
> 
> To address this, allow controller drivers to supply an override for the
> default link state by setting aspm_dflt_link_state for their associated
> pci_host_bridge. During link initialization, if this field is non-zero,
> ASPM and CLKPM defaults are derived from its value instead of being taken
> from BIOS.
> 
> This mechanism enables drivers like VMD to achieve platform-aligned power
> savings by statically defining the expected link configuration at
> enumeration time, without relying on runtime calls such as
> pci_enable_link_state(), which are ineffective when ASPM is disabled
> globally.
> 
> This approach avoids per-controller hacks in ASPM core logic and provides a
> general mechanism for domains that require explicit control over link power
> state defaults.
> 
> Link: https://lore.kernel.org/linux-pm/0b166ece-eeec-ba5d-2212-50d995611cef@panix.com
> Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> ---
> 
> Changes 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/controller/vmd.c | 12 +++++++++---
>  drivers/pci/pcie/aspm.c      | 13 +++++++++++--
>  include/linux/pci.h          |  4 ++++
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 8df064b62a2f..6f0de95c87fd 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
>  }
>  
>  /*
> - * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> + * Enable LTR settings on devices that aren't configured by BIOS.
>   */
>  static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
>  {
> @@ -770,7 +770,6 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
>  	 * PCIe r6.0, sec 5.5.4.
>  	 */
>  	pci_set_power_state_locked(pdev, PCI_D0);

This call becomes useless now.

> -	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
>  	return 0;
>  }
>  
> @@ -785,6 +784,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  	resource_size_t membar2_offset = 0x2000;
>  	struct pci_bus *child;
>  	struct pci_dev *dev;
> +	struct pci_host_bridge *vmd_host_bridge;
>  	int ret;
>  
>  	/*
> @@ -911,8 +911,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		return -ENODEV;
>  	}
>  
> +	vmd_host_bridge = to_pci_host_bridge(vmd->bus->bridge);
> +
> +#ifdef CONFIG_PCIEASPM
> +	vmd_host_bridge->aspm_dflt_link_state = PCIE_LINK_STATE_ALL;
> +#endif

I think it is better to provide an API that accepts the link state. We can
provide a stub if CONFIG_PCIEASPM is not selected. This will avoid the ifdef
clutter in the callers. Like:

void pci_set_default_link_state(struct pci_host_bridge *host_bridge,
				unsigned int state)
{
#ifdef CONFIG_PCIEASPM
	 host_bridge->aspm_default_link_state = state;
#endif	
}

Or you can stub the entire function to align with other ASPM APIs.

One more thought: Since this API is only going to be called by the host bridge
drivers, we can place it in drivers/pci/controller/pci-host-common.c and name it
as pci_host_common_set_default_link_state().

> +
>  	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> -				   to_pci_host_bridge(vmd->bus->bridge));
> +				   vmd_host_bridge);
>  
>  	vmd_attach_resources(vmd);
>  	if (vmd->irq_domain)
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 29fcb0689a91..6f5b34b172f9 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -380,6 +380,7 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>  	u16 reg16;
>  	struct pci_dev *child;
>  	struct pci_bus *linkbus = link->pdev->subordinate;
> +	struct pci_host_bridge *host = pci_find_host_bridge(link->pdev->bus);
>  
>  	/* All functions should have the same cap and state, take the worst */
>  	list_for_each_entry(child, &linkbus->devices, bus_list) {
> @@ -394,7 +395,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 (host && host->aspm_dflt_link_state & PCIE_LINK_STATE_CLKPM)
> +		link->clkpm_default = 1;
> +	else
> +		link->clkpm_default = enabled;
>  	link->clkpm_capable = capable;
>  	link->clkpm_disable = blacklist ? 1 : 0;
>  }
> @@ -794,6 +798,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	u32 parent_lnkcap, child_lnkcap;
>  	u16 parent_lnkctl, child_lnkctl;
>  	struct pci_bus *linkbus = parent->subordinate;
> +	struct pci_host_bridge *host;
>  
>  	if (blacklist) {
>  		/* Set enabled/disable so that we will disable ASPM later */
> @@ -866,7 +871,11 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	}
>  
>  	/* Save default state */
> -	link->aspm_default = link->aspm_enabled;
> +	host = pci_find_host_bridge(parent->bus);

You can initialize 'host' while defining it.

Also, please add a comment on why we are doing this. The inline comment for the
member is not elaborate enough:

	/*
	 * Use the default link state provided by the Host Bridge driver if
	 * available. If the BIOS is not able to provide default ASPM link
	 * state for some reason, the Host Bridge driver could do.
	 */

> +	if (host && host->aspm_dflt_link_state)
> +		link->aspm_default = host->aspm_dflt_link_state;
> +	else
> +		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 05e68f35f392..930028bf52b4 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -614,6 +614,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_dflt_link_state;	/* Controller provided link state */

	/* Controller provided default link state */


Nit: Please expand 'default' as 'dflt' is not a commonly used acronym for
'default'.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists