Re: [PATCH v3] iio: gyro: mpu3050: Fix runtime PM leak and refactor trigger state

[Jonathan Cameron]

On Tue, 14 Jul 2026 18:44:27 +0530
Biren Pandya <birenpandya@xxxxxxxxx> wrote:

> mpu3050_drdy_trigger_set_state() calls pm_runtime_get_sync() when the
> trigger is enabled, but several error paths in the enable branch return
> directly without dropping the usage counter again. pm_runtime_get_sync()
> increments the usage counter, so every failed enable leaks a runtime PM
> reference and the device can no longer autosuspend. The driver state flag
> hw_irq_trigger is also left set after a failed enable.
> 
> To fix the error unwind clearly and avoid an asymmetric goto block inside a
> monolithic function, this patch breaks the trigger state handler into two
> distinct helpers: mpu3050_drdy_trigger_enable() and
> mpu3050_drdy_trigger_disable(). The enable helper correctly implements the
> error unwind path to drop the PM reference and clear the flag.
> 
> Additionally, pm_runtime_get_sync() is replaced with
> pm_runtime_resume_and_get() for robust error checking.
> 
> Fixes: 3904b28efb2c ("iio: gyro: Add driver for the MPU-3050 gyroscope")
> Signed-off-by: Biren Pandya <birenpandya@xxxxxxxxx>

Hmm. I just replied to an ancient version... Reason being you keep
sending new series in response to old ones.  Do not do that as it means
simple date sorting fails!

Anyhow, some comments inline for this approach.

> ---
> 
> Changes in v3:
> - Fixed kernel-doc warning for mpu3050_drdy_trigger_set_state().
> - Fixed the Fixes tag title to exactly match the target commit.
> - Link to v2: https://lore.kernel.org/all/20260615214504.38979-1-birenpandya@xxxxxxxxx/
>  drivers/iio/gyro/mpu3050-core.c | 166 ++++++++++++++++++--------------
>  1 file changed, 92 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> index d84e04e4b4314..9de126c3b4350 100644
> --- a/drivers/iio/gyro/mpu3050-core.c
> +++ b/drivers/iio/gyro/mpu3050-core.c
> @@ -945,102 +945,120 @@ static irqreturn_t mpu3050_irq_thread(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> -/**
> - * 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)
> +static int mpu3050_drdy_trigger_disable(struct iio_trigger *trig)
>  {
>  	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");
> +	/* Disable all interrupts */
> +	ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, 0);
> +	if (ret)
> +		dev_err(mpu3050->dev, "error disabling IRQ\n");

If this fails we loose the error. Just exit on each error we
are effectively in an unknown and probably dead state anyway
if only some of these work.  There are some error paths
where the only right answer is to reset the driver by
an unbind rebind.

>  
> -		/* Clear IRQ flag */
> -		ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
> -		if (ret)
> -			dev_err(mpu3050->dev, "error clearing IRQ status\n");
> +	/* 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 and reset it */
> -		ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
> -		if (ret)
> -			dev_err(mpu3050->dev, "error disabling FIFO\n");
> +	/* 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");
> +	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;
> +	pm_runtime_put_autosuspend(mpu3050->dev);
> +	mpu3050->hw_irq_trigger = false;
>  
> -		return 0;
> -	} else {
> -		/* Else we're enabling the trigger from this point */
> -		pm_runtime_get_sync(mpu3050->dev);
> -		mpu3050->hw_irq_trigger = true;
> +	return 0;

If it failed in any way we should not be returning 0.
Sure there isn't a lot we can do to recover but this hides the problem
from upper layers of the stack.


> +}
>  
> -		/* Disable all things in the FIFO */
> -		ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
> -		if (ret)
> -			return ret;
> +static int mpu3050_drdy_trigger_enable(struct iio_trigger *trig)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct mpu3050 *mpu3050 = iio_priv(indio_dev);
> +	unsigned int val;
> +	int ret;
>  
> -		/* 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 = pm_runtime_resume_and_get(mpu3050->dev);
> +	if (ret)
> +		return ret;

>  
> -		mpu3050->pending_fifo_footer = false;
> +	mpu3050->hw_irq_trigger = true;
Seems like an oddly early place to do this but I guess this is what
the original code did for some reason.  If you can figure that out
add a comment on why this isn't left until we know we successfully
turned the trigger on.

>  
> -		/* 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;
> +	/* Disable all things in the FIFO */
> +	ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
> +	if (ret)
> +		goto err_put_autosuspend;
>  
> -		/* Configure the sample engine */
> -		ret = mpu3050_start_sampling(mpu3050);
> -		if (ret)
> -			return ret;
> +	/* 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)
> +		goto err_put_autosuspend;
>  
> -		/* Clear IRQ flag */
> -		ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val);
> -		if (ret)
> -			dev_err(mpu3050->dev, "error clearing IRQ status\n");
> +	mpu3050->pending_fifo_footer = false;
>  
> -		/* Give us interrupts whenever there is new data ready */
> -		val = MPU3050_INT_RAW_RDY_EN;
> +	/* 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)
> +		goto err_put_autosuspend;
>  
> -		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;
> +	/* Configure the sample engine */
> +	ret = mpu3050_start_sampling(mpu3050);
> +	if (ret)
> +		goto err_put_autosuspend;
>  
> -		ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val);
> -		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");
> +
> +	/* Give us interrupts whenever there is new data ready */
> +	val = MPU3050_INT_RAW_RDY_EN;
> +
> +	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;
> +
> +	ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val);
> +	if (ret)
> +		goto err_put_autosuspend;
>  
>  	return 0;
> +
> +err_put_autosuspend:
> +	pm_runtime_put_autosuspend(mpu3050->dev);
> +	mpu3050->hw_irq_trigger = false;
As above. Just move the setting of this until just above the return 0
and no need to reset it.  There might be a reason though so do
check the driver carefully for how this is used. 
> +	return ret;
> +}
> +
> +/**
> + * 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)
> +{
> +	if (enable)
> +		return mpu3050_drdy_trigger_enable(trig);
> +	else
> +		return mpu3050_drdy_trigger_disable(trig);
>  }
>  
>  static const struct iio_trigger_ops mpu3050_trigger_ops = {