Skip to contentSkip to conversation

calling request_irq() with lock held (+sungem fix)

RFC+PATCH Benjamin Herrenschmidt 2 participants 2 messages Jul 30, 2003 Network drivers

lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

Hrm.. old problem: request_irq() called with a lock held.

This is unclear wether this should be safe or not, I now IDE used to do that, but the current implementation of request_irq() on most archs is definitely not safe to be called in a non-sleeping context.

i386 was sort-fixed by using GFP_ATOMIC in the kmalloc() done inside request_irq() itself, but what about all of the proc related stuff that gets done when setup_irq() calls register_irq_proc() ? So the _fact_ is that the current implementations in archs, including i386, are unsafe to call from "atomic" context.

David: this patch fixes sungem for that.

Cheers,
Ben.

drivers/net/sungem.c +4 -4
diff -urN linux-2.5/drivers/net/sungem.c linuxppc-2.5-benh/drivers/net/sungem.c
@@ -2101,17 +2101,14 @@
 		gp->hw_running = 1;
 	}
 
-	spin_lock_irq(&gp->lock);
-
 	/* We can now request the interrupt as we know it's masked
 	 * on the controller
 	 */
 	if (request_irq(gp->pdev->irq, gem_interrupt,
 			SA_SHIRQ, dev->name, (void *)dev)) {
-		spin_unlock_irq(&gp->lock);
-
 		printk(KERN_ERR "%s: failed to request irq !\n", gp->dev->name);
 
+		spin_lock_irq(&gp->lock);
 #ifdef CONFIG_PPC_PMAC
 		if (!hw_was_up && gp->pdev->vendor == PCI_VENDOR_ID_APPLE)
 			gem_apple_powerdown(gp);

@@ -2120,10 +2117,13 @@
 		gp->pm_timer.expires = jiffies + 10*HZ;
 		add_timer(&gp->pm_timer);
 		up(&gp->pm_sem);
+		spin_unlock_irq(&gp->lock);
 
 		return -EAGAIN;
 	}
 
+       	spin_lock_irq(&gp->lock);
+
 	/* Allocate & setup ring buffers */
 	gem_init_rings(gp);
#1 lore raw
Quoted 5 lines · On 30 Jul 2003 13:30:44 -0400 Benjamin Herrenschmidt…

i386 was sort-fixed by using GFP_ATOMIC in the kmalloc() done inside request_irq() itself, but what about all of the proc related stuff that gets done when setup_irq() calls register_irq_proc() ? So the _fact_ is that the current implementations in archs, including i386, are unsafe to call from "atomic" context.

That's true.

Quoted 1 line

David: this patch fixes sungem for that.

Ok, I'll review this and probably apply it, thanks.