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

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

[Xiangwu Lai]

Hi Jonathan,

Thanks for the review! This version addresses both comments on the
trigger helpers: the dispatcher now uses an else, and the error
return for the IRQ status read in the enable path is propagated.
The disable path is left as is, as you concluded it was not worth
the effort to fix.

Patch 1 splits the enable and disable paths out of
mpu3050_drdy_trigger_set_state() into separate helpers. It is a pure
code move with no functional change. Patch 2 applies the runtime PM
refcount 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 v4:
- use an else for the either/or dispatch in
  mpu3050_drdy_trigger_set_state(), as suggested by the reviewer.
- propagate the error from the IRQ status read in the enable path
  instead of only printing it, as suggested by the reviewer.

Changes in v3:
- no code changes, the series was reposted as a new top-level thread.

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

v3: https://lore.kernel.org/r/20261103092002.1332-1-laixiangwu@cmss.chinamobile.com/
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 v4 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 v4:
- use an else for the either/or dispatch in
  mpu3050_drdy_trigger_set_state(), as suggested by the reviewer.

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

Link: https://lore.kernel.org/r/20261103092002.1332-2-laixiangwu@cmss.chinamobile.com
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..62e416df3 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);
+	else
+		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 v4 1/2] iio: gyro: mpu3050: Split data ready trigger enable/disable into helpers

[Joshua Crofts]

On Wed,  4 Nov 2026 12:00:06 +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>
> ---
> 
> Changes in v4:
> - use an else for the either/or dispatch in
>   mpu3050_drdy_trigger_set_state(), as suggested by the reviewer.
> 
> Changes in v3:
> - none; reposted in a new top-level thread.
> 
> Link: https://lore.kernel.org/r/20261103092002.1332-2-laixiangwu@cmss.chinamobile.com
> 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..62e416df3 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;
>  

Not sure if it's just me, but reading this patch is pretty hard;
please use histogram as your default diff algorithm [1] when sending
a patch next time.

Otherwise LGTM (after viewing with the histogram option :)).

Reviewed-by: Joshua Crofts <crofts@kernel.org>

[1] https://git-scm.com/docs/diff-algorithm-option.html

-- 
Kind regards,
Joshua Crofts

[PATCH v4 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.

Also propagate the error from the IRQ status read that clears the
data ready flag instead of only printing it, so that a failure
anywhere in the enable path consistently rolls the trigger state
back.

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

Changes in v4:
- propagate the error from the IRQ status read instead of only
  printing it, as suggested by the reviewer.

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/20261103092002.1332-3-laixiangwu@cmss.chinamobile.com
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 | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
index 62e416df3..ef4c21c71 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,17 +985,17 @@ 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);
 	if (ret)
-		dev_err(mpu3050->dev, "error clearing IRQ status\n");
+		goto err_put_pm;
 
 	/* Give us interrupts whenever there is new data ready */
 	val = MPU3050_INT_RAW_RDY_EN;
@@ -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 v4 2/2] iio: gyro: mpu3050: Fix runtime PM refcount leak in trigger enable path

[Andy Shevchenko]

On Wed, Nov 04, 2026 at 12:00:07PM +0800, Xiangwu Lai wrote:

> 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.
> 
> Also propagate the error from the IRQ status read that clears the
> data ready flag instead of only printing it, so that a failure
> anywhere in the enable path consistently rolls the trigger state
> back.

...

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

Why not PM_RUNTIME_ACQUIRE*()?
Yes, to make the

	mpu3050->hw_irq_trigger = false;

this needs to be wrapped (two functions, outer one uses PM_RUNTIME_ACQUIRE(),
and handles the flag).

>  	mpu3050->hw_irq_trigger = true;

-- 
With Best Regards,
Andy Shevchenko

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

[Xiangwu Lai]

Hi Andy,

Thanks for the suggestion!

The runtime PM reference in mpu3050_drdy_trigger_enable() is not
scoped to the function: it is taken when the trigger is enabled and
only dropped by mpu3050_drdy_trigger_disable() via
pm_runtime_put_autosuspend() when the trigger is turned off again.
The buffer preenable/postdisable pair in this driver manages its
reference the same way for the buffer lifetime.

With a scoped guard the reference would be released as soon as
mpu3050_drdy_trigger_enable() returns, so the device could runtime
suspend while the data ready trigger is still armed, and the usage
counter would underflow when the disable path drops a reference
that the guard had already released.

So an acquire-here / release-elsewhere lifetime like this does not
seem to be expressible with the ACQUIRE() family. If you see a
formulation that fits, I'm happy to rework the patch onto it.

Best regards,
Xiangwu Lai