[PATCH v2 RESEND] leds: add led_trigger_rename function

[PATCH v2 RESEND] 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 <cooloney@gmail.com>
---

Hi Bryan,

I'm resending this one as it was probably lost it in the mail transition.

Fabio

 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 262eb41..a4fa4bf 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -166,6 +166,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 6e53bb3..8107592 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.12.1

Re: [PATCH v2 RESEND] leds: add led_trigger_rename function

[Bryan Wu]

On Sun, Nov 4, 2012 at 1:54 AM, Fabio Baltieri <fabio.baltieri@gmail.com> 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.
>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> Cc: Kurt Van Dijck <kurt.van.dijck@eia.be>
> Cc: Bryan Wu <cooloney@gmail.com>
> ---
>
> Hi Bryan,
>
> I'm resending this one as it was probably lost it in the mail transition.
>
> Fabio
>
>  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 262eb41..a4fa4bf 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -166,6 +166,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);

Is this strcpy() safe here? Probably strncpy() or strlcpy() is safer.

> +       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 6e53bb3..8107592 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);
> +

Any example how to use this new API?

Thanks,
-Bryan

>  /*
>   * LED Triggers
>   */
> --
> 1.7.12.1
>

Re: [PATCH v2 RESEND] leds: add led_trigger_rename function

[Fabio Baltieri]

Hi Bryan,

On Tue, Nov 13, 2012 at 04:33:14PM -0800, Bryan Wu wrote:
> > +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);
> 
> Is this strcpy() safe here? Probably strncpy() or strlcpy() is safer.

Actually the LED subsystem is not aware of the string allocation size,
so I guess that strcpy is the only option here.

On the other side, the caller, who originally allocated the string,
should do the check properly, such as in:

	snprintf(name, sizeof(name), "%s-tx", netdev->name);
	led_trigger_rename_static(name, priv->tx_led_trig);

> > +extern void led_trigger_rename_static(const char *name,
> > +                                     struct led_trigger *trig);
> > +
> 
> Any example how to use this new API?

Sure!  That was developed as part of CANBUS LED triggers to have trigger
name follow can interfaces name changes.

Original patch using this function, including the whole discussion
behind it, was posted here:

https://lkml.org/lkml/2012/9/10/544

or you can find the complete set on my github branch:

http://github.com/fabiobaltieri/linux.git can-leds-devel

Fabio

-- 
Fabio Baltieri

Re: [PATCH v2 RESEND] leds: add led_trigger_rename function

[Bryan Wu]

OK, I will apply this patch to my tree.

Thanks,
-Bryan

On Wed, Nov 14, 2012 at 12:30 AM, Fabio Baltieri
<fabio.baltieri@gmail.com> wrote:
> Hi Bryan,
>
> On Tue, Nov 13, 2012 at 04:33:14PM -0800, Bryan Wu wrote:
>> > +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);
>>
>> Is this strcpy() safe here? Probably strncpy() or strlcpy() is safer.
>
> Actually the LED subsystem is not aware of the string allocation size,
> so I guess that strcpy is the only option here.
>
> On the other side, the caller, who originally allocated the string,
> should do the check properly, such as in:
>
>         snprintf(name, sizeof(name), "%s-tx", netdev->name);
>         led_trigger_rename_static(name, priv->tx_led_trig);
>
>> > +extern void led_trigger_rename_static(const char *name,
>> > +                                     struct led_trigger *trig);
>> > +
>>
>> Any example how to use this new API?
>
> Sure!  That was developed as part of CANBUS LED triggers to have trigger
> name follow can interfaces name changes.
>
> Original patch using this function, including the whole discussion
> behind it, was posted here:
>
> https://lkml.org/lkml/2012/9/10/544
>
> or you can find the complete set on my github branch:
>
> http://github.com/fabiobaltieri/linux.git can-leds-devel
>
> Fabio
>
> --
> Fabio Baltieri