[PATCH v3 0/2] iio: gyro: mpu3050: Fix runtime PM refcount leak in trigger enable path

[PATCH v3 0/2] iio: gyro: mpu3050: Fix runtime PM refcount leak in trigger enable path

[Xiangwu Lai]

From: laixiangwu <laixiangwu@cmss.chinamobile.com>

Hi Jonathan,

This is a repost of the v2 series as a new top-level thread, as
requested. There are no code changes since v2.

For context, this series fixes a runtime PM refcount leak in the
mpu3050 data ready trigger enable path: patch 1 splits the enable
and disable paths out of mpu3050_drdy_trigger_set_state() into
separate helpers, and patch 2 applies the fix on top, using
pm_runtime_resume_and_get() instead of pm_runtime_get_sync(), which
also keeps the usage counter balanced when the resume fails.

Changes in v3:
- no code changes, the series is reposted as a new top-level thread
  (v2 was mistakenly sent as a reply to the review comments).

Changes in v2:
- split the enable and disable paths out of
  mpu3050_drdy_trigger_set_state() into a precursor patch, as
  suggested by the reviewer.
- use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(),
  as suggested by the reviewer.

v2: https://lore.kernel.org/r/20261101092327.197-1-laixiangwu@cmss.chinamobile.com/
v1: https://lore.kernel.org/r/20261030084305.1660-1-laixiangwu@cmss.chinamobile.com/

laixiangwu (2):
  iio: gyro: mpu3050: Split data ready trigger enable/disable into
    helpers
  iio: gyro: mpu3050: Fix runtime PM refcount leak in trigger enable
    path

 drivers/iio/gyro/mpu3050-core.c | 182 +++++++++++++++++++-------------
 1 file changed, 108 insertions(+), 74 deletions(-)

-- 
2.50.1.windows.1

[PATCH v3 1/2] iio: gyro: mpu3050: Split data ready trigger enable/disable into helpers

[Xiangwu Lai]

From: laixiangwu <laixiangwu@cmss.chinamobile.com>

mpu3050_drdy_trigger_set_state() implements both the enable and the
disable path of the data ready trigger in the two legs of a large
if/else statement.

Split the two legs out into separate mpu3050_drdy_trigger_enable()
and mpu3050_drdy_trigger_disable() helpers and reduce
mpu3050_drdy_trigger_set_state() to dispatching to the appropriate
one. This is a pure code move with no functional change, done to make
the error handling of the enable path easier to read.

Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
---

Changes in v3:
- none; reposted in a new top-level thread.

Link: https://lore.kernel.org/r/20261101092327.197-2-laixiangwu@cmss.chinamobile.com
 drivers/iio/gyro/mpu3050-core.c | 173 ++++++++++++++++++--------------
 1 file changed, 99 insertions(+), 74 deletions(-)

diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
index 07c0f9011..d966911ab 100644
--- a/drivers/iio/gyro/mpu3050-core.c
+++ b/drivers/iio/gyro/mpu3050-core.c
@@ -946,103 +946,128 @@ static irqreturn_t mpu3050_irq_thread(int irq, void *p)
 }
 
 /**
- * mpu3050_drdy_trigger_set_state() - set data ready interrupt state
- * @trig: trigger instance
- * @enable: true if trigger should be enabled, false to disable
+ * mpu3050_drdy_trigger_enable() - enable the data ready trigger
+ * @mpu3050: device to configure
+ *
+ * Power up the device, enable the FIFO and configure the data ready
+ * interrupt.
  */
