Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 13 Dec 2018 22:47:54 -0500
From:   Paul Elder <paul.elder@ideasonboard.com>
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Felipe Balbi <balbi@kernel.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Bin Liu <b-liu@ti.com>, 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
Message-ID: <20181214034754.GB7477@garnet.amanokami.net>
References: <87r2fxtlrj.fsf@linux.intel.com>
 <Pine.LNX.4.44L0.1811071049370.1542-100000@iolanthe.rowland.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Pine.LNX.4.44L0.1811071049370.1542-100000@iolanthe.rowland.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

[snip]

> > >> > Another thing we should do is give function drivers a way to send a
> > >> > STALL response for the status stage.  Currently there's no way to do
> > >> > it, if a data stage is present.
> > >> 
> > >> Status stage can only be stalled if host tries to move data on the wrong
> > >> direction.
> > >
> > > The USB-2 spec disagrees.  See Table 8-7 in section 8.5.3.1 and the
> > > following paragraphs.  (Although, I can't see why a function would ever
> > > fail to complete the command sequence for a control-IN transfer after
> > > the data had already been sent.)
> > 
> > I can't see how we could ever STALL after returning the data requested
> > by the host. Seems like that wasn't well thought out.
> 
> Yes, it doesn't make a lot of sense.  However, STALLing the status
> stage of a control-OUT transfer does make sense, so we should be able
> to do it.  The obvious approach is to call ep0's set_halt() method
> instead of submitting an explicit status request.

Exactly, that's what we want to be able to do.

> > > Checking the length isn't enough.  A data stage can have 0 length.
> > 
> > apologies, I meant wLength, like so:
> > 
> > 	len = le16_to_cpu(ctrl->wLength);
> > 	if (!len) {
> > 		dwc->three_stage_setup = false;
> > 		dwc->ep0_expect_in = false;
> > 		dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
> > 	} else {
> > 		dwc->three_stage_setup = true;
> > 		dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN);
> > 		dwc->ep0_next_event = DWC3_EP0_NRDY_DATA;
> > 	}
> 
> Presumably you invert the value of ep0_expect_in and set ep0_next_event
> to DWC3_EP0_NRDY_STATUS when the next (data-stage) request is submitted
> for ep0.  Okay.
> 
> > 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?
> 
> Agreed.
> 
> > >> > There's also the fact that requests can specify a completion handler, but only 
> > >> > the data stage request would see its completion handler called (unless we 
> > >> > require UDCs to call completion requests at the completion of the status 
> > >> > stage, but I'm not sure that all UDCs can report the event to the driver, and 
> > >> > that would likely be useless as nobody needs that feature).
> > >> 
> > >> 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 
> > > drivers really need to know whether the status stage was processed by 
> > > the host.  Can you point out any examples where such information would 
> > > 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.
> 
> That doesn't seem to make sense, because in reality you don't have
> this guarantee.  Consider the following scenario: The host starts a
> control-IN transfer.  You send the data-stage request succesfully and
> then submit the status-stage request.  That request will complete
> before the host receives the ACK for its 0-length status OUT
> transaction.  In fact, the host may never receive that ACK and so the
> transfer may never complete.
> 
> Besides, you don't need a signal (clear or otherwise) to prepare for
> more control transfers.  You should start preparing as soon as the
> status-stage request has been submitted.  At that point, what else is
> there for you to do?
> 
> For that matter, you should be prepared to receive a new setup callback 
> at any time.  The host doesn't have to wait for an old control transfer 
> to complete before starting a new one.
> 
> I just don't see any value in knowing the completion code of a
> status-stage request.

I agree.

> > >> > To simplify function drivers, do you think the above proposal of adding a flag 
> > >> > to the (data stage) request to request an automatic transition to the status 
> > >> > stage is a good idea ? We could even possibly invert the logic and transition 
> > >> 
> > >> 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 
> > > completion callback for control requests).  It could even be 
> > > implemented by a library routine; the UDC driver would simply have to 
> > > call this routine immediately after invoking the callback.
> > 
> > I don't follow what you mean here.
> 
> Suppose we have a core library routine like this:
> 
> void usb_gadget_control_complete(struct usb_gadget *gadget,
> 		unsigned int no_implicit_status, int status)
> {
> 	struct usb_request *req;
> 
> 	if (no_implicit_status || status != 0)
> 		return;
> 
> 	/* Send an implicit status-stage request for ep0 */
> 	req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
> 	if (req) {
> 		req->length = 0;
> 		req->no_implicit_status = 1;
> 		req->complete = /* req's deallocation routine */
> 		usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
> 	}
> }
> 
> Then all a UDC driver would need to do is call 
> usb_gadget_control_complete() after invoking a control request's 
> completion handler.  The no_implicit_status and status arguments would 
> be taken from the request that was just completed.
> 
> With this one call added to each UDC, all the existing function drivers
> would work correctly.  Even though they don't explicitly queue
> status-stage requests, the new routine will do so for them,
> transparently.  Function drivers that want to handle their own
> status-stage requests explicitly will merely have to set the
> req->no_implicit_status bit.

I think this is a good idea. We still get the benefits of explicit
status stage without being overly intrusive in the conversion, and we
maintain the queue-based API.

Would it be fine for me to proceed in this direction for a v2?

> (We might or might not need to watch out for 0-length control-OUT 
> transfers.  Function drivers _do_ queue status-stage requests for 
> those.)

Thanks,

Paul Elder