On Wed, Feb 25, 2015 at 11:36:10PM +0000, Rafael J. Wysocki wrote:
> On Wednesday, February 25, 2015 02:39:17 PM Lorenzo Pieralisi wrote:
> > On Wed, Feb 25, 2015 at 02:13:23PM +0000, Daniel Lezcano wrote:
> > > On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> > > > On return from cpuidle_enter_freeze() irqs are re-enabled by the
> > > > function
> > > > caller (ie cpuidle_idle_call) in the idle loop. This patch removes a
> > > > stale
> > > > local_irq_disable() call and its stale comment in
> > > > cpuidle_enter_freeze(),
> > > > since they disagree and do not serve a useful purpose.
> > > >
> > > > Cc: Rafael J. Wysocki <[email protected]>
> > > > Cc: Daniel Lezcano <[email protected]>
> > > > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > > > ---
> > > > drivers/cpuidle/cpuidle.c | 3 ---
> > > > 1 file changed, 3 deletions(-)
> > > >
> > > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > > > index 4d53458..f47edc6c 100644
> > > > --- a/drivers/cpuidle/cpuidle.c
> > > > +++ b/drivers/cpuidle/cpuidle.c
> > > > @@ -144,9 +144,6 @@ void cpuidle_enter_freeze(void)
> > > > cpuidle_enter(drv, dev, index);
> > > > else
> > > > arch_cpu_idle();
> > > > -
> > > > - /* Interrupts are enabled again here. */
> > > > - local_irq_disable();
> > > > }
> > >
> > > Hmm, I think Rafael added this prevent lockdep to raise a warning.
> >
> > Ok, so the comment is there to say "at this point of execution IRQs
> > are enabled", it does not refer to local_irq_disable() call effects,
> > that's misleading and not necessarily nice, at least it should
> > be explained.
> >
> > > Otherwise, cpuidle_enter or arch_cpu_idle enables the irq again and then
> > > when exiting the cpu_idle_call, we enable them again, so leading to a
> > > lockdep WARN in trace_hardirqs_on_caller.
> >
> > Would not it be better to enable irqs in cpuidle_enter_freeze() on
> > returning from enter_freeze_proper() and remove the local_irq_enable()
> > call in the cpuidle_idle_call() before jumping to exit_idle ?
> >
> > > That said, if we have to do this, it may reveal something is wrong in
> > > the code.
> >
> > I just spotted code through inspection, I have to say at the moment it
> > is not very clear what it is meant to achieve, so I put together this
> > patch.
>
> So there are two code paths in cpuidle_idle_call(), the enter_freeze_proper()
> one which does *not* re-enable interrupts and the one you modified which does
> that. The local_irq_disable() is to keep things consistent.
>
> I'm not entirely against of re-arranging things here, but a patch like the
> (untested) one below might be more appropriate.
>
> Rafael (who would appreciate it if people asked questions instead of sending
> patches on a hunch).
I understand that, I wanted to just send [patch 2], this patch was more
a way to get a clarification than anything else, asking would have been more
appropriate, sorry.
Anyway, I did not like disabling IRQs to just re-enable them on function
return, in particular the comment below seemed to apply to the following
line, which is a bit misleading.
/* Interrupts are enabled again here. */
local_irq_disable();
>
>
> ---
> drivers/cpuidle/cpuidle.c | 2 +-
> kernel/sched/idle.c | 1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> Index: linux-pm/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> +++ linux-pm/drivers/cpuidle/cpuidle.c
> @@ -132,6 +132,7 @@ void cpuidle_enter_freeze(void)
> index = cpuidle_find_deepest_state(drv, dev, true);
> if (index >= 0) {
> enter_freeze_proper(drv, dev, index);
> + local_irq_enable();
> return;
> }
>
> @@ -146,7 +147,6 @@ void cpuidle_enter_freeze(void)
> arch_cpu_idle();
>
> /* Interrupts are enabled again here. */
> - local_irq_disable();
> }
>
> /**
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -116,7 +116,6 @@ static void cpuidle_idle_call(void)
> */
> if (idle_should_freeze()) {
> cpuidle_enter_freeze();
> - local_irq_enable();
> goto exit_idle;
> }
>
It looks fine, I will test it. I would add a comment to
cpuidle_enter_freeze() to document it must return with IRQs
enabled.
Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
