Re: [PATCH v2] iio: pressure: rohm-bm1390: notify trigger on all error paths

[Matti Vaittinen <mazziesaccount@gmail.com>]

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@gmail.com> 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@gmail.com> 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@vger.kernel.org
>>>>> Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
>>>>
>>>> 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! ~~