Re: [PATCH v7 4/8] iio: accel: mma8452: Support interrupt sharing

[Esben Haabendal]

"Andy Shevchenko" <andriy.shevchenko@xxxxxxxxx> writes:

> On Mon, Aug 31, 2026 at 02:17:07PM +0200, Esben Haabendal wrote:
>> Adding handling of rutnime PM suspension in the interrupt handler allows
>> sharing interrupt with other devices.
>>
>> Keep in mind that the device by default is using push-pull for the irq pin,
>> which might require additional hardware design to allow interrupt sharing.
>>
>> The suspended flag is added together with synchronize_irq() in order to
>> protect against race conditions when doing runtime suspend and device
>> removal. This way we ensure that interrupt handler does not try to access
>> the device while regulators are disabled.
>
> ...
>
>> 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;
>
>>  	return 0;
>>
>>  runtime_resume_failed:
>> +	WRITE_ONCE(data->suspended, true);
>>  	regulator_disable(data->vddio_reg);
>>  	regulator_disable(data->vdd_reg);
>
> But with this, what's the point in having WRITE_ONCE()? It can be read
> just in the middle as true and be immediately changed afterwards. It
> may be that I am missing something, but I think WRITE_ONCE() should be
> done once in this function.

Yes, there does look like there is still a race condition after adding
this data->suspended flag. An irq handler could just have read
data->suspended, gotten false, and thereafter proceeeded with handling
the irq, and then we write data->suspended=true and the irq handler
would just continue with accessing the device, even though we are now
(if possible) powering down the device.

I did go through all the pre-existing runtime pm and other race
condition issues raised by sashiko-bot during this review, and worked
through it all. The result is a quite a bit larger than what I would
like to add on top of this series. Among other things, it converts the
driver to use regmap for accessing the i2c registers, and after various
fixes the data->suspended flag is removed again.

So I am a bit hesitant to pull all those changes into this series, the
combined series would blow up quite a bit. But if required, I guess I
can do that, although I fear that it will not make reviewing easier to
mix things more than maybe needed.

Could we find a way to merge this series first in some way, or should I
post a new version with all the other fixes added on top?

/Esben