irq_mask()/irq_unmask() are not called for nested interrupts. So level
interrupts are never masked, chip's interrupt output is not cleared on
INTCAP or GPIO read, the irq handler is uselessly called again. Nested
irq handler is not called again, because interrupt reason is cleared by
its first call.
/proc/interrupts shows that number of chip's irqs is greater than
number of nested irqs.

This patch adds masking and unmasking level interrupts inside irq handler.

Signed-off-by: Dmitry Mastykin <mastichi@gmail.com>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index f384c72d9554..70d7485ada36 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -382,6 +382,7 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 {
 	struct mcp23s08 *mcp = data;
 	int intcap, intcon, intf, i, gpio, gpio_orig, intcap_mask, defval, gpinten;
+	bool need_unmask = false;
 	unsigned long int enabled_interrupts;
 	unsigned int child_irq;
 	bool intf_set, intcap_changed, gpio_bit_changed,
@@ -396,9 +397,6 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 		goto unlock;
 	}
 
-	if (mcp_read(mcp, MCP_INTCAP, &intcap))
-		goto unlock;
-
 	if (mcp_read(mcp, MCP_INTCON, &intcon))
 		goto unlock;
 
@@ -408,6 +406,16 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 	if (mcp_read(mcp, MCP_DEFVAL, &defval))
 		goto unlock;
 
+	/* Mask level interrupts to avoid their immediate reactivation after clearing */
+	if (intcon) {
+		need_unmask = true;
+		if (mcp_write(mcp, MCP_GPINTEN, gpinten & ~intcon))
+			goto unlock;
+	}
+
+	if (mcp_read(mcp, MCP_INTCAP, &intcap))
+		goto unlock;
+
 	/* This clears the interrupt(configurable on S18) */
 	if (mcp_read(mcp, MCP_GPIO, &gpio))
 		goto unlock;
@@ -470,9 +478,18 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 		}
 	}
 
+	if (need_unmask) {
+		mutex_lock(&mcp->lock);
+		goto unlock;
+	}
+
 	return IRQ_HANDLED;
 
 unlock:
+	if (need_unmask)
+		if (mcp_write(mcp, MCP_GPINTEN, gpinten))
+			dev_err(mcp->chip.parent, "can't unmask GPINTEN\n");
+
 	mutex_unlock(&mcp->lock);
 	return IRQ_HANDLED;
 }
-- 
2.34.1

On Wed, Jan 22, 2025 at 1:05 PM Dmitry Mastykin <mastichi@gmail.com> wrote:

> irq_mask()/irq_unmask() are not called for nested interrupts. So level
> interrupts are never masked, chip's interrupt output is not cleared on
> INTCAP or GPIO read, the irq handler is uselessly called again. Nested
> irq handler is not called again, because interrupt reason is cleared by
> its first call.
> /proc/interrupts shows that number of chip's irqs is greater than
> number of nested irqs.
>
> This patch adds masking and unmasking level interrupts inside irq handler.
>
> Signed-off-by: Dmitry Mastykin <mastichi@gmail.com>

Patch tentatively applied as non-urgent fix.

If this is urgent, I need a Fixes: tags and we should also tag it
for stable, is this a big problem for users? I don't have the big picture
here.

Adding Sebastian, if he's still using this expander.

Yours,
Linus Walleij

Hi,

On Thu, Feb 06, 2025 at 10:25:51AM +0100, Linus Walleij wrote:
> On Wed, Jan 22, 2025 at 1:05 PM Dmitry Mastykin <mastichi@gmail.com> wrote:
> 
> > irq_mask()/irq_unmask() are not called for nested interrupts. So level
> > interrupts are never masked, chip's interrupt output is not cleared on
> > INTCAP or GPIO read, the irq handler is uselessly called again. Nested
> > irq handler is not called again, because interrupt reason is cleared by
> > its first call.
> > /proc/interrupts shows that number of chip's irqs is greater than
> > number of nested irqs.
> >
> > This patch adds masking and unmasking level interrupts inside irq handler.
> >
> > Signed-off-by: Dmitry Mastykin <mastichi@gmail.com>
> 
> Patch tentatively applied as non-urgent fix.
> 
> If this is urgent, I need a Fixes: tags and we should also tag it
> for stable, is this a big problem for users? I don't have the big picture
> here.
> 
> Adding Sebastian, if he's still using this expander.

I use the 16 bit I2C version of this chip in the hackerspace
together with gpio-keys and haven't noticed any IRQ issues. But the
system is running a Debian stable (incl. its kernel), so quite far
from mainline :)

Greetings,

-- Sebastian

Hi Linus,
thank you for the answer.
I made more tests and think that this patch shouldn't be applied.
It removes duplicated interrupts, but sometimes they increase the performance:
a new interrupt may come during handling a spurious one, and a
spurious one will become valid (resulting in a kind of polling). I see
the number of my touchscreen interrupts reduced from 200 to 130 per
second with this patch. It may be a bigger problem for users, than
duplicated interrupts. Sorry.

Kind regards,
Dmitry

On Thu, Feb 6, 2025 at 12:26 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Jan 22, 2025 at 1:05 PM Dmitry Mastykin <mastichi@gmail.com> wrote:
>
> > irq_mask()/irq_unmask() are not called for nested interrupts. So level
> > interrupts are never masked, chip's interrupt output is not cleared on
> > INTCAP or GPIO read, the irq handler is uselessly called again. Nested
> > irq handler is not called again, because interrupt reason is cleared by
> > its first call.
> > /proc/interrupts shows that number of chip's irqs is greater than
> > number of nested irqs.
> >
> > This patch adds masking and unmasking level interrupts inside irq handler.
> >
> > Signed-off-by: Dmitry Mastykin <mastichi@gmail.com>
>
> Patch tentatively applied as non-urgent fix.
>
> If this is urgent, I need a Fixes: tags and we should also tag it
> for stable, is this a big problem for users? I don't have the big picture
> here.
>
> Adding Sebastian, if he's still using this expander.
>
> Yours,
> Linus Walleij