[PATCH V2] accel/amdxdna: Fix runtime suspend deadlock when there is pending job

[PATCH V2] accel/amdxdna: Fix runtime suspend deadlock when there is pending job

[Lizhi Hou]

The runtime suspend callback drains the running job workqueue before
suspending the device. If a job is still executing and calls
pm_runtime_resume_and_get(), it can deadlock with the runtime suspend
path.

Fix this by moving pm_runtime_resume_and_get() from the job execution
routine to the job submission routine, ensuring the device is resumed
before the job is queued and avoiding the deadlock during runtime
suspend.

Fixes: 063db451832b ("accel/amdxdna: Enhance runtime power management")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/aie2_ctx.c    | 14 ++------------
 drivers/accel/amdxdna/amdxdna_ctx.c | 10 ++++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
index afee5e667f77..c0d348884f74 100644
--- a/drivers/accel/amdxdna/aie2_ctx.c
+++ b/drivers/accel/amdxdna/aie2_ctx.c
@@ -165,7 +165,6 @@ aie2_sched_notify(struct amdxdna_sched_job *job)
 
 	trace_xdna_job(&job->base, job->hwctx->name, "signaled fence", job->seq);
 
-	amdxdna_pm_suspend_put(job->hwctx->client->xdna);
 	job->hwctx->priv->completed++;
 	dma_fence_signal(fence);
 
@@ -290,19 +289,11 @@ aie2_sched_job_run(struct drm_sched_job *sched_job)
 	struct dma_fence *fence;
 	int ret;
 
-	ret = amdxdna_pm_resume_get(hwctx->client->xdna);
-	if (ret)
+	if (!hwctx->priv->mbox_chann)
 		return NULL;
 
-	if (!hwctx->priv->mbox_chann) {
-		amdxdna_pm_suspend_put(hwctx->client->xdna);
-		return NULL;
-	}
-
-	if (!mmget_not_zero(job->mm)) {
-		amdxdna_pm_suspend_put(hwctx->client->xdna);
+	if (!mmget_not_zero(job->mm))
 		return ERR_PTR(-ESRCH);
-	}
 
 	kref_get(&job->refcnt);
 	fence = dma_fence_get(job->fence);
