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

[jiale yao]

Hi jackson,

At 2026-10-20 09:26:19, "jackson.lee" <jackson.lee@xxxxxxxxxxxxxxx> wrote:
>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

How about this one, https://lore.kernel.org/all/20261021151552.428590-1-yaojiale02@xxxxxxx/

>
>
>@@ -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