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

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

[Yifei Gao]

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@gmail.com>
---
 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);
 	return -EINVAL;
 }
 
-- 
2.43.0

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@gmail.com. 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@gmail.com>
> ---
>  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
> 

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

[Yifei Gao]

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

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

[Bin Du]

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
> 

[PATCH v3 0/2] media: amd: isp4: fix error handling in isp4sd_pwron_and_init()

[Yifei Gao]

This series fixes the error handling in isp4sd_pwron_and_init().

Patch 1 makes isp4if_alloc_fw_gpumem() release partial allocations on
failure. This is an independent leak that was previously masked by the
deadlock fixed in patch 2: the thread deadlocked before the teardown
that would have freed the buffers could run.

Patch 2 reworks the power-on error path into staged cleanup. This fixes
the self-deadlock and, once the deadlock is removed and the error path
actually runs, the runtime-PM usage-count imbalance and the MMIO access
while unpowered that were previously unreachable.

The staged error path in patch 2 relies on isp4if_start() cleaning up
fully on failure, which patch 1 completes, so patch 1 comes first.

Changes since v2:
- Added patch 1 to release partial allocations in
  isp4if_alloc_fw_gpumem(), as pointed out by Bin Du.
- Patch 2 (the power-on staged cleanup) is unchanged from v2.

v2: https://lore.kernel.org/linux-media/20260727183500.298036-1-gyf161023@gmail.com/
v1: https://lore.kernel.org/linux-media/20260725203640.915626-1-gyf161023@gmail.com/

Yifei Gao (2):
  media: amd: isp4: release partial allocations in
    isp4if_alloc_fw_gpumem()
  media: amd: isp4: fix self-deadlock in isp4sd_pwron_and_init() error
    path

 .../media/platform/amd/isp4/isp4_interface.c  |  1 +
 drivers/media/platform/amd/isp4/isp4_subdev.c | 28 +++++++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] media: amd: isp4: release partial allocations in isp4if_alloc_fw_gpumem()

[Yifei Gao]

isp4if_alloc_fw_gpumem() allocates several GPU memory pools in sequence.
If one of them fails, it jumps to error_no_memory and returns -ENOMEM
without releasing the pools that were already allocated, leaking them.

Release the already-allocated pools before returning. isp4if_gpu_mem_free()
is a no-op on pools that were not allocated, so calling
isp4if_dealloc_fw_gpumem() here safely frees exactly the pools that
succeeded.

Fixes: 4c5feef6a62c ("media: platform: amd: Add isp4 fw and hw interface")
Signed-off-by: Yifei Gao <gyf161023@gmail.com>
---
 drivers/media/platform/amd/isp4/isp4_interface.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/amd/isp4/isp4_interface.c b/drivers/media/platform/amd/isp4/isp4_interface.c
index 8d73f66bb42c..8e3616cce479 100644
--- a/drivers/media/platform/amd/isp4/isp4_interface.c
+++ b/drivers/media/platform/amd/isp4/isp4_interface.c
@@ -201,6 +201,7 @@ static int isp4if_alloc_fw_gpumem(struct isp4_interface *ispif)
 
 error_no_memory:
 	dev_err(dev, "failed to allocate gpu memory\n");
+	isp4if_dealloc_fw_gpumem(ispif);
 	return -ENOMEM;
 }
 
-- 
2.43.0

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

[Yifei Gao]

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

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

[Sakari Ailus]

Hi Yifei,

On Tue, Jul 28, 2026 at 02:16:50PM +0000, 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.

Please make this more concise; the issue being addressed by the patch below
is much easier understood by looking at the patch than reading the above
explanation -- that shouldn't be the case. The aim here is not to describe
each detail of the change.

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

-- 
Regards,

Sakari Ailus

[PATCH v4 0/2] media: amd: isp4: fix error handling in isp4sd_pwron_and_init()

[Yifei Gao]

This series fixes the error handling in isp4sd_pwron_and_init().

Patch 1 makes isp4if_alloc_fw_gpumem() release partial allocations on
failure, an independent leak that was previously masked by the deadlock
fixed in patch 2.

Patch 2 reworks the power-on error path into staged cleanup, fixing the
self-deadlock and, once it is gone, the runtime-PM imbalance and
unpowered MMIO access. The staged path relies on isp4if_start() cleaning
up fully on failure, which patch 1 completes, so patch 1 comes first.

Changes since v3:
- Trimmed the commit message of patch 2 per Sakari's review; no
  functional change.

Changes since v2:
- Added patch 1 to release partial allocations in
  isp4if_alloc_fw_gpumem(), as pointed out by Bin Du.
- Patch 2 is otherwise unchanged from v2.

v3: https://lore.kernel.org/linux-media/20260728141659.62310-1-gyf161023@gmail.com/
v2: https://lore.kernel.org/linux-media/20260727183500.298036-1-gyf161023@gmail.com/
v1: https://lore.kernel.org/linux-media/20260725203640.915626-1-gyf161023@gmail.com/

