Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

[+cc Mika, Sathy, Rafael, David, Ilpo, Ricky, Mario, linux-pci]

On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> 
> I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe behind a
> VMD device:
> 
> ----
> [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family: 0x6, model: 0x9a, stepping: 0x3)
> ----
> 0000:00:0e.0 0104: 8086:467f
>         Subsystem: 1028:0af3
>         Flags: bus master, fast devsel, latency 0, IOMMU group 9
>         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
>         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
>         Capabilities: <access denied>
>         Kernel driver in use: vmd
> ----
> 
> The only release kernel that was able to get this laptop to fully get into
> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from
> Ubuntu
> (remote git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar).
> 
> I'd bisected it to the following commits (in this order):
> 
> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

Thanks for these.  You don't happen to have URLs for those Ubuntu
commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
(which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
Substates Capability for suspend/resume"")).

> Without the patches I never see Pkg%PC8 or higher(? lower?), nor i915 states
> DC5/6, all necssary for SYS%LPI/CPU%LPI. I've attached a little script I use
> alongside turbostat for verifying low-power operation (and also for seeing
> what chipset subsystem may be preventing it).
> 
> The first two are in Linus' trees, but were reverted (4ff116d0d5fd in
> a7152be79b6, 5e85eba6f50d in ff209ecc376a). The last three come from Ubuntu's
> Linux trees (see remote spec above). The first two remain reverted in the
> Ubuntu trees, but if I put them back, I get increased power savings during
> suspend/resume cycles.
> 
> Considering the power draw is really significant without these patches (10s
> of %s per hour) and I'd think Dell would have sold some decent number of
> these laptops, I'd been patiently waiting for these patches, or some variant
> to show up in the stable trees, but so far I'm up to the 6.6 stable kernel
> and still having to manually cherry-pick these, so I thought maybe I could
> bring this to the PM maintainers' attention so at least start a discussion
> about this issue.

Thank you very much for raising this again.  We really need to make
some progress, and Mika recently posted a patch to add the
4ff116d0d5fd functionality again:
https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com

The big problem is that it works on *most* systems, but it still seems
to break a few.  So Mika's current patch relies on a denylist of
systems where we *don't* restore the substates.

It's possible we'll have to give in and use a denylist, but it's
obviously not ideal because (a) we don't know *why* it doesn't work on
those systems, and (b) it means substates work before suspend but not
after resume, which is a poor user experience.

Bjorn

> #!/bin/bash -e
> date
> egrep -Hr . /sys/class/drm/card0/power/rc6_residency_ms \
>  /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us \
>  /sys/kernel/debug/pmc_core/package_cstate_show \
>  /sys/kernel/debug/pmc_core/slp_s0_residency_usec \
>  /sys/kernel/debug/dri/0/i915_edp_psr_status \
>  /sys/kernel/debug/dri/0/i915_dmc_info | tee -a ~/Dropbox/XPS-7390/sleep-params
> egrep '\(ns\): [^0]' /sys/kernel/debug/pmc_core/ltr_show | cut -d'	' -f1,3,4 | sed -e 's;[	][	]*; ;' | tee -a ~/Dropbox/XPS-7390/sleep-params
> egrep -Hr ": On" /sys/kernel/debug/pmc_core/pch_ip_power_gating_status | tee -a /dev/tty | tee -a ~/Dropbox/XPS-7390/sleep-params | wc -l
> egrep No /sys/kernel/debug/pmc_core/slp_s0_debug_status 2>/dev/null | tee -a ~/Dropbox/XPS-7390/sleep-params

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Mika Westerberg]

Hi,

On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> [+cc Mika, Sathy, Rafael, David, Ilpo, Ricky, Mario, linux-pci]
> 
> On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > 
> > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe behind a
> > VMD device:
> > 
> > ----
> > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family: 0x6, model: 0x9a, stepping: 0x3)
> > ----
> > 0000:00:0e.0 0104: 8086:467f
> >         Subsystem: 1028:0af3
> >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
> >         Capabilities: <access denied>
> >         Kernel driver in use: vmd
> > ----
> > 
> > The only release kernel that was able to get this laptop to fully get into
> > low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from
> > Ubuntu
> > (remote git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar).
> > 
> > I'd bisected it to the following commits (in this order):
> > 
> > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
> 
> Thanks for these.  You don't happen to have URLs for those Ubuntu
> commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> Substates Capability for suspend/resume"")).
> 
> > Without the patches I never see Pkg%PC8 or higher(? lower?), nor i915 states
> > DC5/6, all necssary for SYS%LPI/CPU%LPI. I've attached a little script I use
> > alongside turbostat for verifying low-power operation (and also for seeing
> > what chipset subsystem may be preventing it).
> > 
> > The first two are in Linus' trees, but were reverted (4ff116d0d5fd in
> > a7152be79b6, 5e85eba6f50d in ff209ecc376a). The last three come from Ubuntu's
> > Linux trees (see remote spec above). The first two remain reverted in the
> > Ubuntu trees, but if I put them back, I get increased power savings during
> > suspend/resume cycles.
> > 
> > Considering the power draw is really significant without these patches (10s
> > of %s per hour) and I'd think Dell would have sold some decent number of
> > these laptops, I'd been patiently waiting for these patches, or some variant
> > to show up in the stable trees, but so far I'm up to the 6.6 stable kernel
> > and still having to manually cherry-pick these, so I thought maybe I could
> > bring this to the PM maintainers' attention so at least start a discussion
> > about this issue.
> 
> Thank you very much for raising this again.  We really need to make
> some progress, and Mika recently posted a patch to add the
> 4ff116d0d5fd functionality again:
> https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> 
> The big problem is that it works on *most* systems, but it still seems
> to break a few.  So Mika's current patch relies on a denylist of
> systems where we *don't* restore the substates.

According to latest reports it is just that one system where this is
still an issue. The latest patch works in Asus UX305FA even if it is not
in the denylist. That would leave that one system only to the denylist,
at least the ones we are aware about.

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

Hi Mika, Bjorn,

On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> Hi,
> 
> On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > [+cc Mika, Sathy, Rafael, David, Ilpo, Ricky, Mario, linux-pci]
> > 
> > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > 
> > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe behind a
> > > VMD device:
> > > 
> > > ----
> > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family:
> > > 0x6, model: 0x9a, stepping: 0x3)
> > > ----
> > > 0000:00:0e.0 0104: 8086:467f
> > >         Subsystem: 1028:0af3
> > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > [size=1M]
> > >         Capabilities: <access denied>
> > >         Kernel driver in use: vmd
> > > ----
> > > 
> > > The only release kernel that was able to get this laptop to fully get into
> > > low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from
> > > Ubuntu
> > > (remote git://git.launchpad.net/~ubuntu-
> > > kernel/ubuntu/+source/linux/+git/lunar).
> > > 
> > > I'd bisected it to the following commits (in this order):
> > > 
> > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > programming
> > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > domain
> > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
> > 
> > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > Substates Capability for suspend/resume"")).
> > 
> > > Without the patches I never see Pkg%PC8 or higher(? lower?), nor i915
> > > states
> > > DC5/6, all necssary for SYS%LPI/CPU%LPI. I've attached a little script I
> > > use
> > > alongside turbostat for verifying low-power operation (and also for seeing
> > > what chipset subsystem may be preventing it).
> > > 
> > > The first two are in Linus' trees, but were reverted (4ff116d0d5fd in
> > > a7152be79b6, 5e85eba6f50d in ff209ecc376a). The last three come from
> > > Ubuntu's
> > > Linux trees (see remote spec above). The first two remain reverted in the
> > > Ubuntu trees, but if I put them back, I get increased power savings during
> > > suspend/resume cycles.
> > > 
> > > Considering the power draw is really significant without these patches
> > > (10s
> > > of %s per hour) and I'd think Dell would have sold some decent number of
> > > these laptops, I'd been patiently waiting for these patches, or some
> > > variant
> > > to show up in the stable trees, but so far I'm up to the 6.6 stable kernel
> > > and still having to manually cherry-pick these, so I thought maybe I could
> > > bring this to the PM maintainers' attention so at least start a discussion
> > > about this issue.
> > 
> > Thank you very much for raising this again.  We really need to make
> > some progress, and Mika recently posted a patch to add the
> > 4ff116d0d5fd functionality again:
> > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > 
> > The big problem is that it works on *most* systems, but it still seems
> > to break a few.  So Mika's current patch relies on a denylist of
> > systems where we *don't* restore the substates.
> 
> According to latest reports it is just that one system where this is
> still an issue. The latest patch works in Asus UX305FA even if it is not
> in the denylist. That would leave that one system only to the denylist,
> at least the ones we are aware about.

I've been working with Thomas, whose system is the last known to have problems
with Mika's patch. It turns out that his config sets aspm_policy to 'powersave'.
If he sets it to any other policy, Mika's patch works [1]. It's possible that
others may see the same issue if they use 'powersave' as well.

The theory right now is that enabling L1SS in pci_restore_state() is too early.
During boot, if ASPM policy is 'powersave' or 'powersupersave', ASPM enabling is
deferred. The comment in pcie_aspm_init_link_state() that skips it state that:

        /*
         * At this stage drivers haven't had an opportunity to change the
         * link policy setting. Enabling ASPM on broken hardware can cripple
         * it even before the driver has had a chance to disable ASPM, so
         * default to a safe level right now. If we're enabling ASPM beyond
         * the BIOS's expectation, we'll do so once pci_enable_device() is
         * called.
         */

