[PATCH v4 0/3] iio: accel: bmc150: fix event-enable race, then use guard(mutex)

[PATCH v4 0/3] iio: accel: bmc150: fix event-enable race, then use guard(mutex)

[Gabriel Rondon]

This series addresses what came out of the v3 review of the
guard(mutex) conversion [1]: the pre-existing bugs had to be fixed
before the conversion could go in.

The unaligned-timestamp / DMA-safety problem in the trigger path is
no longer part of this series: it is fixed by Yash Suthar's
"iio: accel: bmc150: DMA-safe buffers and use
iio_push_to_buffers_with_ts()" v2 series [2], which this series is
rebased on top of.

Patch 1 sorts the header inclusions (suggested by Andy in the v3
thread) so the cleanup.h include added later lands in order. Patch 2
fixes the event-enable race in write_event_config(), where
ev_enable_state was checked outside the lock. Patch 3 is the
guard(mutex)/scoped_guard() conversion itself, now applying on top of
the fixes.

[1] https://lore.kernel.org/linux-iio/20260525110130.61284-1-grondon@gmail.com/
[2] https://lore.kernel.org/linux-iio/20260815175728.99541-1-yashsuthar983@gmail.com/

Changes in v4:
- Turned into a series: prep and fix first, conversion last
  (Jonathan, v3 review)
- Sorted header inclusions (Andy)
- Fixed the ev_enable_state race under the lock, with Fixes: tag and
  Cc: stable
- The unaligned-timestamp problem raised in the v3 review is fixed by
  [2] rather than by a patch here; the series is rebased on top of [2]

Gabriel Rondon (3):
  iio: accel: bmc150: sort header inclusions alphabetically
  iio: accel: bmc150: take the lock before checking ev_enable_state
  iio: accel: bmc150: use guard(mutex) for mutex handling

 drivers/iio/accel/bmc150-accel-core.c | 92 ++++++++++-----------------
 1 file changed, 33 insertions(+), 59 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH v4 1/3] iio: accel: bmc150: sort header inclusions alphabetically

[Gabriel Rondon]

Sort the header inclusions alphabetically. This eases maintenance and
prepares the file for adding a new include in a follow-up patch without
introducing an out-of-order entry. No functional change.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Gabriel Rondon <grondon@gmail.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 10c67f47b5a0..d067da9b5ce4 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -4,24 +4,24 @@
  * Copyright (c) 2014, Intel Corporation.
  */
 
-#include <linux/module.h>
-#include <linux/i2c.h>
-#include <linux/interrupt.h>
-#include <linux/delay.h>
-#include <linux/slab.h>
 #include <linux/acpi.h>
-#include <linux/pm.h>
-#include <linux/pm_runtime.h>
-#include <linux/property.h>
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
 #include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/slab.h>
 
 #include "bmc150-accel.h"
 
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v4 1/3] iio: accel: bmc150: sort header inclusions alphabetically

[Andy Shevchenko]

On Mon, Aug 17, 2026 at 12:42:29AM +0100, Gabriel Rondon wrote:
> Sort the header inclusions alphabetically. This eases maintenance and
> prepares the file for adding a new include in a follow-up patch without
> introducing an out-of-order entry. No functional change.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 2/3] iio: accel: bmc150: take the lock before checking ev_enable_state

[Gabriel Rondon]

bmc150_accel_write_event_config() compared the requested state against
data->ev_enable_state before acquiring data->mutex. Two threads racing
to enable and disable the same event can both pass the check and then
both call bmc150_accel_set_interrupt(), which tracks enables with an
atomic users count. The inc/dec can become unbalanced, leaving the
interrupt enabled or disabled against the callers' intent.

Move the check inside the locked region so the test of ev_enable_state
and its update are atomic with respect to the interrupt accounting.

Fixes: 14ee64f438b8 ("iio: bmc150: exit early if event / trigger state is not changed")
Cc: stable@vger.kernel.org
Signed-off-by: Gabriel Rondon <grondon@gmail.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index d067da9b5ce4..bcbb9f0c830a 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -805,11 +805,13 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int ret;
 
-	if (state == data->ev_enable_state)
-		return 0;
-
 	mutex_lock(&data->mutex);
 
