Re: [PATCHv15 07/35] v4l2-dev: lock req_queue_mutex
On 02/07/18 15:06, Tomasz Figa wrote:
> Hi Hans,
>
> On Mon, Jun 4, 2018 at 8:48 PM Hans Verkuil 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, >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?
Definitely not, that would seriously impact performance since this is such
a high-level lock.
It is indeed possible to set controls directly even if the same control is
also used in a request. It is intentional that you can still set controls
directly: for some controls (e.g. POWERLINE frequency) it makes no sense to
use them in a request and you just want to be able to set them directly. It
is also very useful for debugging. So even though it can potentially mess
things up, it is practical to allow this.
Drivers need to be able to handle such situations anyway since setting
controls from a request can fail regardless due to HW errors of some kind,
so a control might not have the value that you expected.
And worse case there will only be a wrong control value until a request is
applied containing a new value of this control.
It might be that in the future we need to restrict this for selected controls
in some manner, but for now I think this is fine.
Note that this is different for buffers: once you've switched to using requests
to queue buffers, you can no longer queue a buffer directly. Mixing that is
theoretically possible, but it is very confusing.
>
>> + req_queue_lock = >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.
True, I can remove that test.
Regards,
Hans
>
>> + 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.
Re: [PATCHv15 07/35] v4l2-dev: lock req_queue_mutex
Hi Hans,
On Mon, Jun 4, 2018 at 8:48 PM Hans Verkuil 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, >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 = >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
[PATCHv15 07/35] v4l2-dev: lock req_queue_mutex
From: Hans Verkuil
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
Signed-off-by: Sakari Ailus
---
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 4ffd7d60a901..d23e82ab1e27 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(>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(>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 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, >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 = >v4l2_dev->mdev->req_queue_mutex;
+
+ if (req_queue_lock && mutex_lock_interruptible(req_queue_lock))
+ return -ERESTARTSYS;
+ }
+
lock = v4l2_ioctl_get_lock(vfd, cmd);
- 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;
@@ -2795,6 +2813,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;
}
--
2.17.0
