RE: [PATCH 3/3] irqchip: renesas-rzg2l: Fix irq storm with edge trigger detection for TINT

[Biju Das]

Hi Marc Zyngier,

Thanks for the feedback.

> Subject: Re: [PATCH 3/3] irqchip: renesas-rzg2l: Fix irq storm with edge
> trigger detection for TINT
> 
> On Mon, 18 Sep 2023 13:24:11 +0100,
> Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> >
> > In case of edge trigger detection, enabling the TINT source causes a
> > phantum interrupt that leads to irq storm. So clear the phantum
> > interrupt in rzg2l_irqc_irq_enable().
> >
> > This issue is observed when the irq handler disables the interrupts
> > using
> > disable_irq_nosync() and scheduling a work queue and in the work
> > queue, re-enabling the interrupt with enable_irq().
> >
> > Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller
> > driver")
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > ---
> >  drivers/irqchip/irq-renesas-rzg2l.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c
> > b/drivers/irqchip/irq-renesas-rzg2l.c
> > index 33a22bafedcd..78a9e90512a6 100644
> > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > @@ -144,6 +144,12 @@ static void rzg2l_irqc_irq_enable(struct irq_data
> *d)
> >  		reg = readl_relaxed(priv->base + TSSR(tssr_index));
> >  		reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset);
> >  		writel_relaxed(reg, priv->base + TSSR(tssr_index));
> > +		/*
> > +		 * In case of edge trigger detection, enabling the TINT source
> > +		 * cause a phantum interrupt that leads to irq storm. So clear
> > +		 * the phantum interrupt.
> > +		 */
> > +		rzg2l_tint_eoi(d);
> 
> This looks incredibly unsafe. disable_irq()+enable_irq() with an interrupt
> being made pending in the middle, and you've lost that interrupt.

In this driver that will never happen as it clears the TINT source
during disable(), so there won't be any TINT source for interrupt detection after disable().

Cheers,
Biju

> What prevents this scenario?