diff mbox series

[v7,07/49] media: sti: hva: Use vb2_get_buffer() instead of directly access to buffers array

Message ID 20230914133323.198857-8-benjamin.gaignard@collabora.com
State New
Headers
Series Add DELETE_BUF ioctl |

Commit Message

Benjamin Gaignard Sept. 14, 2023, 1:32 p.m. UTC 
  Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
This could allow to change the type bufs[] field of vb2_buffer structure if
needed.
After each call to vb2_get_buffer() we need to be sure that we get
a valid pointer so check the return value of all of them.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 drivers/media/platform/st/sti/hva/hva-v4l2.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Hans Verkuil Sept. 19, 2023, 9:31 a.m. UTC | #1 
On 14/09/2023 15:32, Benjamin Gaignard wrote:
> Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
> This could allow to change the type bufs[] field of vb2_buffer structure if
> needed.
> After each call to vb2_get_buffer() we need to be sure that we get
> a valid pointer so check the return value of all of them.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  drivers/media/platform/st/sti/hva/hva-v4l2.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/st/sti/hva/hva-v4l2.c b/drivers/media/platform/st/sti/hva/hva-v4l2.c
> index 3a848ca32a0e..326be09bdb55 100644
> --- a/drivers/media/platform/st/sti/hva/hva-v4l2.c
> +++ b/drivers/media/platform/st/sti/hva/hva-v4l2.c
> @@ -577,6 +577,10 @@ static int hva_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
>  		}

Above this line there is a buf->index check...

>  
>  		vb2_buf = vb2_get_buffer(vq, buf->index);
> +		if (!vb2_buf) {
> +			dev_dbg(dev, "%s buffer index %d not found\n", ctx->name, buf->index);
> +			return -EINVAL;
> +		}

...I think that check can be dropped since vb2_get_buffer checks that already.

Regards,

	Hans

>  		stream = to_hva_stream(to_vb2_v4l2_buffer(vb2_buf));
>  		stream->bytesused = buf->bytesused;
>  	}
Benjamin Gaignard Sept. 19, 2023, 10:26 a.m. UTC | #2 
Le 19/09/2023 à 11:31, Hans Verkuil a écrit :
> On 14/09/2023 15:32, Benjamin Gaignard wrote:
>> Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
>> This could allow to change the type bufs[] field of vb2_buffer structure if
>> needed.
>> After each call to vb2_get_buffer() we need to be sure that we get
>> a valid pointer so check the return value of all of them.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   drivers/media/platform/st/sti/hva/hva-v4l2.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/platform/st/sti/hva/hva-v4l2.c b/drivers/media/platform/st/sti/hva/hva-v4l2.c
>> index 3a848ca32a0e..326be09bdb55 100644
>> --- a/drivers/media/platform/st/sti/hva/hva-v4l2.c
>> +++ b/drivers/media/platform/st/sti/hva/hva-v4l2.c
>> @@ -577,6 +577,10 @@ static int hva_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
>>   		}
> Above this line there is a buf->index check...
>
>>   
>>   		vb2_buf = vb2_get_buffer(vq, buf->index);
>> +		if (!vb2_buf) {
>> +			dev_dbg(dev, "%s buffer index %d not found\n", ctx->name, buf->index);
>> +			return -EINVAL;
>> +		}
> ...I think that check can be dropped since vb2_get_buffer checks that already.

No because in "media: sti: hva: Stop direct calls to queue num_buffers field" patch
I remove the above check since it use queue num_buffers field.

Regards,
Benjamin

>
> Regards,
>
> 	Hans
>
>>   		stream = to_hva_stream(to_vb2_v4l2_buffer(vb2_buf));
>>   		stream->bytesused = buf->bytesused;
>>   	}
>
Hans Verkuil Sept. 19, 2023, 11:20 a.m. UTC | #3 
On 19/09/2023 12:26, Benjamin Gaignard wrote:
> 
> Le 19/09/2023 à 11:31, Hans Verkuil a écrit :
>> On 14/09/2023 15:32, Benjamin Gaignard wrote:
>>> Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
>>> This could allow to change the type bufs[] field of vb2_buffer structure if
>>> needed.
>>> After each call to vb2_get_buffer() we need to be sure that we get
>>> a valid pointer so check the return value of all of them.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>> ---
>>>   drivers/media/platform/st/sti/hva/hva-v4l2.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/st/sti/hva/hva-v4l2.c b/drivers/media/platform/st/sti/hva/hva-v4l2.c
>>> index 3a848ca32a0e..326be09bdb55 100644
>>> --- a/drivers/media/platform/st/sti/hva/hva-v4l2.c
>>> +++ b/drivers/media/platform/st/sti/hva/hva-v4l2.c
>>> @@ -577,6 +577,10 @@ static int hva_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
>>>           }
>> Above this line there is a buf->index check...
>>
>>>             vb2_buf = vb2_get_buffer(vq, buf->index);
>>> +        if (!vb2_buf) {
>>> +            dev_dbg(dev, "%s buffer index %d not found\n", ctx->name, buf->index);
>>> +            return -EINVAL;
>>> +        }
>> ...I think that check can be dropped since vb2_get_buffer checks that already.
> 
> No because in "media: sti: hva: Stop direct calls to queue num_buffers field" patch
> I remove the above check since it use queue num_buffers field.

Why not do that here? And drop that later patch? It doesn't make sense to
split it up, and also the commit log of patch 33/49 doesn't match that patch.

Regards,

	Hans

> 
> Regards,
> Benjamin
> 
>>
>> Regards,
>>
>>     Hans
>>
>>>           stream = to_hva_stream(to_vb2_v4l2_buffer(vb2_buf));
>>>           stream->bytesused = buf->bytesused;
>>>       }
>>
diff mbox series

Patch

diff --git a/drivers/media/platform/st/sti/hva/hva-v4l2.c b/drivers/media/platform/st/sti/hva/hva-v4l2.c
index 3a848ca32a0e..326be09bdb55 100644
--- a/drivers/media/platform/st/sti/hva/hva-v4l2.c
+++ b/drivers/media/platform/st/sti/hva/hva-v4l2.c
@@ -577,6 +577,10 @@  static int hva_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
 		}
 
 		vb2_buf = vb2_get_buffer(vq, buf->index);
+		if (!vb2_buf) {
+			dev_dbg(dev, "%s buffer index %d not found\n", ctx->name, buf->index);
+			return -EINVAL;
+		}
 		stream = to_hva_stream(to_vb2_v4l2_buffer(vb2_buf));
 		stream->bytesused = buf->bytesused;
 	}