* [v2,1/3] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt
@ 2018-04-24 20:59 Paul Elder
0 siblings, 0 replies; 2+ messages in thread
From: Paul Elder @ 2018-04-24 20:59 UTC (permalink / raw)
To: linux-usb; +Cc: Paul Elder, laurent.pinchart, balbi, gregkh, rogerq
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 <paul.elder@ideasonboard.com>
---
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [v2,1/3] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt
@ 2018-05-21 8:07 Laurent Pinchart
0 siblings, 0 replies; 2+ messages in thread
From: Laurent Pinchart @ 2018-05-21 8:07 UTC (permalink / raw)
To: Paul Elder; +Cc: linux-usb, balbi, gregkh, rogerq
Hi Paul,
Thank you for the patch.
On Tuesday, 24 April 2018 23:59:34 EEST 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 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 <paul.elder@ideasonboard.com>
> ---
> 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)
Indentation is weird here, uvc should be aligned on the two lines.
> 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,
Let's order the states as theyr should normally occur, STARTING should come
before STREAMING.
> };
>
> 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;
I would move this check after the next one, as the VIDIOC_STREAMON ioctl
should fail if the type isn't valid, even if we're already streaming.
Furthermore, shouldn't we silently ignore the UVC_STATE_STREAMING only ? For
other states, I think we should return an error, as starting the stream isn't
valid for instance when the state is UVC_STATE_DISCONNECTED.
> 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;
Same comment here, this should go after the next check.
While I think extending the state machine this way makes sense, I believe
you're introducing race conditions. The VIDIOC_STREAMON and VIDIOC_STREAMOFF
ioctls can be issued by userspace at any time, completely asynchronously to
the reception of SET_INTERFACE requests. You will need a lock to protect this.
I recommend trying to draw sequence diagrams to see how the different events
can race each other.
> 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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-05-21 8:07 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-21 8:07 [v2,1/3] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt Laurent Pinchart
-- strict thread matches above, loose matches on Subject: below --
2018-04-24 20:59 Paul Elder
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.