Re: [PATCH v5 3/4] iio: light: veml6031x00: add support for triggered buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] AB-BA deadlock between scale_lock and IIO direct mode lock
- [High] Missing IIO buffer channel configuration causes probe failure
- [High] Uninitialized kernel stack data leaked to userspace in IIO triggered buffer
- [High] Unbalanced pm_runtime put leads to negative usage count
--

--- Patch [3]: [PATCH v5 3/4] iio: light: veml6031x00: add support for triggered buffers ---
commit a5266734ac0d8d96ec4ee20e5c8d6088863dee05
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

iio: light: veml6031x00: add support for triggered buffers

Add triggered buffer functionality for the two channels the device
provides (ambient light sensor and IR).

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index 30cccda51ca08..0cccd40d729eb 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c
[ ... ]
> @@ -392,6 +401,10 @@ static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
>  
>  	guard(mutex)(&data->scale_lock);
>  
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(iio, claim);
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +

[Severity: High]
Does this establish a lock order of scale_lock followed by the IIO direct
mode lock?

[ ... ]
> @@ -453,6 +466,10 @@ 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;
> +

[Severity: High]
Can this cause a deadlock?

In veml6031x00_single_read(), data->scale_lock is acquired before the IIO
direct mode lock.

However, here the IIO direct mode lock is acquired first. If this then calls
veml6031x00_set_it(), which acquires data->scale_lock:

veml6031x00_write_raw()
    IIO_DEV_ACQUIRE_DIRECT_MODE(iio, claim)
    veml6031x00_set_it()
        guard(mutex)(&data->scale_lock)

Could concurrent sysfs access to raw data read and integration time write
lead to a deadlock?

>  	switch (mask) {
>  	case IIO_CHAN_INFO_INT_TIME:
>  		return veml6031x00_set_it(iio, val, val2);
[ ... ]
> @@ -484,6 +501,81 @@ static const struct iio_info veml6031x00_info = {
[ ... ]
> +static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	IIO_DECLARE_BUFFER_WITH_TS(__le16, scan, 2);

[Severity: High]
Does this leak uninitialized stack memory to userspace?

The scan buffer is declared without zero-initialization.

When reading only a single active channel (e.g., just the ambient light
sensor), the driver only writes 2 bytes into the array, leaving the
unpopulated channel slots and structure padding uninitialized.

Will iio_push_to_buffers_with_ts() push this uninitialized kernel stack
memory to the ring buffer? Should the scan structure be zero-initialized?

> +	unsigned int i = 0;
> +	int ch, ret;
[ ... ]
> @@ -588,6 +680,14 @@ static int veml6031x00_probe(struct i2c_client *i2c)
[ ... ]
> +	ret = devm_iio_triggered_buffer_setup(regmap_get_device(data->regmap), iio,
> +					      iio_pollfunc_store_time,
> +					      veml6031x00_trig_handler,
> +					      &veml6031x00_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to register triggered buffer\n");
> +

[Severity: High]
Will this probe fail with -EINVAL?

The triggered buffer is registered here, but it appears the driver's
channels array (veml6031x00_channels) was not updated with .scan_index,
.scan_type, or a timestamp channel.

Without .scan_index, the IIO core defaults both channels to index 0.

Does iio_device_register() abort with -EINVAL during probe when
iio_check_unique_scan_index() detects the duplicate scan index 0?

>  	pm_runtime_put_autosuspend(&i2c->dev);

[Severity: High]
Is this pm_runtime_put_autosuspend() call unbalanced?

The probe function appears to use devm_pm_runtime_get_noresume() which
automatically queues a put on unbind.

If we explicitly decrement the usage counter here, will the devres action
firing on unbind drop the counter below zero and trigger a warning?

Should this use the non-devm pm_runtime_get_noresume() instead if the
driver intends to manually drop the reference later?

>  
>  	ret = devm_iio_device_register(&i2c->dev, iio);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-veml6031x00-v5-0-e60876fb3640@gmail.com?part=3