[PATCH 1/4] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt
Down the call stack from the ioctl handler for VIDIOC_STREAMON,
uvc_video_alloc_requests contains a BUG_ON, which in the high level,
triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF
being issued previously.
This could be triggered by uvc_function_set_alt 0 racing and
winning against uvc_v4l2_streamon, or by userspace neglecting to issue
the VIDIOC_STREAMOFF ioctl.
To fix this, add two more uvc states: starting and stopping. Use these
to prevent the racing, and to detect when VIDIOC_STREAMON is issued
without previously issuing VIDIOC_STREAMOFF.
Signed-off-by: Paul Elder
---
drivers/usb/gadget/function/f_uvc.c| 8 ++--
drivers/usb/gadget/function/uvc.h | 2 ++
drivers/usb/gadget/function/uvc_v4l2.c | 19 +--
3 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 439eba660e95..9b63b28a1ee3 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -325,17 +325,19 @@ uvc_function_set_alt(struct usb_function *f, unsigned
interface, unsigned alt)
switch (alt) {
case 0:
- if (uvc->state != UVC_STATE_STREAMING)
+ if (uvc->state != UVC_STATE_STREAMING &&
+ uvc->state != UVC_STATE_STARTING)
return 0;
if (uvc->video.ep)
usb_ep_disable(uvc->video.ep);
+ uvc->state = UVC_STATE_STOPPING;
+
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_STREAMOFF;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
- uvc->state = UVC_STATE_CONNECTED;
return 0;
case 1:
@@ -354,6 +356,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned
interface, unsigned alt)
return ret;
usb_ep_enable(uvc->video.ep);
+ uvc->state = UVC_STATE_STARTING;
+
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_STREAMON;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index a64e07e61f8c..afb2eac1f337 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -131,6 +131,8 @@ enum uvc_state {
UVC_STATE_DISCONNECTED,
UVC_STATE_CONNECTED,
UVC_STATE_STREAMING,
+ UVC_STATE_STARTING,
+ UVC_STATE_STOPPING,
};
struct uvc_device {
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index 9a9019625496..fdf02b6987c0 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -193,6 +193,9 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum
v4l2_buf_type type)
struct uvc_video *video = &uvc->video;
int ret;
+ if (uvc->state != UVC_STATE_STARTING)
+ return 0;
+
if (type != video->queue.queue.type)
return -EINVAL;
@@ -201,12 +204,13 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum
v4l2_buf_type type)
if (ret < 0)
return ret;
+ uvc->state = UVC_STATE_STREAMING;
+
/*
* Complete the alternate setting selection setup phase now that
* userspace is ready to provide video frames.
*/
uvc_function_setup_continue(uvc);
- uvc->state = UVC_STATE_STREAMING;
return 0;
}
@@ -217,11 +221,22 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum
v4l2_buf_type type)
struct video_device *vdev = video_devdata(file);
struct uvc_device *uvc = video_get_drvdata(vdev);
struct uvc_video *video = &uvc->video;
+ int ret;
+
+ if (uvc->state != UVC_STATE_STOPPING)
+ return 0;
if (type != video->queue.queue.type)
return -EINVAL;
- return uvcg_video_enable(video, 0);
+ ret = uvcg_video_enable(video, 0);
+ if (ret < 0)
+ return ret;
+
+ uvc->state = UVC_STATE_CONNECTED;
+
+ return 0;
+
}
static int
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] usb: gadget: composite: add function to increment delayed_status
The completion of the usb status phase from uvc_function_set_alt needs
to be delayed until uvc_v4l2_streamon/off. This is currently done by
uvc_function_set_alt returning USB_GADGET_DELAYED_STATUS and
composite_setup detecting this to increment cdev->delayed_status.
However, if uvc_v4l2_streamon/off is called in between this return and
increment, uvc_function_setup_continue within uvc_v4l2_streamon/off will
WARN that cdev->delayed_status is zero.
To fix situations like this, add a function to increment
cdev->delayed_status.
Signed-off-by: Paul Elder
---
drivers/usb/gadget/composite.c | 6 ++
include/linux/usb/composite.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 77c7ecca816a..c02ab640a7ae 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1548,6 +1548,12 @@ static int fill_ext_prop(struct usb_configuration *c,
int interface, u8 *buf)
return 0;
}
+void usb_composite_setup_delay(struct usb_composite_dev *cdev)
+{
+ cdev->delayed_status++;
+}
+EXPORT_SYMBOL(usb_composite_setup_delay);
+
/*
* The setup() callback implements all the ep0 functionality that's
* not handled lower down, in hardware or the hardware driver(like
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index cef0e44601f8..049f77a4d42b 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -524,6 +524,8 @@ extern int composite_setup(struct usb_gadget *gadget,
extern void composite_suspend(struct usb_gadget *gadget);
extern void composite_resume(struct usb_gadget *gadget);
+extern void usb_composite_setup_delay(struct usb_composite_dev *c);
+
/*
* Some systems will need runtime overrides for the product identifiers
* published in the device descriptor, either numbers or strings or both.
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] usb: gadget: uvc: fix racing between
Down the call stack from the ioctl handler for VIDIOC_STREAMON, uvc_video_alloc_requests contains a BUG_ON, which in the high level, triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF being issued previously. This can happen in a few ways, such as if the userspace uvc gadget application simply doesn't issue VIDIOC_STREAMOFF. Another way is if uvc_function_set_alt with alt 0 is called after it is called with 1 but before VIDIOC_STREAMON is called; in this case, UVC_EVENT_STREAMON will not be queued to userspace, and therefore userspace will never call VIDIOC_STREAMOFF. To fix this, add two more uvc states: starting and stopping. The starting state starts when uvc_function_set_alt 1 is called, and ends in uvc_v4l2_streamon, when the state is changed to streaming. The stopping state starts when uvc_function_set_alt 0 is called, and ends in uvc_v4l2_streamoff, when the state is changed to connected. Along with this fix, the completion of the usb status phase needs to be delayed until uvc_v4l2_streamon/off. This is already the case for uvc_function_set_alt 1, so add this to when alt is 0. However, the delayed_status is only incremented when this function returns, so if uvc_v4l2_streamon/off is called in between returning and incrementing, then uvc_function_setup_continue will WARN that delayed_status is zero. To fix this, add and use usb_composite_setup_delay, which increments the delayed_status. Then, uvc_function_set_alt returns 0 instead of USB_GADGET_DELAYED_STATUS. Finally, there is another way to trigger the aforementioned BUG: start streaming and (physically) disconnect usb. To fix this, call uvcg_video_enable 0 in uvc_function_disable. Paul Elder (4): usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt usb: gadget: composite: add function to increment delayed_status usb: gadget: uvc: synchronize usb status phase delay for uvc_function_set_alt usb: gadget: uvc: disable stream when disconnected drivers/usb/gadget/composite.c | 6 ++ drivers/usb/gadget/function/f_uvc.c| 24 +--- drivers/usb/gadget/function/uvc.h | 2 ++ drivers/usb/gadget/function/uvc_v4l2.c | 21 +++-- include/linux/usb/composite.h | 2 ++ 5 files changed, 50 insertions(+), 5 deletions(-) -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] usb: gadget: uvc: synchronize usb status phase delay for uvc_function_set_alt
This patch depends on the previous patch in this series: [PATCH 2/4] usb: gadget: composite: add function to increment delayed_status The completion of the usb status phase from uvc_function_set_alt needs to be delayed until uvc_v4l2_streamon/off. This is currently done by uvc_function_set_alt returning USB_GADGET_DELAYED_STATUS and composite_setup detecting this to increment cdev->delayed_status. However, if uvc_v4l2_streamon/off is called in between this return and increment, uvc_function_setup_continue within uvc_v4l2_streamon/off will WARN that cdev->delayed_status is zero. To fix this, use usb_composite_setup_delay to increment cdev->delayed_status, and then return 0 so that composite_setup doesn't increment cdev->delayed_status again. Signed-off-by: Paul Elder --- drivers/usb/gadget/function/f_uvc.c| 13 - drivers/usb/gadget/function/uvc_v4l2.c | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 9b63b28a1ee3..3324d36e809c 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -334,6 +334,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) uvc->state = UVC_STATE_STOPPING; + usb_composite_setup_delay(cdev); + memset(&v4l2_event, 0, sizeof(v4l2_event)); v4l2_event.type = UVC_EVENT_STREAMOFF; v4l2_event_queue(&uvc->vdev, &v4l2_event); @@ -358,10 +360,19 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) uvc->state = UVC_STATE_STARTING; + usb_composite_setup_delay(cdev); + memset(&v4l2_event, 0, sizeof(v4l2_event)); v4l2_event.type = UVC_EVENT_STREAMON; v4l2_event_queue(&uvc->vdev, &v4l2_event); - return USB_GADGET_DELAYED_STATUS; + + /* +* Normally we would return USB_GADGET_DELAYED_STATUS +* but status phase might try to continue in between +* this function returning and the delayed status +* actually being set +*/ + return 0; default: return -EINVAL; diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c index fdf02b6987c0..0592e13d861c 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -235,6 +235,8 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type) uvc->state = UVC_STATE_CONNECTED; + uvc_function_setup_continue(uvc); + return 0; } -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] usb: gadget: uvc: disable stream when disconnected
Down the call stack from the ioctl handler for VIDIOC_STREAMON,
uvc_video_alloc_requests contains a BUG_ON, which in the high level,
triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF
being issued previously.
This can also be triggered by starting the stream and then physically
disconnecting usb. To fix this, do the streamoff procedures on usb
disconnect.
Signed-off-by: Paul Elder
---
drivers/usb/gadget/function/f_uvc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 3324d36e809c..e67f42ee2342 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -384,9 +384,12 @@ uvc_function_disable(struct usb_function *f)
{
struct uvc_device *uvc = to_uvc(f);
struct v4l2_event v4l2_event;
+ struct uvc_video *video = &uvc->video;
INFO(f->config->cdev, "uvc_function_disable\n");
+ uvcg_video_enable(video, 0);
+
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_DISCONNECT;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off
I'm sorry; I sent this one again only because I realized I accidentally truncated the subject line on the original cover letter. On Tue, Apr 17, 2018 at 11:20:15PM -0400, Paul Elder wrote: > Down the call stack from the ioctl handler for VIDIOC_STREAMON, > uvc_video_alloc_requests contains a BUG_ON, which in the high level, > triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF > being issued previously. > > This can happen in a few ways, such as if the userspace uvc gadget > application simply doesn't issue VIDIOC_STREAMOFF. Another way is if > uvc_function_set_alt with alt 0 is called after it is called with 1 but > before VIDIOC_STREAMON is called; in this case, UVC_EVENT_STREAMON will > not be queued to userspace, and therefore userspace will never call > VIDIOC_STREAMOFF. > > To fix this, add two more uvc states: starting and stopping. The > starting state starts when uvc_function_set_alt 1 is called, and ends > in uvc_v4l2_streamon, when the state is changed to streaming. The > stopping state starts when uvc_function_set_alt 0 is called, and ends > in uvc_v4l2_streamoff, when the state is changed to connected. > > Along with this fix, the completion of the usb status phase needs to be > delayed until uvc_v4l2_streamon/off. This is already the case for > uvc_function_set_alt 1, so add this to when alt is 0. However, the > delayed_status is only incremented when this function returns, so if > uvc_v4l2_streamon/off is called in between returning and incrementing, > then uvc_function_setup_continue will WARN that delayed_status is zero. > > To fix this, add and use usb_composite_setup_delay, > which increments the delayed_status. Then, uvc_function_set_alt > returns 0 instead of USB_GADGET_DELAYED_STATUS. > > Finally, there is another way to trigger the aforementioned BUG: start > streaming and (physically) disconnect usb. To fix this, call > uvcg_video_enable 0 in uvc_function_disable. > > Paul Elder (4): > usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt > usb: gadget: composite: add function to increment delayed_status > usb: gadget: uvc: synchronize usb status phase delay for > uvc_function_set_alt > usb: gadget: uvc: disable stream when disconnected > > drivers/usb/gadget/composite.c | 6 ++ > drivers/usb/gadget/function/f_uvc.c| 24 +--- > drivers/usb/gadget/function/uvc.h | 2 ++ > drivers/usb/gadget/function/uvc_v4l2.c | 21 +++-- > include/linux/usb/composite.h | 2 ++ > 5 files changed, 50 insertions(+), 5 deletions(-) > > -- > 2.17.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] usb: gadget: uvc: fix racing between
Also forgot to mention that I did this against 4.16. From what I can tell, only drivers/usb/gadget/composite.c has changed between then and 4.17-rc1 :/ On Tue, Apr 17, 2018 at 11:18:12PM -0400, Paul Elder wrote: > Down the call stack from the ioctl handler for VIDIOC_STREAMON, > uvc_video_alloc_requests contains a BUG_ON, which in the high level, > triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF > being issued previously. > > This can happen in a few ways, such as if the userspace uvc gadget > application simply doesn't issue VIDIOC_STREAMOFF. Another way is if > uvc_function_set_alt with alt 0 is called after it is called with 1 but > before VIDIOC_STREAMON is called; in this case, UVC_EVENT_STREAMON will > not be queued to userspace, and therefore userspace will never call > VIDIOC_STREAMOFF. > > To fix this, add two more uvc states: starting and stopping. The > starting state starts when uvc_function_set_alt 1 is called, and ends > in uvc_v4l2_streamon, when the state is changed to streaming. The > stopping state starts when uvc_function_set_alt 0 is called, and ends > in uvc_v4l2_streamoff, when the state is changed to connected. > > Along with this fix, the completion of the usb status phase needs to be > delayed until uvc_v4l2_streamon/off. This is already the case for > uvc_function_set_alt 1, so add this to when alt is 0. However, the > delayed_status is only incremented when this function returns, so if > uvc_v4l2_streamon/off is called in between returning and incrementing, > then uvc_function_setup_continue will WARN that delayed_status is zero. > > To fix this, add and use usb_composite_setup_delay, > which increments the delayed_status. Then, uvc_function_set_alt > returns 0 instead of USB_GADGET_DELAYED_STATUS. > > Finally, there is another way to trigger the aforementioned BUG: start > streaming and (physically) disconnect usb. To fix this, call > uvcg_video_enable 0 in uvc_function_disable. > > Paul Elder (4): > usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt > usb: gadget: composite: add function to increment delayed_status > usb: gadget: uvc: synchronize usb status phase delay for > uvc_function_set_alt > usb: gadget: uvc: disable stream when disconnected > > drivers/usb/gadget/composite.c | 6 ++ > drivers/usb/gadget/function/f_uvc.c| 24 +--- > drivers/usb/gadget/function/uvc.h | 2 ++ > drivers/usb/gadget/function/uvc_v4l2.c | 21 +++-- > include/linux/usb/composite.h | 2 ++ > 5 files changed, 50 insertions(+), 5 deletions(-) > > -- > 2.17.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off
Down the call stack from the ioctl handler for VIDIOC_STREAMON, uvc_video_alloc_requests contains a BUG_ON, which in the high level, triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF being issued previously. This can happen in a few ways, such as if the userspace uvc gadget application simply doesn't issue VIDIOC_STREAMOFF. Another way is if uvc_function_set_alt with alt 0 is called after it is called with 1 but before VIDIOC_STREAMON is called; in this case, UVC_EVENT_STREAMOFF will not be queued to userspace, and therefore userspace will never call VIDIOC_STREAMOFF. To fix this, add two more uvc states: starting and stopping. The starting state is entered when uvc_function_set_alt 1 is called, and is exited in uvc_v4l2_streamon, when the state is changed to streaming. The stopping state is entered when uvc_function_set_alt 0 is called, and is exited in uvc_v4l2_streamoff, when the state is changed to connected. The status phase of the SET_INTERFACE request doesn't need to be delayed by the uvc gadget driver, so that is removed. Finally, there is another way to trigger the aforementioned BUG: start streaming and (physically) disconnect usb. To fix this, call uvcg_video_enable 0 in uvc_function_disable. Changes in v2: 1. Remove delay usb status phase Paul Elder (3): usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt usb: gadget: uvc: remove delay usb status phase from uvc usb: gadget: uvc: disable stream when disconnected drivers/usb/gadget/function/f_uvc.c| 14 +++--- drivers/usb/gadget/function/uvc.h | 2 ++ drivers/usb/gadget/function/uvc_v4l2.c | 21 +++-- 3 files changed, 28 insertions(+), 9 deletions(-) -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] usb: gadget: uvc: disable stream when disconnected
Down the call stack from the ioctl handler for VIDIOC_STREAMON,
uvc_video_alloc_requests contains a BUG_ON, which in the high level,
triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF
being issued previously.
This can also be triggered by starting the stream and then physically
disconnecting usb. To fix this, do the streamoff procedures on usb
disconnect.
Signed-off-by: Paul Elder
---
Changes in v2: Nothing
drivers/usb/gadget/function/f_uvc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index fa34dcbe1197..5bb79888e3f7 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -374,9 +374,12 @@ uvc_function_disable(struct usb_function *f)
{
struct uvc_device *uvc = to_uvc(f);
struct v4l2_event v4l2_event;
+ struct uvc_video *video = &uvc->video;
INFO(f->config->cdev, "uvc_function_disable\n");
+ uvcg_video_enable(video, 0);
+
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_DISCONNECT;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt
Down the call stack from the ioctl handler for VIDIOC_STREAMON,
uvc_video_alloc_requests contains a BUG_ON, which in the high level,
triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF
being issued previously.
This could be triggered by uvc_function_set_alt 0 racing and
winning against uvc_v4l2_streamon, or by userspace neglecting to issue
the VIDIOC_STREAMOFF ioctl.
To fix this, add two more uvc states: starting and stopping. Use these
to prevent the racing, and to detect when VIDIOC_STREAMON is issued
without previously issuing VIDIOC_STREAMOFF.
Signed-off-by: Paul Elder
---
Changes in v2: Nothing
drivers/usb/gadget/function/f_uvc.c| 8 ++--
drivers/usb/gadget/function/uvc.h | 2 ++
drivers/usb/gadget/function/uvc_v4l2.c | 19 +--
3 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 439eba660e95..9b63b28a1ee3 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -325,17 +325,19 @@ uvc_function_set_alt(struct usb_function *f, unsigned
interface, unsigned alt)
switch (alt) {
case 0:
- if (uvc->state != UVC_STATE_STREAMING)
+ if (uvc->state != UVC_STATE_STREAMING &&
+ uvc->state != UVC_STATE_STARTING)
return 0;
if (uvc->video.ep)
usb_ep_disable(uvc->video.ep);
+ uvc->state = UVC_STATE_STOPPING;
+
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_STREAMOFF;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
- uvc->state = UVC_STATE_CONNECTED;
return 0;
case 1:
@@ -354,6 +356,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned
interface, unsigned alt)
return ret;
usb_ep_enable(uvc->video.ep);
+ uvc->state = UVC_STATE_STARTING;
+
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_STREAMON;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index a64e07e61f8c..afb2eac1f337 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -131,6 +131,8 @@ enum uvc_state {
UVC_STATE_DISCONNECTED,
UVC_STATE_CONNECTED,
UVC_STATE_STREAMING,
+ UVC_STATE_STARTING,
+ UVC_STATE_STOPPING,
};
struct uvc_device {
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index 9a9019625496..fdf02b6987c0 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -193,6 +193,9 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum
v4l2_buf_type type)
struct uvc_video *video = &uvc->video;
int ret;
+ if (uvc->state != UVC_STATE_STARTING)
+ return 0;
+
if (type != video->queue.queue.type)
return -EINVAL;
@@ -201,12 +204,13 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum
v4l2_buf_type type)
if (ret < 0)
return ret;
+ uvc->state = UVC_STATE_STREAMING;
+
/*
* Complete the alternate setting selection setup phase now that
* userspace is ready to provide video frames.
*/
uvc_function_setup_continue(uvc);
- uvc->state = UVC_STATE_STREAMING;
return 0;
}
@@ -217,11 +221,22 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum
v4l2_buf_type type)
struct video_device *vdev = video_devdata(file);
struct uvc_device *uvc = video_get_drvdata(vdev);
struct uvc_video *video = &uvc->video;
+ int ret;
+
+ if (uvc->state != UVC_STATE_STOPPING)
+ return 0;
if (type != video->queue.queue.type)
return -EINVAL;
- return uvcg_video_enable(video, 0);
+ ret = uvcg_video_enable(video, 0);
+ if (ret < 0)
+ return ret;
+
+ uvc->state = UVC_STATE_CONNECTED;
+
+ return 0;
+
}
static int
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] usb: gadget: uvc: remove delay usb status phase
The completion of the usb status phase doesn't need to be delayed from uvc_function_set_alt to uvc_v4l2_streamon/off. Remove USB_GADGET_DELAYED_STATUS and usb_composite_setup_delay from these two, respectively. Signed-off-by: Paul Elder --- Changes in v2: 1. Remove delay usb status phase drivers/usb/gadget/function/f_uvc.c| 3 ++- drivers/usb/gadget/function/uvc_v4l2.c | 6 -- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 9b63b28a1ee3..fa34dcbe1197 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -361,7 +361,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) memset(&v4l2_event, 0, sizeof(v4l2_event)); v4l2_event.type = UVC_EVENT_STREAMON; v4l2_event_queue(&uvc->vdev, &v4l2_event); - return USB_GADGET_DELAYED_STATUS; + + return 0; default: return -EINVAL; diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c index fdf02b6987c0..138d95b3b8d1 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -206,12 +206,6 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type) uvc->state = UVC_STATE_STREAMING; - /* -* Complete the alternate setting selection setup phase now that -* userspace is ready to provide video frames. -*/ - uvc_function_setup_continue(uvc); - return 0; } -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
MUSB issue
Hello Felipe, We have discovered an issue in MUSB gadget when receiving control OUT transfers. I have specifically observed it very frequently on the second SET_CUR UVC request when trying to stream video with yavta from a UVC gadget. What happens is that in the DATA phase, the controller copies the incoming data to FIFO which generates an interrupt (as per the docs), then the interrupt handler (actually ep0_rxstate) sends a stall because there is no usb_request to put the data into. Currently this usb_request is supposed to come from userspace by ioctl UVCIOC_SEND_RESPONSE, which means that (more often than not) if the interrupt corresponding to the DATA phase comes before userspace has a chance to call the ioctl (or before the ioctl handler has a chance to finish), then it will send a stall (usbmon ends with EPIPE). I found this in ep0_rxstate: /* read packet and ack; or stall because of gadget driver bug: * should have provided the rx buffer before setup() returned. */ Which I suppose correctly describes the observed behavior. However, what we have now is that setup(), which is composite_setup -> uvc_function_setup, queues a UVC event to userspace so that userspace can do stuff or validation on the usb_request. This means that the rx buffer cannot be provided before setup() returns. So, is this a requrement of the USB gadget stack, or of MUSB? We have also considered enqueuing a usb_request synchronously in the setup handler (only for OUT transfer), instead of having userspace queue it via ioctl. The problem is that there doesn't seem to be any way for the gadget to signal errors from the DATA phase. This means that all parameter validations would have to be done in the SETUP phase, and if the usb_requst has to be queued synchronously then, then the checks can't be performed by userspace. This is a problem since in our case, only userspace has enough information to perform validation. === Log (reordered into threads of execution; timestamps are preserved): (irq for SETUP phase of SET_CUR (21 01)) [ 954.976318] in musb_g_ep0_irq: csr 0001, count 8, ep0stage out/status [ 954.976318] in musb_read_setup: -- setting musb stage to rx (out) [ 954.983154] in musb_g_ep0_irq: -- handled 0, csr 0001, ep0stage out [ 954.983154] in uvc_function_setup: setup request 21 01 value 0100 index 0001 001a [ 954.983184] in musb_g_ep0_irq: -- after, handled = 0, state = out (irq for DATA phase) [ 954.983245] in musb_g_ep0_irq: csr 0001, count 0, ep0stage out [ 954.998504] in ep0_rxstate: sending stall (irq after SENTSTALL) [ 954.998565] in musb_g_ep0_irq: csr 0004, count 0, ep0stage out [ 955.005462] in musb_g_ep0_irq: setting idle state coz need to ack stall [ 955.005462] in musb_g_ep0_irq: new csr = 0 [ 955.012420] in musb_g_ep0_irq: -- setting musb stage to setup (ioctl handler for UVCIOC_SEND_RESPONSE) [ 954.983245] in uvc_v4l2_ioctl_default: handling ioctl [ 955.005462] in uvc_v4l2_ioctl_default: --- sending response [ 955.020385] in uvc_send_response: --- queueing usb ep [ 955.075744] in musb_g_ep0_queue: --- ep0 request queued in state setup [ 955.088134] in musb_g_ep0_queue: ret status from musb_g_ep0_queue is -22 Thanks, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MUSB issue
Hi Alan, On Mon, Jul 02, 2018 at 04:03:34PM -0400, Alan Stern wrote: > On Mon, 2 Jul 2018, Laurent Pinchart wrote: > > > Hi Alan, > > > > On Monday, 2 July 2018 18:13:27 EEST Alan Stern wrote: > > > On Mon, 2 Jul 2018, Paul Elder wrote: > > > > Hello Felipe, > > > > > > > > We have discovered an issue in MUSB gadget when receiving control OUT > > > > transfers. I have specifically observed it very frequently on the second > > > > SET_CUR UVC request when trying to stream video with yavta from a UVC > > > > gadget. > > > > > > > > What happens is that in the DATA phase, the controller copies the > > > > incoming data to FIFO which generates an interrupt (as per the docs), > > > > then the interrupt handler (actually ep0_rxstate) sends a stall because > > > > there is no usb_request to put the data into. > > > > > > > > Currently this usb_request is supposed to come from userspace by ioctl > > > > UVCIOC_SEND_RESPONSE, which means that (more often than not) if the > > > > interrupt corresponding to the DATA phase comes before userspace has a > > > > chance to call the ioctl (or before the ioctl handler has a chance to > > > > finish), then it will send a stall (usbmon ends with EPIPE). > > > > > > Shouldn't the UDC send a NAK install of a STALL under these > > > circumstances? In fact, shouldn't the controller not even bother to > > > copy the incoming data to its FIFO? > > > > > > I realize the MUSB hardware has a number of limitations, but this seems > > > a little peculiar. > > > > Those are questions I've asked myself when trying to help Paul with this > > issue. I realized that I couldn't tell whether MUSB was doing the right > > thing, > > as I didn't know what the UDC API required. > > > > When validating a control OUT request we need to validate both the data > > part > > of the SETUP phase (bRequest, bRequestType, wValue, wIndex) and the data > > part > > of the DATA phase. In the case of the UVC gadget, both validation occurs in > > userspace, so the SETUP validation can't be performed synchronously with > > the > > function's .setup() handler. > > > > In case of control writes, the STALL handshake can be returned in either > > the > > DATA phase or the STATUS phase (as far as I understand the USB > > specification > > doesn't indicate in which use cases it is appropriate to return a STALL > > handshake in the DATA or STATUS phase). As the data validation can > > obviously > > only be performed after the data is received, I assume that signaling a > > data > > error through a STALL handshake can only be performed in the STATUS stage. > > Signaling a setup error, on the other hand, could be done in either phase. > > That's right. However, see below. > > > The question is thus what options does the USB gadget API offer function > > drivers to STALL the control transfer in the DATA or STATUS phase. As far > > as I > > can tell, this can be done by returning an error from the .setup() handler > > (in > > which case I suppose the DATA stage will be STALLed), or calling > > usb_ep_set_halt() before the request completion handler for the DATA stage > > returns. Is that right ? > > Yes. > > > Are there other mechanisms ? If there are no other > > mechanism, I won't be able to validate the DATA stage in userspace as I can > > only send the data to userspace in the DATA stage request completion > > handler, > > and have to then return before receiving a response from userspace. > > Correct. Even if userspace wasn't involved, you still wouldn't be able > to check the data and halt the endpoint in time, because the UDC will > automatically queue a positive (i.e., zlp) response for the STATUS > stage when the DATA stage is complete. Laurent and I have theorized that in the case of MUSB it might be possible to do this by delaying the completion of the DATA phase (and by extension delay the host moving to the STATUS phase). I still need to test to see if this theory will work, plus we still need some way to allow userspace to tell the driver that it should reply with STALL or not :/ > This is a weakness in the gadget API, and there has been at least one > discussion about changing it. In particular, Felipe recommended that > STATUS stage packets should be queued explicitly by gadget drivers, not > sent automatically by UDCs. But
Re: MUSB issue
On Tue, Jul 10, 2018 at 11:23:50AM -0400, Alan Stern wrote: > On Tue, 10 Jul 2018, Paul Elder wrote: > > > > > Are there other mechanisms ? If there are no other > > > > mechanism, I won't be able to validate the DATA stage in userspace as I > > > > can > > > > only send the data to userspace in the DATA stage request completion > > > > handler, > > > > and have to then return before receiving a response from userspace. > > > > > > Correct. Even if userspace wasn't involved, you still wouldn't be able > > > to check the data and halt the endpoint in time, because the UDC will > > > automatically queue a positive (i.e., zlp) response for the STATUS > > > stage when the DATA stage is complete. > > > > Laurent and I have theorized that in the case of MUSB it might be > > possible to do this by delaying the completion of the DATA phase > > (and by extension delay the host moving to the STATUS phase). I still > > How would you delay completion of the DATA stage? As far as I can see, > the only way to do that is to send a NAK handshake back to the host, > and you mention below that MUSB will not do this. MUSB has a DATAEND bit that should be set after all data packets of the DATA stage are received. The theory is that if we delay setting the DATAEND bit then we might be able to delay completion of the DATA stage. > > need to test to see if this theory will work, plus we still need some > > way to allow userspace to tell the driver that it should reply with > > STALL or not :/ > > > > > This is a weakness in the gadget API, and there has been at least one > > > discussion about changing it. In particular, Felipe recommended that > > > STATUS stage packets should be queued explicitly by gadget drivers, not > > > sent automatically by UDCs. But as far as I know, this suggestion has > > > not been implemented. It would require changing pretty much every UDC > > > driver and every gadget driver. > > > > I'm wondering if gadget drivers explicitly queueing STATUS stage packets > > would help, because what I see in the MUSB spec is that the controller > > automatically ACKs (or STALLs if that bit in the register is set) > > the STATUS phase, so the driver isn't involved in the STATUS reply > > except for setting the STALL bit. > > Are you reading the spec correctly? ACK or STALL is an appropriate > response to the STATUS stage of a control-IN transfer. But for a > control-OUT transfer, the device has to send a zero-length data packet > to complete the STATUS stage. Oops, sorry; evidently I wasn't. The MUSB spec actually doesn't mention this. In any case, for a control-OUT transfer it seems that MUSB automatically sends the zero-length data packet, or STALL if the SENDSTALL bit is set, so rather than queue a STATUS stage packet we have to set the bit on time. > > (If it is indeed possible to delay the completion of > > the DATA phase and then have an ioctl actually complete the DATA > > phase and set the STALL bit if necessary then it might work...) > > > > Also, the interrupt signaling the STATUS phase doesn't always happen and > > sometimes gets coalesced with the next request: > > (in musb_g_ep0_irq, in drivers/usb/musb/musb_gadget_ep0.c) > > /* > > * This state is typically (but not always) indiscernible > > * from the status states since the corresponding interrupts > > * tend to happen within too little period of time (with only > > * a zero-length packet in between) and so get coalesced... > > */ > > I guess this isn't too much of an issue but just wanted to point out > > that we can't expect to do anything in response to a STATUS phase > > interrupt if its arrival isn't reliable. > > > > > The current situation is that currently the only way for a gadget > > > driver to send a STALL to the host in response to a control-OUT > > > transfer is to do so before queuing the DATA-stage request. In other > > > words, the decision has to be made before the data has been received. > > > > > > The situation for control-IN transfers is better. The gadget driver > > > has no reason to stall the STATUS stage once it has sent the data to > > > the host, so it doesn't matter that there is currently no mechanism for > > > doing so. > > > > > > > After clarifying the API (and possibly extending it...), let's move to > > > > the > > > > MUSB driver and see how to fix it. > > > > > > Well, t
Re: [PATCH 1/3] usb: gadget: uvc: Factor out video USB request queueing
On Tue, Sep 18, 2018 at 01:35:30PM +0300, Laurent Pinchart wrote:
> USB requests for video data are queued from two different locations in
> the driver, with the same code block occurring twice. Factor it out to a
> function.
>
> Signed-off-by: Laurent Pinchart
For the whole series:
Looks good to me.
Reviewed-by: Paul Elder
Tested-by: Paul Elder
> ---
> drivers/usb/gadget/function/uvc_video.c | 30 --
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c
> b/drivers/usb/gadget/function/uvc_video.c
> index d3567b90343a..a95c8e2364ed 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -125,6 +125,19 @@ uvc_video_encode_isoc(struct usb_request *req, struct
> uvc_video *video,
> * Request handling
> */
>
> +static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request
> *req)
> +{
> + int ret;
> +
> + ret = usb_ep_queue(video->ep, req, GFP_ATOMIC);
> + if (ret < 0) {
> + printk(KERN_INFO "Failed to queue request (%d).\n", ret);
> + usb_ep_set_halt(video->ep);
> + }
> +
> + return ret;
> +}
> +
> /*
> * I somehow feel that synchronisation won't be easy to achieve here. We have
> * three events that control USB requests submission:
> @@ -189,14 +202,13 @@ uvc_video_complete(struct usb_ep *ep, struct
> usb_request *req)
>
> video->encode(req, video, buf);
>
> - if ((ret = usb_ep_queue(ep, req, GFP_ATOMIC)) < 0) {
> - printk(KERN_INFO "Failed to queue request (%d).\n", ret);
> - usb_ep_set_halt(ep);
> - spin_unlock_irqrestore(&video->queue.irqlock, flags);
> + ret = uvcg_video_ep_queue(video, req);
> + spin_unlock_irqrestore(&video->queue.irqlock, flags);
> +
> + if (ret < 0) {
> uvcg_queue_cancel(queue, 0);
> goto requeue;
> }
> - spin_unlock_irqrestore(&video->queue.irqlock, flags);
>
> return;
>
> @@ -316,15 +328,13 @@ int uvcg_video_pump(struct uvc_video *video)
> video->encode(req, video, buf);
>
> /* Queue the USB request */
> - ret = usb_ep_queue(video->ep, req, GFP_ATOMIC);
> + ret = uvcg_video_ep_queue(video, req);
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> +
> if (ret < 0) {
> - printk(KERN_INFO "Failed to queue request (%d)\n", ret);
> - usb_ep_set_halt(video->ep);
> - spin_unlock_irqrestore(&queue->irqlock, flags);
> uvcg_queue_cancel(queue, 0);
> break;
> }
> - spin_unlock_irqrestore(&queue->irqlock, flags);
> }
>
> spin_lock_irqsave(&video->req_lock, flags);
> --
> Regards,
>
> Laurent Pinchart
>
Re: [PATCH 0/4] usb: gadget: uvc: Fix endianness and sign issues
On Tue, Sep 18, 2018 at 03:32:43PM +0300, Laurent Pinchart wrote: > Hello, > > This series addresses endianness and sign issues in the configfs support > of the UVC gadget function driver. > > The first patch starts by fixing the endianness of various UVC > descriptors to match the USB little endian order. The second patch then > fixed the UVC gadget function driver to handle endianness correctly when > parsing configfs attributes. > > The next patch goes on to simplify UVC configfs attribute macros, and > the last patch finally fixes sign issues by using the correct format to > print unsigned int values. For the whole series: Looks good to me. Reviewed-by: Paul Elder Tested-by: Paul Elder > Laurent Pinchart (4): > usb: video: Fix endianness mismatches in descriptor structures > usb: gadget: uvc: configfs: Fix operation on big endian platforms > usb: gadget: uvc: configfs: Simplify attributes macros > usb: gadget: uvc: configfs: Use %u to print unsigned int values > > drivers/usb/gadget/function/uvc_configfs.c | 241 +-- > include/uapi/linux/usb/video.h | 304 > ++--- > 2 files changed, 249 insertions(+), 296 deletions(-) > > -- > Regards, > > Laurent Pinchart >
[PATCH] usb: gadget: musb: fix short isoc packets with inventra dma for pandaboard es
Handling short packets (length < max packet size) in the Inventra DMA
engine in the MUSB driver causes the MUSB DMA controller to hang. An
example of a problem that is caused by this problem is when streaming
video out of a UVC gadget, only the first video frame is transferred.
For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
set manually by the driver. This was previously done in musb_g_tx
(musb_gadget.c), but incorrectly (all csr flags were cleared, and only
MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows
some requests to be transferred correctly, but multiple requests were
often put together in one USB packet, and caused problems if the packet
size was not a multiple of 4.
Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c),
just like host mode transfers, then musb_g_tx forces the packet to be
flushed, by setting MUSB_TXCSR_FLUSHFIFO.
This topic was originally tackled by Nicolas Boichat [0] [1] and is discussed
further at [2] as part of his GSoC project [3].
[0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
[1]
https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
[2]
http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
[3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
I have forward-ported this patch from 2.6.34 to 4.19-rc1.
Signed-off-by: Paul Elder
---
drivers/usb/musb/musb_gadget.c | 21 ++---
drivers/usb/musb/musbhsdma.c | 21 +++--
2 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index eae8b1b1b45b..d3f33f449445 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum)
&& (request->actual == request->length))
short_packet = true;
- if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
- (is_dma && (!dma->desired_mode ||
- (request->actual &
- (musb_ep->packet_sz - 1)
- short_packet = true;
+ if (is_dma && (musb_dma_inventra(musb) ||
musb_dma_ux500(musb))) {
+ if (!dma->desired_mode ||
+ request->actual % musb_ep->packet_sz) {
+ musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n",
+ musb_ep->end_point.name);
+ musb_writew(epio, MUSB_TXCSR,
+ csr | MUSB_TXCSR_FLUSHFIFO);
+ return;
+ }
+ }
if (short_packet) {
/*
@@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
if (csr & MUSB_TXCSR_TXPKTRDY)
return;
- musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE
- | MUSB_TXCSR_TXPKTRDY);
+ musb_dbg(musb, "sending zero pkt (zero=%d, length=%d,
actual=%d, dma->desired_mode=%d)\n",
+request->zero, request->length,
request->actual,
+dma->desired_mode);
+ musb_writew(epio, MUSB_TXCSR, csr |
MUSB_TXCSR_TXPKTRDY);
request->zero = 0;
}
diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index a688f7f87829..e514d4700a6b 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void
*private_data)
channel->status = MUSB_DMA_STATUS_FREE;
/* completed */
- if ((devctl & MUSB_DEVCTL_HM)
- && (musb_channel->transmit)
- && ((channel->desired_mode == 0)
- || (channel->actual_len &
- (musb_channel->max_packet_sz - 1)))
- ) {
+ if (musb_channel->transmit &&
+ (!channel->desired_mode ||
+(channel->actual_len %
+ musb_channel->max
[PATCH 3/6] usb: gadget: uvc: package setup and data for control OUT requests
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
Reviewed-by: Laurent Pinchart
---
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
[PATCH 5/6] usb: musb: gadget: implement send_response
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
Reviewed-by: Laurent Pinchart
---
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;
+ boollast_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
[PATCH 6/6] usb: gadget: uvc: allow ioctl to send response in status stage
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
---
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
[PATCH 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request
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
[PATCH 1/6] usb: uvc: include videodev2.h in g_uvc.h
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 Reviewed-by: Laurent Pinchart --- 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 #include #include +#include #define UVC_EVENT_FIRST(V4L2_EVENT_PRIVATE_START + 0) #define UVC_EVENT_CONNECT (V4L2_EVENT_PRIVATE_START + 0) -- 2.18.0
[PATCH 4/6] usb: gadget: add functions to signal udc driver to delay status stage
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
Reviewed-by: Laurent Pinchart
---
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;
+ booldelayed_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
[PATCH 2/6] usb: gadget: uvc: enqueue usb request in setup handler for control OUT
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
Reviewed-by: Laurent Pinchart
---
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
Re: [PATCH 5/6] usb: musb: gadget: implement send_response
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
> > Reviewed-by: Laurent Pinchart
>
> The patch looks good to me, here is my Acked-by:
>
> Acked-by: Bin Liu
>
> 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 < $wdir/dwFrameInterval
66
10
500
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
Re: [PATCH 4/6] usb: gadget: add functions to signal udc driver to delay status stage
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
> > > > Reviewed-by: Laurent Pinchart
> > > > ---
> > > >
> > > > 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
Re: [PATCH] usb: gadget: musb: fix short isoc packets with inventra dma for pandaboard es
On Tue, Oct 09, 2018 at 02:32:20AM -0400, Paul Elder wrote:
> Handling short packets (length < max packet size) in the Inventra DMA
> engine in the MUSB driver causes the MUSB DMA controller to hang. An
> example of a problem that is caused by this problem is when streaming
> video out of a UVC gadget, only the first video frame is transferred.
>
> For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
> set manually by the driver. This was previously done in musb_g_tx
> (musb_gadget.c), but incorrectly (all csr flags were cleared, and only
> MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows
> some requests to be transferred correctly, but multiple requests were
> often put together in one USB packet, and caused problems if the packet
> size was not a multiple of 4.
>
> Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c),
> just like host mode transfers, then musb_g_tx forces the packet to be
> flushed, by setting MUSB_TXCSR_FLUSHFIFO.
>
> This topic was originally tackled by Nicolas Boichat [0] [1] and is discussed
> further at [2] as part of his GSoC project [3].
>
> [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
> [1]
> https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
> [2]
> http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
> [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
>
> I have forward-ported this patch from 2.6.34 to 4.19-rc1.
>
> Signed-off-by: Paul Elder
Hello all,
ping
I was wondering if any of you had a chance to take a look at this yet.
Thanks,
Paul
> ---
> drivers/usb/musb/musb_gadget.c | 21 ++---
> drivers/usb/musb/musbhsdma.c | 21 +++--
> 2 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index eae8b1b1b45b..d3f33f449445 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> && (request->actual == request->length))
> short_packet = true;
>
> - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
> - (is_dma && (!dma->desired_mode ||
> - (request->actual &
> - (musb_ep->packet_sz - 1)
> - short_packet = true;
> + if (is_dma && (musb_dma_inventra(musb) ||
> musb_dma_ux500(musb))) {
> + if (!dma->desired_mode ||
> + request->actual % musb_ep->packet_sz) {
> + musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n",
> + musb_ep->end_point.name);
> + musb_writew(epio, MUSB_TXCSR,
> + csr | MUSB_TXCSR_FLUSHFIFO);
> + return;
> + }
> + }
>
> if (short_packet) {
> /*
> @@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> if (csr & MUSB_TXCSR_TXPKTRDY)
> return;
>
> - musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE
> - | MUSB_TXCSR_TXPKTRDY);
> + musb_dbg(musb, "sending zero pkt (zero=%d, length=%d,
> actual=%d, dma->desired_mode=%d)\n",
> + request->zero, request->length,
> request->actual,
> + dma->desired_mode);
> + musb_writew(epio, MUSB_TXCSR, csr |
> MUSB_TXCSR_TXPKTRDY);
> request->zero = 0;
> }
>
> diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> index a688f7f87829..e514d4700a6b 100644
> --- a/drivers/usb/musb/musbhsdma.c
> +++ b/drivers/usb/musb/musbhsdma.c
> @@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void
> *private_data)
> channel->status = MUSB_DMA_STATUS_FREE;
>
> /* completed */
> - if ((devctl & MUSB_DEVCTL_HM)
> - && (musb_channel->t
Re: [PATCH 4/6] usb: gadget: add functions to signal udc driver to delay status stage
omatic 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
[PATCH v2 4/6] usb: gadget: add mechanism to specify an explicit status stage
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.
A function driver that wants an explicit status stage should set the
newly added explicit_status flag of the usb_request corresponding to the
data stage. Later on the function driver can explicitly complete the
status stage by enqueueing a usb_request also with the explicit_status
flag set, and with the zero flag set to 1 for ACK, or 0 for STALL.
To support both explicit and implicit status stages, a UDC driver must
call the newly added usb_gadget_control_complete function right after
calling usb_gadget_giveback_request. To support the explicit status
stage, it might then check what stage the usb_request was queued in, or
the explicit_status flag, and the zero flag for what status to send.
Signed-off-by: Paul Elder
v1 Reviewed-by: Laurent Pinchart
---
Changes from v1:
Complete change of API. Now we use a flag that should be set in the
usb_request that is queued for the data stage to signal to the UDC that
we want to delay the status stage (as opposed to setting a flag in the
UDC itself, that persists across all requests). We now also provide a
function for UDC drivers to very easily allow implicit status stages, to
mitigate the need to convert all function drivers to this new API at
once, and to make it easier for UDC drivers to convert.
drivers/usb/gadget/udc/core.c | 33 +
include/linux/usb/gadget.h| 10 ++
2 files changed, 43 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48c1cea..e99481ef9147 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -894,6 +894,39 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
/* - */
+/**
+ * usb_gadget_control_complete - complete the status stage of a control
+ * request, or delay it
+ * Context: in_interrupt()
+ *
+ * @gadget: gadget whose control request's status stage should be completed
+ * @explicit_status: true to delay status stage, false to complete here
+ *
+ * This is called by device controller drivers after returning the completed
+ * request back to the gadget layer, to either complete or delay the status
+ * stage.
+ */
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+ unsigned int explicit_status)
+{
+ struct usb_request *req;
+
+ if (explicit_status)
+ return;
+
+ /* Send an implicit status-stage request for ep0 */
+ req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
+ if (req) {
+ req->length = 0;
+ req->explicit_status = 0;
+ req->complete = usb_ep_free_request;
+ usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
+ }
+}
+EXPORT_SYMBOL_GPL(usb_gadget_control_complete);
+
+/* - */
+
/**
* gadget_find_ep_by_name - returns ep whose name is the same as sting passed
* in second parameter or NULL if searched endpoint not found
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..7a5283a224c7 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -73,6 +73,7 @@ struct usb_ep;
* Note that for writes (IN transfers) some data bytes may still
* reside in a device-side FIFO when the request is reported as
* complete.
+ * @explicit_status: If true, delays the status stage
*
* These are allocated/freed through the endpoint they're used with. The
* hardware's driver can add extra per-request data to the memory it returns,
@@ -114,6 +115,8 @@ struct usb_request {
int status;
unsignedactual;
+
+ unsignedexplicit_status:1;
};
/*-*/
@@ -850,6 +853,13 @@ extern void usb_gadget_giveback_request(struct usb_ep *ep,
/*-*/
+/* utility to complete or delay status stage */
+
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+ unsigned int explicit_status);
+
+/*-*/
+
/* utility to find endpoint by name */
extern struct usb_ep *gadget_find_ep_by_name(struct usb_gadget *g,
--
2.19.2
[PATCH v2 5/6] usb: musb: gadget: implement optional explicit status stage
Implement the mechanism for optional explicit status stage for the MUSB
driver. This allows a function driver to specify what to reply for the
status stage. The functionality for an implicit status stage is
retained.
Signed-off-by: Paul Elder
v1 Reviewed-by: Laurent Pinchart
v1 Acked-by: Bin Liu
---
Changes from v1:
- obvious change to implement v2 mechanism laid out by 4/6 of this
series (send_response, and musb_g_ep0_send_response function has
been removed, call to usb_gadget_control_complete has been added)
- ep0_send_response's ack argument has been changed from stall
- last_packet flag in ep0_rxstate has been removed, since it is equal to
req != NULL
drivers/usb/musb/musb_gadget.c | 1 +
drivers/usb/musb/musb_gadget_ep0.c | 28
2 files changed, 29 insertions(+)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index d3f33f449445..d83315dd22b2 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -145,6 +145,7 @@ __acquires(ep->musb->lock)
trace_musb_req_gb(req);
usb_gadget_giveback_request(&req->ep->end_point, &req->request);
+ usb_gadget_control_complete(&musb->g, request->explicit_status);
spin_lock(&musb->lock);
ep->busy = busy;
}
diff --git a/drivers/usb/musb/musb_gadget_ep0.c
b/drivers/usb/musb/musb_gadget_ep0.c
index 91a5027b5c1f..b3da48f4c63c 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 ack)
+{
+ 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
+ | (ack ? 0 : MUSB_CSR0_P_SENDSTALL);
+
+ 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
*/
@@ -504,10 +523,13 @@ static void ep0_rxstate(struct musb *musb)
if (req) {
musb->ackpend = csr;
musb_g_ep0_giveback(musb, req);
+ if (req->explicit_status)
+ return;
if (!musb->ackpend)
return;
musb->ackpend = 0;
}
+
musb_ep_select(musb->mregs, 0);
musb_writew(regs, MUSB_CSR0, csr);
}
@@ -939,6 +961,12 @@ musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r,
gfp_t gfp_flags)
case MUSB_EP0_STAGE_ACKWAIT:/* zero-length data */
status = 0;
break;
+ case MUSB_EP0_STAGE_STATUSIN:
+ if (r->explicit_status)
+ status = ep0_send_response(musb, r->zero);
+ else
+ status = ep0_send_response(musb, 1);
+ goto cleanup;
default:
musb_dbg(musb, "ep0 request queued in state %d",
musb->ep0_state);
--
2.19.2
[PATCH v2 6/6] usb: gadget: uvc: allow ioctl to send response in status stage
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
---
Changes from v1:
- remove usb_ep_delay_status call from the old proposed API
- changed portions of uvc_send_response to match v2 API
- remove UDC warning that send_response is not implemented
drivers/usb/gadget/function/f_uvc.c| 4 ++--
drivers/usb/gadget/function/uvc_v4l2.c | 19 ---
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index d08957124e42..91388bb647e0 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -209,14 +209,13 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
if (uvc->event_setup_out) {
- uvc->event_setup_out = 0;
-
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
memcpy(&uvc_event->data.setup, &uvc->control_setup,
sizeof(uvc_event->data.setup));
+
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -251,6 +250,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
req->length = uvc->event_length;
req->zero = 0;
+ req->explicit_status = 1;
usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
} else {
struct v4l2_event v4l2_event;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index 35353ffdf3b4..f09aa92a1b01 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -37,10 +37,23 @@ 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) {
+ /*
+* 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;
+
+ req->zero = !data->length;
+ req->explicit_status = 1;
+ return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL);
+ }
if (data->length < 0)
return usb_ep_set_halt(cdev->gadget->ep0);
--
2.19.2
[PATCH v2 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request
This patch series adds a mechanism to allow asynchronously validating the data stage of a control OUT request, and for stalling or suceeding the request accordingly. This mechanism is implemented for MUSB, and is used by UVC. At the same time, UVC packages the setup stage and data stage data together to send to userspace to save on a pair of context switches per control out request. This patch series does change the userspace API. We however believe that it is justified because the current API is broken, and because it isn't being used (because it's broken). The current API is broken such that it is subject to race conditions that cause fatal errors with a high frequency. This is actually what motivated this patch series in the first place. In the current API, not only is there no way to asynchronously validate the data stage of a control OUT request, but an empty buffer is expected to be provided to hold the data stage data -- which is more likely than not to be late. There is even a warning in musb_g_ep0_queue: /* else for sequence #2 (OUT), caller provides a buffer * before the next packet arrives. deferred responses * (after SETUP is acked) are racey. */ This problem has never been reported in years, which is a sign that the API isn't used. Furthermore, the vendor kernels that we have seen using the UVC gadget driver (such as QC and Huawei) are heavily patched with local changes to the API. This corroborates the suspicion that the current mainline API is not being used. Additionally, this API isn't meant to be used by generic applications, but by a dedicated userspace helper. uvc-gadget is one such example, but it has bitrotten and isn't compatible with the current kernel API. The fact that nobody has submitted patches nor complained for a long time again shows that it isn't being used. The conclusion is that since the API hasn't been used for a long time, it is safe to fix it. Changes in v2: Overhaul of status stage delay mechanism/API. Now if a function driver desires an explicit/delayed status stage, it specifies so in a flag in the usb_request that is queued for the data stage. The function driver later enqueues another usb_request for the status stage, also with the explicit_status flag set, and with the zero flag acting as the status. If a function driver does not desire an explicit status stage, then it can set (or ignore) the explicit_status flag in the usb_request that is queued for the data stage. To allow the optional explicit status stage, a UDC driver should call the newly added usb_gadget_control_complete right after usb_gadget_giveback_request, and in its queue function should check if the usb_request is for the status stage and if it has been requested to be explicit, and if so check the status that should be sent. (See 5/6 "usb: musb: gadget: implement optional explicit status stage" for an implementation for MUSB) Paul Elder (6): usb: uvc: include videodev2.h in g_uvc.h usb: gadget: uvc: enqueue usb request in setup handler for control OUT usb: gadget: uvc: package setup and data for control OUT requests usb: gadget: add mechanism to specify an explicit status stage usb: musb: gadget: implement optional explicit status stage usb: gadget: uvc: allow ioctl to send response in status stage drivers/usb/gadget/function/f_uvc.c| 32 ++--- drivers/usb/gadget/function/uvc.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 20 drivers/usb/gadget/udc/core.c | 33 ++ drivers/usb/musb/musb_gadget.c | 1 + drivers/usb/musb/musb_gadget_ep0.c | 28 ++ include/linux/usb/gadget.h | 10 include/uapi/linux/usb/g_uvc.h | 4 +++- 8 files changed, 120 insertions(+), 9 deletions(-) -- 2.19.2
[PATCH v2 2/6] usb: gadget: uvc: enqueue usb request in setup handler for control OUT
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
Reviewed-by: Laurent Pinchart
---
No change from v1
drivers/usb/gadget/function/f_uvc.c| 25 +++--
drivers/usb/gadget/function/uvc_v4l2.c | 7 +++
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 3e07dcb885b7..772c99c297fc 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 7bfa43dbef9e..35353ffdf3b4 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.19.2
[PATCH v2 1/6] usb: uvc: include videodev2.h in g_uvc.h
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 Reviewed-by: Laurent Pinchart --- No change from v1 include/uapi/linux/usb/g_uvc.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h index 3c9ee3020cbb..6698c3263ae8 100644 --- a/include/uapi/linux/usb/g_uvc.h +++ b/include/uapi/linux/usb/g_uvc.h @@ -11,6 +11,7 @@ #include #include #include +#include #define UVC_EVENT_FIRST(V4L2_EVENT_PRIVATE_START + 0) #define UVC_EVENT_CONNECT (V4L2_EVENT_PRIVATE_START + 0) -- 2.19.2
[PATCH] usb: gadget: uvc: add uvcg_warn macro
We only have uvcg_dbg, uvcg_info, and uvcg_err, so add uvcg_warn macro to print gadget device name and function name along with format. Signed-off-by: Paul Elder --- drivers/usb/gadget/function/uvc.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 1d89b1df4ba0..c381ce9e 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -56,6 +56,8 @@ extern unsigned int uvc_gadget_trace_param; dev_dbg(&(f)->config->cdev->gadget->dev, "%s: " fmt, (f)->name, ##args) #define uvcg_info(f, fmt, args...) \ dev_info(&(f)->config->cdev->gadget->dev, "%s: " fmt, (f)->name, ##args) +#define uvcg_warn(f, fmt, args...) \ + dev_warn(&(f)->config->cdev->gadget->dev, "%s: " fmt, (f)->name, ##args) #define uvcg_err(f, fmt, args...) \ dev_err(&(f)->config->cdev->gadget->dev, "%s: " fmt, (f)->name, ##args) -- 2.19.2
[PATCH v2 3/6] usb: gadget: uvc: package setup and data for control OUT requests
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
Reviewed-by: Laurent Pinchart
---
No change from v1
drivers/usb/gadget/function/f_uvc.c | 3 +++
drivers/usb/gadget/function/uvc.h | 1 +
include/uapi/linux/usb/g_uvc.h | 3 ++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 772c99c297fc..d08957124e42 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -215,6 +215,8 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
+ memcpy(&uvc_event->data.setup, &uvc->control_setup,
+ sizeof(uvc_event->data.setup));
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -238,6 +240,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
uvc->event_length = le16_to_cpu(ctrl->wLength);
+ memcpy(&uvc->control_setup, ctrl, sizeof(uvc->control_setup));
if (uvc->event_setup_out) {
struct usb_request *req = uvc->control_req;
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 671020c8a836..1d89b1df4ba0 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -163,6 +163,7 @@ struct uvc_device {
unsigned int control_intf;
struct usb_ep *control_ep;
struct usb_request *control_req;
+ struct usb_ctrlrequest control_setup;
void *control_buf;
unsigned int streaming_intf;
diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h
index 6698c3263ae8..10fbb4382925 100644
--- a/include/uapi/linux/usb/g_uvc.h
+++ b/include/uapi/linux/usb/g_uvc.h
@@ -24,7 +24,8 @@
struct uvc_request_data {
__s32 length;
- __u8 data[60];
+ struct usb_ctrlrequest setup;
+ __u8 data[52];
};
struct uvc_event {
--
2.19.2
[PATCH v3 1/6] usb: uvc: include videodev2.h in g_uvc.h
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 Reviewed-by: Laurent Pinchart --- No change from v2 No change from v1 include/uapi/linux/usb/g_uvc.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h index 3c9ee3020cbb..6698c3263ae8 100644 --- a/include/uapi/linux/usb/g_uvc.h +++ b/include/uapi/linux/usb/g_uvc.h @@ -11,6 +11,7 @@ #include #include #include +#include #define UVC_EVENT_FIRST(V4L2_EVENT_PRIVATE_START + 0) #define UVC_EVENT_CONNECT (V4L2_EVENT_PRIVATE_START + 0) -- 2.19.2
[PATCH v3 5/6] usb: musb: gadget: implement optional explicit status stage
Implement the mechanism for optional explicit status stage for the MUSB
driver. This allows a function driver to specify what to reply for the
status stage. The functionality for an implicit status stage is
retained.
Signed-off-by: Paul Elder
v1 Reviewed-by: Laurent Pinchart
v1 Acked-by: Bin Liu
---
Changes from v2:
- update call to usb_gadget_control_complete to include status
- since sending STALL from the function driver is now done with
usb_ep_set_halt, there is no need for the internal ep0_send_response to
take a stall/ack parameter; remove the parameter and make the function
only send ack, and remove checking for the status reply in the
usb_request for the status stage
Changes from v1:
- obvious change to implement v2 mechanism laid out by 4/6 of this
series (send_response, and musb_g_ep0_send_response function has
been removed, call to usb_gadget_control_complete has been added)
- ep0_send_response's ack argument has been changed from stall
- last_packet flag in ep0_rxstate has been removed, since it is equal to
req != NULL
drivers/usb/musb/musb_gadget.c | 2 ++
drivers/usb/musb/musb_gadget_ep0.c | 23 +++
2 files changed, 25 insertions(+)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index d3f33f449445..a7a992ab0c9d 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -145,6 +145,8 @@ __acquires(ep->musb->lock)
trace_musb_req_gb(req);
usb_gadget_giveback_request(&req->ep->end_point, &req->request);
+ usb_gadget_control_complete(&musb->g, request->explicit_status,
+ request->status);
spin_lock(&musb->lock);
ep->busy = busy;
}
diff --git a/drivers/usb/musb/musb_gadget_ep0.c
b/drivers/usb/musb/musb_gadget_ep0.c
index 91a5027b5c1f..bbce8a9d77e4 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -458,6 +458,23 @@ __acquires(musb->lock)
return handled;
}
+static int ep0_send_ack(struct musb *musb)
+{
+ 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;
+
+ 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
*/
@@ -504,10 +521,13 @@ static void ep0_rxstate(struct musb *musb)
if (req) {
musb->ackpend = csr;
musb_g_ep0_giveback(musb, req);
+ if (req->explicit_status)
+ return;
if (!musb->ackpend)
return;
musb->ackpend = 0;
}
+
musb_ep_select(musb->mregs, 0);
musb_writew(regs, MUSB_CSR0, csr);
}
@@ -939,6 +959,9 @@ musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r,
gfp_t gfp_flags)
case MUSB_EP0_STAGE_ACKWAIT:/* zero-length data */
status = 0;
break;
+ case MUSB_EP0_STAGE_STATUSIN:
+ status = ep0_send_ack(musb);
+ goto cleanup;
default:
musb_dbg(musb, "ep0 request queued in state %d",
musb->ep0_state);
--
2.19.2
[PATCH v3 4/6] usb: gadget: add mechanism to specify an explicit status stage
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.
A function driver that wants an explicit status stage should set the
newly added explicit_status flag of the usb_request corresponding to the
data stage. Later on the function driver can explicitly complete the
status stage by enqueueing a usb_request for ACK, or calling
usb_ep_set_halt() for STALL.
To support both explicit and implicit status stages, a UDC driver must
call the newly added usb_gadget_control_complete function right after
calling usb_gadget_giveback_request. The status of the request that was
just given back must be fed into usb_gadget_control_complete. To support
the explicit status stage, it might then check what stage the
usb_request was queued in, and send an ACK.
Signed-off-by: Paul Elder
v1 Reviewed-by: Laurent Pinchart
---
Changes from v2:
Add status parameter to usb_gadget_control_complete, so that a
usb_request is not queued if the status of the just given back request
is nonzero.
Changes from v1:
Complete change of API. Now we use a flag that should be set in the
usb_request that is queued for the data stage to signal to the UDC that
we want to delay the status stage (as opposed to setting a flag in the
UDC itself, that persists across all requests). We now also provide a
function for UDC drivers to very easily allow implicit status stages, to
mitigate the need to convert all function drivers to this new API at
once, and to make it easier for UDC drivers to convert.
drivers/usb/gadget/udc/core.c | 34 ++
include/linux/usb/gadget.h| 10 ++
2 files changed, 44 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48c1cea..0a0ccd2b7639 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -894,6 +894,40 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
/* - */
+/**
+ * usb_gadget_control_complete - complete the status stage of a control
+ * request, or delay it
+ * Context: in_interrupt()
+ *
+ * @gadget: gadget whose control request's status stage should be completed
+ * @explicit_status: true to delay status stage, false to complete here
+ * @status: completion code of previously completed request
+ *
+ * This is called by device controller drivers after returning the completed
+ * request back to the gadget layer, to either complete or delay the status
+ * stage.
+ */
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+ unsigned int explicit_status, int status)
+{
+ struct usb_request *req;
+
+ if (explicit_status || status)
+ return;
+
+ /* Send an implicit status-stage request for ep0 */
+ req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
+ if (req) {
+ req->length = 0;
+ req->explicit_status = 0;
+ req->complete = usb_ep_free_request;
+ usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
+ }
+}
+EXPORT_SYMBOL_GPL(usb_gadget_control_complete);
+
+/* - */
+
/**
* gadget_find_ep_by_name - returns ep whose name is the same as sting passed
* in second parameter or NULL if searched endpoint not found
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..1e3a23637468 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -73,6 +73,7 @@ struct usb_ep;
* Note that for writes (IN transfers) some data bytes may still
* reside in a device-side FIFO when the request is reported as
* complete.
+ * @explicit_status: If true, delays the status stage
*
* These are allocated/freed through the endpoint they're used with. The
* hardware's driver can add extra per-request data to the memory it returns,
@@ -114,6 +115,8 @@ struct usb_request {
int status;
unsignedactual;
+
+ unsignedexplicit_status:1;
};
/*-*/
@@ -850,6 +853,13 @@ extern void usb_gadget_giveback_request(struct usb_ep *ep,
/*-*/
+/* utility to complete or delay status stage */
+
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+ unsigned int explicit_status, int status);
+
+/*-*/
+
/* utility to find endpoint by name */
extern struct usb_ep *gadget_find_ep_by_name(struct usb_gadget *g,
--
2.19.2
[PATCH v3 2/6] usb: gadget: uvc: enqueue usb request in setup handler for control OUT
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
Reviewed-by: Laurent Pinchart
---
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c| 25 +++--
drivers/usb/gadget/function/uvc_v4l2.c | 7 +++
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 3e07dcb885b7..772c99c297fc 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 7bfa43dbef9e..35353ffdf3b4 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.19.2
[PATCH v3 3/6] usb: gadget: uvc: package setup and data for control OUT requests
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
Reviewed-by: Laurent Pinchart
---
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c | 3 +++
drivers/usb/gadget/function/uvc.h | 1 +
include/uapi/linux/usb/g_uvc.h | 3 ++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 772c99c297fc..d08957124e42 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -215,6 +215,8 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
+ memcpy(&uvc_event->data.setup, &uvc->control_setup,
+ sizeof(uvc_event->data.setup));
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -238,6 +240,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
uvc->event_length = le16_to_cpu(ctrl->wLength);
+ memcpy(&uvc->control_setup, ctrl, sizeof(uvc->control_setup));
if (uvc->event_setup_out) {
struct usb_request *req = uvc->control_req;
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 671020c8a836..1d89b1df4ba0 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -163,6 +163,7 @@ struct uvc_device {
unsigned int control_intf;
struct usb_ep *control_ep;
struct usb_request *control_req;
+ struct usb_ctrlrequest control_setup;
void *control_buf;
unsigned int streaming_intf;
diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h
index 6698c3263ae8..10fbb4382925 100644
--- a/include/uapi/linux/usb/g_uvc.h
+++ b/include/uapi/linux/usb/g_uvc.h
@@ -24,7 +24,8 @@
struct uvc_request_data {
__s32 length;
- __u8 data[60];
+ struct usb_ctrlrequest setup;
+ __u8 data[52];
};
struct uvc_event {
--
2.19.2
[PATCH v3 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request
This patch series adds a mechanism to allow asynchronously validating the data stage of a control OUT request, and for stalling or suceeding the request accordingly. This mechanism is implemented for MUSB, and is used by UVC. At the same time, UVC packages the setup stage and data stage data together to send to userspace to save on a pair of context switches per control out request. This patch series does change the userspace API. We however believe that it is justified because the current API is broken, and because it isn't being used (because it's broken). The current API is broken such that it is subject to race conditions that cause fatal errors with a high frequency. This is actually what motivated this patch series in the first place. In the current API, not only is there no way to asynchronously validate the data stage of a control OUT request, but an empty buffer is expected to be provided to hold the data stage data -- which is more likely than not to be late. There is even a warning in musb_g_ep0_queue: /* else for sequence #2 (OUT), caller provides a buffer * before the next packet arrives. deferred responses * (after SETUP is acked) are racey. */ This problem has never been reported in years, which is a sign that the API isn't used. Furthermore, the vendor kernels that we have seen using the UVC gadget driver (such as QC and Huawei) are heavily patched with local changes to the API. This corroborates the suspicion that the current mainline API is not being used. Additionally, this API isn't meant to be used by generic applications, but by a dedicated userspace helper. uvc-gadget is one such example, but it has bitrotten and isn't compatible with the current kernel API. The fact that nobody has submitted patches nor complained for a long time again shows that it isn't being used. The conclusion is that since the API hasn't been used for a long time, it is safe to fix it. Changes in v3: - Function driver send STALL status stage by simply calling usb_ep_set_halt, and ACK by enqueueing request - Fix signature of usb_gadget_control_complete to check the status of the request that was just given back. - Corresponding changes to musb and uvc gadget Changes in v2: Overhaul of status stage delay mechanism/API. Now if a function driver desires an explicit/delayed status stage, it specifies so in a flag in the usb_request that is queued for the data stage. The function driver later enqueues another usb_request for the status stage, also with the explicit_status flag set, and with the zero flag acting as the status. If a function driver does not desire an explicit status stage, then it can set (or ignore) the explicit_status flag in the usb_request that is queued for the data stage. To allow the optional explicit status stage, a UDC driver should call the newly added usb_gadget_control_complete right after usb_gadget_giveback_request, and in its queue function should check if the usb_request is for the status stage and if it has been requested to be explicit, and if so check the status that should be sent. (See 5/6 "usb: musb: gadget: implement optional explicit status stage" for an implementation for MUSB) Paul Elder (6): usb: uvc: include videodev2.h in g_uvc.h usb: gadget: uvc: enqueue usb request in setup handler for control OUT usb: gadget: uvc: package setup and data for control OUT requests usb: gadget: add mechanism to specify an explicit status stage usb: musb: gadget: implement optional explicit status stage usb: gadget: uvc: allow ioctl to send response in status stage drivers/usb/gadget/function/f_uvc.c| 32 ++-- drivers/usb/gadget/function/uvc.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 18 ++ drivers/usb/gadget/udc/core.c | 34 ++ drivers/usb/musb/musb_gadget.c | 2 ++ drivers/usb/musb/musb_gadget_ep0.c | 23 + include/linux/usb/gadget.h | 10 include/uapi/linux/usb/g_uvc.h | 4 ++- 8 files changed, 115 insertions(+), 9 deletions(-) -- 2.19.2
[PATCH v3 6/6] usb: gadget: uvc: allow ioctl to send response in status stage
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
---
Changes from v2:
- calling usb_ep_set_halt in uvc_send_response if data->length < 0 is
now common for both IN and OUT transfers so make that check common
- remove now unnecessary field setting for the usb_request to be queued
for the status stage
Changes from v1:
- remove usb_ep_delay_status call from the old proposed API
- changed portions of uvc_send_response to match v2 API
- remove UDC warning that send_response is not implemented
drivers/usb/gadget/function/f_uvc.c| 4 ++--
drivers/usb/gadget/function/uvc_v4l2.c | 23 +--
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index d08957124e42..91388bb647e0 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -209,14 +209,13 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
if (uvc->event_setup_out) {
- uvc->event_setup_out = 0;
-
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
memcpy(&uvc_event->data.setup, &uvc->control_setup,
sizeof(uvc_event->data.setup));
+
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -251,6 +250,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
req->length = uvc->event_length;
req->zero = 0;
+ req->explicit_status = 1;
usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
} else {
struct v4l2_event v4l2_event;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index 35353ffdf3b4..0d4993144ab8 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,15 +35,26 @@ uvc_send_response(struct uvc_device *uvc, struct
uvc_request_data *data)
struct usb_composite_dev *cdev = uvc->func.config->cdev;
struct usb_request *req = uvc->control_req;
+ if (data->length < 0)
+ return usb_ep_set_halt(cdev->gadget->ep0);
+
/*
* For control OUT transfers the request has been enqueued synchronously
-* by the setup handler, there's nothing to be done here.
+* by the setup handler, we just need to tell the UDC whether to ACK or
+* STALL the control transfer.
*/
- if (uvc->event_setup_out)
- return 0;
-
- if (data->length < 0)
- return usb_ep_set_halt(cdev->gadget->ep0);
+ if (uvc->event_setup_out) {
+ /*
+* The length field carries the control request status.
+* Negative values signal a STALL and zero values an ACK.
+* Positive values are not valid as there is no data to send
+* back in the status stage.
+*/
+ if (data->length > 0)
+ return -EINVAL;
+
+ return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL);
+ }
req->length = min_t(unsigned int, uvc->event_length, data->length);
req->zero = data->length < uvc->event_length;
--
2.19.2
Re: [PATCH v3 4/6] usb: gadget: add mechanism to specify an explicit status stage
On Wed, Dec 19, 2018 at 11:01:52AM -0500, Alan Stern wrote:
> On Wed, 19 Dec 2018, 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
> -^
> Typo: missing "where"
ack
>
> > asynchronously validate the data of a class-specific request.
> >
> > A function driver that wants an explicit status stage should set the
> > newly added explicit_status flag of the usb_request corresponding to the
> > data stage. Later on the function driver can explicitly complete the
> > status stage by enqueueing a usb_request for ACK, or calling
> > usb_ep_set_halt() for STALL.
> >
> > To support both explicit and implicit status stages, a UDC driver must
> > call the newly added usb_gadget_control_complete function right after
> > calling usb_gadget_giveback_request. The status of the request that was
> > just given back must be fed into usb_gadget_control_complete. To support
> > the explicit status stage, it might then check what stage the
> > usb_request was queued in, and send an ACK.
>
> I don't really understand that last sentence. Perhaps what you mean is
> that depending on the direction of the control transfer, the driver
> should either ACK the host's 0-length data packet (control-IN) or send
> a 0-length DATA1 packet (control-OUT)?
Yes, that is what I meant, though for MUSB I've only implemented and
tested the control OUT case.
> > Signed-off-by: Paul Elder
> > v1 Reviewed-by: Laurent Pinchart
> > ---
> > Changes from v2:
> >
> > Add status parameter to usb_gadget_control_complete, so that a
> > usb_request is not queued if the status of the just given back request
> > is nonzero.
> >
> > Changes from v1:
> >
> > Complete change of API. Now we use a flag that should be set in the
> > usb_request that is queued for the data stage to signal to the UDC that
> > we want to delay the status stage (as opposed to setting a flag in the
> > UDC itself, that persists across all requests). We now also provide a
> > function for UDC drivers to very easily allow implicit status stages, to
> > mitigate the need to convert all function drivers to this new API at
> > once, and to make it easier for UDC drivers to convert.
> >
> > drivers/usb/gadget/udc/core.c | 34 ++
> > include/linux/usb/gadget.h| 10 ++
> > 2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index af88b48c1cea..0a0ccd2b7639 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -894,6 +894,40 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
> >
> > /*
> > - */
> >
> > +/**
> > + * usb_gadget_control_complete - complete the status stage of a control
> > + * request, or delay it
> > + * Context: in_interrupt()
> > + *
> > + * @gadget: gadget whose control request's status stage should be completed
> > + * @explicit_status: true to delay status stage, false to complete here
> > + * @status: completion code of previously completed request
> > + *
> > + * This is called by device controller drivers after returning the
> > completed
> > + * request back to the gadget layer, to either complete or delay the status
> > + * stage.
> > + */
> > +void usb_gadget_control_complete(struct usb_gadget *gadget,
> > + unsigned int explicit_status, int status)
> > +{
> > + struct usb_request *req;
> > +
> > + if (explicit_status || status)
> > + return;
> > +
> > + /* Send an implicit status-stage request for ep0 */
> > + req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
> > + if (req) {
> > + req->length = 0;
> > + req->explicit_status = 0;
>
> Oops! I should have spotted this in the previous version, sorry. The
> implicit status-stage request should have its ->explicit_status set, so
> that we don't try to submit another status request when this one
> completes.
No, it's fine; the previous version had it set to 1 :)
I didn't think it mattered, but the way you describe it makes it seem
like it would matter depending on the UDC driver implementation, so I'll
put it back in.
> Also, would it look better if the type of explicit_status was bool
> instead of unsigned int (bot
[PATCH v4 5/6] usb: musb: gadget: implement optional explicit status stage
Implement the mechanism for optional explicit status stage for the MUSB
driver. This allows a function driver to specify what to reply for the
status stage. The functionality for an implicit status stage is
retained.
Signed-off-by: Paul Elder
v1 Reviewed-by: Laurent Pinchart
v1 Acked-by: Bin Liu
---
No change from v3
Changes from v2:
- update call to usb_gadget_control_complete to include status
- since sending STALL from the function driver is now done with
usb_ep_set_halt, there is no need for the internal ep0_send_response to
take a stall/ack parameter; remove the parameter and make the function
only send ack, and remove checking for the status reply in the
usb_request for the status stage
Changes from v1:
- obvious change to implement v2 mechanism laid out by 4/6 of this
series (send_response, and musb_g_ep0_send_response function has
been removed, call to usb_gadget_control_complete has been added)
- ep0_send_response's ack argument has been changed from stall
- last_packet flag in ep0_rxstate has been removed, since it is equal to
req != NULL
drivers/usb/musb/musb_gadget.c | 2 ++
drivers/usb/musb/musb_gadget_ep0.c | 23 +++
2 files changed, 25 insertions(+)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index d3f33f449445..a7a992ab0c9d 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -145,6 +145,8 @@ __acquires(ep->musb->lock)
trace_musb_req_gb(req);
usb_gadget_giveback_request(&req->ep->end_point, &req->request);
+ usb_gadget_control_complete(&musb->g, request->explicit_status,
+ request->status);
spin_lock(&musb->lock);
ep->busy = busy;
}
diff --git a/drivers/usb/musb/musb_gadget_ep0.c
b/drivers/usb/musb/musb_gadget_ep0.c
index 91a5027b5c1f..bbce8a9d77e4 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -458,6 +458,23 @@ __acquires(musb->lock)
return handled;
}
+static int ep0_send_ack(struct musb *musb)
+{
+ 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;
+
+ 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
*/
@@ -504,10 +521,13 @@ static void ep0_rxstate(struct musb *musb)
if (req) {
musb->ackpend = csr;
musb_g_ep0_giveback(musb, req);
+ if (req->explicit_status)
+ return;
if (!musb->ackpend)
return;
musb->ackpend = 0;
}
+
musb_ep_select(musb->mregs, 0);
musb_writew(regs, MUSB_CSR0, csr);
}
@@ -939,6 +959,9 @@ musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r,
gfp_t gfp_flags)
case MUSB_EP0_STAGE_ACKWAIT:/* zero-length data */
status = 0;
break;
+ case MUSB_EP0_STAGE_STATUSIN:
+ status = ep0_send_ack(musb);
+ goto cleanup;
default:
musb_dbg(musb, "ep0 request queued in state %d",
musb->ep0_state);
--
2.19.2
[PATCH v4 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request
This patch series adds a mechanism to allow asynchronously validating the data stage of a control OUT request, and for stalling or suceeding the request accordingly. This mechanism is implemented for MUSB, and is used by UVC. At the same time, UVC packages the setup stage and data stage data together to send to userspace to save on a pair of context switches per control out request. This patch series does change the userspace API. We however believe that it is justified because the current API is broken, and because it isn't being used (because it's broken). The current API is broken such that it is subject to race conditions that cause fatal errors with a high frequency. This is actually what motivated this patch series in the first place. In the current API, not only is there no way to asynchronously validate the data stage of a control OUT request, but an empty buffer is expected to be provided to hold the data stage data -- which is more likely than not to be late. There is even a warning in musb_g_ep0_queue: /* else for sequence #2 (OUT), caller provides a buffer * before the next packet arrives. deferred responses * (after SETUP is acked) are racey. */ This problem has never been reported in years, which is a sign that the API isn't used. Furthermore, the vendor kernels that we have seen using the UVC gadget driver (such as QC and Huawei) are heavily patched with local changes to the API. This corroborates the suspicion that the current mainline API is not being used. Additionally, this API isn't meant to be used by generic applications, but by a dedicated userspace helper. uvc-gadget is one such example, but it has bitrotten and isn't compatible with the current kernel API. The fact that nobody has submitted patches nor complained for a long time again shows that it isn't being used. The conclusion is that since the API hasn't been used for a long time, it is safe to fix it. Changes in v4: - Change wording and fix typo in patch 4/6 "usb: gadget: add mechanism to specify an explicit status stage" - Set explicit_status in usb_gadget_control_complete - Change explicit_status from unsigned int to bool Changes in v3: - Function driver send STALL status stage by simply calling usb_ep_set_halt, and ACK by enqueueing request - Fix signature of usb_gadget_control_complete to check the status of the request that was just given back. - Corresponding changes to musb and uvc gadget Changes in v2: Overhaul of status stage delay mechanism/API. Now if a function driver desires an explicit/delayed status stage, it specifies so in a flag in the usb_request that is queued for the data stage. The function driver later enqueues another usb_request for the status stage, also with the explicit_status flag set, and with the zero flag acting as the status. If a function driver does not desire an explicit status stage, then it can set (or ignore) the explicit_status flag in the usb_request that is queued for the data stage. To allow the optional explicit status stage, a UDC driver should call the newly added usb_gadget_control_complete right after usb_gadget_giveback_request, and in its queue function should check if the usb_request is for the status stage and if it has been requested to be explicit, and if so check the status that should be sent. (See 5/6 "usb: musb: gadget: implement optional explicit status stage" for an implementation for MUSB) Paul Elder (6): usb: uvc: include videodev2.h in g_uvc.h usb: gadget: uvc: enqueue usb request in setup handler for control OUT usb: gadget: uvc: package setup and data for control OUT requests usb: gadget: add mechanism to specify an explicit status stage usb: musb: gadget: implement optional explicit status stage usb: gadget: uvc: allow ioctl to send response in status stage drivers/usb/gadget/function/f_uvc.c| 32 ++-- drivers/usb/gadget/function/uvc.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 18 ++ drivers/usb/gadget/udc/core.c | 34 ++ drivers/usb/musb/musb_gadget.c | 2 ++ drivers/usb/musb/musb_gadget_ep0.c | 23 + include/linux/usb/gadget.h | 10 include/uapi/linux/usb/g_uvc.h | 4 ++- 8 files changed, 115 insertions(+), 9 deletions(-) -- 2.19.2 Paul Elder (6): usb: uvc: include videodev2.h in g_uvc.h usb: gadget: uvc: enqueue usb request in setup handler for control OUT usb: gadget: uvc: package setup and data for control OUT requests usb: gadget: add mechanism to specify an explicit status stage usb: musb: gadget: implement optional explicit status stage usb: gadget: uvc: allow ioctl to send response in status stage drivers/usb/gadget/function/f_uvc.c| 32 ++-- drivers/usb/gadget/function/uvc.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 18 ++ drivers/usb/gadget/udc/core.c
[PATCH v4 3/6] usb: gadget: uvc: package setup and data for control OUT requests
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
Reviewed-by: Laurent Pinchart
---
No change from v3
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c | 3 +++
drivers/usb/gadget/function/uvc.h | 1 +
include/uapi/linux/usb/g_uvc.h | 3 ++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 96054403418a..aa8262c0a650 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -215,6 +215,8 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
+ memcpy(&uvc_event->data.setup, &uvc->control_setup,
+ sizeof(uvc_event->data.setup));
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -238,6 +240,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
uvc->event_length = le16_to_cpu(ctrl->wLength);
+ memcpy(&uvc->control_setup, ctrl, sizeof(uvc->control_setup));
if (uvc->event_setup_out) {
struct usb_request *req = uvc->control_req;
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 671020c8a836..1d89b1df4ba0 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -163,6 +163,7 @@ struct uvc_device {
unsigned int control_intf;
struct usb_ep *control_ep;
struct usb_request *control_req;
+ struct usb_ctrlrequest control_setup;
void *control_buf;
unsigned int streaming_intf;
diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h
index 6698c3263ae8..10fbb4382925 100644
--- a/include/uapi/linux/usb/g_uvc.h
+++ b/include/uapi/linux/usb/g_uvc.h
@@ -24,7 +24,8 @@
struct uvc_request_data {
__s32 length;
- __u8 data[60];
+ struct usb_ctrlrequest setup;
+ __u8 data[52];
};
struct uvc_event {
--
2.19.2
[PATCH v4 4/6] usb: gadget: add mechanism to specify an explicit status stage
A usb gadget function driver may or may not want to delay the status
stage of a control OUT request. An instance where it might want to is to
asynchronously validate the data of a class-specific request.
A function driver that wants an explicit status stage should set the
newly added explicit_status flag of the usb_request corresponding to the
data stage. Later on the function driver can explicitly complete the
status stage by enqueueing a usb_request for ACK, or calling
usb_ep_set_halt() for STALL.
To support both explicit and implicit status stages, a UDC driver must
call the newly added usb_gadget_control_complete function right after
calling usb_gadget_giveback_request. The status of the request that was
just given back must be fed into usb_gadget_control_complete. To support
the explicit status stage, it might then check what stage the
usb_request was queued in, and for control IN ACK the host's zero-length
data packet, or for control OUT send a zero-length DATA1 ACK packet.
Signed-off-by: Paul Elder
Acked-by: Alan Stern
v1 Reviewed-by: Laurent Pinchart
---
Changes from v3:
- More specific in commit message about what to do for udc driver acking
- Set explicit_status in usb_gadget_control_complete
- Make explicit_status type bool
Changes from v2:
Add status parameter to usb_gadget_control_complete, so that a
usb_request is not queued if the status of the just given back request
is nonzero.
Changes from v1:
Complete change of API. Now we use a flag that should be set in the
usb_request that is queued for the data stage to signal to the UDC that
we want to delay the status stage (as opposed to setting a flag in the
UDC itself, that persists across all requests). We now also provide a
function for UDC drivers to very easily allow implicit status stages, to
mitigate the need to convert all function drivers to this new API at
once, and to make it easier for UDC drivers to convert.
drivers/usb/gadget/udc/core.c | 34 ++
include/linux/usb/gadget.h| 10 ++
2 files changed, 44 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48c1cea..d3071de2b527 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -894,6 +894,40 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
/* - */
+/**
+ * usb_gadget_control_complete - complete the status stage of a control
+ * request, or delay it
+ * Context: in_interrupt()
+ *
+ * @gadget: gadget whose control request's status stage should be completed
+ * @explicit_status: true to delay status stage, false to complete here
+ * @status: completion code of previously completed request
+ *
+ * This is called by device controller drivers after returning the completed
+ * request back to the gadget layer, to either complete or delay the status
+ * stage.
+ */
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+ bool explicit_status, int status)
+{
+ struct usb_request *req;
+
+ if (explicit_status || status)
+ return;
+
+ /* Send an implicit status-stage request for ep0 */
+ req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
+ if (req) {
+ req->length = 0;
+ req->explicit_status = 1;
+ req->complete = usb_ep_free_request;
+ usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
+ }
+}
+EXPORT_SYMBOL_GPL(usb_gadget_control_complete);
+
+/* - */
+
/**
* gadget_find_ep_by_name - returns ep whose name is the same as sting passed
* in second parameter or NULL if searched endpoint not found
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..498e3eaa72f1 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -73,6 +73,7 @@ struct usb_ep;
* Note that for writes (IN transfers) some data bytes may still
* reside in a device-side FIFO when the request is reported as
* complete.
+ * @explicit_status: If true, delays the status stage
*
* These are allocated/freed through the endpoint they're used with. The
* hardware's driver can add extra per-request data to the memory it returns,
@@ -114,6 +115,8 @@ struct usb_request {
int status;
unsignedactual;
+
+ boolexplicit_status;
};
/*-*/
@@ -850,6 +853,13 @@ extern void usb_gadget_giveback_request(struct usb_ep *ep,
/*-*/
+/* utility to complete or delay status stage */
+
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+
[PATCH v4 6/6] usb: gadget: uvc: allow ioctl to send response in status stage
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
---
No change from v3
Changes from v2:
- calling usb_ep_set_halt in uvc_send_response if data->length < 0 is
now common for both IN and OUT transfers so make that check common
- remove now unnecessary field setting for the usb_request to be queued
for the status stage
Changes from v1:
- remove usb_ep_delay_status call from the old proposed API
- changed portions of uvc_send_response to match v2 API
- remove UDC warning that send_response is not implemented
drivers/usb/gadget/function/f_uvc.c| 4 ++--
drivers/usb/gadget/function/uvc_v4l2.c | 23 +--
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index aa8262c0a650..64180529fb36 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -209,14 +209,13 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
if (uvc->event_setup_out) {
- uvc->event_setup_out = 0;
-
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
memcpy(&uvc_event->data.setup, &uvc->control_setup,
sizeof(uvc_event->data.setup));
+
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -251,6 +250,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
req->length = uvc->event_length;
req->zero = 0;
+ req->explicit_status = 1;
usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
} else {
struct v4l2_event v4l2_event;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index 9f7c67f6f4b2..0bf14891b40a 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,15 +35,26 @@ uvc_send_response(struct uvc_device *uvc, struct
uvc_request_data *data)
struct usb_composite_dev *cdev = uvc->func.config->cdev;
struct usb_request *req = uvc->control_req;
+ if (data->length < 0)
+ return usb_ep_set_halt(cdev->gadget->ep0);
+
/*
* For control OUT transfers the request has been enqueued synchronously
-* by the setup handler, there's nothing to be done here.
+* by the setup handler, we just need to tell the UDC whether to ACK or
+* STALL the control transfer.
*/
- if (uvc->event_setup_out)
- return 0;
-
- if (data->length < 0)
- return usb_ep_set_halt(cdev->gadget->ep0);
+ if (uvc->event_setup_out) {
+ /*
+* The length field carries the control request status.
+* Negative values signal a STALL and zero values an ACK.
+* Positive values are not valid as there is no data to send
+* back in the status stage.
+*/
+ if (data->length > 0)
+ return -EINVAL;
+
+ return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL);
+ }
req->length = min_t(unsigned int, uvc->event_length, data->length);
req->zero = data->length < uvc->event_length;
--
2.19.2
[PATCH v4 1/6] usb: uvc: include videodev2.h in g_uvc.h
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 Reviewed-by: Laurent Pinchart --- No change from v3 No change from v2 No change from v1 include/uapi/linux/usb/g_uvc.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h index 3c9ee3020cbb..6698c3263ae8 100644 --- a/include/uapi/linux/usb/g_uvc.h +++ b/include/uapi/linux/usb/g_uvc.h @@ -11,6 +11,7 @@ #include #include #include +#include #define UVC_EVENT_FIRST(V4L2_EVENT_PRIVATE_START + 0) #define UVC_EVENT_CONNECT (V4L2_EVENT_PRIVATE_START + 0) -- 2.19.2
[PATCH v4 2/6] usb: gadget: uvc: enqueue usb request in setup handler for control OUT
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
Reviewed-by: Laurent Pinchart
---
No change from v3
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c| 25 +++--
drivers/usb/gadget/function/uvc_v4l2.c | 7 +++
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 3472a2863436..96054403418a 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 4b0a82cc4763..9f7c67f6f4b2 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.19.2
Re: [PATCH] usb: gadget: musb: fix short isoc packets with inventra dma for pandaboard es
Hi Bin,
On Mon, Jan 07, 2019 at 01:11:57PM -0600, Bin Liu wrote:
> Hi Paul,
>
> Sorry for the delay on reviewing it.
Thanks for the review.
> For the subject, can you please use
>
> usb: musb: gadget: fix short isoc packets with inventra dma
ack
> On Tue, Oct 09, 2018 at 02:32:20AM -0400, Paul Elder wrote:
> > Handling short packets (length < max packet size) in the Inventra DMA
> > engine in the MUSB driver causes the MUSB DMA controller to hang. An
> > example of a problem that is caused by this problem is when streaming
> > video out of a UVC gadget, only the first video frame is transferred.
> >
> > For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
> > set manually by the driver. This was previously done in musb_g_tx
> > (musb_gadget.c), but incorrectly (all csr flags were cleared, and only
> > MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows
> > some requests to be transferred correctly, but multiple requests were
> > often put together in one USB packet, and caused problems if the packet
> > size was not a multiple of 4.
> >
> > Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c),
> > just like host mode transfers, then musb_g_tx forces the packet to be
> > flushed, by setting MUSB_TXCSR_FLUSHFIFO.
> >
> > This topic was originally tackled by Nicolas Boichat [0] [1] and is
> > discussed
>
> this line is longer than 75 chars.
ack
> > further at [2] as part of his GSoC project [3].
> >
> > [0]
> > https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
> > [1]
> > https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
> > [2]
> > http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
> > [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
> >
> > I have forward-ported this patch from 2.6.34 to 4.19-rc1.
>
> this line is irrelevant to the commit message, please move it down to
> under '---'.
ack
> >
> > Signed-off-by: Paul Elder
> > ---
> > drivers/usb/musb/musb_gadget.c | 21 ++---
> > drivers/usb/musb/musbhsdma.c | 21 +++--
> > 2 files changed, 25 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > index eae8b1b1b45b..d3f33f449445 100644
> > --- a/drivers/usb/musb/musb_gadget.c
> > +++ b/drivers/usb/musb/musb_gadget.c
> > @@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > && (request->actual == request->length))
> > short_packet = true;
> >
> > - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
> > - (is_dma && (!dma->desired_mode ||
> > - (request->actual &
> > - (musb_ep->packet_sz - 1)
> > - short_packet = true;
> > + if (is_dma && (musb_dma_inventra(musb) ||
> > musb_dma_ux500(musb))) {
>
> more than 80 chars.
>
> > + if (!dma->desired_mode ||
>
> I understand you forward-port Nicolas' patch, but do you have a specific
> readon to re-write this 'if' condition? I'd like to see minimum code
> change in bug fixes,
>
> > + request->actual % musb_ep->packet_sz) {
>
> but I like this version though, easier to read.
>
> > + musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n",
>
> what is 'KPB'? the message could be more meaningful?
>
> > + musb_ep->end_point.name);
> > + musb_writew(epio, MUSB_TXCSR,
> > + csr | MUSB_TXCSR_FLUSHFIFO);
>
> What if without this line? The short packet doesn't send out? setting
> TXSCR_TXPKTRDY in the dma driver is not sufficient? TXSCR_FLUSHFIFO is
> only used for aborting cases.
I just tested this and you are right, it does work without this line.
Since this is the only significant line in this very complex if block,
I'll just remove this entire block too.
> > + return;
> > + }
> > + }
> >
> > if (short_pack
Re: [PATCH v4 5/6] usb: musb: gadget: implement optional explicit status stage
On Sun, Jan 06, 2019 at 03:03:09PM -0500, Alan Stern wrote: > On Sun, 6 Jan 2019, Paul Elder wrote: > > > Implement the mechanism for optional explicit status stage for the MUSB > > driver. This allows a function driver to specify what to reply for the > > status stage. The functionality for an implicit status stage is > > retained. > > > > Signed-off-by: Paul Elder > > v1 Reviewed-by: Laurent Pinchart > > v1 Acked-by: Bin Liu > > --- > > No change from v3 > > > > Changes from v2: > > - update call to usb_gadget_control_complete to include status > > - since sending STALL from the function driver is now done with > > usb_ep_set_halt, there is no need for the internal ep0_send_response to > > take a stall/ack parameter; remove the parameter and make the function > > only send ack, and remove checking for the status reply in the > > usb_request for the status stage > > > > Changes from v1: > > - obvious change to implement v2 mechanism laid out by 4/6 of this > > series (send_response, and musb_g_ep0_send_response function has > > been removed, call to usb_gadget_control_complete has been added) > > - ep0_send_response's ack argument has been changed from stall > > - last_packet flag in ep0_rxstate has been removed, since it is equal to > > req != NULL > > > > drivers/usb/musb/musb_gadget.c | 2 ++ > > drivers/usb/musb/musb_gadget_ep0.c | 23 +++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c > > index d3f33f449445..a7a992ab0c9d 100644 > > --- a/drivers/usb/musb/musb_gadget.c > > +++ b/drivers/usb/musb/musb_gadget.c > > @@ -145,6 +145,8 @@ __acquires(ep->musb->lock) > > > > trace_musb_req_gb(req); > > usb_gadget_giveback_request(&req->ep->end_point, &req->request); > > + usb_gadget_control_complete(&musb->g, request->explicit_status, > > + request->status); > > I haven't paid much attention to this part of the patch series, not > knowing much about musb. Still, it's clear that > usb_gadget_control_complete should be called only for transfers on > ep0. You need to test the endpoint value. Oh oops, yeah, you're right. > Another problem: the completion handler may deallocate the request. > Dereferencing request->expicit_status and request->status would then > cause errors. Would it be preferable to call > usb_gadget_control_complete before usb_gadget_giveback_request? If > it gets done that way then the arguments could be simplified: we could > pass a pointer to the request instead of the separate explicit_status > and status values. I thought that usb_gadget_control_complete needs to check the status of the request that was given back. Doesn't that mean that usb_gadget_giveback_request must be called first? I was thinking that maybe we could save explicit_status before calling usb_gadget_giveback_request, and if request is still valid then we can pull status from it otherwise use 0, but then would that be considered adding complexity to UDCs that want to implement optional status stage delay? Or add a wrapper function? On the other hand, if we do put usb_gadget_control_complete before usb_gadget_giveback_request, then the control transfer would complete before the function driver has a chance to complete, but if the function driver wanted to intervene/determine the status stage then it would have gone through the new mechanism that we're making here. So it could be fine to switch the order. My tests for it work too, so I guess we'll go with usb_gadget_control_complete before usb_gadget_giveback_request then. In that case usb_gadget_control_complete doesn't need to check the status of the request, since there isn't any, right? Thanks, Paul Elder
[PATCH v5 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request
This patch series adds a mechanism to allow asynchronously validating the data stage of a control OUT request, and for stalling or suceeding the request accordingly. This mechanism is implemented for MUSB, and is used by UVC. At the same time, UVC packages the setup stage and data stage data together to send to userspace to save on a pair of context switches per control out request. This patch series does change the userspace API. We however believe that it is justified because the current API is broken, and because it isn't being used (because it's broken). The current API is broken such that it is subject to race conditions that cause fatal errors with a high frequency. This is actually what motivated this patch series in the first place. In the current API, not only is there no way to asynchronously validate the data stage of a control OUT request, but an empty buffer is expected to be provided to hold the data stage data -- which is more likely than not to be late. There is even a warning in musb_g_ep0_queue: /* else for sequence #2 (OUT), caller provides a buffer * before the next packet arrives. deferred responses * (after SETUP is acked) are racey. */ This problem has never been reported in years, which is a sign that the API isn't used. Furthermore, the vendor kernels that we have seen using the UVC gadget driver (such as QC and Huawei) are heavily patched with local changes to the API. This corroborates the suspicion that the current mainline API is not being used. Additionally, this API isn't meant to be used by generic applications, but by a dedicated userspace helper. uvc-gadget is one such example, but it has bitrotten and isn't compatible with the current kernel API. The fact that nobody has submitted patches nor complained for a long time again shows that it isn't being used. The conclusion is that since the API hasn't been used for a long time, it is safe to fix it. Changes in v5: - Change parameter of usb_gadget_control_complete to simply take a usb_request - Make usb_gadget_control_complete do nothing if the request has no length (ie. no data stage) - musb: call usb_gadget_control_complete before usb_gadget_giveback_request - set musb ep0 state to statusin in ep0_send_ack - make sure to not double-write musb register in ep0_rxstate, since musb_g_ep0_giveback will take care of writing them Changes in v4: - Change wording and fix typo in patch 4/6 "usb: gadget: add mechanism to specify an explicit status stage" - Set explicit_status in usb_gadget_control_complete - Change explicit_status from unsigned int to bool Changes in v3: - Function driver send STALL status stage by simply calling usb_ep_set_halt, and ACK by enqueueing request - Fix signature of usb_gadget_control_complete to check the status of the request that was just given back. - Corresponding changes to musb and uvc gadget Changes in v2: Overhaul of status stage delay mechanism/API. Now if a function driver desires an explicit/delayed status stage, it specifies so in a flag in the usb_request that is queued for the data stage. The function driver later enqueues another usb_request for the status stage, also with the explicit_status flag set, and with the zero flag acting as the status. If a function driver does not desire an explicit status stage, then it can set (or ignore) the explicit_status flag in the usb_request that is queued for the data stage. To allow the optional explicit status stage, a UDC driver should call the newly added usb_gadget_control_complete right after usb_gadget_giveback_request, and in its queue function should check if the usb_request is for the status stage and if it has been requested to be explicit, and if so check the status that should be sent. (See 5/6 "usb: musb: gadget: implement optional explicit status stage" for an implementation for MUSB) Paul Elder (6): usb: uvc: include videodev2.h in g_uvc.h usb: gadget: uvc: enqueue usb request in setup handler for control OUT usb: gadget: uvc: package setup and data for control OUT requests usb: gadget: add mechanism to specify an explicit status stage usb: musb: gadget: implement optional explicit status stage usb: gadget: uvc: allow ioctl to send response in status stage drivers/usb/gadget/function/f_uvc.c| 32 ++-- drivers/usb/gadget/function/uvc.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 18 ++ drivers/usb/gadget/udc/core.c | 34 ++ drivers/usb/musb/musb_gadget.c | 2 ++ drivers/usb/musb/musb_gadget_ep0.c | 29 -- include/linux/usb/gadget.h | 10 include/uapi/linux/usb/g_uvc.h | 4 ++- 8 files changed, 119 insertions(+), 11 deletions(-) -- 2.20.1
[PATCH v5 5/6] usb: musb: gadget: implement optional explicit status stage
Implement the mechanism for optional explicit status stage for the MUSB
driver. This allows a function driver to specify what to reply for the
status stage. The functionality for an implicit status stage is
retained.
Signed-off-by: Paul Elder
v1 Reviewed-by: Laurent Pinchart
v1 Acked-by: Bin Liu
---
Changes from v4:
- call usb_gadget_control_complete before usb_gadget_giveback_request
- set musb ep0 state to statusin in ep0_send_ack
- make sure to not double-write musb register in ep0_rxstate, since
musb_g_ep0_giveback will take care of writing them
No change from v3
Changes from v2:
- update call to usb_gadget_control_complete to include status
- since sending STALL from the function driver is now done with
usb_ep_set_halt, there is no need for the internal ep0_send_response to
take a stall/ack parameter; remove the parameter and make the function
only send ack, and remove checking for the status reply in the
usb_request for the status stage
Changes from v1:
- obvious change to implement v2 mechanism laid out by 4/6 of this
series (send_response, and musb_g_ep0_send_response function has
been removed, call to usb_gadget_control_complete has been added)
- ep0_send_response's ack argument has been changed from stall
- last_packet flag in ep0_rxstate has been removed, since it is equal to
req != NULL
drivers/usb/musb/musb_gadget.c | 2 ++
drivers/usb/musb/musb_gadget_ep0.c | 29 +++--
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 496643f54faa..ff83578c181f 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -144,6 +144,8 @@ __acquires(ep->musb->lock)
unmap_dma_buffer(req, musb);
trace_musb_req_gb(req);
+ if (req->ep->end_point.address == 0)
+ usb_gadget_control_complete(&musb->g, &req->request);
usb_gadget_giveback_request(&req->ep->end_point, &req->request);
spin_lock(&musb->lock);
ep->busy = busy;
diff --git a/drivers/usb/musb/musb_gadget_ep0.c
b/drivers/usb/musb/musb_gadget_ep0.c
index 91a5027b5c1f..dc5abdf9d49e 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -458,6 +458,25 @@ __acquires(musb->lock)
return handled;
}
+static int ep0_send_ack(struct musb *musb)
+{
+ void __iomem *regs = musb->control_ep->regs;
+ u16 csr;
+
+ if (musb->ep0_state != MUSB_EP0_STAGE_RX &&
+ musb->ep0_state != MUSB_EP0_STAGE_STATUSIN)
+ return -EINVAL;
+
+ csr = MUSB_CSR0_P_DATAEND | MUSB_CSR0_P_SVDRXPKTRDY;
+
+ musb_ep_select(musb->mregs, 0);
+ musb_writew(regs, MUSB_CSR0, csr);
+
+ musb->ep0_state = MUSB_EP0_STAGE_STATUSIN;
+
+ return 0;
+}
+
/* we have an ep0out data packet
* Context: caller holds controller lock
*/
@@ -504,12 +523,15 @@ static void ep0_rxstate(struct musb *musb)
if (req) {
musb->ackpend = csr;
musb_g_ep0_giveback(musb, req);
+ if (req->explicit_status)
+ return;
if (!musb->ackpend)
return;
musb->ackpend = 0;
+ } else {
+ musb_ep_select(musb->mregs, 0);
+ musb_writew(regs, MUSB_CSR0, csr);
}
- musb_ep_select(musb->mregs, 0);
- musb_writew(regs, MUSB_CSR0, csr);
}
/*
@@ -939,6 +961,9 @@ musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r,
gfp_t gfp_flags)
case MUSB_EP0_STAGE_ACKWAIT:/* zero-length data */
status = 0;
break;
+ case MUSB_EP0_STAGE_STATUSIN:
+ status = ep0_send_ack(musb);
+ goto cleanup;
default:
musb_dbg(musb, "ep0 request queued in state %d",
musb->ep0_state);
--
2.20.1
[PATCH v5 6/6] usb: gadget: uvc: allow ioctl to send response in status stage
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
---
No change from v4
No change from v3
Changes from v2:
- calling usb_ep_set_halt in uvc_send_response if data->length < 0 is
now common for both IN and OUT transfers so make that check common
- remove now unnecessary field setting for the usb_request to be queued
for the status stage
Changes from v1:
- remove usb_ep_delay_status call from the old proposed API
- changed portions of uvc_send_response to match v2 API
- remove UDC warning that send_response is not implemented
drivers/usb/gadget/function/f_uvc.c| 4 ++--
drivers/usb/gadget/function/uvc_v4l2.c | 23 +--
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 58ed191f636e..053808b01d48 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -209,14 +209,13 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
if (uvc->event_setup_out) {
- uvc->event_setup_out = 0;
-
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
memcpy(&uvc_event->data.setup, &uvc->control_setup,
sizeof(uvc_event->data.setup));
+
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -251,6 +250,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
req->length = uvc->event_length;
req->zero = 0;
+ req->explicit_status = 1;
usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
} else {
struct v4l2_event v4l2_event;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index ac48f49d9f10..353071869e15 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,15 +35,26 @@ uvc_send_response(struct uvc_device *uvc, struct
uvc_request_data *data)
struct usb_composite_dev *cdev = uvc->func.config->cdev;
struct usb_request *req = uvc->control_req;
+ if (data->length < 0)
+ return usb_ep_set_halt(cdev->gadget->ep0);
+
/*
* For control OUT transfers the request has been enqueued synchronously
-* by the setup handler, there's nothing to be done here.
+* by the setup handler, we just need to tell the UDC whether to ACK or
+* STALL the control transfer.
*/
- if (uvc->event_setup_out)
- return 0;
-
- if (data->length < 0)
- return usb_ep_set_halt(cdev->gadget->ep0);
+ if (uvc->event_setup_out) {
+ /*
+* The length field carries the control request status.
+* Negative values signal a STALL and zero values an ACK.
+* Positive values are not valid as there is no data to send
+* back in the status stage.
+*/
+ if (data->length > 0)
+ return -EINVAL;
+
+ return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL);
+ }
req->length = min_t(unsigned int, uvc->event_length, data->length);
req->zero = data->length < uvc->event_length;
--
2.20.1
[PATCH v5 3/6] usb: gadget: uvc: package setup and data for control OUT requests
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
Reviewed-by: Laurent Pinchart
---
No change from v4
No change from v3
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c | 3 +++
drivers/usb/gadget/function/uvc.h | 1 +
include/uapi/linux/usb/g_uvc.h | 3 ++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 36de51ac30ed..58ed191f636e 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -215,6 +215,8 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
+ memcpy(&uvc_event->data.setup, &uvc->control_setup,
+ sizeof(uvc_event->data.setup));
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -238,6 +240,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
uvc->event_length = le16_to_cpu(ctrl->wLength);
+ memcpy(&uvc->control_setup, ctrl, sizeof(uvc->control_setup));
if (uvc->event_setup_out) {
struct usb_request *req = uvc->control_req;
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 671020c8a836..1d89b1df4ba0 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -163,6 +163,7 @@ struct uvc_device {
unsigned int control_intf;
struct usb_ep *control_ep;
struct usb_request *control_req;
+ struct usb_ctrlrequest control_setup;
void *control_buf;
unsigned int streaming_intf;
diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h
index 6698c3263ae8..10fbb4382925 100644
--- a/include/uapi/linux/usb/g_uvc.h
+++ b/include/uapi/linux/usb/g_uvc.h
@@ -24,7 +24,8 @@
struct uvc_request_data {
__s32 length;
- __u8 data[60];
+ struct usb_ctrlrequest setup;
+ __u8 data[52];
};
struct uvc_event {
--
2.20.1
[PATCH v2] usb: gadget: musb: fix short isoc packets with inventra dma
Handling short packets (length < max packet size) in the Inventra DMA
engine in the MUSB driver causes the MUSB DMA controller to hang. An
example of a problem that is caused by this problem is when streaming
video out of a UVC gadget, only the first video frame is transferred.
For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
set manually by the driver. This was previously done in musb_g_tx
(musb_gadget.c), but incorrectly (all csr flags were cleared, and only
MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY were set). Fixing that problem
allows some requests to be transferred correctly, but multiple requests
were often put together in one USB packet, and caused problems if the
packet size was not a multiple of 4. Instead, set MUSB_TXCSR_TXPKTRDY
in dma_controller_irq (musbhsdma.c), just like host mode transfers.
This topic was originally tackled by Nicolas Boichat [0] [1] and is
discussed further at [2] as part of his GSoC project [3].
[0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
[1]
https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
[2]
http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
[3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
Signed-off-by: Paul Elder
---
Changes in v2:
- no more flushing FIFO
- greatly simplified short packet if guard in musb_g_tx, and removed
unnecessary variables
- minor indentation and wording changes
drivers/usb/musb/musb_gadget.c | 19 +--
drivers/usb/musb/musbhsdma.c | 21 +++--
2 files changed, 16 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index eae8b1b1b45b..496643f54faa 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -452,13 +452,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
}
if (request) {
- u8 is_dma = 0;
- boolshort_packet = false;
trace_musb_req_tx(req);
if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
- is_dma = 1;
csr |= MUSB_TXCSR_P_WZC_BITS;
csr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_P_UNDERRUN |
MUSB_TXCSR_TXPKTRDY | MUSB_TXCSR_AUTOSET);
@@ -476,16 +473,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
*/
if ((request->zero && request->length)
&& (request->length % musb_ep->packet_sz == 0)
- && (request->actual == request->length))
- short_packet = true;
+ && (request->actual == request->length)) {
- if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
- (is_dma && (!dma->desired_mode ||
- (request->actual &
- (musb_ep->packet_sz - 1)
- short_packet = true;
-
- if (short_packet) {
/*
* On DMA completion, FIFO may not be
* available yet...
@@ -493,8 +482,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
if (csr & MUSB_TXCSR_TXPKTRDY)
return;
- musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE
- | MUSB_TXCSR_TXPKTRDY);
+ musb_dbg(musb, "sending short pkt (zero=%d, length=%d,
actual=%d, dma->desired_mode=%d)\n",
+request->zero, request->length,
request->actual,
+dma->desired_mode);
+ musb_writew(epio, MUSB_TXCSR, csr |
MUSB_TXCSR_TXPKTRDY);
request->zero = 0;
}
diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index a688f7f87829..5fc6825745f2 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void
*private_data)
channel->status = MUSB_DMA_STATUS_FREE;
/* completed */
- if ((devctl & MUSB_DEVCTL_HM)
- && (musb_channel->transmit)
- && ((channel->desired_mode == 0)
- || (channel->actual_len &
- (musb_channel-&
[PATCH v5 2/6] usb: gadget: uvc: enqueue usb request in setup handler for control OUT
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
Reviewed-by: Laurent Pinchart
---
No change from v4
No change from v3
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c| 25 +++--
drivers/usb/gadget/function/uvc_v4l2.c | 7 +++
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 723ca91e4f2a..36de51ac30ed 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -223,8 +223,6 @@ static int
uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
{
struct uvc_device *uvc = to_uvc(f);
- struct v4l2_event v4l2_event;
- struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
uvcg_info(f, "invalid request type\n");
@@ -241,10 +239,25 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
uvc->event_length = le16_to_cpu(ctrl->wLength);
- memset(&v4l2_event, 0, sizeof(v4l2_event));
- v4l2_event.type = UVC_EVENT_SETUP;
- memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
- v4l2_event_queue(&uvc->vdev, &v4l2_event);
+ if (uvc->event_setup_out) {
+ struct usb_request *req = uvc->control_req;
+
+ /*
+* Enqueue the request immediately for control OUT as the
+* host will start the data stage straight away.
+*/
+ req->length = uvc->event_length;
+ req->zero = 0;
+ usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
+ } else {
+ struct v4l2_event v4l2_event;
+ struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
+
+ memset(&v4l2_event, 0, sizeof(v4l2_event));
+ v4l2_event.type = UVC_EVENT_SETUP;
+ memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
+ v4l2_event_queue(&uvc->vdev, &v4l2_event);
+ }
return 0;
}
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index 7816ea9886e1..ac48f49d9f10 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,6 +35,13 @@ uvc_send_response(struct uvc_device *uvc, struct
uvc_request_data *data)
struct usb_composite_dev *cdev = uvc->func.config->cdev;
struct usb_request *req = uvc->control_req;
+ /*
+* For control OUT transfers the request has been enqueued synchronously
+* by the setup handler, there's nothing to be done here.
+*/
+ if (uvc->event_setup_out)
+ return 0;
+
if (data->length < 0)
return usb_ep_set_halt(cdev->gadget->ep0);
--
2.20.1
[PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage
A usb gadget function driver may or may not want to delay the status
stage of a control OUT request. An instance where it might want to is to
asynchronously validate the data of a class-specific request.
A function driver that wants an explicit status stage should set the
newly added explicit_status flag of the usb_request corresponding to the
data stage. Later on, the function driver can explicitly complete the
status stage by enqueueing a usb_request for ACK, or calling
usb_ep_set_halt() for STALL.
To support both explicit and implicit status stages, a UDC driver must
call the newly added usb_gadget_control_complete function right before
calling usb_gadget_giveback_request. To support the explicit status
stage, it might then check what stage the usb_request was queued in, and
for control IN ACK the host's zero-length data packet, or for control
OUT send a zero-length DATA1 ACK packet.
Signed-off-by: Paul Elder
v4 Acked-by: Alan Stern
v1 Reviewed-by: Laurent Pinchart
---
Changes from v4:
- Change parameter of usb_gadget_control_complete to simply take a
usb_request
- Make usb_gadget_control_complete do nothing if the request has no
length (ie. no data stage)
Changes from v3:
- More specific in commit message about what to do for udc driver acking
- Set explicit_status in usb_gadget_control_complete
- Make explicit_status type bool
Changes from v2:
Add status parameter to usb_gadget_control_complete, so that a
usb_request is not queued if the status of the just given back request
is nonzero.
Changes from v1:
Complete change of API. Now we use a flag that should be set in the
usb_request that is queued for the data stage to signal to the UDC that
we want to delay the status stage (as opposed to setting a flag in the
UDC itself, that persists across all requests). We now also provide a
function for UDC drivers to very easily allow implicit status stages, to
mitigate the need to convert all function drivers to this new API at
once, and to make it easier for UDC drivers to convert.
drivers/usb/gadget/udc/core.c | 34 ++
include/linux/usb/gadget.h| 10 ++
2 files changed, 44 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48c1cea..57b2c2550537 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -894,6 +894,40 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
/* - */
+/**
+ * usb_gadget_control_complete - complete the status stage of a control
+ * request, or delay it
+ * Context: in_interrupt()
+ *
+ * @gadget: gadget whose control request's status stage should be completed
+ * @request: usb request whose status stage should be completed
+ *
+ * This is called by device controller drivers before returning the completed
+ * request back to the gadget layer, to either complete or delay the status
+ * stage. It exits without doing anything if the request has a non-zero status,
+ * if it has zero length, or if its explicit_status flag is set.
+ */
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+ struct usb_request *request)
+{
+ struct usb_request *req;
+
+ if (request->explicit_status || request->status || !request->length)
+ return;
+
+ /* Send an implicit status-stage request for ep0 */
+ req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
+ if (req) {
+ req->length = 0;
+ req->explicit_status = 1;
+ req->complete = usb_ep_free_request;
+ usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
+ }
+}
+EXPORT_SYMBOL_GPL(usb_gadget_control_complete);
+
+/* - */
+
/**
* gadget_find_ep_by_name - returns ep whose name is the same as sting passed
* in second parameter or NULL if searched endpoint not found
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..bf4f021ce139 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -73,6 +73,7 @@ struct usb_ep;
* Note that for writes (IN transfers) some data bytes may still
* reside in a device-side FIFO when the request is reported as
* complete.
+ * @explicit_status: If true, delays the status stage
*
* These are allocated/freed through the endpoint they're used with. The
* hardware's driver can add extra per-request data to the memory it returns,
@@ -114,6 +115,8 @@ struct usb_request {
int status;
unsignedactual;
+
+ boolexplicit_status;
};
/*-*/
@@ -850,6 +853,13 @@ extern void u
[PATCH v5 1/6] usb: uvc: include videodev2.h in g_uvc.h
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 Reviewed-by: Laurent Pinchart --- No change from v4 No change from v3 No change from v2 No change from v1 include/uapi/linux/usb/g_uvc.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h index 3c9ee3020cbb..6698c3263ae8 100644 --- a/include/uapi/linux/usb/g_uvc.h +++ b/include/uapi/linux/usb/g_uvc.h @@ -11,6 +11,7 @@ #include #include #include +#include #define UVC_EVENT_FIRST(V4L2_EVENT_PRIVATE_START + 0) #define UVC_EVENT_CONNECT (V4L2_EVENT_PRIVATE_START + 0) -- 2.20.1
[PATCH v3 3/4] usb: gadget: uvc: disable stream when disconnected
If the streamon ioctl is issued while the stream is already on, then the
kernel BUGs. This happens at the BUG_ON in uvc_video_alloc_requests
within the call stack from the ioctl handler for VIDIOC_STREAMON.
This can also be triggered by starting the stream and then physically
disconnecting usb. To fix this, do the streamoff procedures on usb
disconnect. Since uvcg_video_enable is not interrupt-safe, add an
interrupt-safe version uvcg_video_cancel, and use that.
Signed-off-by: Paul Elder
v2 Reviewed-by: Kieran Bingham
---
Changes in v3:
- added interrupt-safe uvcg_video_cancel and used instead of the
non-interrupt-save uvcg_video_enable 0
Changes in v2: Nothing
drivers/usb/gadget/function/f_uvc.c | 3 +++
drivers/usb/gadget/function/uvc_video.c | 13 +
drivers/usb/gadget/function/uvc_video.h | 2 ++
3 files changed, 18 insertions(+)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index b407b10a95ed..4134117b5f81 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -369,9 +369,12 @@ uvc_function_disable(struct usb_function *f)
{
struct uvc_device *uvc = to_uvc(f);
struct v4l2_event v4l2_event;
+ struct uvc_video *video = &uvc->video;
uvcg_info(f, "%s()\n", __func__);
+ uvcg_video_cancel(video, 1);
+
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_DISCONNECT;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
diff --git a/drivers/usb/gadget/function/uvc_video.c
b/drivers/usb/gadget/function/uvc_video.c
index 5c042f380708..5f3ed9e0b5ad 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -348,6 +348,19 @@ int uvcg_video_pump(struct uvc_video *video)
return 0;
}
+int uvcg_video_cancel(struct uvc_video *video, int disconnect)
+{
+ unsigned int i;
+
+ for (i = 0; i < UVC_NUM_REQUESTS; ++i)
+ if (video->req[i])
+ usb_ep_dequeue(video->ep, video->req[i]);
+
+ uvc_video_free_requests(video);
+ uvcg_queue_cancel(&video->queue, disconnect);
+ return 0;
+}
+
/*
* Enable or disable the video stream.
*/
diff --git a/drivers/usb/gadget/function/uvc_video.h
b/drivers/usb/gadget/function/uvc_video.h
index 278dc52c7604..1f310fa58ce5 100644
--- a/drivers/usb/gadget/function/uvc_video.h
+++ b/drivers/usb/gadget/function/uvc_video.h
@@ -16,6 +16,8 @@ struct uvc_video;
int uvcg_video_pump(struct uvc_video *video);
+int uvcg_video_cancel(struct uvc_video *video, int disconnect);
+
int uvcg_video_enable(struct uvc_video *video, int enable);
int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc);
--
2.20.1
[PATCH v3 4/4] usb: gadget: uvc: remove unused/duplicate function prototypes from uvc.h
Defined in uvc.h, uvc_endpoint_stream doesn't exist, and
uvc_function_connect, uvc_function_disconnect, and
uvc_function_setup_continue have duplicates in f_uvc.h.
Remove these four function prototypes from uvc.h
Signed-off-by: Paul Elder
---
drivers/usb/gadget/function/uvc.h | 10 --
1 file changed, 10 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index f183e349499c..671020c8a836 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -185,14 +185,4 @@ struct uvc_file_handle {
#define to_uvc_file_handle(handle) \
container_of(handle, struct uvc_file_handle, vfh)
-/*
- * Functions
- */
-
-extern void uvc_function_setup_continue(struct uvc_device *uvc);
-extern void uvc_endpoint_stream(struct uvc_device *dev);
-
-extern void uvc_function_connect(struct uvc_device *uvc);
-extern void uvc_function_disconnect(struct uvc_device *uvc);
-
#endif /* _UVC_GADGET_H_ */
--
2.20.1
[PATCH v3 2/4] usb: gadget: uvc: don't delay the status phase of non-zero SET_INTERFACE requests
Reception of a SET_INTERFACE request with a non-zero alternate setting signals the start of the video stream. The gadget has to enable the video streaming endpoint and to signal stream start to userspace, in order to start receiving video frames to transmit over USB. As userspace can be slow to react, the UVC function driver delays the status phase of the SET_INTERFACE control transfer until userspace is ready. The status phase is delayed by returning USB_GADGET_DELAYED_STATUS from the function's .set_alt() handler. This creates a race condition as the userspace application could process the stream start event before the composite layer processes the USB_GADGET_DELAYED_STATUS return value. The race has been observed in practice, and can't be solved without a change to the USB_GADGET_DELAYED_STATUS API. Fortunately the UVC function driver doesn't strictly require delaying the status phase for non-zero SET_INTERFACE, as the only requirement from a USB point of view is that the streaming endpoint must be enabled before the status phase completes, and that is already guaranteed by the current code. We can thus complete the status phase synchronously, removing the race condition at the same time. Without delaying the status phase the host will likely start issuing isochronous transfers before we queue the first USB requests. The UDC will reply with NAKs which should be handled properly by the host. If this ends up causing issues another option will be to modify the status phase delay API to fix the race condition. Signed-off-by: Paul Elder --- Changes in v3: Nothing Changes in v2: 1. Remove delay usb status phase drivers/usb/gadget/function/f_uvc.c| 3 ++- drivers/usb/gadget/function/uvc_v4l2.c | 6 -- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 2ec3b73b2b75..b407b10a95ed 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -356,7 +356,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) memset(&v4l2_event, 0, sizeof(v4l2_event)); v4l2_event.type = UVC_EVENT_STREAMON; v4l2_event_queue(&uvc->vdev, &v4l2_event); - return USB_GADGET_DELAYED_STATUS; + + return 0; default: return -EINVAL; diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c index 97e624214287..7816ea9886e1 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -210,12 +210,6 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type) uvc->state = UVC_STATE_STREAMING; - /* -* Complete the alternate setting selection setup phase now that -* userspace is ready to provide video frames. -*/ - uvc_function_setup_continue(uvc); - return 0; } -- 2.20.1
[PATCH v3 0/4] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off
Down the call stack from the ioctl handler for VIDIOC_STREAMON, uvc_video_alloc_requests contains a BUG_ON, which in the high level, triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF being issued previously. This can happen in a few ways, such as if the userspace uvc gadget application simply doesn't issue VIDIOC_STREAMOFF. Another way is if uvc_function_set_alt with alt 0 is called after it is called with 1 but before VIDIOC_STREAMON is called; in this case, UVC_EVENT_STREAMOFF will not be queued to userspace, and therefore userspace will never call VIDIOC_STREAMOFF. To fix this, add two more uvc states: starting and stopping. The starting state is entered when uvc_function_set_alt 1 is called, and is exited in uvc_v4l2_streamon, when the state is changed to streaming. The stopping state is entered when uvc_function_set_alt 0 is called, and is exited in uvc_v4l2_streamoff, when the state is changed to connected. The status phase of the SET_INTERFACE request doesn't need to be delayed by the uvc gadget driver, so that is removed. Finally, there is another way to trigger the aforementioned BUG: start streaming and (physically) disconnect usb. To fix this, call uvcg_video_enable 0 in uvc_function_disable. Changes in v3: - add state guard to uvc_function_set_alt 1 - add documentation for newly added uvc states - reorder uvc states to more or less follow the flow diagram - add more state guards to ioctl handlers for streamon and streamoff - added interrupt-safe uvcg_video_cancel and used instead of the non-interrupt-save uvcg_video_enable 0 in uvc_function_disable Changes in v2: 1. Remove delay usb status phase Paul Elder (4): usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt usb: gadget: uvc: don't delay the status phase of non-zero SET_INTERFACE requests usb: gadget: uvc: disable stream when disconnected usb: gadget: uvc: remove unused/duplicate function prototypes from uvc.h drivers/usb/gadget/function/f_uvc.c | 23 drivers/usb/gadget/function/uvc.h | 47 +++-- drivers/usb/gadget/function/uvc_v4l2.c | 28 +++ drivers/usb/gadget/function/uvc_video.c | 13 +++ drivers/usb/gadget/function/uvc_video.h | 2 ++ 5 files changed, 91 insertions(+), 22 deletions(-) -- 2.20.1
[PATCH v3 1/4] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt
If the streamon ioctl is issued while the stream is already on, then the
kernel BUGs. This happens at the BUG_ON in uvc_video_alloc_requests
within the call stack from the ioctl handler for VIDIOC_STREAMON.
This could happen when uvc_function_set_alt 0 races and wins against
uvc_v4l2_streamon, or when userspace neglects to issue the
VIDIOC_STREAMOFF ioctl.
To fix this, add two more uvc states: starting and stopping. Use these
to prevent the racing, and to detect when VIDIOC_STREAMON is issued
without previously issuing VIDIOC_STREAMOFF.
Signed-off-by: Paul Elder
---
Changes in v3:
- add state guard to uvc_function_set_alt 1
- add documentation for newly added uvc states
- reorder uvc states to more or less follow the flow diagram
- add more state guards to ioctl handlers for streamon and streamoff
Changes in v2: Nothing
drivers/usb/gadget/function/f_uvc.c| 17
drivers/usb/gadget/function/uvc.h | 37 ++
drivers/usb/gadget/function/uvc_v4l2.c | 26 --
3 files changed, 73 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 8c99392df593..2ec3b73b2b75 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -317,26 +317,31 @@ uvc_function_set_alt(struct usb_function *f, unsigned
interface, unsigned alt)
switch (alt) {
case 0:
- if (uvc->state != UVC_STATE_STREAMING)
+ if (uvc->state != UVC_STATE_STREAMING &&
+ uvc->state != UVC_STATE_STARTING)
return 0;
if (uvc->video.ep)
usb_ep_disable(uvc->video.ep);
+ uvc->state = UVC_STATE_STOPPING;
+
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_STREAMOFF;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
- uvc->state = UVC_STATE_CONNECTED;
return 0;
case 1:
- if (uvc->state != UVC_STATE_CONNECTED)
- return 0;
-
if (!uvc->video.ep)
return -EINVAL;
+ if (uvc->state == UVC_STATE_STOPPING)
+ return -EINVAL;
+
+ if (uvc->state != UVC_STATE_CONNECTED)
+ return 0;
+
uvcg_info(f, "reset UVC\n");
usb_ep_disable(uvc->video.ep);
@@ -346,6 +351,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned
interface, unsigned alt)
return ret;
usb_ep_enable(uvc->video.ep);
+ uvc->state = UVC_STATE_STARTING;
+
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_STREAMON;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 099d650082e5..f183e349499c 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -101,10 +101,47 @@ struct uvc_video {
unsigned int fid;
};
+/**
+ * enum uvc_state - the states of a struct uvc_device
+ * @UVC_STATE_DISCONNECTED: not connected state
+ * - transition to connected state on .set_alt
+ * @UVC_STATE_CONNECTED:connected state
+ * - transition to disconnected state on .disable
+ * and alloc
+ * - transition to starting state on .set_alt 1
+ * @UVC_STATE_STARTING: starting state
+ * - transition to streaming state on streamon ioctl
+ * - transition to stopping state on set_alt 0
+ * @UVC_STATE_STREAMING:streaming state
+ * - transition to stopping state on .set_alt 0
+ * @UVC_STATE_STOPPING: stopping state
+ * - transition to connected on streamoff ioctl
+ *
+ * Diagram of state transitions:
+ *
+ * disable
+ * +---+
+ * v |
+ * +--+ set_alt +---+
+ * | DISCONNECTED | -> | CONNECTED |
+ * +--++---+
+ *| ^
+ * set_alt 1 | | streamoff
+ * +--+ +
+ * V|
+ * +--+ streamon +---+ set_alt 0 +--+
+ * | STARTING | --> | STREAMING | ---> | STOPPING |
+ * +--+ +---+ +--+
+ * |^
+ * | set_alt 0|
+ * +
Re: [PATCH v2] usb: gadget: musb: fix short isoc packets with inventra dma
Hi Bin,
On Wed, Jan 09, 2019 at 09:02:15AM -0600, Bin Liu wrote:
> Hi Paul,
>
> On Wed, Jan 09, 2019 at 02:10:09AM -0500, Paul Elder wrote:
> > Handling short packets (length < max packet size) in the Inventra DMA
> > engine in the MUSB driver causes the MUSB DMA controller to hang. An
> > example of a problem that is caused by this problem is when streaming
> > video out of a UVC gadget, only the first video frame is transferred.
> >
> > For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
> > set manually by the driver. This was previously done in musb_g_tx
> > (musb_gadget.c), but incorrectly (all csr flags were cleared, and only
> > MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY were set). Fixing that problem
> > allows some requests to be transferred correctly, but multiple requests
> > were often put together in one USB packet, and caused problems if the
> > packet size was not a multiple of 4. Instead, set MUSB_TXCSR_TXPKTRDY
> > in dma_controller_irq (musbhsdma.c), just like host mode transfers.
> >
> > This topic was originally tackled by Nicolas Boichat [0] [1] and is
> > discussed further at [2] as part of his GSoC project [3].
> >
> > [0]
> > https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
> > [1]
> > https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
> > [2]
> > http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
> > [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
> >
> > Signed-off-by: Paul Elder
> > ---
> > Changes in v2:
> >
> > - no more flushing FIFO
> > - greatly simplified short packet if guard in musb_g_tx, and removed
> > unnecessary variables
> > - minor indentation and wording changes
> >
> > drivers/usb/musb/musb_gadget.c | 19 +--
> > drivers/usb/musb/musbhsdma.c | 21 +++--
> > 2 files changed, 16 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > index eae8b1b1b45b..496643f54faa 100644
> > --- a/drivers/usb/musb/musb_gadget.c
> > +++ b/drivers/usb/musb/musb_gadget.c
> > @@ -452,13 +452,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > }
> >
> > if (request) {
> > - u8 is_dma = 0;
> > - boolshort_packet = false;
> >
> > trace_musb_req_tx(req);
> >
> > if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
> > - is_dma = 1;
> > csr |= MUSB_TXCSR_P_WZC_BITS;
> > csr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_P_UNDERRUN |
> > MUSB_TXCSR_TXPKTRDY | MUSB_TXCSR_AUTOSET);
> > @@ -476,16 +473,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > */
> > if ((request->zero && request->length)
> > && (request->length % musb_ep->packet_sz == 0)
> > - && (request->actual == request->length))
> > - short_packet = true;
> > + && (request->actual == request->length)) {
> >
> > - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
> > - (is_dma && (!dma->desired_mode ||
> > - (request->actual &
> > - (musb_ep->packet_sz - 1)
> > - short_packet = true;
> > -
> > - if (short_packet) {
> > /*
> > * On DMA completion, FIFO may not be
> > * available yet...
> > @@ -493,8 +482,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > if (csr & MUSB_TXCSR_TXPKTRDY)
> > return;
> >
> > - musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE
> > - | MUSB_TXCSR_TXPKTRDY);
> > + musb_dbg(musb, "sending short pkt (zero=%d, length=%d,
> > actual=%d, dma->desired_mode=%d)\n",
> > +request->zero, request->length,
> > request->actual,
> > +dma->desired_mode);
> > + musb_writew(epio, MUSB_TXCSR, csr |
> > MUSB_TXCSR_TXPKTRDY);
>
> Sorry I didn't catch this in the last review, but this change seems not
> required, isn't it? In the first version of the patch, the code is
> 'returned' in the 'if (musb_dma_inventra())' branch above, doesn't reach
> here.
Do you mean change compared to the last version of the patch, or this
last chunk of the diff?
I guess I did also remove the return when I removed the 'if
(musb_dma_inventra())' block that had the FLUSHFIFIO, but when I tested
it it still worked. In fact, I reverted this last diff chunk and it
still worked.
Paul
Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage
On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote: > On Wed, 9 Jan 2019, Paul Elder wrote: > > > A usb gadget function driver may or may not want to delay the status > > stage of a control OUT request. An instance where it might want to is to > > asynchronously validate the data of a class-specific request. > > > > A function driver that wants an explicit status stage should set the > > newly added explicit_status flag of the usb_request corresponding to the > > data stage. Later on, the function driver can explicitly complete the > > status stage by enqueueing a usb_request for ACK, or calling > > usb_ep_set_halt() for STALL. > > > > To support both explicit and implicit status stages, a UDC driver must > > call the newly added usb_gadget_control_complete function right before > > calling usb_gadget_giveback_request. To support the explicit status > > stage, it might then check what stage the usb_request was queued in, and > > for control IN ACK the host's zero-length data packet, or for control > > OUT send a zero-length DATA1 ACK packet. > > > > Signed-off-by: Paul Elder > > v4 Acked-by: Alan Stern > > v1 Reviewed-by: Laurent Pinchart > > This looks good and has passed my tests so far. Good! Thank you :) > Can you check your uvc > changes using dummy_hcd with the patch below? I'm not sure what to make of the test results. I get the same results with or without the patch. Which I guess makes sense... in dummy_queue, this is getting hit when the uvc function driver tries to complete the delayed status: req = usb_request_to_dummy_request(_req); if (!_req || !list_empty(&req->queue) || !_req->complete) return -EINVAL; So the delayed/explicit status stage is never completed, afaict. Paul
Re: [PATCH v5 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request
On Thu, Jan 10, 2019 at 03:39:25PM -0500, Alan Stern wrote: > On Wed, 9 Jan 2019, Paul Elder wrote: > > > This patch series adds a mechanism to allow asynchronously validating > > the data stage of a control OUT request, and for stalling or suceeding > > the request accordingly. > > One thing we haven't mentioned explicitly: What should happen when the > time for the status stage rolls around if the gadget driver queues a > non-zero length request? Ah, yeah, I missed that. > This can happen in a few different ways. One obvious possibility is > that the gadget driver sets the explicit_status flag and then submits a > non-zero length request. Another is that the gadget driver submits > _two_ requests during the data stage (the second would be interpreted > as the status-stage request). A third is that the gadget driver > submits a data-stage request that is too long and the excess portion is > used for the status stage. > > My feeling is that the behavior in these cases should officially be > undefined. Almost anything could happen: the status stage could STALL, > it could succeed, it could NAK, or it could send a non-zero packet to > the host. The request could return with 0 status or an error status, > and req->actual could take on any reasonable value. > > Alternatively, the UDC driver could detect these errors and report them > somehow. Maybe STALL the status stage and complete the request with > -EPIPE status or some such thing. > > Any preferences or other ideas? I think error detection and reporting would be useful. The question is what action to take after that; either leave it undefined or STALL. I think STALL would be fine, since if a non-zero length request is submitted for a status stage, intentionally or not, it isn't part of proper behavior and should count as an error. > One other thing: Some UDC drivers may assume that the data stage of a > control transfer never spans more than a single usb_request. Should > this become an official requirement? Would the data stage of a control transfer ever need more space than a single usb_request can contain? I know UVC doesn't; that's why we pack it together with the setup stage data in 3/6. If so, I would think we can make it a requirement. Paul
Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage
On Fri, Jan 11, 2019 at 10:50:11AM -0500, Alan Stern wrote: > On Fri, 11 Jan 2019, Paul Elder wrote: > > > On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote: > > > On Wed, 9 Jan 2019, Paul Elder wrote: > > > > > > > A usb gadget function driver may or may not want to delay the status > > > > stage of a control OUT request. An instance where it might want to is to > > > > asynchronously validate the data of a class-specific request. > > > > > > > > A function driver that wants an explicit status stage should set the > > > > newly added explicit_status flag of the usb_request corresponding to the > > > > data stage. Later on, the function driver can explicitly complete the > > > > status stage by enqueueing a usb_request for ACK, or calling > > > > usb_ep_set_halt() for STALL. > > > > > > > > To support both explicit and implicit status stages, a UDC driver must > > > > call the newly added usb_gadget_control_complete function right before > > > > calling usb_gadget_giveback_request. To support the explicit status > > > > stage, it might then check what stage the usb_request was queued in, and > > > > for control IN ACK the host's zero-length data packet, or for control > > > > OUT send a zero-length DATA1 ACK packet. > > > > > > > > Signed-off-by: Paul Elder > > > > v4 Acked-by: Alan Stern > > > > v1 Reviewed-by: Laurent Pinchart > > > > > > This looks good and has passed my tests so far. > > > > Good! Thank you :) > > > > > Can you check your uvc > > > changes using dummy_hcd with the patch below? > > > > I'm not sure what to make of the test results. I get the same results > > with or without the patch. Which I guess makes sense... in dummy_queue, > > this is getting hit when the uvc function driver tries to complete the > > delayed status: > > > > req = usb_request_to_dummy_request(_req); > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > return -EINVAL; > > > > So the delayed/explicit status stage is never completed, afaict. > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > other two tests are trivial. Yes, that is what's happening. > Triggering the !list_empty() test means the request has already been > submitted and not yet completed. This probably indicates there is a > bug in the uvc function driver code. The uvc function driver works with musb, though :/ I compared the sequence of calls to the uvc setup, completion handler, and status stage sending, and for some reason dummy_hcd, after an OUT setup-completion-status sequence, calls a completion-status-completion sequence, and then goes on the the next request. musb simply goes on to the next request after the setup-completion-status sequence. I commented out the paranoia block in dummy_timer, and dummy_hcd still does the extra completion, but it doesn't error out anymore. I doubt that's the/a solution though, especially since I get: [ 22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26). [ 22.624481] uvcvideo: Failed to initialize the device (-5). Not sure if that's a result of dummy_hcd not supporting isochronous transfers or not. I'm not sure where to continue investigating :/ Thanks, Paul
[PATCH v3] usb: gadget: musb: fix short isoc packets with inventra dma
Handling short packets (length < max packet size) in the Inventra DMA
engine in the MUSB driver causes the MUSB DMA controller to hang. An
example of a problem that is caused by this problem is when streaming
video out of a UVC gadget, only the first video frame is transferred.
For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
set manually by the driver. This was previously done in musb_g_tx
(musb_gadget.c), but incorrectly (all csr flags were cleared, and only
MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY were set). Fixing that problem
allows some requests to be transferred correctly, but multiple requests
were often put together in one USB packet, and caused problems if the
packet size was not a multiple of 4. Instead, set MUSB_TXCSR_TXPKTRDY
in dma_controller_irq (musbhsdma.c), just like host mode transfers.
This topic was originally tackled by Nicolas Boichat [0] [1] and is
discussed further at [2] as part of his GSoC project [3].
[0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
[1]
https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
[2]
http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
[3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
Signed-off-by: Paul Elder
---
Changes in v3:
- remove unnecessary chunk for sending short packet
Changes in v2:
- no more flushing FIFO
- greatly simplified short packet if guard in musb_g_tx, and removed
unnecessary variables
- minor indentation and wording changes
drivers/usb/musb/musb_gadget.c | 13 +
drivers/usb/musb/musbhsdma.c | 21 +++--
2 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index eae8b1b1b45b..ffe462a657b1 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -452,13 +452,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
}
if (request) {
- u8 is_dma = 0;
- boolshort_packet = false;
trace_musb_req_tx(req);
if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
- is_dma = 1;
csr |= MUSB_TXCSR_P_WZC_BITS;
csr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_P_UNDERRUN |
MUSB_TXCSR_TXPKTRDY | MUSB_TXCSR_AUTOSET);
@@ -476,16 +473,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
*/
if ((request->zero && request->length)
&& (request->length % musb_ep->packet_sz == 0)
- && (request->actual == request->length))
- short_packet = true;
+ && (request->actual == request->length)) {
- if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
- (is_dma && (!dma->desired_mode ||
- (request->actual &
- (musb_ep->packet_sz - 1)
- short_packet = true;
-
- if (short_packet) {
/*
* On DMA completion, FIFO may not be
* available yet...
diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index a688f7f87829..5fc6825745f2 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void
*private_data)
channel->status = MUSB_DMA_STATUS_FREE;
/* completed */
- if ((devctl & MUSB_DEVCTL_HM)
- && (musb_channel->transmit)
- && ((channel->desired_mode == 0)
- || (channel->actual_len &
- (musb_channel->max_packet_sz - 1)))
- ) {
+ if (musb_channel->transmit &&
+ (!channel->desired_mode ||
+ (channel->actual_len %
+ musb_channel->max_packet_sz))) {
u8 epnum = musb_channel->epnum;
int offset = musb->io.ep_offset(epnum,
MUSB_TXCSR);
@@ -363,11 +361,14 @@ static irqreturn_t dma_controller_irq(int irq, void
*priv
Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage
On Mon, Jan 14, 2019 at 10:24:44AM -0500, Alan Stern wrote: > On Mon, 14 Jan 2019, Paul Elder wrote: > > > > > > Can you check your uvc > > > > > changes using dummy_hcd with the patch below? > > > > > > > > I'm not sure what to make of the test results. I get the same results > > > > with or without the patch. Which I guess makes sense... in dummy_queue, > > > > this is getting hit when the uvc function driver tries to complete the > > > > delayed status: > > > > > > > > req = usb_request_to_dummy_request(_req); > > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > > return -EINVAL; > > > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > > > other two tests are trivial. > > > > Yes, that is what's happening. > > > > > Triggering the !list_empty() test means the request has already been > > > submitted and not yet completed. This probably indicates there is a > > > bug in the uvc function driver code. > > > > The uvc function driver works with musb, though :/ > > > > I compared the sequence of calls to the uvc setup, completion handler, > > and status stage sending, and for some reason dummy_hcd, after an OUT > > setup-completion-status sequence, calls a completion-status-completion > > sequence, and then goes on the the next request. musb simply goes on to > > the next request after the setup-completion-status sequence. > > I don't quite understand. There's a control-OUT transfer, the setup, > data, and status transactions all complete normally, and then what > happens? What do you mean by "a completion-status-completion > sequence"? A more detailed description would help. > I meant the functions (procedures) in the function driver, so the setup handler (uvc_function_setup), the completion handler (uvc_function_ep0_complete), and the status sender (uvc_send_response), although the last one actually sends the data stage for control IN. So after the status is sent on the uvc gadget driver's end, its completion handler is called again without the setup handler being called beforehand and I cant figure out why. > > I commented out the paranoia block in dummy_timer, and dummy_hcd still > > does the extra completion, but it doesn't error out anymore. I doubt > > that's the/a solution though, especially since I get: > > > > [ 22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 > > (exp. 26). > > [ 22.624481] uvcvideo: Failed to initialize the device (-5). > > > > Not sure if that's a result of dummy_hcd not supporting isochronous > > transfers or not. > > > > I'm not sure where to continue investigating :/ > > Perhaps removing the "#if 0" protecting the dev_dbg line in > dummy_queue() would provide some helpful output. It did, but didn't get me much farther :/ > Another thing to check would be if the "implement an emulated > single-request FIFO" in dummy_queue() is causing trouble. There's no > harm in replacing the long "if" condition with "if (0)". That didn't change anything. Although I did notice that the dummy_queue that calls the completion handler without the preceeding setup handler says that it's in the status stage (ep->status_stage == 1). Thanks, Paul
Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage
On Wed, Jan 16, 2019 at 10:06:53AM -0500, Alan Stern wrote: > On Wed, 16 Jan 2019, Paul Elder wrote: > > > On Mon, Jan 14, 2019 at 10:24:44AM -0500, Alan Stern wrote: > > > On Mon, 14 Jan 2019, Paul Elder wrote: > > > > > > > > > > Can you check your uvc > > > > > > > changes using dummy_hcd with the patch below? > > > > > > > > > > > > I'm not sure what to make of the test results. I get the same > > > > > > results > > > > > > with or without the patch. Which I guess makes sense... in > > > > > > dummy_queue, > > > > > > this is getting hit when the uvc function driver tries to complete > > > > > > the > > > > > > delayed status: > > > > > > > > > > > > req = usb_request_to_dummy_request(_req); > > > > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > > > > return -EINVAL; > > > > > > > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > > > > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? > > > > > The > > > > > other two tests are trivial. > > > > > > > > Yes, that is what's happening. > > > > > > > > > Triggering the !list_empty() test means the request has already been > > > > > submitted and not yet completed. This probably indicates there is a > > > > > bug in the uvc function driver code. > > > > > > > > The uvc function driver works with musb, though :/ > > > > > > > > I compared the sequence of calls to the uvc setup, completion handler, > > > > and status stage sending, and for some reason dummy_hcd, after an OUT > > > > setup-completion-status sequence, calls a completion-status-completion > > > > sequence, and then goes on the the next request. musb simply goes on to > > > > the next request after the setup-completion-status sequence. > > > > > > I don't quite understand. There's a control-OUT transfer, the setup, > > > data, and status transactions all complete normally, and then what > > > happens? What do you mean by "a completion-status-completion > > > sequence"? A more detailed description would help. > > > > > > > I meant the functions (procedures) in the function driver, so the setup > > handler (uvc_function_setup), the completion handler > > (uvc_function_ep0_complete), and the status sender (uvc_send_response), > > although the last one actually sends the data stage for control IN. > > So after the status is sent on the uvc gadget driver's end, its > > completion handler is called again without the setup handler being > > called beforehand and I cant figure out why. > > Isn't this what you should expect? Every usb_request, if it is queued > successfully, eventually gets a completion callback. That promise is > made by every UDC driver; it's part of the gadget API. So for a > control transfer with a data stage, you expect to have: > > Setup handler called > Data-stage request submitted > Data-stage request completion callback > Status-stage request submitted > Status-stage request completion callback > > Thus, two completion callbacks but only one setup callback. omg how did I not notice this :/ I guess I have to fix the uvc function driver so it works with that. musb doesn't call the status stage completion callback though; not that it does anything so it seems fine to me, but indeed the function driver has to be ready for it if it is called. > > > > I commented out the paranoia block in dummy_timer, and dummy_hcd still > > > > does the extra completion, but it doesn't error out anymore. I doubt > > > > that's the/a solution though, especially since I get: > > > > > > > > [ 22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 > > > > (exp. 26). > > > > [ 22.624481] uvcvideo: Failed to initialize the device (-5). > > > > > > > > Not sure if that's a result of dummy_hcd not supporting isochronous > > > > transfers or not. > > > > > > > > I'm not sure where to continue investigating :/ > > > > > > Perhaps removing the "#if 0" protecting the dev_dbg line in > > > dummy_queue() would provide some helpful output. > > > > It did, but didn't get me much farther :/ > > > > > Another thing to check would be if the "implement an emulated > > > single-request FIFO" in dummy_queue() is causing trouble. There's no > > > harm in replacing the long "if" condition with "if (0)". > > > > That didn't change anything. > > > > Although I did notice that the dummy_queue that calls the completion > > handler without the preceeding setup handler says that it's in the > > status stage (ep->status_stage == 1). > > That is consistent with the events outlined above. Thanks, Paul
Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage
On Fri, Jan 18, 2019 at 11:52:57AM -0500, Alan Stern wrote: > On Fri, 18 Jan 2019, Paul Elder wrote: > > > > > I meant the functions (procedures) in the function driver, so the setup > > > > handler (uvc_function_setup), the completion handler > > > > (uvc_function_ep0_complete), and the status sender (uvc_send_response), > > > > although the last one actually sends the data stage for control IN. > > > > So after the status is sent on the uvc gadget driver's end, its > > > > completion handler is called again without the setup handler being > > > > called beforehand and I cant figure out why. > > > > > > Isn't this what you should expect? Every usb_request, if it is queued > > > successfully, eventually gets a completion callback. That promise is > > > made by every UDC driver; it's part of the gadget API. So for a > > > control transfer with a data stage, you expect to have: > > > > > > Setup handler called > > > Data-stage request submitted > > > Data-stage request completion callback > > > Status-stage request submitted > > > Status-stage request completion callback > > > > > > Thus, two completion callbacks but only one setup callback. > > > > omg how did I not notice this :/ > > > > I guess I have to fix the uvc function driver so it works with that. > > musb doesn't call the status stage completion callback though; not that > > it does anything so it seems fine to me, but indeed the function driver > > has to be ready for it if it is called. > > musb _has_ to call the status-stage completion callback. As just one > reason, if the explicit_status flag isn't set then that callback is > responsible for deallocating the status request. Without it, the > status request will leak. Ah, I see what you mean. I forgot about that because we reuse the request in uvc gadget. I'll have to add the status stage completion callback to musb too, then. Thanks, Paul
[PATCH v6 4/6] usb: gadget: add mechanism to specify an explicit status stage
A usb gadget function driver may or may not want to delay the status
stage of a control OUT request. An instance where it might want to is to
asynchronously validate the data of a class-specific request.
A function driver that wants an explicit status stage should set the
newly added explicit_status flag of the usb_request corresponding to the
data stage. Later on, the function driver can explicitly complete the
status stage by enqueueing a usb_request for ACK, or calling
usb_ep_set_halt() for STALL.
To support both explicit and implicit status stages, a UDC driver must
call the newly added usb_gadget_control_complete function right before
calling usb_gadget_giveback_request. To support the explicit status
stage, it might then check what stage the usb_request was queued in, and
for control IN ACK the host's zero-length data packet, or for control
OUT send a zero-length DATA1 ACK packet.
Signed-off-by: Paul Elder
v4 Acked-by: Alan Stern
v1 Reviewed-by: Laurent Pinchart
---
No change from v5
Changes from v4:
- Change parameter of usb_gadget_control_complete to simply take a
usb_request
- Make usb_gadget_control_complete do nothing if the request has no
length (ie. no data stage)
Changes from v3:
- More specific in commit message about what to do for udc driver acking
- Set explicit_status in usb_gadget_control_complete
- Make explicit_status type bool
Changes from v2:
Add status parameter to usb_gadget_control_complete, so that a
usb_request is not queued if the status of the just given back request
is nonzero.
Changes from v1:
Complete change of API. Now we use a flag that should be set in the
usb_request that is queued for the data stage to signal to the UDC that
we want to delay the status stage (as opposed to setting a flag in the
UDC itself, that persists across all requests). We now also provide a
function for UDC drivers to very easily allow implicit status stages, to
mitigate the need to convert all function drivers to this new API at
once, and to make it easier for UDC drivers to convert.
drivers/usb/gadget/udc/core.c | 34 ++
include/linux/usb/gadget.h| 10 ++
2 files changed, 44 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48c1cea..57b2c2550537 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -894,6 +894,40 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
/* - */
+/**
+ * usb_gadget_control_complete - complete the status stage of a control
+ * request, or delay it
+ * Context: in_interrupt()
+ *
+ * @gadget: gadget whose control request's status stage should be completed
+ * @request: usb request whose status stage should be completed
+ *
+ * This is called by device controller drivers before returning the completed
+ * request back to the gadget layer, to either complete or delay the status
+ * stage. It exits without doing anything if the request has a non-zero status,
+ * if it has zero length, or if its explicit_status flag is set.
+ */
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+ struct usb_request *request)
+{
+ struct usb_request *req;
+
+ if (request->explicit_status || request->status || !request->length)
+ return;
+
+ /* Send an implicit status-stage request for ep0 */
+ req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
+ if (req) {
+ req->length = 0;
+ req->explicit_status = 1;
+ req->complete = usb_ep_free_request;
+ usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
+ }
+}
+EXPORT_SYMBOL_GPL(usb_gadget_control_complete);
+
+/* - */
+
/**
* gadget_find_ep_by_name - returns ep whose name is the same as sting passed
* in second parameter or NULL if searched endpoint not found
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..bf4f021ce139 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -73,6 +73,7 @@ struct usb_ep;
* Note that for writes (IN transfers) some data bytes may still
* reside in a device-side FIFO when the request is reported as
* complete.
+ * @explicit_status: If true, delays the status stage
*
* These are allocated/freed through the endpoint they're used with. The
* hardware's driver can add extra per-request data to the memory it returns,
@@ -114,6 +115,8 @@ struct usb_request {
int status;
unsignedactual;
+
+ boolexplicit_status;
};
/*-*/
@@ -850,6 +853,13 @@ extern void u
[PATCH v6 6/6] usb: gadget: uvc: allow ioctl to send response in status stage
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. After telling the UDC to delay
the status stage, the ioctl UVCIOC_SEND_RESPONSE should be called to
notify the UDC driver to reply with STALL or ACK, for control OUT
requests. In the case of a control IN request, the ioctl sends the UVC
data as before.
This means that the completion handler will also be called for the
status stage, so make the UVC gadget driver aware of if the
completion handler is called for the status stage, and do nothing (as
opposed to giving userspace the UVC data again).
Signed-off-by: Paul Elder
---
Changes from v5:
- add event_status flag and use to keep track of whether or not the
gadget is in the status stage or not
- do nothing if the completion handler is called during the status stage
No change from v4
No change from v3
Changes from v2:
- calling usb_ep_set_halt in uvc_send_response if data->length < 0 is
now common for both IN and OUT transfers so make that check common
- remove now unnecessary field setting for the usb_request to be queued
for the status stage
Changes from v1:
- remove usb_ep_delay_status call from the old proposed API
- changed portions of uvc_send_response to match v2 API
- remove UDC warning that send_response is not implemented
drivers/usb/gadget/function/f_uvc.c| 11 +--
drivers/usb/gadget/function/uvc.h | 1 +
drivers/usb/gadget/function/uvc_v4l2.c | 24 ++--
3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 6303ed346af9..dd3a06e28435 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -208,15 +208,19 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
struct v4l2_event v4l2_event;
struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
- if (uvc->event_setup_out) {
- uvc->event_setup_out = 0;
+ if (uvc->event_status) {
+ uvc->event_status = 0;
+ return;
+ }
+ if (uvc->event_setup_out) {
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
memcpy(&uvc_event->data.setup, &uvc->control_setup,
sizeof(uvc_event->data.setup));
+
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -242,6 +246,8 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
uvc->event_length = le16_to_cpu(ctrl->wLength);
memcpy(&uvc->control_setup, ctrl, sizeof(uvc->control_setup));
+ uvc->event_status = 0;
+
if (uvc->event_setup_out) {
struct usb_request *req = uvc->control_req;
@@ -251,6 +257,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
req->length = uvc->event_length;
req->zero = 0;
+ req->explicit_status = 1;
usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
} else {
struct v4l2_event v4l2_event;
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 1d89b1df4ba0..5754548d94c5 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -171,6 +171,7 @@ struct uvc_device {
/* Events */
unsigned int event_length;
unsigned int event_setup_out : 1;
+ unsigned int event_status : 1;
};
static inline struct uvc_device *to_uvc(struct usb_function *f)
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index ac48f49d9f10..338811c612c5 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,15 +35,27 @@ uvc_send_response(struct uvc_device *uvc, struct
uvc_request_data *data)
struct usb_composite_dev *cdev = uvc->func.config->cdev;
struct usb_request *req = uvc->control_req;
+ if (data->length < 0)
+ return usb_ep_set_halt(cdev->gadget->ep0);
+
/*
* For control OUT transfers the request has been enqueued synchronously
-* by the setup handler, there's nothing to be done here.
+* by the setup handler, we just need to tell the UDC whether to ACK or
+* STALL the control transfer.
*/
- if (uvc->event_setup_out)
- return 0;
-
- if (data-&g
[PATCH v6 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request
This patch series adds a mechanism to allow asynchronously validating the data stage of a control OUT request, and for stalling or suceeding the request accordingly. This mechanism is implemented for MUSB, and is used by UVC. At the same time, UVC packages the setup stage and data stage data together to send to userspace to save on a pair of context switches per control out request. This patch series does change the userspace API. We however believe that it is justified because the current API is broken, and because it isn't being used (because it's broken). The current API is broken such that it is subject to race conditions that cause fatal errors with a high frequency. This is actually what motivated this patch series in the first place. In the current API, not only is there no way to asynchronously validate the data stage of a control OUT request, but an empty buffer is expected to be provided to hold the data stage data -- which is more likely than not to be late. There is even a warning in musb_g_ep0_queue: /* else for sequence #2 (OUT), caller provides a buffer * before the next packet arrives. deferred responses * (after SETUP is acked) are racey. */ This problem has never been reported in years, which is a sign that the API isn't used. Furthermore, the vendor kernels that we have seen using the UVC gadget driver (such as QC and Huawei) are heavily patched with local changes to the API. This corroborates the suspicion that the current mainline API is not being used. Additionally, this API isn't meant to be used by generic applications, but by a dedicated userspace helper. uvc-gadget is one such example, but it has bitrotten and isn't compatible with the current kernel API. The fact that nobody has submitted patches nor complained for a long time again shows that it isn't being used. The conclusion is that since the API hasn't been used for a long time, it is safe to fix it. Changes in v6: - MUSB giveback usb request to gadget driver after enqueueing for the status stage - Add event_status flag to uvc_device and use to keep track of if the gadget is in the status stage - If the uvc gadget is in the status stage and the completion handler is called, do nothing (as opposed to giving the uvc data back to userspace once more) Changes in v5: - Change parameter of usb_gadget_control_complete to simply take a usb_request - Make usb_gadget_control_complete do nothing if the request has no length (ie. no data stage) - musb: call usb_gadget_control_complete before usb_gadget_giveback_request - set musb ep0 state to statusin in ep0_send_ack - make sure to not double-write musb register in ep0_rxstate, since musb_g_ep0_giveback will take care of writing them Changes in v4: - Change wording and fix typo in patch 4/6 "usb: gadget: add mechanism to specify an explicit status stage" - Set explicit_status in usb_gadget_control_complete - Change explicit_status from unsigned int to bool Changes in v3: - Function driver send STALL status stage by simply calling usb_ep_set_halt, and ACK by enqueueing request - Fix signature of usb_gadget_control_complete to check the status of the request that was just given back. - Corresponding changes to musb and uvc gadget Changes in v2: Overhaul of status stage delay mechanism/API. Now if a function driver desires an explicit/delayed status stage, it specifies so in a flag in the usb_request that is queued for the data stage. The function driver later enqueues another usb_request for the status stage, also with the explicit_status flag set, and with the zero flag acting as the status. If a function driver does not desire an explicit status stage, then it can set (or ignore) the explicit_status flag in the usb_request that is queued for the data stage. To allow the optional explicit status stage, a UDC driver should call the newly added usb_gadget_control_complete right after usb_gadget_giveback_request, and in its queue function should check if the usb_request is for the status stage and if it has been requested to be explicit, and if so check the status that should be sent. (See 5/6 "usb: musb: gadget: implement optional explicit status stage" for an implementation for MUSB) Paul Elder (6): usb: uvc: include videodev2.h in g_uvc.h usb: gadget: uvc: enqueue usb request in setup handler for control OUT usb: gadget: uvc: package setup and data for control OUT requests usb: gadget: add mechanism to specify an explicit status stage usb: musb: gadget: implement optional explicit status stage usb: gadget: uvc: allow ioctl to send response in status stage drivers/usb/gadget/function/f_uvc.c| 39 -- drivers/usb/gadget/function/uvc.h | 2 ++ drivers/usb/gadget/function/uvc_v4l2.c | 19 + drivers/usb/gadget/udc/core.c | 34 ++ drivers/usb/musb/musb_gadget.c | 2 ++ drivers/usb/musb/musb_gadget_
[PATCH v6 1/6] usb: uvc: include videodev2.h in g_uvc.h
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 Reviewed-by: Laurent Pinchart --- No change from v5 No change from v4 No change from v3 No change from v2 No change from v1 include/uapi/linux/usb/g_uvc.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h index 3c9ee3020cbb..6698c3263ae8 100644 --- a/include/uapi/linux/usb/g_uvc.h +++ b/include/uapi/linux/usb/g_uvc.h @@ -11,6 +11,7 @@ #include #include #include +#include #define UVC_EVENT_FIRST(V4L2_EVENT_PRIVATE_START + 0) #define UVC_EVENT_CONNECT (V4L2_EVENT_PRIVATE_START + 0) -- 2.20.1
[PATCH v6 2/6] usb: gadget: uvc: enqueue usb request in setup handler for control OUT
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
Reviewed-by: Laurent Pinchart
---
No change from v5
No change from v4
No change from v3
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c| 25 +++--
drivers/usb/gadget/function/uvc_v4l2.c | 7 +++
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 4134117b5f81..f571623cc6e4 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -223,8 +223,6 @@ static int
uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
{
struct uvc_device *uvc = to_uvc(f);
- struct v4l2_event v4l2_event;
- struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
uvcg_info(f, "invalid request type\n");
@@ -241,10 +239,25 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
uvc->event_length = le16_to_cpu(ctrl->wLength);
- memset(&v4l2_event, 0, sizeof(v4l2_event));
- v4l2_event.type = UVC_EVENT_SETUP;
- memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
- v4l2_event_queue(&uvc->vdev, &v4l2_event);
+ if (uvc->event_setup_out) {
+ struct usb_request *req = uvc->control_req;
+
+ /*
+* Enqueue the request immediately for control OUT as the
+* host will start the data stage straight away.
+*/
+ req->length = uvc->event_length;
+ req->zero = 0;
+ usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
+ } else {
+ struct v4l2_event v4l2_event;
+ struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
+
+ memset(&v4l2_event, 0, sizeof(v4l2_event));
+ v4l2_event.type = UVC_EVENT_SETUP;
+ memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
+ v4l2_event_queue(&uvc->vdev, &v4l2_event);
+ }
return 0;
}
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index 7816ea9886e1..ac48f49d9f10 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,6 +35,13 @@ uvc_send_response(struct uvc_device *uvc, struct
uvc_request_data *data)
struct usb_composite_dev *cdev = uvc->func.config->cdev;
struct usb_request *req = uvc->control_req;
+ /*
+* For control OUT transfers the request has been enqueued synchronously
+* by the setup handler, there's nothing to be done here.
+*/
+ if (uvc->event_setup_out)
+ return 0;
+
if (data->length < 0)
return usb_ep_set_halt(cdev->gadget->ep0);
--
2.20.1
[PATCH v6 3/6] usb: gadget: uvc: package setup and data for control OUT requests
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
Reviewed-by: Laurent Pinchart
---
No change from v5
No change from v4
No change from v3
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c | 3 +++
drivers/usb/gadget/function/uvc.h | 1 +
include/uapi/linux/usb/g_uvc.h | 3 ++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index f571623cc6e4..6303ed346af9 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -215,6 +215,8 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
+ memcpy(&uvc_event->data.setup, &uvc->control_setup,
+ sizeof(uvc_event->data.setup));
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -238,6 +240,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
uvc->event_length = le16_to_cpu(ctrl->wLength);
+ memcpy(&uvc->control_setup, ctrl, sizeof(uvc->control_setup));
if (uvc->event_setup_out) {
struct usb_request *req = uvc->control_req;
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 671020c8a836..1d89b1df4ba0 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -163,6 +163,7 @@ struct uvc_device {
unsigned int control_intf;
struct usb_ep *control_ep;
struct usb_request *control_req;
+ struct usb_ctrlrequest control_setup;
void *control_buf;
unsigned int streaming_intf;
diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h
index 6698c3263ae8..10fbb4382925 100644
--- a/include/uapi/linux/usb/g_uvc.h
+++ b/include/uapi/linux/usb/g_uvc.h
@@ -24,7 +24,8 @@
struct uvc_request_data {
__s32 length;
- __u8 data[60];
+ struct usb_ctrlrequest setup;
+ __u8 data[52];
};
struct uvc_event {
--
2.20.1
[PATCH v6 5/6] usb: musb: gadget: implement optional explicit status stage
Implement the mechanism for optional explicit status stage for the MUSB
driver. This allows a function driver to specify what to reply for the
status stage. The functionality for an implicit status stage is
retained.
Signed-off-by: Paul Elder
v1 Reviewed-by: Laurent Pinchart
v1 Acked-by: Bin Liu
---
Changes from v5:
- giveback usb request to gadget driver after enqueueing for the status
stage
Changes from v4:
- call usb_gadget_control_complete before usb_gadget_giveback_request
- set musb ep0 state to statusin in ep0_send_ack
- make sure to not double-write musb register in ep0_rxstate, since
musb_g_ep0_giveback will take care of writing them
No change from v3
Changes from v2:
- update call to usb_gadget_control_complete to include status
- since sending STALL from the function driver is now done with
usb_ep_set_halt, there is no need for the internal ep0_send_response to
take a stall/ack parameter; remove the parameter and make the function
only send ack, and remove checking for the status reply in the
usb_request for the status stage
Changes from v1:
- obvious change to implement v2 mechanism laid out by 4/6 of this
series (send_response, and musb_g_ep0_send_response function has
been removed, call to usb_gadget_control_complete has been added)
- ep0_send_response's ack argument has been changed from stall
- last_packet flag in ep0_rxstate has been removed, since it is equal to
req != NULL
drivers/usb/musb/musb_gadget.c | 2 ++
drivers/usb/musb/musb_gadget_ep0.c | 32 --
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index ffe462a657b1..2a36bebf955d 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -144,6 +144,8 @@ __acquires(ep->musb->lock)
unmap_dma_buffer(req, musb);
trace_musb_req_gb(req);
+ if (req->ep->end_point.address == 0)
+ usb_gadget_control_complete(&musb->g, &req->request);
usb_gadget_giveback_request(&req->ep->end_point, &req->request);
spin_lock(&musb->lock);
ep->busy = busy;
diff --git a/drivers/usb/musb/musb_gadget_ep0.c
b/drivers/usb/musb/musb_gadget_ep0.c
index 91a5027b5c1f..63c84488d51b 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -458,6 +458,25 @@ __acquires(musb->lock)
return handled;
}
+static int ep0_send_ack(struct musb *musb)
+{
+ void __iomem *regs = musb->control_ep->regs;
+ u16 csr;
+
+ if (musb->ep0_state != MUSB_EP0_STAGE_RX &&
+ musb->ep0_state != MUSB_EP0_STAGE_STATUSIN)
+ return -EINVAL;
+
+ csr = MUSB_CSR0_P_DATAEND | MUSB_CSR0_P_SVDRXPKTRDY;
+
+ musb_ep_select(musb->mregs, 0);
+ musb_writew(regs, MUSB_CSR0, csr);
+
+ musb->ep0_state = MUSB_EP0_STAGE_STATUSIN;
+
+ return 0;
+}
+
/* we have an ep0out data packet
* Context: caller holds controller lock
*/
@@ -504,12 +523,15 @@ static void ep0_rxstate(struct musb *musb)
if (req) {
musb->ackpend = csr;
musb_g_ep0_giveback(musb, req);
+ if (req->explicit_status)
+ return;
if (!musb->ackpend)
return;
musb->ackpend = 0;
+ } else {
+ musb_ep_select(musb->mregs, 0);
+ musb_writew(regs, MUSB_CSR0, csr);
}
- musb_ep_select(musb->mregs, 0);
- musb_writew(regs, MUSB_CSR0, csr);
}
/*
@@ -937,6 +959,7 @@ musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r,
gfp_t gfp_flags)
case MUSB_EP0_STAGE_RX: /* control-OUT data */
case MUSB_EP0_STAGE_TX: /* control-IN data */
case MUSB_EP0_STAGE_ACKWAIT:/* zero-length data */
+ case MUSB_EP0_STAGE_STATUSIN:
status = 0;
break;
default:
@@ -978,6 +1001,11 @@ musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r,
gfp_t gfp_flags)
} else if (musb->ackpend) {
musb_writew(regs, MUSB_CSR0, musb->ackpend);
musb->ackpend = 0;
+
+ /* status stage of OUT with data, issue IN status, then giveback */
+ } else if (musb->ep0_state == MUSB_EP0_STAGE_STATUSIN) {
+ status = ep0_send_ack(musb);
+ musb_g_ep0_giveback(ep->musb, r);
}
cleanup:
--
2.20.1
Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage
On Wed, Jan 23, 2019 at 04:10:12PM -0500, Alan Stern wrote: > On Mon, 14 Jan 2019, Paul Elder wrote: > > > On Fri, Jan 11, 2019 at 10:50:11AM -0500, Alan Stern wrote: > > > On Fri, 11 Jan 2019, Paul Elder wrote: > > > > > > > On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote: > > > > > On Wed, 9 Jan 2019, Paul Elder wrote: > > > > > > > > > > > A usb gadget function driver may or may not want to delay the status > > > > > > stage of a control OUT request. An instance where it might want to > > > > > > is to > > > > > > asynchronously validate the data of a class-specific request. > > > > > > > > > > > > A function driver that wants an explicit status stage should set the > > > > > > newly added explicit_status flag of the usb_request corresponding > > > > > > to the > > > > > > data stage. Later on, the function driver can explicitly complete > > > > > > the > > > > > > status stage by enqueueing a usb_request for ACK, or calling > > > > > > usb_ep_set_halt() for STALL. > > > > > > > > > > > > To support both explicit and implicit status stages, a UDC driver > > > > > > must > > > > > > call the newly added usb_gadget_control_complete function right > > > > > > before > > > > > > calling usb_gadget_giveback_request. To support the explicit status > > > > > > stage, it might then check what stage the usb_request was queued > > > > > > in, and > > > > > > for control IN ACK the host's zero-length data packet, or for > > > > > > control > > > > > > OUT send a zero-length DATA1 ACK packet. > > > > > > > > > > > > Signed-off-by: Paul Elder > > > > > > v4 Acked-by: Alan Stern > > > > > > v1 Reviewed-by: Laurent Pinchart > > > > > > > > > > This looks good and has passed my tests so far. > > > > > > > > Good! Thank you :) > > > > > > > > > Can you check your uvc > > > > > changes using dummy_hcd with the patch below? > > > > > > > > I'm not sure what to make of the test results. I get the same results > > > > with or without the patch. Which I guess makes sense... in dummy_queue, > > > > this is getting hit when the uvc function driver tries to complete the > > > > delayed status: > > > > > > > > req = usb_request_to_dummy_request(_req); > > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > > return -EINVAL; > > > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > > > other two tests are trivial. > > > > Yes, that is what's happening. > > > > > Triggering the !list_empty() test means the request has already been > > > submitted and not yet completed. This probably indicates there is a > > > bug in the uvc function driver code. > > > > The uvc function driver works with musb, though :/ > > Did you ever figure out the reason for the "!list_empty(&req->queue)" > error with dummy_hcd? Was it related to the confusion about completion > callbacks for status requests? Yeah, I'm pretty sure that's what was happening. With what I previously had the uvc function driver wasn't expecting a completion callback for the status stage but the OUT request flag was set so it just kept sending the data stage data to userspace and userspace kept calling the ioctl to send the status stage which kept calling the completion callback and so on, until the dummy_hcd timer triggered and the next request came in. > Interesting new question: How does your code in musb tell the > difference between a 0-length data-stage reply to a control-IN > transfer, and a status-stage request? Both would appear to the UDC > driver as 0-length request submissions for ep0. > Do you explicitly keep track of whether the data stage is pending? musb has a state machine to keep track of which stage it's in, so I just count whatever is queued during the status-IN stage as a status-stage request for control OUT requests. Now that you mention it, I don't actually check that the queued request's length is zero there... gotta fix that. Paul
[PATCH v7 1/6] usb: uvc: include videodev2.h in g_uvc.h
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 Reviewed-by: Laurent Pinchart --- No change from v6 No change from v5 No change from v4 No change from v3 No change from v2 No change from v1 include/uapi/linux/usb/g_uvc.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h index 3c9ee3020cbb..6698c3263ae8 100644 --- a/include/uapi/linux/usb/g_uvc.h +++ b/include/uapi/linux/usb/g_uvc.h @@ -11,6 +11,7 @@ #include #include #include +#include #define UVC_EVENT_FIRST(V4L2_EVENT_PRIVATE_START + 0) #define UVC_EVENT_CONNECT (V4L2_EVENT_PRIVATE_START + 0) -- 2.20.1
[PATCH v7 3/6] usb: gadget: uvc: package setup and data for control OUT requests
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
Reviewed-by: Laurent Pinchart
---
No change from v6
No change from v5
No change from v4
No change from v3
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c | 3 +++
drivers/usb/gadget/function/uvc.h | 1 +
include/uapi/linux/usb/g_uvc.h | 3 ++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index f571623cc6e4..6303ed346af9 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -215,6 +215,8 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
+ memcpy(&uvc_event->data.setup, &uvc->control_setup,
+ sizeof(uvc_event->data.setup));
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -238,6 +240,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
uvc->event_length = le16_to_cpu(ctrl->wLength);
+ memcpy(&uvc->control_setup, ctrl, sizeof(uvc->control_setup));
if (uvc->event_setup_out) {
struct usb_request *req = uvc->control_req;
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 671020c8a836..1d89b1df4ba0 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -163,6 +163,7 @@ struct uvc_device {
unsigned int control_intf;
struct usb_ep *control_ep;
struct usb_request *control_req;
+ struct usb_ctrlrequest control_setup;
void *control_buf;
unsigned int streaming_intf;
diff --git a/include/uapi/linux/usb/g_uvc.h b/include/uapi/linux/usb/g_uvc.h
index 6698c3263ae8..10fbb4382925 100644
--- a/include/uapi/linux/usb/g_uvc.h
+++ b/include/uapi/linux/usb/g_uvc.h
@@ -24,7 +24,8 @@
struct uvc_request_data {
__s32 length;
- __u8 data[60];
+ struct usb_ctrlrequest setup;
+ __u8 data[52];
};
struct uvc_event {
--
2.20.1
[PATCH v7 2/6] usb: gadget: uvc: enqueue usb request in setup handler for control OUT
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
Reviewed-by: Laurent Pinchart
---
No change from v6
No change from v5
No change from v4
No change from v3
No change from v2
No change from v1
drivers/usb/gadget/function/f_uvc.c| 25 +++--
drivers/usb/gadget/function/uvc_v4l2.c | 7 +++
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 4134117b5f81..f571623cc6e4 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -223,8 +223,6 @@ static int
uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
{
struct uvc_device *uvc = to_uvc(f);
- struct v4l2_event v4l2_event;
- struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
uvcg_info(f, "invalid request type\n");
@@ -241,10 +239,25 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
uvc->event_length = le16_to_cpu(ctrl->wLength);
- memset(&v4l2_event, 0, sizeof(v4l2_event));
- v4l2_event.type = UVC_EVENT_SETUP;
- memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
- v4l2_event_queue(&uvc->vdev, &v4l2_event);
+ if (uvc->event_setup_out) {
+ struct usb_request *req = uvc->control_req;
+
+ /*
+* Enqueue the request immediately for control OUT as the
+* host will start the data stage straight away.
+*/
+ req->length = uvc->event_length;
+ req->zero = 0;
+ usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
+ } else {
+ struct v4l2_event v4l2_event;
+ struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
+
+ memset(&v4l2_event, 0, sizeof(v4l2_event));
+ v4l2_event.type = UVC_EVENT_SETUP;
+ memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
+ v4l2_event_queue(&uvc->vdev, &v4l2_event);
+ }
return 0;
}
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index 7816ea9886e1..ac48f49d9f10 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,6 +35,13 @@ uvc_send_response(struct uvc_device *uvc, struct
uvc_request_data *data)
struct usb_composite_dev *cdev = uvc->func.config->cdev;
struct usb_request *req = uvc->control_req;
+ /*
+* For control OUT transfers the request has been enqueued synchronously
+* by the setup handler, there's nothing to be done here.
+*/
+ if (uvc->event_setup_out)
+ return 0;
+
if (data->length < 0)
return usb_ep_set_halt(cdev->gadget->ep0);
--
2.20.1
[PATCH v7 0/6] usb: gadget: add mechanism to asynchronously validate data stage of ctrl OUT request
This patch series adds a mechanism to allow asynchronously validating the data stage of a control OUT request, and for stalling or suceeding the request accordingly. This mechanism is implemented for MUSB, and is used by UVC. At the same time, UVC packages the setup stage and data stage data together to send to userspace to save on a pair of context switches per control out request. This patch series does change the userspace API. We however believe that it is justified because the current API is broken, and because it isn't being used (because it's broken). The current API is broken such that it is subject to race conditions that cause fatal errors with a high frequency. This is actually what motivated this patch series in the first place. In the current API, not only is there no way to asynchronously validate the data stage of a control OUT request, but an empty buffer is expected to be provided to hold the data stage data -- which is more likely than not to be late. There is even a warning in musb_g_ep0_queue: /* else for sequence #2 (OUT), caller provides a buffer * before the next packet arrives. deferred responses * (after SETUP is acked) are racey. */ This problem has never been reported in years, which is a sign that the API isn't used. Furthermore, the vendor kernels that we have seen using the UVC gadget driver (such as QC and Huawei) are heavily patched with local changes to the API. This corroborates the suspicion that the current mainline API is not being used. Additionally, this API isn't meant to be used by generic applications, but by a dedicated userspace helper. uvc-gadget is one such example, but it has bitrotten and isn't compatible with the current kernel API. The fact that nobody has submitted patches nor complained for a long time again shows that it isn't being used. The conclusion is that since the API hasn't been used for a long time, it is safe to fix it. Changes in v7: - MUSB check that the request queued for the status stage of a control OUT request has zero length Changes in v6: - MUSB giveback usb request to gadget driver after enqueueing for the status stage - Add event_status flag to uvc_device and use to keep track of if the gadget is in the status stage - If the uvc gadget is in the status stage and the completion handler is called, do nothing (as opposed to giving the uvc data back to userspace once more) Changes in v5: - Change parameter of usb_gadget_control_complete to simply take a usb_request - Make usb_gadget_control_complete do nothing if the request has no length (ie. no data stage) - musb: call usb_gadget_control_complete before usb_gadget_giveback_request - set musb ep0 state to statusin in ep0_send_ack - make sure to not double-write musb register in ep0_rxstate, since musb_g_ep0_giveback will take care of writing them Changes in v4: - Change wording and fix typo in patch 4/6 "usb: gadget: add mechanism to specify an explicit status stage" - Set explicit_status in usb_gadget_control_complete - Change explicit_status from unsigned int to bool Changes in v3: - Function driver send STALL status stage by simply calling usb_ep_set_halt, and ACK by enqueueing request - Fix signature of usb_gadget_control_complete to check the status of the request that was just given back. - Corresponding changes to musb and uvc gadget Changes in v2: Overhaul of status stage delay mechanism/API. Now if a function driver desires an explicit/delayed status stage, it specifies so in a flag in the usb_request that is queued for the data stage. The function driver later enqueues another usb_request for the status stage, also with the explicit_status flag set, and with the zero flag acting as the status. If a function driver does not desire an explicit status stage, then it can set (or ignore) the explicit_status flag in the usb_request that is queued for the data stage. To allow the optional explicit status stage, a UDC driver should call the newly added usb_gadget_control_complete right after usb_gadget_giveback_request, and in its queue function should check if the usb_request is for the status stage and if it has been requested to be explicit, and if so check the status that should be sent. (See 5/6 "usb: musb: gadget: implement optional explicit status stage" for an implementation for MUSB) Paul Elder (6): usb: uvc: include videodev2.h in g_uvc.h usb: gadget: uvc: enqueue usb request in setup handler for control OUT usb: gadget: uvc: package setup and data for control OUT requests usb: gadget: add mechanism to specify an explicit status stage usb: musb: gadget: implement optional explicit status stage usb: gadget: uvc: allow ioctl to send response in status stage drivers/usb/gadget/function/f_uvc.c| 39 -- drivers/usb/gadget/function/uvc.h | 2 ++ drivers/usb/gadget/function/uvc_v4l2.c | 19 + drivers/usb/gadget/udc/core.c
[PATCH v7 5/6] usb: musb: gadget: implement optional explicit status stage
Implement the mechanism for optional explicit status stage for the MUSB
driver. This allows a function driver to specify what to reply for the
status stage. The functionality for an implicit status stage is
retained.
Signed-off-by: Paul Elder
v1 Reviewed-by: Laurent Pinchart
v1 Acked-by: Bin Liu
---
Changes from v6:
- check that the request queued for the status stage of a control
OUT request has zero length
Changes from v5:
- giveback usb request to gadget driver after enqueueing for the status
stage
Changes from v4:
- call usb_gadget_control_complete before usb_gadget_giveback_request
- set musb ep0 state to statusin in ep0_send_ack
- make sure to not double-write musb register in ep0_rxstate, since
musb_g_ep0_giveback will take care of writing them
No change from v3
Changes from v2:
- update call to usb_gadget_control_complete to include status
- since sending STALL from the function driver is now done with
usb_ep_set_halt, there is no need for the internal ep0_send_response to
take a stall/ack parameter; remove the parameter and make the function
only send ack, and remove checking for the status reply in the
usb_request for the status stage
Changes from v1:
- obvious change to implement v2 mechanism laid out by 4/6 of this
series (send_response, and musb_g_ep0_send_response function has
been removed, call to usb_gadget_control_complete has been added)
- ep0_send_response's ack argument has been changed from stall
- last_packet flag in ep0_rxstate has been removed, since it is equal to
req != NULL
drivers/usb/musb/musb_gadget.c | 2 ++
drivers/usb/musb/musb_gadget_ep0.c | 36 --
2 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index ffe462a657b1..2a36bebf955d 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -144,6 +144,8 @@ __acquires(ep->musb->lock)
unmap_dma_buffer(req, musb);
trace_musb_req_gb(req);
+ if (req->ep->end_point.address == 0)
+ usb_gadget_control_complete(&musb->g, &req->request);
usb_gadget_giveback_request(&req->ep->end_point, &req->request);
spin_lock(&musb->lock);
ep->busy = busy;
diff --git a/drivers/usb/musb/musb_gadget_ep0.c
b/drivers/usb/musb/musb_gadget_ep0.c
index 91a5027b5c1f..e6531ebe789e 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -458,6 +458,25 @@ __acquires(musb->lock)
return handled;
}
+static int ep0_send_ack(struct musb *musb)
+{
+ void __iomem *regs = musb->control_ep->regs;
+ u16 csr;
+
+ if (musb->ep0_state != MUSB_EP0_STAGE_RX &&
+ musb->ep0_state != MUSB_EP0_STAGE_STATUSIN)
+ return -EINVAL;
+
+ csr = MUSB_CSR0_P_DATAEND | MUSB_CSR0_P_SVDRXPKTRDY;
+
+ musb_ep_select(musb->mregs, 0);
+ musb_writew(regs, MUSB_CSR0, csr);
+
+ musb->ep0_state = MUSB_EP0_STAGE_STATUSIN;
+
+ return 0;
+}
+
/* we have an ep0out data packet
* Context: caller holds controller lock
*/
@@ -504,12 +523,15 @@ static void ep0_rxstate(struct musb *musb)
if (req) {
musb->ackpend = csr;
musb_g_ep0_giveback(musb, req);
+ if (req->explicit_status)
+ return;
if (!musb->ackpend)
return;
musb->ackpend = 0;
+ } else {
+ musb_ep_select(musb->mregs, 0);
+ musb_writew(regs, MUSB_CSR0, csr);
}
- musb_ep_select(musb->mregs, 0);
- musb_writew(regs, MUSB_CSR0, csr);
}
/*
@@ -937,6 +959,7 @@ musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r,
gfp_t gfp_flags)
case MUSB_EP0_STAGE_RX: /* control-OUT data */
case MUSB_EP0_STAGE_TX: /* control-IN data */
case MUSB_EP0_STAGE_ACKWAIT:/* zero-length data */
+ case MUSB_EP0_STAGE_STATUSIN:
status = 0;
break;
default:
@@ -978,6 +1001,15 @@ musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r,
gfp_t gfp_flags)
} else if (musb->ackpend) {
musb_writew(regs, MUSB_CSR0, musb->ackpend);
musb->ackpend = 0;
+
+ /* status stage of OUT with data, issue IN status, then giveback */
+ } else if (musb->ep0_state == MUSB_EP0_STAGE_STATUSIN) {
+ if (req->request.length)
+ status = -EINVAL;
+ else {
+ status = ep0_send_ack(musb);
+ musb_g_ep0_giveback(ep->musb, r);
+ }
}
cleanup:
--
2.20.1
[PATCH v7 6/6] usb: gadget: uvc: allow ioctl to send response in status stage
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. After telling the UDC to delay
the status stage, the ioctl UVCIOC_SEND_RESPONSE should be called to
notify the UDC driver to reply with STALL or ACK, for control OUT
requests. In the case of a control IN request, the ioctl sends the UVC
data as before.
This means that the completion handler will also be called for the
status stage, so make the UVC gadget driver aware of if the
completion handler is called for the status stage, and do nothing (as
opposed to giving userspace the UVC data again).
Signed-off-by: Paul Elder
---
No change from v6
Changes from v5:
- add event_status flag and use to keep track of whether or not the
gadget is in the status stage or not
- do nothing if the completion handler is called during the status stage
No change from v4
No change from v3
Changes from v2:
- calling usb_ep_set_halt in uvc_send_response if data->length < 0 is
now common for both IN and OUT transfers so make that check common
- remove now unnecessary field setting for the usb_request to be queued
for the status stage
Changes from v1:
- remove usb_ep_delay_status call from the old proposed API
- changed portions of uvc_send_response to match v2 API
- remove UDC warning that send_response is not implemented
drivers/usb/gadget/function/f_uvc.c| 11 +--
drivers/usb/gadget/function/uvc.h | 1 +
drivers/usb/gadget/function/uvc_v4l2.c | 24 ++--
3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 6303ed346af9..dd3a06e28435 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -208,15 +208,19 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct
usb_request *req)
struct v4l2_event v4l2_event;
struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
- if (uvc->event_setup_out) {
- uvc->event_setup_out = 0;
+ if (uvc->event_status) {
+ uvc->event_status = 0;
+ return;
+ }
+ if (uvc->event_setup_out) {
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_DATA;
uvc_event->data.length = req->actual;
memcpy(&uvc_event->data.data, req->buf, req->actual);
memcpy(&uvc_event->data.setup, &uvc->control_setup,
sizeof(uvc_event->data.setup));
+
v4l2_event_queue(&uvc->vdev, &v4l2_event);
}
}
@@ -242,6 +246,8 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
uvc->event_length = le16_to_cpu(ctrl->wLength);
memcpy(&uvc->control_setup, ctrl, sizeof(uvc->control_setup));
+ uvc->event_status = 0;
+
if (uvc->event_setup_out) {
struct usb_request *req = uvc->control_req;
@@ -251,6 +257,7 @@ uvc_function_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
*/
req->length = uvc->event_length;
req->zero = 0;
+ req->explicit_status = 1;
usb_ep_queue(f->config->cdev->gadget->ep0, req, GFP_KERNEL);
} else {
struct v4l2_event v4l2_event;
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 1d89b1df4ba0..5754548d94c5 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -171,6 +171,7 @@ struct uvc_device {
/* Events */
unsigned int event_length;
unsigned int event_setup_out : 1;
+ unsigned int event_status : 1;
};
static inline struct uvc_device *to_uvc(struct usb_function *f)
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
b/drivers/usb/gadget/function/uvc_v4l2.c
index ac48f49d9f10..338811c612c5 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -35,15 +35,27 @@ uvc_send_response(struct uvc_device *uvc, struct
uvc_request_data *data)
struct usb_composite_dev *cdev = uvc->func.config->cdev;
struct usb_request *req = uvc->control_req;
+ if (data->length < 0)
+ return usb_ep_set_halt(cdev->gadget->ep0);
+
/*
* For control OUT transfers the request has been enqueued synchronously
-* by the setup handler, there's nothing to be done here.
+* by the setup handler, we just need to tell the UDC whether to ACK or
+* STALL the control transfer.
*/
- if (uvc->event_setup_
[PATCH v7 4/6] usb: gadget: add mechanism to specify an explicit status stage
A usb gadget function driver may or may not want to delay the status
stage of a control OUT request. An instance where it might want to is to
asynchronously validate the data of a class-specific request.
A function driver that wants an explicit status stage should set the
newly added explicit_status flag of the usb_request corresponding to the
data stage. Later on, the function driver can explicitly complete the
status stage by enqueueing a usb_request for ACK, or calling
usb_ep_set_halt() for STALL.
To support both explicit and implicit status stages, a UDC driver must
call the newly added usb_gadget_control_complete function right before
calling usb_gadget_giveback_request. To support the explicit status
stage, it might then check what stage the usb_request was queued in, and
for control IN ACK the host's zero-length data packet, or for control
OUT send a zero-length DATA1 ACK packet.
Signed-off-by: Paul Elder
v4 Acked-by: Alan Stern
v1 Reviewed-by: Laurent Pinchart
---
No change from v6
No change from v5
Changes from v4:
- Change parameter of usb_gadget_control_complete to simply take a
usb_request
- Make usb_gadget_control_complete do nothing if the request has no
length (ie. no data stage)
Changes from v3:
- More specific in commit message about what to do for udc driver acking
- Set explicit_status in usb_gadget_control_complete
- Make explicit_status type bool
Changes from v2:
Add status parameter to usb_gadget_control_complete, so that a
usb_request is not queued if the status of the just given back request
is nonzero.
Changes from v1:
Complete change of API. Now we use a flag that should be set in the
usb_request that is queued for the data stage to signal to the UDC that
we want to delay the status stage (as opposed to setting a flag in the
UDC itself, that persists across all requests). We now also provide a
function for UDC drivers to very easily allow implicit status stages, to
mitigate the need to convert all function drivers to this new API at
once, and to make it easier for UDC drivers to convert.
drivers/usb/gadget/udc/core.c | 34 ++
include/linux/usb/gadget.h| 10 ++
2 files changed, 44 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48c1cea..57b2c2550537 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -894,6 +894,40 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
/* - */
+/**
+ * usb_gadget_control_complete - complete the status stage of a control
+ * request, or delay it
+ * Context: in_interrupt()
+ *
+ * @gadget: gadget whose control request's status stage should be completed
+ * @request: usb request whose status stage should be completed
+ *
+ * This is called by device controller drivers before returning the completed
+ * request back to the gadget layer, to either complete or delay the status
+ * stage. It exits without doing anything if the request has a non-zero status,
+ * if it has zero length, or if its explicit_status flag is set.
+ */
+void usb_gadget_control_complete(struct usb_gadget *gadget,
+ struct usb_request *request)
+{
+ struct usb_request *req;
+
+ if (request->explicit_status || request->status || !request->length)
+ return;
+
+ /* Send an implicit status-stage request for ep0 */
+ req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
+ if (req) {
+ req->length = 0;
+ req->explicit_status = 1;
+ req->complete = usb_ep_free_request;
+ usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
+ }
+}
+EXPORT_SYMBOL_GPL(usb_gadget_control_complete);
+
+/* - */
+
/**
* gadget_find_ep_by_name - returns ep whose name is the same as sting passed
* in second parameter or NULL if searched endpoint not found
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..bf4f021ce139 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -73,6 +73,7 @@ struct usb_ep;
* Note that for writes (IN transfers) some data bytes may still
* reside in a device-side FIFO when the request is reported as
* complete.
+ * @explicit_status: If true, delays the status stage
*
* These are allocated/freed through the endpoint they're used with. The
* hardware's driver can add extra per-request data to the memory it returns,
@@ -114,6 +115,8 @@ struct usb_request {
int status;
unsignedactual;
+
+ boolexplicit_status;
};
/*-*/
@@ -850,6 +853,13 @@ extern void u
