public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] iio: magnetometer: rm3100: Modernize locking and control flow
@ 2026-04-28 12:46 Maxwell Doose
  2026-04-28 12:46 ` [PATCH v2 1/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag() Maxwell Doose
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 12:46 UTC (permalink / raw)
  To: songqiang1304521, jic23; +Cc: dlechner, nuno.sa, andy, linux-iio, linux-kernel

The goal of this patch series is to replace the manual mutex_lock() and
mutex_unlock() calls in rm3100-core.c with their more modern
counterparts, guard(mutex)() and scoped_guard(). I've also done some
minor cleanups, removing what are now redundant gotos, and enabling
direct returns.

Following feedback on recent commits, I've learned that it's a better
idea to split changes to be more atomic, and I've done that here in
case any particular change *somehow* causes a build regression. The
changes have been test-compiled however, and according to make, smatch,
and sparse, should be sound.

=== Changelog ===
v2:
 - Added small style fixes per Andy's suggestions (Adding blank lines,
   moving an if statement in a scoped_guard block).
 - Switched out scoped_guard() for guard(mutex)() in certain commits.
 - Fixed error in commit 4 where deadlocks could occur, as goto ignores
   __attribute__((cleanup)). This has been fixed by the above.

Maxwell Doose (4):
  iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag()
  iio: magnetometer: rm3100: Use scoped_guard() in
    rm3100_get_samp_freq()
  iio: magnetometer: rm3100: Use guard(mutex)() in
    rm3100_set_samp_freq()
  iio: magnetometer: rm3100: Use guard(mutex)() in
    rm3100_trigger_handler()

 drivers/iio/magnetometer/rm3100-core.c | 63 ++++++++++----------------
 1 file changed, 24 insertions(+), 39 deletions(-)

-- 
2.53.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag()
  2026-04-28 12:46 [PATCH 0/4] iio: magnetometer: rm3100: Modernize locking and control flow Maxwell Doose
@ 2026-04-28 12:46 ` Maxwell Doose
  2026-04-28 13:26   ` Andy Shevchenko
  2026-04-28 12:46 ` [PATCH v2 2/4] iio: magnetometer: rm3100: Use scoped_guard() in rm3100_get_samp_freq() Maxwell Doose
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 12:46 UTC (permalink / raw)
  To: songqiang1304521, jic23; +Cc: dlechner, nuno.sa, andy, linux-iio, linux-kernel

Replace mutex_lock() and mutex_unlock() calls in rm3100_read_mag() with
the more modern guard(mutex)(). This will help modernize the driver and
bring it up-to-date with modern available macros/functions.

While at it, remove the now unnecessary "unlock_return" goto and
directly return in if statements.

Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
v2:
 - Switch out scoped_guard() for guard(mutex)() per Andy.

 drivers/iio/magnetometer/rm3100-core.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index 2b2884425746..426963935d60 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -204,27 +204,23 @@ static int rm3100_read_mag(struct rm3100_data *data, int idx, int *val)
 	u8 buffer[3];
 	int ret;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
+
 	ret = regmap_write(regmap, RM3100_REG_POLL, BIT(4 + idx));
 	if (ret < 0)
-		goto unlock_return;
+		return ret;
 
 	ret = rm3100_wait_measurement(data);
 	if (ret < 0)
-		goto unlock_return;
+		return ret;
 
 	ret = regmap_bulk_read(regmap, RM3100_REG_MX2 + 3 * idx, buffer, 3);
 	if (ret < 0)
-		goto unlock_return;
-	mutex_unlock(&data->lock);
+		return ret;
 
 	*val = sign_extend32(get_unaligned_be24(&buffer[0]), 23);
 
 	return IIO_VAL_INT;
