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

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

[Paul Elder]

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(-)

-- 
2.20.1

[PATCH v5 1/6] usb: uvc: include videodev2.h in g_uvc.h

[Paul Elder]

V4L2_EVENT_PRIVATE_START is used in g_uvc.h but is defined in
videodev2.h, which is not included and causes a compiler warning:

linux/usb/g_uvc.h:15:28: error: ‘V4L2_EVENT_PRIVATE_START’ undeclared here (not in a function)
 #define UVC_EVENT_FIRST   (V4L2_EVENT_PRIVATE_START + 0)

Include videodev2.h in g_uvc.h.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
No change from v4
No change from v3
No change from v2
No change from v1

 include/uapi/linux/usb/g_uvc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h
index 3c9ee3020cbb..6698c3263ae8 100644
--- a/include/uapi/linux/usb/g_uvc.h
+++ b/include/uapi/linux/usb/g_uvc.h
@@ -11,6 +11,7 @@
 #include <linux/ioctl.h>
 #include <linux/types.h>
 #include <linux/usb/ch9.h>
+#include <linux/videodev2.h>
 
 #define UVC_EVENT_FIRST			(V4L2_EVENT_PRIVATE_START + 0)
 #define UVC_EVENT_CONNECT		(V4L2_EVENT_PRIVATE_START + 0)
-- 
2.20.1

[PATCH v5 2/6] usb: gadget: uvc: enqueue usb request in setup handler for control OUT

[Paul Elder]

Currently, for uvc class-specific control IN and OUT requests, in the
setup handler a UVC_EVENT_SETUP with the setup control is enqueued to
userspace. In response to this, the uvc function driver expects
userspace to call ioctl UVCIOC_SEND_RESPONSE containing uvc request
data.

In the case of control IN this is fine, but for control OUT it causes a
problem. Since the host sends data immediately after the setup stage
completes, it is possible that the empty uvc request data is not
enqueued in time for the UDC driver to put the data stage data into
(this causes some UDC drivers, such as MUSB, to reply with a STALL).
This problem is remedied by having the setup handler enqueue the empty
uvc request data, instead of waiting for userspace to do it.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
No change from v4
No change from v3
No change from v2
No change from v1

 drivers/usb/gadget/function/f_uvc.c    | 25 +++++++++++++++++++------
 drivers/usb/gadget/function/uvc_v4l2.c |  7 +++++++
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 723ca91e4f2a..36de51ac30ed 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -223,8 +223,6 @@ static int
 uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 {
 	struct uvc_device *uvc = to_uvc(f);
-	struct v4l2_event v4l2_event;
-	struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
 
 	if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
 		uvcg_info(f, "invalid request type\n");
@@ -241,10 +239,25 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 	uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
 	uvc->event_length = le16_to_cpu(ctrl->wLength);
 
-	memset(&v4l2_event, 0, sizeof(v4l2_event));
-	v4l2_event.type = UVC_EVENT_SETUP;
-	memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
-	v4l2_event_queue(&uvc->vdev, &v4l2_event);
+	if (uvc->event_setup_out) {
+		struct usb_request *req = uvc->control_req;
+
+		/*
+		 * Enqueue the request immediately for control OUT as the
+		 * host will start the data stage straight away.
+		 */
+		req->length = uvc->event_length;
+		req->zero = 0;
+		usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
+	} else {
+		struct v4l2_event v4l2_event;
+		struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
+
+		memset(&v4l2_event, 0, sizeof(v4l2_event));
+		v4l2_event.type = UVC_EVENT_SETUP;
+		memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
+		v4l2_event_queue(&uvc->vdev, &v4l2_event);
+	}
 
 	return 0;
 }
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 7816ea9886e1..ac48f49d9f10 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,6 +35,13 @@ uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
 	struct usb_composite_dev *cdev = uvc->func.config->cdev;
 	struct usb_request *req = uvc->control_req;
 
+	/*
+	 * For control OUT transfers the request has been enqueued synchronously
+	 * by the setup handler, there's nothing to be done here.
+	 */
+	if (uvc->event_setup_out)
+		return 0;
+
 	if (data->length < 0)
 		return usb_ep_set_halt(cdev->gadget->ep0);
 
-- 
2.20.1

[PATCH v5 3/6] usb: gadget: uvc: package setup and data for control OUT requests

[Paul Elder]