-static int mpu3050_drdy_trigger_set_state(struct iio_trigger *trig,
-					  bool enable)
+static int mpu3050_drdy_trigger_enable(struct mpu3050 *mpu3050)
 {
-	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
-	struct mpu3050 *mpu3050 = iio_priv(indio_dev);
 	unsigned int val;
 	int ret;
 
-	/* Disabling trigger: disable interrupt and return */
-	if (!enable) {
-		/* Disable all interrupts */
-		ret = regmap_write(mpu3050->map,
-				   MPU3050_INT_CFG,
-				   0);
-		if (ret)
-			dev_err(mpu3050->dev, "error disabling IRQ\n");
+	pm_runtime_get_sync(mpu3050->dev);
+	mpu3050->hw_irq_trigger = true;
 
-		/* Clear IRQ flag */
-		ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
-		if (ret)
-			dev_err(mpu3050->dev, "error clearing IRQ status\n");
+	/* Disable all things in the FIFO */
+	ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
+	if (ret)
+		return ret;
 
-		/* Disable all things in the FIFO and reset it */
-		ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
-		if (ret)
-			dev_err(mpu3050->dev, "error disabling FIFO\n");
+	/* 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;
 
-		ret = regmap_write(mpu3050->map, MPU3050_USR_CTRL,
-				   MPU3050_USR_CTRL_FIFO_RST);
-		if (ret)
-			dev_err(mpu3050->dev, "error resetting FIFO\n");
+	mpu3050->pending_fifo_footer = false;
 
-		pm_runtime_put_autosuspend(mpu3050->dev);
-		mpu3050->hw_irq_trigger = false;
+	/* Turn on the FIFO for temp+X+Y+Z */
+	ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN,
+			   MPU3050_FIFO_EN_TEMP_OUT |
+			   MPU3050_FIFO_EN_GYRO_XOUT |
+			   MPU3050_FIFO_EN_GYRO_YOUT |
+			   MPU3050_FIFO_EN_GYRO_ZOUT |
+			   MPU3050_FIFO_EN_FOOTER);
+	if (ret)
+		return ret;
 
-		return 0;
-	} else {
-		/* Else we're enabling the trigger from this point */
-		pm_runtime_get_sync(mpu3050->dev);
-		mpu3050->hw_irq_trigger = true;
+	/* Configure the sample engine */
+	ret = mpu3050_start_sampling(mpu3050);
+	if (ret)
+		return ret;
 
-		/* Disable all things in the FIFO */
-		ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
-		if (ret)
-			return ret;
+	/* Clear IRQ flag */
+	ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
+	if (ret)
+		dev_err(mpu3050->dev, "error clearing IRQ status\n");
 
-		/* 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;
+	/* Give us interrupts whenever there is new data ready */
+	val = MPU3050_INT_RAW_RDY_EN;
 
-		mpu3050->pending_fifo_footer = false;
+	if (mpu3050->irq_actl)
+		val |= MPU3050_INT_ACTL;
+	if (mpu3050->irq_latch)
+		val |= MPU3050_INT_LATCH_EN;
+	if (mpu3050->irq_opendrain)
+		val |= MPU3050_INT_OPEN;
 
-		/* Turn on the FIFO for temp+X+Y+Z */
-		ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN,
-				   MPU3050_FIFO_EN_TEMP_OUT |
-				   MPU3050_FIFO_EN_GYRO_XOUT |
-				   MPU3050_FIFO_EN_GYRO_YOUT |
-				   MPU3050_FIFO_EN_GYRO_ZOUT |
-				   MPU3050_FIFO_EN_FOOTER);
-		if (ret)
-			return ret;
+	ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val);
+	if (ret)
+		return ret;
 
-		/* Configure the sample engine */
-		ret = mpu3050_start_sampling(mpu3050);
-		if (ret)
-			return ret;
+	return 0;
+}
 
-		/* Clear IRQ flag */
-		ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
-		if (ret)
-			dev_err(mpu3050->dev, "error clearing IRQ status\n");
+/**
+ * mpu3050_drdy_trigger_disable() - disable the data ready trigger
+ * @mpu3050: device to configure
+ *
+ * Disable the data ready interrupt, flush the FIFO and drop the
+ * runtime PM reference taken in mpu3050_drdy_trigger_enable().
+ */
+static int mpu3050_drdy_trigger_disable(struct mpu3050 *mpu3050)
+{
+	unsigned int val;
+	int ret;
 
-		/* Give us interrupts whenever there is new data ready */
-		val = MPU3050_INT_RAW_RDY_EN;
+	/* Disable all interrupts */
+	ret = regmap_write(mpu3050->map,
+			   MPU3050_INT_CFG,
+			   0);
+	if (ret)
+		dev_err(mpu3050->dev, "error disabling IRQ\n");
 
-		if (mpu3050->irq_actl)
-			val |= MPU3050_INT_ACTL;
-		if (mpu3050->irq_latch)
-			val |= MPU3050_INT_LATCH_EN;
-		if (mpu3050->irq_opendrain)
-			val |= MPU3050_INT_OPEN;
+	/* Clear IRQ flag */
+	ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
+	if (ret)
+		dev_err(mpu3050->dev, "error clearing IRQ status\n");
 
-		ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val);
-		if (ret)
-			return ret;
-	}
+	/* Disable all things in the FIFO and reset it */
+	ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
+	if (ret)
+		dev_err(mpu3050->dev, "error disabling FIFO\n");
+
+	ret = regmap_write(mpu3050->map, MPU3050_USR_CTRL,
+			   MPU3050_USR_CTRL_FIFO_RST);
+	if (ret)
+		dev_err(mpu3050->dev, "error resetting FIFO\n");
+
+	pm_runtime_put_autosuspend(mpu3050->dev);
+	mpu3050->hw_irq_trigger = false;
 
 	return 0;
 }
 
