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

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

[Melbin K Mathew]

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.

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@vger.kernel.org
Signed-off-by: Melbin K Mathew <mlbnkm1@gmail.com>
---
 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);
+
 	iio_device_unregister(indio_dev);
 
 	pm_runtime_disable(dev);
-- 
2.39.5

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

[Andy Shevchenko]

On Sun, Jul 05, 2026 at 06:27:31AM +0200, Melbin K Mathew 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.
> 
> 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.

In general this is correct fix, but have you checked the rest of remove if it
has any communication with HW and if that communication relies on IRQ to be on?

(*yes, this is very unlikely, but please double check as rarely we have some HW
 that might need that, and in such a case the fix might be different)

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Melbin K Mathew]

Thanks for the review.

I double checked the remove path.  The remaining hardware accesses after
freeing the IRQ are synchronous regmap accesses and do not rely on the 
IRQ being enabled.

In particular, iio_device_unregister() may disable the buffer path, 
which can call into the buffer predisable path and synchronously disable 
the FIFO interrupt, flush the FIFO and update the FIFO mode.  Later 
remove explicitly puts the device into deep suspend via 
bmc150_accel_set_mode().  These paths do not wait for an interrupt or 
use the threaded IRQ handler for completion.

The IRQ handler itself is only used for asynchronous trigger polling,
FIFO/event handling and interrupt latch acknowledgement, so freeing it 
before the rest of teardown should not remove anything that the remove 
path depends on.

On 05/07/2026 07:53, Andy Shevchenko wrote:
> On Sun, Jul 05, 2026 at 06:27:31AM +0200, Melbin K Mathew 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.
>>
>> 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.
> 
> In general this is correct fix, but have you checked the rest of remove if it
> has any communication with HW and if that communication relies on IRQ to be on?
> 
> (*yes, this is very unlikely, but please double check as rarely we have some HW
>   that might need that, and in such a case the fix might be different)
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> 

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

[Jonathan Cameron]

On Sun,  5 Jul 2026 06:27:31 +0200
Melbin K Mathew <mlbnkm1@gmail.com> 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@vger.kernel.org
> Signed-off-by: Melbin K Mathew <mlbnkm1@gmail.com>
> ---
>  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);

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@gmail.com> 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