Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] leds: triggers: send uevent when changing triggers

197 views
Skip to first unread message

Colin Cross

unread,
Jul 24, 2012, 8:40:02 PM7/24/12
to
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 <ccr...@android.com>
---
drivers/leds/led-triggers.c | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 46b4c76..a85ce09 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Bryan Wu

unread,
Jul 25, 2012, 2:20:02 AM7/25/12
to
On Wed, Jul 25, 2012 at 8:32 AM, Colin Cross <ccr...@android.com> wrote:
> 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
Bryan Wu <brya...@canonical.com>
Kernel Developer +86.186-168-78255 Mobile
Canonical Ltd. www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Greg KH

unread,
Jul 25, 2012, 10:40:01 AM7/25/12
to
On Wed, Jul 25, 2012 at 02:11:30PM +0800, Bryan Wu wrote:
> On Wed, Jul 25, 2012 at 8:32 AM, Colin Cross <ccr...@android.com> wrote:
> > 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

Colin Cross

unread,
Jul 25, 2012, 3:00:02 PM7/25/12
to
On Tue, Jul 24, 2012 at 11:11 PM, Bryan Wu <brya...@canonical.com> wrote:
> On Wed, Jul 25, 2012 at 8:32 AM, Colin Cross <ccr...@android.com> wrote:
>> 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

Bryan Wu

unread,
Jul 25, 2012, 11:40:01 PM7/25/12
to
On Thu, Jul 26, 2012 at 2:54 AM, Colin Cross <ccr...@android.com> wrote:
> On Tue, Jul 24, 2012 at 11:11 PM, Bryan Wu <brya...@canonical.com> wrote:
>> On Wed, Jul 25, 2012 at 8:32 AM, Colin Cross <ccr...@android.com> wrote:
>>> 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.
--
Bryan Wu <brya...@canonical.com>
Kernel Developer +86.186-168-78255 Mobile
Canonical Ltd. www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Greg KH

unread,
Jul 26, 2012, 12:10:01 AM7/26/12
to
On Thu, Jul 26, 2012 at 11:29:48AM +0800, Bryan Wu wrote:
> On Thu, Jul 26, 2012 at 2:54 AM, Colin Cross <ccr...@android.com> wrote:
> > On Tue, Jul 24, 2012 at 11:11 PM, Bryan Wu <brya...@canonical.com> wrote:
> >> On Wed, Jul 25, 2012 at 8:32 AM, Colin Cross <ccr...@android.com> wrote:
> >>> 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.

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

greg k-h

Bryan Wu

unread,
Jul 26, 2012, 1:10:01 AM7/26/12
to
On Thu, Jul 26, 2012 at 11:59 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Thu, Jul 26, 2012 at 11:29:48AM +0800, Bryan Wu wrote:
>> On Thu, Jul 26, 2012 at 2:54 AM, Colin Cross <ccr...@android.com> wrote:
>> > On Tue, Jul 24, 2012 at 11:11 PM, Bryan Wu <brya...@canonical.com> wrote:
>> >> On Wed, Jul 25, 2012 at 8:32 AM, Colin Cross <ccr...@android.com> wrote:
>> >>> 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.
>
> 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().

--
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 00012e3..04da869 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -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)
--


--
Bryan Wu <brya...@canonical.com>
Kernel Developer +86.186-168-78255 Mobile
Canonical Ltd. www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Greg KH

unread,
Jul 26, 2012, 1:00:02 PM7/26/12
to
On Thu, Jul 26, 2012 at 01:03:11PM +0800, Bryan Wu wrote:
> Just one quick patch for my idea: emitting a uevent in sysfs_create_file().
>
> --
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 00012e3..04da869 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -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);

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

Bryan Wu

