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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Asynchronous `pm_runtime_put_autosuspend()` is used in a devres cleanup action immediately before `pm_runtime_disable()`, violating PM subsystem guidelines.
- [Medium] Module and trigger device reference leak in the probe error path due to `iio_trigger_get()`.
- [Medium] Unintended I2C read of the configuration register during trigger handler due to iterating over the soft timestamp channel.
--

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

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

This commit adds support for events and data-ready triggers using a shared
interrupt line for the VEML6031X00 light sensor.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index 43a701f62aeaa..6dc7ea7e8a4fa 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c

[ ... ]

> +static void veml6031x00_disable_event_action(void *arg)
> +{
> +	struct veml6031x00_data *data = arg;
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +
> +	guard(mutex)(&data->irq_lock);
> +
> +	if (!data->ev_en)
> +		return;
> +
> +	ret = veml6031x00_set_interrupt(data, false);
> +	if (ret)
> +		dev_err(dev, "Failed to disable events: %d\n", ret);
> +
> +	data->ev_en = false;
> +	pm_runtime_put_autosuspend(dev);

[Severity: Medium]
Will this asynchronous PM put be cancelled during device removal?

Since this function is registered as a devres action via
devm_add_action_or_reset(), it will execute right before
pm_runtime_disable() on the unbind path.

The PM subsystem guidelines indicate that pm_runtime_put_sync() should be
used when pm_runtime_disable() or device removal follows immediately, as
pending async idle/suspend work can be cancelled.

> +}

[ ... ]

> +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) = { };
> +	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++],

[Severity: Medium]
Does this loop perform an unintended I2C read when the soft timestamp
channel is active?

Since IIO_CHAN_SOFT_TIMESTAMP sets the channel address to 0, if the
timestamp channel is in the active scan mask, this loop will call
regmap_bulk_read() with address 0 (VEML6031X00_REG_CONF0).

Should the loop explicitly skip the timestamp channel, or check if the
channel has a valid I2C register address?

> +					       sizeof(*scan));
> +			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;
> +}

[ ... ]

> +static int veml6031x00_setup_irq(struct i2c_client *i2c, struct iio_dev *iio)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +
> +	data->trig = devm_iio_trigger_alloc(dev, "%s-drdy%d",
> +					    iio->name, iio_device_id(iio));
> +	if (!data->trig)
> +		return -ENOMEM;
> +
> +	data->trig->ops = &veml6031x00_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, iio);
> +
> +	ret = devm_iio_trigger_register(dev, data->trig);
> +	if (ret)
> +		return ret;
> +
> +	iio->trig = iio_trigger_get(data->trig);
> +
> +	return devm_request_threaded_irq(dev, i2c->irq,
> +					NULL, veml6031x00_irq,

[Severity: Medium]
Is there a reference leak here if devm_request_threaded_irq() fails?

If the IRQ request fails, the probe will abort. Since
devm_iio_triggered_buffer_setup() has not been called yet, the
INDIO_BUFFER_TRIGGERED flag won't be set in iio->modes.

During cleanup, iio_dev_release() skips
iio_device_unregister_trigger_consumer() if that flag is missing,
permanently leaking the trigger device and module reference acquired by
iio_trigger_get().

> +					IRQF_ONESHOT, iio->name, iio);
> +}

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