[PATCH v3] iio: gyro: bmg160: Discard pm_runtime_put_autosuspend() return values

[PATCH v3] iio: gyro: bmg160: Discard pm_runtime_put_autosuspend() return values

[Jonathan Cameron]

From: Brian Norris <briannorris@chromium.org>

This driver propagates the return value of pm_runtime_put_autosuspend(),
which is not particularly useful.

Returning an error code from pm_runtime_put_autosuspend() merely means
that it has not queued up a timer or work item to check whether or not
the device can be suspended and there are many perfectly valid
situations in which that can happen, like after writing "on" to the
device's runtime PM "control" attribute in sysfs for one example.

For this reason, modify this driver to simply discard the
pm_runtime_put_autosuspend() return value, and inline
bmg160_set_power_state() now that it behaves the same with and without
CONFIG_PM and is otherwise trivial.

Now the return value of the cleanup is ignored, also switch various
paths to use PM_RUNTIEM_ACQUIRE_AUTOSUSPEND() in conjunction
with guard(mutex)() futher simplifying the code.

While at it, I notice that bmg160_write_event_config() and
bmg160_data_rdy_trigger_set_state() mismanage the runtime PM reference
count intentionally, relying on racy autosuspend behavior for
correctness. I refactor this to manage refcounts properly instead.
I use guard() for the mutex, to make this less error prone.

Additionally, bmg160_write_raw() had one error path where a
pm_runtime_resume_and_get() failure would also drop a runtime PM
refcount (pm_runtime_put_autosuspend()), which potentially leads to
underflow. I fix that.

This will facilitate a planned change of the
pm_runtime_put_autosuspend() return type to void in the future, similar
to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return
type to void").

Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
---
I'm continuing to nibble away at Brian's series applying some of the
additional changes Andy asked for (that Brian quite reasonably stated
were beyond what he was going to take on!)  Just this one today.

v3: Make use of ACQUIRE magic in various paths as enabled
by dropping the return value.

With this posted, I'll mark it changes requested in patchwork.

https://patchwork.kernel.org/project/linux-iio/patch/20261021165858.v2.10.I5918e916cbe48281c13e5734733a3281e7be8841@changeid/

---
 drivers/iio/gyro/bmg160_core.c | 237 +++++++++++++--------------------
 1 file changed, 94 insertions(+), 143 deletions(-)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index b007f46d3fd6..a48e01ecc367 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -8,6 +8,7 @@
 #include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/cleanup.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/iio/iio.h>
@@ -313,30 +314,6 @@ static int bmg160_chip_init(struct bmg160_data *data)
 	return 0;
 }
 
-static int bmg160_set_power_state(struct bmg160_data *data, bool on)
-{
-#ifdef CONFIG_PM
-	struct device *dev = regmap_get_device(data->regmap);
-	int ret;
-
-	if (on)
-		ret = pm_runtime_get_sync(dev);
-	else
-		ret = pm_runtime_put_autosuspend(dev);
-
-	if (ret < 0) {
-		dev_err(dev, "Failed: bmg160_set_power_state for %d\n", on);
-
-		if (on)
-			pm_runtime_put_noidle(dev);
-
-		return ret;
-	}
-#endif
-
-	return 0;
-}
-
 static int bmg160_setup_any_motion_interrupt(struct bmg160_data *data,
 					     bool status)
 {
@@ -502,26 +479,20 @@ static int bmg160_get_temp(struct bmg160_data *data, int *val)
 	int ret;
 	unsigned int raw_val;
 
-	mutex_lock(&data->mutex);
-	ret = bmg160_set_power_state(data, true);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
+	guard(mutex)(&data->mutex);
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+	if (PM_RUNTIME_ACQUIRE_ERR(&pm)) {
+		dev_err(dev, "failed to resume\n");
+		return -ENXIO;
 	}
 
 	ret = regmap_read(data->regmap, BMG160_REG_TEMP, &raw_val);
 	if (ret < 0) {
 		dev_err(dev, "Error reading reg_temp\n");
-		bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
 		return ret;
 	}
 
 	*val = sign_extend32(raw_val, 7);
-	ret = bmg160_set_power_state(data, false);
-	mutex_unlock(&data->mutex);
-	if (ret < 0)
-		return ret;
 
 	return IIO_VAL_INT;
 }
