From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751397AbdAPL3Q (ORCPT ); Mon, 16 Jan 2017 06:29:16 -0500 Received: from mail-ot0-f170.google.com ([74.125.82.170]:36575 "EHLO mail-ot0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750864AbdAPL3N (ORCPT ); Mon, 16 Jan 2017 06:29:13 -0500 MIME-Version: 1.0 In-Reply-To: <87h94zmfxm.fsf@linux.intel.com> References: <87h94zmfxm.fsf@linux.intel.com> From: Baolin Wang Date: Mon, 16 Jan 2017 19:29:12 +0800 Message-ID: Subject: Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase To: Felipe Balbi Cc: Greg KH , USB , LKML , Linaro Kernel Mailman List , Mark Brown Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 16 January 2017 at 18:56, Felipe Balbi wrote: > > Hi, > > Baolin Wang 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. Yes, when host set configuration for mass storage driver, it can produce this issue. > > Which gadget driver were you using when you triggered this? mass storage driver. When host issues the setting config request, we will get USB_GADGET_DELAYED_STATUS result from set_config()--->fsg_set_alt(). Then the mass storage driver will issue one thread to complete the status stage by ep0_queue() (this thread may be running on another core), then if the thread issues ep0_queue() too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or before we get into the STATUS stage, then we can not handle this request for the delay STATUS stage in dwc3_gadget_ep0_queue(). > > 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. Yes, maybe. > >> 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 >> --- >> 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... I think it will not change other cases, we only handle the delayed status and I've tested it for a while and I did not find any problem. -- Baolin.wang Best Regards