On Thu, Jun 14, 2018 at 2:58 PM, Chris Chiu <chiu@xxxxxxxxxxxx> wrote: > 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 Gentle ping. Cheers. Chris
