Message-ID: <4xcwba3d4slmz5gfuwypavxqreobnigzyu4vib6powtbibytyp@mmqcns27vlyr>
Date: Thu, 17 Jul 2025 12:25:20 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: "David E. Box" <david.e.box@...ux.intel.com>
Cc: 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 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.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists