From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751176AbdAWL5z (ORCPT ); Mon, 23 Jan 2017 06:57:55 -0500 Received: from mga07.intel.com ([134.134.136.100]:20924 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751019AbdAWL5x (ORCPT ); Mon, 23 Jan 2017 06:57:53 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,274,1477983600"; d="asc'?scan'208";a="51546561" From: Felipe Balbi To: Alan Stern Cc: Baolin Wang , 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: Date: Mon, 23 Jan 2017 13:57:41 +0200 Message-ID: <87h94q6lai.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, Alan Stern writes: > On Mon, 16 Jan 2017, Felipe Balbi wrote: > >> > The gadget driver never calls usb_ep_queue in order to receive the next >> > SETUP packet; the UDC driver takes care of SETUP handling >> > automatically. >>=20 >> yeah, that's another thing I'd like to change. Currently, we have no >> means to either try to implement device-initiated LPM without adding a >> ton of hacks to UDC drivers. If we require upper layers (composite.c, >> most of the time) to usb_ep_queue() separate requests for all 3 phases >> of a ctrl transfer, we can actually rely on the fact that a new SETUP >> phase hasn't been queued yet to trigger U3 entry. > > I haven't given any thought to LPM. okay > However, requiring gadget drivers to request SETUP packets seems rather > questionable. It flies against the USB spec, which requires right, maybe SETUP is a bit of an overkill. DATA and STATUS, however, should be doable. > peripherals to accept SETUP packets at any time -- a device is not > allowed to NAK or STALL a SETUP packet (see 8.4.6.4 in the USB-2 spec).= =20=20 > In fact, the hardware in UDCs probably isn't capable of doing it. > > This means that to do what you want, the UDC driver would have to > accept SETUP packets at any time, and store the most recent packet > contents. Then, when the gadget driver submits a request, the UDC > driver would give it this stored data. It would also have to detect that's right, I missed that part. > and prevent a nasty race where the gadget driver tries to queue a > request on ep0 that is a response to an old SETUP, one that has already > been overwritten. I'm not even sure preventing this race would be > possible in your scheme. > > The advantage to invoking the gadget driver's setup callback directly > from the UDC driver's interrupt handler is that the gadget driver will > know immediately when an old SETUP has become stale. (That's what > ep0_req_tag is for in f_mass_storage.) It also provides a concurrency > guarantee, because the driver does not re-enable UDC SETUP interrupts=20 > until the handler is finished. > >> Another detail that this helps is that PM (overall) becomes simpler as, >> most likely, we won't need to mess with transfer cancellation, for >> example. > > System PM on a gadget is always troublesome. Even if the USB=20 > connection is a wakeup source, it may not be possible to guarantee that=20 > the gadget can wake up quickly enough to handle an incoming packet. that's true. >> > You are suggesting that status stage requests should not be queued=20 >> > automatically by UDC drivers but instead queued explicitly by gadget=20 >> > drivers. This would mean changing every UDC driver and every gadget=20 >> > driver. >>=20 >> yes, a bit of work but has been done before. One example that comes to >> mind is when I added ->udc_start() and ->udc_stop(). It's totally >> doable. We can, for instance, add a temporary >> "wants_explicit_ctrl_phases" flag to struct usb_gadget which, if set, >> will tell composite.c (or whatever) that the UDC wants explicitly queued >> ctrl phases. > > The term used in the USB spec is "stage", not "phase". "Phase" refers > to the packets making up a single transaction: token, data, and > handshake. > > Also, data stages are already explicit. So your temporary flag might=20 > better be called "wants_explicit_status_stages". I stand corrected ;-) >> Then add support for that to each UDC and set the flag. Once all are >> converted, add one extra patch to remove the flag and the legacy >> code. This has, of course, the draw back of increasing complexity until >> everything is converted over; but if it's all done in a single series, I >> can't see any problems with that. >>=20 >> > Also, it won't fix the race that Baolin Wang found. The setup routine >>=20 >> well, it will help... see below. >>=20 >> > is always called in interrupt context, so it can't sleep. Doing >> > anything non-trivial will require a separate task, and it's possible >> > that this task will try to enqueue the data-stage or status-stage >> > request before the UDC driver is ready to handle it (for example,=20 >> > before or shortly after the setup routine returns). >> > >> > To work properly, the UDC driver must be able to accept a request for= =20 >> > ep0 any time after it invokes the setup callback -- either before the= =20 >> > callback returns or after. >>=20 >> Right, all UDCs are *already* required to support this case anyway >> because of USB_GADGET_DELAYED_STATUS. There was a bug in DWC3, sure, but >> it was already required to support this case. >>=20 >> By removing USB_GADGET_DELAYED_STATUS altogether and making phases more >> explict, we enforce this requirement and it'll be much easier to test >> for it IMO. > > Okay, I can see the point of requiring explicit status requests.=20=20 > Implementing it will be a little tricky, because right now some status=20 > requests already are explicit (those for length-0 OUT transfers) while=20 > others are implicit. exactly. And that's source of issues for every new UDC driver we get. > (One possible approach would be to have the setup routine return=20 > different values for explicit and implicit status stages -- for=20 > example, return 1 if it wants to submit an explicit status request.=20=20 > That wouldn't be very different from the current=20 > USB_GADGET_DELAYED_STATUS approach.) not really, no. The idea was for composite.c and/or functions to support both methods (temporarily) and use "gadget->wants_explicit_stages" to explicitly queue DATA and STATUS. That would mean that f_mass_storage wouldn't have to return DELAYED_STATUS if (gadget->wants_explicit_stages). After all UDCs are converted over and set wants_explicit_stages (which should all be done in a single series), then we get rid of the flag and the older method of DELAYED_STATUS. > On the other hand, I am very doubtful about requiring explicit setup=20 > requests. right, me too ;-) =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAliF77UACgkQzL64meEa mQb8TA//fsH6hcZB3m6/2bI7sh8YoZky67Mcc4+9iUnsAAWx+W51Yh8MQz22dlxk qIikgFxSFiUaGz10BF/fe/TyvPWWaPs2wF3ocVsMVAFHfaoz+zCPjiDaPrQeQFqQ cb1czPIXSUyBW1ABN+KAt8MrxfxZHEon3o8VtFrWs5l0e1/psp9ceh5gM4orEw0K rdLZPKNzWQ5vZvoEjfc+zdjQ3TPchjBzQzvohf8xx44bOjg13nQjrBaMO3V2amLe YXtkU3XfoqV1KXmfJfgEtZW4nNNDCfwaG+WHyTFd6sS5ZDl/Vzm0Z9Y7cmlB6iAs XrH1PGSZMpBdcVU9qmOOsUZzaoO9qgvdPIfxEExYEegT4nNfbzCFlUinXOaOmj7/ SK3b0VqmpkUPGuPeETUf5zKr5WzDX9PMl0b2ic+WSTtuOANI9pyWQ3KeyUuOmG+5 KDL8BeBUsIyZC0SeefmrFfTEcll+ge4G6cVcLcbCRoY0oKgUZNXoF8XUNjaVpSba 2SQiBIJqsDsDqnsiVRZWERoH7F18wxGAVORywA5vSjsiTP8jaBuBZMBIXdqrTwu/ Kn48TfJHosS+PvoXzwSOW1ZFJ2kUnDRsMhvzpZ/olZ/OTQggJuemsOVT3b2kyCGX FYosr2n1OJKt+g9jEcRYpwUPrr5Z3QfXGyXu49YJV9fJyiP+OKU= =TjXP -----END PGP SIGNATURE----- --=-=-=--