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