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

[Jonathan Cameron]

On Fri, 29 May 2026 11:21:40 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:

> 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.

Great.  At least it squashes something.

> 
> 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.

After the Disabling IRQ #64 message?

> 
> And now I must run. I hope to be able to dig some more details next week.
> 
> Yours,
> 	-- Matti
> 
>