[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

[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

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