Re: [PATCH v3 3/6] iio: imu: Add driver for the ADI ADIS16607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] spi: DMA cacheline sharing violation between rx and tx buffers
- [Medium] iio: missing hardware state teardown on self-test error
--

commit 7828515c587fd9239b2490c3f7b3aebfd3b79eaf
Author: Radu Sabau <radu.sabau@analog.com>

iio: imu: Add driver for the ADI ADIS16607

This commit introduces the base driver for the Analog Devices ADIS16607
series of IMUs. It includes probe functionality, raw channel reads,
frequency control, startup sequence, and debugfs support.

> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> new file mode 100644
> index 0000000000000..1d471f59f043d
> --- /dev/null
> +++ b/drivers/iio/imu/adis16607.c
> @@ -0,0 +1,855 @@
[ ... ]
> +static int adis16607_spi_read(struct adis *adis, unsigned int reg,
> +			      unsigned int *val, unsigned int size)
> +{
> +	struct spi_message msg;
> +	struct spi_transfer xfers[2] = {
> +		{
> +			.tx_buf = adis->tx,
> +			.len = 4,
> +			.bits_per_word = 8,
> +			.cs_change = 1,
> +		},
> +		{
> +			.tx_buf = adis->tx,
> +			.rx_buf = adis->rx,
> +			.len = 4,
> +			.bits_per_word = 8,
> +		},

[Severity: Medium]
Does this simultaneous DMA mapping of both adis->tx and adis->rx violate
DMA cacheline coherency?

In include/linux/iio/imu/adis.h, struct adis defines tx and rx adjacently:
    u8 tx[10] __aligned(IIO_DMA_MINALIGN);
    u8 rx[4];

Because rx lacks its own alignment boundary and tx is 10 bytes, rx falls
within the same DMA cacheline as tx. On non-cache-coherent architectures,
mapping both buffers concurrently in opposite DMA directions can discard
ongoing cache flushes or CPU writes for tx during the cache invalidation
for rx, potentially leading to hardware data corruption.

[ ... ]
> +static int adis16607_sensor_self_test(struct adis16607 *st)
> +{
> +	struct adis *adis = &st->adis;
[ ... ]
> +	/* Read second set of self-test data */
> +	for (i = 0; i < 6; i++) {
> +		ret = __adis_read_reg_16(adis, ADIS16607_REG_SELF_TEST_DATA(i),
> +					 &st2_val[i]);
> +		if (ret)
> +			return ret;
> +
> +		if (!adis16607_delta_check((s16)st1_val[i], (s16)st2_val[i],
> +					   delta_max[i])) {
> +			dev_err(&adis->spi->dev,
> +				"Self-test failed for axis %d: delta out of range\n", i);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Clear self-test register */
> +	ret = __adis_write_reg_16(adis, ADIS16607_REG_SELF_TEST, 0);

[Severity: Medium]
Will returning early from the loop bypass the teardown of the self-test mode?

If the second read fails or adis16607_delta_check() fails, the function
returns immediately. This skips the final __adis_write_reg_16() that clears
the ADIS16607_REG_SELF_TEST register, leaving the sensor hardware locked
in an active diagnostic/force state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-adis16607-v3-0-f972d4efee85@analog.com?part=3