Re: [PATCH v3 13/23] dmaengine: sdxi: Add unit tests for descriptor ring reservations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The KUnit test advances the reservation iterator twice per loop iteration, verifying only half of the reserved descriptors and evaluating unsequenced arguments.
- [Medium] The `invalid` test intentionally triggers a kernel warning (`WARN_ONCE`) without suppressing it, which pollutes the kernel log and causes crashes in CI environments.

Pre-existing issues:
- [High] The ring reservation API updates the hardware-visible write pointer before descriptors are populated, risking the device processing uninitialized memory (TOCTOU) and lacking necessary memory barriers.
--

commit 4fc6ef8e997ae4fb9c2e324c9e00fd0b37015590
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Add unit tests for descriptor ring reservations

Add KUnit tests for the descriptor ring reservation API, covering valid
reservations and error paths. A .kunitconfig is included ease of use. No
SDXI hardware is required to run these tests.

> diff --git a/drivers/dma/sdxi/ring_kunit.c b/drivers/dma/sdxi/ring_kunit.c
> new file mode 100644
> index 0000000000000..3bc7073e0c39a
> --- /dev/null
> +++ b/drivers/dma/sdxi/ring_kunit.c
> @@ -0,0 +1,105 @@
[ ... ]
> +static void valid(struct kunit *t)
> +{
[ ... ]
> +	sdxi_ring_resv_foreach(&resv, desc) {
> +		KUNIT_EXPECT_NOT_NULL_MSG(t, sdxi_ring_resv_next(&resv),
> +			"unexpected null descriptor for index %llu", resv.iter);
> +	}

[Severity: Medium]
Does this loop inadvertently consume two descriptors per iteration?

The macro sdxi_ring_resv_foreach() implicitly advances the iterator:

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_))

By calling sdxi_ring_resv_next() again inside the loop body, the test
skips validation for half of the elements.

Additionally, could calling sdxi_ring_resv_next(&resv) and reading
resv.iter in the same macro expansion lead to unsequenced argument
evaluation?

> +
> +	ri = cpu_to_le64(1);
[ ... ]
> +static void invalid(struct kunit *t)
> +{
[ ... ]
> +	ri = wi = 0;
> +	sdxi_ring_state_init(&rs, &ri, &wi, SZ_1K, descs);
> +
> +	KUNIT_EXPECT_EQ(t, sdxi_ring_try_reserve(&rs, 0, &resv), -EINVAL);

[Severity: Medium]
Will this intentionally trigger a warning that pollutes the kernel log?

The test passes invalid arguments designed to trigger a WARN_ONCE in
sdxi_ring_try_reserve():

drivers/dma/sdxi/ring.c:sdxi_ring_try_reserve()
	if (WARN_ONCE(nr < 1 || nr > rs->entries,

Since there is no framework mechanism in this test to handle or suppress
this warning, does it permanently consume the global warned state? This
could hide future issues and cause the test suite to panic if panic_on_warn=1
is set.

[Severity: High]
This is a pre-existing issue, but does the ring reservation API update the
hardware-visible write pointer before descriptors are populated?

Looking at sdxi_ring_try_reserve() in drivers/dma/sdxi/ring.c:

drivers/dma/sdxi/ring.c:sdxi_ring_try_reserve()
		if (new_widx - ridx > rs->entries)
			return -EBUSY;

		sdxi_ring_state_store_widx(rs, new_widx);

It updates rs->write_index_ptr in hardware-visible DMA memory immediately
upon reservation, before the caller populates the returned descriptor range.

If Thread A reserves and begins filling a descriptor, and Thread B reserves,
fills, and rings the doorbell, would the device read the updated write
pointer and process Thread A's uninitialized descriptor?

Could this also require a preceding dma_wmb() barrier to ensure descriptor
writes complete before the write pointer is updated?

> +	KUNIT_EXPECT_EQ(t, sdxi_ring_try_reserve(&rs, rs.entries + 1, &resv), -EINVAL);
> +
> +	ri = cpu_to_le64(1);

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