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

PATCH In review David E. Box 3 participants 10 messages Jul 20, 2025 – Aug 10, 2025

Subsystems (3)

PCI PCI native host bridge and endpoint drivers PCI driver for Intel volume management device (vmd)

lore

Share & tools

lore

David E. Box

author

2025-07-20 19:01 UTC #0 lore raw

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

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/[email protected] open here

Signed-off-by David E. Box <[email protected]>

---

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

drivers/pci/controller/vmd.c +9 -3

index 8df064b62a2f..6f0de95c87fd 100644
@@ -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)

drivers/pci/pcie/aspm.c +11 -2

index 29fcb0689a91..6f5b34b172f9 100644
@@ -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;

include/linux/pci.h +4 -0

index 05e68f35f392..930028bf52b4 100644
@@ -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

Signature · 1 line

2.43.0

Manivannan Sadhasivam

maintainer

2025-07-21 08:24 UTC #1 lore raw

Quoted 54 lines · 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/[email protected] open here
Signed-off-by David E. Box <[email protected]>
---
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(-)
drivers/pci/controller/vmd.c +1 -1
index 8df064b62a2f..6f0de95c87fd 100644
@@ -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)
 {

drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 772

Quoted 3 lines

drivers/pci/controller/vmd.c +0 -0

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

Quoted 13 lines

drivers/pci/controller/vmd.c +1 -1

-	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;
 
 	/*

drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 918

Quoted 8 lines

drivers/pci/controller/vmd.c +5 -0

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

Quoted 40 lines

drivers/pci/controller/vmd.c +2 -1

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

drivers/pci/pcie/aspm.c +6 -1

index 29fcb0689a91..6f5b34b172f9 100644
@@ -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 */

drivers/pci/pcie/aspm.c static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) · line 874

Quoted 5 lines

drivers/pci/pcie/aspm.c +1 -1

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

Quoted 12 lines

drivers/pci/pcie/aspm.c +4 -0

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

include/linux/pci.h +0 -0

index 05e68f35f392..930028bf52b4 100644

include/linux/pci.h struct pci_host_bridge { · line 618

Quoted 5 lines

include/linux/pci.h +2 -0

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

Signature · 1 line

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

Rafael J. Wysocki

2025-07-23 11:54 UTC #2 lore raw

Quoted 93 lines · Manivannan Sadhasivam

Quoted 54 lines · 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/[email protected] open here
Signed-off-by David E. Box <[email protected]>
---
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(-)
drivers/pci/controller/vmd.c +1 -1
index 8df064b62a2f..6f0de95c87fd 100644
@@ -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)
 {
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 772
Quoted 3 lines
drivers/pci/controller/vmd.c +0 -0
@@ -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.
Quoted 13 lines
drivers/pci/controller/vmd.c +1 -1
-     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;
 
      /*
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 918
Quoted 8 lines
drivers/pci/controller/vmd.c +5 -0
@@ -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;

Quoted 8 lines

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

Quoted 48 lines

Quoted 40 lines

drivers/pci/controller/vmd.c +2 -1

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

drivers/pci/pcie/aspm.c +6 -1

index 29fcb0689a91..6f5b34b172f9 100644
@@ -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 */

drivers/pci/pcie/aspm.c static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) · line 874

Quoted 5 lines

drivers/pci/pcie/aspm.c +1 -1

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

Quoted 13 lines

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. */
Quoted 4 lines
drivers/pci/pcie/aspm.c +4 -0
+     if (host && host->aspm_dflt_link_state)
+             link->aspm_default = host->aspm_dflt_link_state;
+     else
+             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;

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.

Quoted 11 lines

drivers/pci/pcie/aspm.c +0 -0

      /* Setup initial capable state. Will be updated later */
      link->aspm_capable = link->aspm_support;

include/linux/pci.h +0 -0

index 05e68f35f392..930028bf52b4 100644
@@ -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 */

Quoted 8 lines

