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 <[email protected]> --- 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
