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

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

On Mon, 15 Sep 2025 14:30:23 -0500
Nathan Lynch <nathan.lynch@amd.com> wrote:

+CC Kees given I refer to a prior discussion Kees helped out with
and this is a different related case.

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

That's a fair reason.  Perhaps add a comment just above the first
instance of this or top of file to express that?
> 
> 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.
> 
> 
> >> +};

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

So, this one is 'fun' (and I can hopefully find the references)
The C spec has had some updates that might cover this though
I'm not sure and too lazy to figure it out today.  Anyhow,
that doesn't help anyway as we care about older compilers.

So we cheat and just check the compiler does fill them ;)

Via a reply Kees sent on a discussion of the somewhat related {}
https://lore.kernel.org/linux-iio/202505090942.48EBF01B@keescook/

https://elixir.bootlin.com/linux/v6.17-rc6/source/lib/tests/stackinit_kunit.c

I think the relevant one is __dynamic_all which is used with various hole sizes
and with both bare structures and unions.

+CC Kees who might have time to shout if I have this particular case wrong ;)



> 
> 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().

That would indeed work.

> 
> 
> >> +	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),
> >> +	};