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

[sashiko-bot@kernel.org]

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 <taimuraz@kaitmazov.com>

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/20260826184503.282106-1-taimuraz@kaitmazov.com?part=1