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