Re: [PATCH RESEND v5 4/6] iio: accel: mma8452: Support interrupt sharing

<[email protected]> writes:

> 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 <[email protected]>
>
> 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?

Yes, I make sure to call pm_runtime_put_autosuspend() when
pm_runtime_get_active() returns 0 and suspended flag is set.

Also, I guess we should not set the suspended flag until after checking
the return value from mma8452_standby() in mma8452_runtime_suspend().

In case one of the regulator_disable() calls fails, the suspended flag
will be left set to true. But as the device is in standby, no interrupt
storm should happen.

> [ ... ]
>> @@ -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.

I will move the WRITE_ONCE() to after checking the ret value, so we
don't leave suspended set to true in this case.

>> +	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/[email protected]?part=4