[PATCH] led: simplify led_trigger_register_simple

[PATCH] led: simplify led_trigger_register_simple

[Felipe Balbi]

From: Felipe Balbi <felipe.balbi@nokia.com>

We can make led_trigger_register_simple by returning a
struct led_trigger *, instead of passing a struct led_trigger **
as a parameter and changing it inside the function.

Cc: Anton Vorontsov <cbou@mail.ru>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Greg KH <greg@kroah.com>
Cc: Pierre Ossman <drzeus@drzeus.cx>
Cc: Richard Purdie <rpurdie@linux.intel.com>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
Note that I could only test ide, power_supply and mmc changes with my laptop.
The other changes weren't tested.

 drivers/leds/led-triggers.c         |   25 ++++++++++++++---------
 drivers/leds/ledtrig-ide-disk.c     |    6 ++++-
 drivers/mmc/core/host.c             |    4 ++-
 drivers/mtd/nand/nand_base.c        |    5 +++-
 drivers/power/power_supply_leds.c   |   36 ++++++++++++++++++++++++++++------
 drivers/staging/at76_usb/at76_usb.c |    6 ++++-
 include/linux/leds.h                |    5 +--
 7 files changed, 63 insertions(+), 24 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index f910eaf..24e5851 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -13,6 +13,7 @@
 
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <linux/init.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
@@ -222,24 +223,28 @@ void led_trigger_event(struct led_trigger *trigger,
 }
 EXPORT_SYMBOL_GPL(led_trigger_event);
 
-void led_trigger_register_simple(const char *name, struct led_trigger **tp)
+struct led_trigger *led_trigger_register_simple(const char *name)
 {
 	struct led_trigger *trigger;
 	int err;
 
 	trigger = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
-
-	if (trigger) {
-		trigger->name = name;
-		err = led_trigger_register(trigger);
-		if (err < 0)
-			printk(KERN_WARNING "LED trigger %s failed to register"
-				" (%d)\n", name, err);
-	} else
+	if (!trigger) {
 		printk(KERN_WARNING "LED trigger %s failed to register"
 			" (no memory)\n", name);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	trigger->name = name;
+	err = led_trigger_register(trigger);
+	if (err < 0) {
+		printk(KERN_WARNING "LED trigger %s failed to register"
+				" (%d)\n", name, err);
+		kfree(trigger);
+		return ERR_PTR(err);
+	}
 
-	*tp = trigger;
+	return trigger;
 }
 EXPORT_SYMBOL_GPL(led_trigger_register_simple);
 
diff --git a/drivers/leds/ledtrig-ide-disk.c b/drivers/leds/ledtrig-ide-disk.c
index 883a577..e62e080 100644
--- a/drivers/leds/ledtrig-ide-disk.c
+++ b/drivers/leds/ledtrig-ide-disk.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <linux/init.h>
 #include <linux/timer.h>
 #include <linux/leds.h>
@@ -46,7 +47,10 @@ static void ledtrig_ide_timerfunc(unsigned long data)
 
 static int __init ledtrig_ide_init(void)
 {
-	led_trigger_register_simple("ide-disk", &ledtrig_ide);
+	ledtrig_ide = led_trigger_register_simple("ide-disk");
+	if (IS_ERR(ledtrig_ide))
+		return PTR_ERR(ledtrig_ide);
+
 	return 0;
 }
 
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 5e945e6..07247b7 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -120,7 +120,9 @@ int mmc_add_host(struct mmc_host *host)
 	WARN_ON((host->caps & MMC_CAP_SDIO_IRQ) &&
 		!host->ops->enable_sdio_irq);
 
-	led_trigger_register_simple(dev_name(&host->class_dev), &host->led);
+	host->led = led_trigger_register_simple(dev_name(&host->class_dev));
+	if (IS_ERR(host->led))
+		return PTR_ERR(host->led);
 
 	err = device_add(&host->class_dev);
 	if (err)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 0a9c9cd..b170a62 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2781,7 +2781,10 @@ EXPORT_SYMBOL_GPL(nand_release);
 
 static int __init nand_base_init(void)
 {
-	led_trigger_register_simple("nand-disk", &nand_led_trigger);
+	nand_led_trigger = led_trigger_register_simple("nand-disk");
+	if (IS_ERR(nand_led_trigger))
+		return PTR_ERR(nand_led_trigger);
+
 	return 0;
 }
 
diff --git a/drivers/power/power_supply_leds.c b/drivers/power/power_supply_leds.c
index 2dece40..d1a857c 100644
--- a/drivers/power/power_supply_leds.c
+++ b/drivers/power/power_supply_leds.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <linux/power_supply.h>
 
 #include "power_supply.h"
@@ -63,15 +64,32 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
 	if (!psy->full_trig_name)
 		goto full_failed;
 
-	led_trigger_register_simple(psy->charging_full_trig_name,
-				    &psy->charging_full_trig);
-	led_trigger_register_simple(psy->charging_trig_name,
-				    &psy->charging_trig);
-	led_trigger_register_simple(psy->full_trig_name,
-				    &psy->full_trig);
+	psy->charging_full_trig = led_trigger_register_simple(
+			psy->charging_full_trig_name);
+	if (IS_ERR(psy->charging_full_trig)) {
+		rc = PTR_ERR(psy->charging_full_trig);
+		goto full_failed;
+	}
+
+	psy->charging_trig = led_trigger_register_simple(
+			psy->charging_trig_name);
+	if (IS_ERR(psy->charging_trig)) {
+		rc = PTR_ERR(psy->charging_full_trig);
+		goto charging_trig_failed;
+	}
+
+	psy->full_trig = led_trigger_register_simple(psy->full_trig_name);
+	if (IS_ERR(psy->full_trig)) {
+		rc = PTR_ERR(psy->charging_full_trig);
+		goto full_trig_failed;
+	}
 
 	goto success;
 
+full_trig_failed:
+	led_trigger_unregister_simple(psy->charging_trig);
+charging_trig_failed:
+	led_trigger_unregister_simple(psy->charging_full_trig);
 full_failed:
 	kfree(psy->charging_trig_name);
 charging_failed:
@@ -117,7 +135,11 @@ static int power_supply_create_gen_triggers(struct power_supply *psy)
 	if (!psy->online_trig_name)
 		goto online_failed;
 
-	led_trigger_register_simple(psy->online_trig_name, &psy->online_trig);
+	psy->online_trig = led_trigger_register_simple(psy->online_trig_name);
+	if (IS_ERR(psy->online_trig)) {
+		rc = PTR_ERR(psy->online_trig);
+		goto online_failed;
+	}
 
 	goto success;
 
diff --git a/drivers/staging/at76_usb/at76_usb.c b/drivers/staging/at76_usb/at76_usb.c
index 174e2be..e769aae 100644
--- a/drivers/staging/at76_usb/at76_usb.c
+++ b/drivers/staging/at76_usb/at76_usb.c
@@ -21,6 +21,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
+#include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -5528,7 +5529,10 @@ static int __init at76_mod_init(void)
 		printk(KERN_ERR DRIVER_NAME
 		       ": usb_register failed (status %d)\n", result);
 
-	led_trigger_register_simple("at76_usb-tx", &ledtrig_tx);
+	ledtrig_tx = led_trigger_register_simple("at76_usb-tx");
+	if (IS_ERR(ledtrig_tx))
+		result = PTR_ERR(ledtrig_tx);
+
 	return result;
 }
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index d3a73f5..b796ec0 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -94,8 +94,7 @@ extern void led_trigger_unregister(struct led_trigger *trigger);
 /* Registration functions for simple triggers */
 #define DEFINE_LED_TRIGGER(x)		static struct led_trigger *x;
 #define DEFINE_LED_TRIGGER_GLOBAL(x)	struct led_trigger *x;
-extern void led_trigger_register_simple(const char *name,
-				struct led_trigger **trigger);
+extern struct led_trigger *led_trigger_register_simple(const char *name);
 extern void led_trigger_unregister_simple(struct led_trigger *trigger);
 extern void led_trigger_event(struct led_trigger *trigger,
 				enum led_brightness event);
