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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The interrupt handler incorrectly checks `pm_status == 0` instead of `<= 0`, leading to unpowered hardware access when Runtime PM is disabled.
- [Medium] The runtime suspend callback lacks a call to synchronize_irq() before powering down the hardware, violating PM subsystem guidelines for devices using shared IRQs.
--

--- Patch [4]: [PATCH v4 4/6] iio: accel: mma8452: Support interrupt sharing ---
commit ae80b7f558736b5e80920be8e31037e5cc8645ba
Author: Esben Haabendal <[email protected]>

iio: accel: mma8452: Support interrupt sharing

This commit adds handling of runtime PM suspension in the interrupt handler.
It also adds the IRQF_SHARED flag to the interrupt request to allow sharing
the interrupt with other devices.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 1fb43c5b0b72f..106cdd904dbfd 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1056,14 +1056,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)
> +		return IRQ_NONE; /* device is powered down */
> +

[Severity: High]
Does this correctly handle the case where runtime PM is disabled?

When pm_runtime_disable() is called during device removal,
pm_runtime_get_if_active() can return -EINVAL.

If a shared interrupt fires from another device during this window, the
handler will see a negative return value. Since the check is explicitly
for == 0, the handler will proceed and attempt to read from the I2C bus while
the device regulators might be disabled.

Should this check be <= 0 instead?

>  	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>  	if (src < 0)
> -		return IRQ_NONE;
> +		goto out;

[ ... ]

> @@ -1690,7 +1699,7 @@ static int mma8452_probe(struct i2c_client *client)
>  			dev_info(dev, "invalid irq type, setting default active low\n");
>  			irq_flags = IRQF_TRIGGER_LOW;
>  		}
> -		irq_flags |= IRQF_ONESHOT;
> +		irq_flags |= IRQF_ONESHOT | IRQF_SHARED;

[Severity: Medium]
With the addition of IRQF_SHARED, is synchronize_irq() needed in the runtime
suspend path?

The PM subsystem guidelines require that drivers using shared interrupts must
call synchronize_irq() in their runtime suspend callback before powering down
hardware. This guarantees no interrupt handler is executing mid-flight when
the regulators are disabled.

Would it be safer to add this to mma8452_runtime_suspend() before the device
is placed in standby?

>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
>  					   irq_flags, client->name, indio_dev);
>  		if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4