Since "usb: gadget: uvc: enqueue uvc_request_data in setup handler
for control OUT requests" it is no longer necessary for userspace to
call ioctl UVCIOC_SEND_RESPONSE in response to receiving a
UVC_EVENT_SETUP from the uvc function driver for a control OUT request.

This change means that for control OUT userspace will receive a
UVC_EVENT_SETUP and not do anything with it. This is a waste of a pair
of context switches, so we put the setup and data stage data into a
single UVC_EVENT_DATA to give to userspace. Previously struct
uvc_request_data had 60 bytes allocated for data, and since uvc data at
most is 34 bytes in UVC 1.1 and 48 bytes in UVC 1.5, we can afford to
cut out 8 bytes to store the setup control.

Since the setup control is discarded after the handling of the setup
stage, it must be saved in struct uvc_device during the setup handler in
order for the data stage handler to be able to read it and send it to
userspace.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
No change from v4
No change from v3
No change from v2
No change from v1

 drivers/usb/gadget/function/f_uvc.c | 3 +++
 drivers/usb/gadget/function/uvc.h   | 1 +
 include/uapi/linux/usb/g_uvc.h      | 3 ++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 36de51ac30ed..58ed191f636e 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -215,6 +215,8 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct usb_request *req)
 		v4l2_event.type = UVC_EVENT_DATA;
 		uvc_event->data.length = req->actual;
 		memcpy(&uvc_event->data.data, req->buf, req->actual);
+		memcpy(&uvc_event->data.setup, &uvc->control_setup,
+		       sizeof(uvc_event->data.setup));
 		v4l2_event_queue(&uvc->vdev, &v4l2_event);
 	}
 }
@@ -238,6 +240,7 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 	 */
 	uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
 	uvc->event_length = le16_to_cpu(ctrl->wLength);