@@ -333,7 +324,6 @@ aie2_sched_job_run(struct drm_sched_job *sched_job)
 
 out:
 	if (ret) {
-		amdxdna_pm_suspend_put(hwctx->client->xdna);
 		dma_fence_put(job->fence);
 		aie2_job_put(job);
 		mmput(job->mm);
diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 666dfd7b2a80..838430903a3e 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -17,6 +17,7 @@
 #include "amdxdna_ctx.h"
 #include "amdxdna_gem.h"
 #include "amdxdna_pci_drv.h"
+#include "amdxdna_pm.h"
 
 #define MAX_HWCTX_ID		255
 #define MAX_ARG_COUNT		4095
@@ -445,6 +446,7 @@ amdxdna_arg_bos_lookup(struct amdxdna_client *client,
 void amdxdna_sched_job_cleanup(struct amdxdna_sched_job *job)
 {
 	trace_amdxdna_debug_point(job->hwctx->name, job->seq, "job release");
+	amdxdna_pm_suspend_put(job->hwctx->client->xdna);
 	amdxdna_arg_bos_put(job);
 	amdxdna_gem_put_obj(job->cmd_bo);
 	dma_fence_put(job->fence);
@@ -482,6 +484,12 @@ int amdxdna_cmd_submit(struct amdxdna_client *client,
 		goto cmd_put;
 	}
 
+	ret = amdxdna_pm_resume_get(xdna);
+	if (ret) {
+		XDNA_ERR(xdna, "Resume failed, ret %d", ret);
+		goto put_bos;
+	}
+
 	idx = srcu_read_lock(&client->hwctx_srcu);
 	hwctx = xa_load(&client->hwctx_xa, hwctx_hdl);
 	if (!hwctx) {
@@ -522,6 +530,8 @@ int amdxdna_cmd_submit(struct amdxdna_client *client,
 	dma_fence_put(job->fence);
 unlock_srcu:
 	srcu_read_unlock(&client->hwctx_srcu, idx);
+	amdxdna_pm_suspend_put(xdna);
+put_bos:
 	amdxdna_arg_bos_put(job);
 cmd_put:
 	amdxdna_gem_put_obj(job->cmd_bo);
-- 
2.34.1

Re: [PATCH V2] accel/amdxdna: Fix runtime suspend deadlock when there is pending job

[Mario Limonciello]

On 3/10/26 1:00 PM, Lizhi Hou wrote:
> The runtime suspend callback drains the running job workqueue before
> suspending the device. If a job is still executing and calls
> pm_runtime_resume_and_get(), it can deadlock with the runtime suspend
> path.
> 
> Fix this by moving pm_runtime_resume_and_get() from the job execution
> routine to the job submission routine, ensuring the device is resumed
> before the job is queued and avoiding the deadlock during runtime
> suspend.
> 
> Fixes: 063db451832b ("accel/amdxdna: Enhance runtime power management")
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
>   drivers/accel/amdxdna/aie2_ctx.c    | 14 ++------------
>   drivers/accel/amdxdna/amdxdna_ctx.c | 10 ++++++++++
>   2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index afee5e667f77..c0d348884f74 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -165,7 +165,6 @@ aie2_sched_notify(struct amdxdna_sched_job *job)
>   
>   	trace_xdna_job(&job->base, job->hwctx->name, "signaled fence", job->seq);
>   
> -	amdxdna_pm_suspend_put(job->hwctx->client->xdna);
>   	job->hwctx->priv->completed++;
>   	dma_fence_signal(fence);
>   
> @@ -290,19 +289,11 @@ aie2_sched_job_run(struct drm_sched_job *sched_job)
>   	struct dma_fence *fence;
>   	int ret;
>   
> -	ret = amdxdna_pm_resume_get(hwctx->client->xdna);
> -	if (ret)
> +	if (!hwctx->priv->mbox_chann)
>   		return NULL;
>   
> -	if (!hwctx->priv->mbox_chann) {
> -		amdxdna_pm_suspend_put(hwctx->client->xdna);
> -		return NULL;
> -	}
> -
> -	if (!mmget_not_zero(job->mm)) {
> -		amdxdna_pm_suspend_put(hwctx->client->xdna);
> +	if (!mmget_not_zero(job->mm))
>   		return ERR_PTR(-ESRCH);
> -	}
>   
>   	kref_get(&job->refcnt);
>   	fence = dma_fence_get(job->fence);
> @@ -333,7 +324,6 @@ aie2_sched_job_run(struct drm_sched_job *sched_job)
>   
>   out:
>   	if (ret) {
> -		amdxdna_pm_suspend_put(hwctx->client->xdna);
>   		dma_fence_put(job->fence);
>   		aie2_job_put(job);
>   		mmput(job->mm);
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 666dfd7b2a80..838430903a3e 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -17,6 +17,7 @@
>   #include "amdxdna_ctx.h"
>   #include "amdxdna_gem.h"
>   #include "amdxdna_pci_drv.h"
> +#include "amdxdna_pm.h"
>   
>   #define MAX_HWCTX_ID		255
>   #define MAX_ARG_COUNT		4095
> @@ -445,6 +446,7 @@ amdxdna_arg_bos_lookup(struct amdxdna_client *client,
>   void amdxdna_sched_job_cleanup(struct amdxdna_sched_job *job)
>   {
>   	trace_amdxdna_debug_point(job->hwctx->name, job->seq, "job release");
> +	amdxdna_pm_suspend_put(job->hwctx->client->xdna);
>   	amdxdna_arg_bos_put(job);
>   	amdxdna_gem_put_obj(job->cmd_bo);
>   	dma_fence_put(job->fence);
> @@ -482,6 +484,12 @@ int amdxdna_cmd_submit(struct amdxdna_client *client,
>   		goto cmd_put;
>   	}
>   
> +	ret = amdxdna_pm_resume_get(xdna);
> +	if (ret) {
> +		XDNA_ERR(xdna, "Resume failed, ret %d", ret);
> +		goto put_bos;
> +	}
> +
>   	idx = srcu_read_lock(&client->hwctx_srcu);
>   	hwctx = xa_load(&client->hwctx_xa, hwctx_hdl);
>   	if (!hwctx) {
> @@ -522,6 +530,8 @@ int amdxdna_cmd_submit(struct amdxdna_client *client,
>   	dma_fence_put(job->fence);
>   unlock_srcu:
>   	srcu_read_unlock(&client->hwctx_srcu, idx);
> +	amdxdna_pm_suspend_put(xdna);
> +put_bos:
>   	amdxdna_arg_bos_put(job);
>   cmd_put:
>   	amdxdna_gem_put_obj(job->cmd_bo);

Re: [PATCH V2] accel/amdxdna: Fix runtime suspend deadlock when there is pending job

[Lizhi Hou]

Applied to drm-misc-fixes

On 3/10/26 11:33, Mario Limonciello wrote:
> On 3/10/26 1:00 PM, Lizhi Hou wrote:
>> The runtime suspend callback drains the running job workqueue before
>> suspending the device. If a job is still executing and calls
>> pm_runtime_resume_and_get(), it can deadlock with the runtime suspend
>> path.
>>
>> Fix this by moving pm_runtime_resume_and_get() from the job execution
>> routine to the job submission routine, ensuring the device is resumed
>> before the job is queued and avoiding the deadlock during runtime
>> suspend.
>>
>> Fixes: 063db451832b ("accel/amdxdna: Enhance runtime power management")
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>>   drivers/accel/amdxdna/aie2_ctx.c    | 14 ++------------
>>   drivers/accel/amdxdna/amdxdna_ctx.c | 10 ++++++++++
>>   2 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/aie2_ctx.c 
>> b/drivers/accel/amdxdna/aie2_ctx.c
>> index afee5e667f77..c0d348884f74 100644
>> --- a/drivers/accel/amdxdna/aie2_ctx.c
>> +++ b/drivers/accel/amdxdna/aie2_ctx.c
>> @@ -165,7 +165,6 @@ aie2_sched_notify(struct amdxdna_sched_job *job)
>>         trace_xdna_job(&job->base, job->hwctx->name, "signaled 
>> fence", job->seq);
>>   - amdxdna_pm_suspend_put(job->hwctx->client->xdna);
>>       job->hwctx->priv->completed++;
>>       dma_fence_signal(fence);
>>   @@ -290,19 +289,11 @@ aie2_sched_job_run(struct drm_sched_job 
>> *sched_job)
>>       struct dma_fence *fence;
>>       int ret;
>>   -    ret = amdxdna_pm_resume_get(hwctx->client->xdna);
>> -    if (ret)
>> +    if (!hwctx->priv->mbox_chann)
>>           return NULL;
>>   -    if (!hwctx->priv->mbox_chann) {
>> -        amdxdna_pm_suspend_put(hwctx->client->xdna);
>> -        return NULL;
>> -    }
>> -
>> -    if (!mmget_not_zero(job->mm)) {
>> -        amdxdna_pm_suspend_put(hwctx->client->xdna);
>> +    if (!mmget_not_zero(job->mm))
>>           return ERR_PTR(-ESRCH);
>> -    }
>>         kref_get(&job->refcnt);
>>       fence = dma_fence_get(job->fence);
>> @@ -333,7 +324,6 @@ aie2_sched_job_run(struct drm_sched_job *sched_job)
>>     out:
>>       if (ret) {
>> -        amdxdna_pm_suspend_put(hwctx->client->xdna);
>>           dma_fence_put(job->fence);
>>           aie2_job_put(job);
>>           mmput(job->mm);
>> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c 
>> b/drivers/accel/amdxdna/amdxdna_ctx.c
>> index 666dfd7b2a80..838430903a3e 100644
>> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
>> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
>> @@ -17,6 +17,7 @@
>>   #include "amdxdna_ctx.h"
>>   #include "amdxdna_gem.h"
>>   #include "amdxdna_pci_drv.h"
>> +#include "amdxdna_pm.h"
>>     #define MAX_HWCTX_ID        255
>>   #define MAX_ARG_COUNT        4095
>> @@ -445,6 +446,7 @@ amdxdna_arg_bos_lookup(struct amdxdna_client 
>> *client,
>>   void amdxdna_sched_job_cleanup(struct amdxdna_sched_job *job)
>>   {
>>       trace_amdxdna_debug_point(job->hwctx->name, job->seq, "job 
>> release");
>> +    amdxdna_pm_suspend_put(job->hwctx->client->xdna);
>>       amdxdna_arg_bos_put(job);
>>       amdxdna_gem_put_obj(job->cmd_bo);
>>       dma_fence_put(job->fence);
>> @@ -482,6 +484,12 @@ int amdxdna_cmd_submit(struct amdxdna_client 
>> *client,
>>           goto cmd_put;
>>       }
>>   +    ret = amdxdna_pm_resume_get(xdna);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "Resume failed, ret %d", ret);
>> +        goto put_bos;
>> +    }
>> +
>>       idx = srcu_read_lock(&client->hwctx_srcu);
>>       hwctx = xa_load(&client->hwctx_xa, hwctx_hdl);
>>       if (!hwctx) {
>> @@ -522,6 +530,8 @@ int amdxdna_cmd_submit(struct amdxdna_client 
>> *client,
>>       dma_fence_put(job->fence);
>>   unlock_srcu:
>>       srcu_read_unlock(&client->hwctx_srcu, idx);
>> +    amdxdna_pm_suspend_put(xdna);
>> +put_bos:
>>       amdxdna_arg_bos_put(job);
>>   cmd_put:
>>       amdxdna_gem_put_obj(job->cmd_bo);
>

Claude review: accel/amdxdna: Fix runtime suspend deadlock when there is pending job

[Claude Code Review Bot]

Overall Series Review

Subject: accel/amdxdna: Fix runtime suspend deadlock when there is pending job
Author: Lizhi Hou <lizhi.hou@amd.com>
Patches: 3
Reviewed: 2026-03-11T13:04:35.223006

---

This is a single-patch fix for a runtime suspend deadlock in the amdxdna accelerator driver. The deadlock scenario is well-described: `aie2_hwctx_suspend_cb()` waits for in-flight jobs via `aie2_hwctx_wait_for_idle()` (which calls `dma_fence_wait_timeout()`), but the running job in `aie2_sched_job_run()` calls `amdxdna_pm_resume_get()` → `pm_runtime_resume_and_get()`, which blocks waiting for the runtime suspend to complete. This is a classic PM deadlock.

The fix moves the `pm_runtime_resume_and_get()` to the submit path (before the job is queued to the scheduler) and the `pm_runtime_put_autosuspend()` to `amdxdna_sched_job_cleanup()` (called when the job's refcount drops to zero). This is a reasonable approach that ensures the device stays awake for the duration of a job's lifetime without risking the deadlock.

Overall the approach is sound, but there is one significant concern with the PM reference balance.

---
Generated by Claude Code Patch Reviewer

Claude review: accel/amdxdna: Fix runtime suspend deadlock when there is pending job

[Claude Code Review Bot]

Patch Review

**Deadlock analysis is correct.** The described scenario — suspend path waiting for job completion while job execution path waits for PM resume — is a real deadlock and this approach of moving PM management to submit/cleanup boundaries is the right structural fix.

**PM reference imbalance on error paths in `aie2_sched_job_run()`:**

In the original code, when `aie2_sched_job_run()` succeeds, the PM reference acquired at the start is released later by `aie2_sched_notify()`. With this patch, the `amdxdna_pm_suspend_put()` call is removed from `aie2_sched_notify()` (line 168 in the current tree) and instead placed in `amdxdna_sched_job_cleanup()`, which runs when the job refcount reaches zero via `aie2_job_release()`.

However, looking at the error path in `aie2_sched_job_run()`:

```c
out:
	if (ret) {
-		amdxdna_pm_suspend_put(hwctx->client->xdna);
		dma_fence_put(job->fence);
		aie2_job_put(job);    /* drops refcnt, may call cleanup */
		mmput(job->mm);
		fence = ERR_PTR(ret);
	}
```

When `aie2_sched_job_run()` fails (ret != 0), `aie2_job_put()` will decrement the refcount. If this drops it to zero, `aie2_job_release()` → `amdxdna_sched_job_cleanup()` will call `amdxdna_pm_suspend_put()`. But the refcount was incremented by `kref_get()` earlier in `aie2_sched_job_run()` (at the line `kref_get(&job->refcnt)`), and the initial refcount was set at submit time via `kref_init()`. So `aie2_job_put()` here drops the extra ref taken in `run`, leaving the initial ref still held. The job cleanup (with the PM put) won't happen until the DRM scheduler framework drops the final reference. This should be fine for balance — the PM get happened at submit, the PM put happens at final cleanup.

**But consider this error path:** When `aie2_sched_job_run()` returns `NULL` (not an error pointer):

```c
	if (!hwctx->priv->mbox_chann)
		return NULL;
```

Returning `NULL` from `run` means the scheduler treats it as "no fence" but not an error. The job won't go through the normal completion path (`aie2_sched_notify()`), but the PM reference taken at submit time will only be released when the job is eventually freed through cleanup. This seems acceptable since the scheduler will still call `free_job` eventually.

**The early `return NULL` for PM resume failure is removed but needs attention:**

In the original code:
```c
	ret = amdxdna_pm_resume_get(hwctx->client->xdna);
	if (ret)
		return NULL;
```

Returning `NULL` here was already questionable (no PM reference to worry about since get failed), and now it's simply removed since PM is handled at submit time. This is correct.

**Error path label ordering in `amdxdna_cmd_submit()`:**

```c
+	ret = amdxdna_pm_resume_get(xdna);
+	if (ret) {
+		XDNA_ERR(xdna, "Resume failed, ret %d", ret);
+		goto put_bos;
+	}
...
 unlock_srcu:
 	srcu_read_unlock(&client->hwctx_srcu, idx);
+	amdxdna_pm_suspend_put(xdna);
+put_bos:
 	amdxdna_arg_bos_put(job);
```

The error unwinding looks correct: on success the PM reference is held (released later in `amdxdna_sched_job_cleanup()`); on failure after PM resume, `amdxdna_pm_suspend_put()` is called before falling through to `put_bos`. If PM resume itself fails, we jump directly to `put_bos`, skipping the `suspend_put` — correct, since the `resume_get` failed.

**Potential concern — PM reference held too long:** Previously the PM reference was held from `run` to `notify` (execution window). Now it's held from `submit` to job cleanup (entire job lifetime including time waiting in the scheduler queue). This means the autosuspend timer effectively can never fire if there are queued jobs, even if the hardware is idle. For a device with a 5-second autosuspend delay (`AMDXDNA_AUTOSUSPEND_DELAY`), this could keep the device awake longer than necessary. This is a tradeoff for correctness, which is reasonable, but worth noting.

**`use after put` concern in error path:** In `aie2_sched_job_run()` error path:

```c
	if (ret) {
		dma_fence_put(job->fence);
		aie2_job_put(job);
		mmput(job->mm);    /* <-- accessing job->mm after potential free */
		fence = ERR_PTR(ret);
	}
```

This is a pre-existing issue (not introduced by this patch), but `aie2_job_put()` could theoretically free `job` if the refcount drops to zero, making the subsequent `mmput(job->mm)` a use-after-free. In practice the scheduler likely holds an additional reference, but it's worth noting.

**Summary:** The fix is correct for the stated deadlock. The PM reference lifetime change (submit-to-cleanup vs run-to-notify) is a reasonable tradeoff. The patch is clean, well-described, and the error paths in the new code are properly handled.

---
Generated by Claude Code Patch Reviewer