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

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

[David E. Box]

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().

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.

If the flag is set, the ASPM core uses the driver-supplied value in place
of the firmware one. If not, behavior is unchanged.

Only the immediate parent bus is checked for this flag. If future use cases
require deeper inheritance (e.g., through PCIe switches), the logic can be
extended to walk the bus hierarchy.

This approach avoids adding driver-specific logic to ASPM core code and
keeps the layering clean.

Testing is appreciated as I didn't get a chance to do so yet but plan to.

Thanks, David

---

David E. Box (2):
  PCI/ASPM: Allow drivers to provide ASPM link state via pci_bus
  PCI: vmd: Provide default ASPM link state for synthetic hierarchy

 drivers/pci/controller/vmd.c |  7 +++++--
 drivers/pci/pcie/aspm.c      |  5 ++++-
 include/linux/pci.h          | 12 ++++++++----
 3 files changed, 17 insertions(+), 7 deletions(-)


base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
-- 
2.43.0

[RFC 1/2] PCI/ASPM: Allow drivers to provide ASPM link state via pci_bus

[David E. Box]

Synthetic PCIe hierarchies such as those created by Intel VMD are not
enumerated or configured by firmware, and therefore do not receive
BIOS-provided ASPM defaults. This leaves devices behind such domains with
ASPM effectively disabled, despite platform intent.

Introduce a mechanism to allow the bus owner (e.g. a controller driver) to
supply a default ASPM policy via a new aspm_bus_link_state field in
pci_bus.  A new bus flag, PCI_BUS_FLAGS_ASPM_DEFAULT_OVERRIDE, indicates
that the core should use this value instead of the BIOS default when
initializing link state.

This avoids the need for controller-specific logic in ASPM core and allows
for proper power savings in these otherwise unsupported hierarchies.

Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/pci/pcie/aspm.c |  5 ++++-
 include/linux/pci.h     | 12 ++++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 29fcb0689a91..2ad1852ac9b2 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -866,7 +866,10 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	if (parent->bus->bus_flags & PCI_BUS_FLAGS_NO_ASPM_DEFAULT)
+		link->aspm_default = parent->bus->aspm_bus_link_state;
+	else
+		link->aspm_default = link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f392..7e1c305c419c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -256,10 +256,11 @@ enum pci_irq_reroute_variant {
 
 typedef unsigned short __bitwise pci_bus_flags_t;
 enum pci_bus_flags {
-	PCI_BUS_FLAGS_NO_MSI	= (__force pci_bus_flags_t) 1,
-	PCI_BUS_FLAGS_NO_MMRBC	= (__force pci_bus_flags_t) 2,
-	PCI_BUS_FLAGS_NO_AERSID	= (__force pci_bus_flags_t) 4,
-	PCI_BUS_FLAGS_NO_EXTCFG	= (__force pci_bus_flags_t) 8,
+	PCI_BUS_FLAGS_NO_MSI		= (__force pci_bus_flags_t) 1,
+	PCI_BUS_FLAGS_NO_MMRBC		= (__force pci_bus_flags_t) 2,
+	PCI_BUS_FLAGS_NO_AERSID		= (__force pci_bus_flags_t) 4,
+	PCI_BUS_FLAGS_NO_EXTCFG		= (__force pci_bus_flags_t) 8,
+	PCI_BUS_FLAGS_NO_ASPM_DEFAULT	= (__force pci_bus_flags_t) 16,
 };
 
 /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
@@ -665,6 +666,9 @@ struct pci_bus {
 	void		*sysdata;	/* Hook for sys-specific extension */
 	struct proc_dir_entry *procdir;	/* Directory entry in /proc/bus/pci */
 
+#ifdef CONFIG_PCIEASPM
+	unsigned int	aspm_bus_link_state;	/* Bus owner provided link state */
+#endif
 	unsigned char	number;		/* Bus number */
 	unsigned char	primary;	/* Number of primary bridge */
 	unsigned char	max_bus_speed;	/* enum pci_bus_speed */
-- 
2.43.0

[RFC 2/2] PCI: vmd: Provide default ASPM link state for synthetic hierarchy

[David E. Box]

Devices behind the VMD controller reside on a synthetic PCIe hierarchy that
is not visible to ACPI and not configured by firmware. As a result, these
devices receive no ASPM defaults from the BIOS, and ASPM remains disabled
unless explicitly enabled later.

Now that the ASPM core supports driver-supplied default link states via
pci_bus->aspm_bus_link_state, set this field on the VMD root bus to enable
ASPM for devices in the VMD domain. This ensures the platform's intended
power-saving configuration is applied during initialization without
requiring any special-case logic in the ASPM core.

Link: https://lore.kernel.org/linux-pm/0b166ece-eeec-ba5d-2212-50d995611cef@panix.com
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 8df064b62a2f..a0d4e96ce872 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 }
 
 /*
- * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
+ * Enable LTR settings on devices that aren't configured by BIOS.
  */
 static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 {
@@ -770,7 +770,6 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	 * PCIe r6.0, sec 5.5.4.
 	 */
 	pci_set_power_state_locked(pdev, PCI_D0);
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
 	return 0;
 }
 
@@ -911,6 +910,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 		return -ENODEV;
 	}
 