While pci_enable_device() is called by the PCI core before pci_restore_state()
on resume, it is called again later by the nvme driver in nvme_pci_enable().
This stage seems the intended intercept mentioned in the comment. This ends up
calling pcie_aspm_powersave_config_link() to configure ASPM at that time. During
boot we see ASPM enabling is indeed happening for powersave during
nvme_pci_enable(). With the save/restore patch however it is being restored
before nvme_pci_enable(). I've asked Thomas not to apply Mika's patch, but
instead use a different patch [2] that waits until
pcie_aspm_powersave_config_link() is called to configure ASPM. The need for this
is mentioned below. Hopefully it will fix the hang observed on his system.

Whether that patch works for him, we can address his problem with the current
L1SS save/restore patch by removing the current denylist and instead only do the
save/restore if ASPM policy is 'default' which doesn't hang his system. This
makes sense since it's only the BIOS config that we care to preserve since it
can be lost during suspend, particularly during s2idle. All other policies are
OS controlled if allowed. Instead of save/restore for those we can let it be
configured later when pcie_aspm_powersave_config_link() is called.

The only issue with this is that pcie_aspm_powersave_config_link() will not
configure ASPM if aspm_policy has not changed. This is a problem because we
observed that after resume from S3, BIOS has reenabled L1SS. So we can boot with
powersave (which disables L1SS) but resume with L1SS enabled and policy still
set to powersave. This is a preexisting bug. I've observed this behavior on
Thomas's system and with mainline on our desktop systems. This is the reason for
patch [2]. It will force ASPM to be configured in
pcie_aspm_powersave_config_link() even if the policy is the same. It works on my
system. I'm hoping that it will work on his system to resume successfully with
the correct policy enabled.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=216877#c33
[2] https://bugzilla.kernel.org/attachment.cgi?id=305395&action=diff

David

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

[+cc Matthew, author of 41cd766b0659 ("PCI: Don't enable aspm before drivers have had a chance to veto it")]

On Thu, Nov 16, 2023 at 12:10:02PM -0800, David E. Box wrote:
> On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> > On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > > 
> > > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe
> > > > behind a VMD device:
> > > > 
> > > > ----
> > > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family:
> > > > 0x6, model: 0x9a, stepping: 0x3)
> > > > ----
> > > > 0000:00:0e.0 0104: 8086:467f
> > > >         Subsystem: 1028:0af3
> > > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > > [size=1M]
> > > >         Capabilities: <access denied>
> > > >         Kernel driver in use: vmd
> > > > ----
> > > > 
> > > > The only release kernel that was able to get this laptop to
> > > > fully get into low-power (unfortunately only s0ix) was the
> > > > Ubuntu-6.2.0- ... series from Ubuntu (remote
> > > > git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar).
> > > > 
> > > > I'd bisected it to the following commits (in this order):
> > > > 
> > > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > > programming
> > > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > domain
> > > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> > > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
> > > 
> > > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > > Substates Capability for suspend/resume"")).
> > > 
> > > > Without the patches I never see Pkg%PC8 or higher(? lower?),
> > > > nor i915 states DC5/6, all necssary for SYS%LPI/CPU%LPI. I've
> > > > attached a little script I use alongside turbostat for
> > > > verifying low-power operation (and also for seeing what
> > > > chipset subsystem may be preventing it).
> > > > 
> > > > The first two are in Linus' trees, but were reverted
> > > > (4ff116d0d5fd in a7152be79b6, 5e85eba6f50d in ff209ecc376a).
> > > > The last three come from Ubuntu's Linux trees (see remote spec
> > > > above). The first two remain reverted in the Ubuntu trees, but
> > > > if I put them back, I get increased power savings during
> > > > suspend/resume cycles.
> > > > 
> > > > Considering the power draw is really significant without these
> > > > patches (10s of %s per hour) and I'd think Dell would have
> > > > sold some decent number of these laptops, I'd been patiently
> > > > waiting for these patches, or some variant to show up in the
> > > > stable trees, but so far I'm up to the 6.6 stable kernel and
> > > > still having to manually cherry-pick these, so I thought maybe
> > > > I could bring this to the PM maintainers' attention so at
> > > > least start a discussion about this issue.
> > > 
> > > Thank you very much for raising this again.  We really need to make
> > > some progress, and Mika recently posted a patch to add the
> > > 4ff116d0d5fd functionality again:
> > > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > > 
> > > The big problem is that it works on *most* systems, but it still
> > > seems to break a few.  So Mika's current patch relies on a
> > > denylist of systems where we *don't* restore the substates.
> > 
> > According to latest reports it is just that one system where this
> > is still an issue. The latest patch works in Asus UX305FA even if
> > it is not in the denylist. That would leave that one system only
> > to the denylist, at least the ones we are aware about.
> 
> I've been working with Thomas, whose system is the last known to
> have problems with Mika's patch. It turns out that his config sets
> aspm_policy to 'powersave'.  If he sets it to any other policy,
> Mika's patch works [1]. It's possible that others may see the same
> issue if they use 'powersave' as well.
> 
> The theory right now is that enabling L1SS in pci_restore_state() is
> too early.  

I'd really like to figure out what "too early" means.  We can make it
later by enabling L1SS somewhere else, but unless we know exactly what
needs to happen first, we're likely to break it again.  And if we know
what's required, we can probably figure out a cleaner way to restore
it.

