| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250915125226.000043c1@huawei.com>
Date: Mon, 15 Sep 2025 12:52:26 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@...nel.org>
CC: <nathan.lynch@....com>, Vinod Koul <vkoul@...nel.org>, Wei Huang
<wei.huang2@....com>, Mario Limonciello <mario.limonciello@....com>, "Bjorn
Helgaas" <bhelgaas@...gle.com>, <linux-pci@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <dmaengine@...r.kernel.org>
Subject: Re: [PATCH RFC 03/13] dmaengine: sdxi: Add descriptor encoding and
unit tests
On Fri, 05 Sep 2025 13:48:26 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@...nel.org> wrote:
> From: Nathan Lynch <nathan.lynch@....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@....com>
> Signed-off-by: Wei Huang <wei.huang2@....com>
> Signed-off-by: Nathan Lynch <nathan.lynch@....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.
> +};
Powered by blists - more mailing lists