RE: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode

Mario.Limonciello
2020-06-08T15:46:49Z

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Pali Rohár
> Sent: Monday, June 8, 2020 4:00 AM
> To: Y Paritcher
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> Matthew Garrett
> Subject: Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to
> bios_to_linux_keycode
>
>
> [EXTERNAL EMAIL]
>
> Hello!
>
> On Monday 08 June 2020 00:22:26 Y Paritcher wrote:
> > Increase length of bios_to_linux_keycode to 2 bytes to allow for a new
> > keycode 0xffff, this silences the following messages being logged at
> > startup on a Dell Inspiron 5593

Which event type? Can you show the whole WMI buffer that came through?

> >
> > dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
> > dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff
> >
> > Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> > ---
> > drivers/platform/x86/dell-wmi.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
> wmi.c
> > index f37e7e9093c2..5ef716e3034f 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -196,7 +196,7 @@ struct dell_dmi_results {
> > };
> >
> > /* Uninitialized entries here are KEY_RESERVED == 0. */
> > -static const u16 bios_to_linux_keycode[256] = {
> > +static const u16 bios_to_linux_keycode[65536] = {
>
> This change dramatically increase memory usage. I guess other that
> maintainers would not like such change.
>
> > [0] = KEY_MEDIA,
> > [1] = KEY_NEXTSONG,
> > [2] = KEY_PLAYPAUSE,
> > @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
> > [37] = KEY_UNKNOWN,
> > [38] = KEY_MICMUTE,
> > [255] = KEY_PROG3,
> > + [65535] = KEY_UNKNOWN,
>
> Also it seems that this change is not complete. It looks like that you
> map two different scancodes (0x48 and 0x50) to same keycodes, moreover
> both are unknown.
>
> Could you please describe which key presses (or events) generate
> delivering these WMI scancode events?
>
> Note that purpose of printing unknown/unrecognized keys messages is to
> inform that current pressed key was not processed or that it was
> ignored.
>
> For me it looks like this just just hide information that key was not
> processed correctly as this change does not implement correct processing
> of this key.
>
> Also, could you share documentation about these 0x48/0x50 events? Or it
> is under NDA?
>
> > };
> >
> > /*
> > --
> > 2.27.0
> >

I would actually question if there is value to lines in dell-wmi.c like this:

pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);

and

pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n", type, code);

In both of those cases the information doesn't actually help the user, by default it's
ignored by the driver anyway. It just notifies the user it's something the driver doesn't
comprehend. I would think these are better suited to downgrade to debug. And if
a key combination isn't doing something expected the user can use dyndbg to turn it
back on and can be debugged what should be populated or "explicitly" ignored.