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

["Pali Rohár" <pali.rohar@gmail.com>]

On Tuesday 21 February 2017 15:40:16 Hans de Goede wrote:
> 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@redhat.com>
> >>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.

I agree.

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

Do not remember that libsmbios was there first and kernel code is
reimplementation from that userspace. Without libsmbios no kernel
support would be there.

And we can expect in future if Dell again decide to release some SMBIOS
code it will be in libsmbios...

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

I used it and often, because kernel does not have support for keyboard
backlight. And other people probably too.

So if we agree or not, we need to deal with fact that userspace can and
it is already calling smbios API. And started it doing earlier as kernel.

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

So do we really need this code which prevents update?

> 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);
> >> }
> >>
> >

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