[PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change

[PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change

[Chris Chiu]

Make asus-wmi notify on hotkey kbd brightness changes, listen for
brightness events and update the brightness directly in the driver.
For this purpose, bound check on brightness in kbd_led_set must be
based on the same data type to prevent illegal value been set.

Update the brightness by led_classdev_notify_brightness_hw_changed.
This will allow userspace to monitor (poll) for brightness changes
on the LED without reporting via input keymapping.

Signed-off-by: Chris Chiu <chiu@endlessm.com>
---
 drivers/platform/x86/asus-wmi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 1f6e68f0b646..a18484f4bae3 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work)
 		ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
 
 	asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
+	led_classdev_notify_brightness_hw_changed(&asus->kbd_led, asus->kbd_led_wk);
 }
 
 static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
@@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
 
 	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
 
-	if (value > asus->kbd_led.max_brightness)
+	if ((int)value > (int)asus->kbd_led.max_brightness)
 		value = asus->kbd_led.max_brightness;
-	else if (value < 0)
+	else if ((int)value < 0)
 		value = 0;
 
 	asus->kbd_led_wk = value;
@@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 
 		asus->kbd_led_wk = led_val;
 		asus->kbd_led.name = "asus::kbd_backlight";
+		asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
 		asus->kbd_led.brightness_set = kbd_led_set;
 		asus->kbd_led.brightness_get = kbd_led_get;
 		asus->kbd_led.max_brightness = 3;
@@ -1745,6 +1747,15 @@ static void asus_wmi_notify(u32 value, void *context)
 		}
 	}
 
+	if (code == NOTIFY_KBD_BRTUP) {
+		kbd_led_set(&asus->kbd_led, asus->kbd_led_wk + 1);
+		goto exit;
+	}
+	if (code == NOTIFY_KBD_BRTDWN) {
+		kbd_led_set(&asus->kbd_led, asus->kbd_led_wk - 1);
+		goto exit;
+	}
+
 	if (is_display_toggle(code) &&
 	    asus->driver->quirks->no_display_toggle)
 		goto exit;
-- 
2.11.0

[PATCH v2 2/2] platform/x86: asus-wmi: Add keyboard backlight toggle support

[Chris Chiu]

Some ASUS laptops like UX550GE has hotkey (Fn+F7) for keyboard
backlight toggle which would emit the scan code 0xc7 each keypress.
On the UX550GE, the max keyboard brightness level is 3 so the
toggle would not be simply on/off the led but need to be cyclic.
Per ASUS spec, it should increment the brightness for each keypress,
then toggle(off) the LED when it already reached the max level.

Signed-off-by: Chris Chiu <chiu@endlessm.com>
---
 drivers/platform/x86/asus-wmi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index a18484f4bae3..bfcafca54a7a 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -67,6 +67,7 @@ MODULE_LICENSE("GPL");
 #define NOTIFY_BRNDOWN_MAX		0x2e
 #define NOTIFY_KBD_BRTUP		0xc4
 #define NOTIFY_KBD_BRTDWN		0xc5
+#define NOTIFY_KBD_BRTTOGGLE		0xc7
 
 /* WMI Methods */
 #define ASUS_WMI_METHODID_SPEC	        0x43455053 /* BIOS SPECification */
@@ -1755,6 +1756,13 @@ static void asus_wmi_notify(u32 value, void *context)
 		kbd_led_set(&asus->kbd_led, asus->kbd_led_wk - 1);
 		goto exit;
 	}
+	if (code == NOTIFY_KBD_BRTTOGGLE) {
+		if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
+			kbd_led_set(&asus->kbd_led, 0);
+		else
+			kbd_led_set(&asus->kbd_led, asus->kbd_led_wk + 1);
+		goto exit;
+	}
 
 	if (is_display_toggle(code) &&
 	    asus->driver->quirks->no_display_toggle)
-- 
2.11.0

Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change

[Chris Chiu]

