[PATCH V1] accel/amdxdna: Fix command timeout race

[PATCH V1] accel/amdxdna: Fix command timeout race

[Lizhi Hou]

From: Wendy Liang <wendy.liang@amd.com>

When two commands enter aie2_sched_job_timedout() concurrently, both
check the timeout detection state. The first scheduler thread observes
tdr_status as SIGNALED and updates it to WAIT. The second thread then
observes the updated state instead of the original SIGNALED state, which
may cause the command timeout to be handled incorrectly.

Replace tdr_status with last_signal_ts, which records the timestamp of
the last driver signal. Timeout detection now only reads
last_signal_ts and never modifies it, allowing multiple serialized
detect() calls under dev_lock to evaluate the same signal timestamp
independently. If there is not any new job scheduled or completed
within tdr_timeout_ms, the command will timeout.

Fixes: 9022f010977f ("accel/amdxdna: Check for device hang on job timeout")
Signed-off-by: Wendy Liang <wendy.liang@amd.com>
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/aie2_ctx.c | 22 +++++++++++++++-------
 drivers/accel/amdxdna/aie2_pci.c |  1 +
 drivers/accel/amdxdna/aie2_pci.h |  7 +------
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
index 101f324ee178..94dfee7263bd 100644
--- a/drivers/accel/amdxdna/aie2_ctx.c
+++ b/drivers/accel/amdxdna/aie2_ctx.c
@@ -43,20 +43,22 @@ struct aie2_ctx_health {
 
 static inline void aie2_tdr_signal(struct amdxdna_dev *xdna)
 {
-	WRITE_ONCE(xdna->dev_handle->tdr_status, AIE2_TDR_SIGNALED);
+	WRITE_ONCE(xdna->dev_handle->last_signal_ts, jiffies);
 }
 
 static bool aie2_tdr_detect(struct amdxdna_dev *xdna)
 {
 	struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
+	unsigned long last = READ_ONCE(ndev->last_signal_ts);
 
-	if (READ_ONCE(ndev->tdr_status) == AIE2_TDR_WAIT) {
-		XDNA_ERR(xdna, "TDR timeout detected");
-		return true;
-	}
+	if (!tdr_timeout_ms)
+		return false;
+
+	if (!time_after(jiffies, last + msecs_to_jiffies(tdr_timeout_ms)))
+		return false;
 
-	WRITE_ONCE(ndev->tdr_status, AIE2_TDR_WAIT);
-	return false;
+	XDNA_ERR(xdna, "TDR timeout detected");
+	return true;
 }
 
 static void aie2_cmd_release(struct kref *ref)
@@ -434,6 +436,12 @@ aie2_sched_job_run(struct drm_sched_job *sched_job)
 		mmput(job->mm);
 		fence = ERR_PTR(ret);
 	} else {
+		/*
+		 * Command is successfully posted to hardware, update the
+		 * tdr timestamp. The total pending commands are limited.
+		 * So there will not be a case that driver keeps posting
+		 * commands without getting any hardware respond.
+		 */
 		aie2_tdr_signal(hwctx->client->xdna);
 	}
 	trace_xdna_job(sched_job, hwctx->name, "sent to device",
diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
index 22f66c7f534d..daec1f6b4907 100644
--- a/drivers/accel/amdxdna/aie2_pci.c
+++ b/drivers/accel/amdxdna/aie2_pci.c
@@ -420,6 +420,7 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
 		goto stop_fw;
 	}
 
+	WRITE_ONCE(ndev->last_signal_ts, jiffies);
 	ndev->dev_status = AIE2_DEV_START;
 
 	return 0;
diff --git a/drivers/accel/amdxdna/aie2_pci.h b/drivers/accel/amdxdna/aie2_pci.h
index 77648cc548b6..ea1dac106400 100644
--- a/drivers/accel/amdxdna/aie2_pci.h
+++ b/drivers/accel/amdxdna/aie2_pci.h
@@ -143,11 +143,6 @@ struct aie2_exec_msg_ops {
 	u32 (*get_chain_msg_op)(u32 cmd_op);
 };
 
