| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0hLJwOe-__7DpEEOcZpxan12VOUhucv+WxkYuh-Q+YOGQ@mail.gmail.com>
Date: Thu, 24 Jul 2025 21:12:45 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: David Box <david.e.box@...ux.intel.com>
Cc: Manivannan Sadhasivam <mani@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, bhelgaas@...gle.com,
vicamo.yang@...onical.com, kenny@...ix.com, ilpo.jarvinen@...ux.intel.com,
nirmal.patel@...ux.intel.com, linux-pm@...r.kernel.org,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI/ASPM: Allow controller drivers to override default
ASPM and CLKPM link state
On Thu, Jul 24, 2025 at 9:08 PM David Box <david.e.box@...ux.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@...ux.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@...nel.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@...ux.intel.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes from RFC:
> > > > > > >
> > > > > > > -- Rename field to aspm_dflt_link_state since it stores
> > > > > > > PCIE_LINK_STATE_XXX flags, not a policy enum.
> > > > > > > -- Move the field to struct pci_host_bridge since it's being applied to
> > > > > > > the entire host bridge per Mani's suggestion.
> > > > > > > -- During testing noticed that clkpm remained disabled and this was
> > > > > > > also handled by the formerly used pci_enable_link_state(). Add a
> > > > > > > check in pcie_clkpm_cap_init() as well to enable clkpm during init.
> > > > > > >
> > > > > > > drivers/pci/controller/vmd.c | 12 +++++++++---
> > > > > > > drivers/pci/pcie/aspm.c | 13 +++++++++++--
> > > > > > > include/linux/pci.h | 4 ++++
> > > > > > > 3 files changed, 24 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > > > > > index 8df064b62a2f..6f0de95c87fd 100644
> > > > > > > --- a/drivers/pci/controller/vmd.c
> > > > > > > +++ b/drivers/pci/controller/vmd.c
> > > > > > > @@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
> > > > > > > }
> > > > > > >
> > > > > > > /*
> > > > > > > - * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> > > > > > > + * Enable LTR settings on devices that aren't configured by BIOS.
> > > > > > > */
> > > > > > > static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > > > > > > {
> > > > > > > @@ -770,7 +770,6 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > > > > > > * PCIe r6.0, sec 5.5.4.
> > > > > > > */
> > > > > > > pci_set_power_state_locked(pdev, PCI_D0);
> > > > > >
> > > > > > This call becomes useless now.
> > > >
> > > > 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.
Powered by blists - more mailing lists