Re: [PATCH] iio: accel: bmc150: free irq before teardown

[Andy Shevchenko]

On Mon, Jul 06, 2026 at 12:02:33AM +0100, Jonathan Cameron wrote:
> On Sun,  5 Jul 2026 06:27:31 +0200
> Melbin K Mathew <mlbnkm1@xxxxxxxxx> wrote:

> > bmc150_accel_core_probe() requests the interrupt with
> > devm_request_threaded_irq().  The managed IRQ is released only after the
> > driver remove callback has returned unless it is freed explicitly.
> > 
> > bmc150_accel_core_remove() currently unregisters the IIO device and
> > triggers, cleans up the triggered buffer, suspends the chip and disables
> > the regulators while the IRQ action is still registered.  A late
> > interrupt can therefore run the hard or threaded handler while the IIO
> > trigger state is being torn down or after the device has been put into
> > deep suspend.
> 
> For me this raises a load of questions.

Oh, I was too quick with my glance on this.

> In particular having the interrupt
> torn down before we remove userspace interfaces (as occurs after this change)
> is itself a big source of race conditions as we have to cope with userspace
> being able to poke every interface with the interrupts missing.  So it is
> a design pattern I'm very resistant to!
> 
> Anyhow, is this theoretical or have you seen it in practice? i.e. can we test
> fixes? Are we talking spurious or shared interrupts, or is there a path in
> which a race generates a real interrupt? My guess would be the thread
> running a while after the interrupt but please confirm.  What is the effect
> of talking to the device when powered down? Bus errors, stalls?  A quick
> glance at the datasheet suggests some registers are fine, so this description
> would need to say which ones that are accessed are not.  I think it's only
> the fifo_data but I haven't checked the code or datasheet closely.  What
> actually happens if we access that register? An error or garbage data?
> 
> Maybe we just turn the power on again in the thread handler? Vast majority of the
> time that will just be a ref count increment and decrement, but in the race
> here it will turn the power on again so no problem accessing the device.  
> Or a local flag to say if accessing that fifo register is fine - if it's
> not just erroring out on trying.
> 
> We do have internal infrastructure to close down races around
> teardown (see the exist_lock and how iio_dev->info is set to NULL
> which acts as a marker of a device going away - maybe we need to make
> that available to drivers (though I'd rather not as it's easy to use
> wrong!) I'm not aware of any core interfaces such as accessing the
> buffers or open chardevs etc that are not appropriately guarded so
> hopefully the races you are seeing are just at the driver
> level. The usual route to handling this stuff is to make the interrupt
> handling safe to the transitions that occur on tear down, not reorder
> things to stop the handler running.  Note that making it safe
> can absolutely include simply returning errors from accesses that don't
> work due to power conditions.
> 
> > Free the IRQ at the start of remove so that no handler is running while
> > the rest of the driver state and hardware resources are dismantled.

> > +	if (data->irq > 0)
> > +		devm_free_irq(dev, data->irq, indio_dev);
> 
> If (and it is a very big if) this is the right thing to do then it must
> be accompanied by documentation of why we need the remove to not be in
> the reverse order of probe.  Also, rip out devm registration and
> move to none devm for everything after the request of the irq.
> 
> Note that because userspace interfaces are still up at this point
> we may well get normal operations generating unhandled interrupts, potentially
> resulting in the interrupt core taking that interrupt offline.
> 
> It is for this reason that we generally disable userspace interfaces
> first and then remove the interrupts.

Indeed, the current logic seems correct.

> >  	iio_device_unregister(indio_dev);
> >  
> >  	pm_runtime_disable(dev);

-- 
With Best Regards,
Andy Shevchenko