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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Mixing `devm_pm_runtime_get_noresume()` with manual `pm_runtime_put_autosuspend()` causes a PM usage counter underflow.
- [High] `veml6031x00_write_raw()` modifies hardware registers over I2C without acquiring a runtime PM reference.
- [High] The shutdown devres action performs I2C writes without ensuring the device and its parent are resumed.
- [Medium] Missing locks in `veml6031x00_read_raw()` for `IIO_CHAN_INFO_SCALE` and `IIO_CHAN_INFO_INT_TIME` allow torn reads of configuration fields.
- [Medium] Incorrect scale attribute exposure for the IR channel and failure to mark shared hardware attributes.
- [Medium] The driver unconditionally sleeps for the integration time on every raw read, introducing unnecessary latency.
--

commit 02aa1fda2e77e393230904b3d122bdbb99445c9f
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.

This first implementation provides basic functionality (measurement
configuration, raw reads and ID validation) and defines the different
register regions in preparation for extended features in the subsequent
patches of the series.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> new file mode 100644
> index 0000000000000..50979d2392301
> --- /dev/null
> +++ b/drivers/iio/light/veml6031x00.c
> @@ -0,0 +1,657 @@

[ ... ]

> +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),

[Severity: Medium]
Does this code expose hardware gain globally or independently per channel?

If hardware gain (gain and pd_div4) is globally applied to the entire chip,
writing to a separate attribute must not silently alter another channel's
state. Should shared hardware settings be exposed via info_mask_shared_by_all
instead?

Additionally, the scale value returned for the IR channel is computed using a
lux-to-count base which might be physically meaningless for raw IR intensity.

[ ... ]

> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
> +				   int *val)
> +{

[ ... ]

> +	IIO_DEV_ACQUIRE_DIRECT_MODE(iio, claim);
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(data->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	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]
Does this unconditionally sleep for the integration time on every raw read?

Since the device measures continuously when active, this delay is likely only
needed if the device was just resumed from sleep. Forcing this delay on every
read unnecessarily throttles the maximum polling read rate.

Could this be optimized by manually calling pm_runtime_get_sync() and checking
its return value (which returns 1 if already active) to apply the sleep only
when strictly necessary?

[ ... ]

> +static int veml6031x00_read_raw(struct iio_dev *iio,
> +				struct iio_chan_spec const *chan, int *val,
> +				int *val2, long mask)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return veml6031x00_single_read(iio, chan->type, val);
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		return veml6031x00_get_it(data, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		return veml6031x00_get_scale(data, val, val2);

[Severity: Medium]
Could concurrent reads and writes cause torn reads of configuration fields
here?

When handling IIO_CHAN_INFO_SCALE or IIO_CHAN_INFO_INT_TIME, this function
does not acquire the IIO direct mode lock or any other mutex. It performs
multiple non-atomic regmap_field_read() operations (e.g., reading both
gain and pd_div4 in veml6031x00_get_scale()).

If a sysfs read interleaves with a write, the read might fetch a transient,
inconsistent mix of old and new register bits.

[ ... ]

> +static int veml6031x00_write_raw(struct iio_dev *iio,
> +				 struct iio_chan_spec const *chan,
> +				 int val, int val2, long mask)
> +{
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(iio, claim);
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		return veml6031x00_set_it(iio, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		return veml6031x00_set_scale(iio, val, val2);

[Severity: High]
Does this code modify hardware registers over I2C without acquiring a runtime
PM reference?

Unlike veml6031x00_single_read(), this function directly writes to the
sensor's configuration registers without a PM reference. Because the regmap is
not configured with .pm_runtime = true, I2C transactions may be attempted
while the client device (or its parent controller) is runtime suspended, which
can trigger bus transfer errors.

[ ... ]

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

[ ... ]

> +	ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add shutdown action\n");

[Severity: High]
Can this shutdown devres action perform I2C writes while the device is
suspended?

Because this devres action is registered before
devm_pm_runtime_set_active_enabled(), on driver unbind, PM is disabled and the
device state is forced to suspended before the shutdown action executes due to
devres LIFO execution order.

The shutdown action then attempts to perform I2C writes to the shutdown
register. Accessing the I2C bus while the device state is suspended can cause
bus errors or hangs.

[ ... ]

> +	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_hw_init(iio);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_put_autosuspend(dev);

[Severity: High]
Does mixing devm_pm_runtime_get_noresume() with manual
pm_runtime_put_autosuspend() cause a PM usage counter underflow?

The devm_pm_runtime_get_noresume(dev) call increments the PM usage counter
and registers a devres action to decrement it on driver unbind.

However, the explicit pm_runtime_put_autosuspend(dev) decrements the usage
counter to its pre-get state. When the driver is subsequently removed (or if
probe fails after the put), the devres action will blindly decrement the
counter a second time, resulting in an underflow.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-veml6031x00-v3-0-29165609b2b5@gmail.com?part=2