Re: [PATCH] irqchip/sifive-plic: Unmask interrupt in plic_irq_enable()

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Sep 26 2024 at 17:43, Nam Cao wrote:
> If another task disables the interrupt in the middle of the above steps,
> the interrupt will not get unmasked, and will remain masked when it is
> enabled in the future.
>
> The problem is occasionally observed when PREEMPT_RT is enabled, because
> PREEMPT_RT add the IRQS_ONESHOT flag. But PREEMPT_RT only makes the
> problem more likely to appear, the bug has been around since
> commit a1706a1c5062 ("irqchip/sifive-plic: Separate the enable and mask
> operations").

Correct. It's a general problem independent of RT.

> Fix it by unmasking interrupt in plic_irq_enable().
>
> Fixes: a1706a1c5062 ("irqchip/sifive-plic: Separate the enable and mask operations").
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  drivers/irqchip/irq-sifive-plic.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 2f6ef5c495bd..0efbf14ec9fa 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -128,6 +128,9 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
>  
>  static void plic_irq_enable(struct irq_data *d)
>  {
> +	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
> +
> +	writel(1, priv->regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
>  	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);

Can you please move plic_irq_enable() below plic_irq_unmask() and invoke
the latter instead of duplicating the code?

Also usually unmask() is done after enable(), but the ordering probably
does not matter here.

Thanks,

        tglx