[v5,0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request

Message

Paul Elder Jan. 9, 2019, 7:08 a.m. UTC

This patch series adds a mechanism to allow asynchronously validating
the data stage of a control OUT request, and for stalling or suceeding
the request accordingly. This mechanism is implemented for MUSB, and is
used by UVC. At the same time, UVC packages the setup stage and data
stage data together to send to userspace to save on a pair of context
switches per control out request.

This patch series does change the userspace API. We however believe that
it is justified because the current API is broken, and because it isn't
being used (because it's broken).

The current API is broken such that it is subject to race conditions
that cause fatal errors with a high frequency. This is actually what
motivated this patch series in the first place. In the current API, not
only is there no way to asynchronously validate the data stage of a
control OUT request, but an empty buffer is expected to be provided to
hold the data stage data -- which is more likely than not to be late.
There is even a warning in musb_g_ep0_queue:

/* else for sequence #2 (OUT), caller provides a buffer
 * before the next packet arrives.  deferred responses
 * (after SETUP is acked) are racey.
 */

This problem has never been reported in years, which is a sign that the
API isn't used. Furthermore, the vendor kernels that we have seen using
the UVC gadget driver (such as QC and Huawei) are heavily patched with
local changes to the API. This corroborates the suspicion that the
current mainline API is not being used.

Additionally, this API isn't meant to be used by generic applications,
but by a dedicated userspace helper. uvc-gadget is one such example, but
it has bitrotten and isn't compatible with the current kernel API. The
fact that nobody has submitted patches nor complained for a long time
again shows that it isn't being used.

The conclusion is that since the API hasn't been used for a long time,
it is safe to fix it.

Changes in v5:

- Change parameter of usb_gadget_control_complete to simply take a
  usb_request
- Make usb_gadget_control_complete do nothing if the request has no
  length (ie. no data stage)
- musb: call usb_gadget_control_complete before
  usb_gadget_giveback_request
- set musb ep0 state to statusin in ep0_send_ack
- make sure to not double-write musb register in ep0_rxstate, since
  musb_g_ep0_giveback will take care of writing them

Changes in v4:

- Change wording and fix typo in patch 4/6 "usb: gadget: add mechanism
  to specify an explicit status stage"
- Set explicit_status in usb_gadget_control_complete
- Change explicit_status from unsigned int to bool

Changes in v3:

- Function driver send STALL status stage by simply calling
  usb_ep_set_halt, and ACK by enqueueing request
- Fix signature of usb_gadget_control_complete to check the status of the
  request that was just given back.
- Corresponding changes to musb and uvc gadget

Changes in v2:

Overhaul of status stage delay mechanism/API. Now if a function driver
desires an explicit/delayed status stage, it specifies so in a flag in
the usb_request that is queued for the data stage. The function driver
later enqueues another usb_request for the status stage, also with the
explicit_status flag set, and with the zero flag acting as the status.
If a function driver does not desire an explicit status stage, then it
can set (or ignore) the explicit_status flag in the usb_request that
is queued for the data stage.

To allow the optional explicit status stage, a UDC driver should call
the newly added usb_gadget_control_complete right after
usb_gadget_giveback_request, and in its queue function should check if
the usb_request is for the status stage and if it has been requested to
be explicit, and if so check the status that should be sent. (See 5/6
"usb: musb: gadget: implement optional explicit status stage" for an
implementation for MUSB)

Paul Elder (6):
  usb: uvc: include videodev2.h in g_uvc.h
  usb: gadget: uvc: enqueue usb request in setup handler for control OUT
  usb: gadget: uvc: package setup and data for control OUT requests
  usb: gadget: add mechanism to specify an explicit status stage
  usb: musb: gadget: implement optional explicit status stage
  usb: gadget: uvc: allow ioctl to send response in status stage

 drivers/usb/gadget/function/f_uvc.c    | 32 ++++++++++++++++++------
 drivers/usb/gadget/function/uvc.h      |  1 +
 drivers/usb/gadget/function/uvc_v4l2.c | 18 ++++++++++++++
 drivers/usb/gadget/udc/core.c          | 34 ++++++++++++++++++++++++++
 drivers/usb/musb/musb_gadget.c         |  2 ++
 drivers/usb/musb/musb_gadget_ep0.c     | 29 ++++++++++++++++++++--
 include/linux/usb/gadget.h             | 10 ++++++++
 include/uapi/linux/usb/g_uvc.h         |  4 ++-
 8 files changed, 119 insertions(+), 11 deletions(-)

Comments

Alan Stern Jan. 10, 2019, 8:39 p.m. UTC | #1

On Wed, 9 Jan 2019, Paul Elder wrote:

> This patch series adds a mechanism to allow asynchronously validating
> the data stage of a control OUT request, and for stalling or suceeding
> the request accordingly.

One thing we haven't mentioned explicitly: What should happen when the 
time for the status stage rolls around if the gadget driver queues a 
non-zero length request?

This can happen in a few different ways.  One obvious possibility is
that the gadget driver sets the explicit_status flag and then submits a
non-zero length request.  Another is that the gadget driver submits
_two_ requests during the data stage (the second would be interpreted
as the status-stage request).  A third is that the gadget driver
submits a data-stage request that is too long and the excess portion is
used for the status stage.

My feeling is that the behavior in these cases should officially be
undefined.  Almost anything could happen: the status stage could STALL,
it could succeed, it could NAK, or it could send a non-zero packet to
the host.  The request could return with 0 status or an error status,
and req->actual could take on any reasonable value.

Alternatively, the UDC driver could detect these errors and report them 
somehow.  Maybe STALL the status stage and complete the request with 
-EPIPE status or some such thing.

Any preferences or other ideas?

One other thing: Some UDC drivers may assume that the data stage of a 
control transfer never spans more than a single usb_request.  Should 
this become an official requirement?

Alan Stern

Paul Elder Jan. 11, 2019, 8:43 a.m. UTC | #2

On Thu, Jan 10, 2019 at 03:39:25PM -0500, Alan Stern wrote:
> On Wed, 9 Jan 2019, Paul Elder wrote:
> 
> > This patch series adds a mechanism to allow asynchronously validating
> > the data stage of a control OUT request, and for stalling or suceeding
> > the request accordingly.
> 
> One thing we haven't mentioned explicitly: What should happen when the 
> time for the status stage rolls around if the gadget driver queues a 
> non-zero length request?

Ah, yeah, I missed that.

> This can happen in a few different ways.  One obvious possibility is
> that the gadget driver sets the explicit_status flag and then submits a
> non-zero length request.  Another is that the gadget driver submits
> _two_ requests during the data stage (the second would be interpreted
> as the status-stage request).  A third is that the gadget driver
> submits a data-stage request that is too long and the excess portion is
> used for the status stage.
> 
> My feeling is that the behavior in these cases should officially be
> undefined.  Almost anything could happen: the status stage could STALL,
> it could succeed, it could NAK, or it could send a non-zero packet to
> the host.  The request could return with 0 status or an error status,
> and req->actual could take on any reasonable value.
> 
> Alternatively, the UDC driver could detect these errors and report them 
> somehow.  Maybe STALL the status stage and complete the request with 
> -EPIPE status or some such thing.
> 
> Any preferences or other ideas?

I think error detection and reporting would be useful. The question is
what action to take after that; either leave it undefined or STALL. I
think STALL would be fine, since if a non-zero length request is
submitted for a status stage, intentionally or not, it isn't part of
proper behavior and should count as an error.

> One other thing: Some UDC drivers may assume that the data stage of a 
> control transfer never spans more than a single usb_request.  Should 
> this become an official requirement?

Would the data stage of a control transfer ever need more space than a
single usb_request can contain? I know UVC doesn't; that's why we pack
it together with the setup stage data in 3/6. If so, I would think we
can make it a requirement.


Paul

Alan Stern Jan. 11, 2019, 6:32 p.m. UTC | #3

On Fri, 11 Jan 2019, Paul Elder wrote:

> On Thu, Jan 10, 2019 at 03:39:25PM -0500, Alan Stern wrote:
> > On Wed, 9 Jan 2019, Paul Elder wrote:
> > 
> > > This patch series adds a mechanism to allow asynchronously validating
> > > the data stage of a control OUT request, and for stalling or suceeding
> > > the request accordingly.
> > 
> > One thing we haven't mentioned explicitly: What should happen when the 
> > time for the status stage rolls around if the gadget driver queues a 
> > non-zero length request?
> 
> Ah, yeah, I missed that.
> 
> > This can happen in a few different ways.  One obvious possibility is
> > that the gadget driver sets the explicit_status flag and then submits a
> > non-zero length request.  Another is that the gadget driver submits
> > _two_ requests during the data stage (the second would be interpreted
> > as the status-stage request).  A third is that the gadget driver
> > submits a data-stage request that is too long and the excess portion is
> > used for the status stage.
> > 
> > My feeling is that the behavior in these cases should officially be
> > undefined.  Almost anything could happen: the status stage could STALL,
> > it could succeed, it could NAK, or it could send a non-zero packet to
> > the host.  The request could return with 0 status or an error status,
> > and req->actual could take on any reasonable value.
> > 
> > Alternatively, the UDC driver could detect these errors and report them 
> > somehow.  Maybe STALL the status stage and complete the request with 
> > -EPIPE status or some such thing.
> > 
> > Any preferences or other ideas?
> 
> I think error detection and reporting would be useful. The question is
> what action to take after that; either leave it undefined or STALL. I
> think STALL would be fine, since if a non-zero length request is
> submitted for a status stage, intentionally or not, it isn't part of
> proper behavior and should count as an error.

Okay; I will have to change the code in dummy-hcd to do this.  You
might need to update musb as well.

> > One other thing: Some UDC drivers may assume that the data stage of a 
> > control transfer never spans more than a single usb_request.  Should 
> > this become an official requirement?
> 
> Would the data stage of a control transfer ever need more space than a
> single usb_request can contain? I know UVC doesn't; that's why we pack
> it together with the setup stage data in 3/6. If so, I would think we
> can make it a requirement.

The data stage of a control transfer cannot be larger than 64 KB.  
Certainly a single usb_request can handle that; the question concerns
whether a function driver might want to split the data up among several
different requests just for convenience.

Felipe, any thoughts?

Alan Stern