Hi, Baolin Wang <baolin.wang@xxxxxxxxxx> writes: >>>>>> >> 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? >>> >>> 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. >> >> okay, thanks for describing it again. Another option would be to only >> wait_for_completion() before calling free_irq() is any endpoint has >> END_TRANSFER_PENDING flag set. Something like below: > > Yeah, that is what my original patch did, but you did one better > patch, I will test it too. That's true. I'll give you proper credit in the commit log since you originated the idea. -- balbi
Attachment:
signature.asc
Description: PGP signature
