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

[Joshua Crofts]

On Tue, 25 Aug 2026 15:35:17 +0200
Esben Haabendal <esben@xxxxxxxxxx> wrote:

> "Joshua Crofts" <joshua.crofts1@xxxxxxxxx> writes:
> 
> > On Tue, 25 Aug 2026 10:27:45 +0200
> > Esben Haabendal <esben@xxxxxxxxxx> 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 <esben@xxxxxxxxxx>
> >> ---
> >>  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.

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.

-- 
Kind regards,
Joshua Crofts