Re: [PATCH v2 6/7] remoteproc: sunxi: add KUnit test suite for sunxi remoteproc driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] remoteproc: sunxi: KUnit test mock MMIO reads fail on big-endian
- [Medium] remoteproc: sunxi: false positive KUnit test for kick_msg
- [Low] remoteproc: sunxi: missing Kconfig dependency for KUnit tests
--

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

remoteproc: sunxi: add KUnit test suite for sunxi remoteproc driver
    
Add comprehensive in-kernel KUnit unit test suite (34 tests) for the
Allwinner sunxi remoteproc driver.

> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index d35b033b9106d..c2bddfd955665 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
[ ... ]
> @@ -396,6 +396,16 @@ config SUNXI_REMOTEPROC
>  	  co-processor on Allwinner A523, A527, and T527 SoCs via the
>  	  remote processor framework.
>  
> +config SUNXI_REMOTEPROC_KUNIT_TEST
> +	tristate "KUnit tests for Allwinner sunxi remoteproc" if !KUNIT_ALL_TESTS
> +	depends on KUNIT

[Severity: Low]
Will this allow incompatible build configurations?

If the test is built-in (=y) but the driver is built as a module (=m) or
disabled (=n), the linker will fail with undefined references since vmlinux
cannot link against module-only symbols. Should this include a
depends on SUNXI_REMOTEPROC statement to enforce proper dependencies?

> +	default KUNIT_ALL_TESTS
> +	help
> +	  KUnit tests for the Allwinner sunxi_rproc driver, covering
> +	  da_to_va() address translation logic and boundary conditions
> +	  across dedicated SRAM spaces, DRAM carveouts, and trace regions.
> +	  Say Y here to run these tests during boot or via kunit.py.
> +
>  endif # REMOTEPROC

> diff --git a/drivers/remoteproc/sunxi_rproc_test.c b/drivers/remoteproc/sunxi_rproc_test.c
> new file mode 100644
> index 0000000000000..2546f850f30bd
> --- /dev/null
> +++ b/drivers/remoteproc/sunxi_rproc_test.c
[ ... ]
> +static void test_start_bootaddr_programming(struct kunit *test)
> +{
> +	struct test_context *ctx = create_test_ctx(test);
> +	int ret;
> +
> +	ctx->priv.cfg_va = (void __iomem *)ctx->mock_cfg_regs;
> +	ctx->rproc.bootaddr = 0x40014000;
> +
> +	ret = sunxi_rproc_start(&ctx->rproc);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +	/* Check that bootaddr was written to STA_ADD_REG (offset 0x204) */
> +	KUNIT_EXPECT_EQ(test, ctx->mock_cfg_regs[E906_STA_ADD_REG / 4], 0x40014000U);

[Severity: Medium]
Could this direct array read cause spurious test failures on big-endian
architectures?

The driver code uses writel() for I/O register programming, which mandates a
little-endian memory layout and performs automatic byte-swapping on
big-endian CPUs. Directly reading the raw u32 array elements bypasses this.
On a big-endian architecture, the direct read will interpret the
little-endian bytes in memory as a big-endian integer, returning a
byte-swapped value instead of the expected 0x40014000. Should this test use
readl() to retrieve the value in CPU-endian format?

[ ... ]
> +static void test_prepare_and_unprepare_remap(struct kunit *test)
> +{
> +	struct test_context *ctx = create_test_ctx(test);
> +	int ret;
[ ... ]
> +	/* prepare() should set SUNXI_REMAP_SRAMA3_2_BIT (bit 1) */
> +	ret = sunxi_rproc_prepare(&ctx->rproc);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +	KUNIT_EXPECT_EQ(test, ctx->mock_remap_reg & SUNXI_REMAP_SRAMA3_2_BIT,
> +			SUNXI_REMAP_SRAMA3_2_BIT);

[Severity: Medium]
Does this bitwise check need to account for endianness as well?

Because writel() wrote the bit in little-endian order, this native bitwise
check against mock_remap_reg will also fail on big-endian systems.

[ ... ]
> +static void test_kick_stores_vqid(struct kunit *test)
> +{
> +	struct test_context *ctx = create_test_ctx(test);
> +
> +	/* Verify kick_msg stores the passed vqid to avoid stack UAF */
> +	ctx->priv.kick_msg = 0xDEADBEEF;
> +	ctx->priv.tx_chan = NULL; /* Avoid mbox_send_message dispatch */
> +
> +	sunxi_rproc_kick(&ctx->rproc, 1);
> +	/* Without tx_chan, returns before writing kick_msg */
> +	KUNIT_EXPECT_EQ(test, ctx->priv.kick_msg, 0xDEADBEEFU);

[Severity: Medium]
Is this test validating the intended assignment?

The test intentionally sets ctx->priv.tx_chan to NULL to prevent dispatching.
However, looking at sunxi_rproc_kick(), this triggers an early return guard:

void sunxi_rproc_kick(struct rproc *rproc, int vqid)
{
        ...
        if (!priv->tx_chan)
                return;

        priv->kick_msg = (u32)vqid;
        ...
}

Consequently, the assignment to priv->kick_msg never occurs. The test then
asserts against the unmodified initial 0xDEADBEEFU value, which passes
spuriously without actually exercising the assignment logic.

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