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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The parsed burnout current configuration (`measurement->burnout`) is never applied to the hardware.
- [Medium] Directly checking `fwnode_find_reference` for `avdd-supply` bypasses the regulator framework's dummy fallback, causing probe to fail if the supply is omitted from the devicetree.
- [Medium] Calculation of `ext_ref_uV` can underflow, leading to garbage scaling values and bypassing validation checks.

Pre-existing issues:
- [High] Short reads from `i2c_smbus_read_i2c_block_data()` are not correctly identified as errors, exposing uninitialized kernel stack memory to userspace.
--

commit 61a17369383e7866b0adf6d21770ae99f4aca379
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.
    
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
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c
> @@ -308,106 +463,6 @@ 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_DATA_RATE_CFG,
>  				 ADS112C14_DATA_RATE_CFG_GC_EN,
>  				 FIELD_PREP(ADS112C14_DATA_RATE_CFG_GC_EN,
>  					    measurement->global_chop));
>  	if (ret)
>  		return ret;

[Severity: Medium]
Is the burnout configuration supposed to be applied to the hardware here?

The parsed burn-out-current-nanoamp is saved to measurement->burnout in
ads112c14_parse_channels(), but this function does not appear to write it
to the ADS112C14_REG_DEVICE_CFG register. This might cause the feature to
be silently ignored even when configured in the devicetree.

[ ... ]

> @@ -494,7 +670,6 @@ static int ads112c14_read_raw(struct iio_dev *indio_dev,
>  	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;
>  
>  		switch (data->chip_info->resolution_bits) {
>  		case 16:
>  			*val = get_unaligned_be16(buf);

[Severity: High]
This isn't a bug introduced by this patch, but does this adequately handle
short reads?

The i2c_smbus_read_i2c_block_data() function called by
ads112c14_single_conversion() returns the positive number of bytes read on
success. If a short read occurs, ret will be positive and bypass the
ret < 0 check. 

Could this proceed to process a partially uninitialized buf array and expose
kernel stack memory to userspace?

[ ... ]

> @@ -1021,76 +1142,6 @@ static int ads112c14_probe(struct i2c_client *client)
>  	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: Medium]
Will this rigidly fail the probe if the avdd-supply is absent?

By directly using fwnode_find_reference(), it seems we bypass the regulator
framework's dummy fallback. If the devicetree omits avdd-supply, this
appears to return -ENOENT and fail unconditionally rather than treating
the missing reference as simply not matching refp_fwnode.

>  
>  		/* 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)
>  			return dev_err_probe(dev, ret, "failed to get refn voltage\n");
>  
>  		refn_uV = ret;
>  	} else {
>  		data->refn_is_gnd = true;
>  	}
>  
>  	data->ext_ref_uV = refp_uV - refn_uV;
>  
>  	if (data->ext_ref_uV && data->ext_ref_ohms)
>  		return dev_err_probe(dev, -EINVAL,
>  				     "ti,refp-refn-resistor-ohms property should not be present when refp-supply or refn-supply is present\n");
>  
>  	if (need_ext_ref && !data->ext_ref_uV && !data->ext_ref_ohms)
>  		return dev_err_probe(dev, -EINVAL,
>  				     "external reference measurements require either refp-supply or ti,refp-refn-resistor-ohms property\n");

[Severity: Medium]
Can this subtraction underflow if refp-supply is missing but refn-supply is
present?

If refp_uV is 0 and refn_uV is greater than 0, subtracting them as unsigned
32-bit integers will wrap to a large positive number. This seems like it
would bypass the subsequent !data->ext_ref_uV validation check and lead to
incorrect values when computing full_scale in ads112c14_populate_tables().

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