RE: [PATCH] platform/x86: dell-laptop: Filter out spurious keyboard backlight change events

From: Mario.Limonciello
Date: Fri Jan 05 2018 - 09:22:46 EST



> -----Original Message-----
> From: platform-driver-x86-owner@xxxxxxxxxxxxxxx [mailto:platform-driver-x86-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Pali RohÃr
> Sent: Friday, January 5, 2018 5:07 AM
> To: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>; Darren Hart
> <dvhart@xxxxxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxxxxx>; Platform
> Driver <platform-driver-x86@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> 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@xxxxxxxxx>
>
> > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514969
> > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx