Re: [PATCH v4 4/7] iio: light: ltr501: Power down chip if request irq fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition leading to a Use-After-Free in the IRQ handler if `iio_device_register()` fails during probe.
- [High] Incorrect integer sizes passed to `regmap_bulk_read` and `regmap_bulk_write` cause uninitialized memory reads and incorrect hardware writes on big-endian architectures.
- [Medium] If `ltr501_init()` fails partially while writing the control registers, the ALS sensor is left powered on when probe fails.
--

--- Patch [4]: [PATCH v4 4/7] iio: light: ltr501: Power down chip if request irq fails ---
commit a19bda31a7e1d7988ac494947d945670f4024b2a
Author: Esben Haabendal <esben@geanix.com>

iio: light: ltr501: Power down chip if request irq fails

In case devm_request_threaded_irq() failed, we were leaving the chip
powered instead of powering it down again. Power down the chip switching
ALS and PS modes to standby.

> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index 7d045be78c6df..7746d374688fa 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c
> @@ -1540,7 +1540,7 @@ static int ltr501_probe(struct i2c_client *client)

[Severity: Medium]
This is a pre-existing issue, but does ltr501_probe() leak power if
ltr501_init() fails partially?

If the first I2C write in ltr501_init()->ltr501_write_contr() succeeds but
the second fails, ltr501_init() returns an error with the ALS sensor
powered on:

drivers/iio/light/ltr501.c:ltr501_probe() {
	...
	ret = ltr501_init(data);
	if (ret < 0)
		return ret;
	...
}

This returns directly instead of taking the powerdown_on_error path,
leaving the ALS sensor active.

[Severity: High]
This is also a pre-existing issue, but does using
devm_request_threaded_irq() here create a race condition if
iio_device_register() fails later in probe?

Since the IRQ is managed by devres, it remains active until ltr501_probe()
returns and devres cleans it up.

If iio_device_register() fails, its error path calls
iio_device_unregister_eventset(), which frees the event interface:

drivers/iio/industrialio-event.c:iio_device_unregister_eventset() {
	...
	kfree(ev_int);
	iio_dev_opaque->event_interface = NULL;
	...
}

If a concurrent hardware interrupt fires before devres disables the IRQ,
ltr501_interrupt_handler() will call iio_push_event(), which dereferences
the freed pointer:

drivers/iio/industrialio-event.c:iio_push_event() {
	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;

	if (!ev_int)
		return 0;
	...
}

>  		if (ret) {
>  			dev_err(&client->dev, "request irq (%d) failed\n",
>  				client->irq);
> -			return ret;
> +			goto powerdown_on_error;
>  		}
>  	} else {
>  		indio_dev->info = data->chip_info->info_no_irq;

[Severity: High]
This is another pre-existing issue, but do the regmap_bulk_read() and
regmap_bulk_write() calls in this driver handle endianness correctly?

In functions like ltr501_read_thresh(), ltr501_trigger_handler(), and
ltr501_write_thresh(), 32-bit integer variables are passed by reference
with a 2-byte length:

drivers/iio/light/ltr501.c:ltr501_read_thresh() {
	...
	int thresh_data;
	...
			ret = regmap_bulk_read(data->regmap,
					       LTR501_ALS_THRESH_UP,
					       &thresh_data, 2);
	...
}

On big-endian architectures, this targets the most significant bytes,
leaving the lower bytes uninitialized.

When masked and returned, it yields stack garbage.

During writes, the upper bytes (which are 0) are sent to the sensor,
meaning 0x0000 is always written.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810-liteon-ltr329-v4-0-8bd3dfd15280@geanix.com?part=4