Re: [PATCH] MIPS: make local_irq_disable macro safe for non-mipsr2

[David Daney <ddaney.cavm@gmail.com>]

On 11/27/2013 12:34 PM, Jim Quinlan wrote:
> For non-mipsr2 processors, the local_irq_disable contains an mfc0-mtc0
> pair with instructions inbetween.  With preemption enabled, this sequence
> may get preempted and effect a stale value of CP0_STATUS when executing
> the mtc0 instruction.  This commit avoids this scenario by incrementing
> the preempt count before the mfc0 and decrementing it after the mtc9.
>
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>   arch/mips/include/asm/asmmacro.h |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/include/asm/asmmacro.h b/arch/mips/include/asm/asmmacro.h
> index 6c8342a..3f809a4 100644
> --- a/arch/mips/include/asm/asmmacro.h
> +++ b/arch/mips/include/asm/asmmacro.h
> @@ -9,6 +9,7 @@
>   #define _ASM_ASMMACRO_H
>
>   #include <asm/hazards.h>
> +//#include <asm/asm-offsets.h>

I'm guessing it didn't pass checkpatch.pl

>
>   #ifdef CONFIG_32BIT
>   #include <asm/asmmacro-32.h>
> @@ -54,11 +55,21 @@
>   	.endm
>
>   	.macro	local_irq_disable reg=t0
> +#ifdef CONFIG_PREEMPT
> +	lw      \reg, TI_PRE_COUNT($28)
> +	addi    \reg, \reg, 1
> +	sw      \reg, TI_PRE_COUNT($28)
> +#endif

Does this need to be atomic?

More importantly, how does that prevent the problem you describe?

An interrupt can still occur between the mfc0 and mtc0 causing Status 
bits to be changed.  What bits do we care about?  is it IM*, I doubt you 
would see CU* changing.

Which bits are getting clobbered that shouldn't be?


>   	mfc0	\reg, CP0_STATUS
>   	ori	\reg, \reg, 1
>   	xori	\reg, \reg, 1
>   	mtc0	\reg, CP0_STATUS
>   	irq_disable_hazard
> +#ifdef CONFIG_PREEMPT
> +	lw      \reg, TI_PRE_COUNT($28)
> +	addi    \reg, \reg, -1
> +	sw      \reg, TI_PRE_COUNT($28)
> +#endif
>   	.endm
>   #endif /* CONFIG_MIPS_MT_SMTC */
>
>