Re: [PATCH] iio: gyro: mpu3050: Fix runtime PM refcount leak in trigger enable path

[Jonathan Cameron]

On Fri, 30 Oct 2026 16:43:05 +0800
laixiangwu <laixiangwu@xxxxxxxxxxxxxxxxxxxx> wrote:

> mpu3050_drdy_trigger_set_state() calls pm_runtime_get_sync() when
> enabling the data ready trigger, but all five error returns in the
> enable path return directly without the matching
> pm_runtime_put_autosuspend(). The disable path, in contrast, drops
> the reference correctly.
> 
> As a result, a failed trigger enable leaves the runtime PM usage
> counter incremented and the sensor can never runtime suspend. It also
> leaves hw_irq_trigger set to true, claiming the hardware IRQ is in
> use when it is not.
> 
> Add an error label that drops the runtime PM reference and clears
> hw_irq_trigger, and check the return value of pm_runtime_get_sync()
> like the rest of the driver does.
> 
> Fixes: 3904b28efb2c ("iio: gyro: Add driver for the MPU-3050 gyroscope")
> Signed-off-by: laixiangwu <laixiangwu@xxxxxxxxxxxxxxxxxxxx>
Hi,

> ---
>  drivers/iio/gyro/mpu3050-core.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> index 07c0f9011..9c6d5ba82 100644
> --- a/drivers/iio/gyro/mpu3050-core.c
> +++ b/drivers/iio/gyro/mpu3050-core.c
> @@ -988,20 +988,24 @@ static int mpu3050_drdy_trigger_set_state(struct iio_trigger *trig,
>  		return 0;
>  	} else {
>  		/* Else we're enabling the trigger from this point */
> -		pm_runtime_get_sync(mpu3050->dev);
> +		ret = pm_runtime_get_sync(mpu3050->dev);

Read the documentation of this function.  It suggests what
I think is the appropriate change here. Use pm_runtime_resume_and_get()
not get_sync()

> +		if (ret < 0) {
> +			pm_runtime_put_noidle(mpu3050->dev);
> +			return ret;
> +		}
>  		mpu3050->hw_irq_trigger = true;
>  
>  		/* Disable all things in the FIFO */
>  		ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
>  		if (ret)
> -			return ret;
> +			goto err_put_pm;
>  
>  		/* Reset and enable the FIFO */
>  		ret = regmap_set_bits(mpu3050->map, MPU3050_USR_CTRL,
>  				      MPU3050_USR_CTRL_FIFO_EN |
>  				      MPU3050_USR_CTRL_FIFO_RST);
>  		if (ret)
> -			return ret;
> +			goto err_put_pm;
>  
>  		mpu3050->pending_fifo_footer = false;
>  
> @@ -1013,12 +1017,12 @@ static int mpu3050_drdy_trigger_set_state(struct iio_trigger *trig,
>  				   MPU3050_FIFO_EN_GYRO_ZOUT |
>  				   MPU3050_FIFO_EN_FOOTER);
>  		if (ret)
> -			return ret;
> +			goto err_put_pm;
>  
>  		/* Configure the sample engine */
>  		ret = mpu3050_start_sampling(mpu3050);
>  		if (ret)
> -			return ret;
> +			goto err_put_pm;
>  
>  		/* Clear IRQ flag */
>  		ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
> @@ -1037,10 +1041,16 @@ static int mpu3050_drdy_trigger_set_state(struct iio_trigger *trig,
>  
>  		ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val);
>  		if (ret)
> -			return ret;
> +			goto err_put_pm;
>  	}
>  
>  	return 0;
> +
> +err_put_pm:

I'm not keen on an error block that only applies to one of the two legs
of the if /else in such a large function.  Better here would be to introduce
a pair of helpers for the if and else parts (enable / disable) and call
the appropriate one.  Do that in a precursor patch and then this will
become a more readable change.

> +	pm_runtime_put_autosuspend(mpu3050->dev);
> +	mpu3050->hw_irq_trigger = false;
> +
> +	return ret;
>  }
>  
>  static const struct iio_trigger_ops mpu3050_trigger_ops = {