leds: add led_trigger_rename function

PATCH Superseded Fabio Baltieri 2 participants 6 messages Sep 9, 2012 – Sep 23, 2012 LED

lore

Share & tools

lore

Fabio Baltieri

author

2012-09-09 15:28 UTC #0 lore raw

PATCH 1/1 leds: add led_trigger_rename function

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 <[email protected]>

Cc Kurt Van Dijck <[email protected]>

Cc Bryan Wu <[email protected]>

---
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(-)

drivers/leds/led-triggers.c +12 -1

index b53bf54..995ae5c 100644
@@ -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;

include/linux/leds.h +20 -2

index 5676197..a8546ba 100644
@@ -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,

Signature · 1 line

1.7.11.rc1.9.gf623ca1.dirty

Kurt Van Dijck

2012-09-10 13:55 UTC #1 lore raw

Hey Fabio,

Quoted 2 lines · Fabio Baltieri

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?

Quoted 3 lines

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

Fabio Baltieri

author

2012-09-10 17:56 UTC #2 lore raw

Quoted 5 lines · Kurt Van Dijck

Quoted 2 lines · Fabio Baltieri
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.

Quoted 4 lines

Quoted 2 lines
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

Signature · 1 line

Fabio Baltieri

Fabio Baltieri

author

2012-09-15 16:15 UTC #3 lore raw

In reply to Kurt Van Dijck

Hi Kurt,

Quoted 5 lines · Kurt Van Dijck

Quoted 3 lines
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.

Quoted 9 lines

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

Signature · 1 line

Fabio Baltieri

Fabio Baltieri

author

2012-09-15 16:21 UTC #4 lore raw

PATCH v2 1/1 leds: add led_trigger_rename function

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 <[email protected]>

Cc Kurt Van Dijck <[email protected]>

Cc Bryan Wu <[email protected]>

---
 drivers/leds/led-triggers.c | 13 +++++++++++++
 include/linux/leds.h        | 18 ++++++++++++++++++
 2 files changed, 31 insertions(+)

drivers/leds/led-triggers.c +13 -0

index b53bf54..c0f5d7c 100644
@@ -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)

include/linux/leds.h +18 -0

index 5676197..21644c7 100644
@@ -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
  */

Signature · 1 line

1.7.11.rc1.9.gf623ca1.dirty

Fabio Baltieri

author

2012-09-23 21:08 UTC #5 lore raw

Hello Bryan,

Quoted 5 lines · 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.

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

Signature · 1 line

Fabio Baltieri