-
-unlock_return:
-	mutex_unlock(&data->lock);
-	return ret;
 }
 
 #define RM3100_CHANNEL(axis, idx)					\
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/4] iio: magnetometer: rm3100: Use scoped_guard() in rm3100_get_samp_freq()
  2026-04-28 12:46 [PATCH 0/4] iio: magnetometer: rm3100: Modernize locking and control flow Maxwell Doose
  2026-04-28 12:46 ` [PATCH v2 1/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag() Maxwell Doose
@ 2026-04-28 12:46 ` Maxwell Doose
  2026-04-28 13:26   ` Andy Shevchenko
  2026-04-28 12:46 ` [PATCH v2 3/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_set_samp_freq() Maxwell Doose
  2026-04-28 12:46 ` [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler() Maxwell Doose
  3 siblings, 1 reply; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 12:46 UTC (permalink / raw)
  To: songqiang1304521, jic23; +Cc: dlechner, nuno.sa, andy, linux-iio, linux-kernel

Replace mutex_lock() and mutex_unlock() calls in rm3100_get_samp_freq()
with the more modern scoped_guard(). This will help modernize the
driver and bring it up-to-date with modern available macros/functions.

Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 v2:
 - Move if statement into scoped_guard body per Andy.

 drivers/iio/magnetometer/rm3100-core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index 426963935d60..9f25efb2d02d 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -280,11 +280,12 @@ static int rm3100_get_samp_freq(struct rm3100_data *data, int *val, int *val2)
 	unsigned int tmp;
 	int ret;
 
-	mutex_lock(&data->lock);
-	ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);
-	mutex_unlock(&data->lock);
-	if (ret < 0)
-		return ret;
+	scoped_guard(mutex, &data->lock) {
+		ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);
+
+		if (ret < 0)
+			return ret;
+	}
 	*val = rm3100_samp_rates[tmp - RM3100_TMRC_OFFSET][0];
 	*val2 = rm3100_samp_rates[tmp - RM3100_TMRC_OFFSET][1];
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 3/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_set_samp_freq()
  2026-04-28 12:46 [PATCH 0/4] iio: magnetometer: rm3100: Modernize locking and control flow Maxwell Doose
  2026-04-28 12:46 ` [PATCH v2 1/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag() Maxwell Doose
  2026-04-28 12:46 ` [PATCH v2 2/4] iio: magnetometer: rm3100: Use scoped_guard() in rm3100_get_samp_freq() Maxwell Doose
@ 2026-04-28 12:46 ` Maxwell Doose
  2026-04-28 12:46 ` [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler() Maxwell Doose
  3 siblings, 0 replies; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 12:46 UTC (permalink / raw)
  To: songqiang1304521, jic23; +Cc: dlechner, nuno.sa, andy, linux-iio, linux-kernel

Replace mutex_lock() and mutex_unlock() calls in rm3100_set_samp_freq
with the more modern guard(mutex)(). This will help modernize the
driver and bring it up-to-date with modern available macros/functions.

While at it, remove unlock_return goto and use direct returns instead.

Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
v2:
 - Added blank line below guard(mutex)() per Andy.

 drivers/iio/magnetometer/rm3100-core.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index 9f25efb2d02d..eb59ea8d5bf5 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -335,56 +335,50 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 	int ret;
 	int i;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
+
 	/* All cycle count registers use the same value. */
 	ret = regmap_read(regmap, RM3100_REG_CC_X, &cycle_count);
 	if (ret < 0)
-		goto unlock_return;
+		return ret;
 
 	for (i = 0; i < RM3100_SAMP_NUM; i++) {
 		if (val == rm3100_samp_rates[i][0] &&
 		    val2 == rm3100_samp_rates[i][1])
 			break;
 	}
-	if (i == RM3100_SAMP_NUM) {
-		ret = -EINVAL;
-		goto unlock_return;
-	}
+	if (i == RM3100_SAMP_NUM)
+		return -EINVAL;
 
 	ret = regmap_write(regmap, RM3100_REG_TMRC, i + RM3100_TMRC_OFFSET);
 	if (ret < 0)
-		goto unlock_return;
+		return ret;
 
 	/* Checking if cycle count registers need changing. */
 	if (val == 600 && cycle_count == 200) {
 		ret = rm3100_set_cycle_count(data, 100);
 		if (ret < 0)
-			goto unlock_return;
+			return ret;
 	} else if (val != 600 && cycle_count == 100) {
 		ret = rm3100_set_cycle_count(data, 200);
 		if (ret < 0)
-			goto unlock_return;
+			return ret;
 	}
 
 	if (iio_buffer_enabled(indio_dev)) {
 		/* Writing TMRC registers requires CMM reset. */
 		ret = regmap_write(regmap, RM3100_REG_CMM, 0);
 		if (ret < 0)
-			goto unlock_return;
+			return ret;
 		ret = regmap_write(data->regmap, RM3100_REG_CMM,
 			(*indio_dev->active_scan_mask & 0x7) <<
 			RM3100_CMM_AXIS_SHIFT | RM3100_CMM_START);
 		if (ret < 0)
-			goto unlock_return;
+			return ret;
 	}
-	mutex_unlock(&data->lock);
 
 	data->conversion_time = rm3100_samp_rates[i][2] * 2;
 	return 0;
-
-unlock_return:
-	mutex_unlock(&data->lock);
-	return ret;
 }
 
 static int rm3100_read_raw(struct iio_dev *indio_dev,
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler()
  2026-04-28 12:46 [PATCH 0/4] iio: magnetometer: rm3100: Modernize locking and control flow Maxwell Doose
                   ` (2 preceding siblings ...)
  2026-04-28 12:46 ` [PATCH v2 3/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_set_samp_freq() Maxwell Doose
@ 2026-04-28 12:46 ` Maxwell Doose
  2026-04-28 15:32   ` Jonathan Cameron
  3 siblings, 1 reply; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 12:46 UTC (permalink / raw)
  To: songqiang1304521, jic23; +Cc: dlechner, nuno.sa, andy, linux-iio, linux-kernel

Replace mutex_lock() and mutex_unlock() calls in
rm3100_trigger_handler() with the more modern guard(mutex)(). This will
help modernize the driver and bring it up-to-date with modern available
macros/functions.

Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
  v2:
 - Switched out scoped_guard() for guard(mutex)().
 - Because of the replacement of scoped_guard(), fixed an error due to
   gotos bypassing __attribute__((cleanup)).

 drivers/iio/magnetometer/rm3100-core.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index eb59ea8d5bf5..fdee82c1290f 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -459,11 +459,11 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
 	struct regmap *regmap = data->regmap;
 	int ret, i, bit;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
+
 	switch (scan_mask) {
 	case BIT(0) | BIT(1) | BIT(2):
 		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
-		mutex_unlock(&data->lock);
 		if (ret < 0)
 			goto done;
 		/* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for paddings. */
@@ -472,21 +472,18 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
 		break;
 	case BIT(0) | BIT(1):
 		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
-		mutex_unlock(&data->lock);
 		if (ret < 0)
 			goto done;
 		memmove(data->buffer + 4, data->buffer + 3, 3);
 		break;
 	case BIT(1) | BIT(2):
 		ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
-		mutex_unlock(&data->lock);
 		if (ret < 0)
 			goto done;
 		memmove(data->buffer + 4, data->buffer + 3, 3);
 		break;
 	case BIT(0) | BIT(2):
 		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
-		mutex_unlock(&data->lock);
 		if (ret < 0)
 			goto done;
 		memmove(data->buffer + 4, data->buffer + 6, 3);
@@ -495,12 +492,9 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
 		for_each_set_bit(bit, &scan_mask, mask_len) {
 			ret = regmap_bulk_read(regmap, RM3100_REG_MX2 + 3 * bit,
 					       data->buffer, 3);
-			if (ret < 0) {
-				mutex_unlock(&data->lock);
+			if (ret < 0)
 				goto done;
-			}
 		}
-		mutex_unlock(&data->lock);
 	}
 	/*
 	 * Always using the same buffer so that we wouldn't need to set the
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag()
  2026-04-28 12:46 ` [PATCH v2 1/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag() Maxwell Doose
@ 2026-04-28 13:26   ` Andy Shevchenko
  2026-04-28 14:59     ` Maxwell Doose
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2026-04-28 13:26 UTC (permalink / raw)
  To: Maxwell Doose
  Cc: songqiang1304521, jic23, dlechner, nuno.sa, andy, linux-iio,
	linux-kernel

On Tue, Apr 28, 2026 at 07:46:41AM -0500, Maxwell Doose wrote:
> Replace mutex_lock() and mutex_unlock() calls in rm3100_read_mag() with
> the more modern guard(mutex)(). This will help modernize the driver and
> bring it up-to-date with modern available macros/functions.
> 
> While at it, remove the now unnecessary "unlock_return" goto and
> directly return in if statements.

Perhaps we need to also put a summary of the extended critical section, so that
it will be clear that we thought about it and decided to cover that small memory
access + some bitops.

Otherwise LGTM.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/4] iio: magnetometer: rm3100: Use scoped_guard() in rm3100_get_samp_freq()
  2026-04-28 12:46 ` [PATCH v2 2/4] iio: magnetometer: rm3100: Use scoped_guard() in rm3100_get_samp_freq() Maxwell Doose
@ 2026-04-28 13:26   ` Andy Shevchenko
  2026-04-28 13:59     ` Maxwell Doose
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2026-04-28 13:26 UTC (permalink / raw)
  To: Maxwell Doose
  Cc: songqiang1304521, jic23, dlechner, nuno.sa, andy, linux-iio,
	linux-kernel

On Tue, Apr 28, 2026 at 07:46:42AM -0500, Maxwell Doose wrote:
> Replace mutex_lock() and mutex_unlock() calls in rm3100_get_samp_freq()
> with the more modern scoped_guard(). This will help modernize the
> driver and bring it up-to-date with modern available macros/functions.

...

> -	mutex_lock(&data->lock);
> -	ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);
> -	mutex_unlock(&data->lock);
> -	if (ret < 0)
> -		return ret;
> +	scoped_guard(mutex, &data->lock) {
> +		ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);

> +

Redundant blank line as ret is defined elsewhere.

> +		if (ret < 0)
> +			return ret;
> +	}

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/4] iio: magnetometer: rm3100: Use scoped_guard() in rm3100_get_samp_freq()
  2026-04-28 13:26   ` Andy Shevchenko
@ 2026-04-28 13:59     ` Maxwell Doose
  2026-04-28 15:24       ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 13:59 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: songqiang1304521, jic23, dlechner, nuno.sa, andy, linux-iio,
	linux-kernel

On Tue, Apr 28, 2026 at 8:26 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:

> > -     mutex_lock(&data->lock);
> > -     ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);
> > -     mutex_unlock(&data->lock);
> > -     if (ret < 0)
> > -             return ret;
> > +     scoped_guard(mutex, &data->lock) {
> > +             ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);
>
> > +
>
> Redundant blank line as ret is defined elsewhere.

Shoot. I'm away right now but once I get back I can fix that.

best regards,
maxwell

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag()
  2026-04-28 13:26   ` Andy Shevchenko
@ 2026-04-28 14:59     ` Maxwell Doose
  2026-04-28 15:58       ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 14:59 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: songqiang1304521, jic23, dlechner, nuno.sa, andy, linux-iio,
	linux-kernel

On Tue, Apr 28, 2026 at 8:26 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> Perhaps we need to also put a summary of the extended critical section, so that
> it will be clear that we thought about it and decided to cover that small memory
> access + some bitops.
>
> Otherwise LGTM.
>

Sounds good, but I'm going to wait 1 or 2 days to see if a maintainer
wants to comment on whether or not we *need* the summary about the
mutex scope. If they don't say anything I'll probably just send a v3
with the summary added in the commit message.

best regards,
maxwell

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/4] iio: magnetometer: rm3100: Use scoped_guard() in rm3100_get_samp_freq()
  2026-04-28 13:59     ` Maxwell Doose
@ 2026-04-28 15:24       ` Jonathan Cameron
  2026-04-28 17:08         ` Maxwell Doose
  0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2026-04-28 15:24 UTC (permalink / raw)
  To: Maxwell Doose
  Cc: Andy Shevchenko, songqiang1304521, dlechner, nuno.sa, andy,
	linux-iio, linux-kernel

On Tue, 28 Apr 2026 08:59:48 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Tue, Apr 28, 2026 at 8:26 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> 
> > > -     mutex_lock(&data->lock);
> > > -     ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);
> > > -     mutex_unlock(&data->lock);
> > > -     if (ret < 0)
> > > -             return ret;
> > > +     scoped_guard(mutex, &data->lock) {
> > > +             ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);  
> >  
> > > +  
> >
> > Redundant blank line as ret is defined elsewhere.  
> 
> Shoot. I'm away right now but once I get back I can fix that.

Given the function is:
static int rm3100_get_samp_freq(struct rm3100_data *data, int *val, int *val2)
{
	unsigned int tmp;
	int ret;

	mutex_lock(&data->lock);
	ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);
	mutex_unlock(&data->lock);
	if (ret < 0)
		return ret;
	*val = rm3100_samp_rates[tmp - RM3100_TMRC_OFFSET][0];
	*val2 = rm3100_samp_rates[tmp - RM3100_TMRC_OFFSET][1];

	return IIO_VAL_INT_PLUS_MICRO;
}

