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

[sashiko-bot@kernel.org]

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