Re: [PATCH 3/4] iio: adc: ti-ads112c14: implement gain on internal short SYS_MON channel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `write_raw` and `read_avail` IIO callbacks completely ignore the `long mask` argument.
- [Medium] Data race when reading `sys_mon_chan_short_gain_val` without locking or READ_ONCE().
--

commit 7a8163396f906f80891d134c7fb8faa25cc0a10a
Author: David Lechner (TI) <dlechner@baylibre.com>

iio: adc: ti-ads112c14: implement gain on internal short SYS_MON channel
    
Implement support for the programmable gain amplifier on the internal
short SYS_MON channel. This channel is used for calibration, so it is
useful to be able to set the PGA to the same gain as the external
channels. The gain setting is implemented via the _scale attribute.

> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> index 97097ae2a4872..202a449b4234f 100644
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c
[ ... ]
> @@ -366,6 +395,17 @@ static int ads112c14_read_raw(struct iio_dev *indio_dev,
>  			return IIO_VAL_FRACTIONAL_LOG2;
>  		}
>  
> +		if (chan->channel == ADS112C14_SYS_MON_CHANNEL_SHORT) {
> +			int *scale_avail = data->sys_mon_chan_short_scale_available[
> +				data->sys_mon_chan_short_gain_val
> +			];

[Severity: Medium]
Is there a potential data race when reading data->sys_mon_chan_short_gain_val
here?

This value is modified in ads112c14_write_raw() while holding the direct mode
lock, but it appears to be read here without locking or READ_ONCE(). Could a
concurrent sysfs write cause an issue while looking up the scale available
array?

> +
> +			*val = scale_avail[0];
> +			*val2 = scale_avail[1];
> +
> +			return IIO_VAL_INT_PLUS_NANO;
> +		}
> +
>  		*val = vref_uV / (MICRO / MILLI);
>  		/*
>  		 * Last 3 SYS_MON channels (ext ref, AVDD, DVDD) need to be
> @@ -387,6 +427,61 @@ static int ads112c14_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int ads112c14_read_avail(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan, const int **vals,
> +				int *type, int *length, long mask)
> +{
> +	struct ads112c14_data *data = iio_priv(indio_dev);
> +
> +	switch (chan->channel) {
> +	case ADS112C14_SYS_MON_CHANNEL_SHORT:
> +		*vals = (const int *)data->sys_mon_chan_short_scale_available;
> +		*length = 2 * ARRAY_SIZE(data->sys_mon_chan_short_scale_available);
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		return IIO_AVAIL_LIST;

[Severity: High]
Does this function need to check the mask parameter?

Currently, it returns the scale availability array unconditionally. If other
_available attributes are queried, this might incorrectly return scale data
for those non-scale queries.

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ads112c14_write_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan, int val,
> +			       int val2, long mask)
> +{
> +	struct ads112c14_data *data = iio_priv(indio_dev);
> +
> +	switch (chan->channel) {
> +	case ADS112C14_SYS_MON_CHANNEL_SHORT: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;

[Severity: High]
Should this function also verify that mask == IIO_CHAN_INFO_SCALE?

Since there is no mask check, if the IIO core creates a writable _raw
attribute, any writes to it might be incorrectly interpreted as scale updates.

> +
> +		for (u32 i = 0; i < ARRAY_SIZE(data->sys_mon_chan_short_scale_available); i++) {
> +			if (val == data->sys_mon_chan_short_scale_available[i][0] &&
> +			    val2 == data->sys_mon_chan_short_scale_available[i][1]) {
> +				data->sys_mon_chan_short_gain_val = i;
> +				return 0;
> +			}
> +		}
> +		return -EINVAL;
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-iio-adc-ti-ads122c14-v1-0-e6bdadf7cb2b@baylibre.com?part=3