[PATCH] pinctrl: mcp23s08: Get rid of spurious level interrupts

[PATCH] pinctrl: mcp23s08: Get rid of spurious level interrupts

[Dmitry Mastykin]

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

Re: [PATCH] pinctrl: mcp23s08: Get rid of spurious level interrupts

[Linus Walleij]

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

Re: [PATCH] pinctrl: mcp23s08: Get rid of spurious level interrupts

[Dmitry Mastykin]

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

Re: [PATCH] pinctrl: mcp23s08: Get rid of spurious level interrupts

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] pinctrl: mcp23s08: Get rid of spurious level interrupts

[Linus Walleij]

On Fri, Feb 7, 2025 at 9:36 PM Dmitry Mastykin <mastichi@gmail.com> wrote:

> 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 spurious one will
> become valid (it's kind of a 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.

Don't be sorry about that, we code and learn by our mistakes.

So is this patch causing any regression for users? Like touch
events being slow to react? Also the expander could be used
for other things than touchscreens. If it's not causing any regression
for users it seems like a reasonable patch.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: mcp23s08: Get rid of spurious level interrupts

[Dmitry Mastykin]

Thank you, Linus. No, I have no users. It's only a prototype, using a
touchscreen. I think it has to be redesigned using chipset interrupt
controller's pin instead of the expander to speed-up, although I don't
feel touch gets slower. I spoke about hypothetical users who may use
the expander as an interrupt controller at rates comparable to
mcp23s08_irq() execution time, and may get less interrupts per second.
Kind regards,
Dmitry

On Fri, Feb 14, 2025 at 12:10 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Feb 7, 2025 at 9:36 PM Dmitry Mastykin <mastichi@gmail.com> wrote:
>
> > 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 spurious one will
> > become valid (it's kind of a 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.
>
> Don't be sorry about that, we code and learn by our mistakes.
>
> So is this patch causing any regression for users? Like touch
> events being slow to react? Also the expander could be used
> for other things than touchscreens. If it's not causing any regression
> for users it seems like a reasonable patch.
>
> Yours,
> Linus Walleij

Re: [PATCH] pinctrl: mcp23s08: Get rid of spurious level interrupts

[Linus Walleij]

On Mon, Feb 17, 2025 at 8:18 PM Dmitry Mastykin <mastichi@gmail.com> wrote:

> Thank you, Linus. No, I have no users. It's only a prototype, using a
> touchscreen. I think it has to be redesigned using chipset interrupt
> controller's pin instead of the expander to speed-up, although I don't
> feel touch gets slower. I spoke about hypothetical users who may use
> the expander as an interrupt controller at rates comparable to
> mcp23s08_irq() execution time, and may get less interrupts per second.

Well I think the patch is completely reasonable so I'll just leave
it in for now. If it causes problems we can revert it.

Yours,
Linus Walleij