Re: [PATCH RFC 03/13] dmaengine: sdxi: Add descriptor encoding and unit tests

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 05 Sep 2025 13:48:26 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add support for encoding several types of SDXI descriptors:
> 
> * Copy
> * Interrupt
> * Context start
> * Context stop
> 
> Each type of descriptor has a corresponding parameter struct which is
> an input to its encoder function. E.g. to encode a copy descriptor,
> the client initializes a struct sdxi_copy object with the source,
> destination, size, etc and passes that to sdxi_encode_copy().
> 
> Include unit tests that verify that encoded descriptors have the
> expected values and that fallible encode functions fail on invalid
> inputs.
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>

Hi,
A few comments inline. Mostly it is a case of personal taste
vs what I'm seeing as complexity that is needed.

Jonathan


> ---
>  drivers/dma/sdxi/.kunitconfig       |   4 +
>  drivers/dma/sdxi/descriptor.c       | 197 ++++++++++++++++++++++++++++++++++++
>  drivers/dma/sdxi/descriptor.h       | 107 ++++++++++++++++++++
>  drivers/dma/sdxi/descriptor_kunit.c | 181 +++++++++++++++++++++++++++++++++
>  4 files changed, 489 insertions(+)
> 
> diff --git a/drivers/dma/sdxi/.kunitconfig b/drivers/dma/sdxi/.kunitconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..a98cf19770f03bce82ef86d378d2a2e34da5154a
> --- /dev/null
> +++ b/drivers/dma/sdxi/.kunitconfig
> @@ -0,0 +1,4 @@
> +CONFIG_KUNIT=y
> +CONFIG_DMADEVICES=y
> +CONFIG_SDXI=y
> +CONFIG_SDXI_KUNIT_TEST=y
> diff --git a/drivers/dma/sdxi/descriptor.c b/drivers/dma/sdxi/descriptor.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..6ea5247bf8cdaac19131ca5326ba1640c0b557f8
> --- /dev/null
> +++ b/drivers/dma/sdxi/descriptor.c

> +enum {
> +	SDXI_PACKING_QUIRKS = QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST,
> +};
> +
> +#define sdxi_desc_field(_high, _low, _member) \
> +	PACKED_FIELD(_high, _low, struct sdxi_desc_unpacked, _member)
> +#define sdxi_desc_flag(_bit, _member) \
> +	sdxi_desc_field(_bit, _bit, _member)
> +
> +static const struct packed_field_u16 common_descriptor_fields[] = {
> +	sdxi_desc_flag(0, vl),
> +	sdxi_desc_flag(1, se),
> +	sdxi_desc_flag(2, fe),
> +	sdxi_desc_flag(3, ch),
> +	sdxi_desc_flag(4, csr),
> +	sdxi_desc_flag(5, rb),
> +	sdxi_desc_field(15, 8, subtype),
> +	sdxi_desc_field(26, 16, type),
> +	sdxi_desc_flag(448, np),
> +	sdxi_desc_field(511, 453, csb_ptr),

I'm not immediately seeing the advantage of dealing with unpacking in here
when patch 2 introduced a bunch of field defines that can be used directly
in the tests. 

> +};
> +
> +void sdxi_desc_unpack(struct sdxi_desc_unpacked *to,
> +		      const struct sdxi_desc *from)
> +{
> +	*to = (struct sdxi_desc_unpacked){};
> +	unpack_fields(from, sizeof(*from), to, common_descriptor_fields,
> +		      SDXI_PACKING_QUIRKS);
> +}
> +EXPORT_SYMBOL_IF_KUNIT(sdxi_desc_unpack);

