[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)

[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)