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

[Maciej W. Rozycki]

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.

> +	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