2020-06-08 23:08:41

by Y Paritcher

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

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

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.

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 <[email protected]>
---
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] = {
[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,
};

/*
@@ -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
--
2.27.0


2020-06-09 00:00:36

by Pali Rohár

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

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 <[email protected]>
> > ---
> > 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
>

2020-06-09 00:48:14

by Y Paritcher

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

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.

>>> 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 <[email protected]>
>>> ---
>>> 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.

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

2020-06-09 02:03:14

by Randy Dunlap

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

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
>
> 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 <[email protected]>
> ---
> 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?

> [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,
> };
>
> /*
> @@ -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) <>

--
~Randy

2020-06-09 09:12:18

by Pali Rohár

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

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 <[email protected]>
> >>> ---
> >>> 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
> >>

2020-06-09 20:39:51

by Mario Limonciello

[permalink] [raw]
Subject: RE: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode

>
> 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.
>

The codes are actually 4 bytes in the table, but in practice nothing above the
first two bytes is used.

Those two called out are special though, here are their meanings:

0x00FF is user programmable function
0xFFFF is no function

For the purpose of memory consumption I think it's reasonable to ignore the
upper 2 bytes and special case these two.

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

2020-06-10 12:40:46

by Mario Limonciello

[permalink] [raw]
Subject: RE: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode

> -----Original Message-----
> From: [email protected] <platform-driver-x86-
> [email protected]> On Behalf Of Pali Roh?r
> Sent: Wednesday, June 10, 2020 4:45 AM
> To: Limonciello, Mario
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to
> bios_to_linux_keycode
>
>
> [EXTERNAL EMAIL]
>
> On Tuesday 09 June 2020 19:49:18 [email protected] wrote:
> > >
> > > 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.
> > >
> >
> > The codes are actually 4 bytes in the table, but in practice nothing
> above the
> > first two bytes is used.
> >
> > Those two called out are special though, here are their meanings:
> >
> > 0x00FF is user programmable function
> > 0xFFFF is no function
> >
> > For the purpose of memory consumption I think it's reasonable to ignore
> the
> > upper 2 bytes and special case these two.
>
> Thank you for information!
>
> So 0x00FF is "user programmable" button. Do I understand it correctly
> that Dell/BIOS does not explicitly provide meaning for these buttons,
> they do not have fixed functionality and therefore user should configure
> them as he want?

Correct

>
> And what does mean "no function"? I do not know what should I imagine if
> I receive key press marked as "no function".

It means no action is expected to occur, should behave like a no-op. I think
discarding it makes fine sense.

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

2020-06-10 14:56:24

by Pali Rohár

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

On Tuesday 09 June 2020 19:49:18 [email protected] wrote:
> >
> > 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.
> >
>
> The codes are actually 4 bytes in the table, but in practice nothing above the
> first two bytes is used.
>
> Those two called out are special though, here are their meanings:
>
> 0x00FF is user programmable function
> 0xFFFF is no function
>
> For the purpose of memory consumption I think it's reasonable to ignore the
> upper 2 bytes and special case these two.

Thank you for information!

So 0x00FF is "user programmable" button. Do I understand it correctly
that Dell/BIOS does not explicitly provide meaning for these buttons,
they do not have fixed functionality and therefore user should configure
them as he want?

And what does mean "no function"? I do not know what should I imagine if
I receive key press marked as "no function".

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

2020-06-12 14:16:38

by Pali Rohár

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

On Wednesday 10 June 2020 12:35:09 [email protected] wrote:
> > -----Original Message-----
> > From: [email protected] <platform-driver-x86-
> > [email protected]> On Behalf Of Pali Rohár
> > Sent: Wednesday, June 10, 2020 4:45 AM
> > To: Limonciello, Mario
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to
> > bios_to_linux_keycode
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Tuesday 09 June 2020 19:49:18 [email protected] wrote:
> > > >
> > > > 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.
> > > >
> > >
> > > The codes are actually 4 bytes in the table, but in practice nothing
> > above the
> > > first two bytes is used.
> > >
> > > Those two called out are special though, here are their meanings:
> > >
> > > 0x00FF is user programmable function
> > > 0xFFFF is no function
> > >
> > > For the purpose of memory consumption I think it's reasonable to ignore
> > the
> > > upper 2 bytes and special case these two.
> >
> > Thank you for information!
> >
> > So 0x00FF is "user programmable" button. Do I understand it correctly
> > that Dell/BIOS does not explicitly provide meaning for these buttons,
> > they do not have fixed functionality and therefore user should configure
> > them as he want?
>
> Correct
>
> >
> > And what does mean "no function"? I do not know what should I imagine if
> > I receive key press marked as "no function".
>
> It means no action is expected to occur, should behave like a no-op. I think
> discarding it makes fine sense.

Thank you! This was missing bit of information.

Just I'm curious, why firmware sends "no-op" event which we could ignore? :D

I can imagine that those events / scan codes may contain some
information which we can use...

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

2020-06-12 15:02:09

by Mario Limonciello

[permalink] [raw]
Subject: RE: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode

> >
> > >
> > > And what does mean "no function"? I do not know what should I imagine
> if
> > > I receive key press marked as "no function".
> >
> > It means no action is expected to occur, should behave like a no-op. I
> think
> > discarding it makes fine sense.
>
> Thank you! This was missing bit of information.
>
> Just I'm curious, why firmware sends "no-op" event which we could ignore?
> :D
>

I don't have details on this, but I can at least hypothesize a situation that
there was a manufacturing error with the wrong keyboard applied. Like think if
the system has keyboard without a backlight and there was a keyboard with
a silkscreened key for modifying backlight. In this case firmware events
related to keyboard backlight shouldn't flow through so firmware could instead
send this type of event.

> I can imagine that those events / scan codes may contain some
> information which we can use...

I mean I suppose you can choose to ignore my comments, but I'm telling you this
event doesn't contain anything useful.

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