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
