Re: [PATCH 2/2] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Neil Armstrong <neil.armstrong@linaro.org>]

On 9/23/26 21:19, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] iio: max34417: division by zero in max34417_read_power() when acc_count is 0
> - [High] iio: max34417: division by zero in max34417_probe() if rsense is 0
> - [High] iio: max34417: build failure on 32-bit architectures due to GENMASK(55, 32)
> - [Medium] iio: max34417: fwnode handle leak on error paths in max34417_probe()
> - [Medium] iio: max34417: undeclared channels have uninitialized scale of 0
> --
> 
> commit 281cd76d0d1a3f010bbf1f4241c438deb9b82192
> Author: Neil Armstrong <neil.armstrong@linaro.org>
> 
>      iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator
>      
>      The MAX34417 is a specialized current and voltage monitor used to
>      determine power consumption of portable systems. The driver support
>      getting the channels voltage and accumulated average power over an
>      I2C/SMBUS serial interface.
> 
>> diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
>> new file mode 100644
>> index 0000000000000..004401de6ce7d
>> --- /dev/null
>> +++ b/drivers/iio/adc/max34417.c
> 
> [ ... ]
> 
>> +static int max34417_read_power(struct max34417_data *max34417,
>> +				 const struct iio_chan_spec *chan, int *val, int *val2)
>> +{
>> +	uint32_t acc_count;
>> +	uint64_t power;
>> +	uint8_t buf[8];
>> +	int rc;
>> +
>> +	guard(mutex)(&max34417->lock);
>> +
>> +	rc = max34417_accumulator_update(max34417);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = regmap_noinc_read(max34417->regmap, MAX34417_ACC_COUNT_REG,
>> +			       &buf, 4);
>> +	if (rc)
>> +		return rc;
>> +
>> +	acc_count = buf[3] | ((uint64_t)buf[2] << 8) | ((uint64_t)buf[1] << 16);
>> +
>> +	rc = regmap_noinc_read(max34417->regmap, chan->address, &buf, 8);
>> +	if (rc)
>> +		return rc;
>> +
>> +	power = buf[7];
>> +	power |= ((uint64_t)buf[6] << 8UL);
>> +	power |= ((uint64_t)buf[5] << 16UL);
>> +	power |= ((uint64_t)buf[4] << 24UL);
>> +	power |= ((uint64_t)buf[3] << 32UL);
>> +	power |= ((uint64_t)buf[2] << 40UL);
>> +	power |= ((uint64_t)buf[1] << 48UL);
>> +
>> +	power = div_u64(power, acc_count);
> 
> [Severity: High]
> Is it possible for acc_count to be zero here? If the hardware returns 0 (for
> example, due to an incomplete conversion, I2C read glitch, or uninitialized
> state), wouldn't this div_u64() cause a division by zero panic?

Good point

> 
>> +
>> +	*val = FIELD_GET(GENMASK(31, 0), power);
>> +	*val2 = FIELD_GET(GENMASK(55, 32), power);
> 
> [Severity: High]
> Will this build correctly on 32-bit architectures?
> 
> Since GENMASK() operates on unsigned long, which is 32 bits wide on those
> platforms, attempting to mask bits 55..32 might result in an out-of-bounds
> shift and trigger compiler errors or warnings like BUILD_BUG_ON_ZERO. Should
> GENMASK_ULL(55, 32) be used instead?

Good point

> 
>> +
>> +	return IIO_VAL_INT_64;
>> +}
>> +
>> +static int max34417_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val, int *val2, long mask)
>> +{
>> +	struct max34417_data *max34417 = iio_priv(indio_dev);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		if (chan->type == IIO_VOLTAGE)
>> +			return max34417_read_voltage(max34417, chan, val);
>> +		return -EINVAL;
>> +	case IIO_CHAN_INFO_AVERAGE_RAW:
>> +		if (chan->type == IIO_POWER)
>> +			return max34417_read_power(max34417, chan, val, val2);
>> +		return -EINVAL;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (chan->type == IIO_VOLTAGE) {
>> +			/* Scale to mA */
>> +			*val = MAX34417_VOLTAGE_CORRECTION_SCALE * MILLI;
>> +			*val2 = MAX34417_VOLTAGE_FULL_SCALE_BITS;
>> +
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		} else if (chan->type == IIO_POWER) {
>> +			/* Scale to mW */
>> +			*val = max34417->input_correction[chan->channel] * MILLI;
> 
> [Severity: Medium]
> Does this expose an uninitialized scale of 0 to userspace for omitted
> channels?
> 
> Since all 4 channels are unconditionally exposed by max34417_channels, but
> input_correction is only populated for channels explicitly defined in the
> device tree during probe, userspace reading the scale for a missing channel
> might get a value of 0.

Good point

> 
>> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
>> +
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		}
>> +		return -EINVAL;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
> 
> [ ... ]
> 
>> +static int max34417_probe(struct i2c_client *client)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct max34417_data *max34417;
>> +	struct fwnode_handle *node;
>> +	struct iio_dev *indio_dev;
>> +	struct regmap *regmap;
>> +	int rc;
> 
> [ ... ]
> 
>> +	device_for_each_child_node(dev, node) {
>> +		unsigned int correction;
>> +		u32 rsense, index;
>> +
>> +		if (fwnode_property_read_u32(node, "reg", &index)) {
>> +			dev_err(dev, "missing reg property of %pfwP\n", node);
>> +			return -EINVAL;
> 
> [Severity: Medium]
> Does this code leak the fwnode reference? Since device_for_each_child_node()
> takes a reference on the node during iteration, shouldn't fwnode_handle_put()
> be called before returning early?


Good point

> 
>> +		} else if (index >= MAX34417_CHANNEL_COUNT) {
>> +			dev_err(dev, "invalid reg %d of %pfwP\n", index, node);
>> +			return -EINVAL;
> 
> [Severity: Medium]
> Could this early return also leak the fwnode reference?
> 
>> +		}
>> +
>> +		fwnode_property_read_string(node, "label", &max34417->input_label[index]);
>> +		if (fwnode_property_read_u32(node, "maxim,rsense-val-micro-ohms", &rsense))
>> +			rsense = MAX34417_DEFAULT_RSENSE;
>> +
>> +		/* (100 milliOhm / rsense) * MAX34417_PWR_CORRECTION_SCALE */
>> +		correction = (100 * MILLI * MAX34417_PWR_CORRECTION_SCALE) / rsense;
> 
> [Severity: High]
> Can rsense be zero here?
> 
> If the device tree explicitly sets "maxim,rsense-val-micro-ohms" to 0,
> fwnode_property_read_u32() will succeed and leave rsense as 0, which would
> cause a division by zero when calculating correction.

Good point

> 
>> +
>> +		max34417->input_correction[index] = correction;
>> +	}
>> +
>