+/**
+ * mpu3050_drdy_trigger_set_state() - set data ready interrupt state
+ * @trig: trigger instance
+ * @enable: true if trigger should be enabled, false to disable
+ */
+static int mpu3050_drdy_trigger_set_state(struct iio_trigger *trig,
+					  bool enable)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct mpu3050 *mpu3050 = iio_priv(indio_dev);
+
+	if (enable)
+		return mpu3050_drdy_trigger_enable(mpu3050);
+
+	return mpu3050_drdy_trigger_disable(mpu3050);
+}
+
 static const struct iio_trigger_ops mpu3050_trigger_ops = {
 	.set_trigger_state = mpu3050_drdy_trigger_set_state,
 };
-- 
2.50.1.windows.1

Re: [PATCH v3 1/2] iio: gyro: mpu3050: Split data ready trigger enable/disable into helpers

[Jonathan Cameron]

On Tue,  3 Nov 2026 17:20:01 +0800
Xiangwu Lai <laixiangwu@cmss.chinamobile.com> wrote:

> From: laixiangwu <laixiangwu@cmss.chinamobile.com>
> 
> mpu3050_drdy_trigger_set_state() implements both the enable and the
> disable path of the data ready trigger in the two legs of a large
> if/else statement.
> 
> Split the two legs out into separate mpu3050_drdy_trigger_enable()
> and mpu3050_drdy_trigger_disable() helpers and reduce
> mpu3050_drdy_trigger_set_state() to dispatching to the appropriate
> one. This is a pure code move with no functional change, done to make
> the error handling of the enable path easier to read.
> 
> Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>

Hi. You have definitely picked some ugly code to work on!



