linux-kernel - Re: [PATCH 03/55] media: usb: cx231xx: Stop abusing of min_buffers

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: <44ca55bf-978d-47e8-abd2-8e3adb5071a2@xs4all.nl>
Date:   Tue, 28 Nov 2023 11:18:05 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        mchehab@...nel.org, tfiga@...omium.org, m.szyprowski@...sung.com,
        matt.ranostay@...sulko.com
Cc:     linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linux-staging@...ts.linux.dev, kernel@...labora.com
Subject: Re: [PATCH 03/55] media: usb: cx231xx: Stop abusing of
 min_buffers_needed field

On 27/11/2023 17:54, Benjamin Gaignard wrote:
> 'min_buffers_needed' is suppose to be used to indicate the number
> of buffers needed by DMA engine to start streaming.
> cx231xx driver doesn't use DMA engine and just want to specify
> the minimum number of buffers to allocate when calling VIDIOC_REQBUFS.
> That 'min_reqbufs_allocation' field purpose so use it.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> ---
>  drivers/media/usb/cx231xx/cx231xx-417.c   | 2 +-
>  drivers/media/usb/cx231xx/cx231xx-video.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c
> index 45973fe690b2..66043ed50c8e 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-417.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-417.c
> @@ -1782,7 +1782,7 @@ int cx231xx_417_register(struct cx231xx *dev)
>  	q->ops = &cx231xx_video_qops;
>  	q->mem_ops = &vb2_vmalloc_memops;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> -	q->min_buffers_needed = 1;
> +	q->min_reqbufs_allocation = 1;

There is no point setting min_reqbufs_allocation to 1: you can't allocate
less than 1 buffer after all.

It is different in that respect from min_buffers_needed: you can call
VIDIOC_STREAMON (and thus start_streaming) without any buffers queued.

This also suggests a better name for min_buffers_needed: min_queued_buffers

So 'min_queued_buffers' buffers have to be queued before start_streaming can
be called.

The old min_buffers_needed mixed up the two requirements of the minimum
number of buffers to allocate in REQBUFS and the minimum number of buffers
that need to be queued before you can start streaming. Separating these two
meanings should make things easier to understand.

The only relationship between the two is that min_reqbufs_allocation >
min_queued_buffers, otherwise you would end up in a state where the
driver would just cycle buffers and never be able to return a buffer
to userspace to process.

Regards,

	Hans

>  	q->lock = &dev->lock;
>  	err = vb2_queue_init(q);
>  	if (err)
> diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c
> index c8eb4222319d..df572c466bfb 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-video.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-video.c
> @@ -1811,7 +1811,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev)
>  	q->ops = &cx231xx_video_qops;
>  	q->mem_ops = &vb2_vmalloc_memops;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> -	q->min_buffers_needed = 1;
> +	q->min_reqbufs_allocation = 1;
>  	q->lock = &dev->lock;
>  	ret = vb2_queue_init(q);
>  	if (ret)
> @@ -1871,7 +1871,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev)
>  	q->ops = &cx231xx_vbi_qops;
>  	q->mem_ops = &vb2_vmalloc_memops;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> -	q->min_buffers_needed = 1;
> +	q->min_reqbufs_allocation = 1;
>  	q->lock = &dev->lock;
>  	ret = vb2_queue_init(q);
>  	if (ret)