+#ifdef CONFIG_PCIEASPM
+	vmd->bus->aspm_bus_link_state = PCIE_LINK_STATE_ALL;
+#endif
+
 	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
 				   to_pci_host_bridge(vmd->bus->bridge));
 
-- 
2.43.0

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

[Kenneth R. Crudup]

Unfortunately, having tested the patch (against Linus' master), it doesn't work.

I don't think the ASPM(?) state is making it to the VMD.

LMK if you need more info.

-Kenny

On Wed, 16 Jul 2025, David E. Box wrote:

> Testing is appreciated as I didn't get a chance to do so yet but plan to.

> Thanks, David
>
> ---
>
> David E. Box (2):
>   PCI/ASPM: Allow drivers to provide ASPM link state via pci_bus
>   PCI: vmd: Provide default ASPM link state for synthetic hierarchy
>
>  drivers/pci/controller/vmd.c |  7 +++++--
>  drivers/pci/pcie/aspm.c      |  5 ++++-
>  include/linux/pci.h          | 12 ++++++++----
>  3 files changed, 17 insertions(+), 7 deletions(-)
>
>
> base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
>

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

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

[Manivannan Sadhasivam]

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

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

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

[Manivannan Sadhasivam]

On Wed, Jul 16, 2025 at 11:12:45PM GMT, Kenneth R. Crudup wrote:
> 
> Unfortunately, having tested the patch (against Linus' master), it doesn't work.
> 
> I don't think the ASPM(?) state is making it to the VMD.
> 

Because, the VMD driver is not at all setting the PCI_BUS_FLAGS_NO_ASPM_DEFAULT
flag. But I proposed an alternate method [1] to enable ASPM which would avoid
using the flag.

- Mani

[1] https://lore.kernel.org/linux-pci/4xcwba3d4slmz5gfuwypavxqreobnigzyu4vib6powtbibytyp@mmqcns27vlyr/

> LMK if you need more info.
> 
> -Kenny
> 
> On Wed, 16 Jul 2025, David E. Box wrote:
> 
> > Testing is appreciated as I didn't get a chance to do so yet but plan to.
> 
> > Thanks, David
> >
> > ---
> >
> > David E. Box (2):
> >   PCI/ASPM: Allow drivers to provide ASPM link state via pci_bus
> >   PCI: vmd: Provide default ASPM link state for synthetic hierarchy
> >
> >  drivers/pci/controller/vmd.c |  7 +++++--
> >  drivers/pci/pcie/aspm.c      |  5 ++++-
> >  include/linux/pci.h          | 12 ++++++++----
> >  3 files changed, 17 insertions(+), 7 deletions(-)
> >
> >
> > base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
> >
> 
> -- 
> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA
> 

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

Re: [RFC 1/2] PCI/ASPM: Allow drivers to provide ASPM link state via pci_bus

[Rafael J. Wysocki]

On Thu, Jul 17, 2025 at 2:40 AM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> Synthetic PCIe hierarchies such as those created by Intel VMD are not
> enumerated or configured by firmware, and therefore do not receive
> BIOS-provided ASPM defaults. This leaves devices behind such domains with
> ASPM effectively disabled, despite platform intent.
>
> Introduce a mechanism to allow the bus owner (e.g. a controller driver) to
> supply a default ASPM policy via a new aspm_bus_link_state field in
> pci_bus.  A new bus flag, PCI_BUS_FLAGS_ASPM_DEFAULT_OVERRIDE, indicates

This doesn't seem to match the code - the name of the new flag is
different there.

> that the core should use this value instead of the BIOS default when
> initializing link state.
>
> This avoids the need for controller-specific logic in ASPM core and allows
> for proper power savings in these otherwise unsupported hierarchies.

I'm guessing that VMD is supposed to set
PCI_BUS_FLAGS_NO_ASPM_DEFAULT, but that doesn't happen in patch [2/2]
AFAICS.

And I would just merge the two patches, IMV there's no reason to keep
them separate.

> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/pci/pcie/aspm.c |  5 ++++-
>  include/linux/pci.h     | 12 ++++++++----
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 29fcb0689a91..2ad1852ac9b2 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -866,7 +866,10 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>         }
>
>         /* Save default state */
> -       link->aspm_default = link->aspm_enabled;
> +       if (parent->bus->bus_flags & PCI_BUS_FLAGS_NO_ASPM_DEFAULT)
> +               link->aspm_default = parent->bus->aspm_bus_link_state;
> +       else
> +               link->aspm_default = link->aspm_enabled;

Could you avoid using the new flag by assuming that if
parent->bus->aspm_bus_link_state was zero, link->aspm_enabled would
take effect?  So the check would be something like

if (parent->bus->aspm_bus_link_state)
        link->aspm_default = parent->bus->aspm_bus_link_state;
else
        link->aspm_default = link->aspm_enabled;

>
>         /* Setup initial capable state. Will be updated later */
>         link->aspm_capable = link->aspm_support;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 05e68f35f392..7e1c305c419c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -256,10 +256,11 @@ enum pci_irq_reroute_variant {
>
>  typedef unsigned short __bitwise pci_bus_flags_t;
>  enum pci_bus_flags {
> -       PCI_BUS_FLAGS_NO_MSI    = (__force pci_bus_flags_t) 1,
> -       PCI_BUS_FLAGS_NO_MMRBC  = (__force pci_bus_flags_t) 2,
> -       PCI_BUS_FLAGS_NO_AERSID = (__force pci_bus_flags_t) 4,
> -       PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 8,
> +       PCI_BUS_FLAGS_NO_MSI            = (__force pci_bus_flags_t) 1,
> +       PCI_BUS_FLAGS_NO_MMRBC          = (__force pci_bus_flags_t) 2,
> +       PCI_BUS_FLAGS_NO_AERSID         = (__force pci_bus_flags_t) 4,
> +       PCI_BUS_FLAGS_NO_EXTCFG         = (__force pci_bus_flags_t) 8,
> +       PCI_BUS_FLAGS_NO_ASPM_DEFAULT   = (__force pci_bus_flags_t) 16,
>  };
>
>  /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
> @@ -665,6 +666,9 @@ struct pci_bus {
>         void            *sysdata;       /* Hook for sys-specific extension */
>         struct proc_dir_entry *procdir; /* Directory entry in /proc/bus/pci */
>
> +#ifdef CONFIG_PCIEASPM
> +       unsigned int    aspm_bus_link_state;    /* Bus owner provided link state */
> +#endif
>         unsigned char   number;         /* Bus number */
>         unsigned char   primary;        /* Number of primary bridge */
>         unsigned char   max_bus_speed;  /* enum pci_bus_speed */
> --

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

[Rafael J. Wysocki]

On Thu, Jul 17, 2025 at 8:55 AM Manivannan Sadhasivam <mani@kernel.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!

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

[David Box]

On Thu, Jul 17, 2025 at 12:27:47PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jul 16, 2025 at 11:12:45PM GMT, Kenneth R. Crudup wrote:
> > 
> > Unfortunately, having tested the patch (against Linus' master), it doesn't work.
> > 
> > I don't think the ASPM(?) state is making it to the VMD.
> > 
> 
> Because, the VMD driver is not at all setting the PCI_BUS_FLAGS_NO_ASPM_DEFAULT
> flag.

Correct. I forgot to set the flag in the VMD driver. Other this approach
should have worked.

David

> But I proposed an alternate method [1] to enable ASPM which would avoid
> using the flag.
> 
> - Mani
> 
> [1] https://lore.kernel.org/linux-pci/4xcwba3d4slmz5gfuwypavxqreobnigzyu4vib6powtbibytyp@mmqcns27vlyr/
> 
> > LMK if you need more info.
> > 
> > -Kenny
> > 
> > On Wed, 16 Jul 2025, David E. Box wrote:
> > 
> > > Testing is appreciated as I didn't get a chance to do so yet but plan to.
> > 
> > > Thanks, David
> > >
> > > ---
> > >
> > > David E. Box (2):
> > >   PCI/ASPM: Allow drivers to provide ASPM link state via pci_bus
> > >   PCI: vmd: Provide default ASPM link state for synthetic hierarchy
> > >
> > >  drivers/pci/controller/vmd.c |  7 +++++--
> > >  drivers/pci/pcie/aspm.c      |  5 ++++-
> > >  include/linux/pci.h          | 12 ++++++++----
> > >  3 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > >
> > > base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
> > >
> > 
> > -- 
> > Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

[David Box]

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@kernel.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

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

[Rafael J. Wysocki]

On Thu, Jul 17, 2025 at 4:13 PM David Box <david.e.box@linux.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@kernel.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.

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

[David Box]

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@linux.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@kernel.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