Re: [PATCH 1/5] irqchip/renesas-rzg2l: Prevent IRQ HW race

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Feb 12 2024 at 11:37, Biju Das wrote:
> As per section "8.8.2 Clear Timing of Interrupt Cause" of the RZ/G2L
> hardware manual (Rev.1.45 Jan, 2024), it is mentioned that we need to
> clear the interrupt cause flag in the isr.

We need to clear? Please write changelogs in neutral tone. Also use
proper words instead of acronyms, this is not twatter.

I'm also failing to see the value of above sentence other that it
occupies space. The code already does that, no?

> It takes some time for the cpu to clear the interrupt cause
> flag. Therefore, to prevent another occurrence of interrupt due to
> this delay, the interrupt cause flag is read after clearing.

You really want to explain explicitely what the problem is. The above is
a novel 

Something like this:

  The irq_eoi() callback of the RX/G2L interrupt chip clears the
  relevant interrupt cause bit in the TSCR register.

  This write is not sufficient because the write is posted and therefore
  not guaranteed to immediately clear the bit. Due to that delay the CPU
  can raise the just handled interrupt again.

  Prevent this by reading the register back which causes the posted
  write to be flushed to the hardware before the read completes.

This uses the proper technical term 'posted write' which is well defined
and exactly the cause of the problem, no?

> Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/irqchip/irq-renesas-rzg2l.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> index 9494fc26259c..46f9b07e0e8a 100644
> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -111,8 +111,11 @@ static void rzg2l_tint_eoi(struct irq_data *d)
>  	u32 reg;
>  
>  	reg = readl_relaxed(priv->base + TSCR);
> -	if (reg & bit)
> +	if (reg & bit) {
>  		writel_relaxed(reg & ~bit, priv->base + TSCR);
> +		/* Read to avoid irq generation due to irq clearing delay */

		/*
                 * Enforce that the posted write is flushed to prevent
                 * that the just handled interrupt is raised again.
                 */

Hmm?

> +		readl_relaxed(priv->base + TSCR);

Thanks,

        tglx