Skip to contentSkip to conversation

leds: triggers: send uevent when changing triggers

PATCH Superseded Colin Cross 4 participants 15 messages Jul 25, 2012 – Aug 7, 2012 1 message not in the archive

Subsystems (2)
Driver core LED

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Colin Cross

author
#0 lore raw

PATCH 1/1 leds: triggers: send uevent when changing triggers

Some triggers create sysfs files when they are enabled. Send a uevent "change" notification whenever the trigger is changed to allow userspace processes such as udev to modify permissions on the new files.

Signed-off-by Colin Cross <[email protected]>
---
 drivers/leds/led-triggers.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
drivers/leds/led-triggers.c +13 -0
index 46b4c76..a85ce09 100644
@@ -102,6 +102,12 @@ ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr,
 void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
 {
 	unsigned long flags;
+	char *event = NULL;
+	char *envp[2];
+	const char *name;
+
+	name = trigger ? trigger->name : "none";
+	event = kasprintf(GFP_KERNEL, "TRIGGER=%s", name);
 
 	/* Remove any existing trigger */
 	if (led_cdev->trigger) {

@@ -122,6 +128,13 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
 		if (trigger->activate)
 			trigger->activate(led_cdev);
 	}
+
+	if (event) {
+		envp[0] = event;
+		envp[1] = NULL;
+		kobject_uevent_env(&led_cdev->dev->kobj, KOBJ_CHANGE, envp);
+		kfree(event);
+	}
 }
 EXPORT_SYMBOL_GPL(led_trigger_set);
Signature · 1 line
1.7.7.3
#1 lore raw
Quoted 4 lines · Colin Cross

Some triggers create sysfs files when they are enabled. Send a uevent "change" notification whenever the trigger is changed to allow userspace processes such as udev to modify permissions on the new files.

This looks like an workaround only for led trigger, can we fix this in sysfs level?

Thanks,
-Bryan

Quoted 39 lines
Signed-off-by Colin Cross <[email protected]>
---
 drivers/leds/led-triggers.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
drivers/leds/led-triggers.c +13 -1
index 46b4c76..a85ce09 100644
@@ -102,6 +102,12 @@ ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr,
 void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
 {
        unsigned long flags;
+       char *event = NULL;
+       char *envp[2];
+       const char *name;
+
+       name = trigger ? trigger->name : "none";
+       event = kasprintf(GFP_KERNEL, "TRIGGER=%s", name);
 
        /* Remove any existing trigger */
        if (led_cdev->trigger) {

@@ -122,6 +128,13 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
                if (trigger->activate)
                        trigger->activate(led_cdev);
        }
+
+       if (event) {
+               envp[0] = event;
+               envp[1] = NULL;
+               kobject_uevent_env(&led_cdev->dev->kobj, KOBJ_CHANGE, envp);
+               kfree(event);
+       }
 }
 EXPORT_SYMBOL_GPL(led_trigger_set);
 
--

1.7.7.3

Signature · 4 lines
Bryan Wu <[email protected]>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
maintainer
#2 lore raw
Quoted 8 lines · Bryan Wu
Quoted 4 lines · Colin Cross

Some triggers create sysfs files when they are enabled. Send a uevent "change" notification whenever the trigger is changed to allow userspace processes such as udev to modify permissions on the new files.

This looks like an workaround only for led trigger, can we fix this in sysfs level?

How do you propose doing that?

greg k-h

author
#3 lore raw
In reply to Bryan Wu
Quoted 8 lines · Bryan Wu
Quoted 4 lines · Colin Cross

Some triggers create sysfs files when they are enabled. Send a uevent "change" notification whenever the trigger is changed to allow userspace processes such as udev to modify permissions on the new files.

This looks like an workaround only for led trigger, can we fix this in sysfs level?

See the previous discussion here: https://lkml.org/lkml/2012/7/20/458

#4 lore raw
Quoted 11 lines · Colin Cross
Quoted 8 lines · Bryan Wu
Quoted 4 lines · Colin Cross

Some triggers create sysfs files when they are enabled. Send a uevent "change" notification whenever the trigger is changed to allow userspace processes such as udev to modify permissions on the new files.

This looks like an workaround only for led trigger, can we fix this in sysfs level?

See the previous discussion here: https://lkml.org/lkml/2012/7/20/458

Thanks, I went through this thread here. Actually it was archived in my email account, so I missed that during a trip.

