[PATCH v2 0/1] pinctrl: ocelot: fix system hang on level based interrupts

[PATCH v2 0/1] pinctrl: ocelot: fix system hang on level based interrupts

[Sergey Matsievskiy]

Fix system hang in case of GPIO pin interrupt configured in level mode
and the parent controller configured in edge mode.
Observed on Jaguar2 VSC7448 connected to MAX3421.

v2:
 - Change commit description
 - Add Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

v1:
Link: https://lore.kernel.org/linux-mips/20241006181310.181309-1-matsievskiysv@gmail.com/

Sergey Matsievskiy (1):
  pinctrl: ocelot: fix system hang on level based interrupts

 drivers/pinctrl/pinctrl-ocelot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.39.5

[PATCH v2 1/1] pinctrl: ocelot: fix system hang on level based interrupts

[Sergey Matsievskiy]

The current implementation only calls chained_irq_enter() and
chained_irq_exit() if it detects pending interrupts.

```
for (i = 0; i < info->stride; i++) {
	uregmap_read(info->map, id_reg + 4 * i, &reg);
	if (!reg)
		continue;

	chained_irq_enter(parent_chip, desc);
```

However, in case of GPIO pin configured in level mode and the parent
controller configured in edge mode, GPIO interrupt might be lowered by the
hardware. In the result, if the interrupt is short enough, the parent
interrupt is still pending while the GPIO interrupt is cleared;
chained_irq_enter() never gets called and the system hangs trying to
service the parent interrupt.

Moving chained_irq_enter() and chained_irq_exit() outside the for loop
ensures that they are called even when GPIO interrupt is lowered by the
hardware.

The similar code with chained_irq_enter() / chained_irq_exit() functions
wrapping interrupt checking loop may be found in many other drivers:
```
grep -r -A 10 chained_irq_enter drivers/pinctrl
```

Signed-off-by: Sergey Matsievskiy <matsievskiysv@gmail.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/pinctrl/pinctrl-ocelot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index be9b8c010167..d1ab8450ea93 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -1955,21 +1955,21 @@ static void ocelot_irq_handler(struct irq_desc *desc)
 	unsigned int reg = 0, irq, i;
 	unsigned long irqs;
 
+	chained_irq_enter(parent_chip, desc);
+
 	for (i = 0; i < info->stride; i++) {
 		regmap_read(info->map, id_reg + 4 * i, &reg);
 		if (!reg)
 			continue;
 
-		chained_irq_enter(parent_chip, desc);
-
 		irqs = reg;
 
 		for_each_set_bit(irq, &irqs,
 				 min(32U, info->desc->npins - 32 * i))
 			generic_handle_domain_irq(chip->irq.domain, irq + 32 * i);
-
-		chained_irq_exit(parent_chip, desc);
 	}
+
+	chained_irq_exit(parent_chip, desc);
 }
 
 static int ocelot_gpiochip_register(struct platform_device *pdev,
-- 
2.39.5

Re: [PATCH v2 1/1] pinctrl: ocelot: fix system hang on level based interrupts

[Linus Walleij]

On Sat, Oct 12, 2024 at 12:58 PM Sergey Matsievskiy
<matsievskiysv@gmail.com> wrote:

> The current implementation only calls chained_irq_enter() and
> chained_irq_exit() if it detects pending interrupts.
>
> ```
> for (i = 0; i < info->stride; i++) {
>         uregmap_read(info->map, id_reg + 4 * i, &reg);
>         if (!reg)
>                 continue;
>
>         chained_irq_enter(parent_chip, desc);
> ```
>
> However, in case of GPIO pin configured in level mode and the parent
> controller configured in edge mode, GPIO interrupt might be lowered by the
> hardware. In the result, if the interrupt is short enough, the parent
> interrupt is still pending while the GPIO interrupt is cleared;
> chained_irq_enter() never gets called and the system hangs trying to
> service the parent interrupt.
>
> Moving chained_irq_enter() and chained_irq_exit() outside the for loop
> ensures that they are called even when GPIO interrupt is lowered by the
> hardware.
>
> The similar code with chained_irq_enter() / chained_irq_exit() functions
> wrapping interrupt checking loop may be found in many other drivers:
> ```
> grep -r -A 10 chained_irq_enter drivers/pinctrl
> ```
>
> Signed-off-by: Sergey Matsievskiy <matsievskiysv@gmail.com>
> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Patch applied and tagged for stable!

Yours,
Linus Walleij

Re: [PATCH v2 1/1] pinctrl: ocelot: fix system hang on level based interrupts

[Sergey Matsievskiy]

On Sat, Oct 12, 2024 at 10:04:30PM +0200, Linus Walleij wrote:
> Patch applied and tagged for stable!

Awesome, thanks!

--
Sergey Matsievskiy

Re: [PATCH v2 1/1] pinctrl: ocelot: fix system hang on level based interrupts

[Andy Shevchenko]

On Sat, Oct 12, 2024 at 01:57:43PM +0300, Sergey Matsievskiy wrote:

...

> The similar code with chained_irq_enter() / chained_irq_exit() functions
> wrapping interrupt checking loop may be found in many other drivers:
> ```
> grep -r -A 10 chained_irq_enter drivers/pinctrl
> ```

Side note: `git grep ...` is much much faster if you have a Git tree at hand.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] pinctrl: ocelot: fix system hang on level based interrupts

[Sergey Matsievskiy]

On Sun, Oct 13, 2024 at 10:07:04PM +0300, Andy Shevchenko wrote:
> Side note: `git grep ...` is much much faster if you have a Git tree at hand.

Thanks for the tip! I usually use ripgrep, but git grep has some interesting
features.