[PATCH] smp.h: Use local_irq_{save,restore}() in !SMP version of on_each_cpu().

[PATCH] smp.h: Use local_irq_{save,restore}() in !SMP version of on_each_cpu().

[David Daney]

From: David Daney <david.daney@cavium.com>

Thanks to commit f91eb62f71b (init: scream bloody murder if interrupts
are enabled too early), "bloody murder" is now being screamed.

With a MIPS OCTEON config, we use on_each_cpu() in our
irq_chip.irq_bus_sync_unlock() function.  This gets called in early as
a result of the time_init() call.  Because the !SMP version of
on_each_cpu() unconditionally enables irqs, we get:

------------[ cut here ]------------
WARNING: at init/main.c:560 start_kernel+0x250/0x410()
Interrupts were enabled early
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.10.0-rc5-Cavium-Octeon+ #801
Stack : 0000000000000046 ffffffff808e0000 0000000000000006 0000000000000004
	  0000000000000001 0000000000000000 0000000000000046 0000000000000000
	  ffffffff80a90000 ffffffff8015c020 0000000000000000 ffffffff8015c020
	  ffffffff80a79f70 ffffffff80a80000 ffffffff8072b9c0 ffffffff808a7d77
	  ffffffff80a79f70 ffffffff808a8168 0000000000000000 00000004178a9948
	  0000000417801230 ffffffff805f7610 0000000010000078 ffffffff805fa01c
	  ffffffff8089bd18 ffffffff801595fc ffffffff8089bd28 ffffffff8015d384
	  ffffffff808a7e80 ffffffff8089bc30 0000000000000000 ffffffff80159710
	  0000000000000000 0000000000000000 0000000000000000 0000000000000000
	  0000000000000000 ffffffff80139520 0000000000000000 0000000000000009
	  ...
Call Trace:
[<ffffffff80139520>] show_stack+0x68/0x80
[<ffffffff80159710>] warn_slowpath_common+0x78/0xb0
[<ffffffff801597e8>] warn_slowpath_fmt+0x38/0x48
[<ffffffff8092b768>] start_kernel+0x250/0x410

---[ end trace 139ce121c98e96c9 ]---

Suggested fix: Do what we already do in the SMP version of
on_each_cpu(), and use local_irq_save/local_irq_restore.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 include/linux/smp.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index e6564c1..d8fb04b 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -141,9 +141,10 @@ static inline int up_smp_call_function(smp_call_func_t func, void *info)
 			(up_smp_call_function(func, info))
 #define on_each_cpu(func,info,wait)		\
 	({					\
-		local_irq_disable();		\
+		unsigned long flags;		\
+		local_irq_save(flags);		\
 		func(info);			\
-		local_irq_enable();		\
+		local_irq_restore(flags);	\
 		0;				\
 	})
 /*
-- 
1.7.11.7

Re: [PATCH] smp.h: Use local_irq_{save,restore}() in !SMP version of on_each_cpu().

[Linus Torvalds]

On Thu, Jun 13, 2013 at 6:07 PM, David Daney <ddaney.cavm@gmail.com> wrote:
>
> Suggested fix: Do what we already do in the SMP version of
> on_each_cpu(), and use local_irq_save/local_irq_restore.

I was going to apply this, but started looking a bit more.

Using "flags" as a variable name inside a macro like this is a
*really* bad idea.

Lookie here:

    [torvalds@pixel linux]$ git grep on_each_cpu.*flags
    arch/s390/kernel/perf_cpum_cf.c:        on_each_cpu(setup_pmc_cpu,
&flags, 1);
    arch/s390/kernel/perf_cpum_cf.c:        on_each_cpu(setup_pmc_cpu,
&flags, 1);

and ask yourself what happens when the "info" argument expands to
"&flags", and it all compiles perfectly fine, but the "&flags" takes
the address of the new _inner_ variable called "flags" from the macro
expansion. Not the one that the caller actually intends..

Oops.

Not a good idea.

So I would suggest trivially renaming "flags" as "__flags" or
something, or perhaps even just making it a real function and avoiding
the whole namespace issue.

And rather than doing that blindly by editing the patch at after -rc5,
I'm just going to ask you to re-send a tested patch. Ok?

                    Linus

Re: [PATCH] smp.h: Use local_irq_{save,restore}() in !SMP version of on_each_cpu().

[David Daney]

On 06/13/2013 10:46 PM, Linus Torvalds wrote:
> On Thu, Jun 13, 2013 at 6:07 PM, David Daney <ddaney.cavm@gmail.com> wrote:
>>
>> Suggested fix: Do what we already do in the SMP version of
>> on_each_cpu(), and use local_irq_save/local_irq_restore.
>
> I was going to apply this, but started looking a bit more.
>
> Using "flags" as a variable name inside a macro like this is a
> *really* bad idea.
>
> Lookie here:
>
>      [torvalds@pixel linux]$ git grep on_each_cpu.*flags
>      arch/s390/kernel/perf_cpum_cf.c:        on_each_cpu(setup_pmc_cpu,
> &flags, 1);
>      arch/s390/kernel/perf_cpum_cf.c:        on_each_cpu(setup_pmc_cpu,
> &flags, 1);
>
> and ask yourself what happens when the "info" argument expands to
> "&flags", and it all compiles perfectly fine, but the "&flags" takes
> the address of the new _inner_ variable called "flags" from the macro
> expansion. Not the one that the caller actually intends..
>
> Oops.
>
> Not a good idea.
>

Yeah,  I think making it a static inline function may be the best approach.

I am going to test doing that and send a new patch very soon.

David Daney


> So I would suggest trivially renaming "flags" as "__flags" or
> something, or perhaps even just making it a real function and avoiding
> the whole namespace issue.
>
> And rather than doing that blindly by editing the patch at after -rc5,
> I'm just going to ask you to re-send a tested patch. Ok?
>
>                      Linus
>