Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage

[Paul Elder]

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 <paul.elder@xxxxxxxxxxxxxxxx>
> > > > > > v4 Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > > > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > > > 
> > > > > 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