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

[David Lechner]

On 8/30/26 4:00 PM, Jonathan Cameron 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@xxxxxxxxxxxx>
> Sashiko caught some interesting things in this one.
> 
> See inline.
> 
>>
>> 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))
> 
> 
> This indeed needs more guarding given we hit these interrupts for
> a normal read (to get to the completion)
> 
> 
> Likewise seems valid.

I figured it didn't need guarding since we are claiming direct mode. The
trigger can't be assigned until buffer mode is claimed. and we can't
switch to buffer mode if direct mode is claimed.


> 
>> +		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@xxxxxxxxxxx>:
>  |
>  | [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@xxxxxxxxxxxx
> 
> 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?

I'll just create our own local flag instead of relying on IIO internals.

> 
>> +		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@xxxxxxxxxxx>:
>  |
>  | [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@xxxxxxxxxxxx
> 
> 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.

This uses i2c_smbus_xfer() in the end which returns 0 or negative error.
No partial reads. So I don't see how we could be leaking.

> 
>> +		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];
>>