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

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi Rafael,

On 2019-08-02 12:45, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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@intel.com>
> ---
>
> 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