Em Fri, 10 Aug 2018 09:39:20 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> On 08/09/2018 10:03 PM, 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'll check what sparse says and make this change if needed (I hate
> working around sparse warnings).
For reference, see the discussions I had with Linus and Christopher
about this at sparse ML:
https://www.spinics.net/lists/linux-sparse/msg08069.html
In particular, see this:
https://www.spinics.net/lists/linux-sparse/msg08071.html
(I thought I had c/c media, but it seems that only sparse ML was
c/c).
>
> >
> >> 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'm not sure there is much that can be done here without making the
> code hard to read. I'll see.
You can have a lock-free routine called by another one with
would set the lock if req_queue_lock.
We ended by doing that at the ddbridge driver.
>
> >
> >>
> >> 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.
>
> I've no idea what you mean. Both 'lock' and 'req_queue_lock' are pointers to
> a struct mutex. If NULL, don't unlock, otherwise you need to unlock the mutex
> here since it was locked earlier.
>
> Did you misread this or should the lock/req_queue_lock names be changed to e.g.
> mutex_ptr/req_queue_mutex_ptr?
Yeah, I misread it. Sorry for the noise.
IMO, it is worth to mention at the req_queue_lock field documentation that
all ioctls calls will hold the req_queue_lock.
>
> Regards,
>
> Hans
>
> >
> >> return ret;
> >> }
> >>
> >
> >
> >
> > Thanks,
> > Mauro
> >
>
Thanks,
Mauro