include/linux/pci.h struct pci_host_bridge { · line 618
Quoted 2 lines
include/linux/pci.h +2 -0
+#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.

Rafael J. Wysocki

2025-07-23 11:57 UTC #3 lore raw

Quoted 169 lines · Rafael J. Wysocki

Quoted 93 lines · Manivannan Sadhasivam
Quoted 54 lines · 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/[email protected] open here
Signed-off-by David E. Box <[email protected]>
---
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(-)
drivers/pci/controller/vmd.c +1 -1
index 8df064b62a2f..6f0de95c87fd 100644
@@ -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)
 {
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 772
Quoted 3 lines
drivers/pci/controller/vmd.c +0 -0
@@ -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.
Quoted 13 lines
drivers/pci/controller/vmd.c +1 -1
-     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;
 
      /*
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 918
Quoted 8 lines
drivers/pci/controller/vmd.c +5 -0
@@ -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;
Quoted 8 lines
#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()
Quoted 48 lines
Quoted 40 lines
drivers/pci/controller/vmd.c +2 -1
+
      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)
drivers/pci/pcie/aspm.c +6 -1
index 29fcb0689a91..6f5b34b172f9 100644
@@ -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 */
drivers/pci/pcie/aspm.c static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) · line 874
Quoted 5 lines
drivers/pci/pcie/aspm.c +1 -1
@@ -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.
Quoted 13 lines
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. */
Quoted 4 lines
drivers/pci/pcie/aspm.c +4 -0
+     if (host && host->aspm_dflt_link_state)
+             link->aspm_default = host->aspm_dflt_link_state;
+     else
+             link->aspm_default = link->aspm_enabled;

Quoted 4 lines

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

I omitted the ! above by mistake.

Quoted 1 line

        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;

Quoted 26 lines

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.
Quoted 11 lines
Quoted 11 lines
drivers/pci/pcie/aspm.c +0 -0
      /* Setup initial capable state. Will be updated later */
      link->aspm_capable = link->aspm_support;
include/linux/pci.h +0 -0
index 05e68f35f392..930028bf52b4 100644
@@ -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 */
 
Quoted 8 lines
include/linux/pci.h struct pci_host_bridge { · line 618
Quoted 2 lines
include/linux/pci.h +2 -0
+#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.

David Box

author

2025-07-23 21:27 UTC #4 lore raw

In reply to Rafael J. Wysocki

Quoted 50 lines · Rafael J. Wysocki

Quoted 49 lines · Manivannan Sadhasivam
Quoted 47 lines · 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/[email protected] open here
Signed-off-by David E. Box <[email protected]>
---
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(-)
drivers/pci/controller/vmd.c +0 -0
index 8df064b62a2f..6f0de95c87fd 100644
@@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 }
 

Quoted 12 lines

Quoted 4 lines
Quoted 4 lines
drivers/pci/controller/vmd.c +1 -1
 /*
- * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
+ * Enable LTR settings on devices that aren't configured by BIOS.
  */
Quoted 8 lines
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 772
Quoted 6 lines
drivers/pci/controller/vmd.c +0 -0
 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.

Quoted 35 lines

Quoted 35 lines
Quoted 14 lines
drivers/pci/controller/vmd.c +1 -1
-     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;
 
      /*
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 918
Quoted 8 lines
drivers/pci/controller/vmd.c +5 -0
@@ -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
}

Quoted 5 lines

Quoted 5 lines
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.

Quoted 3 lines

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

Sounds good.

Quoted 65 lines

Quoted 49 lines

Quoted 41 lines

drivers/pci/controller/vmd.c +2 -1

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

drivers/pci/pcie/aspm.c +6 -1

index 29fcb0689a91..6f5b34b172f9 100644
@@ -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 */

drivers/pci/pcie/aspm.c static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) · line 874

Quoted 5 lines

drivers/pci/pcie/aspm.c +1 -1

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

Quoted 13 lines

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. */
Quoted 4 lines
drivers/pci/pcie/aspm.c +4 -0
+     if (host && host->aspm_dflt_link_state)
+             link->aspm_default = host->aspm_dflt_link_state;
+     else
+             link->aspm_default = link->aspm_enabled;

