Re: [PATCH v6 3/9] media: chips-media: wave6: Add Wave6 VPU interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing bounds validation on hardware-supplied frame buffer counts causes array buffer overflows and potential infinite loops.
- [Medium] Leaking dynamically allocated `inst->codec_info` when closing decoder/encoder instance fails.
- [Medium] Hardcoded `BIG_ENDIAN` mismatch leads to byte-swapped decoder pixel formats on little-endian hosts.
- [Low] Endianness bug on Big-Endian architectures during product name string extraction.
- [Low] Mixing goto-based cleanup and `guard()` in the same function violates cleanup subsystem guidelines.
--

commit 61aa06e0583bb5f57f21d3d07d1ad5f6245d0508
Author: Nas Chung <nas.chung@chipsnmedia.com>

media: chips-media: wave6: Add Wave6 VPU interface

Add an interface layer to manage hardware register configuration
and communication with the Chips&Media Wave6 video codec IP.
The interface provides low-level helper functions used by the
Wave6 core driver to implement video encoding and decoding operations.

> diff --git a/drivers/media/platform/chips-media/wave6/wave6-hw.c b/drivers/media/platform/chips-media/wave6/wave6-hw.c
> index 0000000000000..b49348d6119c8 100644
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-hw.c
> @@ -230,22 +230,22 @@ int wave6_vpu_get_version(struct vpu_core_device *core)
>  	if (ret)
>  		return ret;
>  
>  	reg_val = vpu_read_reg(core, W6_RET_PRODUCT_NAME);
>  	str = (u8 *)&reg_val;
>  	attr->product_name[0] = str[3];
>  	attr->product_name[1] = str[2];
>  	attr->product_name[2] = str[1];
>  	attr->product_name[3] = str[0];
>  	attr->product_name[4] = 0;

[Severity: Low]
On big-endian architectures, vpu_read_reg() returns the integer in native
byte order. If the bytes are reversed using fixed indices like this, could
it result in the string being stored backwards or reading a null terminator
first?

[ ... ]

> @@ -345,13 +345,13 @@ int wave6_vpu_dec_register_frame_buffer(struct vpu_instance *inst)
>  
>  	fb_arr = &inst->frame_buf[0];
>  	fbc_count = p_dec_info->seq_info.min_frame_buffer_count;
>  	mv_count = p_dec_info->seq_info.req_mv_buffer_count;
>  
>  	for (i = 0; i < fbc_count; i++) {
>  		if (!inst->aux_vbuf[AUX_BUF_FBC_Y_TBL][i].daddr)
>  			return -EINVAL;
>  		if (!inst->aux_vbuf[AUX_BUF_FBC_C_TBL][i].daddr)
>  			return -EINVAL;
>  	}

[Severity: High]
If the hardware returns a value for fbc_count that is larger than
WAVE6_MAX_FBS (31), could this loop read past the end of the inst->aux_vbuf
arrays?

[ ... ]

> @@ -377,7 +377,7 @@ int wave6_vpu_dec_register_frame_buffer(struct vpu_instance *inst)
>  
>  	fbc_remain = fbc_count;
>  	mv_remain = mv_count;
>  	group_num = (fbc_count > mv_count) ? ((ALIGN(fbc_count, 16) / 16) - 1) :
>  					     ((ALIGN(mv_count, 16) / 16) - 1);

[Severity: High]
If the hardware unexpectedly returns zero for both fbc_count and mv_count,
would the subtraction of 1 after division underflow to SIZE_MAX and cause
an infinite loop here?

[ ... ]

> @@ -445,9 +445,9 @@ int wave6_vpu_dec_register_display_buffer(struct vpu_instance *inst, struct fram
>  
>  	reg_val = SET_DISP_COMMON_PIC_INFO_BWB_ON |
>  		  SET_DISP_COMMON_PIC_INFO_C_FMT_IDC(c_fmt_idc) |
>  		  SET_DISP_COMMON_PIC_INFO_PIXEL_ORDER(PIXEL_ORDER_INCREASING) |
>  		  SET_DISP_COMMON_PIC_INFO_OUT_MODE(out_mode) |
>  		  SET_DISP_COMMON_PIC_INFO_OUT_FMT(out_fmt) |
>  		  SET_DISP_COMMON_PIC_INFO_STRIDE(fb.stride);
>  	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_COMMON_PIC_INFO, reg_val);
>  	reg_val = SET_DISP_OPTION_ENDIAN(VDI_128BIT_BIG_ENDIAN);
>  	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_OPTION, reg_val);

