Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

Hans de Goede <hdegoede@xxxxxxxxxx> · Tue, 21 Feb 2017 15:40:16 +0100

Hi,

On 21-02-17 15:11, Pali Rohár wrote:
On Thursday 09 February 2017 16:44:17 Hans de Goede wrote:
Make dell-wmi notify on hotkey kbd brightness changes, listen for this
in dell-laptop and call led_classdev_notify_brightness_hw_changed.

This will allow userspace to monitor (poll) for brightness changes on
these LEDs caused by the hotkey.

Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Changes in v2:
-Use the new dell_smbios*notify functionality
Changes in v3:
-Simplify the if condition for calling led_notify_brightness_change
Changes in v4:
-Adjust for dell_smbios_*_notifier to dell_laptop_*_notifier rename
Changes in v5:
-Switch to new led-trigger based API for notifying userspace about
 keyboard backlight brightness changes.
Changes in v6:
-Switch to new brightness_hw_changed LED class API
Changes in v8:
-Cache last set brightness value and only call
 led_classdev_notify_brightness_hw_changed if the brightness has changed,
 this is necessary because the WMI events get triggered when we set the
 brightness ourselves too

NAK. This does not work. Brightness can and already is handled also by
userspace application from Dell libsmbios package. Not every brightness
change is going via kernel driver and so new variable kbd_led_brightness
contains just random value.

This shows why it is a bad idea to have userspace code directly poking the
hardware. There is a reason why we've been moving away from the xserver
directly poking gpu-s to doing everything in the kernel.

To me libsmbios is a relic, something of ages long gone by and a normal
user should never use it.

You're right that it can be used to change things underneath the kernel,
but as said normally a user should never do that.

What will happen if a user does that regardless is that an acpi event
will trigger and the following code will execute:

+		mutex_lock(&kbd_led_mutex);
+
+		brightness = kbd_led_level_get(&kbd_led);
+		if (kbd_led_brightness != brightness) {
+			kbd_led_brightness = brightness;
+			led_classdev_notify_brightness_hw_changed(&kbd_led,
+								  brightness);
+		}
+
+		mutex_unlock(&kbd_led_mutex);

After which the kbd_led_brightness will be in sync again, so basically
the only side effect of the user changing the keyboard brightness
through libsmbios will be led_classdev_notify_brightness_hw_changed()
getting called, which is not unsurprising if you change something outside
of the kernel, so I really see no problem here.

Currently the only listener for led_classdev_notify_brightness_hw_changed()
is the gnome3 desktop stack, and having the keyboard brightness
slider in the control panel update when setting the brightness outside
of the normal ways actually is a good thing.

Regards,

Hans

---
 drivers/platform/x86/dell-laptop.c | 47 +++++++++++++++++++++++++++++++++++++-
 drivers/platform/x86/dell-wmi.c    | 14 ++++++++----
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 70951f3..f30938b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
 static u8 kbd_previous_mode_bit;

 static bool kbd_led_present;
+static enum led_brightness kbd_led_brightness;
 static DEFINE_MUTEX(kbd_led_mutex);

 /*
@@ -1943,6 +1944,7 @@ static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
 static int kbd_led_level_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
+	enum led_brightness new_brightness = value;
 	struct kbd_state state;
 	struct kbd_state new_state;
 	u16 num;
@@ -1971,6 +1973,8 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
 		ret = -ENXIO;
 	}

+	if (ret == 0)
+		kbd_led_brightness = new_brightness;
 out:
 	mutex_unlock(&kbd_led_mutex);
 	return ret;
@@ -1978,6 +1982,7 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,

 static struct led_classdev kbd_led = {
 	.name           = "dell::kbd_backlight",
+	.flags		= LED_BRIGHT_HW_CHANGED,
 	.brightness_set_blocking = kbd_led_level_set,
 	.brightness_get = kbd_led_level_get,
 	.groups         = kbd_led_groups,
@@ -1985,6 +1990,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;
@@ -1996,7 +2003,12 @@ 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);
+	kbd_led_brightness = kbd_led_level_get(&kbd_led);
+	ret = led_classdev_register(dev, &kbd_led);
+	if (ret)
+		kbd_led_present = false;
+
+	return ret;
 }

 static void brightness_set_exit(struct led_classdev *led_cdev,
@@ -2013,6 +2025,36 @@ static void kbd_led_exit(void)
 	led_classdev_unregister(&kbd_led);
 }

+static int dell_laptop_notifier_call(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	enum led_brightness brightness;
+
+	switch (action) {
+	case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED:
+		if (!kbd_led_present)
+			break;
+
+		mutex_lock(&kbd_led_mutex);
+
+		brightness = kbd_led_level_get(&kbd_led);
+		if (kbd_led_brightness != brightness) {
+			kbd_led_brightness = brightness;
+			led_classdev_notify_brightness_hw_changed(&kbd_led,
+								  brightness);
+		}
+
+		mutex_unlock(&kbd_led_mutex);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block dell_laptop_notifier = {
+	.notifier_call = dell_laptop_notifier_call,
+};
+
 static int __init dell_init(void)
 {
 	struct calling_interface_buffer *buffer;
@@ -2056,6 +2098,8 @@ static int __init dell_init(void)
 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
 				    &dell_debugfs_fops);

+	dell_laptop_register_notifier(&dell_laptop_notifier);
+
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;

@@ -2107,6 +2151,7 @@ static int __init dell_init(void)

 static void __exit dell_exit(void)
 {
+	dell_laptop_unregister_notifier(&dell_laptop_notifier);
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 75e6370..15740b5 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -297,11 +297,11 @@ static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
 	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },

 	/* Keyboard backlight level changed */
-	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x01e1, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02ea, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02eb, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02ec, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02f6, { KEY_KBDILLUMTOGGLE } },
 };

 static struct input_dev *dell_wmi_input_dev;
@@ -329,6 +329,10 @@ static void dell_wmi_process_key(int type, int code)
 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
 		return;

+	if (key->keycode == KEY_KBDILLUMTOGGLE)
+		dell_laptop_call_notifier(
+			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }








