From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752937Ab2DSCpU (ORCPT ); Wed, 18 Apr 2012 22:45:20 -0400 Received: from mail-iy0-f174.google.com ([209.85.210.174]:56746 "EHLO mail-iy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751129Ab2DSCpS convert rfc822-to-8bit (ORCPT ); Wed, 18 Apr 2012 22:45:18 -0400 MIME-Version: 1.0 In-Reply-To: <20120417155241.0f619a9a.akpm@linux-foundation.org> References: <1334660025-20442-1-git-send-email-bryan.wu@canonical.com> <1334660025-20442-2-git-send-email-bryan.wu@canonical.com> <20120417155241.0f619a9a.akpm@linux-foundation.org> From: Bryan Wu Date: Thu, 19 Apr 2012 10:44:57 +0800 X-Google-Sender-Auth: sYdo-wv9gj-IrmunxSNlUq_GTWc Message-ID: Subject: Re: [PATCH 01/18] led-triggers: create a trigger for CPU activity To: Andrew Morton Cc: nicolas.pitre@linaro.org, linux@arm.linux.org.uk, linus.walleij@linaro.org, linux-kernel@vger.kernel.org, rpurdie@rpsys.net, arnd.bergmann@linaro.org, tim.gardner@canonical.com, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 18, 2012 at 6:52 AM, Andrew Morton wrote: > On Tue, 17 Apr 2012 18:53:28 +0800 > Bryan Wu 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. > > No, the patch doesn't alter arch/arm/kernel/leds.c at all.  This text > is either misleadingly phrased, or stale or something. > OK, Fixed >> It also provides ledtrig_cpu trigger event stub in . >> Although it was inspired by ARM work, it can be used in other arch.) >> >> Cc: Richard Purdie >> Signed-off-by: Linus Walleij >> Signed-off-by: Bryan Wu >> > > A spurious newline. > OK, Fixed >> Reviewed-by: Jamie Iles >> Tested-by: Jochen Friedrich >> >> ... >> >> +config LEDS_TRIGGER_CPU >> +     tristate "LED CPU Trigger" >> +     depends on LEDS_TRIGGERS >> +     help >> +       This allows LEDs to be controlled by active CPUs. This shows >> +       the active CPUs across an array of LEDs so you can see what > > s/what/which/   (I think ;)) > Fixed >> +       CPUs are active on the system at any given moment. >> + >> +       If unsure, say N. >> + >> >> ... >> >> +static int __init ledtrig_cpu_init(void) >> +{ >> +     int cpu; >> + >> +     /* Supports up to 9999 cpu cores */ >> +     BUILD_BUG_ON(CONFIG_NR_CPUS > 9999); > > hm, I wonder if this can be prevented in Kconfig logic.  I guess > "depends on NR_CPUS <= 9999" isn't available. > "default N if (NR_CPUS > 9999)" isn't available, either. >> +     /* >> +      * Registering CPU led trigger for each CPU cores here > > s/cores/core/ > Fixed >> +      * ignores CPU hotplug, but after this CPU hotplug works >> +      * fine with this trigger. >> +      */ >> +     for_each_possible_cpu(cpu) { >> +             struct led_trigger *trig; >> +             char *name = per_cpu(trig_name, cpu); >> +             struct rw_semaphore *lock = &per_cpu(trig_lock, cpu); >> + >> +             init_rwsem(lock); >> + >> +             snprintf(name, MAX_NAME_LEN, "cpu%d", cpu); >> + >> +             down_write(lock); >> +             led_trigger_register_simple(name, &trig); > > OK, problem. > > led_trigger_register_simple() calls kzalloc() and > led_trigger_register(), both of which can fail. > led_trigger_register_simple() just returns void, failing to propagate > the error back.  This is bad, and we (ie you ;)) should fix > led_trigger_register_simple() before proceeding to use it.  If at all > possible.  Please.  Let us not propagate the badness further.  Sorry. > >> +             per_cpu(cpu_trig, cpu) = trig; >> +             up_write(lock); >> +     } >> + >> +     register_syscore_ops(&ledtrig_cpu_syscore_ops); >> + >> +     pr_info("ledtrig-cpu: registered to indicate activity on CPUs\n"); >> + >> +     return 0; >> +} >> +module_init(ledtrig_cpu_init); >> + >> +static void __exit ledtrig_cpu_exit(void) >> +{ >> +     int cpu; >> + >> +     for_each_possible_cpu(cpu) { >> +             struct led_trigger *trig = per_cpu(cpu_trig, cpu); > > grr.  drivers/leds/led-triggers.c sometimes does > >        struct led_trigger trigger; > > and sometimes > >        struct led_trigger trig; > > which makes the code needlessly more difficult to follow.  So if you're > fiddling with drivers/leds/led-triggers.c, please fix that up - use > "trig" everywhere? > If Richard has no objection, I will do this. >> +             char *name = per_cpu(trig_name, cpu); >> + >> +             led_trigger_unregister_simple(trig); > > And what happens if led_trigger_register_simple() had silently failed > to register this trigger?  afacit, nothing: your code handles the > trig==NULL case OK.  Still, we should be checking for those failures! > >> +             per_cpu(cpu_trig, cpu) = NULL; >> +             memset(name, 0, MAX_NAME_LEN); >> +     } >> + >> +     unregister_syscore_ops(&ledtrig_cpu_syscore_ops); >> +} >> +module_exit(ledtrig_cpu_exit); >> + >> +MODULE_AUTHOR("Linus Walleij "); >> +MODULE_AUTHOR("Bryan Wu "); >> +MODULE_DESCRIPTION("CPU LED trigger"); >> +MODULE_LICENSE("GPL"); >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index 5884def..1215b94 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -210,4 +210,27 @@ struct gpio_led_platform_data { >>  struct platform_device *gpio_led_register_device( >>               int id, const struct gpio_led_platform_data *pdata); >> >> +enum cpu_led_event { >> +     CPU_LED_IDLE_START,     /* CPU enters idle */ >> +     CPU_LED_IDLE_END,       /* CPU idle ends */ >> +     CPU_LED_START,          /* Machine starts, especially resume */ >> +     CPU_LED_STOP,           /* Machine stops, especially suspend */ >> +     CPU_LED_HALTED,         /* Machine shutdown */ >> +}; >> +#if defined(CONFIG_LEDS_TRIGGER_CPU) || defined(CONFIG_LEDS_TRIGGER_CPU_MODULE) > > See lkml subject "RFC: strip 15,000 lines from a typical autoconf.h". > We might be able to simplify this.  But the above is OK for now. > >> +/** >> + * ledtrig_cpu - emit a CPU event as a trigger >> + * @evt: CPU event to be emitted >> + * >> + * Emit a CPU event on a CPU core, which will trigger a >> + * binded LED to turn on or turn off. >> + */ > > It's conventional to add kerneldoc at the function definition site, not > at the declaration.  Nobody thinks to look in the .h file for > documentation. > Fixed. >> +extern void ledtrig_cpu(enum cpu_led_event evt); >> +#else >> +static inline void ledtrig_cpu(enum cpu_led_event evt) >> +{ >> +     return; >> +} >> +#endif >> + > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Bryan Wu Kernel Developer    +86.186-168-78255 Mobile Canonical Ltd.      www.canonical.com Ubuntu - Linux for human beings | www.ubuntu.com