| Message ID | 20161019133355.6192-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wednesday 19 October 2016 15:33:54 Hans de Goede wrote: > + itself and the driver can detect this. Changes done by > + kernel triggers / software blinking and writing the brightness > + file are not signalled. Why? In case you have desktop application which show current brightness level and you change manually it via echo something > /sys/ then that application does not have any information about your change. And so it show old value...
Hi, On 19-10-16 15:59, Pali Rohár wrote: > On Wednesday 19 October 2016 15:33:54 Hans de Goede wrote: >> + itself and the driver can detect this. Changes done by >> + kernel triggers / software blinking and writing the brightness >> + file are not signalled. > > Why? In case you have desktop application which show current brightness > level and you change manually it via echo something > /sys/ then that > application does not have any information about your change. And so it > show old value... Well it seems like a bad idea to me to notify on changes caused by triggers and blinking, even though those are visible under sysfs if polling the brightness attribute. At which point it seemed to make sense to only notify on changes done autonomously by the hardware, rather then from any software (running on the main CPU). As for specifically not notifying on write, the sysfs interface is root only, so usually there will be a single daemon controlling the brightness, and any users can go through that daemon and it can distribute change messages itself (and will do so for the POLL_PRI events). Since the usual use case is a single writing process, which is also the same single process listening for POLL_PRI, it seems unnecessary to me to notify the process about the write it has just done. But if people think that we should consider multiple simultaneous users (which seems like a bad idea to me, because of coordination issues, but the API does allow it) and therefor should notify of the brightness change on a write, I'm fine with doing a new version implementing those semantics instead. Either way notifying on brightness changes caused by triggers / blinking seems like a bad idea to me. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, How about exploiting recently added userspace driver for the LED subsystem ((drivers/leds/uleds.c, available in linux-next) instead? If the LED class device to be observed implemented its internal trigger for generating kernel events upon device brightness change, then the userspace could create virtual LED class device, register on the trigger and poll the obtained file descriptor. See tools/leds/uledmon.c. Best regards, Jacek Anaszewski On 10/19/2016 06:07 PM, Hans de Goede wrote: > Hi, > > On 19-10-16 15:59, Pali Rohár wrote: >> On Wednesday 19 October 2016 15:33:54 Hans de Goede wrote: >>> + itself and the driver can detect this. Changes done by >>> + kernel triggers / software blinking and writing the brightness >>> + file are not signalled. >> >> Why? In case you have desktop application which show current brightness >> level and you change manually it via echo something > /sys/ then that >> application does not have any information about your change. And so it >> show old value... > > Well it seems like a bad idea to me to notify on changes caused by > triggers and blinking, even though those are visible under sysfs > if polling the brightness attribute. At which point it seemed to make > sense to only notify on changes done autonomously by the hardware, > rather then from any software (running on the main CPU). > > As for specifically not notifying on write, the sysfs interface is root > only, > so usually there will be a single daemon controlling the brightness, > and any users can go through that daemon and it can distribute change > messages itself (and will do so for the POLL_PRI events). > > Since the usual use case is a single writing process, which is also > the same single process listening for POLL_PRI, it seems unnecessary > to me to notify the process about the write it has just done. > > But if people think that we should consider multiple simultaneous > users (which seems like a bad idea to me, because of coordination > issues, but the API does allow it) and therefor should notify > of the brightness change on a write, I'm fine with doing a new > version implementing those semantics instead. > > Either way notifying on brightness changes caused by triggers / > blinking seems like a bad idea to me. > > Regards, > > Hans > > > -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! On Wednesday 19 October 2016 18:07:47 Hans de Goede wrote: > Hi, > > On 19-10-16 15:59, Pali Rohár wrote: > >On Wednesday 19 October 2016 15:33:54 Hans de Goede wrote: > >>+ itself and the driver can detect this. Changes done by > >>+ kernel triggers / software blinking and writing the brightness > >>+ file are not signalled. > > > >Why? In case you have desktop application which show current brightness > >level and you change manually it via echo something > /sys/ then that > >application does not have any information about your change. And so it > >show old value... > > Well it seems like a bad idea to me to notify on changes caused by > triggers and blinking, even though those are visible under sysfs > if polling the brightness attribute. At which point it seemed to make > sense to only notify on changes done autonomously by the hardware, > rather then from any software (running on the main CPU). I agree, notification for changes done by kernel triggers and blinking is bad idea! I mean only notification when somebody write directly to brightness sysfs attribute. > As for specifically not notifying on write, the sysfs interface is root only, > so usually there will be a single daemon controlling the brightness, > and any users can go through that daemon and it can distribute change > messages itself (and will do so for the POLL_PRI events). It is possible that there will be more daemons doing this thing. And on linux it is supported to have more alternative softwares which do similar/same thing... And once some application will need event that keyboard backlight was changed (e.g. it will show something on screen, etc), then such application must implement interface of any such daemon. It is easier for such application to open sysfs file and listen for POLL_PRI. > Since the usual use case is a single writing process, which is also > the same single process listening for POLL_PRI, it seems unnecessary > to me to notify the process about the write it has just done. Still this can be useful in scenario when only one "keyboard backlight daemon" is installed and running in system. In some cases for users or scripts is easier to change level directly by writing value into sysfs entry. I can imagine that this is what some "universal" power management scripts will do. For bash scripts it is easier to write some value into /sys as clicking on some desktop GUI slider or calling some complex dbus function... And I prefer that echo something > /sys/... too. > But if people think that we should consider multiple simultaneous > users (which seems like a bad idea to me, because of coordination > issues, but the API does allow it) and therefor should notify > of the brightness change on a write, I'm fine with doing a new > version implementing those semantics instead. Probably running more "keyboard backlight daemons" is not what people start doing, but having more applications which write value into /sys is not very rare. And I think that if "running more keyboard backlight daemons" is not supported by current version of kernel, then kernel should not allow such situation... It will be hard to debug such thing if happen. > Either way notifying on brightness changes caused by triggers / > blinking seems like a bad idea to me. Agree.
Hi, On 20-10-16 08:42, Jacek Anaszewski wrote: > Hi Hans, > > How about exploiting recently added userspace driver for the LED > subsystem ((drivers/leds/uleds.c, available in linux-next) instead? > > If the LED class device to be observed implemented its internal trigger > for generating kernel events upon device brightness change, then the > userspace could create virtual LED class device, register on the > trigger and poll the obtained file descriptor. > > See tools/leds/uledmon.c. Erm, this feels like a very wrong way to go about this, so now you want the led_classdev to send a new trigger to be picked up by a fake-led driven from userspace ? Having a led_classdev send triggers feels quite wrong, and having a u-led which will not really be a led at all, just a way to listen to the trigger seems wrong to me too. No this really is the wrong way to do this IMHO. We already have a well defined interface to wait for sysfs attribute changes for devices which export a sysfs interfaces, which is doing POLL_PRI on the sysfs attribute. Looking at the discussion between me and Pali I can see a clear consensus on the semantics of the poll here, we will notify any POLL_PRI listeners on long-lived changes to the brightness, either done by the hw autonomously or those done by a sysfs brightness write. Temporary brightness changes caused by triggers and/or blinking will not lead to a notify. If we can all agree on these semantics, then I believe that this will be a good interface to deal with this. Regards, Hans > > Best regards, > Jacek Anaszewski > > On 10/19/2016 06:07 PM, Hans de Goede wrote: >> Hi, >> >> On 19-10-16 15:59, Pali Rohár wrote: >>> On Wednesday 19 October 2016 15:33:54 Hans de Goede wrote: >>>> + itself and the driver can detect this. Changes done by >>>> + kernel triggers / software blinking and writing the brightness >>>> + file are not signalled. >>> >>> Why? In case you have desktop application which show current brightness >>> level and you change manually it via echo something > /sys/ then that >>> application does not have any information about your change. And so it >>> show old value... >> >> Well it seems like a bad idea to me to notify on changes caused by >> triggers and blinking, even though those are visible under sysfs >> if polling the brightness attribute. At which point it seemed to make >> sense to only notify on changes done autonomously by the hardware, >> rather then from any software (running on the main CPU). >> >> As for specifically not notifying on write, the sysfs interface is root >> only, >> so usually there will be a single daemon controlling the brightness, >> and any users can go through that daemon and it can distribute change >> messages itself (and will do so for the POLL_PRI events). >> >> Since the usual use case is a single writing process, which is also >> the same single process listening for POLL_PRI, it seems unnecessary >> to me to notify the process about the write it has just done. >> >> But if people think that we should consider multiple simultaneous >> users (which seems like a bad idea to me, because of coordination >> issues, but the API does allow it) and therefor should notify >> of the brightness change on a write, I'm fine with doing a new >> version implementing those semantics instead. >> >> Either way notifying on brightness changes caused by triggers / >> blinking seems like a bad idea to me. >> >> Regards, >> >> Hans >> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/20/2016 10:41 AM, Hans de Goede wrote: > Hi, > > On 20-10-16 08:42, Jacek Anaszewski wrote: >> Hi Hans, >> >> How about exploiting recently added userspace driver for the LED >> subsystem ((drivers/leds/uleds.c, available in linux-next) instead? >> >> If the LED class device to be observed implemented its internal trigger >> for generating kernel events upon device brightness change, then the >> userspace could create virtual LED class device, register on the >> trigger and poll the obtained file descriptor. >> >> See tools/leds/uledmon.c. > > Erm, this feels like a very wrong way to go about this, so now you > want the led_classdev to send a new trigger to be picked up by a fake-led > driven from userspace ? Having a led_classdev send triggers feels quite > wrong, and having a u-led which will not really be a led at all, just a > way to listen to the trigger seems wrong to me too. Generally the intention behind introducing userspace LED class driver was to have a means for intercepting kernel LED events. I agree that having LED class device generating trigger events is awkward. It was just the first thing that came to my mind seeing the idea of polling, and having the fresh memory of userspace LED driver. I am inclined to accept your patch, but it will need thorough testing to check if there will be no unpleasant side effects when one or more processes will be polling the brightness sysfs file and in the meantime other process(es) will write to it. Also notifying only about brightness change events not caused by writing brightness file is counter intuitive if we are polling brightness file. What about brightness changes caused by using led_set_brightness() API, without mediation of brightness file? If we want to notify only brightness changes originating from hardware, maybe it would be a good idea to add a dedicated sysfs file? It could appear only if relevant option in the kernel config was turned on. > > No this really is the wrong way to do this IMHO. > > We already have a well defined interface to wait for sysfs attribute > changes for devices which export a sysfs interfaces, which is doing > POLL_PRI on the sysfs attribute. > > Looking at the discussion between me and Pali I can see a clear > consensus on the semantics of the poll here, we will notify any > POLL_PRI listeners on long-lived changes to the brightness, either > done by the hw autonomously or those done by a sysfs brightness write. > Temporary brightness changes caused by triggers and/or blinking will > not lead to a notify. > > If we can all agree on these semantics, then I believe that this will > be a good interface to deal with this. > > Regards, > > Hans > > > >> >> Best regards, >> Jacek Anaszewski >> >> On 10/19/2016 06:07 PM, Hans de Goede wrote: >>> Hi, >>> >>> On 19-10-16 15:59, Pali Rohár wrote: >>>> On Wednesday 19 October 2016 15:33:54 Hans de Goede wrote: >>>>> + itself and the driver can detect this. Changes done by >>>>> + kernel triggers / software blinking and writing the >>>>> brightness >>>>> + file are not signalled. >>>> >>>> Why? In case you have desktop application which show current brightness >>>> level and you change manually it via echo something > /sys/ then that >>>> application does not have any information about your change. And so it >>>> show old value... >>> >>> Well it seems like a bad idea to me to notify on changes caused by >>> triggers and blinking, even though those are visible under sysfs >>> if polling the brightness attribute. At which point it seemed to make >>> sense to only notify on changes done autonomously by the hardware, >>> rather then from any software (running on the main CPU). >>> >>> As for specifically not notifying on write, the sysfs interface is root >>> only, >>> so usually there will be a single daemon controlling the brightness, >>> and any users can go through that daemon and it can distribute change >>> messages itself (and will do so for the POLL_PRI events). >>> >>> Since the usual use case is a single writing process, which is also >>> the same single process listening for POLL_PRI, it seems unnecessary >>> to me to notify the process about the write it has just done. >>> >>> But if people think that we should consider multiple simultaneous >>> users (which seems like a bad idea to me, because of coordination >>> issues, but the API does allow it) and therefor should notify >>> of the brightness change on a write, I'm fine with doing a new >>> version implementing those semantics instead. >>> >>> Either way notifying on brightness changes caused by triggers / >>> blinking seems like a bad idea to me. >>> >>> Regards, >>> >>> Hans >>> >>> >>> >> >> > > >
Hi, On 20-10-16 11:15, Jacek Anaszewski wrote: > On 10/20/2016 10:41 AM, Hans de Goede wrote: >> Hi, >> >> On 20-10-16 08:42, Jacek Anaszewski wrote: >>> Hi Hans, >>> >>> How about exploiting recently added userspace driver for the LED >>> subsystem ((drivers/leds/uleds.c, available in linux-next) instead? >>> >>> If the LED class device to be observed implemented its internal trigger >>> for generating kernel events upon device brightness change, then the >>> userspace could create virtual LED class device, register on the >>> trigger and poll the obtained file descriptor. >>> >>> See tools/leds/uledmon.c. >> >> Erm, this feels like a very wrong way to go about this, so now you >> want the led_classdev to send a new trigger to be picked up by a fake-led >> driven from userspace ? Having a led_classdev send triggers feels quite >> wrong, and having a u-led which will not really be a led at all, just a >> way to listen to the trigger seems wrong to me too. > > Generally the intention behind introducing userspace LED class driver > was to have a means for intercepting kernel LED events. I agree that > having LED class device generating trigger events is awkward. It was > just the first thing that came to my mind seeing the idea of polling, > and having the fresh memory of userspace LED driver. > > I am inclined to accept your patch Ok. > but it will need thorough testing > to check if there will be no unpleasant side effects when one or more > processes will be polling the brightness sysfs file and in the > meantime other process(es) will write to it. Those paths are really separate, sysfs_notify_dirent just schedules a wakeup (it is safe to be called from interrupt context) and does nothing else. Later on the task will wake up, and likely will call brightness_show(). Currently we can already have brightness_store() and brightness_show() race with each other according to a comment in brightness_show() this is safe, but I've my doubts about this, this means that a led_classdev's brightness_set and brightness_get method can be called simultaneously which seems wrong to me / seems to violate the principle of least surprise where I as a led-driver author would expect the led-core to protect me against this. So you're right we need to think about this, but this seems to be an orthogonal pre-existing problem/race which userspace can already trigger. Hmm, looking at the code closer I believe that the led code needs an audit for races in general. E.g. when sw blinking led_set_brightness() does a read-modify-write of led_cdev->flags but led_timer_function() also does read-modify-write of led_cdev->flags and nothing is protecting led_cdev->flags from these 2 happening at the same time. > Also notifying only about brightness change events not caused by > writing brightness file is counter intuitive if we are polling > brightness file. What about brightness changes caused by using > led_set_brightness() API, without mediation of brightness file? A valid question, after carefully reading the code I see that triggers and blinking will make brightness_show() / reading the brightness sysfs attribute return a different value, so yes you're right we should notify each time one of __led_set_brightness or __led_set_brightness_blocking succeeds. > If we want to notify only brightness changes originating from > hardware, maybe it would be a good idea to add a dedicated > sysfs file? It could appear only if relevant option in the > kernel config was turned on. I believe that simply allowing poll on the brightness sysfs attribute is better. Regards, Hans > >> >> No this really is the wrong way to do this IMHO. >> >> We already have a well defined interface to wait for sysfs attribute >> changes for devices which export a sysfs interfaces, which is doing >> POLL_PRI on the sysfs attribute. >> >> Looking at the discussion between me and Pali I can see a clear >> consensus on the semantics of the poll here, we will notify any >> POLL_PRI listeners on long-lived changes to the brightness, either >> done by the hw autonomously or those done by a sysfs brightness write. >> Temporary brightness changes caused by triggers and/or blinking will >> not lead to a notify. >> >> If we can all agree on these semantics, then I believe that this will >> be a good interface to deal with this. >> >> Regards, >> >> Hans >> >> >> >>> >>> Best regards, >>> Jacek Anaszewski >>> >>> On 10/19/2016 06:07 PM, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 19-10-16 15:59, Pali Rohár wrote: >>>>> On Wednesday 19 October 2016 15:33:54 Hans de Goede wrote: >>>>>> + itself and the driver can detect this. Changes done by >>>>>> + kernel triggers / software blinking and writing the >>>>>> brightness >>>>>> + file are not signalled. >>>>> >>>>> Why? In case you have desktop application which show current brightness >>>>> level and you change manually it via echo something > /sys/ then that >>>>> application does not have any information about your change. And so it >>>>> show old value... >>>> >>>> Well it seems like a bad idea to me to notify on changes caused by >>>> triggers and blinking, even though those are visible under sysfs >>>> if polling the brightness attribute. At which point it seemed to make >>>> sense to only notify on changes done autonomously by the hardware, >>>> rather then from any software (running on the main CPU). >>>> >>>> As for specifically not notifying on write, the sysfs interface is root >>>> only, >>>> so usually there will be a single daemon controlling the brightness, >>>> and any users can go through that daemon and it can distribute change >>>> messages itself (and will do so for the POLL_PRI events). >>>> >>>> Since the usual use case is a single writing process, which is also >>>> the same single process listening for POLL_PRI, it seems unnecessary >>>> to me to notify the process about the write it has just done. >>>> >>>> But if people think that we should consider multiple simultaneous >>>> users (which seems like a bad idea to me, because of coordination >>>> issues, but the API does allow it) and therefor should notify >>>> of the brightness change on a write, I'm fine with doing a new >>>> version implementing those semantics instead. >>>> >>>> Either way notifying on brightness changes caused by triggers / >>>> blinking seems like a bad idea to me. >>>> >>>> Regards, >>>> >>>> Hans >>>> >>>> >>>> >>> >>> >> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/20/2016 12:02 PM, Hans de Goede wrote: > Hi, > > On 20-10-16 11:15, Jacek Anaszewski wrote: >> On 10/20/2016 10:41 AM, Hans de Goede wrote: >>> Hi, >>> >>> On 20-10-16 08:42, Jacek Anaszewski wrote: >>>> Hi Hans, >>>> >>>> How about exploiting recently added userspace driver for the LED >>>> subsystem ((drivers/leds/uleds.c, available in linux-next) instead? >>>> >>>> If the LED class device to be observed implemented its internal trigger >>>> for generating kernel events upon device brightness change, then the >>>> userspace could create virtual LED class device, register on the >>>> trigger and poll the obtained file descriptor. >>>> >>>> See tools/leds/uledmon.c. >>> >>> Erm, this feels like a very wrong way to go about this, so now you >>> want the led_classdev to send a new trigger to be picked up by a >>> fake-led >>> driven from userspace ? Having a led_classdev send triggers feels quite >>> wrong, and having a u-led which will not really be a led at all, just a >>> way to listen to the trigger seems wrong to me too. >> >> Generally the intention behind introducing userspace LED class driver >> was to have a means for intercepting kernel LED events. I agree that >> having LED class device generating trigger events is awkward. It was >> just the first thing that came to my mind seeing the idea of polling, >> and having the fresh memory of userspace LED driver. >> >> I am inclined to accept your patch > > Ok. > >> but it will need thorough testing >> to check if there will be no unpleasant side effects when one or more >> processes will be polling the brightness sysfs file and in the >> meantime other process(es) will write to it. > > Those paths are really separate, sysfs_notify_dirent just schedules > a wakeup (it is safe to be called from interrupt context) and does > nothing else. Later on the task will wake up, and likely will > call brightness_show(). > > Currently we can already have brightness_store() and brightness_show() > race with each other according to a comment in brightness_show() > this is safe, but I've my doubts about this, this means that a > led_classdev's brightness_set and brightness_get method can be > called simultaneously which seems wrong to me / seems to violate > the principle of least surprise where I as a led-driver author > would expect the led-core to protect me against this. After taking a look at brightness_show() and its git history it is clear that the comment became invalid after the commit ca3259b36035 ("leds: Add support to leds with readable status"). I will submit a patch adding mutex protection there soon. > > So you're right we need to think about this, but this seems to > be an orthogonal pre-existing problem/race which userspace can > already trigger. > > Hmm, looking at the code closer I believe that the led code > needs an audit for races in general. E.g. when sw blinking > led_set_brightness() does a read-modify-write of led_cdev->flags > but led_timer_function() also does read-modify-write of led_cdev->flags > and nothing is protecting led_cdev->flags from these 2 happening at > the same time. Yes, general lack of synchronization in the LED core also drew my attention at first, but after analysis I came to conclusion that it was harmless and didn't result in leaving the subsystem in an inconsistent state. So far it seems not to have beaten anyone. >> Also notifying only about brightness change events not caused by >> writing brightness file is counter intuitive if we are polling >> brightness file. What about brightness changes caused by using >> led_set_brightness() API, without mediation of brightness file? > > A valid question, after carefully reading the code I see that > triggers and blinking will make brightness_show() / reading > the brightness sysfs attribute return a different value, > so yes you're right we should notify each time one of > __led_set_brightness or __led_set_brightness_blocking > succeeds. > >> If we want to notify only brightness changes originating from >> hardware, maybe it would be a good idea to add a dedicated >> sysfs file? It could appear only if relevant option in the >> kernel config was turned on. > > I believe that simply allowing poll on the brightness sysfs > attribute is better. Yes, if we're going to report all brightness change events consistently then it is a good candidate.
diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led index 86ace28..518b674 100644 --- a/Documentation/ABI/testing/sysfs-class-led +++ b/Documentation/ABI/testing/sysfs-class-led @@ -7,6 +7,11 @@ Description: have hardware brightness support so will just be turned on for non-zero brightness settings. The value is between 0 and /sys/class/leds/<led>/max_brightness. + The file supports poll() to detect changes, changes are only + signalled when the hardware / firmware changes the brightness + itself and the driver can detect this. Changes done by + kernel triggers / software blinking and writing the brightness + file are not signalled. What: /sys/class/leds/<led>/max_brightness Date: March 2006 diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index aa84e5b..3427a65 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -203,6 +203,14 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) dev_warn(parent, "Led %s renamed to %s due to name collision", led_cdev->name, dev_name(led_cdev->dev)); + led_cdev->brightness_kn = sysfs_get_dirent(led_cdev->dev->kobj.sd, + "brightness"); + if (!led_cdev->brightness_kn) { + dev_err(led_cdev->dev, "Error getting brightness kernfs_node\n"); + device_unregister(led_cdev->dev); + return -ENODEV; + } + #ifdef CONFIG_LEDS_TRIGGERS init_rwsem(&led_cdev->trigger_lock); #endif @@ -254,6 +262,7 @@ void led_classdev_unregister(struct led_classdev *led_cdev) flush_work(&led_cdev->set_brightness_work); + sysfs_put(led_cdev->brightness_kn); device_unregister(led_cdev->dev); down_write(&leds_list_lock); diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index 3bce448..ec085a6 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -308,6 +308,12 @@ int led_update_brightness(struct led_classdev *led_cdev) } EXPORT_SYMBOL_GPL(led_update_brightness); +void led_notify_brightness_change(struct led_classdev *led_cdev) +{ + sysfs_notify_dirent(led_cdev->brightness_kn); +} +EXPORT_SYMBOL_GPL(led_notify_brightness_change); + /* Caller must ensure led_cdev->led_access held */ void led_sysfs_disable(struct led_classdev *led_cdev) { diff --git a/include/linux/leds.h b/include/linux/leds.h index ddfcb2d..203eb26 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -13,6 +13,7 @@ #define __LINUX_LEDS_H_INCLUDED #include <linux/device.h> +#include <linux/kernfs.h> #include <linux/list.h> #include <linux/mutex.h> #include <linux/rwsem.h> @@ -94,6 +95,8 @@ struct led_classdev { struct work_struct set_brightness_work; int delayed_set_value; + struct kernfs_node *brightness_kn; + #ifdef CONFIG_LEDS_TRIGGERS /* Protects the trigger data below */ struct rw_semaphore trigger_lock; @@ -193,6 +196,15 @@ extern int led_set_brightness_sync(struct led_classdev *led_cdev, extern int led_update_brightness(struct led_classdev *led_cdev); /** + * led_notify_brightness_change - Notify userspace of hw brightness changes + * @led_cdev: the LED to do the notify on + * + * Let any users waiting for POLL_PRI on the led's brightness sysfs + * atrribute know that the brightness has been changed. + */ +extern void led_notify_brightness_change(struct led_classdev *led_cdev); + +/** * led_sysfs_disable - disable LED sysfs interface * @led_cdev: the LED to set *
In some use-cases led hardware may autonomously change its brightness, userspace may want to be able to listen for this happening. The standard way to listen for sysfs attributes changing is for userspace to poll for POLL_PRI on the attribute. This commit adds a led_notify_brightness_change helper function allowing drivers to signal userspace this way. This is documented in Documentation/ABI/testing/sysfs-class-led, as documented there this is only intended to signal autonomous changes done by the hardware and it is not the intention to do a sysfs_notify for triggers and blinking. One use-case for this is the keyboard backlight used on some laptops, which is controlled by a hardwired (firmware handled) hotkey. In this case we want to signal userspace of the brightness changes triggered by the hotkey (which does not generate key events). Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Documentation/ABI/testing/sysfs-class-led | 5 +++++ drivers/leds/led-class.c | 9 +++++++++ drivers/leds/led-core.c | 6 ++++++ include/linux/leds.h | 12 ++++++++++++ 4 files changed, 32 insertions(+)