| Message ID | 20170209154417.19040-8-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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. > --- > 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); > } >
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. 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); >> } >> >
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); > >> } > >> > >
Hi, On 21-02-17 15:50, Pali Rohár wrote: > 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? Yes, because the ABI specification for the new brightness_hw_changed says that poll() listeners will only be woken up if the brightness is changed outside of the kernel and not when the kernel changes it itself. Regards, Hans > >> 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); >>>> } >>>> >>> >
On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote: > >So do we really need this code which prevents update? > > Yes, because the ABI specification for the new brightness_hw_changed says > that poll() listeners will only be woken up if the brightness is changed > outside of the kernel and not when the kernel changes it itself. So in case there are two applications in userspace which want to monitor brightness change and both of those application could change brightness (via sysfs) then these two applications would not know about every brightness change and would be out-of-sync of reality what is really configured by kernel. This is one part which I did not liked in proposed ABI as it force userspace to choose and use only one brightness monitoring application at same time. Plus if ABI was specified that poll() could be used only for hardware change (and not by software e.g. kernel/userspace) then such functionality is not possible to implement for Dell machines. As it looks like Dell firmware send event about every change of brightness and we cannot distinguish if change was done by software (kernel) or by hardware itself. I do not know now if you already accepted that ABI, I'm just saying that I do not like it due to requirement for one userspace application as listener and also because it is not what can be used for Dell machines. Jacek, was this requirement in ABI already accepted into stable?
Hi, On 21-02-17 16:13, Pali Rohár wrote: > On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote: >>> So do we really need this code which prevents update? >> >> Yes, because the ABI specification for the new brightness_hw_changed says >> that poll() listeners will only be woken up if the brightness is changed >> outside of the kernel and not when the kernel changes it itself. > > So in case there are two applications in userspace which want to monitor > brightness change and both of those application could change brightness > (via sysfs) then these two applications would not know about every > brightness change and would be out-of-sync of reality what is really > configured by kernel. Yes, because with triggers and blinking etc. it is impossible for userspace to continuously monitor brigthness in a way which does not cause a high system load. > This is one part which I did not liked in proposed ABI as it force > userspace to choose and use only one brightness monitoring application > at same time. Oh please, we are not going to have this whole ABI discussion again. I already spend months on this, this ship has long sailed. > Plus if ABI was specified that poll() could be used only for hardware > change (and not by software e.g. kernel/userspace) then such > functionality is not possible to implement for Dell machines. As it > looks like Dell firmware send event about every change of brightness and > we cannot distinguish if change was done by software (kernel) or by > hardware itself. It is possible just fine on Dell machines, except that libsmbios changes will get seen as hardware triggered changes, which in a way they are as libsmbios is making hardware changes behind the kernels back. > I do not know now if you already accepted that ABI Yes the ABI is in current next, and given that the 4.11 merge-window has opened this ship has really sailed, also you're making a problem where there really is none. Now can we please move forward with this ? Regards, Hans
On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote: > Hi, > > On 21-02-17 16:13, Pali Rohár wrote: > >On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote: > >>>So do we really need this code which prevents update? > >> > >>Yes, because the ABI specification for the new brightness_hw_changed says > >>that poll() listeners will only be woken up if the brightness is changed > >>outside of the kernel and not when the kernel changes it itself. > > > >So in case there are two applications in userspace which want to monitor > >brightness change and both of those application could change brightness > >(via sysfs) then these two applications would not know about every > >brightness change and would be out-of-sync of reality what is really > >configured by kernel. > > Yes, because with triggers and blinking etc. it is impossible for > userspace to continuously monitor brigthness in a way which does not > cause a high system load. Triggers and blinking features are out due to high cpu load. Fine. But why also manual writes to /sys/class/leds/... by userspace applications is filtered and not reported via poll()? > >This is one part which I did not liked in proposed ABI as it force > >userspace to choose and use only one brightness monitoring application > >at same time. > > Oh please, we are not going to have this whole ABI discussion again. > I already spend months on this, this ship has long sailed. Ok, I just want to know reason for above one question. > >Plus if ABI was specified that poll() could be used only for hardware > >change (and not by software e.g. kernel/userspace) then such > >functionality is not possible to implement for Dell machines. As it > >looks like Dell firmware send event about every change of brightness and > >we cannot distinguish if change was done by software (kernel) or by > >hardware itself. > > It is possible just fine on Dell machines, except that libsmbios changes > will get seen as hardware triggered changes, which in a way they are > as libsmbios is making hardware changes behind the kernels back. Tool smbios-keyboard-ctl is valid and will be there. If you like it or not we need to deal with fact that libsmbios exists and is used... Due to fundamental reasons we ignore all race condition between libsmbios and kernel (they are basically not possible to solve). I'm fine with this. But why should setting keyboard backlight via smbios-keyboard-ctl and via echo > /sys/class/leds/ behave differently? Only just because we say that some userspace application should not be used and because we invented new kernel ABI which is not fully designed for this case for existing Dell machines? For me it looks like a bad design somewhere... And also I do not like implementation in this dell driver that for every one received event we need to query smbios with another SMM call to get current brightness level. And after few seconds userspace ask again for current level (because it received event that brightness was changed) and we need to do another SMM call. > >I do not know now if you already accepted that ABI > > Yes the ABI is in current next, and given that the 4.11 merge-window has > opened this ship has really sailed, also you're making a problem where > there really is none. Now can we please move forward with this ? > > Regards, > > Hans
On 02/21/2017 04:13 PM, Pali Rohár wrote: > On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote: >>> So do we really need this code which prevents update? >> >> Yes, because the ABI specification for the new brightness_hw_changed says >> that poll() listeners will only be woken up if the brightness is changed >> outside of the kernel and not when the kernel changes it itself. > > So in case there are two applications in userspace which want to monitor > brightness change and both of those application could change brightness > (via sysfs) then these two applications would not know about every > brightness change and would be out-of-sync of reality what is really > configured by kernel. > > This is one part which I did not liked in proposed ABI as it force > userspace to choose and use only one brightness monitoring application > at same time. > > Plus if ABI was specified that poll() could be used only for hardware > change (and not by software e.g. kernel/userspace) then such > functionality is not possible to implement for Dell machines. As it > looks like Dell firmware send event about every change of brightness and > we cannot distinguish if change was done by software (kernel) or by > hardware itself. > > I do not know now if you already accepted that ABI, I'm just saying that > I do not like it due to requirement for one userspace application as > listener and also because it is not what can be used for Dell machines. > > Jacek, was this requirement in ABI already accepted into stable? Yes, Linus today pulled that and it is in mainline now. From LED subsystem point of view I don't see any problem - the file is called brightness_hw_changed and its purpose is to report brightness changes occurred in the hardware, outside of kernel control. It also returns last brightness as was set by the hardware, which can be different from what old brightness file reports in the same time.
Hi, On 21-02-17 18:08, Pali Rohár wrote: > On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote: >> Hi, >> >> On 21-02-17 16:13, Pali Rohár wrote: >>> On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote: >>>>> So do we really need this code which prevents update? >>>> >>>> Yes, because the ABI specification for the new brightness_hw_changed says >>>> that poll() listeners will only be woken up if the brightness is changed >>>> outside of the kernel and not when the kernel changes it itself. >>> >>> So in case there are two applications in userspace which want to monitor >>> brightness change and both of those application could change brightness >>> (via sysfs) then these two applications would not know about every >>> brightness change and would be out-of-sync of reality what is really >>> configured by kernel. >> >> Yes, because with triggers and blinking etc. it is impossible for >> userspace to continuously monitor brigthness in a way which does not >> cause a high system load. > > Triggers and blinking features are out due to high cpu load. Fine. > > But why also manual writes to /sys/class/leds/... by userspace > applications is filtered and not reported via poll()? I agree that having a way for interested userspace to detect those would be good, but that would need to be another API, may poll() on the brightness attribute itself while excluding triggers / blinking changes from wakeup ? Anyways that is something to discuss in a thread specific to the LED subsystem and somewhat orthogonal to this patch. One disadvantage of poll() is that it does not give the source of the change, so in retrospect I actually like the new brightness_hw_changed attribute as that does give the source, which is something which we need to know in userspace. In previous versions of the ABI I had to do the same brightness comparison I'm doing in the dell-laptop driver now in userspace, where it can never be done safely as userspace does not know about other userspace. Since the Dell smbios events don't provide us with a source of the change, we need to compare the brightness to the last set brightness somewhere and IMHO the kernel is the best (least bad) place to do that. > Due to fundamental reasons we ignore all race condition between > libsmbios and kernel (they are basically not possible to solve). I'm > fine with this. > > But why should setting keyboard backlight via smbios-keyboard-ctl and > via echo > /sys/class/leds/ behave differently? Because we cannot solve the smbios-keyboard-ctl case, but we can solve the echo case, as said we could probably use a new kernel ABI to allow userspace to detect changes caused by the echo example, but that really is a whole new discussion. Regards, Hans
On Wednesday 22 February 2017 09:36:08 Hans de Goede wrote: > Hi, > > On 21-02-17 18:08, Pali Rohár wrote: > >On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote: > >>Hi, > >> > >>On 21-02-17 16:13, Pali Rohár wrote: > >>>On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote: > >>>>>So do we really need this code which prevents update? > >>>> > >>>>Yes, because the ABI specification for the new brightness_hw_changed says > >>>>that poll() listeners will only be woken up if the brightness is changed > >>>>outside of the kernel and not when the kernel changes it itself. > >>> > >>>So in case there are two applications in userspace which want to monitor > >>>brightness change and both of those application could change brightness > >>>(via sysfs) then these two applications would not know about every > >>>brightness change and would be out-of-sync of reality what is really > >>>configured by kernel. > >> > >>Yes, because with triggers and blinking etc. it is impossible for > >>userspace to continuously monitor brigthness in a way which does not > >>cause a high system load. > > > >Triggers and blinking features are out due to high cpu load. Fine. > > > >But why also manual writes to /sys/class/leds/... by userspace > >applications is filtered and not reported via poll()? > > I agree that having a way for interested userspace to detect those > would be good, but that would need to be another API, may poll() > on the brightness attribute itself while excluding triggers / blinking > changes from wakeup ? > > Anyways that is something to discuss in a thread specific to the > LED subsystem and somewhat orthogonal to this patch. Ok, lets start discussion about it in new separate thread. I was in impression that this was already part of discussion and in proposed ABI. Now I see that it was just in original cpu consuming ABI which was rejected. > One disadvantage of poll() is that it does not give the source of > the change, so in retrospect I actually like the new brightness_hw_changed > attribute as that does give the source, which is something which we need > to know in userspace. Do you really in userspace need to know source of change? And to distinguish between change from hardware and change from userspace done by echo > /sys/class/leds? I though that this is for informing userspace application that led status was changed and application should update some bar or number which show current state of backlight level. > In previous versions of the ABI I had to do > the same brightness comparison I'm doing in the dell-laptop driver > now in userspace, where it can never be done safely as userspace does > not know about other userspace. > > Since the Dell smbios events don't provide us with a source of the > change, we need to compare the brightness to the last set brightness > somewhere and IMHO the kernel is the best (least bad) place to do > that. Maybe kernel place is the least bad place, but still it is unreliable for Dell machines. You do not know latency and delay how long it will take Dell firmware to deliver event to kernel. It also implies that there is race condition between delivering event and reading new backlight level. Basically it is different and similar race condition as you are fixing by another patch in this series. > >Due to fundamental reasons we ignore all race condition between > >libsmbios and kernel (they are basically not possible to solve). I'm > >fine with this. > > > >But why should setting keyboard backlight via smbios-keyboard-ctl and > >via echo > /sys/class/leds/ behave differently? > > Because we cannot solve the smbios-keyboard-ctl case, but we can solve > the echo case, as said we could probably use a new kernel ABI to allow > userspace to detect changes caused by the echo example, but that > really is a whole new discussion. > > Regards, > > Hans >
HI, On 22-02-17 09:49, Pali Rohár wrote: > On Wednesday 22 February 2017 09:36:08 Hans de Goede wrote: >> Hi, >> >> On 21-02-17 18:08, Pali Rohár wrote: >>> On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote: >>>> Hi, >>>> >>>> On 21-02-17 16:13, Pali Rohár wrote: >>>>> On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote: >>>>>>> So do we really need this code which prevents update? >>>>>> >>>>>> Yes, because the ABI specification for the new brightness_hw_changed says >>>>>> that poll() listeners will only be woken up if the brightness is changed >>>>>> outside of the kernel and not when the kernel changes it itself. >>>>> >>>>> So in case there are two applications in userspace which want to monitor >>>>> brightness change and both of those application could change brightness >>>>> (via sysfs) then these two applications would not know about every >>>>> brightness change and would be out-of-sync of reality what is really >>>>> configured by kernel. >>>> >>>> Yes, because with triggers and blinking etc. it is impossible for >>>> userspace to continuously monitor brigthness in a way which does not >>>> cause a high system load. >>> >>> Triggers and blinking features are out due to high cpu load. Fine. >>> >>> But why also manual writes to /sys/class/leds/... by userspace >>> applications is filtered and not reported via poll()? >> >> I agree that having a way for interested userspace to detect those >> would be good, but that would need to be another API, may poll() >> on the brightness attribute itself while excluding triggers / blinking >> changes from wakeup ? >> >> Anyways that is something to discuss in a thread specific to the >> LED subsystem and somewhat orthogonal to this patch. > > Ok, lets start discussion about it in new separate thread. I was in > impression that this was already part of discussion and in proposed ABI. > Now I see that it was just in original cpu consuming ABI which was > rejected. > >> One disadvantage of poll() is that it does not give the source of >> the change, so in retrospect I actually like the new brightness_hw_changed >> attribute as that does give the source, which is something which we need >> to know in userspace. > > Do you really in userspace need to know source of change? And to > distinguish between change from hardware and change from userspace done > by echo > /sys/class/leds? Yes, a change done through hardware (through the firmware handled hotkeys) will make the desktop environment show an osd overlay with the new kbd backlight brightness, where as a change done through e.g. the brightness slider in gnome-control-panel should not show that same osd. > I though that this is for informing userspace application that led > status was changed and application should update some bar or number > which show current state of backlight level. That is only part of it, we also want the OSD but ONLY when changed through the hotkeys (on other models the hotkeys are handled in userspace software, which will then also show the OSD, we want this to be consistent whether the keys are handled in firmware or in userspace). >> In previous versions of the ABI I had to do >> the same brightness comparison I'm doing in the dell-laptop driver >> now in userspace, where it can never be done safely as userspace does >> not know about other userspace. >> >> Since the Dell smbios events don't provide us with a source of the >> change, we need to compare the brightness to the last set brightness >> somewhere and IMHO the kernel is the best (least bad) place to do >> that. > > Maybe kernel place is the least bad place, but still it is unreliable > for Dell machines. > > You do not know latency and delay how long it will take Dell firmware to > deliver event to kernel. It also implies that there is race condition > between delivering event and reading new backlight level. A race condition which will always exists if userspace polls the backlight periodically say once a minute then it can always end up polling just before the hotkey press is done. Which is exactly why we need the event, so we don't need to poll and when the event is delivered we know the new backlight level. Regards, Hans
On Wednesday 22 February 2017 11:24:13 Hans de Goede wrote: > HI, > > On 22-02-17 09:49, Pali Rohár wrote: > >On Wednesday 22 February 2017 09:36:08 Hans de Goede wrote: > >>Hi, > >> > >>On 21-02-17 18:08, Pali Rohár wrote: > >>>On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote: > >>>>Hi, > >>>> > >>>>On 21-02-17 16:13, Pali Rohár wrote: > >>>>>On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote: > >>>>>>>So do we really need this code which prevents update? > >>>>>> > >>>>>>Yes, because the ABI specification for the new brightness_hw_changed says > >>>>>>that poll() listeners will only be woken up if the brightness is changed > >>>>>>outside of the kernel and not when the kernel changes it itself. > >>>>> > >>>>>So in case there are two applications in userspace which want to monitor > >>>>>brightness change and both of those application could change brightness > >>>>>(via sysfs) then these two applications would not know about every > >>>>>brightness change and would be out-of-sync of reality what is really > >>>>>configured by kernel. > >>>> > >>>>Yes, because with triggers and blinking etc. it is impossible for > >>>>userspace to continuously monitor brigthness in a way which does not > >>>>cause a high system load. > >>> > >>>Triggers and blinking features are out due to high cpu load. Fine. > >>> > >>>But why also manual writes to /sys/class/leds/... by userspace > >>>applications is filtered and not reported via poll()? > >> > >>I agree that having a way for interested userspace to detect those > >>would be good, but that would need to be another API, may poll() > >>on the brightness attribute itself while excluding triggers / blinking > >>changes from wakeup ? > >> > >>Anyways that is something to discuss in a thread specific to the > >>LED subsystem and somewhat orthogonal to this patch. > > > >Ok, lets start discussion about it in new separate thread. I was in > >impression that this was already part of discussion and in proposed ABI. > >Now I see that it was just in original cpu consuming ABI which was > >rejected. > > > >>One disadvantage of poll() is that it does not give the source of > >>the change, so in retrospect I actually like the new brightness_hw_changed > >>attribute as that does give the source, which is something which we need > >>to know in userspace. > > > >Do you really in userspace need to know source of change? And to > >distinguish between change from hardware and change from userspace done > >by echo > /sys/class/leds? > > Yes, a change done through hardware (through the firmware handled > hotkeys) will make the desktop environment show an osd overlay > with the new kbd backlight brightness, where as a change done > through e.g. the brightness slider in gnome-control-panel should > not show that same osd. > > >I though that this is for informing userspace application that led > >status was changed and application should update some bar or number > >which show current state of backlight level. > > That is only part of it, we also want the OSD but ONLY when changed > through the hotkeys (on other models the hotkeys are handled in > userspace software, which will then also show the OSD, we want this > to be consistent whether the keys are handled in firmware or in > userspace). Ok, so userspace application wants to distinguish between these types of events: 1) autonomous hardware decided to change brightness 2) application itself changed brightness 3) other application changed brightness 4) pressed keyboard hotkey changed brightness 4.1) managed by firmware 4.2) managed by some application And for 4) and 1) you want to show notification to user, right? > >>In previous versions of the ABI I had to do > >>the same brightness comparison I'm doing in the dell-laptop driver > >>now in userspace, where it can never be done safely as userspace does > >>not know about other userspace. > >> > >>Since the Dell smbios events don't provide us with a source of the > >>change, we need to compare the brightness to the last set brightness > >>somewhere and IMHO the kernel is the best (least bad) place to do > >>that. > > > >Maybe kernel place is the least bad place, but still it is unreliable > >for Dell machines. > > > >You do not know latency and delay how long it will take Dell firmware to > >deliver event to kernel. It also implies that there is race condition > >between delivering event and reading new backlight level. > > A race condition which will always exists if userspace polls the backlight > periodically say once a minute then it can always end up polling just > before the hotkey press is done. Which is exactly why we need the event, > so we don't need to poll and when the event is delivered we know the new > backlight level. So instead userspace polling you will ask for backlight level after receiving firmware event. This is same race as in userspace. Returned backlight level does not have to be one which caused firmware event. If firmware delays that event (and due to ACPI slowness it can be really delayed) then another backlight change could happen... And you will just use incorrect level for comparison. Basically on Dell machine you cannot distinguish between events done by 1), 2), 3) and 4). You are trying to hide this fact by bringing some logic which on first look can fix it... Also calling another SMBIOS call via SMM for every received event (even if userspace is not interested in it) does not look like good implementation. And another one is issued after userspace receive that event (via poll()) and try to know current value.
Hi, On 22-02-17 13:01, Pali Rohár wrote: > On Wednesday 22 February 2017 11:24:13 Hans de Goede wrote: >> HI, >> >> On 22-02-17 09:49, Pali Rohár wrote: >>> On Wednesday 22 February 2017 09:36:08 Hans de Goede wrote: >>>> Hi, >>>> >>>> On 21-02-17 18:08, Pali Rohár wrote: >>>>> On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote: >>>>>> Hi, >>>>>> >>>>>> On 21-02-17 16:13, Pali Rohár wrote: >>>>>>> On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote: >>>>>>>>> So do we really need this code which prevents update? >>>>>>>> >>>>>>>> Yes, because the ABI specification for the new brightness_hw_changed says >>>>>>>> that poll() listeners will only be woken up if the brightness is changed >>>>>>>> outside of the kernel and not when the kernel changes it itself. >>>>>>> >>>>>>> So in case there are two applications in userspace which want to monitor >>>>>>> brightness change and both of those application could change brightness >>>>>>> (via sysfs) then these two applications would not know about every >>>>>>> brightness change and would be out-of-sync of reality what is really >>>>>>> configured by kernel. >>>>>> >>>>>> Yes, because with triggers and blinking etc. it is impossible for >>>>>> userspace to continuously monitor brigthness in a way which does not >>>>>> cause a high system load. >>>>> >>>>> Triggers and blinking features are out due to high cpu load. Fine. >>>>> >>>>> But why also manual writes to /sys/class/leds/... by userspace >>>>> applications is filtered and not reported via poll()? >>>> >>>> I agree that having a way for interested userspace to detect those >>>> would be good, but that would need to be another API, may poll() >>>> on the brightness attribute itself while excluding triggers / blinking >>>> changes from wakeup ? >>>> >>>> Anyways that is something to discuss in a thread specific to the >>>> LED subsystem and somewhat orthogonal to this patch. >>> >>> Ok, lets start discussion about it in new separate thread. I was in >>> impression that this was already part of discussion and in proposed ABI. >>> Now I see that it was just in original cpu consuming ABI which was >>> rejected. >>> >>>> One disadvantage of poll() is that it does not give the source of >>>> the change, so in retrospect I actually like the new brightness_hw_changed >>>> attribute as that does give the source, which is something which we need >>>> to know in userspace. >>> >>> Do you really in userspace need to know source of change? And to >>> distinguish between change from hardware and change from userspace done >>> by echo > /sys/class/leds? >> >> Yes, a change done through hardware (through the firmware handled >> hotkeys) will make the desktop environment show an osd overlay >> with the new kbd backlight brightness, where as a change done >> through e.g. the brightness slider in gnome-control-panel should >> not show that same osd. >> >>> I though that this is for informing userspace application that led >>> status was changed and application should update some bar or number >>> which show current state of backlight level. >> >> That is only part of it, we also want the OSD but ONLY when changed >> through the hotkeys (on other models the hotkeys are handled in >> userspace software, which will then also show the OSD, we want this >> to be consistent whether the keys are handled in firmware or in >> userspace). > > Ok, so userspace application wants to distinguish between these types of > events: > > 1) autonomous hardware decided to change brightness > 2) application itself changed brightness > 3) other application changed brightness > 4) pressed keyboard hotkey changed brightness > 4.1) managed by firmware > 4.2) managed by some application > > And for 4) and 1) you want to show notification to user, right? Yes. >>>> In previous versions of the ABI I had to do >>>> the same brightness comparison I'm doing in the dell-laptop driver >>>> now in userspace, where it can never be done safely as userspace does >>>> not know about other userspace. >>>> >>>> Since the Dell smbios events don't provide us with a source of the >>>> change, we need to compare the brightness to the last set brightness >>>> somewhere and IMHO the kernel is the best (least bad) place to do >>>> that. >>> >>> Maybe kernel place is the least bad place, but still it is unreliable >>> for Dell machines. >>> >>> You do not know latency and delay how long it will take Dell firmware to >>> deliver event to kernel. It also implies that there is race condition >>> between delivering event and reading new backlight level. >> >> A race condition which will always exists if userspace polls the backlight >> periodically say once a minute then it can always end up polling just >> before the hotkey press is done. Which is exactly why we need the event, >> so we don't need to poll and when the event is delivered we know the new >> backlight level. > > So instead userspace polling you will ask for backlight level after > receiving firmware event. This is same race as in userspace. Returned > backlight level does not have to be one which caused firmware event. 1) These events do not happen 100s of times per second, they happen almost never 2) This is the best we can do given the firmware interface we have > If firmware delays that event (and due to ACPI slowness it can be really > delayed) then another backlight change could happen... And you will just > use incorrect level for comparison. Again given the frequency of these events this is not really an issue. > Basically on Dell machine you cannot distinguish between events done by > 1), 2), 3) and 4). You are trying to hide this fact by bringing some > logic which on first look can fix it... Sure we can distuingish 2, 3 and 4.2 will always go through the kernel and 1 and 4.1 will not, so we can just compare the last written value to the value after the event, this works fine. There really is no problem here. Granted using libsmbios to change things will cause the change to be seen as an autonomous / firmware change rahter then done by userspace, as you wrote yourself in a previous mail: "Due to fundamental reasons we ignore all race condition between libsmbios and kernel (they are basically not possible to solve). I'm fine with this." This is unsolvable, but the end result is that other userspace bits (if present) may mistake the change for an autonomous change and show the OSD, so the only bad side-effect is the OSD being shown in that case. And I really doubt this will ever happen, either the user uses something like GNOME and he will likely never use libsmbios directly, or he runs some more DIY desktop environment and does use smbiosctl in which case no one will be listening for the autonomous changes. > Also calling another SMBIOS call via SMM for every received event (even > if userspace is not interested in it) does not look like good > implementation. And another one is issued after userspace receive that > event (via poll()) and try to know current value. That is not true, the hw_brightness_changed attribute returns the last brightness passed to led_classdev_notify_brightness_hw_changed(), this is the whole reason why it has a brightness argument, this is actually done to avoid races, so that we get the brightness asap after receiving the event and then even if userspace takes it sweet time we actually report the brightness resulting from the event and not some later change (this is part of the ABI definition of brightness_hw_changed). So no we do not end up doing 2 SMBIOS calls and even if we were given the frequency of these events the performance impact would be absolutely negligible. You are really seeing a lot of problems where there are none, remember Linus' motto: perfect is the enemy of good. Now can we please move forward with this ? Regards, Hans
On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote: > 1) These events do not happen 100s of times per second, they happen > almost never 100 events per second is probably not happening, but on my Latitude I see that sometimes more events are delayed and delivered at once. > 2) This is the best we can do given the firmware interface we have What about just dropping upcoming one event in interval of next 2-3 second? Instead of trying to remember last state in kernel and then trying to mach if received event was that which was caused by change? This would allow us to reduce doing one SMM call for each received event and I think it would be same reliable as your approach.
Hi, On 01-03-17 12:15, Pali Rohár wrote: > On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote: >> 1) These events do not happen 100s of times per second, they happen >> almost never > > 100 events per second is probably not happening, but on my Latitude I > see that sometimes more events are delayed and delivered at once. > >> 2) This is the best we can do given the firmware interface we have > > What about just dropping upcoming one event in interval of next 2-3 > second? Instead of trying to remember last state in kernel and then > trying to mach if received event was that which was caused by change? That is way more racy then the solution with the kernel remembering the brightness. With my proposed solution there is basically no race, the only downside is the driver seeing a brightness change caused by libsmbios as one caused by the hotkeys, but it will do so 100% reliably and that is something which I believe we can live with just fine. > This would allow us to reduce doing one SMM call for each received > event and I think it would be same reliable as your approach. As I explained *already* we already have only one SMM call for reach received event, we pass the read-back brightness into led_classdev_notify_brightness_hw_changed and when userspace reads the new brightness_hw_changed event in response to the wakeup the led-core will use the value passed through led_classdev_notify_brightness_hw_changed so only 1 SMM call happens per event. Also again this does not happen 100 times per second you're really trying to optimize something which does not need any optimization at all here. Regards, Hans
On Wednesday 01 March 2017 13:02:58 Hans de Goede wrote: > Hi, > > On 01-03-17 12:15, Pali Rohár wrote: > >On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote: > >>1) These events do not happen 100s of times per second, they happen > >>almost never > > > >100 events per second is probably not happening, but on my Latitude I > >see that sometimes more events are delayed and delivered at once. > > > >>2) This is the best we can do given the firmware interface we have > > > >What about just dropping upcoming one event in interval of next 2-3 > >second? Instead of trying to remember last state in kernel and then > >trying to mach if received event was that which was caused by change? > > That is way more racy then the solution with the kernel remembering > the brightness. With my proposed solution there is basically no race, Is is racy. When you change keyboard backlight via LED sysfs and also in that time kernel receive hardware event about changed backlight, kernel do not know if that event comes as reaction to change via LED sysfs or as autonomous firmware decision or as reaction to pressing key for changing keyboard backlight (managed by firmware). I'm not talking here about userspace libsmbios. All is just by autonomous firmware, keyboard usage and LED sysfs API. And how is my idea about dropping exactly one event for one request for changing backlight via LED sysfs API more racy as yours? Received event from firmware has only the timestamp, no more information yet (*). Therefore state of events does is not ordered and any two events are basically same. In your provides patch you are adding additional information to event. It is currently read brightness level in time when was event delivered, not state of time when event was created. Which means you cannot swap any two received events anymore as they are part of current driver state. In my idea I do not add any additional information to event. Which means all received events are same in current driver state and I can swap them. So I can decide to drop one event when driver is changing backlight level. And because I can swap received events it does not matter if I drop "correct" one (that which was really generated by my backlight level change) or any other before or after (as they are same). The only important is that kernel use correct number of firmware events. Idea for time interval of 2-3 seconds is that if event is delivered later as after 3 seconds it is probably useless as driver application code can be already out-of-sync. But this time interval is just another idea and we do not need to use it. (*) Looks like for some machines in dell-wmi.c we know also which brightness level was set by firmware, but it is not used now and we still have machines without this information. > the only downside is the driver seeing a brightness change caused by > libsmbios as one caused by the hotkeys, but it will do so 100% > reliably and that is something which I believe we can live with > just fine. > > >This would allow us to reduce doing one SMM call for each received > >event and I think it would be same reliable as your approach. > > As I explained *already* we already have only one SMM call for reach > received event, we pass the read-back brightness into > led_classdev_notify_brightness_hw_changed and when userspace > reads the new brightness_hw_changed event in response to the wakeup > the led-core will use the value passed through > led_classdev_notify_brightness_hw_changed so only 1 SMM call happens > per event. > > Also again this does not happen 100 times per second you're really > trying to optimize something which does not need any optimization > at all here. We have reports that on some Dell notebooks is issuing SMM call causing freezing kernel for about 1-3 seconds (as firmware stay in SMM mode for a longer time...). > Regards, > > Hans >
Hi, On 01-03-17 13:55, Pali Rohár wrote: > On Wednesday 01 March 2017 13:02:58 Hans de Goede wrote: >> Hi, >> >> On 01-03-17 12:15, Pali Rohár wrote: >>> On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote: >>>> 1) These events do not happen 100s of times per second, they happen >>>> almost never >>> >>> 100 events per second is probably not happening, but on my Latitude I >>> see that sometimes more events are delayed and delivered at once. >>> >>>> 2) This is the best we can do given the firmware interface we have >>> >>> What about just dropping upcoming one event in interval of next 2-3 >>> second? Instead of trying to remember last state in kernel and then >>> trying to mach if received event was that which was caused by change? >> >> That is way more racy then the solution with the kernel remembering >> the brightness. With my proposed solution there is basically no race, > > Is is racy. When you change keyboard backlight via LED sysfs and also in > that time kernel receive hardware event about changed backlight, kernel > do not know if that event comes as reaction to change via LED sysfs or > as autonomous firmware decision or as reaction to pressing key for > changing keyboard backlight (managed by firmware). In this case there are only a limited number of scenarios: 1. The hotkey event gets the mutex before the sysfs write In this case there is no race as the event is processed before the sysfs write can be proceed. 2. The sysfs write gets the mutex before the hotkey event: There are 2 sub scenarios here: 2a) The sysfs writes the same value as the hotkey press just set: In this case the cached brightness will get updated to this new value before processing either event can be processed, when both events get processed they will fail the new_brightness != brightness check and no event will be generated. Arguably this is a bug but I believe if the user just set the brightness to the same value, making the hotkey change a nop that this behavior is fine. The sysfs write won the race after all, so from a kernel pov it really was first and the hotkey event really is a nop. 2b) The sysfs write writes a different value then the hotkey, in this case a question becomes which of the 2 we actually get, but this is up to the BIOS not up to the kernel, basically this depends on who won the race from the BIOS pov. If the sysfs write was seen last from the BIOS pov then our SMM call to get the current value will report the already cached value for both events and no events get generated. This make sense since the hotkey change got cancelled by the sysfs write. If otoh the hotkey gets handled last, then the first event will read back a different value, after which an events gets send to userspace and the cached value gets updated, changing the second event into a NOP. So in this case things just work as if there was a significant amount of time between the sysfs write and the hotkey press. TL;DR: the very worst then can happen is loosing a hotkey event because the exact same value was written to sysfs after the press was handled by the BIOS but before we get to process the event. > I'm not talking here about userspace libsmbios. All is just by > autonomous firmware, keyboard usage and LED sysfs API. > > And how is my idea about dropping exactly one event for one request for > changing backlight via LED sysfs API more racy as yours? You wrote: "What about just dropping upcoming one event in interval of next 2-3 second?" The time window you added makes things more razy. If we just increment an atomic_t on sysfs_write, and decrement_and_test on an event then that would be fine and would fix your sysfs write vs hotkey press write as we would get 2 events in that case. > Received event from firmware has only the timestamp, no more information > yet (*). Therefore state of events does is not ordered and any two > events are basically same. > > In your provides patch you are adding additional information to event. > It is currently read brightness level in time when was event delivered, Correct, that is part of the ABI definition of the brightness_hw_changed sysfs attribute, we must pass the brightness at the time of the hardware initiated change. > not state of time when event was created. Which means you cannot swap > any two received events anymore as they are part of current driver > state. > > In my idea I do not add any additional information to event. Which means > all received events are same in current driver state and I can swap > them. So I can decide to drop one event when driver is changing > backlight level. And because I can swap received events it does not > matter if I drop "correct" one (that which was really generated by my > backlight level change) or any other before or after (as they are same). > The only important is that kernel use correct number of firmware events. > > Idea for time interval of 2-3 seconds is that if event is delivered > later as after 3 seconds it is probably useless as driver application > code can be already out-of-sync. But this time interval is just another > idea and we do not need to use it. I really don't like the 2-3 seconds stuff, but I'm fine with going with your idea of just swallowing one event after a sysfs-write without the timeout stuff. > (*) Looks like for some machines in dell-wmi.c we know also which > brightness level was set by firmware, but it is not used now and we > still have machines without this information. > >> the only downside is the driver seeing a brightness change caused by >> libsmbios as one caused by the hotkeys, but it will do so 100% >> reliably and that is something which I believe we can live with >> just fine. >> >>> This would allow us to reduce doing one SMM call for each received >>> event and I think it would be same reliable as your approach. >> >> As I explained *already* we already have only one SMM call for reach >> received event, we pass the read-back brightness into >> led_classdev_notify_brightness_hw_changed and when userspace >> reads the new brightness_hw_changed event in response to the wakeup >> the led-core will use the value passed through >> led_classdev_notify_brightness_hw_changed so only 1 SMM call happens >> per event. >> >> Also again this does not happen 100 times per second you're really >> trying to optimize something which does not need any optimization >> at all here. > > We have reports that on some Dell notebooks is issuing SMM call causing > freezing kernel for about 1-3 seconds (as firmware stay in SMM mode for > a longer time...). I doubt those are notebooks with backlight keyboards, but anyways if we swallow 1 event per sysfs write we are only doing SMM calls when actually reporting a change to userspace at which point we need to make that smm call sooner or later anyways... Regards, Hans
On Wednesday 01 March 2017 14:58:45 Hans de Goede wrote: > >In my idea I do not add any additional information to event. Which means > >all received events are same in current driver state and I can swap > >them. So I can decide to drop one event when driver is changing > >backlight level. And because I can swap received events it does not > >matter if I drop "correct" one (that which was really generated by my > >backlight level change) or any other before or after (as they are same). > >The only important is that kernel use correct number of firmware events. > > > >Idea for time interval of 2-3 seconds is that if event is delivered > >later as after 3 seconds it is probably useless as driver application > >code can be already out-of-sync. But this time interval is just another > >idea and we do not need to use it. > > I really don't like the 2-3 seconds stuff, but I'm fine with going > with your idea of just swallowing one event after a sysfs-write > without the timeout stuff. Ok. I'm fine with just dropping exactly one event after setting new value via sysfs. It is OK for you? > >(*) Looks like for some machines in dell-wmi.c we know also which > >brightness level was set by firmware, but it is not used now and we > >still have machines without this information. > > > >>the only downside is the driver seeing a brightness change caused by > >>libsmbios as one caused by the hotkeys, but it will do so 100% > >>reliably and that is something which I believe we can live with > >>just fine. > >> > >>>This would allow us to reduce doing one SMM call for each received > >>>event and I think it would be same reliable as your approach. > >> > >>As I explained *already* we already have only one SMM call for reach > >>received event, we pass the read-back brightness into > >>led_classdev_notify_brightness_hw_changed and when userspace > >>reads the new brightness_hw_changed event in response to the wakeup > >>the led-core will use the value passed through > >>led_classdev_notify_brightness_hw_changed so only 1 SMM call happens > >>per event. > >> > >>Also again this does not happen 100 times per second you're really > >>trying to optimize something which does not need any optimization > >>at all here. > > > >We have reports that on some Dell notebooks is issuing SMM call causing > >freezing kernel for about 1-3 seconds (as firmware stay in SMM mode for > >a longer time...). > > I doubt those are notebooks with backlight keyboards, It is possible and also that no notebooks with backlight keyboard is affected. I just wanted to show that there can be a real problem as we got reports about such freezes in SMM. > but anyways if > we swallow 1 event per sysfs write we are only doing SMM calls when > actually reporting a change to userspace at which point we need to > make that smm call sooner or later anyways... Yes. But I think that we do not have to penalize users who are not using new brightness_hw_changed attribute. > Regards, > > Hans
Hi, On 03-03-17 13:00, Pali Rohár wrote: > On Wednesday 01 March 2017 14:58:45 Hans de Goede wrote: >>> In my idea I do not add any additional information to event. Which means >>> all received events are same in current driver state and I can swap >>> them. So I can decide to drop one event when driver is changing >>> backlight level. And because I can swap received events it does not >>> matter if I drop "correct" one (that which was really generated by my >>> backlight level change) or any other before or after (as they are same). >>> The only important is that kernel use correct number of firmware events. >>> >>> Idea for time interval of 2-3 seconds is that if event is delivered >>> later as after 3 seconds it is probably useless as driver application >>> code can be already out-of-sync. But this time interval is just another >>> idea and we do not need to use it. >> >> I really don't like the 2-3 seconds stuff, but I'm fine with going >> with your idea of just swallowing one event after a sysfs-write >> without the timeout stuff. > > Ok. I'm fine with just dropping exactly one event after setting new > value via sysfs. It is OK for you? Yes that will work for me. I will prepare a new version. Note I probably will not have time for this till the end of this week. One thing I was wondering, is what will happen if we write the same value to the sysfs brightness file twice, do we get events for both writes? One way to avoid this problem would be to compare the written value to the last known value and discard the write (avoiding an expensive SMM call) if they are equal. Discarding double writes does introduce a race between hotkey changes vs sysfs writes, if a hotkey change has been made but the event has not yet been handled then a sysfs write may be seen as being double and get discarded even though it is not double. OTOH if the hotkey press wins the race then the result would have been the write being discarded anyways, so I don't think this is a real problem. You're input on this would be appreciated. If writing the same value twice does not generate events then we must filter out the double writes. If it does drop events we can choose, the simplest would be to not filter in that case, avoiding any races (and userspace normally should not write the same value twice, but we cannot assume that). Regards, Hans
HI, On 06-03-17 14:39, Hans de Goede wrote: > Hi, > > On 03-03-17 13:00, Pali Rohár wrote: >> On Wednesday 01 March 2017 14:58:45 Hans de Goede wrote: >>>> In my idea I do not add any additional information to event. Which means >>>> all received events are same in current driver state and I can swap >>>> them. So I can decide to drop one event when driver is changing >>>> backlight level. And because I can swap received events it does not >>>> matter if I drop "correct" one (that which was really generated by my >>>> backlight level change) or any other before or after (as they are same). >>>> The only important is that kernel use correct number of firmware events. >>>> >>>> Idea for time interval of 2-3 seconds is that if event is delivered >>>> later as after 3 seconds it is probably useless as driver application >>>> code can be already out-of-sync. But this time interval is just another >>>> idea and we do not need to use it. >>> >>> I really don't like the 2-3 seconds stuff, but I'm fine with going >>> with your idea of just swallowing one event after a sysfs-write >>> without the timeout stuff. >> >> Ok. I'm fine with just dropping exactly one event after setting new >> value via sysfs. It is OK for you? > > Yes that will work for me. I will prepare a new version. Note I probably > will not have time for this till the end of this week. > > One thing I was wondering, is what will happen if we write the > same value to the sysfs brightness file twice, do we get events > for both writes? One way to avoid this problem would be to compare > the written value to the last known value and discard the write > (avoiding an expensive SMM call) if they are equal. > > Discarding double writes does introduce a race between hotkey changes > vs sysfs writes, if a hotkey change has been made but the event has > not yet been handled then a sysfs write may be seen as being double > and get discarded even though it is not double. OTOH if the hotkey > press wins the race then the result would have been the write being > discarded anyways, so I don't think this is a real problem. > > You're input on this would be appreciated. If writing the same value > twice does not generate events then we must filter out the double > writes. If it does drop events we can choose, the simplest would be > to not filter in that case, avoiding any races (and userspace normally > should not write the same value twice, but we cannot assume that). Ok, I've spend some time on this today. As I was afraid no new wmi events are generated when writing the same value to sysfs twice, making the ignore one event after write approach tricky. But luckily I've found a better fix, the kbd led related wmi events with a type of 0x0010 are only send when using the hotkeys. So if we ignore the events with a type of 0x0011, by dropping the part of the patch which was mapping those to KEY_KBDILLUMTOGGLE instead of to KEY_RESERVED, we only end up calling dell_laptop_call_notifier from dell-wmi.c on hotkey presses and we can drop the current vs new brightness comparison which you disliked from dell-laptop.c. So we can fix this by making the patch smaller :) I will prepare a v9 with these changes and send that out later today. Regards, Hans
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); }
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 --- drivers/platform/x86/dell-laptop.c | 47 +++++++++++++++++++++++++++++++++++++- drivers/platform/x86/dell-wmi.c | 14 ++++++++---- 2 files changed, 55 insertions(+), 6 deletions(-)