Re: [PATCH] media: amd: isp4: fix self-deadlock in power-on error path

[Bin Du]

Many thanks, Yifei, for catching the deadlock in the failure path.

On 7/26/2026 4:36 AM, Yifei Gao wrote:
> [You don't often get email from gyf161023@xxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> isp4sd_pwron_and_init() holds ops_mutex via guard(mutex) for the entire
> function. On any initialization failure it jumps to the err_deinit label
> and calls isp4sd_pwroff_and_deinit(), which acquires the same ops_mutex
> through its own guard(mutex). Since the guard in isp4sd_pwron_and_init()
> still holds the lock at err_deinit, this re-acquires a non-recursive
> mutex already held by the current thread and deadlocks.
> 
> Every failure path in isp4sd_pwron_and_init() reaches this: a failed
> pm_runtime_resume_and_get(), a failed dev_pm_genpd_set_performance_state(),
> a firmware start failure in isp4if_start(), or a response-thread creation
> failure.
> 
> Move the cleanup logic into a new lockless __isp4sd_pwroff_and_deinit()
> and have isp4sd_pwroff_and_deinit() call it under the lock. The
> err_deinit path, which already holds ops_mutex, now calls the lockless
> helper directly.
> 
> The "stream still running" check remains in the locked wrapper: it guards
> external close requests, and the power-on rollback path never reaches the
> STARTED state. The cleanup steps are safe on a partially initialized
> device: response-thread stop is guarded per-thread, and gpu memory pools
> are released through isp4if_gpu_mem_free(), which is a no-op on
> unallocated pools.
> 
> 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@xxxxxxxxx>
> ---
>  drivers/media/platform/amd/isp4/isp4_subdev.c | 31 ++++++++++---------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/amd/isp4/isp4_subdev.c b/drivers/media/platform/amd/isp4/isp4_subdev.c
> index 48deea79ce6c..eabb6b36705f 100644
> --- a/drivers/media/platform/amd/isp4/isp4_subdev.c
> +++ b/drivers/media/platform/amd/isp4/isp4_subdev.c
> @@ -607,7 +607,8 @@ static int isp4sd_start_resp_proc_threads(struct isp4_subdev *isp_subdev)
>         return 0;
>  }
> 
> -int isp4sd_pwroff_and_deinit(struct v4l2_subdev *sd)
> +/* Caller must hold isp_subdev->ops_mutex. */
> +static void __isp4sd_pwroff_and_deinit(struct v4l2_subdev *sd)
>  {
>         struct isp4_subdev *isp_subdev = to_isp4_subdev(sd);
>         struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info;
> @@ -616,31 +617,20 @@ int isp4sd_pwroff_and_deinit(struct v4l2_subdev *sd)
>         struct device *dev = isp_subdev->dev;
>         int ret;
> 
> -       guard(mutex)(&isp_subdev->ops_mutex);
> -       if (sensor_info->status == ISP4SD_START_STATUS_STARTED) {
> -               dev_err(dev, "fail for stream still running\n");
> -               return -EINVAL;
> -       }
> -
>         sensor_info->status = ISP4SD_START_STATUS_OFF;
> -
>         if (isp_subdev->irq_enabled) {
>                 for (unsigned int i = 0; i < ISP4SD_MAX_FW_RESP_STREAM_NUM; i++)
>                         disable_irq(isp_subdev->irq[i]);
>                 isp_subdev->irq_enabled = false;
>         }
> -
>         isp4sd_stop_resp_proc_threads(isp_subdev);
>         dev_dbg(dev, "isp_subdev stop resp proc threads suc\n");
> -
>         isp4if_stop(ispif);
> -
>         ret = dev_pm_genpd_set_performance_state(dev, perf_state);
>         if (ret)
>                 dev_err(dev,
>                         "fail to set isp_subdev performance state %u,ret %d\n",
>                         perf_state, ret);
> -
>         /* hold ccpu reset */
>         isp4hw_wreg(isp_subdev->mmio, ISP_SOFT_RESET, 0);
>         isp4hw_wreg(isp_subdev->mmio, ISP_POWER_STATUS, 0);
> @@ -649,11 +639,9 @@ int isp4sd_pwroff_and_deinit(struct v4l2_subdev *sd)
>                 dev_err(dev, "power off isp_subdev fail %d\n", ret);
>         else
>                 dev_dbg(dev, "power off isp_subdev suc\n");
> -
>         ispif->status = ISP4IF_STATUS_PWR_OFF;
>         isp4if_clear_cmdq(ispif);
>         isp4sd_module_enable(isp_subdev, false);
> -
>         /*
>          * When opening the camera, isp4sd_module_enable(isp_subdev, true) is
>          * called. Hardware requires at least a 20ms delay between disabling
> @@ -661,7 +649,20 @@ int isp4sd_pwroff_and_deinit(struct v4l2_subdev *sd)
>          * during quick reopen scenarios.
>          */
>         msleep(20);
> +}
> 
> +int isp4sd_pwroff_and_deinit(struct v4l2_subdev *sd)
> +{
> +       struct isp4_subdev *isp_subdev = to_isp4_subdev(sd);
> +       struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info;
> +       struct device *dev = isp_subdev->dev;
> +
> +       guard(mutex)(&isp_subdev->ops_mutex);
> +       if (sensor_info->status == ISP4SD_START_STATUS_STARTED) {
> +               dev_err(dev, "fail for stream still running\n");
> +               return -EINVAL;
> +       }
> +       __isp4sd_pwroff_and_deinit(sd);
>         return 0;
>  }
> 
> @@ -725,7 +726,7 @@ int isp4sd_pwron_and_init(struct v4l2_subdev *sd)
> 
>         return 0;
>  err_deinit:
> -       isp4sd_pwroff_and_deinit(sd);
> +       __isp4sd_pwroff_and_deinit(sd);

However, if pm_runtime_resume_and_get() fails, the patch calls
__isp4sd_pwroff_and_deinit(), which unconditionally accesses ISP MMIO
while the hardware may be powered off and calls pm_runtime_put_sync()
without a corresponding acquired runtime-PM reference. Therefore, the
error paths need staged cleanup.

>         return -EINVAL;
>  }
> 
> --
> 2.43.0
>