[PATCH] leds: add led_trigger_rename function

[PATCH] leds: add led_trigger_rename function

[Fabio Baltieri]

Implements a led_trigger_rename() function to rename a trigger with
proper locking.

This also implies that "name" in "struct led_trigger" is not const
anymore.

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
Cc: Kurt Van Dijck <kurt.van.dijck@eia.be>
Cc: Bryan Wu <bryan.wu@canonical.com>
---
Hi all,

this adds a led_trigger_rename_static() function to change a trigger name
without races with other functions reading trigger names.  That would be
useful, for example, for network devices related triggers, where device name
can be changed by the user.

The implementation is actually just a locked strcpy from a temporary string
into the real one, so it probabily makes sense only for statically allocated
strings (i.e. fixed size), hence the "_static" suffix.

Any comments?

Fabio

 drivers/leds/led-triggers.c | 13 ++++++++++++-
 include/linux/leds.h        | 22 ++++++++++++++++++++--
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index b53bf54..995ae5c 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -153,6 +153,17 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_trigger_set_default);
 
+void led_trigger_rename_static(const char *name, struct led_trigger *trig)
+{
+	/* new name must be on a temporary string to prevent races */
+	BUG_ON(name == trig->name);
+
+	down_write(&triggers_list_lock);
+	strcpy(trig->name, name);
+	up_write(&triggers_list_lock);
+}
+EXPORT_SYMBOL_GPL(led_trigger_rename_static);
+
 /* LED Trigger Interface */
 
 int led_trigger_register(struct led_trigger *trig)
@@ -274,7 +285,7 @@ void led_trigger_blink_oneshot(struct led_trigger *trig,
 }
 EXPORT_SYMBOL_GPL(led_trigger_blink_oneshot);
 
