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

[PATCH V1] 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.

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 V1] accel/amdxdna: Fix runtime suspend deadlock when there is pending job

[Mario Limonciello]

On 3/10/26 12:49 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.
> 
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Fixes tag?
> ---
>   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 V1] accel/amdxdna: Fix runtime suspend deadlock when there is pending job

[Lizhi Hou]

On 3/10/26 10:50, Mario Limonciello wrote:
> On 3/10/26 12:49 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.
>>
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> Fixes tag?

I missed it again. I will send V2.

Thanks,

Lizhi

>> ---
>>   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:06:23.926508

---

This is a single patch fixing a real runtime suspend deadlock in the amdxdna accelerator driver. The deadlock scenario is clearly described: `aie2_sched_job_run()` (called from the DRM scheduler workqueue) calls `amdxdna_pm_resume_get()`, but during runtime suspend the driver drains that workqueue, creating a circular dependency.

The fix moves the PM `resume_get` from the job execution path (`aie2_sched_job_run`) to the job submission path (`amdxdna_cmd_submit`), and moves the matching `suspend_put` from the job notification/error paths to `amdxdna_sched_job_cleanup()` (called via `kref_put` when the job is finally released). The approach is sound in principle, but there are PM reference counting imbalances.

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

**PM reference leak on successful job submission:**

In the patched `amdxdna_cmd_submit()`, on success (line 492 in the original), the function returns 0 *without* calling `amdxdna_pm_suspend_put()`. The intent is that the matching `suspend_put` will be called later in `amdxdna_sched_job_cleanup()`. However, `amdxdna_sched_job_cleanup()` is only called from `aie2_job_release()` (via `kref_put`), and `kref_init(&job->refcnt)` is at line 477 in the current code. The `kref_get` in `aie2_sched_job_run()` increments the refcount, and `aie2_job_put` in the notify/error paths decrements it. On the success path, the flow is:

1. `amdxdna_cmd_submit` → `resume_get` (ref=1)
2. `aie2_sched_job_run` → `kref_get` (refcnt=2), then on success, returns fence
3. `aie2_sched_notify` → `aie2_job_put` (refcnt=1) — but **no longer calls `suspend_put`** since that line is removed
4. Eventually `aie2_job_release` → `amdxdna_sched_job_cleanup` → `suspend_put` (ref=0)

This looks correct — the `suspend_put` moved into cleanup fires once per job lifetime. Good.

**Missing `suspend_put` when `aie2_sched_job_run` returns NULL for `!mbox_chann`:**

```c
-	if (!hwctx->priv->mbox_chann) {
-		amdxdna_pm_suspend_put(hwctx->client->xdna);
-		return NULL;
-	}
+	if (!hwctx->priv->mbox_chann)
+		return NULL;
```

When this returns NULL, the DRM scheduler treats it as the job not needing a fence and the job is effectively dropped. In this path the PM reference was already acquired in `amdxdna_cmd_submit()`. Returning NULL here means the scheduler's `free_job` callback will eventually be called, which should lead to `aie2_job_release` → `amdxdna_sched_job_cleanup` → `suspend_put`. **This is fine** as long as the DRM scheduler does call free_job for jobs whose run callback returned NULL. This should be verified — if the scheduler doesn't call `free_job` in this case, the PM reference will leak.

**Error path in `amdxdna_cmd_submit` — `put_bos` label ordering:**

```c
unlock_srcu:
	srcu_read_unlock(&client->hwctx_srcu, idx);
+	amdxdna_pm_suspend_put(xdna);
+put_bos:
	amdxdna_arg_bos_put(job);
```

The `put_bos` label is placed *after* the `suspend_put` call. When the `amdxdna_arg_bos_lookup()` call fails (jumping to `put_bos`), `suspend_put` is **not** called. But `resume_get` was called just above in the patched code:

```c
ret = amdxdna_pm_resume_get(xdna);
if (ret) {
    XDNA_ERR(xdna, "Resume failed, ret %d", ret);
    goto put_bos;
}
```

Wait — if `resume_get` fails, the code jumps to `put_bos`, which skips `suspend_put`. That's **correct** because the resume failed so there's no reference to drop.

However, if `resume_get` succeeds but a later step (e.g., `hwctx` lookup, `fence_create`, `cmd_submit`) fails, the error path goes to `unlock_srcu`, which calls `suspend_put` before `put_bos`. That is also **correct**.

**Double `suspend_put` on `aie2_sched_job_run` error path:**

In the patched `aie2_sched_job_run`, the error path at the `out:` label no longer calls `suspend_put`:

```c
out:
	if (ret) {
-		amdxdna_pm_suspend_put(hwctx->client->xdna);
		dma_fence_put(job->fence);
		aie2_job_put(job);
```

`aie2_job_put` will eventually lead to `amdxdna_sched_job_cleanup` → `suspend_put`. This is correct — one `resume_get` at submit, one `suspend_put` at cleanup.

**Concern — `suspend_put` in `aie2_sched_notify` removed:**

```c
-	amdxdna_pm_suspend_put(job->hwctx->client->xdna);
 	job->hwctx->priv->completed++;
```

This is the normal success path. The `suspend_put` is now deferred to `amdxdna_sched_job_cleanup()` via the kref release. This changes the semantics: the device stays "resumed" slightly longer (until the job object is freed rather than when the job completes). This is functionally correct but means the device won't autosuspend as quickly. For a deadlock fix, this is an acceptable trade-off.

**Minor style note:** The commit message says `[PATCH V1]` — the `V1` is unnecessary for a first posting (it's the default).

**Summary:** The core logic of the fix is sound. The PM reference is taken at submit time (user context, safe to sleep) and released when the job is cleaned up. The key risk to verify is: does the DRM scheduler guarantee that `free_job` is called (and thus `aie2_job_release` triggers) for every job whose `run` callback returned NULL? If not, the `!mbox_chann` early-return path would leak a PM reference. A comment at that return site explaining why it's safe would be valuable.

---
Generated by Claude Code Patch Reviewer