Re: [PATCH v3 1/2] dt-bindings: iio: light: veml6030: add veml3328

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The runtime PM autosuspend timer is not started in probe, leaving the device powered on indefinitely.
- [Medium] Concurrent sysfs writes to integration time and scale can leave the hardware in an inconsistent state.
- [Low] Jumping over a variable initialization with the __cleanup__ attribute in a switch statement causes a compiler error.
--

commit e6504b133c6cc43d3687ab0ce437962c17f98107
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). Also add a new entry for said driver into
Kconfig and Makefile.

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

[ ... ]

> +static int veml3328_read_avail(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       const int **vals, int *type, int *length,
> +			       long mask)
> +{
> +	struct veml3328_data *data = iio_priv(indio_dev);
> +	struct regmap *regmap = data->regmap;
> +	struct device *dev = regmap_get_device(data->regmap);
> +	unsigned int reg_val;
> +	int ret, it_inx;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*length = ARRAY_SIZE(veml3328_it_times) * 2;
> +		*vals = (const int *)veml3328_it_times;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm);

[Severity: Low]
Does this jump over a variable initialization and cause a build regression?

The PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND() macro declares `pm` with the
__cleanup__ attribute. When `mask` does not match IIO_CHAN_INFO_INT_TIME or
IIO_CHAN_INFO_SCALE, the execution jumps to the `default:` label, which is
physically after the `pm` declaration.

Jumping over a variable initialization with the __cleanup__ attribute causes a
compilation regression in GCC.

Could we wrap this case in a scoped block { ... } to prevent this?

> +		ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
> +		if (it_inx >= ARRAY_SIZE(veml3328_it_times))
> +			return -EINVAL;
> +
> +		*length = 8;
> +		*vals = (const int *)veml3328_scale_vals[it_inx];
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

[ ... ]

> +static int veml3328_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct veml3328_data *data = iio_priv(indio_dev);
> +	struct regmap *regmap = data->regmap;
> +	struct device *dev = regmap_get_device(regmap);
> +	unsigned int reg_val;
> +	int i, it_inx;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	switch (mask) {

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);

[Severity: Medium]
Is there a race condition here that causes a regression during concurrent sysfs
writes?

This performs a lockless read-modify-write operation for IIO_CHAN_INFO_SCALE.
Because veml3328_write_raw() lacks driver-level locking (like a mutex), a
concurrent sysfs write to IIO_CHAN_INFO_INT_TIME could modify the hardware's
integration time index immediately after it is read here.

If that happens, the gain index calculated and applied below will be based on a
stale `it_inx`, leaving the hardware in an inconsistent configuration.

> +		if (it_inx >= ARRAY_SIZE(veml3328_it_times))
> +			return -EINVAL;
> +
> +		for (i = 0; i < 4; i++) {
> +			if (val == veml3328_scale_vals[it_inx][i * 2] &&
> +			    val2 == veml3328_scale_vals[it_inx][i * 2 + 1])
> +				break;
> +		}
> +
> +		if (i == 4)
> +			return -EINVAL;
> +
> +		return regmap_update_bits(regmap, VEML3328_REG_CONF,
> +					  VEML3328_CONF_GAIN_MASK,
> +					  FIELD_PREP(VEML3328_CONF_GAIN_MASK, i));
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

[ ... ]

> +static int veml3328_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct veml3328_data *data;
> +	struct iio_dev *indio_dev;
> +	unsigned int reg_val;
> +	int ret;

[ ... ]

> +	pm_runtime_set_active(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 2000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = devm_pm_runtime_enable(dev);

[Severity: Medium]
Are we missing a call to start the autosuspend timer here, causing a power
regression?

The sensor is physically powered on via veml3328_power_up(), and runtime PM
is initialized as active and enabled.

Since the usage count remains at 0 and the driver does not invoke
pm_runtime_idle() or a matched pm_runtime_get/put cycle at the end of probe,
the autosuspend timer is never scheduled.

Does this leave the device permanently powered on until a userspace application
accesses it?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530-veml3328-v3-0-dd562eaee8d9@gmail.com?part=1