Re: [PATCH v6 7/9] iio: accel: mma8452: Drop unneeded lock acquire on read

"Joshua Crofts" <[email protected]> writes:

> On Tue, 25 Aug 2026 15:35:17 +0200
> Esben Haabendal <[email protected]> wrote:
>
>> "Joshua Crofts" <[email protected]> writes:
>>
>> > On Tue, 25 Aug 2026 10:27:45 +0200
>> > Esben Haabendal <[email protected]> wrote:
>> >
>> >> There is no need to acquire data->lock when calling mma8452_read(), and
>> >> dropping that makes it less likely to end up in an AB-BA deadlock
>> >> situation.
>> >>
>> >> Signed-off-by: Esben Haabendal <[email protected]>
>> >> ---
>> >>  drivers/iio/accel/mma8452.c | 2 --
>> >>  1 file changed, 2 deletions(-)
>> >>
>> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> >> index 7ef1a9a91c31..9ae2c3e60576 100644
>> >> --- a/drivers/iio/accel/mma8452.c
>> >> +++ b/drivers/iio/accel/mma8452.c
>> >> @@ -504,9 +504,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>> >>  		if (!iio_device_claim_direct(indio_dev))
>> >>  			return -EBUSY;
>> >>
>> >> -		mutex_lock(&data->lock);
>> >>  		ret = mma8452_read(data, buffer);
>> >> -		mutex_unlock(&data->lock);
>> >>  		iio_device_release_direct(indio_dev);
>> >>  		if (ret < 0)
>> >>  			return ret;
>> >>
>> >
>> > Sashiko has something to say and I tend to agree at the moment:
>> >
>> > Could removing this lock expose mma8452_read() to race conditions with PM
>> > auto-suspend and event configuration?
>>
>> Yes. I tend to agree as well.
>>
>> > mma8452_read() can be interrupted by the PM auto-suspend worker, which puts
>> > the device in STANDBY and disables regulators while mma8452_drdy() is actively
>> > polling over I2C. This can lead to I/O timeouts or errors.
>>
>> Yes, and causing pain for other I2C devices on same bus :(
>>
>> > Additionally, concurrent sysfs writes to event configurations invoke
>> > mma8452_change_config(), which puts the hardware into STANDBY to modify
>> > registers. The Standby transition flushes the hardware FIFO. If this occurs
>> > between the mma8452_drdy() check and the i2c_smbus_read_i2c_block_data() in
>> > mma8452_read(), the block read will fetch flushed or stale data.
>>
>> Argh. Yet another level of trouble.
>>
>> Maybe we should extend the use of data->lock instead. Holding it
>> 1. whenever doing read-modify-write actions
>> 2. while holding the device in standby mode for changing registers
>> 3. while doing suspend/resume
>>
>> I was just adding support for a open-drain mode irq sharing you know.
>> While sashiko-bot definitely is catching lots of valid problems, and
>> fixing them is a good thing, this is starting to feel like I opened
>> Pandoras box by touching this driver :D
>>
>> I will try to wrap up a patch with the above described extended usage of
>> data->lock, but hope we can find a way to get the irq sharing and open
>> drain mode support merged, without necessarily having to fixing all and every
>> possible existing bugs as a pre-condition, but maybe delay some work to
>> later work.
>>
>
> If they're pre-existing conditions then it's not required to fix those in the
> same patch series, you can come back to it another time or someone else can fix
> those.

Great. I have created a local mma8452-next branch where I will continue
the work on some of these issues.

> Nevertheless this is an issue that will be caused directly by this patch if
> applied. If your only goal is to add support for open-drain irq sharing, you
> can probably just drop this for the time being and just focus on that.

I will drop this particular patch for this series, and work on a proper
fix for all these race conditions in my mma8452-next branch.

/Esben