Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode

["Pali Rohár" <pali@kernel.org>]

Hello!

On Monday 08 June 2020 16:27:10 Randy Dunlap wrote:
> Hi--
> 
> On 6/8/20 4:05 PM, Y Paritcher wrote:
> > Increase length of bios_to_linux_keycode to 2 bytes (the true size of a
> > keycode) to allow for a new keycode 0xffff, this silences the following
> > messages being logged at startup on a Dell Inspiron 5593:
> > 
> >     dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
> >     dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff

Which keys generate these two scancodes? Or how have you been able to
trigger these scancodes (in case they are not generated by key press)?

It is important to know for which key or event or feature we need to
include this patch and therefore what feature is currently
non-functional on that laptop.

> > as per this code comment:
> > 
> >    Log if we find an entry in the DMI table that we don't
> >    understand.  If this happens, we should figure out what
> >    the entry means and add it to bios_to_linux_keycode.
> > 
> > These are keycodes included in the 0xB2 DMI table, for which the
> > corosponding keys are not known.
> 
>   corresponding
> 
> > 
> > Now when a user will encounter this key, a proper message wil be printed:
> > 
> >     dell_wmi: Unknown key with type 0xXXXX and code 0xXXXX pressed
> > 
> > This will then allow the key to be identified properly.
> > 
> > Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> > ---
> >  drivers/platform/x86/dell-wmi.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index 6b510f8431a3..dae1db96b5a0 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] = {
> 
> It surely seems odd to me to expand an array from 512 bytes to 128 Kbytes
> just to handle one special case.  Can't it be handled in code as a
> special case?

I already wrote that more developers would not be happy about this
change. I would rather to see e.g. that Randy's suggestion with 0xffff
check as increasing memory usage.

> >  	[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,

Looking at the last two lines... and for me it looks like that 0x00FF
and 0xFFFF are just "placeholders" or special values for unknown /
custom / unsupported / reserved / special / ... codes.

It is really suspicious why first 38 values are defined, then there is
gap, then one value 255 and then huge gap to 65535.

Mario, this looks like some mapping table between internal Dell BIOS key
code and standard Linux key code. Are you able to get access to some
documentation which contains explanation of those Dell key numbers?
It could really help us to understand these gaps and what is correct
interpretation of these numbers.

E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
generates code 255, which could prove my thesis about "special codes"
(which are probably not found in e.g. Windows or Linux mapping tables).

> >  };
> >  
> >  /*
> > @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)
> >  					&table->keymap[i];
> >  
> >  		/* Uninitialized entries are 0 aka KEY_RESERVED. */
> > -		u16 keycode = (bios_entry->keycode <
> > -			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > -			bios_to_linux_keycode[bios_entry->keycode] :
> > -			KEY_RESERVED;
> > +		u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
> >  
> >  		/*
> >  		 * Log if we find an entry in the DMI table that we don't
> > 
> 
> Something like:
> 
> 		u16 keycode;
> 
> 		keycode = bios_entry->keycode == 0xffff ? KEY_UNKNOWN :
> 			(bios_entry->keycode <
> 			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> 			bios_to_linux_keycode[bios_entry->keycode] :
> 			KEY_RESERVED;
> 
> 
> 
> Also please fix this:
> (no To-header on input) <>

Hint: specifying git send-email with '--to' argument instead of '--cc'
should help.

> 
> -- 
> ~Randy
>