Bug 95457 - KbdBacklight.GetBrightness always returns the cached value for keyboards with hardwired controls
Summary: KbdBacklight.GetBrightness always returns the cached value for keyboards with...
Status: RESOLVED FIXED
Alias: None
Product: upower
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Richard Hughes
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-17 16:18 UTC by Marco Trevisan (Treviño)
Modified: 2016-12-07 02:57 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
UpKbdBacklight: always try to fetch the current brightness value on get (1.80 KB, patch)
2016-05-17 16:20 UTC, Marco Trevisan (Treviño)
Details | Splinter Review
UpKbdBacklight: don't cache the brightness level, always read it from sysnode (4.31 KB, patch)
2016-05-19 02:36 UTC, Marco Trevisan (Treviño)
Details | Splinter Review
UpKbdBacklight: don't cache the brightness level, always read it from sysfs attrib (4.01 KB, patch)
2016-05-20 02:20 UTC, Marco Trevisan (Treviño)
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marco Trevisan (Treviño) 2016-05-17 16:18:38 UTC
In many laptops such as Dells and ThinkPads the keyboard backlight can be changed using keyboard controls which don't emit any KEY_KBDILLUM{UP,DOWN,TOGGLE} since the bios already handles the event (well, actually most of them could be configured at kernel module level to emit a KEY_KBDILLUMTOGGLE on brightness changes, but this would be wrong since the change happens despite userland settings).

So, while it would be nice to have a KEY_KBDILLUMCHANGENOTIFY event or something similar (an udev event?) to be emitted by the kernel in such cases, this is not something easily addressable right now.

So, in this scenario, UPower doesn't get any request from userland to set the brightness to a given level, and thus when calling GetBrightness we always get the latest set value (or the initially cached one).
If the user changes the illumination using these hardwired keys, upower isn't notified in any way and GetBrightness returns the wrong value:

cat /sys/class/leds/tpacpi\:\:kbd_backlight/brightness 
2
# change backlight with the key
marco@tricky:~$ cat /sys/class/leds/tpacpi\:\:kbd_backlight/brightness 
1
marco@tricky:~$ gdbus call --system --dest org.freedesktop.UPower --object-path /org/freedesktop/UPower/KbdBacklight --method org.freedesktop.UPower.KbdBacklight.GetBrightness 
(2,)

This is indeed wrong. So I'd suggest that UPower would instead always fetch the current value from the sysnode, more than returning a cached one which might be invalid.
Comment 1 Marco Trevisan (Treviño) 2016-05-17 16:20:36 UTC
Created attachment 123834 [details] [review]
UpKbdBacklight: always try to fetch the current brightness value on get

When GetBrightness is called it's better to fetch the current value in the
sysnode since many devices doesn't support the emission of brightness
changes to userland, and this could make settings daemons to handle
this value incorrectly
Comment 2 Martin Pitt 2016-05-18 05:47:08 UTC
Comment on attachment 123834 [details] [review]
UpKbdBacklight: always try to fetch the current brightness value on get

I do agree to the general approach. Reading the brightness from sysfs is really quick, and it shouldn't be read very often.

But the actual patch looks too much like "bolted on the side" TBH, duplicates the code for reading the brightness, and leaves the "else" path meaningless as it won't ever return anything else than 0. Let's please clean this up:

 * Drop the "brightness" field from UpKbdBacklightPrivate.
 * In up_kbd_backlight_brightness_write(), drop setting priv->brightness.
 * Move the code for reading the file into a private static function and drop it from up_kbd_backlight_find(). Call it from _find() and _get_brightness().

Thanks!
Comment 3 Marco Trevisan (Treviño) 2016-05-19 02:36:30 UTC
Created attachment 123894 [details] [review]
UpKbdBacklight: don't cache the brightness level, always read it from sysnode

When GetBrightness is called it's better to fetch the current value in the
sysnode since many devices doesn't support the emission of brightness
changes to userland, and this could make settings daemons to handle
this value incorrectly.

