On Mon, 17 Oct 2016, Felipe Balbi wrote: > Hi Baolin, > > Baolin Wang <baolin.wang@xxxxxxxxxx> writes: > >> Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes: > >>> Instead of just delaying for 100us, we should > >>> actually wait for End Transfer Command Complete > >>> interrupt before moving on. Note that this should > >>> only be done if we're dealing with one of the core > >>> revisions that actually require the interrupt before > >>> moving on. > >>> > >>> Reported-by: Baolin Wang <baolin.wang@xxxxxxxxxx> > >>> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> > >> > >> I've updated this one in order to fix the comment we had about delaying > >> 100us. No further changes were made, only the comment. Here it is: > >> > >> 8<------------------------------------------------------------------------ > >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001 > >> From: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> > >> Date: Thu, 13 Oct 2016 14:09:47 +0300 > >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete > >> > >> Instead of just delaying for 100us, we should > >> actually wait for End Transfer Command Complete > >> interrupt before moving on. Note that this should > >> only be done if we're dealing with one of the core > >> revisions that actually require the interrupt before > >> moving on. > >> > >> Reported-by: Baolin Wang <baolin.wang@xxxxxxxxxx> > >> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> > > > > From my testing, there are still some problems caused by the nested > > lock, at lease I found 2 functions will issue the usb_ep_disable() > > function with spinlock: > > Thanks for testing. Seems like I really need to revisit locking in the > entire gadget API. In either case, we need to have a larger discussion > about this situation. > > IMO, usb_ep_disable() and usb_ep_enable() should only be callable from > process context, but that has never been a requirement before. Still, > it's not far-fetched to assume that a controller (such as dwc3, but > probably others) might sleep while cancelling a transfer because they > need to wait for an Interrupt. > > In fact, we know of two controllers that need this: dwc3 and dwc3.1. It's not clear that this should be the deciding factor. That is, does usb_ep_disable() need to wait until all the endpoint's outstanding transfers have been completed before it returns? I don't think it does. > I wonder if there are any other controllers with this > requirement. Either way, We should also consider if there are any strong > reasons to call usb_ep_disable() with interrupts disabled and locks held > considering that UDC _must_ call ->complete() for each and every > request. > > If we go ahead with this, it'll mean a rather large series (again) to > fix all the calling semantics in every single gadget driver for both > usb_ep_enable() and usb_ep_disable(). Then, making sure all UDC drivers > respect the requirement, then we update documentation about the > functions and add might_sleep() to their implementation in udc/core.c > > Anybody objects to this new requirement? The usual times for calling usb_ep_disable() is when the gadget driver processes an altsetting or configuration change, or a reset or disconnect. The notifications for these things happen in interrupt context, so it doesn't seem reasonable to require that endpoints be disabled in process context. f_mass_storage has its own worker thread. Mainly for other reasons, but it can also use the thread to handle these events. But it doesn't seem right to require all gadget drivers to do the same thing. There is still an open question about how a gadget driver can change altsettings, though. A particular endpoint might be bulk in one altsetting and isochronous in another, for example. I don't see how we can change the endpoint's characteristics without being allowed to sleep. Alan Stern -- 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
