Re: [PATCH V1 13/20] accel/amdxdna: Prepare for AIE4 command submission

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race in get_read_index() allows last_read_index to regress
--

commit b18a81613bb4ba1a66541441dd19a7be5abf5cef
Author: David Zhang <yidong.zhang@amd.com>

accel/amdxdna: Prepare for AIE4 command submission

This commit prepares the data structures and completion wait helpers required
for AIE4 command submission. It defines struct amdxdna_cmd_start_dpu, extends
union amdxdna_job_priv, and updates completion wait helpers to handle read
tearing and asynchronous device disconnects.

> diff --git a/drivers/accel/amdxdna/aie4_ctx.c b/drivers/accel/amdxdna/aie4_ctx.c
> index 3927c9fef05f9..59d37bd5c5a08 100644
> --- a/drivers/accel/amdxdna/aie4_ctx.c
> +++ b/drivers/accel/amdxdna/aie4_ctx.c

[ ... ]

> @@ -423,34 +423,92 @@ static inline bool valid_queue_index(u64 read, u64 write, u32 capacity)
>  
>  static u64 get_read_index(struct amdxdna_hwctx *hwctx)
>  {

[ ... ]

>  	if (!valid_queue_index(ri, wi, CTX_MAX_CMDS)) {
> -		XDNA_WARN(xdna, "Invalid index, ri %llu, wi %llu", ri, wi);
> -		usleep_range(100, 200);
> -		ri = READ_ONCE(*hwctx->priv->umq_read_index);
> +		ri = READ_ONCE(*priv->umq_read_index);
> +		/* Order the read_index sample before the write_index sample. */
> +		smp_rmb();
> +		wi = READ_ONCE(priv->write_index);
>  		if (!valid_queue_index(ri, wi, CTX_MAX_CMDS)) {
> -			XDNA_ERR(xdna, "Invalid index after retry, ri %llu, wi %llu", ri, wi);
> -			ri = 0;
> +			/*
> +			 * Still invalid (torn 64-bit read, or a transient
> +			 * accounting skew). Return the last valid read_index
> +			 * instead of 0: read_index only advances, so the cached
> +			 * value is a safe lower bound -- it never reports a
> +			 * command complete that isn't, and never regresses the
> +			 * worker into falsely timing out a finished job.
> +			 */
> +			XDNA_DBG(xdna, "Invalid index, ri %llu, wi %llu", ri, wi);
> +			return READ_ONCE(priv->last_read_index);
>  		}
>  	}
>  
> +	WRITE_ONCE(priv->last_read_index, ri);

[Severity: High]
Can this lockless blind write allow an older read index to overwrite a newer
one when executed concurrently by multiple waiters?

If Thread A reads `ri = 10` from hardware and Thread B reads `ri = 12`, Thread B
could execute `WRITE_ONCE(priv->last_read_index, 12)` before Thread A executes
`WRITE_ONCE(priv->last_read_index, 10)`, causing last_read_index to regress.

This breaks the monotonicity required to safely prevent false timeouts.

>  	return ri;
>  }

[ ... ]

> @@ -466,11 +524,14 @@ int aie4_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq, u32 timeout)
>  		wait_jifs = msecs_to_jiffies(timeout);
>  
>  	ret = wait_event_interruptible_timeout(cert_comp->waitq,
> -					       (check_cmd_done(hwctx, seq)),
> +					       check_cmd_done(hwctx, seq, cert_comp),
>  					       wait_jifs);
>  
>  	if (!ret)
>  		ret = -ETIME;
> +	else if (ret > 0 && get_read_index(hwctx) <= seq)
> +		/* Woke on disconnect/reset, not on real completion. */
> +		ret = -EAGAIN;

[Severity: High]
Could this disconnect-detection logic incorrectly trigger when get_read_index()
returns the corrupted, backward-jumped last_read_index during a torn read?

If last_read_index regresses due to the race condition above, a subsequent call
to get_read_index() experiencing a torn read would fall back to this stale
value. 

This check would then falsely evaluate to true, causing successfully completed
jobs to falsely return -EAGAIN to userspace.

>  
>  	aie4_put_cert_comp(cert_comp);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930033233.1727265-1-yidong.zhang@amd.com?part=13