Re: [PATCH] parisc: avoid WARNING: at kernel/cpu/idle.c:96

Re: [PATCH] parisc: avoid WARNING: at kernel/cpu/idle.c:96

[James Bottomley]

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();
 }
 
 /*

Re: [PATCH] parisc: avoid WARNING: at kernel/cpu/idle.c:96

[James Bottomley]

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

Re: [PATCH] parisc: avoid WARNING: at kernel/cpu/idle.c:96

[Srivatsa S. Bhat]

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

Re: [PATCH] parisc: avoid WARNING: at kernel/cpu/idle.c:96

[Helge Deller]

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