Re: [PATCH v2 3/7] mailbox: sun55i: add KUnit test suite for sun55i msgbox driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] test: MMIO endianness bug in mock registers causes test failures on BE
- [Low] build: missing SUN55I_MSGBOX dependency for KUnit test config
- [Low] test: mock bypass causes startup() flush test to silently succeed via failsafe
--

commit cd5e63e730e04c965eb8f6c0be2bc107e7e91214
Author: Tim Michals <tcmichals@gmail.com>

mailbox: sun55i: add KUnit test suite for sun55i msgbox driver

Adds a KUnit test suite for the Allwinner sun55i hardware Message Box
driver, validating channel routing, mock MMIO message sending, and
interrupt handling.

> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 9067f5161276a..af51794d260df 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -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

[Severity: Low]
Should this test configuration also depend on SUN55I_MSGBOX?

If the kernel is built with SUN55I_MSGBOX as a module and the test is
built-in, or if the driver is disabled completely, could this cause
undefined reference linker errors since the test links against symbols
exported by the driver?

> diff --git a/drivers/mailbox/sun55i_msgbox_test.c b/drivers/mailbox/sun55i_msgbox_test.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/mailbox/sun55i_msgbox_test.c
[ ... ]
> +static struct mock_msgbox_fixture *create_mock_fixture(struct kunit *test)
> +{
> +	struct mock_msgbox_fixture *fix;
> +	int i;
[ ... ]
> +	for (i = 0; i < SUN55I_MAX_PROCESSORS; i++)
> +		fix->mbox.regs[i] = (void __iomem *)fix->regs[i];

[Severity: Medium]
Will this cause the KUnit tests to fail unconditionally on big-endian
architectures?

The test fixture uses a host-endian u32 array for mock memory, but the
driver uses readl() and writel(). On a big-endian host, the device
accessors will perform a byte-swap, resulting in scrambled values when
reading from or writing to the mock registers.

[ ... ]
> +static void mock_rx_cb(struct mbox_client *cl, void *data)
> +{
[ ... ]
> +	/*
> +	 * Emulate hardware FIFO pop behavior: reading a word from the
> +	 * message FIFO decrements MSG_STATUS in real hardware.
> +	 */
> +	if (sink->status_reg && (*sink->status_reg & MSG_NUM_MASK) > 0)
> +		(*sink->status_reg)--;
> +}
[ ... ]
> +static void test_functional_startup_flushes_stale_fifo(struct kunit *test)
> +{
> +	struct mock_msgbox_fixture *fix = create_mock_fixture(test);
> +	u32 en_idx = SUNXI_MSGBOX_READ_IRQ_ENABLE(2) / 4;
> +	u32 stat_idx = SUNXI_MSGBOX_READ_IRQ_STATUS(2) / 4;
> +	u32 msg_stat_idx = SUNXI_MSGBOX_MSG_STATUS(2, 0) / 4;
> +	u32 fifo_idx = SUNXI_MSGBOX_MSG_FIFO(2, 0) / 4;
> +	int ret;
> +
> +	/* Simulate stale messages present in hardware FIFO before channel open */
> +	fix->regs[0][msg_stat_idx] = 4;
> +	fix->regs[0][fifo_idx] = 0xDEADBEEF;
> +
> +	ret = sun55i_msgbox_chan_ops.startup(&fix->chans[8]);
> +	KUNIT_EXPECT_EQ(test, ret, 0);

[Severity: Low]
Does test_functional_startup_flushes_stale_fifo() pass here solely due
to the driver's hardcoded failsafe boundary?

Since the readl() calls in the driver's startup() function do not trigger
the mock IRQ callback mock_rx_cb() (the only place where the mock MSG_STATUS
count is decremented), the status count never drops.

The test intends to simulate a 4-message flush, but it appears the driver
loop will spin until it hits the SUN55I_FIFO_MAX boundary cap instead.

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