Re: [PATCH] accel/amdxdna: make the debug BO command wait interruptible

Re: [PATCH] accel/amdxdna: make the debug BO command wait interruptible

[Lizhi Hou]

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;

Re: [PATCH v2] accel/amdxdna: make the debug BO command wait interruptible

[Lizhi Hou]

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;