Re: [PATCH v3] iio: gyro: mpu3050: fix missing iio_trigger_unregister and irq cleanup

[Jonathan Cameron]

On Wed, 20 May 2026 23:22:36 +0800
Li Xinyu <xinyuili@xxxxxxx> wrote:

> mpu3050_trigger_probe() registers the DRDY trigger with
> iio_trigger_register() but neither mpu3050_common_remove() nor
> the error path in mpu3050_common_probe() calls
> iio_trigger_unregister(). On module unload or probe failure the
> trigger remains in the global trigger list while its memory is
> freed by devm, leaving a dangling entry.
> 
> Also fix a use-after-free risk: when iio_trigger_register() fails,
> mpu3050->irq remained set to a non-zero value, which would cause
> mpu3050_common_remove() to attempt a double-free of the IRQ and
> an unregister of a never-registered trigger. Clear mpu3050->irq
> in the error path to prevent this.
> 
> Revert the v2 devm approach as requested by Jonathan: the driver
> mixes devm and non-devm resource management, so the minimal fix
> is to add the missing unregister calls and keep the existing
> manual resource management style.
> 
> Fixes: 3904b28efb2c ("iio: gyro: Add driver for the MPU-3050 gyroscope")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Li Xinyu <xinyuili@xxxxxxx>
> ---
> Changes in v3:
> - Thanks Jonathan for the feedback on v2. Instead of mixing devm
>   with non-devm resource management in probe, revert to plain
>   iio_trigger_register() and add the missing iio_trigger_unregister()
>   calls in the error path and remove callback.
> - Also noticed that mpu3050->irq was set but not cleared when
>   iio_trigger_register() fails in trigger_probe, which would
>   cause a double-free on module unload. Set mpu3050->irq = 0
>   in the error path to prevent this.

This is interesting. I wonder why we paper over the failed trigger
registration.   Generally that's an error case that should
result in the driver not loading.

The mix of devm and non devm in iio_trigger_register() is also
nasty.

Linus W, I think this was your code, any idea if we actually need
to do that and can't just fail?  If we can modify it to fail
I'd rather do that and avoid using ->irq as a flag to indicate
if we successfully registered or not

> 
> Changes in v2:
> - Fixed the name format in Signed-off-by. Thanks Maxime for
>   catching this.
> ---
>  drivers/iio/gyro/mpu3050-core.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> index bcfa83a46737..459d02aa3d18 100644
> --- a/drivers/iio/gyro/mpu3050-core.c
> +++ b/drivers/iio/gyro/mpu3050-core.c
> @@ -1127,7 +1127,7 @@ static int mpu3050_trigger_probe(struct iio_dev *indio_dev, int irq)
>  	mpu3050->trig->ops = &mpu3050_trigger_ops;
>  	iio_trigger_set_drvdata(mpu3050->trig, indio_dev);
>  
> -	ret = devm_iio_trigger_register(mpu3050->dev, mpu3050->trig);
> +	ret = iio_trigger_register(mpu3050->trig);
>  	if (ret)
>  		goto err_iio_trigger;
>  
> @@ -1137,6 +1137,7 @@ static int mpu3050_trigger_probe(struct iio_dev *indio_dev, int irq)
>  
>  err_iio_trigger:
>  	free_irq(mpu3050->irq, mpu3050->trig);
> +	mpu3050->irq = 0;
>  
>  	return ret;
>  }
> @@ -1260,8 +1261,10 @@ int mpu3050_common_probe(struct device *dev,
>  	pm_runtime_get_sync(dev);
>  	pm_runtime_put_noidle(dev);
>  	pm_runtime_disable(dev);
> -	if (irq)
> +	if (mpu3050->irq) {
> +		iio_trigger_unregister(mpu3050->trig);
>  		free_irq(mpu3050->irq, mpu3050->trig);
> +	}
>  	iio_triggered_buffer_cleanup(indio_dev);
>  err_power_down:
>  	mpu3050_power_down(mpu3050);
> @@ -1278,8 +1281,10 @@ void mpu3050_common_remove(struct device *dev)
>  	pm_runtime_get_sync(dev);
>  	pm_runtime_put_noidle(dev);
>  	pm_runtime_disable(dev);
> -	if (mpu3050->irq)
> +	if (mpu3050->irq) {
> +		iio_trigger_unregister(mpu3050->trig);
>  		free_irq(mpu3050->irq, mpu3050->trig);
> +	}
>  	iio_triggered_buffer_cleanup(indio_dev);
>  	mpu3050_power_down(mpu3050);
>  }