[PATCH v0 0/4] bug fixes

[PATCH v0 0/4] bug fixes

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

A few independent fixes for the Wave5 VPU driver, covering decode
setup, m2m scheduling and stop_streaming robustness.

Jackson Lee (4):
  media: chips-media: wave5: Guard bit depth check with
    initial_info_obtained
  media: chips-media: wave5: Set inst->std during default format
    initialization
  media: chips-media: wave5: avoid skipping device_run while VPU has
    work
  media: chips-media: wave5: Add interrupt timeout while stop_streaming

 .../chips-media/wave5/wave5-vpu-dec.c         | 20 +++++++++++++++----
 .../chips-media/wave5/wave5-vpu-enc.c         |  6 ++++--
 .../chips-media/wave5/wave5-vpuconfig.h       |  2 +-
 3 files changed, 21 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH v0 1/4] media: chips-media: wave5: Guard bit depth check with initial_info_obtained

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

When CAPTURE STREAMON is called before the VPU has completed sequence
initialization (initial_info_obtained == false), the initial_info fields
contain uninitialized data. The driver checks
luma_bitdepth and rejects anything other than 8-bit, so garbage values
(e.g. 15) cause STREAMON to fail spuriously.

This is reproducible with the following multi-threaded test scenario:
  1. Allocate 2 CAPTURE buffers.
  2. Call STREAMON on the CAPTURE queue.
  3. Call DQBUF, which blocks waiting for a decoded frame.
  4. A second thread calls STREAMOFF on the CAPTURE queue.
  5. The blocked DQBUF should be released, allowing graceful termination.

At step 2, STREAMON reads uninitialized luma_bitdepth and rejects the
stream, causing the test to fail.

Fix this by checking initial_info_obtained before accessing the bit
depth fields, so the validation is only performed when the sequence
info has actually been parsed by the VPU.

Fixes: 035371c9e509 ("media: chips-media: wave5: Fix timeout while testing 10bit hevc fluster")
Cc: stable@vger.kernel.org
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
index bb2ba9204a83..01d1368b2965 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1403,6 +1403,7 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
 	} else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
 		struct dec_initial_info *initial_info =
 			&inst->codec_info->dec_info.initial_info;
+		struct dec_info *p_dec_info = &inst->codec_info->dec_info;
 
 		if (inst->state == VPU_INST_STATE_STOP)
 			ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
@@ -1410,6 +1411,7 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
 			goto return_buffers;
 
 		if (inst->state == VPU_INST_STATE_INIT_SEQ &&
+		    p_dec_info->initial_info_obtained &&
 		    inst->dev->product_code == WAVE521C_CODE) {
 			if (initial_info->luma_bitdepth != 8) {
 				dev_info(inst->dev->dev, "%s: no support for %d bit depth",
@@ -1418,7 +1420,6 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
 				goto return_buffers;
 			}
 		}
-
 	}
 	pm_runtime_put_autosuspend(inst->dev->dev);
 	return ret;
-- 
2.43.0

[PATCH v0 2/4] media: chips-media: wave5: Set inst->std during default format initialization

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

When the encoder is opened, wave5_set_default_format() sets up the
default capture format (e.g. H.264) but does not initialize inst->std.
As a result, inst->std remains zero, which does not match any valid
encoder codec.

If STREAMON is called before the user explicitly calls S_FMT on the
capture queue — as v4l2-compliance does in testBlockingDQBuf — the
codec/product check in wave5_vpu_enc_init_seq() fails with
"Unsupported encoder-codec & product combination" because inst->std
is neither W_HEVC_ENC nor W_AVC_ENC, returning -EOPNOTSUPP.

Fix this by setting inst->std via wave5_to_vpu_std() in
wave5_set_default_format(), so that the codec type is always consistent
with the default capture pixel format from the moment the instance is
opened.

Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
Cc: stable@vger.kernel.org
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
index e6c94b6f2671..f9fcdf4c224b 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
@@ -1494,7 +1494,8 @@ static const struct vb2_ops wave5_vpu_enc_vb2_ops = {
 	.stop_streaming = wave5_vpu_enc_stop_streaming,
 };
 
