Messages in this thread | | | From | Bryan Wu <> | Date | Fri, 13 Apr 2012 19:29:36 +0800 | Subject | Re: [PATCH 01/18] led-triggers: create a trigger for CPU activity |
| |
On Sat, Apr 7, 2012 at 6:15 AM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Fri, 30 Mar 2012 19:58:16 +0800 > Bryan Wu <bryan.wu@canonical.com> wrote: > >> Attempting to consolidate the ARM LED code, this removes the >> custom RealView LED trigger code to turn LEDs on and off in >> response to CPU activity and replace it with a standard trigger. >> >> (bryan.wu@canonical.com: >> It moves arch/arm/kernel/leds.c syscore stubs into this trigger. >> It also provides ledtrig_cpu trigger event stub in <linux/leds.h>. >> Although it was inspired by ARM work, it can be used in other arch.) >> >> >> ... >> >> +#include <linux/percpu.h> >> +#include <linux/syscore_ops.h> >> +#include "leds.h" >> + >> +#define MAX_NAME_LEN 8 > > The kernel already has at least eight different definitions of > MAX_NAME_LEN. I guess a ninth won't hurt ;) > >> >> ... >> >> +static void __init ledtrig_cpu_register(void) >> +{ >> + int cpuid = smp_processor_id(); >> + struct led_trigger *trig; >> + char *name = __get_cpu_var(trig_name); >> + >> + snprintf(name, MAX_NAME_LEN, "cpu%d", cpuid); >> + led_trigger_register_simple(name, &trig); >> + >> + pr_info("LED trigger %s indicate activity on CPU %d\n", >> + trig->name, cpuid); >> + >> + __get_cpu_var(cpu_trig) = trig; >> +} >> + >> +static void __exit ledtrig_cpu_unregister(void) >> +{ >> + struct led_trigger *trig = __get_cpu_var(cpu_trig); >> + char *name = __get_cpu_var(trig_name); >> + >> + led_trigger_unregister_simple(trig); >> + __get_cpu_var(cpu_trig) = NULL; >> + memset(name, 0, MAX_NAME_LEN); >> +} >> + >> +static int __init ledtrig_cpu_init(void) >> +{ >> + int cpu; >> + >> + for_each_possible_cpu(cpu) >> + ledtrig_cpu_register(); >> + >> + register_syscore_ops(&ledtrig_cpu_syscore_ops); >> + >> + return 0; >> +} >> +module_init(ledtrig_cpu_init); > > This all looks horridly broken. We call ledtrig_cpu_register() once > for each CPU, but we call it on the same CPU each time, and it uses > smp_processor_id() which a) is wrong and b) will emit a runtime > preemption-is-enabled warning. >
Indeed, my bad. I've already rewritten this function by using per_cpu() API.
> I'm assuming this wasn't tested on SMP. It needs to be, please. >
Yeah, tested on my OMAP4 dual core SMP panda board.
> Also, the code uses for_each_possible_cpu, ignoring CPU hotplug. > That's probably justifiable for this small storage size and not-hotpath > code, but the decision should be given prominence and justified in the > changelog or, better, in code comments please. >
I tested with CPU hotplug after this initial function on Panda and added comments.
>> >> ... >> > > The patchset is basically an ARM thing. Is there some ARM tree via > which we can get it merged?
I put these patches in my git tree, but might need Russell or Arnd to help review and merge.
And I just resent the patchset.
Thanks, -- Bryan Wu <bryan.wu@canonical.com> Kernel Developer +86.186-168-78255 Mobile Canonical Ltd. www.canonical.com Ubuntu - Linux for human beings | www.ubuntu.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |