Message-ID: <CAJZ5v0jZrPyW9+Ccoo955Y4oje2SiAQA9aCChAoPgM28SJqf5g@mail.gmail.com>
Date: Wed, 23 Jul 2025 13:54:41 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Manivannan Sadhasivam <mani@...nel.org>, "David E. Box" <david.e.box@...ux.intel.com>
Cc: 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 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.
>
> > -     pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> >       return 0;
> >  }
> >
> > @@ -785,6 +784,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> >       resource_size_t membar2_offset = 0x2000;
> >       struct pci_bus *child;
> >       struct pci_dev *dev;
> > +     struct pci_host_bridge *vmd_host_bridge;
> >       int ret;
> >
> >       /*
> > @@ -911,8 +911,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> >               return -ENODEV;
> >       }
> >
> > +     vmd_host_bridge = to_pci_host_bridge(vmd->bus->bridge);
> > +
> > +#ifdef CONFIG_PCIEASPM
> > +     vmd_host_bridge->aspm_dflt_link_state = PCIE_LINK_STATE_ALL;
> > +#endif
>
> I think it is better to provide an API that accepts the link state. We can
> provide a stub if CONFIG_PCIEASPM is not selected. This will avoid the ifdef
> clutter in the callers. Like:
>
> void pci_set_default_link_state(struct pci_host_bridge *host_bridge,
>                                 unsigned int state)
> {
> #ifdef CONFIG_PCIEASPM
>          host_bridge->aspm_default_link_state = state;
> #endif
> }
>
> Or you can stub the entire function to align with other ASPM APIs.
>
> One more thought: Since this API is only going to be called by the host bridge
> drivers, we can place it in drivers/pci/controller/pci-host-common.c and name it
> as pci_host_common_set_default_link_state().

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

> > +
> >       vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> > -                                to_pci_host_bridge(vmd->bus->bridge));
> > +                                vmd_host_bridge);
> >
> >       vmd_attach_resources(vmd);
> >       if (vmd->irq_domain)
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 29fcb0689a91..6f5b34b172f9 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -380,6 +380,7 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> >       u16 reg16;
> >       struct pci_dev *child;
> >       struct pci_bus *linkbus = link->pdev->subordinate;
> > +     struct pci_host_bridge *host = pci_find_host_bridge(link->pdev->bus);
> >
> >       /* All functions should have the same cap and state, take the worst */
> >       list_for_each_entry(child, &linkbus->devices, bus_list) {
> > @@ -394,7 +395,10 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> >                       enabled = 0;
> >       }
> >       link->clkpm_enabled = enabled;
> > -     link->clkpm_default = enabled;
> > +     if (host && host->aspm_dflt_link_state & PCIE_LINK_STATE_CLKPM)
> > +             link->clkpm_default = 1;
> > +     else
> > +             link->clkpm_default = enabled;
> >       link->clkpm_capable = capable;
> >       link->clkpm_disable = blacklist ? 1 : 0;
> >  }
> > @@ -794,6 +798,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> >       u32 parent_lnkcap, child_lnkcap;
> >       u16 parent_lnkctl, child_lnkctl;
> >       struct pci_bus *linkbus = parent->subordinate;
> > +     struct pci_host_bridge *host;
> >
> >       if (blacklist) {
> >               /* Set enabled/disable so that we will disable ASPM later */
> > @@ -866,7 +871,11 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> >       }
> >
> >       /* Save default state */
> > -     link->aspm_default = link->aspm_enabled;
> > +     host = pci_find_host_bridge(parent->bus);
>
> You can initialize 'host' while defining it.
>
> Also, please add a comment on why we are doing this. The inline comment for the
> member is not elaborate enough:
>
>         /*
>          * Use the default link state provided by the Host Bridge driver if
>          * available. If the BIOS is not able to provide default ASPM link
>          * state for some reason, the Host Bridge driver could do.
>          */
>
> > +     if (host && host->aspm_dflt_link_state)
> > +             link->aspm_default = host->aspm_dflt_link_state;
> > +     else
> > +             link->aspm_default = link->aspm_enabled;

Or

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

and make pci_host_get_default_pcie_link_state() return 0 on failures.

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

> >
> >       /* Setup initial capable state. Will be updated later */
> >       link->aspm_capable = link->aspm_support;
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 05e68f35f392..930028bf52b4 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -614,6 +614,10 @@ struct pci_host_bridge {
> >       unsigned int    size_windows:1;         /* Enable root bus sizing */
> >       unsigned int    msi_domain:1;           /* Bridge wants MSI domain */
> >
> > +#ifdef CONFIG_PCIEASPM
> > +     unsigned int    aspm_dflt_link_state;   /* Controller provided link state */
>
>         /* Controller provided default link state */
>
>
> Nit: Please expand 'default' as 'dflt' is not a commonly used acronym for
> 'default'.

I agree.

Powered by blists - more mailing lists