-static void wave5_set_default_format(struct v4l2_pix_format_mplane *src_fmt,
+static void wave5_set_default_format(struct vpu_instance *inst,
+				     struct v4l2_pix_format_mplane *src_fmt,
 				     struct v4l2_pix_format_mplane *dst_fmt)
 {
 	src_fmt->pixelformat = enc_fmt_list[VPU_FMT_TYPE_RAW][0].v4l2_pix_fmt;
@@ -1506,6 +1507,7 @@ static void wave5_set_default_format(struct v4l2_pix_format_mplane *src_fmt,
 	wave5_update_pix_fmt(dst_fmt, VPU_FMT_TYPE_CODEC,
 			     W5_DEF_ENC_PIC_WIDTH, W5_DEF_ENC_PIC_HEIGHT,
 			     &enc_frmsize[VPU_FMT_TYPE_CODEC]);
+	inst->std = wave5_to_vpu_std(dst_fmt->pixelformat, inst->type);
 }
 
 static int wave5_vpu_enc_queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
@@ -1770,7 +1772,7 @@ static int wave5_vpu_open_enc(struct file *filp)
 	inst->v4l2_fh.ctrl_handler = v4l2_ctrl_hdl;
 	v4l2_ctrl_handler_setup(v4l2_ctrl_hdl);
 
-	wave5_set_default_format(&inst->src_fmt, &inst->dst_fmt);
+	wave5_set_default_format(inst, &inst->src_fmt, &inst->dst_fmt);
 	inst->conf_win.width = inst->dst_fmt.width;
 	inst->conf_win.height = inst->dst_fmt.height;
 	inst->colorspace = V4L2_COLORSPACE_REC709;
-- 
2.43.0

[PATCH v0 3/4] media: chips-media: wave5: avoid skipping device_run while VPU has work

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

The decoder stalls because empty_queue is set to true even when the
m2m context still has pending commands. As a result, device_run is
never invoked, the m2m source queue fills up, and userspace (e.g.
Chromium) can no longer queue new bitstream buffers to the V4L2
driver.

Fix this by querying the VPU queue status via DEC_GET_QUEUE_STATUS
before deciding whether to skip device_run. Only skip when the
VPU's instance_queue_count equals the number of ready source
buffers in the v4l2-m2m context, which indicates that there is
genuinely no new work to perform. Otherwise, proceed with issuing
a decode command so that the VPU can continue draining its internal
queue.

Fixes: a176ac5e701f ("media: chips-media: wave5: Improve performance of decoder")
Cc: stable@vger.kernel.org
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../media/platform/chips-media/wave5/wave5-vpu-dec.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
index 01d1368b2965..b7fa19ea2aac 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1663,9 +1663,15 @@ static void wave5_vpu_dec_device_run(void *priv)
 		} else if (!inst->eos &&
 				inst->queuing_num == 0 &&
 				inst->state == VPU_INST_STATE_PIC_RUN) {
-			dev_dbg(inst->dev->dev, "%s: no bitstream for feeding, so skip ", __func__);
-			inst->empty_queue = true;
-			goto finish_job_and_return;
+			struct queue_status_info q_status;
+
+			wave5_vpu_dec_give_command(inst, DEC_GET_QUEUE_STATUS, &q_status);
+			if (q_status.instance_queue_count == v4l2_m2m_num_src_bufs_ready(m2m_ctx)) {
+				dev_dbg(inst->dev->dev, "%s: no bitstream, skip\n",
+					__func__);
+				inst->empty_queue = true;
+				goto finish_job_and_return;
+			}
 		}
 	}
 
-- 
2.43.0

[PATCH v0 4/4] media: chips-media: wave5: Add interrupt timeout while stop_streaming

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

When stop_streaming is called, an infinite loop may occur in some cases.
To prevent this, add a timeout based on interrupt status before calling
get_result. If no further interrupts are received, the loop should exit.

Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
Cc: stable@vger.kernel.org
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c   | 5 +++++
 drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
