Re: [PATCH v12 5/6] iio: adc: ad4691: add oversampling support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Iterating over the soft timestamp channel without filtering causes out-of-bounds array access and hardware register corruption.
- [Medium] Strict OSR snapping logic traps users in a configuration deadlock by rejecting valid decreases to the oversampling ratio.
- [Medium] Silent cross-channel side effects and ABI violation due to scaling a shared global oscillator via separate channel attributes.
--

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

iio: adc: ad4691: add oversampling support

This patch adds oversampling ratio (OSR) support to the AD4691 driver for
CNV burst mode. It exposes oversampling_ratio and sampling_frequency
attributes and automatically scales the internal hardware oscillator to
support the requested rates.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index a6588292f3c18..4b32f50d2176f 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
> @@ -131,12 +132,39 @@ struct ad4691_chip_info {
>  	const struct ad4691_channel_info *offload_info;
>  };
>  
> +/* CNV burst mode channel — exposes oversampling ratio. */
>  #define AD4691_CHANNEL(ch)						\
>  	{								\
>  		.type = IIO_VOLTAGE,					\
>  		.indexed = 1,						\
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)		\
> -				    | BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) | \
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\

Could exposing SAMP_FREQ and OVERSAMPLING_RATIO as separate per-channel
attributes cause unexpected side effects?

Writing to either attribute recalculates and modifies st->target_osc_freq_Hz,
which is a global internal oscillator shared by all channels. This means
changing the OSR on one channel will instantly and silently alter the
effective sampling rate of all other active channels.

Since the hardware clock is shared, should these attributes use
info_mask_shared_by_all or info_mask_shared_by_type to conform to the IIO ABI
and ensure userspace expects the cross-channel impacts?

[ ... ]

> @@ -729,7 +971,40 @@ static int ad4691_write_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		return ad4691_set_sampling_freq(st, val);
> +		return ad4691_set_sampling_freq(st, chan, val);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO: {
> +		unsigned int old_effective, found;
> +		bool valid = false;
[ ... ]
> +		/*
> +		 * Hold st->lock while computing the new oscillator frequency
> +		 * and updating both target_osc_freq_Hz and osr[chan] atomically:
> +		 * read_raw for SAMP_FREQ reads both fields under the lock and
> +		 * must see a consistent pair (new osc ↔ new osr).
> +		 *
> +		 * Snap target_osc_freq_Hz to the largest table entry that is
> +		 * both <= old_effective * new_osr and evenly divisible by
> +		 * new_osr, preserving an integer read-back of
> +		 * in_voltageN_sampling_frequency after the OSR change.
> +		 */
> +		guard(mutex)(&st->lock);
> +		old_effective = st->target_osc_freq_Hz / st->osr[chan->channel];
> +		found = ad4691_find_osc_freq(st, old_effective * (unsigned int)val, val);
> +		if (!found)
> +			return -EINVAL;

Can this strict OSR snapping logic trap users in a configuration deadlock?

If the device is currently running at a low effective frequency and the user
attempts to decrease the OSR, the product old_effective * val may be smaller
than the minimum hardware oscillator frequency available.

When this happens, ad4691_find_osc_freq() will return 0, causing this to fail
with -EINVAL. This locks the user out from lowering the OSR unless they
manually increase the channel's sampling_frequency first.

Should the snapping logic gracefully clamp to the lowest available hardware
frequency instead of rejecting valid decreases to the oversampling ratio?

> +		st->target_osc_freq_Hz = found;
> +		st->osr[chan->channel] = val;
> +		return 0;
> +	}

[ ... ]

> @@ -943,6 +1222,12 @@ static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err_unoptimize;
>  
> +	iio_for_each_active_channel(indio_dev, i) {
> +		ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(i), st->osr[i]);
> +		if (ret)
> +			goto err_unoptimize;
> +	}
> +

Does iterating over active channels without filtering cause out-of-bounds
array access and hardware register corruption if the soft timestamp channel
is enabled?

The iio_for_each_active_channel() macro iterates over all set bits in
active_scan_mask, which includes the IIO soft timestamp channel (scan
index 16).

If i == 16, this triggers an out-of-bounds read of st->osr[16], since the
array is sized to AD4691_MAX_CHANNELS (16).

Furthermore, AD4691_ACC_DEPTH_IN(16) evaluates to 0x186 + 16 = 0x196, which is
AD4691_GPIO_MODE1_REG. This would overwrite the GPIO configuration with garbage,
potentially breaking the DATA_READY interrupt and halting conversions.

Additionally, could this same issue cause a buffer overflow later in this
function? If all 16 voltage channels plus the timestamp are enabled, the
existing loop increments k to 17. The code then appends a final state-reset
transfer at st->scan_xfers[2 * k] (index 34), which exceeds the fixed size
of the scan_xfers[34] array.

>  	ret = ad4691_enter_conversion_mode(st);
>  	if (ret)
>  		goto err_unoptimize;

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