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

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

On Fri Aug 7, 2026 at 5:08 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [High] Unbalanced PM runtime usage counter causes an underflow on device unbind or probe error.
> - [High] Uninitialized padding in the local `scan` buffer leaks kernel stack memory to userspace.
> - [High] Spurious interrupts cause I2C reads while the device is runtime suspended.
> - [High] Initialization order and use-after-destroy race due to early IRQ/trigger registration.
> - [High] Triggered buffer is unconditionally registered on the no-IRQ path with a malformed channels array.
> - [Medium] Missing timestamp acquisition for nested trigger handler.
> - [Medium] Trigger disable sequence violates hardware constraints by clearing AF before AF_TRIG.
> --
>
> --- Patch [4]: [PATCH v5 4/4] iio: light: veml6031x00: add support for events and trigger ---
> commit aeccd415dcc79cfa9463cf8b5eec2292ee46dfab
> 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 0cccd40d729eb..e193f87e6740f 100644
>> --- a/drivers/iio/light/veml6031x00.c
>> +++ b/drivers/iio/light/veml6031x00.c
> [ ... ]
>> @@ -494,13 +686,205 @@ static int veml6031x00_write_raw_get_fmt(struct iio_dev *indio_dev,
> [ ... ]
>> +static irqreturn_t veml6031x00_interrupt(int irq, void *private)
>> +{
>> +	struct iio_dev *iio = private;
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	s64 timestamp;
>> +	int regval, ret;
>> +	bool trigger_poll;
>> +
>> +	scoped_guard(mutex, &data->irq_lock) {
>> +		ret = regmap_read(data->regmap, VEML6031X00_REG_INT, &regval);
>
> [Severity: High]
> If a spurious interrupt fires while both ev_en and trig_en are false,
> could this unconditionally read from the I2C bus while the device is
> runtime suspended?
>
> It looks like this might hit a powered-off device since it doesn't
> use pm_runtime_get_if_active() or check software state before
> the register read.
>

The I2C interface stays accessible even while the device is powered down,
so this read doesn't fail or have side effects in case of a spurious
interrupt.

>> +		if (ret) {
>> +			dev_dbg(regmap_get_device(data->regmap),
>> +				"Failed to read interrupt register %d\n", ret);
>> +			return IRQ_NONE;
>> +		}
> [ ... ]
>> @@ -535,11 +919,78 @@ static int veml6031x00_buffer_postdisable(struct iio_dev *iio)
>>  	return 0;
>>  }
>>
>> +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));
>
> [Severity: Medium]
> Since the AF bit must be set before setting AF_TRIG, does the
> disable sequence need to happen in reverse order?
>
> When state is false, this clears the AF bit first, creating a window
> where AF_TRIG is still active but AF is disabled.
>

Disabling AF is what actually turns off auto-force mode, so it should be
done in the current order.

>> +	if (ret) {
>> +		veml6031x00_set_interrupt(data, !state);
>> +
>> +		return ret;
>> +	}
>> +
>> +	ret = regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
>> +				 VEML6031X00_CONF0_AF_TRIG,
>> +				 FIELD_PREP(VEML6031X00_CONF0_AF_TRIG, state));
> [ ... ]
>> +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);
>> +	IIO_DECLARE_BUFFER_WITH_TS(__le16, scan, 2);
>
> [Severity: High]
> Could this leak uninitialized kernel stack memory to userspace?
>
> The IIO_DECLARE_BUFFER_WITH_TS macro allocates the buffer on the stack
> without zeroing the padding bytes between the channel data and the
> timestamp.
>

Already addressed in the reply to Sashiko's feedback about [3/4].

