Re: [PATCH v4 7/9] spi: add KUnit tests for the multi-CS framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] spi: tests: spi_cs_config suite only tests local spi_validate_cs_config()
- [Medium] spi: tests: last_cs_index_mask_filters_slots() does not test mask filtering
--

commit c0274670e3ec066a2b9eb903dcadd09d2e349d14
Author: Jonathan Santos <Jonathan.Santos@analog.com>

spi: add KUnit tests for the multi-CS framework

This commit adds KUnit test suites for the SPI multi-CS framework covering
device naming, last-asserted CS tracking, and CS array validation. It also
exposes SPI_INVALID_CS, spi_dev_set_name(), and spi_is_last_cs() in
drivers/spi/internals.h for KUnit testing.

> diff --git a/drivers/spi/tests/spi-multi-cs-kunit.c b/drivers/spi/tests/spi-multi-cs-kunit.c
> new file mode 100644
> index 0000000000000..314de7dc86064
> --- /dev/null
> +++ b/drivers/spi/tests/spi-multi-cs-kunit.c

[ ... ]

> +/*
> + * cs_index_mask only has bit 0 set even though num_chipselect=2:
> + * slot 1 is skipped by spi_for_each_valid_cs, only slot 0 is checked.
> + * last_cs[0] matches.
> + */
> +static void last_cs_index_mask_filters_slots(struct kunit *test)
> +{
> +	const u8 cs[] = { 0, 1 };
> +	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
> +
> +	/* override mask: only slot 0 active. */
> +	spi->cs_index_mask = BIT(0);
> +	spi->controller->last_cs[0] = 0;
> +	spi->controller->last_cs[1] = SPI_INVALID_CS;
> +
> +	KUNIT_EXPECT_TRUE(test, spi_is_last_cs(spi));
> +}

[Severity: Medium]
In drivers/spi/spi.c, spi_is_last_cs() sets last = true whenever any active
slot matches, and never clears last on a non-matching slot:

drivers/spi/spi.c:spi_is_last_cs() {
	u8 idx;
	bool last = false;

	spi_for_each_valid_cs(spi, idx) {
		if (spi->controller->last_cs[idx] == spi_get_chipselect(spi, idx))
			last = true;
	}
	return last;
}

In last_cs_index_mask_filters_slots(), slot 0 is set to match
(last_cs[0] = 0 with cs[0] = 0) and slot 1 is set not to match
(last_cs[1] = SPI_INVALID_CS with cs[1] = 1) while cs_index_mask is BIT(0).
Because slot 0 matches, spi_is_last_cs() will return true even if
cs_index_mask is ignored and slot 1 is also evaluated.

To verify that cs_index_mask actually filters out inactive slots, shouldn't
this test set the active slot 0 to a non-matching value (such as
SPI_INVALID_CS), set the masked-out slot 1 to match (last_cs[1] = 1), and
expect spi_is_last_cs(spi) to return false?

[ ... ]

> +/*
> + * Validate and normalise the CS configuration of @spi against the controller's
> + * @num_chipselect limit. Arithmetic Analysis only, no device-model calls or bus
> + * scan.
> + *
> + * On success the unused chip_select[] slots are filled with SPI_INVALID_CS.
> + */
> +static int spi_validate_cs_config(struct spi_device *spi,
> +				  u16 ctlr_num_chipselect)
> +{
> +	int idx;
> +
> +	if (spi->num_chipselect > SPI_DEVICE_CS_CNT_MAX)
> +		return -EOVERFLOW;
> +
> +	for (idx = 0; idx < spi->num_chipselect; idx++) {
> +		if (spi_get_chipselect(spi, idx) >= ctlr_num_chipselect)
> +			return -EINVAL;
> +	}
> +
> +	/* No duplicate physical CS values within the same device. */
> +	for (idx = 0; idx < spi->num_chipselect; idx++) {
> +		for (int j = idx + 1; j < spi->num_chipselect; j++) {
> +			if (spi_get_chipselect(spi, idx) == spi_get_chipselect(spi, j))
> +				return -EBUSY;
> +		}
> +	}
> +
> +	/* Fill unused slots before the contiguity check modifies chip_select[]. */
> +	for (idx = spi->num_chipselect; idx < SPI_DEVICE_CS_CNT_MAX; idx++)
> +		spi_set_chipselect(spi, idx, SPI_INVALID_CS);
> +
> +	/* CS values must form a ascending contiguous sequence. */
> +	for (idx = 1; idx < spi->num_chipselect; idx++) {
> +		if (spi_get_chipselect(spi, idx) != spi_get_chipselect(spi, idx - 1) + 1)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
All of the test cases in spi_cs_config_suite (cs_cfg_single_valid(),
cs_cfg_multi_valid(), cs_cfg_overflow(), cs_cfg_out_of_range(),
cs_cfg_duplicate(), cs_cfg_gap(), cs_cfg_descending(), and cs_cfg_max_cs())
call this test-local spi_validate_cs_config() helper rather than exercising
the SPI core validation logic in __spi_add_device() in drivers/spi/spi.c:

drivers/spi/spi.c:__spi_add_device() {
	...
	if (spi->num_chipselect > SPI_DEVICE_CS_CNT_MAX) {
		dev_err(dev, "num_cs %d > max %d\n", spi->num_chipselect,
			SPI_DEVICE_CS_CNT_MAX);
		return -EOVERFLOW;
	}
	...
	/* Restrict CS logical values to a contiguous sequence */
	for (idx = 1; idx < spi->num_chipselect; idx++) {
		if (spi_get_chipselect(spi, idx) != spi_get_chipselect(spi, idx - 1) + 1) {
			...
			return -EINVAL;
		}
	}
	...
}

In addition, alloc_spi() initializes ctlr->num_chipselect = ctlr_num_cs,
which is never read because spi_validate_cs_config() takes a separate
ctlr_num_chipselect argument instead of checking
spi->controller->num_chipselect.

Could the CS validation logic in __spi_add_device() be factored out into a
helper in drivers/spi/spi.c and exposed via VISIBLE_IF_KUNIT and
EXPORT_SYMBOL_IF_KUNIT in drivers/spi/internals.h so that spi_cs_config_suite
tests the actual SPI core implementation?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792435248.git.Jonathan.Santos@analog.com?part=7