RE: [PATCH v2 4/4] media: wave5: handle runtime resume failure when closing

[jackson.lee]

Hi Jiale

Thanks for the update. 
The direction looks right, and the encoder side looks good to me.

> -----Original Message-----
> From: Jiale Yao <yaojiale02@xxxxxxx>
> Sent: Monday, October 19, 2026 5:20 PM
> To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; jackson.lee
> <jackson.lee@xxxxxxxxxxxxxxx>; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>;
> Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>; Sebastian Fricke
> <sebastian.fricke@xxxxxxxxxxxxx>; Hans Verkuil <hverkuil@xxxxxxxxxx>;
> linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: Jiale Yao <yaojiale02@xxxxxxx>
> Subject: [PATCH v2 4/4] media: wave5: handle runtime resume failure when
> closing
> 
> The decoder and encoder close paths ignore runtime resume failures and
> proceed to firmware commands. If enabling the VPU clocks failed, those
> commands access unavailable registers, and the paths later drop a runtime
> PM reference that was not acquired.
> 
> Returning the resume error directly also prevents release_device() from
> calling wave5_cleanup_instance(). The instance has already been removed
> from the device list and its mem2mem context released, so its host-side
> resources leak.
> 
> On a resume failure, skip the firmware command and hardware lock, free the
> close-path resources, and return -EIO. This lets release_device() continue
> with its common instance cleanup without issuing an unmatched runtime PM
> put.
> 
> Fixes: 2092b3833487 ("media: chips-media: wave5: Support runtime
> suspend/resume")
> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
> ---
>  .../platform/chips-media/wave5/wave5-vpuapi.c  | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> index 4c1842f02c..806901e62d 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> @@ -220,7 +220,11 @@ int wave5_vpu_dec_close(struct vpu_instance *inst,
> u32 *fail_res)
>  	if (!inst->codec_info)
>  		return -EINVAL;
> 
> -	pm_runtime_resume_and_get(inst->dev->dev);
> +	ret = pm_runtime_resume_and_get(inst->dev->dev);
> +	if (ret < 0) {
> +		ret = -EIO;
> +		goto free_resources;
> +	}
> 
>  	ret_mutex = mutex_lock_interruptible(&vpu_dev->hw_lock);
>  	if (ret_mutex) {
> @@ -259,6 +263,7 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32
> *fail_res)
> 
>  	dev_dbg(inst->dev->dev, "%s: dec_finish_seq complete\n", __func__);
> 
> +free_resources:
>  	wave5_vdi_free_dma_memory(vpu_dev, &p_dec_info->vb_work);
> 
>  	for (i = 0 ; i < MAX_REG_FRAME; i++) { 

On the decoder side, there is a problem with the ret == -EIO check.
After the free_resources label, the auxiliary buffer loop overwrites ret:

for (i = 0 ; i < MAX_REG_FRAME; i++) {
	ret = reset_auxiliary_buffers(inst, i);
	if (ret) {
		ret = 0;
		break;
	}
}

So ret is always 0 after the loop, the "if (ret == -EIO)" check never matches, and the resume failure path falls through to unlock_and_return. 
It then unlocks hw_lock that was never taken, calls pm_runtime_put_sync() without a reference, and returns 0.
Could you move the host-side freeing into a small helper and call it from both paths, and return -EIO directly on the resume failure? 
That would avoid relying on the value of ret across the cleanup code.

Thanks,
Jackson


@@ -272,6 +277,8 @@ int
> wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
>  	wave5_vdi_free_dma_memory(vpu_dev, &p_dec_info->vb_task);
> 
>  	mutex_destroy(&inst->feed_lock);
> +	if (ret == -EIO)
> +		return ret;
> 
>  unlock_and_return:
>  	mutex_unlock(&vpu_dev->hw_lock);
> @@ -750,7 +757,11 @@ int wave5_vpu_enc_close(struct vpu_instance *inst,
> u32 *fail_res)
>  	if (!inst->codec_info)
>  		return -EINVAL;
> 
> -	pm_runtime_resume_and_get(inst->dev->dev);
> +	ret = pm_runtime_resume_and_get(inst->dev->dev);
> +	if (ret < 0) {
> +		ret = -EIO;
> +		goto free_resources;
> +	}
> 
>  	ret = mutex_lock_interruptible(&vpu_dev->hw_lock);
>  	if (ret) {
> @@ -774,6 +785,7 @@ int wave5_vpu_enc_close(struct vpu_instance *inst, u32
> *fail_res)
> 
>  	dev_dbg(inst->dev->dev, "%s: enc_finish_seq complete\n", __func__);
> 
> +free_resources:
>  	wave5_vdi_free_dma_memory(vpu_dev, &p_enc_info->vb_work);
> 
>  	if (inst->std == W_HEVC_ENC || inst->std == W_AVC_ENC) { @@ -784,6
> +796,8 @@ int wave5_vpu_enc_close(struct vpu_instance *inst, u32 *fail_res)
>  	}
> 
>  	wave5_vdi_free_dma_memory(vpu_dev, &p_enc_info->vb_task);
> +	if (ret == -EIO)
> +		return ret;
> 
>  unlock_and_return:
>  	mutex_unlock(&vpu_dev->hw_lock);
> --
> 2.34.1