[IrDA+SMP] Lockup in handle_IRQ_event

[IrDA+SMP] Lockup in handle_IRQ_event

[Marc ZYNGIER]

Hi all,

Having just started playing with IrDA on my dual celeron (Abit "APIC
error..." BP6), I managed to kill it every single time (NMI watchdog
in handle_IRQ_event) while connecting to my mobile phone (in fact,
when closing the connection to the phone. even 'cat /dev/ircomm0' will
do...). This is perfectly repeatable.

This is with 2.4.0-prerelease (with prerelease-diff as of Wed Jan  3
15:31 UTC 2001), but also crashes with test13-pre7. Didn't test
previous kernel yet, but will do if asked.

hina:~$ ksymoops irda_oops-1
ksymoops 2.3.5 on i686 2.4.0-prerelease.  Options used
     -V (default)
     -k /proc/ksyms (default)
     -l /proc/modules (default)
     -o /lib/modules/2.4.0-prerelease/ (default)
     -m /usr/src/linux/System.map (default)

Warning: You did not tell me where to find symbol information.  I will
assume that the log matches the kernel and modules that are running
right now and I'll use the default options above for symbol resolution.
If the current kernel and/or modules do not match the log, you can get
more accurate output by telling me the kernel version and where to find
map, modules, ksyms etc.  ksymoops -h explains the options.

activating NMI Watchdog ... done.
cpu: 0, clocks: 1002414, slice: 334138
cpu: 1, clocks: 1002414, slice: 334138
NMI Watchdog detected LOCKUP on CPU0, registers:
CPU:    0
EIP:    0010:[<c010a5e5>]
Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00000002
eax: 00000001   ebx: c15bc1e0   ecx: 00000000   edx: 00000000
esi: c02d1860   edi: 00000003   ebp: 00000003   esp: cd9e9d10
ds: 0018   es: 0018   ss: 0018
Process gsmctl (pid: 271, stackpage=cd9e9000)
Stack: c02eed00 c02d1860 00000003 cd9e9d48 c010a7f6 00000003 cd9e9d50 c15bc1e0 
       cdbe2da0 cdbe2da0 00000000 00000000 00000000 c15bc1e0 00000000 c0108fbc 
       cdbe2da0 00000000 cfab8d20 cdbe2da0 00000000 00000000 cfab8d2c cfab0018 
Call Trace: [<c010a7f6>] [<c0108fbc>] [<d0836181>] [<d082f84e>] [<d0837299>] [<d0878d58>] [<d0878767>] 
       [<d08787c0>] [<d0878467>] [<d0838438>] [<d087d2eb>] [<d087d36c>] [<d087c92a>] [<d087bfac>] [<d087b8fa>] 
       [<c017e960>] [<c017efc9>] [<c0132af1>] [<c0131aa2>] [<c01184fb>] [<c0118cd3>] [<c0118e4a>] [<c0108efb>] 
Code: a1 44 cd 2e c0 a8 01 75 f7 be 01 00 00 00 f6 43 07 20 75 01 

