[PATCH 0/3] platform/x86: dell-wmi: new keys

[PATCH 0/3] platform/x86: dell-wmi: new keys

[Y Paritcher]

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

-- 
2.27.0

[PATCH 1/3] platform/x86: dell-wmi: add new backlight events

[Y Paritcher]

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 pressed
dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index c25a4286d766..0b4f72f923cd 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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 } },
 
-- 
2.27.0

Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events

[Pali Rohár]

On Monday 08 June 2020 00:22:24 Y Paritcher wrote:
> 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 pressed
> dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
> 
> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> ---
>  drivers/platform/x86/dell-wmi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index c25a4286d766..0b4f72f923cd 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -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.

>  
> -- 
> 2.27.0
> 

RE: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Pali Rohár
> Sent: Monday, June 8, 2020 3:35 AM
> To: Y Paritcher
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> Matthew Garrett
> Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
> 
> 
> [EXTERNAL EMAIL]
> 
> On Monday 08 June 2020 00:22:24 Y Paritcher wrote:
> > 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 pressed
> > dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
> >
> > Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> > ---
> >  drivers/platform/x86/dell-wmi.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
> wmi.c
> > index c25a4286d766..0b4f72f923cd 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -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 } },
> > +

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.

> >  	/* Keyboard backlight change notification */
> >  	{ KE_IGNORE, 0x3f, { KEY_RESERVED } },
> 
> Please, keep codes sorted.
> 
> >
> > --
> > 2.27.0
> >

Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events

[Y Paritcher]

On 6/8/20 11:30 AM, Mario.Limonciello@dell.com wrote:
>> -----Original Message-----
>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>> owner@vger.kernel.org> On Behalf Of Pali Rohár
>> Sent: Monday, June 8, 2020 3:35 AM
>> To: Y Paritcher
>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> Matthew Garrett
>> Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
>>
>>
>> [EXTERNAL EMAIL]
>>
>> On Monday 08 June 2020 00:22:24 Y Paritcher wrote:
>>> 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 pressed
>>> dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
>>>
>>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
>>> ---
>>>  drivers/platform/x86/dell-wmi.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
>> wmi.c
>>> index c25a4286d766..0b4f72f923cd 100644
>>> --- a/drivers/platform/x86/dell-wmi.c
>>> +++ b/drivers/platform/x86/dell-wmi.c
>>> @@ -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 } },
>>> +
> 
> 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.
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.
> 
>>>  	/* Keyboard backlight change notification */
>>>  	{ KE_IGNORE, 0x3f, { KEY_RESERVED } },
>>
>> Please, keep codes sorted.

Will fix
>>
>>>
>>> --
>>> 2.27.0
>>>

RE: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events

[Mario.Limonciello]

> -----Original Message-----
> From: Y Paritcher <y.linux@paritcher.com>
> Sent: Monday, June 8, 2020 3:12 PM
> To: Limonciello, Mario; pali@kernel.org
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> mjg59@srcf.ucam.org
> Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
> 
> 
> [EXTERNAL EMAIL]
> 
> On 6/8/20 11:30 AM, Mario.Limonciello@dell.com wrote:
> >> -----Original Message-----
> >> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> >> owner@vger.kernel.org> On Behalf Of Pali Rohár
> >> Sent: Monday, June 8, 2020 3:35 AM
> >> To: Y Paritcher
> >> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> >> Matthew Garrett
> >> Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight
> events
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> On Monday 08 June 2020 00:22:24 Y Paritcher wrote:
> >>> 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 pressed
> >>> dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
> >>>
> >>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> >>> ---
> >>>  drivers/platform/x86/dell-wmi.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/drivers/platform/x86/dell-wmi.c
> b/drivers/platform/x86/dell-
> >> wmi.c
> >>> index c25a4286d766..0b4f72f923cd 100644
> >>> --- a/drivers/platform/x86/dell-wmi.c
> >>> +++ b/drivers/platform/x86/dell-wmi.c
> >>> @@ -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 } },
> >>> +
> >
> > 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.
> 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.

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.

> >
> >>>  	/* Keyboard backlight change notification */
> >>>  	{ KE_IGNORE, 0x3f, { KEY_RESERVED } },
> >>
> >> Please, keep codes sorted.
> 
> Will fix
> >>
> >>>
> >>> --
> >>> 2.27.0
> >>>

Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events

[Pali Rohár]

On Monday 08 June 2020 20:14:15 Mario.Limonciello@dell.com wrote:
> > >>> index c25a4286d766..0b4f72f923cd 100644
> > >>> --- a/drivers/platform/x86/dell-wmi.c
> > >>> +++ b/drivers/platform/x86/dell-wmi.c
> > >>> @@ -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 } },
> > >>> +
> > >
> > > 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.
> > 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.
> 
> 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.

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

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár <pali@kernel.org>
> Sent: Monday, June 8, 2020 3:36 PM
> To: Limonciello, Mario
> Cc: y.linux@paritcher.com; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; mjg59@srcf.ucam.org
> Subject: Re: [PATCH 1/3] platform/x86: dell-wmi: add new backlight events
> 
> 
> [EXTERNAL EMAIL]
> 
> On Monday 08 June 2020 20:14:15 Mario.Limonciello@dell.com wrote:
> > > >>> index c25a4286d766..0b4f72f923cd 100644
> > > >>> --- a/drivers/platform/x86/dell-wmi.c
> > > >>> +++ b/drivers/platform/x86/dell-wmi.c
> > > >>> @@ -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 } },
> > > >>> +
> > > >
> > > > 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.
> > > 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.
> >
> > 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.
> 
> 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.

OK thanks, I wasn't aware of this decision elsewhere.

[PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Y Paritcher]

Ignore 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 0x12
dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 0b4f72f923cd..f37e7e9093c2 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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.
-- 
2.27.0

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

Hello!

On Monday 08 June 2020 00:22:25 Y Paritcher wrote:
> Ignore 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:

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.

> dell_wmi: Unknown WMI event type 0x12
> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed

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.

> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> ---
>  drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 0b4f72f923cd..f37e7e9093c2 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -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 */

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.

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

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Y Paritcher]

On 6/8/20 4:50 AM, Pali Rohár wrote:
> Hello!
> 
> On Monday 08 June 2020 00:22:25 Y Paritcher wrote:
>> Ignore 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:
> 
> 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.
>> dell_wmi: Unknown WMI event type 0x12
>> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
> 
> 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.
> 

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.
>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
>> ---
>>  drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> index 0b4f72f923cd..f37e7e9093c2 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -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 */
> 
> 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
>>  			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.
>> -- 
>> 2.27.0
>>

RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Y Paritcher
> Sent: Sunday, June 7, 2020 11:22 PM
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> Matthew Garrett; Pali Rohár
> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
> 
> 
> [EXTERNAL EMAIL]
> 
> Ignore 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 0x12
> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed

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.

> 
> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> ---
>  drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
> wmi.c
> index 0b4f72f923cd..f37e7e9093c2 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -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.
> --
> 2.27.0

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Y Paritcher]

On 6/8/20 11:40 AM, Mario.Limonciello@dell.com wrote:
>> -----Original Message-----
>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>> owner@vger.kernel.org> On Behalf Of Y Paritcher
>> Sent: Sunday, June 7, 2020 11:22 PM
>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> Matthew Garrett; Pali Rohár
>> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Ignore 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 0x12
>> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
> 
> 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.
> 

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

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

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.
>>
>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
>> ---
>>  drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
>> wmi.c
>> index 0b4f72f923cd..f37e7e9093c2 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -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.
>> --
>> 2.27.0
> 

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

Hello!

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

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.

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

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.

> 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

[Mario.Limonciello]

> -----Original Message-----
> From: Y Paritcher <y.linux@paritcher.com>
> Sent: Monday, June 8, 2020 3:13 PM
> To: Limonciello, Mario
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> mjg59@srcf.ucam.org; pali@kernel.org
> Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
> 
> 
> [EXTERNAL EMAIL]
> 
> On 6/8/20 11:40 AM, Mario.Limonciello@dell.com wrote:
> >> -----Original Message-----
> >> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> >> owner@vger.kernel.org> On Behalf Of Y Paritcher
> >> Sent: Sunday, June 7, 2020 11:22 PM
> >> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> >> Matthew Garrett; Pali Rohár
> >> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> Ignore 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 0x12
> >> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
> >
> > 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.
> >
> 
> 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
> 
> 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.

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

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

> >>
> >> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> >> ---
> >>  drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/dell-wmi.c
> b/drivers/platform/x86/dell-
> >> wmi.c
> >> index 0b4f72f923cd..f37e7e9093c2 100644
> >> --- a/drivers/platform/x86/dell-wmi.c
> >> +++ b/drivers/platform/x86/dell-wmi.c
> >> @@ -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.
> >> --
> >> 2.27.0
> >

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Y Paritcher]

