Messages in this thread | | | Date | Tue, 12 May 2009 23:53:23 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface |
| |
On Tue, 12 May 2009 23:30:52 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <wg@grandegger.com> wrote: > > > +int can_restart_now(struct net_device *dev) > > +{ > > + struct can_priv *priv = netdev_priv(dev); > > + struct net_device_stats *stats = &dev->stats; > > + struct sk_buff *skb; > > + struct can_frame *cf; > > + int err; > > + > > + /* Synchronize with dev->hard_start_xmit() */ > > + netif_tx_lock(dev); > > + > > + /* Ensure that no more messages can go out */ > > + if (netif_carrier_ok(dev)) > > + netif_carrier_off(dev); > > + > > + /* Ensure that no more messages can come in */ > > + err = priv->do_set_mode(dev, CAN_MODE_STOP); > > + if (err) > > + return err; > > + > > + /* Now it's save to clean up */ > > + del_timer_sync(&priv->restart_timer); > > This is deadlockable. > > It calls del_timer_sync() while holding netif_tx_lock(). But the timer > handler (can_restart_now()) also takes netif_tx_lock(). So if the > timer handler is presently running, it's sitting there spinning in > netif_tx_lock(). And del_timer_sync() is sitting there waiting for the > timer handler to complete.
Also, I wonder if it's safe to take netif_tx_lock() from a timer handler when other parts of the code might be taking it from process context (I didn't check).
lockdep should be able to detect this, and I trust this code has been fully runtime tested with lockdep enabled?
<boggles at the size of the inlined netif_tx_lock()>
| |