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.
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 functions to signal udc driver to delay status stage
usb: musb: gadget: implement send_response
usb: gadget: uvc: allow ioctl to send response in status stage
drivers/usb/gadget/function/f_uvc.c | 33 ++++++++++++++++-----
drivers/usb/gadget/function/uvc.h | 1 +
drivers/usb/gadget/function/uvc_v4l2.c | 21 +++++++++++++
drivers/usb/gadget/udc/core.c | 40 +++++++++++++++++++++++++
drivers/usb/musb/musb_gadget_ep0.c | 41 ++++++++++++++++++++++++++
include/linux/usb/gadget.h | 11 +++++++
include/uapi/linux/usb/g_uvc.h | 4 ++-
7 files changed, 142 insertions(+), 9 deletions(-)
--
2.18.0
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 <[email protected]>
---
drivers/usb/gadget/function/f_uvc.c | 5 +++--
drivers/usb/gadget/function/uvc_v4l2.c | 20 +++++++++++++++++---
drivers/usb/gadget/udc/core.c | 5 +++++
3 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 9df3eac440ea..ff89a76a7417 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -209,14 +209,15 @@ 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));
+
+ usb_ep_delay_status(ep);
+
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 2e2251bdef03..caa6412c0bda 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -37,10 +37,24 @@ uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
/*
* 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 (uvc->event_setup_out) {
+ struct usb_ep *ep = cdev->gadget->ep0;
+ bool stall = data->length < 0;
+
+ /*
+ * 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_send_response(ep, stall);
+ }
if (data->length < 0)
return usb_ep_set_halt(cdev->gadget->ep0);
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 1ec5ce6b43cd..0f62a3f1aa29 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1369,6 +1369,11 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
dev_dbg(&udc->dev, "registering UDC driver [%s]\n",
driver->function);
+ if (!udc->gadget->ep0->ops->send_response) {
+ dev_warn(&udc->dev, "UDC doesn't implement send_response\n");
+ dev_warn(&udc->dev, "Proper operation can't be guaranteed\n");
+ }
+
udc->driver = driver;
udc->dev.driver = &driver->driver;
udc->gadget->dev.driver = &driver->driver;
--
2.18.0
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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 8452de3dfccc..9df3eac440ea 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 5e75c0e93cc4..03d83eab2b90 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.18.0
This patch implements a mechanism to signal the MUSB driver to reply to
a control OUT request with STALL or ACK.
Signed-off-by: Paul Elder <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
drivers/usb/musb/musb_gadget_ep0.c | 41 ++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/usb/musb/musb_gadget_ep0.c b/drivers/usb/musb/musb_gadget_ep0.c
index 91a5027b5c1f..f0ed1f7472a3 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_response(struct musb *musb, bool stall)
+{
+ void __iomem *regs = musb->control_ep->regs;
+ u16 ackpend;
+
+ if (musb->ep0_state != MUSB_EP0_STAGE_RX &&
+ musb->ep0_state != MUSB_EP0_STAGE_STATUSIN)
+ return -EINVAL;
+
+ ackpend = MUSB_CSR0_P_DATAEND
+ | MUSB_CSR0_P_SVDRXPKTRDY
+ | (stall ? MUSB_CSR0_P_SENDSTALL : 0);
+
+ musb_ep_select(musb->mregs, 0);
+ musb_writew(regs, MUSB_CSR0, ackpend);
+
+ return 0;
+}
+
/* we have an ep0out data packet
* Context: caller holds controller lock
*/
@@ -466,10 +485,13 @@ static void ep0_rxstate(struct musb *musb)
void __iomem *regs = musb->control_ep->regs;
struct musb_request *request;
struct usb_request *req;
+ struct usb_ep *ep;
u16 count, csr;
+ bool last_packet = false;
request = next_ep0_request(musb);
req = &request->request;
+ ep = &request->ep->end_point;
/* read packet and ack; or stall because of gadget driver bug:
* should have provided the rx buffer before setup() returned.
@@ -492,6 +514,7 @@ static void ep0_rxstate(struct musb *musb)
if (count < 64 || req->actual == req->length) {
musb->ep0_state = MUSB_EP0_STAGE_STATUSIN;
csr |= MUSB_CSR0_P_DATAEND;
+ last_packet = true;
} else
req = NULL;
} else
@@ -508,6 +531,10 @@ static void ep0_rxstate(struct musb *musb)
return;
musb->ackpend = 0;
}
+
+ if (last_packet && ep->delayed_status)
+ return;
+
musb_ep_select(musb->mregs, 0);
musb_writew(regs, MUSB_CSR0, csr);
}
@@ -991,6 +1018,19 @@ static int musb_g_ep0_dequeue(struct usb_ep *ep, struct usb_request *req)
return -EINVAL;
}
+static int musb_g_ep0_send_response(struct usb_ep *e, bool stall)
+{
+ struct musb_ep *ep = to_musb_ep(e);
+ struct musb *musb = ep->musb;
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&musb->lock, flags);
+ ret = ep0_send_response(musb, stall);
+ spin_unlock_irqrestore(&musb->lock, flags);
+ return ret;
+}
+
static int musb_g_ep0_halt(struct usb_ep *e, int value)
{
struct musb_ep *ep;
@@ -1059,4 +1099,5 @@ const struct usb_ep_ops musb_g_ep0_ops = {
.queue = musb_g_ep0_queue,
.dequeue = musb_g_ep0_dequeue,
.set_halt = musb_g_ep0_halt,
+ .send_response = musb_g_ep0_send_response,
};
--
2.18.0
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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 dc12d868f9e8..8452de3dfccc 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 038482ff40e8..2e2251bdef03 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.18.0
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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.18.0
A usb gadget function driver may or may not want to delay the status
stage of a control OUT request. An instance it might want to is to
asynchronously validate the data of a class-specific request.
Add a function usb_ep_delay_status to allow function drivers to choose
to delay the status stage in the request completion handler. The UDC
should then check the usb_ep->delayed_status flag and act accordingly to
delay the status stage.
Also add a function usb_ep_send_response as a wrapper for
usb_ep->ops->send_response, whose prototype is added as well. This
function should be called by function drivers to tell the UDC what to
reply in the status stage that it has requested to be delayed.
Signed-off-by: Paul Elder <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
include/linux/usb/gadget.h | 11 +++++++++++
2 files changed, 46 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48c1cea..1ec5ce6b43cd 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
}
EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
+/**
+ * usb_ep_ep_delay_status - set delay_status flag
+ * @ep: the endpoint whose delay_status flag is being set
+ *
+ * This function instructs the UDC to delay the status stage of a control
+ * request. It can only be called from the request completion handler of a
+ * control request.
+ */
+void usb_ep_delay_status(struct usb_ep *ep)
+{
+ ep->delayed_status = true;
+}
+EXPORT_SYMBOL_GPL(usb_ep_delay_status);
+
+/**
+ * usb_ep_send_response - reply to control OUT request
+ * @ep: the endpoint to send reply
+ * @stall: true for STALL, false for ACK
+ *
+ * Instruct the UDC to complete the status stage of a control request that was
+ * previously delayed with a call to usb_ep_delay_status().
+ */
+int usb_ep_send_response(struct usb_ep *ep, bool stall)
+{
+ if (!ep->ops->send_response)
+ return -ENOSYS;
+
+ if (!ep->delayed_status)
+ return -EINVAL;
+
+ ep->delayed_status = false;
+ return ep->ops->send_response(ep, stall);
+}
+EXPORT_SYMBOL_GPL(usb_ep_send_response);
+
/* ------------------------------------------------------------------------- */
/**
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..d39c221d4b68 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -144,6 +144,8 @@ struct usb_ep_ops {
int (*fifo_status) (struct usb_ep *ep);
void (*fifo_flush) (struct usb_ep *ep);
+
+ int (*send_response) (struct usb_ep *ep, bool stall);
};
/**
@@ -209,6 +211,8 @@ struct usb_ep_caps {
* enabled and remains valid until the endpoint is disabled.
* @comp_desc: In case of SuperSpeed support, this is the endpoint companion
* descriptor that is used to configure the endpoint
+ * @delayed_status: True if status stage is being delayed. Valid only for
+ * control endpoints.
*
* the bus controller driver lists all the general purpose endpoints in
* gadget->ep_list. the control endpoint (gadget->ep0) is not in that list,
@@ -232,6 +236,7 @@ struct usb_ep {
u8 address;
const struct usb_endpoint_descriptor *desc;
const struct usb_ss_ep_comp_descriptor *comp_desc;
+ bool delayed_status;
};
/*-------------------------------------------------------------------------*/
@@ -249,6 +254,8 @@ int usb_ep_clear_halt(struct usb_ep *ep);
int usb_ep_set_wedge(struct usb_ep *ep);
int usb_ep_fifo_status(struct usb_ep *ep);
void usb_ep_fifo_flush(struct usb_ep *ep);
+void usb_ep_delay_status(struct usb_ep *ep);
+int usb_ep_send_response(struct usb_ep *ep, bool stall);
#else
static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
unsigned maxpacket_limit)
@@ -278,6 +285,10 @@ static inline int usb_ep_fifo_status(struct usb_ep *ep)
{ return 0; }
static inline void usb_ep_fifo_flush(struct usb_ep *ep)
{ }
+static inline void usb_ep_delay_status(struct usb_ep *ep)
+{ }
+static inline int usb_ep_send_response(struct usb_ep *ep, bool stall)
+{ }
#endif /* USB_GADGET */
/*-------------------------------------------------------------------------*/
--
2.18.0
Hi Paul,
Thank you for the patches.
On Wednesday, 10 October 2018 05:48:57 EEST 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. 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).
I'd like to point out for the reviewers that it changes the API of the UVC
gadget driver only, not any other USB gadget userspace API.
> 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.
>
> 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 functions to signal udc driver to delay status stage
> usb: musb: gadget: implement send_response
> usb: gadget: uvc: allow ioctl to send response in status stage
>
> drivers/usb/gadget/function/f_uvc.c | 33 ++++++++++++++++-----
> drivers/usb/gadget/function/uvc.h | 1 +
> drivers/usb/gadget/function/uvc_v4l2.c | 21 +++++++++++++
> drivers/usb/gadget/udc/core.c | 40 +++++++++++++++++++++++++
> drivers/usb/musb/musb_gadget_ep0.c | 41 ++++++++++++++++++++++++++
> include/linux/usb/gadget.h | 11 +++++++
> include/uapi/linux/usb/g_uvc.h | 4 ++-
> 7 files changed, 142 insertions(+), 9 deletions(-)
--
Regards,
Laurent Pinchart
Hi Paul,
Thank you for the patch.
On Wednesday, 10 October 2018 05:48:58 EEST Paul Elder wrote:
> 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 <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
This is independent of the rest of the series so I've taken it in my tree
already, with the subject line modified to use "usb: gadget: uvc:" as a
prefix.
> ---
> 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)
--
Regards,
Laurent Pinchart
Hi,
On Tue, Oct 09, 2018 at 10:49:02PM -0400, Paul Elder wrote:
> This patch implements a mechanism to signal the MUSB driver to reply to
> a control OUT request with STALL or ACK.
>
> Signed-off-by: Paul Elder <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
The patch looks good to me, here is my Acked-by:
Acked-by: Bin Liu <[email protected]>
but I am unable to test this patch set - the current Greg's usb-next
tree gives deadlock error between composite_disconnect() and
usb_function_deactivate() when loading g_webcam.ko on BeagleboneBlack.
The error happens before applying this patch set.
Regards,
-Bin.
Hi,
On Tue, Oct 09, 2018 at 10:49:01PM -0400, 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 it might want to is to
> asynchronously validate the data of a class-specific request.
>
> Add a function usb_ep_delay_status to allow function drivers to choose
> to delay the status stage in the request completion handler. The UDC
> should then check the usb_ep->delayed_status flag and act accordingly to
> delay the status stage.
>
> Also add a function usb_ep_send_response as a wrapper for
> usb_ep->ops->send_response, whose prototype is added as well. This
> function should be called by function drivers to tell the UDC what to
> reply in the status stage that it has requested to be delayed.
>
> Signed-off-by: Paul Elder <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
> include/linux/usb/gadget.h | 11 +++++++++++
> 2 files changed, 46 insertions(+)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index af88b48c1cea..1ec5ce6b43cd 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> }
> EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
>
> +/**
> + * usb_ep_ep_delay_status - set delay_status flag
> + * @ep: the endpoint whose delay_status flag is being set
> + *
> + * This function instructs the UDC to delay the status stage of a control
> + * request. It can only be called from the request completion handler of a
> + * control request.
> + */
> +void usb_ep_delay_status(struct usb_ep *ep)
> +{
> + ep->delayed_status = true;
> +}
> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
Is usb_ep_set_delay_status() better? I thought it implies get/return
action if a verb is missing in the function name.
Regards,
-Bin.
Hi,
On Tue, Oct 09, 2018 at 10:48:57PM -0400, 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. This mechanism is implemented for MUSB, and is
> used by UVC. At the same time, UVC packages the setup stage and data
Why is this for MUSB only? Other UDC such as DWC3 doesn't need this?
Regards,
-Bin.
Hi Bin,
On Thursday, 11 October 2018 22:31:42 EEST Bin Liu wrote:
> On Tue, Oct 09, 2018 at 10:48:57PM -0400, 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. This mechanism is implemented for MUSB, and is
> > used by UVC. At the same time, UVC packages the setup stage and data
>
> Why is this for MUSB only? Other UDC such as DWC3 doesn't need this?
Unfortunately, the asynchronous control request data stage validation
mechanism must be implemented by every UDC. This patch series only addresses
MUSB as this is Paul's main test platform. Once the core patches get reviewed
and the API accepted (possibly in a modified form), we plan to update the DWC2
and DWC3.
--
Regards,
Laurent Pinchart
Hi Bin,
On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> On Tue, Oct 09, 2018 at 10:49:01PM -0400, 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 it might want to is to
> > asynchronously validate the data of a class-specific request.
> >
> > Add a function usb_ep_delay_status to allow function drivers to choose
> > to delay the status stage in the request completion handler. The UDC
> > should then check the usb_ep->delayed_status flag and act accordingly to
> > delay the status stage.
> >
> > Also add a function usb_ep_send_response as a wrapper for
> > usb_ep->ops->send_response, whose prototype is added as well. This
> > function should be called by function drivers to tell the UDC what to
> > reply in the status stage that it has requested to be delayed.
> >
> > Signed-off-by: Paul Elder <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
> > ---
> >
> > drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
> > include/linux/usb/gadget.h | 11 +++++++++++
> > 2 files changed, 46 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index af88b48c1cea..1ec5ce6b43cd 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> > }
> > EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> >
> > +/**
> > + * usb_ep_ep_delay_status - set delay_status flag
> > + * @ep: the endpoint whose delay_status flag is being set
> > + *
> > + * This function instructs the UDC to delay the status stage of a control
> > + * request. It can only be called from the request completion handler of
> > a
> > + * control request.
> > + */
> > +void usb_ep_delay_status(struct usb_ep *ep)
> > +{
> > + ep->delayed_status = true;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
>
> Is usb_ep_set_delay_status() better? I thought it implies get/return
> action if a verb is missing in the function name.
For what it's worth, I understand the function name as "delay the status
stage", with "delay" being a verb. Maybe the short description could be
updated accordingly.
--
Regards,
Laurent Pinchart
Hi Laurent,
On Thu, Oct 18, 2018 at 02:42:29AM +0300, Laurent Pinchart wrote:
> Hi Bin,
>
> On Thursday, 11 October 2018 22:31:42 EEST Bin Liu wrote:
> > On Tue, Oct 09, 2018 at 10:48:57PM -0400, 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. This mechanism is implemented for MUSB, and is
> > > used by UVC. At the same time, UVC packages the setup stage and data
> >
> > Why is this for MUSB only? Other UDC such as DWC3 doesn't need this?
>
> Unfortunately, the asynchronous control request data stage validation
> mechanism must be implemented by every UDC. This patch series only addresses
> MUSB as this is Paul's main test platform. Once the core patches get reviewed
> and the API accepted (possibly in a modified form), we plan to update the DWC2
> and DWC3.
Thanks for the explanation.
Regards,
-Bin.
Laurent,
On Thu, Oct 18, 2018 at 02:45:32AM +0300, Laurent Pinchart wrote:
> Hi Bin,
>
> On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> > On Tue, Oct 09, 2018 at 10:49:01PM -0400, 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 it might want to is to
> > > asynchronously validate the data of a class-specific request.
> > >
> > > Add a function usb_ep_delay_status to allow function drivers to choose
> > > to delay the status stage in the request completion handler. The UDC
> > > should then check the usb_ep->delayed_status flag and act accordingly to
> > > delay the status stage.
> > >
> > > Also add a function usb_ep_send_response as a wrapper for
> > > usb_ep->ops->send_response, whose prototype is added as well. This
> > > function should be called by function drivers to tell the UDC what to
> > > reply in the status stage that it has requested to be delayed.
> > >
> > > Signed-off-by: Paul Elder <[email protected]>
> > > Reviewed-by: Laurent Pinchart <[email protected]>
> > > ---
> > >
> > > drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
> > > include/linux/usb/gadget.h | 11 +++++++++++
> > > 2 files changed, 46 insertions(+)
> > >
> > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > > index af88b48c1cea..1ec5ce6b43cd 100644
> > > --- a/drivers/usb/gadget/udc/core.c
> > > +++ b/drivers/usb/gadget/udc/core.c
> > > @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> > > }
> > > EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> > >
> > > +/**
> > > + * usb_ep_ep_delay_status - set delay_status flag
> > > + * @ep: the endpoint whose delay_status flag is being set
> > > + *
> > > + * This function instructs the UDC to delay the status stage of a control
> > > + * request. It can only be called from the request completion handler of
> > > a
> > > + * control request.
> > > + */
> > > +void usb_ep_delay_status(struct usb_ep *ep)
> > > +{
> > > + ep->delayed_status = true;
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> >
> > Is usb_ep_set_delay_status() better? I thought it implies get/return
> > action if a verb is missing in the function name.
>
> For what it's worth, I understand the function name as "delay the status
> stage", with "delay" being a verb. Maybe the short description could be
> updated accordingly.
Okay, maybe it is just my own understanding problem. I thought about
delay as the verb, but then notice the var is called delayed_status,
then I was thinking delay_status in the function name as the flag.
No worries.
Regards,
-Bin.
On Thu, 18 Oct 2018, Laurent Pinchart wrote:
> Hi Bin,
>
> On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> > On Tue, Oct 09, 2018 at 10:49:01PM -0400, 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 it might want to is to
> > > asynchronously validate the data of a class-specific request.
> > >
> > > Add a function usb_ep_delay_status to allow function drivers to choose
> > > to delay the status stage in the request completion handler. The UDC
> > > should then check the usb_ep->delayed_status flag and act accordingly to
> > > delay the status stage.
> > >
> > > Also add a function usb_ep_send_response as a wrapper for
> > > usb_ep->ops->send_response, whose prototype is added as well. This
> > > function should be called by function drivers to tell the UDC what to
> > > reply in the status stage that it has requested to be delayed.
> > >
> > > Signed-off-by: Paul Elder <[email protected]>
> > > Reviewed-by: Laurent Pinchart <[email protected]>
> > > ---
> > >
> > > drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
> > > include/linux/usb/gadget.h | 11 +++++++++++
> > > 2 files changed, 46 insertions(+)
> > >
> > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > > index af88b48c1cea..1ec5ce6b43cd 100644
> > > --- a/drivers/usb/gadget/udc/core.c
> > > +++ b/drivers/usb/gadget/udc/core.c
> > > @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> > > }
> > > EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> > >
> > > +/**
> > > + * usb_ep_ep_delay_status - set delay_status flag
> > > + * @ep: the endpoint whose delay_status flag is being set
> > > + *
> > > + * This function instructs the UDC to delay the status stage of a control
> > > + * request. It can only be called from the request completion handler of
> > > a
> > > + * control request.
> > > + */
> > > +void usb_ep_delay_status(struct usb_ep *ep)
> > > +{
> > > + ep->delayed_status = true;
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> >
> > Is usb_ep_set_delay_status() better? I thought it implies get/return
> > action if a verb is missing in the function name.
>
> For what it's worth, I understand the function name as "delay the status
> stage", with "delay" being a verb. Maybe the short description could be
> updated accordingly.
Is there a reason for adding a new function for this? This is exactly
what the USB_GADGET_DELAYED_STATUS return value from the setup callback
is meant for (and it is already used by some gadget drivers).
Is it a question of when the gadget driver learns that it will need to
delay the status stage? If that's the case, why not always return
USB_GADGET_DELAYED_STATUS from the setup callback? Then instead of
calling usb_ep_delay_status() when a delay is needed, you could queue
the status request when a delay isn't needed.
As a more general solution, Felipe has said that a UDC driver should
_never_ carry out the status stage transaction until the gadget driver
has told it to do so. Then there would be no need for any sort of
delay indicator. (But implementing this would require significant
changes to a bunch of different drivers...)
Alan Stern
Hi Bin,
On Thu, Oct 11, 2018 at 11:07:46AM -0500, Bin Liu wrote:
> Hi,
>
> On Tue, Oct 09, 2018 at 10:49:02PM -0400, Paul Elder wrote:
> > This patch implements a mechanism to signal the MUSB driver to reply to
> > a control OUT request with STALL or ACK.
> >
> > Signed-off-by: Paul Elder <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
>
> The patch looks good to me, here is my Acked-by:
>
> Acked-by: Bin Liu <[email protected]>
>
> but I am unable to test this patch set - the current Greg's usb-next
> tree gives deadlock error between composite_disconnect() and
> usb_function_deactivate() when loading g_webcam.ko on BeagleboneBlack.
> The error happens before applying this patch set.
We don't use g_webcam anymore, because it doesn't offer the flexibility
that configfs does (for example, only one function can be configured with
g_webcam). There are no features that g_webcam offers that configfs doesn't.
I was unable to reproduce the deadlock that you mention on Greg's
usb-next tree. Which commit were you on?
I did, however, get the deadlock that you mention upon *killing* the
userspace application providing the stream, not when loading g_webcam.ko.
Here is a sample script for setting up a UVC gadget through configfs
(I haven't tested this exact script; I extracted the functional
components):
CONFIGFS="/sys/kernel/config"
GADGET="$CONFIGFS/usb_gadget"
VID="0x0525"
PID="0xa4a2"
SERIAL="0123456789"
MANUF=$(hostname)
PRODUCT="UVC Gadget"
UDC=`ls /sys/class/udc`
cd $GADGET/g1
echo $VID > idVendor
echo $PID > idProduct
mkdir -p strings/0x409
echo $SERIAL > strings/0x409/serialnumber
echo $MANUF > strings/0x409/manufacturer
echo $PRODUCT > strings/0x409/product
mkdir configs/c.1
mkdir configs/c.1/strings/0x409
# create uvc
CONFIG="configs/c.1"
FUNCTION="uvc.0"
mkdir functions/$FUNCTION
# create frame 640x360 uncompressed
WIDTH=640
HEIGHT=360
wdir=functions/$FUNCTION/streaming/uncompressed/u/${HEIGHT}p
mkdir $wdir
echo $WIDTH > $wdir/wWidth
echo $HEIGHT > $wdir/wHeight
echo $(( $WIDTH * $HEIGHT * 2 )) > $wdir/dwMaxVideoFrameBufferSize
cat <<EOF > $wdir/dwFrameInterval
666666
100000
5000000
EOF
# end create frame
mkdir functions/$FUNCTION/streaming/header/h
cd functions/$FUNCTION/streaming/header/h
ln -s ../../uncompressed/u
cd ../../class/fs
ln -s ../../header/h
cd ../../class/hs
ln -s ../../header/h
cd ../../../control
mkdir header/h
ln -s header/h class/fs
ln -s header/h class/ss
cd ../../../
# Set the packet size: uvc gadget max size is 3k...
echo 3072 > functions/$FUNCTION/streaming_maxpacket
echo 2048 > functions/$FUNCTION/streaming_maxpacket
echo 1024 > functions/$FUNCTION/streaming_maxpacket
ln -s functions/$FUNCTION configs/c.1
# end create uvc
echo $UDC > UDC
Thanks,
Paul
Hi Alan,
On Thu, Oct 18, 2018 at 10:07:36AM -0400, Alan Stern wrote:
> On Thu, 18 Oct 2018, Laurent Pinchart wrote:
>
> > Hi Bin,
> >
> > On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> > > On Tue, Oct 09, 2018 at 10:49:01PM -0400, 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 it might want to is to
> > > > asynchronously validate the data of a class-specific request.
> > > >
> > > > Add a function usb_ep_delay_status to allow function drivers to choose
> > > > to delay the status stage in the request completion handler. The UDC
> > > > should then check the usb_ep->delayed_status flag and act accordingly to
> > > > delay the status stage.
> > > >
> > > > Also add a function usb_ep_send_response as a wrapper for
> > > > usb_ep->ops->send_response, whose prototype is added as well. This
> > > > function should be called by function drivers to tell the UDC what to
> > > > reply in the status stage that it has requested to be delayed.
> > > >
> > > > Signed-off-by: Paul Elder <[email protected]>
> > > > Reviewed-by: Laurent Pinchart <[email protected]>
> > > > ---
> > > >
> > > > drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
> > > > include/linux/usb/gadget.h | 11 +++++++++++
> > > > 2 files changed, 46 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > > > index af88b48c1cea..1ec5ce6b43cd 100644
> > > > --- a/drivers/usb/gadget/udc/core.c
> > > > +++ b/drivers/usb/gadget/udc/core.c
> > > > @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> > > > }
> > > > EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> > > >
> > > > +/**
> > > > + * usb_ep_ep_delay_status - set delay_status flag
> > > > + * @ep: the endpoint whose delay_status flag is being set
> > > > + *
> > > > + * This function instructs the UDC to delay the status stage of a control
> > > > + * request. It can only be called from the request completion handler of
> > > > a
> > > > + * control request.
> > > > + */
> > > > +void usb_ep_delay_status(struct usb_ep *ep)
> > > > +{
> > > > + ep->delayed_status = true;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> > >
> > > Is usb_ep_set_delay_status() better? I thought it implies get/return
> > > action if a verb is missing in the function name.
> >
> > For what it's worth, I understand the function name as "delay the status
> > stage", with "delay" being a verb. Maybe the short description could be
> > updated accordingly.
>
> Is there a reason for adding a new function for this? This is exactly
> what the USB_GADGET_DELAYED_STATUS return value from the setup callback
> is meant for (and it is already used by some gadget drivers).
In theory, we might be able to use USB_GADGET_DELAYED_STATUS for this.
However, there are a few ambiguities that prevent us from doing so.
First of all, we want to delay only the status stage for control OUT
requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
delaying the "data/status stages". Does this mean that it delays the
status stage only or does it delay both stages? If the slash means
"and", then we cannot use USB_GADGET_DELAYED_STATUS.
Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
which has already been observed in the UVC gadget driver previously [0].
The raceiness stems from the fact that things can happen in between
returning USB_GADGET_DELAYED_STATUS and the composite layer reacting to
it - especially if usb_composite_setup_continue is called within that
window it causes a WARN. In any case, the fact that the mechanism itself
is racey suggests that it needs improvement, and using it wouldn't be a
good solution in this case.
> Is it a question of when the gadget driver learns that it will need to
> delay the status stage? If that's the case,
Not really.
> why not always return
> USB_GADGET_DELAYED_STATUS from the setup callback? Then instead of
> calling usb_ep_delay_status() when a delay is needed, you could queue
> the status request when a delay isn't needed.
Theoretically this might work, but see the problems mentioned above.
> As a more general solution, Felipe has said that a UDC driver should
> _never_ carry out the status stage transaction until the gadget driver
> has told it to do so. Then there would be no need for any sort of
> delay indicator.
Yeah, but,
> (But implementing this would require significant
> changes to a bunch of different drivers...)
exactly :/
[0] https://www.spinics.net/lists/linux-usb/msg169208.html
Paul
Hello,
On Friday, 2 November 2018 01:40:59 EET Paul Elder wrote:
> On Thu, Oct 18, 2018 at 10:07:36AM -0400, Alan Stern wrote:
> > On Thu, 18 Oct 2018, Laurent Pinchart wrote:
> >> On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> >>> On Tue, Oct 09, 2018 at 10:49:01PM -0400, 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 it might want to is to
> >>>> asynchronously validate the data of a class-specific request.
> >>>>
> >>>> Add a function usb_ep_delay_status to allow function drivers to
> >>>> choose to delay the status stage in the request completion handler.
> >>>> The UDC should then check the usb_ep->delayed_status flag and act
> >>>> accordingly to delay the status stage.
> >>>>
> >>>> Also add a function usb_ep_send_response as a wrapper for
> >>>> usb_ep->ops->send_response, whose prototype is added as well. This
> >>>> function should be called by function drivers to tell the UDC what
> >>>> to reply in the status stage that it has requested to be delayed.
> >>>>
> >>>> Signed-off-by: Paul Elder <[email protected]>
> >>>> Reviewed-by: Laurent Pinchart <[email protected]>
> >>>> ---
> >>>>
> >>>> drivers/usb/gadget/udc/core.c | 35 ++++++++++++++++++++++++++++++++
> >>>> include/linux/usb/gadget.h | 11 +++++++++++
> >>>> 2 files changed, 46 insertions(+)
> >>>>
> >>>> diff --git a/drivers/usb/gadget/udc/core.c
> >>>> b/drivers/usb/gadget/udc/core.c
> >>>> index af88b48c1cea..1ec5ce6b43cd 100644
> >>>> --- a/drivers/usb/gadget/udc/core.c
> >>>> +++ b/drivers/usb/gadget/udc/core.c
> >>>> @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> >>>>
> >>>> }
> >>>> EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> >>>>
> >>>> +/**
> >>>> + * usb_ep_ep_delay_status - set delay_status flag
> >>>> + * @ep: the endpoint whose delay_status flag is being set
> >>>> + *
> >>>> + * This function instructs the UDC to delay the status stage of a
> >>>> control
> >>>> + * request. It can only be called from the request completion
> >>>> handler of a
> >>>> + * control request.
> >>>> + */
> >>>> +void usb_ep_delay_status(struct usb_ep *ep)
> >>>> +{
> >>>> + ep->delayed_status = true;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> >>>
> >>> Is usb_ep_set_delay_status() better? I thought it implies get/return
> >>> action if a verb is missing in the function name.
> >>
> >> For what it's worth, I understand the function name as "delay the status
> >> stage", with "delay" being a verb. Maybe the short description could be
> >> updated accordingly.
> >
> > Is there a reason for adding a new function for this? This is exactly
> > what the USB_GADGET_DELAYED_STATUS return value from the setup callback
> > is meant for (and it is already used by some gadget drivers).
>
> In theory, we might be able to use USB_GADGET_DELAYED_STATUS for this.
> However, there are a few ambiguities that prevent us from doing so.
>
> First of all, we want to delay only the status stage for control OUT
> requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
> delaying the "data/status stages". Does this mean that it delays the
> status stage only or does it delay both stages? If the slash means
> "and", then we cannot use USB_GADGET_DELAYED_STATUS.
>
> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
> which has already been observed in the UVC gadget driver previously [0].
> The raceiness stems from the fact that things can happen in between
> returning USB_GADGET_DELAYED_STATUS and the composite layer reacting to
> it - especially if usb_composite_setup_continue is called within that
> window it causes a WARN. In any case, the fact that the mechanism itself
> is racey suggests that it needs improvement, and using it wouldn't be a
> good solution in this case.
>
> > Is it a question of when the gadget driver learns that it will need to
> > delay the status stage? If that's the case,
>
> Not really.
>
> > why not always return
> > USB_GADGET_DELAYED_STATUS from the setup callback? Then instead of
> > calling usb_ep_delay_status() when a delay is needed, you could queue
> > the status request when a delay isn't needed.
>
> Theoretically this might work, but see the problems mentioned above.
>
> > As a more general solution, Felipe has said that a UDC driver should
> > _never_ carry out the status stage transaction until the gadget driver
> > has told it to do so. Then there would be no need for any sort of
> > delay indicator.
>
> Yeah, but,
>
> > (But implementing this would require significant
> > changes to a bunch of different drivers...)
>
> exactly :/
>
> [0] https://www.spinics.net/lists/linux-usb/msg169208.html
Alan, Felipe, how do we move forward with this ? There are several issues with
the existing control request handling mechanism in the USB gadget stack, and
while Paul could work on improving the mechanism, we need to provide clear
guidance regarding the direction we want to take.
For reference, the issues I know about for the USB_GADGET_DELAYED_STATUS
mechanism are
- The mechanism is inherently racy. It relies on signaling the delay at the
very end of the processing in the setup handler, which by definition occurs
after the work to process the control request is queued (in the generic sense,
regardless of whether this involves a kernel workqueue or passing the work to
userspace). There is thus a race window after queuing the work and before
signaling the delay during which the work handler could signal completion.
- The mechanism is poorly documented. As Paul mentioned, comments in the code
state that USB_GADGET_DELAYED_STATUS delay the "data/status stages". This is
very unclear, and the only three UDCs that implement the mechanism seem to do
so in different ways:
- The mtu driver states in a comment that it will "handle the delay STATUS
phase till receive ep_queue on ep0".
- The bdc driver states in a comment that "The ep0 state will remain
WAIT_FOR_DATA_START till we received ep_queue on ep0".
- The dwc3 driver seems to handle USB_GADGET_DELAYED_STATUS for the
SET_CONFIG request only.
- The mechanism relies on queueing a request to the UDC to signal that it
should continue with the status stage. That request can be queued either by
the USB gadget function driver directly, or by the composite layer in
usb_composite_setup_continue() (the latter is restricted to requests that
carry no data as it sets the request length to 0). This is problematic if we
want to delay the status phase after completing the data phase, in order to
validate the setup phase data and the data phase data (for a control OUT
request) together.
For those reasons I think a new mechanism is needed. It should either signal
the status phase delay through an explicit function call instead of a return
value (to solve the race mentioned above), or by requiring all requests to be
explicitly completed (but that will require changing all USB function
drivers). Furthermore, the mechanism need to support delaying the status phase
after queuing the request for the data phase, so we need an explicit way to
signal that the UDC should proceed with the status phase, other than queueing
the request.
Thoughts ? Preferences ?
--
Regards,
Laurent Pinchart
Hi Felipe,
On Friday, 2 November 2018 15:17:20 EET Felipe Balbi wrote:
> Laurent Pinchart writes:
> >>>>> +void usb_ep_delay_status(struct usb_ep *ep)
> >>>>>
> >>>>> +{
> >>>>> + ep->delayed_status = true;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> >>>>
> >>>> Is usb_ep_set_delay_status() better? I thought it implies get/return
> >>>> action if a verb is missing in the function name.
> >>>
> >>> For what it's worth, I understand the function name as "delay the
> >>> status stage", with "delay" being a verb. Maybe the short description
> >>> could be updated accordingly.
> >>>
> >>> Is there a reason for adding a new function for this? This is exactly
> >>> what the USB_GADGET_DELAYED_STATUS return value from the setup callback
> >>> is meant for (and it is already used by some gadget drivers).
> >>
> >> In theory, we might be able to use USB_GADGET_DELAYED_STATUS for this.
> >> However, there are a few ambiguities that prevent us from doing so.
> >>
> >> First of all, we want to delay only the status stage for control OUT
> >> requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
> >> delaying the "data/status stages". Does this mean that it delays the
> >> status stage only or does it delay both stages? If the slash means
> >> "and", then we cannot use USB_GADGET_DELAYED_STATUS.
> >>
> >> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
> >> which has already been observed in the UVC gadget driver previously [0].
> >> The raceiness stems from the fact that things can happen in between
> >> returning USB_GADGET_DELAYED_STATUS and the composite layer reacting to
> >> it - especially if usb_composite_setup_continue is called within that
> >> window it causes a WARN. In any case, the fact that the mechanism itself
> >> is racey suggests that it needs improvement, and using it wouldn't be a
> >> good solution in this case.
> >>
> >>> Is it a question of when the gadget driver learns that it will need to
> >>> delay the status stage? If that's the case,
> >>
> >> Not really.
> >>
> >>> why not always return
> >>> USB_GADGET_DELAYED_STATUS from the setup callback? Then instead of
> >>> calling usb_ep_delay_status() when a delay is needed, you could queue
> >>> the status request when a delay isn't needed.
> >>
> >> Theoretically this might work, but see the problems mentioned above.
> >>
> >>> As a more general solution, Felipe has said that a UDC driver should
> >>> _never_ carry out the status stage transaction until the gadget driver
> >>> has told it to do so. Then there would be no need for any sort of
> >>> delay indicator.
> >>
> >> Yeah, but,
> >>
> >>> (But implementing this would require significant
> >>> changes to a bunch of different drivers...)
> >>
> >> exactly :/
>
> add a flag to gadget structure. Something like
> "supports_explicit_status_stage" and add a new return value
> USB_EXPLICIT_STATUS_STAGE.
>
> Then, take uvc for example, implement the new setup:
>
> if (supports_explicit_status_stage)
> return USB_EXPLICIT_STATUS_STAGE;
>
> then on dwc3 you would:
>
> switch (ret) {
> case USB_EXPLICIT_STATUS_STAGE:
> case USB_GADGET_DELAYED_STATUS:
> wait_for_status_request_queue();
> break;
> default:
> start_status_stage();
> }
>
> If this works with dwc3 + uvc, then we have a good recipe on how to
> implement for the other drivers.
Given that we need to delay the status stage and not the data stage, we can't
explicitly request the status stage through a usb request queue. Would a new
explicit function call work for you for that purpose ?
> > Alan, Felipe, how do we move forward with this ? There are several issues
> > with the existing control request handling mechanism in the USB gadget
> > stack, and while Paul could work on improving the mechanism, we need to
> > provide clear guidance regarding the direction we want to take.
> >
> > For reference, the issues I know about for the USB_GADGET_DELAYED_STATUS
> > mechanism are
> >
> > - The mechanism is inherently racy. It relies on signaling the delay at
> > the very end of the processing in the setup handler, which by definition
> > occurs after the work to process the control request is queued (in the
> > generic sense, regardless of whether this involves a kernel workqueue or
> > passing the work to userspace). There is thus a race window after queuing
> > the work and before signaling the delay during which the work handler
> > could signal completion.
>
> We won't fix this until all functions and UDCs are converted over, but
> it's doable.
It could be fixed by signaling the delay through an explicit function call
before queueing the work instead of through a return value though, but I agree
that long term requesting the status stage explicitly would likely be cleaner.
> > - The mechanism is poorly documented. As Paul mentioned, comments in the
> > code state that USB_GADGET_DELAYED_STATUS delay the "data/status stages".
> > This is very unclear, and the only three UDCs that implement the
> > mechanism seem to do so in different ways:
> >
> > - The mtu driver states in a comment that it will "handle the delay
> > STATUS phase till receive ep_queue on ep0".
> >
> > - The bdc driver states in a comment that "The ep0 state will remain
> > WAIT_FOR_DATA_START till we received ep_queue on ep0".
> >
> > - The dwc3 driver seems to handle USB_GADGET_DELAYED_STATUS for the
> > SET_CONFIG request only.
>
> that's the only one that has needed it so far. I'm all for making status
> stage ALWAYS explicit, that will, in the long run, simplify UDC
> drivers and make the API easier to understand.
>
> > - The mechanism relies on queueing a request to the UDC to signal that it
> > should continue with the status stage. That request can be queued either
> > by the USB gadget function driver directly, or by the composite layer in
> > usb_composite_setup_continue() (the latter is restricted to requests that
> > carry no data as it sets the request length to 0). This is problematic if
> > we want to delay the status phase after completing the data phase, in
> > order to validate the setup phase data and the data phase data (for a
> > control OUT request) together.
>
> It shouldn't cause problems, actually. Most of the issues come from the
> fact that sometimes gadget driver just returns a success and expects UDC
> to initiate status stage and sometimes gadget driver wants to handle
> status stage explicitly.
Requesting the status stage explicitly requires an API to do so (quite
obviously). Your proposal is to use the usb request queue as a signal to
continue to the next stage. My point is that this can't work for control OUT
requests, where we may want to delay the status stage after the data is
received by the UDC, and thus after the request is queued. We need a different
API for that.
For control IN, we may want to delay the data stage if we can't respond
directly, or proceed with the data stage immediately. In both cases this can
be signaled by a request being queued. There is no need to delay the status
stage as it's initiated by the host.
For control OUT, we may want to delay the data stage if we need to validate
the setup stage data asynchronously. Proceeding to the data stage can be
signaled by queueing a request. We may also want to delay the status stage if
we need to validate the data stage data asynchronously. This can't be signaled
by queueing a request.
I wonder if there's really a use case for delaying the data stage of control
OUT requests, as it seems to me that we can perform the asynchronous
validation of the setup and data stages together, in which case we would
always proceed to the data stage, and only potentially delay the status stage.
However, if we switch to an explicit API where the transition from the setup
to the data stage is triggered by queueing a request, and given that such a
transition may need to be delayed for the control IN case, delaying the data
stage for control OUT would essentially come for free.
In any case we need an API to delay the status stage of a control OUT request.
There are two options here. We can consider that the status stage shouldn't be
delayed by default and add a new function to be called from the data stage
completion handler to request a status stage delay (using the return value of
the completion handler isn't a good idea as it would be racy as explained
above). A second function would be needed to request the status stage (which,
as explained above too, can't be done by queueing a request). A second option
is to consider that the status stage is delayed by default until explcitly
required. In both cases the same new function is needed to request the status
stage.
Note that delaying the data stage and delaying the status stage are two
different problems, and don't necessarily need to be solved together. However,
if I understand things correctly, we currently delay the data stage of a few
control OUT requests (they all have a 0 bytes data stage) as a mean to
completion of the request. I believe that this use case could be implemented
by delaying the status stage instead, so the two are still related in a way.
If we end up moving to explicit state handling, with the data stage being
entered by queueing a request, and the status stage being entered by calling a
new function, control OUT requests with 0 bytes of data that can be handled
synchronously in the setup handler would require function drivers to both
queue a zero-length request and call the status function. This would make the
function code more complex, and I wonder whether a shortcut would be a good
idea, perhaps in the form of a flag in the request that tells the UDC to
automatically proceed to the status stage immediately after the data stage. Or
we could make that behaviour the default when the request doesn't have a
completion handler (as moving explicitly to the status stage should be done at
the earliest from the data stage completion handler).
> > For those reasons I think a new mechanism is needed. It should either
> > signal the status phase delay through an explicit function call instead
> > of a return value (to solve the race mentioned above), or by requiring
> > all requests to be explicitly completed (but that will require changing
> > all USB function drivers). Furthermore, the mechanism need to support
> > delaying the status phase after queuing the request for the data phase,
> > so we need an explicit way to signal that the UDC should proceed with the
> > status phase, other than queueing the request.
> >
> > Thoughts ? Preferences ?
>
> how about making status stage always explicit?
If we implement a proof of concept, could you help us converting drivers over
to the new API ? I'll assume we'll have to address all UDCs first, and then
the function drivers.
--
Regards,
Laurent Pinchart
On Fri, 2 Nov 2018, Laurent Pinchart wrote:
> Hi Felipe,
>
> On Friday, 2 November 2018 15:17:20 EET Felipe Balbi wrote:
> > Laurent Pinchart writes:
> > >>>>> +void usb_ep_delay_status(struct usb_ep *ep)
> > >>>>>
> > >>>>> +{
> > >>>>> + ep->delayed_status = true;
> > >>>>> +}
> > >>>>> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> > >>>>
> > >>>> Is usb_ep_set_delay_status() better? I thought it implies get/return
> > >>>> action if a verb is missing in the function name.
> > >>>
> > >>> For what it's worth, I understand the function name as "delay the
> > >>> status stage", with "delay" being a verb. Maybe the short description
> > >>> could be updated accordingly.
> > >>>
> > >>> Is there a reason for adding a new function for this? This is exactly
> > >>> what the USB_GADGET_DELAYED_STATUS return value from the setup callback
> > >>> is meant for (and it is already used by some gadget drivers).
> > >>
> > >> In theory, we might be able to use USB_GADGET_DELAYED_STATUS for this.
> > >> However, there are a few ambiguities that prevent us from doing so.
> > >>
> > >> First of all, we want to delay only the status stage for control OUT
> > >> requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
> > >> delaying the "data/status stages". Does this mean that it delays the
> > >> status stage only or does it delay both stages? If the slash means
> > >> "and", then we cannot use USB_GADGET_DELAYED_STATUS.
The comment in composite.h is wrong; it should refer only to the status
stage. In fact, it should refer only to the status stage for
control-OUT transfers; there's no reason ever to delay the status stage
of a control-IN transfer (the driver should instead delay the data
stage if it needs to).
> > >> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
> > >> which has already been observed in the UVC gadget driver previously [0].
> > >> The raceiness stems from the fact that things can happen in between
> > >> returning USB_GADGET_DELAYED_STATUS and the composite layer reacting to
> > >> it - especially if usb_composite_setup_continue is called within that
> > >> window it causes a WARN. In any case, the fact that the mechanism itself
> > >> is racey suggests that it needs improvement, and using it wouldn't be a
> > >> good solution in this case.
I don't understand this at all. The composite layer reacts to
USB_GADGET_DELAYED_STATUS as soon as it receives the return value. Can
Paul or Laurent give a more explicit example of this race?
Assuming you are correct, wouldn't it make sense to fix or eliminate
the race by changing composite.c?
> > >>> Is it a question of when the gadget driver learns that it will need to
> > >>> delay the status stage? If that's the case,
> > >>
> > >> Not really.
> > >>
> > >>> why not always return
> > >>> USB_GADGET_DELAYED_STATUS from the setup callback? Then instead of
> > >>> calling usb_ep_delay_status() when a delay is needed, you could queue
> > >>> the status request when a delay isn't needed.
> > >>
> > >> Theoretically this might work, but see the problems mentioned above.
> > >>
> > >>> As a more general solution, Felipe has said that a UDC driver should
> > >>> _never_ carry out the status stage transaction until the gadget driver
> > >>> has told it to do so. Then there would be no need for any sort of
> > >>> delay indicator.
> > >>
> > >> Yeah, but,
> > >>
> > >>> (But implementing this would require significant
> > >>> changes to a bunch of different drivers...)
> > >>
> > >> exactly :/
> >
> > add a flag to gadget structure. Something like
> > "supports_explicit_status_stage" and add a new return value
> > USB_EXPLICIT_STATUS_STAGE.
> >
> > Then, take uvc for example, implement the new setup:
> >
> > if (supports_explicit_status_stage)
> > return USB_EXPLICIT_STATUS_STAGE;
> >
> > then on dwc3 you would:
> >
> > switch (ret) {
> > case USB_EXPLICIT_STATUS_STAGE:
> > case USB_GADGET_DELAYED_STATUS:
> > wait_for_status_request_queue();
> > break;
> > default:
> > start_status_stage();
> > }
> >
> > If this works with dwc3 + uvc, then we have a good recipe on how to
> > implement for the other drivers.
>
> Given that we need to delay the status stage and not the data stage, we can't
> explicitly request the status stage through a usb request queue.
Why not? The status stage for a control-OUT transfer is simply a
zero-length IN transaction. It's easy to queue a request for such a
transaction. Is the issue that there's no way to specify the direction
of the request (hence no direct way to tell whether a zero-length
request is for the data stage or the status stage)?
Admittedly, it might be nice to provide a library routine in the UDC
core to queue such requests, since it involves a bunch of uninteresting
boilerplate operations.
> Would a new
> explicit function call work for you for that purpose ?
It would be okay, but I question whether one is really needed.
> > > Alan, Felipe, how do we move forward with this ? There are several issues
> > > with the existing control request handling mechanism in the USB gadget
> > > stack, and while Paul could work on improving the mechanism, we need to
> > > provide clear guidance regarding the direction we want to take.
> > >
> > > For reference, the issues I know about for the USB_GADGET_DELAYED_STATUS
> > > mechanism are
> > >
> > > - The mechanism is inherently racy. It relies on signaling the delay at
> > > the very end of the processing in the setup handler, which by definition
> > > occurs after the work to process the control request is queued (in the
> > > generic sense, regardless of whether this involves a kernel workqueue or
> > > passing the work to userspace). There is thus a race window after queuing
> > > the work and before signaling the delay during which the work handler
> > > could signal completion.
I'm not at all sure what you're talking about here. Do you mean this
code near the end of composite_setup()?
check_value:
/* respond with data transfer before status phase? */
if (value >= 0 && value != USB_GADGET_DELAYED_STATUS) {
req->length = value;
req->context = cdev;
req->zero = value < w_length;
value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
Plainly the code checks for USB_GADGET_DELAYED_STATUS _before_ queuing
req, not after. So where's the race?
> > We won't fix this until all functions and UDCs are converted over, but
> > it's doable.
>
> It could be fixed by signaling the delay through an explicit function call
> before queueing the work instead of through a return value though, but I agree
> that long term requesting the status stage explicitly would likely be cleaner.
Yes, I agree that relying on an implicit status stage is not a good
idea for the long term.
> > > - The mechanism is poorly documented. As Paul mentioned, comments in the
> > > code state that USB_GADGET_DELAYED_STATUS delay the "data/status stages".
> > > This is very unclear, and the only three UDCs that implement the
> > > mechanism seem to do so in different ways:
We can fix comments and documentation pretty easily. :-)
> > > - The mtu driver states in a comment that it will "handle the delay
> > > STATUS phase till receive ep_queue on ep0".
> > >
> > > - The bdc driver states in a comment that "The ep0 state will remain
> > > WAIT_FOR_DATA_START till we received ep_queue on ep0".
> > >
> > > - The dwc3 driver seems to handle USB_GADGET_DELAYED_STATUS for the
> > > SET_CONFIG request only.
> >
> > that's the only one that has needed it so far. I'm all for making status
> > stage ALWAYS explicit, that will, in the long run, simplify UDC
> > drivers and make the API easier to understand.
> >
> > > - The mechanism relies on queueing a request to the UDC to signal that it
> > > should continue with the status stage. That request can be queued either
> > > by the USB gadget function driver directly, or by the composite layer in
> > > usb_composite_setup_continue() (the latter is restricted to requests that
> > > carry no data as it sets the request length to 0). This is problematic if
> > > we want to delay the status phase after completing the data phase, in
> > > order to validate the setup phase data and the data phase data (for a
> > > control OUT request) together.
> >
> > It shouldn't cause problems, actually. Most of the issues come from the
> > fact that sometimes gadget driver just returns a success and expects UDC
> > to initiate status stage and sometimes gadget driver wants to handle
> > status stage explicitly.
>
> Requesting the status stage explicitly requires an API to do so (quite
> obviously). Your proposal is to use the usb request queue as a signal to
> continue to the next stage. My point is that this can't work for control OUT
> requests, where we may want to delay the status stage after the data is
> received by the UDC, and thus after the request is queued. We need a different
> API for that.
Why so? Consider the following logic for ep0 in the UDC:
A SETUP packet is received, for an OUT transfer.
If setup.wLength > 0 then the next request queued for ep0 is
the data stage, so it is an OUT request.
Otherwise (i.e., if setup.wLength == 0 or for the second
request queued after the SETUP is received), the next request
is the status stage, so it must be a 0-length IN request.
This requires the UDC to specifically keep track of the direction of
the current transfer and whether or not a data-stage transfer has
already been queued. That shouldn't be hard. (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.)
The corresponding logic for IN transfers is simpler, since IN transfers
are not allowed to have zero length. The first request following the
SETUP packet is the data stage and the second is the status stage.
> For control IN, we may want to delay the data stage if we can't respond
> directly, or proceed with the data stage immediately. In both cases this can
> be signaled by a request being queued. There is no need to delay the status
> stage as it's initiated by the host.
Yes.
> For control OUT, we may want to delay the data stage if we need to validate
> the setup stage data asynchronously. Proceeding to the data stage can be
> signaled by queueing a request. We may also want to delay the status stage if
> we need to validate the data stage data asynchronously. This can't be signaled
> by queueing a request.
It can be, if we use the logic outlined above.
> I wonder if there's really a use case for delaying the data stage of control
> OUT requests, as it seems to me that we can perform the asynchronous
> validation of the setup and data stages together, in which case we would
> always proceed to the data stage, and only potentially delay the status stage.
> However, if we switch to an explicit API where the transition from the setup
> to the data stage is triggered by queueing a request, and given that such a
> transition may need to be delayed for the control IN case, delaying the data
> stage for control OUT would essentially come for free.
>
> In any case we need an API to delay the status stage of a control OUT request.
> There are two options here. We can consider that the status stage shouldn't be
> delayed by default and add a new function to be called from the data stage
> completion handler to request a status stage delay (using the return value of
> the completion handler isn't a good idea as it would be racy as explained
> above). A second function would be needed to request the status stage (which,
> as explained above too, can't be done by queueing a request). A second option
> is to consider that the status stage is delayed by default until explcitly
> required. In both cases the same new function is needed to request the status
> stage.
>
> Note that delaying the data stage and delaying the status stage are two
> different problems, and don't necessarily need to be solved together. However,
> if I understand things correctly, we currently delay the data stage of a few
> control OUT requests (they all have a 0 bytes data stage) as a mean to
> completion of the request. I believe that this use case could be implemented
> by delaying the status stage instead, so the two are still related in a way.
>
> If we end up moving to explicit state handling, with the data stage being
> entered by queueing a request, and the status stage being entered by calling a
> new function, control OUT requests with 0 bytes of data that can be handled
> synchronously in the setup handler would require function drivers to both
> queue a zero-length request and call the status function. This would make the
> function code more complex, and I wonder whether a shortcut would be a good
> idea, perhaps in the form of a flag in the request that tells the UDC to
> automatically proceed to the status stage immediately after the data stage. Or
> we could make that behaviour the default when the request doesn't have a
> completion handler (as moving explicitly to the status stage should be done at
> the earliest from the data stage completion handler).
>
> > > For those reasons I think a new mechanism is needed. It should either
> > > signal the status phase delay through an explicit function call instead
> > > of a return value (to solve the race mentioned above), or by requiring
> > > all requests to be explicitly completed (but that will require changing
> > > all USB function drivers). Furthermore, the mechanism need to support
> > > delaying the status phase after queuing the request for the data phase,
> > > so we need an explicit way to signal that the UDC should proceed with the
> > > status phase, other than queueing the request.
> > >
> > > Thoughts ? Preferences ?
> >
> > how about making status stage always explicit?
>
> If we implement a proof of concept, could you help us converting drivers over
> to the new API ? I'll assume we'll have to address all UDCs first, and then
> the function drivers.
I'll certainly help for the drivers I'm familiar with.
Alan Stern
Hi Alan,
On Friday, 2 November 2018 18:18:45 EET Alan Stern wrote:
> On Fri, 2 Nov 2018, Laurent Pinchart wrote:
> > On Friday, 2 November 2018 15:17:20 EET Felipe Balbi wrote:
> >> Laurent Pinchart writes:
> >>>>>>> +void usb_ep_delay_status(struct usb_ep *ep)
> >>>>>>>
> >>>>>>> +{
> >>>>>>> + ep->delayed_status = true;
> >>>>>>> +}
> >>>>>>> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> >>>>>>
> >>>>>> Is usb_ep_set_delay_status() better? I thought it implies
> >>>>>> get/return action if a verb is missing in the function name.
> >>>>>
> >>>>> For what it's worth, I understand the function name as "delay the
> >>>>> status stage", with "delay" being a verb. Maybe the short
> >>>>> description could be updated accordingly.
> >>>>>
> >>>>> Is there a reason for adding a new function for this? This is
> >>>>> exactly what the USB_GADGET_DELAYED_STATUS return value from the setup
> >>>>> callback is meant for (and it is already used by some gadget drivers).
> >>>>
> >>>> In theory, we might be able to use USB_GADGET_DELAYED_STATUS for
> >>>> this. However, there are a few ambiguities that prevent us from doing
> >>>> so.
> >>>>
> >>>> First of all, we want to delay only the status stage for control OUT
> >>>> requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
> >>>> delaying the "data/status stages". Does this mean that it delays the
> >>>> status stage only or does it delay both stages? If the slash means
> >>>> "and", then we cannot use USB_GADGET_DELAYED_STATUS.
>
> The comment in composite.h is wrong; it should refer only to the status
> stage. In fact, it should refer only to the status stage for
> control-OUT transfers; there's no reason ever to delay the status stage
> of a control-IN transfer (the driver should instead delay the data
> stage if it needs to).
Agreed.
> >>>> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
> >>>> which has already been observed in the UVC gadget driver previously
> >>>> [0]. The raceiness stems from the fact that things can happen in
> >>>> between returning USB_GADGET_DELAYED_STATUS and the composite layer
> >>>> reacting to it - especially if usb_composite_setup_continue is called
> >>>> within that window it causes a WARN. In any case, the fact that the
> >>>> mechanism itself is racey suggests that it needs improvement, and using
> >>>> it wouldn't be a good solution in this case.
>
> I don't understand this at all. The composite layer reacts to
> USB_GADGET_DELAYED_STATUS as soon as it receives the return value. Can
> Paul or Laurent give a more explicit example of this race?
The composite layer only handles USB_GADGET_DELAYED_STATUS for
USB_REQ_SET_CONFIGURATION (in set_config()) and for USB_REQ_SET_INTERFACE (in
composite_setup()). It increments cdev->delayed_status immediately. Then, in
usb_composite_setup_continue(), if cdev->delayed_status is not zero, it queues
a ZLP, and warns otherwise.
This mechanism delays the data stage, not the status stage (or, to be precise,
it delays the status stage insofar as the status stage comes after the data
stage), and only supports control OUT requests with 0 bytes of data (which is
the case of both USB_REQ_SET_INTERFACE and USB_REQ_SET_CONFIGURATION). For all
other requests, the composite layer passes USB_GADGET_DELAYED_STATUS to the
UDC.
The three UDCs that implement USB_GADGET_DELAYED_STATUS support set a
delayed_status flag in an internal structure. I haven't inspected in details
what they do next as I'm not familiar with all of them, but the dwc3 driver
just skips the handling of the status phase in dwc3_ep0_xfernotready() and
delays it to __dwc3_gadget_ep0_queue(). This only works for 0-length requests,
with no data phase.
Even when limited to 0-length control OUT requests, this mechanism is racy.
The setup handler, when it wants to delay the status phase, will queue
asynchronous work that will, when it completes, call
usb_composite_setup_continue() to proceed with the status phase. Queuing the
work has to be done before the setup handler returns, and the cdev-
>delayed_status is only incremented after the setup handler returns, in
composite_setup(). There is thus a time window during which the asynchronous
work can call usb_composite_setup_continue() before cdev->delayed_status has
been incremented. We have managed to hit this in practice, with a surprisingly
high rate seeing how small the window is.
Now that I've written all this, I realize that cdev->delayed_status is guarded
by cdev->lock. I thus wonder whether our analysis was correct, or if we were
hitting a different bug :-S Paul, could you test this again ? Please note,
however, that the race described here is not related to this patch series,
except in how it influences the API design to avoid race conditions.
> Assuming you are correct, wouldn't it make sense to fix or eliminate
> the race by changing composite.c?
I was about to write that we would need to lock access to cdev-
>delayed_status, and found out that we already use cdev->lock to do so. More
investigations are needed.
Please note, however, that USB_GADGET_DELAYED_STATUS is limited to 0-length
control OUT requests, so the problem that led to this patch series still
exists, even if the race condition I thought was there doesn't exist.
> >>>>> Is it a question of when the gadget driver learns that it will need
> >>>>> to delay the status stage? If that's the case,
> >>>>
> >>>> Not really.
> >>>>
> >>>>> why not always return
> >>>>> USB_GADGET_DELAYED_STATUS from the setup callback? Then instead of
> >>>> calling usb_ep_delay_status() when a delay is needed, you could
> >>>>> queue the status request when a delay isn't needed.
> >>>>
> >>>> Theoretically this might work, but see the problems mentioned above.
> >>>>
> >>>>> As a more general solution, Felipe has said that a UDC driver should
> >>>>> _never_ carry out the status stage transaction until the gadget
> >>>>> driver has told it to do so. Then there would be no need for any sort
> >>>>> of delay indicator.
> >>>>
> >>>> Yeah, but,
> >>>>
> >>>>> (But implementing this would require significant
> >>>>> changes to a bunch of different drivers...)
> >>>>
> >>>> exactly :/
> >>
> >> add a flag to gadget structure. Something like
> >> "supports_explicit_status_stage" and add a new return value
> >> USB_EXPLICIT_STATUS_STAGE.
> >>
> >> Then, take uvc for example, implement the new setup:
> >>
> >> if (supports_explicit_status_stage)
> >>
> >> return USB_EXPLICIT_STATUS_STAGE;
> >>
> >> then on dwc3 you would:
> >>
> >> switch (ret) {
> >> case USB_EXPLICIT_STATUS_STAGE:
> >>
> >> case USB_GADGET_DELAYED_STATUS:
> >> wait_for_status_request_queue();
> >>
> >> break;
> >>
> >> default:
> >> start_status_stage();
> >>
> >> }
> >>
> >> If this works with dwc3 + uvc, then we have a good recipe on how to
> >> implement for the other drivers.
> >
> > Given that we need to delay the status stage and not the data stage, we
> > can't explicitly request the status stage through a usb request queue.
>
> Why not? The status stage for a control-OUT transfer is simply a
> zero-length IN transaction. It's easy to queue a request for such a
> transaction. Is the issue that there's no way to specify the direction
> of the request (hence no direct way to tell whether a zero-length
> request is for the data stage or the status stage)?
OK, I suppose we could queue a request for this, in which case we would have
to queue two requests for control OUT transfers (one for the data stage and
one for the status stage). I'm however not convinced that would be the best
API to handle the status stage, as the function driver would need to queue a
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
would be easier for both the function driver and the UDC in my opinion.
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).
> Admittedly, it might be nice to provide a library routine in the UDC
> core to queue such requests, since it involves a bunch of uninteresting
> boilerplate operations.
>
> > Would a new explicit function call work for you for that purpose ?
>
> It would be okay, but I question whether one is really needed.
I think the API would be cleaner, but it might just be a matter of taste.
> >>> Alan, Felipe, how do we move forward with this ? There are several
> >>> issues with the existing control request handling mechanism in the USB
> >>> gadget stack, and while Paul could work on improving the mechanism, we
> >>> need to provide clear guidance regarding the direction we want to
> >>> take.
> >>>
> >>> For reference, the issues I know about for the USB_GADGET_DELAYED_STATUS
> >>> mechanism are
> >>>
> >>> - The mechanism is inherently racy. It relies on signaling the delay
> >>> at the very end of the processing in the setup handler, which by
> >>> definition occurs after the work to process the control request is
> >>> queued (in the generic sense, regardless of whether this involves a
> >>> kernel workqueue or passing the work to userspace). There is thus a race
> >>> window after queuing the work and before signaling the delay during
> >>> which the work handler could signal completion.
>
> I'm not at all sure what you're talking about here. Do you mean this
> code near the end of composite_setup()?
>
> check_value:
> /* respond with data transfer before status phase? */
> if (value >= 0 && value != USB_GADGET_DELAYED_STATUS) {
> req->length = value;
> req->context = cdev;
> req->zero = value < w_length;
> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
>
> Plainly the code checks for USB_GADGET_DELAYED_STATUS _before_ queuing
> req, not after. So where's the race?
Please see above.
> >> We won't fix this until all functions and UDCs are converted over, but
> >> it's doable.
> >
> > It could be fixed by signaling the delay through an explicit function call
> > before queueing the work instead of through a return value though, but I
> > agree that long term requesting the status stage explicitly would likely
> > be cleaner.
>
> Yes, I agree that relying on an implicit status stage is not a good
> idea for the long term.
>
> >>> - The mechanism is poorly documented. As Paul mentioned, comments in
> >>> the code state that USB_GADGET_DELAYED_STATUS delay the "data/status
> >>> stages". This is very unclear, and the only three UDCs that implement
> >>> the mechanism seem to do so in different ways:
>
> We can fix comments and documentation pretty easily. :-)
It's harder to fix them if different implementations interpret them in
different ways :-) That might not be the case though, as mentioned above I
haven't studied the three UDCs that implement this in details, I only had a
look at the dwc3.
> >>> - The mtu driver states in a comment that it will "handle the delay
> >>> STATUS phase till receive ep_queue on ep0".
> >>>
> >>> - The bdc driver states in a comment that "The ep0 state will remain
> >>> WAIT_FOR_DATA_START till we received ep_queue on ep0".
> >>>
> >>> - The dwc3 driver seems to handle USB_GADGET_DELAYED_STATUS for the
> >>> SET_CONFIG request only.
> >>
> >> that's the only one that has needed it so far. I'm all for making status
> >> stage ALWAYS explicit, that will, in the long run, simplify UDC
> >> drivers and make the API easier to understand.
> >>
> >>> - The mechanism relies on queueing a request to the UDC to signal that
> >>> it should continue with the status stage. That request can be queued
> >>> either by the USB gadget function driver directly, or by the composite
> >>> layer in usb_composite_setup_continue() (the latter is restricted to
> >>> requests that carry no data as it sets the request length to 0). This is
> >>> problematic if we want to delay the status phase after completing the
> >>> data phase, in order to validate the setup phase data and the data phase
> >>> data (for a control OUT request) together.
> >>
> >> It shouldn't cause problems, actually. Most of the issues come from the
> >> fact that sometimes gadget driver just returns a success and expects UDC
> >> to initiate status stage and sometimes gadget driver wants to handle
> >> status stage explicitly.
> >
> > Requesting the status stage explicitly requires an API to do so (quite
> > obviously). Your proposal is to use the usb request queue as a signal to
> > continue to the next stage. My point is that this can't work for control
> > OUT requests, where we may want to delay the status stage after the data
> > is received by the UDC, and thus after the request is queued. We need a
> > different API for that.
>
> Why so? Consider the following logic for ep0 in the UDC:
>
> A SETUP packet is received, for an OUT transfer.
>
> If setup.wLength > 0 then the next request queued for ep0 is
> the data stage, so it is an OUT request.
>
> Otherwise (i.e., if setup.wLength == 0 or for the second
> request queued after the SETUP is received), the next request
> is the status stage, so it must be a 0-length IN request.
As explained above, yes, I agree that we could use the request queue operation
both to queue a request for the data stage, and to request processing the
status stage. I still think a separate function would be best to request
processing the status stage (at the very least as a helper that would queue
the request), but I could be convinced otherwise.
> This requires the UDC to specifically keep track of the direction of
> the current transfer and whether or not a data-stage transfer has
> already been queued. That shouldn't be hard.
It's "just" a state machine so it wouldn't be too hard. What we need to agree
on is how the state machine operates, and then the API to control it. That's
what I tried to describe below in my previous e-mail.
> (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.)
I wonder how many UDCs handle that race correctly today :-)
> The corresponding logic for IN transfers is simpler, since IN transfers
> are not allowed to have zero length. The first request following the
> SETUP packet is the data stage and the second is the status stage.
>
> > For control IN, we may want to delay the data stage if we can't respond
> > directly, or proceed with the data stage immediately. In both cases this
> > can be signaled by a request being queued. There is no need to delay the
> > status stage as it's initiated by the host.
>
> Yes.
>
> > For control OUT, we may want to delay the data stage if we need to
> > validate the setup stage data asynchronously. Proceeding to the data stage
> > can be signaled by queueing a request. We may also want to delay the
> > status stage if we need to validate the data stage data asynchronously.
> > This can't be signaled by queueing a request.
>
> It can be, if we use the logic outlined above.
>
> > I wonder if there's really a use case for delaying the data stage of
> > control OUT requests, as it seems to me that we can perform the
> > asynchronous validation of the setup and data stages together, in which
> > case we would always proceed to the data stage, and only potentially
> > delay the status stage. However, if we switch to an explicit API where
> > the transition from the setup to the data stage is triggered by queueing
> > a request, and given that such a transition may need to be delayed for
> > the control IN case, delaying the data stage for control OUT would
> > essentially come for free.
What do you think about this ? Should we allow function drivers to delay the
data stage of control OUT requests ?
> > In any case we need an API to delay the status stage of a control OUT
> > request. There are two options here. We can consider that the status
> > stage shouldn't be delayed by default and add a new function to be called
> > from the data stage completion handler to request a status stage delay
> > (using the return value of the completion handler isn't a good idea as it
> > would be racy as explained above). A second function would be needed to
> > request the status stage (which, as explained above too, can't be done by
> > queueing a request). A second option is to consider that the status stage
> > is delayed by default until explcitly required. In both cases the same
> > new function is needed to request the status stage.
> >
> > Note that delaying the data stage and delaying the status stage are two
> > different problems, and don't necessarily need to be solved together.
> > However, if I understand things correctly, we currently delay the data
> > stage of a few control OUT requests (they all have a 0 bytes data stage)
> > as a mean to completion of the request. I believe that this use case
> > could be implemented by delaying the status stage instead, so the two are
> > still related in a way.
> >
> > If we end up moving to explicit state handling, with the data stage being
> > entered by queueing a request, and the status stage being entered by
> > calling a new function, control OUT requests with 0 bytes of data that
> > can be handled synchronously in the setup handler would require function
> > drivers to both queue a zero-length request and call the status function.
> > This would make the function code more complex, and I wonder whether a
> > shortcut would be a good idea, perhaps in the form of a flag in the
> > request that tells the UDC to automatically proceed to the status stage
> > immediately after the data stage. Or we could make that behaviour the
> > default when the request doesn't have a completion handler (as moving
> > explicitly to the status stage should be done at the earliest from the
> > data stage completion handler).
From an API point of view, towards function drivers, I really want an explicit
function to proceed with the status stage. That could internally queue a ZLP
request or call another API, but in any case I don't want the status stage ZLP
request to be visible to the function drivers. Do you agree with this ?
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
to the status stage when the flag is not set. Essentially this would call the
status stage request function right after the data stage request completion
handler returns, instead of forcing all function drivers to do so explicitly
at the end of the completion handler.
> >>> For those reasons I think a new mechanism is needed. It should either
> >>> signal the status phase delay through an explicit function call
> >>> instead of a return value (to solve the race mentioned above), or by
> >>> requiring all requests to be explicitly completed (but that will require
> >>> changing all USB function drivers). Furthermore, the mechanism need to
> >>> support delaying the status phase after queuing the request for the data
> >>> phase, so we need an explicit way to signal that the UDC should proceed
> >>> with the status phase, other than queueing the request.
> >>>
> >>> Thoughts ? Preferences ?
> >>
> >> how about making status stage always explicit?
> >
> > If we implement a proof of concept, could you help us converting drivers
> > over to the new API ? I'll assume we'll have to address all UDCs first,
> > and then the function drivers.
>
> I'll certainly help for the drivers I'm familiar with.
Thank you.
--
Regards,
Laurent Pinchart
On Fri, 2 Nov 2018, Laurent Pinchart wrote:
> Hi Alan,
Hi, Laurent.
> The composite layer only handles USB_GADGET_DELAYED_STATUS for
> USB_REQ_SET_CONFIGURATION (in set_config()) and for USB_REQ_SET_INTERFACE (in
> composite_setup()). It increments cdev->delayed_status immediately. Then, in
> usb_composite_setup_continue(), if cdev->delayed_status is not zero, it queues
> a ZLP, and warns otherwise.
>
> This mechanism delays the data stage, not the status stage (or, to be precise,
> it delays the status stage insofar as the status stage comes after the data
> stage), and only supports control OUT requests with 0 bytes of data (which is
> the case of both USB_REQ_SET_INTERFACE and USB_REQ_SET_CONFIGURATION). For all
> other requests, the composite layer passes USB_GADGET_DELAYED_STATUS to the
> UDC.
>
> The three UDCs that implement USB_GADGET_DELAYED_STATUS support set a
> delayed_status flag in an internal structure. I haven't inspected in details
> what they do next as I'm not familiar with all of them, but the dwc3 driver
> just skips the handling of the status phase in dwc3_ep0_xfernotready() and
> delays it to __dwc3_gadget_ep0_queue(). This only works for 0-length requests,
> with no data phase.
>
> Even when limited to 0-length control OUT requests, this mechanism is racy.
> The setup handler, when it wants to delay the status phase, will queue
> asynchronous work that will, when it completes, call
> usb_composite_setup_continue() to proceed with the status phase. Queuing the
> work has to be done before the setup handler returns, and the cdev-
> >delayed_status is only incremented after the setup handler returns, in
> composite_setup(). There is thus a time window during which the asynchronous
> work can call usb_composite_setup_continue() before cdev->delayed_status has
> been incremented. We have managed to hit this in practice, with a surprisingly
> high rate seeing how small the window is.
I see. Thanks for the detailed explanation.
> Now that I've written all this, I realize that cdev->delayed_status is guarded
> by cdev->lock. I thus wonder whether our analysis was correct, or if we were
> hitting a different bug :-S Paul, could you test this again ? Please note,
> however, that the race described here is not related to this patch series,
> except in how it influences the API design to avoid race conditions.
Perhaps the async work routine doesn't acquire cdev->lock.
> > Assuming you are correct, wouldn't it make sense to fix or eliminate
> > the race by changing composite.c?
>
> I was about to write that we would need to lock access to cdev-
> >delayed_status, and found out that we already use cdev->lock to do so. More
> investigations are needed.
>
> Please note, however, that USB_GADGET_DELAYED_STATUS is limited to 0-length
> control OUT requests, so the problem that led to this patch series still
> exists, even if the race condition I thought was there doesn't exist.
Yes, it is too limited.
> > > Given that we need to delay the status stage and not the data stage, we
> > > can't explicitly request the status stage through a usb request queue.
> >
> > Why not? The status stage for a control-OUT transfer is simply a
> > zero-length IN transaction. It's easy to queue a request for such a
> > transaction. Is the issue that there's no way to specify the direction
> > of the request (hence no direct way to tell whether a zero-length
> > request is for the data stage or the status stage)?
>
> OK, I suppose we could queue a request for this, in which case we would have
> to queue two requests for control OUT transfers (one for the data stage and
> one for the status stage). I'm however not convinced that would be the best
> API to handle the status stage, as the function driver would need to queue a
> 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
> would be easier for both the function driver and the UDC in my opinion.
> 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).
>
> > Admittedly, it might be nice to provide a library routine in the UDC
> > core to queue such requests, since it involves a bunch of uninteresting
> > boilerplate operations.
> >
> > > Would a new explicit function call work for you for that purpose ?
> >
> > It would be okay, but I question whether one is really needed.
>
> I think the API would be cleaner, but it might just be a matter of taste.
I have to agree that a separate function would be cleaner. Whether
this function should be part of the gadget API or just a core library
routine is something we need to decide.
> > (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.)
>
> I wonder how many UDCs handle that race correctly today :-)
Probably none of them. I can think of two ways of doing it:
Have the UDC driver associate a new integer tag with each
SETUP packet, and have function drivers include the
corresponding tag when they queue a request on ep0. Then the
UDC driver could refuse requests whose tag was out of date.
Create an new API routine for function drivers to call in
their setup handler. This routine would acknowledge receipt
of the new SETUP packet, assuring the UDC driver that no more
requests would be queued in response to earlier SETUPs. Until
the routine was called, the UDC would refuse all requests for
ep0. Function drivers would be responsible for their own
internal synchronization.
Both of these would involve nontrivial API changes, although the first
could be relatively uninvasive. I doubt either of them is worthwhile
at this point.
There's a similar race at the hardware level. What happens if the
controller receives a new SETUP packet and concurrently the driver is
setting up the controller registers for a response to an earlier
SETUP? I don't know how real controllers handle this.
> > > I wonder if there's really a use case for delaying the data stage of
> > > control OUT requests, as it seems to me that we can perform the
> > > asynchronous validation of the setup and data stages together, in which
> > > case we would always proceed to the data stage, and only potentially
> > > delay the status stage. However, if we switch to an explicit API where
> > > the transition from the setup to the data stage is triggered by queueing
> > > a request, and given that such a transition may need to be delayed for
> > > the control IN case, delaying the data stage for control OUT would
> > > essentially come for free.
>
> What do you think about this ? Should we allow function drivers to delay the
> data stage of control OUT requests ?
You mean, should we allow function drivers to queue the data-stage
request after the setup handler has returned? I don't see any reason
why not. After all, some drivers may require this. Likewise for the
data stage of a control-IN.
Another thing we should do is give function drivers a way to send a
STALL response for the status stage. Currently there's no way to do
it, if a data stage is present.
> From an API point of view, towards function drivers, I really want an explicit
> function to proceed with the status stage. That could internally queue a ZLP
> request or call another API, but in any case I don't want the status stage ZLP
> request to be visible to the function drivers. Do you agree with this ?
It's okay with me.
> 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
> to the status stage when the flag is not set. Essentially this would call the
> status stage request function right after the data stage request completion
> handler returns, instead of forcing all function drivers to do so explicitly
> at the end of the completion handler.
That makes sense. Function drivers then wouldn't have to be aware of
the new API. We'd only need to convert the UDC drivers (plus the users
of USB_GADGET_DELAYED_STATUS).
Should control-IN transfers use the flag also? I can't imagine why
anyone would want to delay or otherwise alter a control-IN status
stage.
Alan Stern
Hi,
Laurent Pinchart <[email protected]> writes:
>> >>>> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
>> >>>> which has already been observed in the UVC gadget driver previously
>> >>>> [0]. The raceiness stems from the fact that things can happen in
>> >>>> between returning USB_GADGET_DELAYED_STATUS and the composite layer
>> >>>> reacting to it - especially if usb_composite_setup_continue is called
>> >>>> within that window it causes a WARN. In any case, the fact that the
>> >>>> mechanism itself is racey suggests that it needs improvement, and using
>> >>>> it wouldn't be a good solution in this case.
>>
>> I don't understand this at all. The composite layer reacts to
>> USB_GADGET_DELAYED_STATUS as soon as it receives the return value. Can
>> Paul or Laurent give a more explicit example of this race?
>
> The composite layer only handles USB_GADGET_DELAYED_STATUS for
> USB_REQ_SET_CONFIGURATION (in set_config()) and for USB_REQ_SET_INTERFACE (in
> composite_setup()). It increments cdev->delayed_status immediately. Then, in
> usb_composite_setup_continue(), if cdev->delayed_status is not zero, it queues
> a ZLP, and warns otherwise.
>
> This mechanism delays the data stage, not the status stage (or, to be precise,
> it delays the status stage insofar as the status stage comes after the data
> stage), and only supports control OUT requests with 0 bytes of data (which is
> the case of both USB_REQ_SET_INTERFACE and USB_REQ_SET_CONFIGURATION). For all
> other requests, the composite layer passes USB_GADGET_DELAYED_STATUS to the
> UDC.
DATA stage always depends on a usb_ep_queue() from gadget driver. So
it's always "delayed" in that sense.
> The three UDCs that implement USB_GADGET_DELAYED_STATUS support set a
> delayed_status flag in an internal structure. I haven't inspected in details
> what they do next as I'm not familiar with all of them, but the dwc3 driver
> just skips the handling of the status phase in dwc3_ep0_xfernotready() and
> delays it to __dwc3_gadget_ep0_queue(). This only works for 0-length requests,
> with no data phase.
data stage always depends on usb_ep_queue(). There was never any need
for UDC to handle Data stage internally. Also, status stage is always a ZLP.
> Even when limited to 0-length control OUT requests, this mechanism is racy.
> The setup handler, when it wants to delay the status phase, will queue
> asynchronous work that will, when it completes, call
> usb_composite_setup_continue() to proceed with the status phase. Queuing the
> work has to be done before the setup handler returns, and the cdev-
>>delayed_status is only incremented after the setup handler returns, in
> composite_setup(). There is thus a time window during which the asynchronous
> work can call usb_composite_setup_continue() before cdev->delayed_status has
> been incremented. We have managed to hit this in practice, with a surprisingly
> high rate seeing how small the window is.
that's only the case because we have two different "modes" for this. One
where UDC handles it internally and another where gadget driver has to
queue a request. I'm vouching for making status stage always explicit,
i.e. we should always expect a usb_ep_queue().
> Now that I've written all this, I realize that cdev->delayed_status is guarded
> by cdev->lock. I thus wonder whether our analysis was correct, or if we were
> hitting a different bug :-S Paul, could you test this again ? Please note,
> however, that the race described here is not related to this patch series,
> except in how it influences the API design to avoid race conditions.
>
>> Assuming you are correct, wouldn't it make sense to fix or eliminate
>> the race by changing composite.c?
>
> I was about to write that we would need to lock access to cdev-
>>delayed_status, and found out that we already use cdev->lock to do so. More
> investigations are needed.
>
> Please note, however, that USB_GADGET_DELAYED_STATUS is limited to 0-length
> control OUT requests, so the problem that led to this patch series still
> exists, even if the race condition I thought was there doesn't exist.
And that problem is...?
DATA stage always depends on a usb_ep_queue() from gadget driver.
>> >> If this works with dwc3 + uvc, then we have a good recipe on how to
>> >> implement for the other drivers.
>> >
>> > Given that we need to delay the status stage and not the data stage, we
>> > can't explicitly request the status stage through a usb request queue.
>>
>> Why not? The status stage for a control-OUT transfer is simply a
>> zero-length IN transaction. It's easy to queue a request for such a
>> transaction. Is the issue that there's no way to specify the direction
>> of the request (hence no direct way to tell whether a zero-length
>> request is for the data stage or the status stage)?
>
> OK, I suppose we could queue a request for this, in which case we would have
> to queue two requests for control OUT transfers (one for the data stage and
> one for the status stage). I'm however not convinced that would be the best
that's correct. This is what "make status stage always explicit" mean :)
> API to handle the status stage, as the function driver would need to queue a
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...
> 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();
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)
> would be easier for both the function driver and the UDC in my opinion.
it wouldn't. We would just be moving the special case to another
function, rather than eliminating it.
> 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.
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
>> Admittedly, it might be nice to provide a library routine in the UDC
>> core to queue such requests, since it involves a bunch of uninteresting
>> boilerplate operations.
>>
>> > Would a new explicit function call work for you for that purpose ?
>>
>> It would be okay, but I question whether one is really needed.
>
> I think the API would be cleaner, but it might just be a matter of taste.
From a UDC perspective, I'm more inclined to removing special cases,
rather than making them more apparent. Having a single method for
handling all three stages of a control transfer, IMO, is far more
beneficial as it removes magic return values and several branches on UDC
driver.
>> >>> - The mechanism is poorly documented. As Paul mentioned, comments in
>> >>> the code state that USB_GADGET_DELAYED_STATUS delay the "data/status
>> >>> stages". This is very unclear, and the only three UDCs that implement
>> >>> the mechanism seem to do so in different ways:
>>
>> We can fix comments and documentation pretty easily. :-)
>
> It's harder to fix them if different implementations interpret them in
> different ways :-) That might not be the case though, as mentioned above I
that's where a proper audit of the code comes into play :)
> haven't studied the three UDCs that implement this in details, I only had a
> look at the dwc3.
that's perfectly fine. We can Cc other folks involved with the other
UDCs and have them chip in.
>> This requires the UDC to specifically keep track of the direction of
>> the current transfer and whether or not a data-stage transfer has
>> already been queued. That shouldn't be hard.
>
> It's "just" a state machine so it wouldn't be too hard. What we need to agree
> on is how the state machine operates, and then the API to control it. That's
> what I tried to describe below in my previous e-mail.
>
>> (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. Driver is already required to
handle stalls for several other conditions. If thehre are bugs in that
area, I'd prefer catching them.
>> > I wonder if there's really a use case for delaying the data stage of
>> > control OUT requests, as it seems to me that we can perform the
>> > asynchronous validation of the setup and data stages together, in which
>> > case we would always proceed to the data stage, and only potentially
>> > delay the status stage. However, if we switch to an explicit API where
>> > the transition from the setup to the data stage is triggered by queueing
>> > a request, and given that such a transition may need to be delayed for
>> > the control IN case, delaying the data stage for control OUT would
>> > essentially come for free.
>
> What do you think about this ? Should we allow function drivers to delay the
> data stage of control OUT requests ?
it's already delayed. UDC won't start data stage unless it has a buffer
to put the data. Without a usb_ep_queue(), UDC doesn't have a buffer.
>> > If we end up moving to explicit state handling, with the data stage being
>> > entered by queueing a request, and the status stage being entered by
>> > calling a new function, control OUT requests with 0 bytes of data that
>> > can be handled synchronously in the setup handler would require function
>> > drivers to both queue a zero-length request and call the status function.
>> > This would make the function code more complex, and I wonder whether a
>> > shortcut would be a good idea, perhaps in the form of a flag in the
>> > request that tells the UDC to automatically proceed to the status stage
>> > immediately after the data stage. Or we could make that behaviour the
>> > default when the request doesn't have a completion handler (as moving
>> > explicitly to the status stage should be done at the earliest from the
>> > data stage completion handler).
>
> From an API point of view, towards function drivers, I really want an explicit
> function to proceed with the status stage. That could internally queue a ZLP
> request or call another API, but in any case I don't want the status stage ZLP
> request to be visible to the function drivers. Do you agree with this ?
why can't it be visible? I don't mind having a function wrapping
usb_ep_queue(), but why is it bad to have functions call usb_ep_queue()
directly?
> 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.
--
balbi
Hi,
Alan Stern <[email protected]> writes:
> There's a similar race at the hardware level. What happens if the
> controller receives a new SETUP packet and concurrently the driver is
> setting up the controller registers for a response to an earlier
> SETUP? I don't know how real controllers handle this.
That's HW implementation detail. DWC3, for instance, will ignore the
TRBs and return me the status "setup packet pending". Then I just start
a new SETUP TRB.
>> > > I wonder if there's really a use case for delaying the data stage of
>> > > control OUT requests, as it seems to me that we can perform the
>> > > asynchronous validation of the setup and data stages together, in which
>> > > case we would always proceed to the data stage, and only potentially
>> > > delay the status stage. However, if we switch to an explicit API where
>> > > the transition from the setup to the data stage is triggered by queueing
>> > > a request, and given that such a transition may need to be delayed for
>> > > the control IN case, delaying the data stage for control OUT would
>> > > essentially come for free.
>>
>> What do you think about this ? Should we allow function drivers to delay the
>> data stage of control OUT requests ?
>
> You mean, should we allow function drivers to queue the data-stage
> request after the setup handler has returned? I don't see any reason
that's already done:
static void dwc3_ep0_xfer_complete(struct dwc3 *dwc,
const struct dwc3_event_depevt *event)
{
struct dwc3_ep *dep = dwc->eps[event->endpoint_number];
dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
dep->resource_index = 0;
dwc->setup_packet_pending = false;
switch (dwc->ep0state) {
case EP0_SETUP_PHASE:
dwc3_ep0_inspect_setup(dwc, event);
break;
[...]
}
static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
const struct dwc3_event_depevt *event)
{
struct usb_ctrlrequest *ctrl = (void *) dwc->ep0_trb;
int ret = -EINVAL;
u32 len;
if (!dwc->gadget_driver)
goto out;
trace_dwc3_ctrl_req(ctrl);
len = le16_to_cpu(ctrl->wLength);
if (!len) {
dwc->three_stage_setup = false;
dwc->ep0_expect_in = false;
dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
} else {
dwc->three_stage_setup = true;
dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN);
dwc->ep0_next_event = DWC3_EP0_NRDY_DATA;
}
[...]
}
static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
struct dwc3_request *req)
{
struct dwc3 *dwc = dep->dwc;
req->request.actual = 0;
req->request.status = -EINPROGRESS;
req->epnum = dep->number;
list_add_tail(&req->list, &dep->pending_list);
[...]
if (dwc->three_stage_setup) {
unsigned direction;
direction = dwc->ep0_expect_in;
dwc->ep0state = EP0_DATA_PHASE;
__dwc3_ep0_do_control_data(dwc, dwc->eps[direction], req);
dep->flags &= ~DWC3_EP0_DIR_IN;
}
return 0;
}
Regardless of the direction, control data always depends on a call to
usb_ep_queue()
> why not. After all, some drivers may require this. Likewise for the
> data stage of a control-IN.
>
> Another thing we should do is give function drivers a way to send a
> STALL response for the status stage. Currently there's no way to do
> it, if a data stage is present.
Status stage can only be stalled if host tries to move data on the wrong
direction. Currently, that's handled internally by UDCs since that's
easy enough to track.
Data stage already has explicit stall handling.
--
balbi
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
On Tue, 6 Nov 2018, Felipe Balbi wrote:
>
> Hi,
>
> Alan Stern <[email protected]> writes:
> > There's a similar race at the hardware level. What happens if the
> > controller receives a new SETUP packet and concurrently the driver is
> > setting up the controller registers for a response to an earlier
> > SETUP? I don't know how real controllers handle this.
>
> That's HW implementation detail. DWC3, for instance, will ignore the
> TRBs and return me the status "setup packet pending". Then I just start
> a new SETUP TRB.
You mean the UDC hardware sets a "setup pending" flag in some register,
and then ignores any attempts to do anything with ep0 until the driver
clears this flag?
We could do something similar at the software level. In fact, that
would be one of the two proposals I outlined in an earlier email.
> > You mean, should we allow function drivers to queue the data-stage
> > request after the setup handler has returned? I don't see any reason
>
> that's already done:
>
> static void dwc3_ep0_xfer_complete(struct dwc3 *dwc,
> const struct dwc3_event_depevt *event)
> {
> struct dwc3_ep *dep = dwc->eps[event->endpoint_number];
>
> dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> dep->resource_index = 0;
> dwc->setup_packet_pending = false;
>
> switch (dwc->ep0state) {
> case EP0_SETUP_PHASE:
> dwc3_ep0_inspect_setup(dwc, event);
> break;
> [...]
> }
...
You mean, it's already done in DWC3. What about other UDC drivers?
> > why not. After all, some drivers may require this. Likewise for the
> > data stage of a control-IN.
> >
> > Another thing we should do is give function drivers a way to send a
> > STALL response for the status stage. Currently there's no way to do
> > it, if a data stage is present.
>
> Status stage can only be stalled if host tries to move data on the wrong
> direction.
The USB-2 spec disagrees. See Table 8-7 in section 8.5.3.1 and the
following paragraphs. (Although, I can't see why a function would ever
fail to complete the command sequence for a control-IN transfer after
the data had already been sent.)
Alan Stern
Hi,
Alan Stern <[email protected]> writes:
>> Alan Stern <[email protected]> writes:
>> > There's a similar race at the hardware level. What happens if the
>> > controller receives a new SETUP packet and concurrently the driver is
>> > setting up the controller registers for a response to an earlier
>> > SETUP? I don't know how real controllers handle this.
>>
>> That's HW implementation detail. DWC3, for instance, will ignore the
>> TRBs and return me the status "setup packet pending". Then I just start
>> a new SETUP TRB.
>
> You mean the UDC hardware sets a "setup pending" flag in some register,
> and then ignores any attempts to do anything with ep0 until the driver
> clears this flag?
Yes, except that the "flag" is a status on the TRB itself (TRB is dwc3's
DMA transfer descriptor).
>> > You mean, should we allow function drivers to queue the data-stage
>> > request after the setup handler has returned? I don't see any reason
>>
>> that's already done:
>>
>> static void dwc3_ep0_xfer_complete(struct dwc3 *dwc,
>> const struct dwc3_event_depevt *event)
>> {
>> struct dwc3_ep *dep = dwc->eps[event->endpoint_number];
>>
>> dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>> dep->resource_index = 0;
>> dwc->setup_packet_pending = false;
>>
>> switch (dwc->ep0state) {
>> case EP0_SETUP_PHASE:
>> dwc3_ep0_inspect_setup(dwc, event);
>> break;
>> [...]
>> }
>
> ...
>
> You mean, it's already done in DWC3. What about other UDC drivers?
if they're not implementing this possibility, then that's a bug on
those UDC drivers :)
>> > why not. After all, some drivers may require this. Likewise for the
>> > data stage of a control-IN.
>> >
>> > Another thing we should do is give function drivers a way to send a
>> > STALL response for the status stage. Currently there's no way to do
>> > it, if a data stage is present.
>>
>> Status stage can only be stalled if host tries to move data on the wrong
>> direction.
>
> The USB-2 spec disagrees. See Table 8-7 in section 8.5.3.1 and the
> following paragraphs. (Although, I can't see why a function would ever
> fail to complete the command sequence for a control-IN transfer after
> the data had already been sent.)
I can't see how we could ever STALL after returning the data requested
by the host. Seems like that wasn't well thought out.
--
balbi
Hi,
Alan Stern <[email protected]> writes:
>> 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.
but now we're speculating. Should we really care before we catch
regressions?
>> 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.)
UDC drivers already have to do that.
>> > 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.
apologies, I meant wLength, like so:
len = le16_to_cpu(ctrl->wLength);
if (!len) {
dwc->three_stage_setup = false;
dwc->ep0_expect_in = false;
dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
} else {
dwc->three_stage_setup = true;
dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN);
dwc->ep0_next_event = DWC3_EP0_NRDY_DATA;
}
>> 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.
special return values would be rendered uncessary if there's agreement
that status stage is always explicit. Why would need
USB_GADGET_DELAYED_STATUS if every case returns that?
>> > There's also the fact that requests can specify a completion handler, but only
>> > the data stage request would see its completion handler called (unless we
>> > require UDCs to call completion requests at the completion of the status
>> > stage, but I'm not sure that all UDCs can report the event to the driver, and
>> > that would likely be useless as nobody needs that feature).
>>
>> you still wanna know if the host actually processed your status
>> stage. udc-core can (and should) provide a generic status stage
>> completion function which, at a minimum, aids with some tracepoints.
>
> Helping with tracepoints is fine. However, I don't think function
> drivers really need to know whether the status stage was processed by
> the host. Can you point out any examples where such information would
> be useful?
If you know your STATUS stage completed, you have a guarantee that your
previous control transfer is complete. It's a very clear signal that you
should prepare for more control transfers.
>> >> (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.)
oh okay. This is the setup_packet_pending case.
>> > To simplify function drivers, do you think the above proposal of adding a flag
>> > to the (data stage) request to request an automatic transition to the status
>> > stage is a good idea ? We could even possibly invert the logic and transition
>>
>> no, I don't think so. Making the status phase always explicit is far
>> better. UDCs won't have to check flags, or act on magic return
>> values. It just won't do anything until a request is queued.
>
> I don't agree. This would be a simple test in a localized area (the
> completion callback for control requests). It could even be
> implemented by a library routine; the UDC driver would simply have to
> call this routine immediately after invoking the callback.
I don't follow what you mean here.
--
balbi
On Wed, 7 Nov 2018, Felipe Balbi wrote:
> Hi,
>
> Alan Stern <[email protected]> writes:
> >> Alan Stern <[email protected]> writes:
> >> > There's a similar race at the hardware level. What happens if the
> >> > controller receives a new SETUP packet and concurrently the driver is
> >> > setting up the controller registers for a response to an earlier
> >> > SETUP? I don't know how real controllers handle this.
> >>
> >> That's HW implementation detail. DWC3, for instance, will ignore the
> >> TRBs and return me the status "setup packet pending". Then I just start
> >> a new SETUP TRB.
> >
> > You mean the UDC hardware sets a "setup pending" flag in some register,
> > and then ignores any attempts to do anything with ep0 until the driver
> > clears this flag?
>
> Yes, except that the "flag" is a status on the TRB itself (TRB is dwc3's
> DMA transfer descriptor).
Hmmm. So there must be a mechanism for the driver to tell the hardware
that the endpoint's ring should start up again, right? (I'm assuming
the controller stops the ring when the SETUP is received, to avoid
taking invalid actions for TRBs that are now out of date.)
> >> > Another thing we should do is give function drivers a way to send a
> >> > STALL response for the status stage. Currently there's no way to do
> >> > it, if a data stage is present.
> >>
> >> Status stage can only be stalled if host tries to move data on the wrong
> >> direction.
> >
> > The USB-2 spec disagrees. See Table 8-7 in section 8.5.3.1 and the
> > following paragraphs. (Although, I can't see why a function would ever
> > fail to complete the command sequence for a control-IN transfer after
> > the data had already been sent.)
>
> I can't see how we could ever STALL after returning the data requested
> by the host. Seems like that wasn't well thought out.
Yes, it doesn't make a lot of sense. However, STALLing the status
stage of a control-OUT transfer does make sense, so we should be able
to do it. The obvious approach is to call ep0's set_halt() method
instead of submitting an explicit status request.
> > Checking the length isn't enough. A data stage can have 0 length.
>
> apologies, I meant wLength, like so:
>
> len = le16_to_cpu(ctrl->wLength);
> if (!len) {
> dwc->three_stage_setup = false;
> dwc->ep0_expect_in = false;
> dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
> } else {
> dwc->three_stage_setup = true;
> dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN);
> dwc->ep0_next_event = DWC3_EP0_NRDY_DATA;
> }
Presumably you invert the value of ep0_expect_in and set ep0_next_event
to DWC3_EP0_NRDY_STATUS when the next (data-stage) request is submitted
for ep0. Okay.
> special return values would be rendered uncessary if there's agreement
> that status stage is always explicit. Why would need
> USB_GADGET_DELAYED_STATUS if every case returns that?
Agreed.
> >> > There's also the fact that requests can specify a completion handler, but only
> >> > the data stage request would see its completion handler called (unless we
> >> > require UDCs to call completion requests at the completion of the status
> >> > stage, but I'm not sure that all UDCs can report the event to the driver, and
> >> > that would likely be useless as nobody needs that feature).
> >>
> >> you still wanna know if the host actually processed your status
> >> stage. udc-core can (and should) provide a generic status stage
> >> completion function which, at a minimum, aids with some tracepoints.
> >
> > Helping with tracepoints is fine. However, I don't think function
> > drivers really need to know whether the status stage was processed by
> > the host. Can you point out any examples where such information would
> > be useful?
>
> If you know your STATUS stage completed, you have a guarantee that your
> previous control transfer is complete. It's a very clear signal that you
> should prepare for more control transfers.
That doesn't seem to make sense, because in reality you don't have
this guarantee. Consider the following scenario: The host starts a
control-IN transfer. You send the data-stage request succesfully and
then submit the status-stage request. That request will complete
before the host receives the ACK for its 0-length status OUT
transaction. In fact, the host may never receive that ACK and so the
transfer may never complete.
Besides, you don't need a signal (clear or otherwise) to prepare for
more control transfers. You should start preparing as soon as the
status-stage request has been submitted. At that point, what else is
there for you to do?
For that matter, you should be prepared to receive a new setup callback
at any time. The host doesn't have to wait for an old control transfer
to complete before starting a new one.
I just don't see any value in knowing the completion code of a
status-stage request.
> >> > To simplify function drivers, do you think the above proposal of adding a flag
> >> > to the (data stage) request to request an automatic transition to the status
> >> > stage is a good idea ? We could even possibly invert the logic and transition
> >>
> >> no, I don't think so. Making the status phase always explicit is far
> >> better. UDCs won't have to check flags, or act on magic return
> >> values. It just won't do anything until a request is queued.
> >
> > I don't agree. This would be a simple test in a localized area (the
> > completion callback for control requests). It could even be
> > implemented by a library routine; the UDC driver would simply have to
> > call this routine immediately after invoking the callback.
>
> I don't follow what you mean here.
Suppose we have a core library routine like this:
void usb_gadget_control_complete(struct usb_gadget *gadget,
unsigned int no_implicit_status, int status)
{
struct usb_request *req;
if (no_implicit_status || status != 0)
return;
/* Send an implicit status-stage request for ep0 */
req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
if (req) {
req->length = 0;
req->no_implicit_status = 1;
req->complete = /* req's deallocation routine */
usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
}
}
Then all a UDC driver would need to do is call
usb_gadget_control_complete() after invoking a control request's
completion handler. The no_implicit_status and status arguments would
be taken from the request that was just completed.
With this one call added to each UDC, all the existing function drivers
would work correctly. Even though they don't explicitly queue
status-stage requests, the new routine will do so for them,
transparently. Function drivers that want to handle their own
status-stage requests explicitly will merely have to set the
req->no_implicit_status bit.
(We might or might not need to watch out for 0-length control-OUT
transfers. Function drivers _do_ queue status-stage requests for
those.)
Alan Stern
[snip]
> > >> > Another thing we should do is give function drivers a way to send a
> > >> > STALL response for the status stage. Currently there's no way to do
> > >> > it, if a data stage is present.
> > >>
> > >> Status stage can only be stalled if host tries to move data on the wrong
> > >> direction.
> > >
> > > The USB-2 spec disagrees. See Table 8-7 in section 8.5.3.1 and the
> > > following paragraphs. (Although, I can't see why a function would ever
> > > fail to complete the command sequence for a control-IN transfer after
> > > the data had already been sent.)
> >
> > I can't see how we could ever STALL after returning the data requested
> > by the host. Seems like that wasn't well thought out.
>
> Yes, it doesn't make a lot of sense. However, STALLing the status
> stage of a control-OUT transfer does make sense, so we should be able
> to do it. The obvious approach is to call ep0's set_halt() method
> instead of submitting an explicit status request.
Exactly, that's what we want to be able to do.
> > > Checking the length isn't enough. A data stage can have 0 length.
> >
> > apologies, I meant wLength, like so:
> >
> > len = le16_to_cpu(ctrl->wLength);
> > if (!len) {
> > dwc->three_stage_setup = false;
> > dwc->ep0_expect_in = false;
> > dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
> > } else {
> > dwc->three_stage_setup = true;
> > dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN);
> > dwc->ep0_next_event = DWC3_EP0_NRDY_DATA;
> > }
>
> Presumably you invert the value of ep0_expect_in and set ep0_next_event
> to DWC3_EP0_NRDY_STATUS when the next (data-stage) request is submitted
> for ep0. Okay.
>
> > special return values would be rendered uncessary if there's agreement
> > that status stage is always explicit. Why would need
> > USB_GADGET_DELAYED_STATUS if every case returns that?
>
> Agreed.
>
> > >> > There's also the fact that requests can specify a completion handler, but only
> > >> > the data stage request would see its completion handler called (unless we
> > >> > require UDCs to call completion requests at the completion of the status
> > >> > stage, but I'm not sure that all UDCs can report the event to the driver, and
> > >> > that would likely be useless as nobody needs that feature).
> > >>
> > >> you still wanna know if the host actually processed your status
> > >> stage. udc-core can (and should) provide a generic status stage
> > >> completion function which, at a minimum, aids with some tracepoints.
> > >
> > > Helping with tracepoints is fine. However, I don't think function
> > > drivers really need to know whether the status stage was processed by
> > > the host. Can you point out any examples where such information would
> > > be useful?
> >
> > If you know your STATUS stage completed, you have a guarantee that your
> > previous control transfer is complete. It's a very clear signal that you
> > should prepare for more control transfers.
>
> That doesn't seem to make sense, because in reality you don't have
> this guarantee. Consider the following scenario: The host starts a
> control-IN transfer. You send the data-stage request succesfully and
> then submit the status-stage request. That request will complete
> before the host receives the ACK for its 0-length status OUT
> transaction. In fact, the host may never receive that ACK and so the
> transfer may never complete.
>
> Besides, you don't need a signal (clear or otherwise) to prepare for
> more control transfers. You should start preparing as soon as the
> status-stage request has been submitted. At that point, what else is
> there for you to do?
>
> For that matter, you should be prepared to receive a new setup callback
> at any time. The host doesn't have to wait for an old control transfer
> to complete before starting a new one.
>
> I just don't see any value in knowing the completion code of a
> status-stage request.
I agree.
> > >> > To simplify function drivers, do you think the above proposal of adding a flag
> > >> > to the (data stage) request to request an automatic transition to the status
> > >> > stage is a good idea ? We could even possibly invert the logic and transition
> > >>
> > >> no, I don't think so. Making the status phase always explicit is far
> > >> better. UDCs won't have to check flags, or act on magic return
> > >> values. It just won't do anything until a request is queued.
> > >
> > > I don't agree. This would be a simple test in a localized area (the
> > > completion callback for control requests). It could even be
> > > implemented by a library routine; the UDC driver would simply have to
> > > call this routine immediately after invoking the callback.
> >
> > I don't follow what you mean here.
>
> Suppose we have a core library routine like this:
>
> void usb_gadget_control_complete(struct usb_gadget *gadget,
> unsigned int no_implicit_status, int status)
> {
> struct usb_request *req;
>
> if (no_implicit_status || status != 0)
> return;
>
> /* Send an implicit status-stage request for ep0 */
> req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
> if (req) {
> req->length = 0;
> req->no_implicit_status = 1;
> req->complete = /* req's deallocation routine */
> usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
> }
> }
>
> Then all a UDC driver would need to do is call
> usb_gadget_control_complete() after invoking a control request's
> completion handler. The no_implicit_status and status arguments would
> be taken from the request that was just completed.
>
> With this one call added to each UDC, all the existing function drivers
> would work correctly. Even though they don't explicitly queue
> status-stage requests, the new routine will do so for them,
> transparently. Function drivers that want to handle their own
> status-stage requests explicitly will merely have to set the
> req->no_implicit_status bit.
I think this is a good idea. We still get the benefits of explicit
status stage without being overly intrusive in the conversion, and we
maintain the queue-based API.
Would it be fine for me to proceed in this direction for a v2?
> (We might or might not need to watch out for 0-length control-OUT
> transfers. Function drivers _do_ queue status-stage requests for
> those.)
Thanks,
Paul Elder
On Thu, 13 Dec 2018, Paul Elder wrote:
> > Suppose we have a core library routine like this:
> >
> > void usb_gadget_control_complete(struct usb_gadget *gadget,
> > unsigned int no_implicit_status, int status)
> > {
> > struct usb_request *req;
> >
> > if (no_implicit_status || status != 0)
> > return;
> >
> > /* Send an implicit status-stage request for ep0 */
> > req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
> > if (req) {
> > req->length = 0;
> > req->no_implicit_status = 1;
> > req->complete = /* req's deallocation routine */
> > usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
> > }
> > }
> >
> > Then all a UDC driver would need to do is call
> > usb_gadget_control_complete() after invoking a control request's
> > completion handler. The no_implicit_status and status arguments would
> > be taken from the request that was just completed.
> >
> > With this one call added to each UDC, all the existing function drivers
> > would work correctly. Even though they don't explicitly queue
> > status-stage requests, the new routine will do so for them,
> > transparently. Function drivers that want to handle their own
> > status-stage requests explicitly will merely have to set the
> > req->no_implicit_status bit.
>
> I think this is a good idea. We still get the benefits of explicit
> status stage without being overly intrusive in the conversion, and we
> maintain the queue-based API.
>
> Would it be fine for me to proceed in this direction for a v2?
It is as far as I'm concerned (Felipe might not agree). Knock yourself
out. :-)
Alan Stern
> > (We might or might not need to watch out for 0-length control-OUT
> > transfers. Function drivers _do_ queue status-stage requests for
> > those.)
>
> Thanks,
>
> Paul Elder