>> +	unsigned int i = 0;
>> +	int ch, ret;
>> +
>> +	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,
>> +				       2 * sizeof(*scan));
>> +		if (ret)
>> +			goto done;
>> +	} else {
>> +		iio_for_each_active_channel(iio, ch) {
>> +			ret = regmap_bulk_read(data->regmap,
>> +					       iio->channels[ch].address,
>> +					       &scan[i++],
>> +					       sizeof(*scan));
>> +			if (ret)
>> +				goto done;
>> +		}
>> +	}
>> +
>> +	iio_push_to_buffers_with_ts(iio, scan, sizeof(scan), pf->timestamp);
>
> [Severity: Medium]
> Since this trigger handler is executed synchronously from the interrupt
> handler via iio_trigger_poll_nested() bypassing the top-half
> iio_pollfunc_store_time(), will pf->timestamp always be zero here?
>

The timestamp is already captured in the interrupt handler. I will assign
it to pollfunc->timestamp before calling iio_trigger_poll_nested().

>> +
>> +done:
>> +	iio_trigger_notify_done(iio->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
> [ ... ]
>> @@ -636,15 +1142,29 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>>  				     "Failed to set regmap\n");
>>
>>  	iio->name = data->chip->name;
>> -	iio->channels = veml6031x00_channels;
>> -	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
>>  	iio->modes = INDIO_DIRECT_MODE;
>> -	iio->info = &veml6031x00_info;
>> +	if (i2c->irq) {
>> +		iio->channels = veml6031x00_channels_irq;
>> +		iio->num_channels = ARRAY_SIZE(veml6031x00_channels_irq);
>> +		ret = veml6031x00_setup_irq(i2c, iio);
>
> [Severity: High]
> Does registering the IRQ and trigger here expose a race condition?
>
> veml6031x00_setup_irq() requests the threaded IRQ before devm_mutex_init()
> allocates data->irq_lock and veml6031x00_regfield_init() allocates
> data->rf.int_en. If an interrupt fires immediately, the handler will try
> to lock an uninitialized mutex.
>
> Additionally, since devres cleans up in reverse order, won't the mutex
> be destroyed before the IRQ is unregistered during unbind?
>

For V6: mutex_init and regfield_init moved before setup_irq().

>> +		if (ret)
>> +			return ret;
>> +
>> +		iio->info = &veml6031x00_info;
>> +	} else {
>> +		iio->channels = veml6031x00_channels;
>
> [Severity: High]
> If the device does not provide an interrupt, it falls back to
> veml6031x00_channels which lacks scan_index and scan_type definitions.
>
> Will setting up a triggered buffer unconditionally at the end of probe
> cause IIO core undefined behavior if a user attaches an external
> sysfs/hrtimer trigger, since both channels would default to a
> scan_index of 0 and 0 storage bits?
>

Already addressed in the reply to Sashiko's feedback about [3/4].

>> +		iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
>> +		iio->info = &veml6031x00_info_no_irq;
>> +	}
>>
>>  	ret = devm_mutex_init(&i2c->dev, &data->scale_lock);
>>  	if (ret)
>>  		return ret;
>>
>> +	ret = devm_mutex_init(&i2c->dev, &data->irq_lock);
>> +	if (ret)
>> +		return ret;
>> +
>>  	ret = veml6031x00_regfield_init(data);
>>  	if (ret)
>>  		return dev_err_probe(&i2c->dev, ret, "Failed to init regfield\n");
>> @@ -672,6 +1192,12 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>>  	if (ret)
>>  		return dev_err_probe(&i2c->dev, ret, "Failed to get runtime PM\n");
>>
>
> [Severity: High]
> Could the devm_pm_runtime_get_noresume() call just above this hunk cause
> a usage counter underflow?
>
> The devres action will eventually call pm_runtime_put_noidle() during
> driver unbind. However, the driver also manually calls
> pm_runtime_put_autosuspend() at the end of veml6031x00_probe() to drop
> the initial reference.
>
> This double-put could underflow the usage counter to -1 and corrupt
> device power management. If the driver manually manages the initial
> reference, should it use the non-devm pm_runtime_get_noresume() variant?
>

Already addressed in the reply to Sashiko's feedback about [3/4].


Best regards,
Javier