Re: [RFC 0/2] PCI/ASPM: Allow controller-defined default link state

[Rafael J. Wysocki]

On Thu, Jul 17, 2025 at 8:55 AM Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote:
>
> On Wed, Jul 16, 2025 at 05:40:24PM GMT, David E. Box wrote:
> > Hi all,
> >
> > This RFC series addresses a limitation in the PCIe ASPM subsystem where
> > devices on synthetic PCIe hierarchies, such as those created by Intel’s
> > Volume Management Device (VMD), do not receive default ASPM settings
> > because they are not visible to firmware. As a result, ASPM remains
> > disabled on these devices unless explicitly enabled later by the driver,
> > contrary to platform power-saving expectations.
> >
> > Problem with Current Behavior
> >
> > Today, ASPM default policy is set in pcie_aspm_cap_init() based on values
> > provided by BIOS. For devices under VMD, BIOS has no visibility into the
> > hierarchy, and therefore no ASPM defaults are applied. The VMD driver can
> > attempt to walk the bus hierarchy and enable ASPM post-init using runtime
> > mechanisms, but this fails when aspm_disabled is set because the kernel
> > intentionally blocks runtime ASPM changes under ACPI’s FADT_NO_ASPM flag.
> > However, this flag does not apply to VMD, which controls its domain
> > independently of firmware.
> >
> > Goal
> >
> > The ideal solution is to allow VMD or any controller driver managing a
> > synthetic hierarchy to provide a default ASPM link state at the same time
> > it's set for BIOS, in pcie_aspm_cap_init().
> >
>
> I like the idea and would like to use it to address the similar limitation on
> Qcom SoCs where the BIOS doesn't configure ASPM settings for any devices and
> sometimes there is no BIOS at all (typical for SoCs used in embedded usecases).
> So I was using pci_walk_bus() in the controller driver to enable ASPM for all
> devices, but that obviously has issues with hotplugged devices.
>
> > Solution
> >
> > 1. A new bus flag, PCI_BUS_FLAGS_ASPM_DEFAULT_OVERRIDE, based on Rafael's
> > suggestion, to signal that the driver intends to override the default ASPM
> > setting. 2. A new field, aspm_bus_link_state, in 'struct pci_bus' to supply
> > the desired default link state using the existing PCIE_LINK_STATE_XXX
> > bitmask.
> >
>
> Why would you need to make it the 'bus' specific flag? It is clear that the
> controller driver is providing the default ASPM setting. So pcie_aspm_cap_init()
> should be able to use the value provided by it for all busses.
>
> Like:
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 2ad1852ac9b2..830496e556af 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -791,6 +791,7 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
>  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  {
>         struct pci_dev *child = link->downstream, *parent = link->pdev;
> +       struct pci_host_bridge *host = pci_find_host_bridge(parent->bus);
>         u32 parent_lnkcap, child_lnkcap;
>         u16 parent_lnkctl, child_lnkctl;
>         struct pci_bus *linkbus = parent->subordinate;
> @@ -866,8 +867,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>         }
>
>         /* Save default state */
> -       if (parent->bus->bus_flags & PCI_BUS_FLAGS_NO_ASPM_DEFAULT)
> -               link->aspm_default = parent->bus->aspm_bus_link_state;
> +       if (host && host->aspm_bus_link_state)
> +               link->aspm_default = host->aspm_bus_link_state;
>         else
>                 link->aspm_default = link->aspm_enabled;
>
> This avoids the usage of the bus flag (which your series is not at all making
> use of) and allows setting the 'host_bridge::aspm_bus_link_state' easily by the
> controller drivers.

This is very similar to what I have just suggested and I like this one.

Thanks!