Re: [PATCH 2/2] spi: Add KUnit coverage for DMA mapping error paths

Thanks for the review.

I'll address these comments in v2 and rework the test integration
as suggested.

At 2026-08-06 05:57:41, "Mark Brown" <[email protected]> wrote:
>On Wed, Aug 05, 2026 at 11:14:56PM +0800, Honghui Jiang wrote:
>> Add KUnit tests for the __spi_map_msg() error paths.  The tests verify
>> that mappings created before a later TX or RX failure are unwound, their
>> flags are cleared, and cur_{tx,rx}_dma_dev point to the device used for
>> the mapping.
>
>The tests themselves look good but some style/integration stuff:
>
>> --- /dev/null
>> +++ b/drivers/spi/.kunitconfig
>> @@ -0,0 +1,4 @@
>> +CONFIG_KUNIT=y
>> +CONFIG_SPI=y
>> +CONFIG_SPI_MASTER=y
>> +CONFIG_SPI_KUNIT_TEST=y
>
>This also needs enabling in the KUnit defconfigs in
>tools/testing/kunit/configs.
>
>> index 05a852494..42e337c54 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -5146,3 +5146,7 @@ static int __init spi_init(void)
>>   * include needing to have boardinfo data structures be much more public.
>>   */
>>  postcore_initcall(spi_init);
>> +
>> +#ifdef CONFIG_SPI_KUNIT_TEST
>> +#include "tests/spi_kunit.c"
>> +#endif
>
>If this is just so we can peer into the internals it'd be better to add
>a local header for internals and build as a separate translation unit
>like normal rather than doing this sort of bodge.
>
>> diff --git a/drivers/spi/tests/spi_kunit.c b/drivers/spi/tests/spi_kunit.c
>
>Everywhere else we use - as a separator.
>
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * KUnit tests for the SPI core DMA mapping error paths.
>> + *
>
>Please make the entire comment a C++ one so things look more intentional.
>
>> + * The invariant under test:
>> + *
>> + *   When __spi_map_msg() returns an error, no transfer in the message may
>> + *   still claim a DMA mapping.  Any transfer that was mapped before the
>> + *   failure must have an empty SG table and a cleared *_sg_mapped flag.
>> + *   ctlr->cur_{tx,rx}_dma_dev must identify the device used for this map,
>> + *   rather than a device retained from an earlier message.
>
>This is for one specific test, either this file should have a test
>specific name with everything else namespaced to make room for further
>tests or all this should go with the specific test.  This looks like a
>DMA subsuite so possibly named after that?  The same applies to internal
>identifiers.