Re: [PATCH v9 10/10] iio: accel: mma8452: Support interrupt sharing

[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:
> - [Critical] iio: accel: mma8452: Interrupt storm livelock when IRQ fires during RPM_SUSPENDING
> - [High] iio: accel: mma8452: Invalid I2C access during system suspend due to mishandled -EINVAL
> - [Medium] iio: accel: mma8452: Race condition in probe error path due to pm_runtime_disable() before free_irq()
> --
>
> commit bf19c63685f6fff116a3a5358196fd66dff6090a
> Author: Esben Haabendal <esben@geanix.com>
>
> iio: accel: mma8452: Support interrupt sharing
>
> This commit adds support for shared interrupts to the mma8452 driver. It
> modifies the interrupt handler to check the runtime PM status and ignores
> the interrupt if the device is suspended. It also updates the runtime
> suspend and resume paths to properly disable and restore interrupt sources.
>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 5aea6bf3312af..2a980c0c2e03a 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>
> [ ... ]
>
>> @@ -1086,15 +1088,28 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>>  {
>>  	struct iio_dev *indio_dev = p;
>>  	struct mma8452_data *data = iio_priv(indio_dev);
>> +	struct device *dev = &data->client->dev;
>>  	irqreturn_t ret = IRQ_NONE;
>> +	int pm_status;
>>  	int src;
>>
>> +	pm_status = pm_runtime_get_if_active(dev);
>> +	if (pm_status == 0)
>> +		return IRQ_NONE; /* device is powered down */
>
> [Severity: Critical]
> If the device asserts its level-triggered interrupt when the PM core is
> transitioning the device to RPM_SUSPENDING (e.g., when the autosuspend
> timer expires), pm_runtime_get_if_active() will return 0.
>
> Does returning IRQ_NONE without clearing the hardware interrupt here cause
> a permanent interrupt storm? Since the IRQ line is shared and unmasked upon
> return, the still-asserted hardware interrupt might immediately re-trigger
> the handler, potentially starving the worker thread that executes
> mma8452_runtime_suspend() and hard-locking the system.

This should be handled by the disable of all interrupt sources (clearing
CTRL_REG4) in mma8452_runtime_suspend().

> [Severity: High]
> During system suspend, pm_runtime_force_suspend() disables runtime PM,
> which increments the disable depth. If a shared interrupt fires during this
> window, pm_runtime_get_if_active() returns -EINVAL.
>
> Since this check only catches pm_status == 0, does this allow the handler
> to proceed and perform I2C reads on unpowered hardware? Per subsystem
> guidelines, should this check for pm_status <= 0 instead?

If we bail out on pm_status <= 0, the driver would not work on systems
with CONFIG_PM not enabled.

In order to handle the difference betwen the different reasons for
-EINVAL, it would seem like we would need some #ifdef CONFIG_PM in here.
Do we really want to do that?

>> +
>> +	/*
>> +	 * pm_status is now 1 or -EINVAL. If pm_status==1, runtime PM is enabled
>> +	 * and device is RPM_ACTIVE. If pm_status==-EINVAL, runtime PM is
>> +	 * disabled (e.g. CONFIG_PM not enabled), and we can/must assume device
>> +	 * is active.
>> +	 */
>> +
>>  	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>
> [ ... ]
>
>> @@ -1771,11 +1790,12 @@ static void mma8452_remove(struct i2c_client *client)
>>
>>  	iio_device_unregister(indio_dev);
>>
>> -	pm_runtime_disable(dev);
>> -	pm_runtime_set_suspended(dev);
>> -
>>  	if (client->irq)
>>  		free_irq(client->irq, indio_dev);
>> +	/* No irq will fire beyond this point */
>> +
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_set_suspended(dev);
>
> [Severity: Medium]
> This correctly fixes the race in mma8452_remove() by moving free_irq()
> before pm_runtime_disable(), but does the same race still exist in the
> mma8452_probe() error path?
>
> If mma8452_probe() fails after pm_runtime_enable(), it jumps to the
> runtime_suspend label:
>
>   mma8452_probe() {
>       /* ... */
>   runtime_suspend:
>       pm_runtime_disable(dev);
>       pm_runtime_set_suspended(dev);
>
>   free_irq:
>       if (client->irq)
>           free_irq(client->irq, indio_dev);
>       /* ... */
>   }
>
> If a shared interrupt fires between pm_runtime_disable() and free_irq(),
> could the handler attempt to access unpowered hardware?

I will apply the same reordering in mma8452_probe().

/Esben