[PATCH 0/2] Add led_notify_brightness_change led-core function and use in dell-wmi driver

[PATCH 0/2] Add led_notify_brightness_change led-core function and use in dell-wmi driver

[Hans de Goede]

Hi All,

I was annoyed by the kbd-backlight on my Dell XPS 9550 not working well
together with the GNOME3 ui for kbd-backlight control.

Part of the problem here is that the hotkey on the laptop's keyboard to
control the kbd backlight is fully handled in firmware. So dell-wmi does
not send any key-presses for it. This is the right thing todo since
sending such key-presses will result in userspace adjusting the backlight,
while this has already been done by the firmware.

OTOH userspace should really still get some sort of event when this happens,
just like e.g. on some systems with hardwired volume-up/down buttons the
alsa mixer interface will send events to notify userspace about the changed
volume.

Checking other sysfs drivers, there is a standard way to notify userspace
about changes to a sysfs attributes underlying value. This patch-set adds
a led_notify_brightness_change led-core function using this standard
mechanism and makes the dell-wmi driver call led_notify_brightness_change
on the kbd_backlight led_classdev when the firmware has changed the
brightness. Together with some userspace changes / fixes this leads to
a much nicer / more integrated experience wrt kbd backlight control.

Currently this patch-set introduces only 1 user (dell-wmi) of the new
led_notify_brightness_change function, but I plan to also fix the same
issue on thinkpads (soon), and also one some HP models (when I can borrow
one, hopefully also soon).

Regards,

Hans

[PATCH 1/2] leds: core: Add led_notify_brightness_change helper function

[Hans de Goede]

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

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
  *
-- 
2.9.3

[PATCH 2/2] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Hans de Goede]

Make dell-wmi call led_notify_brightness_change on the kbd_led led_classdev
registered by dell-laptop when the kbd backlight brightness changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/Kconfig       |  1 +
 drivers/platform/x86/dell-laptop.c | 13 ++++++++++++-
 drivers/platform/x86/dell-smbios.c |  4 ++++
 drivers/platform/x86/dell-smbios.h |  2 ++
 drivers/platform/x86/dell-wmi.c    |  6 ++++++
 5 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 81b8dcc..9b6101c 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -124,6 +124,7 @@ config DELL_WMI
 	depends on INPUT
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
 	depends on DELL_SMBIOS
+	select NEW_LEDS
 	select INPUT_SPARSEKMAP
 	---help---
 	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 2c2f02b..6cb55d7 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1942,6 +1942,8 @@ static struct led_classdev kbd_led = {
 
 static int __init kbd_led_init(struct device *dev)
 {
+	int ret;
+
 	kbd_init();
 	if (!kbd_led_present)
 		return -ENODEV;
@@ -1953,7 +1955,15 @@ static int __init kbd_led_init(struct device *dev)
 		if (kbd_led.max_brightness)
 			kbd_led.max_brightness--;
 	}
-	return led_classdev_register(dev, &kbd_led);
+	ret = led_classdev_register(dev, &kbd_led);
+	if (ret) {
+		kbd_led_present = false;
+		return ret;
+	}
+
+	dell_kbd_backlight_led = &kbd_led;
+
+	return 0;
 }
 
 static void brightness_set_exit(struct led_classdev *led_cdev,
@@ -1966,6 +1976,7 @@ static void kbd_led_exit(void)
 {
 	if (!kbd_led_present)
 		return;
+	dell_kbd_backlight_led = NULL;
 	kbd_led.brightness_set = brightness_set_exit;
 	led_classdev_unregister(&kbd_led);
 }
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index d2412ab..7934953 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -21,6 +21,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/io.h>
+#include <linux/leds.h>
 #include "../../firmware/dcdbas.h"
 #include "dell-smbios.h"
 
@@ -40,6 +41,9 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
+struct led_classdev *dell_kbd_backlight_led;
+EXPORT_SYMBOL_GPL(dell_kbd_backlight_led);
+
 int dell_smbios_error(int value)
 {
 	switch (value) {
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index ec7d40a..ba7b90c 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,6 +35,8 @@ struct calling_interface_token {
 	};
 };
 
+extern struct led_classdev *dell_kbd_backlight_led;
+
 int dell_smbios_error(int value);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index da2fe18..de5ac59 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -36,6 +36,7 @@
 #include <linux/acpi.h>
 #include <linux/string.h>
 #include <linux/dmi.h>
+#include <linux/leds.h>
 #include <acpi/video.h>
 #include "dell-smbios.h"
 
@@ -319,6 +320,11 @@ static void dell_wmi_process_key(int type, int code)
 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
 		return;
 
+	if (type == 0x0011 && dell_kbd_backlight_led &&
+			(code == 0x01e1 || code == 0x02ea ||
+			 code == 0x02eb || code == 0x02ec || code == 0x02f6))
+		led_notify_brightness_change(dell_kbd_backlight_led);
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }
 
-- 
2.9.3

Re: [PATCH 1/2] leds: core: Add led_notify_brightness_change helper function

[Pali Rohár]

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

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/2] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Pali Rohár]

