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

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

Thanks, Yifei. v2 addresses the runtime-PM imbalance and unpowered MMIO
access from v1.

On 7/28/2026 2:34 AM, Yifei Gao wrote:
> isp4sd_pwron_and_init() holds ops_mutex through guard(mutex) for the
> whole function. On any initialization failure it jumps to the err_deinit
> label and calls isp4sd_pwroff_and_deinit(), which takes the same
> ops_mutex through its own guard(mutex). Because the guard in
> isp4sd_pwron_and_init() still holds the lock at err_deinit, the call
> re-acquires a non-recursive mutex held by the same thread and deadlocks
> on any initialization failure.
> 
> The error path cannot simply drop the lock and keep calling the full
> teardown, because that teardown is not valid at the earlier failure
> points. In particular, pm_runtime_resume_and_get() drops the usage
> counter again on failure (via pm_runtime_put_noidle()), so no PM
> reference is held when it returns an error; calling pm_runtime_put_sync()
> unconditionally would underflow the usage count, and the teardown would
> also touch ISP MMIO while the device is not powered. This was previously
> masked by the deadlock, since the thread never reached the teardown body.
> 
> Replace the single teardown call with staged labels that unwind only the
> resources actually acquired at each failure point. The error path no
> longer calls the locked isp4sd_pwroff_and_deinit(), which removes the
> deadlock, and each failure now skips the steps it never reached, keeping
> the runtime-PM count balanced and not accessing MMIO while unpowered.
> isp4if_start() and isp4sd_start_resp_proc_threads() already clean up
> after themselves on failure, so their resources are not unwound again.
> 
> 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>
> ---
> v2:
> - Reworked the fix from splitting the lock into staged error-path
>   cleanup, so that each failure unwinds only the resources it acquired.
>   This addresses the runtime-PM imbalance and the MMIO-while-unpowered
>   access that Bin Du pointed out on v1, while still removing the
>   self-deadlock (the error path no longer calls the locked
>   isp4sd_pwroff_and_deinit()).
> 
> Link to v1: https://lore.kernel.org/all/20260725203640.915626-1-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;

One issue remains: isp4if_start() cleans up after an isp4if_fw_boot()
failure, but not after an isp4if_alloc_fw_gpumem() failure. If
allocation fails after some buffers have already been allocated,
error_no_memory returns -ENOMEM without releasing them. Since this new
error path skips isp4if_stop(), those allocations leak. Could
error_no_memory call isp4if_dealloc_fw_gpumem() before returning?

>         }
> 
>         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;
>  }
> 
> --
> 2.43.0
>