On Mon, Jun 11, 2018 at 3:18 PM, Chris Chiu <chiu@endlessm.com> wrote:
> Make asus-wmi notify on hotkey kbd brightness changes, listen for
> brightness events and update the brightness directly in the driver.
> For this purpose, bound check on brightness in kbd_led_set must be
> based on the same data type to prevent illegal value been set.
>
> Update the brightness by led_classdev_notify_brightness_hw_changed.
> This will allow userspace to monitor (poll) for brightness changes
> on the LED without reporting via input keymapping.
>
> Signed-off-by: Chris Chiu <chiu@endlessm.com>
> ---
>  drivers/platform/x86/asus-wmi.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 1f6e68f0b646..a18484f4bae3 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work)
>                 ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>
>         asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> +       led_classdev_notify_brightness_hw_changed(&asus->kbd_led, asus->kbd_led_wk);
>  }
>
>  static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>
>         asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
> -       if (value > asus->kbd_led.max_brightness)
> +       if ((int)value > (int)asus->kbd_led.max_brightness)
>                 value = asus->kbd_led.max_brightness;
> -       else if (value < 0)
> +       else if ((int)value < 0)
>                 value = 0;
>
>         asus->kbd_led_wk = value;
> @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>
>                 asus->kbd_led_wk = led_val;
>                 asus->kbd_led.name = "asus::kbd_backlight";
> +               asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
>                 asus->kbd_led.brightness_set = kbd_led_set;
>                 asus->kbd_led.brightness_get = kbd_led_get;
>                 asus->kbd_led.max_brightness = 3;
> @@ -1745,6 +1747,15 @@ static void asus_wmi_notify(u32 value, void *context)
>                 }
>         }
>
> +       if (code == NOTIFY_KBD_BRTUP) {
> +               kbd_led_set(&asus->kbd_led, asus->kbd_led_wk + 1);
> +               goto exit;
> +       }
> +       if (code == NOTIFY_KBD_BRTDWN) {
> +               kbd_led_set(&asus->kbd_led, asus->kbd_led_wk - 1);
> +               goto exit;
> +       }
> +
>         if (is_display_toggle(code) &&
>             asus->driver->quirks->no_display_toggle)
>                 goto exit;
> --
> 2.11.0
>

Gentle ping. Any suggestions? The v2 is based on the last comment from
Hans, https://lkml.org/lkml/2018/6/5/270. Please let me know if nay. Thanks

Chris

Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change

[Andy Shevchenko]

On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu <chiu@endlessm.com> wrote:
> Make asus-wmi notify on hotkey kbd brightness changes, listen for
> brightness events and update the brightness directly in the driver.

> For this purpose, bound check on brightness in kbd_led_set must be
> based on the same data type to prevent illegal value been set.

> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>
>         asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
> -       if (value > asus->kbd_led.max_brightness)
> +       if ((int)value > (int)asus->kbd_led.max_brightness)
>                 value = asus->kbd_led.max_brightness;
> -       else if (value < 0)
> +       else if ((int)value < 0)
>                 value = 0;

I didn't quite understand this part of the problem.
Does it exist in the current code? Can it be split to a separate change?

Can we avoid those ugly castings?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change

[Chris Chiu]

On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu <chiu@endlessm.com> wrote:
>> Make asus-wmi notify on hotkey kbd brightness changes, listen for
>> brightness events and update the brightness directly in the driver.
>
>> For this purpose, bound check on brightness in kbd_led_set must be
>> based on the same data type to prevent illegal value been set.
>
>> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>>
>>         asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>>
>> -       if (value > asus->kbd_led.max_brightness)
>> +       if ((int)value > (int)asus->kbd_led.max_brightness)
>>                 value = asus->kbd_led.max_brightness;
>> -       else if (value < 0)
>> +       else if ((int)value < 0)
>>                 value = 0;
>
> I didn't quite understand this part of the problem.
> Does it exist in the current code? Can it be split to a separate change?
>
> Can we avoid those ugly castings?
>

I'd like to remove the ugly castings but there's a concern I may need some
advices. I don't know whether if the bound check logic ever verified before.
Maybe the value passed via sysfs is already correctly bounded?

When the kbd_led_wk comes to -1, `if (value > asus->kbd_led.max_brightness)`
returns true and `if (value < 0)` return false which confused me. It
should relate
to the 2nd argument type is enum led_brightness which I consider it as integer.
The unexpected return value cause the KBD_BRTDWN cyclic, 3->2->0->-1
(3 again in kbd_led_set)->2->1. After applying the casting on both operands
`if ((int)value > (int)asus->kbd_led.max_brightness)`, the other
unexpected false
returned by `if (value < 0)` makes each KBD_BRTDOWN to be 3 -> 2 -> 1 -> 0 ->
-1 -> -2 -> -3 ->..... That's what the ugly casting for. I used to
tend to do boundary
limit before calling kbd_led_set as follows