On 6/8/20 4:36 PM, Mario.Limonciello@dell.com wrote:
>> -----Original Message-----
>> From: Y Paritcher <y.linux@paritcher.com>
>> Sent: Monday, June 8, 2020 3:13 PM
>> To: Limonciello, Mario
>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> mjg59@srcf.ucam.org; pali@kernel.org
>> Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
>>
>>
>> [EXTERNAL EMAIL]
>>
>> On 6/8/20 11:40 AM, Mario.Limonciello@dell.com wrote:
>>>> -----Original Message-----
>>>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>>>> owner@vger.kernel.org> On Behalf Of Y Paritcher
>>>> Sent: Sunday, June 7, 2020 11:22 PM
>>>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>>>> Matthew Garrett; Pali Rohár
>>>> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
>>>>
>>>>
>>>> [EXTERNAL EMAIL]
>>>>
>>>> Ignore 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 0x12
>>>> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
>>>
>>> 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.
>>>
>>
>> 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
>>
>> 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.
> 

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
>>
>> 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.
> 
> 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
> 
>>>>
>>>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
>>>> ---
>>>>  drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
>>>>  1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/dell-wmi.c
>> b/drivers/platform/x86/dell-
>>>> wmi.c
>>>> index 0b4f72f923cd..f37e7e9093c2 100644
>>>> --- a/drivers/platform/x86/dell-wmi.c
>>>> +++ b/drivers/platform/x86/dell-wmi.c
>>>> @@ -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.
>>>> --
>>>> 2.27.0
>>>

RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Mario.Limonciello]

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

[Y Paritcher]

On 6/8/20 6:00 PM, Mario.Limonciello@dell.com wrote:
>>
>> 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
>>>>
>>>> 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?
> 

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

[Hans de Goede]

Hi,

On 6/8/20 10:36 PM, Mario.Limonciello@dell.com wrote:
>> -----Original Message-----
>> From: Y Paritcher <y.linux@paritcher.com>
>> Sent: Monday, June 8, 2020 3:13 PM
>> To: Limonciello, Mario
>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> mjg59@srcf.ucam.org; pali@kernel.org
>> Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
>>
>>
>> [EXTERNAL EMAIL]
>>
>> On 6/8/20 11:40 AM, Mario.Limonciello@dell.com wrote:
>>>> -----Original Message-----
>>>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>>>> owner@vger.kernel.org> On Behalf Of Y Paritcher
>>>> Sent: Sunday, June 7, 2020 11:22 PM
>>>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>>>> Matthew Garrett; Pali Rohár
>>>> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
>>>>
>>>>
>>>> [EXTERNAL EMAIL]
>>>>
>>>> Ignore 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 0x12
>>>> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
>>>
>>> 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.
>>>
>>
>> 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
>>
>> 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.
> 
>>
>> 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.
> 
> 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

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










> 
>>>>
>>>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
>>>> ---
>>>>   drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
>>>>   1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/dell-wmi.c
>> b/drivers/platform/x86/dell-
>>>> wmi.c
>>>> index 0b4f72f923cd..f37e7e9093c2 100644
>>>> --- a/drivers/platform/x86/dell-wmi.c
>>>> +++ b/drivers/platform/x86/dell-wmi.c
>>>> @@ -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.
>>>> --
>>>> 2.27.0
>>>

RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Mario.Limonciello]

Loop linux-input mailing list and trim to the relevant conversation.

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

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

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

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.

> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> >
> >>>>
> >>>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> >>>> ---
> >>>>   drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
> >>>>   1 file changed, 17 insertions(+)
> >>>>
> >>>> diff --git a/drivers/platform/x86/dell-wmi.c
> >> b/drivers/platform/x86/dell-
> >>>> wmi.c
> >>>> index 0b4f72f923cd..f37e7e9093c2 100644
> >>>> --- a/drivers/platform/x86/dell-wmi.c
> >>>> +++ b/drivers/platform/x86/dell-wmi.c
> >>>> @@ -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.
> >>>> --
> >>>> 2.27.0
> >>>

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Hans de Goede]

Hi,

On 6/9/20 5:36 PM, Mario.Limonciello@dell.com wrote:
> Loop linux-input mailing list and trim to the relevant conversation.
> 
>>> 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.
> 
> 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.

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

[Mario.Limonciello]

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

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

[Pali Rohár]

On Monday 08 June 2020 20:36:58 Mario.Limonciello@dell.com 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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]

Hi,

On Tue, Jun 09, 2020 at 05:49:38PM +0200, Pali Rohár wrote:
> On Monday 08 June 2020 20:36:58 Mario.Limonciello@dell.com 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.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Hans de Goede]

Hi Sebastian,

On 6/9/20 6:45 PM, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Jun 09, 2020 at 05:49:38PM +0200, Pali Rohár wrote:
>> On Monday 08 June 2020 20:36:58 Mario.Limonciello@dell.com 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.
> 
> 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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3620 bytes --]

Hi,

On Tue, Jun 09, 2020 at 06:59:13PM +0200, Hans de Goede wrote:
> Hi Sebastian,
> 
> On 6/9/20 6:45 PM, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Tue, Jun 09, 2020 at 05:49:38PM +0200, Pali Rohár wrote:
> > > On Monday 08 June 2020 20:36:58 Mario.Limonciello@dell.com 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.
> > 
> > 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.

Ok. So how much information is available over WMI? Exposing the
max. input power via the power-supply API makes sense in any case.

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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Mario.Limonciello]

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

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

[Pali Rohár]

On Monday 08 June 2020 15:40:53 Mario.Limonciello@dell.com wrote:
> > dell_wmi: Unknown WMI event type 0x12
> > dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
> 
> 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.

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

[Y Paritcher]

Increase length of bios_to_linux_keycode to 2 bytes to allow for a new
keycode 0xffff, this silences the following messages being logged at
startup on a Dell Inspiron 5593

dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index f37e7e9093c2..5ef716e3034f 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -196,7 +196,7 @@ struct dell_dmi_results {
 };
 
 /* Uninitialized entries here are KEY_RESERVED == 0. */
