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

[sashiko-bot@kernel.org]

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