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 <[email protected]> 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/[email protected]?part=1