On Wednesday 19 October 2016 15:33:55 Hans de Goede wrote:
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index d2412ab..7934953 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -21,6 +21,7 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
> +#include <linux/leds.h>
>  #include "../../firmware/dcdbas.h"
>  #include "dell-smbios.h"
>  
> @@ -40,6 +41,9 @@ static int da_command_code;
>  static int da_num_tokens;
>  static struct calling_interface_token *da_tokens;
>  
> +struct led_classdev *dell_kbd_backlight_led;
> +EXPORT_SYMBOL_GPL(dell_kbd_backlight_led);
> +
>  int dell_smbios_error(int value)
>  {
>  	switch (value) {

This is ugly! dell-smbios.c file have nothing to do with led and
keyboard backlight. It is generic interface for sending and receiving
smbios requests.

I know, there are "layering problems" and something better should be
invented. Drivers dell-wmi.ko and dell-rbtn.ko provides events and
dell-laptop.ko want to listen for them. There is already hack in
dell-rbnt.ko which I really would like to fix and then delete...

So I rather do not want to see another hacks in that code.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/2] Add led_notify_brightness_change led-core function and use in dell-wmi driver

[Pali Rohár]

On Wednesday 19 October 2016 15:33:53 Hans de Goede wrote:
> Checking other sysfs drivers, there is a standard way to notify userspace
> about changes to a sysfs attributes underlying value. This patch-set adds
> a led_notify_brightness_change led-core function using this standard
> mechanism and makes the dell-wmi driver call led_notify_brightness_change
> on the kbd_backlight led_classdev when the firmware has changed the
> brightness. Together with some userspace changes / fixes this leads to
> a much nicer / more integrated experience wrt kbd backlight control.

Thank you for bringing this back! I had similar solution in my mind, but
I have not had time to implement it yet. It is great to see support for
this.

> Currently this patch-set introduces only 1 user (dell-wmi) of the new
> led_notify_brightness_change function, but I plan to also fix the same
> issue on thinkpads (soon),

Look at thinkpad acpi mailing list discussion. There is acpi information
how to listen for firmware event when keyboard backlight on thinkpad was
changed. Maybe it could help you...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 1/2] leds: core: Add led_notify_brightness_change helper function

[Hans de Goede]

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

Re: [PATCH 2/2] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Hans de Goede]

Hi,

On 19-10-16 16:06, Pali Rohár wrote:
> On Wednesday 19 October 2016 15:33:55 Hans de Goede wrote:
>> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
>> index d2412ab..7934953 100644
>> --- a/drivers/platform/x86/dell-smbios.c
>> +++ b/drivers/platform/x86/dell-smbios.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/mutex.h>
>>  #include <linux/slab.h>
>>  #include <linux/io.h>
>> +#include <linux/leds.h>
>>  #include "../../firmware/dcdbas.h"
>>  #include "dell-smbios.h"
>>
>> @@ -40,6 +41,9 @@ static int da_command_code;
>>  static int da_num_tokens;
>>  static struct calling_interface_token *da_tokens;
>>
>> +struct led_classdev *dell_kbd_backlight_led;
>> +EXPORT_SYMBOL_GPL(dell_kbd_backlight_led);
>> +
>>  int dell_smbios_error(int value)
>>  {
>>  	switch (value) {
>
> This is ugly! dell-smbios.c file have nothing to do with led and
> keyboard backlight. It is generic interface for sending and receiving
> smbios requests.
>
> I know, there are "layering problems" and something better should be
> invented. Drivers dell-wmi.ko and dell-rbtn.ko provides events and
> dell-laptop.ko want to listen for them. There is already hack in
> dell-rbnt.ko which I really would like to fix and then delete...
>
> So I rather do not want to see another hacks in that code.

I agree that this is not pretty, but I could not come up with a
simple other solution. If you've any suggestions on how you want
to see this implemented instead I can give that a try.

Regards,

Hans

Re: [PATCH 1/2] leds: core: Add led_notify_brightness_change helper function

[Jacek Anaszewski]

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

Re: [PATCH 1/2] leds: core: Add led_notify_brightness_change helper function

[Pali Rohár]

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.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/2] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Pali Rohár]

On Wednesday 19 October 2016 18:09:27 Hans de Goede wrote:
> I agree that this is not pretty, but I could not come up with a
> simple other solution. If you've any suggestions on how you want
> to see this implemented instead I can give that a try.

I was thinking about exporting some dell_wmi_notifier_register and
unregister functions (based on atomic_notifier_chain_register) from
dell-wmi. And dell-laptop could use it. Something similar is already
implemented in dell-rbtn+dell-latop, but still I'm consider it as hack.

This one in dell-wmi could be easier as dell-rbtn because dell-wmi is
monolitic. dell-wmi could be loaded only on systems which support it, so
at that register call (from dell-laptop) you already know if you receive
events or not.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 1/2] leds: core: Add led_notify_brightness_change helper function

[Hans de Goede]

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

Re: [PATCH 2/2] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Hans de Goede]

Hi,

On 20-10-16 09:48, Pali Rohár wrote:
> On Wednesday 19 October 2016 18:09:27 Hans de Goede wrote:
>> I agree that this is not pretty, but I could not come up with a
>> simple other solution. If you've any suggestions on how you want
>> to see this implemented instead I can give that a try.
>
> I was thinking about exporting some dell_wmi_notifier_register and
> unregister functions (based on atomic_notifier_chain_register) from
> dell-wmi. And dell-laptop could use it. Something similar is already
> implemented in dell-rbtn+dell-latop, but still I'm consider it as hack.
>
> This one in dell-wmi could be easier as dell-rbtn because dell-wmi is
> monolitic. dell-wmi could be loaded only on systems which support it, so
> at that register call (from dell-laptop) you already know if you receive
> events or not.

Ok, I will take a look at this once we've figured out the kernel led interface
to notify userspace of these changes.

Regards,

Hans

Re: [PATCH 1/2] leds: core: Add led_notify_brightness_change helper function

[Jacek Anaszewski]

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


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] leds: core: Add led_notify_brightness_change helper function

[Hans de Goede]

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

Re: [PATCH 1/2] leds: core: Add led_notify_brightness_change helper function

[Jacek Anaszewski]

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.

-- 
Best regards,
Jacek Anaszewski