| Message ID | 20250122120504.1279790-1-mastichi@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: mcp23s08: Get rid of spurious level interrupts | expand |
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 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
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
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
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
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
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; }
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(-)