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