>>EIP; c010a5e5 <handle_IRQ_event+21/78>   <=====
Trace; c010a7f6 <do_IRQ+a6/f4>
Trace; c0108fbc <ret_from_intr+0/20>
Trace; d0836181 <[irda]hashbin_remove+121/13c>
Trace; d082f84e <[irda]irlmp_disconnect_request+66/98>
Trace; d0837299 <[irda]irttp_disconnect_request+f1/f8>
Trace; d0878d58 <[ircomm]ircomm_ttp_disconnect_request+14/18>
Trace; d0878767 <[ircomm]ircomm_state_conn+83/b8>
Trace; d08787c0 <[ircomm]ircomm_do_event+24/2c>
Trace; d0878467 <[ircomm]ircomm_disconnect_request+17/20>
Trace; d0838438 <[irda]__irias_delete_attrib+0/30>
Trace; d087d2eb <[ircomm-tty]ircomm_tty_state_ready+47/a4>
Trace; d087d36c <[ircomm-tty]ircomm_tty_do_event+24/2c>
Trace; d087c92a <[ircomm-tty]ircomm_tty_detach_cable+72/a8>
Trace; d087bfac <[ircomm-tty]ircomm_tty_shutdown+88/b8>
Trace; d087b8fa <[ircomm-tty]ircomm_tty_close+c2/16c>
Trace; c017e960 <release_dev+244/514>
Trace; c017efc9 <tty_release+2d/68>
Trace; c0132af1 <fput+39/e8>
Trace; c0131aa2 <filp_close+b2/bc>
Trace; c01184fb <put_files_struct+4f/b8>
Trace; c0118cd3 <do_exit+127/274>
Trace; c0118e4a <sys_exit+e/10>
Trace; c0108efb <system_call+33/38>
Code;  c010a5e5 <handle_IRQ_event+21/78>
00000000 <_EIP>:
Code;  c010a5e5 <handle_IRQ_event+21/78>   <=====
   0:   a1 44 cd 2e c0            mov    0xc02ecd44,%eax   <=====
Code;  c010a5ea <handle_IRQ_event+26/78>
   5:   a8 01                     test   $0x1,%al
Code;  c010a5ec <handle_IRQ_event+28/78>
   7:   75 f7                     jne    0 <_EIP>
Code;  c010a5ee <handle_IRQ_event+2a/78>
   9:   be 01 00 00 00            mov    $0x1,%esi
Code;  c010a5f3 <handle_IRQ_event+2f/78>
   e:   f6 43 07 20               testb  $0x20,0x7(%ebx)
Code;  c010a5f7 <handle_IRQ_event+33/78>
  12:   75 01                     jne    15 <_EIP+0x15> c010a5fa <handle_IRQ_event+36/78>

activating NMI Watchdog ... done.
cpu: 0, clocks: 1002374, slice: 334124
cpu: 1, clocks: 1002374, slice: 334124

1 warning issued.  Results may not be reliable.
hina:~$ grep c02ecd44 /usr/src/linux/System.map 
c02ecd44 B global_irq_lock

Irk... CPU1 stuck in an interrupt, or global_irq_lock not released ?
Any idea anyone ? .config upon request.

	M.

PS: if you reply to this mail from the IrDA ML, please CC to me as I'm
only subscribed to linux-kernel...
-- 
Places change, faces change. Life is so very strange.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [IrDA+SMP] Lockup in handle_IRQ_event

[Andrew Morton]

Marc ZYNGIER wrote:
> 
> Hi all,
> 
> Having just started playing with IrDA on my dual celeron (Abit "APIC
> error..." BP6), I managed to kill it every single time (NMI watchdog
> in handle_IRQ_event) while connecting to my mobile phone (in fact,
> when closing the connection to the phone. even 'cat /dev/ircomm0' will
> do...). This is perfectly repeatable.
> 

Try this:

--- linux-2.4.0-prerelease/net/irda/irqueue.c	Tue Nov 21 20:11:22 2000
+++ linux-akpm/net/irda/irqueue.c	Thu Jan  4 10:14:10 2001
@@ -436,7 +436,7 @@
 
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL) {
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
 	} else if ( hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [IrDA+SMP] Lockup in handle_IRQ_event

[Marc ZYNGIER]

>>>>> "AM" == Andrew Morton <andrewm@uow.edu.au> writes:

Hi Andrew,

AM> Try this:

AM> --- linux-2.4.0-prerelease/net/irda/irqueue.c	Tue Nov 21 20:11:22 2000
AM> +++ linux-akpm/net/irda/irqueue.c	Thu Jan  4 10:14:10 2001
AM> @@ -436,7 +436,7 @@
 
AM>  	/* Release lock */
AM>  	if ( hashbin->hb_type & HB_GLOBAL) {
AM> -		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
AM> +		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
AM>  	} else if ( hashbin->hb_type & HB_LOCAL) {
AM>  		restore_flags( flags);


Ok, that wasn't enough, but you pointed me to the right
direction. With your patch, the machine survived a few more seconds,
that crashed with a slightly different path :

>>EIP; c010a5e5 <handle_IRQ_event+21/78>   <=====
Trace; c010a7f6 <do_IRQ+a6/f4>
Trace; c0108fbc <ret_from_intr+0/20>
Trace; d0830018 <[irda]irlmp_unregister_client+20/54>
Trace; d0835f01 <[irda]hashbin_insert+bd/c4>
Trace; d082f863 <[irda]irlmp_disconnect_request+7b/98>
Trace; d0837299 <[irda]irttp_disconnect_request+f1/f8>
Trace; d0878d58 <[ircomm]ircomm_ttp_disconnect_request+14/18>
Trace; d0878767 <[ircomm]ircomm_state_conn+83/b8>
Trace; d08787c0 <[ircomm]ircomm_do_event+24/2c>
Trace; d0878467 <[ircomm]ircomm_disconnect_request+17/20>
Trace; d0838438 <[irda]__irias_delete_attrib+0/30>
Trace; d087d2eb <[ircomm-tty]ircomm_tty_state_ready+47/a4>
Trace; d087d36c <[ircomm-tty]ircomm_tty_do_event+24/2c>
Trace; d087c92a <[ircomm-tty]ircomm_tty_detach_cable+72/a8>
Trace; d087bfac <[ircomm-tty]ircomm_tty_shutdown+88/b8>
Trace; d087b8fa <[ircomm-tty]ircomm_tty_close+c2/16c>
Trace; c017e960 <release_dev+244/514>
Trace; c017efc9 <tty_release+2d/68>
Trace; c0132af1 <fput+39/e8>
Trace; c0131aa2 <filp_close+b2/bc>
Trace; c01184fb <put_files_struct+4f/b8>
Trace; c0118cd3 <do_exit+127/274>
Trace; c0118e4a <sys_exit+e/10>
Trace; c0108efb <system_call+33/38>
Code;  c010a5e5 <handle_IRQ_event+21/78>

hashbin_insert was guilty this time. So, using your patch as a
guideline, I changed all spin_lock_irqsave/spin_unlock_irq pairs to
spin_lock_irqsave/spin_unlock_irqrestore. Which seems to make sense
anyway !

Using this patch, the machine is solid, and I've been able to play
with my phone as much as I wanted to (well... while the battery
lasted, anyway... ;-).

Here's the patch (which of course includes yours). If it's proved to
be correct, it would be a good idea to summit it to Linus while
prerelease is still open to fixes.

Thanks a lot for your help.

	M.

--- linux/net/irda/irqueue.c.prerelease	Thu Jan  4 10:21:13 2001
+++ linux/net/irda/irqueue.c	Thu Jan  4 10:24:39 2001
@@ -198,7 +198,7 @@
 	
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL)
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 	else if (hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);
 	}
@@ -258,7 +258,7 @@
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL) {
 
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
 	} else if ( hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);
@@ -327,7 +327,7 @@
 	
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL) {
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
 	} else if ( hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);
@@ -436,7 +436,7 @@
 
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL) {
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
 	} else if ( hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);
@@ -511,7 +511,7 @@
 
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL) {
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
 	} else if ( hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);

-- 
Places change, faces change. Life is so very strange.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [IrDA+SMP] Lockup in handle_IRQ_event

[Andrew Morton]

Marc ZYNGIER wrote:
> 
> Using this patch, the machine is solid, and I've been able to play
> with my phone as much as I wanted to (well... while the battery
> lasted, anyway... ;-).
> 
> Here's the patch (which of course includes yours). If it's proved to
> be correct, it would be a good idea to summit it to Linus while
> prerelease is still open to fixes.
> 

Looks fine, Marc.

I note that hashbin_unlock() does a restore_flags() where
the flags were passed in as an argument.  This doesn't
work on SPARC.

Conveniently, hashbin_unlock() is not actually used anywhere.

So I suggest you kill that function altogether, test
it, slap your name on it and send it to da man :)

