Message-ID: <2d4b308d-ce03-b13b-0140-4bbe669878c4@paritcher.com>
Date:   Mon, 8 Jun 2020 20:57:38 -0400
From:   Y Paritcher <y.linux@...itcher.com>
To:     Mario.Limonciello@...l.com, pali@...nel.org
Cc:     linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        mjg59@...f.ucam.org
Subject: Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

On 6/8/20 8:26 PM, Mario.Limonciello@...l.com wrote:
>> -----Original Message-----
>> From: Pali Rohár <pali@...nel.org>
>> Sent: Monday, June 8, 2020 6:33 PM
>> To: Y Paritcher
>> Cc: linux-kernel@...r.kernel.org; platform-driver-x86@...r.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@...itcher.com>
>>
>> I'm fine with this patch now.
>>
>> Reviewed-by: Pali Rohár <pali@...nel.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
>>>

Powered by blists - more mailing lists