+	memcpy(&uvc->control_setup, ctrl, sizeof(uvc->control_setup));
 
 	if (uvc->event_setup_out) {
 		struct usb_request *req = uvc->control_req;
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 671020c8a836..1d89b1df4ba0 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -163,6 +163,7 @@ struct uvc_device {
 	unsigned int control_intf;
 	struct usb_ep *control_ep;
 	struct usb_request *control_req;
+	struct usb_ctrlrequest control_setup;
 	void *control_buf;
 
 	unsigned int streaming_intf;
diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h
index 6698c3263ae8..10fbb4382925 100644
--- a/include/uapi/linux/usb/g_uvc.h
+++ b/include/uapi/linux/usb/g_uvc.h
@@ -24,7 +24,8 @@
 
 struct uvc_request_data {
 	__s32 length;
-	__u8 data[60];
+	struct usb_ctrlrequest setup;
+	__u8 data[52];
 };
 
 struct uvc_event {
-- 
2.20.1

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

[Paul Elder]

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@ideasonboard.com>
v4 Acked-by: Alan Stern <stern@rowland.harvard.edu>
v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes from v4:

- 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)

Changes from v3:

- More specific in commit message about what to do for udc driver acking
- Set explicit_status in usb_gadget_control_complete
- Make explicit_status type bool

Changes from v2:

Add status parameter to usb_gadget_control_complete, so that a
usb_request is not queued if the status of the just given back request
is nonzero.

Changes from v1:

Complete change of API. Now we use a flag that should be set in the
usb_request that is queued for the data stage to signal to the UDC that
we want to delay the status stage (as opposed to setting a flag in the
UDC itself, that persists across all requests). We now also provide a
function for UDC drivers to very easily allow implicit status stages, to
mitigate the need to convert all function drivers to this new API at
once, and to make it easier for UDC drivers to convert.

 drivers/usb/gadget/udc/core.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h    | 10 ++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48c1cea..57b2c2550537 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -894,6 +894,40 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
 /* ------------------------------------------------------------------------- */
 
+/**
+ * usb_gadget_control_complete - complete the status stage of a control
+ *	request, or delay it
+ * Context: in_interrupt()
+ *
+ * @gadget: gadget whose control request's status stage should be completed
+ * @request: usb request whose status stage should be completed
+ *
+ * This is called by device controller drivers before returning the completed
+ * request back to the gadget layer, to either complete or delay the status
+ * stage. It exits without doing anything if the request has a non-zero status,
+ * if it has zero length, or if its explicit_status flag is set.
+ */
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+		struct usb_request *request)
+{
+	struct usb_request *req;
+
+	if (request->explicit_status || request->status || !request->length)
+		return;
+
+	/* Send an implicit status-stage request for ep0 */
+	req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
+	if (req) {
+		req->length = 0;
+		req->explicit_status = 1;
+		req->complete = usb_ep_free_request;
+		usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
+	}
+}
+EXPORT_SYMBOL_GPL(usb_gadget_control_complete);
+
+/* ------------------------------------------------------------------------- */
+
 /**
  * gadget_find_ep_by_name - returns ep whose name is the same as sting passed
  *	in second parameter or NULL if searched endpoint not found
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..bf4f021ce139 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -73,6 +73,7 @@ struct usb_ep;
  *	Note that for writes (IN transfers) some data bytes may still
  *	reside in a device-side FIFO when the request is reported as
  *	complete.
+ * @explicit_status: If true, delays the status stage
  *
  * These are allocated/freed through the endpoint they're used with.  The
  * hardware's driver can add extra per-request data to the memory it returns,
@@ -114,6 +115,8 @@ struct usb_request {
 
 	int			status;
 	unsigned		actual;
+
+	bool			explicit_status;
 };
 
 /*-------------------------------------------------------------------------*/
@@ -850,6 +853,13 @@ extern void usb_gadget_giveback_request(struct usb_ep *ep,
 
 /*-------------------------------------------------------------------------*/
 
+/* utility to complete or delay status stage */
+
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+		struct usb_request *request);
+
+/*-------------------------------------------------------------------------*/
+
 /* utility to find endpoint by name */
 
 extern struct usb_ep *gadget_find_ep_by_name(struct usb_gadget *g,
-- 
2.20.1

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

[Alan Stern]

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@ideasonboard.com>
> v4 Acked-by: Alan Stern <stern@rowland.harvard.edu>
> v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This looks good and has passed my tests so far.  Can you check your uvc
changes using dummy_hcd with the patch below?

Alan Stern



Index: usb-4.x/drivers/usb/gadget/udc/dummy_hcd.c
===================================================================
--- usb-4.x.orig/drivers/usb/gadget/udc/dummy_hcd.c
+++ usb-4.x/drivers/usb/gadget/udc/dummy_hcd.c
@@ -88,6 +88,7 @@ struct dummy_ep {
 	unsigned			wedged:1;
 	unsigned			already_seen:1;
 	unsigned			setup_stage:1;
+	unsigned			status_stage:1;
 	unsigned			stream_en:1;
 };
 
@@ -1037,7 +1038,7 @@ static void init_dummy_udc_hw(struct dum
 		ep->ep.ops = &dummy_ep_ops;
 		list_add_tail(&ep->ep.ep_list, &dum->gadget.ep_list);
 		ep->halted = ep->wedged = ep->already_seen =
-				ep->setup_stage = 0;
+				ep->setup_stage = ep->status_stage = 0;
 		usb_ep_set_maxpacket_limit(&ep->ep, ~0);
 		ep->ep.max_streams = 16;
 		ep->last_io = jiffies;
@@ -1386,6 +1387,7 @@ static int transfer(struct dummy_hcd *du
 	struct dummy		*dum = dum_hcd->dum;
 	struct dummy_request	*req;
 	int			sent = 0;
+	bool			is_ep0 = (ep == &dum->ep[0]);
 
 top:
 	/* if there's no request queued, the device is NAKing; return */
@@ -1407,6 +1409,11 @@ top:
 		 * terminate reads.
 		 */
 		host_len = urb->transfer_buffer_length - urb->actual_length;
+		if (ep->status_stage)
+			host_len = 0;
+		else if (is_ep0 && host_len == 0)
+			ep->status_stage = 1;
+
 		dev_len = req->req.length - req->req.actual;
 		len = min(host_len, dev_len);
 
@@ -1471,6 +1478,12 @@ top:
 					req->req.status = 0;
 			}
 
+			/* Truncated control-IN => start the status stage */
+			if (*status == 0 && is_ep0 && !ep->status_stage) {
+				ep->status_stage = 1;
+				*status = -EINPROGRESS;
+			}
+
 		/*
 		 * many requests terminate without a short packet.
 		 * send a zlp if demanded by flags.
@@ -1486,6 +1499,8 @@ top:
 				if (urb->transfer_flags & URB_ZERO_PACKET &&
 				    !to_host)
 					rescan = 1;
+				else if (is_ep0 && !ep->status_stage)
+					ep->status_stage = rescan = 1;
 				else
 					*status = 0;
 			}
@@ -1496,6 +1511,9 @@ top:
 			list_del_init(&req->queue);
 
 			spin_unlock(&dum->lock);
+			if (is_ep0)
+				usb_gadget_control_complete(&dum->gadget,
+						&req->req);
 			usb_gadget_giveback_request(&ep->ep, &req->req);
 			spin_lock(&dum->lock);
 
@@ -1849,6 +1867,7 @@ restart:
 		ep->already_seen = 1;
 		if (ep == &dum->ep[0] && urb->error_count) {
 			ep->setup_stage = 1;	/* a new urb */
+			ep->status_stage = 0;
 			urb->error_count = 0;
 		}
 		if (ep->halted && !ep->setup_stage) {

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

[Paul Elder]

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@ideasonboard.com>
> > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> 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.

Paul

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

[Alan Stern]

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@ideasonboard.com>
> > > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > 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.

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.

Alan Stern

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

[Paul Elder]

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@ideasonboard.com>
> > > > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > 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 :/

I compared the sequence of calls to the uvc setup, completion handler,
and status stage sending, and for some reason dummy_hcd, after an OUT
setup-completion-status sequence, calls a completion-status-completion
sequence, and then goes on the the next request. musb simply goes on to
the next request after the setup-completion-status sequence.

I commented out the paranoia block in dummy_timer, and dummy_hcd still
does the extra completion, but it doesn't error out anymore. I doubt
that's the/a solution though, especially since I get:

[   22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26).
[   22.624481] uvcvideo: Failed to initialize the device (-5).

Not sure if that's a result of dummy_hcd not supporting isochronous
transfers or not.

I'm not sure where to continue investigating :/


Thanks,

Paul

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

[Alan Stern]

On Mon, 14 Jan 2019, Paul Elder wrote:

> > > > 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 :/
> 
> I compared the sequence of calls to the uvc setup, completion handler,
> and status stage sending, and for some reason dummy_hcd, after an OUT
> setup-completion-status sequence, calls a completion-status-completion
> sequence, and then goes on the the next request. musb simply goes on to
> the next request after the setup-completion-status sequence.

I don't quite understand.  There's a control-OUT transfer, the setup, 
data, and status transactions all complete normally, and then what 
happens?  What do you mean by "a completion-status-completion 
sequence"?  A more detailed description would help.

> I commented out the paranoia block in dummy_timer, and dummy_hcd still
> does the extra completion, but it doesn't error out anymore. I doubt
> that's the/a solution though, especially since I get:
> 
> [   22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26).
> [   22.624481] uvcvideo: Failed to initialize the device (-5).
> 
> Not sure if that's a result of dummy_hcd not supporting isochronous
> transfers or not.
> 
> I'm not sure where to continue investigating :/

Perhaps removing the "#if 0" protecting the dev_dbg line in 
dummy_queue() would provide some helpful output.

Another thing to check would be if the "implement an emulated 
single-request FIFO" in dummy_queue() is causing trouble.  There's no 
harm in replacing the long "if" condition with "if (0)".

Alan Stern

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

[Paul Elder]

On Mon, Jan 14, 2019 at 10:24:44AM -0500, Alan Stern wrote:
> On Mon, 14 Jan 2019, Paul Elder wrote:
> 
> > > > > 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 :/
> > 
> > I compared the sequence of calls to the uvc setup, completion handler,
> > and status stage sending, and for some reason dummy_hcd, after an OUT
> > setup-completion-status sequence, calls a completion-status-completion
> > sequence, and then goes on the the next request. musb simply goes on to
> > the next request after the setup-completion-status sequence.
> 
> I don't quite understand.  There's a control-OUT transfer, the setup, 
> data, and status transactions all complete normally, and then what 
> happens?  What do you mean by "a completion-status-completion 
> sequence"?  A more detailed description would help.
> 

I meant the functions (procedures) in the function driver, so the setup
handler (uvc_function_setup), the completion handler
(uvc_function_ep0_complete), and the status sender (uvc_send_response),
although the last one actually sends the data stage for control IN.
So after the status is sent on the uvc gadget driver's end, its
completion handler is called again without the setup handler being
called beforehand and I cant figure out why.

> > I commented out the paranoia block in dummy_timer, and dummy_hcd still
> > does the extra completion, but it doesn't error out anymore. I doubt
> > that's the/a solution though, especially since I get:
> > 
> > [   22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26).
> > [   22.624481] uvcvideo: Failed to initialize the device (-5).
> > 
> > Not sure if that's a result of dummy_hcd not supporting isochronous
> > transfers or not.
> > 
> > I'm not sure where to continue investigating :/
> 
> Perhaps removing the "#if 0" protecting the dev_dbg line in 
> dummy_queue() would provide some helpful output.

It did, but didn't get me much farther :/

> Another thing to check would be if the "implement an emulated 
> single-request FIFO" in dummy_queue() is causing trouble.  There's no 
> harm in replacing the long "if" condition with "if (0)".

That didn't change anything.

Although I did notice that the dummy_queue that calls the completion
handler without the preceeding setup handler says that it's in the
status stage (ep->status_stage == 1).


Thanks,

Paul

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

[Alan Stern]

On Wed, 16 Jan 2019, Paul Elder wrote:

> On Mon, Jan 14, 2019 at 10:24:44AM -0500, Alan Stern wrote:
> > On Mon, 14 Jan 2019, Paul Elder wrote:
> > 
> > > > > > 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 :/
> > > 
> > > I compared the sequence of calls to the uvc setup, completion handler,
> > > and status stage sending, and for some reason dummy_hcd, after an OUT
> > > setup-completion-status sequence, calls a completion-status-completion
> > > sequence, and then goes on the the next request. musb simply goes on to
> > > the next request after the setup-completion-status sequence.
> > 
> > I don't quite understand.  There's a control-OUT transfer, the setup, 
> > data, and status transactions all complete normally, and then what 
> > happens?  What do you mean by "a completion-status-completion 
> > sequence"?  A more detailed description would help.
> > 
> 
> I meant the functions (procedures) in the function driver, so the setup
> handler (uvc_function_setup), the completion handler
> (uvc_function_ep0_complete), and the status sender (uvc_send_response),
> although the last one actually sends the data stage for control IN.
> So after the status is sent on the uvc gadget driver's end, its
> completion handler is called again without the setup handler being
> called beforehand and I cant figure out why.

Isn't this what you should expect?  Every usb_request, if it is queued
successfully, eventually gets a completion callback.  That promise is
made by every UDC driver; it's part of the gadget API.  So for a
control transfer with a data stage, you expect to have:

	Setup handler called
	Data-stage request submitted
	Data-stage request completion callback
	Status-stage request submitted
	Status-stage request completion callback

Thus, two completion callbacks but only one setup callback.

> > > I commented out the paranoia block in dummy_timer, and dummy_hcd still
> > > does the extra completion, but it doesn't error out anymore. I doubt
> > > that's the/a solution though, especially since I get:
> > > 
> > > [   22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26).
> > > [   22.624481] uvcvideo: Failed to initialize the device (-5).
> > > 
> > > Not sure if that's a result of dummy_hcd not supporting isochronous
> > > transfers or not.
> > > 
> > > I'm not sure where to continue investigating :/
> > 
> > Perhaps removing the "#if 0" protecting the dev_dbg line in 
> > dummy_queue() would provide some helpful output.
> 
> It did, but didn't get me much farther :/
> 
> > Another thing to check would be if the "implement an emulated 
> > single-request FIFO" in dummy_queue() is causing trouble.  There's no 
> > harm in replacing the long "if" condition with "if (0)".
> 
> That didn't change anything.
> 
> Although I did notice that the dummy_queue that calls the completion
> handler without the preceeding setup handler says that it's in the
> status stage (ep->status_stage == 1).

That is consistent with the events outlined above.

Alan Stern

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

[Paul Elder]

On Wed, Jan 16, 2019 at 10:06:53AM -0500, Alan Stern wrote:
> On Wed, 16 Jan 2019, Paul Elder wrote:
> 
> > On Mon, Jan 14, 2019 at 10:24:44AM -0500, Alan Stern wrote:
> > > On Mon, 14 Jan 2019, Paul Elder wrote:
> > > 
> > > > > > > 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 :/
> > > > 
> > > > I compared the sequence of calls to the uvc setup, completion handler,
> > > > and status stage sending, and for some reason dummy_hcd, after an OUT
> > > > setup-completion-status sequence, calls a completion-status-completion
> > > > sequence, and then goes on the the next request. musb simply goes on to
> > > > the next request after the setup-completion-status sequence.
> > > 
> > > I don't quite understand.  There's a control-OUT transfer, the setup, 
> > > data, and status transactions all complete normally, and then what 
> > > happens?  What do you mean by "a completion-status-completion 
> > > sequence"?  A more detailed description would help.
> > > 
> > 
> > I meant the functions (procedures) in the function driver, so the setup
> > handler (uvc_function_setup), the completion handler
> > (uvc_function_ep0_complete), and the status sender (uvc_send_response),
> > although the last one actually sends the data stage for control IN.
> > So after the status is sent on the uvc gadget driver's end, its
> > completion handler is called again without the setup handler being
> > called beforehand and I cant figure out why.
> 
> Isn't this what you should expect?  Every usb_request, if it is queued
> successfully, eventually gets a completion callback.  That promise is
> made by every UDC driver; it's part of the gadget API.  So for a
> control transfer with a data stage, you expect to have:
> 
> 	Setup handler called
> 	Data-stage request submitted
> 	Data-stage request completion callback
> 	Status-stage request submitted
> 	Status-stage request completion callback
> 
> Thus, two completion callbacks but only one setup callback.

omg how did I not notice this :/

I guess I have to fix the uvc function driver so it works with that.
musb doesn't call the status stage completion callback though; not that
it does anything so it seems fine to me, but indeed the function driver
has to be ready for it if it is called.

> > > > I commented out the paranoia block in dummy_timer, and dummy_hcd still
> > > > does the extra completion, but it doesn't error out anymore. I doubt
> > > > that's the/a solution though, especially since I get:
> > > > 
> > > > [   22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26).
> > > > [   22.624481] uvcvideo: Failed to initialize the device (-5).
> > > > 
> > > > Not sure if that's a result of dummy_hcd not supporting isochronous
> > > > transfers or not.
> > > > 
> > > > I'm not sure where to continue investigating :/
> > > 
> > > Perhaps removing the "#if 0" protecting the dev_dbg line in 
> > > dummy_queue() would provide some helpful output.
> > 
> > It did, but didn't get me much farther :/
> > 
> > > Another thing to check would be if the "implement an emulated 
> > > single-request FIFO" in dummy_queue() is causing trouble.  There's no 
> > > harm in replacing the long "if" condition with "if (0)".
> > 
> > That didn't change anything.
> > 
> > Although I did notice that the dummy_queue that calls the completion
> > handler without the preceeding setup handler says that it's in the
> > status stage (ep->status_stage == 1).
> 
> That is consistent with the events outlined above.


Thanks,

Paul

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

[Alan Stern]

On Fri, 18 Jan 2019, Paul Elder wrote:

> > > I meant the functions (procedures) in the function driver, so the setup
> > > handler (uvc_function_setup), the completion handler
> > > (uvc_function_ep0_complete), and the status sender (uvc_send_response),
> > > although the last one actually sends the data stage for control IN.
> > > So after the status is sent on the uvc gadget driver's end, its
> > > completion handler is called again without the setup handler being
> > > called beforehand and I cant figure out why.
> > 
> > Isn't this what you should expect?  Every usb_request, if it is queued
> > successfully, eventually gets a completion callback.  That promise is
> > made by every UDC driver; it's part of the gadget API.  So for a
> > control transfer with a data stage, you expect to have:
> > 
> > 	Setup handler called
> > 	Data-stage request submitted
> > 	Data-stage request completion callback
> > 	Status-stage request submitted
> > 	Status-stage request completion callback
> > 
> > Thus, two completion callbacks but only one setup callback.
> 
> omg how did I not notice this :/
> 
> I guess I have to fix the uvc function driver so it works with that.
> musb doesn't call the status stage completion callback though; not that
> it does anything so it seems fine to me, but indeed the function driver
> has to be ready for it if it is called.

musb _has_ to call the status-stage completion callback.  As just one
reason, if the explicit_status flag isn't set then that callback is
responsible for deallocating the status request.  Without it, the
status request will leak.

Alan Stern

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

[Paul Elder]

On Fri, Jan 18, 2019 at 11:52:57AM -0500, Alan Stern wrote:
> On Fri, 18 Jan 2019, Paul Elder wrote:
> 
> > > > I meant the functions (procedures) in the function driver, so the setup
> > > > handler (uvc_function_setup), the completion handler
> > > > (uvc_function_ep0_complete), and the status sender (uvc_send_response),
> > > > although the last one actually sends the data stage for control IN.
> > > > So after the status is sent on the uvc gadget driver's end, its
> > > > completion handler is called again without the setup handler being
> > > > called beforehand and I cant figure out why.
> > > 
> > > Isn't this what you should expect?  Every usb_request, if it is queued
> > > successfully, eventually gets a completion callback.  That promise is
> > > made by every UDC driver; it's part of the gadget API.  So for a
> > > control transfer with a data stage, you expect to have:
> > > 
> > > 	Setup handler called
> > > 	Data-stage request submitted
> > > 	Data-stage request completion callback
> > > 	Status-stage request submitted
> > > 	Status-stage request completion callback
> > > 
> > > Thus, two completion callbacks but only one setup callback.
> > 
> > omg how did I not notice this :/
> > 
> > I guess I have to fix the uvc function driver so it works with that.
> > musb doesn't call the status stage completion callback though; not that
> > it does anything so it seems fine to me, but indeed the function driver
> > has to be ready for it if it is called.
> 
> musb _has_ to call the status-stage completion callback.  As just one
> reason, if the explicit_status flag isn't set then that callback is
> responsible for deallocating the status request.  Without it, the
> status request will leak.

Ah, I see what you mean. I forgot about that because we reuse the
request in uvc gadget. I'll have to add the status stage completion
callback to musb too, then.


Thanks,

Paul

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

[Alan Stern]

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@ideasonboard.com>
> > > > > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > 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?

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?

Alan Stern

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@ideasonboard.com>
> > > > > > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > > > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > 
> > > > > 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

[PATCH v5 5/6] usb: musb: gadget: implement optional explicit status stage

[Paul Elder]

Implement the mechanism for optional explicit status stage for the MUSB
driver. This allows a function driver to specify what to reply for the
status stage. The functionality for an implicit status stage is
retained.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
v1 Acked-by: Bin Liu <b-liu@ti.com>
---
Changes from v4:

- 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

No change from v3

Changes from v2:
- update call to usb_gadget_control_complete to include status
- since sending STALL from the function driver is now done with
  usb_ep_set_halt, there is no need for the internal ep0_send_response to
  take a stall/ack parameter; remove the parameter and make the function
  only send ack, and remove checking for the status reply in the
  usb_request for the status stage

Changes from v1:
- obvious change to implement v2 mechanism laid out by 4/6 of this
  series (send_response, and musb_g_ep0_send_response function has
  been removed, call to usb_gadget_control_complete has been added)
- ep0_send_response's ack argument has been changed from stall
- last_packet flag in ep0_rxstate has been removed, since it is equal to
  req != NULL

 drivers/usb/musb/musb_gadget.c     |  2 ++
 drivers/usb/musb/musb_gadget_ep0.c | 29 +++++++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 496643f54faa..ff83578c181f 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -144,6 +144,8 @@ __acquires(ep->musb->lock)
 		unmap_dma_buffer(req, musb);
 
 	trace_musb_req_gb(req);
+	if (req->ep->end_point.address == 0)
+		usb_gadget_control_complete(&musb->g, &req->request);
 	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
 	spin_lock(&musb->lock);
 	ep->busy = busy;
diff --git a/drivers/usb/musb/musb_gadget_ep0.c b/drivers/usb/musb/musb_gadget_ep0.c
index 91a5027b5c1f..dc5abdf9d49e 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -458,6 +458,25 @@ __acquires(musb->lock)
 	return handled;
 }
 
+static int ep0_send_ack(struct musb *musb)
+{
+	void __iomem *regs = musb->control_ep->regs;
+	u16 csr;
+
+	if (musb->ep0_state != MUSB_EP0_STAGE_RX &&
+	    musb->ep0_state != MUSB_EP0_STAGE_STATUSIN)
+		return -EINVAL;
+
+	csr = MUSB_CSR0_P_DATAEND | MUSB_CSR0_P_SVDRXPKTRDY;
+
+	musb_ep_select(musb->mregs, 0);
+	musb_writew(regs, MUSB_CSR0, csr);
+
+	musb->ep0_state = MUSB_EP0_STAGE_STATUSIN;
+
+	return 0;
+}
+
 /* we have an ep0out data packet
  * Context:  caller holds controller lock
  */
@@ -504,12 +523,15 @@ static void ep0_rxstate(struct musb *musb)
 	if (req) {
 		musb->ackpend = csr;
 		musb_g_ep0_giveback(musb, req);
+		if (req->explicit_status)
+			return;
 		if (!musb->ackpend)
 			return;
 		musb->ackpend = 0;
+	} else {
+		musb_ep_select(musb->mregs, 0);
+		musb_writew(regs, MUSB_CSR0, csr);
 	}
-	musb_ep_select(musb->mregs, 0);
-	musb_writew(regs, MUSB_CSR0, csr);
 }
 
 /*
@@ -939,6 +961,9 @@ musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r, gfp_t gfp_flags)
 	case MUSB_EP0_STAGE_ACKWAIT:	/* zero-length data */
 		status = 0;
 		break;
+	case MUSB_EP0_STAGE_STATUSIN:
+		status = ep0_send_ack(musb);
+		goto cleanup;
 	default:
 		musb_dbg(musb, "ep0 request queued in state %d",
 				musb->ep0_state);
-- 
2.20.1

[PATCH v5 6/6] usb: gadget: uvc: allow ioctl to send response in status stage

[Paul Elder]

We now have a mechanism to signal the UDC driver to reply to a control
OUT request with STALL or ACK, and we have packaged the setup stage data
and the data stage data of a control OUT request into a single
UVC_EVENT_DATA for userspace to consume. The ioctl UVCIOC_SEND_RESPONSE
in the case of a control OUT request sends a response to the data stage,
and so the ioctl now notifies the UDC driver to reply with STALL or ACK.
In the case of a control IN request, the ioctl sends the UVC data as
before.

Also tell the UDC to delay the status stage for this to work.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
No change from v4
No change from v3

Changes from v2:
- calling usb_ep_set_halt in uvc_send_response if data->length < 0 is
  now common for both IN and OUT transfers so make that check common
- remove now unnecessary field setting for the usb_request to be queued
  for the status stage

Changes from v1:
- remove usb_ep_delay_status call from the old proposed API
- changed portions of uvc_send_response to match v2 API
- remove UDC warning that send_response is not implemented

 drivers/usb/gadget/function/f_uvc.c    |  4 ++--
 drivers/usb/gadget/function/uvc_v4l2.c | 23 +++++++++++++++++------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 58ed191f636e..053808b01d48 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -209,14 +209,13 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct usb_request *req)
 	struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
 
 	if (uvc->event_setup_out) {
-		uvc->event_setup_out = 0;
-
 		memset(&v4l2_event, 0, sizeof(v4l2_event));
 		v4l2_event.type = UVC_EVENT_DATA;
 		uvc_event->data.length = req->actual;
 		memcpy(&uvc_event->data.data, req->buf, req->actual);
 		memcpy(&uvc_event->data.setup, &uvc->control_setup,
 		       sizeof(uvc_event->data.setup));
+
 		v4l2_event_queue(&uvc->vdev, &v4l2_event);
 	}
 }