Quoted 8 lines

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.

Quoted 18 lines

Quoted 9 lines

drivers/pci/pcie/aspm.c +0 -0

      /* Setup initial capable state. Will be updated later */
      link->aspm_capable = link->aspm_support;

include/linux/pci.h +0 -0

index 05e68f35f392..930028bf52b4 100644

include/linux/pci.h struct pci_host_bridge { · line 618

Quoted 5 lines

include/linux/pci.h +2 -0

@@ -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 */

Quoted 4 lines

Quoted 2 lines
Nit: Please expand 'default' as 'dflt' is not a commonly used acronym for 'default'.
I agree.

Will do.

Rafael J. Wysocki

2025-07-24 09:58 UTC #5 lore raw

Quoted 104 lines · David Box

Quoted 50 lines · Rafael J. Wysocki
Quoted 49 lines · Manivannan Sadhasivam
Quoted 47 lines · 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/[email protected] open here
Signed-off-by David E. Box <[email protected]>
---
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(-)
drivers/pci/controller/vmd.c +0 -0
index 8df064b62a2f..6f0de95c87fd 100644
@@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 }
 
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
drivers/pci/controller/vmd.c +1 -1
 /*
- * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
+ * Enable LTR settings on devices that aren't configured by BIOS.
  */
Quoted 8 lines
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 772
Quoted 6 lines
drivers/pci/controller/vmd.c +0 -0
 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.
Quoted 37 lines
Quoted 37 lines
Quoted 14 lines
drivers/pci/controller/vmd.c +1 -1
-     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;
 
      /*
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 918
Quoted 8 lines
drivers/pci/controller/vmd.c +5 -0
@@ -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.

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
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?

Quoted 75 lines

Quoted 3 lines

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

Sounds good.

Quoted 69 lines

Quoted 49 lines

Quoted 41 lines

drivers/pci/controller/vmd.c +2 -1

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

drivers/pci/pcie/aspm.c +6 -1

index 29fcb0689a91..6f5b34b172f9 100644
@@ -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 */

drivers/pci/pcie/aspm.c static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) · line 874

Quoted 5 lines

drivers/pci/pcie/aspm.c +1 -1

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

Quoted 13 lines

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. */
Quoted 4 lines
drivers/pci/pcie/aspm.c +4 -0
+     if (host && host->aspm_dflt_link_state)
+             link->aspm_default = host->aspm_dflt_link_state;
+     else
+             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;

Quoted 6 lines

Quoted 4 lines
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!

Quoted 17 lines

include/linux/pci.h struct pci_host_bridge { · line 618

Quoted 13 lines

drivers/pci/pcie/aspm.c +0 -0

      /* Setup initial capable state. Will be updated later */
      link->aspm_capable = link->aspm_support;

include/linux/pci.h +2 -0

index 05e68f35f392..930028bf52b4 100644
@@ -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 */

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Nit: Please expand 'default' as 'dflt' is not a commonly used acronym for 'default'.
I agree.
Will do.

Thanks!

Manivannan Sadhasivam

maintainer

2025-07-24 16:49 UTC #6 lore raw

Quoted 109 lines · Rafael J. Wysocki

Quoted 108 lines · David Box
Quoted 50 lines · Rafael J. Wysocki
Quoted 49 lines · Manivannan Sadhasivam
Quoted 47 lines · 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/[email protected] open here
Signed-off-by David E. Box <[email protected]>
---
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(-)
drivers/pci/controller/vmd.c +0 -0
index 8df064b62a2f..6f0de95c87fd 100644
@@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 }
 
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
drivers/pci/controller/vmd.c +1 -1
 /*
- * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
+ * Enable LTR settings on devices that aren't configured by BIOS.
  */
Quoted 8 lines
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 772
Quoted 6 lines
drivers/pci/controller/vmd.c +0 -0
 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.