@@ -105,7 +104,7 @@ extern void led_trigger_event(struct led_trigger *trigger,
 /* Triggers aren't active - null macros */
 #define DEFINE_LED_TRIGGER(x)
 #define DEFINE_LED_TRIGGER_GLOBAL(x)
-#define led_trigger_register_simple(x, y) do {} while(0)
+#define led_trigger_register_simple(x) NULL
 #define led_trigger_unregister_simple(x) do {} while(0)
 #define led_trigger_event(x, y) do {} while(0)
 
-- 
1.6.0.4.617.g2baf1

Re: [PATCH] led: simplify led_trigger_register_simple

[Anton Vorontsov]

On Thu, Nov 13, 2008 at 05:09:13AM +0200, Felipe Balbi wrote:
> From: Felipe Balbi <felipe.balbi@nokia.com>
> 
> We can make led_trigger_register_simple by returning a
> struct led_trigger *, instead of passing a struct led_trigger **
> as a parameter and changing it inside the function.
> 
> Cc: Anton Vorontsov <cbou@mail.ru>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Greg KH <greg@kroah.com>
> Cc: Pierre Ossman <drzeus@drzeus.cx>
> Cc: Richard Purdie <rpurdie@linux.intel.com>
> Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> ---
> Note that I could only test ide, power_supply and mmc changes with my laptop.
> The other changes weren't tested.
> 
>  drivers/leds/led-triggers.c         |   25 ++++++++++++++---------
>  drivers/leds/ledtrig-ide-disk.c     |    6 ++++-
>  drivers/mmc/core/host.c             |    4 ++-
>  drivers/mtd/nand/nand_base.c        |    5 +++-
>  drivers/power/power_supply_leds.c   |   36 ++++++++++++++++++++++++++++------

For power supply bits:

Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>

Thanks.

Re: [PATCH] led: simplify led_trigger_register_simple

[Richard Purdie]

On Thu, 2008-11-13 at 05:09 +0200, Felipe Balbi wrote:
> From: Felipe Balbi <felipe.balbi@nokia.com>
> 
> We can make led_trigger_register_simple by returning a
> struct led_trigger *, instead of passing a struct led_trigger **
> as a parameter and changing it inside the function.

This misses the whole point that it was intentionally written that way.

> Note that I could only test ide, power_supply and mmc changes with my laptop.
> The other changes weren't tested.
> 
>  drivers/leds/led-triggers.c         |   25 ++++++++++++++---------
>  drivers/leds/ledtrig-ide-disk.c     |    6 ++++-
>  drivers/mmc/core/host.c             |    4 ++-
>  drivers/mtd/nand/nand_base.c        |    5 +++-
>  drivers/power/power_supply_leds.c   |   36 ++++++++++++++++++++++++++++------
>  drivers/staging/at76_usb/at76_usb.c |    6 ++++-
>  include/linux/leds.h                |    5 +--
>  7 files changed, 63 insertions(+), 24 deletions(-)

The simple triggers were designed to cause minimum interference to the
usually external subsystem code they were added into. As an example this
meant things like errors were just handled gracefully with a printk
warning and did not take down the whole subsystem. I therefore don't
regard this patch as a simplification, more a complication.

Regards,

Richard

Re: [PATCH] led: simplify led_trigger_register_simple

[Felipe Balbi]

On Thu, Nov 13, 2008 at 12:38:32PM +0000, Richard Purdie wrote:
> The simple triggers were designed to cause minimum interference to the
> usually external subsystem code they were added into. As an example this
> meant things like errors were just handled gracefully with a printk
> warning and did not take down the whole subsystem. I therefore don't
> regard this patch as a simplification, more a complication.

That's a matter of changing the return ERR_PTR(err); back to a printk.

-- 
balbi

Re: [PATCH] led: simplify led_trigger_register_simple

[Felipe Balbi]

On Thu, Nov 13, 2008 at 08:14:16PM +0200, Felipe Balbi wrote:
> On Thu, Nov 13, 2008 at 12:38:32PM +0000, Richard Purdie wrote:
> > The simple triggers were designed to cause minimum interference to the
> > usually external subsystem code they were added into. As an example this
> > meant things like errors were just handled gracefully with a printk
> > warning and did not take down the whole subsystem. I therefore don't
> > regard this patch as a simplification, more a complication.
> 
> That's a matter of changing the return ERR_PTR(err); back to a printk.

And here you are. I still think we should at least kfree(trigger) in
case of error, though.

======= cut here ===========

>From 4a96a53109f1edc3277ffb2d22641330930e60cd Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@nokia.com>
Date: Thu, 13 Nov 2008 03:28:22 +0200
Subject: [PATCH] led: simplify led_trigger_register_simple

We can make led_trigger_register_simple by returning a
struct led_trigger *, instead of passing a struct led_trigger **
as a parameter and changing it inside the function.

Cc: Anton Vorontsov <cbou@mail.ru>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Greg KH <greg@kroah.com>
Cc: Pierre Ossman <drzeus@drzeus.cx>
Cc: Richard Purdie <rpurdie@linux.intel.com>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/leds/led-triggers.c         |    4 ++--
 drivers/leds/ledtrig-ide-disk.c     |    2 +-
 drivers/mmc/core/host.c             |    2 +-
 drivers/mtd/nand/nand_base.c        |    2 +-
 drivers/power/power_supply_leds.c   |   13 ++++++-------
 drivers/staging/at76_usb/at76_usb.c |    2 +-
 include/linux/leds.h                |    5 ++---
 7 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index f910eaf..4304278 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -222,7 +222,7 @@ void led_trigger_event(struct led_trigger *trigger,
 }
 EXPORT_SYMBOL_GPL(led_trigger_event);
 
-void led_trigger_register_simple(const char *name, struct led_trigger **tp)
+struct led_trigger *led_trigger_register_simple(const char *name)
 {
 	struct led_trigger *trigger;
 	int err;
@@ -239,7 +239,7 @@ void led_trigger_register_simple(const char *name, struct led_trigger **tp)
 		printk(KERN_WARNING "LED trigger %s failed to register"
 			" (no memory)\n", name);
 
-	*tp = trigger;
+	return trigger;
 }
 EXPORT_SYMBOL_GPL(led_trigger_register_simple);
 
diff --git a/drivers/leds/ledtrig-ide-disk.c b/drivers/leds/ledtrig-ide-disk.c
index 883a577..750e166 100644
--- a/drivers/leds/ledtrig-ide-disk.c
+++ b/drivers/leds/ledtrig-ide-disk.c
@@ -46,7 +46,7 @@ static void ledtrig_ide_timerfunc(unsigned long data)
 
 static int __init ledtrig_ide_init(void)
 {
-	led_trigger_register_simple("ide-disk", &ledtrig_ide);
+	ledtrig_ide = led_trigger_register_simple("ide-disk");
 	return 0;
 }
 
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 5e945e6..8b337de 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -120,7 +120,7 @@ int mmc_add_host(struct mmc_host *host)
 	WARN_ON((host->caps & MMC_CAP_SDIO_IRQ) &&
 		!host->ops->enable_sdio_irq);
 
-	led_trigger_register_simple(dev_name(&host->class_dev), &host->led);
+	host->led = led_trigger_register_simple(dev_name(&host->class_dev));
 
 	err = device_add(&host->class_dev);
 	if (err)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 0a9c9cd..9c5f0e2 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2781,7 +2781,7 @@ EXPORT_SYMBOL_GPL(nand_release);
 
 static int __init nand_base_init(void)
 {
-	led_trigger_register_simple("nand-disk", &nand_led_trigger);
+	nand_led_trigger = led_trigger_register_simple("nand-disk");
 	return 0;
 }
 
diff --git a/drivers/power/power_supply_leds.c b/drivers/power/power_supply_leds.c
index 2dece40..da03990 100644
--- a/drivers/power/power_supply_leds.c
+++ b/drivers/power/power_supply_leds.c
@@ -63,12 +63,11 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
 	if (!psy->full_trig_name)
 		goto full_failed;
 
-	led_trigger_register_simple(psy->charging_full_trig_name,
-				    &psy->charging_full_trig);
-	led_trigger_register_simple(psy->charging_trig_name,
-				    &psy->charging_trig);
-	led_trigger_register_simple(psy->full_trig_name,
-				    &psy->full_trig);
+	psy->charging_full_trig = led_trigger_register_simple(
+			psy->charging_full_trig_name);
+	psy->charging_trig = led_trigger_register_simple(
+			psy->charging_trig_name);
+	psy->full_trig = led_trigger_register_simple(psy->full_trig_name);
 
 	goto success;
 
@@ -117,7 +116,7 @@ static int power_supply_create_gen_triggers(struct power_supply *psy)
 	if (!psy->online_trig_name)
 		goto online_failed;
 
-	led_trigger_register_simple(psy->online_trig_name, &psy->online_trig);
+	psy->online_trig = led_trigger_register_simple(psy->online_trig_name);
 
 	goto success;
 
diff --git a/drivers/staging/at76_usb/at76_usb.c b/drivers/staging/at76_usb/at76_usb.c
index 174e2be..ae6dc6e 100644
--- a/drivers/staging/at76_usb/at76_usb.c
+++ b/drivers/staging/at76_usb/at76_usb.c
@@ -5528,7 +5528,7 @@ static int __init at76_mod_init(void)
 		printk(KERN_ERR DRIVER_NAME
 		       ": usb_register failed (status %d)\n", result);
 
-	led_trigger_register_simple("at76_usb-tx", &ledtrig_tx);
+	ledtrig_tx = led_trigger_register_simple("at76_usb-tx");
 	return result;
 }
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index d3a73f5..b796ec0 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -94,8 +94,7 @@ extern void led_trigger_unregister(struct led_trigger *trigger);
 /* Registration functions for simple triggers */
 #define DEFINE_LED_TRIGGER(x)		static struct led_trigger *x;
 #define DEFINE_LED_TRIGGER_GLOBAL(x)	struct led_trigger *x;
