drivers/iio/accel/bmc150-accel-core.c | 68 ++++++++------------------- 1 file changed, 20 insertions(+), 48 deletions(-)
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@gmail.com>
Reviewed-by: Stepan Ionichev <sozdayvek@gmail.com>
---
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(-)
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 2398eb7e12cd..e8c40aa0375d 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -7,6 +7,7 @@
#include <linux/module.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
+#include <linux/cleanup.h>
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/acpi.h>
@@ -600,18 +601,15 @@ static int bmc150_accel_get_temp(struct bmc150_accel_data *data, int *val)
int ret;
unsigned int value;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = regmap_read(data->regmap, BMC150_ACCEL_REG_TEMP, &value);
if (ret < 0) {
dev_err(dev, "Error reading reg_temp\n");
- mutex_unlock(&data->mutex);
return ret;
}
*val = sign_extend32(value, 7);
- mutex_unlock(&data->mutex);
-
return IIO_VAL_INT;
}
@@ -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);
- 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;
@@ -810,17 +804,14 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
if (state == data->ev_enable_state)
return 0;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_ANY_MOTION,
state);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
+ if (ret < 0)
return ret;
- }
data->ev_enable_state = state;
- mutex_unlock(&data->mutex);
return 0;
}
@@ -845,13 +836,10 @@ static ssize_t bmc150_accel_get_fifo_watermark(struct device *dev,
{
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct bmc150_accel_data *data = iio_priv(indio_dev);
- int wm;
- mutex_lock(&data->mutex);
- wm = data->watermark;
- mutex_unlock(&data->mutex);
+ guard(mutex)(&data->mutex);
- return sysfs_emit(buf, "%d\n", wm);
+ return sysfs_emit(buf, "%d\n", data->watermark);
}
static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
@@ -860,13 +848,10 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
{
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct bmc150_accel_data *data = iio_priv(indio_dev);
- bool state;
- mutex_lock(&data->mutex);
- state = data->fifo_mode;
- mutex_unlock(&data->mutex);
+ guard(mutex)(&data->mutex);
- return sysfs_emit(buf, "%d\n", state);
+ return sysfs_emit(buf, "%d\n", data->fifo_mode ? 1 : 0);
}
static const struct iio_mount_matrix *
@@ -906,9 +891,8 @@ static int bmc150_accel_set_watermark(struct iio_dev *indio_dev, unsigned val)
if (val > BMC150_ACCEL_FIFO_LENGTH)
val = BMC150_ACCEL_FIFO_LENGTH;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
data->watermark = val;
- mutex_unlock(&data->mutex);
return 0;
}
@@ -1021,13 +1005,10 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev, unsigned samples)
{
struct bmc150_accel_data *data = iio_priv(indio_dev);
- int ret;
- mutex_lock(&data->mutex);
- ret = __bmc150_accel_fifo_flush(indio_dev, samples, false);
- mutex_unlock(&data->mutex);
+ guard(mutex)(&data->mutex);
- return ret;
+ return __bmc150_accel_fifo_flush(indio_dev, samples, false);
}
static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
@@ -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;
@@ -1230,31 +1210,23 @@ static int bmc150_accel_trigger_set_state(struct iio_trigger *trig,
struct bmc150_accel_data *data = t->data;
int ret;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
- if (t->enabled == state) {
- mutex_unlock(&data->mutex);
+ if (t->enabled == state)
return 0;
- }
if (t->setup) {
ret = t->setup(t, state);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
+ if (ret < 0)
return ret;
- }
}
ret = bmc150_accel_set_interrupt(data, t->intr, state);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
+ if (ret < 0)
return ret;
- }
t->enabled = state;
- mutex_unlock(&data->mutex);
-
return ret;
}
--
2.33.0On Mon, May 25, 2026 at 12:01:27PM +0100, Gabriel Rondon 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. The last paragraph is matter of the comment block or cover letter (but currently it's a single patch, however with the below it becomes a series). ... > #include <linux/module.h> > #include <linux/i2c.h> > #include <linux/interrupt.h> > +#include <linux/cleanup.h> > #include <linux/delay.h> > #include <linux/slab.h> > #include <linux/acpi.h> Please, add a preparatory patch to sort headers alphabetically first. -- With Best Regards, Andy Shevchenko
Hi Gabriel, comments inline.
On Mon, May 25, 2026 at 6:01 AM Gabriel Rondon <grondon@gmail.com> 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@gmail.com>
> Reviewed-by: Stepan Ionichev <sozdayvek@gmail.com>
> ---
> 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.
>
> - 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!):
{
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@gmail.com>
best regards,
max
On Mon, 25 May 2026 15:34:52 -0500
Maxwell Doose <m32285159@gmail.com> wrote:
> Hi Gabriel, comments inline.
>
> On Mon, May 25, 2026 at 6:01 AM Gabriel Rondon <grondon@gmail.com> 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@gmail.com>
> > Reviewed-by: Stepan Ionichev <sozdayvek@gmail.com>
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@gmail.com>
>
> best regards,
> max
© 2016 - 2026 Red Hat, Inc.