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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The buffer preenable functions do not skip the software timestamp channel, leading to array out-of-bounds accesses and invalid SPI commands.
- [High] The IIO trigger reference count is leaked if initialization fails in `ad4691_setup_triggered_buffer`.
--

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

iio: adc: ad4691: add triggered buffer support

This commit adds buffered capture support for the AD4691 family using
the IIO triggered buffer framework. It implements both CNV burst mode
and manual mode for pipeline reading.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
[ ... ]
> +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) {

[Severity: Critical]
Does this loop correctly skip the software timestamp channel?

The iio_for_each_active_channel() macro iterates over all enabled channels,
including the soft timestamp channel (index 16 for AD4691). If all 16
physical channels and the timestamp channel are active, the loop will
execute 17 times.

In this case, k will reach 16, and the final NOOP transfer appended after
the loop will attempt to write to st->scan_tx[17] and st->scan_xfers[17],
which exceeds the allocation sizes of those arrays. Could this cause memory
corruption and malformed SPI messages?

[ ... ]
> +static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	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];

[Severity: Critical]
Does this loop also need to skip the timestamp channel?

Similar to manual mode above, if all 16 physical channels and the timestamp
channel are enabled, k will increment to 17. Because each AVG_IN read takes
two transfers, the appended 4-byte state-reset transfer after the loop will
attempt to write to st->scan_xfers[34], which is past the end of the
34-element array.

[ ... ]
> +static int ad4691_setup_triggered_buffer(struct iio_dev *indio_dev,
> +					 struct ad4691_state *st)
> +{
[ ... ]
> +	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");

[Severity: High]
If fwnode_irq_get_byname() or devm_request_irq() fails, does this leak the
IIO trigger reference?

The iio_trigger_get() call increments the trigger device's reference count.
During cleanup, the IIO core via iio_dev_release() will only call
iio_device_unregister_trigger_consumer() to perform the matching
iio_trigger_put() if the INDIO_ALL_TRIGGERED_MODES flag is set in
indio_dev->modes.

Because INDIO_BUFFER_TRIGGERED is only added to the modes inside
devm_iio_triggered_buffer_setup_ext() later in this function, an error
return here leaves modes as INDIO_DIRECT_MODE. Will the trigger be
permanently leaked preventing the module from being unloaded?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-ad4692-multichannel-sar-adc-driver-v13-0-1b7626d3b35c@analog.com?part=3