Re: [PATCH v4 8/8] iio: adc: ti-ads112c14: add measurement channel support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Probe will erroneously fail if `refp-supply` is present but `avdd-supply` is missing in the device tree.
- [High] An I2C short read leaves stack memory uninitialized, exposing it to user space.
- [Medium] The scale calculation in `ads112c14_populate_scale_available()` can overflow `u64` when a large external reference resistor is configured.
- [Medium] Calculation of excitation currents above 60 µA overflows the 3-bit IMAG bitfields, resulting in silently incorrect hardware configuration.
- [Medium] Incompatible format specifiers break `IIO_CHAN_INFO_SCALE` writing due to integer composition mismatch.
--

--- Patch [8]: [PATCH v4 8/8] iio: adc: ti-ads112c14: add measurement channel support ---
commit 0662f6e0d1a8a66e59b6cab55397eee9f82357fd
Author: David Lechner (TI) <dlechner@baylibre.com>

iio: adc: ti-ads112c14: add measurement channel support

Add support for parsing devicetree properties for measurement channels
and doing direct reads on these. There are quite a lot of conditions
that have to be met for each measurement to be made, so quite a bit of
state and algorithms are required to handle it. Channels are created
dynamically since the number of possibilities is unreasonably large.

> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> index 17bb6472a39b5..fef3a9c37cedc 100644
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c

[ ... ]

