* [PATCH] CPUidle: always return with interrupts enabled
@ 2009-09-30 17:35 Kevin Hilman
2009-09-30 21:21 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Hilman @ 2009-09-30 17:35 UTC (permalink / raw
To: linux-pm; +Cc: linux-kernel, linux-omap
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.
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 */
--
1.6.4.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] CPUidle: always return with interrupts enabled
2009-09-30 17:35 [PATCH] CPUidle: always return with interrupts enabled Kevin Hilman
@ 2009-09-30 21:21 ` Rafael J. Wysocki
2009-10-06 20:34 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2009-09-30 21:21 UTC (permalink / raw
To: Kevin Hilman, Venki Pallipadi
Cc: linux-pm, linux-kernel, linux-omap, Len Brown,
ACPI Devel Maling List
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?
> 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 */
Best,
Rafael
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CPUidle: always return with interrupts enabled
2009-09-30 21:21 ` Rafael J. Wysocki
@ 2009-10-06 20:34 ` Andrew Morton
2009-10-06 21:02 ` Kevin Hilman
2009-10-16 16:25 ` Martin Michlmayr
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2009-10-06 20:34 UTC (permalink / raw
To: Rafael J. Wysocki
Cc: khilman, venkatesh.pallipadi, linux-pm, linux-kernel, linux-omap,
lenb, linux-acpi
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.
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CPUidle: always return with interrupts enabled
2009-10-06 20:34 ` Andrew Morton
@ 2009-10-06 21:02 ` Kevin Hilman
2009-10-16 16:25 ` Martin Michlmayr
1 sibling, 0 replies; 6+ messages in thread
From: Kevin Hilman @ 2009-10-06 21:02 UTC (permalink / raw
To: Andrew Morton
Cc: Rafael J. Wysocki, venkatesh.pallipadi, linux-pm, linux-kernel,
linux-omap, lenb, linux-acpi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CPUidle: always return with interrupts enabled
2009-10-06 20:34 ` Andrew Morton
2009-10-06 21:02 ` Kevin Hilman
@ 2009-10-16 16:25 ` Martin Michlmayr
2009-10-16 21:11 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Martin Michlmayr @ 2009-10-16 16:25 UTC (permalink / raw
To: Andrew Morton
Cc: Rafael J. Wysocki, khilman, venkatesh.pallipadi, linux-pm,
linux-kernel, linux-omap, lenb, linux-acpi
* Andrew Morton <akpm@linux-foundation.org> [2009-10-06 13:34]:
> Rigor mortis is setting in on this one.
> The patch seems correct to me.
Can someone put this patch in now? The problem has also been reported
on Marvell's Kirkwood platform (ARM) by a number of users and the
patch fixes it.
Tested-by: Martin Michlmayr <tbm@cyrius.com>
Please CC stable when you commit the patch since it also needs to go
in for 2.6.31.
Reference with the patch: http://patchwork.kernel.org/patch/50728/
--
Martin Michlmayr
http://www.cyrius.com/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CPUidle: always return with interrupts enabled
2009-10-16 16:25 ` Martin Michlmayr
@ 2009-10-16 21:11 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2009-10-16 21:11 UTC (permalink / raw
To: Martin Michlmayr
Cc: Rafael J. Wysocki, khilman, venkatesh.pallipadi, linux-pm,
linux-kernel, linux-omap, lenb, linux-acpi
On Fri, 16 Oct 2009 17:25:04 +0100
Martin Michlmayr <tbm@cyrius.com> wrote:
> * Andrew Morton <akpm@linux-foundation.org> [2009-10-06 13:34]:
> > Rigor mortis is setting in on this one.
>
> > The patch seems correct to me.
>
> Can someone put this patch in now? The problem has also been reported
> on Marvell's Kirkwood platform (ARM) by a number of users and the
> patch fixes it.
>
> Tested-by: Martin Michlmayr <tbm@cyrius.com>
I have it in my for-2.6.32 queue.
> Please CC stable when you commit the patch since it also needs to go
> in for 2.6.31.
hm, I didn't know that. So noted, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-10-16 21:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-30 17:35 [PATCH] CPUidle: always return with interrupts enabled Kevin Hilman
2009-09-30 21:21 ` Rafael J. Wysocki
2009-10-06 20:34 ` Andrew Morton
2009-10-06 21:02 ` Kevin Hilman
2009-10-16 16:25 ` Martin Michlmayr
2009-10-16 21:11 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).