On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu <chiu@xxxxxxxxxxxx> 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
