Den ons 4 dec. 2024 kl 23:19 skrev Armin Wolf <W_Armin@xxxxxx>: > > Am 04.12.24 um 23:00 schrieb Hans de Goede: > > > 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. > > I agree with this viewpoint. Also the user still sees the event when you filter out > the special keypress, since calling led_classdev_notify_brightness_hw_changed() sends a > poll notification to the led class device. So when using poll(), select(), etc. the user > can still react to this event. > > >> 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 > > > Same problem with any driver which registers a acpi_battery_hook, if you unload the driver you also unload > the ACPI battery driver itself :( The reason for this seems to be that modprobe assumes that the ACPI battery > driver is a pure dependency of the first driver and can be unloaded if no other module depend on it. > > I think this problem could be fixable, but i have no experience when it comes to the kernel modules infrastructure. > > For now using rmmmod instead of modprobe -r should indeed do the job. > > Thanks, > Armin Wolf > Perfect, thank you both! All of this makes perfect sense to me and I will make these suggested changes. I am planning to try and fix up some documentation which I will also add for this platform driver, plus add relevant KConfig/Makefile/etc entries, do some additional testing, and will try to get a patch submitted here hopefully sometime within the next few days. My plan was to take the latest commit at the time from the 'for-next' branch of pdx86/platform-drivers-x86.git and then submit it all together as a patch via a new thread here. Please let me know if there are any problems with that plan or anything else I might need to adjust before doing so. Thank you again! Best regards, Joshua
