Message-ID: <d76038453bfd06ea9d0c90e7583078abc85ce280.camel@ndufresne.ca>
Date: Fri, 29 Aug 2025 14:49:47 -0400
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Qianfeng Rong <rongqianfeng@...o.com>, Tiffany Lin	
 <tiffany.lin@...iatek.com>, Andrew-CT Chen <andrew-ct.chen@...iatek.com>, 
 Yunfei Dong <yunfei.dong@...iatek.com>, Mauro Carvalho Chehab
 <mchehab@...nel.org>, Matthias Brugger	 <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno	 <angelogioacchino.delregno@...labora.com>, Hans
 Verkuil <hverkuil@...nel.org>,  Andrzej Pietrasiewicz
 <andrzejtp2010@...il.com>, Neil Armstrong <neil.armstrong@...aro.org>,
 linux-media@...r.kernel.org, 	linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, 	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] media: mediatek: vcodec: use = { } instead of memset()

Hi,


Le dimanche 03 août 2025 à 21:55 +0800, Qianfeng Rong a écrit :
> Based on testing and recommendations by David Lechner et al. [1][2],
> using = { } to initialize a structure or array is the preferred way
> to do this in the kernel.
> 
> This patch converts memset() to = { }, thereby:
> - Eliminating the risk of sizeof() mismatches.
> - Simplifying the code.

Last month, Irui Wang sent an actual fix [0] for uninitialized data in this
driver. Your patch seems to be related, yet the previous fix is not covered and
this is not marked as a V2. Since this refactoring collide with an actual fix
that I'm waiting for a V2, I'd rather not take it and wait.

Any chances you can respin this with a second patches covering Irui's fix ?

cheers,
Nicolas


[0] https://patchwork.linuxtv.org/project/linux-media/patch/20250715081547.18076-1-irui.wang@mediatek.com/

