Re: [PATCH v12 3/6] iio: adc: ad4691: add triggered buffer support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Reference leak of IIO trigger on probe error or deferral
- [High] Array out-of-bounds writes due to unskipped soft timestamp channel
- [High] Integer overflow in `sampling_frequency_store` leading to Division by Zero kernel panic
- [Medium] Unchecked `spi_sync` return value in trigger handler pushes stale data
--

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

iio: adc: ad4691: add triggered buffer support

This commit adds buffered capture support using the IIO triggered buffer
framework. It supports both CNV Burst Mode and Manual Mode, sharing the same
trigger handler to push a complete scan to the buffer.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index 2d58df8621422..ed60ed5b488c2 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c

[ ... ]

> @@ -548,13 +619,397 @@ static int ad4691_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>  	return regmap_write(st->regmap, reg, writeval);
>  }
>  
> +static int ad4691_set_pwm_freq(struct ad4691_state *st, unsigned int freq)
> +{
> +	if (!freq)
> +		return -EINVAL;
> +
> +	st->cnv_period_ns = DIV_ROUND_UP(NSEC_PER_SEC, freq);

Could this cause an integer overflow on 32-bit architectures? 

A very large value for freq combined with NSEC_PER_SEC could wrap around in 
the DIV_ROUND_UP macro addition, resulting in a numerator that evaluates to 
zero.

If st->cnv_period_ns becomes zero, would the division in 
sampling_frequency_show() later trigger a division by zero?

> +	return 0;
> +}

[ ... ]

> +static int ad4691_manual_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	unsigned int k, i;
> +	int ret;
> +
> +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> +	memset(st->scan_tx, 0, sizeof(st->scan_tx));
> +
> +	spi_message_init(&st->scan_msg);
> +
> +	k = 0;
> +	iio_for_each_active_channel(indio_dev, i) {

Does this loop explicitly need to skip the software timestamp channel?

Since iio_for_each_active_channel() iterates over all set bits in the scan
mask, including the timestamp channel if enabled by userspace, could k reach
17?

If k reaches 17, it appears this would write past the end of the 17-element
scan_tx array here:

> +		/*
> +		 * Channel-select command occupies the first (high) byte of the
> +		 * 16-bit DIN frame; the second byte is a don't-care zero pad.
> +		 * put_unaligned_be16() writes [cmd, 0x00] in memory so the
> +		 * SPI controller sends the command byte first on the wire.
> +		 */
> +		put_unaligned_be16((u16)(AD4691_ADC_CHAN(i) << 8), &st->scan_tx[k]);

[ ... ]

> +static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	unsigned int acc_mask, std_seq_config;
> +	unsigned int k, i;
> +	int ret;
> +
> +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> +	memset(st->scan_tx, 0, sizeof(st->scan_tx));
> +
> +	spi_message_init(&st->scan_msg);
> +
> +	/*
> +	 * Each AVG_IN read needs two transfers: a 2-byte address write phase
> +	 * followed by a 2-byte data read phase. CS toggles between channels
> +	 * (cs_change=1 on the read phase of all but the last channel).
> +	 */
> +	k = 0;
> +	iio_for_each_active_channel(indio_dev, i) {
> +		put_unaligned_be16(0x8000 | AD4691_AVG_IN(i), &st->scan_tx[k]);
> +		st->scan_xfers[2 * k].tx_buf = &st->scan_tx[k];
> +		st->scan_xfers[2 * k].len = sizeof(*st->scan_tx);
> +		spi_message_add_tail(&st->scan_xfers[2 * k], &st->scan_msg);
> +		st->scan_xfers[2 * k + 1].rx_buf = &st->vals[k];
> +		st->scan_xfers[2 * k + 1].len = sizeof(*st->scan_tx);
> +		st->scan_xfers[2 * k + 1].cs_change = 1;
> +		spi_message_add_tail(&st->scan_xfers[2 * k + 1], &st->scan_msg);
> +		k++;
> +	}
> +
> +	/*
> +	 * Append a 4-byte state-reset transfer [addr_hi, addr_lo,
> +	 * STATE_RESET_ALL, OSC_EN=1]. CS is asserted throughout, so
> +	 * ADDR_DESCENDING writes byte[3]=1 to OSC_EN_REG (0x180) as a
> +	 * deliberate side-write, keeping the oscillator enabled.
> +	 * STATE_RESET_ALL starts the next burst; the hardware does not
> +	 * accumulate new conversions until after a STATE_RESET pulse, so
> +	 * no in-progress data is lost.  No cs_change here — CS must
> +	 * deassert normally at end of message to frame the next command.
> +	 */
> +	put_unaligned_be16(AD4691_STATE_RESET_REG, st->scan_tx_reset);
> +	st->scan_tx_reset[2] = AD4691_STATE_RESET_ALL;
> +	st->scan_tx_reset[3] = 1;
> +	st->scan_xfers[2 * k].tx_buf = st->scan_tx_reset;

Similarly to the manual mode preenable function, does this loop need to skip
the software timestamp channel?

If the timestamp channel is included, k would reach 17, and 2 * k would be 34,
which appears to write out of bounds of the 34-element scan_xfers array.

> +	st->scan_xfers[2 * k].len = sizeof(st->scan_tx_reset);

[ ... ]

> +static void ad4691_read_scan(struct iio_dev *indio_dev, s64 ts)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +
> +	spi_sync(st->spi, &st->scan_msg);

Is it safe to ignore the return value of spi_sync() here? 

If the SPI transfer fails due to a bus error or timeout, could this silently
push uninitialized or stale data from st->vals to userspace?

> +
> +	/*
> +	 * rx_buf pointers in scan_xfers point directly into scan.vals, so no
> +	 * copy is needed. The scan_msg already includes a STATE_RESET at the
> +	 * end (appended in preenable), so no explicit reset is needed here.
> +	 */
> +	iio_push_to_buffers_with_ts(indio_dev, st->vals, sizeof(st->vals), ts);
> +}

