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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The trigger handler iterates over all active channels, including the soft timestamp channel, causing an unintended read of the configuration register (0x00) on every trigger.
--

--- Patch [4]: [PATCH v7 4/4] iio: light: veml6031x00: add support for events and trigger ---
commit edb90c2eedbeff107834cc27e54709e4496c1560
Author: Javier Carrasco <[email protected]>

iio: light: veml6031x00: add support for events and trigger

This commit adds support for configurations with and without an interrupt 
line to the VEML6031X00 driver. It enables events and trigger functionality 
when a shared interrupt line is available.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index e6ca98414a01c..068fa7b3ecc98 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c

[ ... ]

> @@ -549,11 +968,83 @@ static int veml6031x00_buffer_postdisable(struct iio_dev *iio)

[ ... ]

> +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);
> +	struct regmap *map = data->regmap;
> +	IIO_DECLARE_BUFFER_WITH_TS(__le16, scan, 2) = { };
> +	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(map, VEML6031X00_REG_ALS_L,
> +				       scan, 2 * sizeof(*scan));
> +		if (ret)
> +			goto done;
> +	} else {
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(map, iio->channels[ch].address,
> +					       &scan[i++], sizeof(*scan));
> +			if (ret)
> +				goto done;
> +		}
                 ^^^^

[Severity: Medium]
Will this loop also iterate over the soft timestamp channel if it is enabled
in the active scan mask?

Since IIO_CHAN_SOFT_TIMESTAMP leaves the .address uninitialized (0), this
could inadvertently issue an I2C read from the configuration register (0x00)
on every trigger event when falling back to this else branch. Should this
loop skip the timestamp channel, for example by checking if the channel
address is valid or if the type is IIO_TIMESTAMP?

> +	}
> +
> +	iio_push_to_buffers_with_ts(iio, scan, sizeof(scan), pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4