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@xxxxxxxxx > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx> > --- > > 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 -- மணிவண்ணன் சதாசிவம்
