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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Buffer pre-enable loops iterate over the soft timestamp channel, causing an out-of-bounds array read and overwriting the GPIO configuration register.
- [High] Missing `.endianness = IIO_BE` in `AD4691_MANUAL_CHANNEL` macro.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race in `ad4691_set_sampling_freq`.
- [Medium] Data race and unprotected shared state in `ad4691_read_avail`.
- [Medium] Unlocked reads in `ad4691_read_raw` lead to torn state reads.
- [Medium] Writing `oversampling_ratio` silently modifies the global oscillator frequency, contradicting the commit message.
--

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

iio: adc: ad4691: add oversampling support

This commit adds per-channel oversampling ratio (OSR) support for CNV
burst mode on the AD4691. It introduces attributes for sampling frequency
and OSR, and splits channel specifications to handle manual mode where OSR
is not applicable.

> Writing oversampling_ratio stores the new OSR for that channel;
> target_osc_freq_Hz is left unchanged. The effective rate read back via
> in_voltageN_sampling_frequency becomes target_osc_freq_Hz / new_osr
> automatically. The two attributes are orthogonal: sampling_frequency
> controls the oscillator, oversampling_ratio controls the averaging depth.

Is this description accurate?

Looking at ad4691_write_raw() for IIO_CHAN_INFO_OVERSAMPLING_RATIO, the code
actively snaps target_osc_freq_Hz to a new value to preserve the previous
effective rate. Because target_osc_freq_Hz corresponds to a single global
hardware oscillator, modifying it here silently changes the effective sampling
rate of all other active channels.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index 011a3cd46da0d..842c4012801cc 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
[ ... ]
> @@ -127,12 +129,39 @@ struct ad4691_chip_info {
>  	const struct ad4691_channel_info *offload_info;
>  };
>  
[ ... ]
> +/*
> + * Manual mode channel — no oversampling ratio attribute. OSR is not
> + * supported in manual mode; ACC_DEPTH_IN is not configured during manual
> + * buffer enable.
> + */
> +#define AD4691_MANUAL_CHANNEL(ch)					\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
>  		.info_mask_separate_available =				\
>  				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),	\
>  		.channel = ch,						\
>  		.scan_index = ch,					\
>  		.scan_type = {						\
>  			.sign = 'u',					\
>  			.realbits = 16,					\
>  			.storagebits = 16,				\
>  		},							\
>  	}

Are we missing the endianness assignment in this macro?

The regular AD4691_CHANNEL macro specifies an endianness of IIO_BE. Without
it here, the IIO core defaults to CPU endianness. Since the hardware
transmits MSB-first over SPI, userspace applications running on little-endian
hosts might interpret the manual mode buffer data incorrectly.

[ ... ]
> @@ -511,36 +602,58 @@ static unsigned int ad4691_samp_freq_start(const struct ad4691_chip_info *info)
>  	return (info->max_rate == 1 * HZ_PER_MHZ) ? 0 : 1;
>  }
>  
[ ... ]
> -static int ad4691_set_sampling_freq(struct iio_dev *indio_dev, int freq)
> +static int ad4691_set_sampling_freq(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan, int freq)
>  {
>  	struct ad4691_state *st = iio_priv(indio_dev);
> -	unsigned int start = ad4691_samp_freq_start(st->info);
> +	unsigned int osr = st->osr[chan->channel];
> +	unsigned int found;
>  
>  	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
>  	if (IIO_DEV_ACQUIRE_FAILED(claim))
>  		return -EBUSY;

Could reading the oversampling ratio before acquiring the IIO direct mode
lock lead to a race condition?

If a concurrent write to the oversampling ratio attribute occurs, this
function will block on the lock. Once it unblocks, it proceeds with the
stale osr value to calculate and set the new target oscillator frequency,
which might not be evenly divisible by the newly set oversampling ratio.

[ ... ]
> @@ -552,10 +665,30 @@ static int ad4691_read_avail(struct iio_dev *indio_dev,
>  	unsigned int start = ad4691_samp_freq_start(st->info);
>  
>  	switch (mask) {
> -	case IIO_CHAN_INFO_SAMP_FREQ:
> -		*vals = &ad4691_osc_freqs_Hz[start];
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		unsigned int osr = st->osr[chan->channel];
> +		int n = 0;
> +
> +		/*
> +		 * Only oscillator frequencies evenly divisible by the channel's
> +		 * OSR yield an integer effective rate; expose those as effective
> +		 * rates (osc / osr) so the user works entirely in output-sample
> +		 * space.
> +		 */
> +		for (unsigned int i = start; i < ARRAY_SIZE(ad4691_osc_freqs_Hz); i++) {
> +			if (ad4691_osc_freqs_Hz[i] % osr)
> +				continue;
> +			st->samp_freq_avail[chan->channel][n++] = ad4691_osc_freqs_Hz[i] / osr;
> +		}
> +		*vals = st->samp_freq_avail[chan->channel];
> +		*type = IIO_VAL_INT;
> +		*length = n;
> +		return IIO_AVAIL_LIST;

Does populating and returning the shared scratch array without locking create
a data race?

Concurrent sysfs reads of the sampling frequency available attribute for the
same channel could result in multiple threads writing to the
st->samp_freq_avail array simultaneously.

[ ... ]
> @@ -632,7 +773,10 @@ static int ad4691_read_raw(struct iio_dev *indio_dev,
>  		return ad4691_single_shot_read(indio_dev, chan, val);
>  	}
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		return ad4691_get_sampling_freq(st, val);
> +		return ad4691_get_sampling_freq(st, st->osr[chan->channel], val);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = st->osr[chan->channel];
> +		return IIO_VAL_INT;

Should these attribute reads be protected by a lock?

Since ad4691_write_raw() modifies both target_osc_freq_Hz and the channel
OSR sequentially, a concurrent unlocked read could observe the newly updated
oscillator frequency but the old oversampling ratio, returning an incorrect
effective sampling rate.

[ ... ]
> @@ -866,6 +1045,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;
> +	}
> +

Can this loop iterate past the bounds of the osr array?

If the IIO soft timestamp channel is enabled, its bit is set with scan_index
16. The macro iterates over all enabled channels, causing this loop to access
st->osr[16], which exceeds the 16-element array.

Writing this out-of-bounds value using AD4691_ACC_DEPTH_IN(16) evaluates to
register 0x196, which is AD4691_GPIO_MODE1_REG. This could inadvertently
overwrite the device's GPIO configuration.

[ ... ]
> @@ -1045,6 +1230,12 @@ static int ad4691_cnv_burst_offload_buffer_postenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		return ret;
>  
> +	iio_for_each_active_channel(indio_dev, bit) {
> +		ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(bit), st->osr[bit]);
> +		if (ret)
> +			return ret;
> +	}
> +

Is it possible for the soft timestamp channel to be active here as well,
leading to the same out-of-bounds access on st->osr[bit]?

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