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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <tbj67d2j4bzf3em5nw73w354lqji3baurajbseyouls53odjxq@4edjrxtdaeum>
Date: Thu, 17 Jul 2025 07:13:38 -0700
From: David Box <david.e.box@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>, mani@...nel.org
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: [RFC 0/2] PCI/ASPM: Allow controller-defined default link state

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@...nel.org> 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