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

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

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.

> - [High] The trigger handler performs an unintended I2C read from address 0 for the software timestamp channel.

Not a real issue.

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

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

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

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

> --
>
> commit c0654b1fbfc516c85d27a7c169844b649c2a1077
> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> 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