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

[Jonathan Cameron]

On Tue, 25 Aug 2026 16:06:32 +0200
Esben Haabendal <esben@xxxxxxxxxx> wrote:

> "Joshua Crofts" <joshua.crofts1@xxxxxxxxx> writes:
> 
> > 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.  
> 
> Great. I have created a local mma8452-next branch where I will continue
> the work on some of these issues.

Great. It is absolutely fine to postpone (or indeed ignore completely) reports
from sashiko that are on preexisting issues.  That even applies when I point
them out or suggest maybe you take a look!  I don't mind folk saying
they are too busy!  I know I am which is why I try to talk others into doing
this stuff ;)

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

Excellent!

I'll wait on a fresh shorter version of this series then get that queued
up to give an upstream base for further work.

Thanks,

Jonathan

> 
> /Esben