| Message ID | 20241006181310.181309-2-matsievskiysv@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pinctrl: ocelot: fix system hang on level based interrupts | expand |
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
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, ®); 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
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, ®); > 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
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, ®); > 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 >
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, ®); 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,
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(-)