From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751054AbdAPLbr (ORCPT ); Mon, 16 Jan 2017 06:31:47 -0500 Received: from mga06.intel.com ([134.134.136.31]:42684 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750864AbdAPLbq (ORCPT ); Mon, 16 Jan 2017 06:31:46 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,238,1477983600"; d="asc'?scan'208";a="49297766" 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> Date: Mon, 16 Jan 2017 13:29:45 +0200 Message-ID: <878tqbmedy.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: > 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_stat= us' >>> 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 del= ay >>> 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 *d= wc, >>> 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 here i= f the >>> + * request for handling delay STATUS has been que= ued >>> + * 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... > > 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 look into dropping USB_GADGET_DELAYED_STATUS altogether? If you're too busy, no issues. It'll stay in my queue. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlh8rqkACgkQzL64meEa mQYlpA/+IfEGpTvXMqRF8hGs/RAwbZcBLCK2IWbbbomi0QhqbqOjqRmnX33gmmW7 YdpbMmCSosHft/qSk9MjmT4fOS8yhb6htUfdm5bd3uyuWH5VKs6k0xc4PE5pE82B vlGEknnDSwnYMUOdxDKY7CdHi6kNB99FSJ+DYKt5D7Un8B8uCPTzFUe4Mf7/uldw iMUkgfDi2b9rtpkC7wAaNRBEtHMb6LH3ZtAkqZjwD2PTKQAsbKtpcP8QV0ccLGA1 XU+K+I3L0+Wv9sWmgKTA20XoWlOnjQJwyYXHvBtZw4pUpT07HPzgNGSo/VNWJnw1 IrSy6l4rM36sFpazBIdN9uf8HmBCJ+IOp+4ZfdDbsgdWz9pMhogq0uqNzYL7ebtV PyTKUaZnY4QU1EkDcb86OIsXKaCCzFa3qKPMGdXtnDKc3jsChIVYCJYjjJUaVYA0 FrdKVj9FlgBuAHpiEk+ju2S1inh9uajYTN7EcMbgVKe4WSPJxPicn0KuGf+LX5ab 17C9wBY1pL7xvKUEOU3YG9kzjgd2vWyzLGAs6nf77roTvf8lC8FSt31erwo9+q42 i+pOpN5lYMc1fYYFfO/2F4NCOqhFIdrPqbEAivocw2glztZnE73FQ5grBROSKeeJ NISE5oBQc9bkAUrT5VkvqxbEtwAWh+tvsxlpBebiS1bhOPNg5fE= =h6uM -----END PGP SIGNATURE----- --=-=-=--