-static const u16 bios_to_linux_keycode[256] = {
+static const u16 bios_to_linux_keycode[65536] = {
 	[0]	= KEY_MEDIA,
 	[1]	= KEY_NEXTSONG,
 	[2]	= KEY_PLAYPAUSE,
@@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
 	[37]	= KEY_UNKNOWN,
 	[38]	= KEY_MICMUTE,
 	[255]	= KEY_PROG3,
+	[65535]	= KEY_UNKNOWN,
 };
 
 /*
-- 
2.27.0

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7444 bytes --]

Hi Paritcher,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on platform-drivers-x86/for-next linus/master linux/master v5.7 next-20200605]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Y-Paritcher/platform-x86-dell-wmi-new-keys/20200608-122408
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 22a1c800c96c83b7f4e3e02fad767502b70124fa
config: i386-randconfig-s002-20200608 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-247-gcadbd124-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 ARCH=i386 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

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 <luto@kernel.org>
:::::: CC: Darren Hart <dvhart@linux.intel.com>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39047 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7706 bytes --]

Hi Paritcher,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on platform-drivers-x86/for-next linus/master v5.7 next-20200605]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Y-Paritcher/platform-x86-dell-wmi-new-keys/20200608-122408
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 22a1c800c96c83b7f4e3e02fad767502b70124fa
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

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 <luto@kernel.org>
:::::: CC: Darren Hart <dvhart@linux.intel.com>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 74448 bytes --]

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

[Pali Rohár]

Hello!

On Monday 08 June 2020 00:22:26 Y Paritcher wrote:
> Increase length of bios_to_linux_keycode to 2 bytes to allow for a new
> keycode 0xffff, this silences the following messages being logged at
> startup on a Dell Inspiron 5593
> 
> dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
> dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff
> 
> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> ---
>  drivers/platform/x86/dell-wmi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index f37e7e9093c2..5ef716e3034f 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -196,7 +196,7 @@ struct dell_dmi_results {
>  };
>  
>  /* Uninitialized entries here are KEY_RESERVED == 0. */
> -static const u16 bios_to_linux_keycode[256] = {
> +static const u16 bios_to_linux_keycode[65536] = {

This change dramatically increase memory usage. I guess other that
maintainers would not like such change.

>  	[0]	= KEY_MEDIA,
>  	[1]	= KEY_NEXTSONG,
>  	[2]	= KEY_PLAYPAUSE,
> @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
>  	[37]	= KEY_UNKNOWN,
>  	[38]	= KEY_MICMUTE,
>  	[255]	= KEY_PROG3,
> +	[65535]	= KEY_UNKNOWN,

Also it seems that this change is not complete. It looks like that you
map two different scancodes (0x48 and 0x50) to same keycodes, moreover
both are unknown.

Could you please describe which key presses (or events) generate
delivering these WMI scancode events?

Note that purpose of printing unknown/unrecognized keys messages is to
inform that current pressed key was not processed or that it was
ignored.

For me it looks like this just just hide information that key was not
processed correctly as this change does not implement correct processing
of this key.

Also, could you share documentation about these 0x48/0x50 events? Or it
is under NDA?

>  };
>  
>  /*
> -- 
> 2.27.0
> 

RE: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Pali Rohár
> Sent: Monday, June 8, 2020 4:00 AM
> To: Y Paritcher
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> Matthew Garrett
> Subject: Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to
> bios_to_linux_keycode
> 
> 
> [EXTERNAL EMAIL]
> 
> Hello!
> 
> On Monday 08 June 2020 00:22:26 Y Paritcher wrote:
> > Increase length of bios_to_linux_keycode to 2 bytes to allow for a new
> > keycode 0xffff, this silences the following messages being logged at
> > startup on a Dell Inspiron 5593

Which event type?  Can you show the whole WMI buffer that came through?

> >
> > dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
> > dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff
> >
> > Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> > ---
> >  drivers/platform/x86/dell-wmi.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
> wmi.c
> > index f37e7e9093c2..5ef716e3034f 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -196,7 +196,7 @@ struct dell_dmi_results {
> >  };
> >
> >  /* Uninitialized entries here are KEY_RESERVED == 0. */
> > -static const u16 bios_to_linux_keycode[256] = {
> > +static const u16 bios_to_linux_keycode[65536] = {
> 
> This change dramatically increase memory usage. I guess other that
> maintainers would not like such change.
> 
> >  	[0]	= KEY_MEDIA,
> >  	[1]	= KEY_NEXTSONG,
> >  	[2]	= KEY_PLAYPAUSE,
> > @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
> >  	[37]	= KEY_UNKNOWN,
> >  	[38]	= KEY_MICMUTE,
> >  	[255]	= KEY_PROG3,
> > +	[65535]	= KEY_UNKNOWN,
> 
> Also it seems that this change is not complete. It looks like that you
> map two different scancodes (0x48 and 0x50) to same keycodes, moreover
> both are unknown.
> 
> Could you please describe which key presses (or events) generate
> delivering these WMI scancode events?
> 
> Note that purpose of printing unknown/unrecognized keys messages is to
> inform that current pressed key was not processed or that it was
> ignored.
> 
> For me it looks like this just just hide information that key was not
> processed correctly as this change does not implement correct processing
> of this key.
> 
> Also, could you share documentation about these 0x48/0x50 events? Or it
> is under NDA?
> 
> >  };
> >
> >  /*
> > --
> > 2.27.0
> >

I would actually question if there is value to lines in dell-wmi.c like this:

pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);

and

pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n", type, code);

In both of those cases the information doesn't actually help the user, by default it's
ignored by the driver anyway.  It just notifies the user it's something the driver doesn't
comprehend.  I would think these are better suited to downgrade to debug.  And if
a key combination isn't doing something expected the user can use dyndbg to turn it
back on and can be debugged what should be populated or "explicitly" ignored.

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

[Y Paritcher]

On 6/8/20 11:46 AM, Mario.Limonciello@dell.com wrote:
>> -----Original Message-----
>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>> owner@vger.kernel.org> On Behalf Of Pali Rohár
>> Sent: Monday, June 8, 2020 4:00 AM
>> To: Y Paritcher
>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> Matthew Garrett
>> Subject: Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to
>> bios_to_linux_keycode
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Hello!
>>
>> On Monday 08 June 2020 00:22:26 Y Paritcher wrote:
>>> Increase length of bios_to_linux_keycode to 2 bytes to allow for a new
>>> keycode 0xffff, this silences the following messages being logged at
>>> startup on a Dell Inspiron 5593
> 
> Which event type?  Can you show the whole WMI buffer that came through?
> 
>>>
>>> dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
>>> dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff
>>>
>>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
>>> ---
>>>  drivers/platform/x86/dell-wmi.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
>> wmi.c
>>> index f37e7e9093c2..5ef716e3034f 100644
>>> --- a/drivers/platform/x86/dell-wmi.c
>>> +++ b/drivers/platform/x86/dell-wmi.c
>>> @@ -196,7 +196,7 @@ struct dell_dmi_results {
>>>  };
>>>
>>>  /* Uninitialized entries here are KEY_RESERVED == 0. */
>>> -static const u16 bios_to_linux_keycode[256] = {
>>> +static const u16 bios_to_linux_keycode[65536] = {
>>
>> This change dramatically increase memory usage. I guess other that
>> maintainers would not like such change.
>>
>>>  	[0]	= KEY_MEDIA,
>>>  	[1]	= KEY_NEXTSONG,
>>>  	[2]	= KEY_PLAYPAUSE,
>>> @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
>>>  	[37]	= KEY_UNKNOWN,
>>>  	[38]	= KEY_MICMUTE,
>>>  	[255]	= KEY_PROG3,
>>> +	[65535]	= KEY_UNKNOWN,
>>
>> Also it seems that this change is not complete. It looks like that you
>> map two different scancodes (0x48 and 0x50) to same keycodes, moreover
>> both are unknown.
>>
>> Could you please describe which key presses (or events) generate
>> delivering these WMI scancode events?
>>
>> Note that purpose of printing unknown/unrecognized keys messages is to
>> inform that current pressed key was not processed or that it was
>> ignored.
>>
>> For me it looks like this just just hide information that key was not
>> processed correctly as this change does not implement correct processing
>> of this key.


The dell_wmi: firmware scancode XXXX maps to unrecognized keycode XXXX
events are emitted at startup when processing DMI table entries that dell-wmi
does not recognize.

my DMI table contains among others:
48 00 FF FF 
50 00 FF FF

the scan/keycodes are 2 bytes the array was only 1 byte until now as there were
never any reported keycodes with the higher byte set.


unlike my other patches this one is not for key press events. None of the keys on my laptop correspond to these scancode/keycode.

the reason for this type of log event is in a comment:

/*
 * Log if we find an entry in the DMI table that we don't
 * understand.  If this happens, we should figure out what
 * the entry means and add it to bios_to_linux_keycode.
 */

therefore I tried adding this to the list of known keycodes.
As of now almost half of the keycodes in the list are KEY_UNKNOWN.

If anyone has a way of figuring out how to map this to a specific key,
i will try to identify it further.


>>
>> Also, could you share documentation about these 0x48/0x50 events? Or it
>> is under NDA?
>>

I am just a regular user. I have no clue what they are nor any inside info.
>>>  };
>>>
>>>  /*
>>> --
>>> 2.27.0
>>>
> 
> I would actually question if there is value to lines in dell-wmi.c like this:
> 
> pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);
> 
> and
> 
> pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n", type, code);
> 
> In both of those cases the information doesn't actually help the user, by default it's
> ignored by the driver anyway.  It just notifies the user it's something the driver doesn't
> comprehend.  I would think these are better suited to downgrade to debug.  And if
> a key combination isn't doing something expected the user can use dyndbg to turn it
> back on and can be debugged what should be populated or "explicitly" ignored.
> 

I agree with this. The only reason i am adding these definitions is to get rid of 
annoying log messages. They should really be debug. 

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

[Pali Rohár]

On Monday 08 June 2020 15:46:44 Mario.Limonciello@dell.com wrote:
> I would actually question if there is value to lines in dell-wmi.c like this:
> 
> pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);
> 
> and
> 
> pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n", type, code);
> 
> In both of those cases the information doesn't actually help the user, by default it's
> ignored by the driver anyway.  It just notifies the user it's something the driver doesn't
> comprehend.  I would think these are better suited to downgrade to debug.  And if
> a key combination isn't doing something expected the user can use dyndbg to turn it
> back on and can be debugged what should be populated or "explicitly" ignored.

My motivation for these messages was to provide information to user that
kernel received event, but was not able to process it as it do not
understand it.

It could help in situation when user press special key and nothing is
delivered to userspace. But he could see that something happened in log.

Similar message is also printed by PS/2 keyboard driver atkbd.c:

	case ATKBD_KEY_UNKNOWN:
		dev_warn(&serio->dev,
			 "Unknown key %s (%s set %d, code %#x on %s).\n",
			 atkbd->release ? "released" : "pressed",
			 atkbd->translated ? "translated" : "raw",
			 atkbd->set, code, serio->phys);
		dev_warn(&serio->dev,
			 "Use 'setkeycodes %s%02x <keycode>' to make it known.\n",
			 code & 0x80 ? "e0" : "", code & 0x7f);
		input_sync(dev);
		break;

RE: [PATCH 3/3] platform/x86: dell-wmi: add keys to bios_to_linux_keycode

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Pali Rohár
> Sent: Monday, June 8, 2020 3:48 PM
> To: Limonciello, Mario
> Cc: y.linux@paritcher.com; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; mjg59@srcf.ucam.org
> Subject: Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to
> bios_to_linux_keycode
> 
> 
> [EXTERNAL EMAIL]
> 
> On Monday 08 June 2020 15:46:44 Mario.Limonciello@dell.com wrote:
> > I would actually question if there is value to lines in dell-wmi.c like
> this:
> >
> > pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);
> >
> > and
> >
> > pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n", type,
> code);
> >
> > In both of those cases the information doesn't actually help the user, by
> default it's
> > ignored by the driver anyway.  It just notifies the user it's something
> the driver doesn't
> > comprehend.  I would think these are better suited to downgrade to debug.
> And if
> > a key combination isn't doing something expected the user can use dyndbg
> to turn it
> > back on and can be debugged what should be populated or "explicitly"
> ignored.
> 
> My motivation for these messages was to provide information to user that
> kernel received event, but was not able to process it as it do not
> understand it.
> 
> It could help in situation when user press special key and nothing is
> delivered to userspace. But he could see that something happened in log.
> 

But does a user know what to do with this information?  From time to time
coming to kernel mailing list, but that's it.

I think same person who would know to come to kernel mailing list for a key
not working can likely also hand turning on dyndbg to get the info.

> Similar message is also printed by PS/2 keyboard driver atkbd.c:
> 
> 	case ATKBD_KEY_UNKNOWN:
> 		dev_warn(&serio->dev,
> 			 "Unknown key %s (%s set %d, code %#x on %s).\n",
> 			 atkbd->release ? "released" : "pressed",
> 			 atkbd->translated ? "translated" : "raw",
> 			 atkbd->set, code, serio->phys);
> 		dev_warn(&serio->dev,
> 			 "Use 'setkeycodes %s%02x <keycode>' to make it known.\n",
> 			 code & 0x80 ? "e0" : "", code & 0x7f);
> 		input_sync(dev);
> 		break;

I think the difference here is that user can actually do something from userland
to do with `setkeycodes` for PS2.

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

[Pali Rohár]

On Monday 08 June 2020 20:58:38 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> > owner@vger.kernel.org> On Behalf Of Pali Rohár
> > Sent: Monday, June 8, 2020 3:48 PM
> > To: Limonciello, Mario
> > Cc: y.linux@paritcher.com; linux-kernel@vger.kernel.org; platform-driver-
> > x86@vger.kernel.org; mjg59@srcf.ucam.org
> > Subject: Re: [PATCH 3/3] platform/x86: dell-wmi: add keys to
> > bios_to_linux_keycode
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > On Monday 08 June 2020 15:46:44 Mario.Limonciello@dell.com wrote:
> > > I would actually question if there is value to lines in dell-wmi.c like
> > this:
> > >
> > > pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);
> > >
> > > and
> > >
> > > pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n", type,
> > code);
> > >
> > > In both of those cases the information doesn't actually help the user, by
> > default it's
> > > ignored by the driver anyway.  It just notifies the user it's something
> > the driver doesn't
> > > comprehend.  I would think these are better suited to downgrade to debug.
> > And if
> > > a key combination isn't doing something expected the user can use dyndbg
> > to turn it
> > > back on and can be debugged what should be populated or "explicitly"
> > ignored.
> > 
> > My motivation for these messages was to provide information to user that
> > kernel received event, but was not able to process it as it do not
> > understand it.
> > 
> > It could help in situation when user press special key and nothing is
> > delivered to userspace. But he could see that something happened in log.
> > 
> 
> But does a user know what to do with this information?  From time to time
> coming to kernel mailing list, but that's it.

That is a good question. I'm really not sure if user can do anything
with it. But also users do not care about these kind of logs. So
probably even do not know about them.

What is nice in this solution that if you want to try "debug" such
problem you just need to ask user for logs. Nothing is needed to enabled
/ disable.

> I think same person who would know to come to kernel mailing list for a key
> not working can likely also hand turning on dyndbg to get the info.

You are probably right.

In past I did one thing thanks to these logs. I searched for these log
messages on interned. More results were on forum discussions. I tried to
contact users of those posts and I think 3-4 people wrote me back with
details which allowed me to extend dell-wmi driver to handle additional
key codes.

So I see two benefits from these logs: 1) no special setup is needed to
gather these logs (useful for non-power users) and 2) ability to search
on internet if we have laptops which generates such unknown key codes
and users are "complaining" or posting their logs for investigation on
places where are no kernel developers available.

So question is if these two points are reason why to stick with these
logs or turn them off by default.

I still think it can be useful.

> > Similar message is also printed by PS/2 keyboard driver atkbd.c:
> > 
> > 	case ATKBD_KEY_UNKNOWN:
> > 		dev_warn(&serio->dev,
> > 			 "Unknown key %s (%s set %d, code %#x on %s).\n",
> > 			 atkbd->release ? "released" : "pressed",
> > 			 atkbd->translated ? "translated" : "raw",
> > 			 atkbd->set, code, serio->phys);
> > 		dev_warn(&serio->dev,
> > 			 "Use 'setkeycodes %s%02x <keycode>' to make it known.\n",
> > 			 code & 0x80 ? "e0" : "", code & 0x7f);
> > 		input_sync(dev);
> > 		break;
> 
> I think the difference here is that user can actually do something from userland
> to do with `setkeycodes` for PS2.

Of course, I agree.

[PATCH v2 0/3] platform/x86: dell-wmi: new keys

[Y Paritcher]

As 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(-)

-- 
2.27.0

[PATCH v2 1/3] platform/x86: dell-wmi: add new backlight events

[Y Paritcher]

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 pressed
dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed

These are brightness events and will be handled by acpi-video

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index c25a4286d766..0b2edfe2767d 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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

Re: [PATCH v2 1/3] platform/x86: dell-wmi: add new backlight events

[Pali Rohár]

On Monday 08 June 2020 19:05:28 Y Paritcher wrote:
> 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 pressed
> dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
> 
> These are brightness events and will be handled by acpi-video
> 
> Signed-off-by: Y Paritcher <y.linux@paritcher.com>

Reviewed-by: Pali Rohár <pali@kernel.org>

> ---
>  drivers/platform/x86/dell-wmi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index c25a4286d766..0b2edfe2767d 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -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 v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Y Paritcher]

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 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 0x12
dell_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.

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 0b2edfe2767d..6b510f8431a3 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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.
-- 
2.27.0

Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

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 0x0012 and a code of 0xe035, 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.

> This silences the following messages being logged when
> pressing the Fn-lock key on a Dell Inspiron 5593:
> 
> dell_wmi: Unknown WMI event type 0x12
> dell_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.
> 
> Signed-off-by: Y Paritcher <y.linux@paritcher.com>

I'm fine with this patch now.

Reviewed-by: Pali Rohár <pali@kernel.org>

> ---
>  drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 0b2edfe2767d..6b510f8431a3 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -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 */

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

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

RE: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár <pali@kernel.org>
> Sent: Monday, June 8, 2020 6:33 PM
> To: Y Paritcher
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> Matthew Garrett; Limonciello, Mario
> Subject: Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type
> 0x0012
> 
> 
> [EXTERNAL EMAIL]
> 
> 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 0x0012 and a code of 0xe035, 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.
> 
> > This silences the following messages being logged when
> > pressing the Fn-lock key on a Dell Inspiron 5593:
> >
> > dell_wmi: Unknown WMI event type 0x12
> > dell_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.
> >
> > Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> 
> I'm fine with this patch now.
> 
> Reviewed-by: Pali Rohár <pali@kernel.org>
> 
> > ---
> >  drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
> wmi.c
> > index 0b2edfe2767d..6b510f8431a3 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -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 */

I don't really like this being handled as a key as it's just discarding all
that extended data.

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

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.

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

Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Y Paritcher]

On 6/8/20 8:26 PM, Mario.Limonciello@dell.com wrote:
>> -----Original Message-----
>> From: Pali Rohár <pali@kernel.org>
>> Sent: Monday, June 8, 2020 6:33 PM
>> To: Y Paritcher
>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> Matthew Garrett; Limonciello, Mario
>> Subject: Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type
>> 0x0012
>>
>>
>> [EXTERNAL EMAIL]
>>
>> 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 0x0012 and a code of 0xe035, 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.
>>
>>> This silences the following messages being logged when
>>> pressing the Fn-lock key on a Dell Inspiron 5593:
>>>
>>> dell_wmi: Unknown WMI event type 0x12
>>> dell_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.
>>>
>>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
>>
>> I'm fine with this patch now.
>>
>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>
>>> ---
>>>  drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++-
>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
>> wmi.c
>>> index 0b2edfe2767d..6b510f8431a3 100644
>>> --- a/drivers/platform/x86/dell-wmi.c
>>> +++ b/drivers/platform/x86/dell-wmi.c
>>> @@ -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 */
> 
> I don't really like this being handled as a key as it's just discarding all
> that extended data.
> 
>>
>> 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...)
> 
> 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.
> 

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

Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

On Monday 08 June 2020 20:57:38 Y Paritcher wrote:
> On 6/8/20 8:26 PM, Mario.Limonciello@dell.com wrote:
> >> -----Original Message-----
> >> From: Pali Rohár <pali@kernel.org>
> >> Sent: Monday, June 8, 2020 6:33 PM
> >> To: Y Paritcher
> >> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> >> Matthew Garrett; Limonciello, Mario
> >> Subject: Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type
> >> 0x0012
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> 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 0x0012 and a code of 0xe035, 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.
> >>
> >>> This silences the following messages being logged when
> >>> pressing the Fn-lock key on a Dell Inspiron 5593:
> >>>
> >>> dell_wmi: Unknown WMI event type 0x12
> >>> dell_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.
> >>>
> >>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> >>
> >> I'm fine with this patch now.
> >>
> >> Reviewed-by: Pali Rohár <pali@kernel.org>
> >>
> >>> ---
> >>>  drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++-
> >>>  1 file changed, 19 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
> >> wmi.c
> >>> index 0b2edfe2767d..6b510f8431a3 100644
> >>> --- a/drivers/platform/x86/dell-wmi.c
> >>> +++ b/drivers/platform/x86/dell-wmi.c
> >>> @@ -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 */
> > 
> > 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...).

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

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

Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Pali Rohár]

