Hi, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes: >> 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. > > 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? >> 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, when the endpoint is disabled it shouldn't trigger any further interrupts. -- balbi
Attachment:
signature.asc
Description: PGP signature
