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

[Jonathan Cameron]

> 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.

> +		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?

> +		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.

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


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!

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>