Re: [PATCH] MIPS: Protect current_cpu_data with preempt disable in delay()

[Yang Shi <yang.shi@windriver.com>]

Ralf Baechle 写道:
> On Thu, Mar 04, 2010 at 10:23:47AM -0800, David Daney wrote:
>
>   
>> On 03/04/2010 01:39 AM, Yang Shi wrote:
>>     
>>> During machine restart with reboot command, get the following
>>> bug info:
>>>
>>> BUG: using smp_processor_id() in preemptible [00000000] code: reboot/1989
>>> caller is __udelay+0x14/0x70
>>> Call Trace:
>>> [<ffffffff8110ad28>] dump_stack+0x8/0x34
>>> [<ffffffff812dde04>] debug_smp_processor_id+0xf4/0x110
>>> [<ffffffff812d90bc>] __udelay+0x14/0x70
>>> [<ffffffff81378274>] md_notify_reboot+0x12c/0x148
>>> [<ffffffff81161054>] notifier_call_chain+0x64/0xc8
>>> [<ffffffff811614dc>] __blocking_notifier_call_chain+0x64/0xc0
>>> [<ffffffff8115566c>] kernel_restart_prepare+0x1c/0x38
>>> [<ffffffff811556cc>] kernel_restart+0x14/0x50
>>> [<ffffffff8115581c>] SyS_reboot+0x10c/0x1f0
>>> [<ffffffff81103684>] handle_sysn32+0x44/0x84
>>>
>>> The root cause is that current_cpu_data is accessed in preemptible
>>> context, so protect it with preempt_disable/preempt_enable pair
>>> in delay().
>>>
>>> Signed-off-by: Yang Shi<yang.shi@windriver.com>
>>> ---
>>>  arch/mips/lib/delay.c |    6 +++++-
>>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/mips/lib/delay.c b/arch/mips/lib/delay.c
>>> index 6b3b1de..dc38064 100644
>>> --- a/arch/mips/lib/delay.c
>>> +++ b/arch/mips/lib/delay.c
>>> @@ -41,7 +41,11 @@ EXPORT_SYMBOL(__delay);
>>>
>>>  void __udelay(unsigned long us)
>>>  {
>>> -	unsigned int lpj = current_cpu_data.udelay_val;
>>> +	unsigned int lpj;
>>> +
>>> +	preempt_disable();
>>> +	lpj = current_cpu_data.udelay_val;
>>> +	preempt_enable();
>>>
>>>  	__delay((us * 0x000010c7ull * HZ * lpj)>>  32);
>>>  }
>>>       
>> This doesn't seem like the best approach.
>>
>> Perhaps we should either use raw_current_cpu_data and no
>> preempt_disable(), or if we are concerned about migrating to a CPU
>> with a different lpj value, move the preempt_enable after the call
>> to __delay().
>>     
>
> Udelay() is supposed to guarantee a minimum delay and when being migrated
> to another CPU with higher bogomips this guarantee might be violated.  So
> it'd even have to be something like:
>
> void __udelay(unsigned long us)
> {
> 	unsigned int lpj = current_cpu_data.udelay_val;
> 	unsigned int lpj;
>
> 	preempt_disable();
> 	lpj = current_cpu_data.udelay_val;
>
> 	__delay((us * 0x000010c7ull * HZ * lpj)>>  32);
> 	preempt_enable();
> }
>
> But preempt_disable() itself is not atomic, so using it from bh or irq
> context could result in a corrupted preemption counter.  So the raw_
> version will have to do.  I doubt it's much of a problem but at some
> point we will have to revisit the delay by c0_count patch submitted a
> while ago.  The patch wasn't right but the problem it was addressing
> is real.
>   

Thanks Ralf. Do we need raw_ version patch before revisiting the delay 
by c0_count patch although it's not an ideal fix.

Regards,
Yang

>   Ralf
>
>