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

David E. Box posted 2 patches 4 months, 4 weeks ago
drivers/pci/controller/vmd.c |  7 +++++--
drivers/pci/pcie/aspm.c      |  5 ++++-
include/linux/pci.h          | 12 ++++++++----
3 files changed, 17 insertions(+), 7 deletions(-)
[RFC 0/2] PCI/ASPM: Allow controller-defined default link state
Posted by David E. Box 4 months, 4 weeks ago
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
Re: [RFC 0/2] PCI/ASPM: Allow controller-defined default link state
Posted by Manivannan Sadhasivam 4 months, 4 weeks ago
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
Posted by Rafael J. Wysocki 4 months, 4 weeks ago
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
Posted by David Box 4 months, 4 weeks ago
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
Posted by Rafael J. Wysocki 4 months, 4 weeks ago
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
Posted by David Box 4 months, 4 weeks ago
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
Re: [RFC 0/2] PCI/ASPM: Allow controller-defined default link state
Posted by Kenneth R. Crudup 4 months, 4 weeks ago
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
Posted by Manivannan Sadhasivam 4 months, 4 weeks ago
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 0/2] PCI/ASPM: Allow controller-defined default link state
Posted by David Box 4 months, 4 weeks ago
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
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்