index b7fa19ea2aac..52ff1ac82e7b 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1543,6 +1543,7 @@ static void wave5_vpu_dec_stop_streaming(struct vb2_queue *q)
 	dev_dbg(inst->dev->dev, "%s: type: %u\n", __func__, q->type);
 	pm_runtime_resume_and_get(inst->dev->dev);
 	inst->empty_queue = true;
+
 	while (check_cmd) {
 		struct queue_status_info q_status;
 		struct dec_output_info dec_output_info;
@@ -1554,6 +1555,10 @@ static void wave5_vpu_dec_stop_streaming(struct vb2_queue *q)
 			q_status.report_queue_count == 0)
 			break;
 
+		if (q_status.instance_queue_count > 0 &&
+		    wave5_vpu_wait_interrupt(inst, VPU_DEC_STOP_TIMEOUT) < 0)
+			break;
+
 		if (wave5_vpu_dec_get_output_info(inst, &dec_output_info))
 			dev_dbg(inst->dev->dev, "there is no output info\n");
 	}
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h b/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
index 4ebd48d5550e..e04f2dbf3b65 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
@@ -59,7 +59,7 @@
 //  application specific configuration
 #define VPU_ENC_TIMEOUT                 60000
 #define VPU_DEC_TIMEOUT                 60000
-#define VPU_DEC_STOP_TIMEOUT            10
+#define VPU_DEC_STOP_TIMEOUT            300
 
 // for WAVE encoder
 #define USE_SRC_PRP_AXI         0
-- 
2.43.0

Re: [PATCH v0 2/4] media: chips-media: wave5: Set inst->std during default format initialization

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Error paths in the open handlers improperly call `ida_free()` with unallocated or negative IDs, corrupting the IDA state and risking instance ID aliasing.
- [High] The V4L2 M2M context is leaked on error paths during instance open because `v4l2_m2m_ctx_release` is never called.
- [High] The file release handler `wave5_vpu_release_device` can abort early on pending signals, permanently leaking the instance and leaving a dangling pointer.
- [High] The condition to free the device SRAM buffer is logically impossible to satisfy, permanently leaking the `gen_pool` allocation for non-WAVE515 chips.
- [High] `inst->state` is read and modified concurrently without any locks in the encoder, leading to race conditions and invalid state transitions.
- [High] Concurrent file opens race to allocate the shared device SRAM buffer, causing memory leaks and pointer corruption.
--

commit 4066d4d2cd1d6dd5099164b5a1d648c7737c2049
Author: Jackson Lee <jackson.lee@chipsnmedia.com>

media: chips-media: wave5: Set inst->std during default format initialization

This commit fixes an issue where inst->std remains uninitialized during the
default capture format setup, causing STREAMON to fail if S_FMT hasn't been
called. It resolves this by setting inst->std via wave5_to_vpu_std() in
wave5_set_default_format() to keep the codec type consistent.

Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
Cc: stable@vger.kernel.org
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>

> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> index e6c94b6f26711..f9fcdf4c224b3 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
[ ... ]
> @@ -1770,7 +1772,7 @@ static int wave5_vpu_open_enc(struct file *filp)
>  	inst->v4l2_fh.ctrl_handler = v4l2_ctrl_hdl;
>  	v4l2_ctrl_handler_setup(v4l2_ctrl_hdl);
>  
> -	wave5_set_default_format(&inst->src_fmt, &inst->dst_fmt);
> +	wave5_set_default_format(inst, &inst->src_fmt, &inst->dst_fmt);
>  	inst->conf_win.width = inst->dst_fmt.width;
>  	inst->conf_win.height = inst->dst_fmt.height;
>  	inst->colorspace = V4L2_COLORSPACE_REC709;

[Severity: High]
This isn't a bug introduced by this patch, but there are multiple issues on
the error paths in wave5_vpu_open_enc() jumping to cleanup_inst.

If an initialization step fails before ida_alloc() is called:

wave5-vpu-enc.c:wave5_vpu_open_enc() {
    ...
	ret = wave5_kfifo_alloc(inst);
	if (ret) {
		dev_err(inst->dev->dev, "failed to allocate fifo\n");
		goto cleanup_inst;
	}
    ...
cleanup_inst:
	wave5_cleanup_instance(inst, filp);
    ...
}

