Hi Baolin,

Baolin Wang <baolin.w...@linaro.org> writes:
>> Felipe Balbi <felipe.ba...@linux.intel.com> 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.w...@linaro.org>
>>> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com>
>>
>> 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.ba...@linux.intel.com>
>> 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.w...@linaro.org>
>> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com>
>
> 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

Reply via email to