Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

[Tony Battersby]

> So we should unconditionally drop the lock (and re-enable
> interrupts) and re-acquire it.

After looking at it carefully, this is true of pci_map_mem, but not
pci_unmap_mem.  pci_unmap_mem can be called from both ->detect and
->release.  io_request_lock is held in ->detect but not in ->release.
So, your patch locks up the system on module unload.

I have put together and tested a new patch which does it correctly,
while still trying to make only minimal changes.
If you approve, this can go into the next 2.4.x release.

Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
---
--- linux-2.4.35/drivers/scsi/sym53c8xx_2/sym_glue.c.orig	2007-10-17 10:16:07.000000000 -0400
+++ linux-2.4.35/drivers/scsi/sym53c8xx_2/sym_glue.c	2007-10-17 10:24:42.000000000 -0400
@@ -224,15 +224,26 @@ static u_long __init pci_map_mem(u_long 
 {
 	u_long page_base	= ((u_long) base) & PAGE_MASK;
 	u_long page_offs	= ((u_long) base) - page_base;
-	u_long page_remapped	= (u_long) ioremap(page_base, page_offs+size);
+	u_long page_remapped;
+
+	spin_unlock_irq(&io_request_lock);
+	page_remapped = (u_long) ioremap(page_base, page_offs+size);
+	spin_lock_irq(&io_request_lock);
 
 	return page_remapped? (page_remapped + page_offs) : 0UL;
 }
 
-static void __init pci_unmap_mem(u_long vaddr, u_long size)
-{
-	if (vaddr)
+static void __init pci_unmap_mem(u_long vaddr,
+                                 u_long size,
+                                 int holding_io_request_lock)
+{
+	if (vaddr) {
+		if (holding_io_request_lock)
+			spin_unlock_irq(&io_request_lock);
 		iounmap((void *) (vaddr & PAGE_MASK));
+		if (holding_io_request_lock)
+			spin_lock_irq(&io_request_lock);
+	}
 }
 #endif
 
@@ -1840,7 +1851,7 @@ static int sym53c8xx_proc_info(char *buf
 /*
  *	Free controller resources.
  */
-static void sym_free_resources(hcb_p np)
+static void sym_free_resources(hcb_p np, int holding_io_request_lock)
 {
 	/*
 	 *  Free O/S specific resources.
@@ -1851,9 +1862,13 @@ static void sym_free_resources(hcb_p np)
 		release_region(np->s.io_port, np->s.io_ws);
 #ifndef SYM_OPT_NO_BUS_MEMORY_MAPPING
 	if (np->s.mmio_va)
-		pci_unmap_mem(np->s.mmio_va, np->s.io_ws);
+		pci_unmap_mem(np->s.mmio_va,
+		              np->s.io_ws,
+		              holding_io_request_lock);
 	if (np->s.ram_va)
-		pci_unmap_mem(np->s.ram_va, np->ram_ws);
+		pci_unmap_mem(np->s.ram_va,
+		              np->ram_ws,
+		              holding_io_request_lock);
 #endif
 	/*
 	 *  Free O/S independant resources.
@@ -2155,7 +2170,7 @@ attach_failed:
 	if (!instance) return -1;
 	printf_info("%s: giving up ...\n", sym_name(np));
 	if (np)
-		sym_free_resources(np);
+		sym_free_resources(np, 1);
 	scsi_unregister(instance);
 
         return -1;
@@ -2197,7 +2212,7 @@ static void __init sym_get_nvram(sym_dev
 #ifdef SYM_CONF_IOMAPPED
 	release_region(devp->s.io_port, 128);
 #else
-	pci_unmap_mem((u_long) devp->s.mmio_va, 128ul);
+	pci_unmap_mem((u_long) devp->s.mmio_va, 128ul, 1);
 #endif
 }
 #endif	/* SYM_CONF_NVRAM_SUPPORT */
@@ -2551,7 +2566,7 @@ sym53c8xx_pci_init(Scsi_Host_Template *t
 		ram_ptr = pci_map_mem(base_2_c, ram_size);
 		if (ram_ptr) {
 			ram_val = readl_raw(ram_ptr + ram_size - 16);
-			pci_unmap_mem(ram_ptr, ram_size);
+			pci_unmap_mem(ram_ptr, ram_size, 1);
 			if (ram_val == 0x52414944) {
 				printf_info("%s: not initializing, "
 				            "driven by RAID controller.\n",
@@ -2980,7 +2995,7 @@ static int sym_detach(hcb_p np)
 	/*
 	 *  Free host resources
 	 */
-	sym_free_resources(np);
+	sym_free_resources(np, 0);
 
 	return 1;
 }

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

[Willy Tarreau]

On Wed, Oct 17, 2007 at 10:53:06AM -0400, Tony Battersby wrote:
> > So we should unconditionally drop the lock (and re-enable
> > interrupts) and re-acquire it.
> 
> After looking at it carefully, this is true of pci_map_mem, but not
> pci_unmap_mem.  pci_unmap_mem can be called from both ->detect and
> ->release.  io_request_lock is held in ->detect but not in ->release.
> So, your patch locks up the system on module unload.
> 
> I have put together and tested a new patch which does it correctly,
> while still trying to make only minimal changes.
> If you approve, this can go into the next 2.4.x release.

(...)

> -static void __init pci_unmap_mem(u_long vaddr, u_long size)
> -{
> -	if (vaddr)
> +static void __init pci_unmap_mem(u_long vaddr,
> +                                 u_long size,
> +                                 int holding_io_request_lock)

This is marked __init, and pci_unmap_mem() is called from
sym_free_resources(), which in turn is called from sym_detach(),
called from sym53c8xx_release() when unloading module. So the section
may not be there anymore upon unload. I wonder how this can work right
now. I'm surely missing something :-/

Willy

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

[Tony Battersby]

Willy Tarreau wrote:
> On Wed, Oct 17, 2007 at 10:53:06AM -0400, Tony Battersby wrote:
>   
>>> So we should unconditionally drop the lock (and re-enable
>>> interrupts) and re-acquire it.
>>>       
>> After looking at it carefully, this is true of pci_map_mem, but not
>> pci_unmap_mem.  pci_unmap_mem can be called from both ->detect and
>> ->release.  io_request_lock is held in ->detect but not in ->release.
>> So, your patch locks up the system on module unload.
>>
>> I have put together and tested a new patch which does it correctly,
>> while still trying to make only minimal changes.
>> If you approve, this can go into the next 2.4.x release.
>>     
>
> (...)
>
>   
>> -static void __init pci_unmap_mem(u_long vaddr, u_long size)
>> -{
>> -	if (vaddr)
>> +static void __init pci_unmap_mem(u_long vaddr,
>> +                                 u_long size,
>> +                                 int holding_io_request_lock)
>>     
>
> This is marked __init, and pci_unmap_mem() is called from
> sym_free_resources(), which in turn is called from sym_detach(),
> called from sym53c8xx_release() when unloading module. So the section
> may not be there anymore upon unload. I wonder how this can work right
> now. I'm surely missing something :-/
>
> Willy
>   
In 2.4, include/linux/init.h has the following:

#ifndef MODULE
#define __init          __attribute__ ((__section__ (".text.init")))
#else
#define __init
#endif

So __init has an effect only if it is built-in.  Apparently 2.6 also
discards __init sections in modules after loading, but 2.4 doesn't.  Of
course, it is still a bug.

Do you want me to redo the patch with __init taken out?

Tony

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

[Willy Tarreau]

On Wed, Oct 17, 2007 at 11:44:08AM -0400, Tony Battersby wrote:
> In 2.4, include/linux/init.h has the following:
> 
> #ifndef MODULE
> #define __init          __attribute__ ((__section__ (".text.init")))
> #else
> #define __init
> #endif
> 
> So __init has an effect only if it is built-in.

Ah yes, you're right.

> Apparently 2.6 also
> discards __init sections in modules after loading, but 2.4 doesn't.  Of
> course, it is still a bug.
> 
> Do you want me to redo the patch with __init taken out?

It would be better. As a general rule of thumb, an __init function should
not be called from a non __init, otherwise it is very hard to track the
call path.

Thanks,
Willy

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

[Tony Battersby]

This patch fixes two problems with sym53c8xx_2.o in 2.4.x kernels:

1) A system hang when loading sym53c8xx_2.o on a SMP system with two
dual-channel LSI HBAs (http://bugzilla.kernel.org/show_bug.cgi?id=3680)

2) A function improperly marked __init.

Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
---
--- linux-2.4.35/drivers/scsi/sym53c8xx_2/sym_glue.c.orig	2007-10-17 12:07:37.000000000 -0400
+++ linux-2.4.35/drivers/scsi/sym53c8xx_2/sym_glue.c	2007-10-17 12:08:32.000000000 -0400
@@ -224,15 +224,26 @@ static u_long __init pci_map_mem(u_long 
 {
 	u_long page_base	= ((u_long) base) & PAGE_MASK;
 	u_long page_offs	= ((u_long) base) - page_base;
-	u_long page_remapped	= (u_long) ioremap(page_base, page_offs+size);
+	u_long page_remapped;
+
+	spin_unlock_irq(&io_request_lock);
+	page_remapped = (u_long) ioremap(page_base, page_offs+size);
+	spin_lock_irq(&io_request_lock);
 
 	return page_remapped? (page_remapped + page_offs) : 0UL;
 }
 
-static void __init pci_unmap_mem(u_long vaddr, u_long size)
-{
-	if (vaddr)
+static void pci_unmap_mem(u_long vaddr,
+                          u_long size,
+                          int holding_io_request_lock)
+{
+	if (vaddr) {
+		if (holding_io_request_lock)
+			spin_unlock_irq(&io_request_lock);
 		iounmap((void *) (vaddr & PAGE_MASK));
+		if (holding_io_request_lock)
+			spin_lock_irq(&io_request_lock);
+	}
 }
 #endif
 
@@ -1840,7 +1851,7 @@ static int sym53c8xx_proc_info(char *buf
 /*
  *	Free controller resources.
  */
-static void sym_free_resources(hcb_p np)
+static void sym_free_resources(hcb_p np, int holding_io_request_lock)
 {
 	/*
 	 *  Free O/S specific resources.
@@ -1851,9 +1862,13 @@ static void sym_free_resources(hcb_p np)
 		release_region(np->s.io_port, np->s.io_ws);
 #ifndef SYM_OPT_NO_BUS_MEMORY_MAPPING
 	if (np->s.mmio_va)
-		pci_unmap_mem(np->s.mmio_va, np->s.io_ws);
+		pci_unmap_mem(np->s.mmio_va,
+		              np->s.io_ws,
+		              holding_io_request_lock);
 	if (np->s.ram_va)
-		pci_unmap_mem(np->s.ram_va, np->ram_ws);
+		pci_unmap_mem(np->s.ram_va,
+		              np->ram_ws,
+		              holding_io_request_lock);
 #endif
 	/*
 	 *  Free O/S independant resources.
@@ -2155,7 +2170,7 @@ attach_failed:
 	if (!instance) return -1;
 	printf_info("%s: giving up ...\n", sym_name(np));
 	if (np)
-		sym_free_resources(np);
+		sym_free_resources(np, 1);
 	scsi_unregister(instance);
 
         return -1;
@@ -2197,7 +2212,7 @@ static void __init sym_get_nvram(sym_dev
 #ifdef SYM_CONF_IOMAPPED
 	release_region(devp->s.io_port, 128);
 #else
-	pci_unmap_mem((u_long) devp->s.mmio_va, 128ul);
+	pci_unmap_mem((u_long) devp->s.mmio_va, 128ul, 1);
 #endif
 }
 #endif	/* SYM_CONF_NVRAM_SUPPORT */
@@ -2551,7 +2566,7 @@ sym53c8xx_pci_init(Scsi_Host_Template *t
 		ram_ptr = pci_map_mem(base_2_c, ram_size);
 		if (ram_ptr) {
 			ram_val = readl_raw(ram_ptr + ram_size - 16);
-			pci_unmap_mem(ram_ptr, ram_size);
+			pci_unmap_mem(ram_ptr, ram_size, 1);
 			if (ram_val == 0x52414944) {
 				printf_info("%s: not initializing, "
 				            "driven by RAID controller.\n",
@@ -2980,7 +2995,7 @@ static int sym_detach(hcb_p np)
 	/*
 	 *  Free host resources
 	 */
-	sym_free_resources(np);
+	sym_free_resources(np, 0);
 
 	return 1;
 }

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

[Matthew Wilcox]

On Wed, Oct 17, 2007 at 10:53:06AM -0400, Tony Battersby wrote:
> After looking at it carefully, this is true of pci_map_mem, but not
> pci_unmap_mem.  pci_unmap_mem can be called from both ->detect and
> ->release.  io_request_lock is held in ->detect but not in ->release.
> So, your patch locks up the system on module unload.

My mistake.  I should have checked the iorl was held over ->release too.

This is a pretty ugly patch, but I think the three alternatives are
worse:

 - Drop the iorl at the beginning of ->detect and reacquire it at exit.
   We don't know what else the iorl might be protecting.  Probably
   nothing, but I don't want to audit it.
 - Convert sym2 back to old error handling so the midlayer doesn't
   acquire the iorl for us in ->detect.
 - Acquire the iorl in ->release.  We might deadlock on something.

So, sure, apply this patch.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

[Willy Tarreau]

On Wed, Oct 17, 2007 at 10:27:47AM -0600, Matthew Wilcox wrote:
> On Wed, Oct 17, 2007 at 10:53:06AM -0400, Tony Battersby wrote:
> > After looking at it carefully, this is true of pci_map_mem, but not
> > pci_unmap_mem.  pci_unmap_mem can be called from both ->detect and
> > ->release.  io_request_lock is held in ->detect but not in ->release.
> > So, your patch locks up the system on module unload.
> 
> My mistake.  I should have checked the iorl was held over ->release too.
> 
> This is a pretty ugly patch, but I think the three alternatives are
> worse:
> 
>  - Drop the iorl at the beginning of ->detect and reacquire it at exit.
>    We don't know what else the iorl might be protecting.  Probably
>    nothing, but I don't want to audit it.
>  - Convert sym2 back to old error handling so the midlayer doesn't
>    acquire the iorl for us in ->detect.
>  - Acquire the iorl in ->release.  We might deadlock on something.
> 
> So, sure, apply this patch.

Matthew,

Sincere thanks for your help and review. I'll apply this patch in
2.4.36-pre2 and in next 2.4.35.X. An ugly fix is clearly better than
a massive change at this stage.

Natalie, if you want, you can close the bug right now, I've queued the
patch for both branches.

Best regards,
Willy

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

[Matthew Wilcox]

On Wed, Oct 17, 2007 at 08:46:48PM +0200, Willy Tarreau wrote:
> Sincere thanks for your help and review. I'll apply this patch in
> 2.4.36-pre2 and in next 2.4.35.X. An ugly fix is clearly better than
> a massive change at this stage.

You're most welcome.  I have no desire to act as maintainer for sym2 in
2.4 at this point, but if you've got any other related bugs, feel free
to point me at them.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."