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

<[email protected]> writes:

> --- 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?

If you mean when runtime PM is disabled (CONFIG_PM not set) or disabled
for the specific device, yes, I believe this is handled correctly. We
will correctly assume that the device is not powered down when
pm_runtime_get_if_active() returns an error (-EINVAL).

> 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.

Ah. That does sound like an issue.

> Should this check be <= 0 instead?

No. That would break usage with runtime PM disabled in the normal cases.

We could add a boolean to struct mma8452_data to mark the device as
being removed, and then check on that also.

Or probably better, we could call free_irq() before
pm_runtime_disable(), and cause a spurious irq if/when the race occurs,
but avoid additional code in the irq handler.

>>  	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.

I am not aware of this guideline, and haven't been able to locate it.
If someone could share a pointer to where I can read about this I would
be happy to be enlightened.

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

I am not a runtime PM expert, but that does sounds like a good solution.

I will try to update the patch with fixes for both this and the above
mentioned issue.

>>  		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