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

[Jonathan Cameron <jic23@kernel.org>]

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

I agree this is worth doing even if we can't trigger it today.

Jonathan

> 
> Best regards,
> Javier