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

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

On Thu Sep 10, 2026 at 4:56 AM CEST, Jonathan Cameron wrote:
> On Mon, 07 Sep 2026 19:11:34 +0200
> "Javier Carrasco" <javier.carrasco.cruz@gmail.com> wrote:
>
>> On Mon Sep 7, 2026 at 3:58 AM CEST, Jonathan Cameron wrote:
>> > On Wed, 02 Sep 2026 16:58:09 +0200
>> > "Javier Carrasco" <javier.carrasco.cruz@gmail.com> wrote:
>> >
>> >> 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 can't remember why we do that. Oh well. Seems it is useful here.
>> >
>>
>> I instrumented some more code, including iio_trigger_attach/detach_poll_func()
>> and current_trigger_store(). When set_trigger_state(false) fails
>> (which I forced), iio_trigger_detach_poll_func() returns without releasing
>> the pollfunc IRQ from the trigger pool, but iio_disable_buffers()
>> continues disabling the buffer.
>>
>> The next buffer enable adds another consumer to the pool without calling
>> set_trigger_state(true), since the trigger is already in use. The following
>> buffer disable removes that new consumer without calling set_trigger_state(false),
>> since there is still one consumer left in the pool.
>
> So do you get two hits on each trigger? Definitely bad as they can be in threads
> running at the same time and we assume that doesn't happen in most drivers.
>
>>
>> So after the first failed disable, subsequent buffer enable/disable cycles do
>> not call set_trigger_state() and the original consumer remains in the pool.
>> That leaves the buffer/enable attribute in a meaningless state: it gets
>> updated from the userspace perspective, but nothing really happens behind
>> the scenes.
>>
>> Is that intended? Maybe there are historical reasons for it, or maybe there
>> was no cleaner way to operate after an error in set_trigger_state(), or
>> maybe this should not work like that.
>
> I'd go with "probably not the intended result" as if it were I 'think'
> I'd have left a comment.
>
> That's not to say I have an immediate answer to how to resolve it.
> If you still have your test set up, can you try just carrying on and
> not returning after the set_trigger_state(trig, false) fails - making
> sure that your injected failure leaves the trigger actually enabled
> and still firing.  That might leave us in a recoverable state as
> next set_triggers_state(trig, true) should work as normal. In meantime
> we will have a bunch of irqs firing but resulting in nothing happening.
>
> Not great and may well leave some drivers stuck because they are
> tightly couple trigger / data readback and need that to happen to
> not get wedged.  Those should have the validate callbacks set but
> will still be stuck.  In theory we could make all triggers that
> have this property do a clear in the reenable but that is a bit
> nasty as we would probably want it conditional so each driver would
> need a 'have I already cleared this' flag set in their buffer
> read and cleared in the reenable.
>
> Thanks for digging into this btw!
>
> Jonathan
>

I still have my test set up, and I will get back to this once I am done
with the driver that triggered the issue. It looks like some action
should be taken if set_trigger_state() fails, but I need to invest more
time on this.

Best regards,
Javier