Basically, I think this issue is a kind of general issue related to sysfs, not just only for led trigger system. And adding this uevent notification to a upper level LED driver is not good to me, if we got similar issue in other subsystem, we should add similar fix there. Why not we add this in sysfs when we call device_create_file(). And this will be benefit for other drivers.

Please point out me why we can't do that in sysfs level. Thanks.

Signature · 4 lines
Bryan Wu <[email protected]>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
maintainer
#5 lore raw
Quoted 16 lines · Bryan Wu
Quoted 11 lines · Colin Cross
Quoted 8 lines · Bryan Wu
Quoted 4 lines · Colin Cross

Some triggers create sysfs files when they are enabled. Send a uevent "change" notification whenever the trigger is changed to allow userspace processes such as udev to modify permissions on the new files.

This looks like an workaround only for led trigger, can we fix this in sysfs level?

See the previous discussion here: https://lkml.org/lkml/2012/7/20/458

Thanks, I went through this thread here. Actually it was archived in my email account, so I missed that during a trip.

Quoted 8 lines

Basically, I think this issue is a kind of general issue related to sysfs, not just only for led trigger system. And adding this uevent notification to a upper level LED driver is not good to me, if we got similar issue in other subsystem, we should add similar fix there. Why not we add this in sysfs when we call device_create_file(). And this will be benefit for other drivers.

Please point out me why we can't do that in sysfs level. Thanks.

Please point out to me how you _can_ do this at a sysfs level :)

greg k-h

#6 lore raw
Quoted 24 lines · Greg KH
Quoted 23 lines · Bryan Wu
Quoted 11 lines · Colin Cross
Quoted 8 lines · Bryan Wu
Quoted 4 lines · Colin Cross

Some triggers create sysfs files when they are enabled. Send a uevent "change" notification whenever the trigger is changed to allow userspace processes such as udev to modify permissions on the new files.

This looks like an workaround only for led trigger, can we fix this in sysfs level?

See the previous discussion here: https://lkml.org/lkml/2012/7/20/458

Thanks, I went through this thread here. Actually it was archived in my email account, so I missed that during a trip.

Basically, I think this issue is a kind of general issue related to sysfs, not just only for led trigger system. And adding this uevent notification to a upper level LED driver is not good to me, if we got similar issue in other subsystem, we should add similar fix there. Why not we add this in sysfs when we call device_create_file(). And this will be benefit for other drivers.

Quoted 5 lines
Quoted 1 line

Please point out me why we can't do that in sysfs level. Thanks.

Please point out to me how you _can_ do this at a sysfs level :)

greg k-h

Just one quick patch for my idea: emitting a uevent in sysfs_create_file().

--

fs/sysfs/file.c +0 -0
index 00012e3..04da869 100644
@@ -570,10 +570,14 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd,

const struct attribute *attr,

int sysfs_create_file(struct kobject * kobj, const struct attribute * attr) {
+ int err = 0;
+

        BUG_ON(!kobj || !kobj->sd || !attr);
-       return sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       err = sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       kobject_uevent(kobj, KOBJ_CHANGE);

+ return err;
}

int sysfs_create_files(struct kobject *kobj, const struct attribute **ptr)
--

Signature · 4 lines
Bryan Wu <[email protected]>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
maintainer
#7 lore raw
Quoted 9 lines · Bryan Wu

Just one quick patch for my idea: emitting a uevent in sysfs_create_file().

--

fs/sysfs/file.c +0 -0
index 00012e3..04da869 100644
@@ -570,10 +570,14 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd,
 const struct attribute *attr,

