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

[Jonathan Cameron]

On Sun,  5 Jul 2026 06:27:31 +0200
Melbin K Mathew <mlbnkm1@xxxxxxxxx> wrote:

Hi Melbin,

Thanks for the patch,

> 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.  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.
> 
> Fixes: 55637c38377a ("iio: bmc150: Split the driver into core and i2c")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Melbin K Mathew <mlbnkm1@xxxxxxxxx>
> ---
>  drivers/iio/accel/bmc150-accel-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 2398eb7e12cd..2adddc965650 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -1766,6 +1766,9 @@ void bmc150_accel_core_remove(struct device *dev)
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
>  
> +	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.

Thanks,

Jonathan

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