@@ -251,6 +250,7 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 		 */
 		req->length = uvc->event_length;
 		req->zero = 0;
+		req->explicit_status = 1;
 		usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
 	} else {
 		struct v4l2_event v4l2_event;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index ac48f49d9f10..353071869e15 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,15 +35,26 @@ uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
 	struct usb_composite_dev *cdev = uvc->func.config->cdev;
 	struct usb_request *req = uvc->control_req;
 
+	if (data->length < 0)
+		return usb_ep_set_halt(cdev->gadget->ep0);
+
 	/*
 	 * For control OUT transfers the request has been enqueued synchronously
-	 * by the setup handler, there's nothing to be done here.
+	 * by the setup handler, we just need to tell the UDC whether to ACK or
+	 * STALL the control transfer.
 	 */
-	if (uvc->event_setup_out)
-		return 0;
-
-	if (data->length < 0)
-		return usb_ep_set_halt(cdev->gadget->ep0);
+	if (uvc->event_setup_out) {
+		/*
+		 * The length field carries the control request status.
+		 * Negative values signal a STALL and zero values an ACK.
+		 * Positive values are not valid as there is no data to send
+		 * back in the status stage.
+		 */
+		if (data->length > 0)
+			return -EINVAL;
+
+		return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL);
+	}
 
 	req->length = min_t(unsigned int, uvc->event_length, data->length);
 	req->zero = data->length < uvc->event_length;
-- 
2.20.1

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

[Alan Stern]

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

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

[Paul Elder]

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

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

[Alan Stern]

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