From 3a10ccebe928691d16a001687552228d32ff7910 Mon Sep 17 00:00:00 2001 From: Ingo Molnar Date: Fri, 30 Jun 2006 02:25:06 -0700 Subject: [PATCH] lock validator: fix ns83820.c irq-flags bug Barry K. Nathan reported the following lockdep warning: [ 197.343948] BUG: warning at kernel/lockdep.c:1856/trace_hardirqs_on() [ 197.345928] [] show_trace_log_lvl+0x5b/0x105 [ 197.346359] [] show_trace+0x1b/0x20 [ 197.346759] [] dump_stack+0x1f/0x24 [ 197.347159] [] trace_hardirqs_on+0xfb/0x185 [ 197.348873] [] _spin_unlock_irq+0x24/0x2d [ 197.350620] [] do_tx_done+0x171/0x179 [ns83820] [ 197.350895] [] ns83820_irq+0x149/0x20b [ns83820] [ 197.351166] [] handle_IRQ_event+0x1d/0x52 [ 197.353216] [] handle_level_irq+0x97/0xe1 [ 197.355157] [] do_IRQ+0x8b/0xac [ 197.355612] [] common_interrupt+0x25/0x2c this is caused because the ns83820 driver re-enables irq flags in hardirq context. While legal in theory, in practice it should only be done if the hardware is really old and has some very high overhead in its ISR. (such as PIO IDE) For modern hardware, running ISRs with irqs enabled is discouraged, because 1) new hardware is fast enough to not cause latency problems 2) allowing the nesting of hardware interrupts only 'spreads out' the handling of the current ISR, causing extra cachemisses that would otherwise not happen. Furthermore, on architectures where ISRs share the kernel stacks, enabling interrupts in ISRs introduces a much higher kernel-stack-nesting and thus kernel-stack-overflow risk. 3) not managing irq-flags via the _irqsave / _irqrestore variants is dangerous: it's easy to forget whether one function nests inside another, and irq flags might be mismanaged. In the few cases where re-enabling interrupts in an ISR is considered useful (and unavoidable), it has to be taught to the lock validator explicitly (because the lock validator needs the "no ISR ever enables hardirqs" artificial simplification to keep the IRQ/softirq locking dependencies manageable). This teaching is done via the explicit use local_irq_enable_in_hardirq(). On a stock kernel this maps to local_irq_enable(). If the lock validator is enabled then this does not enable interrupts. Now, the analysis of drivers/net/ns83820.c's irq flags use: the irq-enabling in irq context seems intentional, but i dont think it's justified. Furthermore, the driver suffers from problem #3 above too, in ns83820_tx_timeout() it disables irqs via local_irq_save(), but then it calls do_tx_done() which does a spin_unlock_irq(), re-enabling for a function that does not expect it! While currently this bug seems harmless (only some debug printout seems to be affected by it), it's nevertheless something to be fixed. So this patch makes the ns83820 ISR irq-flags-safe, and cleans up do_tx_done() use and locking to avoid the ns83820_tx_timeout() bug. From: Arjan van de Ven ns83820_mib_isr takes the misc_lock in IRQ context. All other places that do this in the ISR already use _irqsave versions, make this consistent at least. At some point in the future someone should audit the driver to see if all _irqsave's in the ISR can go away, this is generally an iffy/fragile proposition though; for now get it safe, simple and consistent. From: Arjan van de Ven ok this is a real driver deadlock: The ns83820 driver enabled interrupts (by unlocking the misc_lock with _irq) while still holding the rx_info.lock, which is required to be irq safe since it's used in the ISR like this: writel(1, dev->base + IER); spin_unlock_irq(&dev->misc_lock); kick_rx(ndev); spin_unlock_irq(&dev->rx_info.lock); This is can cause a deadlock if an irq was pending at the first spin_unlock_irq already, or if one would hit during kick_rx(). Simply remove the first _irq solves this Signed-off-by: Ingo Molnar Cc: Benjamin LaHaise Cc: Jeff Garzik Signed-off-by: Arjan van de Ven Signed-off-by: Andrew Morton Signed-off-by: Jeff Garzik --- drivers/net/ns83820.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) (limited to 'drivers') diff --git a/drivers/net/ns83820.c b/drivers/net/ns83820.c index d0ed8646902..0e76859c90a 100644 --- a/drivers/net/ns83820.c +++ b/drivers/net/ns83820.c @@ -803,7 +803,7 @@ static int ns83820_setup_rx(struct net_device *ndev) writel(dev->IMR_cache, dev->base + IMR); writel(1, dev->base + IER); - spin_unlock_irq(&dev->misc_lock); + spin_unlock(&dev->misc_lock); kick_rx(ndev); @@ -1012,8 +1012,6 @@ static void do_tx_done(struct net_device *ndev) struct ns83820 *dev = PRIV(ndev); u32 cmdsts, tx_done_idx, *desc; - spin_lock_irq(&dev->tx_lock); - dprintk("do_tx_done(%p)\n", ndev); tx_done_idx = dev->tx_done_idx; desc = dev->tx_descs + (tx_done_idx * DESC_SIZE); @@ -1069,7 +1067,6 @@ static void do_tx_done(struct net_device *ndev) netif_start_queue(ndev); netif_wake_queue(ndev); } - spin_unlock_irq(&dev->tx_lock); } static void ns83820_cleanup_tx(struct ns83820 *dev) @@ -1281,11 +1278,13 @@ static struct ethtool_ops ops = { .get_link = ns83820_get_link }; +/* this function is called in irq context from the ISR */ static void ns83820_mib_isr(struct ns83820 *dev) { - spin_lock(&dev->misc_lock); + unsigned long flags; + spin_lock_irqsave(&dev->misc_lock, flags); ns83820_update_stats(dev); - spin_unlock(&dev->misc_lock); + spin_unlock_irqrestore(&dev->misc_lock, flags); } static void ns83820_do_isr(struct net_device *ndev, u32 isr); @@ -1307,6 +1306,8 @@ static irqreturn_t ns83820_irq(int foo, void *data, struct pt_regs *regs) static void ns83820_do_isr(struct net_device *ndev, u32 isr) { struct ns83820 *dev = PRIV(ndev); + unsigned long flags; + #ifdef DEBUG if (isr & ~(ISR_PHY | ISR_RXDESC | ISR_RXEARLY | ISR_RXOK | ISR_RXERR | ISR_TXIDLE | ISR_TXOK | ISR_TXDESC)) Dprintk("odd isr? 0x%08x\n", isr); @@ -1321,10 +1322,10 @@ static void ns83820_do_isr(struct net_device *ndev, u32 isr) if ((ISR_RXDESC | ISR_RXOK) & isr) { prefetch(dev->rx_info.next_rx_desc); - spin_lock_irq(&dev->misc_lock); + spin_lock_irqsave(&dev->misc_lock, flags); dev->IMR_cache &= ~(ISR_RXDESC | ISR_RXOK); writel(dev->IMR_cache, dev->base + IMR); - spin_unlock_irq(&dev->misc_lock); + spin_unlock_irqrestore(&dev->misc_lock, flags); tasklet_schedule(&dev->rx_tasklet); //rx_irq(ndev); @@ -1370,16 +1371,18 @@ static void ns83820_do_isr(struct net_device *ndev, u32 isr) * work has accumulated */ if ((ISR_TXDESC | ISR_TXIDLE | ISR_TXOK | ISR_TXERR) & isr) { + spin_lock_irqsave(&dev->tx_lock, flags); do_tx_done(ndev); + spin_unlock_irqrestore(&dev->tx_lock, flags); /* Disable TxOk if there are no outstanding tx packets. */ if ((dev->tx_done_idx == dev->tx_free_idx) && (dev->IMR_cache & ISR_TXOK)) { - spin_lock_irq(&dev->misc_lock); + spin_lock_irqsave(&dev->misc_lock, flags); dev->IMR_cache &= ~ISR_TXOK; writel(dev->IMR_cache, dev->base + IMR); - spin_unlock_irq(&dev->misc_lock); + spin_unlock_irqrestore(&dev->misc_lock, flags); } } @@ -1390,10 +1393,10 @@ static void ns83820_do_isr(struct net_device *ndev, u32 isr) * nature are expected, we must enable TxOk. */ if ((ISR_TXIDLE & isr) && (dev->tx_done_idx != dev->tx_free_idx)) { - spin_lock_irq(&dev->misc_lock); + spin_lock_irqsave(&dev->misc_lock, flags); dev->IMR_cache |= ISR_TXOK; writel(dev->IMR_cache, dev->base + IMR); - spin_unlock_irq(&dev->misc_lock); + spin_unlock_irqrestore(&dev->misc_lock, flags); } /* MIB interrupt: one of the statistics counters is about to overflow */ @@ -1455,7 +1458,7 @@ static void ns83820_tx_timeout(struct net_device *ndev) u32 tx_done_idx, *desc; unsigned long flags; - local_irq_save(flags); + spin_lock_irqsave(&dev->tx_lock, flags); tx_done_idx = dev->tx_done_idx; desc = dev->tx_descs + (tx_done_idx * DESC_SIZE); @@ -1482,7 +1485,7 @@ static void ns83820_tx_timeout(struct net_device *ndev) ndev->name, tx_done_idx, dev->tx_free_idx, le32_to_cpu(desc[DESC_CMDSTS])); - local_irq_restore(flags); + spin_unlock_irqrestore(&dev->tx_lock, flags); } static void ns83820_tx_watch(unsigned long data) -- cgit v1.2.3