Hi, On 18 October 2016 at 15:19, Felipe Balbi <felipe.ba...@linux.intel.com> wrote: > > Hi, > > Alan Stern <st...@rowland.harvard.edu> writes: >>> Baolin Wang <baolin.w...@linaro.org> writes: >>> >> Felipe Balbi <felipe.ba...@linux.intel.com> 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.w...@linaro.org> >>> >>> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com> >>> >> >>> >> 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.ba...@linux.intel.com> >>> >> 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.w...@linaro.org> >>> >> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com> >>> > >>> > 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?
Sure. The problem I met was, when we change the USB function with configfs frequently, sometimes it will hang the system to crash. The reason is, we will start end transfer command when disable the endpoint, but sometimes the end transfer command complete event comes after we have freed the gadget irq (since the xHCI will share the same interrupt number with gadget, thus when free the gadget irq, it will not shutdown this gadget irq line), it will trigger the interrupt line all the time. > >>> 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. > > Oh, that's true. I completely forgot about altsetting change. > >> 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. > > Heh, seems that I ended up touching a subject that hasn't been revisited > in few years :-) > > Anyway, let's see what Baolin says about the IRQ again. Now that I think > of it, free_irq() should be calling synchronize IRQ, right? So by the > time free_irq() returns, we shouldn't get any further interrupts. Also, It will synchronize irq in free_irq() function by issuing synchronize_irq() function. > when the endpoint is disabled it shouldn't trigger any further > interrupts. > > -- > balbi -- Baolin.wang Best Regards -- 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
