Re: [PATCH v2 2/2] iio: light: veml3328: add support for new device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Signed 32-bit integer overflow in the lux scale calculation due to large multiplication.
- [Medium] Inconsistent handling of the IIO_LIGHT channel scale across read, write, and available attributes breaks the IIO ABI.
- [Medium] The scale attribute for IIO_INTENSITY channels incorrectly returns the hardware gain multiplier instead of the true IIO scale factor.
--

commit 9edb449d4e1325159429ae7e39210b37d222a175
Author: Joshua Crofts <joshua.crofts1@gmail.com>

iio: light: veml3328: add support for new device

Add support for the Vishay VEML3328 RGB/IR light sensor communicating
via I2C (SMBus compatible).

> diff --git a/drivers/iio/light/veml3328.c b/drivers/iio/light/veml3328.c
> new file mode 100644
> index 0000000000000..fdfa8fd1666b6
> --- /dev/null
> +++ b/drivers/iio/light/veml3328.c

[ ... ]

> +static const struct iio_chan_spec veml3328_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = VEML3328_REG_DATA_G,
> +	},

[Severity: Medium]
Is the in_illuminance_scale_available sysfs attribute missing here because
info_mask_shared_by_all_available does not include the scale bit for the
IIO_LIGHT channel?

[ ... ]

> +static int veml3328_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		if (chan->type == IIO_LIGHT) {
> +			int it_inx, gain_inx;
> +			int it_us, scale_u;
> +
> +			it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
> +			gain_inx = FIELD_GET(VEML3328_CONF_GAIN_MASK, reg_val);
> +
> +			if (it_inx >= ARRAY_SIZE(veml3328_it_times) ||
> +			    gain_inx >= ARRAY_SIZE(veml3328_scale_vals))
> +				return -EINVAL;
> +
> +			it_us = veml3328_it_times[it_inx][1];
> +
> +			/*
> +			 * Equation for calculating ambient light scale:
> +			 * Scale = 0.384 * (50000 / Current_IT) * (1x / Current_Gain)
> +			 */
> +			scale_u = 384 * USEC_PER_MSEC;
> +			scale_u = scale_u  * 50000 / it_us;

[Severity: High]
Does this code overflow scale_u?

Since scale_u is initially set to 384,000 (384 * 1000) and is a 32-bit signed
integer, the multiplication by 50000 evaluates to 19,200,000,000. This exceeds
the maximum positive value of 2,147,483,647 for a signed 32-bit int.

> +			switch (gain_inx) {

[ ... ]

> +		}
> +
> +		reg = FIELD_GET(VEML3328_CONF_GAIN_MASK, reg_val);
> +		if (reg >= ARRAY_SIZE(veml3328_scale_vals))
> +			return -EINVAL;
> +
> +		*val = veml3328_scale_vals[reg][0];
> +		*val2 = veml3328_scale_vals[reg][1];
> +		return IIO_VAL_INT_PLUS_MICRO;

[Severity: Medium]
For IIO_INTENSITY channels, this returns the hardware gain multiplier directly
from veml3328_scale_vals. Will this instruct userspace to multiply an already
increased raw value by the hardware gain again?

Should the scale instead return the inverse (1 / gain) to correctly normalize
the readings?

[ ... ]

> +static int veml3328_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		for (i = 0; i < ARRAY_SIZE(veml3328_scale_vals); i++) {
> +			if (val == veml3328_scale_vals[i][0] &&
> +			    val2 == veml3328_scale_vals[i][1])
> +				break;
> +		}

[Severity: Medium]
For the IIO_LIGHT channel, veml3328_read_raw() dynamically calculates a lux
scale, but this write function compares the input against the raw hardware
gain multipliers in veml3328_scale_vals without verifying the channel type.

Does this mean writing the read scale value back will fail with -EINVAL?

Writing a valid gain value here seems to succeed and modify the hardware
gain, which would change the lux scale to something entirely different.
Could this inconsistent behavior break the expected IIO ABI principle of
being able to write back the values that were read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-veml3328-v2-0-a57f0d9e28d4@gmail.com?part=2