From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751828AbdCBKvU (ORCPT ); Thu, 2 Mar 2017 05:51:20 -0500 Received: from mga02.intel.com ([134.134.136.20]:65061 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751136AbdCBKtJ (ORCPT ); Thu, 2 Mar 2017 05:49:09 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,230,1484035200"; d="asc'?scan'208";a="70751617" From: Felipe Balbi To: Baolin Wang , Alan Stern 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: Date: Thu, 02 Mar 2017 12:48:15 +0200 Message-ID: <87fuiwt0nk.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: >>> >>>> (One possible approach would be to have the setup routine return >>> >>>> different values for explicit and implicit status stages -- for >>> >>>> example, return 1 if it wants to submit an explicit status request. >>> >>>> That wouldn't be very different from the current >>> >>>> USB_GADGET_DELAYED_STATUS approach.) >>> >>> >>> >>> not really, no. The idea was for composite.c and/or functions to su= pport >>> >>> both methods (temporarily) and use "gadget->wants_explicit_stages" = to >>> >>> explicitly queue DATA and STATUS. That would mean that f_mass_stora= ge >>> >>> wouldn't have to return DELAYED_STATUS if >>> >>> (gadget->wants_explicit_stages). >>> >>> >>> >>> After all UDCs are converted over and set wants_explicit_stages (wh= ich >>> >>> should all be done in a single series), then we get rid of the flag= and >>> >>> the older method of DELAYED_STATUS. >>> >> >>> >> (Sorry for late reply due to my holiday) >>> >> I also met the problem pointed by Alan, from my test, I still want to >>> >> need one return value to indicate if it wants to submit an explicit >>> >> status request. Think about the Control-IN with a data stage, we can >>> >> not get the STATUS phase request from usb_ep_queue() call, and we ne= ed >>> > >>> > why not? wLength tells you that this is a 3-stage transfer. Gadget >>> > driver should be able to figure out that it needs to usb_ep_queue() >>> > another request for status stage. >>> >>> I tried again, but still can not work. Suppose the no-data control: >>> (1) SET_ADDRESS request: function driver will not queue one request >>> for status phase by usb_ep_queue() call. >> >> Function drivers do not handle Set-Address requests at all. The UDC >> driver handles these requests without telling the gadget driver about >> them. > > Correct. What I mean is it will not queue one request for status phase > by usb_ep_queue() call, UDC driver will do that. how the UDC driver handles this case, is up to the UDC driver. In DWC3 I chose to rely on the same ep_queue mechanism; but that's an arbitrary choice. >>> (2) SET_CONFIGURATION request: function driver will queue one 0-length >>> request for status phase by usb_ep_queue() call, especially for >>> mass_storage driver, it will queue one request for status phase >>> later. >>> >>> So I am not sure how the Gadget driver can figure out that it needs to >>> usb_ep_queue() another request for status stage when handling the >>> no-data control? >> >> Gadget drivers already queue status-stage requests for no-data >> control-OUT requests. The difficulty comes when you want to handle an >> IN request or an OUT request with a data stage. >> > > I try to explain that explicitly, In dwc3 driver, we can handle the > STATUS phase request in 2 places: (1) in usb_ep_queue(), (2) in > dwc3_ep0_xfernotready() this is the very detail that what I proposed will change. After what I proposed is implemented, status stage will *always* be done in response to a usb_ep_queue(). > For no-data control-OUT requests: > (1) SET_ADDRESS request: no request queued for status phase by > usb_ep_queue(), dwc3 driver need handle the STATUS phase request when > one not-ready-event comes in dwc3_ep0_xfernotready() function. or we change dwc3 to prepare an internal request and queue it to its own enpdoint. > (2) SET_CONFIGURATION request: function driver will queue one 0-length > request for status phase by usb_ep_queue(), but we can handle this > request in usb_ep_queue() or dwc3_ep0_xfernotready(). When the for DWC3, status stage *must* be done after XFER_NOT_READY event. That's required by the databook. What you're claiming is not correct. The only situation where we start status stage from usb_ep_queue() is for the case when XFER_NOT_READY already triggered and we set PENDING_REQUEST flag for the endpoint. > function driver queued one 0-length request for status phase before > the not-ready-event comes, we need handle this request in > dwc3_ep0_xfernotready() when the not-ready-event comes. When the > function driver queued one 0-length request for status phase after the > not-ready-event comes, we can handle this request in usb_ep_queue(). already implemented. Nothing will change for this case. > So in dwc3_ep0_xfernotready(), we need to check if the request for > status phase has been queued into pending request list, but if the > pending request list is NULL, which means the function driver have not > queued one 0-length request until now (then we can handle it in > usb_ep_queue()), or situation (1) (no request queued for status > phase), then I can not identify this 2 situations to determine where I > can handle the status request. Hope I make it clear. this is already implemented. There's nothing new coming to this case. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAli3+G8ACgkQzL64meEa mQbmCxAAky/TJEDPIWm8FPU+NTIn7A86pA5Z4xk0ETbGdj0Hgg1ozprt8bImMkCw nlEz2ISEH99cvjiJGrTTGCgq6tDOfjNAbPIxpiXMDmItjDYtto9fI6C619EuGXfr sIWboI+d6hg6tkFHnuJcUUNbWf8HMM+3teBjtxMcAEP92Hh1B/5MIuIPFdAccumq /WFR5sm/gX2lUcwzJ4HD3L1l6r30lY3rO47uqF2QfJRk5iBzxaxvlESbK+H44GEq 3KyZKMFLiB86jUEiH/PszHrA2mCqtjWNDQFYKZO23JgpLLAv1JBOq/mnuM/kORqZ y4YCW5eHwqLXVGh6/5ncq1Han+j0ahZIeZawDsFwxsDDJKwJ6J1o8YPilbGgGeHx tnvVmJvlJ22IlXisLrz+6vJT3imgFFLyfwgXUZ7mLE8n7TBWuEWYYMB8G6NwQv0i rCUlpNv8hgMuUL7nIBRaJ2aecJIMyEFcb8A2Xadf7EBUVVQ+yNPVMN98g/Tgoj1/ wo4n1KyqZTY5tG2T5h9RK5PNFkeFo8ewf1vpSOR5JsvuoMjaIQG2I/NS8yu4Nptj wYAbKJMSMlFE+gY9a/qH/dtWWMHMQb2tsoVzSDYpTkaK0HiraN12Ye2Zk1UexEDv 1TRkhXFnWwE4vvqmNG7m+qSARSlRJBkHOIRGrcowDPVi1NZfduY= =0Fu8 -----END PGP SIGNATURE----- --=-=-=--