[PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[David E. Box]

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@linux.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);
-	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
+
 	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);
+	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 */
+#endif
+
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,

base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
-- 
2.43.0

Re: [PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[Manivannan Sadhasivam]

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

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

Re: [PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[Rafael J. Wysocki]

On Mon, Jul 21, 2025 at 10:24 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> 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@linux.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().

I agree with the above except for the new function name.  I'd call it
pci_host_set_default_pcie_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;

Or

link->aspm_default = pci_host_get_default_pcie_link_state(parent);
if (link->aspm_default)
        link->aspm_default = link->aspm_enabled;

and make pci_host_get_default_pcie_link_state() return 0 on failures.

Then you can put all of the relevant information into the
pci_host_get_default_pcie_link_state() kerneldoc comment.

> >
> >       /* 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'.

I agree.

Re: [PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[Rafael J. Wysocki]

On Wed, Jul 23, 2025 at 1:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jul 21, 2025 at 10:24 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> >
> > 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@linux.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().
>
> I agree with the above except for the new function name.  I'd call it
> pci_host_set_default_pcie_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;
>
> Or
>
> link->aspm_default = pci_host_get_default_pcie_link_state(parent);
> if (link->aspm_default)

I omitted the ! above by mistake.

>         link->aspm_default = link->aspm_enabled;

So it should be

link->aspm_default = pci_host_get_default_pcie_link_state(parent);
if (!link->aspm_default)
        link->aspm_default = link->aspm_enabled;

> and make pci_host_get_default_pcie_link_state() return 0 on failures.
>
> Then you can put all of the relevant information into the
> pci_host_get_default_pcie_link_state() kerneldoc comment.
>
> > >
> > >       /* 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'.
>
> I agree.

Re: [PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[David Box]

On Wed, Jul 23, 2025 at 01:54:41PM +0200, Rafael J. Wysocki wrote:
> On Mon, Jul 21, 2025 at 10:24 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> >
> > 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@linux.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.

Missed this. I'll remove it.

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

This would require VMD to select PCI_HOST_COMMON just to set one field in a
common struct. Seems heavy-handed. Thoughts? Also, with this and dropping the D0
call, I'll split the VMD cleanup into a separate patch again.

> 
> I agree with the above except for the new function name.  I'd call it
> pci_host_set_default_pcie_link_state()

Sounds good.

> 
> > > +
> > >       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;
> 
> Or
> 
> link->aspm_default = pci_host_get_default_pcie_link_state(parent);
> if (link->aspm_default)
>         link->aspm_default = link->aspm_enabled;
> 
> and make pci_host_get_default_pcie_link_state() return 0 on failures.
> 
> Then you can put all of the relevant information into the
> pci_host_get_default_pcie_link_state() kerneldoc comment.

Sure. I'll add get/set APIs (plus the !) and put the comment there.

> 
> > >
> > >       /* 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'.
> 
> I agree.

Will do.

Re: [PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[Rafael J. Wysocki]

On Wed, Jul 23, 2025 at 11:27 PM David Box <david.e.box@linux.intel.com> wrote:
>
> On Wed, Jul 23, 2025 at 01:54:41PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Jul 21, 2025 at 10:24 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> > >
> > > 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@linux.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.
>
> Missed this. I'll remove it.
>
> > >
> > > > -     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().
>
> This would require VMD to select PCI_HOST_COMMON just to set one field in a
> common struct. Seems heavy-handed. Thoughts? Also, with this and dropping the D0
> call, I'll split the VMD cleanup into a separate patch again.

So maybe define a __weak pci_host_set_default_pcie_link_state() doing
nothing in the ASPM core and let VMD override it with its own
implementation?

> >
> > I agree with the above except for the new function name.  I'd call it
> > pci_host_set_default_pcie_link_state()
>
> Sounds good.
>
> >
> > > > +
> > > >       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;
> >
> > Or
> >
> > link->aspm_default = pci_host_get_default_pcie_link_state(parent);
> > if (link->aspm_default)
> >         link->aspm_default = link->aspm_enabled;
> >
> > and make pci_host_get_default_pcie_link_state() return 0 on failures.
> >
> > Then you can put all of the relevant information into the
> > pci_host_get_default_pcie_link_state() kerneldoc comment.
>
> Sure. I'll add get/set APIs (plus the !) and put the comment there.

Sounds good!

> > > >
> > > >       /* 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'.
> >
> > I agree.
>
> Will do.

Thanks!

Re: [PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[Manivannan Sadhasivam]

On Thu, Jul 24, 2025 at 11:58:40AM GMT, Rafael J. Wysocki wrote:
> On Wed, Jul 23, 2025 at 11:27 PM David Box <david.e.box@linux.intel.com> wrote:
> >
> > On Wed, Jul 23, 2025 at 01:54:41PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Jul 21, 2025 at 10:24 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> > > >
> > > > 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@linux.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.
> >
> > Missed this. I'll remove it.
> >
> > > >
> > > > > -     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().
> >
> > This would require VMD to select PCI_HOST_COMMON just to set one field in a
> > common struct. Seems heavy-handed. Thoughts? Also, with this and dropping the D0
> > call, I'll split the VMD cleanup into a separate patch again.
> 
> So maybe define a __weak pci_host_set_default_pcie_link_state() doing
> nothing in the ASPM core and let VMD override it with its own
> implementation?
> 

No. There are other controller drivers (like pcie-qcom) going to use this API.
So please move it to the pci-host-common library as it should be.

> > >
> > > I agree with the above except for the new function name.  I'd call it
> > > pci_host_set_default_pcie_link_state()
> >

Ok, looks good to me.

- Mani

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

Re: [PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[David Box]

On Thu, Jul 24, 2025 at 10:18:47PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jul 24, 2025 at 11:58:40AM GMT, Rafael J. Wysocki wrote:
> > On Wed, Jul 23, 2025 at 11:27 PM David Box <david.e.box@linux.intel.com> wrote:
> > >
> > > On Wed, Jul 23, 2025 at 01:54:41PM +0200, Rafael J. Wysocki wrote:
> > > > On Mon, Jul 21, 2025 at 10:24 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> > > > >
> > > > > 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@linux.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.
> > >
> > > Missed this. I'll remove it.
> > >
> > > > >
> > > > > > -     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().
> > >
> > > This would require VMD to select PCI_HOST_COMMON just to set one field in a
> > > common struct. Seems heavy-handed. Thoughts? Also, with this and dropping the D0
> > > call, I'll split the VMD cleanup into a separate patch again.
> > 
> > So maybe define a __weak pci_host_set_default_pcie_link_state() doing
> > nothing in the ASPM core and let VMD override it with its own
> > implementation?
> > 
> 
> No. There are other controller drivers (like pcie-qcom) going to use this API.
> So please move it to the pci-host-common library as it should be.

I was going to suggest that it could simply stay in aspm.c.
pci_enable_link_state_() is there and currently only used by controllers as
well.

David

Re: [PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[Rafael J. Wysocki]

On Thu, Jul 24, 2025 at 9:08 PM David Box <david.e.box@linux.intel.com> wrote:
>
> On Thu, Jul 24, 2025 at 10:18:47PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jul 24, 2025 at 11:58:40AM GMT, Rafael J. Wysocki wrote:
> > > On Wed, Jul 23, 2025 at 11:27 PM David Box <david.e.box@linux.intel.com> wrote:
> > > >
> > > > On Wed, Jul 23, 2025 at 01:54:41PM +0200, Rafael J. Wysocki wrote:
> > > > > On Mon, Jul 21, 2025 at 10:24 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> > > > > >
> > > > > > 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@linux.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.
> > > >
> > > > Missed this. I'll remove it.
> > > >
> > > > > >
> > > > > > > -     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().
> > > >
> > > > This would require VMD to select PCI_HOST_COMMON just to set one field in a
> > > > common struct. Seems heavy-handed. Thoughts? Also, with this and dropping the D0
> > > > call, I'll split the VMD cleanup into a separate patch again.
> > >
> > > So maybe define a __weak pci_host_set_default_pcie_link_state() doing
> > > nothing in the ASPM core and let VMD override it with its own
> > > implementation?
> > >
> >
> > No. There are other controller drivers (like pcie-qcom) going to use this API.
> > So please move it to the pci-host-common library as it should be.
>
> I was going to suggest that it could simply stay in aspm.c.
> pci_enable_link_state_() is there and currently only used by controllers as
> well.

This sounds reasonable to me.

Re: [PATCH] PCI/ASPM: Allow controller drivers to override default ASPM and CLKPM link state

[Manivannan Sadhasivam]

On Thu, Jul 24, 2025 at 12:08:03PM GMT, David Box wrote:
> On Thu, Jul 24, 2025 at 10:18:47PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jul 24, 2025 at 11:58:40AM GMT, Rafael J. Wysocki wrote:
> > > On Wed, Jul 23, 2025 at 11:27 PM David Box <david.e.box@linux.intel.com> wrote:
> > > >
> > > > On Wed, Jul 23, 2025 at 01:54:41PM +0200, Rafael J. Wysocki wrote:
> > > > > On Mon, Jul 21, 2025 at 10:24 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> > > > > >
> > > > > > 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@linux.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.
> > > >
> > > > Missed this. I'll remove it.
> > > >
> > > > > >
> > > > > > > -     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().
> > > >
> > > > This would require VMD to select PCI_HOST_COMMON just to set one field in a
> > > > common struct. Seems heavy-handed. Thoughts? Also, with this and dropping the D0
> > > > call, I'll split the VMD cleanup into a separate patch again.
> > > 
> > > So maybe define a __weak pci_host_set_default_pcie_link_state() doing
> > > nothing in the ASPM core and let VMD override it with its own
> > > implementation?
> > > 
> > 
> > No. There are other controller drivers (like pcie-qcom) going to use this API.
> > So please move it to the pci-host-common library as it should be.
> 
> I was going to suggest that it could simply stay in aspm.c.
> pci_enable_link_state_() is there and currently only used by controllers as
> well.
> 

OK!

- Mani

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