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

[Jonathan Cameron]

On Mon, 28 Sep 2026 12:15:57 +0200
Esben Haabendal <esben@xxxxxxxxxx> wrote:

> "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxx> writes:
> 
> > On Mon, Sep 28, 2026 at 10:26:26AM +0200, Esben Haabendal wrote:  
> >> Adding support for sharing interrupt line with other device requires the
> >> interrupt handler to handle runtime PM suspension properly, ignoring the
> >> irq if the device is suspended (maybe even off). And while at it, we use
> >> the PM reference to ensure we do not get suspended while processing an irq.
> >>
> >> In order to prevent the chip from raising irq while suspended (that is when
> >> using fixed regulator, where suspend just means setting the device in
> >> STANDBY mode), we disable all interrupt sources by clearing CTRL_REG4, and
> >> then restores the value again when resuming.
> >>
> >> The scoped_guard in mma8452_runtime_suspend() is changed to a plain
> >> mutex_lock() instead, both to prevent mixing guards and goto, but also to
> >> ensure that we stay in STANDBY mode while writing to CTRL_REG4 and all the
> >> way up to disabling the device as much as possible.
> >>
> >> With that in place, it is safe to add the IRQF_SHARED flag.
> >>
> >> Keep in mind that the device by default is using push-pull for the irq pin,
> >> which might require additional hardware design to allow interrupt sharing.  
> >
> > ...
> >  
> >> +	pm_status = pm_runtime_get_if_active(dev);
> >> +	if (pm_status == 0)
> >> +		/* device is powered down */
> >> +		return IRQ_NONE;  
> >  
> >> +	if (IS_ENABLED(CONFIG_PM) && pm_status < 0)
> >> +		/* runtime PM was disabled, possibly suspending */
> >> +		return IRQ_HANDLED;  
> >
> > This is very interesting part. I bet this will be the first driver using this.
> > A big question "why?"  
> 
> Because as I understood it, I needed to add this for the patch to be
> accepted [1]. It is definitely possible that I have misunderstood that.
> 
> The technical reason I believe is that during system suspend,
> pm_runtime_force_suspend() disabled runtime PM, and if a shared
> interrupt durings this, pm_runtime_get_if_active returns -EINVAL, and we
> could therefore end up doing I2C reads on unpowered hardware.
> 
> Per that reasoning, we probably should be doing this in other drivers as
> well.

I suspect that the combination of a shared interrupt and runtime pm
is moderately rare and even when it is there are drivers doing their
own tracking to avoid this sort of corner case.

There are a lot of these nasty corners in runtime pm :(

This seems like a reasonable solution to me, but given
all the discussion on this version was around this patch I'm going
to pick up 1-9 to but ask you to send a new version of this one to
make sure we get maximum eyes on it.  Please include the PM maintainer
as well in the +CC in case we have this all wrong!

> 
> >> +	/*
> >> +	 * pm_status is now 1 or -EINVAL (with CONFIG_PM not enabled). If
> >> +	 * pm_status==1, runtime PM is enabled and device is RPM_ACTIVE. If
> >> +	 * pm_status==-EINVAL, runtime PM is build-time disabled (i.e. CONFIG_PM
> >> +	 * not enabled), and we can/must assume device is active.
> >> +	 */
> >> +
> >>  	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
> >>  	if (src < 0)
> >> -		return IRQ_NONE;
> >> +		goto out_runtime_put;
> >>
> >>  	if (!(src & (data->chip_info->enabled_events | MMA8452_INT_DRDY)))
> >> -		return IRQ_NONE;
> >> +		goto out_runtime_put;
> >>
> >>  	if (src & MMA8452_INT_DRDY) {
> >>  		iio_trigger_poll_nested(indio_dev->trig);
> >> @@ -1120,6 +1139,10 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
> >>  		ret = IRQ_HANDLED;
> >>  	}
> >>
> >> +out_runtime_put:
> >> +	if (pm_status > 0)
> >> +		pm_runtime_put_autosuspend(dev);
> >> +
> >>  	return ret;
> >>  }  
> >
> > ...
> >  
> >> +	pm_runtime_enable(dev);
> >> +	pm_runtime_set_autosuspend_delay(dev, MMA8452_AUTO_SUSPEND_DELAY_MS);
> >> +	pm_runtime_use_autosuspend(dev);  
> >
> > I don't see the respective _dont_use_autosuspend() call anywhere.  
> 
> Ah. I was not aware it was attached to a usage counter as well. I will
> add a pm_runtime_dont_use_autosuspend() to the runtime_suspend: error handling.
> 
> > ...
> >
> > I am wondering how many of the above possible scenarios you were able
> > to test.  
> 
> Not enough. I haven't done any additional instrumentation to cause all
> the various error conditions that is handled here in _probe().
> 
> > ...
> >
> > P.S.  I bet you can now make a presentation "PM runtime in Linux and
> > why it is so hard."  
> 
> LOL. I am getting there, yes. I have definitely learned a lot about how
> runtime PM works. Especially since I was mostly starting from scratch on
> that.
> 
> But i fear it would easily end up being quite a chaotic presentation :)

:)

I've vaguely be wondering if we need a bunch of self test + examples
to poke all the more crazy corners of runtime pm.  That way we can
at least be sure that if we do finally understand it we aren't going
to get any regressions.

J
> 
> /Esben