> During boot, if ASPM policy is 'powersave' or
> 'powersupersave', ASPM enabling is deferred. The comment in
> pcie_aspm_init_link_state() that skips it state that:
> 
>         /*
>          * At this stage drivers haven't had an opportunity to change the
>          * link policy setting. Enabling ASPM on broken hardware can cripple
>          * it even before the driver has had a chance to disable ASPM, so
>          * default to a safe level right now. If we're enabling ASPM beyond
>          * the BIOS's expectation, we'll do so once pci_enable_device() is
>          * called.

This is from 41cd766b0659 ("PCI: Don't enable aspm before drivers have
had a chance to veto it") [3].  I assume the idea is that driver probe
methods can use pci_disable_link_state() to veto certain link states.

I think this would be better as a quirk instead of a driver probe
method because I don't think ASPM really has anything to do with the
driver probe.  We do most ASPM configuration at enumeration (before
driver probe), so now we have this exception that we delay it until
probe time if the policy is POLICY_POWERSAVE or
POLICY_POWER_SUPERSAVE.

There are only about a dozen callers of pci_disable_link_state(), so
it doesn't seem impossible to change them to use quirks instead, e.g.,
quirk_disable_aspm_l0s() and quirk_disable_aspm_l0s_l1().

> While pci_enable_device() is called by the PCI core before
> pci_restore_state() on resume, it is called again later by the nvme
> driver in nvme_pci_enable().  This stage seems the intended
> intercept mentioned in the comment. This ends up calling
> pcie_aspm_powersave_config_link() to configure ASPM at that time.
> During boot we see ASPM enabling is indeed happening for powersave
> during nvme_pci_enable(). With the save/restore patch however it is
> being restored before nvme_pci_enable(). I've asked Thomas not to
> apply Mika's patch, but instead use a different patch [2] that waits
> until pcie_aspm_powersave_config_link() is called to configure ASPM.
> The need for this is mentioned below. Hopefully it will fix the hang
> observed on his system.
> 
> Whether that patch works for him, we can address his problem with
> the current L1SS save/restore patch by removing the current denylist
> and instead only do the save/restore if ASPM policy is 'default'
> which doesn't hang his system. This makes sense since it's only the
> BIOS config that we care to preserve since it can be lost during
> suspend, particularly during s2idle. All other policies are OS
> controlled if allowed. Instead of save/restore for those we can let
> it be configured later when pcie_aspm_powersave_config_link() is
> called.
> 
> The only issue with this is that pcie_aspm_powersave_config_link()
> will not configure ASPM if aspm_policy has not changed. This is a
> problem because we observed that after resume from S3, BIOS has
> reenabled L1SS. So we can boot with powersave (which disables L1SS)
> but resume with L1SS enabled and policy still set to powersave. This
> is a preexisting bug. I've observed this behavior on Thomas's system
> and with mainline on our desktop systems. This is the reason for
> patch [2]. It will force ASPM to be configured in
> pcie_aspm_powersave_config_link() even if the policy is the same. It
> works on my system. I'm hoping that it will work on his system to
> resume successfully with the correct policy enabled.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=216877#c33
> [2] https://bugzilla.kernel.org/attachment.cgi?id=305395&action=diff

[3] https://git.kernel.org/linus/41cd766b0659

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Matthew Garrett]

On Thu, Nov 16, 2023 at 05:18:12PM -0600, Bjorn Helgaas wrote:

> I think this would be better as a quirk instead of a driver probe
> method because I don't think ASPM really has anything to do with the
> driver probe.  We do most ASPM configuration at enumeration (before
> driver probe), so now we have this exception that we delay it until
> probe time if the policy is POLICY_POWERSAVE or
> POLICY_POWER_SUPERSAVE.

I think doing this as a quirk would probably work fine, but from an 
aesthetic point of view it feels awkward - this is knowledge that the 
drivers have, and so fundamentally placing that knowledge in the core 
PCI code feels like the wrong place to put it.

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

On Thu, 2023-11-16 at 17:18 -0600, Bjorn Helgaas wrote:
> [+cc Matthew, author of 41cd766b0659 ("PCI: Don't enable aspm before drivers
> have had a chance to veto it")]
> 
> On Thu, Nov 16, 2023 at 12:10:02PM -0800, David E. Box wrote:
> > On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> > > On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > > > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > > > 
> > > > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe
> > > > > behind a VMD device:
> > > > > 
> > > > > ----
> > > > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P
> > > > > (family:
> > > > > 0x6, model: 0x9a, stepping: 0x3)
> > > > > ----
> > > > > 0000:00:0e.0 0104: 8086:467f
> > > > >         Subsystem: 1028:0af3
> > > > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > > > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > > > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > > > [size=1M]
> > > > >         Capabilities: <access denied>
> > > > >         Kernel driver in use: vmd
> > > > > ----
> > > > > 
> > > > > The only release kernel that was able to get this laptop to
> > > > > fully get into low-power (unfortunately only s0ix) was the
> > > > > Ubuntu-6.2.0- ... series from Ubuntu (remote
> > > > > git://git.launchpad.net/~ubuntu-
> > > > > kernel/ubuntu/+source/linux/+git/lunar).
> > > > > 
> > > > > I'd bisected it to the following commits (in this order):
> > > > > 
> > > > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for
> > > > > suspend/resume
> > > > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > > > programming
> > > > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > > domain
> > > > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
> > > > > VMD
> > > > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
> > > > > instead
> > > > 
> > > > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > > > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > > > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > > > Substates Capability for suspend/resume"")).
> > > > 
> > > > > Without the patches I never see Pkg%PC8 or higher(? lower?),
> > > > > nor i915 states DC5/6, all necssary for SYS%LPI/CPU%LPI. I've
> > > > > attached a little script I use alongside turbostat for
> > > > > verifying low-power operation (and also for seeing what
> > > > > chipset subsystem may be preventing it).
> > > > > 
> > > > > The first two are in Linus' trees, but were reverted
> > > > > (4ff116d0d5fd in a7152be79b6, 5e85eba6f50d in ff209ecc376a).
> > > > > The last three come from Ubuntu's Linux trees (see remote spec
> > > > > above). The first two remain reverted in the Ubuntu trees, but
> > > > > if I put them back, I get increased power savings during
> > > > > suspend/resume cycles.
> > > > > 
> > > > > Considering the power draw is really significant without these
> > > > > patches (10s of %s per hour) and I'd think Dell would have
> > > > > sold some decent number of these laptops, I'd been patiently
> > > > > waiting for these patches, or some variant to show up in the
> > > > > stable trees, but so far I'm up to the 6.6 stable kernel and
> > > > > still having to manually cherry-pick these, so I thought maybe
> > > > > I could bring this to the PM maintainers' attention so at
> > > > > least start a discussion about this issue.
> > > > 
> > > > Thank you very much for raising this again.  We really need to make
> > > > some progress, and Mika recently posted a patch to add the
> > > > 4ff116d0d5fd functionality again:
> > > > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > > > 
> > > > The big problem is that it works on *most* systems, but it still
> > > > seems to break a few.  So Mika's current patch relies on a
> > > > denylist of systems where we *don't* restore the substates.
> > > 
> > > According to latest reports it is just that one system where this
> > > is still an issue. The latest patch works in Asus UX305FA even if
> > > it is not in the denylist. That would leave that one system only
> > > to the denylist, at least the ones we are aware about.
> > 
> > I've been working with Thomas, whose system is the last known to
> > have problems with Mika's patch. It turns out that his config sets
> > aspm_policy to 'powersave'.  If he sets it to any other policy,
> > Mika's patch works [1]. It's possible that others may see the same
> > issue if they use 'powersave' as well.
> > 
> > The theory right now is that enabling L1SS in pci_restore_state() is
> > too early.  
> 
> I'd really like to figure out what "too early" means.  We can make it
> later by enabling L1SS somewhere else, but unless we know exactly what
> needs to happen first, we're likely to break it again.  And if we know
> what's required, we can probably figure out a cleaner way to restore
> it.

Still trying to understand this particular failure. The current patch to Thomas
more closely mimics how ASPM is enabled during boot when powersave is set. If it
works we can at least prove that we can get it to work again by using a similar
flow.

David

> 
> > During boot, if ASPM policy is 'powersave' or
> > 'powersupersave', ASPM enabling is deferred. The comment in
> > pcie_aspm_init_link_state() that skips it state that:
> > 
> >         /*
> >          * At this stage drivers haven't had an opportunity to change the
> >          * link policy setting. Enabling ASPM on broken hardware can cripple
> >          * it even before the driver has had a chance to disable ASPM, so
> >          * default to a safe level right now. If we're enabling ASPM beyond
> >          * the BIOS's expectation, we'll do so once pci_enable_device() is
> >          * called.
> 
> This is from 41cd766b0659 ("PCI: Don't enable aspm before drivers have
> had a chance to veto it") [3].  I assume the idea is that driver probe
> methods can use pci_disable_link_state() to veto certain link states.
> 
> I think this would be better as a quirk instead of a driver probe
> method because I don't think ASPM really has anything to do with the
> driver probe.  We do most ASPM configuration at enumeration (before
> driver probe), so now we have this exception that we delay it until
> probe time if the policy is POLICY_POWERSAVE or
> POLICY_POWER_SUPERSAVE.
> 
> There are only about a dozen callers of pci_disable_link_state(), so
> it doesn't seem impossible to change them to use quirks instead, e.g.,
> quirk_disable_aspm_l0s() and quirk_disable_aspm_l0s_l1().
> 
> > While pci_enable_device() is called by the PCI core before
> > pci_restore_state() on resume, it is called again later by the nvme
> > driver in nvme_pci_enable().  This stage seems the intended
> > intercept mentioned in the comment. This ends up calling
> > pcie_aspm_powersave_config_link() to configure ASPM at that time.
> > During boot we see ASPM enabling is indeed happening for powersave
> > during nvme_pci_enable(). With the save/restore patch however it is
> > being restored before nvme_pci_enable(). I've asked Thomas not to
> > apply Mika's patch, but instead use a different patch [2] that waits
> > until pcie_aspm_powersave_config_link() is called to configure ASPM.
> > The need for this is mentioned below. Hopefully it will fix the hang
> > observed on his system.
> > 
> > Whether that patch works for him, we can address his problem with
> > the current L1SS save/restore patch by removing the current denylist
> > and instead only do the save/restore if ASPM policy is 'default'
> > which doesn't hang his system. This makes sense since it's only the
> > BIOS config that we care to preserve since it can be lost during
> > suspend, particularly during s2idle. All other policies are OS
> > controlled if allowed. Instead of save/restore for those we can let
> > it be configured later when pcie_aspm_powersave_config_link() is
> > called.
> > 
> > The only issue with this is that pcie_aspm_powersave_config_link()
> > will not configure ASPM if aspm_policy has not changed. This is a
> > problem because we observed that after resume from S3, BIOS has
> > reenabled L1SS. So we can boot with powersave (which disables L1SS)
> > but resume with L1SS enabled and policy still set to powersave. This
> > is a preexisting bug. I've observed this behavior on Thomas's system
> > and with mainline on our desktop systems. This is the reason for
> > patch [2]. It will force ASPM to be configured in
> > pcie_aspm_powersave_config_link() even if the policy is the same. It
> > works on my system. I'm hoping that it will work on his system to
> > resume successfully with the correct policy enabled.
> > 
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=216877#c33
> > [2] https://bugzilla.kernel.org/attachment.cgi?id=305395&action=diff
> 
> [3] https://git.kernel.org/linus/41cd766b0659

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

On Fri, 2023-11-17 at 16:21 -0800, David E. Box wrote:
> On Thu, 2023-11-16 at 17:18 -0600, Bjorn Helgaas wrote:
> > [+cc Matthew, author of 41cd766b0659 ("PCI: Don't enable aspm before drivers
> > have had a chance to veto it")]
> > 
> > On Thu, Nov 16, 2023 at 12:10:02PM -0800, David E. Box wrote:
> > > On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> > > > On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > > > > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > > > > 
> > > > > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe
> > > > > > behind a VMD device:
> > > > > > 
> > > > > > ----
> > > > > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P
> > > > > > (family:
> > > > > > 0x6, model: 0x9a, stepping: 0x3)
> > > > > > ----
> > > > > > 0000:00:0e.0 0104: 8086:467f
> > > > > >         Subsystem: 1028:0af3
> > > > > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > > > > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > > > > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > > > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > > > > [size=1M]
> > > > > >         Capabilities: <access denied>
> > > > > >         Kernel driver in use: vmd
> > > > > > ----
> > > > > > 
> > > > > > The only release kernel that was able to get this laptop to
> > > > > > fully get into low-power (unfortunately only s0ix) was the
> > > > > > Ubuntu-6.2.0- ... series from Ubuntu (remote
> > > > > > git://git.launchpad.net/~ubuntu-
> > > > > > kernel/ubuntu/+source/linux/+git/lunar).
> > > > > > 
> > > > > > I'd bisected it to the following commits (in this order):
> > > > > > 
> > > > > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for
> > > > > > suspend/resume
> > > > > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > > > > programming
> > > > > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under
> > > > > > VMD
> > > > > > domain
> > > > > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints
> > > > > > behind
> > > > > > VMD
> > > > > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
> > > > > > instead
> > > > > 
> > > > > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > > > > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > > > > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > > > > Substates Capability for suspend/resume"")).
> > > > > 
> > > > > > Without the patches I never see Pkg%PC8 or higher(? lower?),
> > > > > > nor i915 states DC5/6, all necssary for SYS%LPI/CPU%LPI. I've
> > > > > > attached a little script I use alongside turbostat for
> > > > > > verifying low-power operation (and also for seeing what
> > > > > > chipset subsystem may be preventing it).
> > > > > > 
> > > > > > The first two are in Linus' trees, but were reverted
> > > > > > (4ff116d0d5fd in a7152be79b6, 5e85eba6f50d in ff209ecc376a).
> > > > > > The last three come from Ubuntu's Linux trees (see remote spec
> > > > > > above). The first two remain reverted in the Ubuntu trees, but
> > > > > > if I put them back, I get increased power savings during
> > > > > > suspend/resume cycles.
> > > > > > 
> > > > > > Considering the power draw is really significant without these
> > > > > > patches (10s of %s per hour) and I'd think Dell would have
> > > > > > sold some decent number of these laptops, I'd been patiently
> > > > > > waiting for these patches, or some variant to show up in the
> > > > > > stable trees, but so far I'm up to the 6.6 stable kernel and
> > > > > > still having to manually cherry-pick these, so I thought maybe
> > > > > > I could bring this to the PM maintainers' attention so at
> > > > > > least start a discussion about this issue.
> > > > > 
> > > > > Thank you very much for raising this again.  We really need to make
> > > > > some progress, and Mika recently posted a patch to add the
> > > > > 4ff116d0d5fd functionality again:
> > > > > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > > > > 
> > > > > The big problem is that it works on *most* systems, but it still
> > > > > seems to break a few.  So Mika's current patch relies on a
> > > > > denylist of systems where we *don't* restore the substates.
> > > > 
> > > > According to latest reports it is just that one system where this
> > > > is still an issue. The latest patch works in Asus UX305FA even if
> > > > it is not in the denylist. That would leave that one system only
> > > > to the denylist, at least the ones we are aware about.
> > > 
> > > I've been working with Thomas, whose system is the last known to
> > > have problems with Mika's patch. It turns out that his config sets
> > > aspm_policy to 'powersave'.  If he sets it to any other policy,
> > > Mika's patch works [1]. It's possible that others may see the same
> > > issue if they use 'powersave' as well.
> > > 
> > > The theory right now is that enabling L1SS in pci_restore_state() is
> > > too early.  
> > 
> > I'd really like to figure out what "too early" means.  We can make it
> > later by enabling L1SS somewhere else, but unless we know exactly what
> > needs to happen first, we're likely to break it again.  And if we know
> > what's required, we can probably figure out a cleaner way to restore
> > it.
> 
> Still trying to understand this particular failure. The current patch to
> Thomas
> more closely mimics how ASPM is enabled during boot when powersave is set. If
> it
> works we can at least prove that we can get it to work again by using a
> similar
> flow.


With some free time I was able to find a system in our lab that reproduces the
same failure reported on the last problem report from Thomas. That is, with
powersave selected, the nvme fails to come up after resume from S3 with this
patch without a quirk. It's actually obvious when you can see the flow. We
observed that on S3 resume, BIOS has enabled L1.2 (likely back to preboot
setting). Restoring powersave will therefore disable L1.2. Per spec, L1.2 must
be disabled on the downstream first. But pci_restore_state() gets called on
upstream devices first. Indeed, on my system, clearing the L1.2 state on the
root port makes the nvme device inaccessible by the time
pci_aspm_restore_state() is called for it. I've modified the patch to defer L1SS
restore until the downstream component so they can be done together. The patch
clears L1.2 on the child first before the parent, restores both configs and then
reenables them in reverse on the parent then the child. This works on my system.
I've posted the patch as a V5 and on the bugzilla and appreciate if anyone here
can test.

https://lore.kernel.org/linux-pci/20231221011250.191599-1-david.e.box@linux.intel.com/

David

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

On Wed, Dec 20, 2023 at 05:19:34PM -0800, David E. Box wrote:
> On Fri, 2023-11-17 at 16:21 -0800, David E. Box wrote:
> > On Thu, 2023-11-16 at 17:18 -0600, Bjorn Helgaas wrote:
> > > [+cc Matthew, author of 41cd766b0659 ("PCI: Don't enable aspm before drivers
> > > have had a chance to veto it")]
> > > 
> > > On Thu, Nov 16, 2023 at 12:10:02PM -0800, David E. Box wrote:
> > > > On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> > > > > On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > > > > > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > > > > > 
> > > > > > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe
> > > > > > > behind a VMD device:
> > > > > > > 
> > > > > > > ----
> > > > > > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P
> > > > > > > (family:
> > > > > > > 0x6, model: 0x9a, stepping: 0x3)
> > > > > > > ----
> > > > > > > 0000:00:0e.0 0104: 8086:467f
> > > > > > >         Subsystem: 1028:0af3
> > > > > > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > > > > > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > > > > > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > > > > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > > > > > [size=1M]
> > > > > > >         Capabilities: <access denied>
> > > > > > >         Kernel driver in use: vmd
> > > > > > > ----
> > > > > > > 
> > > > > > > The only release kernel that was able to get this laptop to
> > > > > > > fully get into low-power (unfortunately only s0ix) was the
> > > > > > > Ubuntu-6.2.0- ... series from Ubuntu (remote
> > > > > > > git://git.launchpad.net/~ubuntu-
> > > > > > > kernel/ubuntu/+source/linux/+git/lunar).
> > > > > > > 
> > > > > > > I'd bisected it to the following commits (in this order):
> > > > > > > 
> > > > > > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for
> > > > > > > suspend/resume
> > > > > > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > > > > > programming
> > > > > > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under
> > > > > > > VMD
> > > > > > > domain
> > > > > > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints
> > > > > > > behind
> > > > > > > VMD
> > > > > > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
> > > > > > > instead
> > > > > > 
> > > > > > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > > > > > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > > > > > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > > > > > Substates Capability for suspend/resume"")).
> > > > > > 
> > > > > > > Without the patches I never see Pkg%PC8 or higher(? lower?),
> > > > > > > nor i915 states DC5/6, all necssary for SYS%LPI/CPU%LPI. I've
> > > > > > > attached a little script I use alongside turbostat for
> > > > > > > verifying low-power operation (and also for seeing what
> > > > > > > chipset subsystem may be preventing it).
> > > > > > > 
> > > > > > > The first two are in Linus' trees, but were reverted
> > > > > > > (4ff116d0d5fd in a7152be79b6, 5e85eba6f50d in ff209ecc376a).
> > > > > > > The last three come from Ubuntu's Linux trees (see remote spec
> > > > > > > above). The first two remain reverted in the Ubuntu trees, but
> > > > > > > if I put them back, I get increased power savings during
> > > > > > > suspend/resume cycles.
> > > > > > > 
> > > > > > > Considering the power draw is really significant without these
> > > > > > > patches (10s of %s per hour) and I'd think Dell would have
> > > > > > > sold some decent number of these laptops, I'd been patiently
> > > > > > > waiting for these patches, or some variant to show up in the
> > > > > > > stable trees, but so far I'm up to the 6.6 stable kernel and
> > > > > > > still having to manually cherry-pick these, so I thought maybe
> > > > > > > I could bring this to the PM maintainers' attention so at
> > > > > > > least start a discussion about this issue.
> > > > > > 
> > > > > > Thank you very much for raising this again.  We really need to make
> > > > > > some progress, and Mika recently posted a patch to add the
> > > > > > 4ff116d0d5fd functionality again:
> > > > > > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > > > > > 
> > > > > > The big problem is that it works on *most* systems, but it still
> > > > > > seems to break a few.  So Mika's current patch relies on a
> > > > > > denylist of systems where we *don't* restore the substates.
> > > > > 
> > > > > According to latest reports it is just that one system where this
> > > > > is still an issue. The latest patch works in Asus UX305FA even if
> > > > > it is not in the denylist. That would leave that one system only
> > > > > to the denylist, at least the ones we are aware about.
> > > > 
> > > > I've been working with Thomas, whose system is the last known to
> > > > have problems with Mika's patch. It turns out that his config sets
> > > > aspm_policy to 'powersave'.  If he sets it to any other policy,
> > > > Mika's patch works [1]. It's possible that others may see the same
> > > > issue if they use 'powersave' as well.
> > > > 
> > > > The theory right now is that enabling L1SS in pci_restore_state() is
> > > > too early.  
> > > 
> > > I'd really like to figure out what "too early" means.  We can
> > > make it later by enabling L1SS somewhere else, but unless we
> > > know exactly what needs to happen first, we're likely to break
> > > it again.  And if we know what's required, we can probably
> > > figure out a cleaner way to restore it.
> > 
> > Still trying to understand this particular failure. The current
> > patch to Thomas more closely mimics how ASPM is enabled during
> > boot when powersave is set. If it works we can at least prove that
> > we can get it to work again by using a similar flow.
> 
> With some free time I was able to find a system in our lab that
> reproduces the same failure reported on the last problem report from
> Thomas. That is, with powersave selected, the nvme fails to come up
> after resume from S3 with this patch without a quirk. It's actually
> obvious when you can see the flow. We observed that on S3 resume,
> BIOS has enabled L1.2 (likely back to preboot setting). Restoring
> powersave will therefore disable L1.2. Per spec, L1.2 must be
> disabled on the downstream first. But pci_restore_state() gets
> called on upstream devices first. Indeed, on my system, clearing the
> L1.2 state on the root port makes the nvme device inaccessible by
> the time pci_aspm_restore_state() is called for it. I've modified
> the patch to defer L1SS restore until the downstream component so
> they can be done together. The patch clears L1.2 on the child first
> before the parent, restores both configs and then reenables them in
> reverse on the parent then the child. This works on my system.  I've
> posted the patch as a V5 and on the bugzilla and appreciate if
> anyone here can test.

This is FANTASTIC!  Thank you so much for getting to the bottom of
this!

Bjorn

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth R. Crudup]

On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:

> I'd bisected it to the following commits (in this order):
> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming

So the good news is the above two have made it into (the recently-released) 6.9!

Now I'm rooting for these last three:

> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

Applying (refactored for 6.8+ versions) of these last three enable full power
savings for 6.9 .

        -Kenny, fingers crossed

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

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth R. Crudup]

On Mon, 6 Nov 2023, Bjorn Helgaas wrote:

> > I'd bisected it to the following commits (in this order):
> > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

> Thanks for these.  You don't happen to have URLs for those Ubuntu
> commits, do you?

https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981

> Thank you very much for raising this again.

They've been really great for battery life on my laptop, so I'd like to help
these in some form get upstreamed (provided there's no bad side-effects, of
course).

	-Kenny

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

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

[+cc David, Nirmal, linux-pci]

On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
> On 12/12/24 12:56, Bjorn Helgaas wrote:
> > On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
> > > So we're on 6.13-rc2 and the patches are getting closer and
> > > closer, but they still need to be manually added.
> > > 
> > > The good news is now only (variants of) "PCI/ASPM: Enable LTR
> > > for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
> > > under VMD domain" are needed.
> ...

> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679

This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
IDs 0x9a09, 0xa0b0, or 0xa0bc.

This looks equivalent in spirit to upstream
https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
0xa77f, 0xad0b, 0xb06f, 0xb60b.

However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
so I'd be surprised if VMD worked for those devices unless BIOS set
up the VMD itself.

Maybe David or Nirmal can comment on this?

> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526

This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
IDs 0x9a09 and 0xa0b0.

This looks like it should also be handled by upstream f492edb40b54
("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
"pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".

But again, the Device IDs mentioned in the Ubuntu commit are NOT
included in the upstream VMD_FEATS_CLIENT list.

> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981

This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
below any VMD claimed by the "vmd" driver, not just VMDs with Device
IDs 0x9a09, 0xa0b0, or 0xa0bc.

I think the only thing that's missing is that the upstream vmd_ids[]
needs to be updated with some new VMD Device IDs that are tagged with
VMD_FEATS_CLIENT.

I don't know what the vmd_ids[] strategy is, but Kenneth, you might
try an upstream patch like the one below.  If that resolves the
standby/low-power issues, maybe David or Nirmal can figure out the
"right" way to do this.

Bjorn

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 9d9596947350..4de7ff3bbf23 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
                 .driver_data = VMD_FEATS_CLIENT,},
 	{PCI_VDEVICE(INTEL, 0xb06f),
                 .driver_data = VMD_FEATS_CLIENT,},
+	{PCI_VDEVICE(INTEL, 0x9a09),
+                .driver_data = VMD_FEATS_CLIENT,},
+	{PCI_VDEVICE(INTEL, 0xa0b0),
+                .driver_data = VMD_FEATS_CLIENT,},
+	{PCI_VDEVICE(INTEL, 0xa0bc),
+                .driver_data = VMD_FEATS_CLIENT,},
 	{0,}
 };
 MODULE_DEVICE_TABLE(pci, vmd_ids);

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

[-- Attachment #1: Type: text/plain, Size: 4787 bytes --]


I was really hopeful this would have handled it, but no joy.

I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD" 
and "PCI/ASPM: Enable ASPM for links under VMD domain" each separately 
on top of the below quirk patch to no avail.

The only thing that works is the aggregate patch I've added.

 > However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
 > VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
 > so I'd be surprised if VMD worked for those devices unless BIOS set
 > up the VMD itself.

Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten 
"Enable LTR for endpoints behind VMD" to print if the BIOS already does 
that for you, and sent the patch here (since I wasn't seeing the message 
printed when the fixup was being done and wanted to know why).

I'd REALLY like to get this into mainline, so if there's anything I can 
do to help, LMK.

Thanks,
-Kenny


On 12/13/24 08:43, Bjorn Helgaas wrote:
> [+cc David, Nirmal, linux-pci]
> 
> On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
>> On 12/12/24 12:56, Bjorn Helgaas wrote:
>>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>> So we're on 6.13-rc2 and the patches are getting closer and
>>>> closer, but they still need to be manually added.
>>>>
>>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR
>>>> for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
>>>> under VMD domain" are needed.
>> ...
> 
>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
> 
> This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
> VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
> for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
> IDs 0x9a09, 0xa0b0, or 0xa0bc.
> 
> This looks equivalent in spirit to upstream
> https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
> configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
> PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
> VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
> 0xa77f, 0xad0b, 0xb06f, 0xb60b.
> 
> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
> VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
> so I'd be surprised if VMD worked for those devices unless BIOS set
> up the VMD itself.
> 
> Maybe David or Nirmal can comment on this?
> 
>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
> 
> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
> IDs 0x9a09 and 0xa0b0.
> 
> This looks like it should also be handled by upstream f492edb40b54
> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
> 
> But again, the Device IDs mentioned in the Ubuntu commit are NOT
> included in the upstream VMD_FEATS_CLIENT list.
> 
>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
> 
> This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
> instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
> and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
> below any VMD claimed by the "vmd" driver, not just VMDs with Device
> IDs 0x9a09, 0xa0b0, or 0xa0bc.
> 
> I think the only thing that's missing is that the upstream vmd_ids[]
> needs to be updated with some new VMD Device IDs that are tagged with
> VMD_FEATS_CLIENT.
> 
> I don't know what the vmd_ids[] strategy is, but Kenneth, you might
> try an upstream patch like the one below.  If that resolves the
> standby/low-power issues, maybe David or Nirmal can figure out the
> "right" way to do this.
> 
> Bjorn
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 9d9596947350..4de7ff3bbf23 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
>                   .driver_data = VMD_FEATS_CLIENT,},
>   	{PCI_VDEVICE(INTEL, 0xb06f),
>                   .driver_data = VMD_FEATS_CLIENT,},
> +	{PCI_VDEVICE(INTEL, 0x9a09),
> +                .driver_data = VMD_FEATS_CLIENT,},
> +	{PCI_VDEVICE(INTEL, 0xa0b0),
> +                .driver_data = VMD_FEATS_CLIENT,},
> +	{PCI_VDEVICE(INTEL, 0xa0bc),
> +                .driver_data = VMD_FEATS_CLIENT,},
>   	{0,}
>   };
>   MODULE_DEVICE_TABLE(pci, vmd_ids);
> 

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

[-- Attachment #2: aspm-vmd-patch --]
[-- Type: text/plain, Size: 3289 bytes --]

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 28567d457613..6cec8ed1a726 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -846,7 +846,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
+			     PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 76f4df75b08a..c5145e74df73 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6301,3 +6301,70 @@ static void pci_mask_replay_timer_timeout(struct pci_dev *pdev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
 #endif
+/*
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static bool pci_fixup_is_vmd_bridge(struct pci_dev *pdev)
+{
+	struct pci_bus *bus = pdev->bus;
+	struct device *dev;
+	struct pci_driver *pdrv;
+
+	if (!pci_is_root_bus(bus))
+		return false;
+
+	dev = bus->bridge->parent;
+	if (dev == NULL)
+		return false;
+
+	pdrv = pci_dev_driver(to_pci_dev(dev));
+	if (pdrv == NULL || strcmp("vmd", pdrv->name))
+		return false;
+
+	return true;
+}
+
+static void pci_fixup_enable_aspm(struct pci_dev *pdev)
+{
+	if (!pci_fixup_is_vmd_bridge(pdev))
+		return;
+
+	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
+	pci_info(pdev, "enable ASPM for pci bridge behind vmd");
+}
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
+			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);
+
+static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
+{
+	struct pci_dev *parent;
+	int pos;
+	u16 val;
+
+	parent = pci_upstream_bridge(pdev);
+	if (!parent)
+		return;
+
+	if (!pci_fixup_is_vmd_bridge(parent))
+		return;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!pos)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &val);
+	if (val)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, &val);
+	if (val)
+		return;
+
+	/* 3145728ns, i.e. 0x300000ns */
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+	pci_info(pdev, "enable LTR for nvme behind vmd");
+}
+DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
+			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index db9b47ce3eef..9bd8234f1d39 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -245,6 +245,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
 	/* Device does honor MSI masking despite saying otherwise */
 	PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12),
