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

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

Forwarded message from Javier Carrasco on Mon May 25, 2026 at 10:59 AM:
On Mon May 25, 2026 at 2:13 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
>
> New issues:
> - [High] Cache pollution of AF_TRIG causes unintended conversions and unnecessary I2C reads.
> - [High] The trigger reference acquired via iio_trigger_get() is leaked on probe error paths and driver unbind.
> - [High] PM runtime usage counter leaks if the driver unbinds while an event is enabled.
> - [High] Sysfs read/write functions and trigger operations access hardware without acquiring a PM runtime reference.
> - [High] Out-of-bounds array write in veml6031x00_trig_handler when the timestamp channel is enabled.
> - [Medium] Dead event sysfs attributes are exposed when no IRQ is present.
>
> Pre-existing issues:
> - [High] Unbalanced double-put of the runtime PM usage counter on driver unbind.
> - [High] The shutdown action accesses I2C registers after PM runtime has been disabled.
> --
>
> commit cbdd69d1454af4788d5d18a23d3fb6854c9981e9
> 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 9968d4414dc96..c57aaa29abf30 100644
>> --- a/drivers/iio/light/veml6031x00.c
>> +++ b/drivers/iio/light/veml6031x00.c
>
> [ ... ]
>
>> @@ -136,6 +189,8 @@ static const struct iio_chan_spec veml6031x00_channels[] = {
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
>>  		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
>>  		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
>> +		.event_spec = veml6031x00_event_spec,
>> +		.num_event_specs = ARRAY_SIZE(veml6031x00_event_spec),
>
> [Severity: Medium]
> Will this expose dead event sysfs attributes when no IRQ is present?
>
> When i2c->irq is 0, the driver registers veml6031x00_info_no_irq which lacks
> event callbacks. However, this channel array unconditionally defines
> event_spec and num_event_specs. The IIO core will parse these and expose
> event sysfs files. Reading or writing these files will fail with -EINVAL
> because the callbacks are NULL, which violates the sysfs API contract.
>
> [ ... ]
>

@Jonathan: is this how it really works? How can I then dynamically
define even_spec to account for it?

>> +static int veml6031x00_read_event_val(struct iio_dev *iio,
>> +				      const struct iio_chan_spec *chan,
>> +				      enum iio_event_type type,
>> +				      enum iio_event_direction dir,
>> +				      enum iio_event_info info,
>> +				      int *val, int *val2)
>> +{
>
> [Severity: High]
> Are PM runtime references needed here before accessing hardware?
>
> This function, as well as veml6031x00_write_event_val() and
> veml6031x00_set_trigger_state(), performs I2C accesses via regmap without
> first calling pm_runtime_resume_and_get(). If the device is
> runtime-suspended and its PM domain has powered it off, will these I2C
> accesses fail or time out?
>
> [ ... ]
>

As I said for [2/4], reading registers in shutdown mode is absolutely
fine.

>> +static int veml6031x00_write_event_config(struct iio_dev *iio,
>> +					  const struct iio_chan_spec *chan,
>> +					  enum iio_event_type type,
>> +					  enum iio_event_direction dir,
>> +					  bool state)
>> +{
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	int ret;
>> +
>> +	guard(mutex)(&data->lock);
>> +
>> +	/* avoid multiple increments/decrements from one source */
>> +	if (state == data->ev_en)
>> +		return 0;
>> +
>> +	if (state) {
>> +		ret = pm_runtime_resume_and_get(data->dev);
>
> [Severity: High]
> Could this leak the PM runtime usage counter if the driver unbinds?
>
> Enabling an event increments the PM runtime usage counter here. If the
> driver is unbound via sysfs while an event is enabled, the IIO core does
> not automatically disable the event. Since there is no unbind cleanup
> mechanism to release the pending PM reference when data->ev_en is true,
> does this keep the device powered on indefinitely?
>
> [ ... ]
>

@Jonathan: Does the IIO core let the event enabled? What is the
approach to clear it then?

>> +/* AF_TRIG is reset by hardware, but the rest of the fields are persistent */
>> +static int veml6031x00_set_af_trig(struct veml6031x00_data *data, bool state)
>> +{
>> +	regcache_drop_region(data->regmap, VEML6031X00_REG_CONF0,
>> +			     VEML6031X00_REG_CONF0);
>> +
>> +	return regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
>> +				  VEML6031X00_CONF0_AF_TRIG,
>> +				  FIELD_PREP(VEML6031X00_CONF0_AF_TRIG, state));
>> +}
>
> [Severity: High]
> Can this cause unintentional active force conversions due to cache pollution?
>
> This sets the self-clearing AF_TRIG bit using regmap_update_bits().
> Because CONF0 is not marked as a volatile register, regmap_update_bits()
> writes AF_TRIG=1 and caches it. Any subsequent read-modify-write on CONF0
> (e.g., toggling the interrupt or changing integration time) will read the
> cached AF_TRIG=1 and write it back to hardware, unintentionally triggering
> an active force conversion and causing hardware state corruption.
>
> [ ... ]
>

I will define the CONF0 register as volatile to account for this.

>>  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++],
>
> [Severity: High]
> Does this loop cause an out-of-bounds array write if the timestamp
> channel is enabled?
>
> The iio_for_each_active_channel() loop does not exclude the soft timestamp
> channel. If the user enables ALS, IR, and TIMESTAMP channels, the loop
> iterates 3 times. However, the scan.chans array is declared as
> __le16 chans[2]. Processing the timestamp channel increments the index i
> to 2 and writes to scan.chans[2], causing an out-of-bounds memory write.
>
> Furthermore, because the timestamp channel's address is 0, this incorrectly
> attempts a bogus I2C read from VEML6031X00_REG_CONF0.
>
> [ ... ]
>

Not sure if this is a real issue, and the address for channel 0 is not
CONF0 because it is defined in iio_chan_spec
veml6031x00_channels.address.

>> +static int veml6031x00_setup_irq(struct i2c_client *i2c, struct iio_dev *iio)
>> +{
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	struct device *dev = data->dev;
>> +	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);
>
> [Severity: High]
> Is the trigger reference acquired here leaked on driver unbind?
>
> Calling iio_trigger_get() increments the trigger device's refcount and the
> driver module's refcount. If the driver fails probe afterwards or is unbound
> later, iio_trigger_put() is never called because there is no devres action
> registered for it and no custom remove callback. Does this permanently leak
> the trigger device reference and prevent the module from being unloaded?
>
> [ ... ]
>

Same here. I believe that only industrialio-trigger.c makes use of
iio_trigger_put().

>> @@ -618,6 +1039,10 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>
> [ ... ]
>
>>  	ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
>
> [Severity: High]
> This is a pre-existing issue, but does this shutdown action access I2C
> registers after PM runtime has been disabled?
>
> This action is registered via devm_add_action_or_reset() before PM runtime
> is enabled. On teardown, devm actions run in reverse, meaning PM runtime is
> disabled before the shutdown action runs. The shutdown action then attempts
> I2C writes while PM runtime is disabled. If the device was suspended, it
> cannot be resumed, potentially causing I2C transfer failures.
>
> [ ... ]
>
>>  	pm_runtime_put_autosuspend(dev);
>
> [Severity: High]
> This is a pre-existing issue, but is there an unbalanced double-put of the
> runtime PM usage counter on driver unbind?
>
> The probe function registers a cleanup action to drop the PM reference via
> devm_pm_runtime_get_noresume(dev). However, this explicit put at the end
> of probe means the devm action will drop the reference a second time on
> unbind, causing a PM usage counter underflow that breaks the PM runtime
> state machine.

As I said for [2/4], there won't be any counter underflow that way.

Best,
Javier