From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751076AbdAQKmJ (ORCPT ); Tue, 17 Jan 2017 05:42:09 -0500 Received: from mga01.intel.com ([192.55.52.88]:42325 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750985AbdAQKmG (ORCPT ); Tue, 17 Jan 2017 05:42:06 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,244,1477983600"; d="asc'?scan'208";a="31562123" From: Felipe Balbi To: Baolin Wang Cc: Greg KH , USB , LKML , Linaro Kernel Mailman List , Mark Brown Subject: Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase In-Reply-To: References: <87h94zmfxm.fsf@linux.intel.com> <878tqbmedy.fsf@linux.intel.com> <8760lfmcof.fsf@linux.intel.com> Date: Tue, 17 Jan 2017 12:39:02 +0200 Message-ID: <8760lekm2h.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: >>>>>> 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 se= tup >>>>>>> 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 simi= lar >>>>>> 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 ch= ance >>>>>>> to handle the STATUS phase. Thus we should check if the request for= delay >>>>>>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' pha= se 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 dwc= 3 *dwc, >>>>>>> dwc->ep0state =3D EP0_STATUS_PHASE; >>>>>>> >>>>>>> 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 he= re 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_CONFIG= URED); >>>>>> >>>>>> Isn't this patch also changing the normal case when usb_ep_queue() c= omes >>>>>> 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. >>>> >>>> Alright, it's important enough to fix this bug. Can you also have a lo= ok >>>> into dropping USB_GADGET_DELAYED_STATUS altogether? If you're too busy, >>>> no issues. It'll stay in my queue. >>> >>> Okay, I will have a look at f_mass_storage driver to see if we can >>> drop USB_GADGET_DELAYED_STATUS. Thanks. >> >> not only mass storage. It needs to be done for all drivers. The way to >> do that is to teach functions that control transfers are composed of two >> or three phases. If you look at UDC drivers today, they all have >> peculiarities about control transfers to handle stuff that *maybe* >> gadget drivers won't handle. >> >> What we should do here is make sure that *all* 3 phases always have a >> matching usb_ep_queue() coming from the upper layers. Whether >> composite.c or f_*.c handles it, that's an implementation detail. But >> just to illustrate the problem, we should be able to get rid of >> dwc3_ep0_out_start() and assume that the upper layer will call >> usb_ep_queue() when it wants to receive a new SETUP packet. >> >> Likewise, we should be able to assume that STATUS phase will only start >> based on a usb_ep_queue() call. That way we can remove >> USB_GADGET_DELAYED_STATUS altogether, because that will *always* be the >> case. There will be no races to handle apart from the normal case where >> XferNotReady can come before or after usb_ep_queue(), but we already >> have proper handling for that too. > > Thanks to explain, but seems it need lots of work to convert these > drivers, and I saw Alan had some concern about that. So I am not sure > how to convert these drivers which can meet your requirements, maybe > from adding one "wants_explicit_ctrl_phases" flag in struct > usb_gadget as you suggested to start? yeah. Something like this: patch 1: add the new flag and document it patch 2: implement usb_ep_queue() for data and status phases conditional to that flag being set patch 3: (e.g.) change dwc3 to rely on that behavior and pass the flag to usb_gadget. this will be enough to actually test that the idea and implementation works out. After that, just for_each_UDC_driver() port_patch_3(); Then you add: patch N - 1: remove legacy code and force behavior of wants_explicit_ctrl_phases patch N: remove wants_explicit_ctrl_phases something along these lines. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlh99EYACgkQzL64meEa mQbK9hAArkKqm5zeh6BVMhGMr9Md4hlPlKFT+GDJdwCMgXpo00CjEaC/Z+cjtJ2O 5wWSAZvqMVsg48Vif4hBYkc33LXxbvyM5t5ch8dHMk23V+FYSVp4ozQ78uzBVhlz hUSOWrrUmH5EhvA1UcLrBD1pDdy6HVLg5o/de809droI3wOm2F6w+YoehN4BnMyK 98+0YtmVAcm+TLTF8goQMhXy2tQ+VTMSQlBgNNt3xsfO2If76TNjSmQiUOw/JDQM dF9SIaF5l1nBBdytIueW7kOQXELdZlsyioWEVK6AqeK/liB85ODsHd8trYlPIv9l peNi+L3oyc65AozT5lQclhn0d7cd0cWAH1B/Y2Qd7awLmJ3hMUxejylaLLb+Ir3Y xPep9NAa6FzqsMmiqrNiW1hRj6u42PVMMvuBsNF0ft83bf1GpqMYNU1Q4JxtlNdQ cZGsL10i6mkUm+IGNzBRWmRHhlGk5v3kJ4FTPJFE8Wtq0idXjAhgC9NTpmXDW0RR n1DYjAu7YMfhnXRYyXd/sF8Yl7sabAG7TrKeNJB5k5nYvQWQKtFhtXPB1SBJwBOp vDHQqHwUObiy/fgK/3GeL+ZiTONWlfxI5M0uuKZWCzibbngS4jJKIHrL37QFfk2T /3EkaqOXpcczSHJs/amy+2ZOJdA9gRWo3spZwEt5nBRg8kGWS74= =xOY2 -----END PGP SIGNATURE----- --=-=-=--