Re: [PATCH v4 4/9] spi: restrict CS logical values to a contiguous sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spi: __spi_add_device() contiguous CS check breaks non-consecutive multi-CS
--

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

spi: restrict CS logical values to a contiguous sequence

This commit adds a check in __spi_add_device() requiring chip-select values
for multi-CS SPI devices to form a consecutive ascending sequence, returning
-EINVAL otherwise.

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 7a0406ec200e1..2b2eeaa4c15d4 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -902,6 +902,16 @@ static int __spi_add_device(struct spi_device *spi, struct spi_device *parent)
>  	for (idx = spi->num_chipselect; idx < SPI_DEVICE_CS_CNT_MAX; idx++)
>  		spi_set_chipselect(spi, idx, SPI_INVALID_CS);
>  
> +	/* 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) {
> +			dev_err(dev, "cs%d is not consecutive with cs%d\n",
> +				spi_get_chipselect(spi, idx),
> +				spi_get_chipselect(spi, idx - 1));
> +			return -EINVAL;
> +		}
> +	}
> +

[Severity: High]
In drivers/spi/spi.c, idx (0 .. spi->num_chipselect - 1) is the logical CS
index, whereas spi_get_chipselect(spi, idx) returns spi->chip_select[idx],
which is the physical controller CS number mapped to that logical index, as
noted above SPI_INVALID_CS:

/*
 * Zero(0) is a valid physical CS value and can be located at any
 * logical CS in the spi->chip_select[].

During DeviceTree registration, of_register_spi_devices() ->
of_register_spi_device() -> of_spi_parse_dt() populates spi->chip_select[]
from the DT reg array before calling spi_add_device() -> __spi_add_device():

	spi->num_chipselect = rc;
	for (idx = 0; idx < rc; idx++)
		spi_set_chipselect(spi, idx, cs[idx]);

By checking spi_get_chipselect(spi, idx) != spi_get_chipselect(spi, idx - 1)
+ 1, __spi_add_device() requires the physical CS numbers in
spi->chip_select[] to be strictly consecutive and ascending.

For example, arch/arm/boot/dts/nxp/imx/imx53-ppd.dts defines spidev0 with
descending physical chip selects:

	spidev0: spi@1 {
		compatible = "ge,achc", "nxp,kinetis-k20";
		reg = <1>, <0>;
		vdd-supply = <&reg_3v3>;
		vdda-supply = <&reg_3v3>;
		clocks = <&achc_24M>;
		reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
	};

Per Documentation/devicetree/bindings/misc/ge-achc.yaml, reg entry 0 is the
control interface (on CS 1) and reg entry 1 is the firmware programming
interface (on CS 0), which drivers/misc/gehc-achc.c registers in
gehc_achc_probe() via spi_new_ancillary_device():

	ret = of_property_read_u32_index(spi->dev.of_node, "reg", 1, &ezport_reg);
	if (ret)
		return dev_err_probe(&spi->dev, ret, "missing second reg entry!\n");

	achc->ezport = spi_new_ancillary_device(spi, ezport_reg);

With this check in __spi_add_device(), spi_get_chipselect(spi, 1) is 0 while
spi_get_chipselect(spi, 0) + 1 is 2, causing __spi_add_device() to fail with
-EINVAL before gehc_achc_probe() can run.

Won't this cause a regression for imx53-ppd.dts as well as any other
multi-CS SPI peripheral wired to non-consecutive or descending physical
controller chip-select lines (such as reg = <1>, <0> or reg = <0>, <2>)?

>  	/* Set the bus ID string */
>  	spi_dev_set_name(spi);
>

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