Does this call ida_free() with an unallocated or negative ID?

wave5-helper.c:wave5_cleanup_instance() {
    ...
	ida_free(&inst->dev->inst_ida, inst->id);
    ...
}

Since inst->id is 0-initialized, if it fails before ida_alloc(), this frees
ID 0. If another instance is using ID 0, the next open will reuse it,
causing aliasing. If ida_alloc() fails and returns a negative error code,
does this cause an out-of-bounds access?

Also, wave5_cleanup_instance() does not call v4l2_m2m_ctx_release(). Does
this permanently leak the m2m_ctx allocated earlier in wave5_vpu_open_enc()
and leave a dangling pointer?

[Severity: High]
This is a pre-existing issue, but do concurrent file opens race to allocate
the shared device SRAM buffer?

In wave5_vpu_open_enc(), wave5_vdi_allocate_sram() is called without holding
dev->dev_lock:

wave5-vpu-enc.c:wave5_vpu_open_enc() {
    ...
	wave5_vdi_allocate_sram(inst->dev);

	ret = mutex_lock_interruptible(&dev->dev_lock);
    ...
}

Inside wave5_vdi_allocate_sram():

wave5-vdi.c:wave5_vdi_allocate_sram() {
    ...
	if (!vpu_dev->sram_pool || vb->vaddr)
		return;

	size = min_t(size_t, vpu_dev->sram_size,
	             gen_pool_avail(vpu_dev->sram_pool));
	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
    ...
}

If multiple processes open the device simultaneously, can they race past the
vb->vaddr check, leak the gen_pool allocation, and corrupt the shared
pointers?

[Severity: High]
This isn't a bug introduced by this patch, but can wave5_vpu_release_device()
abort early and leak the instance?

wave5-helper.c:wave5_vpu_release_device() {
    ...
	ret = mutex_lock_interruptible(&inst->dev->irq_lock);
	if (ret)
		return ret;
    ...
}

If a process is exiting due to a fatal signal, this lock acquisition will
fail and return -ERESTARTSYS. Because this is the file release handler, the
kernel will destroy the file descriptor anyway. Does this permanently leak
the instance resources and leave a dangling pointer in dev->instances?
Should this use mutex_lock() instead?

[Severity: High]
This is a pre-existing issue, but is the condition to free the device SRAM
buffer logically impossible to satisfy?

wave5-helper.c:wave5_vpu_release_device() {
    ...
	list_del_init(&inst->list);
	wave5_cleanup_instance(inst, filp);
    ...
}

wave5-helper.c:wave5_cleanup_instance() {
    ...
	if (list_is_singular(&inst->list) &&
	    inst->dev->product_code != WAVE515_CODE)
		wave5_vdi_free_sram(inst->dev);
    ...
}

Because list_del_init() reinitializes inst->list as empty,
list_is_singular() will always return false. Does this permanently leak the
gen_pool allocation for non-WAVE515 chips? Should this check if
inst->dev->instances is empty instead?

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race on
inst->state in the encoder?

wave5-vpu-enc.c:switch_state() {
    ...
	inst->state = state;
	return 0;
}

In the encoder, switch_state() directly modifies inst->state without any
locks. Ioctl threads and the V4L2 M2M worker thread can concurrently mutate
this state. Can this cause the worker thread to observe intermediate states,
leading to invalid state transitions? The decoder module seems to protect
state mutations using inst->state_spinlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604020155.77-1-jackson.lee@chipsnmedia.com?part=2

Re: [PATCH v0 1/4] media: chips-media: wave5: Guard bit depth check with initial_info_obtained

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Data race when checking `initial_info_obtained` and `luma_bitdepth` without locks, leading to a TOCTOU-like race that reproduces the original bug.
- [High] Bypassing the bitdepth check for unresolved sequences reintroduces a timeout for unsupported 10-bit HEVC streams on WAVE521C.

