Re: [PATCH] Input: atkbd: Fix so copilot key generates F23 keycode

[Mark Pearson]

On Thu, Dec 19, 2024, at 1:31 PM, Dmitry Torokhov wrote:
> On Thu, Dec 19, 2024 at 07:26:19PM +0100, Hans de Goede wrote:
>> Hi,
>> 
>> On 19-Dec-24 7:15 PM, Dmitry Torokhov wrote:
>> > Hi,
>> > 
>> > On Thu, Dec 19, 2024 at 05:01:09PM +0100, Hans de Goede wrote:
>> >> Hi,
>> >>
>> >> Really +Cc Peter Hutterer this time.
>> >>
>> >> On 19-Dec-24 4:48 PM, Mark Pearson wrote:
>> >>> Hi Hans
>> >>>
>> >>> On Thu, Dec 19, 2024, at 10:28 AM, Hans de Goede wrote:
>> >>>> +Cc Peter Hutterer
>> >>>
>> >>> My bad - I've been discussing this with Peter and should have added him. Thanks for including (sorry Peter!)
>> >>
>> >> Except I forgot to actually add Peter...
>> >>
>> >>>> Hi Mark,
>> >>>>
>> >>>> Thank you for your patch.
>> >>>>
>> >>>> On 19-Dec-24 4:18 PM, Mark Pearson wrote:
>> >>>>> The copilot key on Lenovo laptops doesn't work as scancode 0x6e, which it
>> >>>>> generates is not mapped.
>> >>>>> This change lets scancode 0x6e generate keycode 193 (F23 key) which is
>> >>>>> the expected value for copilot.
>> >>>>>
>> >>>>> Tested on T14s G6 AMD.
>> >>>>> I've had reports from other users that their ThinkBooks are using the same
>> >>>>> scancode.
>> >>>>
>> >>>> Hmm, I'm not sure mapping this to KEY_F23 is the right thing to do,
>> >>>> there are 2 issues with this approach:
>> >>>>
>> >>>> 1. /usr/share/X11/xkb/symbols/inet currently maps this to
>> >>>>    XF86TouchpadOff as F20 - F23 where repurposed to
>> >>>>    TouchPad on/off/toggle / micmute to work around X11
>> >>>>    not allowing key-codes > 247.
>> >>>>
>> >>>>    We are actually working on removing this X11 workaround
>> >>>>    to make F20-F23 available as normal key-codes again
>> >>>>    for keyboards which actually have such keys.
>> >>>>
>> >>>> 2. There are some keyboards which have an actual F23 key
>> >>>>    and mapping the co-pilot key to that and then having
>> >>>>    desktop environments grow default keybindings on top
>> >>>>    of that will basically mean clobbering the F23 key or
>> >>>>    at least making it harder to use.
>> >>>>
>> >>>> I think was is necessary instead is to add a new
>> >>>> KEY_COPILOT to include/uapi/linux/input-event-codes.h
>> >>>> and use that instead.
>> > 
>> > We have discussed this with Peter and came to the conclusion that
>> > KEY_ASSISTANT should cover this use case.
>> > 
>> > Also, this tweak should go into udev rules (/lib/udev/hwdb.d/60-keyboard.hwdb)
>> > instead of adding a vendor-specific tweak to the main atkbd table.
>> > 
>> > For the future releases you may want to add "linux,keymap" device
>> > property to your ACPI/DSDT to control the scancode->keycode mapping when
>> > Linux is running.

I can look into this, but gut feeling is it's a bad solution for the Linux ecosystem as it will limit it to only platforms in the Lenovo Linux program. Be nicer to have a more widespread solution.

>> > 
>> >>>
>> >>> Sorry, should have been clearer in the commit message.
>> >>> I'm doing this just on the Microsoft spec. The co-pilot key is left-shift, Windows/Meta key, F23. Weird combo I know....
>> >>>
>> >>> Somewhere I had a MS page...but this Tom's HW page mentions it:
>> >>> https://www.tomshardware.com/software/windows/windows-copilot-key-is-secretly-from-the-ibm-era-but-you-can-remap-it-with-the-right-tools
>> >>>
>> >>> I'll see if I can find something more formal.
>> >>>
>> >>>>
>> >>>> Peter, I thought I read somewhere that you were looking
>> >>>> into mapping the copilot key to a new  KEY_COPILOT evdev
>> >>>> key for some other keyboards?
>> >>>>
>> >>>
>> >>> Wouldn't this require the kernel catching all three key events and doing the interpretation? I have no idea how this would be done or if it makes sense.
>> >>
>> >> So I guess I got caught off guard by your commit message
>> >> which suggests that only scancode 0x6e is generated.
>> >>
>> >> If indeed a left-shift + Windows/Meta key + 0x6e combination
>> >> is send them this is a different story, since indeed we
>> >> cannot filter on that in the kernel. Although sometimes
>> >> I wonder if we should because we are seeing similar things
>> >> where left-shift + Windows/Meta key + xxxx is send for
>> >> e.g. touchpad on/off toggle.
>> >>
>> >> To workaround this atm GNOME listens for XF86TouchpadToggle
>> >> as well as shift + meta + XF86TouchpadToggle, theoretically it
>> >> would be nice if we can recognize these special key-combos at
>> >> a lower level. But thinking about this that is nasty, because
>> >> then we would get an event sequence like this:
>> >>
>> >> Report shift pressed
>> >> Report meta pressed
>> > 
>> > No, you have to delay to see if it is real press or part of sequence.
>> > 
>> >> <oops special key, release them>
>> >> Report meta released
>> >> Report shift released
>> >> Report KEY_TOUCHPAD_TOGGLE
>> >> <and what do we do with the modifiers now?
>> >>  for correctness I guess we report them
>> >>  as pressed again until the hw reports them released>
>> >> Report shift pressed
>> >> Report meta pressed
>> >> <hw releases the fake modifiers>
>> >> Report meta released
>> >> Report shift released
>> >>
>> >> So yeah handling this in the kernel is not going to be pretty.
>> > 
>> > Yes, we have a form of this in drivers/tty/sysrq.c and it indeed is not
>> > pretty.
>> > 
>> >>
>> >> So I think your right and just mapping this to F23 is probably
>> >> best, but I would like to hear what Peter thinks first.
>> > 
>> > So vendor yet again encoded a shortcut sequence into firmware,
>> > beautiful. I guess you can try to install a 8042 filter
>> > (via i8042_add_filter()) in drivers/platform/x86/lenovo-<something>.c
>> > to monitor for this specific scancode sequence and replace it with
>> > something else (through an auxiliary input device). 
>> 
>> If we want to filter out these in essence fake modifier
>> events then this needs to be done at some core level,
>> because AFAIK the shift + meta + F23 key-combo is what
>> microsoft is telling OEMs to use, so we are going to see this on
>> laptops from all vendors including whitelabel laptops.
>
> Hm, then I'd rather leave it to the userspace shortcut handling to deal
> with. It's probably gonna disappear the same way as others in a couple
> of years ;) and be replaced with some thing else.
>
> And mapping to F23 as I said should be done through udev. I doubt they
> will get all OEMs settle on the same scancode.
>

I'll see if we can find a way to check on other vendor platforms what scancode is used.
If it is a common scancode, across multiple vendors, would the patch be acceptable?

If it isn't then I agree it's not suitable. I assumed it would be common and hadn't really considered that it wouldn't be - my bad.

Mark