Re: [PATCH v3 12/23] dmaengine: sdxi: Add descriptor ring management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Premature hardware index update causes a race condition (Hardware TOCTOU) allowing the hardware to execute uninitialized or stale descriptors.
- [Medium] Expensive 64-bit software division in the descriptor ring iterator.
- [Low] The `sdxi_ring_resv_foreach` macro evaluates its `resv_` argument multiple times, creating side-effect hazards.
--

commit 6a855f532756bc29a5250d62a29d1775d0ef6013
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Add descriptor ring management

Introduce a library for managing SDXI descriptor ring state. It
encapsulates determining the next free space in the ring to deposit
descriptors and performing the update of the write index correctly, as
well as iterating over slices (reservations) of the ring without
dealing directly with ring offsets/indexes.

> diff --git a/drivers/dma/sdxi/ring.c b/drivers/dma/sdxi/ring.c
> new file mode 100644
> index 0000000000000..91b28c7afbbf9
> --- /dev/null
> +++ b/drivers/dma/sdxi/ring.c
[ ... ]
> +static void sdxi_ring_state_store_widx(struct sdxi_ring_state *rs, u64 new_widx)
> +{
> +	lockdep_assert_held(&rs->lock);
> +	rs->write_index = new_widx;
> +	WRITE_ONCE(*rs->write_index_ptr, cpu_to_le64(new_widx));
> +}

[Severity: Critical]
Is it safe to update the hardware-visible write index here before the
descriptors are actually populated?

In sdxi_ring_try_reserve(), this computes the new write index and immediately
writes it to the DMA memory visible to the hardware. The lock is then dropped,
and the caller populates descriptors out-of-order.

If the ring has wrapped around, the old descriptor in a reserved slot might
still have its Validity (V) bit set to 1 from the previous pass (since the
driver does not zero out descriptors upon reclamation).

If another thread concurrently reserves slots, populates them, and rings the
doorbell with its advanced index, the hardware could process all slots up to
that doorbell value. The hardware would then check the V bit on this
preempted thread's slot, find it still set to 1, and erroneously execute the
stale descriptor again, leading to memory corruption.

Can the driver decouple the reservation index from the hardware-visible
index, or clear the V bit inside the reservation lock to prevent this?

[ ... ]
> +static struct sdxi_desc *
> +sdxi_desc_ring_entry(const struct sdxi_ring_state *rs, u64 index)
> +{
> +	return &rs->entry[do_div(index, rs->entries)];
> +}

[Severity: Medium]
Does this cause performance issues on 32-bit architectures?

Using do_div() here means performing a 64-by-32-bit division on every
descriptor access, which compiles to an expensive function call (__do_div64)
on 32-bit systems.

Since descriptor rings are typically sized as powers of 2 (e.g., SZ_1K),
could a bitwise AND (index & (rs->entries - 1)) be used instead?
Alternatively, the iterator could maintain a 32-bit wrapping array index
instead of repeatedly dividing the absolute 64-bit iteration counter.

[ ... ]
> diff --git a/drivers/dma/sdxi/ring.h b/drivers/dma/sdxi/ring.h
> new file mode 100644
> index 0000000000000..d5682687c05cd
> --- /dev/null
> +++ b/drivers/dma/sdxi/ring.h
[ ... ]
> +#define sdxi_ring_resv_foreach(resv_, desc_)			\
> +	for (sdxi_ring_resv_reset(resv_),			\
> +	     desc_ = sdxi_ring_resv_next(resv_);		\
> +	     desc_;						\
> +	     desc_ = sdxi_ring_resv_next(resv_))

[Severity: Low]
Can this macro cause side-effect hazards by evaluating its resv_ argument
multiple times?

The macro passes resv_ into sdxi_ring_resv_reset() once and into
sdxi_ring_resv_next() twice during loop execution. If a caller invokes this
macro with an expression that has side effects (e.g.,
sdxi_ring_resv_foreach(&reservations[i++], desc)), the side effect will be
evaluated multiple times per loop iteration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-sdxi-base-v3-0-4d38ca2bdffe@amd.com?part=12