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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Out-of-bounds array access in manual mode if only the timestamp channel is enabled.
- [High] Incorrect buffer size passed to `iio_push_to_buffers_with_ts()` causes captures to be dropped.
- [High] Unbalanced `disable_irq()` if buffer `postenable` fails permanently disables the hardware IRQ.
- [High] DMA cacheline sharing violation between `scan_tx_reset` and `vals` buffers.
- [High] Software timestamp bit is incorrectly written to the hardware sequencer register for AD4693.
- [High] Unconditional registration of an internal IIO trigger in manual mode exposes a broken default trigger.
- [High] Missing bounds validation in `sampling_frequency_store` allows userspace to break the PWM configuration.
- [Medium] Stale ADC values are leaked to userspace via buffer padding.
- [Medium] DATA_READY interrupt is registered as a threaded handler, degrading timestamp accuracy.
--

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

iio: adc: ad4691: add triggered buffer support

Add buffered capture support using the IIO triggered buffer framework.
It supports CNV Burst Mode via a PWM and DATA_READY IRQ, and a Manual
Mode using SPI N+1 pipelined transfers.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index ba77e1bfef166..bf27d5f33a49f 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
[ ... ]
> @@ -212,8 +246,56 @@ struct ad4691_state {
[ ... ]
> +	u8 scan_tx_reset[4] __aligned(IIO_DMA_MINALIGN);
> +	/*
> +	 * Scan buffer: one BE16 slot per active channel, plus timestamp.
> +	 * DMA-aligned because scan_xfers point rx_buf directly into vals[].
> +	 */
> +	IIO_DECLARE_DMA_BUFFER_WITH_TS(__be16, vals, 16);
>  };

The scan_tx_reset buffer is marked __aligned(IIO_DMA_MINALIGN) but is only
4 bytes long. The subsequent field, the anonymous struct created by
IIO_DECLARE_DMA_BUFFER_WITH_TS, lacks this alignment attribute.

Could this place the vals buffer immediately after scan_tx_reset (at offset
8) within the exact same cacheline?

During a SPI transfer, scan_tx_reset is mapped as DMA_TO_DEVICE while vals
is mapped as DMA_FROM_DEVICE. Does this bidirectional cacheline sharing violate
the DMA API and risk memory corruption on architectures with non-coherent
caches?

[ ... ]
> +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;
[ ... ]
> +	k = 0;
> +	iio_for_each_active_channel(indio_dev, i) {
> +		if (i >= indio_dev->num_channels - 1)
> +			break; /* skip soft timestamp */
[ ... ]
> +		k++;
> +	}
> +
> +	/* Final NOOP transfer retrieves the last channel's result. */
> +	st->scan_xfers[k].tx_buf = &st->scan_tx[k]; /* scan_tx[k] == 0 == NOOP */
> +	st->scan_xfers[k].rx_buf = &st->vals[k - 1];

If only the software timestamp channel is enabled, the loop above breaks
immediately and k remains 0. Since k is unsigned, does k - 1 underflow
to UINT_MAX?

Could this result in a pointer massively out-of-bounds, causing a kernel panic
or memory corruption when spi_sync executes a DMA write to this address?

[ ... ]
> +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;
[ ... ]
> +	std_seq_config = bitmap_read(indio_dev->active_scan_mask, 0,
> +				     iio_get_masklength(indio_dev)) & GENMASK(15, 0);
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG, std_seq_config);

For the AD4693 chip, the software timestamp channel is located at index 8. If
the user enables the timestamp, will bit 8 be included in the mask and written
to the hardware sequencer?

Since the timestamp is a software-only construct, does passing its bit to the
hardware sequencer misconfigure the ADC and cause it to sample an extra
non-existent channel?

[ ... ]
> +static int ad4691_cnv_burst_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	int ret;
> +
[ ... ]
> +	ret = ad4691_sampling_enable(st, true);
> +	if (ret)
> +		return ret;
> +
> +	enable_irq(st->irq);
> +	return 0;
> +}
> +
> +static int ad4691_cnv_burst_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
> +	disable_irq(st->irq);
> +	return ad4691_sampling_enable(st, false);
> +}

