Re: [PATCH V2 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:
- [Medium] amdxdna: monotonic regression in get_read_index() due to lockless update
--

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

accel/amdxdna: Prepare for AIE4 command submission

This commit prepares data structures and completion wait helpers for AIE4
command submission. It extends the amdxdna_job_priv structure and implements
an ordered retry mechanism in get_read_index() when a torn read is detected.

> diff --git a/drivers/accel/amdxdna/aie4_ctx.c b/drivers/accel/amdxdna/aie4_ctx.c
> --- a/drivers/accel/amdxdna/aie4_ctx.c
> +++ b/drivers/accel/amdxdna/aie4_ctx.c

[ ... ]

>  static u64 get_read_index(struct amdxdna_hwctx *hwctx)
>  {

[ ... ]

> +	/*
> +	 * Cache valid sample as fallback for torn reads. Lockless on purpose:
> +	 * a racing caller may store an older value, which only delays seeing
> +	 * a completion until the next wakeup, never reports one early.
> +	 */
> +	WRITE_ONCE(priv->last_read_index, ri);

[Severity: Medium]
Can this lockless assignment allow concurrent get_read_index() callers to
overwrite a newer completion index with an older one?

For example, if Thread A reads ri=10, and Thread B reads ri=11 and writes
last_read_index=11, could Thread A then execute this assignment and write
last_read_index=10? This appears to cause last_read_index to go backward.

>  	return ri;
>  }

[ ... ]

>  int aie4_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq, u32 timeout)
>  {

[ ... ]

> -	if (!ret)
> +	if (!ret) {
>  		ret = -ETIME;
> +	} else if (ret > 0 && !check_cert_comp_linked(hwctx, cert_comp) &&
> +		   get_read_index(hwctx) <= seq) {

[Severity: Medium]
If last_read_index went backward as described above, could this check
falsely evaluate to true?

If a torn read falls back to the stale last_read_index while the device
disconnects or suspends, it seems a successfully completed command might
incorrectly fail and return -EAGAIN.

> +		/* Disconnect (suspend or TDR) before the command completed. */
> +		ret = -EAGAIN;
> +	}
>  
>  	aie4_put_cert_comp(cert_comp);

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