-extern void led_trigger_register_simple(const char *name,
-				struct led_trigger **trigger);
+extern struct led_trigger *led_trigger_register_simple(const char *name);
 extern void led_trigger_unregister_simple(struct led_trigger *trigger);
 extern void led_trigger_event(struct led_trigger *trigger,
 				enum led_brightness event);
@@ -105,7 +104,7 @@ extern void led_trigger_event(struct led_trigger *trigger,
 /* Triggers aren't active - null macros */
 #define DEFINE_LED_TRIGGER(x)
 #define DEFINE_LED_TRIGGER_GLOBAL(x)
-#define led_trigger_register_simple(x, y) do {} while(0)
+#define led_trigger_register_simple(x) NULL
 #define led_trigger_unregister_simple(x) do {} while(0)
 #define led_trigger_event(x, y) do {} while(0)
 
-- 
1.6.0.4.617.g2baf1

-- 
balbi

Re: [PATCH] led: simplify led_trigger_register_simple

[Felipe Balbi]

On Thu, Nov 13, 2008 at 09:10:50PM +0200, ext Felipe Balbi wrote:
> On Thu, Nov 13, 2008 at 08:14:16PM +0200, Felipe Balbi wrote:
> > On Thu, Nov 13, 2008 at 12:38:32PM +0000, Richard Purdie wrote:
> > > The simple triggers were designed to cause minimum interference to the
> > > usually external subsystem code they were added into. As an example this
> > > meant things like errors were just handled gracefully with a printk
> > > warning and did not take down the whole subsystem. I therefore don't
> > > regard this patch as a simplification, more a complication.
> > 
> > That's a matter of changing the return ERR_PTR(err); back to a printk.
> 
> And here you are. I still think we should at least kfree(trigger) in
> case of error, though.

Any comments here ??

> ======= cut here ===========
> 
> >From 4a96a53109f1edc3277ffb2d22641330930e60cd Mon Sep 17 00:00:00 2001
> From: Felipe Balbi <felipe.balbi@nokia.com>
> Date: Thu, 13 Nov 2008 03:28:22 +0200
> Subject: [PATCH] led: simplify led_trigger_register_simple
> 
> We can make led_trigger_register_simple by returning a
> struct led_trigger *, instead of passing a struct led_trigger **
> as a parameter and changing it inside the function.
> 
> Cc: Anton Vorontsov <cbou@mail.ru>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Greg KH <greg@kroah.com>
> Cc: Pierre Ossman <drzeus@drzeus.cx>
> Cc: Richard Purdie <rpurdie@linux.intel.com>
> Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> ---
>  drivers/leds/led-triggers.c         |    4 ++--
>  drivers/leds/ledtrig-ide-disk.c     |    2 +-
>  drivers/mmc/core/host.c             |    2 +-
>  drivers/mtd/nand/nand_base.c        |    2 +-
>  drivers/power/power_supply_leds.c   |   13 ++++++-------
>  drivers/staging/at76_usb/at76_usb.c |    2 +-
>  include/linux/leds.h                |    5 ++---
>  7 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index f910eaf..4304278 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -222,7 +222,7 @@ void led_trigger_event(struct led_trigger *trigger,
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_event);
>  
> -void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> +struct led_trigger *led_trigger_register_simple(const char *name)
>  {
>  	struct led_trigger *trigger;
>  	int err;
> @@ -239,7 +239,7 @@ void led_trigger_register_simple(const char *name, struct led_trigger **tp)
>  		printk(KERN_WARNING "LED trigger %s failed to register"
>  			" (no memory)\n", name);
>  
> -	*tp = trigger;
> +	return trigger;
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_register_simple);
>  
> diff --git a/drivers/leds/ledtrig-ide-disk.c b/drivers/leds/ledtrig-ide-disk.c
> index 883a577..750e166 100644
> --- a/drivers/leds/ledtrig-ide-disk.c
> +++ b/drivers/leds/ledtrig-ide-disk.c
> @@ -46,7 +46,7 @@ static void ledtrig_ide_timerfunc(unsigned long data)
>  
>  static int __init ledtrig_ide_init(void)
>  {
> -	led_trigger_register_simple("ide-disk", &ledtrig_ide);
> +	ledtrig_ide = led_trigger_register_simple("ide-disk");
>  	return 0;
>  }
>  
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 5e945e6..8b337de 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -120,7 +120,7 @@ int mmc_add_host(struct mmc_host *host)
>  	WARN_ON((host->caps & MMC_CAP_SDIO_IRQ) &&
>  		!host->ops->enable_sdio_irq);
>  
> -	led_trigger_register_simple(dev_name(&host->class_dev), &host->led);
> +	host->led = led_trigger_register_simple(dev_name(&host->class_dev));
>  
>  	err = device_add(&host->class_dev);
>  	if (err)
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 0a9c9cd..9c5f0e2 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2781,7 +2781,7 @@ EXPORT_SYMBOL_GPL(nand_release);
>  
>  static int __init nand_base_init(void)
>  {
> -	led_trigger_register_simple("nand-disk", &nand_led_trigger);
> +	nand_led_trigger = led_trigger_register_simple("nand-disk");
>  	return 0;
>  }
>  
> diff --git a/drivers/power/power_supply_leds.c b/drivers/power/power_supply_leds.c
> index 2dece40..da03990 100644
> --- a/drivers/power/power_supply_leds.c
> +++ b/drivers/power/power_supply_leds.c
> @@ -63,12 +63,11 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
>  	if (!psy->full_trig_name)
>  		goto full_failed;
>  
> -	led_trigger_register_simple(psy->charging_full_trig_name,
> -				    &psy->charging_full_trig);
> -	led_trigger_register_simple(psy->charging_trig_name,
> -				    &psy->charging_trig);
> -	led_trigger_register_simple(psy->full_trig_name,
> -				    &psy->full_trig);
> +	psy->charging_full_trig = led_trigger_register_simple(
> +			psy->charging_full_trig_name);
> +	psy->charging_trig = led_trigger_register_simple(
> +			psy->charging_trig_name);
> +	psy->full_trig = led_trigger_register_simple(psy->full_trig_name);
>  
>  	goto success;
>  
> @@ -117,7 +116,7 @@ static int power_supply_create_gen_triggers(struct power_supply *psy)
>  	if (!psy->online_trig_name)
>  		goto online_failed;
>  
> -	led_trigger_register_simple(psy->online_trig_name, &psy->online_trig);
> +	psy->online_trig = led_trigger_register_simple(psy->online_trig_name);
>  
>  	goto success;
>  
> diff --git a/drivers/staging/at76_usb/at76_usb.c b/drivers/staging/at76_usb/at76_usb.c
> index 174e2be..ae6dc6e 100644
> --- a/drivers/staging/at76_usb/at76_usb.c
> +++ b/drivers/staging/at76_usb/at76_usb.c
> @@ -5528,7 +5528,7 @@ static int __init at76_mod_init(void)
>  		printk(KERN_ERR DRIVER_NAME
>  		       ": usb_register failed (status %d)\n", result);
>  
> -	led_trigger_register_simple("at76_usb-tx", &ledtrig_tx);
> +	ledtrig_tx = led_trigger_register_simple("at76_usb-tx");
>  	return result;
>  }
>  
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index d3a73f5..b796ec0 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -94,8 +94,7 @@ extern void led_trigger_unregister(struct led_trigger *trigger);
>  /* Registration functions for simple triggers */
>  #define DEFINE_LED_TRIGGER(x)		static struct led_trigger *x;
>  #define DEFINE_LED_TRIGGER_GLOBAL(x)	struct led_trigger *x;
> -extern void led_trigger_register_simple(const char *name,
> -				struct led_trigger **trigger);
> +extern struct led_trigger *led_trigger_register_simple(const char *name);
>  extern void led_trigger_unregister_simple(struct led_trigger *trigger);
>  extern void led_trigger_event(struct led_trigger *trigger,
>  				enum led_brightness event);
> @@ -105,7 +104,7 @@ extern void led_trigger_event(struct led_trigger *trigger,
>  /* Triggers aren't active - null macros */
>  #define DEFINE_LED_TRIGGER(x)
>  #define DEFINE_LED_TRIGGER_GLOBAL(x)
> -#define led_trigger_register_simple(x, y) do {} while(0)
> +#define led_trigger_register_simple(x) NULL
>  #define led_trigger_unregister_simple(x) do {} while(0)
>  #define led_trigger_event(x, y) do {} while(0)
>  
> -- 
> 1.6.0.4.617.g2baf1
> 
> -- 
> balbi

-- 
balbi

Re: [PATCH] led: simplify led_trigger_register_simple

[Richard Purdie]

On Thu, 2008-11-13 at 21:10 +0200, Felipe Balbi wrote:
> On Thu, Nov 13, 2008 at 08:14:16PM +0200, Felipe Balbi wrote:
> > On Thu, Nov 13, 2008 at 12:38:32PM +0000, Richard Purdie wrote:
> > > The simple triggers were designed to cause minimum interference to the
> > > usually external subsystem code they were added into. As an example this
> > > meant things like errors were just handled gracefully with a printk
> > > warning and did not take down the whole subsystem. I therefore don't
> > > regard this patch as a simplification, more a complication.
> > 
> > That's a matter of changing the return ERR_PTR(err); back to a printk.
> 
> And here you are. I still think we should at least kfree(trigger) in
> case of error, though.

This patch now just changes the calling convention of the function which
doesn't seem to serve much purpose.

In answer to your question about kfree, I agree it needs to be called
upon error. The callers should just be calling
led_trigger_unregister_simple() in their failure paths though which
should take care of all problems? I know we used to register the simple
triggers late in paths so no error handling was needed to keep the code
simple and minimise the LED triggers impact on those systems.

Cheers,

Richard

Re: [PATCH] led: simplify led_trigger_register_simple

[Felipe Balbi]

On Thu, Nov 20, 2008 at 01:33:28PM +0000, ext Richard Purdie wrote:
> On Thu, 2008-11-13 at 21:10 +0200, Felipe Balbi wrote:
> > On Thu, Nov 13, 2008 at 08:14:16PM +0200, Felipe Balbi wrote:
> > > On Thu, Nov 13, 2008 at 12:38:32PM +0000, Richard Purdie wrote:
> > > > The simple triggers were designed to cause minimum interference to the
> > > > usually external subsystem code they were added into. As an example this
> > > > meant things like errors were just handled gracefully with a printk
> > > > warning and did not take down the whole subsystem. I therefore don't
> > > > regard this patch as a simplification, more a complication.
> > > 
> > > That's a matter of changing the return ERR_PTR(err); back to a printk.
> > 
> > And here you are. I still think we should at least kfree(trigger) in
> > case of error, though.
> 
> This patch now just changes the calling convention of the function which
> doesn't seem to serve much purpose.

Well, it's your call then. But I don't like the idea of declaring a
pointer, passing a pointer to that pointer to a function and changing
the value of the original pointer.

I think allocating inside led_trigger_register_simple() and returning
the pointer would look better.

> In answer to your question about kfree, I agree it needs to be called
> upon error. The callers should just be calling
> led_trigger_unregister_simple() in their failure paths though which
> should take care of all problems? I know we used to register the simple
> triggers late in paths so no error handling was needed to keep the code
> simple and minimise the LED triggers impact on those systems.

Well, led_trigger_register_simple() doesn't return anything. Imagine
led_trigger_register_simple() fails, but the driver author decides
it's not a failure if, let's say, a led doesn't turn on when we insert
a mmc card to the slot since it doesn't change functionality.

Now, imagine the user notes the led is not turning on and decides to
unload and reload the module to try again. Once again the led doesn't go
on. If the user keeps trying, it's quite a dangerous memory leak, right
?

-- 
balbi

Re: [PATCH] led: simplify led_trigger_register_simple

[Richard Purdie]

On Thu, 2008-11-20 at 16:14 +0200, Felipe Balbi wrote:
> > In answer to your question about kfree, I agree it needs to be called
> > upon error. The callers should just be calling
> > led_trigger_unregister_simple() in their failure paths though which
> > should take care of all problems? I know we used to register the simple
> > triggers late in paths so no error handling was needed to keep the code
> > simple and minimise the LED triggers impact on those systems.
> 
> Well, led_trigger_register_simple() doesn't return anything. Imagine
> led_trigger_register_simple() fails, but the driver author decides
> it's not a failure if, let's say, a led doesn't turn on when we insert
> a mmc card to the slot since it doesn't change functionality.
> 
> Now, imagine the user notes the led is not turning on and decides to
> unload and reload the module to try again. Once again the led doesn't go
> on. If the user keeps trying, it's quite a dangerous memory leak, right
> ?

So we have the module loading and one of two things happens:

led_trigger_register_simple() succeeds
led_trigger_register_simple() fails (probably from kmalloc failure)

The module doesn't know or care which happened. When the module unloads
it calls led_trigger_unregister_simple() which will free the memory in
the success case and do nothing in the case where it had failed.

So there is no memory leak?

Cheers,

Richard

Re: [PATCH] led: simplify led_trigger_register_simple

[Felipe Balbi]

On Thu, Nov 20, 2008 at 02:45:59PM +0000, ext Richard Purdie wrote:
> On Thu, 2008-11-20 at 16:14 +0200, Felipe Balbi wrote:
> > > In answer to your question about kfree, I agree it needs to be called
> > > upon error. The callers should just be calling
> > > led_trigger_unregister_simple() in their failure paths though which
> > > should take care of all problems? I know we used to register the simple
> > > triggers late in paths so no error handling was needed to keep the code
> > > simple and minimise the LED triggers impact on those systems.
> > 
> > Well, led_trigger_register_simple() doesn't return anything. Imagine
> > led_trigger_register_simple() fails, but the driver author decides
> > it's not a failure if, let's say, a led doesn't turn on when we insert
> > a mmc card to the slot since it doesn't change functionality.
> > 
> > Now, imagine the user notes the led is not turning on and decides to
> > unload and reload the module to try again. Once again the led doesn't go
> > on. If the user keeps trying, it's quite a dangerous memory leak, right
> > ?
> 
> So we have the module loading and one of two things happens:
> 
> led_trigger_register_simple() succeeds
> led_trigger_register_simple() fails (probably from kmalloc failure)
> 
> The module doesn't know or care which happened. When the module unloads
> it calls led_trigger_unregister_simple() which will free the memory in
> the success case and do nothing in the case where it had failed.
> 
> So there is no memory leak?

Hmmm, you are right. Didin't think about the exit path. But freeing in
led_triger_register_simple() if led_trigger_register() fails, also
doesn't seem wrong.

-- 
balbi