From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 74F1FC0044C for ; Wed, 7 Nov 2018 07:00:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1D91C20827 for ; Wed, 7 Nov 2018 07:00:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1D91C20827 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728533AbeKGQ3l (ORCPT ); Wed, 7 Nov 2018 11:29:41 -0500 Received: from mga04.intel.com ([192.55.52.120]:11577 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726298AbeKGQ3k (ORCPT ); Wed, 7 Nov 2018 11:29:40 -0500 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Nov 2018 23:00:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,474,1534834800"; d="asc'?scan'208";a="278994200" Received: from pipin.fi.intel.com (HELO localhost) ([10.237.72.128]) by fmsmga006.fm.intel.com with ESMTP; 06 Nov 2018 23:00:36 -0800 From: Felipe Balbi To: Alan Stern Cc: Laurent Pinchart , Paul Elder , Bin Liu , kieran.bingham@ideasonboard.com, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, rogerq@ti.com Subject: Re: [PATCH 4/6] usb: gadget: add functions to signal udc driver to delay status stage In-Reply-To: References: Date: Wed, 07 Nov 2018 09:00:32 +0200 Message-ID: <87r2fxtlrj.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 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Alan Stern writes: >> DATA stage always depends on a usb_ep_queue() from gadget driver. So >> it's always "delayed" in that sense. > > However, it's conceivable that some UDC drivers might behave=20 > differently depending on whether the usb_ep_queue call occurs within=20 > the setup callback or after that callback returns. They _shouldn't_,=20 > but they might. but now we're speculating. Should we really care before we catch regressions? >> it avoids all the special cases. UDC drivers can implement a single >> handling for struct usb_request. We could do away with special return >> values and so on... > > It's not quite so simple, because the UDC driver will need to keep=20 > track of whether a request queued on ep0 should be in the IN or the OUT=20 > direction. (Maybe they have to do this already, I don't know.) UDC drivers already have to do that. >> > request and the UDC would then need to check whether that request corr= esponds=20 >> > to a status stage and process it accordingly. A new operation specific= to this=20 >>=20 >> no, it wouldn't. UDC would have to check the size of request, that's >> all: >>=20 >> if (r->length =3D=3D 0) >> special_zlp_handling(); >> else >> regular_non_zlp_handling(); > > Checking the length isn't enough. A data stage can have 0 length. apologies, I meant wLength, like so: len =3D le16_to_cpu(ctrl->wLength); if (!len) { dwc->three_stage_setup =3D false; dwc->ep0_expect_in =3D false; dwc->ep0_next_event =3D DWC3_EP0_NRDY_STATUS; } else { dwc->three_stage_setup =3D true; dwc->ep0_expect_in =3D !!(ctrl->bRequestType & USB_DIR_IN); dwc->ep0_next_event =3D DWC3_EP0_NRDY_DATA; } >> But we don't need to care about special return values and the like. We >> don't even need to care (from UDC perspective) if we're dealing with >> 2-stage or 3-stage control transfers (well, dwc3 needs to care because >> of different TRB types that needs to be used, but that's another story) > > No, we do need to care because of the direction issue. special return values would be rendered uncessary if there's agreement that status stage is always explicit. Why would need USB_GADGET_DELAYED_STATUS if every case returns that? >> > There's also the fact that requests can specify a completion handler, = but only=20 >> > the data stage request would see its completion handler called (unless= we=20 >> > require UDCs to call completion requests at the completion of the stat= us=20 >> > stage, but I'm not sure that all UDCs can report the event to the driv= er, and=20 >> > that would likely be useless as nobody needs that feature). >>=20 >> you still wanna know if the host actually processed your status >> stage. udc-core can (and should) provide a generic status stage >> completion function which, at a minimum, aids with some tracepoints. > > Helping with tracepoints is fine. However, I don't think function=20 > drivers really need to know whether the status stage was processed by=20 > the host. Can you point out any examples where such information would=20 > be useful? If you know your STATUS stage completed, you have a guarantee that your previous control transfer is complete. It's a very clear signal that you should prepare for more control transfers. >> >> (But it does involve a >> >> race in cases where the host gets tired of waiting and issues another >> >> SETUP packet before the processing of the first transfer is finished.) >>=20 >> Host would stall first in that case. > > I don't follow. Suppose the host sends a SETUP packet for an IN=20 > transfer, but the gadget takes so long to send the IN data back that=20 > the host times out. So then the host sends a SETUP packet for a new=20 > transfer. No stalls. > > (Besides, hosts never send STALL packets anyway. Only peripherals do.) oh okay. This is the setup_packet_pending case. >> > To simplify function drivers, do you think the above proposal of addin= g a flag=20 >> > to the (data stage) request to request an automatic transition to the = status=20 >> > stage is a good idea ? We could even possibly invert the logic and tra= nsition=20 >>=20 >> no, I don't think so. Making the status phase always explicit is far >> better. UDCs won't have to check flags, or act on magic return >> values. It just won't do anything until a request is queued. > > I don't agree. This would be a simple test in a localized area (the=20 > completion callback for control requests). It could even be=20 > implemented by a library routine; the UDC driver would simply have to=20 > call this routine immediately after invoking the callback. I don't follow what you mean here. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlvijZAACgkQzL64meEa mQaAig/9EeGyVRELHHx4RBGS9zypenEkdvs6zDNWpWHddHDyreb2AoVbO77hKmV5 TYvrRokufGmBfMx4VMmkTa43zgLLwAet1G8uOMC/AdK8BA12KSLEsqSZnBF0U843 P8ZtJ0mu4cvt95JMEJ+pMTmLzqjdIV1KFekqyr/5C66RlxHkAp8CB2MuBaDOBRV2 /1YB9Vzx2zUnZTtnsBhFteK968Phk2VPajjMqdZaupsbQ2LhlgcfNFuaaQLJTPW2 hdS8eQfrVcUCG/KvkoXyUQ5bj8X3qJ0NkdBQmKNxJX6oHNOemIpi1amI7nprUhd8 P+sH5w1CAx35qhwmP17oUla5ENFvAcDu9iWBcWXygH0crDu90zBRhkuviJFJbPUe Bj6oIfsDdNDZPazkv0FTjxGCwXCE8xSTbjqE16e3aRAkFfNITM8r5jKBHY96zD74 sNsnX5NrMnqcI7P1IOip+THJDH/dG9X5RrLZjiuV53CjSKiCw0pG8YrEgmGCB23O FsnnbcHbePCObFUhriFvJecKVCzRzXSUCVXb3UlxXt051ej6w+LgsvgOTSJ8/++b 9rMFjMSaWWBkAwPT3Y7tzdiV4oTQbZMCpaKLxsY8cQIQFFpJ5tdNm/oPQ+IYgAcm 5DSRmZna4WyWKA5XpAtfq2vOAy1LKV80F/S1M4bXu5Mh35Zr0ZY= =648u -----END PGP SIGNATURE----- --=-=-=-- From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [4/6] usb: gadget: add functions to signal udc driver to delay status stage From: Felipe Balbi Message-Id: <87r2fxtlrj.fsf@linux.intel.com> Date: Wed, 07 Nov 2018 09:00:32 +0200 To: Alan Stern Cc: Laurent Pinchart , Paul Elder , Bin Liu , kieran.bingham@ideasonboard.com, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, rogerq@ti.com List-ID: SGksCgpBbGFuIFN0ZXJuIDxzdGVybkByb3dsYW5kLmhhcnZhcmQuZWR1PiB3cml0ZXM6Cj4+IERB VEEgc3RhZ2UgYWx3YXlzIGRlcGVuZHMgb24gYSB1c2JfZXBfcXVldWUoKSBmcm9tIGdhZGdldCBk cml2ZXIuIFNvCj4+IGl0J3MgYWx3YXlzICJkZWxheWVkIiBpbiB0aGF0IHNlbnNlLgo+Cj4gSG93 ZXZlciwgaXQncyBjb25jZWl2YWJsZSB0aGF0IHNvbWUgVURDIGRyaXZlcnMgbWlnaHQgYmVoYXZl IAo+IGRpZmZlcmVudGx5IGRlcGVuZGluZyBvbiB3aGV0aGVyIHRoZSB1c2JfZXBfcXVldWUgY2Fs bCBvY2N1cnMgd2l0aGluIAo+IHRoZSBzZXR1cCBjYWxsYmFjayBvciBhZnRlciB0aGF0IGNhbGxi YWNrIHJldHVybnMuICBUaGV5IF9zaG91bGRuJ3RfLCAKPiBidXQgdGhleSBtaWdodC4KCmJ1dCBu b3cgd2UncmUgc3BlY3VsYXRpbmcuIFNob3VsZCB3ZSByZWFsbHkgY2FyZSBiZWZvcmUgd2UgY2F0 Y2gKcmVncmVzc2lvbnM/Cgo+PiBpdCBhdm9pZHMgYWxsIHRoZSBzcGVjaWFsIGNhc2VzLiBVREMg ZHJpdmVycyBjYW4gaW1wbGVtZW50IGEgc2luZ2xlCj4+IGhhbmRsaW5nIGZvciBzdHJ1Y3QgdXNi X3JlcXVlc3QuIFdlIGNvdWxkIGRvIGF3YXkgd2l0aCBzcGVjaWFsIHJldHVybgo+PiB2YWx1ZXMg YW5kIHNvIG9uLi4uCj4KPiBJdCdzIG5vdCBxdWl0ZSBzbyBzaW1wbGUsIGJlY2F1c2UgdGhlIFVE QyBkcml2ZXIgd2lsbCBuZWVkIHRvIGtlZXAgCj4gdHJhY2sgb2Ygd2hldGhlciBhIHJlcXVlc3Qg cXVldWVkIG9uIGVwMCBzaG91bGQgYmUgaW4gdGhlIElOIG9yIHRoZSBPVVQgCj4gZGlyZWN0aW9u LiAgKE1heWJlIHRoZXkgaGF2ZSB0byBkbyB0aGlzIGFscmVhZHksIEkgZG9uJ3Qga25vdy4pCgpV REMgZHJpdmVycyBhbHJlYWR5IGhhdmUgdG8gZG8gdGhhdC4KCj4+ID4gcmVxdWVzdCBhbmQgdGhl IFVEQyB3b3VsZCB0aGVuIG5lZWQgdG8gY2hlY2sgd2hldGhlciB0aGF0IHJlcXVlc3QgY29ycmVz cG9uZHMgCj4+ID4gdG8gYSBzdGF0dXMgc3RhZ2UgYW5kIHByb2Nlc3MgaXQgYWNjb3JkaW5nbHku IEEgbmV3IG9wZXJhdGlvbiBzcGVjaWZpYyB0byB0aGlzIAo+PiAKPj4gbm8sIGl0IHdvdWxkbid0 LiBVREMgd291bGQgaGF2ZSB0byBjaGVjayB0aGUgc2l6ZSBvZiByZXF1ZXN0LCB0aGF0J3MKPj4g YWxsOgo+PiAKPj4gCWlmIChyLT5sZW5ndGggPT0gMCkKPj4gICAgICAgICAJc3BlY2lhbF96bHBf aGFuZGxpbmcoKTsKPj4gCWVsc2UKPj4gICAgICAgICAJcmVndWxhcl9ub25femxwX2hhbmRsaW5n KCk7Cj4KPiBDaGVja2luZyB0aGUgbGVuZ3RoIGlzbid0IGVub3VnaC4gIEEgZGF0YSBzdGFnZSBj YW4gaGF2ZSAwIGxlbmd0aC4KCmFwb2xvZ2llcywgSSBtZWFudCB3TGVuZ3RoLCBsaWtlIHNvOgoK CWxlbiA9IGxlMTZfdG9fY3B1KGN0cmwtPndMZW5ndGgpOwoJaWYgKCFsZW4pIHsKCQlkd2MtPnRo cmVlX3N0YWdlX3NldHVwID0gZmFsc2U7CgkJZHdjLT5lcDBfZXhwZWN0X2luID0gZmFsc2U7CgkJ ZHdjLT5lcDBfbmV4dF9ldmVudCA9IERXQzNfRVAwX05SRFlfU1RBVFVTOwoJfSBlbHNlIHsKCQlk d2MtPnRocmVlX3N0YWdlX3NldHVwID0gdHJ1ZTsKCQlkd2MtPmVwMF9leHBlY3RfaW4gPSAhIShj dHJsLT5iUmVxdWVzdFR5cGUgJiBVU0JfRElSX0lOKTsKCQlkd2MtPmVwMF9uZXh0X2V2ZW50ID0g RFdDM19FUDBfTlJEWV9EQVRBOwoJfQoKPj4gQnV0IHdlIGRvbid0IG5lZWQgdG8gY2FyZSBhYm91 dCBzcGVjaWFsIHJldHVybiB2YWx1ZXMgYW5kIHRoZSBsaWtlLiBXZQo+PiBkb24ndCBldmVuIG5l ZWQgdG8gY2FyZSAoZnJvbSBVREMgcGVyc3BlY3RpdmUpIGlmIHdlJ3JlIGRlYWxpbmcgd2l0aAo+ PiAyLXN0YWdlIG9yIDMtc3RhZ2UgY29udHJvbCB0cmFuc2ZlcnMgKHdlbGwsIGR3YzMgbmVlZHMg dG8gY2FyZSBiZWNhdXNlCj4+IG9mIGRpZmZlcmVudCBUUkIgdHlwZXMgdGhhdCBuZWVkcyB0byBi ZSB1c2VkLCBidXQgdGhhdCdzIGFub3RoZXIgc3RvcnkpCj4KPiBObywgd2UgZG8gbmVlZCB0byBj YXJlIGJlY2F1c2Ugb2YgdGhlIGRpcmVjdGlvbiBpc3N1ZS4KCnNwZWNpYWwgcmV0dXJuIHZhbHVl cyB3b3VsZCBiZSByZW5kZXJlZCB1bmNlc3NhcnkgaWYgdGhlcmUncyBhZ3JlZW1lbnQKdGhhdCBz dGF0dXMgc3RhZ2UgaXMgYWx3YXlzIGV4cGxpY2l0LiBXaHkgd291bGQgbmVlZApVU0JfR0FER0VU X0RFTEFZRURfU1RBVFVTIGlmIGV2ZXJ5IGNhc2UgcmV0dXJucyB0aGF0PwoKPj4gPiBUaGVyZSdz IGFsc28gdGhlIGZhY3QgdGhhdCByZXF1ZXN0cyBjYW4gc3BlY2lmeSBhIGNvbXBsZXRpb24gaGFu ZGxlciwgYnV0IG9ubHkgCj4+ID4gdGhlIGRhdGEgc3RhZ2UgcmVxdWVzdCB3b3VsZCBzZWUgaXRz IGNvbXBsZXRpb24gaGFuZGxlciBjYWxsZWQgKHVubGVzcyB3ZSAKPj4gPiByZXF1aXJlIFVEQ3Mg dG8gY2FsbCBjb21wbGV0aW9uIHJlcXVlc3RzIGF0IHRoZSBjb21wbGV0aW9uIG9mIHRoZSBzdGF0 dXMgCj4+ID4gc3RhZ2UsIGJ1dCBJJ20gbm90IHN1cmUgdGhhdCBhbGwgVURDcyBjYW4gcmVwb3J0 IHRoZSBldmVudCB0byB0aGUgZHJpdmVyLCBhbmQgCj4+ID4gdGhhdCB3b3VsZCBsaWtlbHkgYmUg dXNlbGVzcyBhcyBub2JvZHkgbmVlZHMgdGhhdCBmZWF0dXJlKS4KPj4gCj4+IHlvdSBzdGlsbCB3 YW5uYSBrbm93IGlmIHRoZSBob3N0IGFjdHVhbGx5IHByb2Nlc3NlZCB5b3VyIHN0YXR1cwo+PiBz dGFnZS4gdWRjLWNvcmUgY2FuIChhbmQgc2hvdWxkKSBwcm92aWRlIGEgZ2VuZXJpYyBzdGF0dXMg c3RhZ2UKPj4gY29tcGxldGlvbiBmdW5jdGlvbiB3aGljaCwgYXQgYSBtaW5pbXVtLCBhaWRzIHdp dGggc29tZSB0cmFjZXBvaW50cy4KPgo+IEhlbHBpbmcgd2l0aCB0cmFjZXBvaW50cyBpcyBmaW5l LiAgSG93ZXZlciwgSSBkb24ndCB0aGluayBmdW5jdGlvbiAKPiBkcml2ZXJzIHJlYWxseSBuZWVk IHRvIGtub3cgd2hldGhlciB0aGUgc3RhdHVzIHN0YWdlIHdhcyBwcm9jZXNzZWQgYnkgCj4gdGhl IGhvc3QuICBDYW4geW91IHBvaW50IG91dCBhbnkgZXhhbXBsZXMgd2hlcmUgc3VjaCBpbmZvcm1h dGlvbiB3b3VsZCAKPiBiZSB1c2VmdWw/CgpJZiB5b3Uga25vdyB5b3VyIFNUQVRVUyBzdGFnZSBj b21wbGV0ZWQsIHlvdSBoYXZlIGEgZ3VhcmFudGVlIHRoYXQgeW91cgpwcmV2aW91cyBjb250cm9s IHRyYW5zZmVyIGlzIGNvbXBsZXRlLiBJdCdzIGEgdmVyeSBjbGVhciBzaWduYWwgdGhhdCB5b3UK c2hvdWxkIHByZXBhcmUgZm9yIG1vcmUgY29udHJvbCB0cmFuc2ZlcnMuCgo+PiA+PiAoQnV0IGl0 IGRvZXMgaW52b2x2ZSBhCj4+ID4+IHJhY2UgaW4gY2FzZXMgd2hlcmUgdGhlIGhvc3QgZ2V0cyB0 aXJlZCBvZiB3YWl0aW5nIGFuZCBpc3N1ZXMgYW5vdGhlcgo+PiA+PiBTRVRVUCBwYWNrZXQgYmVm b3JlIHRoZSBwcm9jZXNzaW5nIG9mIHRoZSBmaXJzdCB0cmFuc2ZlciBpcyBmaW5pc2hlZC4pCj4+ IAo+PiBIb3N0IHdvdWxkIHN0YWxsIGZpcnN0IGluIHRoYXQgY2FzZS4KPgo+IEkgZG9uJ3QgZm9s bG93LiAgU3VwcG9zZSB0aGUgaG9zdCBzZW5kcyBhIFNFVFVQIHBhY2tldCBmb3IgYW4gSU4gCj4g dHJhbnNmZXIsIGJ1dCB0aGUgZ2FkZ2V0IHRha2VzIHNvIGxvbmcgdG8gc2VuZCB0aGUgSU4gZGF0 YSBiYWNrIHRoYXQgCj4gdGhlIGhvc3QgdGltZXMgb3V0LiAgU28gdGhlbiB0aGUgaG9zdCBzZW5k cyBhIFNFVFVQIHBhY2tldCBmb3IgYSBuZXcgCj4gdHJhbnNmZXIuICBObyBzdGFsbHMuCj4KPiAo QmVzaWRlcywgaG9zdHMgbmV2ZXIgc2VuZCBTVEFMTCBwYWNrZXRzIGFueXdheS4gIE9ubHkgcGVy aXBoZXJhbHMgZG8uKQoKb2ggb2theS4gVGhpcyBpcyB0aGUgc2V0dXBfcGFja2V0X3BlbmRpbmcg Y2FzZS4KCj4+ID4gVG8gc2ltcGxpZnkgZnVuY3Rpb24gZHJpdmVycywgZG8geW91IHRoaW5rIHRo ZSBhYm92ZSBwcm9wb3NhbCBvZiBhZGRpbmcgYSBmbGFnIAo+PiA+IHRvIHRoZSAoZGF0YSBzdGFn ZSkgcmVxdWVzdCB0byByZXF1ZXN0IGFuIGF1dG9tYXRpYyB0cmFuc2l0aW9uIHRvIHRoZSBzdGF0 dXMgCj4+ID4gc3RhZ2UgaXMgYSBnb29kIGlkZWEgPyBXZSBjb3VsZCBldmVuIHBvc3NpYmx5IGlu dmVydCB0aGUgbG9naWMgYW5kIHRyYW5zaXRpb24gCj4+IAo+PiBubywgSSBkb24ndCB0aGluayBz by4gTWFraW5nIHRoZSBzdGF0dXMgcGhhc2UgYWx3YXlzIGV4cGxpY2l0IGlzIGZhcgo+PiBiZXR0 ZXIuIFVEQ3Mgd29uJ3QgaGF2ZSB0byBjaGVjayBmbGFncywgb3IgYWN0IG9uIG1hZ2ljIHJldHVy bgo+PiB2YWx1ZXMuIEl0IGp1c3Qgd29uJ3QgZG8gYW55dGhpbmcgdW50aWwgYSByZXF1ZXN0IGlz IHF1ZXVlZC4KPgo+IEkgZG9uJ3QgYWdyZWUuICBUaGlzIHdvdWxkIGJlIGEgc2ltcGxlIHRlc3Qg aW4gYSBsb2NhbGl6ZWQgYXJlYSAodGhlIAo+IGNvbXBsZXRpb24gY2FsbGJhY2sgZm9yIGNvbnRy b2wgcmVxdWVzdHMpLiAgSXQgY291bGQgZXZlbiBiZSAKPiBpbXBsZW1lbnRlZCBieSBhIGxpYnJh cnkgcm91dGluZTsgdGhlIFVEQyBkcml2ZXIgd291bGQgc2ltcGx5IGhhdmUgdG8gCj4gY2FsbCB0 aGlzIHJvdXRpbmUgaW1tZWRpYXRlbHkgYWZ0ZXIgaW52b2tpbmcgdGhlIGNhbGxiYWNrLgoKSSBk b24ndCBmb2xsb3cgd2hhdCB5b3UgbWVhbiBoZXJlLgo=