Re: [PATCH v2 21/23] dmaengine: sdxi: Add unit tests for descriptor encoding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Tests for `copy`, `cxt_start`, and `cxt_stop` bypass the `packing` API verification by directly checking encoded descriptor fields.
- [Low] The `cxt_stop` test incorrectly references the `cxt_start` union member when verifying fields.
- [Low] The `sdxi_serialize_nop` encoder is entirely omitted from the test suite.
- [Low] The test suite setup function (`generic_desc_setup_device`) registers an unused mock device.
--

commit aa4cdaba4b4a985bdcf2f80d3a44e02557352cc5
Author: Nathan Lynch <nathan.lynch@amd.com>
Subject: dmaengine: sdxi: Add unit tests for descriptor encoding

This commit adds KUnit tests for encoding descriptors in the SDXI driver.
The tests use the packing API to decode descriptors produced by the driver
to verify the correct bit offsets without relying on bitmask macros.

> Test the encoder function for each descriptor type currently used by
> the driver.

Does this statement overlook the sdxi_serialize_nop encoder? It appears the
test suite entirely omits tests for sdxi_serialize_nop, leaving this API
untested.

> diff --git a/drivers/dma/sdxi/descriptor_kunit.c b/drivers/dma/sdxi/descriptor_kunit.c
> new file mode 100644
> index 0000000000000..1f3c2e7ab2ddf
> --- /dev/null
> +++ b/drivers/dma/sdxi/descriptor_kunit.c

[ ... ]

> +static void copy(struct kunit *t)
> +{

[ ... ]

> +	KUNIT_EXPECT_EQ(t, 0,
> +			sdxi_encode_copy(&desc,
> +					 &(struct sdxi_copy) {
> +						 .src = 0x1000,
> +						 .dst = 0x2000,
> +						 .len = 0x100,
> +						 .src_akey = 1,
> +						 .dst_akey = 2,
> +					 }));
> +	KUNIT_EXPECT_EQ(t, 0x1000, le64_to_cpu(desc.copy.addr0));
> +	KUNIT_EXPECT_EQ(t, 0x2000, le64_to_cpu(desc.copy.addr1));
> +	KUNIT_EXPECT_EQ(t, 0x100, 1 + le32_to_cpu(desc.copy.size));
> +	KUNIT_EXPECT_EQ(t, 1, le16_to_cpu(desc.copy.akey0));
> +	KUNIT_EXPECT_EQ(t, 2, le16_to_cpu(desc.copy.akey1));

The commit message states the tests use the packing API to decode descriptors
without relying on bitmask definitions. Do these assertions bypass the packing
API verification by directly reading the encoded descriptor fields from the
struct?

If the struct layout in the header file is incorrect, could this allow both the
driver and test code to share the same bug undetected?

[ ... ]

> +static void cxt_start(struct kunit *t)
> +{
> +	struct unpacked_desc unpacked;
> +	struct sdxi_cxt_start start = {
> +		.range = sdxi_cxt_range_single(2),
> +	};
> +	struct sdxi_desc desc;
> +
> +	desc_poison(&desc);
> +	KUNIT_ASSERT_EQ(t, 0, sdxi_encode_cxt_start(&desc, &start));
> +
> +	unpack_cxt_start(&unpacked, &desc);
> +
> +	/* Check op-specific fields. */
> +	KUNIT_EXPECT_EQ(t, 0, desc.cxt_start.vflags);

Does this also bypass the packing API by directly reading the encoded descriptor
field instead of verifying via the unpacked struct?

[ ... ]

> +static void cxt_stop(struct kunit *t)
> +{
> +	struct unpacked_desc unpacked;
> +	struct sdxi_cxt_stop stop = {
> +		.range = sdxi_cxt_range_single(2),
> +	};
> +	struct sdxi_desc desc;
> +
> +	desc_poison(&desc);
> +	KUNIT_ASSERT_EQ(t, 0, sdxi_encode_cxt_stop(&desc, &stop));
> +
> +	unpack_cxt_stop(&unpacked, &desc);
> +
> +	/* Check op-specific fields. */
> +	KUNIT_EXPECT_EQ(t, 0, desc.cxt_start.vflags);

In addition to bypassing the packing API, does this incorrectly reference the
cxt_start union member instead of cxt_stop when verifying fields?

[ ... ]

> +static int generic_desc_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;
> +}

Is this mock device actually used by any of the tests in this suite? Looking at
the test cases, they appear to operate exclusively on memory buffers without
requiring a device.

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