linux-kernel - Re: [PATCH v18 1/9] media: videobuf2: Update vb2_is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fc39d229-67ca-4c6e-b76d-1d71d24b4daa@xs4all.nl>
Date: Mon, 5 Feb 2024 14:59:06 +0100
From: Hans Verkuil <hverkuil@...all.nl>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>, mchehab@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 kernel@...labora.com
Subject: Re: [PATCH v18 1/9] media: videobuf2: Update vb2_is_busy() logic

On 26/01/2024 12:01, Benjamin Gaignard wrote:
> Do not rely on the number of allocated buffers to know if the
> queue is busy but on flag set when at least buffer has been allocated

flag -> a flag
buffer -> one buffer

> by REQBUFS or CREATE_BUFS ioctl.
> The flag is reset when REQBUFS is called with count = 0 or close the
> file handle.

close the file handle -> the file handle is closed

> This is needed because delete buffers feature will be able to remove
> all the buffers from a queue while streaming so relying in the number

in -> on

> of allocated buffers in the queue won't be possible.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 6 +++++-
>  include/media/videobuf2-core.h                  | 4 +++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index b6bf8f232f48..8aa6057df581 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -858,8 +858,10 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  		 * In case of REQBUFS(0) return immediately without calling
>  		 * driver's queue_setup() callback and allocating resources.
>  		 */
> -		if (*count == 0)
> +		if (*count == 0) {
> +			q->is_busy = 0;

No, this line should actually go before the 'if'.

VIDIOC_REQBUFS is a bit odd: if you call it with count > 0, then
it will first delete all existing buffers and implicitly go out
of the 'busy' state. Then it attempts to allocate the new buffers,
and if that is successful, then it is back to the busy state.

So regardless of the count value, you need to set is_busy to 0 here.

>  			return 0;
> +		}
>  	}
>  
>  	/*
> @@ -966,6 +968,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  	 */
>  	*count = allocated_buffers;
>  	q->waiting_for_buffers = !q->is_output;
> +	q->is_busy = 1;
>  
>  	return 0;
>  
> @@ -1091,6 +1094,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  	 * to the userspace.
>  	 */
>  	*count = allocated_buffers;
> +	q->is_busy = 1;
>  
>  	return 0;
>  

You missed vb2_core_queue_release(): that's called when the file handle is closed,
so that should set q->is_busy to 0 as well.

> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 56719a26a46c..b317286a7b08 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -579,6 +579,7 @@ struct vb2_buf_ops {
>   *		called since poll() needs to return %EPOLLERR in that situation.
>   * @is_multiplanar: set if buffer type is multiplanar
>   * @is_output:	set if buffer type is output
> + * @is_busy:	set if at least one buffer has been allocated at some time.
>   * @copy_timestamp: set if vb2-core should set timestamps
>   * @last_buffer_dequeued: used in poll() and DQBUF to immediately return if the
>   *		last decoded buffer was already dequeued. Set for capture queues
> @@ -644,6 +645,7 @@ struct vb2_queue {
>  	unsigned int			waiting_in_dqbuf:1;
>  	unsigned int			is_multiplanar:1;
>  	unsigned int			is_output:1;
> +	unsigned int			is_busy:1;
>  	unsigned int			copy_timestamp:1;
>  	unsigned int			last_buffer_dequeued:1;
>  
> @@ -1163,7 +1165,7 @@ static inline unsigned int vb2_get_num_buffers(struct vb2_queue *q)
>   */
>  static inline bool vb2_is_busy(struct vb2_queue *q)
>  {
> -	return vb2_get_num_buffers(q) > 0;
> +	return !!q->is_busy;
>  }
>  
>  /**

Regards,

	Hans