Re: [PATCH 3/7] mailbox: sun55i: add KUnit tests for routing and registers

[sashiko-bot@kernel.org]

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922034711.190253-1-tcmichals@gmail.com?part=3