[PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

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

While replacing mutex_lock() and mutex_unlock(), the critical sections
of rm3100_read_mag() and rm3100_get_samp_freq() have been extended to
include negligible operations for cleaner logic.

Add new helper-wrapper function rm3100_guarded_regmap_bulk_read() to
help keep rm3100_trigger_handler() switch-cases clean while maintaining
mutex locking and avoiding re-entrancy risks from potential callbacks.

While at it, remove redundant gotos where applicable, and use direct
returns instead.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 v2:
 - The following excerpts are from the original cover letter:
 - 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.

 v3:
 - Added new helper-wrapper function rm3100_guarded_regmap_bulk_read(),
   see commit message.
 - Squashed commit into one commit rather than four.

 drivers/iio/magnetometer/rm3100-core.c | 94 ++++++++++++++------------
 1 file changed, 52 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index 2b2884425746..9cad61e1df94 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)					\
@@ -284,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);
+	guard(mutex)(&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];
 
@@ -338,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,
@@ -458,6 +449,28 @@ static const struct iio_buffer_setup_ops rm3100_buffer_ops = {
 	.postdisable = rm3100_buffer_postdisable,
 };
 
+/**
+ * rm3100_guarded_regmap_bulk_read() - Wrapper around regmap_bulk_read() with a mutex
+ *
+ * @lock: Mutex to lock while performing operations
+ * @map: Register map to read from, passed to regmap_bulk_read()
+ * @reg: First register to be read from, passed to regmap_bulk_read()
+ * @val: Pointer to store read value, in native register size for device,
+ * passed to regmap_bulk_read()
+ * @val_count: Number of registers to read, passed to regmap_bulk_read()
+ *
+ * Intended for use only in rm3100_trigger_handler().
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+static int rm3100_guarded_regmap_bulk_read(struct mutex *lock, struct regmap *map,
+					   unsigned int reg, void *val, size_t val_count)
+{
+	guard(mutex)(lock);
+	return regmap_bulk_read(map, reg, val, val_count);
+}
+
 static irqreturn_t rm3100_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -468,11 +481,10 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
 	struct regmap *regmap = data->regmap;
 	int ret, i, bit;
 
-	mutex_lock(&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);
+		ret = rm3100_guarded_regmap_bulk_read(&data->lock, regmap, RM3100_REG_MX2,
+						      data->buffer, 9);
 		if (ret < 0)
 			goto done;
 		/* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for paddings. */
@@ -480,36 +492,34 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
 			memmove(data->buffer + i * 4, data->buffer + i * 3, 3);
 		break;
 	case BIT(0) | BIT(1):
-		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
-		mutex_unlock(&data->lock);
+		ret = rm3100_guarded_regmap_bulk_read(&data->lock, regmap, RM3100_REG_MX2,
+						      data->buffer, 6);
 		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);
+		ret = rm3100_guarded_regmap_bulk_read(&data->lock, regmap, RM3100_REG_MY2,
+						      data->buffer, 6);
 		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);
+		ret = rm3100_guarded_regmap_bulk_read(&data->lock, regmap, RM3100_REG_MX2,
+						      data->buffer, 9);
 		if (ret < 0)
 			goto done;
 		memmove(data->buffer + 4, data->buffer + 6, 3);
 		break;
 	default:
 		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);
+			ret = rm3100_guarded_regmap_bulk_read(&data->lock, regmap,
+							      RM3100_REG_MX2 + 3 * bit,
+							      data->buffer, 3);
+			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

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Andy Shevchenko]

On Tue, Apr 28, 2026 at 05:26:00PM -0500, Maxwell Doose wrote:
> Replace mutex_lock() and mutex_unlock() calls in rm3100-core.c with
> the more modern guard(mutex)() family. This will help modernize the
> driver and bring it up-to-date with modern available macros/functions.
> 
> While replacing mutex_lock() and mutex_unlock(), the critical sections
> of rm3100_read_mag() and rm3100_get_samp_freq() have been extended to
> include negligible operations for cleaner logic.
> 
> Add new helper-wrapper function rm3100_guarded_regmap_bulk_read() to
> help keep rm3100_trigger_handler() switch-cases clean while maintaining
> mutex locking and avoiding re-entrancy risks from potential callbacks.
> 
> While at it, remove redundant gotos where applicable, and use direct
> returns instead.

...

> +/**
> + * rm3100_guarded_regmap_bulk_read() - Wrapper around regmap_bulk_read() with a mutex
> + *
> + * @lock: Mutex to lock while performing operations
> + * @map: Register map to read from, passed to regmap_bulk_read()
> + * @reg: First register to be read from, passed to regmap_bulk_read()
> + * @val: Pointer to store read value, in native register size for device,
> + * passed to regmap_bulk_read()
> + * @val_count: Number of registers to read, passed to regmap_bulk_read()
> + *
> + * Intended for use only in rm3100_trigger_handler().
> + *

I don't mind having kernel-doc for a static function, but if you do that,
validate. Here is "Return section is missing".

Needs to be

 * Return:

> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.

 * A value of zero on success, a negative errno in error cases.

> + */
> +static int rm3100_guarded_regmap_bulk_read(struct mutex *lock, struct regmap *map,

No, supply *data and not a *lock.

> +					   unsigned int reg, void *val, size_t val_count)

