linux-kernel - Re: [PATCH] drivers: cpuidle: assign enter_freeze to same as enter callback function

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: <20161109183925.GA19565@red-moon>
Date:   Wed, 9 Nov 2016 18:39:25 +0000
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Sudeep Holla <sudeep.holla@....com>
Cc:     linux-pm@...r.kernel.org, "Rafael J . Wysocki" <rjw@...ysocki.net>,
        linux-kernel@...r.kernel.org,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Andy Gross <andy.gross@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH] drivers: cpuidle: assign enter_freeze to same as enter
 callback function

On Wed, Nov 09, 2016 at 05:43:30PM +0000, Sudeep Holla wrote:
> enter_freeze() callback is expected atleast to do the same as enter()
> but it has to guarantee that interrupts aren't enabled at any point
> in its execution, as the tick is frozen.
> 
> CPUs execute ->enter_freeze with the local tick or entire timekeeping
> suspended, so it must not re-enable interrupts at any point (even
> temporarily) or attempt to change states of clock event devices.
> 
> It will be called when the system goes to suspend-to-idle and will
> reduce power usage because CPUs won't be awaken for unnecessary IRQs
> (i.e. woken up only on IRQs from "wakeup sources")
> 
> Since for all the states that have CPUIDLE_FLAG_TIMER_STOP flag set,
> local tick is stopped, we can reuse the same code for both the enter()
> and enter_freeze() callbacks. Only "coupled" cpuidle mechanism enables
> interrupts and doing that with timekeeping suspended is generally not
> safe. Since this generic DT based idle driver doesn't support "coupled"
> states, it is safe to assume that the interrupts are not re-enabled.
> 
> This patch assign enter_freeze to same as enter callback function which
> helps to save power without any intermittent spurious wakeups from
> suspend-to-idle.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> ---
>  drivers/cpuidle/dt_idle_states.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> index a5c111b67f37..5a087d108475 100644
> --- a/drivers/cpuidle/dt_idle_states.c
> +++ b/drivers/cpuidle/dt_idle_states.c
> @@ -79,8 +79,17 @@ static int init_state_node(struct cpuidle_state *idle_state,
>  		desc = state_node->name;
> 
>  	idle_state->flags = 0;
> -	if (of_property_read_bool(state_node, "local-timer-stop"))
> +	if (of_property_read_bool(state_node, "local-timer-stop")) {
>  		idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> +		/*
> +		 * CPUIDLE_FLAG_TIMER_STOP guarantees that the local tick is
> +		 * stopped and since this is not a "coupled" state interrupts
> +		 * won't be enabled when it exits allowing the tick to be
> +		 * frozen safely. So enter() can be also enter_freeze()
> +		 * callback.
> +		 */

I do not think that represents a guarantee for enter_freeze() to be
functional, we can initialize enter_freeze() with a function that
does _not_ enable IRQs while executing, it has not much to do with
the local timer losing HW state.

I would just init the enter_freeze() pointer and be done with that,
adding code to check whether the idle back-end enables IRQs when it
enters idle is a major PITA that really is not worth the hassle and
apart from coupled C-states (which we do not support in DT as you said)
I can't find another example (and on top of that it is not even
something we can solve through DT since it is not a property of the idle
state but more related to its kernel implementation).

If we wanted to do it _properly_ we have to add an arch hook to check
if the given idle state enter function back-end, ie cpu_ops on ARM64 or
or cpuidle_ops on ARM, enables IRQs while executing, I would honestly
avoid it but comments are nonetheless welcome.

Thanks for putting it together,
Lorenzo

> +		idle_state->enter_freeze = match_id->data;
> +	}
>  	/*
>  	 * TODO:
>  	 *	replace with kstrdup and pointer assignment when name
> --
> 2.7.4
>