unread,
Jul 27, 2012, 12:10:02 AM7/27/12
to
On Fri, Jul 27, 2012 at 12:51 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Thu, Jul 26, 2012 at 01:03:11PM +0800, Bryan Wu wrote:
>> Just one quick patch for my idea: emitting a uevent in sysfs_create_file().
>>
>> --
>> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
>> index 00012e3..04da869 100644
>> --- a/fs/sysfs/file.c
>> +++ b/fs/sysfs/file.c
>> @@ -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);
>
> 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

Colin Cross

unread,
Jul 31, 2012, 2:30:02 PM7/31/12
to
Given the rejection of the other solutions to this problem, and chance
of getting this acked?

Bryan Wu

unread,
Aug 6, 2012, 11:00:02 PM8/6/12
to
Greg, Richard and Henrique, can I take you guys' Ack here?

Thanks,
--
Bryan Wu <brya...@canonical.com>
Kernel Developer +86.186-168-78255 Mobile
Canonical Ltd. www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Greg KH

unread,
Aug 6, 2012, 11:40:01 PM8/6/12
to
Ack for what specific patch are you referring to?

greg k-h

Greg KH

unread,
Aug 6, 2012, 11:50:02 PM8/6/12
to
On Tue, Aug 07, 2012 at 11:38:10AM +0800, Bryan Wu wrote:
> 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...

Henrique de Moraes Holschuh

unread,
Aug 7, 2012, 10:40:01 AM8/7/12
to
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?

--
"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

Colin Cross

unread,
Aug 7, 2012, 3:50:02 PM8/7/12
to
On Tue, Aug 7, 2012 at 7:36 AM, Henrique de Moraes Holschuh
<h...@hmh.eng.br> wrote:
> On Tue, 07 Aug 2012, Bryan Wu wrote:
<snip>

>> 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?

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.

Colin Cross

unread,
Aug 7, 2012, 4:40:02 PM8/7/12
to
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.

A change notification will also be sent during registration of led class
devices or led triggers if the default trigger of an led class device
is found.

Colin Cross

unread,
Aug 7, 2012, 4:50:01 PM8/7/12
to
On Tue, Aug 7, 2012 at 1:42 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Tue, Aug 07, 2012 at 12:58:15PM -0700, Colin Cross wrote:
>> 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.
>>
>> A change notification will also be sent during registration of led class
>> devices or led triggers if the default trigger of an led class device
>> is found.
>
> Why would a change event be needed at this point in time? Nothing would
> have changed from the original "add" notification, right?

If the led class device has a default trigger configured,
device_create will called before the trigger has been set and the
trigger sysfs files have been created. The "change" notification will
happen after the extra files have been created.

Greg KH

unread,
Aug 7, 2012, 4:50:01 PM8/7/12
to
On Tue, Aug 07, 2012 at 12:58:15PM -0700, Colin Cross wrote:
> 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.
>
> A change notification will also be sent during registration of led class
> devices or led triggers if the default trigger of an led class device
> is found.

Why would a change event be needed at this point in time? Nothing would
have changed from the original "add" notification, right?

greg k-h

Greg KH

unread,
Aug 7, 2012, 5:00:02 PM8/7/12
to
On Tue, Aug 07, 2012 at 12:58:15PM -0700, Colin Cross wrote:
> 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.
>
> A change notification will also be sent during registration of led class
> devices or led triggers if the default trigger of an led class device
> is found.
>
> Signed-off-by: Colin Cross <ccr...@android.com>

Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

Greg KH

unread,
Aug 7, 2012, 5:00:02 PM8/7/12
to
On Tue, Aug 07, 2012 at 01:47:23PM -0700, Colin Cross wrote:
> On Tue, Aug 7, 2012 at 1:42 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> > On Tue, Aug 07, 2012 at 12:58:15PM -0700, Colin Cross wrote:
> >> 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.
> >>
> >> A change notification will also be sent during registration of led class
> >> devices or led triggers if the default trigger of an led class device
> >> is found.
> >
> > Why would a change event be needed at this point in time? Nothing would
> > have changed from the original "add" notification, right?
>
> If the led class device has a default trigger configured,
> device_create will called before the trigger has been set and the
> trigger sysfs files have been created. The "change" notification will
> happen after the extra files have been created.