+	if (state == data->ev_enable_state) {
+		mutex_unlock(&data->mutex);
+		return 0;
+	}
+
 	ret = bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_ANY_MOTION,
 					 state);
 	if (ret < 0) {
-- 
2.50.1 (Apple Git-155)

[PATCH v4 3/3] iio: accel: bmc150: use guard(mutex) for mutex handling

[Gabriel Rondon]

Replace manual mutex_lock()/mutex_unlock() pairs with guard(mutex) and
scoped_guard() from cleanup.h in the functions where the critical
section covers the whole function body or a single statement. This
simplifies the error paths by removing the explicit unlock calls
before returning.

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.

Call sites that take and drop the mutex several times per function
(read_raw, write_raw) or unlock through a goto label
(buffer_postenable/predisable) are left untouched and can be converted
separately.

Reviewed-by: Stepan Ionichev <sozdayvek@gmail.com>
Signed-off-by: Gabriel Rondon <grondon@gmail.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 74 +++++++++------------------
 1 file changed, 23 insertions(+), 51 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index bcbb9f0c830a..470f5da267ea 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/iio/buffer.h>
@@ -599,18 +600,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;
 }
 
@@ -622,25 +620,22 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
 	int ret;
 	int axis = chan->scan_index;
 
-	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),
 			       &data->regval, sizeof(data->regval));
 	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(data->regval) >> 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;
 
@@ -805,22 +800,17 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	if (state == data->ev_enable_state) {
-		mutex_unlock(&data->mutex);
+	if (state == data->ev_enable_state)
 		return 0;
-	}
 
 	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 +835,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 +847,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 +890,9 @@ 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;
 }
@@ -1023,13 +1007,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(
@@ -1189,10 +1170,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->scan.channels, AXIS_MAX * 2);
-	mutex_unlock(&data->mutex);
+	scoped_guard(mutex, &data->mutex)
+		ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
+				       data->scan.channels, AXIS_MAX * 2);
 	if (ret < 0)
 		goto err_read;
 
@@ -1232,31 +1212,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.50.1 (Apple Git-155)

Re: [PATCH v4 3/3] iio: accel: bmc150: use guard(mutex) for mutex handling

[Andy Shevchenko]

On Mon, Aug 17, 2026 at 12:42:31AM +0100, Gabriel Rondon wrote:
> Replace manual mutex_lock()/mutex_unlock() pairs with guard(mutex) and
> scoped_guard() from cleanup.h in the functions where the critical
> section covers the whole function body or a single statement. This
> simplifies the error paths by removing the explicit unlock calls
> before returning.
> 
> 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.
> 
> Call sites that take and drop the mutex several times per function
> (read_raw, write_raw) or unlock through a goto label
> (buffer_postenable/predisable) are left untouched and can be converted
> separately.

...

> 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->scan.channels, AXIS_MAX * 2);
> -	mutex_unlock(&data->mutex);
> +	scoped_guard(mutex, &data->mutex)
> +		ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
> +				       data->scan.channels, AXIS_MAX * 2);
>  	if (ret < 0)
>  		goto err_read;

Not directly related to this change, but even in the original code what is
protected here by the mutex? data->scan.channels?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] iio: accel: bmc150: use guard(mutex) for mutex handling

[Gabriel Rondon]

On Mon, Aug 17, 2026 at 06:39:18PM +0300, Andy Shevchenko wrote:
> Not directly related to this change, but even in the original code what is
> protected here by the mutex? data->scan.channels?

Agreed it is out of scope for this cleanup, so I left the scope unchanged.

Honest answer: not much that is obvious. data->scan.channels does not need
it, since the triggered handler is the only writer and the core serializes
it. The accel one-shot read is already gated with -EBUSY while buffering, so
bmc150_accel_get_axis cannot race the handler. What is left sharing
data->mutex and still reachable during capture is the temperature read, the
bandwidth get/set and scale set, and the event config. But since the handler
is a single regmap_bulk_read and regmap already serializes the bus, it is not
clear the driver mutex guards a real invariant here, and it may well be
vestigial.

Dropping it is a behavioral change worth its own patch and testing rather
than something to fold into a lock-syntax cleanup. Happy to look into that
separately.

Thanks,
Gabriel

Re: [PATCH v4 3/3] iio: accel: bmc150: use guard(mutex) for mutex handling

[Andy Shevchenko]

On Tue, Aug 18, 2026 at 04:41:35PM +0100, Gabriel Rondon wrote:
> On Mon, Aug 17, 2026 at 06:39:18PM +0300, Andy Shevchenko wrote:
> > Not directly related to this change, but even in the original code what is
> > protected here by the mutex? data->scan.channels?
> 
> Agreed it is out of scope for this cleanup, so I left the scope unchanged.
> 
> Honest answer: not much that is obvious. data->scan.channels does not need
> it, since the triggered handler is the only writer and the core serializes
> it. The accel one-shot read is already gated with -EBUSY while buffering, so
> bmc150_accel_get_axis cannot race the handler. What is left sharing
> data->mutex and still reachable during capture is the temperature read, the
> bandwidth get/set and scale set, and the event config. But since the handler
> is a single regmap_bulk_read and regmap already serializes the bus, it is not
> clear the driver mutex guards a real invariant here, and it may well be
> vestigial.

