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

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 <[email protected]>

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.

[ ... ]

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2