On Mon, Oct 17, 2016 at 11:06:49AM +0300, 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. > > 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? > At chipidea driver, it might call usb_ep_disable at interrupt context, eg, the bus reset interrupt. See chipidea/udc.c isr_reset_handler-> _gadget_stop_activity->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
