linux-kernel - Re: [PATCH] PM / sleep: enable suspend-to-idle even without registered suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1549989.BLB0oyvVbJ@vostro.rjw.lan>
Date:	Thu, 18 Aug 2016 13:06:49 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Sudeep Holla <sudeep.holla@....com>
Cc:	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
	Andy Gross <andy.gross@...aro.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Subject: Re: [PATCH] PM / sleep: enable suspend-to-idle even without registered suspend_ops

On Thursday, August 18, 2016 10:19:24 AM Sudeep Holla wrote:
> Suspend-to-idle (aka the "freeze" sleep state) is a system sleep state
> in which all of the processors enter deepest possible idle state and
> wait for interrupts right after suspending all the devices.
> 
> There is no hard requirement for a platform to support and register
> platform specific suspend_ops to enter suspend-to-idle/freeze state.
> Only deeper system sleep states like PM_SUSPEND_STANDBY and
> PM_SUSPEND_MEM rely on such low level support/implementation.
> 
> suspend-to-idle can be entered as along as all the devices can be
> suspended. This patch enables the support for suspend-to-idle even on
> systems that don't have any low level support for deeper system sleep
> states and/or don't register any platform specific suspend_ops.
> 
> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> ---
>  kernel/power/main.c    | 5 +++++
>  kernel/power/suspend.c | 8 +++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> Hi Rafael,
> 
> I am not sure if you like this approach. I found this to be the simplest
> but I may have missed to consider all possible corner cases especially
> for x86 and other platforms. I don't see any such issues/cases with ARM
> systems.
>
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 5ea50b1b7595..0f0fd9184f39 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -651,6 +651,11 @@ static int __init pm_init(void)
>  	if (error)
>  		return error;
>  	pm_print_times_init();
> +	/*
> +	 * freeze state should be supported even without any suspend_ops,
> +	 * calling suspend_set_ops without any ops will setup freeze state
> +	 */
> +	suspend_set_ops(NULL);

Well, this is a core initcall, so suspend_set_ops() invocations from platforms
really should happen after that, so something like this should be sufficient here:

	pm_state[PM_SUSPEND_FREEZE] = pm_labels[relative_states ? PM_SUSPEND_MEM : PM_SUSPEND_FREEZE];

if I'm not mistaken.

>  	return pm_autosleep_init();
>  }
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 0acab9d7f96f..37d64f811ecd 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -138,7 +138,9 @@ void suspend_set_ops(const struct platform_suspend_ops *ops)
> 
>  	lock_system_sleep();
> 
> -	suspend_ops = ops;
> +	WARN_ONCE(ops && suspend_ops, "overriding suspend_ops");
> +	if (ops)
> +		suspend_ops = ops;

And this should not be necessary then.

>  	for (i = PM_SUSPEND_MEM; i >= PM_SUSPEND_STANDBY; i--)
>  		if (valid_state(i)) {
>  			pm_states[i] = pm_labels[j++];
> @@ -211,7 +213,7 @@ static int platform_suspend_begin(suspend_state_t state)
>  {
>  	if (state == PM_SUSPEND_FREEZE && freeze_ops && freeze_ops->begin)
>  		return freeze_ops->begin();
> -	else if (suspend_ops->begin)
> +	else if (suspend_ops && suspend_ops->begin)
>  		return suspend_ops->begin(state);
>  	else
>  		return 0;
> @@ -221,7 +223,7 @@ static void platform_resume_end(suspend_state_t state)
>  {
>  	if (state == PM_SUSPEND_FREEZE && freeze_ops && freeze_ops->end)
>  		freeze_ops->end();
> -	else if (suspend_ops->end)
> +	else if (suspend_ops && suspend_ops->end)
>  		suspend_ops->end();
>  }

And this is still needed, of course.

Thanks,
Rafael