-enum aie2_tdr_status {
-	AIE2_TDR_WAIT,
-	AIE2_TDR_SIGNALED,
-};
-
 struct amdxdna_dev_hdl {
 	struct aie_device		aie;
 	const struct amdxdna_dev_priv	*priv;
@@ -179,7 +174,7 @@ struct amdxdna_dev_hdl {
 	u32				hwctx_num;
 
 	struct amdxdna_async_error	last_async_err;
-	enum aie2_tdr_status		tdr_status;
+	unsigned long			last_signal_ts;
 };
 
 struct aie2_hw_ops {
-- 
2.34.1

Re: [PATCH V1] accel/amdxdna: Fix command timeout race

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Setting tdr_timeout_ms = 0 causes an infinite workqueue spin loop, pegging a CPU core at 100%.
- [Medium] Non-deterministic timeout duration doubling due to strict time_after() check.

Pre-existing issues:
- [Critical] DRM scheduler is restarted before the hardware context is fully reinitialized during TDR, causing a race condition that leads to a kernel panic.
- [High] Device-global TDR timestamp tracking masks per-context (hardware context) hangs, preventing recovery.
--

--- Patch [1]: [PATCH V1] accel/amdxdna: Fix command timeout race ---
commit fd4b8a27528eea23d910e8bac5083a3f93093603
Author: Wendy Liang <wendy.liang@amd.com>
Date:   Sat Jul 18 08:34:09 2026 +0000

accel/amdxdna: Fix command timeout race

This commit replaces the TDR status state machine with a timestamp check to
prevent race conditions when multiple jobs time out concurrently. It uses
last_signal_ts to record the timestamp of the last driver signal.

> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -43,20 +43,22 @@ struct aie2_ctx_health {
>  
>  static inline void aie2_tdr_signal(struct amdxdna_dev *xdna)
>  {
> -	WRITE_ONCE(xdna->dev_handle->tdr_status, AIE2_TDR_SIGNALED);
> +	WRITE_ONCE(xdna->dev_handle->last_signal_ts, jiffies);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the global
timestamp tracking to mask per-context hangs?

If Context A hangs, but Context B continues to run and update last_signal_ts
via aie2_tdr_signal(), Context A's scheduler timeout in
aie2_sched_job_timedout() will call aie2_tdr_detect(). That function reads
the updated global timestamp and returns false, causing the DRM scheduler to
see NO_HANG and re-arm the timer forever, preventing recovery for Context A.

>  static bool aie2_tdr_detect(struct amdxdna_dev *xdna)
>  {
>  	struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
> +	unsigned long last = READ_ONCE(ndev->last_signal_ts);
>  
> -	if (READ_ONCE(ndev->tdr_status) == AIE2_TDR_WAIT) {
> -		XDNA_ERR(xdna, "TDR timeout detected");
> -		return true;
> -	}
> +	if (!tdr_timeout_ms)
> +		return false;

[Severity: High]
Can setting tdr_timeout_ms to 0 cause an infinite spin loop?

If an administrator sets tdr_timeout_ms = 0 to disable TDR, the DRM scheduler
is initialized with a 0-jiffy timeout. The delayed work fires immediately,
and since aie2_tdr_detect() returns false, the scheduler sees a NO_HANG
result and unconditionally re-arms the delayed work with a 0 timeout. This
pegs a CPU core at 100%.

> +	if (!time_after(jiffies, last + msecs_to_jiffies(tdr_timeout_ms)))
> +		return false;

[Severity: Medium]
Might this strict time_after() check cause the timeout duration to double?

If the DRM scheduler timer fires precisely when jiffies equals
last + msecs_to_jiffies(tdr_timeout_ms), the time_after() check evaluates
to false. This makes the driver return NO_HANG, which causes the DRM
scheduler to restart the wait for another full timeout period.

> -	WRITE_ONCE(ndev->tdr_status, AIE2_TDR_WAIT);
> -	return false;
> +	XDNA_ERR(xdna, "TDR timeout detected");
> +	return true;
>  }

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does aie2_sched_job_timedout() have a race
condition that leads to an uninitialized pointer dereference?

During a TDR event, aie2_sched_job_timedout() calls aie2_hwctx_stop()
followed by aie2_hwctx_restart(). If aie2_hwctx_stop() prematurely calls
drm_sched_start() before the hardware context is fully reinitialized, the
scheduler kthread is unparked early.

This allows the scheduler to pull new jobs and execute aie2_sched_job_run()
concurrently while the TDR thread is still running aie2_hwctx_restart().
If the scheduler thread observes hwctx->priv->mbox_chann as non-NULL before
its internal xarray is fully initialized via xdna_mailbox_start_channel(),
it will attempt to access the uninitialized memory when submitting a job.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718083409.1825940-1-lizhi.hou@amd.com?part=1