On 09/08/18 22:03, Mauro Carvalho Chehab wrote:
> Em Sat, 4 Aug 2018 14:45:00 +0200
> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> We need to serialize streamon/off with queueing new requests.
>> These ioctls may trigger the cancellation of a streaming
>> operation, and that should not be mixed with queuing a new
>> request at the same time.
>>
>> Finally close() needs this lock since that too can trigger the
>> cancellation of a streaming operation.
>>
>> We take the req_queue_mutex here before any other locks since
>> it is a very high-level lock.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>> ---
>> drivers/media/v4l2-core/v4l2-dev.c | 13 +++++++++++++
>> drivers/media/v4l2-core/v4l2-ioctl.c | 22 +++++++++++++++++++++-
>> 2 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 69e775930fc4..53018e4a4c78 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -444,8 +444,21 @@ static int v4l2_release(struct inode *inode, struct file *filp)
>> struct video_device *vdev = video_devdata(filp);
>> int ret = 0;
>>
>> + /*
>> + * We need to serialize the release() with queueing new requests.
>> + * The release() may trigger the cancellation of a streaming
>> + * operation, and that should not be mixed with queueing a new
>> + * request at the same time.
>> + */
>> + if (v4l2_device_supports_requests(vdev->v4l2_dev))
>> + mutex_lock(&vdev->v4l2_dev->mdev->req_queue_mutex);
>> +
>> if (vdev->fops->release)
>> ret = vdev->fops->release(filp);
>> +
>> + if (v4l2_device_supports_requests(vdev->v4l2_dev))
>> + mutex_unlock(&vdev->v4l2_dev->mdev->req_queue_mutex);
>> +
>
> This will very likely generate sparse warnings. See my discussions
> with that regards with Linus.
>
> The only way to avoid it would be to do something like:
>
> if (v4l2_device_supports_requests(vdev->v4l2_dev)) {
> mutex_lock(&vdev->v4l2_dev->mdev->req_queue_mutex);
> if (vdev->fops->release)
> ret = vdev->fops->release(filp);
> mutex_unlock(&vdev->v4l2_dev->mdev->req_queue_mutex);
> } else {
> if (vdev->fops->release)
> ret = vdev->fops->release(filp);
> }
I've changed this.
>
>> if (vdev->dev_debug & V4L2_DEV_DEBUG_FOP)
>> dprintk("%s: release\n",
>> video_device_node_name(vdev));
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 54afc9c7ee6e..ea475d833dd6 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -2780,6 +2780,7 @@ static long __video_do_ioctl(struct file *file,
>> unsigned int cmd, void *arg)
>> {
>> struct video_device *vfd = video_devdata(file);
>> + struct mutex *req_queue_lock = NULL;
>> struct mutex *lock; /* ioctl serialization mutex */
>> const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops;
>> bool write_only = false;
>> @@ -2799,10 +2800,27 @@ static long __video_do_ioctl(struct file *file,
>> if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
>> vfh = file->private_data;
>>
>> + /*
>> + * We need to serialize streamon/off with queueing new requests.
>> + * These ioctls may trigger the cancellation of a streaming
>> + * operation, and that should not be mixed with queueing a new
>> + * request at the same time.
>> + */
>> + if (v4l2_device_supports_requests(vfd->v4l2_dev) &&
>> + (cmd == VIDIOC_STREAMON || cmd == VIDIOC_STREAMOFF)) {
>> + req_queue_lock = &vfd->v4l2_dev->mdev->req_queue_mutex;
>> +
>> + if (mutex_lock_interruptible(req_queue_lock))
>> + return -ERESTARTSYS;
>> + }
>> +
>> lock = v4l2_ioctl_get_lock(vfd, vfh, cmd, arg);
>>
>> - if (lock && mutex_lock_interruptible(lock))
>> + if (lock && mutex_lock_interruptible(lock)) {
>> + if (req_queue_lock)
>> + mutex_unlock(req_queue_lock);
>> return -ERESTARTSYS;
>> + }
>
> Same applies here.
I've kept this. I don't get any sparse warnings for this, but if they appear
then the only way to fix it is likely to split this function into two,
one that takes the lock(s) and one unlocked variant that does the actual
job.
If we need that, then that's a separate patch on top of this patch series.
Regards,
Hans
>
>>
>> if (!video_is_registered(vfd)) {
>> ret = -ENODEV;
>> @@ -2861,6 +2879,8 @@ static long __video_do_ioctl(struct file *file,
>> unlock:
>> if (lock)
>> mutex_unlock(lock);
>> + if (req_queue_lock)
>> + mutex_unlock(req_queue_lock);
>
> This code looks really weird! are you locking in order to get a
> lock pointer?
>
> That seems broken by design.
>
>> return ret;
>> }
>>
>
>
>
> Thanks,
> Mauro
>
