Re: [PATCH v2 4/4] iio: magnetometer: rm3100: Use guard(mutex)() in rm3100_trigger_handler()

[Jonathan Cameron]

On Tue, 28 Apr 2026 07:46:44 -0500
Maxwell Doose <m32285159@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>   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