static int rm3100_guarded_regmap_bulk_read(struct rm3100_data *data, unsigned int reg,
					   void *val, size_t val_count)
> +{
> +	guard(mutex)(lock);
> +	return regmap_bulk_read(map, reg, val, val_count);

	guard(mutex)(&data->lock);
	return regmap_bulk_read(data->regmap, reg, val, val_count);

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Jonathan Cameron]

On Tue, 28 Apr 2026 17:26:00 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> Replace mutex_lock() and mutex_unlock() calls in rm3100-core.c with
> the more modern guard(mutex)() family. This will help modernize the
> driver and bring it up-to-date with modern available macros/functions.
> 
> While replacing mutex_lock() and mutex_unlock(), the critical sections
> of rm3100_read_mag() and rm3100_get_samp_freq() have been extended to
> include negligible operations for cleaner logic.
> 
> Add new helper-wrapper function rm3100_guarded_regmap_bulk_read() to
> help keep rm3100_trigger_handler() switch-cases clean while maintaining
> mutex locking and avoiding re-entrancy risks from potential callbacks.
> 
> While at it, remove redundant gotos where applicable, and use direct
> returns instead.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
Hi Maxwell.

The use of the helper worked out well but it also made me look
at some existing code and wonder why it is so complex!

>  static irqreturn_t rm3100_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -468,11 +481,10 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
>  	struct regmap *regmap = data->regmap;
>  	int ret, i, bit;
>  
> -	mutex_lock(&data->lock);
>  	switch (scan_mask) {

>  	default:
>  		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);
> +			ret = rm3100_guarded_regmap_bulk_read(&data->lock, regmap,
> +							      RM3100_REG_MX2 + 3 * bit,
> +							      data->buffer, 3);

Hmm. Not one for this patch, but the only reason this doesn't
hammer the lock is the bitmap is one hot.  Which is why the read is always to the same
location in the buffer. That could have been a lot easier to spot if instead
of a loop find_first_bit() was used.

I don't think we can ever get here with no bits set but maybe check that as well
(it's not supposed to be possible as doesn't make any sense to measure nothing ;)
 




