Hi Hans,
On Mon, Jun 4, 2018 at 8:48 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
[snip]
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 965fd301f617..27a893aa0664 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2714,6 +2714,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;
> @@ -2733,10 +2734,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)) {
Are STREAMON and STREAMOFF the only ioctls we need to acquire
req_queue_lock for? How about a race with, for example, S_CTRL(control
X, request_fd = -1) and req_validate() on a request that depends on
the value of control X? Maybe we should just lock here for any ioctl?
> + req_queue_lock = &vfd->v4l2_dev->mdev->req_queue_mutex;
> +
> + if (req_queue_lock && mutex_lock_interruptible(req_queue_lock))
I believe it isn't possible for req_queue_lock to be NULL here.
> + return -ERESTARTSYS;
I guess it isn't really possible for mutex_lock_interruptible() to
return anything non-zero other than this, but I'd still return what it
returns here just in case.
Best regards,
Tomasz
