On Mon, 25 Nov 2013, David Laight wrote: > > From: Alan Stern > > On Mon, 25 Nov 2013, Xenia Ragiadakou wrote: > > > > > > You can simplify part of the problem by not allowing an endpoint to be > > > > reset if it has any pending URBs. Just fail the reset in this case. > > > > > > Yes, you are right since, from what i understand, it is the > > > responsibility of the device driver to unlink any pending URBs before > > > issuing a clear halt to the endpoint. So we expect the device driver to > > > call usb_unlink_urb() for each pending URB, which in turn calls > > > > Actually you should expect the driver to call usb_kill_urb(), not > > usb_unlink_urb(). The difference is that usb_kill_urb() waits for the > > URB to complete. > > What happens if the URB completed just before the driver tried to kill it?
Then usb_kill_urb() would return an error, and the driver would go on to reset the endpoint. > In this case the xhci code will be performing the upcall into the driver > at the same time - chaos is bound to happen. What upcall? Do you mean the call to the URB's completion handler? At the same time as what -- the call to usb_kill_urb()? What actually would happen is that when xhci-hcd's unlink code called usb_hcd_check_unlink_urb(), the return value would be -EIDRM, and so the unlink would fail right away with -EIDRM. > Upcalls are horrid things, very difficult to lock correctly and usually > require a 3-way handshake to cancel. "Horrid" is a matter of opinion. Anyway, cancelling the upcall isn't an issue here; the USB stack has no way to cancel an URB's completion handler. If an URB is submitted successfully then the completion handler _will_ be called exactly once for that submission, guaranteed. > IIRC xhci releases its own mutex before making the completion upcall. Spinlock, not mutex. > If it removes the URB from its lists before this, then the cancellation > request can reference a URB that no longer exists. We assume the client device driver is smart enough not to call usb_kill_urb() for an URB that it has already freed. > If it reacquires the mutex and tidies up afterwards then any cancellation > request still has to be rejected - otherwise the driver will tidy up twice. There is sufficient locking between HCDs and the USB core to insure that an HCD never tries to unlink an URB that has already completed (or has already been unlinked). > The only obvious solution is that the xhci driver cannot assume the URB > address is valid (so must scan the active requests looking for it) and > then must call the completion function with a status of 'cancelled'. > > If is all a lot easier if there are strong constraints on what the driver > can do in the upcall function. Clearly you need to become more familiar with the details of the USB stack. In particular, study the documentation and usage of usb_hcd_link_urb_to_ep(), usb_hcd_check_unlink_urb(), and usb_hcd_unlink_urb_from_ep(). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
