On Fri, 14 Feb 2025, Marco Crivellari wrote: > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > index a572ce36a24f..a78d5132c940 100644 > --- a/arch/mips/kernel/genex.S > +++ b/arch/mips/kernel/genex.S > @@ -104,18 +104,16 @@ 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 > + /* start of idle interrupt region */ > + MFC0 k0, CP0_STATUS > + /* Enable Interrupt */ > + ori k0, 0x1f Data from CP0 will be delivered late to $k0 on MIPS III, you need to fill the coprocessor move delay slot here. Existing code takes care about it. I bet you won't trigger this with QEMU, you need to verify with real hw. For CONFIG_CPU_HAS_DIEI you want EI here instead. > + xori k0, 0x1e > + MTC0 k0, CP0_STATUS You need to clear the CP0 hazard here, just as `raw_local_irq_enable' does, because WAIT halts the pipeline and the interrupt enable state may not have been propagated beforehand otherwise. Again, not triggerable with QEMU. > @@ -123,11 +121,17 @@ LEAF(__r4k_wait) > nop > #endif > .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) */ > -1: > + /* end of idle interrupt region (the region size must be power of two) */ Have you verified this still stands with CONFIG_CPU_MICROMIPS after your change? > +SYM_INNER_LABEL(__r4k_wait_exit, SYM_L_LOCAL) > jr ra > - nop > .set pop > END(__r4k_wait) You're dropping the delay slot NOP here, meaning that whatever comes next gets there, possibly "mfc0 $k0, $c0_epc" (depending on alignment), why? > @@ -136,10 +140,10 @@ LEAF(__r4k_wait) > .set push > .set noat > MFC0 k0, CP0_EPC > - PTR_LA k1, __r4k_wait > - ori k0, 0x1f /* 32 byte rollback region */ > - xori k0, 0x1f > - bne k0, k1, \handler > + PTR_LA k1, __r4k_wait_exit > + /* 3 instructions rollback region */ > + ori k0, k0, 0x0c > + bne k0, k1, \handler How is `__r4k_wait_exit' guaranteed to have bits 3:2 set and bit 1 (for CONFIG_CPU_MICROMIPS) clear? Also EPC may or may not have bit 1 set. NB not a full review and certainly not for the change of semantics, but for some issues spotted with the low-level correctness of new code. Maciej
