lkml.org 
[lkml]   [2018]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
    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

    \
     
     \ /
      Last update: 2018-06-20 16:41    [W:2.277 / U:0.284 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site