Added static up_kbd_backlight_brightness_read that is now called
also in initialization, it returns -1 on errors, and use it everywhere we need
to read or check the current brightness.
Comment 4 Marco Trevisan (Treviño) 2016-05-19 02:39:49 UTC
(In reply to Martin Pitt from comment #2)
> Comment on attachment 123834 [details] [review] [review]
> UpKbdBacklight: always try to fetch the current brightness value on get
> 
> I do agree to the general approach. Reading the brightness from sysfs is
> really quick, and it shouldn't be read very often.

Good.

> But the actual patch looks too much like "bolted on the side" TBH,
> duplicates the code for reading the brightness, and leaves the "else" path
> meaningless as it won't ever return anything else than 0.

Thanks for the review Martin, I've updated the patch to follow your comments, I also made the dbus Get method to return an error in case of invalid reads.

Cheers
Comment 5 Martin Pitt 2016-05-19 08:52:15 UTC
Comment on attachment 123894 [details] [review]
UpKbdBacklight: don't cache the brightness level, always read it from sysnode

Review of attachment 123894 [details] [review]:
-----------------------------------------------------------------

I like the general structure of this a lot more, thanks! I have some nitpicks below.

Please change the commit message like s/sysnode/sysfs attribute/, and s/Added/Add/.

::: src/up-kbd-backlight.c
@@ +62,5 @@
> +	if (kbd_backlight->priv->fd < 0) {
> +		g_warning ("cannot read kbd_backlight as file not open");
> +		goto out;
> +	}
> +

Please use g_return_val_if_fail(kbd_backlight->priv->fd >= 0, -1), as this should be an internal assertion. This also avoids the "goto", and you can drop the -1 initialization.

@@ +67,5 @@
> +	lseek (kbd_backlight->priv->fd, 0, SEEK_SET);
> +	len = read (kbd_backlight->priv->fd, buf, G_N_ELEMENTS (buf));
> +
> +	if (len > 0) {
> +		buf[MIN(len, (int) G_N_ELEMENTS (buf) - 1)] = '\0';

If you only read n-1 elements in the read() call, then you don't need this MIN dance and can just set buf[len] = '\0'.

@@ +72,5 @@
> +		brightness = g_ascii_strtoll (buf, &end, 10);
> +
> +		if (brightness < 0 ||
> +		    brightness > kbd_backlight->priv->max_brightness ||
> +		    (brightness == 0 && end == buf)) {

A parse error would be indicated by *end != '\0', i. e. there are leftover chars in the string that aren't numeric. "end == buf" should only happen if there are no characters at all, but I think this should be a bit more robust. So how about

   [...] || end == buf || *end != '\0'

?

@@ +92,4 @@
>  	gchar *text = NULL;
>  	gint retval;
>  	gint length;
> +	gint brightness;

This seems to be unused.
Comment 6 Marco Trevisan (Treviño) 2016-05-20 02:20:17 UTC
Comment on attachment 123894 [details] [review]
UpKbdBacklight: don't cache the brightness level, always read it from sysnode

Review of attachment 123894 [details] [review]:
-----------------------------------------------------------------

Thanks for the review again, I'm attaching a new revision of the patch in a sec.

::: src/up-kbd-backlight.c
@@ +62,5 @@
> +	if (kbd_backlight->priv->fd < 0) {
> +		g_warning ("cannot read kbd_backlight as file not open");
> +		goto out;
> +	}
> +

Yeah, I do agree...  I quite hate using labels and goto, I just used that since it was somewhat consistent with the rest of the code.
But I would have done the same, although I also liked the idea of being more informative on errors (indeed it's implicit that if the fd is invalid, then there has been an open error, but still...).

Also, I've to keep the initialization to -1 around, or we could return an un-initialized value if len = 0 (i.e. if read returns 0).
Anyway I find this more visible than explicitly including it into the g_return_val_if_fail statement.

@@ +67,5 @@
> +	lseek (kbd_backlight->priv->fd, 0, SEEK_SET);
> +	len = read (kbd_backlight->priv->fd, buf, G_N_ELEMENTS (buf));
> +
> +	if (len > 0) {
> +		buf[MIN(len, (int) G_N_ELEMENTS (buf) - 1)] = '\0';

Sure.

@@ +72,5 @@
> +		brightness = g_ascii_strtoll (buf, &end, 10);
> +
> +		if (brightness < 0 ||
> +		    brightness > kbd_backlight->priv->max_brightness ||
> +		    (brightness == 0 && end == buf)) {

Sure, adding *end != '\0' allows to be even stricter... And it's fine from my POV.
However as per g_ascii_strtoll docs, "end == buf" also happens on generic parsing errors (together with "brightness == 0"), that's why I added that, and it should already cover all the cases. So are we sure we need the *end != '\0'?

I'm adding this in the new patch, anyway.

@@ +92,4 @@
>  	gchar *text = NULL;
>  	gint retval;
>  	gint length;
> +	gint brightness;

Yeah, sorry... An unnoticed leftover.
Comment 7 Marco Trevisan (Treviño) 2016-05-20 02:20:59 UTC
Created attachment 123933 [details] [review]
UpKbdBacklight: don't cache the brightness level, always read it from sysfs attrib

When GetBrightness is called it's better to fetch the current value in the
sysfs attribute since many devices doesn't support the emission of brightness
changes to userland, and this could make settings daemons to handle
this value incorrectly.

Add static up_kbd_backlight_brightness_read that is now called during
initialization too, it returns -1 on errors, and use it everywhere we need
to read or check the current brightness.
Comment 8 Martin Pitt 2016-05-24 09:21:21 UTC
Perfect, thank you!

I applied this with a minor whitespace fix:

.git/rebase-apply/patch:61: space before tab in indent.
Comment 9 Hans de Goede 2016-10-24 07:50:05 UTC
Hi,

(In reply to Marco Trevisan (Treviño) from comment #0)
> So, while it would be nice to have a KEY_KBDILLUMCHANGENOTIFY event or
> something similar (an udev event?) to be emitted by the kernel in such
> cases, this is not something easily addressable right now.

So while working on the kbd backlight control on my Dell XPS 15, not really playing well together with gnome, I hit the same problem. So I've ended up writing some kernel patches to allow poll() on the sysfs brightness attribute (there is a standard way to do poll on sysfs attributes):

http://www.spinics.net/lists/platform-driver-x86/msg09612.html

This is v2, v1 has received a somewhat favorable review (some changes were needed but no blockers)
so I expect this to go upstream, but you should probably wait with merging the 2nd patch till this is at least acked upstream. Note that without the kernel changes / on older kernels the poll() will simply never wakeup so this is safe to do on older kernels.

FYI: I've also implemented actually waking up the poll when a hardwired hotkey changes the keyboard backlight brightness for dell laptops:
http://www.spinics.net/lists/platform-driver-x86/msg09615.html
http://www.spinics.net/lists/platform-driver-x86/msg09613.html

Next on my list is adding support for this for Thinkpads.

I've also submitted a patch to make upower use poll(), see bug 98404.

As well as some gnome-settings-daemon fixes + support for monitoring for changes through upower:
https://bugzilla.gnome.org/show_bug.cgi?id=773402
https://bugzilla.gnome.org/show_bug.cgi?id=773403
https://bugzilla.gnome.org/show_bug.cgi?id=773405

Regards,

Hans


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.