Re: [PATCH v7 4/8] iio: accel: mma8452: Support interrupt sharing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing rollback of `data->suspended` on runtime suspend failure permanently disables interrupt handling and breaks device operation.
- [Medium] Unconditional call to `synchronize_irq(client->irq)` may incorrectly synchronize with IRQ 0 if the device operates in polling mode.
--

commit a488fa629f355995847b3d4e006a55add5fc9ce7
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Support interrupt sharing

Adding handling of rutnime PM suspension in the interrupt handler allows
sharing interrupt with other devices.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 1fb43c5b0b72f..936eeb2efb2dd 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1056,14 +1057,24 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>  	struct iio_dev *indio_dev = p;
>  	struct mma8452_data *data = iio_priv(indio_dev);
>  	irqreturn_t ret = IRQ_NONE;
> +	int pm_status;
>  	int src;
>  
> +	pm_status = pm_runtime_get_if_active(&data->client->dev);
> +	if (pm_status == 0)
> +		return IRQ_NONE; /* device is powered down */
> +	if (READ_ONCE(data->suspended)) {
> +		/* device is being removed */
> +		ret = IRQ_NONE;
> +		goto out_runtime_put;
> +	}

[Severity: High]
If mma8452_runtime_suspend() fails and leaves data->suspended set to true,
won't this cause all subsequent interrupts to be dropped here?

[ ... ]

> @@ -1774,6 +1790,10 @@ static int mma8452_runtime_suspend(struct device *dev)
>  		return -EAGAIN;
>  	}
>  
> +	WRITE_ONCE(data->suspended, true);
> +
> +	synchronize_irq(client->irq);

[Severity: Medium]
If the device operates in polling mode and is instantiated without a hardware
interrupt, client->irq will be 0. Calling synchronize_irq(0) on architectures
where IRQ 0 is a valid system interrupt (like the legacy timer on x86) might
needlessly block execution waiting for an unrelated system handler.

Could we check if client->irq is non-zero before calling synchronize_irq()?

> +
>  	ret = regulator_disable(data->vddio_reg);
>  	if (ret) {
>  		dev_err(dev, "failed to disable VDDIO regulator\n");
>  		return ret;
>  	}

[Severity: High]
If regulator_disable() fails here and returns an error, the PM core will abort
the suspend transition leaving the device in the RPM_ACTIVE state. However,
data->suspended is left as true, and the hardware is left in standby.

As mentioned above, since data->suspended remains true, won't the interrupt
handler unconditionally return IRQ_NONE for all future interrupts? This seems
like it would break the device's event handling until the module is reloaded.
Should we revert data->suspended to false and restore the hardware state on
this error path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-mma8452-open-drain-v7-0-22946812c928@geanix.com?part=4