I'd just use a guard and not worry about two value lookups from static const data
being under the guard()

Why is this series broken up into lots of patches doing same type of change
in different places?  This could definitely be combined with the previous patch.

Thanks,

Jonathan

> 
> best regards,
> maxwell


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler()
  2026-04-28 12:46 ` [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler() Maxwell Doose
@ 2026-04-28 15:32   ` Jonathan Cameron
  2026-04-28 17:20     ` Maxwell Doose
  2026-04-28 17:27     ` Maxwell Doose
  0 siblings, 2 replies; 17+ messages in thread
From: Jonathan Cameron @ 2026-04-28 15:32 UTC (permalink / raw)
  To: Maxwell Doose
  Cc: songqiang1304521, dlechner, nuno.sa, andy, linux-iio,
	linux-kernel

On Tue, 28 Apr 2026 07:46:44 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> Replace mutex_lock() and mutex_unlock() calls in
> rm3100_trigger_handler() with the more modern guard(mutex)(). This will
> help modernize the driver and bring it up-to-date with modern available
> macros/functions.
> 
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
>   v2:
>  - Switched out scoped_guard() for guard(mutex)().
>  - Because of the replacement of scoped_guard(), fixed an error due to
>    gotos bypassing __attribute__((cleanup)).

Read the guidance in cleanup.h  It's pretty clear that nothing from
that file should ever be used in functions that use gotos.

