Re: [PATCH v3 3/4] iio: light: veml6031x00: add support for triggered buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The trigger handler does not skip the soft timestamp channel, leading to an out-of-bounds array write and an invalid I2C read.
- [Medium] Runtime PM usage counter underflow on device unbind due to mixing devm_pm_runtime_get_noresume() with a manual pm_runtime_put_autosuspend().
--

commit 7b07348868ecc638382ae3a2ca8a3bea1a0c8680
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

iio: light: veml6031x00: add support for triggered buffers

Add triggered buffer functionality for the two channels the device
provides (ALS and IR).

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index 50979d2392301..9968d4414dc96 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c
[ ... ]
> @@ -461,6 +484,82 @@ static const struct iio_info veml6031x00_info = {
>  	.write_raw_get_fmt = veml6031x00_write_raw_get_fmt,
>  };
[ ... ]
> +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 (*iio->active_scan_mask == (BIT(VEML6031X00_SCAN_ALS) |
> +				       BIT(VEML6031X00_SCAN_IR))) {
> +		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 properly handle the timestamp channel?

If the user enables the timestamp channel alongside ALS and IR, the timestamp
bit is included in iio->active_scan_mask. The optimization check above will
evaluate to false, falling back to this else block.

Inside this loop, the timestamp channel isn't skipped. Since the soft
timestamp channel has no assigned address, does it default to 0 and
cause an unintended I2C read of the configuration register?

Additionally, scan.chans is defined as an array of 2 elements. Could this
cause an out-of-bounds write to scan.chans[2] as i increments, corrupting
the struct padding or timestamp field?

> +			if (ret)
> +				goto done;
> +		}
> +	}
> +
> +	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan), pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> @@ -564,6 +663,13 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> +					      veml6031x00_trig_handler,
> +					      &veml6031x00_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register triggered buffer\n");
> +
>  	pm_runtime_put_autosuspend(dev);

[Severity: Medium]
Can this manual pm_runtime_put_autosuspend() cause a usage counter underflow
on device unbind?

The driver acquires a runtime PM reference earlier using
devm_pm_runtime_get_noresume(), which registers a devres cleanup action to
decrement the usage counter on unbind.

With this manual put in probe, will the devres unbind action drop the
counter below zero and trigger a warning in the PM core?

>  
>  	ret = devm_iio_device_register(dev, iio);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-veml6031x00-v3-0-29165609b2b5@gmail.com?part=3