Re: [PATCH v11 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] UAPI Violation: `sampling_frequency` is exposed as a separate attribute but modifies shared hardware state.
- [High] Logic Error: Eager snapping of `target_osc_freq_Hz` prevents setting attributes in arbitrary order as claimed.
- [Medium] Data Race: Lockless read of `samp_freq_avail` by the IIO core can result in torn or out-of-bounds reads.
--

commit 263cf27684f7e7bf6a4d514df77024b26ca11416
Author: Radu Sabau <radu.sabau@analog.com>
Subject: iio: adc: ad4691: add oversampling support

This commit adds per-channel oversampling ratio (OSR) support for CNV
burst mode in the ad4691 driver. It introduces manual and offload channel
variations and dynamically computes available sampling frequencies based
on the channel's current OSR.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index 25f7a6939b0fb..39244e0e4a2d8 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;
>  };
>  
> +/* 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 this be a UAPI violation? The sampling_frequency is exposed with
info_mask_separate, giving each channel its own sampling frequency attribute
(e.g., in_voltage0_sampling_frequency).

However, writing to this attribute modifies st->target_osc_freq_Hz, which is
the shared base clock for all channels. If channels have different
oversampling ratios, writing to one channel's sampling frequency will
silently alter the effective sampling rate of other channels.

Should this be exposed with info_mask_shared_by_all (or shared_by_type)
instead, so userspace knows the clock is shared?

[ ... ]
> @@ -588,41 +732,65 @@ 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_get_sampling_freq(struct ad4691_state *st, int *val)
> +/*
> + * Find the largest oscillator table entry that is both <= needed_osc and
> + * evenly divisible by osr (guaranteeing an integer effective rate on
> + * read-back). Returns 0 if no such entry exists in the chip's valid range.
> + */
> +static unsigned int ad4691_find_osc_freq(struct ad4691_state *st,
> +					 unsigned int needed_osc,
> +					 unsigned int osr)
>  {
[ ... ]
> -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, found;
>  
>  	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
>  	if (IIO_DEV_ACQUIRE_FAILED(claim))
>  		return -EBUSY;
>  
> -	for (unsigned int i = start; i < ARRAY_SIZE(ad4691_osc_freqs_Hz); i++) {
> -		if (ad4691_osc_freqs_Hz[i] != freq)
> -			continue;
> -		return regmap_update_bits(st->regmap, AD4691_OSC_FREQ_REG,
> -					  AD4691_OSC_FREQ_MASK, i);
> -	}
> +	/*
> +	 * Read osr under st->lock: osr[chan] and target_osc_freq_Hz are
> +	 * modified together under the lock; reading after acquiring it ensures
> +	 * we see a consistent snapshot with no concurrent write racing us.
> +	 */
> +	guard(mutex)(&st->lock);
> +	osr = st->osr[chan->channel];
>  
> -	return -EINVAL;
> +	if (freq <= 0 || (unsigned int)freq > st->info->max_rate / osr)
> +		return -EINVAL;

Does this eager check break the ability to set attributes in any order?

The commit message states that sampling_frequency and oversampling_ratio
can be set in any order because they are deferred until buffer enable.
However, if transitioning from a high OSR (e.g. 32) to a low OSR (e.g. 1)
with a high sampling frequency, writing the frequency first will fail here
because it is evaluated against the old OSR of 32.

[ ... ]
> @@ -634,10 +802,46 @@ 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;
> +		int n = 0;
> +
> +		/*
> +		 * Hold the lock while reading osr[chan] and populating the
> +		 * scratch buffer: a concurrent oversampling_ratio write modifies
> +		 * both target_osc_freq_Hz and osr[] under the lock, so we must
> +		 * read osr atomically with respect to that write. The scratch
> +		 * buffer is per-channel, so concurrent reads on different
> +		 * channels do not race; concurrent reads on the same channel
> +		 * would compute identical values, but holding the lock avoids
> +		 * the formal data race.
> +		 */
> +		scoped_guard(mutex, &st->lock) {
> +			osr = st->osr[chan->channel];
> +
> +			/*
> +			 * 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 = ARRAY_SIZE(ad4691_osc_freqs_Hz) - start;
> +		*length = n;
> +		return IIO_AVAIL_LIST;

Could this lead to a data race with the IIO core?

The st->samp_freq_avail array is populated while holding st->lock, but the
lock is released before returning. The IIO core will read the array locklessly
when formatting the sysfs string. If another thread writes to
oversampling_ratio concurrently, the array might be repopulated with a new
shorter length. This could cause the IIO core to read stale or uninitialized
values out of bounds of the new length.

[ ... ]
> @@ -728,9 +953,48 @@ static int ad4691_write_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int val, int val2, long mask)
>  {
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		return ad4691_set_sampling_freq(indio_dev, val);
> +		return ad4691_set_sampling_freq(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO: {
[ ... ]
> +		/*
> +		 * 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;
> +		st->target_osc_freq_Hz = found;
> +		st->osr[chan->channel] = val;
> +		return 0;

Similarly, does this logic force a write-order trap for oversampling_ratio?

If a user writes the OSR first when transitioning between setups, this code
multiplies old_effective by the new OSR. If no valid oscillator frequency
exists below that product, this returns -EINVAL. It might also snap the base
oscillator frequency to an unpredictably low intermediate value, contradicting
the commit message's claim that both attributes can be set in any order.

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