Message-ID: <20180105110724.cjxngyawhzotkkj3@pali>
Date:   Fri, 5 Jan 2018 12:07:24 +0100
From:   Pali Rohár <pali.rohar@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.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@...hat.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@...il.com>

> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514969
> > Signed-off-by: Hans de Goede <hdegoede@...hat.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
> >
> 
> 
> 

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

Powered by blists - more mailing lists