> +int sdxi_encode_cxt_stop(struct sdxi_desc *desc,
> +			  const struct sdxi_cxt_stop *params)
> +{
> +	u16 cxt_start;
> +	u16 cxt_end;

I'd either combine like types, or assign at point of declaration to
cut down on a few lines of code.

> +	u64 csb_ptr;
> +	u32 opcode;
> +
> +	opcode = (FIELD_PREP(SDXI_DSC_VL, 1) |
> +		  FIELD_PREP(SDXI_DSC_FE, 1) |
> +		  FIELD_PREP(SDXI_DSC_SUBTYPE, SDXI_DSC_OP_SUBTYPE_CXT_STOP) |
> +		  FIELD_PREP(SDXI_DSC_TYPE, SDXI_DSC_OP_TYPE_ADMIN));
> +
> +	cxt_start = params->range.cxt_start;
> +	cxt_end = params->range.cxt_end;
> +
> +	csb_ptr = FIELD_PREP(SDXI_DSC_NP, 1);
> +
> +	desc_clear(desc);

Not particularly important, but I'd be tempted to combine these with

	*desc = (struct sdxi_desc) {
		.ctx_stop = {
			.opcode = cpu_to_le32(opcode),
			.cxt_start = cpu_to_le16(cxt_start),
			.cxt_end = cpu_to_le16(cxt_end),
			.csb_ptr = cpu_to_le64(csb_ptr),
		},
	};

To me that more clearly shows what is set and that the
rest is zeroed.

> +	desc->cxt_stop = (struct sdxi_dsc_cxt_stop) {
> +		.opcode = cpu_to_le32(opcode),
> +		.cxt_start = cpu_to_le16(cxt_start),
> +		.cxt_end = cpu_to_le16(cxt_end),
> +		.csb_ptr = cpu_to_le64(csb_ptr),
> +	};
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_IF_KUNIT(sdxi_encode_cxt_stop);
> diff --git a/drivers/dma/sdxi/descriptor.h b/drivers/dma/sdxi/descriptor.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..141463dfd56bd4a88b4b3c9d45b13cc8101e1961
> --- /dev/null
> +++ b/drivers/dma/sdxi/descriptor.h
> @@ -0,0 +1,107 @@

> +/*
> + * Fields common to all SDXI descriptors in "unpacked" form, for use
> + * with pack_fields() and unpack_fields().
> + */
> +struct sdxi_desc_unpacked {
> +	u64 csb_ptr;
> +	u16 type;
> +	u8 subtype;
> +	bool vl;
> +	bool se;
> +	bool fe;
> +	bool ch;
> +	bool csr;
> +	bool rb;
> +	bool np;
> +};
> +
> +void sdxi_desc_unpack(struct sdxi_desc_unpacked *to,
> +		      const struct sdxi_desc *from);
> +
> +#endif /* DMA_SDXI_DESCRIPTOR_H */
> diff --git a/drivers/dma/sdxi/descriptor_kunit.c b/drivers/dma/sdxi/descriptor_kunit.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..eb89d5a152cd789fb8cfa66b78bf30e583a1680d
> --- /dev/null
> +++ b/drivers/dma/sdxi/descriptor_kunit.c
> @@ -0,0 +1,181 @@

> +static void cxt_stop(struct kunit *t)
> +{
> +	struct sdxi_cxt_stop stop = {
> +		.range = sdxi_cxt_range(1, U16_MAX)
> +	};
> +	struct sdxi_desc desc = {};
> +	struct sdxi_desc_unpacked unpacked;
> +
> +	KUNIT_EXPECT_EQ(t, 0, sdxi_encode_cxt_stop(&desc, &stop));
> +
> +	/* Check op-specific fields */
> +	KUNIT_EXPECT_EQ(t, 0, desc.cxt_stop.vflags);
> +	KUNIT_EXPECT_EQ(t, 0, desc.cxt_stop.vf_num);
> +	KUNIT_EXPECT_EQ(t, 1, desc.cxt_stop.cxt_start);
> +	KUNIT_EXPECT_EQ(t, U16_MAX, desc.cxt_stop.cxt_end);
> +
> +	/*
> +	 * Check generic fields. Some flags have mandatory values
> +	 * according to the operation type.
> +	 */
> +	sdxi_desc_unpack(&unpacked, &desc);

Follow up on the comments on unpacking above, to me just pulling the
values directly is simpler to follow.

	KUNIT_EXPECT_EQ(t, 0, FIELD_GET(desc.generic.opcode, SDXI_DSC_VL));

or something along those lines.

> +	KUNIT_EXPECT_EQ(t, unpacked.vl, 1);
> +	KUNIT_EXPECT_EQ(t, unpacked.se, 0);
> +	KUNIT_EXPECT_EQ(t, unpacked.fe, 1);
> +	KUNIT_EXPECT_EQ(t, unpacked.ch, 0);
> +	KUNIT_EXPECT_EQ(t, unpacked.subtype, SDXI_DSC_OP_SUBTYPE_CXT_STOP);
> +	KUNIT_EXPECT_EQ(t, unpacked.type, SDXI_DSC_OP_TYPE_ADMIN);
> +	KUNIT_EXPECT_EQ(t, unpacked.csb_ptr, 0);
> +	KUNIT_EXPECT_EQ(t, unpacked.np, 1);
> +}
> +
> +static struct kunit_case generic_desc_tcs[] = {
> +	KUNIT_CASE(copy),
> +	KUNIT_CASE(intr),
> +	KUNIT_CASE(cxt_start),
> +	KUNIT_CASE(cxt_stop),
> +	{},

Trivial but I'd drop that comma as nothing can come after this.

> +};