Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 18, 2016 at 10:19:38AM +0300, Felipe Balbi wrote:
> 
> 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?
> 

Per my understanding, if there are pending transfers, the gadget driver
needs to call usb_ep_dequeue to cancel transfers first, then call
usb_ep_disable.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux