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

[sashiko-bot@kernel.org]

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.

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

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

> +	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?

> +
> +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?

> +		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?

> +		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?

> +	ret = devm_add_action_or_reset(&i2c->dev,
> +				       veml6031x00_disable_event_action, data);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to add event cleanup action\n");
> +
>  	ret = veml6031x00_validate_part_id(data);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-veml6031x00-v5-0-e60876fb3640@gmail.com?part=4