Re: [PATCH] iio: gyro: mpu3050: add missing trigger cleanup on remove

["Nuno Sá" <nuno.sa@analog.com>]

On Sun, Oct 11, 2026 at 07:12:40PM +0200, Joshua Crofts wrote:
> On Sun, 11 Oct 2026 17:53:01 +0200
> Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> >  drivers/iio/gyro/mpu3050-core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> 
> Sashiko review came back and the issue doesn't seem so
> straight-forward. I'll probably introduce a local pointer
> and then assign it to the mpu3050 data struct.

Hmm the probe seems a mess of mixed devm with manual cleanup. Ideally we
would move all of it managed resources (and that would fix the issue).
Other than that, seems to me we have two things in play. First:

if (irq) {
	ret = mpu3050_trigger_probe(indio_dev, irq);
	if (ret)
		dev_err(dev, "failed to register trigger\n");
}

Do we really want to ignore the error? I would say no... 
So, if we change the above to not ignore errors, we can assume that, on
success, our trigger was registered. Which means that on the if (irq) at
the errir patch we could unregister the trigger. Not that pretty but 
consistent with what we have :).

If someone has the HW, moving this into devm would probably be the
better approach (but even in that case, we might want the simple fix
first for backporting).

- Nuno Sá

> 
> Attaching Sashiko review below.
> 
> > diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> > index 07c0f9011c63..096fd0dc608e 100644
> > --- a/drivers/iio/gyro/mpu3050-core.c
> > +++ b/drivers/iio/gyro/mpu3050-core.c
> > @@ -1260,6 +1260,8 @@ int mpu3050_common_probe(struct device *dev,
> >  	pm_runtime_get_sync(dev);
> >  	pm_runtime_put_noidle(dev);
> >  	pm_runtime_disable(dev);
> > +	if (mpu3050->trig)
> > +		iio_trigger_unregister(mpu3050->trig);
> 
> 
> Does this unconditionally unregister a trigger that may not have been
> successfully registered?
> 
> If mpu3050_trigger_probe() fails (for example, if request_threaded_irq()
> fails), mpu3050->trig is allocated via devm_iio_trigger_alloc() but it will
> never have successfully completed iio_trigger_register().
> 
> mpu3050_common_probe() catches that error, logs "failed to register trigger",
> and continues. If a subsequent step like iio_device_register() fails, this
> code will call iio_trigger_unregister() on the uninitialized trigger.
> 
> Because the trigger was never registered via iio_trigger_register(),
> iio_trigger_unregister() will call device_del() on a device that was never
> added, and ida_free() on an unallocated ID, which can lead to a crash.
> 
> >  	if (irq)
> >  		free_irq(mpu3050->irq, mpu3050->trig);
> >  	iio_triggered_buffer_cleanup(indio_dev);
> > @@ -1278,6 +1280,8 @@ void mpu3050_common_remove(struct device *dev)
> >  	pm_runtime_get_sync(dev);
> >  	pm_runtime_put_noidle(dev);
> >  	pm_runtime_disable(dev);
> > +	if (mpu3050->trig)
> > +		iio_trigger_unregister(mpu3050->trig);
> 
> Can this cause a kernel crash during normal driver teardown if the trigger
> probe originally failed?
> 
> Similar to the probe error path above, if mpu3050_trigger_probe() failed
> during initialization, mpu3050->trig is left allocated but unregistered.
> 
> Calling iio_trigger_unregister() here on that uninitialized trigger will
> cause the same teardown issues during driver removal.
> 
> -- 
> Kind regards,
> Joshua Crofts