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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: concurrent use of shared DMA buffers st->rx_buf and st->tx_buf
- [High] spi: cs_off = true in triggered buffer breaks 4-wire mode communication
- [High] iio: ADC output frame resets due to looped SPI transfers
--

commit 29f8197d694e692f78d7a5142e0a389309c577b0
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

    iio: adc: ad4134: Support buffered data read
    
    Enable users to run buffered data captures triggered by IIO trigger device.
    Add an IIO timestamp channel so each data scan is provided with measurement
    time information. Require single-read operations to be in IIO device direct
    access mode to prevent buffered and single-shot data captures to disrupt
    each other.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 14996a0d1fe88..f0196126515e6 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c

[ ... ]

> @@ -333,6 +344,43 @@ static const struct regmap_config ad4134_regmap_config = {
>  	.max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set) - 1),
>  };
>  
> +static irqreturn_t ad4134_trigger_handler(int irq, void *p)
> +{
> +	IIO_DECLARE_BUFFER_WITH_TS(u32, channels, AD4134_NUM_CHANNELS) = { };
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	struct spi_transfer xfer = {
> +		.rx_buf = st->rx_buf,

[Severity: High]
If userspace reads or writes ADC registers via debugfs while the triggered
buffer is running in minimum I/O mode (NO_CS), ad4134_debugfs_reg_access()
can run concurrently with ad4134_trigger_handler().

Because the trigger handler does not acquire any locks to protect the shared
DMA buffers, and while regmap acquires st->access_lock, this lock is
explicitly not held during buffer capture in NO_CS mode (as setup_ops is
NULL), both threads can use the shared st->rx_buf and st->tx_buf
simultaneously.

Does this concurrent use cause DMA buffer corruption, potential kernel panics
during concurrent DMA mappings, or incorrect data reads?

> +		.cs_off = true,

[Severity: High]
When the driver is configured in 4-wire mode (adi,spi-mode = "4-wire") and
falls back to the software trigger handler, this flag explicitly instructs
the SPI controller to leave the chip select line inactive (high) for the
duration of the transfer.

Since the AD4134 requires an active (low) chip select to frame the data in
4-wire mode, does this setting break 4-wire mode communication and prevent
the device from outputting valid sample data?

> +		.len = BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS),
> +	};
> +	int ret;
> +
> +	gpiod_set_value_cansleep(st->odr_gpio, 1);
> +	fsleep(1);
> +	gpiod_set_value_cansleep(st->odr_gpio, 0);
> +
> +	/* Execute transfers for all channels so the entire data frame is read. */
> +	for (int ch = 0, i = 0; ch < AD4134_NUM_CHANNELS; ch++) {
> +		ret = spi_sync_transfer(st->spi, &xfer, 1);

[Severity: High]
In 4-wire mode, the AD4134 resets its data output frame if the chip select
goes high or if the DCLK is idle for more than 250ns.

Does calling spi_sync_transfer() in a loop for each channel introduce
software delays far exceeding 250ns and toggle the CS line between calls?

Could this violate the device's continuous clocking and CS timing
requirements, causing the frame to reset on every iteration and reading
Channel 0 repeatedly instead of streaming consecutive channels?

Should a single bulk SPI transfer be used here instead of a loop?

> +		if (ret)
> +			goto out;
> +
> +		if (!test_bit(ch, indio_dev->active_scan_mask))
> +			continue;
> +
> +		channels[i++] = get_unaligned_be24(st->rx_buf);
> +	}
> +
> +	iio_push_to_buffers_with_ts(indio_dev, channels, sizeof(channels),
> +				    pf->timestamp);
> +
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}

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