If ad4691_sampling_enable() fails in postenable, the function returns an
error before calling enable_irq().

When the IIO core unwinds the failure, it will call predisable(), which
unconditionally calls disable_irq(). Since the IRQ was never enabled, does
this increase the IRQ disable depth to 2?

Could this leave the IRQ permanently masked and halt all future data captures,
as subsequent successful buffer enables will only decrement the depth to 1?

[ ... ]
> +static ssize_t sampling_frequency_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	unsigned int freq;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &freq);
> +	if (ret)
> +		return ret;
> +
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +
> +	ret = ad4691_set_pwm_freq(st, freq);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}

Does this function need bounds validation for the user-provided frequency?

If userspace writes a frequency that exceeds st->info->max_rate (500 kHz),
the computed period in ad4691_set_pwm_freq() could be smaller than the
hardcoded 380 ns duty cycle.

Since this store function will silently return success, will the subsequent
buffer enable permanently fail when pwm_apply_might_sleep() rejects the
invalid PWM state?

[ ... ]
> +static void ad4691_trigger_reenable(struct iio_trigger *trig)
> +{
> +	struct ad4691_state *st = iio_trigger_get_drvdata(trig);
> +
> +	enable_irq(st->irq);
> +}
[ ... ]
> +static int ad4691_read_scan(struct iio_dev *indio_dev, s64 ts)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = spi_sync(st->spi, &st->scan_msg);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * 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);
> +	return 0;
> +}

The size passed here is sizeof(st->vals), which evaluates only to the size of
the array (32 bytes) and does not include the 8-byte timestamp.

If all channels and the timestamp are active, indio_dev->scan_bytes is 40.
Since the IIO core strictly checks if data_total_len < scan_bytes, will this
cause iio_push_to_buffers_with_ts() to return -ENOSPC and drop the valid
capture?

Additionally, the st->vals array does not appear to be cleared between buffer
captures. If a user reduces the number of active channels, could the IIO core
push uninitialized padding (containing stale ADC values from previous captures)
to userspace?

[ ... ]
> +static int ad4691_setup_triggered_buffer(struct iio_dev *indio_dev,
> +					 struct ad4691_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct iio_trigger *trig;
> +	unsigned int i;
> +	int irq, ret;
> +
> +	indio_dev->channels = st->info->sw_info->channels;
> +	indio_dev->num_channels = st->info->sw_info->num_channels;
> +	indio_dev->info = st->manual_mode ? &ad4691_manual_info : &ad4691_cnv_burst_info;
> +
> +	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);
> +
> +	if (st->manual_mode)
> +		return devm_iio_triggered_buffer_setup(dev, indio_dev,
> +						       &iio_pollfunc_store_time,
> +						       &ad4691_trigger_handler,
> +						       &ad4691_manual_buffer_setup_ops);

The internal trigger is unconditionally allocated, registered, and set as
indio_dev->trig before checking for manual mode.

Since st->irq remains 0 in manual mode, does exposing this trigger to userspace
allow a broken configuration where the trigger never fires?

If userspace polls this broken trigger and it is subsequently disabled, the IIO
core will invoke the .reenable callback which executes enable_irq(st->irq).
Can calling enable_irq(0) cause system instability or kernel warnings?

[ ... ]
> +	/*
> +	 * IRQ is kept disabled until the buffer is enabled to prevent
> +	 * spurious DATA_READY events before the SPI message is set up.
> +	 */
> +	ret = devm_request_threaded_irq(dev, irq, NULL,
> +					&ad4691_irq,
> +					IRQF_ONESHOT | IRQF_NO_AUTOEN,
> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return ret;

This registers the DATA_READY interrupt with a NULL primary handler, causing
ad4691_irq to run as a threaded function.

Since ad4691_irq() just calls disable_irq_nosync() and iio_trigger_poll()
(which are both hardirq safe), should this handler run in hardirq context
instead? Running it in process context might delay the software timestamp
capture by the length of a thread scheduling latency, introducing unnecessary
jitter.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-ad4692-multichannel-sar-adc-driver-v11-0-eab27d852ac2@analog.com?part=3