start_kernel(): bug: interrupts were enabled early
in [Kernel]
|
From: Américo Wang on 8 Apr 2010 12:00 On Wed, Apr 07, 2010 at 12:09:17PM -0700, Kevin Hilman wrote: >Linus Torvalds <torvalds(a)linux-foundation.org> writes: > >> On Wed, 31 Mar 2010, H. Peter Anvin wrote: >>> >>> The obvious way to fix this would be to use >>> spin_lock_irqsave..spin_lock_irqrestore in __down_read as well as in the >>> other locations; I don't have a good feel for what the cost of doing so >>> would be, though. On x86 it's fairly expensive simply because the only >>> way to save the state is to push it on the stack, which the compiler >>> doesn't deal well with, but this code isn't used on x86. >> > >[...] > >> So making the slow-path do the spin_[un]lock_irq{save,restore}() versions >> sounds like the right thing. It won't be a performance issue: it _is_ the >> slow-path, and we're already doing the expensive part (the spinlock itself >> and the irq thing). >> >> So ACK on the idea. Who wants to write the trivial patch and test it? > >OK, I'll bite since I was seeing boot-time hangs on ARM (TI OMAP3) due >to this. Patch below. > >Kevin > > >From 7baff4008353bbfd2a2e2a4da22b87bc4efa4194 Mon Sep 17 00:00:00 2001 >From: Kevin Hilman <khilman(a)deeprootsystems.com> >Date: Wed, 7 Apr 2010 11:52:46 -0700 >Subject: [PATCH] rwsem generic spinlock: use IRQ save/restore spinlocks > >rwsems can be used with IRQs disabled, particularily in early boot >before IRQs are enabled. Currently the spin_unlock_irq() usage in the >slow-patch will unconditionally enable interrupts and cause problems >since interrupts are not yet initialized or enabled. > >This patch uses save/restore versions of IRQ spinlocks in the slowpath >to ensure interrupts are not unintentionally disabled. > >Signed-off-by: Kevin Hilman <khilman(a)deeprootsystems.com> This looks reasonable and fine for me. Reviewed-by: WANG Cong <xiyou.wangcong(a)gmail.com> Thanks. >--- > lib/rwsem-spinlock.c | 14 ++++++++------ > 1 files changed, 8 insertions(+), 6 deletions(-) > >diff --git a/lib/rwsem-spinlock.c b/lib/rwsem-spinlock.c >index ccf95bf..ffc9fc7 100644 >--- a/lib/rwsem-spinlock.c >+++ b/lib/rwsem-spinlock.c >@@ -143,13 +143,14 @@ void __sched __down_read(struct rw_semaphore *sem) > { > struct rwsem_waiter waiter; > struct task_struct *tsk; >+ unsigned long flags; > >- spin_lock_irq(&sem->wait_lock); >+ spin_lock_irqsave(&sem->wait_lock, flags); > > if (sem->activity >= 0 && list_empty(&sem->wait_list)) { > /* granted */ > sem->activity++; >- spin_unlock_irq(&sem->wait_lock); >+ spin_unlock_irqrestore(&sem->wait_lock, flags); > goto out; > } > >@@ -164,7 +165,7 @@ void __sched __down_read(struct rw_semaphore *sem) > list_add_tail(&waiter.list, &sem->wait_list); > > /* we don't need to touch the semaphore struct anymore */ >- spin_unlock_irq(&sem->wait_lock); >+ spin_unlock_irqrestore(&sem->wait_lock, flags); > > /* wait to be given the lock */ > for (;;) { >@@ -209,13 +210,14 @@ void __sched __down_write_nested(struct rw_semaphore *sem, int subclass) > { > struct rwsem_waiter waiter; > struct task_struct *tsk; >+ unsigned long flags; > >- spin_lock_irq(&sem->wait_lock); >+ spin_lock_irqsave(&sem->wait_lock, flags); > > if (sem->activity == 0 && list_empty(&sem->wait_list)) { > /* granted */ > sem->activity = -1; >- spin_unlock_irq(&sem->wait_lock); >+ spin_unlock_irqrestore(&sem->wait_lock, flags); > goto out; > } > >@@ -230,7 +232,7 @@ void __sched __down_write_nested(struct rw_semaphore *sem, int subclass) > list_add_tail(&waiter.list, &sem->wait_list); > > /* we don't need to touch the semaphore struct anymore */ >- spin_unlock_irq(&sem->wait_lock); >+ spin_unlock_irqrestore(&sem->wait_lock, flags); > > /* wait to be given the lock */ > for (;;) { >-- >1.7.0.2 > >-- >To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >the body of a message to majordomo(a)vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html >Please read the FAQ at http://www.tux.org/lkml/ -- Live like a child, think like the god. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rafael J. Wysocki on 8 Apr 2010 16:10
On Thursday 08 April 2010, H. Peter Anvin wrote: > On 04/07/2010 02:13 PM, Rafael J. Wysocki wrote: > > This message has been generated automatically as a part of a summary report > > of recent regressions. > > > > The following bug entry is on the current list of known regressions > > from 2.6.33. Please verify if it still should be listed and let the tracking team > > know (either way). > > > > > > Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=15668 > > Subject : start_kernel(): bug: interrupts were enabled early > > Submitter : Rabin Vincent <rabin(a)rab.in> > > Date : 2010-03-25 19:53 (14 days old) > > First-Bad-Commit: http://git.kernel.org/git/linus/773e3eb7b81e5ba13b5155dfb3bb75b8ce37f8f9 > ^^^^ > > FWIW, this /git is redundant. Ah, thanks for the hint. :-) Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |