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

[Matti Vaittinen]

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

Yours,
	-- Matti

--
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~