| Message ID | 1368047134.8623.5.camel@dabdike (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2013-05-08 at 14:05 -0700, James Bottomley wrote: > On Wed, 2013-05-08 at 22:42 +0200, Helge Deller wrote: > > We need to enable local irqs to avoid this runtime warning. > > > > Signed-off-by: Helge Deller <deller@gmx.de> > > > > diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c > > index 55f92b6..2840d43 100644 > > --- a/arch/parisc/kernel/process.c > > +++ b/arch/parisc/kernel/process.c > > @@ -286,3 +286,8 @@ void *dereference_function_descriptor(void *ptr) > > return ptr; > > } > > #endif > > + > > +void arch_cpu_idle(void) > > +{ > > + local_irq_enable(); > > This is definitely wrong. we'd need at least a cpu_relax() in there. > Without that the cpu will bounce around in the idle loop. I'd advise > that we simply keep the default cpu_idle_poll(). > > However, this looks like a kernel bug to me in cpu_idle_loop()? not > something we should be working around in the arch code. > > Thomas, what's going on here? It looks like you can never avoid the > WARN_ON_ONCE if you don't provide any arch specific idle functions. If > you wish to allow this case then the correct patch (keeping the WARN_ON) > would appear to be this one. Ping on this, please, Thomas ... the patch looks fairly obviously correct, do you just want us to take it through the parisc tree? James > James > > ---- > diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c > index 8b86c0c..829e41f 100644 > --- a/kernel/cpu/idle.c > +++ b/kernel/cpu/idle.c > @@ -56,6 +56,7 @@ void __weak arch_cpu_idle_dead(void) { } > void __weak arch_cpu_idle(void) > { > cpu_idle_force_poll = 1; > + local_irq_enable(); > } > > /* > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-parisc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/09/2013 02:35 AM, James Bottomley wrote: > On Wed, 2013-05-08 at 22:42 +0200, Helge Deller wrote: >> We need to enable local irqs to avoid this runtime warning. >> >> Signed-off-by: Helge Deller <deller@gmx.de> >> >> diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c >> index 55f92b6..2840d43 100644 >> --- a/arch/parisc/kernel/process.c >> +++ b/arch/parisc/kernel/process.c >> @@ -286,3 +286,8 @@ void *dereference_function_descriptor(void *ptr) >> return ptr; >> } >> #endif >> + >> +void arch_cpu_idle(void) >> +{ >> + local_irq_enable(); > > This is definitely wrong. we'd need at least a cpu_relax() in there. > Without that the cpu will bounce around in the idle loop. I'd advise > that we simply keep the default cpu_idle_poll(). > > However, this looks like a kernel bug to me in cpu_idle_loop()? not > something we should be working around in the arch code. > > Thomas, what's going on here? It looks like you can never avoid the > WARN_ON_ONCE if you don't provide any arch specific idle functions. If > you wish to allow this case then the correct patch (keeping the WARN_ON) > would appear to be this one. > > James > > ---- > diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c > index 8b86c0c..829e41f 100644 > --- a/kernel/cpu/idle.c > +++ b/kernel/cpu/idle.c > @@ -56,6 +56,7 @@ void __weak arch_cpu_idle_dead(void) { } > void __weak arch_cpu_idle(void) > { > cpu_idle_force_poll = 1; > + local_irq_enable(); > } > > /* > This patch looks like the correct fix to me. Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/17/2013 09:25 PM, Srivatsa S. Bhat wrote: > On 05/09/2013 02:35 AM, James Bottomley wrote: >> On Wed, 2013-05-08 at 22:42 +0200, Helge Deller wrote: >>> We need to enable local irqs to avoid this runtime warning. >>> >>> Signed-off-by: Helge Deller <deller@gmx.de> >>> >>> diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c >>> index 55f92b6..2840d43 100644 >>> --- a/arch/parisc/kernel/process.c >>> +++ b/arch/parisc/kernel/process.c >>> @@ -286,3 +286,8 @@ void *dereference_function_descriptor(void *ptr) >>> return ptr; >>> } >>> #endif >>> + >>> +void arch_cpu_idle(void) >>> +{ >>> + local_irq_enable(); >> >> This is definitely wrong. we'd need at least a cpu_relax() in there. >> Without that the cpu will bounce around in the idle loop. I'd advise >> that we simply keep the default cpu_idle_poll(). >> >> However, this looks like a kernel bug to me in cpu_idle_loop()? not >> something we should be working around in the arch code. >> >> Thomas, what's going on here? It looks like you can never avoid the >> WARN_ON_ONCE if you don't provide any arch specific idle functions. If >> you wish to allow this case then the correct patch (keeping the WARN_ON) >> would appear to be this one. >> >> James >> >> ---- >> diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c >> index 8b86c0c..829e41f 100644 >> --- a/kernel/cpu/idle.c >> +++ b/kernel/cpu/idle.c >> @@ -56,6 +56,7 @@ void __weak arch_cpu_idle_dead(void) { } >> void __weak arch_cpu_idle(void) >> { >> cpu_idle_force_poll = 1; >> + local_irq_enable(); >> } >> >> /* >> > > This patch looks like the correct fix to me. > > Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> Thanks Srivatsa! I can push it through the parisc tree if nobody objects... Helge -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c index 8b86c0c..829e41f 100644 --- a/kernel/cpu/idle.c +++ b/kernel/cpu/idle.c @@ -56,6 +56,7 @@ void __weak arch_cpu_idle_dead(void) { } void __weak arch_cpu_idle(void) { cpu_idle_force_poll = 1; + local_irq_enable(); } /*