-void led_trigger_register_simple(const char *name, struct led_trigger **tp)
+void led_trigger_register_simple(char *name, struct led_trigger **tp)
 {
 	struct led_trigger *trig;
 	int err;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 5676197..a8546ba 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -139,6 +139,24 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
 extern void led_set_brightness(struct led_classdev *led_cdev,
 			       enum led_brightness brightness);
 
+/**
+ * led_trigger_rename_static - rename a trigger
+ * @name: the new trigger name
+ * @trig: the LED trigger to rename
+ *
+ * Change a LED trigger name by copying the string passed in
+ * name into current trigger name, which MUST be large
+ * enough for the new string.
+ *
+ * Note that name must NOT point to the same string used
+ * during LED registration, as that could lead to races.
+ *
+ * This is meant to be used on triggers with statically
+ * allocated name.
+ */
+extern void led_trigger_rename_static(const char *name,
+				      struct led_trigger *trig);
+
 /*
  * LED Triggers
  */
@@ -148,7 +166,7 @@ extern void led_set_brightness(struct led_classdev *led_cdev,
 
 struct led_trigger {
 	/* Trigger Properties */
-	const char	 *name;
+	char		*name;
 	void		(*activate)(struct led_classdev *led_cdev);
 	void		(*deactivate)(struct led_classdev *led_cdev);
 
@@ -167,7 +185,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,
+extern void led_trigger_register_simple(char *name,
 				struct led_trigger **trigger);
 extern void led_trigger_unregister_simple(struct led_trigger *trigger);
 extern void led_trigger_event(struct led_trigger *trigger,
-- 
1.7.11.rc1.9.gf623ca1.dirty

Re: [PATCH] leds: add led_trigger_rename function

[Kurt Van Dijck]

Hey Fabio,

On Sun, Sep 09, 2012 at 05:27:54PM +0200, Fabio Baltieri wrote:
> Implements a led_trigger_rename() function to rename a trigger with
> proper locking.
This is the way to go.
Maybe add 'default_trigger' tests? In a later phase?

> 
> This also implies that "name" in "struct led_trigger" is not const
> anymore.
IMHO, this is wrong. This forces all other led_triggers
to put their name in non-const storage.

I see 2 options here:
* let 'name' be a 'const char *', and do a typecast.
  This works as long as led_trigger_rename is called only on led_triggers
  with non-const 'name' storage.
* put the storage of trigger name inside struct led_trigger conditionally
  (like can_dev does with their echo_skb), and when calling
  led_trigger_rename(), test if 'name' points to that storage.

Maybe there are other possibilities.

The latter is the safe way to do, but takes a bit more code.
The first way is a quick one ...

Kind regards,
Kurt

Re: [PATCH] leds: add led_trigger_rename function

[Fabio Baltieri]

On Mon, Sep 10, 2012 at 03:55:35PM +0200, Kurt Van Dijck wrote:
> On Sun, Sep 09, 2012 at 05:27:54PM +0200, Fabio Baltieri wrote:
> > Implements a led_trigger_rename() function to rename a trigger with
> > proper locking.
> This is the way to go.
> Maybe add 'default_trigger' tests? In a later phase?

Do you think that default_trigger should be processed on renames?  I
usually think of default_trigger as something just on boot time, but
that could make sense after all.

I think that could go for later phase anyway.

> > This also implies that "name" in "struct led_trigger" is not const
> > anymore.
> IMHO, this is wrong. This forces all other led_triggers
> to put their name in non-const storage.

Uh - is that really a problem?

Fabio

-- 
Fabio Baltieri

Re: [PATCH] leds: add led_trigger_rename function

[Fabio Baltieri]

Hi Kurt,

On Mon, Sep 10, 2012 at 03:55:35PM +0200, Kurt Van Dijck wrote:
> > 
> > This also implies that "name" in "struct led_trigger" is not const
> > anymore.
> IMHO, this is wrong. This forces all other led_triggers
> to put their name in non-const storage.

I had a second check on the patch and now I see your point.

> I see 2 options here:
> * let 'name' be a 'const char *', and do a typecast.
>   This works as long as led_trigger_rename is called only on led_triggers
>   with non-const 'name' storage.
> * put the storage of trigger name inside struct led_trigger conditionally
>   (like can_dev does with their echo_skb), and when calling
>   led_trigger_rename(), test if 'name' points to that storage.
> 
> Maybe there are other possibilities.

Modifying other driver would involve some useless kstrdup on constant
strigs or another unsafe casting, so I prefer to cast here in that case.

I'll send a v2 with the first solution you proposed, but I don't see a
really clean implementation here.

Any better idea?

Fabio

-- 
Fabio Baltieri

[PATCH v2] leds: add led_trigger_rename function

[Fabio Baltieri]

Implements a "led_trigger_rename" function to rename a trigger with
proper locking.

This assumes that led name was originally allocated in non-constant
storage.

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
Cc: Kurt Van Dijck <kurt.van.dijck@eia.be>
Cc: Bryan Wu <bryan.wu@canonical.com>
---
 drivers/leds/led-triggers.c | 13 +++++++++++++
 include/linux/leds.h        | 18 ++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index b53bf54..c0f5d7c 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -153,6 +153,19 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_trigger_set_default);
 
+void led_trigger_rename_static(const char *name, struct led_trigger *trig)
+{
+	/* new name must be on a temporary string to prevent races */
+	BUG_ON(name == trig->name);
+
+	down_write(&triggers_list_lock);
+	/* this assumes that trig->name was originaly allocated to
+	 * non constant storage */
+	strcpy((char *)trig->name, name);
+	up_write(&triggers_list_lock);
+}
+EXPORT_SYMBOL_GPL(led_trigger_rename_static);
+
 /* LED Trigger Interface */
 
 int led_trigger_register(struct led_trigger *trig)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 5676197..21644c7 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -139,6 +139,24 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
 extern void led_set_brightness(struct led_classdev *led_cdev,
 			       enum led_brightness brightness);
 
+/**
+ * led_trigger_rename_static - rename a trigger
+ * @name: the new trigger name
+ * @trig: the LED trigger to rename
+ *
+ * Change a LED trigger name by copying the string passed in
+ * name into current trigger name, which MUST be large
+ * enough for the new string.
+ *
+ * Note that name must NOT point to the same string used
+ * during LED registration, as that could lead to races.
+ *
+ * This is meant to be used on triggers with statically
+ * allocated name.
+ */
+extern void led_trigger_rename_static(const char *name,
+				      struct led_trigger *trig);
+
 /*
  * LED Triggers
  */
-- 
1.7.11.rc1.9.gf623ca1.dirty

Re: [PATCH v2] leds: add led_trigger_rename function

[Fabio Baltieri]

Hello Bryan,

On Sat, Sep 15, 2012 at 06:18:44PM +0200, Fabio Baltieri wrote:
> Implements a "led_trigger_rename" function to rename a trigger with
> proper locking.
> 
> This assumes that led name was originally allocated in non-constant
> storage.

Did you have any chance to review this patch?

I'm interested into getting some opinion for this feature for use in
another patches currently in a can-next branch.

Thanks,
Fabio

-- 
Fabio Baltieri