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

[Esben Haabendal <esben@geanix.com>]

"Jonathan Cameron" <jic23@kernel.org> writes:

> On Thu, 17 Sep 2026 08:40:06 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> <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()
>> > --
>> >
> So far we haven't enabled sashiko emails to the linux-iio list
> (will probably move to that fairly soon) so fun side effect is this
> reply was shouting into the void - except that b4 picks it up.
>
> +CC linux-iio. I'm too lazy to add everyone by hand who was on original thread.
>
> Key here is looks like you already plan a v10.

Yes, sashiko-bot clearly catches a lot of valid problems. Also flags
some things that is not valid. And for a patch series with many
versions, like this one, these invalid findings keeps getting repeated.

Are there some guidelines how to handle this? Is it enough to write a
reply to the list(s) explaining why the finding is invalid on the first
report by sashiko-bot, or do we have to repeat every time that
sashiko-bot repeats the rebuted finding?

>> > 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?
>
> That if_enabled() function is a pain.  Definitely not ifdef but
> how about
>
>  if (IS_ENABLED(CONFIG_PM) && ret < 0) or something like that?

That should work. Now, in this case, do we want to report IRQ_HANDLED or
IRQ_NONE? We obviosly did not really handle it, but we also don't know
if the irq was for this device.

>> >> +
>> >> +	/*
>> >> +	 * 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().
>
> Sounds good to me

I will send it when I have the above question resolved.

/Esben