Re: [PATCH 3/3] iio: light: ltr501: Add ltr329 driver support

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Probe error paths skip device power-down, leaving the sensor active and leaking power.

Pre-existing issues:
- [High] Asymmetric teardown of the devm-managed IRQ and manual IIO device unregistration leads to a Use-After-Free on the IIO event interface.
- [High] Enabling the software timestamp channel on sensors without proximity support causes the trigger handler to hang and drop all captured data.
--

--- Patch [3]: [PATCH 3/3] iio: light: ltr501: Add ltr329 driver support ---
commit 6c505591ba68d4c8fcdd0eb98eb4532e6610696f
Author: Esben Haabendal <esben@geanix.com>

iio: light: ltr501: Add ltr329 driver support
    
This adds support for the LTR-329ALS-01 chip, which is similar to
LTR-303ALS-01, except for interrupt, which LTR-329ALS-01 chip does not
have.

> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index 7d045be78c6df..815b67a10837d 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c

[ ... ]

> @@ -1257,6 +1269,18 @@ static const struct ltr501_chip_info ltr501_chip_info_tbl[] = {
>  		.channels = ltr301_channels,
>  		.no_channels = ARRAY_SIZE(ltr301_channels),
>  	},
> +	[ltr329] = {
> +		.partid = 0x0A,
> +		.als_gain = ltr559_als_gain_tbl,
> +		.als_gain_tbl_size = ARRAY_SIZE(ltr559_als_gain_tbl),
> +		.als_mode_active = BIT(0),
> +		.als_gain_mask = BIT(2) | BIT(3) | BIT(4),
> +		.als_gain_shift = 2,
> +		.info = &ltr301_info_no_irq,
> +		.info_no_irq = &ltr301_info_no_irq,
> +		.channels = ltr301_channels,

[Severity: High]
This is a pre-existing issue, but does using ltr301_channels cause the
trigger handler to hang if the timestamp channel is enabled?

Looking at ltr301_channels, the timestamp channel occupies scan_index 2. If a
user enables the timestamp channel, bit 2 is set in active_scan_mask.

However, ltr501_trigger_handler() assumes bit 2 is always the proximity
sensor:

ltr501_trigger_handler() {
        ...
        if (test_bit(2, indio_dev->active_scan_mask))
                mask |= LTR501_STATUS_PS_RDY;

        ret = ltr501_drdy(data, mask);
        ...
}

Since sensors using ltr301_channels lack a proximity sensor, the hardware
ready bit is never set. Could this cause ltr501_drdy() to loop and drop
data?

> +		.no_channels = ARRAY_SIZE(ltr301_channels),
> +	},
>  };

[ ... ]

> @@ -1531,6 +1555,11 @@ static int ltr501_probe(struct i2c_client *client)
>  		return ret;
>  
>  	if (client->irq > 0) {
> +		if (!ltr501_has_irq_support(data->chip_info)) {
> +			dev_err(&client->dev, "chip does not support irq\n");
> +			return -EINVAL;
> +		}

[Severity: Medium]
Does this early return bypass device power-down on probe failure?

If the device tree specifies an IRQ for a chip that does not support it,
this will return -EINVAL directly instead of jumping to the
powerdown_on_error label, which could leave the device powered on.

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
risk when the threaded IRQ is requested here?

The driver uses devm_request_threaded_irq() during probe but manually calls
iio_device_unregister() in ltr501_remove().

Because devm-managed resources are freed after ltr501_remove() completes,
the interrupt remains active while iio_device_unregister() destroys the
event interface.

If an interrupt fires during this window, ltr501_interrupt_handler() calls
iio_push_event(), which accesses the freed event interface structure.

> +
>  		ret = devm_request_threaded_irq(&client->dev, client->irq,
>  						NULL, ltr501_interrupt_handler,
>  						IRQF_TRIGGER_FALLING |

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-liteon-ltr329-v1-0-31f027051594@geanix.com?part=3