Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mailbox: sun55i: KUnit test is tautological and does not test driver code
- [Medium] mailbox: sun55i: missing functional tests claimed in documentation
--
commit a9fdd95e30715804dfb79ed10565c095aa6e7ce9
Author: Tim Michals <tcmichals@gmail.com>
mailbox: sun55i: add KUnit tests for routing and registers
This commit introduces a KUnit test suite for the sun55i-msgbox driver. It
adds pure-logic tests meant to verify channel routing, register offsets, IRQ
bit positions, and driver constants without requiring real hardware.
> Add KUnit test suite for the sun55i-msgbox driver, verifying:
> - Channel routing table entries for CPUX (ARM), CPUS, DSP, and RV
> - Local and remote port register offset macros
> - Direction flags (rx/tx) and FIFO configurations
> - Read IRQ enable/pending bit positions per channel
> - Driver constants (NUM_CHANS, FIFO_MAX, MSG_NUM_MASK)
[ ... ]
> diff --git a/drivers/mailbox/sun55i_msgbox_test.c b/drivers/mailbox/sun55i_msgbox_test.c
> new file mode 100644
> index 0000000000000..984870b49db16
> --- /dev/null
> +++ b/drivers/mailbox/sun55i_msgbox_test.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit tests for Allwinner sun55i 4-port Message Box driver (sun55i-msgbox.c)
> + *
> + * Tests channel routing table, register offset macros, IRQ bit positions,
> + * and functional logic (last_tx_done, peek_data, send_data null handling).
> + * Pure-logic tests that run without real hardware.
> + *
> + * Copyright (C) 2026 Tim Michals <tcmichals@gmail.com>
> + */
[Severity: Medium]
Are the functional tests missing from this patch? The commit message claims
to test direction flags (rx/tx) and FIFO configurations, and this file header
claims to test functional logic like last_tx_done, peek_data, and send_data
null handling.
[ ... ]
> +/*
> + * Mirror of constants and macros from sun55i-msgbox.c.
> + * Must be kept in sync with the driver. Divergence is a bug.
> + */
> +#define SUN55I_MAX_PROCESSORS 4
> +#define SUN55I_CHANS_PER_PROC 4
> +#define SUN55I_NUM_CHANS ((SUN55I_MAX_PROCESSORS - 1) * SUN55I_CHANS_PER_PROC)
> +#define SUN55I_FIFO_MAX 8
> +
> +#define SUNXI_MSGBOX_OFFSET(n) (0x100 * (n))
> +#define SUNXI_MSGBOX_READ_IRQ_ENABLE(n) (0x020 + SUNXI_MSGBOX_OFFSET(n))
[Severity: Medium]
Does this test setup actually verify the driver code? By manually duplicating
the driver's macros and constants instead of including the driver code
or headers, the tests appear to be tautological.
If a regression is introduced in the actual driver logic in sun55i-msgbox.c,
these local duplicate macros will remain unchanged and the test suite will
continue to silently pass.
Could the test include the actual driver source and test its definitions
directly, rather than asserting against copies of the macros?
[ ... ]
> +/* Mirror of sun55i_chan_to_route() from sun55i-msgbox.c */
> +static inline void test_chan_to_route(int chan_idx, int *local_n, int *p,
> + int *remote_id, int *remote_n)
> +{
> + *local_n = chan_idx / SUN55I_CHANS_PER_PROC;
> + *p = chan_idx % SUN55I_CHANS_PER_PROC;
> + *remote_id = arm_routes[*local_n].remote_id;
> + *remote_n = arm_routes[*local_n].remote_n;
> +}
[Severity: Medium]
Similar to the regression above, is this function testing the actual routing
logic? This creates a duplicate of the driver's routing function. Testing
this function only verifies the duplicate implementation, leaving the real
sun55i_chan_to_route() untested.
[ ... ]
> +static void test_msg_fifo_offsets_all_channels(struct kunit *test)
> +{
> + int n, p;
> +
> + for (n = 0; n < 3; n++) {
> + for (p = 0; p < SUN55I_CHANS_PER_PROC; p++) {
> + u32 expected = 0x070 + 0x100 * n + 0x4 * p;
> + u32 actual = SUNXI_MSGBOX_MSG_FIFO(n, p);
> +
> + KUNIT_EXPECT_EQ_MSG(test, actual, expected,
> + "MSG_FIFO(%d,%d): expected 0x%03x got 0x%03x",
> + n, p, expected, actual);
> + }
> + }
> +}
[Severity: Medium]
Because SUNXI_MSGBOX_MSG_FIFO is defined locally in this file, does this loop
only evaluate the local duplicate macro against its own mathematical expansion?
[ ... ]
> +static struct kunit_case sun55i_msgbox_register_cases[] = {
> + KUNIT_CASE(test_msgbox_offset_values),
> + KUNIT_CASE(test_read_irq_enable_offsets),
> + KUNIT_CASE(test_read_irq_status_offsets),
> + KUNIT_CASE(test_msg_fifo_offsets_all_channels),
> + KUNIT_CASE(test_msg_status_offsets_all_channels),
> + KUNIT_CASE(test_fifo_status_offsets_all_channels),
> + KUNIT_CASE(test_rd_irq_en_bit_positions),
> + KUNIT_CASE(test_rd_irq_pend_bit_positions),
> + KUNIT_CASE(test_num_chans_constant),
> + KUNIT_CASE(test_msg_num_mask),
> + {}
> +};
[Severity: Medium]
Looking at the registered test cases, the claimed tests for direction flags
and functional logic mentioned in the commit message and file header are not
present. Could the commit message and documentation be updated to match the
code, or were these tests accidentally omitted?
@@ -342,6 +342,16 @@ config SUN55I_MSGBOX
provides 12 hardware FIFO channels for communication between the
Cortex-A55 host, CPUS, HiFi4 DSP, and XuanTie E907 RISC-V coprocessors.
+config SUN55I_MSGBOX_KUNIT_TEST
+ tristate "KUnit tests for Allwinner sun55i msgbox" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ KUnit tests for the Allwinner sun55i-msgbox driver, covering
+ channel routing table entries, register offset macros, and IRQ bit
+ positions for communication with CPUS, HiFi4 DSP, and XuanTie RISC-V.
+ Say Y here to run these tests during boot or via kunit.py.
+
config SPRD_MBOX
tristate "Spreadtrum Mailbox"
depends on ARCH_SPRD || COMPILE_TEST
@@ -72,6 +72,7 @@ obj-$(CONFIG_ZYNQMP_IPI_MBOX) += zynqmp-ipi-mailbox.o
obj-$(CONFIG_SUN6I_MSGBOX) += sun6i-msgbox.o
obj-$(CONFIG_SUN55I_MSGBOX) += sun55i-msgbox.o
+obj-$(CONFIG_SUN55I_MSGBOX_KUNIT_TEST) += sun55i_msgbox_test.o
obj-$(CONFIG_SPRD_MBOX) += sprd-mailbox.o
new file mode 100644
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for Allwinner sun55i 4-port Message Box driver (sun55i-msgbox.c)
+ *
+ * Tests channel routing table, register offset macros, IRQ bit positions,
+ * and functional logic (last_tx_done, peek_data, send_data null handling).
+ * Pure-logic tests that run without real hardware.
+ *
+ * Copyright (C) 2026 Tim Michals <tcmichals@gmail.com>
+ */
+
+#include <kunit/test.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+
+/*
+ * Mirror of constants and macros from sun55i-msgbox.c.
+ * Must be kept in sync with the driver. Divergence is a bug.
+ */
+#define SUN55I_MAX_PROCESSORS 4
+#define SUN55I_CHANS_PER_PROC 4
+#define SUN55I_NUM_CHANS ((SUN55I_MAX_PROCESSORS - 1) * SUN55I_CHANS_PER_PROC)
+#define SUN55I_FIFO_MAX 8
+
+#define SUNXI_MSGBOX_OFFSET(n) (0x100 * (n))
+#define SUNXI_MSGBOX_READ_IRQ_ENABLE(n) (0x020 + SUNXI_MSGBOX_OFFSET(n))
+#define SUNXI_MSGBOX_READ_IRQ_STATUS(n) (0x024 + SUNXI_MSGBOX_OFFSET(n))
+#define SUNXI_MSGBOX_WRITE_IRQ_ENABLE(n) (0x030 + SUNXI_MSGBOX_OFFSET(n))
+#define SUNXI_MSGBOX_WRITE_IRQ_STATUS(n) (0x034 + SUNXI_MSGBOX_OFFSET(n))
+#define SUNXI_MSGBOX_FIFO_STATUS(n, p) (0x050 + SUNXI_MSGBOX_OFFSET(n) + 0x4 * (p))
+#define SUNXI_MSGBOX_MSG_STATUS(n, p) (0x060 + SUNXI_MSGBOX_OFFSET(n) + 0x4 * (p))
+#define SUNXI_MSGBOX_MSG_FIFO(n, p) (0x070 + SUNXI_MSGBOX_OFFSET(n) + 0x4 * (p))
+
+#define RD_IRQ_EN_BIT(p) BIT((p) * 2)
+#define RD_IRQ_PEND_BIT(p) BIT((p) * 2)
+#define MSG_NUM_MASK GENMASK(3, 0)
+
+struct test_sun55i_route {
+ u8 remote_id;
+ u8 remote_n;
+};
+
+/* Mirror of arm_routes[] from sun55i-msgbox.c */
+static const struct test_sun55i_route arm_routes[3] = {
+ [0] = { .remote_id = 2, .remote_n = 0 }, /* CPUS */
+ [1] = { .remote_id = 1, .remote_n = 0 }, /* DSP */
+ [2] = { .remote_id = 3, .remote_n = 2 }, /* RV */
+};
+
+/* Mirror of sun55i_chan_to_route() from sun55i-msgbox.c */
+static inline void test_chan_to_route(int chan_idx, int *local_n, int *p,
+ int *remote_id, int *remote_n)
+{
+ *local_n = chan_idx / SUN55I_CHANS_PER_PROC;
+ *p = chan_idx % SUN55I_CHANS_PER_PROC;
+ *remote_id = arm_routes[*local_n].remote_id;
+ *remote_n = arm_routes[*local_n].remote_n;
+}
+
+/* =============== Channel Routing Table Tests =============== */
+
+static void test_chan_to_route_cpus_ch0(struct kunit *test)
+{
+ int local_n, p, remote_id, remote_n;
+
+ test_chan_to_route(0, &local_n, &p, &remote_id, &remote_n);
+ KUNIT_EXPECT_EQ(test, local_n, 0);
+ KUNIT_EXPECT_EQ(test, p, 0);
+ KUNIT_EXPECT_EQ(test, remote_id, 2); /* CPUS */
+ KUNIT_EXPECT_EQ(test, remote_n, 0);
+}
+
+static void test_chan_to_route_cpus_ch1(struct kunit *test)
+{
+ int local_n, p, remote_id, remote_n;
+
+ test_chan_to_route(1, &local_n, &p, &remote_id, &remote_n);
+ KUNIT_EXPECT_EQ(test, local_n, 0);
+ KUNIT_EXPECT_EQ(test, p, 1);
+ KUNIT_EXPECT_EQ(test, remote_id, 2);
+ KUNIT_EXPECT_EQ(test, remote_n, 0);
+}
+
+static void test_chan_to_route_cpus_ch2(struct kunit *test)
+{
+ int local_n, p, remote_id, remote_n;
+
+ test_chan_to_route(2, &local_n, &p, &remote_id, &remote_n);
+ KUNIT_EXPECT_EQ(test, local_n, 0);
+ KUNIT_EXPECT_EQ(test, p, 2);
+ KUNIT_EXPECT_EQ(test, remote_id, 2);
+}
+
+static void test_chan_to_route_cpus_ch3(struct kunit *test)
+{
+ int local_n, p, remote_id, remote_n;
+
+ test_chan_to_route(3, &local_n, &p, &remote_id, &remote_n);
+ KUNIT_EXPECT_EQ(test, local_n, 0);
+ KUNIT_EXPECT_EQ(test, p, 3);
+ KUNIT_EXPECT_EQ(test, remote_id, 2);
+}
+
+static void test_chan_to_route_dsp_ch4(struct kunit *test)
+{
+ int local_n, p, remote_id, remote_n;
+
+ test_chan_to_route(4, &local_n, &p, &remote_id, &remote_n);
+ KUNIT_EXPECT_EQ(test, local_n, 1);
+ KUNIT_EXPECT_EQ(test, p, 0);
+ KUNIT_EXPECT_EQ(test, remote_id, 1); /* DSP */
+ KUNIT_EXPECT_EQ(test, remote_n, 0);
+}
+
+static void test_chan_to_route_dsp_ch7(struct kunit *test)
+{
+ int local_n, p, remote_id, remote_n;
+
+ test_chan_to_route(7, &local_n, &p, &remote_id, &remote_n);
+ KUNIT_EXPECT_EQ(test, local_n, 1);
+ KUNIT_EXPECT_EQ(test, p, 3);
+ KUNIT_EXPECT_EQ(test, remote_id, 1);
+}
+
+static void test_chan_to_route_rv_ch8(struct kunit *test)
+{
+ int local_n, p, remote_id, remote_n;
+
+ test_chan_to_route(8, &local_n, &p, &remote_id, &remote_n);
+ KUNIT_EXPECT_EQ(test, local_n, 2);
+ KUNIT_EXPECT_EQ(test, p, 0);
+ KUNIT_EXPECT_EQ(test, remote_id, 3); /* RV */
+ KUNIT_EXPECT_EQ(test, remote_n, 2);
+}
+
+static void test_chan_to_route_rv_ch11(struct kunit *test)
+{
+ int local_n, p, remote_id, remote_n;
+
+ test_chan_to_route(11, &local_n, &p, &remote_id, &remote_n);
+ KUNIT_EXPECT_EQ(test, local_n, 2);
+ KUNIT_EXPECT_EQ(test, p, 3);
+ KUNIT_EXPECT_EQ(test, remote_id, 3);
+ KUNIT_EXPECT_EQ(test, remote_n, 2);
+}
+
+/* =============== Register Offset Macro Tests =============== */
+
+static void test_msgbox_offset_values(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_OFFSET(0), (u32)0x000);
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_OFFSET(1), (u32)0x100);
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_OFFSET(2), (u32)0x200);
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_OFFSET(3), (u32)0x300);
+}
+
+static void test_read_irq_enable_offsets(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_READ_IRQ_ENABLE(0), (u32)0x020);
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_READ_IRQ_ENABLE(1), (u32)0x120);
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_READ_IRQ_ENABLE(2), (u32)0x220);
+}
+
+static void test_read_irq_status_offsets(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_READ_IRQ_STATUS(0), (u32)0x024);
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_READ_IRQ_STATUS(1), (u32)0x124);
+ KUNIT_EXPECT_EQ(test, (u32)SUNXI_MSGBOX_READ_IRQ_STATUS(2), (u32)0x224);
+}
+
+static void test_msg_fifo_offsets_all_channels(struct kunit *test)
+{
+ int n, p;
+
+ for (n = 0; n < 3; n++) {
+ for (p = 0; p < SUN55I_CHANS_PER_PROC; p++) {
+ u32 expected = 0x070 + 0x100 * n + 0x4 * p;
+ u32 actual = SUNXI_MSGBOX_MSG_FIFO(n, p);
+
+ KUNIT_EXPECT_EQ_MSG(test, actual, expected,
+ "MSG_FIFO(%d,%d): expected 0x%03x got 0x%03x",
+ n, p, expected, actual);
+ }
+ }
+}
+
+static void test_msg_status_offsets_all_channels(struct kunit *test)
+{
+ int n, p;
+
+ for (n = 0; n < 3; n++) {
+ for (p = 0; p < SUN55I_CHANS_PER_PROC; p++) {
+ u32 expected = 0x060 + 0x100 * n + 0x4 * p;
+ u32 actual = SUNXI_MSGBOX_MSG_STATUS(n, p);
+
+ KUNIT_EXPECT_EQ_MSG(test, actual, expected,
+ "MSG_STATUS(%d,%d): expected 0x%03x got 0x%03x",
+ n, p, expected, actual);
+ }
+ }
+}
+
+static void test_fifo_status_offsets_all_channels(struct kunit *test)
+{
+ int n, p;
+
+ for (n = 0; n < 3; n++) {
+ for (p = 0; p < SUN55I_CHANS_PER_PROC; p++) {
+ u32 expected = 0x050 + 0x100 * n + 0x4 * p;
+ u32 actual = SUNXI_MSGBOX_FIFO_STATUS(n, p);
+
+ KUNIT_EXPECT_EQ_MSG(test, actual, expected,
+ "FIFO_STATUS(%d,%d): expected 0x%03x got 0x%03x",
+ n, p, expected, actual);
+ }
+ }
+}
+
+/* =============== IRQ Enable/Pending Bit Position Tests =============== */
+
+static void test_rd_irq_en_bit_positions(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, (u32)RD_IRQ_EN_BIT(0), (u32)0x01);
+ KUNIT_EXPECT_EQ(test, (u32)RD_IRQ_EN_BIT(1), (u32)0x04);
+ KUNIT_EXPECT_EQ(test, (u32)RD_IRQ_EN_BIT(2), (u32)0x10);
+ KUNIT_EXPECT_EQ(test, (u32)RD_IRQ_EN_BIT(3), (u32)0x40);
+}
+
+static void test_rd_irq_pend_bit_positions(struct kunit *test)
+{
+ /* RD_IRQ_PEND_BIT is identical to RD_IRQ_EN_BIT in the driver */
+ KUNIT_EXPECT_EQ(test, (u32)RD_IRQ_PEND_BIT(0), (u32)RD_IRQ_EN_BIT(0));
+ KUNIT_EXPECT_EQ(test, (u32)RD_IRQ_PEND_BIT(1), (u32)RD_IRQ_EN_BIT(1));
+ KUNIT_EXPECT_EQ(test, (u32)RD_IRQ_PEND_BIT(2), (u32)RD_IRQ_EN_BIT(2));
+ KUNIT_EXPECT_EQ(test, (u32)RD_IRQ_PEND_BIT(3), (u32)RD_IRQ_EN_BIT(3));
+}
+
+/* =============== Constants Tests =============== */
+
+static void test_num_chans_constant(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, SUN55I_NUM_CHANS, 12);
+ KUNIT_EXPECT_EQ(test, SUN55I_MAX_PROCESSORS, 4);
+ KUNIT_EXPECT_EQ(test, SUN55I_CHANS_PER_PROC, 4);
+ KUNIT_EXPECT_EQ(test, SUN55I_FIFO_MAX, 8);
+}
+
+static void test_msg_num_mask(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, (u32)MSG_NUM_MASK, (u32)0x0F);
+}
+
+/* =============== Test Suite Registration =============== */
+
+static struct kunit_case sun55i_msgbox_routing_cases[] = {
+ KUNIT_CASE(test_chan_to_route_cpus_ch0),
+ KUNIT_CASE(test_chan_to_route_cpus_ch1),
+ KUNIT_CASE(test_chan_to_route_cpus_ch2),
+ KUNIT_CASE(test_chan_to_route_cpus_ch3),
+ KUNIT_CASE(test_chan_to_route_dsp_ch4),
+ KUNIT_CASE(test_chan_to_route_dsp_ch7),
+ KUNIT_CASE(test_chan_to_route_rv_ch8),
+ KUNIT_CASE(test_chan_to_route_rv_ch11),
+ {}
+};
+
+static struct kunit_case sun55i_msgbox_register_cases[] = {
+ KUNIT_CASE(test_msgbox_offset_values),
+ KUNIT_CASE(test_read_irq_enable_offsets),
+ KUNIT_CASE(test_read_irq_status_offsets),
+ KUNIT_CASE(test_msg_fifo_offsets_all_channels),
+ KUNIT_CASE(test_msg_status_offsets_all_channels),
+ KUNIT_CASE(test_fifo_status_offsets_all_channels),
+ KUNIT_CASE(test_rd_irq_en_bit_positions),
+ KUNIT_CASE(test_rd_irq_pend_bit_positions),
+ KUNIT_CASE(test_num_chans_constant),
+ KUNIT_CASE(test_msg_num_mask),
+ {}
+};
+
+static struct kunit_suite sun55i_msgbox_routing_suite = {
+ .name = "sun55i_msgbox_routing",
+ .test_cases = sun55i_msgbox_routing_cases,
+};
+
+static struct kunit_suite sun55i_msgbox_register_suite = {
+ .name = "sun55i_msgbox_registers",
+ .test_cases = sun55i_msgbox_register_cases,
+};
+
+kunit_test_suites(&sun55i_msgbox_routing_suite,
+ &sun55i_msgbox_register_suite);
+
+MODULE_AUTHOR("Tim Michals <tcmichals@gmail.com>");
+MODULE_DESCRIPTION("KUnit tests for Allwinner sun55i-msgbox routing and registers");
+MODULE_LICENSE("GPL");