kbd_led_set(&asus->kbd_led, MIN(asus->kbd_led_wk + 1,
asus->kbd_led.max_brightness);
and
kbd_led_set(&asus->kbd_led, MAX(asus->kbd_led_wk - 1, 0));

If so, the boundary limit logic would be totally redundant but I think
it may be still
useful to check the value passed from sysfs? Any suggestion which one would
be better?

Chris

> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change

[Chris Chiu]

On Thu, Jun 14, 2018 at 2:58 PM, Chris Chiu <chiu@endlessm.com> wrote:
> On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu <chiu@endlessm.com> wrote:
>>> Make asus-wmi notify on hotkey kbd brightness changes, listen for
>>> brightness events and update the brightness directly in the driver.
>>
>>> For this purpose, bound check on brightness in kbd_led_set must be
>>> based on the same data type to prevent illegal value been set.
>>
>>> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>>>
>>>         asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>>>
>>> -       if (value > asus->kbd_led.max_brightness)
>>> +       if ((int)value > (int)asus->kbd_led.max_brightness)
>>>                 value = asus->kbd_led.max_brightness;
>>> -       else if (value < 0)
>>> +       else if ((int)value < 0)
>>>                 value = 0;
>>
>> I didn't quite understand this part of the problem.
>> Does it exist in the current code? Can it be split to a separate change?
>>
>> Can we avoid those ugly castings?
>>
>
> I'd like to remove the ugly castings but there's a concern I may need some
> advices. I don't know whether if the bound check logic ever verified before.
> Maybe the value passed via sysfs is already correctly bounded?
>
> When the kbd_led_wk comes to -1, `if (value > asus->kbd_led.max_brightness)`
> returns true and `if (value < 0)` return false which confused me. It
> should relate
> to the 2nd argument type is enum led_brightness which I consider it as integer.
> The unexpected return value cause the KBD_BRTDWN cyclic, 3->2->0->-1
> (3 again in kbd_led_set)->2->1. After applying the casting on both operands
> `if ((int)value > (int)asus->kbd_led.max_brightness)`, the other
> unexpected false
> returned by `if (value < 0)` makes each KBD_BRTDOWN to be 3 -> 2 -> 1 -> 0 ->
> -1 -> -2 -> -3 ->..... That's what the ugly casting for. I used to
> tend to do boundary
> limit before calling kbd_led_set as follows
>
> kbd_led_set(&asus->kbd_led, MIN(asus->kbd_led_wk + 1,
> asus->kbd_led.max_brightness);
> and
> kbd_led_set(&asus->kbd_led, MAX(asus->kbd_led_wk - 1, 0));
>
> If so, the boundary limit logic would be totally redundant but I think
> it may be still
> useful to check the value passed from sysfs? Any suggestion which one would
> be better?
>
> Chris
>
>> --
>> With Best Regards,
>> Andy Shevchenko

Gentle ping.

Cheers.

Chris

Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change

[Daniel Drake]

Hi,

On Thu, Jun 14, 2018 at 1:58 AM, Chris Chiu <chiu@endlessm.com> wrote:
>
> On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu <chiu@endlessm.com> wrote:
> >> Make asus-wmi notify on hotkey kbd brightness changes, listen for
> >> brightness events and update the brightness directly in the driver.
> >
> >> For this purpose, bound check on brightness in kbd_led_set must be
> >> based on the same data type to prevent illegal value been set.
> >
> >> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
> >>
> >>         asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> >>
> >> -       if (value > asus->kbd_led.max_brightness)
> >> +       if ((int)value > (int)asus->kbd_led.max_brightness)
> >>                 value = asus->kbd_led.max_brightness;
> >> -       else if (value < 0)
> >> +       else if ((int)value < 0)
> >>                 value = 0;
> >
> > I didn't quite understand this part of the problem.
> > Does it exist in the current code? Can it be split to a separate change?
> >
> > Can we avoid those ugly castings?
> >
>
> I'd like to remove the ugly castings but there's a concern I may need some
> advices. I don't know whether if the bound check logic ever verified before.
> Maybe the value passed via sysfs is already correctly bounded?

The casts come from the underlying need to limit the minumum and
maximum brightness within available bounds, plus difficulties doing
that when you are dealing with an enum data type.

kbd_led_set is a function pointer (from led_classdev.brightness_set)
which has this definition:

void (*brightness_set)(struct led_classdev *led_cdev, enum
led_brightness brightness);

It seems that the compiler has the choice of whether to use a signed
or unsigned type for enums. According to your tests, and a quick test
app below, it seems like gcc is using unsigned.

  #include <stdio.h>
  enum led_brightness { LED_OFF = 0 };
  int main(void) {
    enum led_brightness tmp = -1;
    if (tmp < 0)
      printf("less than zero\n");
    else
      printf("gt zero\n");
  }

This test app prints "gt zero"

led-class.c:brightness_store() uses kstrtoul() so there is no chance
of passing a negative value through this codepath, as you suspected.
But we do need to do something with negative bounds for the code that
you are adding here.

I suggest doing it like this:

static void __kbd_led_set(struct led_classdev *led_cdev, int value)
{
    struct asus_wmi *asus;

    asus = container_of(led_cdev, struct asus_wmi, kbd_led);

    if (value > asus->kbd_led.max_brightness)
        value = asus->kbd_led.max_brightness;
    else if (value < 0)
        value = 0;

    asus->kbd_led_wk = value;
    queue_work(asus->led_workqueue, &asus->kbd_led_work);
}

static void kbd_led_set(struct led_classdev *led_cdev,
            enum led_brightness value)
{
    return __kbd_led_set(led_cdev, value);
}

Now kbd_led_set can continue being a correctly typed function pointer
for led_classdev.brightness_set. And from the code you are adding here
you can call __kbd_led_set directly with signed integer values, and
rely on correct bounds correction without ugly casts.

Andy, what do you think?

Thanks
Daniel

Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change

[Andy Shevchenko]

On Tue, Jun 19, 2018 at 7:46 PM, Daniel Drake <drake@endlessm.com> wrote:

>> > Can we avoid those ugly castings?


> Now kbd_led_set can continue being a correctly typed function pointer
> for led_classdev.brightness_set. And from the code you are adding here
> you can call __kbd_led_set directly with signed integer values, and
> rely on correct bounds correction without ugly casts.
>
> Andy, what do you think?

Thanks, Daniel for investigation.

I'm fine with solution(s) that has no ugly casting and no regression
on the hardware we support.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change

[Chris Chiu]

On Wed, Jun 20, 2018 at 12:46 AM, Daniel Drake <drake@endlessm.com> wrote:
> Hi,
>
> On Thu, Jun 14, 2018 at 1:58 AM, Chris Chiu <chiu@endlessm.com> wrote:
>>
>> On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>> > On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu <chiu@endlessm.com> wrote:
>> >> Make asus-wmi notify on hotkey kbd brightness changes, listen for
>> >> brightness events and update the brightness directly in the driver.
>> >
>> >> For this purpose, bound check on brightness in kbd_led_set must be
>> >> based on the same data type to prevent illegal value been set.
>> >
>> >> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>> >>
>> >>         asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>> >>
>> >> -       if (value > asus->kbd_led.max_brightness)
>> >> +       if ((int)value > (int)asus->kbd_led.max_brightness)
>> >>                 value = asus->kbd_led.max_brightness;
>> >> -       else if (value < 0)
>> >> +       else if ((int)value < 0)
>> >>                 value = 0;
>> >
>> > I didn't quite understand this part of the problem.
>> > Does it exist in the current code? Can it be split to a separate change?
>> >
>> > Can we avoid those ugly castings?
>> >
>>
>> I'd like to remove the ugly castings but there's a concern I may need some
>> advices. I don't know whether if the bound check logic ever verified before.
>> Maybe the value passed via sysfs is already correctly bounded?
>
> The casts come from the underlying need to limit the minumum and
> maximum brightness within available bounds, plus difficulties doing
> that when you are dealing with an enum data type.
>
> kbd_led_set is a function pointer (from led_classdev.brightness_set)
> which has this definition:
>
> void (*brightness_set)(struct led_classdev *led_cdev, enum
> led_brightness brightness);
>
> It seems that the compiler has the choice of whether to use a signed
> or unsigned type for enums. According to your tests, and a quick test
> app below, it seems like gcc is using unsigned.
>
>   #include <stdio.h>
>   enum led_brightness { LED_OFF = 0 };
>   int main(void) {
>     enum led_brightness tmp = -1;
>     if (tmp < 0)
>       printf("less than zero\n");
>     else
>       printf("gt zero\n");
>   }
>
> This test app prints "gt zero"
>
> led-class.c:brightness_store() uses kstrtoul() so there is no chance
> of passing a negative value through this codepath, as you suspected.
> But we do need to do something with negative bounds for the code that
> you are adding here.
>
> I suggest doing it like this:
>
> static void __kbd_led_set(struct led_classdev *led_cdev, int value)
> {
>     struct asus_wmi *asus;
>
>     asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
>     if (value > asus->kbd_led.max_brightness)
>         value = asus->kbd_led.max_brightness;

The `value > asus->kbd_led.max_brightness` will still return false here.
So I would modify as follows in v3.
    int max_level = asus->kbd_led.max_brightness;

    if (value > max_level)
        value = max_level;

I've verified there's no regression on led_classdev call path via sysfs.

>     else if (value < 0)
>         value = 0;
>
>     asus->kbd_led_wk = value;
>     queue_work(asus->led_workqueue, &asus->kbd_led_work);
> }
>
> static void kbd_led_set(struct led_classdev *led_cdev,
>             enum led_brightness value)
> {
>     return __kbd_led_set(led_cdev, value);
> }
>
> Now kbd_led_set can continue being a correctly typed function pointer
> for led_classdev.brightness_set. And from the code you are adding here
> you can call __kbd_led_set directly with signed integer values, and
> rely on correct bounds correction without ugly casts.
>
> Andy, what do you think?
>
> Thanks
> Daniel