Pre-existing issues:
- [Critical] Kernel heap buffer overflow in `wave5_prepare_fb` due to unvalidated `fbc_buf_count` and `linear_num`.
- [Critical] Physical memory corruption (DMA overflow) in `wave5_prepare_fb` due to missing buffer size validation against bitstream dimensions.
- [High] Ignored return value of `pm_runtime_resume_and_get()` leads to unpowered hardware access and usage count underflow.
- [High] Lockless modification of `inst->state` in `wave5_vpu_dec_start_streaming()`.
--

commit 26ff861a58ecc6c2122ff143493bf266fceeafb4
Author: Jackson Lee <jackson.lee@chipsnmedia.com>

media: chips-media: wave5: Guard bit depth check with initial_info_obtained

When CAPTURE STREAMON is called before the VPU has completed sequence
initialization, the initial_info fields contain uninitialized data. The driver
checks luma_bitdepth and rejects anything other than 8-bit, so garbage values
cause STREAMON to fail spuriously. This fix checks initial_info_obtained
before accessing the bit depth fields.

> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> index bb2ba9204a836..01d1368b29656 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> @@ -1403,6 +1403,7 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
>  	} else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>  		struct dec_initial_info *initial_info =
>  			&inst->codec_info->dec_info.initial_info;
> +		struct dec_info *p_dec_info = &inst->codec_info->dec_info;
>  
>  		if (inst->state == VPU_INST_STATE_STOP)
>  			ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);

[Severity: High]
This is a pre-existing issue, but does this code safely modify inst->state?

The state appears to be protected by inst->state_spinlock, but here it is
read and modified directly without acquiring the lock.

If the M2M worker thread modifies the state asynchronously via
set_instance_state() which takes the lock, could this lack of serialization
result in an inconsistent internal driver state machine?

> @@ -1410,6 +1411,7 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
>  			goto return_buffers;
>  
>  		if (inst->state == VPU_INST_STATE_INIT_SEQ &&
> +		    p_dec_info->initial_info_obtained &&
>  		    inst->dev->product_code == WAVE521C_CODE) {

[Severity: High]
Can this introduce a data race when checking p_dec_info->initial_info_obtained
and initial_info->luma_bitdepth?

If the M2M worker thread in wave5_vpu_dec_complete_seq_init() updates this
state concurrently:

wave5_vpu_dec_complete_seq_init()
    if (!ret)
        p_dec_info->initial_info_obtained = true;
    ...
    p_dec_info->initial_info = *info;

Could a concurrent ioctl observe initial_info_obtained as true while
luma_bitdepth is still uninitialized, observing garbage and reproducing the
same spurious failure this patch intended to resolve?

[Severity: High]
By bypassing the bitdepth validation here when initial_info_obtained is false,
do we defer the rejection of unsupported 10-bit streams to the worker thread?

If wave5_prepare_fb() catches the 10-bit stream later, its error handler
in wave5_vpu_dec_device_run() simply stops the instance:

wave5_vpu_dec_device_run()
    ret = wave5_prepare_fb(inst);
    if (ret) {
        ...
        set_instance_state(inst, VPU_INST_STATE_STOP);
        break;
    }

Since this doesn't return the queued capture buffers via v4l2_m2m_buf_done(),
could userspace hang indefinitely on DQBUF waiting for buffers that will
never arrive?

>  			if (initial_info->luma_bitdepth != 8) {
>  				dev_info(inst->dev->dev, "%s: no support for %d bit depth",
> @@ -1418,7 +1420,6 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
>  				goto return_buffers;
>  			}
>  		}
> -
>  	}
>  	pm_runtime_put_autosuspend(inst->dev->dev);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
pm_runtime_resume_and_get() earlier in this function?

If the initial resume fails, it returns a negative error and intrinsically
drops the usage counter.

Proceeding unconditionally and calling pm_runtime_put_autosuspend() here
would cause a double-decrement of the PM usage counter. Could this lead to
an underflow and unpowered hardware access?

>  	return ret;

[ ... ]

Additionally, there are some pre-existing issues in other parts of the driver
that might be worth reviewing:

[Severity: Critical]
This is a pre-existing issue, but does wave5_prepare_fb() properly bound
the number of frame buffers?

The function loops up to non_linear_num (which comes from the bitstream)
and linear_num (userspace queued capture buffers):