+	/* Enable ASPM regardless of how LnkCtl is programmed */
+	PCI_DEV_FLAGS_ENABLE_ASPM = (__force pci_dev_flags_t) (1 << 13),
 };
 
 enum pci_irq_reroute_variant {

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

Turns out that my current VMD is already listed as "VMD_FEATS_CLIENT" 
after all:

----
         {PCI_VDEVICE(INTEL, 0x467f),
                 .driver_data = VMD_FEATS_CLIENT,},
----
0000:00:0e.0 0104: 8086:467f
         Subsystem: 1028:0af3
         Flags: bus master, fast devsel, latency 0, IOMMU group 9
         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
         Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
         Capabilities: [80] MSI-X: Enable+ Count=19 Masked-
         Capabilities: [90] Express Root Complex Integrated Endpoint, MSI 00
         Capabilities: [e0] Power Management version 3
         Kernel driver in use: vmd
----

I'm going to compare the commits you'd pointed out in master to what the 
working (Ubuntu) patches do and figure out where the disconnect is.

-K

On 12/13/24 11:48, Kenneth Crudup wrote:
> 
> I was really hopeful this would have handled it, but no joy.
> 
> I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD" 
> and "PCI/ASPM: Enable ASPM for links under VMD domain" each separately 
> on top of the below quirk patch to no avail.
> 
> The only thing that works is the aggregate patch I've added.
> 
>  > However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>  > VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>  > so I'd be surprised if VMD worked for those devices unless BIOS set
>  > up the VMD itself.
> 
> Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten 
> "Enable LTR for endpoints behind VMD" to print if the BIOS already does 
> that for you, and sent the patch here (since I wasn't seeing the message 
> printed when the fixup was being done and wanted to know why).
> 
> I'd REALLY like to get this into mainline, so if there's anything I can 
> do to help, LMK.
> 
> Thanks,
> -Kenny
> 
> 
> On 12/13/24 08:43, Bjorn Helgaas wrote:
>> [+cc David, Nirmal, linux-pci]
>>
>> On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
>>> On 12/12/24 12:56, Bjorn Helgaas wrote:
>>>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>>> So we're on 6.13-rc2 and the patches are getting closer and
>>>>> closer, but they still need to be manually added.
>>>>>
>>>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR
>>>>> for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
>>>>> under VMD domain" are needed.
>>> ...
>>
>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>> lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
>>
>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
>> VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
>> for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>
>> This looks equivalent in spirit to upstream
>> https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
>> configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
>> PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
>> VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
>> 0xa77f, 0xad0b, 0xb06f, 0xb60b.
>>
>> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>> VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>> so I'd be surprised if VMD worked for those devices unless BIOS set
>> up the VMD itself.
>>
>> Maybe David or Nirmal can comment on this?
>>
>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
>>
>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
>> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
>> IDs 0x9a09 and 0xa0b0.
>>
>> This looks like it should also be handled by upstream f492edb40b54
>> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
>> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
>>
>> But again, the Device IDs mentioned in the Ubuntu commit are NOT
>> included in the upstream VMD_FEATS_CLIENT list.
>>
>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>> lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
>>
>> This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
>> instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
>> and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
>> below any VMD claimed by the "vmd" driver, not just VMDs with Device
>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>
>> I think the only thing that's missing is that the upstream vmd_ids[]
>> needs to be updated with some new VMD Device IDs that are tagged with
>> VMD_FEATS_CLIENT.
>>
>> I don't know what the vmd_ids[] strategy is, but Kenneth, you might
>> try an upstream patch like the one below.  If that resolves the
>> standby/low-power issues, maybe David or Nirmal can figure out the
>> "right" way to do this.
>>
>> Bjorn
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index 9d9596947350..4de7ff3bbf23 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
>>                   .driver_data = VMD_FEATS_CLIENT,},
>>       {PCI_VDEVICE(INTEL, 0xb06f),
>>                   .driver_data = VMD_FEATS_CLIENT,},
>> +    {PCI_VDEVICE(INTEL, 0x9a09),
>> +                .driver_data = VMD_FEATS_CLIENT,},
>> +    {PCI_VDEVICE(INTEL, 0xa0b0),
>> +                .driver_data = VMD_FEATS_CLIENT,},
>> +    {PCI_VDEVICE(INTEL, 0xa0bc),
>> +                .driver_data = VMD_FEATS_CLIENT,},
>>       {0,}
>>   };
>>   MODULE_DEVICE_TABLE(pci, vmd_ids);
>>
> 

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

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

