S3 suspend broken on AMD Ryzen Pro 7 5850U (regression in kernel 6.1)

changed the description

mentioned in issue #2171 (closed)

I think you captured your dmesg incorrectly, it looks empty.

Indeed, sorry for that (the journal was either corrupted or too full, I hadn't checked).

Here it comes, xz-ipped: dmesg_6.2-rc4.xz.

Anyway, I could confirm that (with amd_pmc module loaded) kernel 6.2-rc4:

suspends and resumes correctly in "Windows mode" (acpidump_windows_mode)
does not finish suspending in "Linux mode" (acpidump_linux_mode)
but does suspend and resume correctly with echo disabled > /sys/bus/i2c/devices/i2c-ELAN0678\:00/power/wakeup launched first, in "Linux mode".

I now have a working upstream kernel building setup, feel free to throw patches at me!

OK here's my asks.

Can you please get me a dmidecode for your system? I'll figure out the best strings to quirk with.
Can you please confirm if you can wake up the system from touchpad in s0ix?

Here is a test kernel with a quirk (with guessed DMI strings from what I saw on the X13 Gen2 that I think should cover it (not Gen2a)). Please test it in both modes.

https://gitlab.freedesktop.org/superm1/linux/-/commits/mlimonci/rhbz-2162013-gitlab-2357

If you were able to wake the system from touchpad in s0ix before (ask 2 above), I expect you won't be able to anymore.

Here comes the dmidecode: dmidecode.
Confirmed; the wake up from s0ix works by fiddling the touchpad

I'll try building and loop-reboot; then report back.

Here comes the dmidecode: dmidecode. I'll try building and loop-reboot; then report back.

Thanks, it looks like the quirk should cover your system then.

Confirmed; the wake up from s0ix works by fiddling the touchpad

Well then that's something we still need to explain. Why does this failure only happen in S3 mode. If we quirk it to fix S3 mode, we're going to break it being able to wake from s2idle mode.

@rrangel1 any ideas?

OK. I still need to test without the amd_pmc module, but with it loaded, on your patch line:

suspend/wakeup works both in S3 and s0ix,
but while wakeup worked in s0ix by fiddling the touchpad or keyboard, it only wakesup via keyboard.
wakeup doesn't trigger in S3 with either (only power button)

Ah, if it matters, I also realized I had not upgraded the firmware recently (still on 1.21, not 1.24).

Don't worry about testing without amd_pmc. We need that for s0ix, if it's missing it's an invalid test. If it's (manually) loaded for S3 it doesn't do anything.

Can I please see your /sys/kernel/debug/gpio file while booted to S3 mode without my quirk in place?

Sure, here goes: gpio_6.2rc4_unpatched

added S3 label

Thanks for creating the Issue! I can confirm the same issue on my machine. I don't have the exact same Lenovo Laptop, but it seems to be built very similar.

Bisecting came to the conclusion that the following commit is the culprit:

➜  linux-mainline git:(b38f2d5d9615) ✗ git bisect bad
b38f2d5d9615cf991fb68626e70b042cb8b6dc3e is the first bad commit
commit b38f2d5d9615cf991fb68626e70b042cb8b6dc3e
Author: Raul E Rangel <rrangel@chromium.org>
Date:   Thu Sep 29 10:19:11 2022 -0600

    i2c: acpi: Use ACPI wake capability bit to set wake_irq

Hardware description: ThinkPad T14s Gen 2a

CPU: AMD Ryzen 7 PRO 5850U with Radeon Graphics
System Memory: 32GB
Display: Built-in Display. No external displays connected.
Type of Display Connection: eDP

System information:

Distro name and Version: Arch Linux
Kernel version: v6.2-rc4
Custom kernel: -
AMD official driver version: -

Log files (for system lockups / game freezes / crashes)

bad_commit_dmesg.txt

Thanks for bisecting! I think it's very likely the same as https://bugzilla.redhat.com/show_bug.cgi?id=2162013 then.

Can you please do the following:

Disable wakeup for I2C based touchpad/touchscreen.

Something like this: echo disabled > /sys/bus/i2c/devices/i2c-ELAN0415\:00/power/wakeup

Add acpidump for your system(s).

This seems to be indeed the issue! With this command, the S3 Standby is working again.

# echo disabled > /sys/bus/i2c/devices/i2c-ELAN0679:00/power/wakeup

Also works for me!

I'm using Lenovo Xiaoxin pro 14 2021 (this model seems to be only available in Chinese market, the same with yoga 14s) with amd ryzen 5800H, 6.1.6 linux zen kernel, nixos.

The device for me is i2c-MSFT0001:00.

"Lenovo IdeaPad 3 15ALC6" AMD 5500U, 6.1.8-gentoo-dist

disabled > i2c-MSFT0001:00 also works for me.

(CC @@rrangel1, this looks like it might be more fallout from the series)

Hrmm, I guess one side effect of the patch listed above is that it now enables the wake capability by default for all i2c devices if the wake_capable bit is set: https://github.com/torvalds/linux/blob/ec9187ecea142593c54cf7a73ef2e1a3d517495a/drivers/i2c/i2c-core-base.c#L530

Previously it was up to the individual drivers. Some of them called device_init_wakeup(dev, true) to always enable wake ups. It looks like the i2c-hid driver never enabled wakeups by default though. So now it's uncovering all the misconfigured ACPI tables.

One option is to call device_set_wakeup_enabled(dev, false) in the i2c-hid-acpi driver to return to this previous behavior.

Yeah; it's really a question of how big of a quirk list we want to try to build versus doing that. It looks to me that we probably have 4 models affected now.

Clevo laptop (the one you commented on)
Two different Lenovo laptops
System76 pangolin

Do you have a link to the System76 bug? I'm sure we can get them to fix it :)

Do you have a link to the System76 bug? I'm sure we can get them to fix it

https://bugzilla.redhat.com/show_bug.cgi?id=2162013

It does suspend correctly with s0ix ("Windows mode"); this bug is about how it used to work in 6.0 kernels with S3 ("Linux mode") and is now broken.

As this looks like fallout from the I2C wakeup series, I do expect that 6.2-rc fails with s0ix too.

S0ix Standby seems to work for me on v6.2-rc4

Can you confirm the system is actually entering S0i3. cat /sys/kernel/debug/amd_pmc/s0ix_stats after doing an S0i3 cycle.

Hmm, that's actually surprising. Can you please get me a report with s0ix suspend using https://gitlab.freedesktop.org/drm/amd/-/blob/master/scripts/amd_s2idle.py

That will capture a ton of debug data.

A maybe noteworthy point is that resuming from s0ix takes ~3-5 seconds. Not sure if this is normal.

s2idle_report.txt

# sudo cat /sys/kernel/debug/amd_pmc/s0ix_stats
=== S0ix statistics ===
S0ix Entry Time: 13536237951
S0ix Exit Time: 14367551427
Residency Time: 17319030

I wonder if the EC nominally controls the rail to the touchpad in your design? It looks like there are some ACPI table problems, so perhaps the _DSM call didn't get to notify the EC of s0i3 entry.

Does S3 fail on 6.2-rc4 still as well?

I guess that's a question for Lenovo?

S3 and s0ix testing was done with 6.2-rc4.

I guess that's a question for Lenovo?

Are you on the latest BIOS already? If not; can you please upgrade to it so we can see if that _DSM thing is fixed.

S3 and s0ix testing was done with 6.2-rc4.

Well that's pretty odd then to me. Can you please share two acpidumps, one while you're in S3 mode and one while you're in s2idle mode. Lenovo does change the tables with different modes, and maybe the bug only exists in S3 mode.

Are you on the latest BIOS already? If not; can you please upgrade to it so we can see if that _DSM thing is fixed.

Yes, updated the BIOS after first encountering this issue. Didn't change anything.

Well that's pretty odd then to me. Can you please share two acpidumps, one while you're in S3 mode and one while you're in s2idle mode. Lenovo does change the tables with different modes, and maybe the bug only exists in S3 mode.

t14s_gen2a_s3.txt

t14s_gen2a_s0ix.txt

The GpioInt resource is at least configured identically in both modes.

                GpioInt (Level, ActiveLow, ExclusiveAndWake, PullNone, 0x0000,
                    "\\_SB.GPIO", 0x00, ResourceConsumer, ,
                    )
                    {   // Pin list
                        0x0007
                    }

Can you share /sys/kernel/debug/gpio from S3 mode? We're looking for GPIO 7.

We already have it in s2idle mode in the above report:

#7	int + (😛)| active-↓| level-⚡| S0i3  ∅| S3  ∅| S4/S5  ∅| pull-↑ ∅ (  )| pull-↓ ∅| input  ↑| debounce   ∅ (🕑 46875us)|  0x51b00

Also - in s0ix mode, can you actually wake the system from suspend using the touchpad?

GPIO debug files

s3_gpio.txt

s0ix_gpio.txt

Wakeup from s0ix with the Touchpad works.

Sorry, I'm kind of confused. Are you saying the system can enter S0ix using v6.2-rc4 and also wake up with the touch pad?

Sorry, I'm kind of confused. Are you saying the system can enter S0ix using v6.2-rc4 and also wake up with the touch pad?

(specifically v6.2-rc4 and no patches)

GPIO debug files

Looking at difference from these, GPIO 7 has a debounce value programmed in the s0ix case.

Yes, wake-up from s0ix with the Touchpad seems to work fine. 6.2-rc4 built with the Arch Linux config (https://aur.archlinux.org/pkgbase/linux-mainline).

I think it's a red herring due to a mistake in the debug output not resetting when debounce is off, the register value (last field) is the same on both.

OK guys, I think the right way to solve this is going to be to control policy based on whether S3 is enabled.

Can you both please try this branch in both s3 and s2idle? The touchpad should be able to wake from s2idle, and not from s3 and both sleep modes should work. https://gitlab.freedesktop.org/superm1/linux/-/tree/mlimonci/rhbz-2162013-gitlab-2357-v2

Thanks!

Thank you for the patch!

S0ix: Wakeup via Touchpad, Keyboard and opening the closed lid works fine.

S3: Wakeup via Touchpad and Keyboard doesn't work. Opening the closed lid wakes the system fine. Waking via pressing the button also works.

This looks like the same behavior as with the LTS Kernel (5.15). So I guess with this patch everything should be fine again.

Hrmm, your CL makes me question some things...

So the _PRW method is used to convey the wake GPIOs via an _AEI table, or via a GPE. This method also defines the sleep state the system can be in so the device can still wake the system. i.e., for S3 use GPE X or GPIO Y. Everything is clearly defined. One thing that always bugged me is how undocumented SharedAndWake keyword is. It doesn't have any constraints. I'm starting to wonder if SharedAndWake was specifically added for the Modern Standby (S0ix) case, and shouldn't be used for any other scenarios. Those other scenarios should be covered by the _PRW case. I'm wondering if the proper fix is to remove _S3 from the pinctrl. Thoughts?

This is actually something I was thinking about too when I was looking earlier. There's another part of the documentation that indicates it is intended to be used for S3 or low power idle though.

The “Wake” designation indicates that the interrupt is capable of waking the system from a low-power idle state or a system sleep state

The thing is the /sys/kernel/debug/gpio output showed that wake from S3 wasn't actually enabled in pinctrl-amd. Because of this - I suspect the "wake" must have been an interrupt that occurred before the SOC got into S3.

I was thinking about it more and maybe you're on to something.

enable_irq_wake isn't called by the kernel until the suspend flow for S3, so those bits wouldn't have been set checking at runtime.
We had a really similar regression I remember with S4, and the fix was to stop setting the register for S4.

Checking your idea is cheap and that's a cleaner solution if it works.

@OdyX_ch @berkutta can either of you have a try with this: https://gitlab.freedesktop.org/superm1/linux/-/tree/mlimonci/rhbz-2162013-gitlab-2357-v3

I found a section on wake-capable-interrupts:

On HW-reduced ACPI platforms, wakeup is an attribute of connected interrupts. Interrupts that are designed to wake the processor or the entire platform are defined as wake-capable. Wake-capable interrupts, when enabled by OSPM, wake the system when they assert.

The _PRW section also says the following:

For HW-Reduced ACPI platforms that do not support wake on GPIO-signaled or Interrupt-signaled events, the EventInfo structure is an Integer with value of zero, and is ignored by OSPM. Therefore, _PRW is only required on such platforms if power resources for wakeup must be managed by OSPM (e.g. the _PRW provides a list of Power Resources). Instead, for a device to wake the system, its interrupt must be wake-capable and enabled by the driver. See Interrupt-based Wake Events.

Some Windows documentation says:

If a connected interrupt is wake-capable (capable of waking the system from a low-power idle state, then it must be configured as ExclusiveAndWake or SharedAndWake; for more information, see Device Power Management.

The Windows Device Power Management section says:

Sx Device Wake State (_SxW)

There is an SxW object for each supported system power state, Sx. Because SoC platforms are always in S0, the only object of interest here is _S0W. This object specifies the platform's ability to wake from a low-power idle state in response to a device's wake signal. The object is used by Windows to determine the target D-state for the device during system low-power idle.

For most SoC platforms, devices are aggressively power-managed to the D3 state when idle, and the system is capable of waking from low-power idle while the device is in this state. For such a system, the _S0W object returns 3 (or 4, if it also supports D3cold).

Wake-capable interrupts (_CRS)

The resource description for a device indicates that the device is capable of detecting and signaling a wake event by marking an interrupt as "wake-capable" (either ExclusiveAndWake or SharedAndWake). Windows and device drivers provide special handling of such interrupts to ensure that they are enabled when the device transitions to a low-power state.

Power Resources for Wake (_PRW)

In some cases, additional power resources must be turned on for a device to be enabled for wake. In this case, the device can provide the _PRW object to list those additional power resources.

_PRW is also used to define the wake capability for traditional (full-ACPI hardware) PC platforms.

All of this makes it sound like SharedAndWake is only used to signal S0ix wake capabilities. I think we need to teach the dev_pm_set_wake_irq to only enable the wake IRQ when entering an S0ix state (if the system is ACPI). Or the other option is to move management of the Wake interrupt into the ACPI subsystem (just like _PRW wake is handled today). This will remove the need for the drivers to register the Wake IRQ (for ACPI systems).

Checking your idea is cheap and that's a cleaner solution if it works.

Having though about this idea over night, I think it's going to break the _AEI based GPIOs. I'm assuming acpi-gpio subsystem will eventually call irq_set_wake on the GPIO controller. :/

Having though about this idea over night, I think it's going to break the _AEI based GPIOs. I'm assuming acpi-gpio subsystem will eventually call irq_set_wake on the GPIO controller. :/

What's worth noting is in the implementations I've seen _AEI based GPIOs are disabled when the system is set to S3. If you compare the ACPI dumps above from those Lenovo systems (specifically ssdt5.dat) you'll see they only use _AEI for S2idle mode. That is likely implementation dependent though and I don't see a reason _AEI couldn't be used in a design with S3.

All of this makes it sound like SharedAndWake is only used to signal S0ix wake capabilities. I think we need to teach the dev_pm_set_wake_irq to only enable the wake IRQ when entering an S0ix state (if the system is ACPI). Or the other option is to move management of the Wake interrupt into the ACPI subsystem (just like _PRW wake is handled today). This will remove the need for the drivers to register the Wake IRQ (for ACPI systems).

Would you mind taking a stab at threading that needle? Depending upon how complex it turns out to be, maybe one of the other solutions outlined here can be temporarily put into place for fixing this in 6.2/6.1, and reverted for 6.3 with that.

I don't currently have the cycles :/ I'm wondering if we hacked in an:

if !ACPI || !S0i3 
    device_wakeup_arm_wake_irqs()

into dpm_suspend_noirq would do the trick.

Or maybe we go back to making i2c_acpi_get_irq return false for wake_capable if the system has S0i3 enabled in the FADT?

That's reasonable enough. Here's a v4 to try guys: https://gitlab.freedesktop.org/superm1/linux/-/commits/mlimonci/rhbz-2162013-gitlab-2357-v4/

So I've tested this patchline on top of Debian's 6.1.7, and can indeed confirm that in both S3/Linux and S0ix/Windows BIOS configuration, suspend and restore work fine now. Awesome, thanks!

Thanks for checking.

I've submitted them:

https://lore.kernel.org/linux-gpio/20230121134812.16637-1-mario.limonciello@amd.com/T/#m388a72c975f9f51043d00977f2e763515351c62b

Thanks for leading the charge on this Mario!

Closing this issue as it is fixed in Linus' tree. It will come back to 6.1.y as well.

https://github.com/torvalds/linux/commit/d63f11c02b8d3e54bdb65d8c309f73b7f474aec4

closed

mentioned in commit nouveau@9d369cd5

https://ubuntu.pl/forum/viewtopic.php?p=1042866

https://bugzilla.kernel.org/show_bug.cgi?id=217664 on mesa driver (LLVMPIPE) doesnt go sleep, stuck with _ in top left corner, on "correct" drivers hard dead

Please open your own issue with all details instead of necrobouncing a closed other issue.

locked this issue

S3 suspend broken on AMD Ryzen Pro 7 5850U (regression in kernel 6.1)

Hardware description: Thinkpad X13 Gen 2a

System information:

How to reproduce the issue:

Attached files:

Designs

Child items ...

Activity

Hardware description: ThinkPad T14s Gen 2a

System information:

Log files (for system lockups / game freezes / crashes)

Sx Device Wake State (_SxW)

Wake-capable interrupts (_CRS)

Power Resources for Wake (_PRW)

Admin message

Admin message

S3 suspend broken on AMD Ryzen Pro 7 5850U (regression in kernel 6.1)

Hardware description: Thinkpad X13 Gen 2a

System information:

How to reproduce the issue:

Attached files:

Activity

Hardware description: ThinkPad T14s Gen 2a

System information:

Log files (for system lockups / game freezes / crashes)

Sx Device Wake State (_SxW)

Wake-capable interrupts (_CRS)

Power Resources for Wake (_PRW)