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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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;
>>>   };
>>>   
>>>   #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);
>>>
>>




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux