On Thu, Mar 22, 2012 at 12:12 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Wed, 21 Mar 2012, Ming Lei wrote: > >> On Wed, Mar 21, 2012 at 10:35 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: >> > On Wed, 21 Mar 2012, Ming Lei wrote: >> > >> >> Looks it is a general issue about usb_hcd_unlink_urb. >> >> >> >> Alan, how about increasing URB reference count before calling unlink1 >> >> inside usb_hcd_unlink_urb to fix this kind of problem? >> > >> > No, that won't fix the problem. The URB could complete and be >> > deallocated even before usb_hcd_unlink_urb() is called, so nothing that >> > function can do will prevent the error. >> >> IMO, driver should not call usb_hcd_unlink_urb after urb is freed from >> the driver, >> but this problem is that URB may be freed during usb_hcd_unlink_urb. > > Drivers don't call usb_hcd_unlink_urb; they call usb_unlink_urb. This > sort of thing can happen: > > Driver Interrupt handler > ------ ----------------- > call usb_unlink_urb > URB completion interrupt occurs > call usb_hcd_giveback_urb > completion routine calls usb_free_urb > URB is deallocated > call usb_hcd_unlink_urb > try to increment URB's refcount > oops because URB is gone Got it, thanks for your detailed explanation. >> In fact, it is allowed that usb_free_urb is called inside .complete handler, >> at least as said in Documentation/URB.txt: >> >> "You may free an urb that you've submitted,..." >> >> So looks reasonable to increase the URB reference count before calling >> unlink1(), just like that done inside usb_hcd_flush_endpoint(). And I >> think it is a general solution for avoiding this kind of problem. > > It will not solve the problem illustrated above. The driver must avoid > freeing the URB before usb_unlink_urb returns. In this case, > increasing the refcount around the unlink call would work. Yes, it is the right fix. >> > It is the caller's responsibility to make sure that the URB does not >> > get freed before usb_unlink_urb() or usb_kill_urb() returns. We >> > probably should mention this in the kerneldoc... >> >> If so, looks it is a bit contrary with Documentation/URB.txt, also >> this may add extra constraint(maybe unnecessary) to the driver. > > It's not contradictory. You may indeed free an URB that you have > submitted, so long as you don't free it while usb_unlink_urb (or > related routines like usb_kill_urb) is running. Maybe it should be documented. So looks the correct fix should be below: diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 4b8b52c..e36a821 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -588,7 +588,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) entry = (struct skb_data *) skb->cb; urb = entry->urb; - + usb_get_urb(urb); spin_unlock_irqrestore(&q->lock, flags); // during some PM-driven resume scenarios, // these (async) unlinks complete immediately @@ -597,6 +597,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) netdev_dbg(dev->net, "unlink urb err, %d\n", retval); else count++; + usb_put_urb(urb); spin_lock_irqsave(&q->lock, flags); } spin_unlock_irqrestore (&q->lock, flags); @@ -1028,7 +1029,6 @@ static void tx_complete (struct urb *urb) } usb_autopm_put_interface_async(dev->intf); - urb->dev = NULL; entry->state = tx_done; defer_bh(dev, skb, &dev->txq); } Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
