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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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@vger.kernel.org
>> Signed-off-by: Stepan Ionichev <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..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! ~~