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=-8.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT 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 07FF6C6786F for ; Thu, 1 Nov 2018 23:41:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B4F582251D for ; Thu, 1 Nov 2018 23:41:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B4F582251D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com 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 S1728269AbeKBIqV (ORCPT ); Fri, 2 Nov 2018 04:46:21 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:54120 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728060AbeKBIqU (ORCPT ); Fri, 2 Nov 2018 04:46:20 -0400 Received: from garnet.amanokami.net (unknown [96.44.9.229]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 813211A4A; Fri, 2 Nov 2018 00:41:12 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1541115673; bh=c7Xl9cAuWIBbVuqkkxCabtuFhwmP221sT7XlHdAm59Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=d8QvzOKmY6ST+Jf7Ha3YbEMhGe7i1OiFSG+DE5vbb3Tl7OOIPqSXOmAsaJfnZLh9Q K4bTbfOtkaK4cMfgeC6JW1H0Xz3Ayc/bxGm6wXPkyNo3as7weF/l55Vtp7QXH1n+VJ SheOw6zsSX7brJbzj+XprFfTDHTtmq2ZOwoGL+Eg= Date: Thu, 1 Nov 2018 19:40:59 -0400 From: Paul Elder To: Alan Stern Cc: Laurent Pinchart , Bin Liu , kieran.bingham@ideasonboard.com, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, balbi@kernel.org, rogerq@ti.com Subject: Re: [PATCH 4/6] usb: gadget: add functions to signal udc driver to delay status stage Message-ID: <20181101234059.GA28068@garnet.amanokami.net> References: <3055233.Lvy0NcWSZ5@avalon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alan, On Thu, Oct 18, 2018 at 10:07:36AM -0400, Alan Stern wrote: > On Thu, 18 Oct 2018, Laurent Pinchart wrote: > > > Hi Bin, > > > > On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote: > > > On Tue, Oct 09, 2018 at 10:49:01PM -0400, Paul Elder wrote: > > > > A usb gadget function driver may or may not want to delay the status > > > > stage of a control OUT request. An instance it might want to is to > > > > asynchronously validate the data of a class-specific request. > > > > > > > > Add a function usb_ep_delay_status to allow function drivers to choose > > > > to delay the status stage in the request completion handler. The UDC > > > > should then check the usb_ep->delayed_status flag and act accordingly to > > > > delay the status stage. > > > > > > > > Also add a function usb_ep_send_response as a wrapper for > > > > usb_ep->ops->send_response, whose prototype is added as well. This > > > > function should be called by function drivers to tell the UDC what to > > > > reply in the status stage that it has requested to be delayed. > > > > > > > > Signed-off-by: Paul Elder > > > > Reviewed-by: Laurent Pinchart > > > > --- > > > > > > > > drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++ > > > > include/linux/usb/gadget.h | 11 +++++++++++ > > > > 2 files changed, 46 insertions(+) > > > > > > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > > > > index af88b48c1cea..1ec5ce6b43cd 100644 > > > > --- a/drivers/usb/gadget/udc/core.c > > > > +++ b/drivers/usb/gadget/udc/core.c > > > > @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep) > > > > } > > > > EXPORT_SYMBOL_GPL(usb_ep_fifo_flush); > > > > > > > > +/** > > > > + * usb_ep_ep_delay_status - set delay_status flag > > > > + * @ep: the endpoint whose delay_status flag is being set > > > > + * > > > > + * This function instructs the UDC to delay the status stage of a control > > > > + * request. It can only be called from the request completion handler of > > > > a > > > > + * control request. > > > > + */ > > > > +void usb_ep_delay_status(struct usb_ep *ep) > > > > +{ > > > > + ep->delayed_status = true; > > > > +} > > > > +EXPORT_SYMBOL_GPL(usb_ep_delay_status); > > > > > > Is usb_ep_set_delay_status() better? I thought it implies get/return > > > action if a verb is missing in the function name. > > > > For what it's worth, I understand the function name as "delay the status > > stage", with "delay" being a verb. Maybe the short description could be > > updated accordingly. > > Is there a reason for adding a new function for this? This is exactly > what the USB_GADGET_DELAYED_STATUS return value from the setup callback > is meant for (and it is already used by some gadget drivers). In theory, we might be able to use USB_GADGET_DELAYED_STATUS for this. However, there are a few ambiguities that prevent us from doing so. First of all, we want to delay only the status stage for control OUT requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for delaying the "data/status stages". Does this mean that it delays the status stage only or does it delay both stages? If the slash means "and", then we cannot use USB_GADGET_DELAYED_STATUS. Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey, which has already been observed in the UVC gadget driver previously [0]. The raceiness stems from the fact that things can happen in between returning USB_GADGET_DELAYED_STATUS and the composite layer reacting to it - especially if usb_composite_setup_continue is called within that window it causes a WARN. In any case, the fact that the mechanism itself is racey suggests that it needs improvement, and using it wouldn't be a good solution in this case. > Is it a question of when the gadget driver learns that it will need to > delay the status stage? If that's the case, Not really. > why not always return > USB_GADGET_DELAYED_STATUS from the setup callback? Then instead of > calling usb_ep_delay_status() when a delay is needed, you could queue > the status request when a delay isn't needed. Theoretically this might work, but see the problems mentioned above. > As a more general solution, Felipe has said that a UDC driver should > _never_ carry out the status stage transaction until the gadget driver > has told it to do so. Then there would be no need for any sort of > delay indicator. Yeah, but, > (But implementing this would require significant > changes to a bunch of different drivers...) exactly :/ [0] https://www.spinics.net/lists/linux-usb/msg169208.html Paul 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: Paul Elder Message-Id: <20181101234059.GA28068@garnet.amanokami.net> Date: Thu, 1 Nov 2018 19:40:59 -0400 To: Alan Stern Cc: Laurent Pinchart , Bin Liu , kieran.bingham@ideasonboard.com, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, balbi@kernel.org, rogerq@ti.com List-ID: SGkgQWxhbiwKCk9uIFRodSwgT2N0IDE4LCAyMDE4IGF0IDEwOjA3OjM2QU0gLTA0MDAsIEFsYW4g U3Rlcm4gd3JvdGU6Cj4gT24gVGh1LCAxOCBPY3QgMjAxOCwgTGF1cmVudCBQaW5jaGFydCB3cm90 ZToKPiAKPiA+IEhpIEJpbiwKPiA+IAo+ID4gT24gVGh1cnNkYXksIDExIE9jdG9iZXIgMjAxOCAx OToxMDoyMSBFRVNUIEJpbiBMaXUgd3JvdGU6Cj4gPiA+IE9uIFR1ZSwgT2N0IDA5LCAyMDE4IGF0 IDEwOjQ5OjAxUE0gLTA0MDAsIFBhdWwgRWxkZXIgd3JvdGU6Cj4gPiA+ID4gQSB1c2IgZ2FkZ2V0 IGZ1bmN0aW9uIGRyaXZlciBtYXkgb3IgbWF5IG5vdCB3YW50IHRvIGRlbGF5IHRoZSBzdGF0dXMK PiA+ID4gPiBzdGFnZSBvZiBhIGNvbnRyb2wgT1VUIHJlcXVlc3QuIEFuIGluc3RhbmNlIGl0IG1p Z2h0IHdhbnQgdG8gaXMgdG8KPiA+ID4gPiBhc3luY2hyb25vdXNseSB2YWxpZGF0ZSB0aGUgZGF0 YSBvZiBhIGNsYXNzLXNwZWNpZmljIHJlcXVlc3QuCj4gPiA+ID4gCj4gPiA+ID4gQWRkIGEgZnVu Y3Rpb24gdXNiX2VwX2RlbGF5X3N0YXR1cyB0byBhbGxvdyBmdW5jdGlvbiBkcml2ZXJzIHRvIGNo b29zZQo+ID4gPiA+IHRvIGRlbGF5IHRoZSBzdGF0dXMgc3RhZ2UgaW4gdGhlIHJlcXVlc3QgY29t cGxldGlvbiBoYW5kbGVyLiBUaGUgVURDCj4gPiA+ID4gc2hvdWxkIHRoZW4gY2hlY2sgdGhlIHVz Yl9lcC0+ZGVsYXllZF9zdGF0dXMgZmxhZyBhbmQgYWN0IGFjY29yZGluZ2x5IHRvCj4gPiA+ID4g ZGVsYXkgdGhlIHN0YXR1cyBzdGFnZS4KPiA+ID4gPiAKPiA+ID4gPiBBbHNvIGFkZCBhIGZ1bmN0 aW9uIHVzYl9lcF9zZW5kX3Jlc3BvbnNlIGFzIGEgd3JhcHBlciBmb3IKPiA+ID4gPiB1c2JfZXAt Pm9wcy0+c2VuZF9yZXNwb25zZSwgd2hvc2UgcHJvdG90eXBlIGlzIGFkZGVkIGFzIHdlbGwuIFRo aXMKPiA+ID4gPiBmdW5jdGlvbiBzaG91bGQgYmUgY2FsbGVkIGJ5IGZ1bmN0aW9uIGRyaXZlcnMg dG8gdGVsbCB0aGUgVURDIHdoYXQgdG8KPiA+ID4gPiByZXBseSBpbiB0aGUgc3RhdHVzIHN0YWdl IHRoYXQgaXQgaGFzIHJlcXVlc3RlZCB0byBiZSBkZWxheWVkLgo+ID4gPiA+IAo+ID4gPiA+IFNp Z25lZC1vZmYtYnk6IFBhdWwgRWxkZXIgPHBhdWwuZWxkZXJAaWRlYXNvbmJvYXJkLmNvbT4KPiA+ ID4gPiBSZXZpZXdlZC1ieTogTGF1cmVudCBQaW5jaGFydCA8bGF1cmVudC5waW5jaGFydEBpZGVh c29uYm9hcmQuY29tPgo+ID4gPiA+IC0tLQo+ID4gPiA+IAo+ID4gPiA+ICBkcml2ZXJzL3VzYi9n YWRnZXQvdWRjL2NvcmUuYyB8IDM1ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr Cj4gPiA+ID4gIGluY2x1ZGUvbGludXgvdXNiL2dhZGdldC5oICAgIHwgMTEgKysrKysrKysrKysK PiA+ID4gPiAgMiBmaWxlcyBjaGFuZ2VkLCA0NiBpbnNlcnRpb25zKCspCj4gPiA+ID4gCj4gPiA+ ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvdXNiL2dhZGdldC91ZGMvY29yZS5jIGIvZHJpdmVycy91 c2IvZ2FkZ2V0L3VkYy9jb3JlLmMKPiA+ID4gPiBpbmRleCBhZjg4YjQ4YzFjZWEuLjFlYzVjZTZi NDNjZCAxMDA2NDQKPiA+ID4gPiAtLS0gYS9kcml2ZXJzL3VzYi9nYWRnZXQvdWRjL2NvcmUuYwo+ ID4gPiA+ICsrKyBiL2RyaXZlcnMvdXNiL2dhZGdldC91ZGMvY29yZS5jCj4gPiA+ID4gQEAgLTQ0 Myw2ICs0NDMsNDEgQEAgdm9pZCB1c2JfZXBfZmlmb19mbHVzaChzdHJ1Y3QgdXNiX2VwICplcCkK PiA+ID4gPiAgfQo+ID4gPiA+ICBFWFBPUlRfU1lNQk9MX0dQTCh1c2JfZXBfZmlmb19mbHVzaCk7 Cj4gPiA+ID4gCj4gPiA+ID4gKy8qKgo+ID4gPiA+ICsgKiB1c2JfZXBfZXBfZGVsYXlfc3RhdHVz IC0gc2V0IGRlbGF5X3N0YXR1cyBmbGFnCj4gPiA+ID4gKyAqIEBlcDogdGhlIGVuZHBvaW50IHdo b3NlIGRlbGF5X3N0YXR1cyBmbGFnIGlzIGJlaW5nIHNldAo+ID4gPiA+ICsgKgo+ID4gPiA+ICsg KiBUaGlzIGZ1bmN0aW9uIGluc3RydWN0cyB0aGUgVURDIHRvIGRlbGF5IHRoZSBzdGF0dXMgc3Rh Z2Ugb2YgYSBjb250cm9sCj4gPiA+ID4gKyAqIHJlcXVlc3QuIEl0IGNhbiBvbmx5IGJlIGNhbGxl ZCBmcm9tIHRoZSByZXF1ZXN0IGNvbXBsZXRpb24gaGFuZGxlciBvZgo+ID4gPiA+IGEKPiA+ID4g PiArICogY29udHJvbCByZXF1ZXN0Lgo+ID4gPiA+ICsgKi8KPiA+ID4gPiArdm9pZCB1c2JfZXBf ZGVsYXlfc3RhdHVzKHN0cnVjdCB1c2JfZXAgKmVwKQo+ID4gPiA+ICt7Cj4gPiA+ID4gKwllcC0+ ZGVsYXllZF9zdGF0dXMgPSB0cnVlOwo+ID4gPiA+ICt9Cj4gPiA+ID4gK0VYUE9SVF9TWU1CT0xf R1BMKHVzYl9lcF9kZWxheV9zdGF0dXMpOwo+ID4gPiAKPiA+ID4gSXMgdXNiX2VwX3NldF9kZWxh eV9zdGF0dXMoKSBiZXR0ZXI/IEkgdGhvdWdodCBpdCBpbXBsaWVzIGdldC9yZXR1cm4KPiA+ID4g YWN0aW9uIGlmIGEgdmVyYiBpcyBtaXNzaW5nIGluIHRoZSBmdW5jdGlvbiBuYW1lLgo+ID4gCj4g PiBGb3Igd2hhdCBpdCdzIHdvcnRoLCBJIHVuZGVyc3RhbmQgdGhlIGZ1bmN0aW9uIG5hbWUgYXMg ImRlbGF5IHRoZSBzdGF0dXMgCj4gPiBzdGFnZSIsIHdpdGggImRlbGF5IiBiZWluZyBhIHZlcmIu IE1heWJlIHRoZSBzaG9ydCBkZXNjcmlwdGlvbiBjb3VsZCBiZSAKPiA+IHVwZGF0ZWQgYWNjb3Jk aW5nbHkuCj4gCj4gSXMgdGhlcmUgYSByZWFzb24gZm9yIGFkZGluZyBhIG5ldyBmdW5jdGlvbiBm b3IgdGhpcz8gIFRoaXMgaXMgZXhhY3RseQo+IHdoYXQgdGhlIFVTQl9HQURHRVRfREVMQVlFRF9T VEFUVVMgcmV0dXJuIHZhbHVlIGZyb20gdGhlIHNldHVwIGNhbGxiYWNrCj4gaXMgbWVhbnQgZm9y IChhbmQgaXQgaXMgYWxyZWFkeSB1c2VkIGJ5IHNvbWUgZ2FkZ2V0IGRyaXZlcnMpLgoKSW4gdGhl b3J5LCB3ZSBtaWdodCBiZSBhYmxlIHRvIHVzZSBVU0JfR0FER0VUX0RFTEFZRURfU1RBVFVTIGZv ciB0aGlzLgpIb3dldmVyLCB0aGVyZSBhcmUgYSBmZXcgYW1iaWd1aXRpZXMgdGhhdCBwcmV2ZW50 IHVzIGZyb20gZG9pbmcgc28uCgpGaXJzdCBvZiBhbGwsIHdlIHdhbnQgdG8gZGVsYXkgb25seSB0 aGUgc3RhdHVzIHN0YWdlIGZvciBjb250cm9sIE9VVApyZXF1ZXN0czsgYWNjb3JkaW5nIHRvIGNv bXBvc2l0ZS5oLCBVU0JfR0FER0VUX0RFTEFZRURfU1RBVFVTIGlzIGZvcgpkZWxheWluZyB0aGUg ImRhdGEvc3RhdHVzIHN0YWdlcyIuIERvZXMgdGhpcyBtZWFuIHRoYXQgaXQgZGVsYXlzIHRoZQpz dGF0dXMgc3RhZ2Ugb25seSBvciBkb2VzIGl0IGRlbGF5IGJvdGggc3RhZ2VzPyBJZiB0aGUgc2xh c2ggbWVhbnMKImFuZCIsIHRoZW4gd2UgY2Fubm90IHVzZSBVU0JfR0FER0VUX0RFTEFZRURfU1RB VFVTLgoKRnVydGhlcm1vcmUsIHdlIGhhdmUgZm91bmQgdGhhdCBVU0JfR0FER0VUX0RFTEFZRURf U1RBVFVTIGlzIHJhY2V5LAp3aGljaCBoYXMgYWxyZWFkeSBiZWVuIG9ic2VydmVkIGluIHRoZSBV VkMgZ2FkZ2V0IGRyaXZlciBwcmV2aW91c2x5IFswXS4KVGhlIHJhY2VpbmVzcyBzdGVtcyBmcm9t IHRoZSBmYWN0IHRoYXQgdGhpbmdzIGNhbiBoYXBwZW4gaW4gYmV0d2VlbgpyZXR1cm5pbmcgVVNC X0dBREdFVF9ERUxBWUVEX1NUQVRVUyBhbmQgdGhlIGNvbXBvc2l0ZSBsYXllciByZWFjdGluZyB0 bwppdCAtIGVzcGVjaWFsbHkgaWYgdXNiX2NvbXBvc2l0ZV9zZXR1cF9jb250aW51ZSBpcyBjYWxs ZWQgd2l0aGluIHRoYXQKd2luZG93IGl0IGNhdXNlcyBhIFdBUk4uIEluIGFueSBjYXNlLCB0aGUg ZmFjdCB0aGF0IHRoZSBtZWNoYW5pc20gaXRzZWxmCmlzIHJhY2V5IHN1Z2dlc3RzIHRoYXQgaXQg bmVlZHMgaW1wcm92ZW1lbnQsIGFuZCB1c2luZyBpdCB3b3VsZG4ndCBiZSBhCmdvb2Qgc29sdXRp b24gaW4gdGhpcyBjYXNlLgoKPiBJcyBpdCBhIHF1ZXN0aW9uIG9mIHdoZW4gdGhlIGdhZGdldCBk cml2ZXIgbGVhcm5zIHRoYXQgaXQgd2lsbCBuZWVkIHRvCj4gZGVsYXkgdGhlIHN0YXR1cyBzdGFn ZT8gIElmIHRoYXQncyB0aGUgY2FzZSwKCk5vdCByZWFsbHkuCgo+IHdoeSBub3QgYWx3YXlzIHJl dHVybgo+IFVTQl9HQURHRVRfREVMQVlFRF9TVEFUVVMgZnJvbSB0aGUgc2V0dXAgY2FsbGJhY2s/ ICBUaGVuIGluc3RlYWQgb2YKPiBjYWxsaW5nIHVzYl9lcF9kZWxheV9zdGF0dXMoKSB3aGVuIGEg ZGVsYXkgaXMgbmVlZGVkLCB5b3UgY291bGQgcXVldWUKPiB0aGUgc3RhdHVzIHJlcXVlc3Qgd2hl biBhIGRlbGF5IGlzbid0IG5lZWRlZC4KClRoZW9yZXRpY2FsbHkgdGhpcyBtaWdodCB3b3JrLCBi dXQgc2VlIHRoZSBwcm9ibGVtcyBtZW50aW9uZWQgYWJvdmUuCgo+IEFzIGEgbW9yZSBnZW5lcmFs IHNvbHV0aW9uLCBGZWxpcGUgaGFzIHNhaWQgdGhhdCBhIFVEQyBkcml2ZXIgc2hvdWxkIAo+IF9u ZXZlcl8gY2Fycnkgb3V0IHRoZSBzdGF0dXMgc3RhZ2UgdHJhbnNhY3Rpb24gdW50aWwgdGhlIGdh ZGdldCBkcml2ZXIgCj4gaGFzIHRvbGQgaXQgdG8gZG8gc28uICBUaGVuIHRoZXJlIHdvdWxkIGJl IG5vIG5lZWQgZm9yIGFueSBzb3J0IG9mIAo+IGRlbGF5IGluZGljYXRvci4KClllYWgsIGJ1dCwK Cj4gKEJ1dCBpbXBsZW1lbnRpbmcgdGhpcyB3b3VsZCByZXF1aXJlIHNpZ25pZmljYW50IAo+IGNo YW5nZXMgdG8gYSBidW5jaCBvZiBkaWZmZXJlbnQgZHJpdmVycy4uLikKCmV4YWN0bHkgOi8KClsw XSBodHRwczovL3d3dy5zcGluaWNzLm5ldC9saXN0cy9saW51eC11c2IvbXNnMTY5MjA4Lmh0bWwK CgpQYXVsCg==