fs/sysfs/file.c int sysfs_add_file(struct sysfs_dirent *dir_sd, · line 579

Quoted 10 lines
fs/sysfs/file.c +4 -1
 
 int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)
 {
+       int err = 0;
+
        BUG_ON(!kobj || !kobj->sd || !attr);
 
-       return sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       err = sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       kobject_uevent(kobj, KOBJ_CHANGE);

That's a veritable flood of change events when a new kobject is created, right? It also created uevents for a device that has not told userspace that it is even present, which could cause massive confusion, don't you think?

greg k-h

#8 lore raw
Quoted 17 lines · Greg KH
Quoted 16 lines · Bryan Wu

Just one quick patch for my idea: emitting a uevent in sysfs_create_file().

--

fs/sysfs/file.c +2 -0
index 00012e3..04da869 100644
@@ -570,10 +570,14 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd,
 const struct attribute *attr,
 
 int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)
 {
+       int err = 0;
+
        BUG_ON(!kobj || !kobj->sd || !attr);
Quoted 9 lines

fs/sysfs/file.c int sysfs_add_file(struct sysfs_dirent *dir_sd, · line 579

Quoted 3 lines
fs/sysfs/file.c +2 -1
-       return sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       err = sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       kobject_uevent(kobj, KOBJ_CHANGE);

That's a veritable flood of change events when a new kobject is created, right? It also created uevents for a device that has not told userspace that it is even present, which could cause massive confusion, don't you think?

Indeed, this is unacceptable. I reworked a new patchset and just sent our for you review.

Thanks,
-Bryan

author
#9 lore raw
Quoted 28 lines · Bryan Wu
Quoted 17 lines · Greg KH
Quoted 16 lines · Bryan Wu

Just one quick patch for my idea: emitting a uevent in sysfs_create_file().

--

fs/sysfs/file.c +2 -0
index 00012e3..04da869 100644
@@ -570,10 +570,14 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd,
 const struct attribute *attr,
 
 int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)
 {
+       int err = 0;
+
        BUG_ON(!kobj || !kobj->sd || !attr);
Quoted 9 lines

fs/sysfs/file.c int sysfs_add_file(struct sysfs_dirent *dir_sd, · line 579

Quoted 3 lines
fs/sysfs/file.c +2 -1
-       return sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       err = sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       kobject_uevent(kobj, KOBJ_CHANGE);

That's a veritable flood of change events when a new kobject is created, right? It also created uevents for a device that has not told userspace that it is even present, which could cause massive confusion, don't you think?

Quoted 5 lines

Indeed, this is unacceptable. I reworked a new patchset and just sent our for you review.

Thanks,
-Bryan

Given the rejection of the other solutions to this problem, and chance of getting this acked?

#10 lore raw
Quoted 29 lines · Colin Cross
Quoted 28 lines · Bryan Wu
Quoted 26 lines · Greg KH
Quoted 9 lines · Bryan Wu

Just one quick patch for my idea: emitting a uevent in sysfs_create_file().

--

fs/sysfs/file.c +0 -0
index 00012e3..04da869 100644
@@ -570,10 +570,14 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd,
 const struct attribute *attr,

fs/sysfs/file.c int sysfs_add_file(struct sysfs_dirent *dir_sd, · line 579

Quoted 10 lines
fs/sysfs/file.c +4 -1
 
 int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)
 {
+       int err = 0;
+
        BUG_ON(!kobj || !kobj->sd || !attr);
 
-       return sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       err = sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       kobject_uevent(kobj, KOBJ_CHANGE);

That's a veritable flood of change events when a new kobject is created, right? It also created uevents for a device that has not told userspace that it is even present, which could cause massive confusion, don't you think?

Quoted 8 lines
Quoted 5 lines

Indeed, this is unacceptable. I reworked a new patchset and just sent our for you review.

Thanks,
-Bryan

Given the rejection of the other solutions to this problem, and chance of getting this acked?

Greg, Richard and Henrique, can I take you guys' Ack here?

Thanks,

Signature · 4 lines
Bryan Wu <[email protected]>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
maintainer
#11 lore raw
Quoted 33 lines · Bryan Wu
Quoted 32 lines · Colin Cross
Quoted 31 lines · Bryan Wu
Quoted 17 lines · Greg KH
Quoted 16 lines · Bryan Wu

Just one quick patch for my idea: emitting a uevent in sysfs_create_file().

--

fs/sysfs/file.c +2 -0
index 00012e3..04da869 100644
@@ -570,10 +570,14 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd,
 const struct attribute *attr,
 
 int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)
 {
+       int err = 0;
+
        BUG_ON(!kobj || !kobj->sd || !attr);
Quoted 9 lines

fs/sysfs/file.c int sysfs_add_file(struct sysfs_dirent *dir_sd, · line 579

Quoted 3 lines
fs/sysfs/file.c +2 -1
-       return sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       err = sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       kobject_uevent(kobj, KOBJ_CHANGE);

That's a veritable flood of change events when a new kobject is created, right? It also created uevents for a device that has not told userspace that it is even present, which could cause massive confusion, don't you think?

Indeed, this is unacceptable. I reworked a new patchset and just sent our for you review.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Thanks,
-Bryan

Given the rejection of the other solutions to this problem, and chance of getting this acked?

Greg, Richard and Henrique, can I take you guys' Ack here?

Ack for what specific patch are you referring to?

greg k-h

A message that is not in the archive (sent off-list or not yet received): CAK5ve-KLdwEqW6MLbusMRkaHBQkxTqOGLoVWzSmiuo2qxwtwmA@mail.gmail.com

maintainer
#13 lore raw
Quoted 48 lines · Bryan Wu
Quoted 5 lines · 在 2012-8-7 上午11:34，"Greg KH" <[email protected]>写道：
Quoted 3 lines · Bryan Wu
Quoted 2 lines · Colin Cross
Quoted 1 line · Bryan Wu

On Fri, Jul 27, 2012 at 12:51 AM, Greg KH <[email protected]>

Quoted 2 lines · wrote:
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines · Bryan Wu
Quoted 1 line

Just one quick patch for my idea: emitting a uevent in

sysfs_create_file().

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

--

fs/sysfs/file.c +0 -0
index 00012e3..04da869 100644
@@ -570,10 +570,14 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd,
 const struct attribute *attr,

 int sysfs_create_file(struct kobject * kobj, const struct attribute *

attr)

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 8 lines

 {
+       int err = 0;
+
        BUG_ON(!kobj || !kobj->sd || !attr);

-       return sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       err = sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       kobject_uevent(kobj, KOBJ_CHANGE);

That's a veritable flood of change events when a new kobject is

created,

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

right?  It also created uevents for a device that has not told

userspace

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
Quoted 9 lines
Quoted 3 lines

that it is even present, which could cause massive confusion, don't you think?

Indeed, this is unacceptable. I reworked a new patchset and just sent our for you review.

Thanks,
-Bryan

Given the rejection of the other solutions to this problem, and chance of getting this acked?

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

Greg, Richard and Henrique, can I take you guys' Ack here?

Ack for what specific patch are you referring to?

For Colin's patch in the first email of this loop.

I have no idea what that patch contained anymore, that was 1000+ emails ago...

#14 lore raw
In reply to Bryan Wu
Quoted 33 lines · Bryan Wu
Quoted 32 lines · Colin Cross
Quoted 31 lines · Bryan Wu
Quoted 17 lines · Greg KH
Quoted 16 lines · Bryan Wu

Just one quick patch for my idea: emitting a uevent in sysfs_create_file().

--

fs/sysfs/file.c +2 -0
index 00012e3..04da869 100644
@@ -570,10 +570,14 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd,
 const struct attribute *attr,
 
 int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)
 {
+       int err = 0;
+
        BUG_ON(!kobj || !kobj->sd || !attr);
Quoted 9 lines

fs/sysfs/file.c int sysfs_add_file(struct sysfs_dirent *dir_sd, · line 579

Quoted 3 lines
fs/sysfs/file.c +2 -1
-       return sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       err = sysfs_add_file(kobj->sd, attr, SYSFS_KOBJ_ATTR);
+       kobject_uevent(kobj, KOBJ_CHANGE);

That's a veritable flood of change events when a new kobject is created, right? It also created uevents for a device that has not told userspace that it is even present, which could cause massive confusion, don't you think?

Indeed, this is unacceptable. I reworked a new patchset and just sent our for you review.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Thanks,
-Bryan

Given the rejection of the other solutions to this problem, and chance of getting this acked?

Greg, Richard and Henrique, can I take you guys' Ack here?

Yes, you have my Acked-by, provided that the uevent is NOT sent before the led is fully registered (I cannot check right now if the patch does this right or not. I apologise in advance if this was an unecessary question).

I don't care whether the uevent gets sent right after registration, or only when the trigger *changes* after registering. But someone might, so it would be nice to document this.

Considering Greg's answer, maybe it would be best to resend the patch with the point above clarified in the commit message or in the in-tree documentation of the LED class?

Signature · 4 lines
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
author
#15 lore raw
Quoted 1 line · Henrique de Moraes Holschuh

On Tue, 07 Aug 2012, Bryan Wu wrote:

<snip>

Quoted 7 lines
Quoted 1 line

Greg, Richard and Henrique, can I take you guys' Ack here?

Yes, you have my Acked-by, provided that the uevent is NOT sent before the led is fully registered (I cannot check right now if the patch does this right or not. I apologise in advance if this was an unecessary question).

Quoted 7 lines

I don't care whether the uevent gets sent right after registration, or only when the trigger *changes* after registering. But someone might, so it would be nice to document this.

Considering Greg's answer, maybe it would be best to resend the patch with the point above clarified in the commit message or in the in-tree documentation of the LED class?

led_trigger_set_default is called last from led_classdev_register, so it will send a uevent during registration but after it is fully registered. I will resend the patch with the clarification.