Re: [PATCH v4 3/3] iio: adc: ti-ads112c14: add continuous mode support

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 30 Aug 2026 22:00:34 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> > Add support for continuous mode in the TI ADS112C14 ADC driver. In this
> > mode the ADC itself is starting each conversion, so we add a trigger
> > based on the DRDY interrupt to read each sample. This mode is also
> > limited in that only one channel can be enabled at a time since the
> > chip does not have a sequencer or simultaneous sampling capability.
> > Continuous mode will only be used when this new trigger is the current
> > trigger.
> > 
> > Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>  
> Sashiko caught some interesting things in this one.
> 
> See inline.

Sorry - messed up my use of b4 review tui and failed to include the
sashiko blobs I was talking about. I can't work out how to reply to review
I already did in there, so doing this via cut and paste.

> 
> >
> > diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> > index 23f15be303fb..878764deffc5 100644
> > --- a/drivers/iio/adc/ti-ads112c14.c
> > +++ b/drivers/iio/adc/ti-ads112c14.c
> > @@ -9,6 +9,7 @@
> >   */
> >  
> >  #include <linux/bitfield.h>
> > +#include <linux/bitmap.h>
> >  #include <linux/cleanup.h>
> >  #include <linux/completion.h>
> >  #include <linux/crc8.h>
> > @@ -18,6 +19,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/iio.h>
> > +#include <linux/iio/trigger.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/triggered_buffer.h>
> >  #include <linux/interrupt.h>
> > @@ -257,6 +259,7 @@ struct ads112c14_measurement {
> >  struct ads112c14_data {
> >  	const struct ads112c14_chip_info *chip_info;
> >  	struct regmap *regmap;
> > +	struct iio_trigger *drdy_trig;
> >  	/* Synchronizes access to register value fields. */
> >  	struct mutex lock;
> >  	int drdy_irq;
> > @@ -280,11 +283,18 @@ static irqreturn_t ads112c14_drdy_irq_handler(int irq, void *private)
> >  	struct iio_dev *indio_dev = private;
> >  	struct ads112c14_data *data = iio_priv(indio_dev);
> >  
> > -	complete(&data->drdy_completion);
> > +	if (indio_dev->trig && iio_trigger_using_own(indio_dev))  
> 
> 
 | sashiko.dev <sashiko@sashiko.dev>:
 |
 | [Severity: Critical]
 | Can this sequence race with a concurrent sysfs write?
 |
 | If a direct read is initiated and indio_dev->trig is checked without a lock,
 | could a concurrent sysfs write to current_trigger nullify or free the
 | trigger right before iio_trigger_using_own() dereferences it, leading to a
 | NULL pointer dereference or use-after-free crash?

> This indeed needs more guarding given we hit these interrupts for
> a normal read (to get to the completion)
> 
> 
 |
 | [Severity: High]
 | Does this logic cause direct reads to hang?
 |
 | During a direct read (e.g., via sysfs) while the buffer is disabled but a
 | trigger is assigned, this condition evaluates to true. Will this route the
 | interrupt completion to iio_trigger_poll(), leaving the direct read waiting
 | indefinitely for data->drdy_completion and resulting in a timeout?
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com

> Likewise seems valid.
> 
> > +		iio_trigger_poll(data->drdy_trig);
> > +	else
> > +		complete(&data->drdy_completion);
> >  
> >  	return IRQ_HANDLED;
> >  }  
> 
> ...
> 
> >  static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
> >  {
> >  	struct iio_poll_func *pf = private;
> > @@ -961,6 +984,26 @@ static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
> >  	u32 i;
> >  	int ret;
> >  
> > +	if (iio_trigger_using_own(indio_dev)) {  
> 
>  | sashiko.dev <sashiko@sashiko.dev>:
>  |
>  | [Severity: High]
>  | Will this check incorrectly enable continuous mode processing for any
>  | external trigger that belongs to its own device?
>  |
>  | If a user assigns an external trigger that sets its attached_own_device
>  | property to true, iio_trigger_using_own() will evaluate to true. Could this
>  | misroute the interrupt handler into assuming continuous mode when it should
>  | not?
>  |
>  | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com
> 
> I think this is triggered by the more complex handling you have
> to detect use of the dataready trigger below.  There is only one trigger
> currently so they end up the same, but perhaps we should make the handling
> more similar?
> 
> > +		const struct iio_chan_spec *chan;
> > +
> > +		chan = ads112c14_first_active_channel(indio_dev);
> > +		if (!chan)
> > +			goto out;
> > +
> > +		ret = ads112c14_scan_read(data, (u8 *)&data->scan[0]);  
> 
>  | sashiko.dev <sashiko@sashiko.dev>:
>  |
>  | [Severity: High]
>  | This is a pre-existing issue, but is there a missing length check in the
>  | underlying I2C block read that leaves stack memory partially uninitialized?
>  |
>  | If i2c_smbus_read_i2c_block_data() (called by ads112c14_scan_read()) returns
>  | a positive value smaller than the requested length, the condition if (ret < 0)
>  | fails to catch the short read. Does this leak uninitialized stack memory to
>  | userspace when data->scan is later pushed to the IIO buffers?
>  |
>  | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com
> 
> I haven't chased this one through - but in a similar example in a review
> I did yesterday I suggested just initializing the memory anyway
> so we don't have to care one way or the other.
> 
> > +		if (ret) {
> > +			dev_err_once(indio_dev->dev.parent,
> > +				     "failed to read channel %d: %pe; additional errors will be suppressed\n",
> > +				     chan->channel, ERR_PTR(ret));
> > +			goto out;
> > +		}
> > +
> > +		iio_push_to_buffers_with_ts(indio_dev, data->scan,
> > +					    sizeof(data->scan), pf->timestamp);
> > +		goto out;
> > +	}
> > +
> >  	iio_for_each_active_channel(indio_dev, i) {
> >  		const struct iio_chan_spec *chan = &indio_dev->channels[i];
> >  
> > @@ -992,6 +1035,88 @@ static const struct iio_info ads112c14_info = {
> >  	.read_label = ads112c14_read_label,
> >  };
> >    
> 
> ...
> 
> > +static int ads112c14_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +	struct ads112c14_data *data = iio_priv(indio_dev);
> > +	const struct iio_chan_spec *chan;
> > +	int ret;
> > +
> > +	if (!ads112c14_using_drdy_trigger(indio_dev))
> > +		return 0;
> > +
> > +	chan = ads112c14_first_active_channel(indio_dev);
> > +	if (!chan)
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	ret = ads112c14_prepare_channel(data, chan);  
> 
 | sashiko.dev <sashiko@sashiko.dev>:
 |
 | [Severity: High]
 | Can a user bypass the single-channel limitation and push corrupted data
 | to the ring buffer here?
 |
 | Since ads112c14_validate_scan_mask() allows multiple channels if a non-DRDY
 | trigger is currently selected, a user could enable multiple channels via
 | scan_elements, then switch current_trigger to the DRDY trigger before
 | enabling the IIO buffer.

> 
> Hmm. Indeed an interesting corner...  From a quick look it is correct.
> I'm not sure if this is in practice an existing bug - do other
> drivers change acceptable channels based on another change such as
> which trigger is in use?   Either way we need to close this.
> 
> We can't just go clearing the set bits on setting the trigger as
> that might cause a regression.  So I think all we can do is add
> a well commented additional check early in the buffer enable path.
> 
> Given the behaviour that is causing problems is present in this
> driver we could either add the protection in fix and rely on that
> going upstream first, or add it as first patch in this series and
> let it work its way upstream with this patch.
> 
> Nice catch to sashiko!
> 

 |
 | Because this driver does not provide an available_scan_masks array, the
 | mask isn't re-validated during buffer enablement. Since
 | ads112c14_buffer_postenable() only configures the first active channel,
 | will the trigger handler push uninitialized heap memory for the remaining
 | channels when the full array is sent to userspace?
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com

I think that's all the feedback I replied to...

J