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

On Fri Aug 7, 2026 at 4:49 PM CEST, sashiko-bot wrote:
> 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 <[email protected]>
>
> 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?
>

I will reorder the lock acquisition order, always acquiring direct mode
first and then scale_lock.

>>  	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?
>

I will fix it in V6 with = { } on the declaration.

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

The problem is that the no_irq channel definition is missing the scan
attributes. I will add them for V6.

>>  	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);

I yield. This warning is going to show up a million times during the
review, and probably every time a change is applied to this driver. I
will use the simple pm_runtime_get_noresume() and add the
pm_runtime_put_noidle() in the error paths after it in the probe.

Best regards,
Javier