Yifei Gao (2):
  media: amd: isp4: release partial allocations in
    isp4if_alloc_fw_gpumem()
  media: amd: isp4: fix self-deadlock in isp4sd_pwron_and_init() error
    path

 .../media/platform/amd/isp4/isp4_interface.c  |  1 +
 drivers/media/platform/amd/isp4/isp4_subdev.c | 28 +++++++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] media: amd: isp4: release partial allocations in isp4if_alloc_fw_gpumem()

[Yifei Gao]

isp4if_alloc_fw_gpumem() allocates several GPU memory pools in sequence.
If one of them fails, it jumps to error_no_memory and returns -ENOMEM
without releasing the pools that were already allocated, leaking them.

Release the already-allocated pools before returning. isp4if_gpu_mem_free()
is a no-op on pools that were not allocated, so calling
isp4if_dealloc_fw_gpumem() here safely frees exactly the pools that
succeeded.

Fixes: 4c5feef6a62c ("media: platform: amd: Add isp4 fw and hw interface")
Signed-off-by: Yifei Gao <gyf161023@gmail.com>
---
 drivers/media/platform/amd/isp4/isp4_interface.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/amd/isp4/isp4_interface.c b/drivers/media/platform/amd/isp4/isp4_interface.c
index 8d73f66bb42c..8e3616cce479 100644
--- a/drivers/media/platform/amd/isp4/isp4_interface.c
+++ b/drivers/media/platform/amd/isp4/isp4_interface.c
@@ -201,6 +201,7 @@ static int isp4if_alloc_fw_gpumem(struct isp4_interface *ispif)
 
 error_no_memory:
 	dev_err(dev, "failed to allocate gpu memory\n");
+	isp4if_dealloc_fw_gpumem(ispif);
 	return -ENOMEM;
 }
 
-- 
2.43.0

Re: [PATCH v4 1/2] media: amd: isp4: release partial allocations in isp4if_alloc_fw_gpumem()

[Bin Du]

On 7/29/2026 3:07 AM, Yifei Gao wrote:
> isp4if_alloc_fw_gpumem() allocates several GPU memory pools in sequence.
> If one of them fails, it jumps to error_no_memory and returns -ENOMEM
> without releasing the pools that were already allocated, leaking them.
> 
> Release the already-allocated pools before returning. isp4if_gpu_mem_free()
> is a no-op on pools that were not allocated, so calling
> isp4if_dealloc_fw_gpumem() here safely frees exactly the pools that
> succeeded.
> 
> Fixes: 4c5feef6a62c ("media: platform: amd: Add isp4 fw and hw interface")
> Signed-off-by: Yifei Gao <gyf161023@gmail.com>
> ---
>  drivers/media/platform/amd/isp4/isp4_interface.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/amd/isp4/isp4_interface.c b/drivers/media/platform/amd/isp4/isp4_interface.c
> index 8d73f66bb42c..8e3616cce479 100644
> --- a/drivers/media/platform/amd/isp4/isp4_interface.c
> +++ b/drivers/media/platform/amd/isp4/isp4_interface.c
> @@ -201,6 +201,7 @@ static int isp4if_alloc_fw_gpumem(struct isp4_interface *ispif)
>  
>  error_no_memory:
>  	dev_err(dev, "failed to allocate gpu memory\n");
> +	isp4if_dealloc_fw_gpumem(ispif);

The cleanup is needed, but isp4if_gpu_mem_free() currently logs
"invalid mem_info" at error level for every NULL entry. NULL entries are
expected during partial-allocation cleanup, so an early failure can
generate multiple misleading errors.

Please make the NULL case silent or call isp4if_gpu_mem_free() only for
allocated entries. With that fixed, the patch looks good to me.

>  	return -ENOMEM;
>  }
>  

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

[Yifei Gao]

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

[PATCH v5 0/2] media: amd: isp4: fix error handling in isp4sd_pwron_and_init()

[Yifei Gao]

This series fixes the error handling in isp4sd_pwron_and_init().

Patch 1 makes isp4if_alloc_fw_gpumem() release partial allocations on
failure, an independent leak that was previously masked by the deadlock
fixed in patch 2.

Patch 2 reworks the power-on error path into staged cleanup, fixing the
self-deadlock and, once it is gone, the runtime-PM imbalance and
unpowered MMIO access. The staged path relies on isp4if_start() cleaning
up fully on failure, which patch 1 completes, so patch 1 comes first.

Changes since v4:
- Made isp4if_gpu_mem_free() silent on NULL entries, which are expected
  during partial-allocation cleanup, per Bin Du's review.

Changes since v3:
- Trimmed the commit message of patch 2 per Sakari's review; no
  functional change.

Changes since v2:
- Added patch 1 to release partial allocations in
  isp4if_alloc_fw_gpumem(), as pointed out by Bin Du.
- Patch 2 is otherwise unchanged from v2.

v4: https://lore.kernel.org/linux-media/20260728190754.363464-1-gyf161023@gmail.com/
v3: https://lore.kernel.org/linux-media/20260728141659.62310-1-gyf161023@gmail.com/
v2: https://lore.kernel.org/linux-media/20260727183500.298036-1-gyf161023@gmail.com/
v1: https://lore.kernel.org/linux-media/20260725203640.915626-1-gyf161023@gmail.com/

