Re: [PATCH v5 2/2] media: amd: isp4: fix self-deadlock in isp4sd_pwron_and_init() error path

[Bin Du <Bin.Du@amd.com>]

On 7/30/2026 10:14 PM, Yifei Gao wrote:
> isp4sd_pwron_and_init() holds ops_mutex via guard(mutex) and, on any
> init failure, jumps to err_deinit and calls isp4sd_pwroff_and_deinit().
> That helper takes the same ops_mutex, re-acquiring a non-recursive mutex
> already held by the current thread, so any init failure deadlocks.
> 
> Unwind the error path in stages instead, releasing only what each
> failure point acquired. This also avoids the issues that an
> unconditional teardown would hit at the earlier failures, such as a
> runtime-PM underflow from pm_runtime_resume_and_get() and MMIO access
> while the device is unpowered.
> 
> Fixes: 4e5e7a7ddb4a ("media: platform: amd: isp4 subdev and firmware loading handling added")
> Assisted-by: Claude:claude-opus-4-8 smatch
> Signed-off-by: Yifei Gao <gyf161023@gmail.com>
> ---
>  drivers/media/platform/amd/isp4/isp4_subdev.c | 28 +++++++++++++++----
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/amd/isp4/isp4_subdev.c b/drivers/media/platform/amd/isp4/isp4_subdev.c
> index 48deea79ce6c..868d1c74d35e 100644
> --- a/drivers/media/platform/amd/isp4/isp4_subdev.c
> +++ b/drivers/media/platform/amd/isp4/isp4_subdev.c
> @@ -687,7 +687,7 @@ int isp4sd_pwron_and_init(struct v4l2_subdev *sd)
>  		if (ret) {
>  			dev_err(dev, "fail to power on isp_subdev ret %d\n",
>  				ret);
> -			goto err_deinit;
> +			goto err_module_disable;
>  		}
>  
>  		/* ISPPG ISP Power Status */
> @@ -697,7 +697,7 @@ int isp4sd_pwron_and_init(struct v4l2_subdev *sd)
>  			dev_err(dev,
>  				"fail to set performance state %u, ret %d\n",
>  				perf_state, ret);
> -			goto err_deinit;
> +			goto err_power_off;
>  		}
>  
>  		ispif->status = ISP4IF_STATUS_PWR_ON;
> @@ -709,12 +709,12 @@ int isp4sd_pwron_and_init(struct v4l2_subdev *sd)
>  	ret = isp4if_start(ispif);
>  	if (ret) {
>  		dev_err(dev, "fail to start isp_subdev interface\n");
> -		goto err_deinit;
> +		goto err_perf_restore;
>  	}
>  
>  	if (isp4sd_start_resp_proc_threads(isp_subdev)) {
>  		dev_err(dev, "isp_start_resp_proc_threads fail\n");
> -		goto err_deinit;
> +		goto err_stop_interface;
>  	}
>  
>  	dev_dbg(dev, "create resp threads ok\n");
> @@ -724,8 +724,24 @@ int isp4sd_pwron_and_init(struct v4l2_subdev *sd)
>  	isp_subdev->irq_enabled = true;
>  
>  	return 0;
> -err_deinit:
> -	isp4sd_pwroff_and_deinit(sd);
> +
> +err_stop_interface:
> +	isp4if_stop(ispif);
> +err_perf_restore:
> +	ret = dev_pm_genpd_set_performance_state(dev, ISP4SD_PERFORMANCE_STATE_LOW);
> +	if (ret)
> +		dev_err(dev, "fail to set performance state %u, ret %d\n",
> +			ISP4SD_PERFORMANCE_STATE_LOW, ret);
> +err_power_off:
> +	isp4hw_wreg(isp_subdev->mmio, ISP_SOFT_RESET, 0);
> +	isp4hw_wreg(isp_subdev->mmio, ISP_POWER_STATUS, 0);
> +	ret = pm_runtime_put_sync(dev);
> +	if (ret)
> +		dev_err(dev, "power off isp_subdev fail %d\n", ret);
> +	ispif->status = ISP4IF_STATUS_PWR_OFF;
> +err_module_disable:
> +	isp4sd_module_enable(isp_subdev, false);
> +	msleep(20);
>  	return -EINVAL;
>  }
>  

Reviewed-by: Bin Du <bin.du@amd.com>