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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Feb 2026 10:33:25 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Sun, Feb 15, 2026 at 08:54:52PM -0500, Neel Bullywon wrote:
> > Use guard() and scoped_guard() to replace manual mutex lock/unlock
> > calls. This simplifies error handling and ensures RAII-style cleanup.
> > 
> > guard() is used in read_raw, write_raw, trig_reen, trigger_set_state,
> > suspend, and resume. Case blocks using guard() in read_raw and
> > write_raw are wrapped in braces at the case label level to ensure
> > clear scope for the cleanup guards.
> > 
> > scoped_guard() is used in remove and runtime_suspend where a short
> > mutex-protected scope is needed for a single function call.
> > 
> > The trigger_handler function is left unchanged as mixing guard() with
> > goto error paths can be fragile.  
> 
> ...
> 
> > -	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);  
> 
> Wonder if we can move this to a helper:
> 
> static int bmc150_magn_set_power_mode_locked(data, mode) // locked or unlocked, dunno with proper naming
> {
> 	guard(mutex)(&data->mutex)
> 	return bmc150_magn_set_power_mode(data, mode, true);
> }
> 
> ...
Whilst it involves taking the mutex in probe() before it's strictly
necessary (as everything is serialized anyway until we register the
userspace interfaces, I wonder if a simpler option is to rely on the
mutex_init() being early enough and just put the guard unconditionally
in bmc150_magn_set_power_mode()?

The snag is in runtime PM.  The locking in there is really odd
(smells broken to me)
It requires the lock to already be held for resume, but must not
be held for runtime_suspend.  Superficially I suspect this only works
because the autosuspend delay gets us past the unlock in suspend path.

I think we can unwind that mess though by changing read_raw() to
not hold the lock across both power and reading phases (I don't think
it matters if we drop it and grab it again in quick succession as
read_raw is never really a fast path).

The usage of runtime pm in buffer_preenable / postdisable should be
fine but at this point I'm getting nervous.  Neel, do you have the
hardware for this one to check we don't break anything trying to clean
this up?  Or does anyone else who is willing to test any changes?

Jonathan



> 
> > -	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);
> >  	if (ret < 0) {
> >  		dev_err(dev, "powering off device failed\n");
> >  		return ret;
> >  	}  
> 
> Ditto.
> 
> 
> ...
> 
> >  {
> >  	struct iio_dev *indio_dev = dev_get_drvdata(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);
> >  
> > -	return ret;
> > +	guard(mutex)(&data->mutex);
> > +	return bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SLEEP,
> > +					  true);  
> 
> Ditto.
> 
> >  }
> >  
> >  static int bmc150_magn_resume(struct device *dev)
> >  {
> >  	struct iio_dev *indio_dev = dev_get_drvdata(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_NORMAL,
> > -					 true);
> > -	mutex_unlock(&data->mutex);
> >  
> > -	return ret;
> > +	guard(mutex)(&data->mutex);
> > +	return bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
> > +					  true);  
> 
> Ditto.
> 
> >  }  
>