Re: [PATCH] media: mediatek: encoder: memset encoder structure data

["Irui Wang (王瑞)" <Irui.Wang@mediatek.com>]

Dear Angelo,

Thanks for your reviewing.

On Tue, 2025-07-15 at 15:26 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 15/07/25 10:15, Irui Wang ha scritto:
> > Utilized memset to set all bytes of encoder structure to zero,
> > this prevents any undefined behavior due to uninitialized use.
> > 
> > Signed-off-by: Irui Wang <irui.wang@mediatek.com>
> 
> This commit needs a Fixes tag, as you're fixing something important
> here.
> 
> Also, please clarify what is this undefined behavior that you've seen
> and
> what problem are you trying to resolve by zeroing all those mem
> locations.
OK, fix it in next patch.
> 
> There's also more feedback, check below...
> 
> > ---
> >   .../media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c  | 5
> > +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > index a01dc25a7699..ecac1aec7215 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > @@ -886,6 +886,7 @@ static int vb2ops_venc_start_streaming(struct
> > vb2_queue *q, unsigned int count)
> >                       return 0;
> >       }
> > 
> > +     memset(&param, 0, sizeof(param));
> 
> Have you considered doing, instead...
> 
>         struct venc_enc_param param = { 0 }; ?
OK, fix it in next patch.
> 
> >       mtk_venc_set_param(ctx, &param);
> >       ret = venc_if_set_param(ctx, VENC_SET_PARAM_ENC, &param);
> >       if (ret) {
> > @@ -1021,12 +1022,14 @@ static int mtk_venc_encode_header(void
> > *priv)
> >       struct mtk_vcodec_mem bs_buf;
> >       struct venc_done_result enc_result;
> 
>         struct venc_done_result enc_result = { 0 };
> 
> > 
> > +     memset(&enc_result, 0, sizeof(enc_result));
> >       dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> >       if (!dst_buf) {
> >               mtk_v4l2_venc_dbg(1, ctx, "No dst buffer");
> >               return -EINVAL;
> >       }
> > 
> > +     memset(&bs_buf, 0, sizeof(bs_buf));
> >       bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> >       bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf-
> > >vb2_buf, 0);
> >       bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;
> > @@ -1143,6 +1146,7 @@ static void mtk_venc_worker(struct
> > work_struct *work)
> >       struct venc_done_result enc_result;
> >       int ret, i;
> > 
> > +     memset(&enc_result, 0, sizeof(enc_result));
> 
> You should probably move this to before the first usage, instead.
OK, fix it in next patch.
> 
> >       /* check dst_buf, dst_buf may be removed in device_run
> >        * to stored encdoe header so we need check dst_buf and
> >        * call job_finish here to prevent recursion
> > @@ -1175,6 +1179,7 @@ static void mtk_venc_worker(struct
> > work_struct *work)
> >               frm_buf.fb_addr[i].size =
> >                               (size_t)src_buf-
> > >vb2_buf.planes[i].length;
> >       }
> > +     memset(&bs_buf, 0, sizeof(bs_buf));
> 
> here it's fine to use memset, as there are multiple ways out before
> actually using
> bs_buf.
We will remove the bs_buf memset, because the bs_buf's variables are
set(va/dma_add/size) after definition.

Thanks!
> 
> Cheers,
> Angelo
> 
> >       bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> >       bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf-
> > >vb2_buf, 0);
> >       bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;
> 
>