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

[Jonathan Cameron]

On Mon, 01 Jun 2026 12:01:10 +0200
"Javier Carrasco" <javier.carrasco.cruz@xxxxxxxxx> wrote:

> My view about the issues Sashiko reported.
> 
> On Sun May 31, 2026 at 10:43 PM CEST, sashiko-bot wrote:
> > Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> >
> > New issues:
> > - [High] Missing `.validate_device` in trigger operations allows other devices to attach to a sensor-specific trigger.  
> 
> Tested with no real issues, but the trigger is mean for this device
> only.

We've always been a bit vague on whether the expectation is that a trigger
can be used by another device when not being used by this one.  It shouldn't
crash, but you may well not get data ;)

> 
> > - [High] PM runtime usage count is leaked if the device is unbound while events are enabled.  
> 
> The usage count is set to 0 by the devres action.

Rather than decrementing?

> 
> > - [High] Stale values in the regmap cache for the hardware-cleared `AF_TRIG` bit cause unintended triggers.  
> 
> Real issue, I forgot to set CONF0 as a volatile register.

If you set it volatile, I assume the dance around marking the cache as
wrong is not needed?

> 
> > - [Medium] Mixing `goto` error handling with `guard()` scope-based cleanup in the same function.  
> 
> Not a real issue because the goto's happen after the guard, but it's easy to
> get rid of them.
Please do.  Those rules are about code fragility rather than just bugs.


> 
> > - [Medium] Non-functional event sysfs attributes are exposed to userspace when the device lacks an IRQ.  
> 
> Missing fix to be added to V5.
> 
> >
> > Pre-existing issues:
> > - [High] Unbalanced PM runtime usage counter due to mixing devm action with manual put.  
> 
> Not a real issue because an underflow is not possible.

True, but still nice to balance if easily done.

