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

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

On Fri Aug 7, 2026 at 4:19 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Mixing `devm_pm_runtime_get_noresume()` with a manual `pm_runtime_put_autosuspend()` in `probe` causes a PM usage counter underflow on device unbind or probe failure.
> - [High] An unprotected read-modify-write in `veml6031x00_set_power_state()` races with sysfs configuration changes, causing lost user configuration, and a lock inversion prevents a simple fix.
> --
>
> --- Patch [2]: [PATCH v5 2/4] iio: light: add support for veml6031x00 ALS series ---
> commit 2cb7c39d884f4873be61de5deba3865dbd24f394
> 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..30cccda51ca08
>> --- /dev/null
>> +++ b/drivers/iio/light/veml6031x00.c
>
> [ ... ]
>
>> +static int veml6031x00_set_power_state(struct veml6031x00_data *data, bool on)
>> +{
>> +	u8 conf[2];
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(data->regmap, VEML6031X00_REG_CONF0,
>> +			       conf, sizeof(conf));
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (on) {
>> +		conf[0] &= ~VEML6031X00_CONF0_SD;
>> +		conf[1] &= ~VEML6031X00_CONF1_IR_SD;
>> +	} else {
>> +		conf[0] |= VEML6031X00_CONF0_SD;
>> +		conf[1] |= VEML6031X00_CONF1_IR_SD;
>> +	}
>> +
>> +	return regmap_bulk_write(data->regmap, VEML6031X00_REG_CONF0,
>> +				 conf, sizeof(conf));
>> +}
>
> [Severity: High]
> Does this unprotected read-modify-write race with concurrent sysfs configuration
> updates?
>
> Because CONF0 and CONF1 are in the volatile ranges, regmap does not protect
> them with its cache lock. Concurrent regmap_field_write() calls triggered by
> veml6031x00_write_raw() could change the gain or integration time between the
> bulk_read and bulk_write here, causing the user configuration to be silently
> overwritten and lost.
>
> Additionally, could acquiring the scale_lock here to fix the race cause a
> deadlock?
>
> Looking at veml6031x00_single_read(), it holds the scale_lock before acquiring
> the PM runtime reference:
>
> veml6031x00_single_read()
>     guard(mutex)(&data->scale_lock);
>     ...
>     PM_RUNTIME_ACQUIRE_AUTOSUSPEND(...)
>
> If the autosuspend acquisition triggers veml6031x00_runtime_resume() which
> then calls veml6031x00_set_power_state(), attempting to take scale_lock again
> would deadlock.
>
> [ ... ]
>

This issue can be avoided by using regmap_update_bits() instead, which
actually was used in previous versions. I modified it to test the
difference after I get confirmation from the manufacturer that the
registers must be updated one after the other, not at the same time.
I will get back to regmap_update_bits().

>> +static int veml6031x00_probe(struct i2c_client *i2c)
>> +{
>
> [ ... ]
>
>> +	ret = devm_pm_runtime_set_active_enabled(&i2c->dev);
>> +	if (ret)
>> +		return dev_err_probe(&i2c->dev, ret, "Failed to enable runtime PM\n");
>> +
>> +	ret = devm_pm_runtime_get_noresume(&i2c->dev);
>> +	if (ret)
>> +		return dev_err_probe(&i2c->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(&i2c->dev);
>
> [Severity: High]
> Will mixing devm_pm_runtime_get_noresume() with a manual
> pm_runtime_put_autosuspend() here cause a PM usage counter underflow?
>
> The devm_pm_runtime_get_noresume() call increments the usage counter and
> registers a devres action to decrement it on unbind or probe failure.
>
> However, the manual pm_runtime_put_autosuspend() here drops the counter back
> to zero.
>
> When the driver is subsequently unbound, the devres action will execute and
> decrement the counter again to -1, which could corrupt the runtime PM state
> machine and prevent the device or its parent from suspending.
>

This was discussed in V4, and it is not a real issue because this
configuration can never reach to decrement the counter to -1 (long story
short: atomic_add_unless(&dev->power.usage_count, -1, 0)). It was also
validated by loading and unloading the driver under different
circumstances.


Best regards,
Javier