Ok, that make sense, and is a good reason to send the change event,
thanks for explaining it.

greg k-h

Henrique de Moraes Holschuh

unread,
Aug 7, 2012, 9:20:01 PM8/7/12
to
On Tue, 07 Aug 2012, Colin Cross wrote:
> 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.
>
> A change notification will also be sent during registration of led class
> devices or led triggers if the default trigger of an led class device
> is found.
>
> Signed-off-by: Colin Cross <ccr...@android.com>

Acked-by: Henrique de Moraes Holschuh <h...@hmh.eng.br>

--
"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

Bryan Wu

unread,
Aug 7, 2012, 10:20:01 PM8/7/12
to
On Wed, Aug 8, 2012 at 9:12 AM, Henrique de Moraes Holschuh
<h...@hmh.eng.br> wrote:
> On Tue, 07 Aug 2012, Colin Cross wrote:
>> 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.
>>
>> A change notification will also be sent during registration of led class
>> devices or led triggers if the default trigger of an led class device
>> is found.
>>
>> Signed-off-by: Colin Cross <ccr...@android.com>
>
> Acked-by: Henrique de Moraes Holschuh <h...@hmh.eng.br>
>

Colin, could you please resend an update patch with some
documentations as Henrique pointed out? Then I will add Ack from
Henrique and Greg and apply this patch.

Thanks,
-Bryan

Bryan Wu

unread,
Aug 14, 2012, 7:40:02 AM8/14/12
to
On Wed, Aug 8, 2012 at 10:16 AM, Bryan Wu <brya...@canonical.com> wrote:
> On Wed, Aug 8, 2012 at 9:12 AM, Henrique de Moraes Holschuh
> <h...@hmh.eng.br> wrote:
>> On Tue, 07 Aug 2012, Colin Cross wrote:
>>> 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.
>>>
>>> A change notification will also be sent during registration of led class
>>> devices or led triggers if the default trigger of an led class device
>>> is found.
>>>
>>> Signed-off-by: Colin Cross <ccr...@android.com>
>>
>> Acked-by: Henrique de Moraes Holschuh <h...@hmh.eng.br>
>>
>
> Colin, could you please resend an update patch with some
> documentations as Henrique pointed out? Then I will add Ack from
> Henrique and Greg and apply this patch.
>

Colin, will you update the patch or just keep this one.

Bryan Wu

unread,
Aug 26, 2012, 9:50:01 PM8/26/12
to
My bad. I apologize to the delay. I missed the v2 original patch email
in this thread, but googled and found it.
Applied to my for-next branch now.

Thanks,
-Bryan

On Tue, Aug 14, 2012 at 7:34 PM, Bryan Wu <brya...@canonical.com> wrote:
> On Wed, Aug 8, 2012 at 10:16 AM, Bryan Wu <brya...@canonical.com> wrote:
>> On Wed, Aug 8, 2012 at 9:12 AM, Henrique de Moraes Holschuh
>> <h...@hmh.eng.br> wrote:
>>> On Tue, 07 Aug 2012, Colin Cross wrote:
>>>> 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.
>>>>
>>>> A change notification will also be sent during registration of led class
>>>> devices or led triggers if the default trigger of an led class device
>>>> is found.
>>>>
>>>> Signed-off-by: Colin Cross <ccr...@android.com>
>>>
>>> Acked-by: Henrique de Moraes Holschuh <h...@hmh.eng.br>
>>>
>>
>> Colin, could you please resend an update patch with some
>> documentations as Henrique pointed out? Then I will add Ack from
>> Henrique and Greg and apply this patch.
>>
>
> Colin, will you update the patch or just keep this one.
>
> -Bryan



--
Bryan Wu <brya...@canonical.com>
Kernel Developer +86.186-168-78255 Mobile
Canonical Ltd. www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
0 new messages