Hi Mani, Rafael,
On Thu, Jul 17, 2025 at 12:03:32PM +0200, Rafael J. Wysocki wrote:
> 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);
I see. This is better. I'll make this change.
> > 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.
I considered this. But 0 could technically mean that the controller wants
ASPM to be disabled. The VMD driver doesn't need to do this though and if
others don't currently need this than I can drop the flag.
David