[Severity: Medium]
Is it expected to strictly use VDI_128BIT_BIG_ENDIAN here? On little-endian
hosts, enforcing BIG_ENDIAN for output buffers might cause the hardware
state machine to output byte-swapped pixels. Should this use HOST_ENDIAN
instead?

[ ... ]

> @@ -1585,11 +1585,11 @@ static void wave6_gen_set_fb_reg(struct vpu_instance *inst, struct enc_cmd_set_f
>  	reg->num_fb = SET_FB_NUM_FBC_END_IDX(fbc_count - 1) |
>  		      SET_FB_NUM_MV_COL_END_IDX(mv_count - 1);
>  	reg->fbc_stride = SET_FB_FBC_STRIDE_L(fb_arr[0].stride) |
>  			  SET_FB_FBC_STRIDE_C(fb_arr[0].stride_c);
>  	reg->default_cdf = 0;
>  
>  	for (i = 0; i < fbc_count; i++) {
>  		reg->fbc_y[i] = fb_arr[i].buf_y;
>  		reg->fbc_c[i] = fb_arr[i].buf_cb;
>  		reg->fbc_cr[i] = fb_arr[i].buf_cr;
>  		reg->fbc_y_offset[i] = inst->aux_vbuf[AUX_BUF_FBC_Y_TBL][i].daddr;

[Severity: High]
Could this loop overflow the reg->fbc_y array if fbc_count exceeds
WAVE6_MAX_FBS?

[ ... ]

> @@ -1617,11 +1617,11 @@ int wave6_vpu_enc_register_frame_buffer(struct vpu_instance *inst)
>  	struct enc_cmd_set_fb_reg *reg;
>  	struct enc_info *p_enc_info;
>  	u32 fbc_count, mv_count, idx;
>  	int ret;
>  
>  	guard(mutex)(&inst->dev->hw_lock);
>  
>  	p_enc_info = &inst->codec_info->enc_info;

[ ... ]

> @@ -1665,11 +1665,11 @@ int wave6_vpu_enc_register_frame_buffer(struct vpu_instance *inst)
>  		goto exit;
>  	}
>  
>  	if (!vpu_read_reg(inst->dev, W6_RET_SUCCESS)) {
>  		ret = -EIO;
>  		goto exit;
>  	}
>  
>  	ret = 0;
>  
>  exit:
>  	kfree(reg);
>  	return ret;
>  }

[Severity: Low]
The cleanup subsystem guidelines strictly forbid mixing goto-based error
handling with scope-based cleanup helpers like guard() in the same function,
as it can create confusing ownership semantics. Could the dynamically
allocated reg object be managed using __free(kfree) to avoid manual goto
blocks?

> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpuapi.c b/drivers/media/platform/chips-media/wave6/wave6-vpuapi.c
> index 0000000000000..266885de92afd 100644
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpuapi.c
> @@ -42,16 +42,16 @@ int wave6_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
>  	if (WARN_ON(!inst->codec_info))
>  		return -EINVAL;
>  
>  	guard(mutex)(&inst->dev->hw_lock);
>  
>  	ret = wave6_vpu_dec_fini_seq(inst, fail_res);
>  	if (ret) {
>  		dev_warn(inst->dev->dev, "dec seq end timed out\n");
>  
>  		return ret;
>  	}
>  
>  	dev_dbg(inst->dev->dev, "dec seq end complete\n");
>  
>  	kfree(inst->codec_info);
>  
>  	return 0;
>  }

[Severity: Medium]
If hardware teardown in wave6_vpu_dec_fini_seq() fails and returns an
error, does this skip freeing the inst->codec_info struct?

[ ... ]

> @@ -233,16 +233,16 @@ int wave6_vpu_enc_close(struct vpu_instance *inst, u32 *fail_res)
>  	if (WARN_ON(!inst->codec_info))
>  		return -EINVAL;
>  
>  	guard(mutex)(&inst->dev->hw_lock);
>  
>  	ret = wave6_vpu_enc_fini_seq(inst, fail_res);
>  	if (ret) {
>  		dev_warn(inst->dev->dev, "enc seq end timed out\n");
>  
>  		return ret;
>  	}
>  
>  	dev_dbg(inst->dev->dev, "enc seq end complete\n");
>  	kfree(inst->codec_info);
>  
>  	return 0;
>  }

[Severity: Medium]
Similarly, if wave6_vpu_enc_fini_seq() fails here, will it result in a
memory leak of inst->codec_info?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624072043.238-1-nas.chung@chipsnmedia.com?part=3