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] [day] [month] [year] [list]
Message-ID: <six3j62wzjgby3lpu6lixzudm7ktqhbz7or5mdfe5k4vgwt4gs@poy42cz7dox3>
Date: Thu, 17 Jul 2025 10:49:07 -0700
From: David Box <david.e.box@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: mani@...nel.org, 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

On Thu, Jul 17, 2025 at 05:37:01PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jul 17, 2025 at 4:13 PM David Box <david.e.box@...ux.intel.com> wrote:
> >
> > 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 then I can drop the flag.
> 
> Until anyone wants 0 to mean something different from "figure out the
> default settings for me", I would not use the flag.
> 

Okay. Thanks for the review. I'll send the next out as a regular patch
after testing.

David