Whilst there might not be a bug here, it ends up being fragile
as people changing the code may not noticed that can't use an existing
label and hence jump the hidden __free() in that guard() call.

The cleanest way to use guard() here is to factor out the code under the guard.
Don't increase the scope as the final stuff in here is not trivial.

There is even an outside chance of deadlock if you hold a local lock
when calling iio_trigger_notify_done() as that can call back into the driver.
Here you are ok because no trigger_ops are set so there isn't a reenable()
callback. 

Be careful to ensure any expanded scope only contains calls where you can
easily see they don't deadlock or do anything time consuming.

Jonathan



> 
>  drivers/iio/magnetometer/rm3100-core.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
> index eb59ea8d5bf5..fdee82c1290f 100644
> --- a/drivers/iio/magnetometer/rm3100-core.c
> +++ b/drivers/iio/magnetometer/rm3100-core.c
> @@ -459,11 +459,11 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
>  	struct regmap *regmap = data->regmap;
>  	int ret, i, bit;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
> +
>  	switch (scan_mask) {
>  	case BIT(0) | BIT(1) | BIT(2):
>  		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> -		mutex_unlock(&data->lock);
>  		if (ret < 0)
>  			goto done;
>  		/* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for paddings. */
> @@ -472,21 +472,18 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
>  		break;
>  	case BIT(0) | BIT(1):
>  		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
> -		mutex_unlock(&data->lock);
>  		if (ret < 0)
>  			goto done;
>  		memmove(data->buffer + 4, data->buffer + 3, 3);
>  		break;
>  	case BIT(1) | BIT(2):
>  		ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
> -		mutex_unlock(&data->lock);
>  		if (ret < 0)
>  			goto done;
>  		memmove(data->buffer + 4, data->buffer + 3, 3);
>  		break;
>  	case BIT(0) | BIT(2):
>  		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> -		mutex_unlock(&data->lock);
>  		if (ret < 0)
>  			goto done;
>  		memmove(data->buffer + 4, data->buffer + 6, 3);
> @@ -495,12 +492,9 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
>  		for_each_set_bit(bit, &scan_mask, mask_len) {
>  			ret = regmap_bulk_read(regmap, RM3100_REG_MX2 + 3 * bit,
>  					       data->buffer, 3);
> -			if (ret < 0) {
> -				mutex_unlock(&data->lock);
> +			if (ret < 0)
>  				goto done;
> -			}
>  		}
> -		mutex_unlock(&data->lock);
>  	}
>  	/*
>  	 * Always using the same buffer so that we wouldn't need to set the


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag()
  2026-04-28 14:59     ` Maxwell Doose
@ 2026-04-28 15:58       ` Jonathan Cameron
  0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2026-04-28 15:58 UTC (permalink / raw)
  To: Maxwell Doose
  Cc: Andy Shevchenko, songqiang1304521, dlechner, nuno.sa, andy,
	linux-iio, linux-kernel

On Tue, 28 Apr 2026 09:59:55 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Tue, Apr 28, 2026 at 8:26 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > Perhaps we need to also put a summary of the extended critical section, so that
> > it will be clear that we thought about it and decided to cover that small memory
> > access + some bitops.
> >
> > Otherwise LGTM.
> >  
> 
> Sounds good, but I'm going to wait 1 or 2 days to see if a maintainer
> wants to comment on whether or not we *need* the summary about the
> mutex scope. If they don't say anything I'll probably just send a v3
> with the summary added in the commit message.

Where scope changes, it is indeed good to add a brief note on what
ends up incorporated in the expanded scope and whether that is worth
thinking about or not when reviewing the patch.

Here I think it's easy to conclude we don't care.  The last patch not
so much!

J
> 
> best regards,
> maxwell


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/4] iio: magnetometer: rm3100: Use scoped_guard() in rm3100_get_samp_freq()
  2026-04-28 15:24       ` Jonathan Cameron
@ 2026-04-28 17:08         ` Maxwell Doose
  0 siblings, 0 replies; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 17:08 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Andy Shevchenko, songqiang1304521, dlechner, nuno.sa, andy,
	linux-iio, linux-kernel

On Tue, Apr 28, 2026 at 10:25 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> I'd just use a guard and not worry about two value lookups from static const data
> being under the guard()

We'd probably want to keep the scope as close as possible to the
original code, but it's your call, I can use guard(mutex)() instead.

> Why is this series broken up into lots of patches doing same type of change
> in different places?  This could definitely be combined with the previous patch.

I've had some suggestions on other patches to split some changes, and
thus, I've been erring on the side of atomicity. I can see now how
this probably should be combined, and I'll merge these for a v3 and
add the suggested fixes for when I have free time.

best regards,
maxwell

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler()
  2026-04-28 15:32   ` Jonathan Cameron
@ 2026-04-28 17:20     ` Maxwell Doose
  2026-04-28 17:27     ` Maxwell Doose
  1 sibling, 0 replies; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 17:20 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: songqiang1304521, dlechner, nuno.sa, andy, linux-iio,
	linux-kernel

On Tue, Apr 28, 2026 at 10:32 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Read the guidance in cleanup.h  It's pretty clear that nothing from
> that file should ever be used in functions that use gotos.
>
> Whilst there might not be a bug here, it ends up being fragile
> as people changing the code may not noticed that can't use an existing
> label and hence jump the hidden __free() in that guard() call.
>

Then I guess if we really want to do this then we should probably get
rid of the gotos? Because I can certainly do that. Either that or we
just keep the manual locks and unlocks.

> Be careful to ensure any expanded scope only contains calls where you can
> easily see they don't deadlock or do anything time consuming.

I'll also double-check that.

best regards,
maxwell

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler()
  2026-04-28 15:32   ` Jonathan Cameron
  2026-04-28 17:20     ` Maxwell Doose
@ 2026-04-28 17:27     ` Maxwell Doose
  2026-04-28 17:39       ` Jonathan Cameron
  1 sibling, 1 reply; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 17:27 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: songqiang1304521, dlechner, nuno.sa, andy, linux-iio,
	linux-kernel

On Tue, Apr 28, 2026 at 10:32 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
[snip]
>
> There is even an outside chance of deadlock if you hold a local lock
> when calling iio_trigger_notify_done() as that can call back into the driver.
> Here you are ok because no trigger_ops are set so there isn't a reenable()
> callback.
>

Given this, I'll also likely go back to scoped_guard as well once we
remove the gotos, that way we'll be able to unlock it before we get to
a potential callback.

best regards,
maxwell

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler()
  2026-04-28 17:27     ` Maxwell Doose
@ 2026-04-28 17:39       ` Jonathan Cameron
  2026-04-28 19:09         ` Maxwell Doose
  0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2026-04-28 17:39 UTC (permalink / raw)
  To: Maxwell Doose
  Cc: songqiang1304521, dlechner, nuno.sa, andy, linux-iio,
	linux-kernel

On Tue, 28 Apr 2026 12:27:18 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Tue, Apr 28, 2026 at 10:32 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> [snip]
> >
> > There is even an outside chance of deadlock if you hold a local lock
> > when calling iio_trigger_notify_done() as that can call back into the driver.
> > Here you are ok because no trigger_ops are set so there isn't a reenable()
> > callback.
> >  
> 
> Given this, I'll also likely go back to scoped_guard as well once we
> remove the gotos, that way we'll be able to unlock it before we get to
> a potential callback.
> 
I'd prefer a helper function with straight forward guard() + returns.
Then you can have a single exit point from the outer function.

ret = helper()
if (ret)
	return ret;

other stuff.

> best regards,
> maxwell


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler()
  2026-04-28 17:39       ` Jonathan Cameron
@ 2026-04-28 19:09         ` Maxwell Doose
  0 siblings, 0 replies; 17+ messages in thread
From: Maxwell Doose @ 2026-04-28 19:09 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: songqiang1304521, dlechner, nuno.sa, andy, linux-iio,
	linux-kernel

On Tue, Apr 28, 2026 at 12:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> I'd prefer a helper function with straight forward guard() + returns.
> Then you can have a single exit point from the outer function.
>
> ret = helper()
> if (ret)
>         return ret;
>
> other stuff.
>

Sounds like a great idea, I'm eager to start when I've got time.

best regards,
maxwell

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2026-04-28 19:09 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-28 12:46 [PATCH 0/4] iio: magnetometer: rm3100: Modernize locking and control flow Maxwell Doose
2026-04-28 12:46 ` [PATCH v2 1/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_read_mag() Maxwell Doose
2026-04-28 13:26   ` Andy Shevchenko
2026-04-28 14:59     ` Maxwell Doose
2026-04-28 15:58       ` Jonathan Cameron
2026-04-28 12:46 ` [PATCH v2 2/4] iio: magnetometer: rm3100: Use scoped_guard() in rm3100_get_samp_freq() Maxwell Doose
2026-04-28 13:26   ` Andy Shevchenko
2026-04-28 13:59     ` Maxwell Doose
2026-04-28 15:24       ` Jonathan Cameron
2026-04-28 17:08         ` Maxwell Doose
2026-04-28 12:46 ` [PATCH v2 3/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_set_samp_freq() Maxwell Doose
2026-04-28 12:46 ` [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler() Maxwell Doose
2026-04-28 15:32   ` Jonathan Cameron
2026-04-28 17:20     ` Maxwell Doose
2026-04-28 17:27     ` Maxwell Doose
2026-04-28 17:39       ` Jonathan Cameron
2026-04-28 19:09         ` Maxwell Doose

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox