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?
> };
>
> #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