On Tuesday 09 June 2020 00:26:45 Mario.Limonciello@dell.com wrote:
> > 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...)
> 
> 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.

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

[Y Paritcher]

Increase length of bios_to_linux_keycode to 2 bytes (the true size of a
keycode) to allow for a new keycode 0xffff, this silences the following
messages being logged at startup on a Dell Inspiron 5593:

    dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
    dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff

as per this code comment:

   Log if we find an entry in the DMI table that we don't
   understand.  If this happens, we should figure out what
   the entry means and add it to bios_to_linux_keycode.

These are keycodes included in the 0xB2 DMI table, for which the
corosponding keys are not known.

Now when a user will encounter this key, a proper message wil be printed:

    dell_wmi: Unknown key with type 0xXXXX and code 0xXXXX pressed

This will then allow the key to be identified properly.

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 6b510f8431a3..dae1db96b5a0 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -196,7 +196,7 @@ struct dell_dmi_results {
 };
 
 /* Uninitialized entries here are KEY_RESERVED == 0. */
-static const u16 bios_to_linux_keycode[256] = {
+static const u16 bios_to_linux_keycode[65536] = {
 	[0]	= KEY_MEDIA,
 	[1]	= KEY_NEXTSONG,
 	[2]	= KEY_PLAYPAUSE,
@@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
 	[37]	= KEY_UNKNOWN,
 	[38]	= KEY_MICMUTE,
 	[255]	= KEY_PROG3,
+	[65535]	= KEY_UNKNOWN,
 };
 
 /*
@@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)
 					&table->keymap[i];
 
 		/* Uninitialized entries are 0 aka KEY_RESERVED. */
-		u16 keycode = (bios_entry->keycode <
-			       ARRAY_SIZE(bios_to_linux_keycode)) ?
-			bios_to_linux_keycode[bios_entry->keycode] :
-			KEY_RESERVED;
+		u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
 
 		/*
 		 * Log if we find an entry in the DMI table that we don't
-- 
2.27.0

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

[Randy Dunlap]

Hi--

On 6/8/20 4:05 PM, Y Paritcher wrote:
> Increase length of bios_to_linux_keycode to 2 bytes (the true size of a
> keycode) to allow for a new keycode 0xffff, this silences the following
> messages being logged at startup on a Dell Inspiron 5593:
> 
>     dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
>     dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff
> 
> as per this code comment:
> 
>    Log if we find an entry in the DMI table that we don't
>    understand.  If this happens, we should figure out what
>    the entry means and add it to bios_to_linux_keycode.
> 
> These are keycodes included in the 0xB2 DMI table, for which the
> corosponding keys are not known.

  corresponding

> 
> Now when a user will encounter this key, a proper message wil be printed:
> 
>     dell_wmi: Unknown key with type 0xXXXX and code 0xXXXX pressed
> 
> This will then allow the key to be identified properly.
> 
> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> ---
>  drivers/platform/x86/dell-wmi.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 6b510f8431a3..dae1db96b5a0 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -196,7 +196,7 @@ struct dell_dmi_results {
>  };
>  
>  /* Uninitialized entries here are KEY_RESERVED == 0. */
> -static const u16 bios_to_linux_keycode[256] = {
> +static const u16 bios_to_linux_keycode[65536] = {

It surely seems odd to me to expand an array from 512 bytes to 128 Kbytes
just to handle one special case.  Can't it be handled in code as a
special case?

>  	[0]	= KEY_MEDIA,
>  	[1]	= KEY_NEXTSONG,
>  	[2]	= KEY_PLAYPAUSE,
> @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
>  	[37]	= KEY_UNKNOWN,
>  	[38]	= KEY_MICMUTE,
>  	[255]	= KEY_PROG3,
> +	[65535]	= KEY_UNKNOWN,
>  };
>  
>  /*
> @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)
>  					&table->keymap[i];
>  
>  		/* Uninitialized entries are 0 aka KEY_RESERVED. */
> -		u16 keycode = (bios_entry->keycode <
> -			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> -			bios_to_linux_keycode[bios_entry->keycode] :
> -			KEY_RESERVED;
> +		u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
>  
>  		/*
>  		 * Log if we find an entry in the DMI table that we don't
> 

Something like:

		u16 keycode;

		keycode = bios_entry->keycode == 0xffff ? KEY_UNKNOWN :
			(bios_entry->keycode <
			       ARRAY_SIZE(bios_to_linux_keycode)) ?
			bios_to_linux_keycode[bios_entry->keycode] :
			KEY_RESERVED;



Also please fix this:
(no To-header on input) <>

-- 
~Randy

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

[Pali Rohár]

Hello!

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

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

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

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

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

> >  	[0]	= KEY_MEDIA,
> >  	[1]	= KEY_NEXTSONG,
> >  	[2]	= KEY_PLAYPAUSE,
> > @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
> >  	[37]	= KEY_UNKNOWN,
> >  	[38]	= KEY_MICMUTE,
> >  	[255]	= KEY_PROG3,
> > +	[65535]	= KEY_UNKNOWN,

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

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

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

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

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

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

> 
> -- 
> ~Randy
> 

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

[Y Paritcher]

On 6/8/20 7:55 PM, Pali Rohár wrote:
> Hello!
> 
> On Monday 08 June 2020 16:27:10 Randy Dunlap wrote:
>> Hi--
>>
>> On 6/8/20 4:05 PM, Y Paritcher wrote:
>>> Increase length of bios_to_linux_keycode to 2 bytes (the true size of a
>>> keycode) to allow for a new keycode 0xffff, this silences the following
>>> messages being logged at startup on a Dell Inspiron 5593:
>>>
>>>     dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
>>>     dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff
> 
> Which keys generate these two scancodes? Or how have you been able to
> trigger these scancodes (in case they are not generated by key press)?
> 
> It is important to know for which key or event or feature we need to
> include this patch and therefore what feature is currently
> non-functional on that laptop.
> 

As I said before:
The DMI contains a table of firmware scancode to linux keycode mappings.
this is parsed at boot and used together with the bios_to_linux_keycode
entries & dell_wmi_keymap_type_ tables to create a keymap.

If a DMI entry does not have a corresponding entry in bios_to_linux_keycode
we log a message to allow adding the correct linux keycode if known.
This is regardless of if the key actually exists on the device.

To date, I have not been able to generate this keycode on my computer.

>>> as per this code comment:
>>>
>>>    Log if we find an entry in the DMI table that we don't
>>>    understand.  If this happens, we should figure out what
>>>    the entry means and add it to bios_to_linux_keycode.
>>>
>>> These are keycodes included in the 0xB2 DMI table, for which the
>>> corosponding keys are not known.
>>
>>   corresponding
>>
>>>
>>> Now when a user will encounter this key, a proper message wil be printed:
>>>
>>>     dell_wmi: Unknown key with type 0xXXXX and code 0xXXXX pressed
>>>
>>> This will then allow the key to be identified properly.
>>>
>>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
>>> ---
>>>  drivers/platform/x86/dell-wmi.c | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>>> index 6b510f8431a3..dae1db96b5a0 100644
>>> --- a/drivers/platform/x86/dell-wmi.c
>>> +++ b/drivers/platform/x86/dell-wmi.c
>>> @@ -196,7 +196,7 @@ struct dell_dmi_results {
>>>  };
>>>  
>>>  /* Uninitialized entries here are KEY_RESERVED == 0. */
>>> -static const u16 bios_to_linux_keycode[256] = {
>>> +static const u16 bios_to_linux_keycode[65536] = {
>>
>> It surely seems odd to me to expand an array from 512 bytes to 128 Kbytes
>> just to handle one special case.  Can't it be handled in code as a
>> special case?
> 
> I already wrote that more developers would not be happy about this
> change. I would rather to see e.g. that Randy's suggestion with 0xffff
> check as increasing memory usage.
> 

Will change

>>>  	[0]	= KEY_MEDIA,
>>>  	[1]	= KEY_NEXTSONG,
>>>  	[2]	= KEY_PLAYPAUSE,
>>> @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
>>>  	[37]	= KEY_UNKNOWN,
>>>  	[38]	= KEY_MICMUTE,
>>>  	[255]	= KEY_PROG3,
>>> +	[65535]	= KEY_UNKNOWN,
> 
> Looking at the last two lines... and for me it looks like that 0x00FF
> and 0xFFFF are just "placeholders" or special values for unknown /
> custom / unsupported / reserved / special / ... codes.
> 

Probably so, but i have no way of knowing.

I just don't think there is a point spamming a users log with info that
they can't do anything with. If this is turned into a debug print then
i don't care to leave this as is, i had thought this might be helpful
just to know that this keycode mapping appears in the wild.

> It is really suspicious why first 38 values are defined, then there is
> gap, then one value 255 and then huge gap to 65535.
> 
> Mario, this looks like some mapping table between internal Dell BIOS key
> code and standard Linux key code. Are you able to get access to some
> documentation which contains explanation of those Dell key numbers?
> It could really help us to understand these gaps and what is correct
> interpretation of these numbers.
> 
> E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
> generates code 255, which could prove my thesis about "special codes"
> (which are probably not found in e.g. Windows or Linux mapping tables).
> 
>>>  };
>>>  
>>>  /*
>>> @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)
>>>  					&table->keymap[i];
>>>  
>>>  		/* Uninitialized entries are 0 aka KEY_RESERVED. */
>>> -		u16 keycode = (bios_entry->keycode <
>>> -			       ARRAY_SIZE(bios_to_linux_keycode)) ?
>>> -			bios_to_linux_keycode[bios_entry->keycode] :
>>> -			KEY_RESERVED;
>>> +		u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
>>>  
>>>  		/*
>>>  		 * Log if we find an entry in the DMI table that we don't
>>>
>>
>> Something like:
>>
>> 		u16 keycode;
>>
>> 		keycode = bios_entry->keycode == 0xffff ? KEY_UNKNOWN :
>> 			(bios_entry->keycode <
>> 			       ARRAY_SIZE(bios_to_linux_keycode)) ?
>> 			bios_to_linux_keycode[bios_entry->keycode] :
>> 			KEY_RESERVED;
>>
>>
>>
>> Also please fix this:
>> (no To-header on input) <>
> 
> Hint: specifying git send-email with '--to' argument instead of '--cc'
> should help.
> 

Sorry about that.
>>
>> -- 
>> ~Randy
>>

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

[Pali Rohár]

On Monday 08 June 2020 20:43:45 Y Paritcher wrote:
> On 6/8/20 7:55 PM, Pali Rohár wrote:
> > Hello!
> > 
> > On Monday 08 June 2020 16:27:10 Randy Dunlap wrote:
> >> Hi--
> >>
> >> On 6/8/20 4:05 PM, Y Paritcher wrote:
> >>> Increase length of bios_to_linux_keycode to 2 bytes (the true size of a
> >>> keycode) to allow for a new keycode 0xffff, this silences the following
> >>> messages being logged at startup on a Dell Inspiron 5593:
> >>>
> >>>     dell_wmi: firmware scancode 0x48 maps to unrecognized keycode 0xffff
> >>>     dell_wmi: firmware scancode 0x50 maps to unrecognized keycode 0xffff
> > 
> > Which keys generate these two scancodes? Or how have you been able to
> > trigger these scancodes (in case they are not generated by key press)?
> > 
> > It is important to know for which key or event or feature we need to
> > include this patch and therefore what feature is currently
> > non-functional on that laptop.
> > 
> 
> As I said before:
> The DMI contains a table of firmware scancode to linux keycode mappings.
> this is parsed at boot and used together with the bios_to_linux_keycode
> entries & dell_wmi_keymap_type_ tables to create a keymap.
> 
> If a DMI entry does not have a corresponding entry in bios_to_linux_keycode
> we log a message to allow adding the correct linux keycode if known.
> This is regardless of if the key actually exists on the device.
> 
> To date, I have not been able to generate this keycode on my computer.

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

You should include this information into commit message.

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

> >>> as per this code comment:
> >>>
> >>>    Log if we find an entry in the DMI table that we don't
> >>>    understand.  If this happens, we should figure out what
> >>>    the entry means and add it to bios_to_linux_keycode.
> >>>
> >>> These are keycodes included in the 0xB2 DMI table, for which the
> >>> corosponding keys are not known.
> >>
> >>   corresponding
> >>
> >>>
> >>> Now when a user will encounter this key, a proper message wil be printed:
> >>>
> >>>     dell_wmi: Unknown key with type 0xXXXX and code 0xXXXX pressed
> >>>
> >>> This will then allow the key to be identified properly.
> >>>
> >>> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> >>> ---
> >>>  drivers/platform/x86/dell-wmi.c | 8 +++-----
> >>>  1 file changed, 3 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> >>> index 6b510f8431a3..dae1db96b5a0 100644
> >>> --- a/drivers/platform/x86/dell-wmi.c
> >>> +++ b/drivers/platform/x86/dell-wmi.c
> >>> @@ -196,7 +196,7 @@ struct dell_dmi_results {
> >>>  };
> >>>  
> >>>  /* Uninitialized entries here are KEY_RESERVED == 0. */
> >>> -static const u16 bios_to_linux_keycode[256] = {
> >>> +static const u16 bios_to_linux_keycode[65536] = {
> >>
> >> It surely seems odd to me to expand an array from 512 bytes to 128 Kbytes
> >> just to handle one special case.  Can't it be handled in code as a
> >> special case?
> > 
> > I already wrote that more developers would not be happy about this
> > change. I would rather to see e.g. that Randy's suggestion with 0xffff
> > check as increasing memory usage.
> > 
> 
> Will change
> 
> >>>  	[0]	= KEY_MEDIA,
> >>>  	[1]	= KEY_NEXTSONG,
> >>>  	[2]	= KEY_PLAYPAUSE,
> >>> @@ -237,6 +237,7 @@ static const u16 bios_to_linux_keycode[256] = {
> >>>  	[37]	= KEY_UNKNOWN,
> >>>  	[38]	= KEY_MICMUTE,
> >>>  	[255]	= KEY_PROG3,
> >>> +	[65535]	= KEY_UNKNOWN,
> > 
> > Looking at the last two lines... and for me it looks like that 0x00FF
> > and 0xFFFF are just "placeholders" or special values for unknown /
> > custom / unsupported / reserved / special / ... codes.
> > 
> 
> Probably so, but i have no way of knowing.

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

> I just don't think there is a point spamming a users log with info that
> they can't do anything with. If this is turned into a debug print then
> i don't care to leave this as is, i had thought this might be helpful
> just to know that this keycode mapping appears in the wild.
> 
> > It is really suspicious why first 38 values are defined, then there is
> > gap, then one value 255 and then huge gap to 65535.
> > 
> > Mario, this looks like some mapping table between internal Dell BIOS key
> > code and standard Linux key code. Are you able to get access to some
> > documentation which contains explanation of those Dell key numbers?
> > It could really help us to understand these gaps and what is correct
> > interpretation of these numbers.
> > 
> > E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
> > generates code 255, which could prove my thesis about "special codes"
> > (which are probably not found in e.g. Windows or Linux mapping tables).
> > 
> >>>  };
> >>>  
> >>>  /*
> >>> @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)
> >>>  					&table->keymap[i];
> >>>  
> >>>  		/* Uninitialized entries are 0 aka KEY_RESERVED. */
> >>> -		u16 keycode = (bios_entry->keycode <
> >>> -			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> >>> -			bios_to_linux_keycode[bios_entry->keycode] :
> >>> -			KEY_RESERVED;
> >>> +		u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
> >>>  
> >>>  		/*
> >>>  		 * Log if we find an entry in the DMI table that we don't
> >>>
> >>
> >> Something like:
> >>
> >> 		u16 keycode;
> >>
> >> 		keycode = bios_entry->keycode == 0xffff ? KEY_UNKNOWN :
> >> 			(bios_entry->keycode <
> >> 			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> >> 			bios_to_linux_keycode[bios_entry->keycode] :
> >> 			KEY_RESERVED;
> >>
> >>
> >>
> >> Also please fix this:
> >> (no To-header on input) <>
> > 
> > Hint: specifying git send-email with '--to' argument instead of '--cc'
> > should help.
> > 
> 
> Sorry about that.
> >>
> >> -- 
> >> ~Randy
> >>

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

[Mario.Limonciello]

> 
> Looking at the last two lines... and for me it looks like that 0x00FF
> and 0xFFFF are just "placeholders" or special values for unknown /
> custom / unsupported / reserved / special / ... codes.
> 
> It is really suspicious why first 38 values are defined, then there is
> gap, then one value 255 and then huge gap to 65535.
> 
> Mario, this looks like some mapping table between internal Dell BIOS key
> code and standard Linux key code. Are you able to get access to some
> documentation which contains explanation of those Dell key numbers?
> It could really help us to understand these gaps and what is correct
> interpretation of these numbers.
> 

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

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

0x00FF is user programmable function
0xFFFF is no function

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

> E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
> generates code 255, which could prove my thesis about "special codes"
> (which are probably not found in e.g. Windows or Linux mapping tables).
> 
> > >  };
> > >
> > >  /*
> > > @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
> dmi_header *dm, void *opaque)
> > >  					&table->keymap[i];
> > >
> > >  		/* Uninitialized entries are 0 aka KEY_RESERVED. */
> > > -		u16 keycode = (bios_entry->keycode <
> > > -			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > -			bios_to_linux_keycode[bios_entry->keycode] :
> > > -			KEY_RESERVED;
> > > +		u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
> > >
> > >  		/*
> > >  		 * Log if we find an entry in the DMI table that we don't
> > >
> >
> > Something like:
> >
> > 		u16 keycode;
> >
> > 		keycode = bios_entry->keycode == 0xffff ? KEY_UNKNOWN :
> > 			(bios_entry->keycode <
> > 			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > 			bios_to_linux_keycode[bios_entry->keycode] :
> > 			KEY_RESERVED;
> >
> >
> >
> > Also please fix this:
> > (no To-header on input) <>
> 
> Hint: specifying git send-email with '--to' argument instead of '--cc'
> should help.
> 
> >
> > --
> > ~Randy
> >

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

[Pali Rohár]

On Tuesday 09 June 2020 19:49:18 Mario.Limonciello@dell.com wrote:
> > 
> > Looking at the last two lines... and for me it looks like that 0x00FF
> > and 0xFFFF are just "placeholders" or special values for unknown /
> > custom / unsupported / reserved / special / ... codes.
> > 
> > It is really suspicious why first 38 values are defined, then there is
> > gap, then one value 255 and then huge gap to 65535.
> > 
> > Mario, this looks like some mapping table between internal Dell BIOS key
> > code and standard Linux key code. Are you able to get access to some
> > documentation which contains explanation of those Dell key numbers?
> > It could really help us to understand these gaps and what is correct
> > interpretation of these numbers.
> > 
> 
> The codes are actually 4 bytes in the table, but in practice nothing above the
> first two bytes is used.
> 
> Those two called out are special though, here are their meanings:
> 
> 0x00FF is user programmable function
> 0xFFFF is no function
> 
> For the purpose of memory consumption I think it's reasonable to ignore the
> upper 2 bytes and special case these two.

Thank you for information!

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

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

> > E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
> > generates code 255, which could prove my thesis about "special codes"
> > (which are probably not found in e.g. Windows or Linux mapping tables).
> > 
> > > >  };
> > > >
> > > >  /*
> > > > @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
> > dmi_header *dm, void *opaque)
> > > >  					&table->keymap[i];
> > > >
> > > >  		/* Uninitialized entries are 0 aka KEY_RESERVED. */
> > > > -		u16 keycode = (bios_entry->keycode <
> > > > -			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > -			bios_to_linux_keycode[bios_entry->keycode] :
> > > > -			KEY_RESERVED;
> > > > +		u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
> > > >
> > > >  		/*
> > > >  		 * Log if we find an entry in the DMI table that we don't
> > > >
> > >
> > > Something like:
> > >
> > > 		u16 keycode;
> > >
> > > 		keycode = bios_entry->keycode == 0xffff ? KEY_UNKNOWN :
> > > 			(bios_entry->keycode <
> > > 			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > 			bios_to_linux_keycode[bios_entry->keycode] :
> > > 			KEY_RESERVED;
> > >
> > >
> > >
> > > Also please fix this:
> > > (no To-header on input) <>
> > 
> > Hint: specifying git send-email with '--to' argument instead of '--cc'
> > should help.
> > 
> > >
> > > --
> > > ~Randy
> > >

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

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Pali Rohár
> Sent: Wednesday, June 10, 2020 4:45 AM
> To: Limonciello, Mario
> Cc: rdunlap@infradead.org; y.linux@paritcher.com; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> mjg59@srcf.ucam.org
> Subject: Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to
> bios_to_linux_keycode
> 
> 
> [EXTERNAL EMAIL]
> 
> On Tuesday 09 June 2020 19:49:18 Mario.Limonciello@dell.com wrote:
> > >
> > > Looking at the last two lines... and for me it looks like that 0x00FF
> > > and 0xFFFF are just "placeholders" or special values for unknown /
> > > custom / unsupported / reserved / special / ... codes.
> > >
> > > It is really suspicious why first 38 values are defined, then there is
> > > gap, then one value 255 and then huge gap to 65535.
> > >
> > > Mario, this looks like some mapping table between internal Dell BIOS
> key
> > > code and standard Linux key code. Are you able to get access to some
> > > documentation which contains explanation of those Dell key numbers?
> > > It could really help us to understand these gaps and what is correct
> > > interpretation of these numbers.
> > >
> >
> > The codes are actually 4 bytes in the table, but in practice nothing
> above the
> > first two bytes is used.
> >
> > Those two called out are special though, here are their meanings:
> >
> > 0x00FF is user programmable function
> > 0xFFFF is no function
> >
> > For the purpose of memory consumption I think it's reasonable to ignore
> the
> > upper 2 bytes and special case these two.
> 
> Thank you for information!
> 
> So 0x00FF is "user programmable" button. Do I understand it correctly
> that Dell/BIOS does not explicitly provide meaning for these buttons,
> they do not have fixed functionality and therefore user should configure
> them as he want?

Correct

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

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

> 
> > > E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
> > > generates code 255, which could prove my thesis about "special codes"
> > > (which are probably not found in e.g. Windows or Linux mapping tables).
> > >
> > > > >  };
> > > > >
> > > > >  /*
> > > > > @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
> > > dmi_header *dm, void *opaque)
> > > > >  					&table->keymap[i];
> > > > >
> > > > >  		/* Uninitialized entries are 0 aka KEY_RESERVED. */
> > > > > -		u16 keycode = (bios_entry->keycode <
> > > > > -			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > > -			bios_to_linux_keycode[bios_entry->keycode] :
> > > > > -			KEY_RESERVED;
> > > > > +		u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
> > > > >
> > > > >  		/*
> > > > >  		 * Log if we find an entry in the DMI table that we don't
> > > > >
> > > >
> > > > Something like:
> > > >
> > > > 		u16 keycode;
> > > >
> > > > 		keycode = bios_entry->keycode == 0xffff ? KEY_UNKNOWN :
> > > > 			(bios_entry->keycode <
> > > > 			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > 			bios_to_linux_keycode[bios_entry->keycode] :
> > > > 			KEY_RESERVED;
> > > >
> > > >
> > > >
> > > > Also please fix this:
> > > > (no To-header on input) <>
> > >
> > > Hint: specifying git send-email with '--to' argument instead of '--cc'
> > > should help.
> > >
> > > >
> > > > --
> > > > ~Randy
> > > >

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

[Pali Rohár]

On Wednesday 10 June 2020 12:35:09 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> > owner@vger.kernel.org> On Behalf Of Pali Rohár
> > Sent: Wednesday, June 10, 2020 4:45 AM
> > To: Limonciello, Mario
> > Cc: rdunlap@infradead.org; y.linux@paritcher.com; linux-
> > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> > mjg59@srcf.ucam.org
> > Subject: Re: [PATCH v2 3/3] platform/x86: dell-wmi: add new dmi keys to
> > bios_to_linux_keycode
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > On Tuesday 09 June 2020 19:49:18 Mario.Limonciello@dell.com wrote:
> > > >
> > > > Looking at the last two lines... and for me it looks like that 0x00FF
> > > > and 0xFFFF are just "placeholders" or special values for unknown /
> > > > custom / unsupported / reserved / special / ... codes.
> > > >
> > > > It is really suspicious why first 38 values are defined, then there is
> > > > gap, then one value 255 and then huge gap to 65535.
> > > >
> > > > Mario, this looks like some mapping table between internal Dell BIOS
> > key
> > > > code and standard Linux key code. Are you able to get access to some
> > > > documentation which contains explanation of those Dell key numbers?
> > > > It could really help us to understand these gaps and what is correct
> > > > interpretation of these numbers.
> > > >
> > >
> > > The codes are actually 4 bytes in the table, but in practice nothing
> > above the
> > > first two bytes is used.
> > >
> > > Those two called out are special though, here are their meanings:
> > >
> > > 0x00FF is user programmable function
> > > 0xFFFF is no function
> > >
> > > For the purpose of memory consumption I think it's reasonable to ignore
> > the
> > > upper 2 bytes and special case these two.
> > 
> > Thank you for information!
> > 
> > So 0x00FF is "user programmable" button. Do I understand it correctly
> > that Dell/BIOS does not explicitly provide meaning for these buttons,
> > they do not have fixed functionality and therefore user should configure
> > them as he want?
> 
> Correct
> 
> > 
> > And what does mean "no function"? I do not know what should I imagine if
> > I receive key press marked as "no function".
> 
> It means no action is expected to occur, should behave like a no-op.  I think
> discarding it makes fine sense.

Thank you! This was missing bit of information.

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

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

> > 
> > > > E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
> > > > generates code 255, which could prove my thesis about "special codes"
> > > > (which are probably not found in e.g. Windows or Linux mapping tables).
> > > >
> > > > > >  };
> > > > > >
> > > > > >  /*
> > > > > > @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
> > > > dmi_header *dm, void *opaque)
> > > > > >  					&table->keymap[i];
> > > > > >
> > > > > >  		/* Uninitialized entries are 0 aka KEY_RESERVED. */
> > > > > > -		u16 keycode = (bios_entry->keycode <
> > > > > > -			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > > > -			bios_to_linux_keycode[bios_entry->keycode] :
> > > > > > -			KEY_RESERVED;
> > > > > > +		u16 keycode = bios_to_linux_keycode[bios_entry->keycode];
> > > > > >
> > > > > >  		/*
> > > > > >  		 * Log if we find an entry in the DMI table that we don't
> > > > > >
> > > > >
> > > > > Something like:
> > > > >
> > > > > 		u16 keycode;
> > > > >
> > > > > 		keycode = bios_entry->keycode == 0xffff ? KEY_UNKNOWN :
> > > > > 			(bios_entry->keycode <
> > > > > 			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > > 			bios_to_linux_keycode[bios_entry->keycode] :
> > > > > 			KEY_RESERVED;
> > > > >
> > > > >
> > > > >
> > > > > Also please fix this:
> > > > > (no To-header on input) <>
> > > >
> > > > Hint: specifying git send-email with '--to' argument instead of '--cc'
> > > > should help.
> > > >
> > > > >
> > > > > --
> > > > > ~Randy
> > > > >

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

[Mario.Limonciello]

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

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

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

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

> 
> > >
> > > > > E.g. I remember that pressing Fn+Q or Fn+W on some Dell Latitude
> > > > > generates code 255, which could prove my thesis about "special
> codes"
> > > > > (which are probably not found in e.g. Windows or Linux mapping
> tables).
> > > > >
> > > > > > >  };
> > > > > > >
> > > > > > >  /*
> > > > > > > @@ -503,10 +504,7 @@ static void handle_dmi_entry(const struct
> > > > > dmi_header *dm, void *opaque)
> > > > > > >  					&table->keymap[i];
> > > > > > >
> > > > > > >  		/* Uninitialized entries are 0 aka KEY_RESERVED. */
> > > > > > > -		u16 keycode = (bios_entry->keycode <
> > > > > > > -			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > > > > -			bios_to_linux_keycode[bios_entry->keycode] :
> > > > > > > -			KEY_RESERVED;
> > > > > > > +		u16 keycode = bios_to_linux_keycode[bios_entry-
> >keycode];
> > > > > > >
> > > > > > >  		/*
> > > > > > >  		 * Log if we find an entry in the DMI table that we
> don't
> > > > > > >
> > > > > >
> > > > > > Something like:
> > > > > >
> > > > > > 		u16 keycode;
> > > > > >
> > > > > > 		keycode = bios_entry->keycode == 0xffff ? KEY_UNKNOWN :
> > > > > > 			(bios_entry->keycode <
> > > > > > 			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > > > > 			bios_to_linux_keycode[bios_entry->keycode] :
> > > > > > 			KEY_RESERVED;
> > > > > >
> > > > > >
> > > > > >
> > > > > > Also please fix this:
> > > > > > (no To-header on input) <>
> > > > >
> > > > > Hint: specifying git send-email with '--to' argument instead of '--
> cc'
> > > > > should help.
> > > > >
> > > > > >
> > > > > > --
> > > > > > ~Randy
> > > > > >

[PATCH v3 0/3] platform/x86: dell-wmi: new keys

[Y Paritcher]

Extended 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(-)

-- 
2.27.0

[PATCH v3 1/3] platform/x86: dell-wmi: add new backlight events

[Y Paritcher]

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 pressed
dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed

These are brightness events and will be handled by acpi-video

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index c25a4286d766..0b2edfe2767d 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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

RE: [PATCH v3 1/3] platform/x86: dell-wmi: add new backlight events

[Mario.Limonciello]

> -----Original Message-----
> From: Y Paritcher <y.linux@paritcher.com>
> Sent: Monday, June 8, 2020 10:53 PM
> To: Pali Rohár
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> 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 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 pressed
> dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed
> 
> These are brightness events and will be handled by acpi-video
> 
> Signed-off-by: Y Paritcher <y.linux@paritcher.com>

Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>

> ---
>  drivers/platform/x86/dell-wmi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
> wmi.c
> index c25a4286d766..0b2edfe2767d 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -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

[Y Paritcher]

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 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 0x12
dell_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.

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 0b2edfe2767d..e3bc2601e631 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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.
-- 
2.27.0

[PATCH v3 3/3] platform/x86: dell-wmi: add new dmi mapping for keycode 0xffff

[Y Paritcher]

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

as per this code comment:

   Log if we find an entry in the DMI table that we don't
   understand.  If this happens, we should figure out what
   the entry means and add it to bios_to_linux_keycode.

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index e3bc2601e631..bbdb3e860892 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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
-- 
2.27.0

Re: [PATCH v3 3/3] platform/x86: dell-wmi: add new dmi mapping for keycode 0xffff

[Pali Rohár]

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.

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 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 0xffff
> 
> as per this code comment:
> 
>    Log if we find an entry in the DMI table that we don't
>    understand.  If this happens, we should figure out what
>    the entry means and add it to bios_to_linux_keycode.
> 
> Signed-off-by: Y Paritcher <y.linux@paritcher.com>
> ---
>  drivers/platform/x86/dell-wmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index e3bc2601e631..bbdb3e860892 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -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.

>  
>  		/*
>  		 * Log if we find an entry in the DMI table that we don't
> -- 
> 2.27.0
> 

[PATCH v4 0/3] platform/x86: dell-wmi: new keys

[Y Paritcher]

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

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

-- 
2.27.0

[PATCH v4 1/3] platform/x86: dell-wmi: add new backlight events

[Y Paritcher]

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 pressed
dell_wmi: Unknown key with type 0x0010 and code 0x0058 pressed

These are brightness events and will be handled by acpi-video

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index c25a4286d766..0b2edfe2767d 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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 v4 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Y Paritcher]

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 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 0x12
dell_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.

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 0b2edfe2767d..e3bc2601e631 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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.
-- 
2.27.0

[PATCH v4 3/3] platform/x86: dell-wmi: add new dmi mapping for keycode 0xffff

[Y Paritcher]

This 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 0xffff

as per this code comment:

   Log if we find an entry in the DMI table that we don't
   understand.  If this happens, we should figure out what
   the entry means and add it to bios_to_linux_keycode.

Signed-off-by: Y Paritcher <y.linux@paritcher.com>
---
 drivers/platform/x86/dell-wmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index e3bc2601e631..bbdb3e860892 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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
-- 
2.27.0

RE: [PATCH v4 0/3] platform/x86: dell-wmi: new keys

[Mario.Limonciello]

> -----Original Message-----
> From: Y Paritcher <y.linux@paritcher.com>
> Sent: Wednesday, June 10, 2020 12:57 PM
> To: Pali Rohár
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> 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?
> 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(-)
> 
> --
> 2.27.0

Andy,

The whole series looks good to me now.  You can put this on the patches
when they're swooped up.

Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>

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.

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

Can you please advise which way you would rather have the subsystem go?

Re: [PATCH v4 0/3] platform/x86: dell-wmi: new keys

[Andy Shevchenko]

On Wed, Jun 10, 2020 at 10:23 PM <Mario.Limonciello@dell.com> wrote:
>
> > -----Original Message-----
> > From: Y Paritcher <y.linux@paritcher.com>
> > Sent: Wednesday, June 10, 2020 12:57 PM
> > To: Pali Rohár
> > Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> > 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?
> > 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(-)
> >
> > --
> > 2.27.0
>
> Andy,
>
> The whole series looks good to me now.  You can put this on the patches
> when they're swooped up.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>
>
> 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.
>
> > 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.
>
> 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!


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/3] platform/x86: dell-wmi: new keys

[Pali Rohár]

On Thursday 09 July 2020 22:29:42 Andy Shevchenko wrote:
> On Wed, Jun 10, 2020 at 10:23 PM <Mario.Limonciello@dell.com> wrote:
> >
> > > -----Original Message-----
> > > From: Y Paritcher <y.linux@paritcher.com>
> > > Sent: Wednesday, June 10, 2020 12:57 PM
> > > To: Pali Rohár
> > > Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> > > 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?
> > > 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(-)
> > >
> > > --
> > > 2.27.0
> >
> > Andy,
> >
> > The whole series looks good to me now.  You can put this on the patches
> > when they're swooped up.
> >
> > Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>
> >
> > 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.
> >
> > > 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.
> >
> > 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.

Re: [PATCH v4 0/3] platform/x86: dell-wmi: new keys

[Pali Rohár]

On Monday 13 July 2020 09:29:12 Pali Rohár wrote:
> On Thursday 09 July 2020 22:29:42 Andy Shevchenko wrote:
> > On Wed, Jun 10, 2020 at 10:23 PM <Mario.Limonciello@dell.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Y Paritcher <y.linux@paritcher.com>
> > > > Sent: Wednesday, June 10, 2020 12:57 PM
> > > > To: Pali Rohár
> > > > Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> > > > 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?
> > > > 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(-)
> > > >
> > > > --
> > > > 2.27.0
> > >
> > > Andy,
> > >
> > > The whole series looks good to me now.  You can put this on the patches
> > > when they're swooped up.
> > >
> > > Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>
> > >
> > > 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.
> > >
> > > > 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.
> > >
> > > 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?

Re: [PATCH v4 0/3] platform/x86: dell-wmi: new keys

[Pali Rohár]

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
> 
> 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(-)
> 
> -- 
> 2.27.0
> 

I'm fine with this patch series too.

Reviewed-by: Pali Rohár <pali@kernel.org>