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

[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

[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