-
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

[PATCH] [IrDA+SMP] Lockup in handle_IRQ_event

[Marc ZYNGIER]

Hi Linus,

Please find enclosed a patch against 2.4.0-prerelease that :

- Fixes a lockup in handle_IRQ_event when using IrDA on an SMP
machine. It changes spin_lock_irqsave/spin_unlock_irq pairs to
spin_lock_irqsave/spin_unlock_irqrestore, which seems to be the
logical thing to do.

- Removes 2 unused functions that are broken on SPARC (hashbin_unlock
does a restore_flags when save_flags is done in hashbin_lock, which
messes register windows).

With this patch, IrDA is rock solid on my x86 SMP box (at least with
my usage patern...). It was crashing immediatly before (NMI watchdog).

A previous version of this patch has been reviewed by Andrew Morton,
who said it looked ok.

Thanks for applying it to your tree.

	M.

--- linux/net/irda/irqueue.c.prerelease	Thu Jan  4 10:21:13 2001
+++ linux/net/irda/irqueue.c	Thu Jan  4 14:30:35 2001
@@ -8,6 +8,8 @@
  * Created at:    Tue Jun  9 13:29:31 1998
  * Modified at:   Sun Dec 12 13:48:22 1999
  * Modified by:   Dag Brattli <dagb@cs.uit.no>
+ * Modified at:   Thu Jan  4 14:29:10 CET 2001
+ * Modified by:   Marc Zyngier <mzyngier@freesurf.fr>
  * 
  *     Copyright (C) 1998-1999, Aage Kvalnes <aage@cs.uit.no>
  *     Copyright (C) 1998, Dag Brattli, 
