Re: [PATCH RESEND v5 4/6] 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:

New issues:
- [High] Failed runtime suspend leaves the `suspended` flag set to `true`, causing an unrecoverable PM usage counter leak and a potential interrupt storm.
- [Low] Compilation Error: undeclared variable `client` used in `mma8452_runtime_suspend`.

Pre-existing issues:
- [High] Unbalanced `regulator_disable()` calls and I2C writes to a powered-off device in `mma8452_remove()`.
--

--- Patch [4]: [PATCH RESEND v5 4/6] iio: accel: mma8452: Support interrupt sharing ---
commit 79ad9089167d804daaa97086d058cd3a0e2f4199
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Support interrupt sharing

This commit adds handling of runtime PM suspension in the interrupt handler
to allow sharing interrupts with other devices. It introduces a suspended flag
and uses synchronize_irq() to protect against race conditions during runtime
suspend and device removal.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 1fb43c5b0b72f..25bb2dd69381e 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
[ ... ]
> @@ -1056,14 +1057,19 @@ 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 || READ_ONCE(data->suspended))
> +		return IRQ_NONE; /* device is powered down or being removed */
> +

[Severity: High]
If mma8452_runtime_suspend() in drivers/iio/accel/mma8452.c fails and
leaves data->suspended set to true, can this early return cause a PM
usage counter leak and an interrupt storm?

If the device is kept in the RPM_ACTIVE state after a suspend failure,
pm_runtime_get_if_active() will return 1. Because data->suspended is true,
the handler will return IRQ_NONE immediately.

This skips pm_runtime_put_autosuspend(), leaking the usage counter. Also,
by returning IRQ_NONE on a shared level-triggered IRQ without clearing the
hardware interrupt, wouldn't this trigger an interrupt storm?

[ ... ]
> @@ -1768,12 +1778,15 @@ static int mma8452_runtime_suspend(struct device *dev)
>  
>  	mutex_lock(&data->lock);
>  	ret = mma8452_standby(data);
> +	WRITE_ONCE(data->suspended, true);
>  	mutex_unlock(&data->lock);
>  	if (ret < 0) {
>  		dev_err(&data->client->dev, "powering off device failed\n");
>  		return -EAGAIN;
>  	}

[Severity: High]
If mma8452_standby() fails, should data->suspended be reverted to false?

When returning -EAGAIN on failure in drivers/iio/accel/mma8452.c,
leaving data->suspended set to true creates the inconsistent state that
breaks mma8452_interrupt() as noted above.

>  
> +	synchronize_irq(client->irq);
> +

[Severity: Low]
Does this code compile?

There is no local variable named client in mma8452_runtime_suspend() in
drivers/iio/accel/mma8452.c. The I2C client is available via
data->client or to_i2c_client(dev), so using client->irq directly
appears to cause a compilation failure.

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem during
device removal in mma8452_remove() in drivers/iio/accel/mma8452.c:

drivers/iio/accel/mma8452.c:mma8452_remove() {
    ...
	pm_runtime_disable(&client->dev);
	pm_runtime_set_suspended(&client->dev);
    ...
	mma8452_standby(iio_priv(indio_dev));

	regulator_disable(data->vddio_reg);
	regulator_disable(data->vdd_reg);
}

If the device is already in the RPM_SUSPENDED state when mma8452_remove() is
called, its regulators have already been disabled by
mma8452_runtime_suspend().

Because pm_runtime_disable() does not resume the device, calling
regulator_disable() again will underflow the regulator enable count. Could
this forcefully disable shared regulators and cause system instability?

Additionally, mma8452_standby() performs an I2C write. Is it safe to
unconditionally access the hardware when it might be powered off?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-mma8452-open-drain-v5-0-d8aa590d7c36@geanix.com?part=4