wave5_prepare_fb()
    for (i = 0; i < non_linear_num; i++) {
        struct frame_buffer *frame = &inst->frame_buf[i];

    ...
    for (i = 0; i < linear_num; i++) {
        struct frame_buffer *frame = &inst->frame_buf[non_linear_num + i];

The bounds check (num_of_decoding_fbs >= WAVE5_MAX_FBS) seems to happen
after these loops. Since the inst->frame_buf array is fixed size, could a
malicious bitstream or a large number of queued buffers cause an
out-of-bounds write into inst->frame_buf[] here?

[Severity: Critical]
This is a pre-existing issue, but does wave5_prepare_fb() verify the size
of the backing vb2 buffer before computing physical DMA addresses?

When computing the addresses based on dynamic bitstream dimensions:

wave5_prepare_fb()
    buf_addr_y = vb2_dma_contig_plane_dma_addr(vb, 0);
    buf_addr_cb = buf_addr_y + luma_size;
    buf_addr_cr = buf_addr_cb + chroma_size;

If userspace continues streaming with previously queued small buffers, could
the VPU hardware decode a large frame into undersized physical addresses
and cause out-of-bounds DMA writes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604020155.77-1-jackson.lee@chipsnmedia.com?part=1

Re: [PATCH v0 4/4] media: chips-media: wave5: Add interrupt timeout while stop_streaming

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] The newly added `wave5_vpu_wait_interrupt()` call in `wave5_vpu_dec_stop_streaming()` waits on a completion that is never signaled for picture decodes, leading to an unconditional 300ms stall, premature abort, and severe DMA Use-After-Free memory corruption.
- [High] Multiple concurrent threads waiting on the shared `inst->irq_done` completion can steal wakeups and cause a severe hang in the v4l2 m2m workqueue.

Pre-existing issues:
- [High] `wave5_vpu_wait_interrupt()` in `wave5_vpu_enc_stop_streaming()` suffers from a guaranteed timeout due to waiting on an unsignaled completion, causing an unconditional 60-second delay.
- [High] Ignoring the return value of `pm_runtime_resume_and_get()` leads to hardware register access while suspended and usage count underflow.
- [High] Failing to re-initialize the `irq_done` completion on timeout causes spurious wakeups and state machine corruption in subsequent operations.
--

commit 127f8c1856489ec0f52be02b9a57dc3b14051e6d
Author: Jackson Lee <jackson.lee@chipsnmedia.com>
Subject: media: chips-media: wave5: Add interrupt timeout while stop_streaming

This patch adds a timeout based on interrupt status in the stop_streaming loop
before calling get_result. If no further interrupts are received, the loop
exits to prevent infinite loops during stream termination.

> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> index b7fa19ea2aacb..52ff1ac82e7bf 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> @@ -1543,6 +1543,7 @@ static void wave5_vpu_dec_stop_streaming(struct vb2_queue *q)
>  	dev_dbg(inst->dev->dev, "%s: type: %u\n", __func__, q->type);
>  	pm_runtime_resume_and_get(inst->dev->dev);

[Severity: High]
This isn't a bug introduced by this patch, but does ignoring the return value
of pm_runtime_resume_and_get() lead to hardware access while suspended?

If the hardware resume fails, proceeding to execute wave5_vpu_dec_give_command()
and wave5_vpu_dec_get_output_info() will access powered-off hardware registers,
which could lead to crashes.

Additionally, pm_runtime_put_autosuspend() is called unconditionally at the end
of wave5_vpu_dec_stop_streaming(). If the initial resume failed, wouldn't this
unmatched put operation underflow the usage counter and corrupt the power
management state?

>  	inst->empty_queue = true;
> +
>  	while (check_cmd) {
>  		struct queue_status_info q_status;
>  		struct dec_output_info dec_output_info;
> @@ -1554,6 +1555,10 @@ static void wave5_vpu_dec_stop_streaming(struct vb2_queue *q)
>  			q_status.report_queue_count == 0)
>  			break;
>  
> +		if (q_status.instance_queue_count > 0 &&
> +		    wave5_vpu_wait_interrupt(inst, VPU_DEC_STOP_TIMEOUT) < 0)

[Severity: Critical]
Does this wait_for_completion actually get signaled for picture decodes?

Looking at wave5_vpu_handle_irq() in wave5-vpu.c, complete(&inst->irq_done) is
only called for sequence initialization (INT_WAVE5_INIT_SEQ) and parameter sets.

For standard picture decoding (INT_WAVE5_DEC_PIC), the handler only queues a
kfifo entry to wake a worker thread and explicitly omits signaling the
completion:

if (irq_reason & BIT(INT_WAVE5_DEC_PIC) ||
    irq_reason & BIT(INT_WAVE5_ENC_PIC)) {
    ...
    val = BIT(INT_WAVE5_DEC_PIC);
    kfifo_in(&inst->irq_status, &val, sizeof(int));
}

Since the completion is omitted for DEC_PIC, won't this wait unconditionally
block for the full 300ms timeout, return -ETIMEDOUT, and break the loop early?

If the loop breaks prematurely, the driver unconditionally proceeds to
streamoff_capture(), which returns destination buffers to userspace while the
hardware is still actively decoding. When the delayed VPU DMA completes, could
it overwrite the freed memory and cause a DMA use-after-free?

[Severity: High]
Can this concurrent wait cause wakeups to be stolen from the m2m workqueue?

Because stop_streaming runs in ioctl context, it can execute concurrently with
wave5_vpu_dec_device_run() in the m2m workqueue. If the device run queue is
executing initialize_sequence(), both threads will block on the exact same
inst->irq_done completion.

Completions use WQ_FLAG_EXCLUSIVE, so a call to complete() wakes exactly one
thread. If stop_streaming is awoken, it will consume the completion and call
reinit_completion(). Could this leave initialize_sequence() permanently asleep
until its 60-second timeout expires?

> +			break;
> +
>  		if (wave5_vpu_dec_get_output_info(inst, &dec_output_info))
>  			dev_dbg(inst->dev->dev, "there is no output info\n");
>  	}

