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

[sashiko-bot@kernel.org]

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?

> +
> +	*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?

> +
> +	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.

> +			*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?

> +		} 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.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-topic-sm8x50-iio-max34417-adc-v1-0-41d4ba1bfc41@linaro.org?part=2