Re: [PATCH v6 4/9] 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] If regulator_disable() fails in mma8452_runtime_suspend(), the data->suspended flag is not rolled back, leading to a permanent IRQ storm on subsequent interrupts.

Pre-existing issues:
- [High] mma8452_remove() does not wake up the device before disabling runtime PM, leading to unbalanced regulator disables (underflow) and illegal I2C access while powered off.
- [High] Registers are not restored after runtime resume, causing the device to lose critical configuration.
--

--- Patch [4]: [PATCH v6 4/9] iio: accel: mma8452: Support interrupt sharing ---
commit 5bc641da9d60875f38db9937a3a1dae1afd9a94e
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 the interrupt 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..8eb97e6793d6f 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]
Is the READ_ONCE(data->suspended) check reachable during normal suspend?

Since pm_runtime_get_if_active() returns 0 when the device state is
RPM_SUSPENDING, it seems the handler will return early before reaching
the data->suspended check.

[ ... ]
> @@ -1774,6 +1789,10 @@ static int mma8452_runtime_suspend(struct device *dev)
>  		return -EAGAIN;
>  	}
>  
> +	WRITE_ONCE(data->suspended, true);
> +
> +	synchronize_irq(client->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, the function returns without resetting
data->suspended to false.

If this happens, the PM core leaves the device in RPM_ACTIVE. Would this
cause a permanent IRQ storm since the next interrupt will see pm_status > 0,
but data->suspended is true, causing it to continually return IRQ_NONE
without clearing the level-triggered interrupt?

[ ... ]
> @@ -1808,6 +1827,8 @@ static int mma8452_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	WRITE_ONCE(data->suspended, false);
> +
>  	ret = mma8452_active(data);
>  	if (ret < 0)
>  		goto runtime_resume_failed;

[Severity: High]
This is a pre-existing issue, but does mma8452_runtime_resume() need to
restore other device registers like MMA8452_CTRL_REG4 and MMA8452_DATA_CFG?

When the regulators are disabled during suspend, the device loses power and
these registers are reset. If they aren't restored here, will the device
lose its configuration and permanently disable hardware interrupts since
CTRL_REG4 defaults to 0?

[ ... ]
> @@ -1822,6 +1843,7 @@ static int mma8452_runtime_resume(struct device *dev)
>  	return 0;
>  
>  runtime_resume_failed:
> +	WRITE_ONCE(data->suspended, true);
>  	regulator_disable(data->vddio_reg);
>  	regulator_disable(data->vdd_reg);
>  

[Severity: High]
This is a pre-existing issue, but if the device is unbound via sysfs while
runtime-suspended, will mma8452_remove() run into issues?

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);
}

Since pm_runtime_disable() leaves the device suspended, won't
mma8452_standby() attempt an I2C write on an unpowered chip? Also, won't
calling regulator_disable() here cause an underflow since the regulators
were already disabled during runtime suspend? Does this need a
pm_runtime_get_sync() before teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=4