Looking at the surrounding timeout handling and encoder teardown code, there
are related pre-existing issues:

[Severity: High]
This isn't a bug introduced by this patch, but does wave5_vpu_wait_interrupt()
properly clean up the completion state on timeout?

drivers/media/platform/chips-media/wave5/wave5-vpu.c:wave5_vpu_wait_interrupt() {
    ret = wait_for_completion_timeout(&inst->irq_done,
                                      msecs_to_jiffies(timeout));
    if (!ret)
        return -ETIMEDOUT;

    reinit_completion(&inst->irq_done);
    return 0;
}

If wait_for_completion_timeout() times out, it returns -ETIMEDOUT and skips
calling reinit_completion().

If the hardware eventually finishes and fires an interrupt, the handler will
call complete(&inst->irq_done). Wouldn't the next operation on this instance
(such as a retried INIT_SEQ) immediately consume this stale completion,
prematurely read unready hardware registers, and cause desynchronization?

[Severity: High]
This isn't a bug introduced by this patch, but does the encoder suffer from a
similar unconditional timeout issue?

In drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c,
wave5_vpu_enc_stop_streaming() has this same timeout pattern:

    if (wave5_vpu_wait_interrupt(inst, VPU_ENC_TIMEOUT) < 0)
        break;

Since inst->irq_done is never signaled for INT_WAVE5_ENC_PIC interrupts, won't
this always block for the full 60-second VPU_ENC_TIMEOUT whenever the report
queue is not empty, causing a massive delay before prematurely aborting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604020155.77-1-jackson.lee@chipsnmedia.com?part=4

Re: [PATCH v0 3/4] media: chips-media: wave5: avoid skipping device_run while VPU has work

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Invalid comparison between VPU hardware command count and V4L2 software source buffer count causes decoding stalls for multi-frame bitstream buffers.
- [Medium] Bypassing the queue-empty check allows `queuing_num` to underflow, permanently breaking the driver's job scheduling optimization.
- [Low] Variable shadowing of `q_status` in `wave5_vpu_dec_device_run`.

