Re: [PATCH] CPUidle: always return with interrupts enabled

[Kevin Hilman <khilman@deeprootsystems.com>]

On Tue, Oct 6, 2009 at 1:34 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 30 Sep 2009 23:21:33 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
>> On Wednesday 30 September 2009, Kevin Hilman wrote:
>> > In the case where cpuidle_idle_call() returns before changing state
>> > due to a need_resched(), it was returning with IRQs disabled.
>> >
>> > This patch ensures IRQs are (re)enabled before returning.
>>
>> Venki, any comments on this?
>
> Rigor mortis is setting in on this one.
>
> Venki's most recent linux-acpi email was on July 31.
>
>> > Reported-by: Hemanth V <hemanthv@ti.com>
>> > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> > ---
>> >  drivers/cpuidle/cpuidle.c |    5 ++++-
>> >  1 files changed, 4 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> > index ad41f19..12fdd39 100644
>> > --- a/drivers/cpuidle/cpuidle.c
>> > +++ b/drivers/cpuidle/cpuidle.c
>> > @@ -76,8 +76,11 @@ static void cpuidle_idle_call(void)
>> >  #endif
>> >     /* ask the governor for the next state */
>> >     next_state = cpuidle_curr_governor->select(dev);
>> > -   if (need_resched())
>> > +   if (need_resched()) {
>> > +           local_irq_enable();
>> >             return;
>> > +   }
>> > +
>> >     target_state = &dev->states[next_state];
>> >
>> >     /* enter the state and update stats */
>
> The patch seems correct to me.  The code is hopelessly poorly
> documented as per usual, but other paths in that function, including
> the call to target_state->enter() (which devolves to default_idle) also
> enable interrupts.
>
> Kevin, the changelog is not good.  It tells us what was wrong with the
> code but does not describe the user-visible effects of the bug.

The idle path assumes that the platform specific idle code returns
with interrupts enabled (although this too is undocumented AFAICT) and
on ARM we have a WARN_ON(!(irqs_disabled()) when returning from the
idle loop, so the user-visible effects were only a warning since
interrupts were eventually re-enabled later.

> I'm unable to find any bug report from Hemanth so I'm all in the dark.
>
> Your cc to linux-omap makes me suspect that <whatever the problem was>
> was exhibited on a non-x86 platform, under some circumstances.  Perhaps
> that explains (for unknown reasons) why <whatever the problem was> was
> not observed on x86.  But I'm totally in the dark and grasping for
> clues and have no way of determining (for example) whether we should
> backport the fix to earlier kernels.
>
> Please send along the additional information?
>

On x86, this same problem exists, but there is no WARN_ON() to detect
it.  As on ARM, the interrupts are eventually re-enabled, so I'm not
sure of any actual bugs triggered by this.  It's primarily a
correctness/consistency fix.

Thanks,

Kevin