Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755159AbcKJK22 (ORCPT ); Thu, 10 Nov 2016 05:28:28 -0500 Received: from mail-yw0-f179.google.com ([209.85.161.179]:34486 "EHLO mail-yw0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755033AbcKJK2Z (ORCPT ); Thu, 10 Nov 2016 05:28:25 -0500 MIME-Version: 1.0 In-Reply-To: References: <1478713410-10727-1-git-send-email-sudeep.holla@arm.com> <20161109183925.GA19565@red-moon> From: Vincent Guittot Date: Thu, 10 Nov 2016 11:28:03 +0100 Message-ID: Subject: Re: [PATCH] drivers: cpuidle: assign enter_freeze to same as enter callback function To: Sudeep Holla Cc: Lorenzo Pieralisi , "linux-pm@vger.kernel.org" , "Rafael J . Wysocki" , linux-kernel , Daniel Lezcano , Andy Gross Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4088 Lines: 98 On 9 November 2016 at 19:48, Sudeep Holla wrote: > > > On 09/11/16 18:39, Lorenzo Pieralisi wrote: >> >> 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 >>> --- >>> 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 agree, and I didn't mean that with the above comment. But reading > again, I see your point. > >> 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). >> > > Makes sense, I was just trying to avoid setting for a state like > CPU/Cluster retention but I agree, we need not do that. I agree with Lorenzo and would prefer to keep it simple Regards, Vincent > >> 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. >> > > Yes, that's may be unnecessary addition of some code when we can do it > in simple ways, but I am open to suggestions. > > -- > Regards, > Sudeep