From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751328AbdAPK6Y (ORCPT ); Mon, 16 Jan 2017 05:58:24 -0500 Received: from mga11.intel.com ([192.55.52.93]:14348 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751030AbdAPK6W (ORCPT ); Mon, 16 Jan 2017 05:58:22 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,238,1477983600"; d="asc'?scan'208";a="213863874" From: Felipe Balbi To: Baolin Wang , gregkh@linuxfoundation.org Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, linaro-kernel@lists.linaro.org, broonie@kernel.org, baolin.wang@linaro.org Subject: Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase In-Reply-To: References: Date: Mon, 16 Jan 2017 12:56:21 +0200 Message-ID: <87h94zmfxm.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable 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. 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 > --- > 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 =3D EP0_STATUS_PHASE; >=20=20 > if (dwc->delayed_status) { > + struct dwc3_ep *dep =3D dwc->eps[0]; > + > WARN_ON_ONCE(event->endpoint_number !=3D 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 =3D 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... =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlh8ptUACgkQzL64meEa mQbydRAAyjNgY9uNDAp02cNly0SXVq1/xOr0n8nq8UMTjIHjQrrQy0DPlbZnH5ES yLRntHUbkUeGlSSu1Q+9oxg+fN9GhnPnx1LS1L4kkvffOeuDP83MqHcGugJNJW9t GECXTlOjEvClFrpvUkf0DiENa6kRRR7rGbaAHy60YnpX7qQt2ANtvIQ8VIxbDDqy KNO9IDcn8sR3xJ+zJ42FTg3VBkGd6Wvi4MHvxgw5OybsH/hF50BkVayJFY8oNeLH Ibdppf0kVEO2IwCkE50k/RdvK7JS//H+zQdbTofYHs0gqlJgmkSc5t5Y3I1seZM3 cj4fV98Ug28Q7uyZrlhAUV6TK9Gl8QFKmwWog4a7qXX5qDfp+UTU4VxV9dgMjXOT rUv6Yg0VfQ8KkqW5K8BcjsLsBe/fdzPFSPqP580B3cyrJ8aRaLWBzGG/ve6Ofh4M zXPt3D7wlBjQP4aykbHfmwclT74H2z/+u8h2UEQ8A4NXCUcuvopsxw6jerwoCvmx d40vFrafstIgbSX0yHPtfHzhi6eViS2LqwlYJ0oafmSxG6Cx8aa7lG3FenBwv/Hg mrrA2kvkfuMxCngunCpAV+gfYhbehR5ebyuHQ4kVmSQJW30htGSowSqwiflFIq5U mw3m7komFreDBt9Ow3ysH0y3VpJ8iEarI6hd08HiDUeWrxRMzm0= =VLDs -----END PGP SIGNATURE----- --=-=-=--