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: <CAJZ5v0h+v5pUP39vTWpNNK2D8=X2UdjUTtZ7yQHCQ2k=r2kkMg@mail.gmail.com>
Date: Thu, 17 Jul 2025 12:03:32 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: "David E. Box" <david.e.box@...ux.intel.com>, rafael@...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 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);
>         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!