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

[Brian Norris <briannorris@chromium.org>]

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;
>  }