@@ -142,69 +144,6 @@
 }
 
 /*
- * Function hashbin_lock (hashbin, hashv, name)
- *
- *    Lock the hashbin
- *
- */
-void hashbin_lock(hashbin_t* hashbin, __u32 hashv, char* name, 
-		  unsigned long flags)
-{
-	int bin;
-	
-	IRDA_DEBUG(0, "hashbin_lock\n");
-
-	ASSERT(hashbin != NULL, return;);
-	ASSERT(hashbin->magic == HB_MAGIC, return;);
-
-	/*
-	 * Locate hashbin
-	 */
-	if (name)
-		hashv = hash(name);
-	bin = GET_HASHBIN(hashv);
-	
-	/* Synchronize */
-	if ( hashbin->hb_type & HB_GLOBAL )
-		spin_lock_irqsave(&hashbin->hb_mutex[ bin], flags);
-	else {
-		save_flags(flags);
-		cli();
-	}
-}
-
-/*
- * Function hashbin_unlock (hashbin, hashv, name)
- *
- *    Unlock the hashbin
- *
- */
-void hashbin_unlock(hashbin_t* hashbin, __u32 hashv, char* name, 
-		    unsigned long flags)
-{
-	int bin;
-
-	IRDA_DEBUG(0, "hashbin_unlock()\n");
-
-	ASSERT(hashbin != NULL, return;);
-	ASSERT(hashbin->magic == HB_MAGIC, return;);
-	
-	/*
-	 * Locate hashbin
-	 */
-	if (name )
-		hashv = hash(name);
-	bin = GET_HASHBIN(hashv);
-	
-	/* Release lock */
-	if ( hashbin->hb_type & HB_GLOBAL)
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
-	else if (hashbin->hb_type & HB_LOCAL) {
-		restore_flags( flags);
-	}
-}
-
-/*
  * Function hashbin_insert (hashbin, entry, name)
  *
  *    Insert an entry into the hashbin
@@ -258,7 +197,7 @@
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL) {
 
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
 	} else if ( hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);
@@ -327,7 +266,7 @@
 	
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL) {
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
 	} else if ( hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);
@@ -436,7 +375,7 @@
 
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL) {
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
 	} else if ( hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);
@@ -511,7 +450,7 @@
 
 	/* Release lock */
 	if ( hashbin->hb_type & HB_GLOBAL) {
-		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
+		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
 
 	} else if ( hashbin->hb_type & HB_LOCAL) {
 		restore_flags( flags);

-- 
Places change, faces change. Life is so very strange.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [Linux-IrDA]Re: [IrDA+SMP] Lockup in handle_IRQ_event

[Florian Lohoff]

On Thu, Jan 04, 2001 at 10:18:46AM +1100, Andrew Morton wrote:
> Marc ZYNGIER wrote:
> > 
> > Hi all,
> > 
> > Having just started playing with IrDA on my dual celeron (Abit "APIC
> > error..." BP6), I managed to kill it every single time (NMI watchdog
> > in handle_IRQ_event) while connecting to my mobile phone (in fact,
> > when closing the connection to the phone. even 'cat /dev/ircomm0' will
> > do...). This is perfectly repeatable.
> > 
> 
> Try this:
> 
> --- linux-2.4.0-prerelease/net/irda/irqueue.c	Tue Nov 21 20:11:22 2000
> +++ linux-akpm/net/irda/irqueue.c	Thu Jan  4 10:14:10 2001
> @@ -436,7 +436,7 @@
>  
>  	/* Release lock */
>  	if ( hashbin->hb_type & HB_GLOBAL) {
> -		spin_unlock_irq( &hashbin->hb_mutex[ bin]);
> +		spin_unlock_irqrestore( &hashbin->hb_mutex[ bin], flags);
>  
>  	} else if ( hashbin->hb_type & HB_LOCAL) {
>  		restore_flags( flags);

BTW: What i have seen in the ircomm_tty.c (2.2.18):

    647     save_flags(flags);
    648     cli();
    649 
    650     skb = self->tx_skb;
    651     self->tx_skb = NULL;
    652 
    653     restore_flags(flags);

and a lot of other places simply use "save_flags(flags); cli();
restore_flags()". Can someone enlighten me how this is supposed to work
on SMP machines ? AFAIK "cli()" only disables IRQs on the local
CPU so a different CPU could easily stumple half way as this
is definitly non atomic. Or is the tty layer protected by some
"big tty lock" ?

Flo
-- 
Florian Lohoff                  flo@rfc822.org             +49-5201-669912
     Why is it called "common sense" when nobody seems to have any?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [Linux-IrDA]Re: [IrDA+SMP] Lockup in handle_IRQ_event

[Anton Blanchard]

 
> BTW: What i have seen in the ircomm_tty.c (2.2.18):
> 
>     647     save_flags(flags);
>     648     cli();
>     649 
>     650     skb = self->tx_skb;
>     651     self->tx_skb = NULL;
>     652 
>     653     restore_flags(flags);
> 
> and a lot of other places simply use "save_flags(flags); cli();
> restore_flags()". Can someone enlighten me how this is supposed to work
> on SMP machines ? AFAIK "cli()" only disables IRQs on the local
> CPU so a different CPU could easily stumple half way as this
> is definitly non atomic. Or is the tty layer protected by some
> "big tty lock" ?

On SMP __cli() disables interrupts on the local cpu, cli() also grabs
the global irq lock which serialises irqs between cpus. 

Using a spinlock is much preferred if possible as grabbing the global
irq lock is expensive (and in most cases unnecessary).

Anton
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [Linux-IrDA]Re: [IrDA+SMP] Lockup in handle_IRQ_event

[Keith Owens]

On Thu, 4 Jan 2001 11:51:59 +0100, 
Florian Lohoff <flo@rfc822.org> wrote:
>and a lot of other places simply use "save_flags(flags); cli();
>restore_flags()". Can someone enlighten me how this is supposed to work
>on SMP machines ? AFAIK "cli()" only disables IRQs on the local
>CPU so a different CPU could easily stumple half way as this
>is definitly non atomic.

cli() is not the cli instruction anymore.  On smp cli() maps to
__global_cli() which is responsible for disabling interrupts on the
other processors.  See __global_cli in arch/*/kernel/irq.c.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/