> ---
> 
> Changes in v3:
> - none; reposted in a new top-level thread.
> 
> Link: https://lore.kernel.org/r/20261101092327.197-2-laixiangwu@cmss.chinamobile.com
>  drivers/iio/gyro/mpu3050-core.c | 173 ++++++++++++++++++--------------
>  1 file changed, 99 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> index 07c0f9011..d966911ab 100644
> --- a/drivers/iio/gyro/mpu3050-core.c
> +++ b/drivers/iio/gyro/mpu3050-core.c
> @@ -946,103 +946,128 @@ static irqreturn_t mpu3050_irq_thread(int irq, void *p)
>  }
>  
>  /**
> - * mpu3050_drdy_trigger_set_state() - set data ready interrupt state
> - * @trig: trigger instance
> - * @enable: true if trigger should be enabled, false to disable
> + * mpu3050_drdy_trigger_enable() - enable the data ready trigger
> + * @mpu3050: device to configure
> + *
> + * Power up the device, enable the FIFO and configure the data ready
> + * interrupt.
>   */
> -static int mpu3050_drdy_trigger_set_state(struct iio_trigger *trig,
> -					  bool enable)
> +static int mpu3050_drdy_trigger_enable(struct mpu3050 *mpu3050)
>  {
> -	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> -	struct mpu3050 *mpu3050 = iio_priv(indio_dev);
>  	unsigned int val;
>  	int ret;
>  
> -	/* Disabling trigger: disable interrupt and return */
> -	if (!enable) {
> -		/* Disable all interrupts */
> -		ret = regmap_write(mpu3050->map,
> -				   MPU3050_INT_CFG,
> -				   0);
> -		if (ret)
> -			dev_err(mpu3050->dev, "error disabling IRQ\n");
> +	pm_runtime_get_sync(mpu3050->dev);
> +	mpu3050->hw_irq_trigger = true;
>  
> -		/* Clear IRQ flag */
> -		ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
> -		if (ret)
> -			dev_err(mpu3050->dev, "error clearing IRQ status\n");
> +	/* Disable all things in the FIFO */
> +	ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
> +	if (ret)
> +		return ret;
>  
> -		/* Disable all things in the FIFO and reset it */
> -		ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
> -		if (ret)
> -			dev_err(mpu3050->dev, "error disabling FIFO\n");
> +	/* 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;
>  
> -		ret = regmap_write(mpu3050->map, MPU3050_USR_CTRL,
> -				   MPU3050_USR_CTRL_FIFO_RST);
> -		if (ret)
> -			dev_err(mpu3050->dev, "error resetting FIFO\n");
> +	mpu3050->pending_fifo_footer = false;
>  
> -		pm_runtime_put_autosuspend(mpu3050->dev);
> -		mpu3050->hw_irq_trigger = false;
> +	/* Turn on the FIFO for temp+X+Y+Z */
> +	ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN,
> +			   MPU3050_FIFO_EN_TEMP_OUT |
> +			   MPU3050_FIFO_EN_GYRO_XOUT |
> +			   MPU3050_FIFO_EN_GYRO_YOUT |
> +			   MPU3050_FIFO_EN_GYRO_ZOUT |
> +			   MPU3050_FIFO_EN_FOOTER);
> +	if (ret)
> +		return ret;
>  
> -		return 0;
> -	} else {
> -		/* Else we're enabling the trigger from this point */
> -		pm_runtime_get_sync(mpu3050->dev);
> -		mpu3050->hw_irq_trigger = true;
> +	/* Configure the sample engine */
> +	ret = mpu3050_start_sampling(mpu3050);
> +	if (ret)
> +		return ret;
>  
> -		/* Disable all things in the FIFO */
> -		ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
> -		if (ret)
> -			return ret;
> +	/* Clear IRQ flag */
> +	ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
> +	if (ret)
> +		dev_err(mpu3050->dev, "error clearing IRQ status\n");

The original code indeed didn't do anything beyond printing this
error but to me that seems random and inconsistent.  So should
probably be tidied up as part of this series. 

>  
> -		/* 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;
> +	/* Give us interrupts whenever there is new data ready */
> +	val = MPU3050_INT_RAW_RDY_EN;
>  
> -		mpu3050->pending_fifo_footer = false;
> +	if (mpu3050->irq_actl)
> +		val |= MPU3050_INT_ACTL;
> +	if (mpu3050->irq_latch)
> +		val |= MPU3050_INT_LATCH_EN;
> +	if (mpu3050->irq_opendrain)
> +		val |= MPU3050_INT_OPEN;
>  
> -		/* Turn on the FIFO for temp+X+Y+Z */
> -		ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN,
> -				   MPU3050_FIFO_EN_TEMP_OUT |
> -				   MPU3050_FIFO_EN_GYRO_XOUT |
> -				   MPU3050_FIFO_EN_GYRO_YOUT |
> -				   MPU3050_FIFO_EN_GYRO_ZOUT |
> -				   MPU3050_FIFO_EN_FOOTER);
> -		if (ret)
> -			return ret;
> +	ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val);
> +	if (ret)
> +		return ret;
>  
> -		/* Configure the sample engine */
> -		ret = mpu3050_start_sampling(mpu3050);
> -		if (ret)
> -			return ret;
> +	return 0;
> +}
>  
> -		/* Clear IRQ flag */
> -		ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
> -		if (ret)
> -			dev_err(mpu3050->dev, "error clearing IRQ status\n");
> +/**
> + * mpu3050_drdy_trigger_disable() - disable the data ready trigger
> + * @mpu3050: device to configure
> + *
> + * Disable the data ready interrupt, flush the FIFO and drop the
> + * runtime PM reference taken in mpu3050_drdy_trigger_enable().
> + */
> +static int mpu3050_drdy_trigger_disable(struct mpu3050 *mpu3050)
> +{
> +	unsigned int val;
> +	int ret;
>  
> -		/* Give us interrupts whenever there is new data ready */
> -		val = MPU3050_INT_RAW_RDY_EN;
> +	/* Disable all interrupts */
> +	ret = regmap_write(mpu3050->map,
> +			   MPU3050_INT_CFG,
> +			   0);
> +	if (ret)
> +		dev_err(mpu3050->dev, "error disabling IRQ\n");
>  
> -		if (mpu3050->irq_actl)
> -			val |= MPU3050_INT_ACTL;
> -		if (mpu3050->irq_latch)
> -			val |= MPU3050_INT_LATCH_EN;
> -		if (mpu3050->irq_opendrain)
> -			val |= MPU3050_INT_OPEN;
> +	/* Clear IRQ flag */
> +	ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
> +	if (ret)
> +		dev_err(mpu3050->dev, "error clearing IRQ status\n");
>  
> -		ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val);
> -		if (ret)
> -			return ret;
> -	}
> +	/* Disable all things in the FIFO and reset it */
> +	ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
> +	if (ret)
> +		dev_err(mpu3050->dev, "error disabling FIFO\n");
> +
> +	ret = regmap_write(mpu3050->map, MPU3050_USR_CTRL,
> +			   MPU3050_USR_CTRL_FIFO_RST);
> +	if (ret)
> +		dev_err(mpu3050->dev, "error resetting FIFO\n");
Obviously fine in this patch as it's a direct refactor, but
shouldn't a failure to disable the trigger result in an error return?

