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

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

On Monday 08 June 2020 20:43:45 Y Paritcher wrote:
> On 6/8/20 7:55 PM, Pali Rohár wrote:
> > 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 I said before:
> The DMI contains a table of firmware scancode to linux keycode mappings.
> this is parsed at boot and used together with the bios_to_linux_keycode
> entries & dell_wmi_keymap_type_ tables to create a keymap.
> 
> If a DMI entry does not have a corresponding entry in bios_to_linux_keycode
> we log a message to allow adding the correct linux keycode if known.
> This is regardless of if the key actually exists on the device.
> 
> To date, I have not been able to generate this keycode on my computer.

Ok, so you have just these bios scan codes in your DMI table, but you do
not know what they means nor if you can trigger them somehow.

You should include this information into commit message.

It is possible that your particular laptop model does not have
physically keys which can trigger these codes... (Just guessing)

> >>> 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.
> > 
> 
> Will 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,
> > 
> > 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.
> > 
> 
> Probably so, but i have no way of knowing.

This was question for Mario as he is probably the only person who can
bring some light to this area.

> I just don't think there is a point spamming a users log with info that
> they can't do anything with. If this is turned into a debug print then
> i don't care to leave this as is, i had thought this might be helpful
> just to know that this keycode mapping appears in the wild.
> 
> > 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.
> > 
> 
> Sorry about that.
> >>
> >> -- 
> >> ~Randy
> >>