From d7811e623dd4be3e3bdba2d6330f7f15541ee45f Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Mon, 18 Sep 2006 00:22:30 -0700 Subject: [NET]: Drop tx lock in dev_watchdog_up Fix lockdep warning with GRE, iptables and Speedtouch ADSL, PPP over ATM. On Sat, Sep 02, 2006 at 08:39:28PM +0000, Krzysztof Halasa wrote: > > ======================================================= > [ INFO: possible circular locking dependency detected ] > ------------------------------------------------------- > swapper/0 is trying to acquire lock: > (&dev->queue_lock){-+..}, at: [] dev_queue_xmit+0x56/0x290 > > but task is already holding lock: > (&dev->_xmit_lock){-+..}, at: [] dev_queue_xmit+0x224/0x290 > > which lock already depends on the new lock. This turns out to be a genuine bug. The queue lock and xmit lock are intentionally taken out of order. Two things are supposed to prevent dead-locks from occuring: 1) When we hold the queue_lock we're supposed to only do try_lock on the tx_lock. 2) We always drop the queue_lock after taking the tx_lock and before doing anything else. > > the existing dependency chain (in reverse order) is: > > -> #1 (&dev->_xmit_lock){-+..}: > [] lock_acquire+0x76/0xa0 > [] _spin_lock_bh+0x31/0x40 > [] dev_activate+0x69/0x120 This path obviously breaks assumption 1) and therefore can lead to ABBA dead-locks. I've looked at the history and there seems to be no reason for the lock to be held at all in dev_watchdog_up. The lock appeared in day one and even there it was unnecessary. In fact, people added __dev_watchdog_up precisely in order to get around the tx lock there. The function dev_watchdog_up is already serialised by rtnl_lock since its only caller dev_activate is always called under it. So here is a simple patch to remove the tx lock from dev_watchdog_up. In 2.6.19 we can eliminate the unnecessary __dev_watchdog_up and replace it with dev_watchdog_up. Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- net/sched/sch_generic.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 0834c2ee9174..6f9151899795 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -238,9 +238,7 @@ void __netdev_watchdog_up(struct net_device *dev) static void dev_watchdog_up(struct net_device *dev) { - netif_tx_lock_bh(dev); __netdev_watchdog_up(dev); - netif_tx_unlock_bh(dev); } static void dev_watchdog_down(struct net_device *dev) -- cgit v1.2.3