* [PATCH] MIPS: Protect current_cpu_data with preempt disable in delay()
@ 2010-03-04 9:39 Yang Shi
2010-03-04 18:23 ` David Daney
0 siblings, 1 reply; 6+ messages in thread
From: Yang Shi @ 2010-03-04 9:39 UTC (permalink / raw)
To: ralf; +Cc: linux-mips
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);
}
--
1.6.3.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] MIPS: Protect current_cpu_data with preempt disable in delay()
2010-03-04 9:39 [PATCH] MIPS: Protect current_cpu_data with preempt disable in delay() Yang Shi
@ 2010-03-04 18:23 ` David Daney
2010-03-05 1:10 ` Yang Shi
2010-03-10 15:33 ` Ralf Baechle
0 siblings, 2 replies; 6+ messages in thread
From: David Daney @ 2010-03-04 18:23 UTC (permalink / raw)
To: Yang Shi, ralf; +Cc: linux-mips
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().
David Daney
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] MIPS: Protect current_cpu_data with preempt disable in delay()
2010-03-04 18:23 ` David Daney
@ 2010-03-05 1:10 ` Yang Shi
2010-03-08 9:42 ` Yang Shi
2010-03-10 15:33 ` Ralf Baechle
1 sibling, 1 reply; 6+ messages in thread
From: Yang Shi @ 2010-03-05 1:10 UTC (permalink / raw)
To: David Daney; +Cc: ralf, linux-mips
David Daney 写道:
> 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().
>
Thanks David.
Yes, actually I also has this concern, so this patch is just a rough
fix. And I tried raw_current_cpu_data as well, but I'm not sure if it's
safe or not. Another proposal is to change cpu_data and current_cpu_data
to per CPU variables, of course this is a big change.
Regards,
Yang
> David Daney
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] MIPS: Protect current_cpu_data with preempt disable in delay()
2010-03-05 1:10 ` Yang Shi
@ 2010-03-08 9:42 ` Yang Shi
0 siblings, 0 replies; 6+ messages in thread
From: Yang Shi @ 2010-03-08 9:42 UTC (permalink / raw)
To: Yang Shi; +Cc: David Daney, ralf, linux-mips
Hi folks,
Any advice on this topic?
Thanks,
Yang
Yang Shi 写道:
> David Daney 写道:
>
>> 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().
>>
>>
>
> Thanks David.
>
> Yes, actually I also has this concern, so this patch is just a rough
> fix. And I tried raw_current_cpu_data as well, but I'm not sure if it's
> safe or not. Another proposal is to change cpu_data and current_cpu_data
> to per CPU variables, of course this is a big change.
>
> Regards,
> Yang
>
>
>> David Daney
>>
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS: Protect current_cpu_data with preempt disable in delay()
2010-03-04 18:23 ` David Daney
2010-03-05 1:10 ` Yang Shi
@ 2010-03-10 15:33 ` Ralf Baechle
2010-03-11 3:12 ` Yang Shi
1 sibling, 1 reply; 6+ messages in thread
From: Ralf Baechle @ 2010-03-10 15:33 UTC (permalink / raw)
To: David Daney; +Cc: Yang Shi, linux-mips
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.
Ralf
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] MIPS: Protect current_cpu_data with preempt disable in delay()
2010-03-10 15:33 ` Ralf Baechle
@ 2010-03-11 3:12 ` Yang Shi
0 siblings, 0 replies; 6+ messages in thread
From: Yang Shi @ 2010-03-11 3:12 UTC (permalink / raw)
To: Ralf Baechle; +Cc: David Daney, linux-mips
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-03-11 3:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-04 9:39 [PATCH] MIPS: Protect current_cpu_data with preempt disable in delay() Yang Shi
2010-03-04 18:23 ` David Daney
2010-03-05 1:10 ` Yang Shi
2010-03-08 9:42 ` Yang Shi
2010-03-10 15:33 ` Ralf Baechle
2010-03-11 3:12 ` Yang Shi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.