From: Alan Stern <stern@rowland.harvard.edu>
To: Felipe Balbi <balbi@kernel.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Paul Elder <paul.elder@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
Date: Tue, 6 Nov 2018 09:51:06 -0500 (EST) [thread overview]
Message-ID: <Pine.LNX.4.44L0.1811060939160.1450-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <87d0riv4jw.fsf@linux.intel.com>
On Tue, 6 Nov 2018, Felipe Balbi wrote:
> 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
differently depending on whether the usb_ep_queue call occurs within
the setup callback or after that callback returns. They _shouldn't_,
but they might.
> 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
track of whether a request queued on ep0 should be in the IN or the OUT
direction. (Maybe they have to do this already, I don't know.)
> > request and the UDC would then need to check whether that request corresponds
> > to a status stage and process it accordingly. A new operation specific to this
>
> no, it wouldn't. UDC would have to check the size of request, that's
> all:
>
> if (r->length == 0)
> special_zlp_handling();
> else
> regular_non_zlp_handling();
Checking the length isn't enough. A data stage can have 0 length.
> 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.
> > 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?
> One way to satisfy what you want, with what I want is to have UDC core
> implement something like below:
>
> int usb_ep_start_status_stage(struct usb_gadget *g)
> {
> return usb_ep_queue(g->ep0, &g->ep0_status_request);
> }
>
> special function for you, usb_ep_queue() for me :-p
Sure, this is one of the options Laurent and I have discussed.
> >> (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.)
>
> Host would stall first in that case.
I don't follow. Suppose the host sends a SETUP packet for an IN
transfer, but the gadget takes so long to send the IN data back that
the host times out. So then the host sends a SETUP packet for a new
transfer. No stalls.
(Besides, hosts never send STALL packets anyway. Only peripherals do.)
> Driver is already required to
> handle stalls for several other conditions. If thehre are bugs in that
> area, I'd prefer catching them.
> > 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.
Alan Stern
next prev parent reply other threads:[~2018-11-06 14:51 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-10 2:48 [PATCH 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request Paul Elder
2018-10-10 2:48 ` [PATCH 1/6] usb: uvc: include videodev2.h in g_uvc.h Paul Elder
2018-10-10 13:42 ` Laurent Pinchart
2018-10-10 2:48 ` [PATCH 2/6] usb: gadget: uvc: enqueue usb request in setup handler for control OUT Paul Elder
2018-10-10 2:49 ` [PATCH 3/6] usb: gadget: uvc: package setup and data for control OUT requests Paul Elder
2018-10-10 2:49 ` [PATCH 4/6] usb: gadget: add functions to signal udc driver to delay status stage Paul Elder
2018-10-11 16:10 ` Bin Liu
2018-10-17 23:45 ` Laurent Pinchart
2018-10-18 12:46 ` Bin Liu
2018-10-18 14:07 ` Alan Stern
2018-11-01 23:40 ` Paul Elder
2018-11-02 12:44 ` Laurent Pinchart
[not found] ` <87h8gzy5y7.fsf@linux.intel.com>
2018-11-02 14:36 ` Laurent Pinchart
2018-11-02 16:18 ` Alan Stern
2018-11-02 17:10 ` Laurent Pinchart
2018-11-02 19:46 ` Alan Stern
2018-11-06 11:24 ` Felipe Balbi
2018-11-06 15:01 ` Alan Stern
2018-11-07 6:53 ` Felipe Balbi
2018-11-06 11:17 ` Felipe Balbi
2018-11-06 14:51 ` Alan Stern [this message]
2018-11-07 7:00 ` Felipe Balbi
2018-11-07 16:23 ` Alan Stern
2018-12-14 3:47 ` Paul Elder
2018-12-14 15:35 ` Alan Stern
2018-10-10 2:49 ` [PATCH 5/6] usb: musb: gadget: implement send_response Paul Elder
2018-10-11 16:07 ` Bin Liu
2018-10-31 23:26 ` Paul Elder
2018-10-10 2:49 ` [PATCH 6/6] usb: gadget: uvc: allow ioctl to send response in status stage Paul Elder
2018-10-10 12:57 ` [PATCH 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request Laurent Pinchart
2018-10-11 19:31 ` Bin Liu
2018-10-17 23:42 ` Laurent Pinchart
2018-10-18 12:40 ` Bin Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.44L0.1811060939160.1450-100000@iolanthe.rowland.org \
--to=stern@rowland.harvard.edu \
--cc=b-liu@ti.com \
--cc=balbi@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=kieran.bingham@ideasonboard.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=paul.elder@ideasonboard.com \
--cc=rogerq@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).