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

[Jonathan Cameron]

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.

> ---
> 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);
So question 1.
- 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.

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.

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. This drivers trigger may be missing a reenable() callback
(which would typically reread the status register to clear any such interrupt).

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

>  		}
>  	}
>  
> @@ -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);
If we are doing this Andy's suggestion of a helper is neater.

Anyhow, upshot is to get this stuff right requires device specific knowledge.
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.

Jonathan


>  }
>  
>  /* Get timestamps and wake the thread if we need to read data */