Hi Joshua, On 4-Dec-24 9:33 PM, Joshua Grisham wrote: > Hi Hans, thank you so much for taking the time to read through the > questions and get back to me! > > Den ons 4 dec. 2024 kl 18:31 skrev Hans de Goede <hdegoede@xxxxxxxxxx>: >> >> So I think it is best to just emulate what the laptops where >> the cycling is directly done by the embedded-control do. >> >> That is: >> >> 1. Add LED_BRIGHT_HW_CHANGED to the flags of the led_classdev >> for the "xxx:kbd_backlight" led class device you expose >> >> 2. Filter out kbd-backlight-cycle keypresses and on such >> a keypress: >> >> 2.1 Determine new brightness level >> 2.2 Apply new brightness level >> 2.3 Call: >> >> led_classdev_notify_brightness_hw_changed(&kbd_backlight_led_classdev, new_brightness_level); >> > > This is actually exactly what I have already implemented with the one > exception: I am executing exactly the same kind of logic you mentioned > (via schedule_work()) but I have NOT filtered out the keypress; > instead, it is just scheduling this logic to run in a workqueue and > then going ahead and passing along the keypress as well, just in case > anyone wanted to trigger any other kind of event from this hotkey. > > I have actually submitted a patch to the keyboard hwdb which was > merged in to systemd that maps this particular key to "unknown" with > the idea that someone who has this model would also likely have this > platform driver module loaded, so by default the kernel-space action > to actually change the brightness level would be executed (the > "EC-like" behavior as you mentioned that they could not change), but > the user would also have the option of remapping the key and > triggering additional actions on top of this if they wanted. Does > that sound appropriate or is it better to just filter out the keypress > entirely once the above actions are scheduled/executed? In my experience it is best to pick one approach of 1. Deliver event to userspace and let userspace handle everything 2. Handle everything in kernel and stick with that We actually have what you are suggesting for display brightness up/down presses in the drivers/acpi/acpi_video.c driver which exposes both a /sys/class/backlight device and an evdev device delivering key-press events and which automatically increases the brightness of the /sys/class/backlight device on brightness up/down hotkey presses. And that combination is a hot mess. GNOME/KDE see the keypress and then race with the kernel increasing the brightness. Typically they loose the race reading the new brigthness increasing the brightness by 2 steps on one keypress. And some older laptops have only 8 steps, so that is a problem. I disabled the in kernel handling of the brightness up/down keypresses in the ACPI video bus driver because of this, but some users complained about this breaking old X11 setups using e.g. Window Maker of fvwm. Linus Torvalds ended up "fixing" this by instead of having the kernel immediately react giving userspace like 0.25 seconds or something to respond and if it does not, then handle it in the kernel. Which of course is racy so sometimes users still hit the 2 steps for one keypress issue if the laptop is under load. Note this is meant as an example of what NOT to do. As for the hwdb mapping of they keypress to unknown I predict that at some point a well intending user is going to notice this, map it to KEY_KBDILLUMTOGGLE and submit a PR to systemd upstream. Then the systemd upstream maintainers will trust this user, who actually has such a laptop which they don't to be doing the right thing and merge it. And then if GNOME/KDE/xxxx grow support for actually acting on KEY_KBDILLUMTOGGLE (if they do not do so already) we have the kernel hotkey and userspace hotkey handling fighting each other just like the example above. So based on this I would strongly advice you to filter out the key event completely at the kernel level. If someone ever really needs / wants that event then my suggestion would be to add a module option which *completely* disables all in kernel handling for the key in kernelspace and instead delivers the events to userspace. TL;DR: IMHO mixing in kernel handling with keypress reporting is a bad idea. Please chose one model and stick with it. > Also as an aside, I have had a few users who have mentioned that if > they have compiled and loaded i8042 as a module (which is then marked > as "used by" samsung_galaxybook due to the i8042 filter), if they > execute a modprobe -r then it also removes i8042 and their keyboard > stops working. Is this known/expected behavior and/or is there > anything that can be done in this driver itself to try and help > prevent this from happening? Otherwise I guess a "fix" for this would > be if users compile their kernel with CONFIG_SERIO_I8042=y then they > would not have this problem? IMHO, the best way to solve this issue is to tell users to do "rmmod samsung_galaxybook" instead of modprobe -r. And you can do the same in any Makefiles / scripts you may have. Regards, Hans
