Bug 100041 - zero-based value (mis)handling of maximum brightness (max_brightness) for keyboard backlight
Summary: zero-based value (mis)handling of maximum brightness (max_brightness) for key...
Status: RESOLVED MOVED
Alias: None
Product: upower
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Richard Hughes
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-03 03:04 UTC by Gabriel M. Elder
Modified: 2017-09-08 17:16 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Gabriel M. Elder 2017-03-03 03:04:20 UTC
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?
Comment 1 Bastien Nocera 2017-09-08 15:18:31 UTC
Hans, if you have hardware to test this...
Comment 2 Hans de Goede 2017-09-08 17:16:00 UTC
So I just tested this (again) on my XPS 15 and max_brightness there is 2 and there are 3 levels:
0 off
1 medium
2 high

Also per: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-led

"""

What:		/sys/class/leds/<led>/brightness
Date:		March 2006
KernelVersion:	2.6.17
Contact:	Richard Purdie <rpurdie@rpsys.net>
Description:
		Set the brightness of the LED. Most LEDs don't
		have hardware brightness support, so will just be turned on for
		non-zero brightness settings. The value is between 0 and
		/sys/class/leds/<led>/max_brightness.

What:		/sys/class/leds/<led>/max_brightness
Date:		March 2006
KernelVersion:	2.6.17
Contact:	Richard Purdie <rpurdie@rpsys.net>
Description:
		Maximum brightness level for this LED, default is 255 (LED_FULL).

		If the LED does not support different brightness levels, this
		should be 1.

"""

Things really are 0 based, so if on your dell max_brightness is 10, but you can only write up to 9 to the brightness attribute, that is a kernel bug and you should file a bug against the kernel for this.

Note I'm not saying there is no bug, just that the problem is not in upower and that it needs to be fixed in the right place.

Given the above, I'm closing this bug. Please file a bug at bugzilla.kernel.org and send a mail to:

[hans@shalem linux]$ scripts/get_maintainer.pl -f drivers/platform/x86/dell-laptop.c 
"Pali Rohár" <pali.rohar@gmail.com> (maintainer:DELL LAPTOP DRIVER)
Darren Hart <dvhart@infradead.org> (maintainer:X86 PLATFORM DRIVERS)
Andy Shevchenko <andy@infradead.org> (maintainer:X86 PLATFORM DRIVERS)
platform-driver-x86@vger.kernel.org (open list:DELL LAPTOP DRIVER)
linux-kernel@vger.kernel.org (open list)

Pointing to the bug toy filed.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.