* [PATCH] usb: gadget: uvc: fix multiple opens @ 2020-11-05 10:31 thomas.haemmerle 2020-11-05 10:37 ` Greg KH 0 siblings, 1 reply; 32+ messages in thread From: thomas.haemmerle @ 2020-11-05 10:31 UTC (permalink / raw) To: laurent.pinchart; +Cc: balbi, linux-usb, m.tretter, Thomas Haemmerle 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 setup_subscriber` 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> --- drivers/usb/gadget/function/uvc.h | 2 + drivers/usb/gadget/function/uvc_v4l2.c | 57 +++++++++++++++++++++----- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 23ee25383c1f..deeec2b80786 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 4ca89eab6159..c0c2588b0efb 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -227,17 +227,60 @@ 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); + + mutex_lock(&uvc->video.mutex); + uvcg_video_enable(&uvc->video, 0); + uvcg_free_buffers(&uvc->video.queue); + mutex_unlock(&uvc->video.mutex); } 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 @@ -292,7 +335,6 @@ uvc_v4l2_open(struct file *file) handle->device = &uvc->video; file->private_data = &handle->vfh; - uvc_function_connect(uvc); return 0; } @@ -302,14 +344,9 @@ 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); + if (handle->connected) + uvc_v4l2_disable(uvc); file->private_data = NULL; v4l2_fh_del(&handle->vfh); -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH] usb: gadget: uvc: fix multiple opens 2020-11-05 10:31 [PATCH] usb: gadget: uvc: fix multiple opens thomas.haemmerle @ 2020-11-05 10:37 ` Greg KH 2020-11-09 8:59 ` [PATCH v2] " thomas.haemmerle 2020-11-10 8:25 ` thomas.haemmerle 0 siblings, 2 replies; 32+ messages in thread From: Greg KH @ 2020-11-05 10:37 UTC (permalink / raw) To: thomas.haemmerle; +Cc: laurent.pinchart, balbi, linux-usb, m.tretter On Thu, Nov 05, 2020 at 11:31:19AM +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 setup_subscriber` > 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> > --- > drivers/usb/gadget/function/uvc.h | 2 + > drivers/usb/gadget/function/uvc_v4l2.c | 57 +++++++++++++++++++++----- > 2 files changed, 49 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 23ee25383c1f..deeec2b80786 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 4ca89eab6159..c0c2588b0efb 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -227,17 +227,60 @@ 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); > + > + mutex_lock(&uvc->video.mutex); > + uvcg_video_enable(&uvc->video, 0); > + uvcg_free_buffers(&uvc->video.queue); > + mutex_unlock(&uvc->video.mutex); > } > > 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 > @@ -292,7 +335,6 @@ uvc_v4l2_open(struct file *file) > handle->device = &uvc->video; > file->private_data = &handle->vfh; > > - uvc_function_connect(uvc); > return 0; > } > > @@ -302,14 +344,9 @@ 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); > + if (handle->connected) > + uvc_v4l2_disable(uvc); What prevents connected from changing between the test and the next call? I think you need a lock somewhere, a simple integer isn't going to protect you from anything (hint, and neither will an atomic variable...) thanks, greg k-h ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-05 10:37 ` Greg KH @ 2020-11-09 8:59 ` thomas.haemmerle 2020-11-10 8:25 ` thomas.haemmerle 1 sibling, 0 replies; 32+ messages in thread From: thomas.haemmerle @ 2020-11-09 8:59 UTC (permalink / raw) To: laurent.pinchart; +Cc: balbi, linux-usb, m.tretter, Thomas Haemmerle 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; }; #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); -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-05 10:37 ` Greg KH 2020-11-09 8:59 ` [PATCH v2] " thomas.haemmerle @ 2020-11-10 8:25 ` thomas.haemmerle 2020-11-10 8:40 ` Greg KH 2020-11-10 10:31 ` [PATCH v2] " Hans Verkuil 1 sibling, 2 replies; 32+ messages in thread From: thomas.haemmerle @ 2020-11-10 8:25 UTC (permalink / raw) To: gregkh Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media, Thomas Haemmerle 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; }; #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); -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 8:25 ` thomas.haemmerle @ 2020-11-10 8:40 ` Greg KH 2020-11-10 9:56 ` Thomas Hämmerle 2020-11-10 10:31 ` [PATCH v2] " Hans Verkuil 1 sibling, 1 reply; 32+ messages in thread From: Greg KH @ 2020-11-10 8:40 UTC (permalink / raw) To: thomas.haemmerle Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 8:40 ` Greg KH @ 2020-11-10 9:56 ` Thomas Hämmerle 2020-11-10 10:06 ` Greg KH 0 siblings, 1 reply; 32+ messages in thread From: Thomas Hämmerle @ 2020-11-10 9:56 UTC (permalink / raw) To: Greg KH; +Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media On 10.11.20 09:40, Greg KH wrote: > 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? The mutex in `struct uvc_video`. The lock in video_device is set to it in `uvc_register_video()` in f_uvc.c. > >> }; >> >> #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? It is possible to handle more than one subscription but I can't think of a scenario where this would make sense. For the reason that it is basically possible I decided for integer. > > And what prevents the value from changing right after you test it here? > The ioctl here are serialized, because the mutex pointer in the video device is set. >> + >> + 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? The function is either called from ioctl `uvc_v4l2_unsubscribe_event()` or from `uvc_v4l2_release()`. In both cases the video mutex is locked. Thomas ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 9:56 ` Thomas Hämmerle @ 2020-11-10 10:06 ` Greg KH 2020-11-10 14:30 ` [PATCH v3] " thomas.haemmerle 0 siblings, 1 reply; 32+ messages in thread From: Greg KH @ 2020-11-10 10:06 UTC (permalink / raw) To: Thomas Hämmerle Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media On Tue, Nov 10, 2020 at 10:56:05AM +0100, Thomas Hämmerle wrote: > On 10.11.20 09:40, Greg KH wrote: > > 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? > > The mutex in `struct uvc_video`. The lock in video_device is set to it in > `uvc_register_video()` in f_uvc.c. > > > > > > }; > > > #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? > > It is possible to handle more than one subscription but I can't think of a > scenario where this would make sense. > For the reason that it is basically possible I decided for integer. If it is not possible, just make it a bool so that we all can instantly see that this is the case. If it needs to be changed in the future, make that change then. thanks, greg k-h ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v3] usb: gadget: uvc: fix multiple opens 2020-11-10 10:06 ` Greg KH @ 2020-11-10 14:30 ` thomas.haemmerle 2020-11-13 13:36 ` Greg KH 2020-11-16 15:48 ` Laurent Pinchart 0 siblings, 2 replies; 32+ messages in thread From: thomas.haemmerle @ 2020-11-10 14:30 UTC (permalink / raw) To: gregkh Cc: laurent.pinchart, balbi, hverkuil, linux-usb, m.tretter, linux-media, Thomas Haemmerle 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 the UVC application potentially is streaming. Move activation of UVC function to subscription on UVC_EVENT_SETUP because there we can guarantee for a userspace application utilizing UVC. Block subscription on UVC_EVENT_SETUP while another application already is subscribed to it, indicated by `bool func_connected` in `struct uvc_device`. Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` to tag it as the handle used by the userspace UVC application. With this a process is able to check capabilities of the v4l2 device without deactivating the function for the actual UVC application. Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> --- v3: - replace `unsigned int connections` with `bool func_connected` - rename `bool connected` to `bool is_uvc_app_handle` 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 | 54 +++++++++++++++++++++----- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 73da4f9a8d4c..d6d0fd2dffa0 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; + bool func_connected; /* 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 is_uvc_app_handle; }; #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..3c0b7a969107 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -228,17 +228,55 @@ 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->func_connected) + return -EBUSY; + + ret = v4l2_event_subscribe(fh, sub, 2, NULL); + if (ret < 0) + return ret; + + if (sub->type == UVC_EVENT_SETUP) { + uvc->func_connected = true; + handle->is_uvc_app_handle = true; + uvc_function_connect(uvc); + } + + return 0; +} + +static void uvc_v4l2_disable(struct uvc_device *uvc) +{ + uvc->func_connected = false; + 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->is_uvc_app_handle) { + uvc_v4l2_disable(uvc); + handle->is_uvc_app_handle = false; + } + + return 0; } static long @@ -293,7 +331,6 @@ uvc_v4l2_open(struct file *file) handle->device = &uvc->video; file->private_data = &handle->vfh; - uvc_function_connect(uvc); return 0; } @@ -303,14 +340,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->is_uvc_app_handle) + uvc_v4l2_disable(uvc); + mutex_unlock(&uvc->video.mutex); file->private_data = NULL; v4l2_fh_del(&handle->vfh); -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v3] usb: gadget: uvc: fix multiple opens 2020-11-10 14:30 ` [PATCH v3] " thomas.haemmerle @ 2020-11-13 13:36 ` Greg KH 2020-11-16 15:48 ` Laurent Pinchart 1 sibling, 0 replies; 32+ messages in thread From: Greg KH @ 2020-11-13 13:36 UTC (permalink / raw) To: thomas.haemmerle Cc: laurent.pinchart, balbi, hverkuil, linux-usb, m.tretter, linux-media On Tue, Nov 10, 2020 at 03:30:15PM +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 the > UVC application potentially is streaming. > > Move activation of UVC function to subscription on UVC_EVENT_SETUP > because there we can guarantee for a userspace application utilizing UVC. > Block subscription on UVC_EVENT_SETUP while another application already > is subscribed to it, indicated by `bool func_connected` in > `struct uvc_device`. > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > to tag it as the handle used by the userspace UVC application. > > With this a process is able to check capabilities of the v4l2 device > without deactivating the function for the actual UVC application. > > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > --- > v3: > - replace `unsigned int connections` with `bool func_connected` > - rename `bool connected` to `bool is_uvc_app_handle` Can you fix this up based on Hans's review? thanks, greg k-h ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3] usb: gadget: uvc: fix multiple opens 2020-11-10 14:30 ` [PATCH v3] " thomas.haemmerle 2020-11-13 13:36 ` Greg KH @ 2020-11-16 15:48 ` Laurent Pinchart 2020-11-21 12:50 ` Thomas Hämmerle 1 sibling, 1 reply; 32+ messages in thread From: Laurent Pinchart @ 2020-11-16 15:48 UTC (permalink / raw) To: thomas.haemmerle Cc: gregkh, balbi, hverkuil, linux-usb, m.tretter, linux-media Hi Thomas, Thank you for the patch, and sorry for the late review. I was mostly absent last week. On Tue, Nov 10, 2020 at 03:30:15PM +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 the > UVC application potentially is streaming. > > Move activation of UVC function to subscription on UVC_EVENT_SETUP > because there we can guarantee for a userspace application utilizing UVC. > Block subscription on UVC_EVENT_SETUP while another application already > is subscribed to it, indicated by `bool func_connected` in > `struct uvc_device`. > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > to tag it as the handle used by the userspace UVC application. > > With this a process is able to check capabilities of the v4l2 device > without deactivating the function for the actual UVC application. > > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > --- > v3: > - replace `unsigned int connections` with `bool func_connected` > - rename `bool connected` to `bool is_uvc_app_handle` > > 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 | 54 +++++++++++++++++++++----- > 2 files changed, 46 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 73da4f9a8d4c..d6d0fd2dffa0 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; > + bool func_connected; > > /* 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 is_uvc_app_handle; > }; > > #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..3c0b7a969107 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -228,17 +228,55 @@ 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->func_connected) No need for the inner parentheses. > + return -EBUSY; > + > + ret = v4l2_event_subscribe(fh, sub, 2, NULL); > + if (ret < 0) > + return ret; > + > + if (sub->type == UVC_EVENT_SETUP) { > + uvc->func_connected = true; > + handle->is_uvc_app_handle = true; > + uvc_function_connect(uvc); > + } > + > + return 0; > +} > + > +static void uvc_v4l2_disable(struct uvc_device *uvc) > +{ > + uvc->func_connected = false; > + 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->is_uvc_app_handle) { No need for the inner parentheses. > + uvc_v4l2_disable(uvc); > + handle->is_uvc_app_handle = false; Calling uvc_v4l2_disable() here means that we'll stop everything when unsubscribing from the event, which sounds like it could cause issues as that behaviour is not expected. Wouldn't it be enough to only handle this in uvc_v4l2_release() ? > + } > + > + return 0; > } > > static long > @@ -293,7 +331,6 @@ uvc_v4l2_open(struct file *file) > handle->device = &uvc->video; > file->private_data = &handle->vfh; > > - uvc_function_connect(uvc); > return 0; > } > > @@ -303,14 +340,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); Could you please keep keep the local video variable, and use &video->mutex here ? The driver has a single video device at the moment, but could be extended in the future with support for multiple video devices in a single UVC device (lots of changes would be needed though). > + if (handle->is_uvc_app_handle) > + uvc_v4l2_disable(uvc); > + mutex_unlock(&uvc->video.mutex); Note that this lock isn't the same as the lock taken by __video_do_ioctl(), which alls uvc_v4l2_subscribe_event() and uvc_v4l2_unsubscribe_event(). I think Hans got confused in his review, it appears that there's nothing protecting concurrent access to is_uvc_app_handle and func_connected in v3. I think you need to take the driver-specific lock in uvc_v4l2_subscribe_event() and uvc_v4l2_unsubscribe_event(). > > file->private_data = NULL; > v4l2_fh_del(&handle->vfh); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3] usb: gadget: uvc: fix multiple opens 2020-11-16 15:48 ` Laurent Pinchart @ 2020-11-21 12:50 ` Thomas Hämmerle 2020-12-01 19:27 ` [PATCH v4] " Thomas Haemmerle 2021-10-04 23:55 ` [PATCH v3] " Laurent Pinchart 0 siblings, 2 replies; 32+ messages in thread From: Thomas Hämmerle @ 2020-11-21 12:50 UTC (permalink / raw) To: Laurent Pinchart Cc: gregkh, balbi, hverkuil, linux-usb, m.tretter, linux-media Hi Laurent, sorry for my late response! On 16.11.20 16:48, Laurent Pinchart wrote: > Hi Thomas, > > Thank you for the patch, and sorry for the late review. I was mostly > absent last week. > > On Tue, Nov 10, 2020 at 03:30:15PM +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 the >> UVC application potentially is streaming. >> >> Move activation of UVC function to subscription on UVC_EVENT_SETUP >> because there we can guarantee for a userspace application utilizing UVC. >> Block subscription on UVC_EVENT_SETUP while another application already >> is subscribed to it, indicated by `bool func_connected` in >> `struct uvc_device`. >> Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` >> to tag it as the handle used by the userspace UVC application. >> >> With this a process is able to check capabilities of the v4l2 device >> without deactivating the function for the actual UVC application. >> >> Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> >> --- >> v3: >> - replace `unsigned int connections` with `bool func_connected` >> - rename `bool connected` to `bool is_uvc_app_handle` >> >> 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 | 54 +++++++++++++++++++++----- >> 2 files changed, 46 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h >> index 73da4f9a8d4c..d6d0fd2dffa0 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; >> + bool func_connected; >> >> /* 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 is_uvc_app_handle; >> }; >> >> #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..3c0b7a969107 100644 >> --- a/drivers/usb/gadget/function/uvc_v4l2.c >> +++ b/drivers/usb/gadget/function/uvc_v4l2.c >> @@ -228,17 +228,55 @@ 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->func_connected) > > No need for the inner parentheses. I will change this. > >> + return -EBUSY; >> + >> + ret = v4l2_event_subscribe(fh, sub, 2, NULL); >> + if (ret < 0) >> + return ret; >> + >> + if (sub->type == UVC_EVENT_SETUP) { >> + uvc->func_connected = true; >> + handle->is_uvc_app_handle = true; >> + uvc_function_connect(uvc); >> + } >> + >> + return 0; >> +} >> + >> +static void uvc_v4l2_disable(struct uvc_device *uvc) >> +{ >> + uvc->func_connected = false; >> + 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->is_uvc_app_handle) { > > No need for the inner parentheses. I will change this. > >> + uvc_v4l2_disable(uvc); >> + handle->is_uvc_app_handle = false; > > Calling uvc_v4l2_disable() here means that we'll stop everything when > unsubscribing from the event, which sounds like it could cause issues as > that behaviour is not expected. Wouldn't it be enough to only handle > this in uvc_v4l2_release() ? > Of course it would be enough. But maybe a UVC gadget application wants to release the device for another application without closing it and since the function is activated on subscription the logical consequence is to deactivate it on unsubscription. >> + } >> + >> + return 0; >> } >> >> static long >> @@ -293,7 +331,6 @@ uvc_v4l2_open(struct file *file) >> handle->device = &uvc->video; >> file->private_data = &handle->vfh; >> >> - uvc_function_connect(uvc); >> return 0; >> } >> >> @@ -303,14 +340,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); > > Could you please keep keep the local video variable, and use > &video->mutex here ? The driver has a single video device at the moment, > but could be extended in the future with support for multiple video > devices in a single UVC device (lots of changes would be needed though). Yes. > >> + if (handle->is_uvc_app_handle) >> + uvc_v4l2_disable(uvc); >> + mutex_unlock(&uvc->video.mutex); > > Note that this lock isn't the same as the lock taken by > __video_do_ioctl(), which alls uvc_v4l2_subscribe_event() and > uvc_v4l2_unsubscribe_event(). I think Hans got confused in his review, > it appears that there's nothing protecting concurrent access to > is_uvc_app_handle and func_connected in v3. I think you need to take the > driver-specific lock in uvc_v4l2_subscribe_event() and > uvc_v4l2_unsubscribe_event(). Why isn't this the same lock taken by __video_do_ioctl()? The lock in video_device is set to it in `uvc_register_video()` in f_uvc.c: uvc->vdev.lock = &uvc->video.mutex; So this should be the same, right? Regards Thomas ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v4] usb: gadget: uvc: fix multiple opens 2020-11-21 12:50 ` Thomas Hämmerle @ 2020-12-01 19:27 ` Thomas Haemmerle 2020-12-07 8:53 ` Michael Tretter 2021-01-15 13:09 ` Felipe Balbi 2021-10-04 23:55 ` [PATCH v3] " Laurent Pinchart 1 sibling, 2 replies; 32+ messages in thread From: Thomas Haemmerle @ 2020-12-01 19:27 UTC (permalink / raw) To: laurent.pinchart Cc: gregkh, balbi, hverkuil, linux-usb, m.tretter, linux-media, Thomas Haemmerle 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 the UVC application potentially is streaming. Move activation of UVC function to subscription on UVC_EVENT_SETUP because there we can guarantee for a userspace application utilizing UVC. Block subscription on UVC_EVENT_SETUP while another application already is subscribed to it, indicated by `bool func_connected` in `struct uvc_device`. Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` to tag it as the handle used by the userspace UVC application. With this a process is able to check capabilities of the v4l2 device without deactivating the function for the actual UVC application. Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> --- v4: - remove unnecessary inner parentheses - keep and use the local video variable in `uvc_v4l2_release()` v3: - replace `unsigned int connections` with `bool func_connected` - rename `bool connected` to `bool is_uvc_app_handle` 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 | 49 ++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 23ee25383c1f..893aaa70f81a 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; + bool func_connected; /* 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 is_uvc_app_handle; }; #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 4ca89eab6159..197c26f7aec6 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -227,17 +227,55 @@ 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->func_connected) + return -EBUSY; + + ret = v4l2_event_subscribe(fh, sub, 2, NULL); + if (ret < 0) + return ret; + + if (sub->type == UVC_EVENT_SETUP) { + uvc->func_connected = true; + handle->is_uvc_app_handle = true; + uvc_function_connect(uvc); + } + + return 0; +} + +static void uvc_v4l2_disable(struct uvc_device *uvc) +{ + uvc->func_connected = false; + 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->is_uvc_app_handle) { + uvc_v4l2_disable(uvc); + handle->is_uvc_app_handle = false; + } + + return 0; } static long @@ -292,7 +330,6 @@ uvc_v4l2_open(struct file *file) handle->device = &uvc->video; file->private_data = &handle->vfh; - uvc_function_connect(uvc); return 0; } @@ -304,11 +341,9 @@ uvc_v4l2_release(struct file *file) 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); + if (handle->is_uvc_app_handle) + uvc_v4l2_disable(uvc); mutex_unlock(&video->mutex); file->private_data = NULL; -- 2.25.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v4] usb: gadget: uvc: fix multiple opens 2020-12-01 19:27 ` [PATCH v4] " Thomas Haemmerle @ 2020-12-07 8:53 ` Michael Tretter 2021-01-15 12:55 ` Michael Tretter 2021-01-15 13:09 ` Felipe Balbi 1 sibling, 1 reply; 32+ messages in thread From: Michael Tretter @ 2020-12-07 8:53 UTC (permalink / raw) To: Thomas Haemmerle Cc: laurent.pinchart, gregkh, balbi, hverkuil, linux-usb, linux-media On Tue, 01 Dec 2020 20:27:30 +0100, Thomas Haemmerle wrote: > 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 the > UVC application potentially is streaming. > > Move activation of UVC function to subscription on UVC_EVENT_SETUP > because there we can guarantee for a userspace application utilizing > UVC. > Block subscription on UVC_EVENT_SETUP while another application already > is subscribed to it, indicated by `bool func_connected` in > `struct uvc_device`. > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > to tag it as the handle used by the userspace UVC application. > > With this a process is able to check capabilities of the v4l2 device > without deactivating the function for the actual UVC application. > > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> While I agree that this driver overall could really use some love, this patch at least fixes the driver for user space applications that scan for v4l2 devices. Reviewed-By: Michael Tretter <m.tretter@pengutronix.de> > --- > v4: > - remove unnecessary inner parentheses > - keep and use the local video variable in `uvc_v4l2_release()` > > v3: > - replace `unsigned int connections` with `bool func_connected` > - rename `bool connected` to `bool is_uvc_app_handle` > > 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 | 49 ++++++++++++++++++++++---- > 2 files changed, 44 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 23ee25383c1f..893aaa70f81a 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; > + bool func_connected; > > /* 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 is_uvc_app_handle; > }; > > #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 4ca89eab6159..197c26f7aec6 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -227,17 +227,55 @@ 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->func_connected) > + return -EBUSY; > + > + ret = v4l2_event_subscribe(fh, sub, 2, NULL); > + if (ret < 0) > + return ret; > + > + if (sub->type == UVC_EVENT_SETUP) { > + uvc->func_connected = true; > + handle->is_uvc_app_handle = true; > + uvc_function_connect(uvc); > + } > + > + return 0; > +} > + > +static void uvc_v4l2_disable(struct uvc_device *uvc) > +{ > + uvc->func_connected = false; > + 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->is_uvc_app_handle) { > + uvc_v4l2_disable(uvc); > + handle->is_uvc_app_handle = false; > + } > + > + return 0; > } > > static long > @@ -292,7 +330,6 @@ uvc_v4l2_open(struct file *file) > handle->device = &uvc->video; > file->private_data = &handle->vfh; > > - uvc_function_connect(uvc); > return 0; > } > > @@ -304,11 +341,9 @@ uvc_v4l2_release(struct file *file) > 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); > + if (handle->is_uvc_app_handle) > + uvc_v4l2_disable(uvc); > mutex_unlock(&video->mutex); > > file->private_data = NULL; > -- > 2.25.1 > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4] usb: gadget: uvc: fix multiple opens 2020-12-07 8:53 ` Michael Tretter @ 2021-01-15 12:55 ` Michael Tretter 0 siblings, 0 replies; 32+ messages in thread From: Michael Tretter @ 2021-01-15 12:55 UTC (permalink / raw) To: Thomas Haemmerle, laurent.pinchart, gregkh, balbi, hverkuil, linux-usb, linux-media On Mon, 07 Dec 2020 09:53:10 +0100, Michael Tretter wrote: > On Tue, 01 Dec 2020 20:27:30 +0100, Thomas Haemmerle wrote: > > 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 the > > UVC application potentially is streaming. > > > > Move activation of UVC function to subscription on UVC_EVENT_SETUP > > because there we can guarantee for a userspace application utilizing > > UVC. > > Block subscription on UVC_EVENT_SETUP while another application already > > is subscribed to it, indicated by `bool func_connected` in > > `struct uvc_device`. > > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > > to tag it as the handle used by the userspace UVC application. > > > > With this a process is able to check capabilities of the v4l2 device > > without deactivating the function for the actual UVC application. > > > > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > > While I agree that this driver overall could really use some love, this patch > at least fixes the driver for user space applications that scan for v4l2 > devices. > > Reviewed-By: Michael Tretter <m.tretter@pengutronix.de> Ping. Is there anything missing for getting this patch merged? Michael > > > --- > > v4: > > - remove unnecessary inner parentheses > > - keep and use the local video variable in `uvc_v4l2_release()` > > > > v3: > > - replace `unsigned int connections` with `bool func_connected` > > - rename `bool connected` to `bool is_uvc_app_handle` > > > > 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 | 49 ++++++++++++++++++++++---- > > 2 files changed, 44 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > > index 23ee25383c1f..893aaa70f81a 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; > > + bool func_connected; > > > > /* 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 is_uvc_app_handle; > > }; > > > > #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 4ca89eab6159..197c26f7aec6 100644 > > --- a/drivers/usb/gadget/function/uvc_v4l2.c > > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > > @@ -227,17 +227,55 @@ 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->func_connected) > > + return -EBUSY; > > + > > + ret = v4l2_event_subscribe(fh, sub, 2, NULL); > > + if (ret < 0) > > + return ret; > > + > > + if (sub->type == UVC_EVENT_SETUP) { > > + uvc->func_connected = true; > > + handle->is_uvc_app_handle = true; > > + uvc_function_connect(uvc); > > + } > > + > > + return 0; > > +} > > + > > +static void uvc_v4l2_disable(struct uvc_device *uvc) > > +{ > > + uvc->func_connected = false; > > + 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->is_uvc_app_handle) { > > + uvc_v4l2_disable(uvc); > > + handle->is_uvc_app_handle = false; > > + } > > + > > + return 0; > > } > > > > static long > > @@ -292,7 +330,6 @@ uvc_v4l2_open(struct file *file) > > handle->device = &uvc->video; > > file->private_data = &handle->vfh; > > > > - uvc_function_connect(uvc); > > return 0; > > } > > > > @@ -304,11 +341,9 @@ uvc_v4l2_release(struct file *file) > > 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); > > + if (handle->is_uvc_app_handle) > > + uvc_v4l2_disable(uvc); > > mutex_unlock(&video->mutex); > > > > file->private_data = NULL; > > -- > > 2.25.1 > > > > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4] usb: gadget: uvc: fix multiple opens 2020-12-01 19:27 ` [PATCH v4] " Thomas Haemmerle 2020-12-07 8:53 ` Michael Tretter @ 2021-01-15 13:09 ` Felipe Balbi 2021-02-11 15:04 ` Michael Tretter 2021-10-03 20:13 ` [RESEND PATCH " Michael Grzeschik 1 sibling, 2 replies; 32+ messages in thread From: Felipe Balbi @ 2021-01-15 13:09 UTC (permalink / raw) To: Thomas Haemmerle, laurent.pinchart Cc: gregkh, hverkuil, linux-usb, m.tretter, linux-media, Thomas Haemmerle [-- Attachment #1: Type: text/plain, Size: 1326 bytes --] Hi, Thomas Haemmerle <thomas.haemmerle@wolfvision.net> writes: > 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 the > UVC application potentially is streaming. > > Move activation of UVC function to subscription on UVC_EVENT_SETUP > because there we can guarantee for a userspace application utilizing > UVC. > Block subscription on UVC_EVENT_SETUP while another application already > is subscribed to it, indicated by `bool func_connected` in > `struct uvc_device`. > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > to tag it as the handle used by the userspace UVC application. > > With this a process is able to check capabilities of the v4l2 device > without deactivating the function for the actual UVC application. > > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> Laurent, do you agree with the change? -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 857 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4] usb: gadget: uvc: fix multiple opens 2021-01-15 13:09 ` Felipe Balbi @ 2021-02-11 15:04 ` Michael Tretter 2021-10-03 20:13 ` [RESEND PATCH " Michael Grzeschik 1 sibling, 0 replies; 32+ messages in thread From: Michael Tretter @ 2021-02-11 15:04 UTC (permalink / raw) To: Felipe Balbi Cc: Thomas Haemmerle, laurent.pinchart, gregkh, hverkuil, linux-usb, linux-media On Fri, 15 Jan 2021 15:09:39 +0200, Felipe Balbi wrote: > Thomas Haemmerle <thomas.haemmerle@wolfvision.net> writes: > > 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 the > > UVC application potentially is streaming. > > > > Move activation of UVC function to subscription on UVC_EVENT_SETUP > > because there we can guarantee for a userspace application utilizing > > UVC. > > Block subscription on UVC_EVENT_SETUP while another application already > > is subscribed to it, indicated by `bool func_connected` in > > `struct uvc_device`. > > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > > to tag it as the handle used by the userspace UVC application. > > > > With this a process is able to check capabilities of the v4l2 device > > without deactivating the function for the actual UVC application. > > > > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > > Laurent, do you agree with the change? Gentle ping. Michael ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RESEND PATCH v4] usb: gadget: uvc: fix multiple opens 2021-01-15 13:09 ` Felipe Balbi 2021-02-11 15:04 ` Michael Tretter @ 2021-10-03 20:13 ` Michael Grzeschik 2021-10-04 23:53 ` Laurent Pinchart 1 sibling, 1 reply; 32+ messages in thread From: Michael Grzeschik @ 2021-10-03 20:13 UTC (permalink / raw) To: laurent.pinchart Cc: linux-usb, balbi, hverkuil, gregkh, m.tretter, linux-media 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 the UVC application potentially is streaming. Move activation of UVC function to subscription on UVC_EVENT_SETUP because there we can guarantee for a userspace application utilizing UVC. Block subscription on UVC_EVENT_SETUP while another application already is subscribed to it, indicated by `bool func_connected` in `struct uvc_device`. Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` to tag it as the handle used by the userspace UVC application. With this a process is able to check capabilities of the v4l2 device without deactivating the function for the actual UVC application. Reviewed-By: Michael Tretter <m.tretter@pengutronix.de> Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v4: - remove unnecessary inner parentheses - keep and use the local video variable in `uvc_v4l2_release()` v3: - replace `unsigned int connections` with `bool func_connected` - rename `bool connected` to `bool is_uvc_app_handle` 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 | 49 ++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 255a61bd6a6a8..9d5f17b551bbd 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -126,6 +126,7 @@ struct uvc_device { enum uvc_state state; struct usb_function func; struct uvc_video video; + bool func_connected; /* Descriptors */ struct { @@ -156,6 +157,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 is_uvc_app_handle; }; #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 4ca89eab61590..197c26f7aec63 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -227,17 +227,55 @@ 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->func_connected) + return -EBUSY; + + ret = v4l2_event_subscribe(fh, sub, 2, NULL); + if (ret < 0) + return ret; + + if (sub->type == UVC_EVENT_SETUP) { + uvc->func_connected = true; + handle->is_uvc_app_handle = true; + uvc_function_connect(uvc); + } + + return 0; +} + +static void uvc_v4l2_disable(struct uvc_device *uvc) +{ + uvc->func_connected = false; + 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->is_uvc_app_handle) { + uvc_v4l2_disable(uvc); + handle->is_uvc_app_handle = false; + } + + return 0; } static long @@ -292,7 +330,6 @@ uvc_v4l2_open(struct file *file) handle->device = &uvc->video; file->private_data = &handle->vfh; - uvc_function_connect(uvc); return 0; } @@ -304,11 +341,9 @@ uvc_v4l2_release(struct file *file) 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); + if (handle->is_uvc_app_handle) + uvc_v4l2_disable(uvc); mutex_unlock(&video->mutex); file->private_data = NULL; -- 2.30.2 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RESEND PATCH v4] usb: gadget: uvc: fix multiple opens 2021-10-03 20:13 ` [RESEND PATCH " Michael Grzeschik @ 2021-10-04 23:53 ` Laurent Pinchart 2021-10-05 10:59 ` Greg KH 0 siblings, 1 reply; 32+ messages in thread From: Laurent Pinchart @ 2021-10-04 23:53 UTC (permalink / raw) To: Michael Grzeschik Cc: linux-usb, balbi, hverkuil, gregkh, m.tretter, linux-media Hi Michael, Thank you for resending this. On Sun, Oct 03, 2021 at 10:13:55PM +0200, Michael Grzeschik 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 the > UVC application potentially is streaming. > > Move activation of UVC function to subscription on UVC_EVENT_SETUP > because there we can guarantee for a userspace application utilizing > UVC. > Block subscription on UVC_EVENT_SETUP while another application already > is subscribed to it, indicated by `bool func_connected` in > `struct uvc_device`. > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > to tag it as the handle used by the userspace UVC application. Reflowing the paragraph would be nice (this could be done when applying the patch, or not at all). > With this a process is able to check capabilities of the v4l2 device > without deactivating the function for the actual UVC application. > > Reviewed-By: Michael Tretter <m.tretter@pengutronix.de> > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Felipe, please let me know if you want me to take this in my tree and issue a pull request, otherwise I'll assume you'll pick it up. > --- > v4: > - remove unnecessary inner parentheses > - keep and use the local video variable in `uvc_v4l2_release()` > > v3: > - replace `unsigned int connections` with `bool func_connected` > - rename `bool connected` to `bool is_uvc_app_handle` > > 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 | 49 ++++++++++++++++++++++---- > 2 files changed, 44 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 255a61bd6a6a8..9d5f17b551bbd 100644 > --- a/drivers/usb/gadget/function/uvc.h > +++ b/drivers/usb/gadget/function/uvc.h > @@ -126,6 +126,7 @@ struct uvc_device { > enum uvc_state state; > struct usb_function func; > struct uvc_video video; > + bool func_connected; > > /* Descriptors */ > struct { > @@ -156,6 +157,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 is_uvc_app_handle; > }; > > #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 4ca89eab61590..197c26f7aec63 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -227,17 +227,55 @@ 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->func_connected) > + return -EBUSY; > + > + ret = v4l2_event_subscribe(fh, sub, 2, NULL); > + if (ret < 0) > + return ret; > + > + if (sub->type == UVC_EVENT_SETUP) { > + uvc->func_connected = true; > + handle->is_uvc_app_handle = true; > + uvc_function_connect(uvc); > + } > + > + return 0; > +} > + > +static void uvc_v4l2_disable(struct uvc_device *uvc) > +{ > + uvc->func_connected = false; > + 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->is_uvc_app_handle) { > + uvc_v4l2_disable(uvc); > + handle->is_uvc_app_handle = false; > + } > + > + return 0; > } > > static long > @@ -292,7 +330,6 @@ uvc_v4l2_open(struct file *file) > handle->device = &uvc->video; > file->private_data = &handle->vfh; > > - uvc_function_connect(uvc); > return 0; > } > > @@ -304,11 +341,9 @@ uvc_v4l2_release(struct file *file) > 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); > + if (handle->is_uvc_app_handle) > + uvc_v4l2_disable(uvc); > mutex_unlock(&video->mutex); > > file->private_data = NULL; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RESEND PATCH v4] usb: gadget: uvc: fix multiple opens 2021-10-04 23:53 ` Laurent Pinchart @ 2021-10-05 10:59 ` Greg KH 2021-10-05 11:06 ` Felipe Balbi 0 siblings, 1 reply; 32+ messages in thread From: Greg KH @ 2021-10-05 10:59 UTC (permalink / raw) To: Laurent Pinchart Cc: Michael Grzeschik, linux-usb, balbi, hverkuil, m.tretter, linux-media On Tue, Oct 05, 2021 at 02:53:48AM +0300, Laurent Pinchart wrote: > Hi Michael, > > Thank you for resending this. > > On Sun, Oct 03, 2021 at 10:13:55PM +0200, Michael Grzeschik 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 the > > UVC application potentially is streaming. > > > > Move activation of UVC function to subscription on UVC_EVENT_SETUP > > because there we can guarantee for a userspace application utilizing > > UVC. > > Block subscription on UVC_EVENT_SETUP while another application already > > is subscribed to it, indicated by `bool func_connected` in > > `struct uvc_device`. > > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > > to tag it as the handle used by the userspace UVC application. > > Reflowing the paragraph would be nice (this could be done when applying > the patch, or not at all). > > > With this a process is able to check capabilities of the v4l2 device > > without deactivating the function for the actual UVC application. > > > > Reviewed-By: Michael Tretter <m.tretter@pengutronix.de> > > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Felipe, please let me know if you want me to take this in my tree and > issue a pull request, otherwise I'll assume you'll pick it up. I'll pick it up now, thanks. greg k-h ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RESEND PATCH v4] usb: gadget: uvc: fix multiple opens 2021-10-05 10:59 ` Greg KH @ 2021-10-05 11:06 ` Felipe Balbi 2021-10-05 11:37 ` Greg KH 0 siblings, 1 reply; 32+ messages in thread From: Felipe Balbi @ 2021-10-05 11:06 UTC (permalink / raw) To: Greg KH Cc: Laurent Pinchart, Michael Grzeschik, linux-usb, hverkuil, m.tretter, linux-media Greg KH <gregkh@linuxfoundation.org> writes: > On Tue, Oct 05, 2021 at 02:53:48AM +0300, Laurent Pinchart wrote: >> Hi Michael, >> >> Thank you for resending this. >> >> On Sun, Oct 03, 2021 at 10:13:55PM +0200, Michael Grzeschik 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 the >> > UVC application potentially is streaming. >> > >> > Move activation of UVC function to subscription on UVC_EVENT_SETUP >> > because there we can guarantee for a userspace application utilizing >> > UVC. >> > Block subscription on UVC_EVENT_SETUP while another application already >> > is subscribed to it, indicated by `bool func_connected` in >> > `struct uvc_device`. >> > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` >> > to tag it as the handle used by the userspace UVC application. >> >> Reflowing the paragraph would be nice (this could be done when applying >> the patch, or not at all). >> >> > With this a process is able to check capabilities of the v4l2 device >> > without deactivating the function for the actual UVC application. >> > >> > Reviewed-By: Michael Tretter <m.tretter@pengutronix.de> >> > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> >> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Felipe, please let me know if you want me to take this in my tree and >> issue a pull request, otherwise I'll assume you'll pick it up. > > I'll pick it up now, thanks. I guess it's too late for an Ack. FWIW: Acked-by: Felipe Balbi <balbi@kernel.org> -- balbi ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RESEND PATCH v4] usb: gadget: uvc: fix multiple opens 2021-10-05 11:06 ` Felipe Balbi @ 2021-10-05 11:37 ` Greg KH 0 siblings, 0 replies; 32+ messages in thread From: Greg KH @ 2021-10-05 11:37 UTC (permalink / raw) To: Felipe Balbi Cc: Laurent Pinchart, Michael Grzeschik, linux-usb, hverkuil, m.tretter, linux-media On Tue, Oct 05, 2021 at 02:06:22PM +0300, Felipe Balbi wrote: > > Greg KH <gregkh@linuxfoundation.org> writes: > > > On Tue, Oct 05, 2021 at 02:53:48AM +0300, Laurent Pinchart wrote: > >> Hi Michael, > >> > >> Thank you for resending this. > >> > >> On Sun, Oct 03, 2021 at 10:13:55PM +0200, Michael Grzeschik 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 the > >> > UVC application potentially is streaming. > >> > > >> > Move activation of UVC function to subscription on UVC_EVENT_SETUP > >> > because there we can guarantee for a userspace application utilizing > >> > UVC. > >> > Block subscription on UVC_EVENT_SETUP while another application already > >> > is subscribed to it, indicated by `bool func_connected` in > >> > `struct uvc_device`. > >> > Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > >> > to tag it as the handle used by the userspace UVC application. > >> > >> Reflowing the paragraph would be nice (this could be done when applying > >> the patch, or not at all). > >> > >> > With this a process is able to check capabilities of the v4l2 device > >> > without deactivating the function for the actual UVC application. > >> > > >> > Reviewed-By: Michael Tretter <m.tretter@pengutronix.de> > >> > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > >> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > >> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > >> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> Felipe, please let me know if you want me to take this in my tree and > >> issue a pull request, otherwise I'll assume you'll pick it up. > > > > I'll pick it up now, thanks. > > I guess it's too late for an Ack. FWIW: > > Acked-by: Felipe Balbi <balbi@kernel.org> Nope, not too late, just added, thanks! greg k-h ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3] usb: gadget: uvc: fix multiple opens 2020-11-21 12:50 ` Thomas Hämmerle 2020-12-01 19:27 ` [PATCH v4] " Thomas Haemmerle @ 2021-10-04 23:55 ` Laurent Pinchart 1 sibling, 0 replies; 32+ messages in thread From: Laurent Pinchart @ 2021-10-04 23:55 UTC (permalink / raw) To: Thomas Hämmerle Cc: gregkh, balbi, hverkuil, linux-usb, m.tretter, linux-media On Sat, Nov 21, 2020 at 01:50:55PM +0100, Thomas Hämmerle wrote: > Hi Laurent, > > sorry for my late response! What should I say... > On 16.11.20 16:48, Laurent Pinchart wrote: > > Hi Thomas, > > > > Thank you for the patch, and sorry for the late review. I was mostly > > absent last week. > > > > On Tue, Nov 10, 2020 at 03:30:15PM +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 the > >> UVC application potentially is streaming. > >> > >> Move activation of UVC function to subscription on UVC_EVENT_SETUP > >> because there we can guarantee for a userspace application utilizing UVC. > >> Block subscription on UVC_EVENT_SETUP while another application already > >> is subscribed to it, indicated by `bool func_connected` in > >> `struct uvc_device`. > >> Extend the `struct uvc_file_handle` with member `bool is_uvc_app_handle` > >> to tag it as the handle used by the userspace UVC application. > >> > >> With this a process is able to check capabilities of the v4l2 device > >> without deactivating the function for the actual UVC application. > >> > >> Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > >> --- > >> v3: > >> - replace `unsigned int connections` with `bool func_connected` > >> - rename `bool connected` to `bool is_uvc_app_handle` > >> > >> 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 | 54 +++++++++++++++++++++----- > >> 2 files changed, 46 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > >> index 73da4f9a8d4c..d6d0fd2dffa0 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; > >> + bool func_connected; > >> > >> /* 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 is_uvc_app_handle; > >> }; > >> > >> #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..3c0b7a969107 100644 > >> --- a/drivers/usb/gadget/function/uvc_v4l2.c > >> +++ b/drivers/usb/gadget/function/uvc_v4l2.c > >> @@ -228,17 +228,55 @@ 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->func_connected) > > > > No need for the inner parentheses. > > I will change this. > > > > >> + return -EBUSY; > >> + > >> + ret = v4l2_event_subscribe(fh, sub, 2, NULL); > >> + if (ret < 0) > >> + return ret; > >> + > >> + if (sub->type == UVC_EVENT_SETUP) { > >> + uvc->func_connected = true; > >> + handle->is_uvc_app_handle = true; > >> + uvc_function_connect(uvc); > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static void uvc_v4l2_disable(struct uvc_device *uvc) > >> +{ > >> + uvc->func_connected = false; > >> + 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->is_uvc_app_handle) { > > > > No need for the inner parentheses. > > I will change this. > > >> + uvc_v4l2_disable(uvc); > >> + handle->is_uvc_app_handle = false; > > > > Calling uvc_v4l2_disable() here means that we'll stop everything when > > unsubscribing from the event, which sounds like it could cause issues as > > that behaviour is not expected. Wouldn't it be enough to only handle > > this in uvc_v4l2_release() ? > > Of course it would be enough. But maybe a UVC gadget application wants > to release the device for another application without closing it and > since the function is activated on subscription the logical consequence > is to deactivate it on unsubscription. It still bothers me a tiny bit to make ownership an implicit side effect of event subscription, but I think it's the best option for now without requiring API changes, so I'm fine with it. > >> + } > >> + > >> + return 0; > >> } > >> > >> static long > >> @@ -293,7 +331,6 @@ uvc_v4l2_open(struct file *file) > >> handle->device = &uvc->video; > >> file->private_data = &handle->vfh; > >> > >> - uvc_function_connect(uvc); > >> return 0; > >> } > >> > >> @@ -303,14 +340,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); > > > > Could you please keep keep the local video variable, and use > > &video->mutex here ? The driver has a single video device at the moment, > > but could be extended in the future with support for multiple video > > devices in a single UVC device (lots of changes would be needed though). > > Yes. > > >> + if (handle->is_uvc_app_handle) > >> + uvc_v4l2_disable(uvc); > >> + mutex_unlock(&uvc->video.mutex); > > > > Note that this lock isn't the same as the lock taken by > > __video_do_ioctl(), which alls uvc_v4l2_subscribe_event() and > > uvc_v4l2_unsubscribe_event(). I think Hans got confused in his review, > > it appears that there's nothing protecting concurrent access to > > is_uvc_app_handle and func_connected in v3. I think you need to take the > > driver-specific lock in uvc_v4l2_subscribe_event() and > > uvc_v4l2_unsubscribe_event(). > > Why isn't this the same lock taken by __video_do_ioctl()? > The lock in video_device is set to it in `uvc_register_video()` in f_uvc.c: > uvc->vdev.lock = &uvc->video.mutex; > > So this should be the same, right? Now I wonder what I meant by the above... Let's ignore my comment. I've acked v4. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 8:25 ` thomas.haemmerle 2020-11-10 8:40 ` Greg KH @ 2020-11-10 10:31 ` Hans Verkuil 2020-11-10 11:53 ` Thomas Hämmerle 1 sibling, 1 reply; 32+ messages in thread From: Hans Verkuil @ 2020-11-10 10:31 UTC (permalink / raw) To: thomas.haemmerle, gregkh Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media On 10/11/2020 09:25, 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; > }; > > #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); > + } This makes no sense. Why would subscribing to a SETUP event mean that you are 'connected'? It should be possible to open a V4L2 device node any number of times, and any filehandle can subscribe to any event, but typically once userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) then that filehandle is marked as the owner of the device and other open filehandles are no longer allowed to allocate buffers or stream video. See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. Unfortunately this UVC gadget driver is rather old and is not using these helper functions. Running 'v4l2-compliance' will likely fail on a lot of tests for this driver. This driver probably could use some TLC. Regards, Hans > + > + 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); > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 10:31 ` [PATCH v2] " Hans Verkuil @ 2020-11-10 11:53 ` Thomas Hämmerle 2020-11-10 14:43 ` Hans Verkuil 0 siblings, 1 reply; 32+ messages in thread From: Thomas Hämmerle @ 2020-11-10 11:53 UTC (permalink / raw) To: Hans Verkuil, gregkh Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media On 10.11.20 11:31, Hans Verkuil wrote: > On 10/11/2020 09:25, 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; >> }; >> >> #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); >> + } > > This makes no sense. Why would subscribing to a SETUP event > mean that you are 'connected'? Subscribing to a SETUP does not mean that you are connected - on a subscription to SETUP we can expect that there is a userspace process, utilizing UVC -- which is required for the UVC gadget function -- and everything is ready to enable the function by calling uvc_function_connect. How about calling it `function_connected`? > > It should be possible to open a V4L2 device node any number of times, > and any filehandle can subscribe to any event, but typically once > userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) > then that filehandle is marked as the owner of the device and other > open filehandles are no longer allowed to allocate buffers or stream video. Sure - this can be also done if userspace allocates buffers. But why does it make more sense to call uvc_function_connect on VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS instead of subscribtion to a UVC event? > > See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c > and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. > > Unfortunately this UVC gadget driver is rather old and is not using > these helper functions. > > Running 'v4l2-compliance' will likely fail on a lot of tests for this > driver. > > This driver probably could use some TLC. I totally agree with that, however this change fixes at least one test of 'v4l2-compliance'. Currently a running UVC connection can be corrupted by another process which just opens and closes the device. Thomas > > Regards, > > Hans > >> + >> + 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); >> > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 11:53 ` Thomas Hämmerle @ 2020-11-10 14:43 ` Hans Verkuil 2020-11-10 15:10 ` Thomas Hämmerle 0 siblings, 1 reply; 32+ messages in thread From: Hans Verkuil @ 2020-11-10 14:43 UTC (permalink / raw) To: Thomas Hämmerle, gregkh Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media On 10/11/2020 12:53, Thomas Hämmerle wrote: > On 10.11.20 11:31, Hans Verkuil wrote: >> On 10/11/2020 09:25, 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; >>> }; >>> >>> #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); >>> + } >> >> This makes no sense. Why would subscribing to a SETUP event >> mean that you are 'connected'? > > Subscribing to a SETUP does not mean that you are connected - on a > subscription to SETUP we can expect that there is a userspace process, > utilizing UVC -- which is required for the UVC gadget function -- and > everything is ready to enable the function by calling uvc_function_connect. > How about calling it `function_connected`? Any application can open the device node and subscribe to this event. This is not a good place to call uvc_function_connect(), it's an abuse of the V4L2 API. I dug a bit more into this (I have very little gadget driver experience), and isn't calling uvc_function_connect() something that should be done on the first open? And when the last filehandle is closed, then uvc_function_disconnect() can be called to clean up? That would make much more sense. Grep for v4l2_fh_is_singular_file() on how to do this, quite a few media drivers need this. Regards, Hans > >> >> It should be possible to open a V4L2 device node any number of times, >> and any filehandle can subscribe to any event, but typically once >> userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) >> then that filehandle is marked as the owner of the device and other >> open filehandles are no longer allowed to allocate buffers or stream video. > > Sure - this can be also done if userspace allocates buffers. > But why does it make more sense to call uvc_function_connect on > VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS instead of subscribtion to a UVC event? > >> >> See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c >> and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. >> >> Unfortunately this UVC gadget driver is rather old and is not using >> these helper functions. >> >> Running 'v4l2-compliance' will likely fail on a lot of tests for this >> driver. >> >> This driver probably could use some TLC. > > I totally agree with that, however this change fixes at least one test > of 'v4l2-compliance'. > Currently a running UVC connection can be corrupted by another process > which just opens and closes the device. > > Thomas > >> >> Regards, >> >> Hans >> >>> + >>> + 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); >>> >> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 14:43 ` Hans Verkuil @ 2020-11-10 15:10 ` Thomas Hämmerle 2020-11-10 15:36 ` Hans Verkuil 0 siblings, 1 reply; 32+ messages in thread From: Thomas Hämmerle @ 2020-11-10 15:10 UTC (permalink / raw) To: Hans Verkuil, gregkh Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media On 10.11.20 15:43, Hans Verkuil wrote: > On 10/11/2020 12:53, Thomas Hämmerle wrote: >> On 10.11.20 11:31, Hans Verkuil wrote: >>> On 10/11/2020 09:25, 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; >>>> }; >>>> >>>> #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); >>>> + } >>> >>> This makes no sense. Why would subscribing to a SETUP event >>> mean that you are 'connected'? >> >> Subscribing to a SETUP does not mean that you are connected - on a >> subscription to SETUP we can expect that there is a userspace process, >> utilizing UVC -- which is required for the UVC gadget function -- and >> everything is ready to enable the function by calling uvc_function_connect. >> How about calling it `function_connected`? > > Any application can open the device node and subscribe to this event. > This is not a good place to call uvc_function_connect(), it's an abuse of > the V4L2 API. Of course - any application can subscribe to this event but since the event is defined in g_uvc.h I thought that a subscription is most likely done by a UVC application. > > I dug a bit more into this (I have very little gadget driver experience), > and isn't calling uvc_function_connect() something that should be done > on the first open? And when the last filehandle is closed, then > uvc_function_disconnect() can be called to clean up? Isn't the possibility of the delayed activation of the usb function in composite.c intended to wait for a corresponding userspace application, which is required to make the gadget work? If so, a simple open of the v4l2 device does not mean that the gadget is ready to go: starting pipewire for example results in quering the device capabilities of all devices. But this does not mean that the gadget will work. Therefore I was looking for a place, where we can expect, that the application opened the device will utilize UVC and found that a subscription to a UVC specific event is the right place. Regards, Thomas > > That would make much more sense. Grep for v4l2_fh_is_singular_file() on how > to do this, quite a few media drivers need this. > > Regards, > > Hans > >> >>> >>> It should be possible to open a V4L2 device node any number of times, >>> and any filehandle can subscribe to any event, but typically once >>> userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) >>> then that filehandle is marked as the owner of the device and other >>> open filehandles are no longer allowed to allocate buffers or stream video. >> >> Sure - this can be also done if userspace allocates buffers. >> But why does it make more sense to call uvc_function_connect on >> VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS instead of subscribtion to a UVC event? >> >>> >>> See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c >>> and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. >>> >>> Unfortunately this UVC gadget driver is rather old and is not using >>> these helper functions. >>> >>> Running 'v4l2-compliance' will likely fail on a lot of tests for this >>> driver. >>> >>> This driver probably could use some TLC. >> >> I totally agree with that, however this change fixes at least one test >> of 'v4l2-compliance'. >> Currently a running UVC connection can be corrupted by another process >> which just opens and closes the device. >> >> Thomas >> >>> >>> Regards, >>> >>> Hans >>> >>>> + >>>> + 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); >>>> >>> > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 15:10 ` Thomas Hämmerle @ 2020-11-10 15:36 ` Hans Verkuil 2020-11-10 15:50 ` Thomas Hämmerle 0 siblings, 1 reply; 32+ messages in thread From: Hans Verkuil @ 2020-11-10 15:36 UTC (permalink / raw) To: Thomas Hämmerle, gregkh Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media On 10/11/2020 16:10, Thomas Hämmerle wrote: > On 10.11.20 15:43, Hans Verkuil wrote: >> On 10/11/2020 12:53, Thomas Hämmerle wrote: >>> On 10.11.20 11:31, Hans Verkuil wrote: >>>> On 10/11/2020 09:25, 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; >>>>> }; >>>>> >>>>> #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); >>>>> + } >>>> >>>> This makes no sense. Why would subscribing to a SETUP event >>>> mean that you are 'connected'? >>> >>> Subscribing to a SETUP does not mean that you are connected - on a >>> subscription to SETUP we can expect that there is a userspace process, >>> utilizing UVC -- which is required for the UVC gadget function -- and >>> everything is ready to enable the function by calling uvc_function_connect. >>> How about calling it `function_connected`? >> >> Any application can open the device node and subscribe to this event. >> This is not a good place to call uvc_function_connect(), it's an abuse of >> the V4L2 API. > > Of course - any application can subscribe to this event but since the > event is defined in g_uvc.h I thought that a subscription is most likely > done by a UVC application. > >> >> I dug a bit more into this (I have very little gadget driver experience), >> and isn't calling uvc_function_connect() something that should be done >> on the first open? And when the last filehandle is closed, then >> uvc_function_disconnect() can be called to clean up? > > Isn't the possibility of the delayed activation of the usb function in > composite.c intended to wait for a corresponding userspace application, > which is required to make the gadget work? > > If so, a simple open of the v4l2 device does not mean that the gadget is > ready to go: starting pipewire for example results in quering the device > capabilities of all devices. But this does not mean that the gadget will > work. > Therefore I was looking for a place, where we can expect, that the > application opened the device will utilize UVC and found that a > subscription to a UVC specific event is the right place. That's why I suggested to do this when buffers are allocated. That's when the application commits resources to actually use the device. Until that point everything that an application does is just querying or configuring, but not actually using it. BTW, Laurent Pinchart who maintains this driver is away for a week, so he might know more about this when he comes back. Regards, Hans > > Regards, > Thomas > >> >> That would make much more sense. Grep for v4l2_fh_is_singular_file() on how >> to do this, quite a few media drivers need this. >> >> Regards, >> >> Hans >> >>> >>>> >>>> It should be possible to open a V4L2 device node any number of times, >>>> and any filehandle can subscribe to any event, but typically once >>>> userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) >>>> then that filehandle is marked as the owner of the device and other >>>> open filehandles are no longer allowed to allocate buffers or stream video. >>> >>> Sure - this can be also done if userspace allocates buffers. >>> But why does it make more sense to call uvc_function_connect on >>> VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS instead of subscribtion to a UVC event? >>> >>>> >>>> See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c >>>> and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. >>>> >>>> Unfortunately this UVC gadget driver is rather old and is not using >>>> these helper functions. >>>> >>>> Running 'v4l2-compliance' will likely fail on a lot of tests for this >>>> driver. >>>> >>>> This driver probably could use some TLC. >>> >>> I totally agree with that, however this change fixes at least one test >>> of 'v4l2-compliance'. >>> Currently a running UVC connection can be corrupted by another process >>> which just opens and closes the device. >>> >>> Thomas >>> >>>> >>>> Regards, >>>> >>>> Hans >>>> >>>>> + >>>>> + 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); >>>>> >>>> >> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 15:36 ` Hans Verkuil @ 2020-11-10 15:50 ` Thomas Hämmerle 2020-11-10 16:01 ` Hans Verkuil 0 siblings, 1 reply; 32+ messages in thread From: Thomas Hämmerle @ 2020-11-10 15:50 UTC (permalink / raw) To: Hans Verkuil, gregkh Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media On 10.11.20 16:36, Hans Verkuil wrote: > On 10/11/2020 16:10, Thomas Hämmerle wrote: >> On 10.11.20 15:43, Hans Verkuil wrote: >>> On 10/11/2020 12:53, Thomas Hämmerle wrote: >>>> On 10.11.20 11:31, Hans Verkuil wrote: >>>>> On 10/11/2020 09:25, 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; >>>>>> }; >>>>>> >>>>>> #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); >>>>>> + } >>>>> >>>>> This makes no sense. Why would subscribing to a SETUP event >>>>> mean that you are 'connected'? >>>> >>>> Subscribing to a SETUP does not mean that you are connected - on a >>>> subscription to SETUP we can expect that there is a userspace process, >>>> utilizing UVC -- which is required for the UVC gadget function -- and >>>> everything is ready to enable the function by calling uvc_function_connect. >>>> How about calling it `function_connected`? >>> >>> Any application can open the device node and subscribe to this event. >>> This is not a good place to call uvc_function_connect(), it's an abuse of >>> the V4L2 API. >> >> Of course - any application can subscribe to this event but since the >> event is defined in g_uvc.h I thought that a subscription is most likely >> done by a UVC application. >> >>> >>> I dug a bit more into this (I have very little gadget driver experience), >>> and isn't calling uvc_function_connect() something that should be done >>> on the first open? And when the last filehandle is closed, then >>> uvc_function_disconnect() can be called to clean up? >> >> Isn't the possibility of the delayed activation of the usb function in >> composite.c intended to wait for a corresponding userspace application, >> which is required to make the gadget work? >> >> If so, a simple open of the v4l2 device does not mean that the gadget is >> ready to go: starting pipewire for example results in quering the device >> capabilities of all devices. But this does not mean that the gadget will >> work. >> Therefore I was looking for a place, where we can expect, that the >> application opened the device will utilize UVC and found that a >> subscription to a UVC specific event is the right place. > > That's why I suggested to do this when buffers are allocated. That's when > the application commits resources to actually use the device. Until that > point everything that an application does is just querying or configuring, > but not actually using it. The userspace application implementations (including the one from Laurent Pinchart) usually request buffers on the UVC events sent by the host. Therefore we would run into chicken-and-egg problem. > > BTW, Laurent Pinchart who maintains this driver is away for a week, so he > might know more about this when he comes back. Okay, so I suggest to wait for him. Thanks for the review so far! Regards Thomas > > Regards, > > Hans > >> >> Regards, >> Thomas >> >>> >>> That would make much more sense. Grep for v4l2_fh_is_singular_file() on how >>> to do this, quite a few media drivers need this. >>> >>> Regards, >>> >>> Hans >>> >>>> >>>>> >>>>> It should be possible to open a V4L2 device node any number of times, >>>>> and any filehandle can subscribe to any event, but typically once >>>>> userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) >>>>> then that filehandle is marked as the owner of the device and other >>>>> open filehandles are no longer allowed to allocate buffers or stream video. >>>> >>>> Sure - this can be also done if userspace allocates buffers. >>>> But why does it make more sense to call uvc_function_connect on >>>> VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS instead of subscribtion to a UVC event? >>>> >>>>> >>>>> See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c >>>>> and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. >>>>> >>>>> Unfortunately this UVC gadget driver is rather old and is not using >>>>> these helper functions. >>>>> >>>>> Running 'v4l2-compliance' will likely fail on a lot of tests for this >>>>> driver. >>>>> >>>>> This driver probably could use some TLC. >>>> >>>> I totally agree with that, however this change fixes at least one test >>>> of 'v4l2-compliance'. >>>> Currently a running UVC connection can be corrupted by another process >>>> which just opens and closes the device. >>>> >>>> Thomas >>>> >>>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>>>> + >>>>>> + 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); >>>>>> >>>>> >>> > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 15:50 ` Thomas Hämmerle @ 2020-11-10 16:01 ` Hans Verkuil 2020-11-16 15:31 ` Laurent Pinchart 0 siblings, 1 reply; 32+ messages in thread From: Hans Verkuil @ 2020-11-10 16:01 UTC (permalink / raw) To: Thomas Hämmerle, gregkh Cc: laurent.pinchart, balbi, linux-usb, m.tretter, linux-media On 10/11/2020 16:50, Thomas Hämmerle wrote: > On 10.11.20 16:36, Hans Verkuil wrote: >> On 10/11/2020 16:10, Thomas Hämmerle wrote: >>> On 10.11.20 15:43, Hans Verkuil wrote: >>>> On 10/11/2020 12:53, Thomas Hämmerle wrote: >>>>> On 10.11.20 11:31, Hans Verkuil wrote: >>>>>> On 10/11/2020 09:25, 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; >>>>>>> }; >>>>>>> >>>>>>> #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); >>>>>>> + } >>>>>> >>>>>> This makes no sense. Why would subscribing to a SETUP event >>>>>> mean that you are 'connected'? >>>>> >>>>> Subscribing to a SETUP does not mean that you are connected - on a >>>>> subscription to SETUP we can expect that there is a userspace process, >>>>> utilizing UVC -- which is required for the UVC gadget function -- and >>>>> everything is ready to enable the function by calling uvc_function_connect. >>>>> How about calling it `function_connected`? >>>> >>>> Any application can open the device node and subscribe to this event. >>>> This is not a good place to call uvc_function_connect(), it's an abuse of >>>> the V4L2 API. >>> >>> Of course - any application can subscribe to this event but since the >>> event is defined in g_uvc.h I thought that a subscription is most likely >>> done by a UVC application. >>> >>>> >>>> I dug a bit more into this (I have very little gadget driver experience), >>>> and isn't calling uvc_function_connect() something that should be done >>>> on the first open? And when the last filehandle is closed, then >>>> uvc_function_disconnect() can be called to clean up? >>> >>> Isn't the possibility of the delayed activation of the usb function in >>> composite.c intended to wait for a corresponding userspace application, >>> which is required to make the gadget work? >>> >>> If so, a simple open of the v4l2 device does not mean that the gadget is >>> ready to go: starting pipewire for example results in quering the device >>> capabilities of all devices. But this does not mean that the gadget will >>> work. >>> Therefore I was looking for a place, where we can expect, that the >>> application opened the device will utilize UVC and found that a >>> subscription to a UVC specific event is the right place. >> >> That's why I suggested to do this when buffers are allocated. That's when >> the application commits resources to actually use the device. Until that >> point everything that an application does is just querying or configuring, >> but not actually using it. > > The userspace application implementations (including the one from > Laurent Pinchart) usually request buffers on the UVC events sent by the > host. Therefore we would run into chicken-and-egg problem. Ah, stupid of me. You're right. For now I go with calling uvc_function_connect() on the first open. If nothing else, this is compatible with the current situation, while still allowing for multiple opens. Regards, Hans > >> >> BTW, Laurent Pinchart who maintains this driver is away for a week, so he >> might know more about this when he comes back. > > Okay, so I suggest to wait for him. > > Thanks for the review so far! > > Regards > Thomas > >> >> Regards, >> >> Hans >> >>> >>> Regards, >>> Thomas >>> >>>> >>>> That would make much more sense. Grep for v4l2_fh_is_singular_file() on how >>>> to do this, quite a few media drivers need this. >>>> >>>> Regards, >>>> >>>> Hans >>>> >>>>> >>>>>> >>>>>> It should be possible to open a V4L2 device node any number of times, >>>>>> and any filehandle can subscribe to any event, but typically once >>>>>> userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) >>>>>> then that filehandle is marked as the owner of the device and other >>>>>> open filehandles are no longer allowed to allocate buffers or stream video. >>>>> >>>>> Sure - this can be also done if userspace allocates buffers. >>>>> But why does it make more sense to call uvc_function_connect on >>>>> VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS instead of subscribtion to a UVC event? >>>>> >>>>>> >>>>>> See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c >>>>>> and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. >>>>>> >>>>>> Unfortunately this UVC gadget driver is rather old and is not using >>>>>> these helper functions. >>>>>> >>>>>> Running 'v4l2-compliance' will likely fail on a lot of tests for this >>>>>> driver. >>>>>> >>>>>> This driver probably could use some TLC. >>>>> >>>>> I totally agree with that, however this change fixes at least one test >>>>> of 'v4l2-compliance'. >>>>> Currently a running UVC connection can be corrupted by another process >>>>> which just opens and closes the device. >>>>> >>>>> Thomas >>>>> >>>>>> >>>>>> Regards, >>>>>> >>>>>> Hans >>>>>> >>>>>>> + >>>>>>> + 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); >>>>>>> >>>>>> >>>> >> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-10 16:01 ` Hans Verkuil @ 2020-11-16 15:31 ` Laurent Pinchart 2020-11-16 15:48 ` Hans Verkuil 0 siblings, 1 reply; 32+ messages in thread From: Laurent Pinchart @ 2020-11-16 15:31 UTC (permalink / raw) To: Hans Verkuil Cc: Thomas Hämmerle, gregkh, balbi, linux-usb, m.tretter, linux-media Hello everybody, Sorry for chiming in late. On Tue, Nov 10, 2020 at 05:01:31PM +0100, Hans Verkuil wrote: > On 10/11/2020 16:50, Thomas Hämmerle wrote: > > On 10.11.20 16:36, Hans Verkuil wrote: > >> On 10/11/2020 16:10, Thomas Hämmerle wrote: > >>> On 10.11.20 15:43, Hans Verkuil wrote: > >>>> On 10/11/2020 12:53, Thomas Hämmerle wrote: > >>>>> On 10.11.20 11:31, Hans Verkuil wrote: > >>>>>> On 10/11/2020 09:25, 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; > >>>>>>> }; > >>>>>>> > >>>>>>> #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); > >>>>>>> + } > >>>>>> > >>>>>> This makes no sense. Why would subscribing to a SETUP event > >>>>>> mean that you are 'connected'? > >>>>> > >>>>> Subscribing to a SETUP does not mean that you are connected - on a > >>>>> subscription to SETUP we can expect that there is a userspace process, > >>>>> utilizing UVC -- which is required for the UVC gadget function -- and > >>>>> everything is ready to enable the function by calling uvc_function_connect. > >>>>> How about calling it `function_connected`? > >>>> > >>>> Any application can open the device node and subscribe to this event. > >>>> This is not a good place to call uvc_function_connect(), it's an abuse of > >>>> the V4L2 API. > >>> > >>> Of course - any application can subscribe to this event but since the > >>> event is defined in g_uvc.h I thought that a subscription is most likely > >>> done by a UVC application. I agree with Thomas here, subscribing to UVC_EVENT_SETUP means that the application is a UVC gadget application. It's a bit of a hack, but short of introducing a custom ioctl, I think it's a fairly good option. > >>>> I dug a bit more into this (I have very little gadget driver experience), > >>>> and isn't calling uvc_function_connect() something that should be done > >>>> on the first open? And when the last filehandle is closed, then > >>>> uvc_function_disconnect() can be called to clean up? > >>> > >>> Isn't the possibility of the delayed activation of the usb function in > >>> composite.c intended to wait for a corresponding userspace application, > >>> which is required to make the gadget work? > >>> > >>> If so, a simple open of the v4l2 device does not mean that the gadget is > >>> ready to go: starting pipewire for example results in quering the device > >>> capabilities of all devices. But this does not mean that the gadget will > >>> work. > >>> Therefore I was looking for a place, where we can expect, that the > >>> application opened the device will utilize UVC and found that a > >>> subscription to a UVC specific event is the right place. > >> > >> That's why I suggested to do this when buffers are allocated. That's when > >> the application commits resources to actually use the device. Until that > >> point everything that an application does is just querying or configuring, > >> but not actually using it. > > > > The userspace application implementations (including the one from > > Laurent Pinchart) usually request buffers on the UVC events sent by the > > host. Therefore we would run into chicken-and-egg problem. > > Ah, stupid of me. You're right. > > For now I go with calling uvc_function_connect() on the first open. If > nothing else, this is compatible with the current situation, while still > allowing for multiple opens. The problem with connecting on first open is that any application querying the device (v4l_id from udev, pipewire as Thomas mentioned, ...) will generate spurious connect/disconnect events. This being said, the current code already suffers from the spurious connect/disconnect issue, so we could possibly fix the race condition first with a patch that calls uvc_function_connect() on the first open and uvc_function_disconnect() on last close, and then move the logic to event subscription in a separate patch. Not sure it would be worth it though. > >> BTW, Laurent Pinchart who maintains this driver is away for a week, so he > >> might know more about this when he comes back. > > > > Okay, so I suggest to wait for him. > > > > Thanks for the review so far! > > > >>>> That would make much more sense. Grep for v4l2_fh_is_singular_file() on how > >>>> to do this, quite a few media drivers need this. > >>>> > >>>>>> It should be possible to open a V4L2 device node any number of times, > >>>>>> and any filehandle can subscribe to any event, but typically once > >>>>>> userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) > >>>>>> then that filehandle is marked as the owner of the device and other > >>>>>> open filehandles are no longer allowed to allocate buffers or stream video. > >>>>> > >>>>> Sure - this can be also done if userspace allocates buffers. > >>>>> But why does it make more sense to call uvc_function_connect on > >>>>> VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS instead of subscribtion to a UVC event? > >>>>> > >>>>>> See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c > >>>>>> and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. > >>>>>> > >>>>>> Unfortunately this UVC gadget driver is rather old and is not using > >>>>>> these helper functions. > >>>>>> > >>>>>> Running 'v4l2-compliance' will likely fail on a lot of tests for this > >>>>>> driver. I don't think this driver will ever be able to pass v4l2-compliance as it's not a typical V4L2 output device. It uses V4L2 as that was the best match for a userspace API, but custom ioctls and events are required to drive the device, and the userspace application needs to work hand-in-hand with the driver to respond to UVC requests originating from the host. No other V4L2 application will be able to use the device. This being said, the driver is indeed old, and I'm sure improvements are possible, but full v4l2-compliance isn't a reasonable goal. > >>>>>> This driver probably could use some TLC. > >>>>> > >>>>> I totally agree with that, however this change fixes at least one test > >>>>> of 'v4l2-compliance'. > >>>>> Currently a running UVC connection can be corrupted by another process > >>>>> which just opens and closes the device. > >>>>> > >>>>>>> + > >>>>>>> + 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); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-16 15:31 ` Laurent Pinchart @ 2020-11-16 15:48 ` Hans Verkuil 2020-11-16 15:50 ` Laurent Pinchart 0 siblings, 1 reply; 32+ messages in thread From: Hans Verkuil @ 2020-11-16 15:48 UTC (permalink / raw) To: Laurent Pinchart Cc: Thomas Hämmerle, gregkh, balbi, linux-usb, m.tretter, linux-media On 16/11/2020 16:31, Laurent Pinchart wrote: > Hello everybody, > > Sorry for chiming in late. > > On Tue, Nov 10, 2020 at 05:01:31PM +0100, Hans Verkuil wrote: >> On 10/11/2020 16:50, Thomas Hämmerle wrote: >>> On 10.11.20 16:36, Hans Verkuil wrote: >>>> On 10/11/2020 16:10, Thomas Hämmerle wrote: >>>>> On 10.11.20 15:43, Hans Verkuil wrote: >>>>>> On 10/11/2020 12:53, Thomas Hämmerle wrote: >>>>>>> On 10.11.20 11:31, Hans Verkuil wrote: >>>>>>>> On 10/11/2020 09:25, 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; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> #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); >>>>>>>>> + } >>>>>>>> >>>>>>>> This makes no sense. Why would subscribing to a SETUP event >>>>>>>> mean that you are 'connected'? >>>>>>> >>>>>>> Subscribing to a SETUP does not mean that you are connected - on a >>>>>>> subscription to SETUP we can expect that there is a userspace process, >>>>>>> utilizing UVC -- which is required for the UVC gadget function -- and >>>>>>> everything is ready to enable the function by calling uvc_function_connect. >>>>>>> How about calling it `function_connected`? >>>>>> >>>>>> Any application can open the device node and subscribe to this event. >>>>>> This is not a good place to call uvc_function_connect(), it's an abuse of >>>>>> the V4L2 API. >>>>> >>>>> Of course - any application can subscribe to this event but since the >>>>> event is defined in g_uvc.h I thought that a subscription is most likely >>>>> done by a UVC application. > > I agree with Thomas here, subscribing to UVC_EVENT_SETUP means that the > application is a UVC gadget application. It's a bit of a hack, but short > of introducing a custom ioctl, I think it's a fairly good option. Yuck. OK, if you do this, then this should be documented, both in the driver and esp. in the public g_uvc.h header. This is completely non-standard and unexpected behavior. It would be nice BTW if all these UVC events are documented in the header. Regards, Hans > >>>>>> I dug a bit more into this (I have very little gadget driver experience), >>>>>> and isn't calling uvc_function_connect() something that should be done >>>>>> on the first open? And when the last filehandle is closed, then >>>>>> uvc_function_disconnect() can be called to clean up? >>>>> >>>>> Isn't the possibility of the delayed activation of the usb function in >>>>> composite.c intended to wait for a corresponding userspace application, >>>>> which is required to make the gadget work? >>>>> >>>>> If so, a simple open of the v4l2 device does not mean that the gadget is >>>>> ready to go: starting pipewire for example results in quering the device >>>>> capabilities of all devices. But this does not mean that the gadget will >>>>> work. >>>>> Therefore I was looking for a place, where we can expect, that the >>>>> application opened the device will utilize UVC and found that a >>>>> subscription to a UVC specific event is the right place. >>>> >>>> That's why I suggested to do this when buffers are allocated. That's when >>>> the application commits resources to actually use the device. Until that >>>> point everything that an application does is just querying or configuring, >>>> but not actually using it. >>> >>> The userspace application implementations (including the one from >>> Laurent Pinchart) usually request buffers on the UVC events sent by the >>> host. Therefore we would run into chicken-and-egg problem. >> >> Ah, stupid of me. You're right. >> >> For now I go with calling uvc_function_connect() on the first open. If >> nothing else, this is compatible with the current situation, while still >> allowing for multiple opens. > > The problem with connecting on first open is that any application > querying the device (v4l_id from udev, pipewire as Thomas mentioned, > ...) will generate spurious connect/disconnect events. > > This being said, the current code already suffers from the spurious > connect/disconnect issue, so we could possibly fix the race condition > first with a patch that calls uvc_function_connect() on the first open > and uvc_function_disconnect() on last close, and then move the logic to > event subscription in a separate patch. Not sure it would be worth it > though. > >>>> BTW, Laurent Pinchart who maintains this driver is away for a week, so he >>>> might know more about this when he comes back. >>> >>> Okay, so I suggest to wait for him. >>> >>> Thanks for the review so far! >>> >>>>>> That would make much more sense. Grep for v4l2_fh_is_singular_file() on how >>>>>> to do this, quite a few media drivers need this. >>>>>> >>>>>>>> It should be possible to open a V4L2 device node any number of times, >>>>>>>> and any filehandle can subscribe to any event, but typically once >>>>>>>> userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) >>>>>>>> then that filehandle is marked as the owner of the device and other >>>>>>>> open filehandles are no longer allowed to allocate buffers or stream video. >>>>>>> >>>>>>> Sure - this can be also done if userspace allocates buffers. >>>>>>> But why does it make more sense to call uvc_function_connect on >>>>>>> VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS instead of subscribtion to a UVC event? >>>>>>> >>>>>>>> See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c >>>>>>>> and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. >>>>>>>> >>>>>>>> Unfortunately this UVC gadget driver is rather old and is not using >>>>>>>> these helper functions. >>>>>>>> >>>>>>>> Running 'v4l2-compliance' will likely fail on a lot of tests for this >>>>>>>> driver. > > I don't think this driver will ever be able to pass v4l2-compliance as > it's not a typical V4L2 output device. It uses V4L2 as that was the best > match for a userspace API, but custom ioctls and events are required to > drive the device, and the userspace application needs to work > hand-in-hand with the driver to respond to UVC requests originating from > the host. No other V4L2 application will be able to use the device. > > This being said, the driver is indeed old, and I'm sure improvements are > possible, but full v4l2-compliance isn't a reasonable goal. > >>>>>>>> This driver probably could use some TLC. >>>>>>> >>>>>>> I totally agree with that, however this change fixes at least one test >>>>>>> of 'v4l2-compliance'. >>>>>>> Currently a running UVC connection can be corrupted by another process >>>>>>> which just opens and closes the device. >>>>>>> >>>>>>>>> + >>>>>>>>> + 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); > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] usb: gadget: uvc: fix multiple opens 2020-11-16 15:48 ` Hans Verkuil @ 2020-11-16 15:50 ` Laurent Pinchart 0 siblings, 0 replies; 32+ messages in thread From: Laurent Pinchart @ 2020-11-16 15:50 UTC (permalink / raw) To: Hans Verkuil Cc: Thomas Hämmerle, gregkh, balbi, linux-usb, m.tretter, linux-media Hi Hans, On Mon, Nov 16, 2020 at 04:48:21PM +0100, Hans Verkuil wrote: > On 16/11/2020 16:31, Laurent Pinchart wrote: > > On Tue, Nov 10, 2020 at 05:01:31PM +0100, Hans Verkuil wrote: > >> On 10/11/2020 16:50, Thomas Hämmerle wrote: > >>> On 10.11.20 16:36, Hans Verkuil wrote: > >>>> On 10/11/2020 16:10, Thomas Hämmerle wrote: > >>>>> On 10.11.20 15:43, Hans Verkuil wrote: > >>>>>> On 10/11/2020 12:53, Thomas Hämmerle wrote: > >>>>>>> On 10.11.20 11:31, Hans Verkuil wrote: > >>>>>>>> On 10/11/2020 09:25, 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; > >>>>>>>>> }; > >>>>>>>>> > >>>>>>>>> #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); > >>>>>>>>> + } > >>>>>>>> > >>>>>>>> This makes no sense. Why would subscribing to a SETUP event > >>>>>>>> mean that you are 'connected'? > >>>>>>> > >>>>>>> Subscribing to a SETUP does not mean that you are connected - on a > >>>>>>> subscription to SETUP we can expect that there is a userspace process, > >>>>>>> utilizing UVC -- which is required for the UVC gadget function -- and > >>>>>>> everything is ready to enable the function by calling uvc_function_connect. > >>>>>>> How about calling it `function_connected`? > >>>>>> > >>>>>> Any application can open the device node and subscribe to this event. > >>>>>> This is not a good place to call uvc_function_connect(), it's an abuse of > >>>>>> the V4L2 API. > >>>>> > >>>>> Of course - any application can subscribe to this event but since the > >>>>> event is defined in g_uvc.h I thought that a subscription is most likely > >>>>> done by a UVC application. > > > > I agree with Thomas here, subscribing to UVC_EVENT_SETUP means that the > > application is a UVC gadget application. It's a bit of a hack, but short > > of introducing a custom ioctl, I think it's a fairly good option. > > Yuck. > > OK, if you do this, then this should be documented, both in the driver and > esp. in the public g_uvc.h header. This is completely non-standard and > unexpected behavior. > > It would be nice BTW if all these UVC events are documented in the header. Agreed. We have worked on the UVC gadget driver a couple of years ago to fix a few issues, but had to stop abruptely due to the customer going bankrupt. I would really like to find another opportunity to continue this work, as the driver could really do with some love. > >>>>>> I dug a bit more into this (I have very little gadget driver experience), > >>>>>> and isn't calling uvc_function_connect() something that should be done > >>>>>> on the first open? And when the last filehandle is closed, then > >>>>>> uvc_function_disconnect() can be called to clean up? > >>>>> > >>>>> Isn't the possibility of the delayed activation of the usb function in > >>>>> composite.c intended to wait for a corresponding userspace application, > >>>>> which is required to make the gadget work? > >>>>> > >>>>> If so, a simple open of the v4l2 device does not mean that the gadget is > >>>>> ready to go: starting pipewire for example results in quering the device > >>>>> capabilities of all devices. But this does not mean that the gadget will > >>>>> work. > >>>>> Therefore I was looking for a place, where we can expect, that the > >>>>> application opened the device will utilize UVC and found that a > >>>>> subscription to a UVC specific event is the right place. > >>>> > >>>> That's why I suggested to do this when buffers are allocated. That's when > >>>> the application commits resources to actually use the device. Until that > >>>> point everything that an application does is just querying or configuring, > >>>> but not actually using it. > >>> > >>> The userspace application implementations (including the one from > >>> Laurent Pinchart) usually request buffers on the UVC events sent by the > >>> host. Therefore we would run into chicken-and-egg problem. > >> > >> Ah, stupid of me. You're right. > >> > >> For now I go with calling uvc_function_connect() on the first open. If > >> nothing else, this is compatible with the current situation, while still > >> allowing for multiple opens. > > > > The problem with connecting on first open is that any application > > querying the device (v4l_id from udev, pipewire as Thomas mentioned, > > ...) will generate spurious connect/disconnect events. > > > > This being said, the current code already suffers from the spurious > > connect/disconnect issue, so we could possibly fix the race condition > > first with a patch that calls uvc_function_connect() on the first open > > and uvc_function_disconnect() on last close, and then move the logic to > > event subscription in a separate patch. Not sure it would be worth it > > though. > > > >>>> BTW, Laurent Pinchart who maintains this driver is away for a week, so he > >>>> might know more about this when he comes back. > >>> > >>> Okay, so I suggest to wait for him. > >>> > >>> Thanks for the review so far! > >>> > >>>>>> That would make much more sense. Grep for v4l2_fh_is_singular_file() on how > >>>>>> to do this, quite a few media drivers need this. > >>>>>> > >>>>>>>> It should be possible to open a V4L2 device node any number of times, > >>>>>>>> and any filehandle can subscribe to any event, but typically once > >>>>>>>> userspace allocates buffers (VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS) > >>>>>>>> then that filehandle is marked as the owner of the device and other > >>>>>>>> open filehandles are no longer allowed to allocate buffers or stream video. > >>>>>>> > >>>>>>> Sure - this can be also done if userspace allocates buffers. > >>>>>>> But why does it make more sense to call uvc_function_connect on > >>>>>>> VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS instead of subscribtion to a UVC event? > >>>>>>> > >>>>>>>> See e.g. drivers/media/common/videobuf2/videobuf2-v4l2.c > >>>>>>>> and vb2_ioctl_reqbufs and other vb2_ioctl_* functions. > >>>>>>>> > >>>>>>>> Unfortunately this UVC gadget driver is rather old and is not using > >>>>>>>> these helper functions. > >>>>>>>> > >>>>>>>> Running 'v4l2-compliance' will likely fail on a lot of tests for this > >>>>>>>> driver. > > > > I don't think this driver will ever be able to pass v4l2-compliance as > > it's not a typical V4L2 output device. It uses V4L2 as that was the best > > match for a userspace API, but custom ioctls and events are required to > > drive the device, and the userspace application needs to work > > hand-in-hand with the driver to respond to UVC requests originating from > > the host. No other V4L2 application will be able to use the device. > > > > This being said, the driver is indeed old, and I'm sure improvements are > > possible, but full v4l2-compliance isn't a reasonable goal. > > > >>>>>>>> This driver probably could use some TLC. > >>>>>>> > >>>>>>> I totally agree with that, however this change fixes at least one test > >>>>>>> of 'v4l2-compliance'. > >>>>>>> Currently a running UVC connection can be corrupted by another process > >>>>>>> which just opens and closes the device. > >>>>>>> > >>>>>>>>> + > >>>>>>>>> + 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); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2021-10-05 11:37 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-05 10:31 [PATCH] usb: gadget: uvc: fix multiple opens thomas.haemmerle 2020-11-05 10:37 ` Greg KH 2020-11-09 8:59 ` [PATCH v2] " thomas.haemmerle 2020-11-10 8:25 ` thomas.haemmerle 2020-11-10 8:40 ` Greg KH 2020-11-10 9:56 ` Thomas Hämmerle 2020-11-10 10:06 ` Greg KH 2020-11-10 14:30 ` [PATCH v3] " thomas.haemmerle 2020-11-13 13:36 ` Greg KH 2020-11-16 15:48 ` Laurent Pinchart 2020-11-21 12:50 ` Thomas Hämmerle 2020-12-01 19:27 ` [PATCH v4] " Thomas Haemmerle 2020-12-07 8:53 ` Michael Tretter 2021-01-15 12:55 ` Michael Tretter 2021-01-15 13:09 ` Felipe Balbi 2021-02-11 15:04 ` Michael Tretter 2021-10-03 20:13 ` [RESEND PATCH " Michael Grzeschik 2021-10-04 23:53 ` Laurent Pinchart 2021-10-05 10:59 ` Greg KH 2021-10-05 11:06 ` Felipe Balbi 2021-10-05 11:37 ` Greg KH 2021-10-04 23:55 ` [PATCH v3] " Laurent Pinchart 2020-11-10 10:31 ` [PATCH v2] " Hans Verkuil 2020-11-10 11:53 ` Thomas Hämmerle 2020-11-10 14:43 ` Hans Verkuil 2020-11-10 15:10 ` Thomas Hämmerle 2020-11-10 15:36 ` Hans Verkuil 2020-11-10 15:50 ` Thomas Hämmerle 2020-11-10 16:01 ` Hans Verkuil 2020-11-16 15:31 ` Laurent Pinchart 2020-11-16 15:48 ` Hans Verkuil 2020-11-16 15:50 ` Laurent Pinchart
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.