diff mbox series

[v2] usb: gadget: uvc: fix multiple opens

Message ID 20201110082504.26134-1-thomas.haemmerle@wolfvision.net
State New
Headers show
Series [v2] usb: gadget: uvc: fix multiple opens | expand

Commit Message

Thomas Haemmerle Nov. 10, 2020, 8:25 a.m. UTC 
From: Thomas Haemmerle <thomas.haemmerle@wolfvision.net>

Currently, the UVC function is activated when open on the corresponding
v4l2 device is called.
On another open the activation of the function fails since the
deactivation counter in `usb_function_activate` equals 0. However the
error is not returned to userspace since the open of the v4l2 device is
successful.

On a close the function is deactivated (since deactivation counter still
equals 0) and the video is disabled in `uvc_v4l2_release`, although
another process potentially is streaming.

Move activation of UVC function to subscription on UVC_EVENT_SETUP and
keep track of the number of subscribers (limited to 1) because there we
can guarantee for a userspace program utilizing UVC.
Extend the `struct uvc_file_handle` with member `bool connected` to tag 
it for a deactivation of the function.

With this a process is able to check capabilities of the v4l2 device
without deactivating the function for another process actually using the
device for UVC streaming.

Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net>
---
v2:
 - fix deadlock in `uvc_v4l2_unsubscribe_event()` (mutex is already
   locked in v4l2-core) introduced in v1
 - lock mutex in `uvc_v4l2_release()` to suppress ioctls and protect
   connected

 drivers/usb/gadget/function/uvc.h      |  2 +
 drivers/usb/gadget/function/uvc_v4l2.c | 56 +++++++++++++++++++++-----
 2 files changed, 48 insertions(+), 10 deletions(-)

Comments

