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

[jackson.lee]

Hi Jiale,

Thanks, this version looks good to me. The decoder problem is fixed.

One small idea: you could also use a helper function for the encoder, like you did for the decoder. Then both close functions would work the same way.
This is optional. The encoder code works fine as it is.

Reviewed-by: Jackson Lee jackson.lee@xxxxxxxxxxxxxxx

Thanks,
Jackson

> -----Original Message-----
> From: Jiale Yao <yaojiale02@xxxxxxx>
> Sent: Thursday, October 22, 2026 12:16 AM
> 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: stable@xxxxxxxxxxxxxxx
> Subject: [PATCH v3 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. Use a decoder cleanup helper so
> auxiliary-buffer cleanup cannot overwrite the resume failure status.
> This lets release_device() continue with its common instance cleanup
> without unlocking an unheld mutex or issuing an unmatched runtime PM put.
> 
> Fixes: 2092b3833487 ("media: chips-media: wave5: Support runtime
> suspend/resume")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
> ---
>  .../platform/chips-media/wave5/wave5-vpuapi.c | 47 ++++++++++++-------
>  1 file changed, 29 insertions(+), 18 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..31be2581ae 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> @@ -206,13 +206,26 @@ static int reset_auxiliary_buffers(struct
> vpu_instance *inst, unsigned int index
>  	return 0;
>  }
> 
> -int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
> +static void wave5_vpu_dec_free_resources(struct vpu_instance *inst)
>  {
>  	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
> +	int i;
> +
> +	wave5_vdi_free_dma_memory(inst->dev, &p_dec_info->vb_work);
> +
> +	for (i = 0; i < MAX_REG_FRAME; i++)
> +		if (reset_auxiliary_buffers(inst, i))
> +			break;
> +
> +	wave5_vdi_free_dma_memory(inst->dev, &p_dec_info->vb_task);
> +	mutex_destroy(&inst->feed_lock);
> +}
> +
> +int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res) {
>  	int ret;
>  	int retry = 0;
>  	struct vpu_device *vpu_dev = inst->dev;
> -	int i;
>  	struct dec_output_info dec_info;
>  	int ret_mutex;
> 
> @@ -220,7 +233,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) {
> +		wave5_vpu_dec_free_resources(inst);
> +		return -EIO;
> +	}
> 
>  	ret_mutex = mutex_lock_interruptible(&vpu_dev->hw_lock);
>  	if (ret_mutex) {
> @@ -258,20 +275,7 @@ int wave5_vpu_dec_close(struct vpu_instance *inst,
> u32 *fail_res)
>  	} while (ret != 0);
> 
>  	dev_dbg(inst->dev->dev, "%s: dec_finish_seq complete\n", __func__);
> -
> -	wave5_vdi_free_dma_memory(vpu_dev, &p_dec_info->vb_work);
> -
> -	for (i = 0 ; i < MAX_REG_FRAME; i++) {
> -		ret = reset_auxiliary_buffers(inst, i);
> -		if (ret) {
> -			ret = 0;
> -			break;
> -		}
> -	}
> -
> -	wave5_vdi_free_dma_memory(vpu_dev, &p_dec_info->vb_task);
> -
> -	mutex_destroy(&inst->feed_lock);
> +	wave5_vpu_dec_free_resources(inst);
> 
>  unlock_and_return:
>  	mutex_unlock(&vpu_dev->hw_lock);
> @@ -750,7 +754,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 +782,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
> +793,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