Bug 100041

I don't know what percentage of computers with backlit keyboards this affects, but on my Dell E6410, with the keyboard backlight brightness turned all the way up to the maximum brightness, when I:

$ cat /sys/class/leds/dell\:\:kbd_backlight/max_brightness
10

$ cat /sys/class/leds/dell\:\:kbd_backlight/brightness
9

The source in https://cgit.freedesktop.org/upower/tree/src/up-kbd-backlight.c does not appear to take into account (afaict) the fact that, at least on my laptop, brightness levels are referred to in a zero-based counting scheme, i.e. 0-9, while max_brightness is read in from the filesystem, converted, and then assigned directly to kbd_backlight->priv->max_brightness without taking this into account:

276  path_max = g_build_filename (dir_path, "max_brightness", NULL);
277  ret = g_file_get_contents (path_max, &buf_max, NULL, &error);
...
283  kbd_backlight->priv->max_brightness = g_ascii_strtoull (buf_max, &end, 10);

The problem potentially becomes worse in up_kbd_backlight_brightnesss_write():

113  value = CLAMP (value, 0, kbd_backlight->priv->max_brightness);

whereby, for instance, a value of 10 could attempt to be assigned (in my laptop's case), which is clearly outside the range of supported / allowable values.

If my understanding of the situation is correct up to this point, we could simply subtract 1 from the value prior to assignment on line 283, but that would complicate the check on the line immediately after:

284  if (kbd_backlight->priv->max_brightness == 0 && end == buf_max) {

Which could then instead be:

284  if (kbd_backlight->priv->max_brightness <= 0 && end == buf_max) {

but I'm not sure I like that, as it depends on the g_ascii_strtoull() conventional return behavior (among other possible reasons). Maybe inserting after the if:

288  else
289    kbd_backlight->priv->max_brightness--;  // correct syntax? Don't decrement any pointers, just the value of max_brightness by 1 -or-
289    kbd_backlight->priv->max_brightness -= 1;  // whatever gets the job done

So, unless this is compensated for elsewhere, or I'm imagining things, or missing something, am I right to report this and should this be addressed?