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

["Javier Carrasco" <javier.carrasco.cruz@gmail.com>]

On Mon May 25, 2026 at 12:53 AM CEST, sashiko-bot wrote:
> 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.
> --

...

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

The registers are always accessible as long as the device is powered,
both in normal operation and power down. Therefore, this function cannot
be compared to veml6031x00_single_read() that acquires a PM reference to
carry out a measurement, and the write operations work fine.

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

I believe Sashiko is misinterpreting how this pattern works (not
introduced by me, it is already available upstream in
drivers/spi/atmel-quadspi.c). Decrementing the usage counter is fine
because the devres action uses pm_runtime_put_noidle(), which in turns
uses atomic_add_unless(usage_count, -1, 0) to avoid decrementing the
usage_count if it is already 0. Sashiko has reported the same issue in
other patches of this series.

Best,
Javier