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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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@gmail.com> 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@vger.kernel.org
>> Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
> 
> 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@gmail.com/
>>
>>   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.

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.

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

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! ~~