> Dropping it is a behavioral change worth its own patch and testing rather
> than something to fold into a lock-syntax cleanup.

Of course! Just made an observation.

> Happy to look into that separately.

If you have HW to test, I would like to see such a change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] iio: accel: bmc150: use guard(mutex) for mutex handling

[Jonathan Cameron]

On Tue, 18 Aug 2026 18:47:56 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Aug 18, 2026 at 04:41:35PM +0100, Gabriel Rondon wrote:
> > On Mon, Aug 17, 2026 at 06:39:18PM +0300, Andy Shevchenko wrote:  
> > > Not directly related to this change, but even in the original code what is
> > > protected here by the mutex? data->scan.channels?  
> > 
> > Agreed it is out of scope for this cleanup, so I left the scope unchanged.
> > 
> > Honest answer: not much that is obvious. data->scan.channels does not need
> > it, since the triggered handler is the only writer and the core serializes
> > it. The accel one-shot read is already gated with -EBUSY while buffering, so
> > bmc150_accel_get_axis cannot race the handler. What is left sharing
> > data->mutex and still reachable during capture is the temperature read, the
> > bandwidth get/set and scale set, and the event config. But since the handler
> > is a single regmap_bulk_read and regmap already serializes the bus, it is not
> > clear the driver mutex guards a real invariant here, and it may well be
> > vestigial.  
> 
> > Dropping it is a behavioral change worth its own patch and testing rather
> > than something to fold into a lock-syntax cleanup.  
> 
> Of course! Just made an observation.
> 
> > Happy to look into that separately.  
> 
> If you have HW to test, I would like to see such a change.
> 
See my reply to Yash's thread.  The lock scope may well expand as a trade
off against bloat of the iio_priv() structure.  If following what I suggest
it will be protecting the set of cachelines containing the various buffers
used for bulk reads during both DMA and whatever else is done with them.

Jonathan

Re: [PATCH v4 0/3] iio: accel: bmc150: fix event-enable race, then use guard(mutex)

[Jonathan Cameron]

On Mon, 17 Aug 2026 00:42:28 +0100
Gabriel Rondon <grondon@gmail.com> wrote:

> This series addresses what came out of the v3 review of the
> guard(mutex) conversion [1]: the pre-existing bugs had to be fixed
> before the conversion could go in.
> 
> The unaligned-timestamp / DMA-safety problem in the trigger path is
> no longer part of this series: it is fixed by Yash Suthar's
> "iio: accel: bmc150: DMA-safe buffers and use
> iio_push_to_buffers_with_ts()" v2 series [2], which this series is
> rebased on top of.
> 
> Patch 1 sorts the header inclusions (suggested by Andy in the v3
> thread) so the cleanup.h include added later lands in order. Patch 2
> fixes the event-enable race in write_event_config(), where
> ev_enable_state was checked outside the lock. Patch 3 is the
> guard(mutex)/scoped_guard() conversion itself, now applying on top of
> the fixes.
> 
> [1] https://lore.kernel.org/linux-iio/20260525110130.61284-1-grondon@gmail.com/
> [2] https://lore.kernel.org/linux-iio/20260815175728.99541-1-yashsuthar983@gmail.com/
> 
Given I've asked for some changes to Yash's series, this will need
a rebase. So I've marked it as changes requested in patchwork.

Jonathan

> Changes in v4:
> - Turned into a series: prep and fix first, conversion last
>   (Jonathan, v3 review)
> - Sorted header inclusions (Andy)
> - Fixed the ev_enable_state race under the lock, with Fixes: tag and
>   Cc: stable
> - The unaligned-timestamp problem raised in the v3 review is fixed by
>   [2] rather than by a patch here; the series is rebased on top of [2]
> 
> Gabriel Rondon (3):
>   iio: accel: bmc150: sort header inclusions alphabetically
>   iio: accel: bmc150: take the lock before checking ev_enable_state
>   iio: accel: bmc150: use guard(mutex) for mutex handling
> 
>  drivers/iio/accel/bmc150-accel-core.c | 92 ++++++++++-----------------
>  1 file changed, 33 insertions(+), 59 deletions(-)
>