* Re: [PATCH v2] accel/amdxdna: make the debug BO command wait interruptible
[not found] ` <20260826184503.282106-1-taimuraz@kaitmazov.com>
@ 2026-08-26 20:04 ` Lizhi Hou
0 siblings, 0 replies; 2+ 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] 2+ messages in thread