Re: [PATCH v2 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:
- [Medium] The `sdxi_ring_resv_foreach` loop double-advances the iterator, silently skipping half of the descriptors.
- [Medium] The `invalid` test intentionally triggers a `WARN_ONCE`, which will crash CI systems running with `panic_on_warn`.
- [Low] The mock device allocated in the test suite's init function is completely unused.
--

commit 810cd03f2a72b539ae4d775dcbc2ceb2fa73ee06
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Add unit tests for descriptor ring reservations

This commit adds KUnit tests for the SDXI descriptor ring reservation
API. It covers valid reservations as well as error paths, and does not
require hardware to execute.

> 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

[ ... ]

> +static void valid(struct kunit *t)
> +{
> +	__le64 wi, ri;
> +	struct sdxi_ring_state r;
> +	struct sdxi_ring_resv resv;
> +	struct sdxi_desc *descs, *desc;
> +
> +
> +	descs = kunit_kmalloc_array(t, SZ_1K, sizeof(descs[0]),
> +				    GFP_KERNEL | __GFP_ZERO);
> +	KUNIT_ASSERT_NOT_NULL(t, descs);
> +
> +	ri = wi = 0;
> +	sdxi_ring_state_init(&r, &ri, &wi, SZ_1K, descs);
> +
> +	KUNIT_EXPECT_EQ(t, sdxi_ring_try_reserve(&r, r.entries, &resv), 0);
> +	KUNIT_EXPECT_EQ(t, resv.range.start, 0);
> +	KUNIT_EXPECT_EQ(t, resv.range.end, r.entries - 1);
> +	KUNIT_EXPECT_EQ(t, le64_to_cpu(wi), r.entries);
> +	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);
> +	}

Does this loop double-advance the iterator? The sdxi_ring_resv_foreach
macro already calls sdxi_ring_resv_next() on each iteration to populate
the desc variable. Calling sdxi_ring_resv_next() again inside the loop
body means we silently skip half of the descriptors in the ring. Should
this assert directly on the desc variable instead?

> +
> +	ri = cpu_to_le64(1);
> +	KUNIT_EXPECT_EQ(t, sdxi_ring_try_reserve(&r, 1, &resv), 0);
> +	KUNIT_EXPECT_EQ(t, le64_to_cpu(wi), r.entries + 1);
> +	KUNIT_EXPECT_NOT_NULL(t, sdxi_ring_resv_next(&resv));
> +}
> +
> +static void invalid(struct kunit *t)
> +{
> +	__le64 wi, ri;
> +	struct sdxi_ring_state rs;
> +	struct sdxi_ring_resv resv;
> +	struct sdxi_desc *descs;
> +
> +	descs = kunit_kmalloc_array(t, SZ_1K, sizeof(descs[0]),
> +				    GFP_KERNEL | __GFP_ZERO);
> +	KUNIT_ASSERT_NOT_NULL(t, descs);
> +
> +	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);
> +	KUNIT_EXPECT_EQ(t, sdxi_ring_try_reserve(&rs, rs.entries + 1, &resv), -EINVAL);

Will these tests trigger the WARN_ONCE in sdxi_ring_try_reserve() when
given an invalid size? Unconditionally hitting a WARN path in a KUnit
test will cause kernel panics on CI systems configured with panic_on_warn.

> +
> +	ri = cpu_to_le64(1);

[ ... ]

> +static int setup_device(struct kunit *t)
> +{
> +	struct device *dev = kunit_device_register(t, "sdxi-mock-device");
> +
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(t, dev);
> +	t->priv = dev;
> +	return 0;
> +}

This isn't a bug, but is this mock device setup needed? Neither the valid
nor invalid test cases appear to access t->priv or require a device pointer
for the ring APIs being tested.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-sdxi-base-v2-0-889cfed17e3f@amd.com?part=13