Yifei Gao (2):
  media: amd: isp4: release partial allocations in
    isp4if_alloc_fw_gpumem()
  media: amd: isp4: fix self-deadlock in isp4sd_pwron_and_init() error
    path

 .../media/platform/amd/isp4/isp4_interface.c  |  6 ++--
 drivers/media/platform/amd/isp4/isp4_subdev.c | 28 +++++++++++++++----
 2 files changed, 24 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH v5 1/2] media: amd: isp4: release partial allocations in isp4if_alloc_fw_gpumem()

[Yifei Gao]

isp4if_alloc_fw_gpumem() allocates several GPU memory pools in sequence.
If one of them fails, it jumps to error_no_memory and returns -ENOMEM
without releasing the pools that were already allocated, leaking them.

Release the already-allocated pools before returning. isp4if_gpu_mem_free()
is a no-op on pools that were not allocated, so calling
isp4if_dealloc_fw_gpumem() here safely frees exactly the pools that
succeeded.

isp4if_gpu_mem_free() previously logged an error for a NULL entry, which
is a normal case during partial-allocation cleanup, so make it silent.

Fixes: 4c5feef6a62c ("media: platform: amd: Add isp4 fw and hw interface")
Signed-off-by: Yifei Gao <gyf161023@gmail.com>
---
 drivers/media/platform/amd/isp4/isp4_interface.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/amd/isp4/isp4_interface.c b/drivers/media/platform/amd/isp4/isp4_interface.c
index 8d73f66bb42c..4fefd0f7fbab 100644
--- a/drivers/media/platform/amd/isp4/isp4_interface.c
+++ b/drivers/media/platform/amd/isp4/isp4_interface.c
@@ -148,12 +148,9 @@ static void isp4if_gpu_mem_free(struct isp4_interface *ispif,
 				struct isp4if_gpu_mem_info **mem_info_ptr)
 {
 	struct isp4if_gpu_mem_info *mem_info = *mem_info_ptr;
-	struct device *dev = ispif->dev;
 
-	if (!mem_info) {
-		dev_err(dev, "invalid mem_info\n");
+	if (!mem_info)
 		return;
-	}
 
 	*mem_info_ptr = NULL;
 	isp_kernel_buffer_free(&mem_info->mem_handle, &mem_info->gpu_mc_addr,
@@ -201,6 +198,7 @@ static int isp4if_alloc_fw_gpumem(struct isp4_interface *ispif)
 
 error_no_memory:
 	dev_err(dev, "failed to allocate gpu memory\n");
+	isp4if_dealloc_fw_gpumem(ispif);
 	return -ENOMEM;
 }
 
-- 
2.43.0

Re: [PATCH v5 1/2] media: amd: isp4: release partial allocations in isp4if_alloc_fw_gpumem()

[Bin Du]

On 7/30/2026 10:14 PM, Yifei Gao wrote:
> isp4if_alloc_fw_gpumem() allocates several GPU memory pools in sequence.
> If one of them fails, it jumps to error_no_memory and returns -ENOMEM
> without releasing the pools that were already allocated, leaking them.
> 
> Release the already-allocated pools before returning. isp4if_gpu_mem_free()
> is a no-op on pools that were not allocated, so calling
> isp4if_dealloc_fw_gpumem() here safely frees exactly the pools that
> succeeded.
> 
> isp4if_gpu_mem_free() previously logged an error for a NULL entry, which
> is a normal case during partial-allocation cleanup, so make it silent.
> 
> Fixes: 4c5feef6a62c ("media: platform: amd: Add isp4 fw and hw interface")
> Signed-off-by: Yifei Gao <gyf161023@gmail.com>
> ---
>  drivers/media/platform/amd/isp4/isp4_interface.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/amd/isp4/isp4_interface.c b/drivers/media/platform/amd/isp4/isp4_interface.c
> index 8d73f66bb42c..4fefd0f7fbab 100644
> --- a/drivers/media/platform/amd/isp4/isp4_interface.c
> +++ b/drivers/media/platform/amd/isp4/isp4_interface.c
> @@ -148,12 +148,9 @@ static void isp4if_gpu_mem_free(struct isp4_interface *ispif,
>  				struct isp4if_gpu_mem_info **mem_info_ptr)
>  {
>  	struct isp4if_gpu_mem_info *mem_info = *mem_info_ptr;
> -	struct device *dev = ispif->dev;
>  
> -	if (!mem_info) {
> -		dev_err(dev, "invalid mem_info\n");
> +	if (!mem_info)
>  		return;
> -	}
>  
>  	*mem_info_ptr = NULL;
>  	isp_kernel_buffer_free(&mem_info->mem_handle, &mem_info->gpu_mc_addr,
> @@ -201,6 +198,7 @@ static int isp4if_alloc_fw_gpumem(struct isp4_interface *ispif)
>  
>  error_no_memory:
>  	dev_err(dev, "failed to allocate gpu memory\n");
> +	isp4if_dealloc_fw_gpumem(ispif);
>  	return -ENOMEM;
>  }
>  

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

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

[Yifei Gao]

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

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

[Bin Du]

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>