| Message ID | 20180104085256.31431-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Thu, Jan 4, 2018 at 10:52 AM, Hans de Goede <hdegoede@redhat.com> wrote: > On some Dell XPS models WMI events of type 0x0000 reporting a keycode of > 0xe00c get reported when the brightness of the LCD panel changes. > > This leads to us reporting false-positive kbd_led change events to > userspace which in turn leads to the kbd backlight OSD showing when it > should not. > > We already read the current keyboard backlight brightness value when > reporting events because the led_classdev_notify_brightness_hw_changed > API requires this. Compare this value to the last known value and filter > out duplicate events, fixing this. > > Note the fixed issue is esp. a problem on XPS models with an ambient light > sensor and automatic brightness adjustments turned on, this causes the kbd > backlight OSD to show all the time there. > Should it have Fixes tag? Pali, any comment on this? > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514969 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/dell-laptop.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c > index cd4725e7e0b5..2ef3297a9efc 100644 > --- a/drivers/platform/x86/dell-laptop.c > +++ b/drivers/platform/x86/dell-laptop.c > @@ -1133,6 +1133,7 @@ static u8 kbd_previous_mode_bit; > > static bool kbd_led_present; > static DEFINE_MUTEX(kbd_led_mutex); > +static enum led_brightness kbd_led_level; > > /* > * NOTE: there are three ways to set the keyboard backlight level. > @@ -1947,6 +1948,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_value = value; > struct kbd_state state; > struct kbd_state new_state; > u16 num; > @@ -1976,6 +1978,9 @@ static int kbd_led_level_set(struct led_classdev *led_cdev, > } > > out: > + if (ret == 0) > + kbd_led_level = new_value; > + > mutex_unlock(&kbd_led_mutex); > return ret; > } > @@ -2003,6 +2008,9 @@ static int __init kbd_led_init(struct device *dev) > if (kbd_led.max_brightness) > kbd_led.max_brightness--; > } > + > + kbd_led_level = kbd_led_level_get(NULL); > + > ret = led_classdev_register(dev, &kbd_led); > if (ret) > kbd_led_present = false; > @@ -2027,13 +2035,25 @@ static void kbd_led_exit(void) > static int dell_laptop_notifier_call(struct notifier_block *nb, > unsigned long action, void *data) > { > + bool changed = false; > + enum led_brightness new_kbd_led_level; > + > switch (action) { > case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED: > if (!kbd_led_present) > break; > > - led_classdev_notify_brightness_hw_changed(&kbd_led, > - kbd_led_level_get(&kbd_led)); > + mutex_lock(&kbd_led_mutex); > + new_kbd_led_level = kbd_led_level_get(&kbd_led); > + if (kbd_led_level != new_kbd_led_level) { > + kbd_led_level = new_kbd_led_level; > + changed = true; > + } > + mutex_unlock(&kbd_led_mutex); > + > + if (changed) > + led_classdev_notify_brightness_hw_changed(&kbd_led, > + kbd_led_level); > break; > } > > -- > 2.14.3 >
Hi, On 05-01-18 11:40, Andy Shevchenko wrote: > On Thu, Jan 4, 2018 at 10:52 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> On some Dell XPS models WMI events of type 0x0000 reporting a keycode of >> 0xe00c get reported when the brightness of the LCD panel changes. >> >> This leads to us reporting false-positive kbd_led change events to >> userspace which in turn leads to the kbd backlight OSD showing when it >> should not. >> >> We already read the current keyboard backlight brightness value when >> reporting events because the led_classdev_notify_brightness_hw_changed >> API requires this. Compare this value to the last known value and filter >> out duplicate events, fixing this. >> >> Note the fixed issue is esp. a problem on XPS models with an ambient light >> sensor and automatic brightness adjustments turned on, this causes the kbd >> backlight OSD to show all the time there. >> > > Should it have Fixes tag? Good point, yes it should probably have a: Fixes: 9c656b0799 ("platform/x86: dell-*: Call new led hw_changed API on kbd brightness change") Regards, Hans > > Pali, any comment on this? > >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514969 >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/platform/x86/dell-laptop.c | 24 ++++++++++++++++++++++-- >> 1 file changed, 22 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c >> index cd4725e7e0b5..2ef3297a9efc 100644 >> --- a/drivers/platform/x86/dell-laptop.c >> +++ b/drivers/platform/x86/dell-laptop.c >> @@ -1133,6 +1133,7 @@ static u8 kbd_previous_mode_bit; >> >> static bool kbd_led_present; >> static DEFINE_MUTEX(kbd_led_mutex); >> +static enum led_brightness kbd_led_level; >> >> /* >> * NOTE: there are three ways to set the keyboard backlight level. >> @@ -1947,6 +1948,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_value = value; >> struct kbd_state state; >> struct kbd_state new_state; >> u16 num; >> @@ -1976,6 +1978,9 @@ static int kbd_led_level_set(struct led_classdev *led_cdev, >> } >> >> out: >> + if (ret == 0) >> + kbd_led_level = new_value; >> + >> mutex_unlock(&kbd_led_mutex); >> return ret; >> } >> @@ -2003,6 +2008,9 @@ static int __init kbd_led_init(struct device *dev) >> if (kbd_led.max_brightness) >> kbd_led.max_brightness--; >> } >> + >> + kbd_led_level = kbd_led_level_get(NULL); >> + >> ret = led_classdev_register(dev, &kbd_led); >> if (ret) >> kbd_led_present = false; >> @@ -2027,13 +2035,25 @@ static void kbd_led_exit(void) >> static int dell_laptop_notifier_call(struct notifier_block *nb, >> unsigned long action, void *data) >> { >> + bool changed = false; >> + enum led_brightness new_kbd_led_level; >> + >> switch (action) { >> case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED: >> if (!kbd_led_present) >> break; >> >> - led_classdev_notify_brightness_hw_changed(&kbd_led, >> - kbd_led_level_get(&kbd_led)); >> + mutex_lock(&kbd_led_mutex); >> + new_kbd_led_level = kbd_led_level_get(&kbd_led); >> + if (kbd_led_level != new_kbd_led_level) { >> + kbd_led_level = new_kbd_led_level; >> + changed = true; >> + } >> + mutex_unlock(&kbd_led_mutex); >> + >> + if (changed) >> + led_classdev_notify_brightness_hw_changed(&kbd_led, >> + kbd_led_level); >> break; >> } >> >> -- >> 2.14.3 >> > > >
On Friday 05 January 2018 12:40:04 Andy Shevchenko wrote: > On Thu, Jan 4, 2018 at 10:52 AM, Hans de Goede <hdegoede@redhat.com> wrote: > > On some Dell XPS models WMI events of type 0x0000 reporting a keycode of > > 0xe00c get reported when the brightness of the LCD panel changes. > > > > This leads to us reporting false-positive kbd_led change events to > > userspace which in turn leads to the kbd backlight OSD showing when it > > should not. > > > > We already read the current keyboard backlight brightness value when > > reporting events because the led_classdev_notify_brightness_hw_changed > > API requires this. Compare this value to the last known value and filter > > out duplicate events, fixing this. > > > > Note the fixed issue is esp. a problem on XPS models with an ambient light > > sensor and automatic brightness adjustments turned on, this causes the kbd > > backlight OSD to show all the time there. > > > > Should it have Fixes tag? > > Pali, any comment on this? See comments below. If there are not any problems, add my: Acked-by: Pali Rohár <pali.rohar@gmail.com> > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514969 > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/platform/x86/dell-laptop.c | 24 ++++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c > > index cd4725e7e0b5..2ef3297a9efc 100644 > > --- a/drivers/platform/x86/dell-laptop.c > > +++ b/drivers/platform/x86/dell-laptop.c > > @@ -1133,6 +1133,7 @@ static u8 kbd_previous_mode_bit; > > > > static bool kbd_led_present; > > static DEFINE_MUTEX(kbd_led_mutex); > > +static enum led_brightness kbd_led_level; Introduction of cached state of brightness level in kernel driver can lead to problem as userspace is able to change brightness (via direct DELL SMM API) without notice of kernel. But now patches for new interaction with DELL SMM via WMI are in kernel and there is explicit filter which blocks request for keyboard backlight. So hopefully this would not be a problem anymore... (or correct me if there still can be). > > /* > > * NOTE: there are three ways to set the keyboard backlight level. > > @@ -1947,6 +1948,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_value = value; > > struct kbd_state state; > > struct kbd_state new_state; > > u16 num; > > @@ -1976,6 +1978,9 @@ static int kbd_led_level_set(struct led_classdev *led_cdev, > > } > > > > out: > > + if (ret == 0) > > + kbd_led_level = new_value; > > + > > mutex_unlock(&kbd_led_mutex); > > return ret; > > } > > @@ -2003,6 +2008,9 @@ static int __init kbd_led_init(struct device *dev) > > if (kbd_led.max_brightness) > > kbd_led.max_brightness--; > > } > > + > > + kbd_led_level = kbd_led_level_get(NULL); Technically, kbd_led_level_get() is API function for struct led_cdev and therefore should take valid kbd_led, NULL there looks suspicious. But in reality this function itself ignores it first parameter. Probably the clean solution would be to have one private function for struct led_cdev usage and one which get current level for all other cases. This looks ugly too, so if somebody else would not come with better idea, let this code as is. > > + > > ret = led_classdev_register(dev, &kbd_led); > > if (ret) > > kbd_led_present = false; > > @@ -2027,13 +2035,25 @@ static void kbd_led_exit(void) > > static int dell_laptop_notifier_call(struct notifier_block *nb, > > unsigned long action, void *data) > > { > > + bool changed = false; > > + enum led_brightness new_kbd_led_level; > > + > > switch (action) { > > case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED: > > if (!kbd_led_present) > > break; > > > > - led_classdev_notify_brightness_hw_changed(&kbd_led, > > - kbd_led_level_get(&kbd_led)); > > + mutex_lock(&kbd_led_mutex); > > + new_kbd_led_level = kbd_led_level_get(&kbd_led); > > + if (kbd_led_level != new_kbd_led_level) { > > + kbd_led_level = new_kbd_led_level; > > + changed = true; > > + } > > + mutex_unlock(&kbd_led_mutex); > > + > > + if (changed) > > + led_classdev_notify_brightness_hw_changed(&kbd_led, > > + kbd_led_level); > > break; > > } > > > > -- > > 2.14.3 > > > > >
Hi, On 05-01-18 12:07, Pali Rohár wrote: > On Friday 05 January 2018 12:40:04 Andy Shevchenko wrote: >> On Thu, Jan 4, 2018 at 10:52 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> On some Dell XPS models WMI events of type 0x0000 reporting a keycode of >>> 0xe00c get reported when the brightness of the LCD panel changes. >>> >>> This leads to us reporting false-positive kbd_led change events to >>> userspace which in turn leads to the kbd backlight OSD showing when it >>> should not. >>> >>> We already read the current keyboard backlight brightness value when >>> reporting events because the led_classdev_notify_brightness_hw_changed >>> API requires this. Compare this value to the last known value and filter >>> out duplicate events, fixing this. >>> >>> Note the fixed issue is esp. a problem on XPS models with an ambient light >>> sensor and automatic brightness adjustments turned on, this causes the kbd >>> backlight OSD to show all the time there. >>> >> >> Should it have Fixes tag? >> >> Pali, any comment on this? > > See comments below. If there are not any problems, add my: > Acked-by: Pali Rohár <pali.rohar@gmail.com> > >>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514969 >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/platform/x86/dell-laptop.c | 24 ++++++++++++++++++++++-- >>> 1 file changed, 22 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c >>> index cd4725e7e0b5..2ef3297a9efc 100644 >>> --- a/drivers/platform/x86/dell-laptop.c >>> +++ b/drivers/platform/x86/dell-laptop.c >>> @@ -1133,6 +1133,7 @@ static u8 kbd_previous_mode_bit; >>> >>> static bool kbd_led_present; >>> static DEFINE_MUTEX(kbd_led_mutex); >>> +static enum led_brightness kbd_led_level; > > Introduction of cached state of brightness level in kernel driver can > lead to problem as userspace is able to change brightness (via direct > DELL SMM API) without notice of kernel. > > But now patches for new interaction with DELL SMM via WMI are in kernel > and there is explicit filter which blocks request for keyboard > backlight. So hopefully this would not be a problem anymore... (or > correct me if there still can be). Right, as discussed previously now that we've kernel support for this userspace really should not be directly touching this. >>> /* >>> * NOTE: there are three ways to set the keyboard backlight level. >>> @@ -1947,6 +1948,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_value = value; >>> struct kbd_state state; >>> struct kbd_state new_state; >>> u16 num; >>> @@ -1976,6 +1978,9 @@ static int kbd_led_level_set(struct led_classdev *led_cdev, >>> } >>> >>> out: >>> + if (ret == 0) >>> + kbd_led_level = new_value; >>> + >>> mutex_unlock(&kbd_led_mutex); >>> return ret; >>> } >>> @@ -2003,6 +2008,9 @@ static int __init kbd_led_init(struct device *dev) >>> if (kbd_led.max_brightness) >>> kbd_led.max_brightness--; >>> } >>> + >>> + kbd_led_level = kbd_led_level_get(NULL); > > Technically, kbd_led_level_get() is API function for struct led_cdev and > therefore should take valid kbd_led, NULL there looks suspicious. Right, but I did not want to pass in a non-registered led_cdev and since kbd_led_level_get() does not dereference the passed in led_cdev passing NULL seems the cleanest solution. > But in > reality this function itself ignores it first parameter. Probably the > clean solution would be to have one private function for struct led_cdev > usage and one which get current level for all other cases. This looks > ugly too, so if somebody else would not come with better idea, let this > code as is. Ack, lets keep this as in, as said it is not perfect but passing NULL is the cleanest solution. Regards, Hans > >>> + >>> ret = led_classdev_register(dev, &kbd_led); >>> if (ret) >>> kbd_led_present = false; >>> @@ -2027,13 +2035,25 @@ static void kbd_led_exit(void) >>> static int dell_laptop_notifier_call(struct notifier_block *nb, >>> unsigned long action, void *data) >>> { >>> + bool changed = false; >>> + enum led_brightness new_kbd_led_level; >>> + >>> switch (action) { >>> case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED: >>> if (!kbd_led_present) >>> break; >>> >>> - led_classdev_notify_brightness_hw_changed(&kbd_led, >>> - kbd_led_level_get(&kbd_led)); >>> + mutex_lock(&kbd_led_mutex); >>> + new_kbd_led_level = kbd_led_level_get(&kbd_led); >>> + if (kbd_led_level != new_kbd_led_level) { >>> + kbd_led_level = new_kbd_led_level; >>> + changed = true; >>> + } >>> + mutex_unlock(&kbd_led_mutex); >>> + >>> + if (changed) >>> + led_classdev_notify_brightness_hw_changed(&kbd_led, >>> + kbd_led_level); >>> break; >>> } >>> >>> -- >>> 2.14.3 >>> >> >> >> >
> -----Original Message----- > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > owner@vger.kernel.org] On Behalf Of Pali Rohár > Sent: Friday, January 5, 2018 5:07 AM > To: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Hans de Goede <hdegoede@redhat.com>; Darren Hart > <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; Platform > Driver <platform-driver-x86@vger.kernel.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org> > Subject: Re: [PATCH] platform/x86: dell-laptop: Filter out spurious keyboard > backlight change events > > On Friday 05 January 2018 12:40:04 Andy Shevchenko wrote: > > On Thu, Jan 4, 2018 at 10:52 AM, Hans de Goede <hdegoede@redhat.com> > wrote: > > > On some Dell XPS models WMI events of type 0x0000 reporting a keycode of > > > 0xe00c get reported when the brightness of the LCD panel changes. > > > > > > This leads to us reporting false-positive kbd_led change events to > > > userspace which in turn leads to the kbd backlight OSD showing when it > > > should not. > > > > > > We already read the current keyboard backlight brightness value when > > > reporting events because the led_classdev_notify_brightness_hw_changed > > > API requires this. Compare this value to the last known value and filter > > > out duplicate events, fixing this. > > > > > > Note the fixed issue is esp. a problem on XPS models with an ambient light > > > sensor and automatic brightness adjustments turned on, this causes the kbd > > > backlight OSD to show all the time there. > > > > > > > Should it have Fixes tag? > > > > Pali, any comment on this? > > See comments below. If there are not any problems, add my: > Acked-by: Pali Rohár <pali.rohar@gmail.com> > > > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514969 > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > drivers/platform/x86/dell-laptop.c | 24 ++++++++++++++++++++++-- > > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell- > laptop.c > > > index cd4725e7e0b5..2ef3297a9efc 100644 > > > --- a/drivers/platform/x86/dell-laptop.c > > > +++ b/drivers/platform/x86/dell-laptop.c > > > @@ -1133,6 +1133,7 @@ static u8 kbd_previous_mode_bit; > > > > > > static bool kbd_led_present; > > > static DEFINE_MUTEX(kbd_led_mutex); > > > +static enum led_brightness kbd_led_level; > > Introduction of cached state of brightness level in kernel driver can > lead to problem as userspace is able to change brightness (via direct > DELL SMM API) without notice of kernel. > > But now patches for new interaction with DELL SMM via WMI are in kernel > and there is explicit filter which blocks request for keyboard > backlight. So hopefully this would not be a problem anymore... (or > correct me if there still can be). > It's still technically possible to do it directly from dcdbas. Once new userspace libsmbios release is out with the WMI support it will be prioritized however over direct dcdbas access. If you want to entirely block all requests from userspace via filter it would have to be done directly at dcdbas level. I don't think this is a common enough problem to warrant that however. > > > /* > > > * NOTE: there are three ways to set the keyboard backlight level. > > > @@ -1947,6 +1948,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_value = value; > > > struct kbd_state state; > > > struct kbd_state new_state; > > > u16 num; > > > @@ -1976,6 +1978,9 @@ static int kbd_led_level_set(struct led_classdev > *led_cdev, > > > } > > > > > > out: > > > + if (ret == 0) > > > + kbd_led_level = new_value; > > > + > > > mutex_unlock(&kbd_led_mutex); > > > return ret; > > > } > > > @@ -2003,6 +2008,9 @@ static int __init kbd_led_init(struct device *dev) > > > if (kbd_led.max_brightness) > > > kbd_led.max_brightness--; > > > } > > > + > > > + kbd_led_level = kbd_led_level_get(NULL); > > Technically, kbd_led_level_get() is API function for struct led_cdev and > therefore should take valid kbd_led, NULL there looks suspicious. But in > reality this function itself ignores it first parameter. Probably the > clean solution would be to have one private function for struct led_cdev > usage and one which get current level for all other cases. This looks > ugly too, so if somebody else would not come with better idea, let this > code as is. > > > > + > > > ret = led_classdev_register(dev, &kbd_led); > > > if (ret) > > > kbd_led_present = false; > > > @@ -2027,13 +2035,25 @@ static void kbd_led_exit(void) > > > static int dell_laptop_notifier_call(struct notifier_block *nb, > > > unsigned long action, void *data) > > > { > > > + bool changed = false; > > > + enum led_brightness new_kbd_led_level; > > > + > > > switch (action) { > > > case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED: > > > if (!kbd_led_present) > > > break; > > > > > > - led_classdev_notify_brightness_hw_changed(&kbd_led, > > > - kbd_led_level_get(&kbd_led)); > > > + mutex_lock(&kbd_led_mutex); > > > + new_kbd_led_level = kbd_led_level_get(&kbd_led); > > > + if (kbd_led_level != new_kbd_led_level) { > > > + kbd_led_level = new_kbd_led_level; > > > + changed = true; > > > + } > > > + mutex_unlock(&kbd_led_mutex); > > > + > > > + if (changed) > > > + led_classdev_notify_brightness_hw_changed(&kbd_led, > > > + kbd_led_level); > > > break; > > > } > > > > > > -- > > > 2.14.3 > > > > > > > > > > > -- > Pali Rohár > pali.rohar@gmail.com
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c index cd4725e7e0b5..2ef3297a9efc 100644 --- a/drivers/platform/x86/dell-laptop.c +++ b/drivers/platform/x86/dell-laptop.c @@ -1133,6 +1133,7 @@ static u8 kbd_previous_mode_bit; static bool kbd_led_present; static DEFINE_MUTEX(kbd_led_mutex); +static enum led_brightness kbd_led_level; /* * NOTE: there are three ways to set the keyboard backlight level. @@ -1947,6 +1948,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_value = value; struct kbd_state state; struct kbd_state new_state; u16 num; @@ -1976,6 +1978,9 @@ static int kbd_led_level_set(struct led_classdev *led_cdev, } out: + if (ret == 0) + kbd_led_level = new_value; + mutex_unlock(&kbd_led_mutex); return ret; } @@ -2003,6 +2008,9 @@ static int __init kbd_led_init(struct device *dev) if (kbd_led.max_brightness) kbd_led.max_brightness--; } + + kbd_led_level = kbd_led_level_get(NULL); + ret = led_classdev_register(dev, &kbd_led); if (ret) kbd_led_present = false; @@ -2027,13 +2035,25 @@ static void kbd_led_exit(void) static int dell_laptop_notifier_call(struct notifier_block *nb, unsigned long action, void *data) { + bool changed = false; + enum led_brightness new_kbd_led_level; + switch (action) { case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED: if (!kbd_led_present) break; - led_classdev_notify_brightness_hw_changed(&kbd_led, - kbd_led_level_get(&kbd_led)); + mutex_lock(&kbd_led_mutex); + new_kbd_led_level = kbd_led_level_get(&kbd_led); + if (kbd_led_level != new_kbd_led_level) { + kbd_led_level = new_kbd_led_level; + changed = true; + } + mutex_unlock(&kbd_led_mutex); + + if (changed) + led_classdev_notify_brightness_hw_changed(&kbd_led, + kbd_led_level); break; }
On some Dell XPS models WMI events of type 0x0000 reporting a keycode of 0xe00c get reported when the brightness of the LCD panel changes. This leads to us reporting false-positive kbd_led change events to userspace which in turn leads to the kbd backlight OSD showing when it should not. We already read the current keyboard backlight brightness value when reporting events because the led_classdev_notify_brightness_hw_changed API requires this. Compare this value to the last known value and filter out duplicate events, fixing this. Note the fixed issue is esp. a problem on XPS models with an ambient light sensor and automatic brightness adjustments turned on, this causes the kbd backlight OSD to show all the time there. BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514969 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/dell-laptop.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)