Re: [PATCH] usbnet: fix race condition caused spinlock bad magicissue

[Ingo Molnar]

* wangbiao <biao.wang@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/