Re: [PATCH v2] iio: pressure: rohm-bm1390: notify trigger on all error paths
From: Matti Vaittinen
Date: Tue May 19 2026 - 01:48:52 EST
Thanks Jonathan,
Your post give me something to think about ;)
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.
---So question 1.
v2:
- Use a bool and IRQ_RETVAL() instead of irqreturn_t (Andy)
v1: https://lore.kernel.org/all/20260517160801.269-1-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..81368e578 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);
+ bool handled = true;
int ret, status;
/* DRDY is acked by reading status reg */
ret = regmap_read(data->regmap, BM1390_REG_STATUS, &status);
- What actually is device state if this read fails? We have no idea.
It might have failed on the 'to device' path in which case the device
didn't see the read. Or it might have failed on the 'from device path'.
Gets more complex...
- if (ret || !status)
- return IRQ_NONE;
The trigger in use might well be the dataready trigger provided by this driver
(though I note this device has no validate callbacks so we do allow other
triggers - that may or may not be a bug!) I really dislike read to clear
register designs as they make this stuff more complex.
I have a strong feeling it should be the dataready. Still, I have no idea about actual systems using this driver, so I am a bit cautious adding new restrictions.
Anyhow question 2:
- What happens if we don't clear it and do acknowledge the interrupt plus
ack the trigger (which is what iio_trigger_done() is doing?
Two obvious options - wedged device, it re interrupts immediately.
If we are wedged, then meh device dead. Without adding retry loops
(don't) recovery path is reset the driver by unbinding and rebinding.
The BM1390 keeps the IRQ pin asserted.
Fun follow up is what happens if having acked the data ready trigger
by this read, we get another read before getting to iio_trigger_notify_done()?
Quite possibly we wedge.
I see. This isn't fun at all. Even more so if the trigger use-count now prevents us from calling the handler, and returning further IRQ_NONEs, preventing the safety-mechanism intended to disable the offending IRQ. I have a feeling there is IRQF_ONESHOT set though, so perhaps we are safe from this (when no error path is taken in the handler).
This drivers trigger may be missing a reenable() callback
(which would typically reread the status register to clear any such interrupt).
Which works for case where we "get another read before getting to iio_trigger_notify_done()" - but not for a case where we might have the bus stuck, causing read errors.
Whether it does is again a device implementation specific thing.
+ if (ret || !status) {
+ handled = false;
+ goto done;
+ }
dev_dbg(data->dev, "DRDY trig status 0x%x\n", status);
@@ -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.
}If we are doing this Andy's suggestion of a helper is neater.
}
@@ -648,15 +652,16 @@ static irqreturn_t bm1390_trigger_handler(int irq, void *p)
&data->buf.temp, sizeof(data->buf.temp));
if (ret) {
dev_warn(data->dev, "temp read failed %d\n", ret);
- return IRQ_HANDLED;
+ goto done;
}
}
iio_push_to_buffers_with_ts(idev, &data->buf, sizeof(data->buf),
data->timestamp);
+done:
iio_trigger_notify_done(idev->trig);
- return IRQ_HANDLED;
+ return IRQ_RETVAL(handled);
Anyhow, upshot is to get this stuff right requires device specific knowledge.
And time... :)
Ideally the author tests injecting errors at each point to verify if the
data capture survives. However, it's up to a driver author to decide if they
care. There are normally dozens of paths in a driver that will result in needing
a reset (unbind/bind for most IIO drivers) - that's expensive, complex, fragile
handling code to maintain, so personally I consider it optional.
I am not going to try adding any such recovery code in driver. I am afraid it would be way too complex for me to maintain (with my memory, code I've seen last month is new Today) for the added benefit. If we have such a delicate system where this type of 'failure recovery w/o reset' is required, then such code should (in my opinion) be system specific and not generic. Most of the device users will never benefit from it, but will need to look at it...
What I DO care is the IRQ gets disabled (from host side) if it can't be acked (from device side). That shouldn't be so complex (although, it seems it is more complex I thought when I wrote this driver).
After all this babbling I've done - if I understood it right, omitting the call to iio_trigger_notify_done() will prevent further returns of the IRQ_NONE, even if the IRQ stays asserted. So yes, I would definitely like to see this fix getting in.
Thanks guys for giving me a lesson again!
}
/* Get timestamps and wake the thread if we need to read data */
Yours,
-- Matti
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~