Re: [PATCH v1 07/13] iio: adc: ad4134: Support buffered data read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The trigger handler loops up to masklength instead of the physical channel count, causing an out-of-bounds array write and issuing an invalid SPI read for the software timestamp channel.
- [High] The scan buffer is undersized for captures with a timestamp, causing `iio_push_to_buffers_with_ts()` to always fail with `-ENOSPC`.
- [High] Adding the timestamp channel breaks single-shot reads by causing `ad4134_data_read()` to execute 5 SPI transfers instead of 4.
- [High] Removing DMA alignment from `rx_buf` and `tx_buf` causes them to share a cacheline with `st->scan`, leading to potential DMA data corruption.
- [Low] Buffer compaction logic uses `memcpy` with identically overlapping memory regions, triggering Undefined Behavior.
--

commit 923506d627d1a5bd978cd92880886f1fdb5f8bfd
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>
Date:   Wed Sep 2 17:23:42 2026 +0000

    iio: adc: ad4134: Support buffered data read
    
    This commit adds buffered data capture support for the AD4134, triggered by
    an IIO trigger device, and introduces a software timestamp channel. It also
    enforces direct access mode for single-read operations to prevent
    interference with buffered reads.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index a1474316e330d..0751b89dec8f3 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c

[ ... ]

> @@ -133,6 +144,7 @@ static const struct iio_chan_spec ad4134_chan_set[] = {
>  	AD4134_CHANNEL(1),
>  	AD4134_CHANNEL(2),
>  	AD4134_CHANNEL(3),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };

[Severity: High]
Will adding this timestamp channel break single-shot reads?

Looking at ad4134_data_read(), the loop iterating over this set uses
ARRAY_SIZE(ad4134_chan_set):

    for (i = 0; i < ARRAY_SIZE(ad4134_chan_set); i++) {
        ret = spi_write_then_read(...)

Since the array size is now 5, this will execute 5 times instead of the
4 times expected by the hardware. Could this clock out invalid data and
disrupt the ADC's data framing state machine?

>  struct ad4134_state {
> @@ -151,7 +163,8 @@ struct ad4134_state {
>  	 * DMA (thus cache coherency maintenance) requires the transfer buffers
>  	 * to live in their own cache lines.
>  	 */
> -	u8 rx_buf[AD4134_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> +	u32 scan[AD4134_NUM_CHANNELS] __aligned(IIO_DMA_MINALIGN);
> +	u8 rx_buf[AD4134_SPI_MAX_XFER_LEN];
>  	u8 tx_buf[AD4134_SPI_MAX_XFER_LEN];
>  };

[Severity: High]
Does the removal of the __aligned(IIO_DMA_MINALIGN) attribute from rx_buf
and tx_buf risk silent DMA data corruption?

Because scan is only 16 bytes (4 channels * 4 bytes), rx_buf and tx_buf
could now share the same cacheline. A concurrent debugfs/sysfs register
read/write could cause cacheline invalidation during a DMA unmap, resulting
in lost CPU writes to the scan buffer during a triggered capture.

Also, is the scan buffer large enough to hold the timestamp? The scan buffer
allocates 16 bytes, but the naturally-aligned 64-bit IIO timestamp appended
by IIO will require an additional 8 bytes of space.

[ ... ]

> @@ -336,6 +349,36 @@ static const struct regmap_config ad4134_regmap_config = {
>  	.max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set)),
>  };
>  
> +static irqreturn_t ad4134_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	unsigned int i = 0;
> +	int ret;
> +
> +	gpiod_set_value_cansleep(st->odr_gpio, 1);
> +	fsleep(1);
> +	gpiod_set_value_cansleep(st->odr_gpio, 0);
> +
> +	for (unsigned int ch = 0; ch < iio_get_masklength(indio_dev); ch++) {
> +		ret = spi_write_then_read(st->spi, NULL, 0, &st->scan[ch],
> +					  BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));

[Severity: High]
When the software timestamp channel is enabled, does iio_get_masklength()
evaluate to 5?

If so, this loop will exceed the hardware channel count (4) and write out of
bounds to st->scan[4]. This could directly corrupt the adjacent rx_buf field
and issue an invalid SPI read to the device.

> +		if (ret)
> +			goto err_out;
> +
> +		if (test_bit(ch, indio_dev->active_scan_mask))
> +			memcpy(&st->scan[i++], &st->scan[ch], sizeof(st->scan[ch]));

[Severity: Low]
Is it safe to use memcpy() here when the source and destination overlap?

When ch == i, the memory regions overlap identically. Since memcpy()
restricts overlapping pointers, this can trigger undefined behavior.

> +	}
> +
> +	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
> +				    pf->timestamp);

[Severity: High]
Does this fail with -ENOSPC when the timestamp channel is enabled?

The length passed here is sizeof(st->scan), which is 16 bytes. The IIO core
requires 24 bytes (16 bytes data + 8 bytes timestamp) for pushing captures
with a timestamp, which could cause every sample to be rejected and dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788368334.git.marcelo.schmitt@analog.com?part=7