> @@ -263,12 +302,112 @@ static const struct regmap_config ads112c14_regmap_config = {
>  	.cache_type = REGCACHE_MAPLE,
>  };
>  
> +static int ads112c14_prepare_measurement_channel(struct ads112c14_data *data,
> +						 const struct iio_chan_spec *chan)
> +{
> +	struct ads112c14_measurement *measurement = &data->measurements[chan->scan_index];
> +	u32 refp_buf_en, refn_buf_en, ref_val, ref_sel;
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, ADS112C14_REG_MUX_CFG,

[ ... ]

> +	ret = regmap_update_bits(data->regmap, ADS112C14_REG_IDAC_MAG_CFG,
> +				 ADS112C14_IDAC_MAG_CFG_I2MAG |
> +				 ADS112C14_IDAC_MAG_CFG_I1MAG,
> +				 FIELD_PREP(ADS112C14_IDAC_MAG_CFG_I2MAG,
> +					    measurement->idac2_mag) |
> +				 FIELD_PREP(ADS112C14_IDAC_MAG_CFG_I1MAG,
> +					    measurement->idac1_mag));
                                            ^^^^

[Severity: Medium]
If measurement->idac1_mag or idac2_mag exceeds the 3-bit capacity of the
I1MAG and I2MAG register fields, will FIELD_PREP silently truncate the value?

Because FIELD_PREP is being called with a non-constant variable, its
compile-time bounds checks are bypassed. The ads112c14_populate_idac_mag()
function can calculate values up to 11, which would truncate to 3 here and
silently misconfigure the hardware excitation current.

> +	if (ret)
> +		return ret;

[ ... ]

> @@ -356,6 +496,7 @@ static int ads112c14_read_raw(struct iio_dev *indio_dev,
>  			      int *val, int *val2, long mask)
>  {
>  	struct ads112c14_data *data = iio_priv(indio_dev);
> +	struct ads112c14_measurement *measurement = NULL;
>  	const int *scale_avail;
>  	u32 vref_uV, fsr_bits;
>  
> @@ -363,8 +504,8 @@ static int ads112c14_read_raw(struct iio_dev *indio_dev,
>  	vref_uV = ADS112C14_INT_REF1_mV * (MICRO / MILLI);
>  
>  	if (chan->channel < ADS112C14_SYS_MON_CHANNEL_BASE) {
> -		/* Not implemented yet. */
> -		return -EINVAL;
> +		measurement = &data->measurements[chan->scan_index];
> +		fsr_bits = data->chip_info->resolution_bits - measurement->bipolar;
>  	} else {
>  		/* All SYS_MON channels are using signed coding. */
>  		fsr_bits = data->chip_info->resolution_bits - 1;
> @@ -394,7 +535,8 @@ static int ads112c14_read_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  
> -		*val = sign_extend32(*val, fsr_bits);
> +		if (!measurement || measurement->bipolar)
> +			*val = sign_extend32(*val, fsr_bits);
                            ^^^^

[Severity: High]
Does an I2C short read in ads112c14_single_conversion() leave the buffer
uninitialized?

When ads112c14_single_conversion() calls i2c_smbus_read_i2c_block_data(), it
returns the number of bytes read upon success. A short read (e.g. 1 or 2 bytes)
would return a positive value, bypassing the (ret < 0) error check in
ads112c14_read_raw().

If that happens, could the remaining uninitialized bytes on the kernel stack be
sign-extended and exposed to user space?

>  
>  		return IIO_VAL_INT;
>  	}

[ ... ]

> @@ -486,7 +650,13 @@ static int ads112c14_write_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SCALE: {
>  		guard(mutex)(&data->lock);
>  
> -		if (chan->channel == ADS112C14_SYS_MON_CHANNEL_SHORT) {
> +		if (chan->channel < ADS112C14_SYS_MON_CHANNEL_BASE) {
> +			struct ads112c14_measurement *measurement;
> +
> +			measurement = &data->measurements[chan->scan_index];
> +			scale_avail = measurement->scale_available;
> +			gain_val = &measurement->gain_val;
> +		} else if (chan->channel == ADS112C14_SYS_MON_CHANNEL_SHORT) {
>  			scale_avail = data->sys_mon_chan_short_scale_available;
>  			gain_val = &data->sys_mon_chan_short_gain_val;
>  		} else {

[Severity: Medium]
Will this break IIO_CHAN_INFO_SCALE writing due to mismatched units?

The driver's ads112c14_write_raw_get_fmt() specifies IIO_VAL_INT_PLUS_NANO,
which tells the IIO core to parse user strings into nanounits. However, the
scale_avail arrays generated by ads112c14_populate_scale_available() store
values in picounits.

When the user value (in nanounits) is checked via iio_val_s64_compose() against
the scale_avail array (in picounits) further down in ads112c14_write_raw(),
will it always fail to match and return -EINVAL?

[ ... ]

> +static void ads112c14_populate_scale_available(int (*scale_avail)[2],
> +					       u32 full_scale, u32 fsr_bits)
> +{
> +	for (u32 i = 0; i < ARRAY_SIZE(ads112c14_pga_gains_x10); i++) {
> +		u64 gain_x10 = ads112c14_pga_gains_x10[i];
> +		s64 scale;
> +
> +		scale = div64_u64((u64)PICO * 10U * full_scale,
> +				  gain_x10 * BIT(fsr_bits));
                ^^^^

[Severity: Medium]
Could this calculation overflow a 64-bit unsigned integer?

If a large external reference resistor is configured (e.g., 10 Megaohms) via
the ti,refp-refn-resistor-ohms property, full_scale will be 10,000,000.
The arithmetic (u64)PICO * 10U * 10,000,000 evaluates to 10^20, which exceeds
the maximum capacity of a u64 (~1.84 * 10^19).

> +
> +		iio_val_s64_decompose(scale, &scale_avail[i][0],
> +				      &scale_avail[i][1]);
> +	}
> +}

[ ... ]

> @@ -608,13 +1029,76 @@ static int ads112c14_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	if (device_property_present(dev, "ti,refp-refn-resistor-ohms")) {

[ ... ]

> +	if (device_property_present(dev, "refp-supply")) {
> +		ret = devm_regulator_get_enable_read_voltage(dev, "refp");
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "failed to get refp voltage\n");
> +
> +		refp_uV = ret;
> +
> +		struct fwnode_handle *refp_fwnode __free(fwnode_handle) =
> +			fwnode_find_reference(dev->fwnode, "refp-supply", 0);
> +		if (IS_ERR(refp_fwnode))
> +			return dev_err_probe(dev, PTR_ERR(refp_fwnode),
> +					     "failed to get refp fwnode\n");
> +
> +		struct fwnode_handle *avdd_fwnode __free(fwnode_handle) =
> +			fwnode_find_reference(dev->fwnode, "avdd-supply", 0);
> +		if (IS_ERR(avdd_fwnode))
> +			return dev_err_probe(dev, PTR_ERR(avdd_fwnode),
> +					     "failed to get avdd fwnode\n");
                        ^^^^

[Severity: High]
Does this cause the probe to erroneously abort if "avdd-supply" is omitted in
the device tree?

If "refp-supply" is defined but "avdd-supply" is not, fwnode_find_reference()
will return -ENOENT for avdd_fwnode. The probe function will unconditionally
fail here, even though the regulator framework can safely provide a dummy
regulator for an optional avdd-supply.

> +
> +		/* REFP buffer should not be enabled when connected to AVDD */
> +		data->refp_is_avdd = refp_fwnode == avdd_fwnode;
> +	}
> +
> +	if (device_property_present(dev, "refn-supply")) {
> +		ret = devm_regulator_get_enable_read_voltage(dev, "refn");
> +		if (ret < 0)

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-iio-adc-ti-ads122c14-v4-0-25f8e3084485@baylibre.com?part=8