diff options
Diffstat (limited to 'SOURCES/v2-ally-suspend-fix.patch')
| -rw-r--r-- | SOURCES/v2-ally-suspend-fix.patch | 705 |
1 files changed, 440 insertions, 265 deletions
diff --git a/SOURCES/v2-ally-suspend-fix.patch b/SOURCES/v2-ally-suspend-fix.patch index 4084770..42d2182 100644 --- a/SOURCES/v2-ally-suspend-fix.patch +++ b/SOURCES/v2-ally-suspend-fix.patch @@ -1,231 +1,7 @@ -From 57632b4abbcf3f95278c1b71b9de5040463753fe Mon Sep 17 00:00:00 2001 -From: Antheas Kapenekakis <lkml@antheas.dev> -Date: Sun, 22 Sep 2024 12:15:57 +0200 -Subject: [PATCH v2 0/5] acpi/x86: s2idle: move Display off/on calls outside - suspend (fixes ROG Ally suspend) - -The following series moves the Display off/on calls outside of the suspend -sequence, as they are performed in Windows. This fixes certain issues that appear -in devices that use the calls and expect the kernel to be active during their -call (especially in the case of the ROG Ally devices) and opens the possibility -of implementing a "Screen Off" state in the future (which mirrors Windows). -In addition, it adds a quirk table that will allow for adding delays between -Modern Standby transitions, to help resolve racing conditions. - -This series requires a bit of background on how modern standby works in Windows. -Fundamentally, it is composed of four states: "Active", "Screen Off", "Sleep", -and "DRIPS". Here, I take the liberty of naming the state "Active", as it is -implied in Windows documentation. - -When the user actively interacts with a device, it is in the "Active" state. -The screen is on, all devices are connected, and desired software is running. -The other 3 stages play a role once the user stops interacting with the device. -This is triggered in two main ways: either by pressing the power button or by -inactivity. Once either of those targets is met, the system enters Modern Standby. - -Modern Standby consists of an orchestration of the "Screen Off", "Sleep", and -"DRIPS" states. Windows is free to move throughout these states until the user -interacts with the device again, where the device will transition to being -"Active". Moving between the states implies a transition, where Windows performs -a set of actions. In addition, Windows can only move between adjacent states -as follows: - -"Active" <-> "Screen Off" <-> "Sleep" <-> "DRIPS" - -"Screen Off" is the state where all active displays in the device (whether -*virtual* or real; this means unrelated to DRM) are off. The user might still -be interacting with the device or running programs (e.g., compiling a kernel). - -"Sleep" is a newer state, in which the device turns off its fan and pulses its -power button, but still supports running software activities. As part of this, -and to avoid overheating the device a lot of manufacturers lower the TDP (PLx) -of the device [3; _DSM 9 description]. - -Finally, DRIPS stands for Deepest Runtime Idle Power State, i.e. suspend. - -While Windows may transition from any state to any state, doing so implies -performing all transitions to reach that state. All states other than DRIPS -have a fully active kernel (Wifi, USB at least) and allow userspace activity. -What changes is the extent of the activity, and whether some power consuming -devices are turned off (this is done with Modern Standby aware drivers and -firmware notifications). The Windows kernel suspends during the transition from -the "Sleep" state to the "DRIPS" state. In all other states it is fully active. - -After finishing each transition, the kernel performs a firmware notification, -described in the _DSM document [3]. Moving from left to right with function num., -we have Display Off (3; Active -> Screen Off), Sleep Entry (7; Screen Off -> Sleep), -and Lowest Power State Entry Notification (5; LPSEN; Sleep -> DRIPS). Then, from -right to left, we have Lowest Power State Exit Notification (6; DRIPS -> Sleep), -Sleep Exit (8; Sleep -> Screen) and Display On (4; Screen Off -> Active). - -The Linux kernel is not currently Modern Standby aware but will still make these -calls. So, what is the problem? The kernel calls all of the firmware notifications -at the point LPSEN (5, 6) should be called, which is when the kernel is mostly -suspended. This is a clear deviation from Windows, and causes undesirable -behavior in certain devices, the main one focused in this patch series being -the ROG Ally. - -The ROG Ally is a Modern Standby capable device (uses Secure Core too; really -ticks all the MS boxes) and in it, the breakage is double: both Display 3,4 -calls and Sleep 7,8 calls cause issues. - -The Display 3,4 calls are responsible for the controller. The Display Off call -disconnects (if powersave is off) or powers off (if powersave is on and on DC -power) the MCU(s) responsible for the controller and deactivates the RGB of the -device. Display On powers on or reconnects the controller respectively. -This controller, in the Ally X, is composed of 6 HID devices that register to -the kernel. As can be inferred, the handling of the calls during the suspend -sequence leads to a set of undesirable outcomes, such as the controller -soft-locking or only half of the HID devices coming back after resume. - -The Sleep 7,8 calls, in general, are responsible for setting a low power state -that is safe to use while the device is sleeping, making the suspend light -pulse, and turning off the fan. Due to a variety of race conditions, there is -a rare occasion where the Ally EC can get stuck in its Sleep mode, where the -TDP is 5W, and prevent increasing it until the device reboots. The sleep entries -contain actions in the Ally, so there is a suspicion that calling them during -DRIPS is causing issues. However, this is not the subject of this patch and -has not been verified yet. - -This patch centers around moving the Display 3,4 calls outside the suspend -sequence (which is the transition from Sleep to DRIPS in Modern Standby terms), -and by implementing the proper locks necessary, opening up the possibility of -making these calls as part of a more elaborate "Modern Standby"-like userspace -suspend/wakelock implementation. As of this patch, they are only called before -the suspend sequence, including with the possibility of adding a delay. - -This makes the intent of this patch primarily compatibility focused, as it aims -to fix issues by the current implementation. And to that end it works. -After moving the calls outside of the suspend sequence, my ROG Ally X test unit -can suspend more than 50 times without rebooting, both with powersave on or off, -regardless of whether it is plugged/unplugged during suspend, and still have the -controller work with full reliability. Furthermore, in V2, after adding a slight -delay prior to the Display Off call, the controller RGB fades to off, prior the -device suspending and journalctl shows the controller disconnecting prior to -the suspend sequence. - -I will note here that as part of developing Handheld Daemon, I have seen the -suspend sequence of many devices. All of them disconnect the XInput controller -during suspend, and all of them have it show up in the logs after suspend. -The Ally controller after patch (3) is the first one to do it cleanly, before -suspend. Therefore, in my eyes, the quirk table is about to get a lot bigger. - -In addition, moving the calls outside of the suspend sequence (and the validation -work it implies) is an important first step in including "Modern Standby"-like -features in Linux. For example, consider an endpoint /sys/power/standby, that -allows for entering "active", "inactive" (for Screen Off; since the name causes -too much confusion), "sleep" values. Those values will then in turn call the -respective firmware notifications (and driver callbacks in the very future) -for all transitions required to reach the entered state. Here, the value -"suspend" (for DRIPS; another confusing name as it can refer to drivers) is -missing, as userspace will never be able to see it. The kernel should support -suspending at all standby states, orchestrating the required transitions to -reach suspend/DRIPS and after suspend returning to the last state. - -Therefore, if userspace is not standby aware, the kernel will work the same way -it works today. In addition, depending on hardware generation, certain power -states might not be supported. It is important to inform userspace of this, as -if the hardware does not support sleep, and userspace holds a wakelock for sleep, -it will just overheat and drain the device battery. - -This series is worth backing this up with sources, so as part of it I reference -Microsoft's documentation on Modern standby [1-3] that explains the whole -process, including a document by Dell [7] and how to prepare for them and attach a -repository of more than 15 device DSDT tables [4] from different manufacturers. -This repository also contains instructions on how to decode the DSDT tables on -a test laptop, to figure out what the _DSM calls will do on that device. - -Moreover, I conduct a short behavioral test in Windows with the Ally X to showcase -the documentation empirically. The Ally is great for such a test, as it contains -visual indicators for all Microsoft suspend points: "Display Off/On" calls are -indicated with the Controller RGB turning off/on, "Screen Off" is indicated with -the suspend light and fan being on, and Sleep is indicated with the suspend -light blinking. - -Unfortunately, as part of this testing, I never found how to see if the device -is actually suspended. As the ROG Ally X NOOPs on firmware notifications 5,6, -and even though I disabled a Mouse from waking up a device, it still would wake -up my Ally X dev unit. - -Referencing Microsoft's documentation, "Screen Off" is entered either through -inactivity or by pressing the power button, so I conduct two tests: one by pressing -the powerbutton, and one for entering Screen Off due to inactivity. - -1) Powerbutton test: -When pressing the powerbutton, the screen of the Ally turns off, and the RGB of -the controller faints to off within 1s. Following, depending on whether the -system is plugged in, the power light and fan stay on for 5 seconds to 10 minutes. -After this point, the power light begins to blink and the fan turns off, showing -that the system has entered the "Sleep" state. - -2) Inactivity test: -I set the Windows power settings to turn off the screen after 1 minute and wait. -After one minute, the display turns off, and after 5 seconds, the controller RGB -turns off. This indicates to me that "Screen Off" is not defined by the screen -being off, but is rather characterized by it. During those 5 seconds while the -RGB is on, I can use the controller to wake up the device. Afterwards it cannot. - -Those tests validate Microsoft's documentation and show that "Screen Off" -seems to more closely correlate to lockscreen behavior (button locks instantly, -inactivity after 5 seconds) than the screen being off. One other behavior I -notice is that, as I look at my Ally X dev right now, with its screen off, I -notice the RGB is still on, which is kind of bothersome, as in Windows the -device would turn the RGB off. Whether as a side effect or planned, it is still -a nice touch. - -This patch series is co-developed with help from Mario Limonciello, and, to be -bisection friendly, is structured based on a patch series he made connecting the -callbacks to the drm subsystem suspend [5]. It also references (already) -upstream work by Luke Jones on Asus-wmi for the Ally controller quirk that is -removed on patch (5) and an issue on amd-drm in 2023 in preparation for the -work in that quirk [6]. Since patch (3) now uses part of the dmi table removed -in patch (5), @Luke I can add you as Suggested-by. - -We will begin thorough testing on the patch series, and there will probably be -a V3, where testing acknowledgements are added. - -Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/prepare-hardware-for-modern-standby [1] -Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/prepare-software-for-modern-standby [2] -Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications [3] -Link: https://github.com/hhd-dev/hwinfo/tree/master/devices [4] -Link: https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/log/?h=superm1/dsm-screen-on-off [5] -Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2719 [6] -Link: https://dl.dell.com/manuals/all-products/esuprt_solutions_int/esuprt_solutions_int_solutions_resources/client-mobile-solution-resources_white-papers45_en-us.pdf [7] - -Changes in v2: - - Rewrote cover letter to better reflect the Windows Modern Standby sequence - - Renamed the "screen_" callbacks to "display_" to match Microsoft's naming - - Added attribution to Mario Limonciello and changed the text to reflect that - - Made the screen on/off callbacks warn and bail with -EINVAL if they are - called in the wrong order (currently impossible) - - Changed patch 2 to not bail the suspend sequence when receiving an error - (as these calls are not part of the suspend sequence and failing the - suspend sequence would cause a user visible fault) - - Fixed bug reported by Denis Benato by adding a quirk table in patch (3) - - The ROG controllers get a slight delay after the Display Off call - - Reworded patches to reflect that - -Antheas Kapenekakis (5): - acpi/x86: s2idle: add support for Display Off and Display On callbacks - acpi/x86: s2idle: handle Display On/Off calls outside of suspend - sequence - acpi/x86: s2idle: add quirk table for modern standby delays - acpi/x86: s2idle: call Display On/Off as part of callbacks and rename - platform/x86: asus-wmi: remove Ally (1st gen) and Ally X suspend quirk - - drivers/acpi/x86/s2idle.c | 89 +++++++++++++++++++++++---------- - drivers/platform/x86/asus-wmi.c | 54 -------------------- - include/linux/suspend.h | 10 ++++ - kernel/power/suspend.c | 62 +++++++++++++++++++++++ - 4 files changed, 134 insertions(+), 81 deletions(-) - --- -2.46.1 - From ff87284862db614a4067b2d7383ecb173e1454de Mon Sep 17 00:00:00 2001 From: Antheas Kapenekakis <lkml@antheas.dev> Date: Wed, 18 Sep 2024 23:53:53 +0200 -Subject: [PATCH v2 1/5] acpi/x86: s2idle: add support for Display Off and +Subject: [PATCH v3 01/10] acpi/x86: s2idle: add support for Display Off and Display On callbacks The Display Off and Display On firmware notifications are meant to signify @@ -301,14 +77,14 @@ index 09f8397bae15..c527dc0ae5ae 100644 { return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare ? -- -2.46.1 +2.46.2 -From a48e63268c59847e5349d0f1d383be2fe60291e7 Mon Sep 17 00:00:00 2001 +From 0f25cd27999b0fe6c0382208452422fd1866da1e Mon Sep 17 00:00:00 2001 From: Antheas Kapenekakis <lkml@antheas.dev> Date: Thu, 19 Sep 2024 00:02:32 +0200 -Subject: [PATCH v2 2/5] acpi/x86: s2idle: handle Display On/Off calls outside - of suspend sequence +Subject: [PATCH v3 02/10] acpi/x86: s2idle: handle Display On/Off calls + outside of suspend sequence Currently, the Display On/Off calls are handled within the suspend sequence, which is a deviation from Windows. This causes issues with @@ -328,12 +104,12 @@ Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> 1 file changed, 9 insertions(+) diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c -index c527dc0ae5ae..8f951cc0cb64 100644 +index c527dc0ae5ae..610f8ecaeebd 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c -@@ -507,6 +507,13 @@ int suspend_devices_and_enter(suspend_state_t state) - - pm_suspend_target_state = state; +@@ -589,6 +589,13 @@ static int enter_state(suspend_state_t state) + if (state == PM_SUSPEND_TO_IDLE) + s2idle_begin(); + /* + * Linux does not have the concept of a "Screen Off" state, so call @@ -342,26 +118,26 @@ index c527dc0ae5ae..8f951cc0cb64 100644 + */ + platform_suspend_display_off(); + - if (state == PM_SUSPEND_TO_IDLE) - pm_set_suspend_no_platform(); - -@@ -540,6 +547,8 @@ int suspend_devices_and_enter(suspend_state_t state) - Close: - platform_resume_end(state); - pm_suspend_target_state = PM_SUSPEND_ON; + if (sync_on_suspend_enabled) { + trace_suspend_resume(TPS("sync_filesystems"), 0, true); + ksys_sync_helper(); +@@ -616,6 +623,8 @@ static int enter_state(suspend_state_t state) + suspend_finish(); + Unlock: + mutex_unlock(&system_transition_mutex); + + platform_suspend_display_on(); return error; + } - Recover_platform: -- -2.46.1 +2.46.2 -From faa52b2b9af894ba3b752e1d35ac389fa2b915cc Mon Sep 17 00:00:00 2001 +From e0cad271a26ff16ea9456e0104ea10e1195f6d47 Mon Sep 17 00:00:00 2001 From: Antheas Kapenekakis <lkml@antheas.dev> Date: Sun, 22 Sep 2024 11:47:29 +0200 -Subject: [PATCH v2 3/5] acpi/x86: s2idle: add quirk table for modern standby +Subject: [PATCH v3 03/10] acpi/x86: s2idle: add quirk table for modern standby delays Unfortunately, some modern standby systems, including the ROG Ally, rely @@ -394,7 +170,7 @@ index 8f33249cc067..d7e2a4d8ab0c 100644 extern suspend_state_t pm_suspend_target_state; extern suspend_state_t mem_sleep_current; diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c -index 8f951cc0cb64..af807e621f28 100644 +index 610f8ecaeebd..af2abdd2f8c3 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c @@ -11,6 +11,7 @@ @@ -436,11 +212,10 @@ index 8f951cc0cb64..af807e621f28 100644 /** * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend. * -@@ -506,6 +531,18 @@ int suspend_devices_and_enter(suspend_state_t state) - return -ENOSYS; +@@ -589,12 +614,26 @@ static int enter_state(suspend_state_t state) + if (state == PM_SUSPEND_TO_IDLE) + s2idle_begin(); - pm_suspend_target_state = state; -+ + /* + * Windows transitions between Modern Standby states slowly, over multiple + * seconds. Certain manufacturers may rely on this, introducing race @@ -452,35 +227,35 @@ index 8f951cc0cb64..af807e621f28 100644 + ); + const struct platform_s2idle_quirks *s2idle_quirks = s2idle_sysid ? + s2idle_sysid->driver_data : NULL; - ++ /* * Linux does not have the concept of a "Screen Off" state, so call -@@ -513,6 +550,8 @@ int suspend_devices_and_enter(suspend_state_t state) + * the platform functions for Display On/Off prior to the suspend * sequence, mirroring Windows which calls them outside of it as well. */ platform_suspend_display_off(); + if (s2idle_quirks && s2idle_quirks->delay_display_off) + msleep(s2idle_quirks->delay_display_off); - if (state == PM_SUSPEND_TO_IDLE) - pm_set_suspend_no_platform(); -@@ -548,6 +587,8 @@ int suspend_devices_and_enter(suspend_state_t state) - platform_resume_end(state); - pm_suspend_target_state = PM_SUSPEND_ON; + if (sync_on_suspend_enabled) { + trace_suspend_resume(TPS("sync_filesystems"), 0, true); +@@ -624,6 +663,8 @@ static int enter_state(suspend_state_t state) + Unlock: + mutex_unlock(&system_transition_mutex); + if (s2idle_quirks && s2idle_quirks->delay_display_on) + msleep(s2idle_quirks->delay_display_on); platform_suspend_display_on(); return error; - + } -- -2.46.1 +2.46.2 -From 02137e51d6a558fccf7a4661ae31e8c019cae984 Mon Sep 17 00:00:00 2001 +From 230395bedb589220c3c126d7f33156edbb3dd1c7 Mon Sep 17 00:00:00 2001 From: Antheas Kapenekakis <lkml@antheas.dev> Date: Thu, 19 Sep 2024 00:22:03 +0200 -Subject: [PATCH v2 4/5] acpi/x86: s2idle: call Display On/Off as part of +Subject: [PATCH v3 04/10] acpi/x86: s2idle: call Display On/Off as part of callbacks and rename Move the Display On/Off notifications into dedicated callbacks that gate @@ -667,14 +442,14 @@ index dd0b40b9bbe8..a17e28b91326 100644 void __init acpi_s2idle_setup(void) -- -2.46.1 +2.46.2 -From 57632b4abbcf3f95278c1b71b9de5040463753fe Mon Sep 17 00:00:00 2001 +From 69cdca368e53713088771d2866adc48ac19cbf29 Mon Sep 17 00:00:00 2001 From: Antheas Kapenekakis <lkml@antheas.dev> Date: Thu, 19 Sep 2024 00:29:59 +0200 -Subject: [PATCH v2 5/5] platform/x86: asus-wmi: remove Ally (1st gen) and Ally - X suspend quirk +Subject: [PATCH v3 05/10] platform/x86: asus-wmi: remove Ally (1st gen) and + Ally X suspend quirk By moving the Display On/Off calls outside of the suspend sequence and introducing a slight delay after Display Off, the ROG Ally controller @@ -770,5 +545,405 @@ index 37636e5a38e3..2c9656e0afda 100644 /* Registration ***************************************************************/ -- -2.46.1 +2.46.2 + + +From 2d446e1e66be5ae07bb7c243a7c4383b49b1aa3e Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Wed, 25 Sep 2024 14:10:11 +0200 +Subject: [PATCH v3 06/10] acpi/x86: s2idle: add support for Sleep Entry and + Sleep Exit callbacks + +The Sleep Entry and Sleep Exit firmware notifications allow the platform +to enter Modern Standby. In this state, if supported, the platform turns +off auxiliary USB devices (e.g., the controllers of the Legion Go), +makes the power light of the device flash, and lowers the power envelope +to a minimum that still allows for software activity without affecting +battery life. + +Allow for entering this state prior to initiating the suspend sequence. +This fixes issues where the EC or the USB of the device need time to +power down before entering the suspend sequence, and allows for entering +this power state without suspending the device. + +Suggested-by: Mario Limonciello <mario.limonciello@amd.com> +Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> +--- + include/linux/suspend.h | 4 ++++ + kernel/power/suspend.c | 12 ++++++++++++ + 2 files changed, 16 insertions(+) + +diff --git a/include/linux/suspend.h b/include/linux/suspend.h +index d7e2a4d8ab0c..66c5b434334d 100644 +--- a/include/linux/suspend.h ++++ b/include/linux/suspend.h +@@ -133,6 +133,7 @@ struct platform_suspend_ops { + + struct platform_s2idle_ops { + int (*display_off)(void); ++ int (*sleep_entry)(void); + int (*begin)(void); + int (*prepare)(void); + int (*prepare_late)(void); +@@ -141,6 +142,7 @@ struct platform_s2idle_ops { + void (*restore_early)(void); + void (*restore)(void); + void (*end)(void); ++ int (*sleep_exit)(void); + int (*display_on)(void); + }; + +@@ -168,6 +170,8 @@ extern unsigned int pm_suspend_global_flags; + #define PM_SUSPEND_FLAG_NO_PLATFORM BIT(2) + + int platform_suspend_display_off(void); ++int platform_suspend_sleep_entry(void); ++int platform_suspend_sleep_exit(void); + int platform_suspend_display_on(void); + + static inline void pm_suspend_clear_flags(void) +diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c +index af2abdd2f8c3..dab299e28195 100644 +--- a/kernel/power/suspend.c ++++ b/kernel/power/suspend.c +@@ -285,6 +285,18 @@ int platform_suspend_display_off(void) + } + EXPORT_SYMBOL_GPL(platform_suspend_display_off); + ++int platform_suspend_sleep_entry(void) ++{ ++ return s2idle_ops && s2idle_ops->sleep_entry ? s2idle_ops->sleep_entry() : 0; ++} ++EXPORT_SYMBOL_GPL(platform_suspend_sleep_entry); ++ ++int platform_suspend_sleep_exit(void) ++{ ++ return s2idle_ops && s2idle_ops->sleep_exit ? s2idle_ops->sleep_exit() : 0; ++} ++EXPORT_SYMBOL_GPL(platform_suspend_sleep_exit); ++ + int platform_suspend_display_on(void) + { + return s2idle_ops && s2idle_ops->display_on ? s2idle_ops->display_on() : 0; +-- +2.46.2 + + +From ec33ea0c341d5b75a090910664c572d8d4793bbc Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Wed, 25 Sep 2024 14:19:00 +0200 +Subject: [PATCH v3 07/10] acpi/x86: s2idle: handle Sleep Entry/Exit calls + outside of suspend sequence + +As with Display On/Off, these calls should be made outside the suspend +sequence, to allow the EC and USB devices that are affected to complete +their power off sequence before the kernel suspends their power rails +and interrupts. + +Suggested-by: Mario Limonciello <mario.limonciello@amd.com> +Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> +--- + kernel/power/suspend.c | 9 +++++++++ + 1 file changed, 9 insertions(+) + +diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c +index dab299e28195..9dcdd5273318 100644 +--- a/kernel/power/suspend.c ++++ b/kernel/power/suspend.c +@@ -547,6 +547,13 @@ int suspend_devices_and_enter(suspend_state_t state) + if (state == PM_SUSPEND_TO_IDLE) + pm_set_suspend_no_platform(); + ++ /* ++ * Linux does not have the concept of a "Sleep" state. As with Display ++ * On/Off, call the platform functions for Sleep Entry/Exit prior to the ++ * suspend sequence. ++ */ ++ platform_suspend_sleep_entry(); ++ + error = platform_suspend_begin(state); + if (error) + goto Close; +@@ -577,6 +584,8 @@ int suspend_devices_and_enter(suspend_state_t state) + Close: + platform_resume_end(state); + pm_suspend_target_state = PM_SUSPEND_ON; ++ ++ platform_suspend_sleep_exit(); + return error; + + Recover_platform: +-- +2.46.2 + + +From 0825b095f26cbea829078f70d4c757ff9b2eab38 Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Wed, 25 Sep 2024 14:29:42 +0200 +Subject: [PATCH v3 08/10] acpi/x86: s2idle: update quirk table for Sleep + Entry/Exit + +Add delays between the Sleep Entry and Sleep Exit calls, to avoid issues +in devices that rely on them that need time to power off. + +Especially for the ROG Ally, this should allow its EC to suspend gracefully, +avoiding issues where it is stuck in its suspend state. Since the delays +are additive, steal some of the delay from Display On/Off. + +Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> +--- + include/linux/suspend.h | 2 ++ + kernel/power/suspend.c | 21 +++++++++++++++++++-- + 2 files changed, 21 insertions(+), 2 deletions(-) + +diff --git a/include/linux/suspend.h b/include/linux/suspend.h +index 66c5b434334d..5b4d4d9ef65a 100644 +--- a/include/linux/suspend.h ++++ b/include/linux/suspend.h +@@ -148,6 +148,8 @@ struct platform_s2idle_ops { + + struct platform_s2idle_quirks { + int delay_display_off; ++ int delay_sleep_entry; ++ int delay_sleep_exit; + int delay_display_on; + }; + +diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c +index 9dcdd5273318..1352c4066822 100644 +--- a/kernel/power/suspend.c ++++ b/kernel/power/suspend.c +@@ -65,8 +65,11 @@ static DEFINE_RAW_SPINLOCK(s2idle_lock); + // The ROG Ally series disconnects its controllers on Display Off, without + // holding a lock, introducing a race condition. Add a delay to allow the + // controller to disconnect cleanly prior to suspend. ++// In addition, the EC of the device rarely (1/20 attempts) may get stuck ++// after suspend in an invalid state, where it mirros Sleep behavior. + static const struct platform_s2idle_quirks rog_ally_quirks = { +- .delay_display_off = 500, ++ .delay_display_off = 200, ++ .delay_sleep_entry = 300, + }; + + static const struct dmi_system_id platform_s2idle_quirks[] = { +@@ -548,11 +551,23 @@ int suspend_devices_and_enter(suspend_state_t state) + pm_set_suspend_no_platform(); + + /* +- * Linux does not have the concept of a "Sleep" state. As with Display ++ * Windows transitions between Modern Standby states slowly, as with ++ * Display On/Off, query the appropriate delays here for Sleep Entry/Exit. ++ */ ++ const struct dmi_system_id *s2idle_sysid = dmi_first_match( ++ platform_s2idle_quirks ++ ); ++ const struct platform_s2idle_quirks *s2idle_quirks = s2idle_sysid ? ++ s2idle_sysid->driver_data : NULL; ++ ++ /* ++ * Linux does not have the concept of a "Sleep" state. As done with Display + * On/Off, call the platform functions for Sleep Entry/Exit prior to the + * suspend sequence. + */ + platform_suspend_sleep_entry(); ++ if (s2idle_quirks && s2idle_quirks->delay_sleep_entry) ++ msleep(s2idle_quirks->delay_sleep_entry); + + error = platform_suspend_begin(state); + if (error) +@@ -585,6 +600,8 @@ int suspend_devices_and_enter(suspend_state_t state) + platform_resume_end(state); + pm_suspend_target_state = PM_SUSPEND_ON; + ++ if (s2idle_quirks && s2idle_quirks->delay_sleep_exit) ++ msleep(s2idle_quirks->delay_sleep_exit); + platform_suspend_sleep_exit(); + return error; + +-- +2.46.2 + + +From 49cafd9d1cf20250e31f34a849c505d205968ef5 Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Wed, 25 Sep 2024 14:41:03 +0200 +Subject: [PATCH v3 09/10] acpi/x86: s2idle: call Sleep Entry/Exit as part of + callbacks. + +Move the Sleep Entry/Exit notifications outside the suspend sequence, +with their own ACPI lock, as was done for Display On/Off. + +Suggested-by: Mario Limonciello <mario.limonciello@amd.com> +Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> +--- + drivers/acpi/x86/s2idle.c | 69 ++++++++++++++++++++++++++++++--------- + 1 file changed, 53 insertions(+), 16 deletions(-) + +diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c +index a17e28b91326..6ff5e34c016e 100644 +--- a/drivers/acpi/x86/s2idle.c ++++ b/drivers/acpi/x86/s2idle.c +@@ -43,8 +43,8 @@ static const struct acpi_device_id lps0_device_ids[] = { + #define ACPI_LPS0_DISPLAY_ON 4 + #define ACPI_LPS0_ENTRY 5 + #define ACPI_LPS0_EXIT 6 +-#define ACPI_LPS0_MS_ENTRY 7 +-#define ACPI_LPS0_MS_EXIT 8 ++#define ACPI_LPS0_SLEEP_ENTRY 7 ++#define ACPI_LPS0_SLEEP_EXIT 8 + + /* AMD */ + #define ACPI_LPS0_DSM_UUID_AMD "e3f32452-febc-43ce-9039-932122d37721" +@@ -61,6 +61,7 @@ static guid_t lps0_dsm_guid_microsoft; + static int lps0_dsm_func_mask_microsoft; + static int lps0_dsm_state; + static bool lsp0_dsm_in_display_off; ++static bool lsp0_dsm_in_sleep; + + /* Device constraint entry structure */ + struct lpi_device_info { +@@ -370,10 +371,10 @@ static const char *acpi_sleep_dsm_state_to_str(unsigned int state) + return "lps0 entry"; + case ACPI_LPS0_EXIT: + return "lps0 exit"; +- case ACPI_LPS0_MS_ENTRY: +- return "lps0 ms entry"; +- case ACPI_LPS0_MS_EXIT: +- return "lps0 ms exit"; ++ case ACPI_LPS0_SLEEP_ENTRY: ++ return "lps0 sleep entry"; ++ case ACPI_LPS0_SLEEP_EXIT: ++ return "lps0 sleep exit"; + } + } else { + switch (state) { +@@ -567,6 +568,48 @@ static int acpi_s2idle_display_off(void) + return 0; + } + ++static int acpi_s2idle_sleep_entry(void) ++{ ++ if (!lps0_device_handle || sleep_no_lps0 || lps0_dsm_func_mask_microsoft <= 0) ++ return 0; ++ ++ if (WARN_ON(lsp0_dsm_in_sleep)) ++ return -EINVAL; ++ ++ lsp0_dsm_in_sleep = true; ++ acpi_scan_lock_acquire(); ++ ++ /* Modern Standby Sleep Entry */ ++ if (lps0_dsm_func_mask_microsoft > 0) ++ acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_ENTRY, ++ lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); ++ ++ acpi_scan_lock_release(); ++ ++ return 0; ++} ++ ++static int acpi_s2idle_sleep_exit(void) ++{ ++ if (!lps0_device_handle || sleep_no_lps0 || lps0_dsm_func_mask_microsoft <= 0) ++ return 0; ++ ++ if (WARN_ON(!lsp0_dsm_in_sleep)) ++ return -EINVAL; ++ ++ lsp0_dsm_in_sleep = false; ++ acpi_scan_lock_acquire(); ++ ++ /* Modern Standby Sleep Exit */ ++ if (lps0_dsm_func_mask_microsoft > 0) ++ acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_EXIT, ++ lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); ++ ++ acpi_scan_lock_release(); ++ ++ return 0; ++} ++ + static int acpi_s2idle_display_on(void) + { + if (!lps0_device_handle || sleep_no_lps0) +@@ -608,13 +651,9 @@ int acpi_s2idle_prepare_late(void) + acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY_AMD, + lps0_dsm_func_mask, lps0_dsm_guid); + +- if (lps0_dsm_func_mask_microsoft > 0) { +- /* Modern Standby entry */ +- acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_ENTRY, +- lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); ++ if (lps0_dsm_func_mask_microsoft > 0) + acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY, + lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); +- } + + if (lps0_dsm_func_mask > 0 && !acpi_s2idle_vendor_amd()) + acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY, +@@ -659,17 +698,14 @@ void acpi_s2idle_restore_early(void) + ACPI_LPS0_EXIT, + lps0_dsm_func_mask, lps0_dsm_guid); + +- if (lps0_dsm_func_mask_microsoft > 0) { ++ if (lps0_dsm_func_mask_microsoft > 0) + acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT, + lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); +- /* Modern Standby exit */ +- acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT, +- lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); +- } + } + + static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = { + .display_off = acpi_s2idle_display_off, ++ .sleep_entry = acpi_s2idle_sleep_entry, + .begin = acpi_s2idle_begin, + .prepare = acpi_s2idle_prepare, + .prepare_late = acpi_s2idle_prepare_late, +@@ -678,6 +714,7 @@ static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = { + .restore_early = acpi_s2idle_restore_early, + .restore = acpi_s2idle_restore, + .end = acpi_s2idle_end, ++ .sleep_exit = acpi_s2idle_sleep_exit, + .display_on = acpi_s2idle_display_on, + }; + +-- +2.46.2 + + +From 93f8e91ade24bbf3c30c5633d57a3730d0f03a8a Mon Sep 17 00:00:00 2001 +From: Antheas Kapenekakis <lkml@antheas.dev> +Date: Wed, 25 Sep 2024 14:42:54 +0200 +Subject: [PATCH v3 10/10] [NOT FOR UPSTREAM] acpi/x86: s2idle: apply delay to + transitions for all devices + +For the purposes of testing, apply the suspend delay to all devices. +Before moving the sleep notifications outside of the suspend sequence, +the EC NOOPed in regards to the CPU of the system (e.g., did not lower +its TDP or had a race condition). Therefore, add this delay to see if +other Modern Standby systems are affected by the same quirk of the ROG +Ally. +--- + kernel/power/suspend.c | 4 ++++ + 1 file changed, 4 insertions(+) + +diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c +index 1352c4066822..fa9e148f4e54 100644 +--- a/kernel/power/suspend.c ++++ b/kernel/power/suspend.c +@@ -85,6 +85,10 @@ static const struct dmi_system_id platform_s2idle_quirks[] = { + }, + .driver_data = (void *)&rog_ally_quirks + }, ++ { ++ .matches = {}, ++ .driver_data = (void *)&rog_ally_quirks ++ }, + {} + }; + +-- +2.46.2 |