* Re: [PATCH] accel/amdxdna: make the debug BO command wait interruptible
2026-08-26 17:34 ` Lizhi Hou
@ 2026-08-26 18:07 ` Taimuraz Kaitmazov
2026-08-26 18:45 ` [PATCH v2] " Taimuraz Kaitmazov
2026-08-26 19:57 ` Taimuraz Kaitmazov
2 siblings, 0 replies; 12+ messages in thread
From: Taimuraz Kaitmazov @ 2026-08-26 18:07 UTC (permalink / raw)
To: Lizhi Hou, Min Ma, Oded Gabbay; +Cc: dri-devel, linux-kernel
I found out about that fix only after I had created this patch, but I
still want to state that the fix is not full: it unwedges the device,
but not the tasks already waiting. Those stay in an uninterruptible
wait, holding the debug BO and the command, and cannot be killed.
tdr_timeout_ms bounds that at 2 s by default; with tdr_timeout_ms=0 it
is unbounded.
I am still researching whether any race conditions can happen in the
current driver state.
Taimuraz
On 8/26/26 20:34, Lizhi Hou wrote:
> On 8/26/26 08:07, Taimuraz Kaitmazov wrote:
>> aie2_cmd_wait() waits with dma_fence_wait_timeout(..., false,
>> MAX_SCHEDULE_TIMEOUT): uninterruptible and unbounded. Both callers reach
>> it from an ioctl holding xdna->dev_lock, and the only thing that can
>> signal the fence when firmware does not answer is
>> aie2_sched_job_timedout(), which takes that same lock. A debug BO
>> command
>> that never completes therefore blocks the ioctl forever, leaves the task
>> unkillable, and stalls every other ioctl on the device behind dev_lock.
>>
>> Wait interruptibly and propagate the result. This does not remove the
>> lock dependency, it makes it survivable.
>
> This is fixed by
> https://lore.kernel.org/all/20260707055732.479103-1-lizhi.hou@amd.com/
>
> could you sync to drm-misc-fixes or the latest upstream kernel?
>
> Thanks,
>
> Lizhi
>
>> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
>> ---
>> drivers/accel/amdxdna/aie2_ctx.c | 19 ++++++++++++++-----
>> 1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/aie2_ctx.c
>> b/drivers/accel/amdxdna/aie2_ctx.c
>> index 54486960cbf5..1f910ee4941c 100644
>> --- a/drivers/accel/amdxdna/aie2_ctx.c
>> +++ b/drivers/accel/amdxdna/aie2_ctx.c
>> @@ -897,17 +897,20 @@ static int aie2_hwctx_cu_config(struct
>> amdxdna_hwctx *hwctx, void *buf, u32 size
>> return ret;
>> }
>> -static void aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
>> +static int aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
>> {
>> struct dma_fence *out_fence = aie2_cmd_get_out_fence(hwctx, seq);
>> + long ret;
>> if (!out_fence) {
>> XDNA_ERR(hwctx->client->xdna, "Failed to get fence");
>> - return;
>> + return -EINVAL;
>> }
>> - dma_fence_wait_timeout(out_fence, false, MAX_SCHEDULE_TIMEOUT);
>> + ret = dma_fence_wait_timeout(out_fence, true,
>> MAX_SCHEDULE_TIMEOUT);
>> dma_fence_put(out_fence);
>> +
>> + return ret < 0 ? ret : 0;
>> }
>> static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx,
>> u32 bo_hdl,
>> @@ -954,7 +957,10 @@ static int aie2_hwctx_cfg_debug_bo(struct
>> amdxdna_hwctx *hwctx, u32 bo_hdl,
>> goto put_cmd;
>> }
>> - aie2_cmd_wait(hwctx, seq);
>> + ret = aie2_cmd_wait(hwctx, seq);
>> + if (ret)
>> + goto put_cmd;
>> +
>> if (cmd->result) {
>> XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
>> ret = -EINVAL;
>> @@ -1014,7 +1020,10 @@ int aie2_hwctx_sync_debug_bo(struct
>> amdxdna_hwctx *hwctx, u32 debug_bo_hdl)
>> goto put_cmd;
>> }
>> - aie2_cmd_wait(hwctx, seq);
>> + ret = aie2_cmd_wait(hwctx, seq);
>> + if (ret)
>> + goto put_cmd;
>> +
>> if (cmd->result) {
>> XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
>> ret = -EINVAL;
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2] accel/amdxdna: make the debug BO command wait interruptible
2026-08-26 17:34 ` Lizhi Hou
2026-08-26 18:07 ` Taimuraz Kaitmazov
@ 2026-08-26 18:45 ` Taimuraz Kaitmazov
2026-08-26 18:59 ` sashiko-bot
2026-08-26 20:04 ` Lizhi Hou
2026-08-26 19:57 ` Taimuraz Kaitmazov
2 siblings, 2 replies; 12+ messages in thread
From: Taimuraz Kaitmazov @ 2026-08-26 18:45 UTC (permalink / raw)
To: Lizhi Hou, Min Ma, Oded Gabbay; +Cc: taimuraz, dri-devel, linux-kernel
aie2_cmd_wait() waits uninterruptibly and without a timeout. Commit
c8d2530791cb ("accel/amdxdna: Fix deadlock on debug BO command timeout")
drops dev_lock around that wait, but the callers still hold client_lock,
and that one is device wide. So while a debug BO command is outstanding,
nobody can open the device, nobody can close their DRM file and exit, and
the module cannot be unloaded.
Normally the scheduler timeout ends it: tdr_timeout_ms defaults to 2000.
But with tdr_timeout_ms=0 there is no scheduler timeout at all, the wait
never ends, and since it is uninterruptible you cannot even kill the
stuck task to get client_lock back.
So wait interruptibly and pass the result up. I return -EINTR and not
-ERESTARTSYS on purpose: the command is still in flight, and a restarted
ioctl would just submit a second one.
If the wait is interrupted we do not know what firmware did, so
aie2_hwctx_cfg_debug_bo() cannot tell whether the BO got attached. It
records it as attached, because DETACH is refused for a BO that is not
assigned to the context, and the other way round would leave a BO
attached in firmware with no way to detach it. aie2_hwctx_sync_debug_bo()
does not touch that state, so it needs nothing here.
Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
v2:
- Rebased onto drm-misc-fixes, as you asked. v1 was against
drm-misc-next, which does not carry that commit, so the deadlock
reasoning in its message is gone. Please also disregard my follow-up
question on the v1 thread: it asked which of two shapes you would
prefer, and one of them was what you had already done.
- Return -EINTR instead of -ERESTARTSYS.
- Record the debug BO as attached when the wait is interrupted.
- Leave the !out_fence path returning success as before, so this patch
changes only the wait.
drivers/accel/amdxdna/aie2_ctx.c | 33 +++++++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
index b713a57b3a3b..175fceea5c8f 100644
--- a/drivers/accel/amdxdna/aie2_ctx.c
+++ b/drivers/accel/amdxdna/aie2_ctx.c
@@ -908,20 +908,30 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx *hwctx, void *buf, u32 size
return ret;
}
-static void aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
+static int aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
{
struct dma_fence *out_fence = aie2_cmd_get_out_fence(hwctx, seq);
struct amdxdna_dev *xdna = hwctx->client->xdna;
+ long ret;
if (!out_fence) {
XDNA_ERR(xdna, "Failed to get fence");
- return;
+ return 0;
}
mutex_unlock(&xdna->dev_lock);
- dma_fence_wait_timeout(out_fence, false, MAX_SCHEDULE_TIMEOUT);
+ ret = dma_fence_wait_timeout(out_fence, true, MAX_SCHEDULE_TIMEOUT);
mutex_lock(&xdna->dev_lock);
dma_fence_put(out_fence);
+
+ /*
+ * The command is still in flight. If the ioctl were restarted it
+ * would submit a second one.
+ */
+ if (ret == -ERESTARTSYS)
+ return -EINTR;
+
+ return ret < 0 ? ret : 0;
}
static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx, u32 bo_hdl,
@@ -968,7 +978,17 @@ static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx, u32 bo_hdl,
goto put_cmd;
}
- aie2_cmd_wait(hwctx, seq);
+ ret = aie2_cmd_wait(hwctx, seq);
+ if (ret) {
+ /*
+ * The command may still attach the BO. Record it as attached,
+ * or DETACH will refuse it and there is no way back.
+ */
+ if (attach)
+ abo->assigned_hwctx = hwctx->id;
+ goto put_cmd;
+ }
+
if (cmd->result) {
XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
ret = -EINVAL;
@@ -1028,7 +1048,10 @@ int aie2_hwctx_sync_debug_bo(struct amdxdna_hwctx *hwctx, u32 debug_bo_hdl)
goto put_cmd;
}
- aie2_cmd_wait(hwctx, seq);
+ ret = aie2_cmd_wait(hwctx, seq);
+ if (ret)
+ goto put_cmd;
+
if (cmd->result) {
XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
ret = -EINVAL;
--
2.55.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2] accel/amdxdna: make the debug BO command wait interruptible
2026-08-26 18:45 ` [PATCH v2] " Taimuraz Kaitmazov
@ 2026-08-26 18:59 ` sashiko-bot
2026-08-26 20:04 ` Lizhi Hou
1 sibling, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-08-26 18:59 UTC (permalink / raw)
To: Taimuraz Kaitmazov; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Interrupting `aie2_cmd_wait()` during debug BO configuration can leave the BO's assignment state permanently inconsistent between the driver and firmware.
Pre-existing issues:
- [High] Failure to obtain the out fence causes the driver to incorrectly assume successful hardware execution.
- [High] `aie2_cmd_wait` ignores fence errors, leading to silent state corruption if a DRM scheduler job times out or is aborted.
--
commit 05f58ac37b249cd07c7d16bba1c96b7014f1993a
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
accel/amdxdna: make the debug BO command wait interruptible
This commit updates aie2_cmd_wait() to use an interruptible fence wait
and handles interrupted debug BO ATTACH operations by optimistically
marking them as attached.
> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index baf9a8b90a4d6..d3c80f760a1d3 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -908,20 +908,30 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx *hwctx, void *buf, u32 size
> return ret;
> }
>
> -static void aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
> +static int aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
> {
> struct dma_fence *out_fence = aie2_cmd_get_out_fence(hwctx, seq);
> struct amdxdna_dev *xdna = hwctx->client->xdna;
> + long ret;
>
> if (!out_fence) {
> XDNA_ERR(xdna, "Failed to get fence");
> - return;
> + return 0;
[Severity: High]
This is a pre-existing issue, but does returning 0 here mask the error from
the caller?
If aie2_cmd_get_out_fence() fails to return a fence, returning 0 signals
success to the caller in aie2_hwctx_cfg_debug_bo(). Because cmd->result
remains 0, the caller will incorrectly assume the hardware executed the
command successfully, potentially corrupting internal driver state.
> }
>
> mutex_unlock(&xdna->dev_lock);
> - dma_fence_wait_timeout(out_fence, false, MAX_SCHEDULE_TIMEOUT);
> + ret = dma_fence_wait_timeout(out_fence, true, MAX_SCHEDULE_TIMEOUT);
> mutex_lock(&xdna->dev_lock);
> dma_fence_put(out_fence);
> +
> + /*
> + * The command is still in flight. If the ioctl were restarted it
> + * would submit a second one.
> + */
> + if (ret == -ERESTARTSYS)
> + return -EINTR;
> +
> + return ret < 0 ? ret : 0;
[Severity: High]
This is a pre-existing issue, but does this logic ignore asynchronous fence
errors?
When a DRM scheduler job is aborted (such as during a TDR timeout), it will
signal the job's out-fence with an error code. Since dma_fence_wait_timeout()
returns > 0 (time remaining) when the fence signals, returning 0 here without
checking dma_fence_get_error(out_fence) hides the failure.
The caller will proceed as if the hardware completed the command successfully.
> }
>
> static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx, u32 bo_hdl,
> @@ -968,7 +978,17 @@ static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx, u32 bo_hdl,
> goto put_cmd;
> }
>
> - aie2_cmd_wait(hwctx, seq);
> + ret = aie2_cmd_wait(hwctx, seq);
> + if (ret) {
> + /*
> + * The command may still attach the BO. Record it as attached,
> + * or DETACH will refuse it and there is no way back.
> + */
> + if (attach)
> + abo->assigned_hwctx = hwctx->id;
> + goto put_cmd;
> + }
> +
> if (cmd->result) {
> XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
> ret = -EINVAL;
[Severity: High]
Could this lead to a permanently wedged buffer object?
If aie2_cmd_wait() is interrupted during an ATTACH operation, the driver
optimistically marks the BO as attached by setting abo->assigned_hwctx.
However, if the firmware did not actually process the ATTACH command, a
subsequent DETACH operation will fail in firmware and return a non-zero
cmd->result.
When cmd->result is non-zero during the DETACH, this error handling path
bails out early, skipping the cleanup logic below that would normally reset
abo->assigned_hwctx to AMDXDNA_INVALID_CTX_HANDLE. This leaves the BO state
mismatched between the driver and firmware, locking it permanently.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260826184503.282106-1-taimuraz@kaitmazov.com?part=1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] accel/amdxdna: make the debug BO command wait interruptible
2026-08-26 18:45 ` [PATCH v2] " Taimuraz Kaitmazov
2026-08-26 18:59 ` sashiko-bot
@ 2026-08-26 20:04 ` Lizhi Hou
2026-08-26 21:23 ` Taimuraz Kaitmazov
1 sibling, 1 reply; 12+ messages in thread
From: Lizhi Hou @ 2026-08-26 20:04 UTC (permalink / raw)
To: Taimuraz Kaitmazov, Min Ma, Oded Gabbay; +Cc: dri-devel, linux-kernel
On 8/26/26 11:45, Taimuraz Kaitmazov wrote:
> aie2_cmd_wait() waits uninterruptibly and without a timeout. Commit
> c8d2530791cb ("accel/amdxdna: Fix deadlock on debug BO command timeout")
> drops dev_lock around that wait, but the callers still hold client_lock,
> and that one is device wide. So while a debug BO command is outstanding,
> nobody can open the device, nobody can close their DRM file and exit, and
> the module cannot be unloaded.
That is expected. It relies on the 2 seconds timeout to remove the
context, kill the job and return. Exiting earlier and allowing other
operations like open/close/remove module while firmware is dealing with
command may cause other issues. And It should never happen that the
command runs more than 2 seconds unless a firmware/hardware issue.
>
> Normally the scheduler timeout ends it: tdr_timeout_ms defaults to 2000.
> But with tdr_timeout_ms=0 there is no scheduler timeout at all, the wait
> never ends, and since it is uninterruptible you cannot even kill the
> stuck task to get client_lock back.
tdr_timeout_ms is used for debugging. User should never change it.
Lizhi
>
> So wait interruptibly and pass the result up. I return -EINTR and not
> -ERESTARTSYS on purpose: the command is still in flight, and a restarted
> ioctl would just submit a second one.
>
> If the wait is interrupted we do not know what firmware did, so
> aie2_hwctx_cfg_debug_bo() cannot tell whether the BO got attached. It
> records it as attached, because DETACH is refused for a BO that is not
> assigned to the context, and the other way round would leave a BO
> attached in firmware with no way to detach it. aie2_hwctx_sync_debug_bo()
> does not touch that state, so it needs nothing here.
>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
> v2:
> - Rebased onto drm-misc-fixes, as you asked. v1 was against
> drm-misc-next, which does not carry that commit, so the deadlock
> reasoning in its message is gone. Please also disregard my follow-up
> question on the v1 thread: it asked which of two shapes you would
> prefer, and one of them was what you had already done.
> - Return -EINTR instead of -ERESTARTSYS.
> - Record the debug BO as attached when the wait is interrupted.
> - Leave the !out_fence path returning success as before, so this patch
> changes only the wait.
>
> drivers/accel/amdxdna/aie2_ctx.c | 33 +++++++++++++++++++++++++++-----
> 1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index b713a57b3a3b..175fceea5c8f 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -908,20 +908,30 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx *hwctx, void *buf, u32 size
> return ret;
> }
>
> -static void aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
> +static int aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
> {
> struct dma_fence *out_fence = aie2_cmd_get_out_fence(hwctx, seq);
> struct amdxdna_dev *xdna = hwctx->client->xdna;
> + long ret;
>
> if (!out_fence) {
> XDNA_ERR(xdna, "Failed to get fence");
> - return;
> + return 0;
> }
>
> mutex_unlock(&xdna->dev_lock);
> - dma_fence_wait_timeout(out_fence, false, MAX_SCHEDULE_TIMEOUT);
> + ret = dma_fence_wait_timeout(out_fence, true, MAX_SCHEDULE_TIMEOUT);
> mutex_lock(&xdna->dev_lock);
> dma_fence_put(out_fence);
> +
> + /*
> + * The command is still in flight. If the ioctl were restarted it
> + * would submit a second one.
> + */
> + if (ret == -ERESTARTSYS)
> + return -EINTR;
> +
> + return ret < 0 ? ret : 0;
> }
>
> static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx, u32 bo_hdl,
> @@ -968,7 +978,17 @@ static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx, u32 bo_hdl,
> goto put_cmd;
> }
>
> - aie2_cmd_wait(hwctx, seq);
> + ret = aie2_cmd_wait(hwctx, seq);
> + if (ret) {
> + /*
> + * The command may still attach the BO. Record it as attached,
> + * or DETACH will refuse it and there is no way back.
> + */
> + if (attach)
> + abo->assigned_hwctx = hwctx->id;
> + goto put_cmd;
> + }
> +
> if (cmd->result) {
> XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
> ret = -EINVAL;
> @@ -1028,7 +1048,10 @@ int aie2_hwctx_sync_debug_bo(struct amdxdna_hwctx *hwctx, u32 debug_bo_hdl)
> goto put_cmd;
> }
>
> - aie2_cmd_wait(hwctx, seq);
> + ret = aie2_cmd_wait(hwctx, seq);
> + if (ret)
> + goto put_cmd;
> +
> if (cmd->result) {
> XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
> ret = -EINVAL;
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] accel/amdxdna: make the debug BO command wait interruptible
2026-08-26 20:04 ` Lizhi Hou
@ 2026-08-26 21:23 ` Taimuraz Kaitmazov
0 siblings, 0 replies; 12+ messages in thread
From: Taimuraz Kaitmazov @ 2026-08-26 21:23 UTC (permalink / raw)
To: Lizhi Hou, Min Ma, Oded Gabbay; +Cc: dri-devel, linux-kernel
Thanks for the response. I agree on the design: interrupting the wait
does not stop the command, so returning from the ioctl while firmware
still owns the BO trades one problem for another. Dropping this patch.
Still, one more thing which might be useful. The 2 s bound holds only
while the device is otherwise idle: last_signal_ts is per device and
refreshed on every dispatch, so a busy neighbour keeps aie2_tdr_detect()
returning false and the job is never reset. The comment there reasons
from the per-hwctx credit limit, but the timestamp lives on the device.
Taimuraz
On 8/26/26 23:04, Lizhi Hou wrote:
>
> On 8/26/26 11:45, Taimuraz Kaitmazov wrote:
>> aie2_cmd_wait() waits uninterruptibly and without a timeout. Commit
>> c8d2530791cb ("accel/amdxdna: Fix deadlock on debug BO command timeout")
>> drops dev_lock around that wait, but the callers still hold client_lock,
>> and that one is device wide. So while a debug BO command is outstanding,
>> nobody can open the device, nobody can close their DRM file and exit,
>> and
>> the module cannot be unloaded.
>
> That is expected. It relies on the 2 seconds timeout to remove the
> context, kill the job and return. Exiting earlier and allowing other
> operations like open/close/remove module while firmware is dealing
> with command may cause other issues. And It should never happen that
> the command runs more than 2 seconds unless a firmware/hardware issue.
>
>>
>> Normally the scheduler timeout ends it: tdr_timeout_ms defaults to 2000.
>> But with tdr_timeout_ms=0 there is no scheduler timeout at all, the wait
>> never ends, and since it is uninterruptible you cannot even kill the
>> stuck task to get client_lock back.
>
> tdr_timeout_ms is used for debugging. User should never change it.
>
> Lizhi
>
>>
>> So wait interruptibly and pass the result up. I return -EINTR and not
>> -ERESTARTSYS on purpose: the command is still in flight, and a restarted
>> ioctl would just submit a second one.
>>
>> If the wait is interrupted we do not know what firmware did, so
>> aie2_hwctx_cfg_debug_bo() cannot tell whether the BO got attached. It
>> records it as attached, because DETACH is refused for a BO that is not
>> assigned to the context, and the other way round would leave a BO
>> attached in firmware with no way to detach it.
>> aie2_hwctx_sync_debug_bo()
>> does not touch that state, so it needs nothing here.
>>
>> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
>> ---
>> v2:
>> - Rebased onto drm-misc-fixes, as you asked. v1 was against
>> drm-misc-next, which does not carry that commit, so the deadlock
>> reasoning in its message is gone. Please also disregard my follow-up
>> question on the v1 thread: it asked which of two shapes you would
>> prefer, and one of them was what you had already done.
>> - Return -EINTR instead of -ERESTARTSYS.
>> - Record the debug BO as attached when the wait is interrupted.
>> - Leave the !out_fence path returning success as before, so this patch
>> changes only the wait.
>>
>> drivers/accel/amdxdna/aie2_ctx.c | 33 +++++++++++++++++++++++++++-----
>> 1 file changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/aie2_ctx.c
>> b/drivers/accel/amdxdna/aie2_ctx.c
>> index b713a57b3a3b..175fceea5c8f 100644
>> --- a/drivers/accel/amdxdna/aie2_ctx.c
>> +++ b/drivers/accel/amdxdna/aie2_ctx.c
>> @@ -908,20 +908,30 @@ static int aie2_hwctx_cu_config(struct
>> amdxdna_hwctx *hwctx, void *buf, u32 size
>> return ret;
>> }
>> -static void aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
>> +static int aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
>> {
>> struct dma_fence *out_fence = aie2_cmd_get_out_fence(hwctx, seq);
>> struct amdxdna_dev *xdna = hwctx->client->xdna;
>> + long ret;
>> if (!out_fence) {
>> XDNA_ERR(xdna, "Failed to get fence");
>> - return;
>> + return 0;
>> }
>> mutex_unlock(&xdna->dev_lock);
>> - dma_fence_wait_timeout(out_fence, false, MAX_SCHEDULE_TIMEOUT);
>> + ret = dma_fence_wait_timeout(out_fence, true,
>> MAX_SCHEDULE_TIMEOUT);
>> mutex_lock(&xdna->dev_lock);
>> dma_fence_put(out_fence);
>> +
>> + /*
>> + * The command is still in flight. If the ioctl were restarted it
>> + * would submit a second one.
>> + */
>> + if (ret == -ERESTARTSYS)
>> + return -EINTR;
>> +
>> + return ret < 0 ? ret : 0;
>> }
>> static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx,
>> u32 bo_hdl,
>> @@ -968,7 +978,17 @@ static int aie2_hwctx_cfg_debug_bo(struct
>> amdxdna_hwctx *hwctx, u32 bo_hdl,
>> goto put_cmd;
>> }
>> - aie2_cmd_wait(hwctx, seq);
>> + ret = aie2_cmd_wait(hwctx, seq);
>> + if (ret) {
>> + /*
>> + * The command may still attach the BO. Record it as attached,
>> + * or DETACH will refuse it and there is no way back.
>> + */
>> + if (attach)
>> + abo->assigned_hwctx = hwctx->id;
>> + goto put_cmd;
>> + }
>> +
>> if (cmd->result) {
>> XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
>> ret = -EINVAL;
>> @@ -1028,7 +1048,10 @@ int aie2_hwctx_sync_debug_bo(struct
>> amdxdna_hwctx *hwctx, u32 debug_bo_hdl)
>> goto put_cmd;
>> }
>> - aie2_cmd_wait(hwctx, seq);
>> + ret = aie2_cmd_wait(hwctx, seq);
>> + if (ret)
>> + goto put_cmd;
>> +
>> if (cmd->result) {
>> XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
>> ret = -EINVAL;
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] accel/amdxdna: make the debug BO command wait interruptible
2026-08-26 17:34 ` Lizhi Hou
2026-08-26 18:07 ` Taimuraz Kaitmazov
2026-08-26 18:45 ` [PATCH v2] " Taimuraz Kaitmazov
@ 2026-08-26 19:57 ` Taimuraz Kaitmazov
2026-08-26 20:14 ` sashiko-bot
2 siblings, 1 reply; 12+ messages in thread
From: Taimuraz Kaitmazov @ 2026-08-26 19:57 UTC (permalink / raw)
To: Lizhi Hou, Min Ma, Oded Gabbay; +Cc: taimuraz, dri-devel, linux-kernel
aie2_cmd_wait() waits uninterruptibly and without a timeout. Commit
c8d2530791cb ("accel/amdxdna: Fix deadlock on debug BO command timeout")
drops dev_lock across it, but the callers still hold client_lock, which is
device wide: while a debug BO command is outstanding nobody can open the
device, close their DRM file, or unload the module. tdr_timeout_ms=0 makes
that unbounded, and the task cannot be killed to release the lock.
Wait interruptibly. Return -EINTR, not -ERESTARTSYS: the command stays in
flight and a restarted ioctl would submit a second one. An interrupted
ATTACH leaves the outcome unknown, so record the BO as attached, since
DETACH is refused otherwise.
Fixes: 7ea046838021 ("accel/amdxdna: Support firmware debug buffer")
Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
v2:
- Rebased onto drm-misc-fixes, as you asked; the deadlock reasoning in v1
is gone with it. Please also disregard my follow-up question on the v1
thread, it asked about a shape you had already taken.
- Added the Fixes: tag c8d2530791cb carries for this function.
- Return -EINTR instead of -ERESTARTSYS.
- Record the debug BO as attached when the wait is interrupted.
- Leave the !out_fence path returning success, so only the wait changes.
drivers/accel/amdxdna/aie2_ctx.c | 33 +++++++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
index b713a57b3a3b..175fceea5c8f 100644
--- a/drivers/accel/amdxdna/aie2_ctx.c
+++ b/drivers/accel/amdxdna/aie2_ctx.c
@@ -908,20 +908,30 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx *hwctx, void *buf, u32 size
return ret;
}
-static void aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
+static int aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
{
struct dma_fence *out_fence = aie2_cmd_get_out_fence(hwctx, seq);
struct amdxdna_dev *xdna = hwctx->client->xdna;
+ long ret;
if (!out_fence) {
XDNA_ERR(xdna, "Failed to get fence");
- return;
+ return 0;
}
mutex_unlock(&xdna->dev_lock);
- dma_fence_wait_timeout(out_fence, false, MAX_SCHEDULE_TIMEOUT);
+ ret = dma_fence_wait_timeout(out_fence, true, MAX_SCHEDULE_TIMEOUT);
mutex_lock(&xdna->dev_lock);
dma_fence_put(out_fence);
+
+ /*
+ * The command is still in flight. If the ioctl were restarted it
+ * would submit a second one.
+ */
+ if (ret == -ERESTARTSYS)
+ return -EINTR;
+
+ return ret < 0 ? ret : 0;
}
static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx, u32 bo_hdl,
@@ -968,7 +978,17 @@ static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx, u32 bo_hdl,
goto put_cmd;
}
- aie2_cmd_wait(hwctx, seq);
+ ret = aie2_cmd_wait(hwctx, seq);
+ if (ret) {
+ /*
+ * The command may still attach the BO. Record it as attached,
+ * or DETACH will refuse it and there is no way back.
+ */
+ if (attach)
+ abo->assigned_hwctx = hwctx->id;
+ goto put_cmd;
+ }
+
if (cmd->result) {
XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
ret = -EINVAL;
@@ -1028,7 +1048,10 @@ int aie2_hwctx_sync_debug_bo(struct amdxdna_hwctx *hwctx, u32 debug_bo_hdl)
goto put_cmd;
}
- aie2_cmd_wait(hwctx, seq);
+ ret = aie2_cmd_wait(hwctx, seq);
+ if (ret)
+ goto put_cmd;
+
if (cmd->result) {
XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
ret = -EINVAL;
--
2.55.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2] accel/amdxdna: make the debug BO command wait interruptible
2026-08-26 19:57 ` Taimuraz Kaitmazov
@ 2026-08-26 20:14 ` sashiko-bot
0 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-08-26 20:14 UTC (permalink / raw)
To: Taimuraz Kaitmazov; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] Returning `-EINTR` while leaving the software state modified breaks standard ioctl restart semantics.
Pre-existing issues:
- [High] Hardware Page Fault / Use-After-Free when debug BO is closed while attached.
- [High] Missing Error Handling in aie2_cmd_wait allows state desynchronization.
- [High] Denial of Service via device-wide client_lock during unbounded wait.
--
commit 5562369312b08033baebb62691f056cab3d5a6c1
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
Subject: accel/amdxdna: make the debug BO command wait interruptible
This commit changes aie2_cmd_wait() to wait interruptibly instead of
uninterruptibly, returning -EINTR on interruption. It also leaves the BO
attached when interrupted to prevent detachment failures if the hardware
is successfully attached.
> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index baf9a8b90a4d6..d3c80f760a1d3 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -908,20 +908,30 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx *hwctx, void *buf, u32 size
> return ret;
> }
>
> -static void aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
> +static int aie2_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq)
> {
> struct dma_fence *out_fence = aie2_cmd_get_out_fence(hwctx, seq);
> struct amdxdna_dev *xdna = hwctx->client->xdna;
> + long ret;
>
> if (!out_fence) {
> XDNA_ERR(xdna, "Failed to get fence");
> - return;
> + return 0;
[Severity: High]
This is a pre-existing issue, but does returning 0 here indicate success to
the caller despite the inability to wait for the command?
If aie2_cmd_get_out_fence() fails (e.g., an unprivileged user concurrently
resets hwctx->syncobj_hdl), the driver skips waiting for the hardware but
proceeds as if the command finished.
It seems this would cause subsequent logic to evaluate an uninitialized or
zeroed cmd->result, leading to software state desynchronization.
> }
>
> mutex_unlock(&xdna->dev_lock);
> - dma_fence_wait_timeout(out_fence, false, MAX_SCHEDULE_TIMEOUT);
> + ret = dma_fence_wait_timeout(out_fence, true, MAX_SCHEDULE_TIMEOUT);
[Severity: High]
This is a pre-existing issue, but is it safe to sleep here for potentially an
unbounded amount of time without dropping the device-wide client_lock?
While dev_lock is explicitly dropped, the caller in
amdxdna_drm_config_hwctx_ioctl() acquires the device-wide client_lock before
initiating the wait:
drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_drm_config_hwctx_ioctl() {
...
mutex_lock(&xdna->client_lock);
mutex_lock(&xdna->dev_lock);
...
}
An unprivileged user could submit a long-running compute job, issue a config
ioctl (e.g., ATTACH) that waits behind the compute job, and simply let their
thread sleep without sending a signal.
This would keep the device-wide client_lock held indefinitely, blocking all
other users from opening or closing the device.
> mutex_lock(&xdna->dev_lock);
> dma_fence_put(out_fence);
[ ... ]
> @@ -968,7 +978,17 @@ static int aie2_hwctx_cfg_debug_bo(struct amdxdna_hwctx *hwctx, u32 bo_hdl,
> goto put_cmd;
> }
>
> - aie2_cmd_wait(hwctx, seq);
> + ret = aie2_cmd_wait(hwctx, seq);
> + if (ret) {
> + /*
> + * The command may still attach the BO. Record it as attached,
> + * or DETACH will refuse it and there is no way back.
> + */
> + if (attach)
> + abo->assigned_hwctx = hwctx->id;
> + goto put_cmd;
> + }
> +
> if (cmd->result) {
> XDNA_ERR(xdna, "Response failure 0x%x", cmd->result);
> ret = -EINVAL;
[Severity: High]
Does returning -EINTR while leaving the software state modified break standard
ioctl restart semantics?
When aie2_cmd_wait() is interrupted, userspace receives -EINTR and will
typically retry the identical syscall.
If an interrupted ATTACH_DEBUG_BO proactively sets abo->assigned_hwctx,
userspace treating -EINTR as a failure will not issue a subsequent DETACH,
permanently stranding the BO.
If a DETACH_DEBUG_BO is interrupted, abo->assigned_hwctx intentionally skips
cleanup.
If the hardware successfully detached the BO, retrying the ioctl sends another
DETACH command. The firmware rejects the redundant detach, and the driver
handles the failure here in the cmd->result check.
This again skips clearing abo->assigned_hwctx, cementing the
desynchronization and leaving the BO permanently stuck as "attached".
[Severity: High]
This is a pre-existing issue, but does dropping the reference to the BO after
the goto put_cmd; jump allow a use-after-free and hardware page fault?
When aie2_hwctx_cfg_debug_bo() jumps to put_cmd, it falls through to put_obj:
put_obj:
amdxdna_gem_put_obj(abo);
return ret;
The reference to the BO is completely dropped before returning, meaning the
kernel does not artificially keep the BO alive while it remains attached to
the hardware context.
If user space closes the BO handle while it is attached, the BO is destroyed:
drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_gem_dev_obj_free() {
...
if (abo->pinned)
amdxdna_gem_unpin(abo);
amdxdna_gem_vunmap(abo);
amdxdna_gem_heap_free(abo);
drm_gem_object_release(gobj);
amdxdna_gem_destroy_obj(abo);
}
The BO is freed and unmapped from the IOMMU without checking
abo->assigned_hwctx or sending a DETACH command to the hardware.
The hardware context remains configured to write debug data to the freed
memory.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260826195751.358629-1-taimuraz@kaitmazov.com?part=1
^ permalink raw reply [flat|nested] 12+ messages in thread