On 10.11.20 09:40, Greg KH wrote:
On Tue, Nov 10, 2020 at 09:25:04AM +0100, thomas.haemmerle@xxxxxxxxxxxxxx wrote:
From: Thomas Haemmerle <thomas.haemmerle@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
---
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