Pre-existing issues:
- [High] If the hardware ringbuffer lacks space for an incoming source buffer, the buffer is permanently dropped from the feed list.
--

commit 483963c314a78ddc15cfbde8b0ce5a96c14f95c1
Author: Jackson Lee <jackson.lee@chipsnmedia.com>

media: chips-media: wave5: avoid skipping device_run while VPU has work
    
This patch fixes an issue where the decoder stalls because empty_queue is
set to true even when the m2m context has pending commands. It queries the
VPU queue status before deciding whether to skip device_run, ensuring that
commands continue to be issued if the VPU is actively processing.

> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> index 01d1368b29656..b7fa19ea2aacb 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> @@ -1663,9 +1663,15 @@ static void wave5_vpu_dec_device_run(void *priv)
>  		} else if (!inst->eos &&
>  				inst->queuing_num == 0 &&
>  				inst->state == VPU_INST_STATE_PIC_RUN) {
> -			dev_dbg(inst->dev->dev, "%s: no bitstream for feeding, so skip ", __func__);
> -			inst->empty_queue = true;
> -			goto finish_job_and_return;
> +			struct queue_status_info q_status;

[Severity: Low]
Is this declaration of q_status shadowing an existing variable?

Looking at the top of wave5_vpu_dec_device_run(), q_status is already
declared. Could we reuse the existing variable instead of declaring a new
one here?

> +
> +			wave5_vpu_dec_give_command(inst, DEC_GET_QUEUE_STATUS, &q_status);
> +			if (q_status.instance_queue_count == v4l2_m2m_num_src_bufs_ready(m2m_ctx)) {

[Severity: High]
Does this comparison accurately reflect the remaining work if a single source
buffer contains multiple encoded frames?

The instance_queue_count tracks the number of active commands in the
hardware, while v4l2_m2m_num_src_bufs_ready counts unconsumed userspace
bitstream buffers. If a user queues a single source buffer holding three
frames, num_src_bufs_ready is 1. When the hardware starts processing the
first frame, instance_queue_count becomes 1.

If they are equal, this code sets empty_queue to true and stops scheduling.
When the hardware finishes the first frame, instance_queue_count drops to 0,
but num_src_bufs_ready remains 1 because the buffer is only partially
consumed.

Can this cause the remaining frames in the buffer to permanently stall since
the worker might refuse to schedule until empty_queue is cleared by
another buffer?

[Severity: Medium]
What happens to the queuing_num counter if this condition falls through?

If instance_queue_count is not equal to num_src_bufs_ready, the block falls
through and the PIC_RUN state handling unconditionally calls start_decode().
If the VPU accepts the command, the routine executes:

	if (fail_res == WAVE5_SYSERR_QUEUEING_FAIL) {
		...
	} else {
		inst->retry = false;
		if (!inst->eos)
			inst->queuing_num--;
	}

Since we only entered this block because inst->queuing_num was 0, would this
decrement cause queuing_num to underflow to -1?

Because it is signed, it would remain negative indefinitely, potentially
breaking the idle tracking logic and causing the driver to repeatedly issue
start_decode() commands without bitstream data.

> +				dev_dbg(inst->dev->dev, "%s: no bitstream, skip\n",
> +					__func__);
> +				inst->empty_queue = true;
> +				goto finish_job_and_return;
> +			}
>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does the fill_ringbuffer()
routine leak source buffers when the ringbuffer is full?

Looking at fill_ringbuffer() in the same file:

	while ((vpu_buf = inst_src_buf_remove(inst)) != NULL) {
		...
		if (remain_size < src_size) {
			dev_dbg(inst->dev->dev,
				"%s: remaining size: %zu < source size: %zu for src buf (%u)\n",
				__func__, remain_size, src_size, vbuf->vb2_buf.index);
			break;
		}
		...
	}

The function inst_src_buf_remove() takes the buffer off the available list.
When remain_size is less than src_size, the loop breaks out early, but the
buffer doesn't seem to be added back to the available list.

Will this permanently drop the buffer from the feed list, leading to lost
bitstream chunks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604020155.77-1-jackson.lee@chipsnmedia.com?part=3