diff mbox series

[8/9] media: vb2: vb2_core_queue_init(): sanity check lock and wait_prepare/finish

Message ID 8eb606d30e33ccee7256a329f9d4a31793864e29.1725285495.git.hverkuil-cisco@xs4all.nl (mailing list archive)
State New
Delegated to: Hans Verkuil
Headers
Series media: vb2: prepare for vb2_ops_wait_prepare/finish removal |

Commit Message

Hans Verkuil Sept. 2, 2024, 2:04 p.m. UTC 
  Add two new checks:

1) wait_prepare and wait_finish callbacks are either both present or
   both unset, you can't mix.
2) if lock == NULL, then wait_prepare (and due to check 1 also
   wait_finish) must be present.

These checks should prevent the case where lock == NULL, but there
is no way to release/reacquire whatever lock is used when waiting
for a buffer to arrive in VIDIOC_DQBUF.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Laurent Pinchart Sept. 9, 2024, 2:59 p.m. UTC | #1 
Hi Hans,

Thank you for the patch.

On Mon, Sep 02, 2024 at 04:04:54PM +0200, Hans Verkuil wrote:
> Add two new checks:
> 
> 1) wait_prepare and wait_finish callbacks are either both present or
>    both unset, you can't mix.
> 2) if lock == NULL, then wait_prepare (and due to check 1 also
>    wait_finish) must be present.
> 
> These checks should prevent the case where lock == NULL, but there
> is no way to release/reacquire whatever lock is used when waiting
> for a buffer to arrive in VIDIOC_DQBUF.

Don't we use the video_device lock when the queue lock is NULL ?
Shouldn't it be valid to not set wait_prepare and wait_finish in that
case ?

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 29a8d876e6c2..6335ac7b771a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -2644,6 +2644,14 @@ int vb2_core_queue_init(struct vb2_queue *q)
>  	if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers))
>  		return -EINVAL;
>  
> +	/* Either both or none are set */
> +	if (WARN_ON(!q->ops->wait_prepare ^ !q->ops->wait_finish))
> +		return -EINVAL;
> +
> +	/* Warn if q->lock is NULL and no custom wait_prepare is provided */
> +	if (WARN_ON(!q->lock && !q->ops->wait_prepare))
> +		return -EINVAL;
> +
>  	INIT_LIST_HEAD(&q->queued_list);
>  	INIT_LIST_HEAD(&q->done_list);
>  	spin_lock_init(&q->done_lock);
Hans Verkuil Sept. 10, 2024, 11:08 a.m. UTC | #2 
On 09/09/2024 16:59, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Mon, Sep 02, 2024 at 04:04:54PM +0200, Hans Verkuil wrote:
>> Add two new checks:
>>
>> 1) wait_prepare and wait_finish callbacks are either both present or
>>    both unset, you can't mix.
>> 2) if lock == NULL, then wait_prepare (and due to check 1 also
>>    wait_finish) must be present.
>>
>> These checks should prevent the case where lock == NULL, but there
>> is no way to release/reacquire whatever lock is used when waiting
>> for a buffer to arrive in VIDIOC_DQBUF.
> 
> Don't we use the video_device lock when the queue lock is NULL ?
> Shouldn't it be valid to not set wait_prepare and wait_finish in that
> case ?

If q->lock is NULL, then vb2 doesn't know which mutex needs to be
unlocked while waiting for a buffer to arrive (and reacquired afterwards).
So in that case you need to provide a function. Remember that vb2 doesn't
know about the video_device lock. It is the driver that normally fills in
q->lock, and often (or even always?) that is indeed the video_device lock.

If q->lock is NULL, and these functions are not provided, then that is
almost certainly a bug since there is almost certainly some serialization
mutex. And if you don't unlock that while waiting, then you can't issue
other ioctls. In the unlikely event that there really is no mutex involved,
then you have to supply empty functions. There is no driver like that, though.

Regards,

	Hans

> 
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 29a8d876e6c2..6335ac7b771a 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -2644,6 +2644,14 @@ int vb2_core_queue_init(struct vb2_queue *q)
>>  	if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers))
>>  		return -EINVAL;
>>  
>> +	/* Either both or none are set */
>> +	if (WARN_ON(!q->ops->wait_prepare ^ !q->ops->wait_finish))
>> +		return -EINVAL;
>> +
>> +	/* Warn if q->lock is NULL and no custom wait_prepare is provided */
>> +	if (WARN_ON(!q->lock && !q->ops->wait_prepare))
>> +		return -EINVAL;
>> +
>>  	INIT_LIST_HEAD(&q->queued_list);
>>  	INIT_LIST_HEAD(&q->done_list);
>>  	spin_lock_init(&q->done_lock);
>
diff mbox series

Patch

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 29a8d876e6c2..6335ac7b771a 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2644,6 +2644,14 @@  int vb2_core_queue_init(struct vb2_queue *q)
 	if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers))
 		return -EINVAL;
 
+	/* Either both or none are set */
+	if (WARN_ON(!q->ops->wait_prepare ^ !q->ops->wait_finish))
+		return -EINVAL;
+
+	/* Warn if q->lock is NULL and no custom wait_prepare is provided */
+	if (WARN_ON(!q->lock && !q->ops->wait_prepare))
+		return -EINVAL;
+
 	INIT_LIST_HEAD(&q->queued_list);
 	INIT_LIST_HEAD(&q->done_list);
 	spin_lock_init(&q->done_lock);