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

[Matti Vaittinen]

On 17/05/2026 20:12, David Lechner wrote:
> On 5/17/26 11:08 AM, Stepan Ionichev wrote:
> > bm1390_trigger_handler() has three error returns:
> >
> > 	if (ret || !status)
> > 		return IRQ_NONE;          /* status read failed */
> > 	...
> > 	if (ret) {
> > 		dev_warn(...);
> > 		return IRQ_NONE;          /* pressure read failed */
> > 	}
> > 	...
> > 	if (ret) {
> > 		dev_warn(...);
> > 		return IRQ_HANDLED;       /* temp read failed */
> > 	}
> >
> > None of them call iio_trigger_notify_done(). The success path at the
> > end does, so on a single transient regmap or pressure-read error the
> > trigger never sees its use_count decremented, and the
> > !atomic_read(&trig->use_count) guard in iio_trigger_poll_chained()
> > drops every subsequent dispatch for that trigger. The buffered-data
> > flow stays wedged until the trigger is detached.

I don't really know the intended logic of the use_count, so I'll leave 
this to those who understand it better. I'll just add some thoughts this 
invoked.

I think it is not really nice to require (or trust) drivers to call the 
"iio_trigger_notify_done()" if the handler fails. Maybe it would be 
better to do something like:

void iio_trigger_poll_nested(struct iio_trigger *trig)
{
        int i;

        if (!atomic_read(&trig->use_count)) {
                atomic_set(&trig->use_count, 
CONFIG_IIO_CONSUMERS_PER_TRIGGER);

                for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
                        if (trig->subirqs[i].enabled)
                                handle_nested_irq(trig->subirq_base + i);
                        else
                                iio_trigger_notify_done(trig);
                }
		atomic_set(&trig->use_count, 0); /* Clear the use_count if drivers 
didn't */
        }
}

to prevent this class of problems once and for all. But yeah, wiser 
minds have designed this - so let's hear some other opinions as well :)

> > The IRQ_HANDLED return on the temperature path additionally leaves
> > the temp branch's last partial state in &data->buf.temp without
> > pushing the sample, which is the existing intended behaviour; only
> > the missing notify_done() needs fixing.
> >
> > Funnel all returns through a single 'done' label that calls
> > iio_trigger_notify_done() before returning the saved irqreturn_t.
> >
> > Fixes: 81ca5979b6ed ("iio: pressure: Support ROHM BU1390")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Stepan Ionichev <sozdayvek@xxxxxxxxx>
> > ---
> >   drivers/iio/pressure/rohm-bm1390.c | 15 ++++++++++-----
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
> > index 08146ca0f..c18352399 100644
> > --- a/drivers/iio/pressure/rohm-bm1390.c
> > +++ b/drivers/iio/pressure/rohm-bm1390.c
> > @@ -626,12 +626,15 @@ static irqreturn_t bm1390_trigger_handler(int irq, void *p)
> >   	struct iio_poll_func *pf = p;
> >   	struct iio_dev *idev = pf->indio_dev;
> >   	struct bm1390_data *data = iio_priv(idev);
> > +	irqreturn_t result = IRQ_HANDLED;
> >   	int ret, status;
> >   
> >   	/* DRDY is acked by reading status reg */
> >   	ret = regmap_read(data->regmap, BM1390_REG_STATUS, &status);
> > -	if (ret || !status)
> > -		return IRQ_NONE;
> > +	if (ret || !status) {
> > +		result = IRQ_NONE;
>
> IRQ_NONE means that the interrupt wasn't handled, so it won't be cleared
> and the handler will likely just run again immediately. So it probably
> isn't the right thing to be returning in the first place.

This is exactly why IRQ-none is returned, and what it is used for. If 
the problem with bus-access / device persists, the kernel will (after 
XXXX fails indicated by IRQ_NONE - don't remember exact numbers) disable 
the IRQ from the host side, and emit the, ass-saving, "nobody cared" -print.

This is (in my opinion) the only RightThing(tm). (Especially so, if the 
device is accessed from the fast handler, and is system is single-core). 
There is a tremendous difference when debugging a system which just 
hangs in IRQ loop forever (and you can't get no contact to it), and when 
debugging a system which, after a relatively short hang-up, let's you 
see the magic "nobody cared" -print telling a misbehaving IRQ was disabled.

Furthermore, if the status register read failure was a temporary one, 
then we should be getting new IRQ as soon as the handler exists. This 
should then successfully handle the IRQ.

Yours,
	-- Matti

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

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