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: [v5,4/6] usb: gadget: add mechanism to specify an explicit status stage From: Paul Elder Message-Id: <20190124024816.GG7331@localhost.localdomain> Date: Wed, 23 Jan 2019 21:48:16 -0500 To: Alan Stern Cc: laurent.pinchart@ideasonboard.com, kieran.bingham@ideasonboard.com, b-liu@ti.com, rogerq@ti.com, balbi@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-ID: T24gV2VkLCBKYW4gMjMsIDIwMTkgYXQgMDQ6MTA6MTJQTSAtMDUwMCwgQWxhbiBTdGVybiB3cm90 ZToKPiBPbiBNb24sIDE0IEphbiAyMDE5LCBQYXVsIEVsZGVyIHdyb3RlOgo+IAo+ID4gT24gRnJp LCBKYW4gMTEsIDIwMTkgYXQgMTA6NTA6MTFBTSAtMDUwMCwgQWxhbiBTdGVybiB3cm90ZToKPiA+ ID4gT24gRnJpLCAxMSBKYW4gMjAxOSwgUGF1bCBFbGRlciB3cm90ZToKPiA+ID4gCj4gPiA+ID4g T24gV2VkLCBKYW4gMDksIDIwMTkgYXQgMDI6MDY6MzFQTSAtMDUwMCwgQWxhbiBTdGVybiB3cm90 ZToKPiA+ID4gPiA+IE9uIFdlZCwgOSBKYW4gMjAxOSwgUGF1bCBFbGRlciB3cm90ZToKPiA+ID4g PiA+IAo+ID4gPiA+ID4gPiBBIHVzYiBnYWRnZXQgZnVuY3Rpb24gZHJpdmVyIG1heSBvciBtYXkg bm90IHdhbnQgdG8gZGVsYXkgdGhlIHN0YXR1cwo+ID4gPiA+ID4gPiBzdGFnZSBvZiBhIGNvbnRy b2wgT1VUIHJlcXVlc3QuIEFuIGluc3RhbmNlIHdoZXJlIGl0IG1pZ2h0IHdhbnQgdG8gaXMgdG8K PiA+ID4gPiA+ID4gYXN5bmNocm9ub3VzbHkgdmFsaWRhdGUgdGhlIGRhdGEgb2YgYSBjbGFzcy1z cGVjaWZpYyByZXF1ZXN0Lgo+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gQSBmdW5jdGlvbiBkcml2 ZXIgdGhhdCB3YW50cyBhbiBleHBsaWNpdCBzdGF0dXMgc3RhZ2Ugc2hvdWxkIHNldCB0aGUKPiA+ ID4gPiA+ID4gbmV3bHkgYWRkZWQgZXhwbGljaXRfc3RhdHVzIGZsYWcgb2YgdGhlIHVzYl9yZXF1 ZXN0IGNvcnJlc3BvbmRpbmcgdG8gdGhlCj4gPiA+ID4gPiA+IGRhdGEgc3RhZ2UuIExhdGVyIG9u LCB0aGUgZnVuY3Rpb24gZHJpdmVyIGNhbiBleHBsaWNpdGx5IGNvbXBsZXRlIHRoZQo+ID4gPiA+ ID4gPiBzdGF0dXMgc3RhZ2UgYnkgZW5xdWV1ZWluZyBhIHVzYl9yZXF1ZXN0IGZvciBBQ0ssIG9y IGNhbGxpbmcKPiA+ID4gPiA+ID4gdXNiX2VwX3NldF9oYWx0KCkgZm9yIFNUQUxMLgo+ID4gPiA+ ID4gPiAKPiA+ID4gPiA+ID4gVG8gc3VwcG9ydCBib3RoIGV4cGxpY2l0IGFuZCBpbXBsaWNpdCBz dGF0dXMgc3RhZ2VzLCBhIFVEQyBkcml2ZXIgbXVzdAo+ID4gPiA+ID4gPiBjYWxsIHRoZSBuZXds eSBhZGRlZCB1c2JfZ2FkZ2V0X2NvbnRyb2xfY29tcGxldGUgZnVuY3Rpb24gcmlnaHQgYmVmb3Jl Cj4gPiA+ID4gPiA+IGNhbGxpbmcgdXNiX2dhZGdldF9naXZlYmFja19yZXF1ZXN0LiBUbyBzdXBw b3J0IHRoZSBleHBsaWNpdCBzdGF0dXMKPiA+ID4gPiA+ID4gc3RhZ2UsIGl0IG1pZ2h0IHRoZW4g Y2hlY2sgd2hhdCBzdGFnZSB0aGUgdXNiX3JlcXVlc3Qgd2FzIHF1ZXVlZCBpbiwgYW5kCj4gPiA+ ID4gPiA+IGZvciBjb250cm9sIElOIEFDSyB0aGUgaG9zdCdzIHplcm8tbGVuZ3RoIGRhdGEgcGFj a2V0LCBvciBmb3IgY29udHJvbAo+ID4gPiA+ID4gPiBPVVQgc2VuZCBhIHplcm8tbGVuZ3RoIERB VEExIEFDSyBwYWNrZXQuCj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiBTaWduZWQtb2ZmLWJ5OiBQ YXVsIEVsZGVyIDxwYXVsLmVsZGVyQGlkZWFzb25ib2FyZC5jb20+Cj4gPiA+ID4gPiA+IHY0IEFj a2VkLWJ5OiBBbGFuIFN0ZXJuIDxzdGVybkByb3dsYW5kLmhhcnZhcmQuZWR1Pgo+ID4gPiA+ID4g PiB2MSBSZXZpZXdlZC1ieTogTGF1cmVudCBQaW5jaGFydCA8bGF1cmVudC5waW5jaGFydEBpZGVh c29uYm9hcmQuY29tPgo+ID4gPiA+ID4gCj4gPiA+ID4gPiBUaGlzIGxvb2tzIGdvb2QgYW5kIGhh cyBwYXNzZWQgbXkgdGVzdHMgc28gZmFyLgo+ID4gPiA+IAo+ID4gPiA+IEdvb2QhIFRoYW5rIHlv dSA6KQo+ID4gPiA+IAo+ID4gPiA+ID4gQ2FuIHlvdSBjaGVjayB5b3VyIHV2Ywo+ID4gPiA+ID4g Y2hhbmdlcyB1c2luZyBkdW1teV9oY2Qgd2l0aCB0aGUgcGF0Y2ggYmVsb3c/Cj4gPiA+ID4gCj4g PiA+ID4gSSdtIG5vdCBzdXJlIHdoYXQgdG8gbWFrZSBvZiB0aGUgdGVzdCByZXN1bHRzLiBJIGdl dCB0aGUgc2FtZSByZXN1bHRzCj4gPiA+ID4gd2l0aCBvciB3aXRob3V0IHRoZSBwYXRjaC4gV2hp Y2ggSSBndWVzcyBtYWtlcyBzZW5zZS4uLiBpbiBkdW1teV9xdWV1ZSwKPiA+ID4gPiB0aGlzIGlz IGdldHRpbmcgaGl0IHdoZW4gdGhlIHV2YyBmdW5jdGlvbiBkcml2ZXIgdHJpZXMgdG8gY29tcGxl dGUgdGhlCj4gPiA+ID4gZGVsYXllZCBzdGF0dXM6Cj4gPiA+ID4gCj4gPiA+ID4gCXJlcSA9IHVz Yl9yZXF1ZXN0X3RvX2R1bW15X3JlcXVlc3QoX3JlcSk7Cj4gPiA+ID4gCWlmICghX3JlcSB8fCAh bGlzdF9lbXB0eSgmcmVxLT5xdWV1ZSkgfHwgIV9yZXEtPmNvbXBsZXRlKQo+ID4gPiA+IAkJcmV0 dXJuIC1FSU5WQUw7Cj4gPiA+ID4gCj4gPiA+ID4gU28gdGhlIGRlbGF5ZWQvZXhwbGljaXQgc3Rh dHVzIHN0YWdlIGlzIG5ldmVyIGNvbXBsZXRlZCwgYWZhaWN0Lgo+ID4gPiAKPiA+ID4gSSBwcmVz dW1lIHlvdSBhcmUgaGl0dGluZyB0aGUgIWxpc3RfZW1wdHkoJnJlcS0+cXVldWUpIHRlc3QsIHll cz8gIFRoZSAKPiA+ID4gb3RoZXIgdHdvIHRlc3RzIGFyZSB0cml2aWFsLgo+ID4gCj4gPiBZZXMs IHRoYXQgaXMgd2hhdCdzIGhhcHBlbmluZy4KPiA+IAo+ID4gPiBUcmlnZ2VyaW5nIHRoZSAhbGlz dF9lbXB0eSgpIHRlc3QgbWVhbnMgdGhlIHJlcXVlc3QgaGFzIGFscmVhZHkgYmVlbgo+ID4gPiBz dWJtaXR0ZWQgYW5kIG5vdCB5ZXQgY29tcGxldGVkLiAgVGhpcyBwcm9iYWJseSBpbmRpY2F0ZXMg dGhlcmUgaXMgYQo+ID4gPiBidWcgaW4gdGhlIHV2YyBmdW5jdGlvbiBkcml2ZXIgY29kZS4KPiA+ IAo+ID4gVGhlIHV2YyBmdW5jdGlvbiBkcml2ZXIgd29ya3Mgd2l0aCBtdXNiLCB0aG91Z2ggOi8K PiAKPiBEaWQgeW91IGV2ZXIgZmlndXJlIG91dCB0aGUgcmVhc29uIGZvciB0aGUgIiFsaXN0X2Vt cHR5KCZyZXEtPnF1ZXVlKSIgCj4gZXJyb3Igd2l0aCBkdW1teV9oY2Q/ICBXYXMgaXQgcmVsYXRl ZCB0byB0aGUgY29uZnVzaW9uIGFib3V0IGNvbXBsZXRpb24gCj4gY2FsbGJhY2tzIGZvciBzdGF0 dXMgcmVxdWVzdHM/CgpZZWFoLCBJJ20gcHJldHR5IHN1cmUgdGhhdCdzIHdoYXQgd2FzIGhhcHBl bmluZy4gV2l0aCB3aGF0IEkgcHJldmlvdXNseQpoYWQgdGhlIHV2YyBmdW5jdGlvbiBkcml2ZXIg d2Fzbid0IGV4cGVjdGluZyBhIGNvbXBsZXRpb24gY2FsbGJhY2sgZm9yCnRoZSBzdGF0dXMgc3Rh Z2UgYnV0IHRoZSBPVVQgcmVxdWVzdCBmbGFnIHdhcyBzZXQgc28gaXQganVzdCBrZXB0CnNlbmRp bmcgdGhlIGRhdGEgc3RhZ2UgZGF0YSB0byB1c2Vyc3BhY2UgYW5kIHVzZXJzcGFjZSBrZXB0IGNh bGxpbmcgdGhlCmlvY3RsIHRvIHNlbmQgdGhlIHN0YXR1cyBzdGFnZSB3aGljaCBrZXB0IGNhbGxp bmcgdGhlIGNvbXBsZXRpb24KY2FsbGJhY2sgYW5kIHNvIG9uLCB1bnRpbCB0aGUgZHVtbXlfaGNk IHRpbWVyIHRyaWdnZXJlZCBhbmQgdGhlIG5leHQKcmVxdWVzdCBjYW1lIGluLgoKPiBJbnRlcmVz dGluZyBuZXcgcXVlc3Rpb246IEhvdyBkb2VzIHlvdXIgY29kZSBpbiBtdXNiIHRlbGwgdGhlCj4g ZGlmZmVyZW5jZSBiZXR3ZWVuIGEgMC1sZW5ndGggZGF0YS1zdGFnZSByZXBseSB0byBhIGNvbnRy b2wtSU4KPiB0cmFuc2ZlciwgYW5kIGEgc3RhdHVzLXN0YWdlIHJlcXVlc3Q/ICBCb3RoIHdvdWxk IGFwcGVhciB0byB0aGUgVURDCj4gZHJpdmVyIGFzIDAtbGVuZ3RoIHJlcXVlc3Qgc3VibWlzc2lv bnMgZm9yIGVwMC4KPiBEbyB5b3UgZXhwbGljaXRseSBrZWVwIHRyYWNrIG9mIHdoZXRoZXIgdGhl IGRhdGEgc3RhZ2UgaXMgcGVuZGluZz8KCm11c2IgaGFzIGEgc3RhdGUgbWFjaGluZSB0byBrZWVw IHRyYWNrIG9mIHdoaWNoIHN0YWdlIGl0J3MgaW4sIHNvIEkKanVzdCBjb3VudCB3aGF0ZXZlciBp cyBxdWV1ZWQgZHVyaW5nIHRoZSBzdGF0dXMtSU4gc3RhZ2UgYXMgYQpzdGF0dXMtc3RhZ2UgcmVx dWVzdCBmb3IgY29udHJvbCBPVVQgcmVxdWVzdHMuIE5vdyB0aGF0IHlvdSBtZW50aW9uIGl0LApJ IGRvbid0IGFjdHVhbGx5IGNoZWNrIHRoYXQgdGhlIHF1ZXVlZCByZXF1ZXN0J3MgbGVuZ3RoIGlz IHplcm8gdGhlcmUuLi4KZ290dGEgZml4IHRoYXQuCgoKUGF1bAo= 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=-5.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,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 DC100C282C0 for ; Thu, 24 Jan 2019 02:48:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9B7192184B for ; Thu, 24 Jan 2019 02:48:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="SG6mzxld" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727181AbfAXCs1 (ORCPT ); Wed, 23 Jan 2019 21:48:27 -0500 Received: from perceval.ideasonboard.com ([213.167.242.64]:52726 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726783AbfAXCs1 (ORCPT ); Wed, 23 Jan 2019 21:48:27 -0500 Received: from localhost.localdomain (unknown [96.44.9.117]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id EEE8B23D; Thu, 24 Jan 2019 03:48:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1548298104; bh=3h+uxe2sAA9BCJmU9gFmlyspGVcHAmaKH4MwAt4++Ms=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=SG6mzxld8CtthHEL7dSRRg2ZlmAsRvpix0AeMLEPtPPossxmhgs4PeuzI1ibyDDvk ixG+L8EXrmdMDwoj7J2/RmqJ4gvZ6uTlhPga9CTWvPGAGX+IMIlPeDoz5aAnEWaGB7 t2aWAvzoVMZT8nvwWEUkqV/s47149RSGPAf9aBpg= Date: Wed, 23 Jan 2019 21:48:16 -0500 From: Paul Elder To: Alan Stern Cc: laurent.pinchart@ideasonboard.com, kieran.bingham@ideasonboard.com, b-liu@ti.com, rogerq@ti.com, balbi@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage Message-ID: <20190124024816.GG7331@localhost.localdomain> References: <20190114051113.GD32268@localhost.localdomain> 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 On Wed, Jan 23, 2019 at 04:10:12PM -0500, Alan Stern wrote: > On Mon, 14 Jan 2019, Paul Elder wrote: > > > On Fri, Jan 11, 2019 at 10:50:11AM -0500, Alan Stern wrote: > > > On Fri, 11 Jan 2019, Paul Elder wrote: > > > > > > > On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote: > > > > > On Wed, 9 Jan 2019, 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 where it might want to is to > > > > > > asynchronously validate the data of a class-specific request. > > > > > > > > > > > > A function driver that wants an explicit status stage should set the > > > > > > newly added explicit_status flag of the usb_request corresponding to the > > > > > > data stage. Later on, the function driver can explicitly complete the > > > > > > status stage by enqueueing a usb_request for ACK, or calling > > > > > > usb_ep_set_halt() for STALL. > > > > > > > > > > > > To support both explicit and implicit status stages, a UDC driver must > > > > > > call the newly added usb_gadget_control_complete function right before > > > > > > calling usb_gadget_giveback_request. To support the explicit status > > > > > > stage, it might then check what stage the usb_request was queued in, and > > > > > > for control IN ACK the host's zero-length data packet, or for control > > > > > > OUT send a zero-length DATA1 ACK packet. > > > > > > > > > > > > Signed-off-by: Paul Elder > > > > > > v4 Acked-by: Alan Stern > > > > > > v1 Reviewed-by: Laurent Pinchart > > > > > > > > > > This looks good and has passed my tests so far. > > > > > > > > Good! Thank you :) > > > > > > > > > Can you check your uvc > > > > > changes using dummy_hcd with the patch below? > > > > > > > > I'm not sure what to make of the test results. I get the same results > > > > with or without the patch. Which I guess makes sense... in dummy_queue, > > > > this is getting hit when the uvc function driver tries to complete the > > > > delayed status: > > > > > > > > req = usb_request_to_dummy_request(_req); > > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > > return -EINVAL; > > > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > > > other two tests are trivial. > > > > Yes, that is what's happening. > > > > > Triggering the !list_empty() test means the request has already been > > > submitted and not yet completed. This probably indicates there is a > > > bug in the uvc function driver code. > > > > The uvc function driver works with musb, though :/ > > Did you ever figure out the reason for the "!list_empty(&req->queue)" > error with dummy_hcd? Was it related to the confusion about completion > callbacks for status requests? Yeah, I'm pretty sure that's what was happening. With what I previously had the uvc function driver wasn't expecting a completion callback for the status stage but the OUT request flag was set so it just kept sending the data stage data to userspace and userspace kept calling the ioctl to send the status stage which kept calling the completion callback and so on, until the dummy_hcd timer triggered and the next request came in. > Interesting new question: How does your code in musb tell the > difference between a 0-length data-stage reply to a control-IN > transfer, and a status-stage request? Both would appear to the UDC > driver as 0-length request submissions for ep0. > Do you explicitly keep track of whether the data stage is pending? musb has a state machine to keep track of which stage it's in, so I just count whatever is queued during the status-IN stage as a status-stage request for control OUT requests. Now that you mention it, I don't actually check that the queued request's length is zero there... gotta fix that. Paul