Hi,
Baolin Wang baolin.wang@linaro.org writes:
When handing the SETUP packet by composite_setup(), we will release the dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup function, which means we need to delay handling the STATUS phase.
this sentence needs a little work. Seems like it's missing some information.
anyway, I get that we release the lock but...
But during the lock release period, maybe the request for handling delay
execution of ->setup() itself should be locked. I can see that it's only locked for set_config() which is rather peculiar.
What exact request you had when you triggered this? (Hint: dwc3 tracepoints print out ctrl request bytes). IIRC, only set_config() or f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
Which gadget driver were you using when you triggered this?
Another point here is that the really robust way of fixing this is to get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure gadget drivers know how to queue requests for all three phases of a Control Transfer.
A lot of code will be removed from all gadget drivers and UDC drivers while combining all of it in a single place in composite.c.
The reason I'm saying this is that other UDC drivers might have similar races already but they just haven't triggered.
STATUS phase has been queued into list before we set 'dwc->delayed_status' flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance to handle the STATUS phase. Thus we should check if the request for delay STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in dwc3_ep0_xfernotready(), if so, we should handle it.
Signed-off-by: Baolin Wang baolin.wang@linaro.org
drivers/usb/dwc3/ep0.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 9bb1f85..e689ced 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc, dwc->ep0state = EP0_STATUS_PHASE; if (dwc->delayed_status) {
struct dwc3_ep *dep = dwc->eps[0];WARN_ON_ONCE(event->endpoint_number != 1);/** We should handle the delay STATUS phase here if the* request for handling delay STATUS has been queued* into the list.*/if (!list_empty(&dep->pending_list)) {dwc->delayed_status = false;usb_gadget_set_state(&dwc->gadget,USB_STATE_CONFIGURED);
Isn't this patch also changing the normal case when usb_ep_queue() comes later? I guess list_empty() protects against that...