| Message ID | 20250218090203.43137-2-marco.crivellari@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | MIPS: Fix idle VS timer enqueue | expand |
On Tue, Feb 18, 2025 at 10:02:03AM +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. > > 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. > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> > --- > arch/mips/kernel/genex.S | 39 +++++++++++++++++++++------------------ > arch/mips/kernel/idle.c | 1 - > 2 files changed, 21 insertions(+), 19 deletions(-) > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > index a572ce36a24f..9747b216648f 100644 > --- a/arch/mips/kernel/genex.S > +++ b/arch/mips/kernel/genex.S > @@ -104,25 +104,27 @@ 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 My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but they are here for a purpose. I might still need them... > + /* start of idle interrupt region */ > + MFC0 t0, CP0_STATUS > + /* Enable Interrput */ > + ori t0, 0x1f > + xori t0, 0x1e > + MTC0 t0, CP0_STATUS > + _ssnop > + _ssnop > + _ssnop instead of handcoded hazard nops, use __irq_enable_hazard for that > .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: > @@ -136,9 +138,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 > + PTR_LA k1, 1b > + /* 32 byte idle interrupt region */ > + ori k0, 0x1f > + daddiu k0, 1 /local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S: Assembler messages: /local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S:151: Error: opcode not supported on this processor: mips32r2 (mips32r2) `daddiu $26,1' /local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S:271: Error: opcode not supported on this processor: mips32r2 (mips32r2) `daddiu $26,1' looks like you haven't compiled this code for 32bit. Use PTR_ADDIU, which will use the correct instuction for 32 and 64bit. But I doubt this works, because the wait instruction is not aligned to a 32 byte boundary, but the code assuemes this, IMHO. Thomas.
Hi, Thomas, On Tue, Feb 18, 2025 at 7:59 PM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: > > On Tue, Feb 18, 2025 at 10:02:03AM +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. > > > > 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. > > > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> > > --- > > arch/mips/kernel/genex.S | 39 +++++++++++++++++++++------------------ > > arch/mips/kernel/idle.c | 1 - > > 2 files changed, 21 insertions(+), 19 deletions(-) > > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > > index a572ce36a24f..9747b216648f 100644 > > --- a/arch/mips/kernel/genex.S > > +++ b/arch/mips/kernel/genex.S > > @@ -104,25 +104,27 @@ 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 > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but > they are here for a purpose. I might still need them... The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is always 4 bytes, so we can remove #ifdefs. > > > + /* start of idle interrupt region */ > > + MFC0 t0, CP0_STATUS > > + /* Enable Interrput */ > > + ori t0, 0x1f > > + xori t0, 0x1e > > + MTC0 t0, CP0_STATUS > > + _ssnop > > + _ssnop > > + _ssnop > > instead of handcoded hazard nops, use __irq_enable_hazard for that No, I don't think so, this region should make sure be 32 bytes on each platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the fallback implementation but available for all MIPS. > > > .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: > > @@ -136,9 +138,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 > > + PTR_LA k1, 1b > > + /* 32 byte idle interrupt region */ > > + ori k0, 0x1f > > + daddiu k0, 1 > > /local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S: Assembler messages: > /local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S:151: Error: opcode not supported on this processor: mips32r2 (mips32r2) `daddiu $26,1' > /local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S:271: Error: opcode not supported on this processor: mips32r2 (mips32r2) `daddiu $26,1' > > looks like you haven't compiled this code for 32bit. Use PTR_ADDIU, which > will use the correct instuction for 32 and 64bit. Sorry, this is my mistake. > > But I doubt this works, because the wait instruction is not aligned to > a 32 byte boundary, but the code assuemes this, IMHO. Why? After this patch we only use 4 byte instructions. Huacai > > Thomas. > > -- > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > good idea. [ RFC1925, 2.3 ]
On Tue, Feb 18, 2025 at 08:14:43PM +0800, Huacai Chen wrote: > Hi, Thomas, > > On Tue, Feb 18, 2025 at 7:59 PM Thomas Bogendoerfer > <tsbogend@alpha.franken.de> wrote: > > > > On Tue, Feb 18, 2025 at 10:02:03AM +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. > > > > > > 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. > > > > > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> > > > --- > > > arch/mips/kernel/genex.S | 39 +++++++++++++++++++++------------------ > > > arch/mips/kernel/idle.c | 1 - > > > 2 files changed, 21 insertions(+), 19 deletions(-) > > > > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > > > index a572ce36a24f..9747b216648f 100644 > > > --- a/arch/mips/kernel/genex.S > > > +++ b/arch/mips/kernel/genex.S > > > @@ -104,25 +104,27 @@ 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 > > > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but > > they are here for a purpose. I might still need them... > The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in > MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is > always 4 bytes, so we can remove #ifdefs. ic > > > + _ssnop > > > + _ssnop > > > + _ssnop > > > > instead of handcoded hazard nops, use __irq_enable_hazard for that > No, I don't think so, this region should make sure be 32 bytes on each > platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the > fallback implementation but available for all MIPS. you are right for most cases, but there is one case #elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \ defined(CONFIG_CPU_BMIPS) which uses #define __irq_enable_hazard \ ___ssnop; \ ___ssnop; \ ___ssnop; \ ___ehb if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as irq enable hazard. > > But I doubt this works, because the wait instruction is not aligned to > > a 32 byte boundary, but the code assuemes this, IMHO. > Why? After this patch we only use 4 byte instructions. I've should have looked at the compiled output, sorry for the noise Still this construct feels rather fragile. Thomas.
On Tue, 18 Feb 2025, Thomas Bogendoerfer wrote: > > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > > > > index a572ce36a24f..9747b216648f 100644 > > > > --- a/arch/mips/kernel/genex.S > > > > +++ b/arch/mips/kernel/genex.S > > > > @@ -104,25 +104,27 @@ 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 > > > > > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but > > > they are here for a purpose. I might still need them... > > The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in > > MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is > > always 4 bytes, so we can remove #ifdefs. > > ic This was wrong anyway (and I had arguments about it with people back in the time, but it was a hopeless battle). Instead `.align ...' or an equivalent pseudo-op (`.balign', etc.) should have been used, removing the fragility of this piece or the need for CONFIG_CPU_MICROMIPS conditional. Here and in other places. > > > > + _ssnop > > > > + _ssnop > > > > + _ssnop > > > > > > instead of handcoded hazard nops, use __irq_enable_hazard for that > > No, I don't think so, this region should make sure be 32 bytes on each > > platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the > > fallback implementation but available for all MIPS. > > you are right for most cases, but there is one case > > #elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \ > defined(CONFIG_CPU_BMIPS) > > which uses > > #define __irq_enable_hazard \ > ___ssnop; \ > ___ssnop; \ > ___ssnop; \ > ___ehb > > if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as > irq enable hazard. Indeed, EHB must always be included, because for R2+ CPUs SSNOP doesn't clear the hazard (it never clears, but with earlier CPUs it just stalls the pipeline long enough for the hazard to eventually clear by itself). > > > But I doubt this works, because the wait instruction is not aligned to > > > a 32 byte boundary, but the code assuemes this, IMHO. > > Why? After this patch we only use 4 byte instructions. > > I've should have looked at the compiled output, sorry for the noise Umm, there's the commit description to document justification for such changes made and it's not the reviewer's duty to chase every such detail that has been omitted from a submission. FAOD this is a request to include this detail in v3. > Still this construct feels rather fragile. I think `.align', etc. can still be used to ensure enough space has been consumed and if you want to make a code sequence's size check, then a piece such as: 0: nop nop nop nop 1: .ifne 1b - 0b - 32 .error "code consistency verification failure" .endif should do even where alignment does not matter. And you could possibly do smarter stuff with `.rept' if (1b - 0b) turns out below rather than above the threshold required, but I'm leaving it as an exercise for the reader. Maciej
On Tue, Feb 18, 2025 at 9:51 PM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: > > On Tue, Feb 18, 2025 at 08:14:43PM +0800, Huacai Chen wrote: > > Hi, Thomas, > > > > On Tue, Feb 18, 2025 at 7:59 PM Thomas Bogendoerfer > > <tsbogend@alpha.franken.de> wrote: > > > > > > On Tue, Feb 18, 2025 at 10:02:03AM +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. > > > > > > > > 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. > > > > > > > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> > > > > --- > > > > arch/mips/kernel/genex.S | 39 +++++++++++++++++++++------------------ > > > > arch/mips/kernel/idle.c | 1 - > > > > 2 files changed, 21 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > > > > index a572ce36a24f..9747b216648f 100644 > > > > --- a/arch/mips/kernel/genex.S > > > > +++ b/arch/mips/kernel/genex.S > > > > @@ -104,25 +104,27 @@ 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 > > > > > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but > > > they are here for a purpose. I might still need them... > > The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in > > MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is > > always 4 bytes, so we can remove #ifdefs. > > ic > > > > > + _ssnop > > > > + _ssnop > > > > + _ssnop > > > > > > instead of handcoded hazard nops, use __irq_enable_hazard for that > > No, I don't think so, this region should make sure be 32 bytes on each > > platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the > > fallback implementation but available for all MIPS. > > you are right for most cases, but there is one case > > #elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \ > defined(CONFIG_CPU_BMIPS) > > which uses > > #define __irq_enable_hazard \ > ___ssnop; \ > ___ssnop; \ > ___ssnop; \ > ___ehb > > if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as > irq enable hazard. Emm, this is a problem. I think we can add _ehb after 3 _ssnop. And then change the below "daddiu k0, 1" to "PTR_ADDIU k0, 5". Maybe there is a better solution, but I think this is the simplest. Huacai > > > > But I doubt this works, because the wait instruction is not aligned to > > > a 32 byte boundary, but the code assuemes this, IMHO. > > Why? After this patch we only use 4 byte instructions. > > I've should have looked at the compiled output, sorry for the noise > > Still this construct feels rather fragile. > > Thomas. > > -- > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > good idea. [ RFC1925, 2.3 ]
Hi, Sorry for the late reply. > Umm, there's the commit description to document justification for such >changes made and it's not the reviewer's duty to chase every such detail >that has been omitted from a submission. > >FAOD this is a request to include this detail in v3. How does it sound integrate the v3 commit message with: 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. something more accurate to suggest? Thank you On Tue, Feb 18, 2025 at 4:23 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote: > > On Tue, 18 Feb 2025, Thomas Bogendoerfer wrote: > > > > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > > > > > index a572ce36a24f..9747b216648f 100644 > > > > > --- a/arch/mips/kernel/genex.S > > > > > +++ b/arch/mips/kernel/genex.S > > > > > @@ -104,25 +104,27 @@ 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 > > > > > > > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but > > > > they are here for a purpose. I might still need them... > > > The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in > > > MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is > > > always 4 bytes, so we can remove #ifdefs. > > > > ic > > This was wrong anyway (and I had arguments about it with people back in > the time, but it was a hopeless battle). Instead `.align ...' or an > equivalent pseudo-op (`.balign', etc.) should have been used, removing the > fragility of this piece or the need for CONFIG_CPU_MICROMIPS conditional. > Here and in other places. > > > > > > + _ssnop > > > > > + _ssnop > > > > > + _ssnop > > > > > > > > instead of handcoded hazard nops, use __irq_enable_hazard for that > > > No, I don't think so, this region should make sure be 32 bytes on each > > > platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the > > > fallback implementation but available for all MIPS. > > > > you are right for most cases, but there is one case > > > > #elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \ > > defined(CONFIG_CPU_BMIPS) > > > > which uses > > > > #define __irq_enable_hazard \ > > ___ssnop; \ > > ___ssnop; \ > > ___ssnop; \ > > ___ehb > > > > if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as > > irq enable hazard. > > Indeed, EHB must always be included, because for R2+ CPUs SSNOP doesn't > clear the hazard (it never clears, but with earlier CPUs it just stalls > the pipeline long enough for the hazard to eventually clear by itself). > > > > > But I doubt this works, because the wait instruction is not aligned to > > > > a 32 byte boundary, but the code assuemes this, IMHO. > > > Why? After this patch we only use 4 byte instructions. > > > > I've should have looked at the compiled output, sorry for the noise > > Umm, there's the commit description to document justification for such > changes made and it's not the reviewer's duty to chase every such detail > that has been omitted from a submission. > > FAOD this is a request to include this detail in v3. > > > Still this construct feels rather fragile. > > I think `.align', etc. can still be used to ensure enough space has been > consumed and if you want to make a code sequence's size check, then a > piece such as: > > 0: > nop > nop > nop > nop > 1: > .ifne 1b - 0b - 32 > .error "code consistency verification failure" > .endif > > should do even where alignment does not matter. And you could possibly do > smarter stuff with `.rept' if (1b - 0b) turns out below rather than above > the threshold required, but I'm leaving it as an exercise for the reader. > > Maciej
diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S index a572ce36a24f..9747b216648f 100644 --- a/arch/mips/kernel/genex.S +++ b/arch/mips/kernel/genex.S @@ -104,25 +104,27 @@ 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 */ + ori t0, 0x1f + xori t0, 0x1e + MTC0 t0, CP0_STATUS + _ssnop + _ssnop + _ssnop .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: @@ -136,9 +138,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 + PTR_LA k1, 1b + /* 32 byte idle interrupt region */ + ori k0, 0x1f + daddiu k0, 1 bne k0, k1, \handler MTC0 k0, CP0_EPC .set pop diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c index 5abc8b7340f8..1f74c0589f7e 100644 --- a/arch/mips/kernel/idle.c +++ b/arch/mips/kernel/idle.c @@ -37,7 +37,6 @@ static void __cpuidle r3081_wait(void) void __cpuidle r4k_wait(void) { - raw_local_irq_enable(); __r4k_wait(); raw_local_irq_disable(); }
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. 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. Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> --- arch/mips/kernel/genex.S | 39 +++++++++++++++++++++------------------ arch/mips/kernel/idle.c | 1 - 2 files changed, 21 insertions(+), 19 deletions(-)