Re: [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices

From: Marek Szyprowski
Date: Fri Aug 09 2019 - 08:00:57 EST

Next message: Mark Brown: "Re: [PATCH 07/22] ARM: omap1: move perseus spi pinconf to board file"
Previous message: Ardelean, Alexandru: "Re: [PATCH v2 02/15] net: phy: adin: hook genphy_read_abilities() to get_features"
Next in thread: Rafael J. Wysocki: "Re: [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Rafael,

On 2019-08-02 12:45, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> According to Section 3.5 of the "Intel Low Power S0 Idle" document [1],
> Function 5 of the LPS0 _DSM is expected to be invoked when the system
> configuration matches the criteria for entering the target low-power
> state of the platform. In particular, this means that all devices
> should be suspended and in low-power states already when that function
> is invoked.
>
> This is not the case currently, however, because Function 5 of the
> LPS0 _DSM is invoked by it before the "noirq" phase of device suspend,
> which means that some devices may not have been put into low-power
> states yet at that point. That is a consequence of the previous
> design of the suspend-to-idle flow that allowed the "noirq" phase of
> device suspend and the "noirq" phase of device resume to be carried
> out for multiple times while "suspended" (if any spurious wakeup
> events were detected) and the point of the LPS0 _DSM Function 5
> invocation was chosen so as to call it (and LPS0 _DSM Function 6
> analogously) once per suspend-resume cycle (regardless of how many
> times the "noirq" phases of device suspend and resume were carried
> out while "suspended").
>
> Now that the suspend-to-idle flow has been redesigned to carry out
> the "noirq" phases of device suspend and resume once in each cycle,
> the code can be reordered to follow the specification that it is
> based on more closely.
>
> For this purpose, add ->prepare_late and ->restore_early platform
> callbacks for suspend-to-idle, to be executed, respectively, after
> the "noirq" phase of suspending devices and before the "noirq"
> phase of resuming them and make ACPI use them for the invocation
> of LPS0 _DSM functions as appropriate.
>
> While at it, move the LPS0 entry requirements check to be made
> before invoking Functions 3 and 5 of the LPS0 _DSM (also once
> per cycle) as follows from the specification [1].
>
> Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf # [1]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
>
> In v2 this was patch 2.
>
> ---
> drivers/acpi/sleep.c | 36 ++++++++++++++++++++++++------------
> include/linux/suspend.h | 2 ++
> kernel/power/suspend.c | 12 +++++++++---
> 3 files changed, 35 insertions(+), 15 deletions(-)
>
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -954,11 +954,6 @@ static int acpi_s2idle_begin(void)
>
> static int acpi_s2idle_prepare(void)
> {
> - if (lps0_device_handle && !sleep_no_lps0) {
> - acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> - acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> - }
> -
> if (acpi_sci_irq_valid())
> enable_irq_wake(acpi_sci_irq);
>
> @@ -972,11 +967,22 @@ static int acpi_s2idle_prepare(void)
> return 0;
> }
>
> -static void acpi_s2idle_wake(void)
> +static int acpi_s2idle_prepare_late(void)
> {
> - if (lps0_device_handle && !sleep_no_lps0 && pm_debug_messages_on)
> + if (!lps0_device_handle || sleep_no_lps0)
> + return 0;
> +
> + if (pm_debug_messages_on)
> lpi_check_constraints();
>
> + acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> + acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> +
> + return 0;
> +}
> +
> +static void acpi_s2idle_wake(void)
> +{
> /*
> * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the SCI has
> * not triggered while suspended, so bail out.
> @@ -1011,6 +1017,15 @@ static void acpi_s2idle_wake(void)
> rearm_wake_irq(acpi_sci_irq);
> }
>
> +static void acpi_s2idle_restore_early(void)
> +{
> + if (!lps0_device_handle || sleep_no_lps0)
> + return;
> +
> + acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
> + acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
> +}
> +
> static void acpi_s2idle_restore(void)
> {
> s2idle_wakeup = false;
> @@ -1021,11 +1036,6 @@ static void acpi_s2idle_restore(void)
>
> if (acpi_sci_irq_valid())
> disable_irq_wake(acpi_sci_irq);
> -
> - if (lps0_device_handle && !sleep_no_lps0) {
> - acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
> - acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
> - }
> }
>
> static void acpi_s2idle_end(void)
> @@ -1036,7 +1046,9 @@ static void acpi_s2idle_end(void)
> static const struct platform_s2idle_ops acpi_s2idle_ops = {
> .begin = acpi_s2idle_begin,
> .prepare = acpi_s2idle_prepare,
> + .prepare_late = acpi_s2idle_prepare_late,
> .wake = acpi_s2idle_wake,
> + .restore_early = acpi_s2idle_restore_early,
> .restore = acpi_s2idle_restore,
> .end = acpi_s2idle_end,
> };
> Index: linux-pm/kernel/power/suspend.c
> ===================================================================
> --- linux-pm.orig/kernel/power/suspend.c
> +++ linux-pm/kernel/power/suspend.c
> @@ -253,13 +253,19 @@ static int platform_suspend_prepare_late
>
> static int platform_suspend_prepare_noirq(suspend_state_t state)
> {
> - return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare_late ?
> - suspend_ops->prepare_late() : 0;
> + if (state == PM_SUSPEND_TO_IDLE) {
> + if (s2idle_ops && s2idle_ops->prepare_late)
> + return s2idle_ops->prepare_late();
> + }
> + return suspend_ops->prepare_late ? suspend_ops->prepare_late() : 0;

This unconditionally references suspend_ops here, what wasn't done
earlier. On one of my test boards (OdroidXU) it causes following NULL
pointer dereference since Linux next-20190809 (the first -next, which
contains this patch):

root@target:~# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Fri AugÂ 9 12:21:43 2019
PM: suspend entry (s2idle)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
smsc95xx 1-2:1.0 eth0: entering SUSPEND2 mode
wake enabled for irq 111
usb3503 4-0008: switched to STANDBY mode
samsung-pinctrl 13400000.pinctrl: No retention data configured bank with
external wakeup interrupt. Wake-up mask will not be set.
8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 0000000c
pgd = ac9cf656
[0000000c] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 2027 Comm: rtcwake Not tainted 5.3.0-rc3-next-20190809 #6373
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
PC is at suspend_devices_and_enter+0x330/0xf2c
LR is at lock_is_held_type+0x80/0x108
pc : [<c0195b78>]ÂÂÂ lr : [<c01883dc>]ÂÂÂ psr: 60000013
sp : e73dfe20Â ip : 00000001Â fp : 00000001
r10: c10b9394Â r9 : c10ce36cÂ r8 : c1007648
r7 : c16d26c8Â r6 : 00000001Â r5 : 00000000Â r4 : c16d26dc
r3 : 00000000Â r2 : 00000000Â r1 : c1014220Â r0 : 00000000
Flags: nZCvÂ IRQs onÂ FIQs onÂ Mode SVC_32Â ISA ARMÂ Segment none
Control: 10c5387dÂ Table: 671ac06aÂ DAC: 00000051
Process rtcwake (pid: 2027, stack limit = 0x59af371a)
Stack: (0xe73dfe20 to 0xe73e0000)
...
[<c0195b78>] (suspend_devices_and_enter) from [<c0196d0c>]
(pm_suspend+0x598/0xcb8)
[<c0196d0c>] (pm_suspend) from [<c0194848>] (state_store+0x6c/0xc8)
[<c0194848>] (state_store) from [<c0330100>] (kernfs_fop_write+0x100/0x1e0)
[<c0330100>] (kernfs_fop_write) from [<c02a1594>] (__vfs_write+0x30/0x1d0)
[<c02a1594>] (__vfs_write) from [<c02a4090>] (vfs_write+0xa4/0x180)
[<c02a4090>] (vfs_write) from [<c02a42f0>] (ksys_write+0x60/0xdc)
[<c02a42f0>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
Exception stack(0xe73dffa8 to 0xe73dfff0)
...
---[ end trace 627069b7dfd482c9 ]---

> }
>
> static void platform_resume_noirq(suspend_state_t state)
> {
> - if (state != PM_SUSPEND_TO_IDLE && suspend_ops->wake)
> + if (state == PM_SUSPEND_TO_IDLE) {
> + if (s2idle_ops && s2idle_ops->restore_early)
> + s2idle_ops->restore_early();
> + } else if (suspend_ops->wake)
> suspend_ops->wake();
> }
>
> Index: linux-pm/include/linux/suspend.h
> ===================================================================
> --- linux-pm.orig/include/linux/suspend.h
> +++ linux-pm/include/linux/suspend.h
> @@ -190,7 +190,9 @@ struct platform_suspend_ops {
> struct platform_s2idle_ops {
> int (*begin)(void);
> int (*prepare)(void);
> + int (*prepare_late)(void);
> void (*wake)(void);
> + void (*restore_early)(void);
> void (*restore)(void);
> void (*end)(void);
> };

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Next message: Mark Brown: "Re: [PATCH 07/22] ARM: omap1: move perseus spi pinconf to board file"
Previous message: Ardelean, Alexandru: "Re: [PATCH v2 02/15] net: phy: adin: hook genphy_read_abilities() to get_features"
Next in thread: Rafael J. Wysocki: "Re: [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]