On Tue, Oct 18, 2016 at 10:19:38AM +0300, Felipe Balbi wrote: > > Hi, > > Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes: > >> 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. > > not all, no. And, frankly, this is really only a problem if we're > unregistering a gadget while there are still pending transfers. > > The original problem statement is that we unregister a gadget, issue End > Transfer, but CmdCompletion for the EndTransfer comes way too > late. Baolin, can you clarify again what happens when the IRQ comes > late? > Per my understanding, if there are pending transfers, the gadget driver needs to call usb_ep_dequeue to cancel transfers first, then call usb_ep_disable. -- Best Regards, Peter Chen -- 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
