| Summary: | KbdBacklight.GetBrightness always returns the cached value for keyboards with hardwired controls | ||
|---|---|---|---|
| Product: | upower | Reporter: | Marco Trevisan (Treviño) <mail> |
| Component: | general | Assignee: | Richard Hughes <richard> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | CC: | darose, katonag, mail, martin.pitt |
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
UpKbdBacklight: always try to fetch the current brightness value on get
UpKbdBacklight: don't cache the brightness level, always read it from sysnode UpKbdBacklight: don't cache the brightness level, always read it from sysfs attrib |
||
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 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! 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. (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 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 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. 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. Perfect, thank you! I applied this with a minor whitespace fix: .git/rebase-apply/patch:61: space before tab in indent. 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.
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.