> 
> > --
> >
> > commit c0654b1fbfc516c85d27a7c169844b649c2a1077
> > Author: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
> >
> > iio: light: veml6031x00: add support for events and trigger
> >
> > The device provides a shared interrupt line for to notify events and
> > data ready, which can be used as a trigger. The interrupt line is not a
> > requirement for the device to work. Implement variants for the cases
> > whether the interrupt line is provided or not.
> >  
> >> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> >> index facb1b8e42418..5e79eb942f09c 100644
> >> --- a/drivers/iio/light/veml6031x00.c
> >> +++ b/drivers/iio/light/veml6031x00.c  
> >
> > [ ... ]
> >  
> >> @@ -144,6 +197,8 @@ static const struct iio_chan_spec veml6031x00_channels[] = {
> >>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> >>  		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> >>  		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> >> +		.event_spec = veml6031x00_event_spec,
> >> +		.num_event_specs = ARRAY_SIZE(veml6031x00_event_spec),  
> >
> > [Severity: Medium]
> > Will this unconditionally expose event sysfs attributes even when the device
> > lacks an IRQ?
> >
> > If i2c->irq is 0, the driver uses veml6031x00_info_no_irq to omit event
> > callbacks. However, the IIO core checks num_event_specs during
> > iio_check_for_dynamic_events() and creates sysfs attributes anyway. If these
> > attributes are accessed from userspace without the callbacks, will they
> > return -EINVAL?
> >  
> 
> This fix is also missing. I will add it to V5.
> 
> > [ ... ]
> >  
> >> +static int veml6031x00_set_af_trig(struct veml6031x00_data *data, bool state)
> >> +{
> >> +	regcache_drop_region(data->regmap, VEML6031X00_REG_CONF0,
> >> +			     VEML6031X00_REG_CONF0);
> >> +
> >> +	return regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> >> +				  VEML6031X00_CONF0_AF_TRIG,
> >> +				  FIELD_PREP(VEML6031X00_CONF0_AF_TRIG, state));
> >> +}  
> >
> > [Severity: High]
> > Does this leave a stale AF_TRIG value in the non-volatile regmap cache?
> >
> > The AF_TRIG bit is cleared automatically by hardware. While this function
> > drops the cache region before setting the bit, VEML6031X00_REG_CONF0 is not
> > marked as volatile in veml6031x00_volatile_ranges. Because of this,
> > regmap_update_bits() writes the new value to hardware and also caches it.
> >
> > Would any subsequent updates to CONF0 (such as changing AF or SD bits) read
> > the cached AF_TRIG=1 and write it back, causing an unintended measurement
> > trigger?
> >  
> 
> Absolutely right, I forgot to update this although I said I would do it.
> 
> > [ ... ]
> >  
> >> +static int veml6031x00_write_event_config(struct iio_dev *iio,
> >> +					  const struct iio_chan_spec *chan,
> >> +					  enum iio_event_type type,
> >> +					  enum iio_event_direction dir,
> >> +					  bool state)
> >> +{
> >> +	struct veml6031x00_data *data = iio_priv(iio);
> >> +	int ret;
> >> +
> >> +	guard(mutex)(&data->irq_lock);
> >> +
> >> +	/* avoid multiple increments/decrements from one source */
> >> +	if (state == data->ev_en)
> >> +		return 0;
> >> +
> >> +	if (state) {
> >> +		ret = pm_runtime_resume_and_get(data->dev);
> >> +		if (ret)
> >> +			return ret;
> >> +	}  
> >
> > [Severity: High]
> > Is the PM runtime usage count leaked if the device is unbound while events
> > are enabled?
> >
> > The pm_runtime_resume_and_get() call keeps the device powered on, but there
> > does not appear to be a custom devres action or remove callback to disable
> > the event and drop this reference if the driver is unbound. Could this
> > prevent the parent I2C controller from ever entering runtime suspend?
> >  
> 
> The devres action that was registered in the probe function will
> decrement the counter if the driver is unbound. I explained the
> mechanism at the end of the mail.
> 
> > [ ... ]
> >  
> >> +static int veml6031x00_set_trigger_state(struct iio_trigger *trig, bool state)
> >> +{
> >> +	struct iio_dev *iio = iio_trigger_get_drvdata(trig);
> >> +	struct veml6031x00_data *data = iio_priv(iio);
> >> +	int ret;
> >> +
> >> +	guard(mutex)(&data->irq_lock);
> >> +
> >> +	if (state == data->trig_en)
> >> +		return 0;
> >> +
> >> +	ret = veml6031x00_set_interrupt(data, state);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* The AF bit must be set before setting AF_TRIG */
> >> +	ret = regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> >> +				 VEML6031X00_CONF0_AF,
> >> +				 FIELD_PREP(VEML6031X00_CONF0_AF, state));
> >> +	if (ret)
> >> +		goto err_disable_interrupt;
> >> +
> >> +	ret = veml6031x00_set_af_trig(data, state);
> >> +	if (ret)
> >> +		goto err_clear_af;  
> >
> > [Severity: Medium]
> > Is it safe to mix goto-based error handling with guard() scope-based cleanup
> > in this function?
> >
> > According to the cleanup subsystem guidelines, mixing goto with guard() or
> > __free() in the same function can create confusing ownership semantics.
> > Should this function either use goto entirely or be fully converted to
> > scoped cleanups?
> >  
> 
> The real issue arises when there are goto calls before the cleanup, but
> I think it is cleaner to get rid of all goto calls where cleanup magic
> is used. I will remove them and add function calls and error return
> in the error paths.
> 
> > [ ... ]
> >  
> >> +static const struct iio_trigger_ops veml6031x00_trigger_ops = {
> >> +	.set_trigger_state = veml6031x00_set_trigger_state,
> >> +};  
> >
> > [Severity: High]
> > Is a .validate_device callback needed here to prevent other devices from
> > attaching to this sensor-specific trigger?
> >
> > Without setting .validate_device to iio_trigger_validate_own_device, another
> > IIO device could attach to it. If that happens, veml6031x00_set_trigger_state()
> > would enable the DRDY interrupt while the sensor is PM runtime suspended
> > (since its own buffer_preenable isn't called). Would this cause the dependent
> > device's buffer captures to hang?
> >  
> 
> This trigger is intented to be used by its own device. I will add
> iio_trigger_validate_own_device to iio_trigger_ops.
> 
> > [ ... ]
> >  
> >> +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);
> >> +	int ch, ret, i = 0;
> >> +	struct {
> >> +		__le16 chans[2];
> >> +		aligned_s64 timestamp;
> >> +	} scan = { };
> >> +
> >> +	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
> >> +	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {
> >> +		ret = regmap_bulk_read(data->regmap,
> >> +				       VEML6031X00_REG_ALS_L,
> >> +				       &scan.chans, sizeof(scan.chans));
> >> +		if (ret)
> >> +			goto done;
> >> +	} else {
> >> +		iio_for_each_active_channel(iio, ch) {
> >> +			ret = regmap_bulk_read(data->regmap,
> >> +					       iio->channels[ch].address,
> >> +					       &scan.chans[i++],
> >> +					       sizeof(*scan.chans));  
> >
> > [Severity: High]
> > Does this loop inadvertently perform an I2C read from address 0 for the
> > software timestamp channel?
> >
> > When IIO_CHAN_SOFT_TIMESTAMP is active and the fast-path isn't taken, it
> > iterates over all active channels. Since the timestamp channel does not define
> > a hardware address, its .address is 0.
> >
> > Would regmap_bulk_read() read VEML6031X00_REG_CONF0 instead of data,
> > wasting I2C bandwidth and writing the configuration register's value into
> > the scan buffer just before the timestamp?
> >  
> 
> We have mentioned before that Sashiko is wrong about this. Is there any
> way to tell Sashiko why this is ok?
> 
> > [ ... ]
> >  
> >> @@ -677,12 +1102,21 @@ static int veml6031x00_probe(struct i2c_client *i2c)
> >>  	iio->channels = veml6031x00_channels;
> >>  	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
> >>  	iio->modes = INDIO_DIRECT_MODE;
> >>
> >>  	ret = veml6031x00_hw_init(iio);
> >>  	if (ret)
> >>  		return ret;
> >>
> >> +	if (i2c->irq) {
> >> +		ret = veml6031x00_setup_irq(i2c, iio);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		iio->info = &veml6031x00_info;
> >> +	} else {
> >> +		iio->info = &veml6031x00_info_no_irq;
> >> +	}
> >> +
> >>  	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> >>  					      veml6031x00_trig_handler,
> >>  					      &veml6031x00_buffer_setup_ops);  
> >
> > [Severity: High]
> > This isn't a bug introduced by this patch, but is the PM runtime usage
> > counter unbalanced at the end of veml6031x00_probe()?
> >
> > The driver acquires a reference using devm_pm_runtime_get_noresume() which
> > associates a devres cleanup action. If pm_runtime_put_autosuspend() is
> > called manually at the end of probe (immediately after this buffer setup),
> > the devres cleanup will still fire when the device is unbound, performing a
> > second put. Could this cause the PM runtime usage counter to underflow
> > to -1?  
> 
> This cannot happen because the devres action uses pm_runtime_put_noidle(),
> which in turns uses atomic_add_unless(usage_count, -1, 0) to avoid
> decrementing the usage_count if it is already 0.
> 
> Best regards,
> Javier