Re: [PATCH v2] iio: pressure: rohm-bm1390: notify trigger on all error paths
From: Matti Vaittinen
Date: Fri May 29 2026 - 04:22:38 EST
On 22/05/2026 15:38, Matti Vaittinen wrote:
On 20/05/2026 14:08, Jonathan Cameron wrote:
On Tue, 19 May 2026 08:48:13 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
Thanks Jonathan,
Your post give me something to think about ;)
This is a can of worms. More below.
I'm unconcerned as long as (and ideally someone should check it)
we can get of being stuck by unbind/rebind of driver. Anything
else is best effort.
On 18/05/2026 18:15, Jonathan Cameron wrote:
On Mon, 18 May 2026 14:42:38 +0500
Stepan Ionichev <sozdayvek@xxxxxxxxx> wrote:
bm1390_trigger_handler() returns from three error paths without
calling iio_trigger_notify_done(). The success path at the end
does, so on a single transient regmap or read failure the trigger
use_count is never decremented, and the !atomic_read(&trig->use_count)
guard in iio_trigger_poll_chained() drops every subsequent dispatch.
The buffered-data flow stays wedged until the trigger is detached.
Funnel all returns through a single done label that calls
iio_trigger_notify_done() and reports the outcome via IRQ_RETVAL().
Fixes: 81ca5979b6ed ("iio: pressure: Support ROHM BU1390")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Stepan Ionichev <sozdayvek@xxxxxxxxx>
These error path 'fixes' are fixes for hardware failure - so if anything
they are hardending against a possible error condition. I don't mind
that bit it's not a bug to not do this so fixes tag an stable are not
appropriate for any of these.
Note however that hardening against these conditions is not this simple.
It takes careful analysis of exactly how the hardware behaves and what
each error condition 'might' mean. Whilst they are probably harmless
I'm also very dubious about taking them without comprehensive testing
on the particular device.
---
//snip
@@ -639,7 +642,8 @@ static irqreturn_t bm1390_trigger_handler(int irq, void *p)
ret = bm1390_pressure_read(data, &data->buf.pressure);
if (ret) {
dev_warn(data->dev, "sample read failed %d\n", ret);
- return IRQ_NONE;
+ handled = false;
+ goto done;
Hopefully all this stuff is unrelated to the trigger. For these it is fair to
ack the trigger and the interrupt. Curiously the driver does it partly for the
next one (IRQ_HANDLED).
I would keep the IRQ_NONE here because, if we keep constantly failing
the reads, then the bus is likely to be unerliable - and disabling the
useless IRQ is probably very sane thing to do. It should help debugging.
What comes to acking the trigger - I am starting to agree with Stepan,
we should probably ack the trigger in any case. If we don't ack the
trigger, then the IRQ_NONE does not serve the purpose it is intended for.
The interrupt that we'd get spurious detection on here would not be the device
one it would be the software emulated one deep in the iio trigger stuff.
Might still be useful for debug. Anyone fancy hacking an error in and reporting
back what we actually get from the debug hardware? (with that trigger acked
as you suggest?)
No promises but I'll see if I can try out something next week...
The week has been horrible... I only had around half an hour for this (just now). Quick:
+++ b/drivers/iio/pressure/rohm-bm1390.c
@@ -621,6 +621,16 @@ static const struct iio_buffer_setup_ops bm1390_buffer_ops = {
.predisable = bm1390_buffer_predisable,
};
+/*
+ * Test case where IRQ status is nopt read (acked). Useful for evaluating the
+ * impact of returning the IRQ_NONE from the trigger handler. define also the
+ * TEST_FORCE_IRQ_NOTIFY if you wish to cause the trigger to be notified.
+ *
+ * Note, in case it is not obvious, this will cause IRQ storm.
+ */
+#define TEST_FORCE_IRQ_NONE
+#define TEST_FORCE_IRQ_NOTIFY
+
static irqreturn_t bm1390_trigger_handler(int irq, void *p)
{
struct iio_poll_func *pf = p;
@@ -628,12 +638,27 @@ static irqreturn_t bm1390_trigger_handler(int irq, void *p)
struct bm1390_data *data = iio_priv(idev);
int ret, status;
+#ifdef TEST_FORCE_IRQ_NONE
+ static unsigned long int first = 1, first2 = 0;
+ ret = 0;
+
+ if (first) {
+ pr_info("Skip read\n");
+ first = 0;
+ }
+ #ifdef TEST_FORCE_IRQ_NOTIFY
+ status = BIT(BM1390_CHAN_PRESSURE);
+ #else
+ status = 0;
+ #endif
+#else
/* DRDY is acked by reading status reg */
ret = regmap_read(data->regmap, BM1390_REG_STATUS, &status);
if (ret || !status)
return IRQ_NONE;
+#endif
- dev_dbg(data->dev, "DRDY trig status 0x%x\n", status);
+// dev_dbg(data->dev, "DRDY trig status 0x%x\n", status);
if (test_bit(BM1390_CHAN_PRESSURE, idev->active_scan_mask)) {
ret = bm1390_pressure_read(data, &data->buf.pressure);
@@ -656,7 +681,17 @@ static irqreturn_t bm1390_trigger_handler(int irq, void *p)
data->timestamp);
iio_trigger_notify_done(idev->trig);
+#ifdef TEST_FORCE_IRQ_NONE
+ /* HACK, return IRQ_NONE and see if IRQ gets disabled */
+ if (!(first2 % 1000))
+ pr_info("Hack, return IRQ_NONE (%lu th)\n", first2);
+
+ first2++;
+
+ return IRQ_NONE;
+#else
return IRQ_HANDLED;
+#endif
}
/* Get timestamps and wake the thread if we need to read data */
resulted:
root@arm:/home/debian# /iio_generic_buffer -a -c1000000 --device-name bm1390 -T0 > /dev/null
Enabling all channels
[ 115.098819] Skip read
[ 115.102442] Hack, return IRQ_NONE (0 th)
[ 116.459049] Hack, return IRQ_NONE (1000 th)
[ 117.851037] Hack, return IRQ_NONE (2000 th)
[ 119.214843] Hack, return IRQ_NONE (3000 th)
[ 120.598114] Hack, return IRQ_NONE (4000 th)
[ 121.960255] Hack, return IRQ_NONE (5000 th)
[ 123.322424] Hack, return IRQ_NONE (6000 th)
//snip
[ 237.726666] Hack, return IRQ_NONE (90000 th)
[ 239.095910] Hack, return IRQ_NONE (91000 th)
[ 240.481233] Hack, return IRQ_NONE (92000 th)
[ 241.846072] Hack, return IRQ_NONE (93000 th)
[ 243.206432] Hack, return IRQ_NONE (94000 th)
[ 244.570636] Hack, return IRQ_NONE (95000 th)
[ 245.928964] Hack, return IRQ_NONE (96000 th)
[ 247.286839] Hack, return IRQ_NONE (97000 th)
[ 248.647986] Hack, return IRQ_NONE (98000 th)
[ 250.011214] Hack, return IRQ_NONE (99000 th)
[ 251.368583] irq 64: nobody cared (try booting with the "irqpoll" option)
[ 251.375463] CPU: 0 UID: 0 PID: 835 Comm: irq/63-2-005d-b Tainted: G O 7.1.0-rc1-00002-g3b459deb7222-dirty #249 VOLUNTARY
[ 251.375501] Tainted: [O]=OOT_MODULE
[ 251.375511] Hardware name: Generic AM33XX (Flattened Device Tree)
[ 251.375525] Call trace:
[ 251.375545] unwind_backtrace from show_stack+0x10/0x14
[ 251.375607] show_stack from dump_stack_lvl+0x50/0x64
[ 251.375646] dump_stack_lvl from __report_bad_irq+0x30/0xbc
[ 251.375680] __report_bad_irq from note_interrupt+0x2b4/0x32c
[ 251.375722] note_interrupt from handle_nested_irq+0x13c/0x14c
[ 251.375758] handle_nested_irq from iio_trigger_poll_nested+0x4c/0x68 [industrialio]
[ 251.375917] iio_trigger_poll_nested [industrialio] from bm1390_irq_thread_handler+0x54/0x7c [rohm_bm1390]
[ 251.375994] bm1390_irq_thread_handler [rohm_bm1390] from irq_thread_fn+0x1c/0x78
[ 251.376028] irq_thread_fn from irq_thread+0x18c/0x324
[ 251.376057] irq_thread from kthread+0xf8/0x130
[ 251.376091] kthread from ret_from_fork+0x14/0x20
[ 251.376114] Exception stack(0xe0355fb0 to 0xe0355ff8)
[ 251.376136] 5fa0: 00000000 00000000 00000000 00000000
[ 251.376156] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 251.376175] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 251.376189] handlers:
[ 251.498714] [<2ec7a5d9>] iio_pollfunc_store_time [industrialio] threaded [<7f4268a2>] bm1390_trigger_handler [rohm_bm1390]
[ 251.509974] Disabling IRQ #64
Message from syslogd@arm at Jan 1 01:17:33 ...
kernel:[ 251.509974] Disabling IRQ #64
[ 252.822500] sched: RT throttling activated
Things I very hastly picked up:
1. The throttling mechanism works even though the handling is invoked via iio_trigger_poll_nested(), Probably because this propagates the call to the handle_nested_irq() - which does bookkeeping.
2. For some reason (which I didn't have time to check yet), the beaglebone black which I used to run this, was not completely blocked by the IRQ. We can see the "Hack, return IRQ_NONE (xxx th)" -prints emerging just fine.
And now I must run. I hope to be able to dig some more details next week.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~