Re: [PATCH v3] iio: accel: bmc150: use guard(mutex) for mutex handling

[Jonathan Cameron]

On Mon, 25 May 2026 15:34:52 -0500
Maxwell Doose <m32285159@xxxxxxxxx> wrote:

> Hi Gabriel, comments inline.
> 
> On Mon, May 25, 2026 at 6:01 AM Gabriel Rondon <grondon@xxxxxxxxx> wrote:
> >
> > Replace manual mutex_lock()/mutex_unlock() pairs with guard(mutex)
> > and scoped_guard() from cleanup.h. This simplifies error paths by
> > removing the need for explicit unlock calls before returning.
> >
> > Most converted functions hold the lock for their entire body, so
> > guard(mutex) applies directly. bmc150_accel_trigger_handler() only
> > holds the lock around a single register read, so scoped_guard() is
> > used there to keep the lock scope unchanged.
> >
> > Signed-off-by: Gabriel Rondon <grondon@xxxxxxxxx>
> > Reviewed-by: Stepan Ionichev <sozdayvek@xxxxxxxxx>
I'm not going to apply this quite yet because one of the bugs sashiko
pointed out needs fixing more urgently than this patch.
https://sashiko.dev/#/patchset/20260525110130.61284-1-grondon%40gmail.com

static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
{
	struct iio_poll_func *pf = p;
	struct iio_dev *indio_dev = pf->indio_dev;
	struct bmc150_accel_data *data = iio_priv(indio_dev);
	int ret;

	mutex_lock(&data->mutex);
	ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
			       data->buffer, AXIS_MAX * 2);
	mutex_unlock(&data->mutex);
	if (ret < 0)
		goto err_read;

	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
//Buffer isn't aligned to an s64
					   pf->timestamp);
err_read:
	iio_trigger_notify_done(indio_dev->trig);

	return IRQ_HANDLED;
}

The timestamp may end up as an unaligned s64 write which on some
architectures will fail.  

I'm not sure why the driver isn't using data->scan for this path
It seems much more logical as a target and is already appropriately
aligned for this call - it should actually be aligned with
__aligned(IIO_DMA_MINALIGN) and moved to the end of the structure
because this driver supports SPI transfers and we are still not
assuming a regmap_bulk_read() won't use the buffer directly for DMA.

The other comment sashiko made that I think might be valid is the
one about:

static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
					   const struct iio_chan_spec *chan,
					   enum iio_event_type type,
					   enum iio_event_direction dir,
					   bool state)
{
	struct bmc150_accel_data *data = iio_priv(indio_dev);
	int ret;

	if (state == data->ev_enable_state)
//check not under lock so two racing threads can pass it.
		return 0;

	mutex_lock(&data->mutex);

	ret = bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_ANY_MOTION,
//and increment the counter burried in this call, but on the disable
//path we may see only one decrement.

					 state);
	if (ret < 0) {
		mutex_unlock(&data->mutex);
		return ret;
	}

	data->ev_enable_state = state;
	mutex_unlock(&data->mutex);

	return 0;
}

Good to fix that one before this guard() patch - will need an extra unlock
obviously, then follow up with a 3rd patch that is similar to this but
with the guard() a few lines earlier

> > ---
> > Changes since v2:
> >  - bmc150_accel_get_fifo_state(): normalize fifo_mode to 0/1 in the
> >    sysfs output (data->fifo_mode ? 1 : 0). v2 dropped the bool
> >    intermediate and printed the raw mode value, which would emit the
> >    FIFO mode bit (0x40) as 64 instead of 0/1. The original 0/1 output
> >    is preserved. Reported by Jonathan Cameron.
> >  - Collected Reviewed-by from Stepan Ionichev.
> >
> > Changes since v1:
> >  - Drop the verbose list of converted functions from the commit message.
> >  - bmc150_accel_get_temp(): keep the original declaration order; no
> >    unrelated line movement.
> >  - bmc150_accel_get_axis(): convert to guard(mutex) as well, the only
> >    code outside the old lock scope was a trivial error check.
> >  - bmc150_accel_trigger_handler(): use scoped_guard() so that no manual
> >    mutex_lock()/mutex_unlock() pairs are left behind.
> >  - bmc150_accel_get_fifo_watermark() / bmc150_accel_get_fifo_state():
> >    drop the intermediate variable and return directly.
> >
> >  drivers/iio/accel/bmc150-accel-core.c | 68 ++++++++-------------------
> >  1 file changed, 20 insertions(+), 48 deletions(-)
> >  
> [snip]
> > @@ -624,25 +622,21 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
> >         int axis = chan->scan_index;
> >         __le16 raw_val;
> >
> > -       mutex_lock(&data->mutex);
> > +       guard(mutex)(&data->mutex);
> >         ret = bmc150_accel_set_power_state(data, true);
> >  
> 
> Just a heads up, Jonathan or Andy may ask you to add a blank line in
> between the guard()() and the function call below.
Yup. I might have tweaked that whilst applying. 
> 
> >
> > -       if (ret < 0) {
> > -               mutex_unlock(&data->mutex);
> > +       if (ret < 0)
> >                 return ret;
> > -       }
> >
> >         ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
> >                                &raw_val, sizeof(raw_val));
> >         if (ret < 0) {
> >                 dev_err(dev, "Error reading axis %d\n", axis);
> >                 bmc150_accel_set_power_state(data, false);
> > -               mutex_unlock(&data->mutex);
> >                 return ret;
> >         }
> >         *val = sign_extend32(le16_to_cpu(raw_val) >> chan->scan_type.shift,
> >                              chan->scan_type.realbits - 1);
> >         ret = bmc150_accel_set_power_state(data, false);
> > -       mutex_unlock(&data->mutex);
> >         if (ret < 0)
> >                 return ret;
> >  
> [snip]
> > @@ -1187,10 +1168,9 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
> >         struct bmc150_accel_data *data = iio_priv(indio_dev);
> >         int ret;
> >
> > -       mutex_lock(&data->mutex);
> > -       ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
> > -                              data->buffer, AXIS_MAX * 2);
> > -       mutex_unlock(&data->mutex);
> > +       scoped_guard(mutex, &data->mutex)
> > +               ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
> > +                                      data->buffer, AXIS_MAX * 2);
> >         if (ret < 0)
> >                 goto err_read;
> >  
> 
> Another heads up, you may be asked to refactor the critical section
> into a helper or do guard()() with {}. So, something like (and not
> compiled! Also, don't use this directly, the mail client I'm using to
> send this converts tabs to spaces!):
> 

Whilst I don't  much like scoped_guard() for readability reasons, the
one time I'm normally ok with it is for single calls like this.
The helper isn't going to add much, unlike when we have a bunch of calls
under the lock.

It's a bit marginal as it sort of violates the no guard + goto rules
so maybe a helper is a better idea as you suggest.

> {
>         guard(mutex)(&data->mutex);
>         ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
>                                                data->buffer, AXIS_MAX * 2);
> }
> 
> or:
> 
> static int bmc150_accel_trigger_handler_helper(struct bmc150_accel_data *data)
> {
>         guard(mutex)(&data->mutex);
>         return regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
>                                                  data->buffer, AXIS_MAX * 2);
> }
> 
> Obviously, the patch as-is seems correct to me, so
> 
> Reviewed-by: Maxwell Doose <m32285159@xxxxxxxxx>
> 
> best regards,
> max