Message-ID: <CAJZ5v0hDEX_ZMiAZU-PwriCpURiw04f=JLAVwP9UJ54wv3HBEg@mail.gmail.com>
Date: Thu, 24 Jul 2025 11:58:40 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: David Box <david.e.box@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Manivannan Sadhasivam <mani@...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 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?

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

Powered by blists - more mailing lists