> +			if (ret < 0)
>  				goto done;
> -			}
>  		}
> -		mutex_unlock(&data->lock);
>  	}
>  	/*
>  	 * Always using the same buffer so that we wouldn't need to set the

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

Hi Andy,

On Wed, Apr 29, 2026 at 4:12 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> ...
>
> > +/**
> > + * rm3100_guarded_regmap_bulk_read() - Wrapper around regmap_bulk_read() with a mutex
> > + *
> > + * @lock: Mutex to lock while performing operations
> > + * @map: Register map to read from, passed to regmap_bulk_read()
> > + * @reg: First register to be read from, passed to regmap_bulk_read()
> > + * @val: Pointer to store read value, in native register size for device,
> > + * passed to regmap_bulk_read()
> > + * @val_count: Number of registers to read, passed to regmap_bulk_read()
> > + *
> > + * Intended for use only in rm3100_trigger_handler().
> > + *
>
> I don't mind having kernel-doc for a static function, but if you do that,
> validate. Here is "Return section is missing".
>

I'll make sure to add that in the v4, I probably should've been
explicit in that description.
>
> > + */
> > +static int rm3100_guarded_regmap_bulk_read(struct mutex *lock, struct regmap *map,
>
> No, supply *data and not a *lock.
>

Understood, I'll also change the function signature for that in the v4.

best regards,
maxwell

> > +                                        unsigned int reg, void *val, size_t val_count)
>
> static int rm3100_guarded_regmap_bulk_read(struct rm3100_data *data, unsigned int reg,
>                                            void *val, size_t val_count)
> > +{
> > +     guard(mutex)(lock);
> > +     return regmap_bulk_read(map, reg, val, val_count);
>
>         guard(mutex)(&data->lock);
>         return regmap_bulk_read(data->regmap, reg, val, val_count);
>
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

Hi Jonathan,

On Wed, Apr 29, 2026 at 5:42 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Hi Maxwell.
>
> The use of the helper worked out well but it also made me look
> at some existing code and wonder why it is so complex!
>

Glad the helper worked out, I'm also on a similar page with you about
the code complexity. I feel that if there are similar patterns in the
code, we could probably condens those down into either inlined
functions or macros (we want to avoid macros for obvious reasons but I
digress). If I find other patterns that do this kind of:
lock(&my_mutex);
ret = my_function(params);
unlock(&my_mutex);
then it may be a good idea to consider writing a helper-wrapper in a
header that takes in a function pointer and its params. It would take
more thought, and we'd probably have to avoid variadic arguments, but
it may be a good idea just so we can keep the same scope and keep the
logic clean. Just thinking ahead here.

> >  static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> >  {
> >       struct iio_poll_func *pf = p;
> > @@ -468,11 +481,10 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> >       struct regmap *regmap = data->regmap;
> >       int ret, i, bit;
> >
> > -     mutex_lock(&data->lock);
> >       switch (scan_mask) {
>
> >       default:
> >               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);
> > +                     ret = rm3100_guarded_regmap_bulk_read(&data->lock, regmap,
> > +                                                           RM3100_REG_MX2 + 3 * bit,
> > +                                                           data->buffer, 3);
>
> Hmm. Not one for this patch, but the only reason this doesn't
> hammer the lock is the bitmap is one hot.  Which is why the read is always to the same
> location in the buffer. That could have been a lot easier to spot if instead
> of a loop find_first_bit() was used.
>

Thanks for pointing that out, I didn't realize that. I don't have much
experience with the bitops.h macros, but I think that would be sound
for a separate patch. I'd ordinarily include that in this patch, but I
feel at that point we might be deviating from the original purpose of
the patch.

> I don't think we can ever get here with no bits set but maybe check that as well
> (it's not supposed to be possible as doesn't make any sense to measure nothing ;)

I'll consider it for the v4, but I might send it as a separate patch
as well since again, at that point we might be deviating from the
original purpose.

best regards,
maxwell

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 09:26:53AM -0500, Maxwell Doose wrote:
> On Wed, Apr 29, 2026 at 4:12 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:

...

> > > + * rm3100_guarded_regmap_bulk_read() - Wrapper around regmap_bulk_read() with a mutex

And perhaps make the name shorter

rm3100_regmap_bulk_read_locked() // also use more standard pattern with suffix(es).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 10:19:14AM -0500, Maxwell Doose wrote:
> On Wed, Apr 29, 2026 at 5:42 AM Jonathan Cameron <jic23@kernel.org> wrote:

...

> If I find other patterns that do this kind of:
> lock(&my_mutex);
> ret = my_function(params);
> unlock(&my_mutex);
> then it may be a good idea to consider writing a helper-wrapper in a
> header that takes in a function pointer and its params. It would take
> more thought, and we'd probably have to avoid variadic arguments, but
> it may be a good idea just so we can keep the same scope and keep the
> logic clean.

Won't fly. I am pretty sure most of the maintainers find that bad. The problem
is that the caller should be crystal clear to show what the critical section
is. Hiding that information behind macros drastically reduces understanding of
the code,

...

> I think that would be sound
> for a separate patch. I'd ordinarily include that in this patch, but I
> feel at that point we might be deviating from the original purpose of
> the patch.

I agree.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

On Wed, Apr 29, 2026 at 1:22 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Apr 29, 2026 at 09:26:53AM -0500, Maxwell Doose wrote:
> > On Wed, Apr 29, 2026 at 4:12 AM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
>
> ...
>
> > > > + * rm3100_guarded_regmap_bulk_read() - Wrapper around regmap_bulk_read() with a mutex
>
> And perhaps make the name shorter
>
> rm3100_regmap_bulk_read_locked() // also use more standard pattern with suffix(es).
>

Understood, that will come in the v4 as well.

best regards,
maxwell




> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

On Wed, Apr 29, 2026 at 1:26 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> Won't fly. I am pretty sure most of the maintainers find that bad. The problem
> is that the caller should be crystal clear to show what the critical section
> is. Hiding that information behind macros drastically reduces understanding of
> the code,
>

Was just a thought. Even if it were to be implemented, it certainly
wouldn't be a macro.

best regards,
maxwell




>
> > I think that would be sound
> > for a separate patch. I'd ordinarily include that in this patch, but I
> > feel at that point we might be deviating from the original purpose of
> > the patch.
>
> I agree.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 02:00:05PM -0500, Maxwell Doose wrote:
> On Wed, Apr 29, 2026 at 1:26 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > Won't fly. I am pretty sure most of the maintainers find that bad. The problem
> > is that the caller should be crystal clear to show what the critical section
> > is. Hiding that information behind macros drastically reduces understanding of
> > the code,
> 
> Was just a thought. Even if it were to be implemented, it certainly
> wouldn't be a macro.

Still the locking wrapper that takes function as a parameter doesn't smell good
to me. The reason is the same, hiding important details of critical section,
decreases readability. Also long term maintenance might suffer if one needs to
alter that piece of code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

On Wed, Apr 29, 2026 at 2:12 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> Still the locking wrapper that takes function as a parameter doesn't smell good
> to me. The reason is the same, hiding important details of critical section,
> decreases readability. Also long term maintenance might suffer if one needs to
> alter that piece of code.
>

Good point.

best regards,
maxwell






> --
> With Best Regards,
> Andy Shevchenko
>
>