On Thu, 7 Mar 2013, Alan Stern wrote: > On Thu, 7 Mar 2013, Paul Zimmerman wrote: > > > Hi Alan, > > > > I was talking about uhci_hcd_endpoint_disable(). It drops the spinlock, > > reacquires it, but doesn't reload qh from hep->hcpriv before continuing. > > Maybe that's not required for uhci, I didn't look any deeper. > > No, it's not required. hep->hcpriv doesn't matter once we have used it > to find the correct qh. > > > Actually, I see now I misread ohci_endpoint_disable(), it doesn't reload > > from ep->hcpriv either, only ehci_endpoint_disable() does. > > That's right. I suppose we might get into trouble if any one of the > *_endpoint_disable() routines was called concurrently in different > threads for the same endpoint. Fortunately usbcore is smart enough not > to do that.
Looking back over this thread, I now see what Felipe was objecting to. It appears that you never call usb_hcd_link_urb_to_ep(), usb_hcd_unlink_urb_from_ep(), or usb_hcd_check_unlink_urb(). An HCD has to call those routines while holding its private lock; not to do so is a bug. Also, although not stated anywhere explicitly, your urb_enqueue routine shouldn't try to acquire a qh until after it calls usb_hcd_link_urb_to_ep(). Otherwise you might end up using a stale qh -- one that's in the process of being disabled and deallocated. (That's the bug in ohci-hcd I mentioned earlier.) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