> 
> [1]: https://lore.kernel.org/linux-iio/202505090942.48EBF01B@keescook/
> [2]: https://lore.kernel.org/lkml/20250614151844.50524610@jic23-huawei/
> 
> Signed-off-by: Qianfeng Rong <rongqianfeng@...o.com>
> ---
>  .../mediatek/vcodec/decoder/vdec/vdec_vp9_if.c    |  3 +--
>  .../mediatek/vcodec/decoder/vdec_vpu_if.c         | 12 ++++--------
>  .../mediatek/vcodec/encoder/mtk_vcodec_enc.c      |  6 ++----
>  .../mediatek/vcodec/encoder/venc_vpu_if.c         | 15 +++++----------
>  4 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> index eb3354192853..80554b2c26c0 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> @@ -548,10 +548,9 @@ static bool vp9_wait_dec_end(struct vdec_vp9_inst *inst)
>  static struct vdec_vp9_inst *vp9_alloc_inst(struct mtk_vcodec_dec_ctx *ctx)
>  {
>  	int result;
> -	struct mtk_vcodec_mem mem;
> +	struct mtk_vcodec_mem mem = { };
>  	struct vdec_vp9_inst *inst;
>  
> -	memset(&mem, 0, sizeof(mem));
>  	mem.size = sizeof(struct vdec_vp9_inst);
>  	result = mtk_vcodec_mem_alloc(ctx, &mem);
>  	if (result)
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> index 145958206e38..d5e943f81c15 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> @@ -181,12 +181,11 @@ static int vcodec_vpu_send_msg(struct vdec_vpu_inst
> *vpu, void *msg, int len)
>  
>  static int vcodec_send_ap_ipi(struct vdec_vpu_inst *vpu, unsigned int msg_id)
>  {
> -	struct vdec_ap_ipi_cmd msg;
> +	struct vdec_ap_ipi_cmd msg = { };
>  	int err = 0;
>  
>  	mtk_vdec_debug(vpu->ctx, "+ id=%X", msg_id);
>  
> -	memset(&msg, 0, sizeof(msg));
>  	msg.msg_id = msg_id;
>  	if (vpu->fw_abi_version < 2)
>  		msg.vpu_inst_addr = vpu->inst_addr;
> @@ -201,7 +200,7 @@ static int vcodec_send_ap_ipi(struct vdec_vpu_inst *vpu,
> unsigned int msg_id)
>  
>  int vpu_dec_init(struct vdec_vpu_inst *vpu)
>  {
> -	struct vdec_ap_ipi_init msg;
> +	struct vdec_ap_ipi_init msg = { };
>  	int err;
>  
>  	init_waitqueue_head(&vpu->wq);
> @@ -225,7 +224,6 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu)
>  		}
>  	}
>  
> -	memset(&msg, 0, sizeof(msg));
>  	msg.msg_id = AP_IPIMSG_DEC_INIT;
>  	msg.ap_inst_addr = (unsigned long)vpu;
>  	msg.codec_type = vpu->codec_type;
> @@ -245,7 +243,7 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu)
>  
>  int vpu_dec_start(struct vdec_vpu_inst *vpu, uint32_t *data, unsigned int
> len)
>  {
> -	struct vdec_ap_ipi_dec_start msg;
> +	struct vdec_ap_ipi_dec_start msg = { };
>  	int i;
>  	int err = 0;
>  
> @@ -254,7 +252,6 @@ int vpu_dec_start(struct vdec_vpu_inst *vpu, uint32_t
> *data, unsigned int len)
>  		return -EINVAL;
>  	}
>  
> -	memset(&msg, 0, sizeof(msg));
>  	msg.msg_id = AP_IPIMSG_DEC_START;
>  	if (vpu->fw_abi_version < 2)
>  		msg.vpu_inst_addr = vpu->inst_addr;
> @@ -273,7 +270,7 @@ int vpu_dec_start(struct vdec_vpu_inst *vpu, uint32_t
> *data, unsigned int len)
>  int vpu_dec_get_param(struct vdec_vpu_inst *vpu, uint32_t *data,
>  		      unsigned int len, unsigned int param_type)
>  {
> -	struct vdec_ap_ipi_get_param msg;
> +	struct vdec_ap_ipi_get_param msg = { };
>  	int err;
>  
>  	if (len > ARRAY_SIZE(msg.data)) {
> @@ -281,7 +278,6 @@ int vpu_dec_get_param(struct vdec_vpu_inst *vpu, uint32_t
> *data,
>  		return -EINVAL;
>  	}
>  
> -	memset(&msg, 0, sizeof(msg));
>  	msg.msg_id = AP_IPIMSG_DEC_GET_PARAM;
>  	msg.inst_id = vpu->inst_id;
>  	memcpy(msg.data, data, sizeof(unsigned int) * len);
> 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..bc6435a7543f 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> @@ -1064,7 +1064,7 @@ static int mtk_venc_encode_header(void *priv)
>  
>  static int mtk_venc_param_change(struct mtk_vcodec_enc_ctx *ctx)
>  {
> -	struct venc_enc_param enc_prm;
> +	struct venc_enc_param enc_prm = { };
>  	struct vb2_v4l2_buffer *vb2_v4l2 = v4l2_m2m_next_src_buf(ctx-
> >m2m_ctx);
>  	struct mtk_video_enc_buf *mtk_buf;
>  	int ret = 0;
> @@ -1075,7 +1075,6 @@ static int mtk_venc_param_change(struct
> mtk_vcodec_enc_ctx *ctx)
>  
>  	mtk_buf = container_of(vb2_v4l2, struct mtk_video_enc_buf,
> m2m_buf.vb);
>  
> -	memset(&enc_prm, 0, sizeof(enc_prm));
>  	if (mtk_buf->param_change == MTK_ENCODE_PARAM_NONE)
>  		return 0;
>  
> @@ -1138,7 +1137,7 @@ static void mtk_venc_worker(struct work_struct *work)
>  	struct mtk_vcodec_enc_ctx *ctx = container_of(work, struct
> mtk_vcodec_enc_ctx,
>  				    encode_work);
>  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> -	struct venc_frm_buf frm_buf;
> +	struct venc_frm_buf frm_buf = { };
>  	struct mtk_vcodec_mem bs_buf;
>  	struct venc_done_result enc_result;
>  	int ret, i;
> @@ -1168,7 +1167,6 @@ static void mtk_venc_worker(struct work_struct *work)
>  		return;
>  	}
>  
> -	memset(&frm_buf, 0, sizeof(frm_buf));
>  	for (i = 0; i < src_buf->vb2_buf.num_planes ; i++) {
>  		frm_buf.fb_addr[i].dma_addr =
>  				vb2_dma_contig_plane_dma_addr(&src_buf-
> >vb2_buf, i);
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> index 51bb7ee141b9..55627b71348d 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> @@ -132,7 +132,7 @@ static int vpu_enc_send_msg(struct venc_vpu_inst *vpu,
> void *msg,
>  int vpu_enc_init(struct venc_vpu_inst *vpu)
>  {
>  	int status;
> -	struct venc_ap_ipi_msg_init out;
> +	struct venc_ap_ipi_msg_init out = { };
>  
>  	init_waitqueue_head(&vpu->wq_hd);
>  	vpu->signaled = 0;
> @@ -148,7 +148,6 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
>  		return -EINVAL;
>  	}
>  
> -	memset(&out, 0, sizeof(out));
>  	out.msg_id = AP_IPIMSG_ENC_INIT;
>  	out.venc_inst = (unsigned long)vpu;
>  	if (vpu_enc_send_msg(vpu, &out, sizeof(out))) {
> @@ -191,11 +190,10 @@ int vpu_enc_set_param(struct venc_vpu_inst *vpu,
>  	size_t msg_size = is_ext ?
>  		sizeof(struct venc_ap_ipi_msg_set_param_ext) :
>  		sizeof(struct venc_ap_ipi_msg_set_param);
> -	struct venc_ap_ipi_msg_set_param_ext out;
> +	struct venc_ap_ipi_msg_set_param_ext out = { };
>  
>  	mtk_venc_debug(vpu->ctx, "id %d ->", id);
>  
> -	memset(&out, 0, sizeof(out));
>  	out.base.msg_id = AP_IPIMSG_ENC_SET_PARAM;
>  	out.base.vpu_inst_addr = vpu->inst_addr;
>  	out.base.param_id = id;
> @@ -258,11 +256,10 @@ static int vpu_enc_encode_32bits(struct venc_vpu_inst
> *vpu,
>  	size_t msg_size = is_ext ?
>  		sizeof(struct venc_ap_ipi_msg_enc_ext) :
>  		sizeof(struct venc_ap_ipi_msg_enc);
> -	struct venc_ap_ipi_msg_enc_ext out;
> +	struct venc_ap_ipi_msg_enc_ext out = { };
>  
>  	mtk_venc_debug(vpu->ctx, "bs_mode %d ->", bs_mode);
>  
> -	memset(&out, 0, sizeof(out));
>  	out.base.msg_id = AP_IPIMSG_ENC_ENCODE;
>  	out.base.vpu_inst_addr = vpu->inst_addr;
>  	out.base.bs_mode = bs_mode;
> @@ -302,12 +299,11 @@ static int vpu_enc_encode_34bits(struct venc_vpu_inst
> *vpu,
>  				 struct mtk_vcodec_mem *bs_buf,
>  				 struct venc_frame_info *frame_info)
>  {
> -	struct venc_ap_ipi_msg_enc_ext_34 out;
> +	struct venc_ap_ipi_msg_enc_ext_34 out = { };
>  	size_t msg_size = sizeof(struct venc_ap_ipi_msg_enc_ext_34);
>  
>  	mtk_venc_debug(vpu->ctx, "bs_mode %d ->", bs_mode);
>  
> -	memset(&out, 0, sizeof(out));
>  	out.msg_id = AP_IPIMSG_ENC_ENCODE;
>  	out.vpu_inst_addr = vpu->inst_addr;
>  	out.bs_mode = bs_mode;
> @@ -367,9 +363,8 @@ int vpu_enc_encode(struct venc_vpu_inst *vpu, unsigned int
> bs_mode,
>  
>  int vpu_enc_deinit(struct venc_vpu_inst *vpu)
>  {
> -	struct venc_ap_ipi_msg_deinit out;
> +	struct venc_ap_ipi_msg_deinit out = { };
>  
> -	memset(&out, 0, sizeof(out));
>  	out.msg_id = AP_IPIMSG_ENC_DEINIT;
>  	out.vpu_inst_addr = vpu->inst_addr;
>  	if (vpu_enc_send_msg(vpu, &out, sizeof(out))) {

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists