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@xxxxxxxxxxxxx>
> >---
> > 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
