Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Runtime PM usage counter underflow due to unbalanced puts.
- [Medium] The IR intensity channel does not expose its available scales to userspace.
- [Medium] Missing lock in raw read path allows concurrent configuration changes to corrupt sensor readings.
--

commit 5e5c3dc820c07a34f2519b89a89cabf3c88212a1
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

iio: light: add support for veml6031x00 ALS series

These sensors provide two light channels (ALS and IR), I2C communication
and a multiplexed interrupt line to signal data ready and configurable
threshold alarms.

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

[ ... ]

> +static const struct iio_chan_spec veml6031x00_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.address = VEML6031X00_REG_ALS_L,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.address = VEML6031X00_REG_IR_L,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_IR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +	},

[Severity: Medium]
Will userspace be able to discover the valid scale multipliers for the IR
channel?

The ALS channel includes BIT(IIO_CHAN_INFO_SCALE) in its
info_mask_separate_available bitmask, but it appears to be omitted here
for the IR channel.

Because of this omission, the IIO core won't create the
in_intensity_ir_scale_available sysfs attribute.

[ ... ]

> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
> +				   int *val)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int addr, it_usec, ret;
> +	__le16 reg;

[ ... ]

> +	ret = veml6031x00_get_it(data, &it_usec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* integration time + 10 % to ensure completion */
> +	fsleep(it_usec + (it_usec / 10));

[Severity: Medium]
Can a concurrent sysfs write corrupt this sensor reading?

This read sequence sleeps here using fsleep() without holding a lock to
serialize the read against concurrent sysfs writes.

If a user thread concurrently modifies the integration time or scale via
veml6031x00_write_raw(), the hardware's measurement cycle might be reset
mid-measurement. When single_read() wakes up, it could read the data register
before the new measurement has finished, returning stale or corrupted data.

> +
> +	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
> +	if (ret)
> +		return ret;

[ ... ]

> +static int veml6031x00_probe(struct i2c_client *i2c)
> +{

[ ... ]

> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = devm_pm_runtime_get_noresume(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get runtime PM\n");
> +
> +	ret = veml6031x00_validate_part_id(data);
> +	if (ret)
> +		return ret;

[ ... ]

> +	ret = veml6031x00_hw_init(iio);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_put_autosuspend(dev);

[Severity: High]
Could this lead to a Runtime PM usage count underflow on driver unbind?

Earlier in veml6031x00_probe(), the driver acquires a PM runtime reference
using the devres helper:

ret = devm_pm_runtime_get_noresume(dev);

This automatically registers a cleanup action to decrement the usage count upon
driver removal or probe failure. 

However, manually dropping the reference here with pm_runtime_put_autosuspend()
allows the device to autosuspend, but leaves the devres cleanup action queued.
When the driver is unbound (or if devm_iio_device_register() fails below),
the devres cleanup action will execute and decrement the usage counter again.

> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d@gmail.com?part=2