[ ... ]

> +static int ad4691_setup_triggered_buffer(struct iio_dev *indio_dev,
> +					 struct ad4691_state *st)
> +{

[ ... ]

> +	/*
> +	 * CNV burst mode: allocate an internal trigger driven by the
> +	 * DATA_READY IRQ on the GP pin.
> +	 */
> +	trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> +				      iio_device_id(indio_dev));
> +	if (!trig)
> +		return -ENOMEM;
> +
> +	trig->ops = &ad4691_trigger_ops;
> +	iio_trigger_set_drvdata(trig, st);
> +
> +	ret = devm_iio_trigger_register(dev, trig);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "IIO trigger register failed\n");
> +
> +	indio_dev->trig = iio_trigger_get(trig);
> +
> +	/*
> +	 * The GP pin named in interrupt-names asserts at end-of-conversion.
> +	 * The IRQ handler fires the IIO trigger so the trigger handler can
> +	 * read and push the sample to the buffer. The IRQ is kept disabled
> +	 * until the buffer is enabled.
> +	 */
> +	irq = -ENXIO;
> +	for (i = 0; i < ARRAY_SIZE(ad4691_gp_names); i++) {
> +		irq = fwnode_irq_get_byname(dev_fwnode(dev),
> +					    ad4691_gp_names[i]);
> +		if (irq > 0 || irq == -EPROBE_DEFER)
> +			break;
> +	}
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq, "failed to get GP interrupt\n");

If ad4691_setup_triggered_buffer() returns an error here (or in the
subsequent setup steps below) and probe aborts, will the reference count
acquired by iio_trigger_get() be leaked?

Since iio_device_register() would not be reached, the devres cleanup won't
invoke iio_device_unregister_trigger_consumer() to call iio_trigger_put().

> +
> +	st->irq = irq;
> +
> +	ret = ad4691_gpio_setup(st, i);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-ad4692-multichannel-sar-adc-driver-v12-0-5b335162aa51@analog.com?part=3