On Mon, Oct 17, 2016 at 12:51:48PM +0300, Felipe Balbi wrote: > > Hi, > > Peter Chen <hzpeterchen@xxxxxxxxx> writes: > > 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. > > That implementation is wrong. You should tell the gadget driver that > reset happened by calling usb_gadget_udc_reset(). It's just that > usb_gadget_udc_reset() would also need to be called with IRQs enabled > after this rework happens. > I see, the usb_ep_disable should be called from gadget driver if a bus reset occurs. Would you consider add special handling like work queue for dwc3's ep_disable API to wait something? -- 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
