Messages in this thread | | | Date | Fri, 14 Jun 2013 09:35:50 -0700 | From | David Daney <> | Subject | Re: [PATCH] smp.h: Use local_irq_{save,restore}() in !SMP version of on_each_cpu(). |
| |
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 >
| |