Re: [PATCH v4] usb: gadget: uvc: fix multiple opens

[Michael Tretter <m.tretter@pengutronix.de>]

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
> > 
> > 
>