dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] accel/amdxdna: make the debug BO command wait interruptible
@ 2026-08-26 15:07 Taimuraz Kaitmazov
  2026-08-26 15:10 ` Taimuraz Kaitmazov
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Taimuraz Kaitmazov @ 2026-08-26 15:07 UTC (permalink / raw)
  To: Lizhi Hou, Min Ma, Oded Gabbay; +Cc: taimuraz, dri-devel, linux-kernel

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.

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] accel/amdxdna: make the debug BO command wait interruptible
  2026-08-26 15:07 [PATCH] accel/amdxdna: make the debug BO command wait interruptible Taimuraz Kaitmazov
@ 2026-08-26 15:10 ` Taimuraz Kaitmazov
  2026-08-26 15:21 ` sashiko-bot
  2026-08-26 17:34 ` Lizhi Hou
  2 siblings, 0 replies; 12+ messages in thread
From: Taimuraz Kaitmazov @ 2026-08-26 15:10 UTC (permalink / raw)
  To: Lizhi Hou, Min Ma, Oded Gabbay; +Cc: taimuraz, dri-devel, linux-kernel

This makes the wait survivable but leaves the lock cycle in place: both
callers hold dev_lock across aie2_cmd_wait(), and aie2_sched_job_timedout()
needs that same lock before it can force the fence to signal.

The wait itself does not need the lock. aie2_cmd_get_out_fence() already
returns a referenced fence and drv_cmd is krefed, so the sequence could be:
submit and take the fence under dev_lock, drop the lock, wait, retake it.
Nothing after the wait touches hwctx. amdxdna_pm_resume_get_locked() and
amdxdna_update_heap() already drop a caller's lock this way, so the shape is
not new to this driver.

The alternative is to change the op contract instead: have hwctx_config() and
hwctx_sync_debug_bo() return once the command is submitted and let the ioctl
do the waiting, which keeps the lock discipline in one place.

Which of the two would you rather see? I have not been able to reproduce a
debug BO command that does not complete, so I would rather ask than guess at
the invariants that have to hold across the unlocked window.

Taimuraz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] accel/amdxdna: make the debug BO command wait interruptible
  2026-08-26 15:07 [PATCH] accel/amdxdna: make the debug BO command wait interruptible Taimuraz Kaitmazov
  2026-08-26 15:10 ` Taimuraz Kaitmazov
@ 2026-08-26 15:21 ` sashiko-bot
  2026-08-26 16:12   ` Taimuraz Kaitmazov
  2026-08-26 17:34 ` Lizhi Hou
  2 siblings, 1 reply; 12+ messages in thread
From: sashiko-bot @ 2026-08-26 15:21 UTC (permalink / raw)
  To: Taimuraz Kaitmazov; +Cc: dri-devel

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

New issues:
- [High] Interruptible wait causes software state to desynchronize from hardware, potentially leading to IOMMU faults or memory corruption.

Pre-existing issues:
- [High] Unresolved ABBA deadlock between `xdna->dev_lock` and the DRM TDR (Timeout Detection and Recovery) handler permanently wedges the device on a hardware hang.
--

commit ce5b5167020e9399e64c273e9bded5901cfbc67d
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: make the debug BO command wait interruptible

This commit changes the aie2_cmd_wait() function to wait for debug BO
commands interruptibly instead of uninterruptibly. It propagates the return
code up to the callers to allow userspace processes blocked on a hardware
hang to be killed.

> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index 54486960cbf5e..1f910ee4941c9 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;
>  }

[Severity: High]
This is a pre-existing issue, but does making the wait interruptible fully
resolve the underlying deadlock with the timeout handler?

While this prevents an unkillable task, the ioctl still holds xdna->dev_lock
across the wait:

amdxdna_drm_config_hwctx_ioctl()
   guard(mutex)(&xdna->dev_lock);
   ...
   aie2_hwctx_cfg_debug_bo()
      aie2_cmd_wait()
         dma_fence_wait_timeout()

If the firmware hangs, the DRM scheduler invokes aie2_sched_job_timedout()
to recover. However, aie2_sched_job_timedout() also requires xdna->dev_lock.
This deadlocks against the waiting ioctl thread, preventing automatic TDR
recovery unless the waiting process is manually killed.

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

[Severity: High]
If the wait is interrupted and returns an error, could this early return
cause the software state to desynchronize from the hardware?

By taking the goto put_cmd path, the function skips the state update later
in the function:

aie2_hwctx_cfg_debug_bo() {
    ...
    if (attach)
        abo->assigned_hwctx = hwctx->id;
    else
        abo->assigned_hwctx = AMDXDNA_INVALID_CTX_HANDLE;
    ...
}

If the hardware successfully processes the command but the software update
is skipped due to the interrupted wait, the driver will consider the BO
unattached.

Can this allow userspace to prematurely free the BO while the firmware still
has it mapped, leading to IOMMU faults or memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826150726.126296-1-taimuraz@kaitmazov.com?part=1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] accel/amdxdna: make the debug BO command wait interruptible
  2026-08-26 15:21 ` sashiko-bot
@ 2026-08-26 16:12   ` Taimuraz Kaitmazov
  0 siblings, 0 replies; 12+ messages in thread
From: Taimuraz Kaitmazov @ 2026-08-26 16:12 UTC (permalink / raw)
  To: Lizhi Hou, Min Ma, Oded Gabbay; +Cc: taimuraz, dri-devel, linux-kernel

The desync is real. The early return skipped abo->assigned_hwctx, so a
command that firmware went on to complete could leave the BO attached in
firmware while the driver believed it unassigned, and DETACH is refused in
that state. v2 records the BO as attached when the wait is interrupted.

The premature-free path is not there. assigned_hwctx has three consumers --
the ATTACH guard, the DETACH guard, and the hwctx lookup in
amdxdna_hwctx_sync_debug_bo() -- and appears in no free or close path. The
debug BO is submitted as an argument BO, so the job holds a drm_gem_object
reference on it until the job is freed; its lifetime does not depend on
assigned_hwctx.

Taimuraz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] accel/amdxdna: make the debug BO command wait interruptible
  2026-08-26 15:07 [PATCH] accel/amdxdna: make the debug BO command wait interruptible Taimuraz Kaitmazov
  2026-08-26 15:10 ` Taimuraz Kaitmazov
  2026-08-26 15:21 ` sashiko-bot
@ 2026-08-26 17:34 ` Lizhi Hou
  2026-08-26 18:07   ` Taimuraz Kaitmazov
                     ` (2 more replies)
  2 siblings, 3 replies; 12+ messages in thread
From: Lizhi Hou @ 2026-08-26 17:34 UTC (permalink / raw)
  To: Taimuraz Kaitmazov, Min Ma, Oded Gabbay; +Cc: dri-devel, linux-kernel

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

* 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

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

* 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

end of thread, other threads:[~2026-08-26 21:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-26 15:07 [PATCH] accel/amdxdna: make the debug BO command wait interruptible Taimuraz Kaitmazov
2026-08-26 15:10 ` Taimuraz Kaitmazov
2026-08-26 15:21 ` sashiko-bot
2026-08-26 16:12   ` Taimuraz Kaitmazov
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 18:59     ` sashiko-bot
2026-08-26 20:04     ` Lizhi Hou
2026-08-26 21:23       ` Taimuraz Kaitmazov
2026-08-26 19:57   ` Taimuraz Kaitmazov
2026-08-26 20:14     ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox