Re: [PATCH v2 6/8] iio: adc: add ti-ads112c14 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Short I2C reads lead to the use of uninitialized stack memory for ADC readings.
- [High] The driver inadvertently sets the device into Continuous Conversion mode instead of the intended Single-Shot mode.
--

commit 610f816911f47ce29d3c83f300222d6ab72f8820
Author: David Lechner (TI) <dlechner@baylibre.com>

iio: adc: add ti-ads112c14 driver

Add a new driver for the TI ADS112C14/ADS122C14 ADC chips.

This first step is adding a very basic driver that only supports power
on/reset and reading the system monitor channels.

> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> new file mode 100644
> index 0000000000000..c61d472447325
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads112c14.c

[ ... ]

> +static int ads112c14_single_conversion(struct ads112c14_data *data,
> +				       const struct iio_chan_spec *chan,
> +				       u8 *buf)
> +{

[ ... ]

> +	return i2c_smbus_read_i2c_block_data(client, ADS112C14_CMD_RDATA,
> +					     BITS_TO_BYTES(data->chip_info->resolution_bits),
> +					     buf);
> +}
> +
> +static int ads112c14_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int *val,
> +			      int *val2, long mask)
> +{

[ ... ]

> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		u8 buf[3];
> +		int ret;
> +
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		ret = ads112c14_single_conversion(data, chan, buf);
> +		iio_device_release_direct(indio_dev);
> +		if (ret < 0)
> +			return ret;

[Severity: High]
Does this code handle short I2C reads correctly?

i2c_smbus_read_i2c_block_data() returns the positive byte count on success.
If the device returns fewer bytes than requested, ret will be positive but
less than BITS_TO_BYTES(data->chip_info->resolution_bits).

Since ads112c14_read_raw() only checks if (ret < 0), won't a short read
bypass error handling and cause it to process uninitialized stack memory from
buf, leading to an information leak and corrupted ADC readings?

> +
> +		switch (data->chip_info->resolution_bits) {
> +		case 16:
> +			*val = get_unaligned_be16(buf);

[ ... ]

> +static int ads112c14_probe(struct i2c_client *client)
> +{

[ ... ]

> +	/* Place in single-shot conversion mode to make ready for raw read. */
> +	ret = regmap_set_bits(data->regmap, ADS112C14_REG_DEVICE_CFG,
> +			      ADS112C14_DEVICE_CFG_CONV_MODE);

[Severity: High]
Does setting ADS112C14_DEVICE_CFG_CONV_MODE actually put the device into
continuous conversion mode instead of the intended single-shot mode?

According to the comment, the intention is to use single-shot mode (which is
typically the default value of 0). Setting the bit to 1 appears to enable
continuous conversion mode, which could cause hardware race conditions and
corrupted data when multiplexer settings are changed for new sysfs readings.

> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = info->name;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-iio-adc-ti-ads122c14-v2-0-ceb9b0b561cb@baylibre.com?part=6