| Message ID | 20180804124526.46206-9-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Request API | expand |
Em Sat, 4 Aug 2018 14:45:00 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > From: Hans Verkuil <hans.verkuil@cisco.com> > > 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@cisco.com> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > 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); } > 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. > > 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
On 08/09/2018 10:03 PM, Mauro Carvalho Chehab wrote: > Em Sat, 4 Aug 2018 14:45:00 +0200 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> 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@cisco.com> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> --- >> 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). > >> 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. > >> >> 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? Regards, Hans > >> return ret; >> } >> > > > > Thanks, > Mauro >
Em Fri, 10 Aug 2018 09:39:20 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 08/09/2018 10:03 PM, Mauro Carvalho Chehab wrote: > > Em Sat, 4 Aug 2018 14:45:00 +0200 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > >> From: Hans Verkuil <hans.verkuil@cisco.com> > >> > >> 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@cisco.com> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> --- > >> 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
On 09/08/18 22:03, Mauro Carvalho Chehab wrote: > Em Sat, 4 Aug 2018 14:45:00 +0200 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> 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@cisco.com> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> --- >> 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 >
Em Tue, 14 Aug 2018 14:00:16 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 09/08/18 22:03, Mauro Carvalho Chehab wrote: > > Em Sat, 4 Aug 2018 14:45:00 +0200 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > >> From: Hans Verkuil <hans.verkuil@cisco.com> > >> > >> 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@cisco.com> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> --- > >> 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. Works for me. Thanks, Mauro
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); + 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; + } 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); return ret; }