Re: [PATCH v6 1/3] iio: magnetometer: bmc150_magn: use automated cleanup for mutex

[David Lechner]

On 2/10/26 5:39 PM, Neel Bullywon wrote:
> Use guard() and scoped_guard() to replace manual mutex lock/unlock
> calls. This simplifies error handling and ensures RAII-style cleanup.
> 
> scoped_guard() is used in read_raw IIO_CHAN_INFO_RAW case for a
> multi-statement mutex-protected block, as well as in remove,
> runtime_suspend, and suspend where a short mutex-protected scope
> is needed for a single function call.
> 
> guard() is used in write_raw and trig_reen where the mutex scope
> extends to the end of the function or case block. Case blocks in
> write_raw are wrapped in braces to ensure clear scope for the
> cleanup guards.
> 
> The trigger_handler function is left unchanged as mixing guard()
> with goto error paths can be fragile.
> 
> Signed-off-by: Neel Bullywon <neelb2403@xxxxxxxxx>
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 130 ++++++++++---------------
>  1 file changed, 52 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index 6a73f6e2f1f0..5eb710eb020d 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -11,6 +11,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/i2c.h>
> +#include <linux/cleanup.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> @@ -456,29 +457,22 @@ static int bmc150_magn_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		if (iio_buffer_enabled(indio_dev))
>  			return -EBUSY;
> -		mutex_lock(&data->mutex);
> -
> -		ret = bmc150_magn_set_power_state(data, true);
> -		if (ret < 0) {
> -			mutex_unlock(&data->mutex);
> -			return ret;
> -		}
> +		scoped_guard(mutex, &data->mutex) {

I am very wary of scoped_guard(), especially in case statements.
There is a hidden for loop in the macro, so if `break;` is used
inside of the scoped_guard() {}, it will only break out of the
guard scope (for loop) and not the case.

It would be better if we could use `guard()` instead (in the style
that Andy suggested).


>  static const struct iio_trigger_ops bmc150_magn_trigger_ops = {
> @@ -980,9 +961,8 @@ void bmc150_magn_remove(struct device *dev)
>  	if (data->dready_trig)
>  		iio_trigger_unregister(data->dready_trig);
>  
> -	mutex_lock(&data->mutex);
> -	bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND, true);
> -	mutex_unlock(&data->mutex);
> +	scoped_guard(mutex, &data->mutex)
> +		bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND, true);
>  
>  	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>  }
> @@ -995,10 +975,9 @@ static int bmc150_magn_runtime_suspend(struct device *dev)
>  	struct bmc150_magn_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&data->mutex);
> -	ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SLEEP,
> -					 true);
> -	mutex_unlock(&data->mutex);
> +	scoped_guard(mutex, &data->mutex)
> +		ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SLEEP,
> +						 true);


This way of using scoped_guard() is OK since there is no { }.


>  	if (ret < 0) {
>  		dev_err(dev, "powering off device failed\n");
>  		return ret;
> @@ -1026,10 +1005,9 @@ static int bmc150_magn_suspend(struct device *dev)
>  	struct bmc150_magn_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&data->mutex);
> -	ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SLEEP,
> -					 true);
> -	mutex_unlock(&data->mutex);
> +	scoped_guard(mutex, &data->mutex)
> +		ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SLEEP,
> +						 true);

This one can be regular guard() and return directly.

>  
>  	return ret;
>  }