Message-ID: <CAJZ5v0ggQSgQZjM5TiScY1nb4qcZ07-_OLBZbPg6tmPGc+R32A@mail.gmail.com>
Date: Wed, 23 Jul 2025 13:56:57 +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 Wed, Jul 23, 2025 at 1:54 PM Rafael J. Wysocki <rafael@...nel.org> 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.
> >
> > > -     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)

I omitted the ! above by mistake.

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

> 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