Quoted 40 lines
Quoted 40 lines
Quoted 14 lines
drivers/pci/controller/vmd.c +1 -1
-     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;
 
      /*
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 918
Quoted 8 lines
drivers/pci/controller/vmd.c +5 -0
@@ -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().

Quoted 8 lines

Quoted 3 lines
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.

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
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

Signature · 1 line

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

David Box

author

2025-07-24 19:08 UTC #7 lore raw

Quoted 114 lines · Manivannan Sadhasivam

Quoted 113 lines · Rafael J. Wysocki
Quoted 111 lines · David Box
Quoted 50 lines · Rafael J. Wysocki
Quoted 49 lines · Manivannan Sadhasivam
Quoted 47 lines · 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/[email protected] open here
Signed-off-by David E. Box <[email protected]>
---
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(-)
drivers/pci/controller/vmd.c +0 -0
index 8df064b62a2f..6f0de95c87fd 100644
@@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 }
 
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
drivers/pci/controller/vmd.c +1 -1
 /*
- * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
+ * Enable LTR settings on devices that aren't configured by BIOS.
  */
Quoted 8 lines
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 772
Quoted 6 lines
drivers/pci/controller/vmd.c +0 -0
 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.
Quoted 35 lines
Quoted 35 lines
Quoted 14 lines
drivers/pci/controller/vmd.c +1 -1
-     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;
 
      /*
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 918
Quoted 8 lines
drivers/pci/controller/vmd.c +5 -0
@@ -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
}
Quoted 5 lines
Quoted 5 lines
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.

Quoted 7 lines

Quoted 4 lines
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

Rafael J. Wysocki

2025-07-24 19:12 UTC #8 lore raw

Quoted 121 lines · David Box

Quoted 119 lines · Manivannan Sadhasivam
Quoted 117 lines · Rafael J. Wysocki
Quoted 104 lines · David Box
Quoted 50 lines · Rafael J. Wysocki
Quoted 49 lines · Manivannan Sadhasivam
Quoted 47 lines · 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/[email protected] open here
Signed-off-by David E. Box <[email protected]>
---
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(-)
drivers/pci/controller/vmd.c +0 -0
index 8df064b62a2f..6f0de95c87fd 100644
@@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 }
 
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
drivers/pci/controller/vmd.c +1 -1
 /*
- * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
+ * Enable LTR settings on devices that aren't configured by BIOS.
  */
Quoted 8 lines
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 772
Quoted 6 lines
drivers/pci/controller/vmd.c +0 -0
 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.
Quoted 37 lines
Quoted 37 lines
Quoted 14 lines
drivers/pci/controller/vmd.c +1 -1
-     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;
 
      /*
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 918
Quoted 8 lines
drivers/pci/controller/vmd.c +5 -0
@@ -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.
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
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?

Quoted 6 lines

Quoted 2 lines
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.

Manivannan Sadhasivam

maintainer

2025-08-10 11:06 UTC #9 lore raw

In reply to David Box

Quoted 120 lines · David Box

Quoted 119 lines · Manivannan Sadhasivam
Quoted 117 lines · Rafael J. Wysocki
Quoted 104 lines · David Box
Quoted 50 lines · Rafael J. Wysocki
Quoted 49 lines · Manivannan Sadhasivam
Quoted 47 lines · 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/[email protected] open here
Signed-off-by David E. Box <[email protected]>
---
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(-)
drivers/pci/controller/vmd.c +0 -0
index 8df064b62a2f..6f0de95c87fd 100644
@@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 }
 
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
drivers/pci/controller/vmd.c +1 -1
 /*
- * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
+ * Enable LTR settings on devices that aren't configured by BIOS.
  */
Quoted 8 lines
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 772
Quoted 6 lines
drivers/pci/controller/vmd.c +0 -0
 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.
Quoted 37 lines
Quoted 37 lines
Quoted 14 lines
drivers/pci/controller/vmd.c +1 -1
-     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;
 
      /*
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 918
Quoted 8 lines
drivers/pci/controller/vmd.c +5 -0
@@ -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.
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
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?

Quoted 7 lines

Quoted 2 lines
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

Signature · 1 line

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