Re: [PATCH v8 4/4] iio: light: veml6031x00: add support for events and trigger

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

On Tue Sep 1, 2026 at 10:19 PM CEST, Javier Carrasco wrote:
> On Tue Sep 1, 2026 at 3:22 PM CEST, sashiko-bot wrote:
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>> - [High] Use-After-Free of iio->pollfunc in IRQ handler during device unbind or probe failure.
>> - [Medium] Unintended I2C read of configuration register when pushing timestamp to buffer
>> - [Medium] The interrupt_users counter can permanently underflow during error recovery, breaking hardware interrupt control.
>> --
>>
>> commit c87f3d48e1ee614fc184995b816c8f8679a6b774
>> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>
>> iio: light: veml6031x00: add support for events and trigger
>>
>> The device provides a shared interrupt line to notify events and data ready.
>> Add support for configurations with and without an interrupt line, providing
>> events and trigger support when an interrupt line is available.
>>
>>> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
>>> index 3d5a27a0baaed..7bd7975eb1a76 100644
>>> --- a/drivers/iio/light/veml6031x00.c
>>> +++ b/drivers/iio/light/veml6031x00.c
>> [ ... ]
>>> +static int __veml6031x00_set_trigger_state(struct veml6031x00_data *data, bool state)
>>> +	__must_hold(&data->irq_lock)
>>> +{
>> [ ... ]
>>> +	data->trig_en = state;
>>> +
>>> +	return 0;
>>> +
>>> +clear_interrupt:
>>> +	veml6031x00_set_interrupt(data, !state);
>>> +
>>> +	return ret;
>>> +}
>>
>> [Severity: Medium]
>> Can data->interrupt_users permanently underflow here during error recovery?
>>
>> If an I2C error occurs in __veml6031x00_set_trigger_state() while disabling
>> the trigger, this error recovery path calls veml6031x00_set_interrupt() to
>> revert the counter, but fails to update the software state (data->trig_en).
>>
>> If a subsequent successful disable happens later, the counter will be
>> decremented again, potentially underflowing to UINT_MAX and silently skipping
>> all future hardware writes.
>>
>> [ ... ]
>
> That is not true because the error path will *increment* the counter to
> re-enable the IRQ and keept it consistent with data->trig_en. If an I2C
> operation fails, the trigger will stay enabled in the device registers,
> and therefore data-trig_en should stay enabled.
>
> I tested it by adding some hacks to force that situation, and what may
> actually happen is that if *on top* of that failure, the I2C operation
> in veml6031x00_set_interrupt() also fails, interrupt_users will be
> decremented (as it should, because the interrupt could not be
> re-enabled), and *then* a second call to disable the trigger will set
> interrupt_users to UINT_MAX.
>
> Although this is very unlikely, the fix seems to be trivial:
>
>  if (state) {
>  	data->interrupt_users++;
>  	if (data->interrupt_users > 1)
>  		return 0;
>  } else {
>  	+	if (!data->interrupt_users)
>  	+		return 0;
>
>  	data->interrupt_users--;
>  	if (data->interrupt_users > 0)
>  		return 0;
>  }
>
> Note that this fix is not a hack to just get rid of reaching UINT_MAX, it
> reflects the fact that the previous failed operations lead to a disabled
> interrupt and an enabled trigger in the hardware, and there is no need
> to act on the interrupt again when trying to disable the trigger a
> second time.
>
> I will add it for V9 after testing it with some hacks to force that
> scenario.
>

I am trying to reproduce that scenario with forced errors, and even if
those two errors happen during the first attempt to disable the trigger,
it's not really possible to reach interrupt_users = UINT_MAX.
The 'buffer/enable' attribute is set to 0 by the core even if
set_trigger_state() returns an error code, and the next time
'buffer/enable' is set to 0, nothing really happens because
set_trigger_state() is not even called.

I could still add the check I suggested in V9 and avoid future issues,
but I don't see how to reproduce this in the current state of the core
and driver, even forcing those failed I2C operations.

Best regards,
Javier