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

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

[Sergey Matsievskiy]

I've encountered the interrupt handle loop when using Jaguar VSC7448
chip with MAX3421, configured in level interrupt mode (default mode for
MAX3421). Upon connecting thumb drive to MAX3421, the system would
hang. After some investigation, I've traced the problem to the GPIO
interrupt controller code: by the time ocelot_irq_handler() is executed,
MAX3421 would clear its interrupt, subsequently lowering bit in GPIO
controller interrupt identify register. Because of this,
chained_irq_enter() would never be executed, and the parent interrupt
would never be cleared, resulting in a continuous interrupt handling
loop.

This could be fixed by clearing parent interrupt unconditionally, even
when no bits are set in interrupt identify registers.

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 1/1] pinctrl: ocelot: fix system hang on level based interrupts

[Sergey Matsievskiy]

Fix interrupt handle loops, produced by spurious and short level based
interrupts by unconditionally clearing the parent interrupt, even when
no GPIO interrupts are pending.

Signed-off-by: Sergey Matsievskiy <matsievskiysv@gmail.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 1/1] pinctrl: ocelot: fix system hang on level based interrupts

[Linus Walleij]

On Sun, Oct 6, 2024 at 8:13 PM Sergey Matsievskiy
<matsievskiysv@gmail.com> wrote:

> Fix interrupt handle loops, produced by spurious and short level based
> interrupts by unconditionally clearing the parent interrupt, even when
> no GPIO interrupts are pending.
>
> Signed-off-by: Sergey Matsievskiy <matsievskiysv@gmail.com>

This needs to describe how moving the chained irq calls achieves
this effect.

I'm a bit puzzled by the patch because I don't understand it.

Yours,
Linus Walleij

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

[Sergey Matsievskiy]

On Fri, Oct 11, 2024 at 11:18:55AM +0200, Linus Walleij wrote:
> I'm a bit puzzled by the patch because I don't understand it.

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
```

> This needs to describe how moving the chained irq calls achieves
> this effect.

If the explanation above satisfies you, I'll elaborate the commit message and
resend the patch.

--
Sergey Matsievskiy

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

[Linus Walleij]

On Fri, Oct 11, 2024 at 5:40 PM Sergey Matsievskiy
<matsievskiysv@gmail.com> wrote:
> On Fri, Oct 11, 2024 at 11:18:55AM +0200, Linus Walleij wrote:

> > I'm a bit puzzled by the patch because I don't understand it.
>
> 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
> ```
>
> > This needs to describe how moving the chained irq calls achieves
> > this effect.
>
> If the explanation above satisfies you, I'll elaborate the commit message and
> resend the patch.

Excellent explanation Sergey, just put it all in the committ message
and I'll apply it!

Yours,
Linus Walleij

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

[Alexandre Belloni]

On 06/10/2024 21:13:10+0300, Sergey Matsievskiy wrote:
> Fix interrupt handle loops, produced by spurious and short level based
> interrupts by unconditionally clearing the parent interrupt, even when
> no GPIO interrupts are pending.
> 
> 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
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com