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

[Nathan Lynch <nathan.lynch@amd.com>]

Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> On Fri, 05 Sep 2025 13:48:26 -0500
> Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:
>> +++ 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.

My idea is to use the bitfield macros (GENMASK etc) for the real code
that encodes descriptors while using the packing API in the tests for
those functions.

By limiting what's shared between the real code and the tests I get more
confidence in both. If both the driver code and the tests rely on the
bitfield macros, and then upon adding a new descriptor field I
mistranslate the bit numbering from the spec, that error is more likely
to propagate to the tests undetected than if the test code relies on a
separate mechanism for decoding descriptors.

I find the packing API quite convenient to use for the SDXI descriptor
tests since the spec defines the fields in terms of bit offsets that can
be directly copied to a packed_field_ array.


>> +};
>> +
>> +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.

OK.


>> +	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.

Maybe I prefer your version too. Just mentioning in case it's not clear:
cxt_stop is a union member with the same size as the enclosing struct
sdxi_desc. Each member of struct sdxi_desc's interior anonymous union is
intended to completely overlay the entire object.

The reason for the preceding desc_clear() is that the designated
initializer construct does not necessarily zero padding bytes in the
object. Now, there *shouldn't* be any padding bytes in SDXI descriptors
as I've defined them, so I'm hoping the redundant stores are discarded
in the generated code. But I haven't checked this.

And it looks like I neglected to mark all the descriptor structs __packed,
oops.

I think I can add the __packed to struct sdxi_desc et al, use your
suggested initializer, and discard desc_clear().


>> +	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.

Sure.