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? -- balbi
Attachment:
signature.asc
Description: PGP signature
