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

[Esben Haabendal <esben@geanix.com>]

<sashiko-bot@kernel.org> writes:

> 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.

Ok. I guess I should fix the same issue for error handling of the
devm_request_threaded_irq() call.

> [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