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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 21 Sep 2026 07:35:11 +0200
Esben Haabendal <esben@geanix.com> wrote:

> "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?

No need to repeat.  One reply as you say and then add a note either
under the --- cut mark in the commit description or in the
cover letter.


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

We never get consensus on this but I tend to go with IRQ_HANDLED when
we don't know it wasn't ours.

Jonathan

>