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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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.

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 }; ?

>   	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.

>   	/* 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.

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;