Greg Kroah-Hartman Nov. 10, 2020, 8:40 a.m. UTC | #1 
On Tue, Nov 10, 2020 at 09:25:04AM +0100, thomas.haemmerle@wolfvision.net wrote:
> From: Thomas Haemmerle <thomas.haemmerle@wolfvision.net>
> 
> Currently, the UVC function is activated when open on the corresponding
> v4l2 device is called.
> On another open the activation of the function fails since the
> deactivation counter in `usb_function_activate` equals 0. However the
> error is not returned to userspace since the open of the v4l2 device is
> successful.
> 
> On a close the function is deactivated (since deactivation counter still
> equals 0) and the video is disabled in `uvc_v4l2_release`, although
> another process potentially is streaming.
> 
> Move activation of UVC function to subscription on UVC_EVENT_SETUP and
> keep track of the number of subscribers (limited to 1) because there we
> can guarantee for a userspace program utilizing UVC.
> Extend the `struct uvc_file_handle` with member `bool connected` to tag 
> it for a deactivation of the function.
> 
> With this a process is able to check capabilities of the v4l2 device
> without deactivating the function for another process actually using the
> device for UVC streaming.
> 
> Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net>
> ---
> v2:
>  - fix deadlock in `uvc_v4l2_unsubscribe_event()` (mutex is already
>    locked in v4l2-core) introduced in v1
>  - lock mutex in `uvc_v4l2_release()` to suppress ioctls and protect
>    connected
> 
>  drivers/usb/gadget/function/uvc.h      |  2 +
>  drivers/usb/gadget/function/uvc_v4l2.c | 56 +++++++++++++++++++++-----
>  2 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 73da4f9a8d4c..0d0bcbffc8fd 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -117,6 +117,7 @@ struct uvc_device {
>  	enum uvc_state state;
>  	struct usb_function func;
>  	struct uvc_video video;
> +	unsigned int connections;
>  
>  	/* Descriptors */
>  	struct {
> @@ -147,6 +148,7 @@ static inline struct uvc_device *to_uvc(struct usb_function *f)
>  struct uvc_file_handle {
>  	struct v4l2_fh vfh;
>  	struct uvc_video *device;
> +	bool connected;

What protects these two fields you are adding?

>  };
>  
>  #define to_uvc_file_handle(handle) \
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 67922b1355e6..aee4888e17b1 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -228,17 +228,57 @@ static int
>  uvc_v4l2_subscribe_event(struct v4l2_fh *fh,
>  			 const struct v4l2_event_subscription *sub)
>  {
> +	struct uvc_device *uvc = video_get_drvdata(fh->vdev);
> +	struct uvc_file_handle *handle = to_uvc_file_handle(fh);
> +	int ret;
> +
>  	if (sub->type < UVC_EVENT_FIRST || sub->type > UVC_EVENT_LAST)
>  		return -EINVAL;
>  
> -	return v4l2_event_subscribe(fh, sub, 2, NULL);
> +	if ((sub->type == UVC_EVENT_SETUP) && (uvc->connections >= 1))
> +		return -EBUSY;

Are you sure you can't handle more than one connection?

If so, why is it an integer and not just a boolean?

And what prevents the value from changing right after you test it here?

> +
> +	ret = v4l2_event_subscribe(fh, sub, 2, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (sub->type == UVC_EVENT_SETUP) {
> +		uvc->connections++;
> +		handle->connected = true;
> +		uvc_function_connect(uvc);
> +	}
> +
> +	return 0;
> +}
> +
> +static void uvc_v4l2_disable(struct uvc_device *uvc)
> +{
> +	if (--uvc->connections)
> +		return;
> +

What prevents "connections" from changing right after testing it here?

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 73da4f9a8d4c..0d0bcbffc8fd 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -117,6 +117,7 @@  struct uvc_device {
 	enum uvc_state state;
 	struct usb_function func;
 	struct uvc_video video;
+	unsigned int connections;
 
 	/* Descriptors */
 	struct {
@@ -147,6 +148,7 @@  static inline struct uvc_device *to_uvc(struct usb_function *f)
 struct uvc_file_handle {
 	struct v4l2_fh vfh;
 	struct uvc_video *device;
+	bool connected;
 };
 
 #define to_uvc_file_handle(handle) \
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 67922b1355e6..aee4888e17b1 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -228,17 +228,57 @@  static int
 uvc_v4l2_subscribe_event(struct v4l2_fh *fh,
 			 const struct v4l2_event_subscription *sub)
 {
+	struct uvc_device *uvc = video_get_drvdata(fh->vdev);
+	struct uvc_file_handle *handle = to_uvc_file_handle(fh);
+	int ret;
+
 	if (sub->type < UVC_EVENT_FIRST || sub->type > UVC_EVENT_LAST)
 		return -EINVAL;
 
-	return v4l2_event_subscribe(fh, sub, 2, NULL);
+	if ((sub->type == UVC_EVENT_SETUP) && (uvc->connections >= 1))
+		return -EBUSY;
+
+	ret = v4l2_event_subscribe(fh, sub, 2, NULL);
+	if (ret < 0)
+		return ret;
+
+	if (sub->type == UVC_EVENT_SETUP) {
+		uvc->connections++;
+		handle->connected = true;
+		uvc_function_connect(uvc);
+	}
+
+	return 0;
+}
+
+static void uvc_v4l2_disable(struct uvc_device *uvc)
+{
+	if (--uvc->connections)
+		return;
+
+	uvc_function_disconnect(uvc);
+	uvcg_video_enable(&uvc->video, 0);
+	uvcg_free_buffers(&uvc->video.queue);
 }
 
 static int
 uvc_v4l2_unsubscribe_event(struct v4l2_fh *fh,
 			   const struct v4l2_event_subscription *sub)
 {
-	return v4l2_event_unsubscribe(fh, sub);
+	struct uvc_device *uvc = video_get_drvdata(fh->vdev);
+	struct uvc_file_handle *handle = to_uvc_file_handle(fh);
+	int ret;
+
+	ret = v4l2_event_unsubscribe(fh, sub);
+	if (ret < 0)
+		return ret;
+
+	if ((sub->type == UVC_EVENT_SETUP) && handle->connected) {
+		uvc_v4l2_disable(uvc);
+		handle->connected = false;
+	}
+
+	return 0;
 }
 
 static long
@@ -293,7 +333,6 @@  uvc_v4l2_open(struct file *file)
 	handle->device = &uvc->video;
 	file->private_data = &handle->vfh;
 
-	uvc_function_connect(uvc);
 	return 0;
 }
 
@@ -303,14 +342,11 @@  uvc_v4l2_release(struct file *file)
 	struct video_device *vdev = video_devdata(file);
 	struct uvc_device *uvc = video_get_drvdata(vdev);
 	struct uvc_file_handle *handle = to_uvc_file_handle(file->private_data);
-	struct uvc_video *video = handle->device;
-
-	uvc_function_disconnect(uvc);
 
-	mutex_lock(&video->mutex);
-	uvcg_video_enable(video, 0);
-	uvcg_free_buffers(&video->queue);
-	mutex_unlock(&video->mutex);
+	mutex_lock(&uvc->video.mutex);
+	if (handle->connected)
+		uvc_v4l2_disable(uvc);
+	mutex_unlock(&uvc->video.mutex);
 
 	file->private_data = NULL;
 	v4l2_fh_del(&handle->vfh);