linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marek Szyprowski <m.szyprowski@samsung.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Linux ACPI <linux-acpi@vger.kernel.org>
Cc: Linux PM <linux-pm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Zhang Rui <rui.zhang@intel.com>,
	Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Mario Limonciello <mario.limonciello@dell.com>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>
Subject: Re: [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices
Date: Fri, 9 Aug 2019 14:00:50 +0200	[thread overview]
Message-ID: <1b181f35-29c3-c6ce-6c42-ae55e890579e@samsung.com> (raw)
In-Reply-To: <74514118.QN1Ey1fWSL@kreacher>

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


  parent reply	other threads:[~2019-08-09 12:00 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
2019-08-02 10:38 ` [PATCH v3 1/8] ACPI: PM: Set up EC GPE for system wakeup from drivers that need it Rafael J. Wysocki
2019-08-02 10:38 ` [PATCH v3 2/8] ACPI: PM: s2idle: Rearrange lps0_device_attach() Rafael J. Wysocki
2019-08-02 10:38 ` [PATCH v3 3/8] ACPI: PM: s2idle: Add acpi.sleep_no_lps0 module parameter Rafael J. Wysocki
2019-08-02 10:40 ` [PATCH v3 4/8] ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend Rafael J. Wysocki
2019-08-02 10:41 ` [PATCH v3 5/8] ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events() Rafael J. Wysocki
2019-08-02 10:43 ` [PATCH v3 6/8] ACPI: EC: PM: Consolidate some code depending on PM_SLEEP Rafael J. Wysocki
2019-08-02 10:44 ` [PATCH v3 7/8] ACPI: EC: PM: Make acpi_ec_dispatch_gpe() print debug message Rafael J. Wysocki
2019-08-02 10:45 ` [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices Rafael J. Wysocki
     [not found]   ` <CGME20190809120052eucas1p11b56806662ef4f4efb82a152ad651481@eucas1p1.samsung.com>
2019-08-09 12:00     ` Marek Szyprowski [this message]
2019-08-09 12:15       ` Rafael J. Wysocki
2019-08-10 11:24       ` [PATCH] PM: suspend: Fix platform_suspend_prepare_noirq() Rafael J. Wysocki
2019-08-05 16:25 ` [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Kai-Heng Feng
2019-08-12 13:55 ` Bhardwaj, Rajneesh
2019-08-12 21:32   ` Rafael J. Wysocki
2019-08-16 20:20 ` Kristian Klausen
2019-08-19  7:59   ` Rafael J. Wysocki
2019-08-19  9:05     ` Rafael J. Wysocki
2019-08-19 15:45       ` Kristian Klausen
2019-08-19 20:41         ` Rafael J. Wysocki
2019-08-20 13:10           ` Kristian Klausen
2019-08-20 13:29             ` Rafael J. Wysocki
2019-08-20 21:38               ` Rafael J. Wysocki
2019-08-20 22:47                 ` Kristian Klausen
2019-08-21  7:30                   ` Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1b181f35-29c3-c6ce-6c42-ae55e890579e@samsung.com \
    --to=m.szyprowski@samsung.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=kai.heng.feng@canonical.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mario.limonciello@dell.com \
    --cc=rajneesh.bhardwaj@linux.intel.com \
    --cc=rjw@rjwysocki.net \
    --cc=rui.zhang@intel.com \
    --subject='Re: [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).