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

[sashiko-bot@kernel.org]

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.

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

> +
> +	/*
> +	 * 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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-mma8452-open-drain-v9-0-0dd24fa2f3d7@geanix.com?part=10