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

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

/Esben