@@ -532,27 +503,21 @@ static int bmg160_get_axis(struct bmg160_data *data, int axis, int *val)
 	int ret;
 	__le16 raw_val;
 
-	mutex_lock(&data->mutex);
-	ret = bmg160_set_power_state(data, true);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
+	guard(mutex)(&data->mutex);
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+	if (PM_RUNTIME_ACQUIRE_ERR(&pm)) {
+		dev_err(dev, "failed to resume\n");
+		return -ENXIO;
 	}
 
 	ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(axis), &raw_val,
 			       sizeof(raw_val));
 	if (ret < 0) {
 		dev_err(dev, "Error reading axis %d\n", axis);
-		bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
 		return ret;
 	}
 
 	*val = sign_extend32(le16_to_cpu(raw_val), 15);
-	ret = bmg160_set_power_state(data, false);
-	mutex_unlock(&data->mutex);
-	if (ret < 0)
-		return ret;
 
 	return IIO_VAL_INT;
 }
@@ -624,11 +589,11 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
 			    int val, int val2, long mask)
 {
 	struct bmg160_data *data = iio_priv(indio_dev);
-	int ret;
+	struct device *dev = regmap_get_device(data->regmap);
 
 	switch (mask) {
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		mutex_lock(&data->mutex);
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		guard(mutex)(&data->mutex);
 		/*
 		 * Section 4.2 of spec
 		 * In suspend mode, the only supported operations are reading
@@ -636,65 +601,41 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
 		 * register. Since we will be in suspend mode by default, change
 		 * mode to power on for other writes.
 		 */
-		ret = bmg160_set_power_state(data, true);
-		if (ret < 0) {
-			mutex_unlock(&data->mutex);
-			return ret;
-		}
-		ret = bmg160_set_bw(data, val);
-		if (ret < 0) {
-			bmg160_set_power_state(data, false);
-			mutex_unlock(&data->mutex);
-			return ret;
+		PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+		if (PM_RUNTIME_ACQUIRE_ERR(&pm)) {
+			dev_err(dev, "failed to resume\n");
+			return -ENXIO;
 		}
-		ret = bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
-	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+
+		return bmg160_set_bw(data, val);
+	}
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
 		if (val2)
 			return -EINVAL;
 
-		mutex_lock(&data->mutex);
-		ret = bmg160_set_power_state(data, true);
-		if (ret < 0) {
-			bmg160_set_power_state(data, false);
-			mutex_unlock(&data->mutex);
-			return ret;
-		}
-		ret = bmg160_set_filter(data, val);
-		if (ret < 0) {
-			bmg160_set_power_state(data, false);
-			mutex_unlock(&data->mutex);
-			return ret;
+		guard(mutex)(&data->mutex);
+		PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+		if (PM_RUNTIME_ACQUIRE_ERR(&pm)) {
+			dev_err(dev, "failed to resume\n");
+			return -ENXIO;
 		}
-		ret = bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
-	case IIO_CHAN_INFO_SCALE:
+		return bmg160_set_filter(data, val);
+	}
+	case IIO_CHAN_INFO_SCALE: {
 		if (val)
 			return -EINVAL;
 
-		mutex_lock(&data->mutex);
-		/* Refer to comments above for the suspend mode ops */
-		ret = bmg160_set_power_state(data, true);
-		if (ret < 0) {
-			mutex_unlock(&data->mutex);
-			return ret;
+		guard(mutex)(&data->mutex);
+		PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+		if (PM_RUNTIME_ACQUIRE_ERR(&pm)) {
+			dev_err(dev, "failed to resume\n");
+			return -ENXIO;
 		}
-		ret = bmg160_set_scale(data, val2);
-		if (ret < 0) {
-			bmg160_set_power_state(data, false);
-			mutex_unlock(&data->mutex);
-			return ret;
-		}
-		ret = bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
+		return bmg160_set_scale(data, val2);
+	}
 	default:
 		return -EINVAL;
 	}
-
-	return -EINVAL;
 }
 
 static int bmg160_read_event(struct iio_dev *indio_dev,
@@ -759,42 +700,37 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev,
 				     bool state)
 {
 	struct bmg160_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 
 	if (state && data->ev_enable_state)
 		return 0;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	if (!state && data->motion_trigger_on) {
 		data->ev_enable_state = 0;
-		mutex_unlock(&data->mutex);
 		return 0;
 	}
-	/*
-	 * We will expect the enable and disable to do operation
-	 * in reverse order. This will happen here anyway as our
-	 * resume operation uses sync mode runtime pm calls, the
-	 * suspend operation will be delayed by autosuspend delay
-	 * So the disable operation will still happen in reverse of
-	 * enable operation. When runtime pm is disabled the mode
-	 * is always on so sequence doesn't matter
-	 */
-	ret = bmg160_set_power_state(data, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+	if (state) {
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret < 0) {
+			dev_err(dev, "failed to resume: %d\n", ret);
+			return ret;
+		}
 
-	ret =  bmg160_setup_any_motion_interrupt(data, state);
-	if (ret < 0) {
-		bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+		ret = bmg160_setup_any_motion_interrupt(data, true);
+		if (ret < 0) {
+			pm_runtime_put_autosuspend(dev);
+			return ret;
+		}
+	} else {
+		ret = bmg160_setup_any_motion_interrupt(data, false);
+		if (ret < 0)
+			return ret;
 
-	data->ev_enable_state = state;
-	mutex_unlock(&data->mutex);
+		pm_runtime_put_autosuspend(dev);
+	}
 
 	return 0;
 }
@@ -929,41 +865,47 @@ static int bmg160_data_rdy_trigger_set_state(struct iio_trigger *trig,
 {
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
 	struct bmg160_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	if (!state && data->ev_enable_state && data->motion_trigger_on) {
 		data->motion_trigger_on = false;
-		mutex_unlock(&data->mutex);
 		return 0;
 	}
 
-	/*
-	 * Refer to comment in bmg160_write_event_config for
-	 * enable/disable operation order
-	 */
-	ret = bmg160_set_power_state(data, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
-	if (data->motion_trig == trig)
-		ret =  bmg160_setup_any_motion_interrupt(data, state);
-	else
-		ret = bmg160_setup_new_data_interrupt(data, state);
-	if (ret < 0) {
-		bmg160_set_power_state(data, false);
-		mutex_unlock(&data->mutex);
-		return ret;
+	if (state) {
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret < 0) {
+			dev_err(dev, "failed to resume: %d\n", ret);
+			return ret;
+		}
+
+		if (data->motion_trig == trig)
+			ret = bmg160_setup_any_motion_interrupt(data, true);
+		else
+			ret = bmg160_setup_new_data_interrupt(data, true);
+		if (ret < 0) {
+			pm_runtime_put_autosuspend(dev);
+			return ret;
+		}
+	} else {
+		if (data->motion_trig == trig)
+			ret = bmg160_setup_any_motion_interrupt(data, false);
+		else
+			ret = bmg160_setup_new_data_interrupt(data, false);
+		if (ret < 0)
+			return ret;
+
+		pm_runtime_put_autosuspend(dev);
 	}
+
 	if (data->motion_trig == trig)
 		data->motion_trigger_on = state;
 	else
 		data->dready_trigger_on = state;
 
-	mutex_unlock(&data->mutex);
-
 	return 0;
 }
 
@@ -1046,15 +988,24 @@ static irqreturn_t bmg160_data_rdy_trig_poll(int irq, void *private)
 static int bmg160_buffer_preenable(struct iio_dev *indio_dev)
 {
 	struct bmg160_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		dev_err(dev, "failed to resume: %d\n", ret);
 
-	return bmg160_set_power_state(data, true);
+	return ret;
 }
 
 static int bmg160_buffer_postdisable(struct iio_dev *indio_dev)
 {
 	struct bmg160_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
 
-	return bmg160_set_power_state(data, false);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
 }
 
 static const struct iio_buffer_setup_ops bmg160_buffer_setup_ops = {
-- 
2.56.0

Re: [PATCH v3] iio: gyro: bmg160: Discard pm_runtime_put_autosuspend() return values

[Brian Norris]

Hi Jonathan,

On Mon, Nov 02, 2026 at 01:48:30AM +0000, Jonathan Cameron wrote:
> From: Brian Norris <briannorris@chromium.org>
> 
> This driver propagates the return value of pm_runtime_put_autosuspend(),
> which is not particularly useful.
> 
> Returning an error code from pm_runtime_put_autosuspend() merely means
> that it has not queued up a timer or work item to check whether or not
> the device can be suspended and there are many perfectly valid
> situations in which that can happen, like after writing "on" to the
> device's runtime PM "control" attribute in sysfs for one example.
> 
> For this reason, modify this driver to simply discard the
> pm_runtime_put_autosuspend() return value, and inline
> bmg160_set_power_state() now that it behaves the same with and without
> CONFIG_PM and is otherwise trivial.
> 
> Now the return value of the cleanup is ignored, also switch various
> paths to use PM_RUNTIEM_ACQUIRE_AUTOSUSPEND() in conjunction
> with guard(mutex)() futher simplifying the code.
> 
> While at it, I notice that bmg160_write_event_config() and
> bmg160_data_rdy_trigger_set_state() mismanage the runtime PM reference
> count intentionally, relying on racy autosuspend behavior for
> correctness. I refactor this to manage refcounts properly instead.
> I use guard() for the mutex, to make this less error prone.
> 
> Additionally, bmg160_write_raw() had one error path where a
> pm_runtime_resume_and_get() failure would also drop a runtime PM
> refcount (pm_runtime_put_autosuspend()), which potentially leads to
> underflow. I fix that.
> 
> This will facilitate a planned change of the
> pm_runtime_put_autosuspend() return type to void in the future, similar
> to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return
> type to void").
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> ---
> I'm continuing to nibble away at Brian's series applying some of the
> additional changes Andy asked for (that Brian quite reasonably stated
> were beyond what he was going to take on!)  Just this one today.
> 
> v3: Make use of ACQUIRE magic in various paths as enabled
> by dropping the return value.
> 
> With this posted, I'll mark it changes requested in patchwork.
> 
> https://patchwork.kernel.org/project/linux-iio/patch/20261021165858.v2.10.I5918e916cbe48281c13e5734733a3281e7be8841@changeid/
> 
> ---
>  drivers/iio/gyro/bmg160_core.c | 237 +++++++++++++--------------------
>  1 file changed, 94 insertions(+), 143 deletions(-)
> 
> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> index b007f46d3fd6..a48e01ecc367 100644
> --- a/drivers/iio/gyro/bmg160_core.c
> +++ b/drivers/iio/gyro/bmg160_core.c


> @@ -759,42 +700,37 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev,
>  				     bool state)
>  {
>  	struct bmg160_data *data = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(data->regmap);
>  	int ret;
>  
>  	if (state && data->ev_enable_state)
>  		return 0;
>  
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  
>  	if (!state && data->motion_trigger_on) {
>  		data->ev_enable_state = 0;
> -		mutex_unlock(&data->mutex);
>  		return 0;
>  	}
> -	/*
> -	 * We will expect the enable and disable to do operation
> -	 * in reverse order. This will happen here anyway as our
> -	 * resume operation uses sync mode runtime pm calls, the
> -	 * suspend operation will be delayed by autosuspend delay
> -	 * So the disable operation will still happen in reverse of
> -	 * enable operation. When runtime pm is disabled the mode
> -	 * is always on so sequence doesn't matter
> -	 */
> -	ret = bmg160_set_power_state(data, state);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +	if (state) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to resume: %d\n", ret);
> +			return ret;
> +		}
>  
> -	ret =  bmg160_setup_any_motion_interrupt(data, state);
> -	if (ret < 0) {
> -		bmg160_set_power_state(data, false);
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> +		ret = bmg160_setup_any_motion_interrupt(data, true);
> +		if (ret < 0) {
> +			pm_runtime_put_autosuspend(dev);
> +			return ret;
> +		}
> +	} else {
> +		ret = bmg160_setup_any_motion_interrupt(data, false);
> +		if (ret < 0)
> +			return ret;
>  
> -	data->ev_enable_state = state;

This is an unexpected (to me) delta vs v2: you've stopped setting
data->ev_enable_state here. I think that's a regression.

This is also one reason I think it's unwise to make so many changes
together here. One person's "churn" (to apply several patches to similar
code) is another person's "hard to review / easy to make mistakes".

Brian

> -	mutex_unlock(&data->mutex);
> +		pm_runtime_put_autosuspend(dev);
> +	}
>  
>  	return 0;
>  }