linux-kernel - Re: [PATCH 02/19] led-triggers: create a trigger for CPU activity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F9E8D8C.9090704@canonical.com>
Date:	Mon, 30 Apr 2012 07:03:08 -0600
From:	Tim Gardner <tim.gardner@...onical.com>
To:	Bryan Wu <bryan.wu@...onical.com>
CC:	linux@....linux.org.uk, rpurdie@...ys.net,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linus.walleij@...aro.org, akpm@...ux-foundation.org,
	arnd.bergmann@...aro.org, nicolas.pitre@...aro.org
Subject: Re: [PATCH 02/19] led-triggers: create a trigger for CPU activity

On 04/30/2012 01:37 AM, 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@...onical.com:
> It introduces several 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.)
> 
> Cc: Richard Purdie <rpurdie@...ys.net>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> Signed-off-by: Bryan Wu <bryan.wu@...onical.com>
> Reviewed-by: Jamie Iles <jamie@...ieiles.com>
> Tested-by: Jochen Friedrich <jochen@...am.de>
> ---
>  drivers/leds/Kconfig       |   10 +++
>  drivers/leds/Makefile      |    1 +
>  drivers/leds/ledtrig-cpu.c |  150 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/leds.h       |   16 +++++
>  4 files changed, 177 insertions(+)
>  create mode 100644 drivers/leds/ledtrig-cpu.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 5f12659..dbf8a4c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -465,6 +465,16 @@ config LEDS_TRIGGER_BACKLIGHT
>  
>  	  If unsure, say N.
>  
> +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 which
> +	  CPUs are active on the system at any given moment.
> +
> +	  If unsure, say N.
> +
>  config LEDS_TRIGGER_GPIO
>  	tristate "LED GPIO Trigger"
>  	depends on LEDS_TRIGGERS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 9475bbb..ea1efb2 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -56,4 +56,5 @@ obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
>  obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)	+= ledtrig-backlight.o
>  obj-$(CONFIG_LEDS_TRIGGER_GPIO)		+= ledtrig-gpio.o
> +obj-$(CONFIG_LEDS_TRIGGER_CPU)		+= ledtrig-cpu.o
>  obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
> diff --git a/drivers/leds/ledtrig-cpu.c b/drivers/leds/ledtrig-cpu.c
> new file mode 100644
> index 0000000..1f752e1
> --- /dev/null
> +++ b/drivers/leds/ledtrig-cpu.c
> @@ -0,0 +1,150 @@
> +/*
> + * ledtrig-cpu.c - LED trigger based on CPU activity
> + *
> + * This LED trigger will be registered for each possible CPU and named as
> + * cpu0, cpu1, cpu2, cpu3, etc.
> + *
> + * It can be bound to any LED just like other triggers using either a
> + * board file or via sysfs interface.
> + *
> + * An API named ledtrig_cpu is exported for any user, who want to add CPU
> + * activity indication in their code
> + *
> + * Copyright 2011 Linus Walleij <linus.walleij@...aro.org>
> + * Copyright 2011 - 2012 Bryan Wu <bryan.wu@...onical.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/percpu.h>
> +#include <linux/syscore_ops.h>
> +#include <linux/rwsem.h>
> +#include "leds.h"
> +
> +#define MAX_NAME_LEN	8
> +
> +static DEFINE_PER_CPU(struct led_trigger *, cpu_trig);
> +static DEFINE_PER_CPU(char [MAX_NAME_LEN], trig_name);
> +static DEFINE_PER_CPU(struct rw_semaphore, trig_lock);

Semaphores are overkill for this. I think a simple 'struct mutex' is
sufficient. Especially since there is unlikely to be much contention on
any one of the locks. mutext_init(), mutext_lock(), mutex_unlock(), etc.

> +
> +/**
> + * 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.
> + */
> +void ledtrig_cpu(enum cpu_led_event ledevt)
> +{

I think you still need to acquire trig_lock _before_ reading
__get_cpu_var(cpu_trig). Otherwise, acquiring the lock in
ledtrig_cpu_init() is useless.

> +	struct led_trigger *trig = __get_cpu_var(cpu_trig);
> +
> +	if (!trig)
> +		return;

Unnecessary since led_trigger_event() also checks for (!trig).

I've attached the changes I think you should make.

rtg
-- 
Tim Gardner tim.gardner@...onical.com

View attachment "0001-led-triggers-create-a-trigger-for-CPU-activity.patch" of type "text/x-patch" (7257 bytes)