[-- Attachment #1: Type: text/plain, Size: 7538 bytes --]


OK, it looks like the effective change (that's not already contained in 
the LTR SNOOP patches already in Linus' master (et al.)) comes from this 
line from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable 
ASPM for links under VMD domain"):

----
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 00143f5fb83a..d2ff44e7fbb1 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct 
pcie_link_state *link, int blacklist)
         aspm_l1ss_init(link);

         /* Save default state */
-       link->aspm_default = link->aspm_enabled;
+       link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
+                            ASPM_STATE_ALL : link->aspm_enabled;
----

Where "PCI_DEV_FLAGS_ENABLE_ASPM" (a new flag) is set if the parent 
bridge is a VMD (determined via Ubuntu commit 154d48da2 ("UBUNTU: SAUCE: 
vmd: fixup bridge ASPM by driver name instead").

So, as we've both noticed, most of the three patches I'd pointed out 
aren't required as the LTR SNOOP values are either fixed up, or 
previously set in the BIOS.

So I've folded the new commits into the attached patch (against Linus' 
master), which now gives me full power-saving.

-K


On 12/13/24 12:27, Kenneth Crudup wrote:
> 
> Turns out that my current VMD is already listed as "VMD_FEATS_CLIENT" 
> after all:
> 
> ----
>          {PCI_VDEVICE(INTEL, 0x467f),
>                  .driver_data = VMD_FEATS_CLIENT,},
> ----
> 0000:00:0e.0 0104: 8086:467f
>          Subsystem: 1028:0af3
>          Flags: bus master, fast devsel, latency 0, IOMMU group 9
>          Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
>          Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
>          Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
>          Capabilities: [80] MSI-X: Enable+ Count=19 Masked-
>          Capabilities: [90] Express Root Complex Integrated Endpoint, 
> MSI 00
>          Capabilities: [e0] Power Management version 3
>          Kernel driver in use: vmd
> ----
> 
> I'm going to compare the commits you'd pointed out in master to what the 
> working (Ubuntu) patches do and figure out where the disconnect is.
> 
> -K
> 
> On 12/13/24 11:48, Kenneth Crudup wrote:
>>
>> I was really hopeful this would have handled it, but no joy.
>>
>> I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD" 
>> and "PCI/ASPM: Enable ASPM for links under VMD domain" each separately 
>> on top of the below quirk patch to no avail.
>>
>> The only thing that works is the aggregate patch I've added.
>>
>>  > However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>>  > VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>>  > so I'd be surprised if VMD worked for those devices unless BIOS set
>>  > up the VMD itself.
>>
>> Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten 
>> "Enable LTR for endpoints behind VMD" to print if the BIOS already 
>> does that for you, and sent the patch here (since I wasn't seeing the 
>> message printed when the fixup was being done and wanted to know why).
>>
>> I'd REALLY like to get this into mainline, so if there's anything I 
>> can do to help, LMK.
>>
>> Thanks,
>> -Kenny
>>
>>
>> On 12/13/24 08:43, Bjorn Helgaas wrote:
>>> [+cc David, Nirmal, linux-pci]
>>>
>>> On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
>>>> On 12/12/24 12:56, Bjorn Helgaas wrote:
>>>>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>>>> So we're on 6.13-rc2 and the patches are getting closer and
>>>>>> closer, but they still need to be manually added.
>>>>>>
>>>>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR
>>>>>> for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
>>>>>> under VMD domain" are needed.
>>>> ...
>>>
>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>> lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
>>>
>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
>>> VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
>>> for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
>>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>>
>>> This looks equivalent in spirit to upstream
>>> https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
>>> configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
>>> PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
>>> VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
>>> 0xa77f, 0xad0b, 0xb06f, 0xb60b.
>>>
>>> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>>> VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>>> so I'd be surprised if VMD worked for those devices unless BIOS set
>>> up the VMD itself.
>>>
>>> Maybe David or Nirmal can comment on this?
>>>
>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
>>>
>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
>>> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
>>> IDs 0x9a09 and 0xa0b0.
>>>
>>> This looks like it should also be handled by upstream f492edb40b54
>>> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
>>> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
>>>
>>> But again, the Device IDs mentioned in the Ubuntu commit are NOT
>>> included in the upstream VMD_FEATS_CLIENT list.
>>>
>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>> lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
>>>
>>> This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
>>> instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
>>> and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
>>> below any VMD claimed by the "vmd" driver, not just VMDs with Device
>>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>>
>>> I think the only thing that's missing is that the upstream vmd_ids[]
>>> needs to be updated with some new VMD Device IDs that are tagged with
>>> VMD_FEATS_CLIENT.
>>>
>>> I don't know what the vmd_ids[] strategy is, but Kenneth, you might
>>> try an upstream patch like the one below.  If that resolves the
>>> standby/low-power issues, maybe David or Nirmal can figure out the
>>> "right" way to do this.
>>>
>>> Bjorn
>>>
>>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>>> index 9d9596947350..4de7ff3bbf23 100644
>>> --- a/drivers/pci/controller/vmd.c
>>> +++ b/drivers/pci/controller/vmd.c
>>> @@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
>>>                   .driver_data = VMD_FEATS_CLIENT,},
>>>       {PCI_VDEVICE(INTEL, 0xb06f),
>>>                   .driver_data = VMD_FEATS_CLIENT,},
>>> +    {PCI_VDEVICE(INTEL, 0x9a09),
>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>> +    {PCI_VDEVICE(INTEL, 0xa0b0),
>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>> +    {PCI_VDEVICE(INTEL, 0xa0bc),
>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>>       {0,}
>>>   };
>>>   MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>
>>
> 

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

[-- Attachment #2: aspm-enable-take-2 --]
[-- Type: text/plain, Size: 1557 bytes --]

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 28567d457613..a5df6230cf3c 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -768,6 +768,31 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
 }
 
+/*
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static bool pci_fixup_vmd_bridge_enable_aspm(struct pci_dev *pdev)
+{
+       struct pci_bus *bus = pdev->bus;
+       struct device *dev;
+       struct pci_driver *pdrv;
+
+       if (!pci_is_root_bus(bus))
+               return false;
+
+       dev = bus->bridge->parent;
+       if (dev == NULL)
+               return false;
+
+       pdrv = pci_dev_driver(to_pci_dev(dev));
+       if (pdrv == NULL || strcmp("vmd", pdrv->name))
+               return false;
+
+       pci_info(pdev, "enable ASPM for pci bridge behind vmd");
+       return true;
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
@@ -846,7 +871,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
+		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

Ah, that sweet power savings! :)

----
C1E%    C10%    CPU%c7  PkgTmp  GFX%rc6 GFXMHz  GFXAMHz CPUGFX% Pkg%pc8 
Pk%pc10 CPU%LPI SYS%LPI PkgWatt CorWatt GFXWatt
0.65    93.77   36.12   49      0.00    1033    1050    2.93    7.53 
21.66   21.70   0.00    3.28    1.34    0.02
0.09    99.23   41.68   48      0.00    1033    1050    0.91    11.98 
10.22   10.24   0.00    1.95    0.22    0.01
0.09    99.17   41.41   49      0.00    1033    1050    0.25    29.83 
39.18   39.26   11.38   1.10    0.34    0.00
0.02    99.78   42.40   48      0.00    1033    1050    0.20    31.63 
48.31   48.40   42.58   0.55    0.11    0.00
0.06    99.35   41.67   46      0.00    1033    1050    0.21    23.86 
51.95   52.04   46.83   0.80    0.30    0.00
C1E%    C10%    CPU%c7  PkgTmp  GFX%rc6 GFXMHz  GFXAMHz CPUGFX% Pkg%pc8 
Pk%pc10 CPU%LPI SYS%LPI PkgWatt CorWatt GFXWatt
0.04    99.71   42.38   46      0.00    1033    1050    0.20    24.74 
58.05   58.15   51.85   0.44    0.11    0.00
0.07    99.41   41.77   46      0.00    1033    1050    0.17    25.20 
52.57   52.65   46.99   0.73    0.27    0.00
0.03    99.76   42.39   45      0.00    1033    1050    0.18    26.67 
56.59   56.68   50.16   0.46    0.11    0.00
0.06    99.24   41.54   45      0.00    1033    1050    0.24    23.89 
46.83   46.90   41.40   0.91    0.32    0.00
0.03    99.75   42.42   45      0.00    1033    1050    0.20    22.65 
59.64   59.73   52.87   0.46    0.10    0.00
----

-K

On 12/13/24 14:26, Kenneth Crudup wrote:
> 
> OK, it looks like the effective change (that's not already contained in 
> the LTR SNOOP patches already in Linus' master (et al.)) comes from this 
> line from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable 
> ASPM for links under VMD domain"):
> 
> ----
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 00143f5fb83a..d2ff44e7fbb1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct 
> pcie_link_state *link, int blacklist)
>          aspm_l1ss_init(link);
> 
>          /* Save default state */
> -       link->aspm_default = link->aspm_enabled;
> +       link->aspm_default = parent->dev_flags & 
> PCI_DEV_FLAGS_ENABLE_ASPM ?
> +                            ASPM_STATE_ALL : link->aspm_enabled;
> ----
> 
> Where "PCI_DEV_FLAGS_ENABLE_ASPM" (a new flag) is set if the parent 
> bridge is a VMD (determined via Ubuntu commit 154d48da2 ("UBUNTU: SAUCE: 
> vmd: fixup bridge ASPM by driver name instead").
> 
> So, as we've both noticed, most of the three patches I'd pointed out 
> aren't required as the LTR SNOOP values are either fixed up, or 
> previously set in the BIOS.
> 
> So I've folded the new commits into the attached patch (against Linus' 
> master), which now gives me full power-saving.
> 
> -K
> 
> 
> On 12/13/24 12:27, Kenneth Crudup wrote:
>>
>> Turns out that my current VMD is already listed as "VMD_FEATS_CLIENT" 
>> after all:
>>
>> ----
>>          {PCI_VDEVICE(INTEL, 0x467f),
>>                  .driver_data = VMD_FEATS_CLIENT,},
>> ----
>> 0000:00:0e.0 0104: 8086:467f
>>          Subsystem: 1028:0af3
>>          Flags: bus master, fast devsel, latency 0, IOMMU group 9
>>          Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
>>          Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
>>          Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
>>          Capabilities: [80] MSI-X: Enable+ Count=19 Masked-
>>          Capabilities: [90] Express Root Complex Integrated Endpoint, 
>> MSI 00
>>          Capabilities: [e0] Power Management version 3
>>          Kernel driver in use: vmd
>> ----
>>
>> I'm going to compare the commits you'd pointed out in master to what 
>> the working (Ubuntu) patches do and figure out where the disconnect is.
>>
>> -K
>>
>> On 12/13/24 11:48, Kenneth Crudup wrote:
>>>
>>> I was really hopeful this would have handled it, but no joy.
>>>
>>> I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD" 
>>> and "PCI/ASPM: Enable ASPM for links under VMD domain" each 
>>> separately on top of the below quirk patch to no avail.
>>>
>>> The only thing that works is the aggregate patch I've added.
>>>
>>>  > However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>>>  > VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>>>  > so I'd be surprised if VMD worked for those devices unless BIOS set
>>>  > up the VMD itself.
>>>
>>> Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten 
>>> "Enable LTR for endpoints behind VMD" to print if the BIOS already 
>>> does that for you, and sent the patch here (since I wasn't seeing the 
>>> message printed when the fixup was being done and wanted to know why).
>>>
>>> I'd REALLY like to get this into mainline, so if there's anything I 
>>> can do to help, LMK.
>>>
>>> Thanks,
>>> -Kenny
>>>
>>>
>>> On 12/13/24 08:43, Bjorn Helgaas wrote:
>>>> [+cc David, Nirmal, linux-pci]
>>>>
>>>> On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
>>>>> On 12/12/24 12:56, Bjorn Helgaas wrote:
>>>>>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>>>>> So we're on 6.13-rc2 and the patches are getting closer and
>>>>>>> closer, but they still need to be manually added.
>>>>>>>
>>>>>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR
>>>>>>> for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
>>>>>>> under VMD domain" are needed.
>>>>> ...
>>>>
>>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>>> lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
>>>>
>>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
>>>> VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
>>>> for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
>>>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>>>
>>>> This looks equivalent in spirit to upstream
>>>> https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
>>>> configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
>>>> PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
>>>> VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
>>>> 0xa77f, 0xad0b, 0xb06f, 0xb60b.
>>>>
>>>> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>>>> VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>>>> so I'd be surprised if VMD worked for those devices unless BIOS set
>>>> up the VMD itself.
>>>>
>>>> Maybe David or Nirmal can comment on this?
>>>>
>>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>>> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
>>>>
>>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
>>>> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
>>>> IDs 0x9a09 and 0xa0b0.
>>>>
>>>> This looks like it should also be handled by upstream f492edb40b54
>>>> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
>>>> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
>>>>
>>>> But again, the Device IDs mentioned in the Ubuntu commit are NOT
>>>> included in the upstream VMD_FEATS_CLIENT list.
>>>>
>>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>>> lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
>>>>
>>>> This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
>>>> instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
>>>> and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
>>>> below any VMD claimed by the "vmd" driver, not just VMDs with Device
>>>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>>>
>>>> I think the only thing that's missing is that the upstream vmd_ids[]
>>>> needs to be updated with some new VMD Device IDs that are tagged with
>>>> VMD_FEATS_CLIENT.
>>>>
>>>> I don't know what the vmd_ids[] strategy is, but Kenneth, you might
>>>> try an upstream patch like the one below.  If that resolves the
>>>> standby/low-power issues, maybe David or Nirmal can figure out the
>>>> "right" way to do this.
>>>>
>>>> Bjorn
>>>>
>>>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/ 
>>>> vmd.c
>>>> index 9d9596947350..4de7ff3bbf23 100644
>>>> --- a/drivers/pci/controller/vmd.c
>>>> +++ b/drivers/pci/controller/vmd.c
>>>> @@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
>>>>                   .driver_data = VMD_FEATS_CLIENT,},
>>>>       {PCI_VDEVICE(INTEL, 0xb06f),
>>>>                   .driver_data = VMD_FEATS_CLIENT,},
>>>> +    {PCI_VDEVICE(INTEL, 0x9a09),
>>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>>> +    {PCI_VDEVICE(INTEL, 0xa0b0),
>>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>>> +    {PCI_VDEVICE(INTEL, 0xa0bc),
>>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>>>       {0,}
>>>>   };
>>>>   MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>>
>>>
>>
> 

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

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

[cc->to: David, Nirmal]

On Fri, Dec 13, 2024 at 02:26:37PM -0800, Kenneth Crudup wrote:
> OK, it looks like the effective change (that's not already contained in the
> LTR SNOOP patches already in Linus' master (et al.)) comes from this line
> from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for
> links under VMD domain"):
> 
> ----
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 00143f5fb83a..d2ff44e7fbb1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state
> *link, int blacklist)
>         aspm_l1ss_init(link);
> 
>         /* Save default state */
> -       link->aspm_default = link->aspm_enabled;
> +       link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
> +                            ASPM_STATE_ALL : link->aspm_enabled;

So I thought the "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)" in
f492edb40b54 would effectively do the same thing:

> > > > > https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/
> > > > > lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
> > > > 
> > > > This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
> > > > IDs 0x9a09 and 0xa0b0.
> > > > 
> > > > This looks like it should also be handled by upstream f492edb40b54
> > > > ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
> > > > "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".

But I guess it doesn't actually work.  I'm hoping David or Nirmal can
figure out why it doesn't because it seems obvious that it's the
intent.

Thanks a lot for all your work to narrow it down to this!

Bjorn

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

Hi Kenneth,

On Fri, 2024-12-13 at 17:02 -0600, Bjorn Helgaas wrote:
> [cc->to: David, Nirmal]
> 
> On Fri, Dec 13, 2024 at 02:26:37PM -0800, Kenneth Crudup wrote:
> > OK, it looks like the effective change (that's not already contained in the
> > LTR SNOOP patches already in Linus' master (et al.)) comes from this line
> > from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for
> > links under VMD domain"):
> > 
> > ----
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 00143f5fb83a..d2ff44e7fbb1 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state
> > *link, int blacklist)
> >         aspm_l1ss_init(link);
> > 
> >         /* Save default state */
> > -       link->aspm_default = link->aspm_enabled;
> > +       link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
> > +                            ASPM_STATE_ALL : link->aspm_enabled;
> 
> So I thought the "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)" in
> f492edb40b54 would effectively do the same thing:
> 
> > > > > > https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/
> > > > > > lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
> > > > > 
> > > > > This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > > domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
> > > > > IDs 0x9a09 and 0xa0b0.
> > > > > 
> > > > > This looks like it should also be handled by upstream f492edb40b54
> > > > > ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
> > > > > "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
> 
> But I guess it doesn't actually work.  I'm hoping David or Nirmal can
> figure out why it doesn't because it seems obvious that it's the
> intent.

Is PCIe ASPM disabled? In the kernel log do you see:

    "can't override BIOS ASPM; OS doesn't have ASPM control"

David

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

I do see that:

----
[E0] 781 /usr/src/ubuntu-kernel> dmesg | fgrep -i aspm
[    0.164233] ACPI FADT declares the system doesn't support PCIe ASPM, 
so disable it
[    0.579946] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM 
ClockPM Segments MSI EDR HPX-Type3]
[    0.587377] acpi PNP0A08:00: FADT indicates ASPM is unsupported, 
using BIOS configuration
[    1.309826] pci 10000:e0:06.0: enable ASPM for pci bridge behind vmd
[    1.622705] pci 10000:e1:00.0: can't override BIOS ASPM; OS doesn't 
have ASPM control
[110757.878494] pcieport 0000:00:07.0: ASPM: current common clock 
configuration is inconsistent, reconfiguring
[171953.284616] pcieport 0000:00:07.0: ASPM: current common clock 
configuration is inconsistent, reconfiguring
----

On 12/19/24 08:25, David E. Box wrote:
> Hi Kenneth,
> 
> On Fri, 2024-12-13 at 17:02 -0600, Bjorn Helgaas wrote:
>> [cc->to: David, Nirmal]
>>
>> On Fri, Dec 13, 2024 at 02:26:37PM -0800, Kenneth Crudup wrote:
>>> OK, it looks like the effective change (that's not already contained in the
>>> LTR SNOOP patches already in Linus' master (et al.)) comes from this line
>>> from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for
>>> links under VMD domain"):
>>>
>>> ----
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index 00143f5fb83a..d2ff44e7fbb1 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state
>>> *link, int blacklist)
>>>          aspm_l1ss_init(link);
>>>
>>>          /* Save default state */
>>> -       link->aspm_default = link->aspm_enabled;
>>> +       link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
>>> +                            ASPM_STATE_ALL : link->aspm_enabled;
>>
>> So I thought the "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)" in
>> f492edb40b54 would effectively do the same thing:
>>
>>>>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/
>>>>>>> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
>>>>>>
>>>>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
>>>>>> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
>>>>>> IDs 0x9a09 and 0xa0b0.
>>>>>>
>>>>>> This looks like it should also be handled by upstream f492edb40b54
>>>>>> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
>>>>>> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
>>
>> But I guess it doesn't actually work.  I'm hoping David or Nirmal can
>> figure out why it doesn't because it seems obvious that it's the
>> intent.
> 
> Is PCIe ASPM disabled? In the kernel log do you see:
> 
>      "can't override BIOS ASPM; OS doesn't have ASPM control"
> 
> David
> 

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

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

+Rafael

On Thu, 2024-12-19 at 10:17 -0800, Kenneth Crudup wrote:
> I do see that:
> 
> ----
> [E0] 781 /usr/src/ubuntu-kernel> dmesg | fgrep -i aspm
> [    0.164233] ACPI FADT declares the system doesn't support PCIe ASPM, 
> so disable it

So, PCIe ASPM refers to OS control of ASPM. Disabling it means the BIOS alone
controls it, leaving the OS to stick with the defaults programmed into the
controllers by the BIOS. This might happen due to critical bugs in certain ASPM
states or simply because the OEM decided to configure it that way. We don't know
the exact reason.

The issue with VMD is that its controllers are hidden from the BIOS, so ASPM
defaults are never programmed into them. When PCIe ASPM is disabled, it's
unclear whether this should apply to controllers in VMD mode. To be cautious, we
avoid modifying ASPM settings in this scenario.

If you want to override this behavior, you can try setting pcie_aspm=force on
the kernel command line.

David

> [    0.579946] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM 
> ClockPM Segments MSI EDR HPX-Type3]
> [    0.587377] acpi PNP0A08:00: FADT indicates ASPM is unsupported, 
> using BIOS configuration
> [    1.309826] pci 10000:e0:06.0: enable ASPM for pci bridge behind vmd
> [    1.622705] pci 10000:e1:00.0: can't override BIOS ASPM; OS doesn't 
> have ASPM control
> [110757.878494] pcieport 0000:00:07.0: ASPM: current common clock 
> configuration is inconsistent, reconfiguring
> [171953.284616] pcieport 0000:00:07.0: ASPM: current common clock 
> configuration is inconsistent, reconfiguring
> ----
> 
> On 12/19/24 08:25, David E. Box wrote:
> > Hi Kenneth,
> > 
> > On Fri, 2024-12-13 at 17:02 -0600, Bjorn Helgaas wrote:
> > > [cc->to: David, Nirmal]
> > > 
> > > On Fri, Dec 13, 2024 at 02:26:37PM -0800, Kenneth Crudup wrote:
> > > > OK, it looks like the effective change (that's not already contained in
> > > > the
> > > > LTR SNOOP patches already in Linus' master (et al.)) comes from this
> > > > line
> > > > from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable ASPM
> > > > for
> > > > links under VMD domain"):
> > > > 
> > > > ----
> > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > > index 00143f5fb83a..d2ff44e7fbb1 100644
> > > > --- a/drivers/pci/pcie/aspm.c
> > > > +++ b/drivers/pci/pcie/aspm.c
> > > > @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct
> > > > pcie_link_state
> > > > *link, int blacklist)
> > > >          aspm_l1ss_init(link);
> > > > 
> > > >          /* Save default state */
> > > > -       link->aspm_default = link->aspm_enabled;
> > > > +       link->aspm_default = parent->dev_flags &
> > > > PCI_DEV_FLAGS_ENABLE_ASPM ?
> > > > +                            ASPM_STATE_ALL : link->aspm_enabled;
> > > 
> > > So I thought the "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)" in
> > > f492edb40b54 would effectively do the same thing:
> > > 
> > > > > > > > https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/
> > > > > > > > lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
> > > > > > > 
> > > > > > > This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > > > > domain", which adds "link->aspm_default = ASPM_STATE_ALL" for
> > > > > > > device
> > > > > > > IDs 0x9a09 and 0xa0b0.
> > > > > > > 
> > > > > > > This looks like it should also be handled by upstream f492edb40b54
> > > > > > > ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which
> > > > > > > adds
> > > > > > > "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
> > > 
> > > But I guess it doesn't actually work.  I'm hoping David or Nirmal can
> > > figure out why it doesn't because it seems obvious that it's the
> > > intent.
> > 
> > Is PCIe ASPM disabled? In the kernel log do you see:
> > 
> >      "can't override BIOS ASPM; OS doesn't have ASPM control"
> > 
> > David
> > 
> 

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]


Or, just use (some version of) the attached patch (against Linus' recent 
master) that enables VMD ASPM this for us "automatically" if there's a 
detected VMD ?

I'd prefer a scalpel to a bludgeon (and have been trying to get some 
version of these fixes into mainline for a while).

-Kenny

On 12/19/24 11:52, David E. Box wrote:

> If you want to override this behavior, you can try setting pcie_aspm=force on
> the kernel command line.

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

[-- Attachment #2: aspm-enable-take-2 --]
[-- Type: text/plain, Size: 1557 bytes --]

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 28567d457613..a5df6230cf3c 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -768,6 +768,31 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
 }
 
+/*
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static bool pci_fixup_vmd_bridge_enable_aspm(struct pci_dev *pdev)
+{
+       struct pci_bus *bus = pdev->bus;
+       struct device *dev;
+       struct pci_driver *pdrv;
+
+       if (!pci_is_root_bus(bus))
+               return false;
+
+       dev = bus->bridge->parent;
+       if (dev == NULL)
+               return false;
+
+       pdrv = pci_dev_driver(to_pci_dev(dev));
+       if (pdrv == NULL || strcmp("vmd", pdrv->name))
+               return false;
+
+       pci_info(pdev, "enable ASPM for pci bridge behind vmd");
+       return true;
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
@@ -846,7 +871,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
+		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;