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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 7 May 2026 11:56:53 +0000
"Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

> Addressing Sashiko's review for the oversampling support patch.
> 
One thing inline. I think Sashiko got it wrong..

> > @@ -691,6 +857,10 @@ static int ad4691_enter_conversion_mode(struct
> > ad4691_state *st)
> >  		return regmap_update_bits(st->regmap,
> > AD4691_DEVICE_SETUP,
> >  					  AD4691_MANUAL_MODE,
> > AD4691_MANUAL_MODE);
> > 
> > +	ret = ad4691_write_osc_freq(st);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = regmap_update_bits(st->regmap, AD4691_ADC_SETUP,
> >  				 AD4691_ADC_MODE_MASK,
> > AD4691_CNV_BURST_MODE);
> >  	if (ret)
> > @@ -844,6 +1014,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;
> > +	}
> > +  
> 
> "If the IIO soft timestamp channel is enabled, its scan_index is 16.
> iio_for_each_active_channel() iterates over all active channels including
> the timestamp channel.

I saw this one and think it's wrong.

It is sort of true and sort of not.  active_scan_mask never has the timestamp
channel set.  There is special handling for that channel.

https://elixir.bootlin.com/linux/v7.0.1/source/drivers/iio/industrialio-buffer.c#L613

IIRC the bitmap is technically one bit too large given that quirk
(I've not checked that today so I'm relying on memory of when this last came up).

Jonathan



> Will this loop process i = 16 and access st->osr[16] out of bounds, since
> st->osr is an array of size 16?
> Additionally, since AD4691_ACC_DEPTH_IN(16) resolves to 0x196
> (AD4691_GPIO_MODE1_REG), will this silently corrupt the GPIO configuration
> and potentially break the DATA_READY interrupt?
> Furthermore, does the presence of the timestamp channel cause the loop
> iteration to advance k to 17, making the state-reset SPI transfers appended
> after the loop write to st->scan_xfers[34] and [35]? This would overflow
> the scan_xfers array which is sized 34 and could corrupt the surrounding
> struct ad4691_state."
> 
> Valid for the osr[] out-of-bounds and GPIO corruption concerns. With
> the soft timestamp enabled (scan_index = 16), the loop would access
> st->osr[16] out-of-bounds and write to AD4691_ACC_DEPTH_IN(16) = 0x196,
> which is AD4691_GPIO_MODE1_REG, silently corrupting the GPIO configuration
> and potentially breaking the DATA_READY interrupt.
> 
> Added the same guard used by the scan_xfers loops in the triggered-buffer
> commit: if (i >= indio_dev->num_channels - 1) break.
> 
> The scan_xfers k-overflow concern is already handled by that existing guard
> in a separate loop — it is not affected by the ACC_DEPTH_IN loop added
> here.
>