Messages in this thread | | | Date | Mon, 11 Nov 2013 09:23:30 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH] usbnet: fix race condition caused spinlock bad magic issue |
| |
* wangbiao <biao.wang@intel.com> wrote:
> @@ -1448,8 +1448,10 @@ static void usbnet_bh (unsigned long param) > > // waiting for all pending urbs to complete? > if (dev->wait) { > + wait_queue_head_t *wait_d = dev->wait; > if ((dev->txq.qlen + dev->rxq.qlen + dev->done.qlen) == 0) { > - wake_up (dev->wait); > + if (wait_d) > + wake_up(wait_d); > } > > // or are we maybe short a few urbs?
1)
Nit: the scope of 'wait_d' is unnecessarily broad, it could be moved to the block that uses it.
2)
Also, the changelog mentions that dev->wait can race - it would be nice to add to the changelog what exact synchronization mechanism protects usbnet_terminate_urbs() and usbnet_bh() from seeing/modifying that value at once - as the code was clearly written without such interaction in mind.
> @@ -1602,6 +1604,7 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) > init_timer (&dev->delay); > mutex_init (&dev->phy_mutex); > mutex_init(&dev->interrupt_mutex); > + init_waitqueue_head(&unlink_wakeup);
3)
Can that runtime initialization be avoided by using DECLARE_WAIT_QUEUE_HEAD()?
Thanks,
Ingo
| |