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

[Frederic Weisbecker]

Le Wed, Mar 19, 2025 at 12:07:07PM +0100, Thomas Bogendoerfer a écrit :
> On Sat, Mar 15, 2025 at 08:40:02PM +0100, Marco Crivellari wrote:
> > MIPS re-enables interrupts on its idle routine and performs
> > a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep.
> > 
> > The IRQs firing between the check and the 'wait' instruction may set the
> > TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs
> > interrupting __r4k_wait() rollback their return address to the
> > beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked
> > again before going back to sleep.
> > 
> > However idle IRQs can also queue timers that may require a tick
> > reprogramming through a new generic idle loop iteration but those timers
> > would go unnoticed here because __r4k_wait() only checks
> > TIF_NEED_RESCHED. It doesn't check for pending timers.
> 
> can you give a commit ID, when this change got introduced ?
> 
> > Fix this with fast-forwarding idle IRQs return address to the end of the
> > idle routine instead of the beginning, so that the generic idle loop
> > handles both TIF_NEED_RESCHED and pending timers.
> > 
> > CONFIG_CPU_MICROMIPS has been removed along with the nop instructions.
> > There, NOPs are 2 byte in size, so change the code with 3 _ssnop which are
> > always 4 byte and remove the ifdef. Added ehb to make sure the hazard
> > is always cleared.
> > 
> > Signed-off-by: Marco Crivellari <marco.crivellari@xxxxxxxx>
> > ---
> >  arch/mips/kernel/genex.S | 42 ++++++++++++++++++++++------------------
> >  arch/mips/kernel/idle.c  |  1 -
> >  2 files changed, 23 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > index a572ce36a24f..4e012421d00f 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 interrupt. */
> > +	ori 	t0, 0x1f
> > +	xori	t0, 0x1e
> > +	MTC0	t0, CP0_STATUS
> > +	_ssnop
> > +	_ssnop
> > +	_ssnop
> > +	_ehb
> >  	.set	MIPS_ISA_ARCH_LEVEL_RAW
> > +	/*
> > +	 * If an interrupt lands here, between enabling interrupts above and
> > +	 * going idle on the next instruction, we must *NOT* go idle since the
> > +	 * interrupt could have set TIF_NEED_RESCHED or caused a timer to need
> > +	 * resched. Fall through -- see rollback_handler below -- and have
> > +	 * the idle loop take care of things.
> > +	 */
> >  	wait
> > -	/* end of rollback region (the region size must be power of two) */
> > +	/* End of idle interrupt region. */
> >  1:
> 
> please give this label a name for example __r4k_wait_exit and do a
> runtime check that it really has 36 bytes offset to __r4k_wait

Where would be the best place for that?

arch/mips/kernel/setup.c:setup_arch() maybe?

Thanks.