Re: [PATCH] m68k: coldfire: Prevent spurious interrupts when masking IMR

[Geert Uytterhoeven]

Hi Jean-Michel,

On Tue, 4 Feb 2025 at 19:38, Jean-Michel Hautbois
<jeanmichel.hautbois@xxxxxxxxxx> wrote:
> The ColdFire interrupt controller can generate spurious interrupts if an
> interrupt source is masked in the IMR while the CPU interrupt priority
> mask (SR[I]) is set lower than the interrupt level.
>
> The reference manual states:
>
> To avoid this situation for interrupts sources with levels 1-6, first
> write a higher level interrupt mask to the status register, before
> setting the mask in the IMR or the module’s interrupt mask register.
> After the mask is set, return the interrupt mask in the status register
> to its previous value.
>
> It can be tested like this:
> - Prepare a iperf3 server on the coldfire target (iperf3 -s -D)
> - Start a high priority cyclictest:
>     cyclictest --secaligned -m -p 99 -i 2500 -q
> - Start iperf3 -c $COLDFIRE_IP -t 0
>
> After a few seconds the dmesg may display:
> [   84.784301] irq 24, desc: dbc502da, depth: 1, count: 0, unhandled: 0
> [   84.784455] ->handle_irq():  0ba0aca3, handle_bad_irq+0x0/0x1e0
> [   84.784610] ->irq_data.chip(): c6779d4f, 0x41652544
> [   84.784719] ->action(): 00000000
> [   84.784770] unexpected IRQ trap at vector 18
>
> With this patch, I never saw it in a few hours testing.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxx>

Thanks for your patch!

> --- a/arch/m68k/coldfire/intc-simr.c
> +++ b/arch/m68k/coldfire/intc-simr.c
> @@ -58,6 +58,14 @@ static inline unsigned int irq2ebit(unsigned int irq)
>
>  #endif
>
> +static inline void intc_irq_setlevel(unsigned long level)
> +{
> +       asm volatile ("move.w %0,%%sr"
> +                     : /* no outputs */
> +                     : "d" (0x2000 | ((level) << 8))
> +                     : "memory");
> +}
> +
>  /*
>   *     There maybe one, two or three interrupt control units, each has 64
>   *     interrupts. If there is no second or third unit then MCFINTC1_* or
> @@ -67,13 +75,17 @@ static inline unsigned int irq2ebit(unsigned int irq)
>  static void intc_irq_mask(struct irq_data *d)
>  {
>         unsigned int irq = d->irq - MCFINT_VECBASE;
> +       unsigned long flags = arch_local_save_flags();
>
> +       intc_irq_setlevel(7);

Can't all of the above just be replaced by

    unsigned long flags = arch_local_irq_save();

>         if (MCFINTC2_SIMR && (irq > 127))
>                 __raw_writeb(irq - 128, MCFINTC2_SIMR);
>         else if (MCFINTC1_SIMR && (irq > 63))
>                 __raw_writeb(irq - 64, MCFINTC1_SIMR);
>         else
>                 __raw_writeb(irq, MCFINTC0_SIMR);
> +
> +       arch_local_irq_restore(flags);
>  }
>
>  static void intc_irq_unmask(struct irq_data *d)
>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds