On Sun, Aug 26, 2012 at 05:56:49PM -0700, Jim Quinlan wrote:
> irqflags.h depends on asm-offsets.h, but asm-offsets.h depends
> on irqflags.h when generating asm-offsets.h. Adding a definition
> to the top of asm-offsets.c allows us to break this circle. There
> is a similar define in bounds.c
>
> Signed-off-by: Jim Quinlan <jim2101024@xxxxxxxxx>
> ---
> arch/mips/kernel/asm-offsets.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/kernel/asm-offsets.c b/arch/mips/kernel/asm-offsets.c
> index 6b30fb2..035f167 100644
> --- a/arch/mips/kernel/asm-offsets.c
> +++ b/arch/mips/kernel/asm-offsets.c
> @@ -8,6 +8,7 @@
> * Kevin Kissell, kevink@xxxxxxxx and Carsten Langgaard, carstenl@xxxxxxxx
> * Copyright (C) 2000 MIPS Technologies, Inc.
> */
> +#define __GENERATING_OFFSETS_S
> #include <linux/compat.h>
> #include <linux/types.h>
> #include <linux/sched.h>
> --
> 1.7.6
>
>
> >From ab76333c041140e4fc1835b581044ff42906881c Mon Sep 17 00:00:00 2001
Something went seriously, seriously wrong in submission of this patch.
First your two part series ended up in a single email. I was able to
verify that it actually arrived as a single email on linux-mips.org from
your mailserver.
Next it at some point of processing got split into two emails at above
>From line resulting in the 2nd email being archived with improper headers
in linux-mips.org's email archive. So there's at least one bug at each
end.
> From: Jim Quinlan <jim2101024@xxxxxxxxx>
> Date: Sun, 26 Aug 2012 18:08:43 -0400
> Subject: [PATCH 2/2] MIPS: irqflags.h: make funcs preempt-safe for non-mipsr2
>
> For non MIPSr2 processors, such as the BMIPS 5000, calls to
> arch_local_irq_disable() and others may be preempted, and in doing
> so a stale value may be restored to c0_status. This fix disables
> preemption for such processors prior to the call and enables it
> after the call.
>
> This bug was observed in a BMIPS 5000, occuring once every few hours
> in a continuous reboot test. It was traced to the write_lock_irq()
> function which was being invoked in release_task() in exit.c.
> By placing a number of "nops" inbetween the mfc0/mtc0 pair in
> arch_local_irq_disable(), which is called by write_lock_irq(), we
> were able to greatly increase the occurance of this bug. Similarly,
> the application of this commit silenced the bug.
>
> It is better to use the preemption functions declared in <linux/preempt.h>
> rather than defining new ones as is done in this commit. However,
> including that file from irqflags effected many compiler errors.
This is the 2nd time non-atomic RMW operations on c0_status bite in the
kernel.
> Signed-off-by: Jim Quinlan <jim2101024@xxxxxxxxx>
> ---
> arch/mips/include/asm/irqflags.h | 81 ++++++++++++++++++++++++++++++++++++++
> 1 files changed, 81 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/include/asm/irqflags.h b/arch/mips/include/asm/irqflags.h
> index 309cbcd..2732f5f 100644
> --- a/arch/mips/include/asm/irqflags.h
> +++ b/arch/mips/include/asm/irqflags.h
> @@ -16,6 +16,71 @@
> #include <linux/compiler.h>
> #include <asm/hazards.h>
>
> +#if defined(__GENERATING_BOUNDS_H) || defined(__GENERATING_OFFSETS_S)
> +#define __TI_PRE_COUNT (-1)
> +#else
> +#include <asm/asm-offsets.h>
> +#define __TI_PRE_COUNT TI_PRE_COUNT
> +#endif
This part is too ugly to live. Yet it seems it needs to live.
> +
> +
> +/*
> + * Non-mipsr2 processors executing functions such as arch_local_irq_disable()
> + * are not preempt-safe: if preemption occurs between the mfc0 and the mtc0,
> + * a stale status value may be stored. To prevent this, we define
> + * here arch_local_preempt_disable() and arch_local_preempt_enable(), which
> + * are called before the mfc0 and after the mtc0, respectively. A better
> + * solution would "#include <linux/preempt.h> and use its declared routines,
> + * but that is not viable due to numerous compile errors.
> + *
> + * MipsR2 processors with atomic interrupt enable/disable instructions
> + * (ei/di) do not have this issue.
> + */
> +__asm__(
> + " .macro arch_local_preempt_disable ti_pre_count \n"
> + " .set push \n"
> + " .set noat \n"
> + " lw $1, \\ti_pre_count($28) \n"
> + " addi $1, $1, 1 \n"
> + " sw $1, \\ti_pre_count($28) \n"
> + " .set pop \n"
> + " .endm");
> +static inline void arch_local_preempt_disable(void)
> +{
> +#if defined(CONFIG_PREEMPT) && !defined(CONFIG_CPU_MIPSR2)
> + __asm__ __volatile__(
> + "arch_local_preempt_disable\t%0"
> + : /* no outputs */
> + : "n" (__TI_PRE_COUNT)
> + : "memory");
> + barrier();
> +#endif
> +}
> +
> +
> +__asm__(
> + " .macro arch_local_preempt_enable ti_pre_count \n"
> + " .set push \n"
> + " .set noat \n"
> + " lw $1, \\ti_pre_count($28) \n"
> + " addi $1, $1, -1 \n"
> + " sw $1, \\ti_pre_count($28) \n"
> + " .set pop \n"
> + " .endm");
> +
> +static inline void arch_local_preempt_enable(void)
> +{
> +#if defined(CONFIG_PREEMPT) && !defined(CONFIG_CPU_MIPSR2)
> + __asm__ __volatile__(
> + "arch_local_preempt_enable\t%0"
> + : /* no outputs */
> + : "n" (__TI_PRE_COUNT)
> + : "memory");
> + barrier();
> +#endif
> +}
> +
> +
> __asm__(
> " .macro arch_local_irq_enable \n"
> " .set push \n"
> @@ -99,11 +164,15 @@ __asm__(
>
> static inline void arch_local_irq_disable(void)
> {
> + arch_local_preempt_disable();
> +
> __asm__ __volatile__(
> "arch_local_irq_disable"
> : /* no outputs */
> : /* no inputs */
> : "memory");
> +
> + arch_local_preempt_enable();
> }
>
> __asm__(
> @@ -153,10 +222,15 @@ __asm__(
> static inline unsigned long arch_local_irq_save(void)
> {
> unsigned long flags;
> +
> + arch_local_preempt_disable();
> +
> asm volatile("arch_local_irq_save\t%0"
> : "=r" (flags)
> : /* no inputs */
> : "memory");
> +
> + arch_local_preempt_enable();
> return flags;
> }
>
> @@ -214,23 +288,30 @@ static inline void arch_local_irq_restore(unsigned long flags)
> if (unlikely(!(flags & 0x0400)))
> smtc_ipi_replay();
> #endif
> + arch_local_preempt_disable();
>
> __asm__ __volatile__(
> "arch_local_irq_restore\t%0"
> : "=r" (__tmp1)
> : "0" (flags)
> : "memory");
> +
> + arch_local_preempt_enable();
> }
>
> static inline void __arch_local_irq_restore(unsigned long flags)
> {
> unsigned long __tmp1;
>
> + arch_local_preempt_disable();
> +
> __asm__ __volatile__(
> "arch_local_irq_restore\t%0"
> : "=r" (__tmp1)
> : "0" (flags)
> : "memory");
> +
> + arch_local_preempt_enable();
> }
>
> static inline int arch_irqs_disabled_flags(unsigned long flags)
> --
> 1.7.6
>
>
Ralf