might be later in the series.
Now the error return doesn't propogate far in the iio core.
That's intentional because cleaning up this sort of failure
to disable is hard and rarely useful.  I'd still expect the
driver to not be making that decision however.

I guess maybe it's not worth the effort to fix this.


> +
> +	pm_runtime_put_autosuspend(mpu3050->dev);
> +	mpu3050->hw_irq_trigger = false;
>  
>  	return 0;
>  }
>  
> +/**
> + * mpu3050_drdy_trigger_set_state() - set data ready interrupt state
> + * @trig: trigger instance
> + * @enable: true if trigger should be enabled, false to disable
> + */
> +static int mpu3050_drdy_trigger_set_state(struct iio_trigger *trig,
> +					  bool enable)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct mpu3050 *mpu3050 = iio_priv(indio_dev);
> +
> +	if (enable)
> +		return mpu3050_drdy_trigger_enable(mpu3050);
> +
Personally I'd use an else to give a visual clue that this is
an either / or situation.

> +	return mpu3050_drdy_trigger_disable(mpu3050);
> +}
> +
>  static const struct iio_trigger_ops mpu3050_trigger_ops = {
>  	.set_trigger_state = mpu3050_drdy_trigger_set_state,
>  };

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

[Xiangwu Lai]

From: laixiangwu <laixiangwu@cmss.chinamobile.com>

mpu3050_drdy_trigger_enable() 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.

Use pm_runtime_resume_and_get(), which does not leave the usage
counter incremented when resume fails, and add an error label that
drops the runtime PM reference and clears hw_irq_trigger so that a
failed enable returns with the device in the same state as before
the attempt.

Fixes: 3904b28efb2c ("iio: gyro: Add driver for the MPU-3050 gyroscope")
Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
---

Changes in v3:
- none; reposted in a new top-level thread.

Changes in v2:
- Rebase the fix on a new precursor patch that splits the enable and
  disable paths of mpu3050_drdy_trigger_set_state() into separate
  helpers, as suggested by the reviewer, so the error label only
  applies to the enable path.
- Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(),
  as suggested by the reviewer; it also keeps the usage counter
  balanced when the resume fails, so no manual
  pm_runtime_put_noidle() is needed in the caller.

Link: https://lore.kernel.org/r/20261101092327.197-3-laixiangwu@cmss.chinamobile.com
Link: https://lore.kernel.org/r/20261030084305.1660-1-laixiangwu@cmss.chinamobile.com
 drivers/iio/gyro/mpu3050-core.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
index d966911ab..fc305564d 100644
--- a/drivers/iio/gyro/mpu3050-core.c
+++ b/drivers/iio/gyro/mpu3050-core.c
@@ -957,20 +957,23 @@ static int mpu3050_drdy_trigger_enable(struct mpu3050 *mpu3050)
 	unsigned int val;
 	int ret;
 
-	pm_runtime_get_sync(mpu3050->dev);
+	ret = pm_runtime_resume_and_get(mpu3050->dev);
+	if (ret)
+		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;
 
@@ -982,12 +985,12 @@ static int mpu3050_drdy_trigger_enable(struct mpu3050 *mpu3050)
 			   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);
