linux-kernel - Re: [PATCH] counter: 104-quad-8: Fix incorrect return value in IRQ handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aTRaQzwbHXhGKE_n@smile.fi.intel.com>
Date: Sat, 6 Dec 2025 18:30:59 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: William Breathitt Gray <wbg@...nel.org>
Cc: Haotian Zhang <vulab@...as.ac.cn>, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] counter: 104-quad-8: Fix incorrect return value in IRQ
 handler

On Sat, Dec 06, 2025 at 01:24:16PM +0900, William Breathitt Gray wrote:
> On Tue, Dec 02, 2025 at 04:39:52PM +0800, Haotian Zhang wrote:
> > quad8_irq_handler() should return irqreturn_t enum values, but it
> > directly returns negative errno codes from regmap operations on error.
> > 
> > Return IRQ_NONE instead of raw errno codes on regmap operation failures.

...

> >  	ret = regmap_read(priv->map, QUAD8_INTERRUPT_STATUS, &status);
> >  	if (ret)
> > -		return ret;
> > +		return IRQ_NONE;

This return is correct and we can't do much, if we got again to this handler
due to unserviced IRQ, hopefully the second attempt will succeed. At the end it
will mean something really bad happened to the HW state.

> >  	if (!status)
> >  		return IRQ_NONE;

...

> >  	ret = regmap_write(priv->map, QUAD8_CHANNEL_OPERATION, CLEAR_PENDING_INTERRUPTS);
> >  	if (ret)
> > -		return ret;
> > +		return IRQ_NONE;
> > 
> >  	return IRQ_HANDLED;
> 
> You are correct, we should return a value of irqreturn_t and not raw
> errno codes. However, it would be nice to indicate to users why the IRQ
> was left unserviced before return IRQ_NONE. Is there a way to indicate
> the regmap_read failure, perhaps via WARN_ONCE() or similar? Is
> regmap_read actually capable of failing in this context, or should we
> just remove the conditional check entirely?

I'm not sure about this case, clearing pending interrupts is something
that should not affect much the flow, I think if we return IRQ_HANDLED here
we can re-enter to the handler and re-read the status. Yes, it will be
spurious interrupt, but at least it will reduce the probability of IRQ storm
registration (when when we return IRQ_NONE).

-- 
With Best Regards,
Andy Shevchenko