platform/x86: dell-wmi: new keys
Subsystems (2)
cover platform/x86: dell-wmi: new keys
add new backlight events with a type of 0x0010 and a code of 0x57 / 0x58, add a new keymap type table 0x0012 for keycode of 0xe035 from the Fn-lock key
extend bios_to_linux_keycode to 2 bytes to allow for a new keycode 0xffff
Y Paritcher (3):
platform/x86: dell-wmi: add new backlight events platform/x86: dell-wmi: add new keymap type 0x0012 platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
drivers/platform/x86/dell-wmi.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)
Signature · 1 line
2.27.0
PATCH 1/3 platform/x86: dell-wmi: add new backlight events
Ignore events with a type of 0x0010 and a code of 0x57 / 0x58, this silences the following messages being logged on a Dell Inspiron 5593:
dell_wmi: Unknown key with type 0x0010 and code 0x0057 presseddell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/platform/x86/dell-wmi.c +4 -0
@@ -252,6 +252,10 @@ static const struct key_entry dell_wmi_keymap_type_0010[] = {/* Fn-lock switched to multimedia keys */ { KE_IGNORE, 0x1, { KEY_RESERVED } }, +/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } },
Signature · 1 line
2.27.0
Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
Quoted 17 lines · On Monday 08 June 2020 00:22:24 Y Paritcher wrote:
Ignore events with a type of
0x0010and a code of0x57/0x58, this silences the following messages being logged on a Dell Inspiron 5593:
dell_wmi: Unknown key with type0x0010and code0x0057presseddell_wmi: Unknown key with type0x0010and code0x0058pressedSigned-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/platform/x86/dell-wmi.c +0
-0
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0010[] = { · line 260
Quoted 9 lines
drivers/platform/x86/dell-wmi.c +4
-0@@ -252,6 +252,10 @@ static const struct key_entry dell_wmi_keymap_type_0010[] = {/* Fn-lock switched to multimedia keys */ { KE_IGNORE, 0x1, { KEY_RESERVED } }, +/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } },
Please, keep codes sorted.
Quoted 4 lines
Signature · 2 lines
-- 2.27.0
RE: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
Quoted 32 lines
-----Original Message-----
From: [email protected] <platform-driver-x86- [email protected]> On Behalf Of Pali Rohár
Sent: Monday, June 8, 2020 3:35 AM
To: Y Paritcher
Cc: [email protected]; [email protected]; Matthew Garrett
Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events[EXTERNAL EMAIL]
drivers/platform/x86/dell-
Quoted 13 lines · On Monday 08 June 2020 00:22:24 Y Paritcher wrote:
Ignore events with a type of
0x0010and a code of0x57/0x58, this silences the following messages being logged on a Dell Inspiron 5593:
dell_wmi: Unknown key with type0x0010and code0x0057presseddell_wmi: Unknown key with type0x0010and code0x0058pressedSigned-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.cdrivers/platform/x86/dell-wmi.c static const struct key_entry
Quoted 4 lines
index
c25a4286d766..0b4f72f923cd100644drivers/platform/x86/dell-wmi.c +0
-0@@ -252,6 +252,10 @@ static const struct key_entrydell_wmi_keymap_type_0010[] = {
Quoted 7 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0010[] = { · line 258
Quoted 7 lines
drivers/platform/x86/dell-wmi.c +4
-0/* Fn-lock switched to multimedia keys */ { KE_IGNORE, 0x1, { KEY_RESERVED } }, +/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +
For these particular events are they emitted by another interface as well in this platform?
If so they should be KE_IGNORE so you don't end up with double notifications to userspace.
Quoted 9 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0010[] = { · line 260
Quoted 2 lines
drivers/platform/x86/dell-wmi.c +0
-0/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } },Please, keep codes sorted.
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +0
-1--2.27.0
Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
Quoted 43 lines · On 6/8/20 11:30 AM, [email protected] wrote:
Quoted 32 lines
-----Original Message-----
From: [email protected] <platform-driver-x86- [email protected]> On Behalf Of Pali Rohár
Sent: Monday, June 8, 2020 3:35 AM
To: Y Paritcher
Cc: [email protected]; [email protected]; Matthew Garrett
Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events[EXTERNAL EMAIL]
drivers/platform/x86/dell-
Quoted 13 lines · On Monday 08 June 2020 00:22:24 Y Paritcher wrote:
Ignore events with a type of
0x0010and a code of0x57/0x58, this silences the following messages being logged on a Dell Inspiron 5593:
dell_wmi: Unknown key with type0x0010and code0x0057presseddell_wmi: Unknown key with type0x0010and code0x0058pressedSigned-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.cdrivers/platform/x86/dell-wmi.c static const struct key_entry
Quoted 4 lines
index
c25a4286d766..0b4f72f923cd100644drivers/platform/x86/dell-wmi.c +0
-0@@ -252,6 +252,10 @@ static const struct key_entrydell_wmi_keymap_type_0010[] = {
Quoted 7 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0010[] = { · line 258
Quoted 7 lines
drivers/platform/x86/dell-wmi.c +4
-0/* Fn-lock switched to multimedia keys */ { KE_IGNORE, 0x1, { KEY_RESERVED } }, +/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +For these particular events are they emitted by another interface as well in this platform?
Quoted 2 lines
If so they should be
KE_IGNOREso you don't end up with double notifications to userspace.
Thank you both for the review,
This is my first patch so if i am doing something wrong please let me know.
Both before and after this change they are only emitted once (verified via showkeys)
this is because dell_wmi_process_key() calls acpi_video_handles_brightness_key_presses()
for brightness events, and acpi_video_handles_brightness_key_presses() makes sure no duplicate acpi-video events are sent.
Quoted 5 lines
Quoted 5 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0010[] = { · line 260
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +0
-0/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } },Please, keep codes sorted.
Will fix
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/platform/x86/dell-wmi.c +0
-1--2.27.0
RE: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
Quoted 64 lines
-----Original Message-----
From: Y Paritcher <[email protected]>
Sent: Monday, June 8, 2020 3:12 PM
To: Limonciello, Mario; [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events[EXTERNAL EMAIL]
Quoted 8 lines · On 6/8/20 11:30 AM, [email protected] wrote:
Quoted 8 lines
-----Original Message-----
From: [email protected] <platform-driver-x86- [email protected]> On Behalf Of Pali Rohár
Sent: Monday, June 8, 2020 3:35 AM
To: Y Paritcher
Cc: [email protected]; [email protected]; Matthew Garrett
Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlightevents
Quoted 12 lines
Quoted 12 lines
[EXTERNAL EMAIL]
Quoted 7 lines · On Monday 08 June 2020 00:22:24 Y Paritcher wrote:
Ignore events with a type of
0x0010and a code of0x57/0x58, this silences the following messages being logged on a Dell Inspiron 5593:
dell_wmi: Unknown key with type0x0010and code0x0057presseddell_wmi: Unknown key with type0x0010and code0x0058pressedQuoted 6 lines
Quoted 6 lines
Review
Quoted 6 lines
Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)(quoted diff) +0
-0b/drivers/platform/x86/dell-
Quoted 9 lines
Quoted 9 lines
wmi.cdrivers/platform/x86/dell-wmi.c static const struct key_entry
Quoted 4 lines
index
c25a4286d766..0b4f72f923cd100644drivers/platform/x86/dell-wmi.c +0
-0@@ -252,6 +252,10 @@ static const struct key_entrydell_wmi_keymap_type_0010[] = {
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +0
-0/* Fn-lock switched to multimedia keys */ { KE_IGNORE, 0x1, { KEY_RESERVED } },Quoted 6 lines
Quoted 4 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0010[] = { · line 258
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +4
-0+/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +For these particular events are they emitted by another interface as well
in this
Quoted 3 lines
platform?
If so they should be
KE_IGNOREso you don't end up with doublenotifications to
Quoted 1 line
userspace.
Thank you both for the review,
This is my first patch so if i am doing something wrong please let me know.
Quoted 6 lines
Both before and after this change they are only emitted once (verified via showkeys)
this is becausedell_wmi_process_key()callsacpi_video_handles_brightness_key_presses()
for brightness events, andacpi_video_handles_brightness_key_presses()makes sure no duplicate acpi-video events are sent.
That's good to hear that it also filters it, but my opinion is that dell-wmi.c should also filter it. So just change KE_KEY to KE_IGNORE like the other events.
Quoted 12 lines
Quoted 5 lines
Quoted 5 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0010[] = { · line 260
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +0
-0/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } },Please, keep codes sorted.
Will fix
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/platform/x86/dell-wmi.c +0
-1--2.27.0
Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
Quoted 30 lines · On Monday 08 June 2020 20:14:15 [email protected] wrote:
Quoted 23 lines
Quoted 8 lines
Quoted 8 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry
Quoted 4 lines
index
c25a4286d766..0b4f72f923cd100644drivers/platform/x86/dell-wmi.c +0
-0@@ -252,6 +252,10 @@ static const struct key_entrydell_wmi_keymap_type_0010[] = {
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +0
-0/* Fn-lock switched to multimedia keys */ { KE_IGNORE, 0x1, { KEY_RESERVED } },Quoted 6 lines
Quoted 4 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0010[] = { · line 258
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +4
-0+/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +For these particular events are they emitted by another interface as well
in this
Quoted 3 lines
platform?
If so they should be
KE_IGNOREso you don't end up with doublenotifications to
Quoted 1 line
userspace.
Thank you both for the review,
This is my first patch so if i am doing something wrong please let me know.Quoted 6 lines
Both before and after this change they are only emitted once (verified via showkeys)
this is becausedell_wmi_process_key()callsacpi_video_handles_brightness_key_presses()
for brightness events, andacpi_video_handles_brightness_key_presses()makes sure no duplicate acpi-video events are sent.
Quoted 2 lines
That's good to hear that it also filters it, but my opinion is that
dell-wmi.cshould also filter it. So just changeKE_KEYtoKE_IGNORElike the other events.
IIRC for other existing KEY_BRIGHTNESS* lines, KE_KEY/KE_IGNORE decision is there just because kernel can be configured if ACPI layer would handle them or not. And based on acpi_video_handles_brightness_key_presses() function we know if ACPI layer processed these keys or not.
So in my opinion we should handle these new KEY_BRIGHTNESS* events in the same way. So dell-wmi should process these events, but only in case when ACPI layer did not processed them.
RE: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
Quoted 57 lines
-----Original Message-----
From: Pali Rohár <[email protected]>
Sent: Monday, June 8, 2020 3:36 PM
To: Limonciello, Mario
Cc: [email protected]; [email protected]; platform-driver- [email protected]; [email protected]
Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events[EXTERNAL EMAIL]
Quoted 8 lines · On Monday 08 June 2020 20:14:15 [email protected] wrote:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry
Quoted 4 lines
index
c25a4286d766..0b4f72f923cd100644drivers/platform/x86/dell-wmi.c +0
-0@@ -252,6 +252,10 @@ static const struct key_entrydell_wmi_keymap_type_0010[] = {
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +0
-0/* Fn-lock switched to multimedia keys */ { KE_IGNORE, 0x1, { KEY_RESERVED } },Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0010[] = { · line 258
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +4
-0+/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +For these particular events are they emitted by another interface as
well
Quoted 8 lines
Quoted 8 lines
in this
Quoted 3 lines
platform?
If so they should be
KE_IGNOREso you don't end up with doublenotifications to
Quoted 1 line
userspace.
Thank you both for the review,
This is my first patch so if i am doing something wrong please let meknow.
Quoted 2 lines
Quoted 2 lines
Both before and after this change they are only emitted once (verified
via
Quoted 4 lines
Quoted 4 lines
showkeys)
this is becausedell_wmi_process_key()callsacpi_video_handles_brightness_key_presses()
for brightness events, and
acpi_video_handles_brightness_key_presses()Quoted 3 lines
Quoted 1 line
makes sure no duplicate acpi-video events are sent.
That's good to hear that it also filters it, but my opinion is that dell-
wmi.cQuoted 1 line
should also filter it. So just change
KE_KEYtoKE_IGNORElike the otherevents.
IIRC for other existing KEY_BRIGHTNESS* lines, KE_KEY/KE_IGNORE decision is there just because kernel can be configured if ACPI layer would handle them or not. And based on
acpi_video_handles_brightness_key_presses()
function we know if ACPI layer processed these keys or not.
Quoted 3 lines
So in my opinion we should handle these new KEY_BRIGHTNESS* events in the same way. So dell-wmi should process these events, but only in case when ACPI layer did not processed them.
OK thanks, I wasn't aware of this decision elsewhere.
PATCH 2/3 platform/x86: dell-wmi: add new keymap type 0x0012
In reply to Y ParitcherIgnore events with a type of 0x0012 and a code of 0xe035, this silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type 0x12dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
--- drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
drivers/platform/x86/dell-wmi.c +17 -0
@@ -334,6 +334,14 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code) {struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);
@@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ +case 0x0012 :/* Sequence of events occurred */ for (i =2 ; i < len; ++i)dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[i]);
@@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {
@@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI.
Signature · 1 line
2.27.0
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Hello!
Quoted 3 lines · On Monday 08 June 2020 00:22:25 Y Paritcher wrote:
Ignore events with a type of
0x0012and a code of0xe035, this silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
Could you please explain why to ignore these events instead of sending them to userspace via input layer? I think that userspace can be interested in knowing when Fn lock key was pressed and I can imagine that some it can use it for some purposes.
Quoted 2 lines
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressed
These messages are printed to inform about fact that some events were not processed. And they should not be silenced without reason. If for some reasons it is needed to completely ignore some kind of events then this reason should be documented (e.g. in commit message) so other developers would know why that code is there. Not all Linux developers have all those Dell machines for testing so they do not know all hardware details.
Quoted 25 lines
Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)drivers/platform/x86/dell-wmi.c +8
-0@@ -334,6 +334,14 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code) {struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device *wdev, · line 436
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +1
-0@@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ +case 0x0012 :/* Sequence of events occurred */
It is really sequence of events? Because you wrote that Fn-lock key was pressed (and not generic event). Also it is really sequence? And not just one event/key-press (with possibility of some additional details in buffer)? It would be nice to put documentation for this type of events to check and review that implementation is correct.
Quoted 28 lines
drivers/platform/x86/dell-wmi.c +8
-0for (i =2 ; i < len; ++i)dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[i]);@@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {@@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI. Signature · 1 line
2.27.0
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 12 lines · Pali Rohár
Hello!
Quoted 3 lines · On Monday 08 June 2020 00:22:25 Y Paritcher wrote:
Ignore events with a type of
0x0012and a code of0xe035, this silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:Could you please explain why to ignore these events instead of sending them to userspace via input layer? I think that userspace can be interested in knowing when Fn lock key was pressed and I can imagine that some it can use it for some purposes.
I followed what was already done for the Fn lock key on other models. The Fn lock key toggle is adjusted by the keyboard controller so there is nothing userspace should do with it.
If this is wrong the behavior should be changed for all Fn lock key entries.
Quoted 11 lines
Quoted 2 lines
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedThese messages are printed to inform about fact that some events were not processed. And they should not be silenced without reason. If for some reasons it is needed to completely ignore some kind of events then this reason should be documented (e.g. in commit message) so other developers would know why that code is there. Not all Linux developers have all those Dell machines for testing so they do not know all hardware details.
I could be wrong, but i understood these messages to inform about a unknown event. once the event is identified there should be no reason for them.
Quoted 27 lines
Quoted 27 lines
Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)drivers/platform/x86/dell-wmi.c +8
-0@@ -334,6 +334,14 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code) {struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);@@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,break ;case 0x0010 :/* Sequence of keys pressed */
Quoted 9 lines
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device *wdev, · line 436
Quoted 2 lines
drivers/platform/x86/dell-wmi.c +1
-0case 0x0011 :/* Sequence of events occurred */ +case 0x0012 :/* Sequence of events occurred */ It is really sequence of events? Because you wrote that Fn-lock key was pressed (and not generic event). Also it is really sequence? And not just one event/key-press (with possibility of some additional details in buffer)? It would be nice to put documentation for this type of events to check and review that implementation is correct.
see Mario Limonciello's answer
Quoted 28 lines
Quoted 28 lines
drivers/platform/x86/dell-wmi.c +8
-0for (i =2 ; i < len; ++i)dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[i]);@@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {@@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI. Signature · 1 line
2.27.0
RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
In reply to Y ParitcherQuoted 9 lines
-----Original Message-----
From: [email protected] <platform-driver-x86- [email protected]> On Behalf Of Y Paritcher
Sent: Sunday, June 7, 2020 11:22 PM
Cc: [email protected]; [email protected]; Matthew Garrett; Pali Rohár
Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type0x0012
Quoted 8 lines
[EXTERNAL EMAIL]
Ignore events with a type of
0x0012and a code of0xe035, this silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressed
Event type 0x12 is for "System Events". This is the type of events that you typically would see come in for things "like" the wrong power adapter being plugged in on Windows or stuff about plugging a Thunderbolt dock into a port that doesn't support Thunderbolt.
A message might look something like (paraphrased)
"Your system requires a 180W power adapter to charge effectively, but you plugged in a 60W adapter."
There often is extended data with these events. As such I don't believe all information in event type 0x0012 should be treated like scan codes like those in 0x10 or 0x11.
I would guess that Fn-lock on this machine probably has extended data in the next showing whether it was turned on and off. If it does, perhaps it makes sense to send this information to userspace as an evdev switch instead.
Quoted 62 lines
Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.c
index0b4f72f923cd..f37e7e9093c2100644drivers/platform/x86/dell-wmi.c +8
-0@@ -334,6 +334,14 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { {KE_IGNORE ,KBD_LED_AUTO_100_TOKEN , {KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code){
drivers/platform/x86/dell-wmi.c +2
-0struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);@@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ +case 0x0012 :/* Sequence of events occurred */ for (i =2 ; i < len; ++i)dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[i]);@@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);drivers/platform/x86/dell-wmi.c +7
-0if (!keymap) {@@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI.
--
2.27.0
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 27 lines · On 6/8/20 11:40 AM, [email protected] wrote:
Quoted 9 lines
-----Original Message-----
From: [email protected] <platform-driver-x86- [email protected]> On Behalf Of Y Paritcher
Sent: Sunday, June 7, 2020 11:22 PM
Cc: [email protected]; [email protected]; Matthew Garrett; Pali Rohár
Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type0x0012Quoted 8 lines
[EXTERNAL EMAIL]
Ignore events with a type of
0x0012and a code of0xe035, this silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedEvent type
0x12is for "System Events". This is the type of events that you typically would see come in for things "like" the wrong power adapter being plugged in on Windows or stuff about plugging a Thunderbolt dock into a port that doesn't support Thunderbolt.A message might look something like (paraphrased)
"Your system requires a 180W power adapter to charge effectively, but you plugged in a 60W adapter."
Quoted 8 lines
There often is extended data with these events. As such I don't believe all information in event type
0x0012should be treated like scan codes like those in0x10or0x11.I would guess that Fn-lock on this machine probably has extended data in the next showing whether it was turned on and off. If it does, perhaps it makes sense to send this information to userspace as an evdev switch instead.
You are right.
I had assumed (incorrectly) the were the same.
I turned on dyndbg and got the events with the extended data.
Fn lock key switched to multimedia keysdell_wmi: Received WMI event (02 00 12 00 35 e0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 01
Fn-lock switched to function keysdell_wmi: Received WMI event (02 00 12 00 35 e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 00
Therefore i agree this should have it's own case in dell_wmi_process_key but i am not sure yet how to deal with it. any suggestion are helpful.
About sending it to userspace, I just followed what was already done, if that is not desired we should change it for all the models.
Quoted 63 lines
Quoted 62 lines
Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.c
index0b4f72f923cd..f37e7e9093c2100644drivers/platform/x86/dell-wmi.c +8
-0@@ -334,6 +334,14 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { {KE_IGNORE ,KBD_LED_AUTO_100_TOKEN , {KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code){
drivers/platform/x86/dell-wmi.c +2
-0struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);@@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ +case 0x0012 :/* Sequence of events occurred */ for (i =2 ; i < len; ++i)dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[i]);@@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);drivers/platform/x86/dell-wmi.c +7
-0if (!keymap) {@@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI.
--
2.27.0
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Hello!
Quoted 11 lines · On Monday 08 June 2020 16:12:52 Y Paritcher wrote:
You are right.
I had assumed (incorrectly) the were the same.
I turned on dyndbg and got the events with the extended data.Fn lock key switched to multimedia keys
dell_wmi: Received WMI event (02 00 12 00 35 e0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 01Fn-lock switched to function keys
dell_wmi: Received WMI event (02 00 12 00 35 e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 00
That is why I asked if buffer is really sequence of events. From your log can be seen that it is not a sequence, but rather just one event.
Quoted 2 lines
Therefore i agree this should have it's own case in
dell_wmi_process_keybut i am not sure yet how to deal with it. any suggestion are helpful.
I guess you could handle it like for event type 0x0000 which also process one event where can be additional information in buffer. See relevant code for 0x0000:
case 0x0000: /* One key pressed or event occurred */ if (len > 2) dell_wmi_process_key(wdev, 0x0000, buffer_entry[2]); /* Other entries could contain additional information */ break;
Just processing of additional information from buffer is not implemented yet, probably nobody needed it yet.
Quoted 2 lines
About sending it to userspace, I just followed what was already done, if that is not desired we should change it for all the models.
Main question if we need to send this event to userspace. Or if we should drop this event as it is duplicate which was already processed by other layer. This is something which we do not know and it needs to be investigated and documented/explained. So in future when will do some changes in that code, we would know how to handle it without breaking existing systems.
I would suggest to not describe changes in commit message, but rather describe explanation of those changes in commit message. E.g. what was decision and why you are doing it in that way.
It would help in future to know why such code is needed.
E.g. "event XYZ needs to be ignored as kernel receive its duplicate also via keyboard controller". Or e.g. "event needs to be processed by wmi driver because notebook's embedded controller sends it only via wmi".
RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
In reply to Y ParitcherQuoted 63 lines
-----Original Message-----
From: Y Paritcher <[email protected]>
Sent: Monday, June 8, 2020 3:13 PM
To: Limonciello, Mario
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type0x0012[EXTERNAL EMAIL]
Quoted 15 lines · On 6/8/20 11:40 AM, [email protected] wrote:
Quoted 15 lines
-----Original Message-----
From: [email protected] <platform-driver-x86- [email protected]> On Behalf Of Y Paritcher
Sent: Sunday, June 7, 2020 11:22 PM
Cc: [email protected]; [email protected]; Matthew Garrett; Pali Rohár
Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type0x0012[EXTERNAL EMAIL]
Ignore events with a type of
0x0012and a code of0xe035, this silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:Quoted 6 lines
Quoted 2 lines
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedEvent type
0x12is for "System Events". This is the type of events that you typically would see come in for things "like" the wrong power adapter being plugged in on Windows or stuff about plugging a Thunderbolt dockinto
Quoted 7 lines
a port that doesn't support Thunderbolt.
A message might look something like (paraphrased)
"Your system requires a 180W power adapter to charge effectively, but you plugged in a 60W adapter."There often is extended data with these events. As such I don't believe
all
Quoted 1 line
information in event type
0x0012should be treated like scan codes likethose in
Quoted 3 lines
0x10or0x11.I would guess that Fn-lock on this machine probably has extended data in
the next
Quoted 1 line
showing whether it was turned on and off. If it does, perhaps it makes
sense to
Quoted 2 lines
send this information to userspace as an evdev switch instead.
You are right.
I had assumed (incorrectly) the were the same.
I turned on dyndbg and got the events with the extended data.Fn lock key switched to multimedia keys
dell_wmi: Received WMI event (02 00 12 00 35 e0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 01
Quoted 5 lines
Fn-lock switched to function keys
dell_wmi: Received WMI event (02 00 12 00 35 e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 00
To be clear - do the function keys not send different scan codes on this laptop in the two different modes? I expected that they should be sending separate scan codes. If they are not sending different scan codes, then this actually needs to be captured in the kernel and a translation map is needed which is platform specific.
Quoted 8 lines
Therefore i agree this should have it's own case in
dell_wmi_process_keybut i am
not sure yet how to deal with it. any suggestion are helpful.About sending it to userspace, I just followed what was already done, if that is not
desired we should change it for all the models.
Right, I don't think this was a bad first attempt. I just think it's different than the 0x10/0x11 events.
I'm not saying it shouldn't apply to more models, but just that events from this 0x12 table should be treated differently.
I feel we need a different way to send these types of events to userspace than a keycode.
I for example think that the power adapter and dock events are also potentially useful but realistically userspace needs to be able to show translated messages to a user.
Hans,
Can you please comment here how you would like to see events like this should come through to userspace?
* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock mode
Quoted 64 lines
Quoted 7 lines
Review
Quoted 7 lines
Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)(quoted diff) +0
-0b/drivers/platform/x86/dell-
Quoted 56 lines
Quoted 55 lines
wmi.c
index0b4f72f923cd..f37e7e9093c2100644drivers/platform/x86/dell-wmi.c +8
-0@@ -334,6 +334,14 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { {KE_IGNORE ,KBD_LED_AUTO_100_TOKEN , {KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code){
drivers/platform/x86/dell-wmi.c +2
-0struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);@@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ +case 0x0012 :/* Sequence of events occurred */ for (i =2 ; i < len; ++i)dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[i]);@@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);drivers/platform/x86/dell-wmi.c +7
-0if (!keymap) {@@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI.
--
2.27.0
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 69 lines · On 6/8/20 4:36 PM, [email protected] wrote:
Quoted 63 lines
-----Original Message-----
From: Y Paritcher <[email protected]>
Sent: Monday, June 8, 2020 3:13 PM
To: Limonciello, Mario
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type0x0012[EXTERNAL EMAIL]
Quoted 15 lines · On 6/8/20 11:40 AM, [email protected] wrote:
Quoted 15 lines
-----Original Message-----
From: [email protected] <platform-driver-x86- [email protected]> On Behalf Of Y Paritcher
Sent: Sunday, June 7, 2020 11:22 PM
Cc: [email protected]; [email protected]; Matthew Garrett; Pali Rohár
Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type0x0012[EXTERNAL EMAIL]
Ignore events with a type of
0x0012and a code of0xe035, this silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:Quoted 6 lines
Quoted 2 lines
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedEvent type
0x12is for "System Events". This is the type of events that you typically would see come in for things "like" the wrong power adapter being plugged in on Windows or stuff about plugging a Thunderbolt dockinto
Quoted 7 lines
a port that doesn't support Thunderbolt.
A message might look something like (paraphrased)
"Your system requires a 180W power adapter to charge effectively, but you plugged in a 60W adapter."There often is extended data with these events. As such I don't believe
all
Quoted 1 line
information in event type
0x0012should be treated like scan codes likethose in
Quoted 3 lines
0x10or0x11.I would guess that Fn-lock on this machine probably has extended data in
the next
Quoted 1 line
showing whether it was turned on and off. If it does, perhaps it makes
sense to
Quoted 2 lines
send this information to userspace as an evdev switch instead.
You are right.
I had assumed (incorrectly) the were the same.
I turned on dyndbg and got the events with the extended data.Fn lock key switched to multimedia keys
dell_wmi: Received WMI event (02 00 12 00 35 e0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 01Quoted 5 lines
Fn-lock switched to function keys
dell_wmi: Received WMI event (02 00 12 00 35 e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 00
Quoted 6 lines
To be clear - do the function keys not send different scan codes on this laptop in the two different modes? I expected that they should be sending separate scan codes. If they are not sending different scan codes, then this actually needs to be captured in the kernel and a translation map is needed which is platform specific.
this is the WMI event from pressing the Fn lock key. this indicates which mode it is switching to.
this changes if the default for pressing the F[1-12] should be Function or media. the scancodes of the Fn keys are properly transmitted, this just inverts which ones are sent by default and which are sent when pressing the Fn+F[1-12]
In other words, there are 24 scancode the only difference is which 12 are default and which 12 are when pressing with the Fn key
Quoted 95 lines
Quoted 8 lines
Therefore i agree this should have it's own case in
dell_wmi_process_keybut i am
not sure yet how to deal with it. any suggestion are helpful.About sending it to userspace, I just followed what was already done, if that is not
desired we should change it for all the models.Right, I don't think this was a bad first attempt. I just think it's different than the 0x10/0x11 events.
I'm not saying it shouldn't apply to more models, but just that events from this
0x12table should be treated differently.I feel we need a different way to send these types of events to userspace than a keycode.
I for example think that the power adapter and dock events are also potentially useful but realistically userspace needs to be able to show translated messages to a user.
Hans,
Can you please comment here how you would like to see events like this should come through to userspace?
* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock modeQuoted 64 lines
Quoted 7 lines
Review
Quoted 7 lines
Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)(quoted diff) +0
-0b/drivers/platform/x86/dell-
Quoted 56 lines
Quoted 55 lines
wmi.c
index0b4f72f923cd..f37e7e9093c2100644drivers/platform/x86/dell-wmi.c +8
-0@@ -334,6 +334,14 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { {KE_IGNORE ,KBD_LED_AUTO_100_TOKEN , {KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code){
drivers/platform/x86/dell-wmi.c +2
-0struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);@@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ +case 0x0012 :/* Sequence of events occurred */ for (i =2 ; i < len; ++i)dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[i]);@@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);drivers/platform/x86/dell-wmi.c +7
-0if (!keymap) {@@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI.
--
2.27.0
RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 10 lines
this is the WMI event from pressing the Fn lock key. this indicates which mode it is switching to.
this changes if the default for pressing the F[1-12] should be Function or media.
the scancodes of the Fn keys are properly transmitted, this just inverts which
ones are sent by default and which are sent when pressing the Fn+F[1-12]
Quoted 5 lines
In other words, there are 24 scancode the only difference is which 12 are default
and which 12 are when pressing with the Fn keyQuoted 2 lines
Quoted 2 lines
Therefore i agree this should have it's own case in
To me this sounds like it makes most sense to either be an evdev switch which indicates which mode the fn key is set to when an event comes in. You can populate the starting mode by looking up from a token.
https://github.com/dell/libsmbios/blob/master/doc/token_list.csv#L987
Any other thoughts from others?
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 14 lines · On 6/8/20 6:00 PM, [email protected] wrote:
Quoted 14 lines
this is the WMI event from pressing the Fn lock key. this indicates which mode it is switching to.
this changes if the default for pressing the F[1-12] should be Function or media.
the scancodes of the Fn keys are properly transmitted, this just inverts which
ones are sent by default and which are sent when pressing the Fn+F[1-12]In other words, there are 24 scancode the only difference is which 12 are default
and which 12 are when pressing with the Fn keyQuoted 1 line
Quoted 1 line
Quoted 9 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Therefore i agree this should have it's own case in
To me this sounds like it makes most sense to either be an evdev switch which indicates which mode the fn key is set to when an event comes in. You can populate the starting mode by looking up from a token.
https://github.com/dell/libsmbios/blob/master/doc/token_list.csv#L987Any other thoughts from others?
beware that sometimes the key will do the unexpected.
If the Fn lock is turned off in the bios pressing the Fn lock key will send an event with the current mode again, as switching is disallowed.
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
In reply to [email protected]Hi,
Quoted 97 lines · On 6/8/20 10:36 PM, [email protected] wrote:
Quoted 63 lines
-----Original Message-----
From: Y Paritcher <[email protected]>
Sent: Monday, June 8, 2020 3:13 PM
To: Limonciello, Mario
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type0x0012[EXTERNAL EMAIL]
Quoted 15 lines · On 6/8/20 11:40 AM, [email protected] wrote:
Quoted 15 lines
-----Original Message-----
From: [email protected] <platform-driver-x86- [email protected]> On Behalf Of Y Paritcher
Sent: Sunday, June 7, 2020 11:22 PM
Cc: [email protected]; [email protected]; Matthew Garrett; Pali Rohár
Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type0x0012[EXTERNAL EMAIL]
Ignore events with a type of
0x0012and a code of0xe035, this silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:Quoted 6 lines
Quoted 2 lines
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedEvent type
0x12is for "System Events". This is the type of events that you typically would see come in for things "like" the wrong power adapter being plugged in on Windows or stuff about plugging a Thunderbolt dockinto
Quoted 7 lines
a port that doesn't support Thunderbolt.
A message might look something like (paraphrased)
"Your system requires a 180W power adapter to charge effectively, but you plugged in a 60W adapter."There often is extended data with these events. As such I don't believe
all
Quoted 1 line
information in event type
0x0012should be treated like scan codes likethose in
Quoted 3 lines
0x10or0x11.I would guess that Fn-lock on this machine probably has extended data in
the next
Quoted 1 line
showing whether it was turned on and off. If it does, perhaps it makes
sense to
Quoted 2 lines
send this information to userspace as an evdev switch instead.
You are right.
I had assumed (incorrectly) the were the same.
I turned on dyndbg and got the events with the extended data.Fn lock key switched to multimedia keys
dell_wmi: Received WMI event (02 00 12 00 35 e0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 01Quoted 5 lines
Fn-lock switched to function keys
dell_wmi: Received WMI event (02 00 12 00 35 e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00) the extended data is e0 00To be clear - do the function keys not send different scan codes on this laptop in the two different modes? I expected that they should be sending separate scan codes. If they are not sending different scan codes, then this actually needs to be captured in the kernel and a translation map is needed which is platform specific.
Quoted 8 lines
Therefore i agree this should have it's own case in
dell_wmi_process_keybut i am
not sure yet how to deal with it. any suggestion are helpful.About sending it to userspace, I just followed what was already done, if that is not
desired we should change it for all the models.Right, I don't think this was a bad first attempt. I just think it's different than the 0x10/0x11 events.
I'm not saying it shouldn't apply to more models, but just that events from this
0x12table should be treated differently.I feel we need a different way to send these types of events to userspace than a keycode.
I for example think that the power adapter and dock events are also potentially useful but realistically userspace needs to be able to show translated messages to a user.
Quoted 8 lines
Hans,
Can you please comment here how you would like to see events like this should come through to userspace?
* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock mode
Note just thinking out loud here.
I'm thinking we just need a mechanism to show a "user notification". This would be just a plain text string passed from the kernel to userspace. I guess we may also want some mechanism to build (on the kernel side) a small file with all possible messages for translation from US English to other languages.
So the idea would be that e.g. gnome-shell can listen for these in some way and then show a notification in its notification mechanism with the message, like how it does for when software updates are available for example.
I think we can make this as simple as using the normal printk buffer for this and prefixing the messages with "USER NOTIFY", we already have some messages in the kernel which would qualify for this, e.g. in the USB core we have:
dev_info(&udev->dev, "not running at top speed; "
"connect to a high speed hub\n");This one is about USB1 vs USB2 ports, but we have a similar one somewhere for USB2 vs USB3 ports (I think) which would also be an interesting message to actually show to the user inside the desktop environment.
So sticking with the above example, we could change that to
#define USER_NOTIFY "USER NOTIFY: "
dev_info(&udev->dev, USER_NOTIFY "not running at top speed; connect to a high speed hub\n");
And then userspace would trigger on the "USER NOTIFY: " part, keep the bit before it (which describes the device) as is, try to translate the text after it and then combine the text before it + the possibly translated text after it and show that as a notification.
The reason for (ab)using the printk ring-buffer for this is that we will still want to have these messages in dmesg too anyways, so why add a new mechanism and send the same message twice if we can just tag the messages inside the printk ring-buffer ?
Note the dev_info above would likely be replaced with some new helper which also does some magic to help with gathering a list of strings to translate.
Again just thinking out loud here. If anyone has any initial reaction to this please let me know...
Regards,
Hans
Quoted 65 lines
Quoted 65 lines
Quoted 8 lines
Review
Quoted 8 lines
Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)(quoted diff) +0
-0b/drivers/platform/x86/dell-
Quoted 56 lines
Quoted 55 lines
wmi.c
index0b4f72f923cd..f37e7e9093c2100644drivers/platform/x86/dell-wmi.c +8
-0@@ -334,6 +334,14 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { {KE_IGNORE ,KBD_LED_AUTO_100_TOKEN , {KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code){ struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);drivers/platform/x86/dell-wmi.c +2
-0@@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ +case 0x0012 :/* Sequence of events occurred */ for (i =2 ; i < len; ++i)dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[i]);@@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {drivers/platform/x86/dell-wmi.c +7
-0@@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI.
--
2.27.0
RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Loop linux-input mailing list and trim to the relevant conversation.
Quoted 8 lines
Quoted 1 line
Can you please comment here how you would like to see events like this
should come
Quoted 5 lines
through to userspace?
* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock mode
Quoted 8 lines
Note just thinking out loud here.
I'm thinking we just need a mechanism to show a "user notification". This would
be just a plain text string passed from the kernel to userspace. I guess we may also want some mechanism to build (on the kernel side) a small file with all possible messages for translation from US English to other languages.
The part that falls apart here is that some strings have dynamic data added to them. For example in the case I said wrong power adapter there will be some numbers put into the string based on what comes into the buffer. So how will you translate these?
I guess you can draw a line in the sand and say all strings that can be emitted must be "static and generic".
Quoted 36 lines
So the idea would be that e.g. gnome-shell can listen for these in some way and then show a notification in its notification mechanism with the message,
like how it does for when software updates are available for example.I think we can make this as simple as using the normal printk buffer for this
and prefixing the messages with "USER NOTIFY", we already have some messages
in the kernel which would qualify for this, e.g. in the USB core we have:dev_info(&udev->dev, "not running at top speed; " "connect to a high speed hub\n");This one is about USB1 vs USB2 ports, but we have a similar one somewhere for USB2 vs USB3 ports (I think) which would also be an interesting message to actually show to the user inside the desktop environment.
So sticking with the above example, we could change that to
#define
USER_NOTIFY"USER NOTIFY: "
dev_info(&udev->dev, USER_NOTIFY "not running at top speed; connect to a high speed hub\n");And then userspace would trigger on the "USER NOTIFY: " part, keep the bit before it (which describes the device) as is, try to translate the text after it and then combine the text before it + the possibly translated text after it and show that as a notification.
The reason for (ab)using the printk ring-buffer for this is that we will still want to have these messages in dmesg too anyways, so why add a new mechanism and send the same message twice if we can just tag the messages inside the printk ring-buffer ?
Quoted 7 lines
Note the
dev_infoabove would likely be replaced with some new helper which also does some magic to help with gathering a list of strings to translate.Again just thinking out loud here. If anyone has any initial reaction to this please let me know...
As a general comment, I think it captures very well the possibility that the kernel has more information than userspace about the circumstances of something that a user should be notified. Definitely that's the case for these WMI/ACPI events, and I would think similar circumstances can apply to other subsystem too.
Quoted 81 lines
Regards,
Hans
Quoted 18 lines
Quoted 18 lines
Quoted 8 lines
Review
Quoted 8 lines
Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)(quoted diff) +0
-0b/drivers/platform/x86/dell-
Quoted 9 lines
Quoted 9 lines
wmi.c
index0b4f72f923cd..f37e7e9093c2100644drivers/platform/x86/dell-wmi.c +0
-0@@ -334,6 +334,14 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { {KE_IGNORE ,KBD_LED_AUTO_100_TOKEN , {KEY_RESERVED } }, };Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0011[] = {
Quoted 9 lines
drivers/platform/x86/dell-wmi.c +8
-0+/* +* Keymap for WMI events of type 0x0012 +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; + static void dell_wmi_process_key(struct wmi_device *wdev, int type,int
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +0
-0code){ struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);drivers/platform/x86/dell-wmi.c +0
-0@@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device*wdev,
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
drivers/platform/x86/dell-wmi.c +2
-0break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ +case 0x0012 :/* Sequence of events occurred */ for (i =2 ; i < len; ++i)dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[i]);@@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {drivers/platform/x86/dell-wmi.c +0
-0@@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; }Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/platform/x86/dell-wmi.c static int dell_wmi_input_setup(struct wmi_device *wdev)
Quoted 9 lines
drivers/platform/x86/dell-wmi.c +7
-0+/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
* them are reported also on laptops which have scancodes in DMI.
--
2.27.0
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Hi,
Quoted 19 lines · On 6/9/20 5:36 PM, [email protected] wrote:
Loop linux-input mailing list and trim to the relevant conversation.
Quoted 8 lines
Quoted 1 line
Can you please comment here how you would like to see events like this
should come
Quoted 5 lines
through to userspace?
* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock modeQuoted 8 lines
Note just thinking out loud here.
I'm thinking we just need a mechanism to show a "user notification". This would
be just a plain text string passed from the kernel to userspace. I guess we may also want some mechanism to build (on the kernel side) a small file with all possible messages for translation from US English to other languages.
Quoted 7 lines
The part that falls apart here is that some strings have dynamic data added to them. For example in the case I said wrong power adapter there will be some numbers put into the string based on what comes into the buffer. So how will you translate these?
I guess you can draw a line in the sand and say all strings that can be emitted must be "static and generic".
Right, that is what I was thinking, although for the power adapter case I was thinking there are not so much variants so we can just do a couple of fixed strings for the combos, or maybe just sat that the adapter does not delivers enough power and that at minimum X watts is necessary" then we only have 1 variable and we can probably easily do fixed strings for the few cases of X.
Or we could get fancy and do some generic notification mechanism outside of printk/dmesg where we push a format string + parameters to the format string to userspace. So that the translation can be done on the format string rather then on the end result. I'm not sure we need to make things that complicated though.
Quoted 43 lines
Quoted 35 lines
So the idea would be that e.g. gnome-shell can listen for these in some way and then show a notification in its notification mechanism with the message,
like how it does for when software updates are available for example.I think we can make this as simple as using the normal printk buffer for this
and prefixing the messages with "USER NOTIFY", we already have some messages
in the kernel which would qualify for this, e.g. in the USB core we have:dev_info(&udev->dev, "not running at top speed; " "connect to a high speed hub\n");This one is about USB1 vs USB2 ports, but we have a similar one somewhere for USB2 vs USB3 ports (I think) which would also be an interesting message to actually show to the user inside the desktop environment.
So sticking with the above example, we could change that to
#define
USER_NOTIFY"USER NOTIFY: "
dev_info(&udev->dev, USER_NOTIFY "not running at top speed; connect to a high speed hub\n");And then userspace would trigger on the "USER NOTIFY: " part, keep the bit before it (which describes the device) as is, try to translate the text after it and then combine the text before it + the possibly translated text after it and show that as a notification.
The reason for (ab)using the printk ring-buffer for this is that we will still want to have these messages in dmesg too anyways, so why add a new mechanism and send the same message twice if we can just tag the messages inside the printk ring-buffer ?
Quoted 7 lines
Note the
dev_infoabove would likely be replaced with some new helper which also does some magic to help with gathering a list of strings to translate.Again just thinking out loud here. If anyone has any initial reaction to this please let me know...
Quoted 5 lines
As a general comment, I think it captures very well the possibility that the kernel has more information than userspace about the circumstances of something that a user should be notified. Definitely that's the case for these WMI/ACPI events, and I would think similar circumstances can apply to other subsystem too.
Right, that was my idea behind having a generic notification mechanism.
Regards,
Hans
RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 6 lines
Right, that is what I was thinking, although for the power adapter case I was thinking there are not so much variants so we can just do a couple of fixed strings for the combos, or maybe just sat that the adapter does not delivers enough power and that at minimum X watts is necessary" then we only have 1 variable and we can probably easily do fixed strings for the few cases of X.
I would rather have a generic fixed string or fixed strings with a single than an array. But the problem then is that the numbers are not discoverable from anywhere and would need to be hardcoded. So in that regard I think generic fixed strings is the only way this can work.
Quoted 66 lines
Or we could get fancy and do some generic notification mechanism outside of printk/dmesg where we push a format string + parameters to the format string to userspace. So that the translation can be done on the format string rather then on the end result. I'm not sure we need to make things that complicated though.
Quoted 1 line
Quoted 1 line
So the idea would be that e.g. gnome-shell can listen for these in some
way
Quoted 9 lines
Quoted 9 lines
and then show a notification in its notification mechanism with the message,
like how it does for when software updates are available for example.I think we can make this as simple as using the normal printk buffer for this
and prefixing the messages with "USER NOTIFY", we already have some messages
in the kernel which would qualify for this, e.g. in the USB core wehave:
Quoted 5 lines
Quoted 5 lines
dev_info(&udev->dev, "not running at top speed; " "connect to a high speed hub\n");This one is about USB1 vs USB2 ports, but we have a similar one
somewhere
Quoted 1 line
Quoted 1 line
for USB2 vs USB3 ports (I think) which would also be an interesting
message
Quoted 23 lines
Quoted 23 lines
to actually show to the user inside the desktop environment.
So sticking with the above example, we could change that to
#define
USER_NOTIFY"USER NOTIFY: "
dev_info(&udev->dev, USER_NOTIFY "not running at top speed; connect to a high speed hub\n");And then userspace would trigger on the "USER NOTIFY: " part, keep the bit before it (which describes the device) as is, try to translate the text after it and then combine the text before it + the possibly translated text after it and show that as a notification.
The reason for (ab)using the printk ring-buffer for this is that we will still want to have these messages in dmesg too anyways, so why add a new mechanism and send the same message twice if we can just tag the messages inside the printk ring-buffer ?
Note the
dev_infoabove would likely be replaced with some new helper which also does some magic to help with gathering a list of strings to translate.Quoted 6 lines
Quoted 3 lines
Again just thinking out loud here. If anyone has any initial reaction to this please let me know...
As a general comment, I think it captures very well the possibility that the kernel has more information than userspace about the
circumstances
Quoted 3 lines
of something that a user should be notified. Definitely that's the case for these WMI/ACPI events, and I would think similar circumstances can apply to other subsystem too.
Right, that was my idea behind having a generic notification mechanism.
Regards,
Hans
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
In reply to [email protected]Quoted 6 lines · On Monday 08 June 2020 20:36:58 [email protected] wrote:
Can you please comment here how you would like to see events like this should come through to userspace?
* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock mode
In my opinion, Fn-lock mode is related to input subsystem and should be probably reported via input device. For a user, fn-lock is similar like caps-lock, scroll-lock or num-lock. Also fn-lock is provided by other laptops and therefore I would expect that kernel provide fn-lock state for all laptops (thinkpad / latitude / ...) via same interface. And not via dell-specific interface or general-vendor-message interface.
Wrong power adapter sounds like something related to power subsystem. Adding Sebastian to the loop, maybe he can provide some useful ideas about it.
And plugged dock into wrong port. This is probably dell-specific event and some interface for "vendor" messages from kernel to userspace would be needed to deliver such things.
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Hi,
Quoted 15 lines · Pali Rohár
Quoted 6 lines · On Monday 08 June 2020 20:36:58 [email protected] wrote:
Can you please comment here how you would like to see events like this should come through to userspace?
* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock modeIn my opinion, Fn-lock mode is related to input subsystem and should be probably reported via input device. For a user, fn-lock is similar like caps-lock, scroll-lock or num-lock. Also fn-lock is provided by other laptops and therefore I would expect that kernel provide fn-lock state for all laptops (thinkpad / latitude / ...) via same interface. And not via dell-specific interface or general-vendor-message interface.
Quoted 3 lines
Wrong power adapter sounds like something related to power subsystem. Adding Sebastian to the loop, maybe he can provide some useful ideas about it.
I'm missing a bit of context. I suppose this is about connecting a non-genuine power adapter rejected by the embedded controller? Support for that should be hooked into 'drivers/acpi/ac.c' (note:
so far there is no standard power-supply class property for this). Also printing a warning to dmesg seems sensible.
-- Sebastian
Quoted 3 lines
And plugged dock into wrong port. This is probably dell-specific event and some interface for "vendor" messages from kernel to userspace would be needed to deliver such things.
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Hi Sebastian,
Quoted 22 lines · Sebastian Reichel
Hi,
Quoted 15 lines · Pali Rohár
Quoted 6 lines · On Monday 08 June 2020 20:36:58 [email protected] wrote:
Can you please comment here how you would like to see events like this should come through to userspace?
* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock modeIn my opinion, Fn-lock mode is related to input subsystem and should be probably reported via input device. For a user, fn-lock is similar like caps-lock, scroll-lock or num-lock. Also fn-lock is provided by other laptops and therefore I would expect that kernel provide fn-lock state for all laptops (thinkpad / latitude / ...) via same interface. And not via dell-specific interface or general-vendor-message interface.
Quoted 3 lines
Wrong power adapter sounds like something related to power subsystem. Adding Sebastian to the loop, maybe he can provide some useful ideas about it.
Quoted 5 lines
I'm missing a bit of context. I suppose this is about connecting a non-genuine power adapter rejected by the embedded controller? Support for that should be hooked into '
drivers/acpi/ac.c' (note:
so far there is no standard power-supply class property for this). Also printing a warning to dmesg seems sensible.
This is not so much about non-genuine as about the adapter having the wrong wattage. E.g. plugging a 65W adapter in a laptop which has a 45W CPU + a 35W discrete GPU will not allow the laptop to really charge while it is in use.
One issue I see with doing this in the power_supply class is that the charger is represented by the standard ACPI AC adapter stuff, which does not have this info. This sort of extra info comes from WMI. Now we could have the WMI driver register a second power_supply device, but that means having 2 power_supply devices representing the 1 AC adapter which does not feel right.
I was myself actually thinking more along the lines of adding a new mechanism to the kernel where the kernel can send messages to userspace (either with some special tag inside dmesg, or through a new mechanism) indication that the message should be shown as a notification (dialog/bubble/whatever) inside the UI.
This could be useful for this adapter case, but e.g. also for pluging a thunderbolt device into a non thunderbolt capable Type-C port, a superspeed USB device into a USB-2 only USB port and probably other cases.
Rather then inventing separate userspace APIs for all these cases having a general notification mechanism might be quite useful for this (as long as the kernel does not over use it).
Regards,
Hans
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Hi,
Quoted 31 lines · Hans de Goede
Hi Sebastian,
Quoted 22 lines · Sebastian Reichel
Hi,
Quoted 15 lines · Pali Rohár
Quoted 6 lines · On Monday 08 June 2020 20:36:58 [email protected] wrote:
Can you please comment here how you would like to see events like this should come through to userspace?
* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock modeIn my opinion, Fn-lock mode is related to input subsystem and should be probably reported via input device. For a user, fn-lock is similar like caps-lock, scroll-lock or num-lock. Also fn-lock is provided by other laptops and therefore I would expect that kernel provide fn-lock state for all laptops (thinkpad / latitude / ...) via same interface. And not via dell-specific interface or general-vendor-message interface.
Quoted 3 lines
Wrong power adapter sounds like something related to power subsystem. Adding Sebastian to the loop, maybe he can provide some useful ideas about it.
Quoted 5 lines
I'm missing a bit of context. I suppose this is about connecting a non-genuine power adapter rejected by the embedded controller? Support for that should be hooked into '
drivers/acpi/ac.c' (note:
so far there is no standard power-supply class property for this). Also printing a warning to dmesg seems sensible.
Quoted 4 lines
This is not so much about non-genuine as about the adapter having the wrong wattage. E.g. plugging a 65W adapter in a laptop which has a 45W CPU + a 35W discrete GPU will not allow the laptop to really charge while it is in use.
Ok. So how much information is available over WMI? Exposing the max. input power via the power-supply API makes sense in any case.
Quoted 6 lines
One issue I see with doing this in the
power_supplyclass is that the charger is represented by the standard ACPI AC adapter stuff, which does not have this info. This sort of extra info comes from WMI. Now we could have the WMI driver register a secondpower_supplydevice, but that means having 2power_supplydevices representing the 1 AC adapter which does not feel right.
I agree. WMI and ACPI information need to be merged and exposed as one device to userspace. It's not the first time we have this kind of requirement, but so far it was about merging battery info from two places. Unfortunately no code has been written so far to support this.
Quoted 11 lines
I was myself actually thinking more along the lines of adding a new mechanism to the kernel where the kernel can send messages to userspace (either with some special tag inside dmesg, or through a new mechanism) indication that the message should be shown as a notification (dialog/bubble/whatever) inside the UI.
This could be useful for this adapter case, but e.g. also for pluging a thunderbolt device into a non thunderbolt capable Type-C port, a superspeed USB device into a USB-2 only USB port and probably other cases.
Quoted 4 lines
Rather then inventing separate userspace APIs for all these cases having a general notification mechanism might be quite useful for this (as long as the kernel does not over use it).
I don't think this is a good idea. It brings all kind of localization problems. Also the information is not machine parsable. It looks more like a hack to get things working quickly by avoiding using/designing proper APIs.
-- Sebastian
RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 8 lines
Quoted 5 lines
This is not so much about non-genuine as about the adapter having the wrong wattage. E.g. plugging a 65W adapter in a laptop which has a 45W CPU + a 35W discrete GPU will not allow the laptop to really charge while it is in use.
Ok. So how much information is available over WMI? Exposing the max. input power via the power-supply API makes sense in any case.
WMI is event driven. You plug in the adapter and the platform will evaluate its power needs and advertise it to the OS in the event.
It's important to note this is not a fixed value.
For example if you have a dock connected the power needs might be higher.
Quoted 30 lines
Quoted 7 lines
One issue I see with doing this in the
power_supplyclass is that the charger is represented by the standard ACPI AC adapter stuff, which does not have this info. This sort of extra info comes from WMI. Now we could have the WMI driver register a secondpower_supplydevice, but that means having 2power_supplydevices representing the 1 AC adapter which does not feel right.I agree. WMI and ACPI information need to be merged and exposed as one device to userspace. It's not the first time we have this kind of requirement, but so far it was about merging battery info from two places. Unfortunately no code has been written so far to support this.
Quoted 11 lines
I was myself actually thinking more along the lines of adding a new mechanism to the kernel where the kernel can send messages to userspace (either with some special tag inside dmesg, or through a new mechanism) indication that the message should be shown as a notification (dialog/bubble/whatever) inside the UI.
This could be useful for this adapter case, but e.g. also for pluging a thunderbolt device into a non thunderbolt capable Type-C port, a superspeed USB device into a USB-2 only USB port and probably other cases.
Quoted 4 lines
Rather then inventing separate userspace APIs for all these cases having a general notification mechanism might be quite useful for this (as long as the kernel does not over use it).
Quoted 4 lines
I don't think this is a good idea. It brings all kind of localization problems. Also the information is not machine parsable. It looks more like a hack to get things working quickly by avoiding using/designing proper APIs.
When you have the data to populate in sysfs at init time I would agree, but at least in this case it's not always static data that can be queried on demand. You would have to wait until the first event comes around and populate some kernel structures for the sysfs attributes to read from at that time.
As a similar suggestion to Hans', what about letting the kernel advertise a table of fixed printf style strings for translation? When the dynamic data comes in the event can just be an index to one of those strings and the data in the following bytes. Userland could then map the strings accordingly.
Running with this concept, it could even be an overhaul to your typical content in dmesg to allow errors and info messages to be translatable too.
Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
In reply to [email protected]Quoted 12 lines · On Monday 08 June 2020 15:40:53 [email protected] wrote:
Quoted 2 lines
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedEvent type
0x12is for "System Events". This is the type of events that you typically would see come in for things "like" the wrong power adapter being plugged in on Windows or stuff about plugging a Thunderbolt dock into a port that doesn't support Thunderbolt.A message might look something like (paraphrased)
"Your system requires a 180W power adapter to charge effectively, but you plugged in a 60W adapter."
Quoted 7 lines
There often is extended data with these events. As such I don't believe all information in event type
0x0012should be treated like scan codes like those in0x10or0x11.I would guess that Fn-lock on this machine probably has extended data in the next showing whether it was turned on and off. If it does, perhaps it makes sense to send this information to userspace as an evdev switch instead.
Thank you for information!
Interesting is that I got this email only now, after all other emails in this thread. But seems that you have wrote it prior I asked question about documentation for these events. So probably some email delivery problem / delay.
PATCH 3/3 platform/x86: dell-wmi: add keys to bios_to_linux_keycode
In reply to Y ParitcherIncrease 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 0xffffdell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff
--- drivers/platform/x86/dell-wmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/platform/x86/dell-wmi.c +2 -1
@@ -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, };/*
Signature · 1 line
2.27.0
kernel test robot
build botRe: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode
Thank you for the patch! Perhaps something to improve:
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
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/kbuild-all(a)lists.01.org
kernel test robot
build botRe: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode
In reply to Y ParitcherThank you for the patch! Perhaps something to improve:
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
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/kbuild-all(a)lists.01.org
Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode
In reply to Y ParitcherHello!
Quoted 17 lines · On Monday 08 June 2020 00:22:26 Y Paritcher wrote:
Increase length of
bios_to_linux_keycodeto 2 bytes to allow for a new keycode0xffff, this silences the following messages being logged at startup on a Dell Inspiron 5593
dell_wmi: firmware scancode0x48maps to unrecognized keycode0xffffdell_wmi: firmware scancode0x50maps to unrecognized keycode0xffffSigned-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/platform/x86/dell-wmi.c +0
-0
drivers/platform/x86/dell-wmi.c struct dell_dmi_results { · line 199
Quoted 5 lines
drivers/platform/x86/dell-wmi.c +1
-1@@ -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.
drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] = { · line 240
Quoted 8 lines
drivers/platform/x86/dell-wmi.c +1
-0[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?
Quoted 6 lines
drivers/platform/x86/dell-wmi.c +0
-0};/* Signature · 1 line
2.27.0
RE: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode
Quoted 11 lines
-----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 tobios_to_linux_keycode
Quoted 8 lines
[EXTERNAL EMAIL]
Hello!
Quoted 3 lines · On Monday 08 June 2020 00:22:26 Y Paritcher wrote:
Increase length of
bios_to_linux_keycodeto 2 bytes to allow for a new keycode0xffff, 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?
Quoted 44 lines
drivers/platform/x86/dell-
Quoted 10 lines
dell_wmi: firmware scancode0x48maps to unrecognized keycode0xffffdell_wmi: firmware scancode0x50maps to unrecognized keycode0xffffSigned-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.cdrivers/platform/x86/dell-wmi.c struct dell_dmi_results { · line 199
Quoted 9 lines
index
f37e7e9093c2..5ef716e3034f100644drivers/platform/x86/dell-wmi.c +1
-1@@ -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.
drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] = { · line 240
Quoted 8 lines
drivers/platform/x86/dell-wmi.c +1
-0[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 (
0x48and0x50) 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.
Quoted 13 lines
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?
Quoted 6 lines
drivers/platform/x86/dell-wmi.c +0
-1};/* -- 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.
Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode
Quoted 13 lines · On 6/8/20 11:46 AM, [email protected] wrote:
Quoted 13 lines
-----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 tobios_to_linux_keycode[EXTERNAL EMAIL]
Quoted 56 lines
Quoted 6 lines
Hello!
Quoted 3 lines · On Monday 08 June 2020 00:22:26 Y Paritcher wrote:
Increase length of
bios_to_linux_keycodeto 2 bytes to allow for a new keycode0xffff, this silences the following messages being logged at startup on a Dell Inspiron 5593Which event type? Can you show the whole WMI buffer that came through?
Quoted 47 lines
drivers/platform/x86/dell-
Quoted 10 lines
dell_wmi: firmware scancode0x48maps to unrecognized keycode0xffffdell_wmi: firmware scancode0x50maps to unrecognized keycode0xffffSigned-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.cdrivers/platform/x86/dell-wmi.c struct dell_dmi_results { · line 199
Quoted 9 lines
index
f37e7e9093c2..5ef716e3034f100644drivers/platform/x86/dell-wmi.c +1
-1@@ -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.
drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] = { · line 240
Quoted 8 lines
drivers/platform/x86/dell-wmi.c +1
-0[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 (
0x48and0x50) 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.
Quoted 4 lines
Quoted 4 lines
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.
Quoted 13 lines
Quoted 6 lines
Quoted 6 lines
drivers/platform/x86/dell-wmi.c +0
-1};/* -- 2.27.0
I would actually question if there is value to lines in
dell-wmi.clike this:
pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);and
Quoted 8 lines
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.
Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode
In reply to [email protected]Quoted 6 lines · On Monday 08 June 2020 15:46:44 [email protected] wrote:
I would actually question if there is value to lines in
dell-wmi.clike this:
pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);and
Quoted 7 lines
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;
RE: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode
Quoted 33 lines
-----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 tobios_to_linux_keycode[EXTERNAL EMAIL]
Quoted 1 line · On Monday 08 June 2020 15:46:44 [email protected] wrote:
I would actually question if there is value to lines in
dell-wmi.clikethis:
Quoted 6 lines
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);
Quoted 2 lines
In both of those cases the information doesn't actually help the user, by
default it's
Quoted 1 line
ignored by the driver anyway. It just notifies the user it's something
the driver doesn't
Quoted 1 line
comprehend. I would think these are better suited to downgrade to debug.
And if
Quoted 1 line
a key combination isn't doing something expected the user can use dyndbg
to turn it
Quoted 1 line
back on and can be debugged what should be populated or "explicitly"
Quoted 9 lines
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.
Quoted 13 lines
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.
Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode
Quoted 39 lines · On Monday 08 June 2020 20:58:38 [email protected] wrote:
Quoted 39 lines
-----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 tobios_to_linux_keycode[EXTERNAL EMAIL]
Quoted 1 line · On Monday 08 June 2020 15:46:44 [email protected] wrote:
I would actually question if there is value to lines in
dell-wmi.clikethis:
Quoted 6 lines
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);
Quoted 2 lines
In both of those cases the information doesn't actually help the user, by
default it's
Quoted 1 line
ignored by the driver anyway. It just notifies the user it's something
the driver doesn't
Quoted 1 line
comprehend. I would think these are better suited to downgrade to debug.
And if
Quoted 1 line
a key combination isn't doing something expected the user can use dyndbg
to turn it
Quoted 1 line
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 14 lines
Quoted 13 lines
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;
Quoted 2 lines
I think the difference here is that user can actually do something from userland to do with
setkeycodesfor PS2.
Of course, I agree.
v2 cover platform/x86: dell-wmi: new keys
In reply to Y ParitcherAs per discussion backlight events are passed on as they are filtered by acpi-video.
Events of type 0x0012 have extended data in the rest of the buffer, this data is currently ignored.
The Fn lock key currently is ignored. If userspace has a way to deal with this a function to pass on the extended data can be added.
messages of type:
firmware scancode 0xXX maps to unrecognized keycode 0xXXXX
are from unknown keycodes in the DMI table and should be added to bios_to_linux_keycode to allow them to be processed.
Y Paritcher (3):
platform/x86: dell-wmi: add new backlight events platform/x86: dell-wmi: add new keymap type 0x0012 platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
drivers/platform/x86/dell-wmi.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)
Signature · 1 line
2.27.0
PATCH v2 1/3 platform/x86: dell-wmi: add new backlight events
Add events with a type of 0x0010 and a code of 0x57 / 0x58, this silences the following messages being logged on a Dell Inspiron 5593:
dell_wmi: Unknown key with type 0x0010 and code 0x0057 presseddell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
These are brightness events and will be handled by acpi-video
--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/platform/x86/dell-wmi.c +4 -0
@@ -255,6 +255,10 @@ static const struct key_entry dell_wmi_keymap_type_0010[] = {/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } }, +/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +/* Mic mute */ { KE_KEY, 0x150, { KEY_MICMUTE } },
Signature · 1 line
2.27.0
Re: [PATCH v2 1/3] platform/x86: dell-wmi: add new backlight events
Quoted 10 lines · On Monday 08 June 2020 19:05:28 Y Paritcher wrote:
Add events with a type of
0x0010and a code of0x57/0x58, this silences the following messages being logged on a Dell Inspiron 5593:
dell_wmi: Unknown key with type0x0010and code0x0057presseddell_wmi: Unknown key with type0x0010and code0x0058pressedThese are brightness events and will be handled by acpi-video
Signed-off-by Y Paritcher <[email protected]>
Quoted 22 lines
--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/platform/x86/dell-wmi.c +4
-0@@ -255,6 +255,10 @@ static const struct key_entry dell_wmi_keymap_type_0010[] = {/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } }, +/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +/* Mic mute */ { KE_KEY, 0x150, { KEY_MICMUTE } },Signature · 1 line
2.27.0
PATCH v2 2/3 platform/x86: dell-wmi: add new keymap type 0x0012
In reply to Y ParitcherThese are events with extended data. The extended data is currently ignored as userspace does not have a way to deal it.
Ignore event with a type of 0x0012 and a code of 0xe035, as the keyboard controller takes care of Fn lock events by itself. This silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type 0x12dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
This is consistent with the behavior for the Fn-lock key elsewhere in this file.
--- drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/platform/x86/dell-wmi.c +19 -1
@@ -334,6 +334,15 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +* They are events with extended data +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code) {struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);
@@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device *wdev,switch (buffer_entry[1 ]) {case 0x0000 :/* One key pressed or event occurred */ +case 0x0012 :/* Event with extended data occurred */ if (len >2 )dell_wmi_process_key (wdev,0x0000 , buffer_entry[2 ]); -/* Other entries could contain additional information */ +/* Extended data is currently ignored */ break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */
@@ -556,6 +566,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {
@@ -600,6 +611,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI.
Signature · 1 line
2.27.0
Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 6 lines · On Monday 08 June 2020 19:05:29 Y Paritcher wrote:
These are events with extended data. The extended data is currently ignored as userspace does not have a way to deal it.
Ignore event with a type of
0x0012and a code of0xe035, as the keyboard controller takes care of Fn lock events by itself.
Nice! This is information which is really important and need to have it documented.
Quoted 10 lines
This silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedThis is consistent with the behavior for the Fn-lock key elsewhere in this file.
Signed-off-by Y Paritcher <[email protected]>
I'm fine with this patch now.
Quoted 25 lines
--- drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)drivers/platform/x86/dell-wmi.c +9
-0@@ -334,6 +334,15 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +* They are events with extended data +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code) {struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device *wdev, · line 430
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +1
-0@@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device *wdev,switch (buffer_entry[1 ]) {case 0x0000 :/* One key pressed or event occurred */ +case 0x0012 :/* Event with extended data occurred */
Mario, are you able to get some official documentation for these 0x0012 event types? I think it could be really useful for community so they can understand and add easily new type of code and events. Because currently we are just guessing what it could be. (It is sequence? Or single event? Or single event with extended data? It is generic event? Or it is real keypress? etc...)
Quoted 33 lines
drivers/platform/x86/dell-wmi.c +9
-1if (len >2 )dell_wmi_process_key (wdev,0x0000 , buffer_entry[2 ]); -/* Other entries could contain additional information */ +/* Extended data is currently ignored */ break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ @@ -556,6 +566,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {@@ -600,6 +611,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI. Signature · 1 line
2.27.0
RE: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 63 lines
-----Original Message-----
From: Pali Rohár <[email protected]>
Sent: Monday, June 8, 2020 6:33 PM
To: Y Paritcher
Cc: [email protected]; [email protected]; Matthew Garrett; Limonciello, Mario
Subject: Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type0x0012[EXTERNAL EMAIL]
Quoted 6 lines · On Monday 08 June 2020 19:05:29 Y Paritcher wrote:
These are events with extended data. The extended data is currently ignored as userspace does not have a way to deal it.
Ignore event with a type of
0x0012and a code of0xe035, as the keyboard controller takes care of Fn lock events by itself.Nice! This is information which is really important and need to have it documented.
Quoted 10 lines
This silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedThis is consistent with the behavior for the Fn-lock key elsewhere in this file.
Signed-off-by Y Paritcher <[email protected]>I'm fine with this patch now.
Reviewed-by Pali Rohár <[email protected]>drivers/platform/x86/dell-
Quoted 5 lines
--- drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.cdrivers/platform/x86/dell-wmi.c static const struct key_entry
Quoted 4 lines
index
0b2edfe2767d..6b510f8431a3100644drivers/platform/x86/dell-wmi.c +0
-0@@ -334,6 +334,15 @@ static const struct key_entrydell_wmi_keymap_type_0011[] = {
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0011[] = {
Quoted 13 lines
drivers/platform/x86/dell-wmi.c +9
-0{ KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +* They are events with extended data +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; + static void dell_wmi_process_key(struct wmi_device *wdev, int type, intcode)
Quoted 8 lines
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +0
-0{struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);@@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device*wdev,
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device *wdev, · line 430
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +1
-0switch (buffer_entry[1 ]) {case 0x0000 :/* One key pressed or event occurred */ +case 0x0012 :/* Event with extended data occurred */
I don't really like this being handled as a key as it's just discarding all that extended data.
Quoted 7 lines
Mario, are you able to get some official documentation for these
0x0012event types? I think it could be really useful for community so they can understand and add easily new type of code and events. Because currently we are just guessing what it could be. (It is sequence? Or single event? Or single event with extended data? It is generic event? Or it is real keypress? etc...)
It's a single event with more data in the subsequent words. It is definitely not a real keypress. It's supposed to be data that a user application would show.
Remember the way WMI works on Linux and Windows is different. On Windows userland applications get the events directly. On Linux kernel drivers get the events and either use it internally, pass to another kernel driver or pass to userland in the form of a translated event.
So on Windows the whole buffer gets looked at directly by the application and the application will decode it to show a translated string.
I can certainly discuss internally about our team releasing a patch to export all these other events. I would like to know what interface to recommend it pass to userspace though, because as I said this is more than just a keycode that comes through in the event. It's not useful to just do dev_info, it really should be something that userspace can act on and show a translated message. I don't think we want to add another 15 Dell specific keycodes to the kernel for the various events and add another 4 more when a laptop introduces another set of keys.
Quoted 36 lines
drivers/platform/x86/dell-wmi.c static int dell_wmi_input_setup(struct wmi_device
Quoted 10 lines
drivers/platform/x86/dell-wmi.c +1
-1if (len >2 )dell_wmi_process_key (wdev,0x0000 , buffer_entry[2 ]); -/* Other entries could contain additional information */ +/* Extended data is currently ignored */ break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ @@ -556,6 +566,7 @@ static int dell_wmi_input_setup(struct wmi_device*wdev)
drivers/platform/x86/dell-wmi.c static int dell_wmi_input_setup(struct wmi_device
Quoted 8 lines
drivers/platform/x86/dell-wmi.c +1
-0ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {@@ -600,6 +611,13 @@ static int dell_wmi_input_setup(struct wmi_device*wdev)
Quoted 16 lines
drivers/platform/x86/dell-wmi.c +7
-1pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI. -- 2.27.0
Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 94 lines · On 6/8/20 8:26 PM, [email protected] wrote:
Quoted 63 lines
-----Original Message-----
From: Pali Rohár <[email protected]>
Sent: Monday, June 8, 2020 6:33 PM
To: Y Paritcher
Cc: [email protected]; [email protected]; Matthew Garrett; Limonciello, Mario
Subject: Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type0x0012[EXTERNAL EMAIL]
Quoted 6 lines · On Monday 08 June 2020 19:05:29 Y Paritcher wrote:
These are events with extended data. The extended data is currently ignored as userspace does not have a way to deal it.
Ignore event with a type of
0x0012and a code of0xe035, as the keyboard controller takes care of Fn lock events by itself.Nice! This is information which is really important and need to have it documented.
Quoted 10 lines
This silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedThis is consistent with the behavior for the Fn-lock key elsewhere in this file.
Signed-off-by Y Paritcher <[email protected]>I'm fine with this patch now.
Reviewed-by Pali Rohár <[email protected]>drivers/platform/x86/dell-
Quoted 5 lines
--- drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.cdrivers/platform/x86/dell-wmi.c static const struct key_entry
Quoted 4 lines
index
0b2edfe2767d..6b510f8431a3100644drivers/platform/x86/dell-wmi.c +0
-0@@ -334,6 +334,15 @@ static const struct key_entrydell_wmi_keymap_type_0011[] = {
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0011[] = {
Quoted 13 lines
drivers/platform/x86/dell-wmi.c +9
-0{ KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +* They are events with extended data +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; + static void dell_wmi_process_key(struct wmi_device *wdev, int type, intcode)
Quoted 8 lines
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +0
-0{struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);@@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device*wdev,
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device *wdev, · line 430
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +1
-0switch (buffer_entry[1 ]) {case 0x0000 :/* One key pressed or event occurred */ +case 0x0012 :/* Event with extended data occurred */ I don't really like this being handled as a key as it's just discarding all that extended data.
Quoted 7 lines
Mario, are you able to get some official documentation for these
0x0012event types? I think it could be really useful for community so they can understand and add easily new type of code and events. Because currently we are just guessing what it could be. (It is sequence? Or single event? Or single event with extended data? It is generic event? Or it is real keypress? etc...)It's a single event with more data in the subsequent words. It is definitely not a real keypress. It's supposed to be data that a user application would show.
Remember the way WMI works on Linux and Windows is different. On Windows userland applications get the events directly. On Linux kernel drivers get the events and either use it internally, pass to another kernel driver or pass to userland in the form of a translated event.
So on Windows the whole buffer gets looked at directly by the application and the application will decode it to show a translated string.
Quoted 8 lines
I can certainly discuss internally about our team releasing a patch to export all these other events. I would like to know what interface to recommend it pass to userspace though, because as I said this is more than just a keycode that comes through in the event. It's not useful to just do
dev_info, it really should be something that userspace can act on and show a translated message. I don't think we want to add another 15 Dell specific keycodes to the kernel for the various events and add another 4 more when a laptop introduces another set of keys.
We can treat this as a key for now. This gets rid of the unnecessary log messages.
When adecision is made about how to handle extended events it will be very easy just to make this its own case statement and handle with it separately.
However if you want to pass Fn lock event to userspace there are keys from older models of type 0010:
/* Fn-lock switched to function keys */
{ KE_IGNORE, 0x0, { KEY_RESERVED } },
/* Fn-lock switched to multimedia keys */
{ KE_IGNORE, 0x1, { KEY_RESERVED } },They should also be changed to be passed to userspace and they don't signify status with extended data rather by which event is called.
This means that passing these types of events to userspace is a separate task, that might require rethinking the entire design of how all event types are treated.
The current patch will be necessary regardless of what is decided in regards to passing the events on, and continues the status quo.
Quoted 36 lines
Quoted 36 lines
drivers/platform/x86/dell-wmi.c static int dell_wmi_input_setup(struct wmi_device
Quoted 10 lines
drivers/platform/x86/dell-wmi.c +1
-1if (len >2 )dell_wmi_process_key (wdev,0x0000 , buffer_entry[2 ]); -/* Other entries could contain additional information */ +/* Extended data is currently ignored */ break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ @@ -556,6 +566,7 @@ static int dell_wmi_input_setup(struct wmi_device*wdev)
drivers/platform/x86/dell-wmi.c static int dell_wmi_input_setup(struct wmi_device
Quoted 8 lines
drivers/platform/x86/dell-wmi.c +1
-0ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {@@ -600,6 +611,13 @@ static int dell_wmi_input_setup(struct wmi_device*wdev)
Quoted 16 lines
drivers/platform/x86/dell-wmi.c +7
-1pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI. -- 2.27.0
Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
Quoted 69 lines · On Monday 08 June 2020 20:57:38 Y Paritcher wrote:
Quoted 68 lines · On 6/8/20 8:26 PM, [email protected] wrote:
Quoted 68 lines
-----Original Message-----
From: Pali Rohár <[email protected]>
Sent: Monday, June 8, 2020 6:33 PM
To: Y Paritcher
Cc: [email protected]; [email protected]; Matthew Garrett; Limonciello, Mario
Subject: Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type0x0012[EXTERNAL EMAIL]
Quoted 6 lines · On Monday 08 June 2020 19:05:29 Y Paritcher wrote:
These are events with extended data. The extended data is currently ignored as userspace does not have a way to deal it.
Ignore event with a type of
0x0012and a code of0xe035, as the keyboard controller takes care of Fn lock events by itself.Nice! This is information which is really important and need to have it documented.
Quoted 10 lines
This silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type0x12dell_wmi: Unknown key with type0x0012and code0xe035pressedThis is consistent with the behavior for the Fn-lock key elsewhere in this file.
Signed-off-by Y Paritcher <[email protected]>I'm fine with this patch now.
Reviewed-by Pali Rohár <[email protected]>drivers/platform/x86/dell-
Quoted 5 lines
--- drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.cdrivers/platform/x86/dell-wmi.c static const struct key_entry
Quoted 4 lines
index
0b2edfe2767d..6b510f8431a3100644drivers/platform/x86/dell-wmi.c +0
-0@@ -334,6 +334,15 @@ static const struct key_entrydell_wmi_keymap_type_0011[] = {
drivers/platform/x86/dell-wmi.c static const struct key_entry dell_wmi_keymap_type_0011[] = {
Quoted 13 lines
drivers/platform/x86/dell-wmi.c +9
-0{ KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +* They are events with extended data +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; + static void dell_wmi_process_key(struct wmi_device *wdev, int type, intcode)
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +0
-0{struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);@@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device*wdev,
Quoted 1 line
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
drivers/platform/x86/dell-wmi.c static void dell_wmi_notify(struct wmi_device *wdev, · line 430
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +1
-0switch (buffer_entry[1 ]) {case 0x0000 :/* One key pressed or event occurred */ +case 0x0012 :/* Event with extended data occurred */ I don't really like this being handled as a key as it's just discarding all that extended data.
It is not ideal. But currently we handle all events as key press. And non-key events are marked as ignored.
I agree, it should be handled in better way. But that needs to extend driver. But it is something which should be done in next patches, not in this one (if somebody has time to do it, etc...).
Quoted 28 lines
Quoted 19 lines
Quoted 7 lines
Mario, are you able to get some official documentation for these
0x0012event types? I think it could be really useful for community so they can understand and add easily new type of code and events. Because currently we are just guessing what it could be. (It is sequence? Or single event? Or single event with extended data? It is generic event? Or it is real keypress? etc...)It's a single event with more data in the subsequent words. It is definitely not a real keypress. It's supposed to be data that a user application would show.
Remember the way WMI works on Linux and Windows is different. On Windows userland applications get the events directly. On Linux kernel drivers get the events and either use it internally, pass to another kernel driver or pass to userland in the form of a translated event.
So on Windows the whole buffer gets looked at directly by the application and the application will decode it to show a translated string.
Quoted 8 lines
I can certainly discuss internally about our team releasing a patch to export all these other events. I would like to know what interface to recommend it pass to userspace though, because as I said this is more than just a keycode that comes through in the event. It's not useful to just do
dev_info, it really should be something that userspace can act on and show a translated message. I don't think we want to add another 15 Dell specific keycodes to the kernel for the various events and add another 4 more when a laptop introduces another set of keys.
Quoted 4 lines
We can treat this as a key for now. This gets rid of the unnecessary log messages.
When adecision is made about how to handle extended events it will be very easy just to make this its own case statement and handle with it separately.
I agree. Handle it in the same way like we handle all other events. That would make it easier to migrate to the new way, once it is there.
Quoted 52 lines
However if you want to pass Fn lock event to userspace there are keys from older models of type 0010:
/* Fn-lock switched to function keys */ { KE_IGNORE, 0x0, { KEY_RESERVED } }, /* Fn-lock switched to multimedia keys */ { KE_IGNORE, 0x1, { KEY_RESERVED } },They should also be changed to be passed to userspace and they don't signify status with extended data rather by which event is called.
This means that passing these types of events to userspace is a separate task, that might require rethinking the entire design of how all event types are treated.
The current patch will be necessary regardless of what is decided in regards to passing the events on, and continues the status quo.
Quoted 36 lines
Quoted 36 lines
drivers/platform/x86/dell-wmi.c static int dell_wmi_input_setup(struct wmi_device
Quoted 10 lines
drivers/platform/x86/dell-wmi.c +1
-1if (len >2 )dell_wmi_process_key (wdev,0x0000 , buffer_entry[2 ]); -/* Other entries could contain additional information */ +/* Extended data is currently ignored */ break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */ @@ -556,6 +566,7 @@ static int dell_wmi_input_setup(struct wmi_device*wdev)
drivers/platform/x86/dell-wmi.c static int dell_wmi_input_setup(struct wmi_device
Quoted 8 lines
drivers/platform/x86/dell-wmi.c +1
-0ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {@@ -600,6 +611,13 @@ static int dell_wmi_input_setup(struct wmi_device*wdev)
Quoted 16 lines
drivers/platform/x86/dell-wmi.c +7
-1pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI. -- 2.27.0
Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
In reply to [email protected]Quoted 18 lines · On Tuesday 09 June 2020 00:26:45 [email protected] wrote:
Quoted 6 lines
Mario, are you able to get some official documentation for these
0x0012event types? I think it could be really useful for community so they can understand and add easily new type of code and events. Because currently we are just guessing what it could be. (It is sequence? Or single event? Or single event with extended data? It is generic event? Or it is real keypress? etc...)It's a single event with more data in the subsequent words. It is definitely not a real keypress. It's supposed to be data that a user application would show.
Remember the way WMI works on Linux and Windows is different. On Windows userland applications get the events directly. On Linux kernel drivers get the events and either use it internally, pass to another kernel driver or pass to userland in the form of a translated event.
So on Windows the whole buffer gets looked at directly by the application and the application will decode it to show a translated string.
Quoted 7 lines
I can certainly discuss internally about our team releasing a patch to export all these other events. I would like to know what interface to recommend it pass to userspace though, because as I said this is more than just a keycode that comes through in the event. It's not useful to just do
dev_info, it really should be something that userspace can act on and show a translated message. I don't think we want to add another 15 Dell specific keycodes to the kernel for the various events and add another 4 more when a laptop introduces another set of keys.
Which interface to use for events? That is a good question and probably this should be bring to the linux-input mailinglist. I think that linux-input maintainers could have idea how to do it properly. We need some interface which would be general enough and usable also by other drivers / components and I'm sure that ACPI/WMI is not the only subsystem which needs to send events from kernel to userspace.
PATCH v2 3/3 platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
In reply to Y ParitcherIncrease 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 0xffffas 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.
--- drivers/platform/x86/dell-wmi.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/platform/x86/dell-wmi.c +3 -5
@@ -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
Signature · 1 line
2.27.0
Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
Hi--
Quoted 7 lines · Y Paritcher
Increase length of
bios_to_linux_keycodeto 2 bytes (the true size of a keycode) to allow for a new keycode0xffff, 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
Quoted 8 lines
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
0xB2DMI table, for which the corosponding keys are not known.
corresponding
Quoted 17 lines
Now when a user will encounter this key, a proper message wil be printed:
dell_wmi: Unknown key with type 0xXXXX and code 0xXXXX pressedThis 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(-)drivers/platform/x86/dell-wmi.c +0
-0
drivers/platform/x86/dell-wmi.c struct dell_dmi_results { · line 199
Quoted 5 lines
drivers/platform/x86/dell-wmi.c +1
-1@@ -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?
Quoted 14 lines
drivers/platform/x86/dell-wmi.c +1
-0[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];
drivers/platform/x86/dell-wmi.c static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) · line 510
Quoted 10 lines
drivers/platform/x86/dell-wmi.c +1
-4/* 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) <>
Signature · 1 line
~Randy
Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
Hello!
Quoted 9 lines · On Monday 08 June 2020 16:27:10 Randy Dunlap wrote:
Hi--
Quoted 6 lines · Y Paritcher
Increase length of
bios_to_linux_keycodeto 2 bytes (the true size of a keycode) to allow for a new keycode0xffff, 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.
Quoted 30 lines
Quoted 8 lines
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
0xB2DMI table, for which the corosponding keys are not known.corresponding
Quoted 19 lines
Now when a user will encounter this key, a proper message wil be printed:
dell_wmi: Unknown key with type 0xXXXX and code 0xXXXX pressedThis 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(-)drivers/platform/x86/dell-wmi.c +0
-0@@ -196,7 +196,7 @@ struct dell_dmi_results { };
Quoted 7 lines
drivers/platform/x86/dell-wmi.c struct dell_dmi_results { · line 199
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +1
-1/* 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.
Quoted 8 lines
drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] = { · line 240
Quoted 8 lines
drivers/platform/x86/dell-wmi.c +1
-0[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).
Quoted 29 lines
Quoted 6 lines
drivers/platform/x86/dell-wmi.c +0
-0};/* @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) &table->keymap[i];drivers/platform/x86/dell-wmi.c static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) · line 510
Quoted 10 lines
drivers/platform/x86/dell-wmi.c +1
-4/* 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;
Quoted 2 lines
Also please fix this:
(no To-header on input) <>
Hint: specifying git send-email with '--to' argument instead of '--cc' should help.
Quoted 4 lines
Signature · 2 lines
-- ~Randy
Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
Quoted 13 lines · Pali Rohár
Hello!
Quoted 9 lines · On Monday 08 June 2020 16:27:10 Randy Dunlap wrote:
Hi--
Quoted 6 lines · Y Paritcher
Increase length of
bios_to_linux_keycodeto 2 bytes (the true size of a keycode) to allow for a new keycode0xffff, 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
Quoted 7 lines
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.
Quoted 38 lines
Quoted 30 lines
Quoted 8 lines
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
0xB2DMI table, for which the corosponding keys are not known.corresponding
Quoted 19 lines
Now when a user will encounter this key, a proper message wil be printed:
dell_wmi: Unknown key with type 0xXXXX and code 0xXXXX pressedThis 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(-)drivers/platform/x86/dell-wmi.c +0
-0@@ -196,7 +196,7 @@ struct dell_dmi_results { };Quoted 7 lines
drivers/platform/x86/dell-wmi.c struct dell_dmi_results { · line 199
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +1
-1/* 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?
Quoted 4 lines
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
0xffffcheck as increasing memory usage.
Will change
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +0
-0[0] = KEY_MEDIA, [1] = KEY_NEXTSONG, [2] = KEY_PLAYPAUSE,@@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
Quoted 9 lines
Quoted 4 lines
drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] = { · line 240
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +1
-0[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
0x00FFand0xFFFFare 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.
Quoted 42 lines
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).
Quoted 29 lines
Quoted 6 lines
drivers/platform/x86/dell-wmi.c +0
-0};/* @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) &table->keymap[i];drivers/platform/x86/dell-wmi.c static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) · line 510
Quoted 10 lines
drivers/platform/x86/dell-wmi.c +1
-4/* 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;
Quoted 6 lines
Quoted 2 lines
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.
Quoted 4 lines
Quoted 4 lines
Signature · 2 lines
-- ~Randy
Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
Quoted 27 lines · On Monday 08 June 2020 20:43:45 Y Paritcher wrote:
Quoted 13 lines · Pali Rohár
Hello!
Quoted 9 lines · On Monday 08 June 2020 16:27:10 Randy Dunlap wrote:
Hi--
Quoted 6 lines · Y Paritcher
Increase length of
bios_to_linux_keycodeto 2 bytes (the true size of a keycode) to allow for a new keycode0xffff, 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 0xffffQuoted 7 lines
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 thebios_to_linux_keycodeentries &dell_wmi_keymap_type_tables to create a keymap.
Quoted 5 lines
If a DMI entry does not have a corresponding entry in
bios_to_linux_keycodewe 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)
Quoted 59 lines
Quoted 38 lines
Quoted 30 lines
Quoted 8 lines
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
0xB2DMI table, for which the corosponding keys are not known.corresponding
Quoted 19 lines
Now when a user will encounter this key, a proper message wil be printed:
dell_wmi: Unknown key with type 0xXXXX and code 0xXXXX pressedThis 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(-)drivers/platform/x86/dell-wmi.c +0
-0@@ -196,7 +196,7 @@ struct dell_dmi_results { };Quoted 7 lines
drivers/platform/x86/dell-wmi.c struct dell_dmi_results { · line 199
Quoted 3 lines
drivers/platform/x86/dell-wmi.c +1
-1/* 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?
Quoted 4 lines
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
0xffffcheck as increasing memory usage.Will change
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +0
-0[0] = KEY_MEDIA, [1] = KEY_NEXTSONG, [2] = KEY_PLAYPAUSE,@@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {Quoted 9 lines
Quoted 4 lines
drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] = { · line 240
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +1
-0[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
0x00FFand0xFFFFare just "placeholders" or special values for unknown / custom / unsupported / reserved / special / ... codes.
Quoted 1 line
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.
Quoted 59 lines
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.
Quoted 42 lines
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).
Quoted 29 lines
Quoted 6 lines
drivers/platform/x86/dell-wmi.c +0
-0};/* @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) &table->keymap[i];drivers/platform/x86/dell-wmi.c static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) · line 510
Quoted 10 lines
drivers/platform/x86/dell-wmi.c +1
-4/* 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;Quoted 6 lines
Quoted 2 lines
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.
Quoted 4 lines
Quoted 4 lines
Signature · 2 lines
-- ~Randy
RE: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
In reply to Pali RohárQuoted 5 lines
Looking at the last two lines... and for me it looks like that
0x00FFand0xFFFFare just "placeholders" or special values for unknown / custom / unsupported / reserved / special / ... codes.
Quoted 9 lines
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 function0xFFFF 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.
Quoted 44 lines
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).
Quoted 4 lines
Review static void handle_dmi_entry(const struct
Quoted 4 lines
};
/*
(quoted diff) +0
-0@@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
dmi_header*dm, void *opaque)Quoted 25 lines
drivers/platform/x86/dell-wmi.c static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) · line 510
Quoted 12 lines
drivers/platform/x86/dell-wmi.c +1
-4&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;Quoted 2 lines
Also please fix this:
(no To-header on input) <>Hint: specifying git send-email with '--to' argument instead of '--cc' should help.
Quoted 4 lines
--
~Randy
Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
Quoted 18 lines · On Tuesday 09 June 2020 19:49:18 [email protected] wrote:
Quoted 5 lines
Looking at the last two lines... and for me it looks like that
0x00FFand0xFFFFare just "placeholders" or special values for unknown / custom / unsupported / reserved / special / ... codes.Quoted 9 lines
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.
Quoted 7 lines
Those two called out are special though, here are their meanings:
0x00FFis user programmable function0xFFFFis no functionFor 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".
Quoted 44 lines
Quoted 44 lines
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).
Quoted 4 lines
Review static void handle_dmi_entry(const struct
Quoted 4 lines
};
/*
(quoted diff) +0
-0@@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
dmi_header*dm, void *opaque)Quoted 25 lines
drivers/platform/x86/dell-wmi.c static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) · line 510
Quoted 12 lines
drivers/platform/x86/dell-wmi.c +1
-4&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;Quoted 2 lines
Also please fix this:
(no To-header on input) <>Hint: specifying git send-email with '--to' argument instead of '--cc' should help.
Quoted 4 lines
--
~Randy
RE: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
Quoted 42 lines
-----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 tobios_to_linux_keycode[EXTERNAL EMAIL]
Quoted 9 lines · On Tuesday 09 June 2020 19:49:18 [email protected] wrote:
Quoted 9 lines
Looking at the last two lines... and for me it looks like that
0x00FFand0xFFFFare 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
Quoted 7 lines
Quoted 5 lines
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
Quoted 8 lines
first two bytes is used.
Those two called out are special though, here are their meanings:
0x00FFis user programmable function0xFFFFis no functionFor the purpose of memory consumption I think it's reasonable to ignore
the
Quoted 8 lines
Quoted 1 line
upper 2 bytes and special case these two.
Thank you for information!
So
0x00FFis "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
Quoted 3 lines
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.
Quoted 45 lines
Quoted 45 lines
Quoted 45 lines
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).
Quoted 4 lines
Review static void handle_dmi_entry(const struct
Quoted 4 lines
};
/*
(quoted diff) +0
-0@@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
dmi_header*dm, void *opaque)Quoted 25 lines
drivers/platform/x86/dell-wmi.c static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) · line 510
Quoted 12 lines
drivers/platform/x86/dell-wmi.c +1
-4&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;Quoted 2 lines
Also please fix this:
(no To-header on input) <>Hint: specifying git send-email with '--to' argument instead of '--cc' should help.
Quoted 4 lines
--
~Randy
Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
Quoted 51 lines · On Wednesday 10 June 2020 12:35:09 [email protected] wrote:
Quoted 42 lines
-----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 tobios_to_linux_keycode[EXTERNAL EMAIL]
Quoted 9 lines · On Tuesday 09 June 2020 19:49:18 [email protected] wrote:
Quoted 9 lines
Looking at the last two lines... and for me it looks like that
0x00FFand0xFFFFare 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
Quoted 7 lines
Quoted 5 lines
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
Quoted 8 lines
first two bytes is used.
Those two called out are special though, here are their meanings:
0x00FFis user programmable function0xFFFFis no functionFor the purpose of memory consumption I think it's reasonable to ignore
the
Quoted 8 lines
Quoted 1 line
upper 2 bytes and special case these two.
Thank you for information!
So
0x00FFis "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?
Quoted 8 lines
Correct
Quoted 3 lines
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...
Quoted 45 lines
Quoted 45 lines
Quoted 45 lines
Quoted 45 lines
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).
Quoted 4 lines
Review static void handle_dmi_entry(const struct
Quoted 4 lines
};
/*
(quoted diff) +0
-0@@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
dmi_header*dm, void *opaque)Quoted 25 lines
drivers/platform/x86/dell-wmi.c static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) · line 510
Quoted 12 lines
drivers/platform/x86/dell-wmi.c +1
-4&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;Quoted 2 lines
Also please fix this:
(no To-header on input) <>Hint: specifying git send-email with '--to' argument instead of '--cc' should help.
Quoted 4 lines
--
~Randy
RE: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to bios_to_linux_keycode
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
And what does mean "no function"? I do not know what should I imagine
if
Quoted 2 lines
Quoted 1 line
I receive key press marked as "no function".
Quoted 9 lines
Quoted 1 line
It means no action is expected to occur, should behave like a no-op. I
think
Quoted 1 line
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.
Quoted 2 lines
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.
Quoted 51 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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"
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(which are probably not found in e.g. Windows or Linux mapping
tables).
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
Review static void handle_dmi_entry(const struct
Quoted 5 lines
};
/*
(quoted diff) +0
-0@@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
dmi_header*dm, void *opaque)Quoted 2 lines
Quoted 2 lines
(quoted diff) +0
-0&table->keymap[i];Quoted 10 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Review static void handle_dmi_entry(const struct · line 507
Quoted 6 lines
(quoted diff) +1
-4/* 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];
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(quoted diff) +0
-0/* * Log if we find an entry in the DMI table that wedon't
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 1 line
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;Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Also please fix this:
(no To-header on input) <>Hint: specifying git send-email with '--to' argument instead of '--
cc'
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
should help.
Quoted 4 lines
--
~Randy
v3 cover platform/x86: dell-wmi: new keys
In reply to Y ParitcherExtended data and events like Fn lock are currently ignored. This is consistent with what was done until now. Changing this is out of scope of this patch and would require rethinking how events are processed, as on some devices the status is sent as it own event, and on some devices via extended data. That is also dependent on better docs from the team at Dell.
The keycode 0xffff look to be a special case and was added as an exception code (Thanks Randy for the implementation). It was not found for any key on my device, it is only located in the DMI table parsed at boot into the keymap.
Overall I am trying to get useless data (to me) out of my syslog by documenting the correct scancode/keycode mappings
Y Paritcher (3):
platform/x86: dell-wmi: add new backlight events platform/x86: dell-wmi: add new keymap type 0x0012 platform/x86: dell-wmi: add new dmi mapping for keycode 0xffff
drivers/platform/x86/dell-wmi.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)
Signature · 1 line
2.27.0
PATCH v3 1/3 platform/x86: dell-wmi: add new backlight events
Add events with a type of 0x0010 and a code of 0x57 / 0x58, this silences the following messages being logged on a Dell Inspiron 5593:
dell_wmi: Unknown key with type 0x0010 and code 0x0057 presseddell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
These are brightness events and will be handled by acpi-video
--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/platform/x86/dell-wmi.c +4 -0
@@ -255,6 +255,10 @@ static const struct key_entry dell_wmi_keymap_type_0010[] = {/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } }, +/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +/* Mic mute */ { KE_KEY, 0x150, { KEY_MICMUTE } },
Signature · 1 line
2.27.0
RE: [PATCH v3 1/3] platform/x86: dell-wmi: add new backlight events
Quoted 17 lines
-----Original Message-----
From: Y Paritcher <[email protected]>
Sent: Monday, June 8, 2020 10:53 PM
To: Pali Rohár
Cc: [email protected]; [email protected]; Matthew Garrett; Limonciello, Mario
Subject: [PATCH v3 1/3] platform/x86: dell-wmi: add new backlight events[EXTERNAL EMAIL]
Add events with a type of
0x0010and a code of0x57/0x58, this silences the following messages being logged on a Dell Inspiron 5593:
Quoted 6 lines
dell_wmi: Unknown key with type0x0010and code0x0057presseddell_wmi: Unknown key with type0x0010and code0x0058pressedThese are brightness events and will be handled by acpi-video
Signed-off-by Y Paritcher <[email protected]>
Quoted 23 lines
--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-wmi.c +0
-0
wmi.c
indexc25a4286d766..0b2edfe2767d100644drivers/platform/x86/dell-wmi.c +4
-1@@ -255,6 +255,10 @@ static const struct key_entry dell_wmi_keymap_type_0010[] = {/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } }, +/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +/* Mic mute */ { KE_KEY, 0x150, { KEY_MICMUTE } }, --2.27.0
PATCH v3 2/3 platform/x86: dell-wmi: add new keymap type 0x0012
In reply to Y ParitcherThese are events with extended data. The extended data is currently ignored as userspace does not have a way to deal it.
Ignore event with a type of 0x0012 and a code of 0xe035, as the keyboard controller takes care of Fn lock events by itself. This silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type 0x12dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
This is consistent with the behavior for the Fn-lock key elsewhere in this file.
--- drivers/platform/x86/dell-wmi.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)
drivers/platform/x86/dell-wmi.c +20 -2
@@ -334,6 +334,15 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +* They are events with extended data +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code) {struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);
@@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device *wdev,switch (buffer_entry[1 ]) {case 0x0000 :/* One key pressed or event occurred */ +case 0x0012 :/* Event with extended data occurred */ if (len >2 ) -dell_wmi_process_key (wdev,0x0000 , +dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[2 ]); -/* Other entries could contain additional information */ +/* Extended data is currently ignored */ break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */
@@ -556,6 +566,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {
@@ -600,6 +611,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI.
Signature · 1 line
2.27.0
PATCH v3 3/3 platform/x86: dell-wmi: add new dmi mapping for keycode 0xffff
In reply to Y ParitcherThis looks to be a special value for some sort of custom scancode. This code could not be triggered for any keypress and is included from the 0xB2 DMI table.
This prevents the following messages from 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 0xffffas 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.
--- drivers/platform/x86/dell-wmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/x86/dell-wmi.c +1 -1
@@ -506,7 +506,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)u16 keycode = (bios_entry->keycode <ARRAY_SIZE (bios_to_linux_keycode)) ? bios_to_linux_keycode[bios_entry->keycode] : -KEY_RESERVED ; + (bios_entry->keycode ==0xffff ?KEY_UNKNOWN :KEY_RESERVED );/* * Log if we find an entry in the DMI table that we don't
Signature · 1 line
2.27.0
Re: [PATCH v3 3/3] platform/x86: dell-wmi: add new dmi mapping for keycode 0xffff
Adding Darren and Andy, please look at this issue as this is something which should be decided how to handle in all platform/wmi drivers.
Quoted 26 lines · On Monday 08 June 2020 23:52:54 Y Paritcher wrote:
This looks to be a special value for some sort of custom scancode. This code could not be triggered for any keypress and is included from the
0xB2DMI table.This prevents the following messages from 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 0xffffas 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.Signed-off-by Y Paritcher <[email protected]>--- drivers/platform/x86/dell-wmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/platform/x86/dell-wmi.c +0
-0
drivers/platform/x86/dell-wmi.c static void handle_dmi_entry(const struct dmi_header *dm, void *opaque) · line 509
Quoted 5 lines
drivers/platform/x86/dell-wmi.c +1
-1@@ -506,7 +506,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)u16 keycode = (bios_entry->keycode <ARRAY_SIZE (bios_to_linux_keycode)) ? bios_to_linux_keycode[bios_entry->keycode] : -KEY_RESERVED ; + (bios_entry->keycode ==0xffff ?KEY_UNKNOWN :KEY_RESERVED );
I really do not like this patch in this form as it is inconsistent with how are these key codes processed. And also because it does not solve any problem. It just hides existence of the problem. And this is something which should be avoided as it makes debugging harder and basically does not bring any value.
To explain it a bit more. We have mapping table bios_to_linux_keycode[] from Dell BIOS key codes to Linux key codes. This mapping table is used for processing Dell WMI buffer of type 0x0010.
You are not able to trigger these events and therefore you and nobody else in this discussion does not know yet what real key triggers this Dell BIOS key code.
So it means that you are proposing to add "bogus" value into bios_to_linux_keycode[] table (just implicitly via check to reduce memory usage, but it does not matter) just because to hide existence of the problem.
This bogus value "KEY_UNKNOWN" is there just as a placeholder because nobody know correct value and nobody was able to trigger it.
Also how it differs situation with unknown keycode "39" which is also missing in the table from yours with keycode "65535" which is also missing in the table?
I think there is no difference, for both values we do not know what is correct mapping to Linux keycodes and therefore value is not present in bios_to_linux_keycode[] table.
If I accept to take this bogus value for keycode "65535", tomorrow can somebody send me a patch which adds bogus value "1000" for the same reason as you as it makes precedense. But it also does not solve any problem.
We would just end up with mess and garbage in bios_to_linux_keycode[] mapping table together with some correct values in table.
So we should ask here two main questions:
1) Is dell-wmi able to process key presses (or events) with scancode 0x48 and keycode 0xffff?
2) If we are not able to process them, what should we do?
Answer for first question is definitely "no".
And answer for second question, I'm not sure, but I thought that throwing warning or info message is the correct solution. Driver cannot handle something, so it inform about it, instead of silently dropping event. Same behavior I'm seeing in other kernel drivers.
But looks like that you and Mario have opposite opinion, that kernel should not log unknown events and rather should drop them.
I would like to have behavior of dell-wmi same as in other drivers for consistency, so the best would be to ask WMI/platform maintainers. They could have opinion how to handle these problem globally.
...
Darren & Andy, could you please say something to this, what do you think about silently dropping events/actions which are currently unknown for dell-wmi driver? It is better to log them or not? Currently we are logging them.
Quoted 6 lines
drivers/platform/x86/dell-wmi.c +0
-0/* * Log if we find an entry in the DMI table that we don't Signature · 1 line
2.27.0
v4 cover platform/x86: dell-wmi: new keys
In reply to Y Paritcherchange since v3:
No code changes.
Update commit message to reflect info given by Mario at dell.
Is there anything more i have to do for the patches that were reviewed or will they be picked up by the maintainers?
Thanks
Y Paritcher (3):
platform/x86: dell-wmi: add new backlight events platform/x86: dell-wmi: add new keymap type 0x0012 platform/x86: dell-wmi: add new dmi mapping for keycode 0xffff
drivers/platform/x86/dell-wmi.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)
Signature · 1 line
2.27.0
PATCH v4 1/3 platform/x86: dell-wmi: add new backlight events
Add events with a type of 0x0010 and a code of 0x57 / 0x58, this silences the following messages being logged on a Dell Inspiron 5593:
dell_wmi: Unknown key with type 0x0010 and code 0x0057 presseddell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
These are brightness events and will be handled by acpi-video
--- drivers/platform/x86/dell-wmi.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/platform/x86/dell-wmi.c +4 -0
@@ -255,6 +255,10 @@ static const struct key_entry dell_wmi_keymap_type_0010[] = {/* Keyboard backlight change notification */ { KE_IGNORE, 0x3f, { KEY_RESERVED } }, +/* Backlight brightness level */ + { KE_KEY, 0x57, { KEY_BRIGHTNESSDOWN } }, + { KE_KEY, 0x58, { KEY_BRIGHTNESSUP } }, +/* Mic mute */ { KE_KEY, 0x150, { KEY_MICMUTE } },
Signature · 1 line
2.27.0
PATCH v4 2/3 platform/x86: dell-wmi: add new keymap type 0x0012
In reply to Y ParitcherThese are events with extended data. The extended data is currently ignored as userspace does not have a way to deal it.
Ignore event with a type of 0x0012 and a code of 0xe035, as the keyboard controller takes care of Fn lock events by itself. This silences the following messages being logged when pressing the Fn-lock key on a Dell Inspiron 5593:
dell_wmi: Unknown WMI event type 0x12dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
This is consistent with the behavior for the Fn-lock key elsewhere in this file.
--- drivers/platform/x86/dell-wmi.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)
drivers/platform/x86/dell-wmi.c +20 -2
@@ -334,6 +334,15 @@ static const struct key_entry dell_wmi_keymap_type_0011[] = { { KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } }, }; +/* +* Keymap for WMI events of type 0x0012 +* They are events with extended data +*/ +static const struct key_entry dell_wmi_keymap_type_0012[] = { +/* Fn-lock button pressed */ + {KE_IGNORE ,0xe035 , {KEY_RESERVED } }, +}; +static void dell_wmi_process_key (struct wmi_device *wdev,int type,int code) {struct dell_wmi_priv *priv =dev_get_drvdata (&wdev->dev);
@@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device *wdev,switch (buffer_entry[1 ]) {case 0x0000 :/* One key pressed or event occurred */ +case 0x0012 :/* Event with extended data occurred */ if (len >2 ) -dell_wmi_process_key (wdev,0x0000 , +dell_wmi_process_key (wdev, buffer_entry[1 ], buffer_entry[2 ]); -/* Other entries could contain additional information */ +/* Extended data is currently ignored */ break ;case 0x0010 :/* Sequence of keys pressed */ case 0x0011 :/* Sequence of events occurred */
@@ -556,6 +566,7 @@ static int dell_wmi_input_setup(struct wmi_device *wdev)ARRAY_SIZE (dell_wmi_keymap_type_0000) +ARRAY_SIZE (dell_wmi_keymap_type_0010) +ARRAY_SIZE (dell_wmi_keymap_type_0011) + +ARRAY_SIZE (dell_wmi_keymap_type_0012) +1 , sizeof(struct key_entry), GFP_KERNEL);if (!keymap) {
@@ -600,6 +611,13 @@ static int dell_wmi_input_setup(struct wmi_device *wdev) pos++; } +/* Append table with events of type 0x0012 */ +for (i =0 ; i <ARRAY_SIZE (dell_wmi_keymap_type_0012); i++) { + keymap[pos] = dell_wmi_keymap_type_0012[i]; + keymap[pos].code |= (0x0012 <<16 ); + pos++; + } +/* * Now append also table with "legacy" events of type 0x0000. Some of * them are reported also on laptops which have scancodes in DMI.
Signature · 1 line
2.27.0
PATCH v4 3/3 platform/x86: dell-wmi: add new dmi mapping for keycode 0xffff
In reply to Y ParitcherThis keycode is used by Dell as a no-op for keys that should have no function.
This keycode is never triggered by a keypress in practice, rather it is included from the 0xB2 DMI table at startup.
This prevents the following messages from 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 0xffffas 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.
--- drivers/platform/x86/dell-wmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/x86/dell-wmi.c +1 -1
@@ -506,7 +506,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)u16 keycode = (bios_entry->keycode <ARRAY_SIZE (bios_to_linux_keycode)) ? bios_to_linux_keycode[bios_entry->keycode] : -KEY_RESERVED ; + (bios_entry->keycode ==0xffff ?KEY_UNKNOWN :KEY_RESERVED );/* * Log if we find an entry in the DMI table that we don't
Signature · 1 line
2.27.0
Quoted 24 lines
-----Original Message-----
From: Y Paritcher <[email protected]>
Sent: Wednesday, June 10, 2020 12:57 PM
To: Pali Rohár
Cc: [email protected]; [email protected]; Matthew Garrett; Limonciello, Mario
Subject: [PATCH v4 0/3] platform/x86: dell-wmi: new keys[EXTERNAL EMAIL]
change since v3:
No code changes.
Update commit message to reflect info given by Mario at dell.Is there anything more i have to do for the patches that were reviewed or will they be picked up by the maintainers?
ThanksY Paritcher (3):
platform/x86: dell-wmi: add new backlight events platform/x86: dell-wmi: add new keymap type0x0012platform/x86: dell-wmi: add new dmi mapping for keycode0xffff
Quoted 5 lines
drivers/platform/x86/dell-wmi.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)--
2.27.0
Andy,
The whole series looks good to me now. You can put this on the patches when they're swooped up.
However I would like to note there was a comment that you had a direct question asked by Pali that probably got lost in the thread. This was on patch 3/3 on v3. I think it's worth answering as it could dictate a follow up patch to change behavior.
The summary of my argument which led to his is nested somewhere in the thread was that to most users this isn't useful since they can't act on it. IE they can't use something like setkeycodes and go on their merry way. The user who could act on it by coming to upstream and submitting questions and patches is more technical and having them use dyndbg to turn on the messages about unknown shouldn't be a big deal.
Quoted 12 lines
I'm not sure, but I thought that
throwing warning or info message is the correct solution. Driver cannot handle something, so it inform about it, instead of silently dropping event. Same behavior I'm seeing in other kernel drivers.But looks like that you and Mario have opposite opinion, that kernel should not log unknown events and rather should drop them.
I would like to have behavior of dell-wmi same as in other drivers for consistency, so the best would be to ask WMI/platform maintainers. They could have opinion how to handle these problem globally.
Quoted 6 lines
...
Darren & Andy, could you please say something to this, what do you think about silently dropping events/actions which are currently unknown for dell-wmi driver? It is better to log them or not? Currently we are logging them.
Can you please advise which way you would rather have the subsystem go?
Quoted 60 lines · On Wed, Jun 10, 2020 at 10:23 PM <[email protected]> wrote:
Quoted 25 lines
-----Original Message-----
From: Y Paritcher <[email protected]>
Sent: Wednesday, June 10, 2020 12:57 PM
To: Pali Rohár
Cc: [email protected]; [email protected]; Matthew Garrett; Limonciello, Mario
Subject: [PATCH v4 0/3] platform/x86: dell-wmi: new keys[EXTERNAL EMAIL]
change since v3:
No code changes.
Update commit message to reflect info given by Mario at dell.Is there anything more i have to do for the patches that were reviewed or will they be picked up by the maintainers?
ThanksY Paritcher (3):
platform/x86: dell-wmi: add new backlight events platform/x86: dell-wmi: add new keymap type0x0012platform/x86: dell-wmi: add new dmi mapping for keycode0xffffQuoted 5 lines
drivers/platform/x86/dell-wmi.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)--
2.27.0Andy,
The whole series looks good to me now. You can put this on the patches when they're swooped up.
Reviewed-by Mario Limonciello <[email protected]>However I would like to note there was a comment that you had a direct question asked by Pali that probably got lost in the thread. This was on patch 3/3 on v3. I think it's worth answering as it could dictate a follow up patch to change behavior.
The summary of my argument which led to his is nested somewhere in the thread was that to most users this isn't useful since they can't act on it. IE they can't use something like setkeycodes and go on their merry way. The user who could act on it by coming to upstream and submitting questions and patches is more technical and having them use dyndbg to turn on the messages about unknown shouldn't be a big deal.
Quoted 11 lines
I'm not sure, but I thought that
throwing warning or info message is the correct solution. Driver cannot handle something, so it inform about it, instead of silently dropping event. Same behavior I'm seeing in other kernel drivers.But looks like that you and Mario have opposite opinion, that kernel should not log unknown events and rather should drop them.
I would like to have behavior of dell-wmi same as in other drivers for consistency, so the best would be to ask WMI/platform maintainers. They could have opinion how to handle these problem globally.
Quoted 8 lines
Quoted 6 lines
...
Darren & Andy, could you please say something to this, what do you think about silently dropping events/actions which are currently unknown for dell-wmi driver? It is better to log them or not? Currently we are logging them.
Can you please advise which way you would rather have the subsystem go?
Seems Pali is okay with this version, so everything is settled I suppose. I will add it to my queue, thanks!
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 68 lines · On Thursday 09 July 2020 22:29:42 Andy Shevchenko wrote:
Quoted 67 lines · On Wed, Jun 10, 2020 at 10:23 PM <[email protected]> wrote:
Quoted 25 lines
-----Original Message-----
From: Y Paritcher <[email protected]>
Sent: Wednesday, June 10, 2020 12:57 PM
To: Pali Rohár
Cc: [email protected]; [email protected]; Matthew Garrett; Limonciello, Mario
Subject: [PATCH v4 0/3] platform/x86: dell-wmi: new keys[EXTERNAL EMAIL]
change since v3:
No code changes.
Update commit message to reflect info given by Mario at dell.Is there anything more i have to do for the patches that were reviewed or will they be picked up by the maintainers?
ThanksY Paritcher (3):
platform/x86: dell-wmi: add new backlight events platform/x86: dell-wmi: add new keymap type0x0012platform/x86: dell-wmi: add new dmi mapping for keycode0xffffQuoted 5 lines
drivers/platform/x86/dell-wmi.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)--
2.27.0Andy,
The whole series looks good to me now. You can put this on the patches when they're swooped up.
Reviewed-by Mario Limonciello <[email protected]>However I would like to note there was a comment that you had a direct question asked by Pali that probably got lost in the thread. This was on patch 3/3 on v3. I think it's worth answering as it could dictate a follow up patch to change behavior.
The summary of my argument which led to his is nested somewhere in the thread was that to most users this isn't useful since they can't act on it. IE they can't use something like setkeycodes and go on their merry way. The user who could act on it by coming to upstream and submitting questions and patches is more technical and having them use dyndbg to turn on the messages about unknown shouldn't be a big deal.
Quoted 18 lines
I'm not sure, but I thought that
throwing warning or info message is the correct solution. Driver cannot handle something, so it inform about it, instead of silently dropping event. Same behavior I'm seeing in other kernel drivers.But looks like that you and Mario have opposite opinion, that kernel should not log unknown events and rather should drop them.
I would like to have behavior of dell-wmi same as in other drivers for consistency, so the best would be to ask WMI/platform maintainers. They could have opinion how to handle these problem globally.
...
Darren & Andy, could you please say something to this, what do you think about silently dropping events/actions which are currently unknown for dell-wmi driver? It is better to log them or not? Currently we are logging them.
Quoted 4 lines
Quoted 1 line
Can you please advise which way you would rather have the subsystem go?
Seems Pali is okay with this version, so everything is settled I suppose. I will add it to my queue, thanks!
Hello Andy! Yes, I'm fine with this patch series, but question how to handle these "unknown" events still remains.
Quoted 69 lines · On Monday 13 July 2020 09:29:12 Pali Rohár wrote:
Quoted 68 lines · On Thursday 09 July 2020 22:29:42 Andy Shevchenko wrote:
Quoted 67 lines · On Wed, Jun 10, 2020 at 10:23 PM <[email protected]> wrote:
Quoted 25 lines
-----Original Message-----
From: Y Paritcher <[email protected]>
Sent: Wednesday, June 10, 2020 12:57 PM
To: Pali Rohár
Cc: [email protected]; [email protected]; Matthew Garrett; Limonciello, Mario
Subject: [PATCH v4 0/3] platform/x86: dell-wmi: new keys[EXTERNAL EMAIL]
change since v3:
No code changes.
Update commit message to reflect info given by Mario at dell.Is there anything more i have to do for the patches that were reviewed or will they be picked up by the maintainers?
ThanksY Paritcher (3):
platform/x86: dell-wmi: add new backlight events platform/x86: dell-wmi: add new keymap type0x0012platform/x86: dell-wmi: add new dmi mapping for keycode0xffffQuoted 5 lines
drivers/platform/x86/dell-wmi.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)--
2.27.0Andy,
The whole series looks good to me now. You can put this on the patches when they're swooped up.
Reviewed-by Mario Limonciello <[email protected]>However I would like to note there was a comment that you had a direct question asked by Pali that probably got lost in the thread. This was on patch 3/3 on v3. I think it's worth answering as it could dictate a follow up patch to change behavior.
The summary of my argument which led to his is nested somewhere in the thread was that to most users this isn't useful since they can't act on it. IE they can't use something like setkeycodes and go on their merry way. The user who could act on it by coming to upstream and submitting questions and patches is more technical and having them use dyndbg to turn on the messages about unknown shouldn't be a big deal.
Quoted 18 lines
I'm not sure, but I thought that
throwing warning or info message is the correct solution. Driver cannot handle something, so it inform about it, instead of silently dropping event. Same behavior I'm seeing in other kernel drivers.But looks like that you and Mario have opposite opinion, that kernel should not log unknown events and rather should drop them.
I would like to have behavior of dell-wmi same as in other drivers for consistency, so the best would be to ask WMI/platform maintainers. They could have opinion how to handle these problem globally.
...
Darren & Andy, could you please say something to this, what do you think about silently dropping events/actions which are currently unknown for dell-wmi driver? It is better to log them or not? Currently we are logging them.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Can you please advise which way you would rather have the subsystem go?
Seems Pali is okay with this version, so everything is settled I suppose. I will add it to my queue, thanks!
Hello Andy! Yes, I'm fine with this patch series, but question how to handle these "unknown" events still remains.
Hello Andy! This question for platform-x86 maintainers is still open. Could you look at it?
Quoted 8 lines · On Wednesday 10 June 2020 13:56:55 Y Paritcher wrote:
change since v3:
No code changes.
Update commit message to reflect info given by Mario at dell.Is there anything more i have to do for the patches that were reviewed or will they be picked up by the maintainers?
Thanks
Quoted 11 lines
Y Paritcher (3):
platform/x86: dell-wmi: add new backlight events platform/x86: dell-wmi: add new keymap type0x0012platform/x86: dell-wmi: add new dmi mapping for keycode0xffffdrivers/platform/x86/dell-wmi.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)Signature · 1 line
2.27.0
I'm fine with this patch series too.