@@ -1006,9 +1009,15 @@ static int mpu3050_drdy_trigger_enable(struct mpu3050 *mpu3050)
 
 	ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val);
 	if (ret)
-		return ret;
+		goto err_put_pm;
 
 	return 0;
+
+err_put_pm:
+	pm_runtime_put_autosuspend(mpu3050->dev);
+	mpu3050->hw_irq_trigger = false;
+
+	return ret;
 }
 
 /**
-- 
2.50.1.windows.1

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

[Jonathan Cameron]

On Tue,  3 Nov 2026 17:20:02 +0800
Xiangwu Lai <laixiangwu@cmss.chinamobile.com> wrote:

> From: laixiangwu <laixiangwu@cmss.chinamobile.com>
> 
> mpu3050_drdy_trigger_enable() 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.
> 
> Use pm_runtime_resume_and_get(), which does not leave the usage
> counter incremented when resume fails, and add an error label that
> drops the runtime PM reference and clears hw_irq_trigger so that a
> failed enable returns with the device in the same state as before
> the attempt.
> 
> Fixes: 3904b28efb2c ("iio: gyro: Add driver for the MPU-3050 gyroscope")
> Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>

Other than the missing error return I called out in the previous patch this
looks fine.

> ---
> 
> Changes in v3:
> - none; reposted in a new top-level thread.
> 
> Changes in v2:
> - Rebase the fix on a new precursor patch that splits the enable and
>   disable paths of mpu3050_drdy_trigger_set_state() into separate
>   helpers, as suggested by the reviewer, so the error label only
>   applies to the enable path.
> - Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(),
>   as suggested by the reviewer; it also keeps the usage counter
>   balanced when the resume fails, so no manual
>   pm_runtime_put_noidle() is needed in the caller.
> 
> Link: https://lore.kernel.org/r/20261101092327.197-3-laixiangwu@cmss.chinamobile.com
> Link: https://lore.kernel.org/r/20261030084305.1660-1-laixiangwu@cmss.chinamobile.com
>  drivers/iio/gyro/mpu3050-core.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> index d966911ab..fc305564d 100644
> --- a/drivers/iio/gyro/mpu3050-core.c
> +++ b/drivers/iio/gyro/mpu3050-core.c
> @@ -957,20 +957,23 @@ static int mpu3050_drdy_trigger_enable(struct mpu3050 *mpu3050)
>  	unsigned int val;
>  	int ret;
>  
> -	pm_runtime_get_sync(mpu3050->dev);
> +	ret = pm_runtime_resume_and_get(mpu3050->dev);
> +	if (ret)
> +		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;
>  
> @@ -982,12 +985,12 @@ static int mpu3050_drdy_trigger_enable(struct mpu3050 *mpu3050)
>  			   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);
> @@ -1006,9 +1009,15 @@ static int mpu3050_drdy_trigger_enable(struct mpu3050 *mpu3050)
>  
>  	ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val);
>  	if (ret)
> -		return ret;
> +		goto err_put_pm;
>  
>  	return 0;
> +
> +err_put_pm:
> +	pm_runtime_put_autosuspend(mpu3050->dev);
> +	mpu3050->hw_irq_trigger = false;
> +
> +	return ret;
>  }
>  
>  /**

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

[Jonathan Cameron]

On Tue,  3 Nov 2026 17:20:00 +0800
Xiangwu Lai <laixiangwu@cmss.chinamobile.com> wrote:

> From: laixiangwu <laixiangwu@cmss.chinamobile.com>
> 
> Hi Jonathan,
> 
> This is a repost of the v2 series as a new top-level thread, as
> requested. There are no code changes since v2.
> 
> For context, this series fixes a runtime PM refcount leak in the
> mpu3050 data ready trigger enable path: patch 1 splits the enable
> and disable paths out of mpu3050_drdy_trigger_set_state() into
> separate helpers, and patch 2 applies the fix on top, using
> pm_runtime_resume_and_get() instead of pm_runtime_get_sync(), which
> also keeps the usage counter balanced when the resume fails.
> 
> Changes in v3:
> - no code changes, the series is reposted as a new top-level thread
>   (v2 was mistakenly sent as a reply to the review comments).
Future reference. Don't do that unless specifically asked to do
so. Obviously you should never be in that position again, so this
is mostly for anyone else who sees your patch!

Thanks,

Jonathan