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
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 <[email protected]>
---
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] = {
[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,
};
/*
--
2.27.0
Hi Paritcher,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/auto-latest]
[also build test WARNING on platform-drivers-x86/for-next linus/master linux/master v5.7 next-20200605]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Y-Paritcher/platform-x86-dell-wmi-new-keys/20200608-122408
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 22a1c800c96c83b7f4e3e02fad767502b70124fa
config: i386-randconfig-s002-20200608 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-247-gcadbd124-dirty
# save the attached .config to linux build tree
make W=1 C=1 ARCH=i386 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
drivers/platform/x86/dell-wmi.c: In function 'handle_dmi_entry':
>> drivers/platform/x86/dell-wmi.c:506:38: warning: comparison is always true due to limited range of data type [-Wtype-limits]
506 | u16 keycode = (bios_entry->keycode <
| ^
vim +506 drivers/platform/x86/dell-wmi.c
a464afb9581f6a Andy Lutomirski 2016-02-15 464
bff589be59c509 Andy Lutomirski 2015-11-25 465 static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)
5ea2559726b786 Rezwanul Kabir 2009-11-02 466 {
18b6f80f509503 Andy Lutomirski 2016-02-15 467 struct dell_dmi_results *results = opaque;
18b6f80f509503 Andy Lutomirski 2016-02-15 468 struct dell_bios_hotkey_table *table;
a464afb9581f6a Andy Lutomirski 2016-02-15 469 int hotkey_num, i, pos = 0;
890a7c8e8dc2d0 Dmitry Torokhov 2010-08-04 470 struct key_entry *keymap;
18b6f80f509503 Andy Lutomirski 2016-02-15 471
18b6f80f509503 Andy Lutomirski 2016-02-15 472 if (results->err || results->keymap)
18b6f80f509503 Andy Lutomirski 2016-02-15 473 return; /* We already found the hotkey table. */
18b6f80f509503 Andy Lutomirski 2016-02-15 474
074df51ca84d32 Andy Lutomirski 2016-02-17 475 /* The Dell hotkey table is type 0xB2. Scan until we find it. */
b13de7019c1b67 Andy Lutomirski 2016-02-15 476 if (dm->type != 0xb2)
18b6f80f509503 Andy Lutomirski 2016-02-15 477 return;
18b6f80f509503 Andy Lutomirski 2016-02-15 478
18b6f80f509503 Andy Lutomirski 2016-02-15 479 table = container_of(dm, struct dell_bios_hotkey_table, header);
18b6f80f509503 Andy Lutomirski 2016-02-15 480
b13de7019c1b67 Andy Lutomirski 2016-02-15 481 hotkey_num = (table->header.length -
b13de7019c1b67 Andy Lutomirski 2016-02-15 482 sizeof(struct dell_bios_hotkey_table)) /
18b6f80f509503 Andy Lutomirski 2016-02-15 483 sizeof(struct dell_bios_keymap_entry);
b13de7019c1b67 Andy Lutomirski 2016-02-15 484 if (hotkey_num < 1) {
b13de7019c1b67 Andy Lutomirski 2016-02-15 485 /*
b13de7019c1b67 Andy Lutomirski 2016-02-15 486 * Historically, dell-wmi would ignore a DMI entry of
b13de7019c1b67 Andy Lutomirski 2016-02-15 487 * fewer than 7 bytes. Sizes between 4 and 8 bytes are
b13de7019c1b67 Andy Lutomirski 2016-02-15 488 * nonsensical (both the header and all entries are 4
b13de7019c1b67 Andy Lutomirski 2016-02-15 489 * bytes), so we approximate the old behavior by
b13de7019c1b67 Andy Lutomirski 2016-02-15 490 * ignoring tables with fewer than one entry.
b13de7019c1b67 Andy Lutomirski 2016-02-15 491 */
b13de7019c1b67 Andy Lutomirski 2016-02-15 492 return;
b13de7019c1b67 Andy Lutomirski 2016-02-15 493 }
5ea2559726b786 Rezwanul Kabir 2009-11-02 494
e075b3c898e405 Pali Roh?r 2016-06-15 495 keymap = kcalloc(hotkey_num, sizeof(struct key_entry), GFP_KERNEL);
18b6f80f509503 Andy Lutomirski 2016-02-15 496 if (!keymap) {
18b6f80f509503 Andy Lutomirski 2016-02-15 497 results->err = -ENOMEM;
18b6f80f509503 Andy Lutomirski 2016-02-15 498 return;
18b6f80f509503 Andy Lutomirski 2016-02-15 499 }
5ea2559726b786 Rezwanul Kabir 2009-11-02 500
5ea2559726b786 Rezwanul Kabir 2009-11-02 501 for (i = 0; i < hotkey_num; i++) {
890a7c8e8dc2d0 Dmitry Torokhov 2010-08-04 502 const struct dell_bios_keymap_entry *bios_entry =
18b6f80f509503 Andy Lutomirski 2016-02-15 503 &table->keymap[i];
cbc61f114af5fb Andy Lutomirski 2015-11-30 504
cbc61f114af5fb Andy Lutomirski 2015-11-30 505 /* Uninitialized entries are 0 aka KEY_RESERVED. */
cbc61f114af5fb Andy Lutomirski 2015-11-30 @506 u16 keycode = (bios_entry->keycode <
cbc61f114af5fb Andy Lutomirski 2015-11-30 507 ARRAY_SIZE(bios_to_linux_keycode)) ?
890a7c8e8dc2d0 Dmitry Torokhov 2010-08-04 508 bios_to_linux_keycode[bios_entry->keycode] :
890a7c8e8dc2d0 Dmitry Torokhov 2010-08-04 509 KEY_RESERVED;
8cb8e63b569895 Gabriele Mazzotta 2014-12-04 510
cbc61f114af5fb Andy Lutomirski 2015-11-30 511 /*
cbc61f114af5fb Andy Lutomirski 2015-11-30 512 * Log if we find an entry in the DMI table that we don't
cbc61f114af5fb Andy Lutomirski 2015-11-30 513 * understand. If this happens, we should figure out what
cbc61f114af5fb Andy Lutomirski 2015-11-30 514 * the entry means and add it to bios_to_linux_keycode.
cbc61f114af5fb Andy Lutomirski 2015-11-30 515 */
cbc61f114af5fb Andy Lutomirski 2015-11-30 516 if (keycode == KEY_RESERVED) {
cbc61f114af5fb Andy Lutomirski 2015-11-30 517 pr_info("firmware scancode 0x%x maps to unrecognized keycode 0x%x\n",
cbc61f114af5fb Andy Lutomirski 2015-11-30 518 bios_entry->scancode, bios_entry->keycode);
cbc61f114af5fb Andy Lutomirski 2015-11-30 519 continue;
cbc61f114af5fb Andy Lutomirski 2015-11-30 520 }
cbc61f114af5fb Andy Lutomirski 2015-11-30 521
8cb8e63b569895 Gabriele Mazzotta 2014-12-04 522 if (keycode == KEY_KBDILLUMTOGGLE)
a464afb9581f6a Andy Lutomirski 2016-02-15 523 keymap[pos].type = KE_IGNORE;
8cb8e63b569895 Gabriele Mazzotta 2014-12-04 524 else
a464afb9581f6a Andy Lutomirski 2016-02-15 525 keymap[pos].type = KE_KEY;
a464afb9581f6a Andy Lutomirski 2016-02-15 526 keymap[pos].code = bios_entry->scancode;
a464afb9581f6a Andy Lutomirski 2016-02-15 527 keymap[pos].keycode = keycode;
a464afb9581f6a Andy Lutomirski 2016-02-15 528
a464afb9581f6a Andy Lutomirski 2016-02-15 529 pos++;
a464afb9581f6a Andy Lutomirski 2016-02-15 530 }
a464afb9581f6a Andy Lutomirski 2016-02-15 531
18b6f80f509503 Andy Lutomirski 2016-02-15 532 results->keymap = keymap;
e075b3c898e405 Pali Roh?r 2016-06-15 533 results->keymap_size = pos;
5ea2559726b786 Rezwanul Kabir 2009-11-02 534 }
5ea2559726b786 Rezwanul Kabir 2009-11-02 535
:::::: The code at line 506 was first introduced by commit
:::::: cbc61f114af5fb078d84dc8864152f4db1712bc5 dell-wmi: Improve unknown hotkey handling
:::::: TO: Andy Lutomirski <[email protected]>
:::::: CC: Darren Hart <[email protected]>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Paritcher,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/auto-latest]
[also build test WARNING on platform-drivers-x86/for-next linus/master v5.7 next-20200605]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Y-Paritcher/platform-x86-dell-wmi-new-keys/20200608-122408
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 22a1c800c96c83b7f4e3e02fad767502b70124fa
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> drivers/platform/x86/dell-wmi.c:506:38: warning: result of comparison of constant 65536 with expression of type 'const u16' (aka 'const unsigned short') is always true [-Wtautological-constant-out-of-range-compare]
u16 keycode = (bios_entry->keycode <
~~~~~~~~~~~~~~~~~~~ ^
1 warning generated.
vim +506 drivers/platform/x86/dell-wmi.c
a464afb9581f6a Andy Lutomirski 2016-02-15 464
bff589be59c509 Andy Lutomirski 2015-11-25 465 static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)
5ea2559726b786 Rezwanul Kabir 2009-11-02 466 {
18b6f80f509503 Andy Lutomirski 2016-02-15 467 struct dell_dmi_results *results = opaque;
18b6f80f509503 Andy Lutomirski 2016-02-15 468 struct dell_bios_hotkey_table *table;
a464afb9581f6a Andy Lutomirski 2016-02-15 469 int hotkey_num, i, pos = 0;
890a7c8e8dc2d0 Dmitry Torokhov 2010-08-04 470 struct key_entry *keymap;
18b6f80f509503 Andy Lutomirski 2016-02-15 471
18b6f80f509503 Andy Lutomirski 2016-02-15 472 if (results->err || results->keymap)
18b6f80f509503 Andy Lutomirski 2016-02-15 473 return; /* We already found the hotkey table. */
18b6f80f509503 Andy Lutomirski 2016-02-15 474
074df51ca84d32 Andy Lutomirski 2016-02-17 475 /* The Dell hotkey table is type 0xB2. Scan until we find it. */
b13de7019c1b67 Andy Lutomirski 2016-02-15 476 if (dm->type != 0xb2)
18b6f80f509503 Andy Lutomirski 2016-02-15 477 return;
18b6f80f509503 Andy Lutomirski 2016-02-15 478
18b6f80f509503 Andy Lutomirski 2016-02-15 479 table = container_of(dm, struct dell_bios_hotkey_table, header);
18b6f80f509503 Andy Lutomirski 2016-02-15 480
b13de7019c1b67 Andy Lutomirski 2016-02-15 481 hotkey_num = (table->header.length -
b13de7019c1b67 Andy Lutomirski 2016-02-15 482 sizeof(struct dell_bios_hotkey_table)) /
18b6f80f509503 Andy Lutomirski 2016-02-15 483 sizeof(struct dell_bios_keymap_entry);
b13de7019c1b67 Andy Lutomirski 2016-02-15 484 if (hotkey_num < 1) {
b13de7019c1b67 Andy Lutomirski 2016-02-15 485 /*
b13de7019c1b67 Andy Lutomirski 2016-02-15 486 * Historically, dell-wmi would ignore a DMI entry of
b13de7019c1b67 Andy Lutomirski 2016-02-15 487 * fewer than 7 bytes. Sizes between 4 and 8 bytes are
b13de7019c1b67 Andy Lutomirski 2016-02-15 488 * nonsensical (both the header and all entries are 4
b13de7019c1b67 Andy Lutomirski 2016-02-15 489 * bytes), so we approximate the old behavior by
b13de7019c1b67 Andy Lutomirski 2016-02-15 490 * ignoring tables with fewer than one entry.
b13de7019c1b67 Andy Lutomirski 2016-02-15 491 */
b13de7019c1b67 Andy Lutomirski 2016-02-15 492 return;
b13de7019c1b67 Andy Lutomirski 2016-02-15 493 }
5ea2559726b786 Rezwanul Kabir 2009-11-02 494
e075b3c898e405 Pali Roh?r 2016-06-15 495 keymap = kcalloc(hotkey_num, sizeof(struct key_entry), GFP_KERNEL);
18b6f80f509503 Andy Lutomirski 2016-02-15 496 if (!keymap) {
18b6f80f509503 Andy Lutomirski 2016-02-15 497 results->err = -ENOMEM;
18b6f80f509503 Andy Lutomirski 2016-02-15 498 return;
18b6f80f509503 Andy Lutomirski 2016-02-15 499 }
5ea2559726b786 Rezwanul Kabir 2009-11-02 500
5ea2559726b786 Rezwanul Kabir 2009-11-02 501 for (i = 0; i < hotkey_num; i++) {
890a7c8e8dc2d0 Dmitry Torokhov 2010-08-04 502 const struct dell_bios_keymap_entry *bios_entry =
18b6f80f509503 Andy Lutomirski 2016-02-15 503 &table->keymap[i];
cbc61f114af5fb Andy Lutomirski 2015-11-30 504
cbc61f114af5fb Andy Lutomirski 2015-11-30 505 /* Uninitialized entries are 0 aka KEY_RESERVED. */
cbc61f114af5fb Andy Lutomirski 2015-11-30 @506 u16 keycode = (bios_entry->keycode <
cbc61f114af5fb Andy Lutomirski 2015-11-30 507 ARRAY_SIZE(bios_to_linux_keycode)) ?
890a7c8e8dc2d0 Dmitry Torokhov 2010-08-04 508 bios_to_linux_keycode[bios_entry->keycode] :
890a7c8e8dc2d0 Dmitry Torokhov 2010-08-04 509 KEY_RESERVED;
8cb8e63b569895 Gabriele Mazzotta 2014-12-04 510
cbc61f114af5fb Andy Lutomirski 2015-11-30 511 /*
cbc61f114af5fb Andy Lutomirski 2015-11-30 512 * Log if we find an entry in the DMI table that we don't
cbc61f114af5fb Andy Lutomirski 2015-11-30 513 * understand. If this happens, we should figure out what
cbc61f114af5fb Andy Lutomirski 2015-11-30 514 * the entry means and add it to bios_to_linux_keycode.
cbc61f114af5fb Andy Lutomirski 2015-11-30 515 */
cbc61f114af5fb Andy Lutomirski 2015-11-30 516 if (keycode == KEY_RESERVED) {
cbc61f114af5fb Andy Lutomirski 2015-11-30 517 pr_info("firmware scancode 0x%x maps to unrecognized keycode 0x%x\n",
cbc61f114af5fb Andy Lutomirski 2015-11-30 518 bios_entry->scancode, bios_entry->keycode);
cbc61f114af5fb Andy Lutomirski 2015-11-30 519 continue;
cbc61f114af5fb Andy Lutomirski 2015-11-30 520 }
cbc61f114af5fb Andy Lutomirski 2015-11-30 521
8cb8e63b569895 Gabriele Mazzotta 2014-12-04 522 if (keycode == KEY_KBDILLUMTOGGLE)
a464afb9581f6a Andy Lutomirski 2016-02-15 523 keymap[pos].type = KE_IGNORE;
8cb8e63b569895 Gabriele Mazzotta 2014-12-04 524 else
a464afb9581f6a Andy Lutomirski 2016-02-15 525 keymap[pos].type = KE_KEY;
a464afb9581f6a Andy Lutomirski 2016-02-15 526 keymap[pos].code = bios_entry->scancode;
a464afb9581f6a Andy Lutomirski 2016-02-15 527 keymap[pos].keycode = keycode;
a464afb9581f6a Andy Lutomirski 2016-02-15 528
a464afb9581f6a Andy Lutomirski 2016-02-15 529 pos++;
a464afb9581f6a Andy Lutomirski 2016-02-15 530 }
a464afb9581f6a Andy Lutomirski 2016-02-15 531
18b6f80f509503 Andy Lutomirski 2016-02-15 532 results->keymap = keymap;
e075b3c898e405 Pali Roh?r 2016-06-15 533 results->keymap_size = pos;
5ea2559726b786 Rezwanul Kabir 2009-11-02 534 }
5ea2559726b786 Rezwanul Kabir 2009-11-02 535
:::::: The code at line 506 was first introduced by commit
:::::: cbc61f114af5fb078d84dc8864152f4db1712bc5 dell-wmi: Improve unknown hotkey handling
:::::: TO: Andy Lutomirski <[email protected]>
:::::: CC: Darren Hart <[email protected]>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
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
>
> 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 <[email protected]>
> ---
> 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
>
> -----Original Message-----
> From: [email protected] <platform-driver-x86-
> [email protected]> On Behalf Of Pali Roh?r
> Sent: Monday, June 8, 2020 4:00 AM
> To: Y Paritcher
> Cc: [email protected]; [email protected];
> 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 <[email protected]>
> > ---
> > 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.
On 6/8/20 11:46 AM, [email protected] wrote:
>> -----Original Message-----
>> From: [email protected] <platform-driver-x86-
>> [email protected]> On Behalf Of Pali Rohár
>> Sent: Monday, June 8, 2020 4:00 AM
>> To: Y Paritcher
>> Cc: [email protected]; [email protected];
>> 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 <[email protected]>
>>> ---
>>> 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.
The dell_wmi: firmware scancode XXXX maps to unrecognized keycode XXXX
events are emitted at startup when processing DMI table entries that dell-wmi
does not recognize.
my DMI table contains among others:
48 00 FF FF
50 00 FF FF
the scan/keycodes are 2 bytes the array was only 1 byte until now as there were
never any reported keycodes with the higher byte set.
unlike my other patches this one is not for key press events. None of the keys on my laptop correspond to these scancode/keycode.
the reason for this type of log event is in a 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.
*/
therefore I tried adding this to the list of known keycodes.
As of now almost half of the keycodes in the list are KEY_UNKNOWN.
If anyone has a way of figuring out how to map this to a specific key,
i will try to identify it further.
>>
>> Also, could you share documentation about these 0x48/0x50 events? Or it
>> is under NDA?
>>
I am just a regular user. I have no clue what they are nor any inside info.
>>> };
>>>
>>> /*
>>> --
>>> 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.
>
I agree with this. The only reason i am adding these definitions is to get rid of
annoying log messages. They should really be debug.
On Monday 08 June 2020 15:46:44 [email protected] wrote:
> 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.
My motivation for these messages was to provide information to user that
kernel received event, but was not able to process it as it do not
understand it.
It could help in situation when user press special key and nothing is
delivered to userspace. But he could see that something happened in log.
Similar message is also printed by PS/2 keyboard driver atkbd.c:
case ATKBD_KEY_UNKNOWN:
dev_warn(&serio->dev,
"Unknown key %s (%s set %d, code %#x on %s).\n",
atkbd->release ? "released" : "pressed",
atkbd->translated ? "translated" : "raw",
atkbd->set, code, serio->phys);
dev_warn(&serio->dev,
"Use 'setkeycodes %s%02x <keycode>' to make it known.\n",
code & 0x80 ? "e0" : "", code & 0x7f);
input_sync(dev);
break;
> -----Original Message-----
> From: [email protected] <platform-driver-x86-
> [email protected]> On Behalf Of Pali Roh?r
> Sent: Monday, June 8, 2020 3:48 PM
> To: Limonciello, Mario
> Cc: [email protected]; [email protected]; platform-driver-
> [email protected]; [email protected]
> Subject: Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to
> bios_to_linux_keycode
>
>
> [EXTERNAL EMAIL]
>
> On Monday 08 June 2020 15:46:44 [email protected] wrote:
> > 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.
>
> My motivation for these messages was to provide information to user that
> kernel received event, but was not able to process it as it do not
> understand it.
>
> It could help in situation when user press special key and nothing is
> delivered to userspace. But he could see that something happened in log.
>
But does a user know what to do with this information? From time to time
coming to kernel mailing list, but that's it.
I think same person who would know to come to kernel mailing list for a key
not working can likely also hand turning on dyndbg to get the info.
> Similar message is also printed by PS/2 keyboard driver atkbd.c:
>
> case ATKBD_KEY_UNKNOWN:
> dev_warn(&serio->dev,
> "Unknown key %s (%s set %d, code %#x on %s).\n",
> atkbd->release ? "released" : "pressed",
> atkbd->translated ? "translated" : "raw",
> atkbd->set, code, serio->phys);
> dev_warn(&serio->dev,
> "Use 'setkeycodes %s%02x <keycode>' to make it known.\n",
> code & 0x80 ? "e0" : "", code & 0x7f);
> input_sync(dev);
> break;
I think the difference here is that user can actually do something from userland
to do with `setkeycodes` for PS2.
On Monday 08 June 2020 20:58:38 [email protected] wrote:
> > -----Original Message-----
> > From: [email protected] <platform-driver-x86-
> > [email protected]> On Behalf Of Pali Rohár
> > Sent: Monday, June 8, 2020 3:48 PM
> > To: Limonciello, Mario
> > Cc: [email protected]; [email protected]; platform-driver-
> > [email protected]; [email protected]
> > Subject: Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to
> > bios_to_linux_keycode
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Monday 08 June 2020 15:46:44 [email protected] wrote:
> > > 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.
> >
> > My motivation for these messages was to provide information to user that
> > kernel received event, but was not able to process it as it do not
> > understand it.
> >
> > It could help in situation when user press special key and nothing is
> > delivered to userspace. But he could see that something happened in log.
> >
>
> But does a user know what to do with this information? From time to time
> coming to kernel mailing list, but that's it.
That is a good question. I'm really not sure if user can do anything
with it. But also users do not care about these kind of logs. So
probably even do not know about them.
What is nice in this solution that if you want to try "debug" such
problem you just need to ask user for logs. Nothing is needed to enabled
/ disable.
> I think same person who would know to come to kernel mailing list for a key
> not working can likely also hand turning on dyndbg to get the info.
You are probably right.
In past I did one thing thanks to these logs. I searched for these log
messages on interned. More results were on forum discussions. I tried to
contact users of those posts and I think 3-4 people wrote me back with
details which allowed me to extend dell-wmi driver to handle additional
key codes.
So I see two benefits from these logs: 1) no special setup is needed to
gather these logs (useful for non-power users) and 2) ability to search
on internet if we have laptops which generates such unknown key codes
and users are "complaining" or posting their logs for investigation on
places where are no kernel developers available.
So question is if these two points are reason why to stick with these
logs or turn them off by default.
I still think it can be useful.
> > Similar message is also printed by PS/2 keyboard driver atkbd.c:
> >
> > case ATKBD_KEY_UNKNOWN:
> > dev_warn(&serio->dev,
> > "Unknown key %s (%s set %d, code %#x on %s).\n",
> > atkbd->release ? "released" : "pressed",
> > atkbd->translated ? "translated" : "raw",
> > atkbd->set, code, serio->phys);
> > dev_warn(&serio->dev,
> > "Use 'setkeycodes %s%02x <keycode>' to make it known.\n",
> > code & 0x80 ? "e0" : "", code & 0x7f);
> > input_sync(dev);
> > break;
>
> I think the difference here is that user can actually do something from userland
> to do with `setkeycodes` for PS2.
Of course, I agree.