| Message ID | 20230914133323.198857-8-benjamin.gaignard@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v7,01/49] media: videobuf2: Rework offset 'cookie' encoding pattern | expand |
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; > }
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; >> } >
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 --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; }
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(+)