Message ID | 20100710201813.a3e5c79c.ken_kawasaki@spring.nifty.jp |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Sat, 2010-07-10 at 20:18 +0900, Ken Kawasaki wrote: > axnet_cs: > use spin_lock_irqsave instead of spin_lock in ax_interrupt [...] I assume this is because it's now called from ei_watchdog() and not only from interrupt context. Perhaps you should explain that in the commit message. Ben.
From: Ben Hutchings <bhutchings@solarflare.com> Date: Sun, 11 Jul 2010 03:46:28 +0100 > On Sat, 2010-07-10 at 20:18 +0900, Ken Kawasaki wrote: >> axnet_cs: >> use spin_lock_irqsave instead of spin_lock in ax_interrupt > [...] > > I assume this is because it's now called from ei_watchdog() and not only > from interrupt context. Perhaps you should explain that in the commit > message. No, interrupt handlers in general may not assume that interrupts are off or on when they are invoked. Therefore they must use irqflags saving/restoring. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2010-07-10 at 19:49 -0700, David Miller wrote: > From: Ben Hutchings <bhutchings@solarflare.com> > Date: Sun, 11 Jul 2010 03:46:28 +0100 > > > On Sat, 2010-07-10 at 20:18 +0900, Ken Kawasaki wrote: > >> axnet_cs: > >> use spin_lock_irqsave instead of spin_lock in ax_interrupt > > [...] > > > > I assume this is because it's now called from ei_watchdog() and not only > > from interrupt context. Perhaps you should explain that in the commit > > message. > > No, interrupt handlers in general may not assume that interrupts > are off or on when they are invoked. > > Therefore they must use irqflags saving/restoring. But an interrupt handler will not be called recursively for the same IRQ. Since this device only uses one IRQ, surely it was OK to use spin_lock() in this function so long as it was only called from the interrupt handler. Ben.
From: Ben Hutchings <bhutchings@solarflare.com> Date: Sun, 11 Jul 2010 04:12:34 +0100 > But an interrupt handler will not be called recursively for the same > IRQ. Since this device only uses one IRQ, surely it was OK to use > spin_lock() in this function so long as it was only called from the > interrupt handler. It seems your right, I'll make a note about the real reason we're doing this in the commit message. But frankly anything else is dangerous. Especially if one intends to support ->poll_controller() which we pretty much expect every modern and future driver to do. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- linux-2.6.35-git7/drivers/net/pcmcia/axnet_cs.c.orig 2010-07-04 08:28:15.000000000 +0900 +++ linux-2.6.35-git7/drivers/net/pcmcia/axnet_cs.c 2010-07-10 19:07:20.000000000 +0900 @@ -1168,6 +1168,7 @@ static irqreturn_t ax_interrupt(int irq, int interrupts, nr_serviced = 0, i; struct ei_device *ei_local; int handled = 0; + unsigned long flags; e8390_base = dev->base_addr; ei_local = netdev_priv(dev); @@ -1176,7 +1177,7 @@ static irqreturn_t ax_interrupt(int irq, * Protect the irq test too. */ - spin_lock(&ei_local->page_lock); + spin_lock_irqsave(&ei_local->page_lock, flags); if (ei_local->irqlock) { @@ -1188,7 +1189,7 @@ static irqreturn_t ax_interrupt(int irq, dev->name, inb_p(e8390_base + EN0_ISR), inb_p(e8390_base + EN0_IMR)); #endif - spin_unlock(&ei_local->page_lock); + spin_unlock_irqrestore(&ei_local->page_lock, flags); return IRQ_NONE; } @@ -1261,7 +1262,7 @@ static irqreturn_t ax_interrupt(int irq, ei_local->irqlock = 0; outb_p(ENISR_ALL, e8390_base + EN0_IMR); - spin_unlock(&ei_local->page_lock); + spin_unlock_irqrestore(&ei_local->page_lock, flags); return IRQ_RETVAL(handled); }
axnet_cs: use spin_lock_irqsave instead of spin_lock in ax_interrupt Signed-off-by: Ken Kawasaki <ken_kawasaki@spring.nifty.jp> --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html