Re: [PATCH v5 1/1] MIPS: Fix idle VS timer enqueue

From: Huacai Chen
Date: Mon Mar 03 2025 - 03:14:03 EST


Hi, Maciej,

On Sun, Mar 2, 2025 at 8:54 AM Maciej W. Rozycki <macro@xxxxxxxxxxx> wrote:
>
> On Fri, 28 Feb 2025, Marco Crivellari wrote:
>
> > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > index a572ce36a24f..474738d9124e 100644
> > --- a/arch/mips/kernel/genex.S
> > +++ b/arch/mips/kernel/genex.S
> > @@ -104,27 +104,30 @@ handle_vcei:
> >
> > __FINIT
> >
> > - .align 5 /* 32 byte rollback region */
> > + .align 5
> > LEAF(__r4k_wait)
> > .set push
> > .set noreorder
> > - /* start of rollback region */
> > - LONG_L t0, TI_FLAGS($28)
> > - nop
> > - andi t0, _TIF_NEED_RESCHED
> > - bnez t0, 1f
> > - nop
> > - nop
> > - nop
> > -#ifdef CONFIG_CPU_MICROMIPS
> > - nop
> > - nop
> > - nop
> > - nop
> > -#endif
> > + /* start of idle interrupt region */
> > + MFC0 t0, CP0_STATUS
> > + /* Enable Interrput */
> ^^
> Typo here; also please capitalise sentences and use full stops.
>
> > + ori t0, 0x1f
>
> No time for a thorough review here as I'm heading for a holiday right
> away, but I can see you still have a coprocessor move hazard here with
> MIPS III hardware. The incoming value of $t0 to ORI is not what MFC0 has
> obtained. It's the value from before MFC0.
If this is a problem, then the current local_irq_enable() is also
wrong for all MIPS III hardware, because this patch uses the same
instruction sequence of local_irq_enable().


Huacai

>
> > + xori t0, 0x1e
>
> And then it's only this XORI that sees the output from MFC0 (though
> there's actually a race between the two competing writes to $t0), so
> effectively you clobber the CP0.Status register...
>
> > + MTC0 t0, CP0_STATUS
>
> ... here. You need to schedule your instructions differently. But do
> you need `.set noreorder' in the first place? Can you maybe find a way to
> avoid it, making all the hazard avoidance stuff much easier?
>
> Maciej