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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
> -----Original Message-----
> From: [email protected] <platform-driver-x86-
> [email protected]> On Behalf Of Y Paritcher
> Sent: Sunday, June 7, 2020 11:22 PM
> Cc: [email protected]; [email protected];
> Matthew Garrett; Pali Roh?r
> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap 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 <[email protected]>
> ---
> 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
On 6/8/20 11:40 AM, [email protected] wrote:
>> -----Original Message-----
>> From: [email protected] <platform-driver-x86-
>> [email protected]> On Behalf Of Y Paritcher
>> Sent: Sunday, June 7, 2020 11:22 PM
>> Cc: [email protected]; [email protected];
>> Matthew Garrett; Pali Rohár
>> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap 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 <[email protected]>
>> ---
>> 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
>
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 <[email protected]>
>> ---
>> 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
>>
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".
> -----Original Message-----
> From: Y Paritcher <[email protected]>
> Sent: Monday, June 8, 2020 3:13 PM
> To: Limonciello, Mario
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
>
>
> [EXTERNAL EMAIL]
>
> On 6/8/20 11:40 AM, [email protected] wrote:
> >> -----Original Message-----
> >> From: [email protected] <platform-driver-x86-
> >> [email protected]> On Behalf Of Y Paritcher
> >> Sent: Sunday, June 7, 2020 11:22 PM
> >> Cc: [email protected]; [email protected];
> >> Matthew Garrett; Pali Rohár
> >> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap 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 <[email protected]>
> >> ---
> >> 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
> >
On 6/8/20 4:36 PM, [email protected] wrote:
>> -----Original Message-----
>> From: Y Paritcher <[email protected]>
>> Sent: Monday, June 8, 2020 3:13 PM
>> To: Limonciello, Mario
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]
>> Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
>>
>>
>> [EXTERNAL EMAIL]
>>
>> On 6/8/20 11:40 AM, [email protected] wrote:
>>>> -----Original Message-----
>>>> From: [email protected] <platform-driver-x86-
>>>> [email protected]> On Behalf Of Y Paritcher
>>>> Sent: Sunday, June 7, 2020 11:22 PM
>>>> Cc: [email protected]; [email protected];
>>>> Matthew Garrett; Pali Rohár
>>>> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap 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 <[email protected]>
>>>> ---
>>>> 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
>>>
>
> 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?
On 6/8/20 6:00 PM, [email protected] 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.
On Monday 08 June 2020 15:40:53 [email protected] 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.
Hi,
On 6/8/20 10:36 PM, [email protected] wrote:
>> -----Original Message-----
>> From: Y Paritcher <[email protected]>
>> Sent: Monday, June 8, 2020 3:13 PM
>> To: Limonciello, Mario
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]
>> Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
>>
>>
>> [EXTERNAL EMAIL]
>>
>> On 6/8/20 11:40 AM, [email protected] wrote:
>>>> -----Original Message-----
>>>> From: [email protected] <platform-driver-x86-
>>>> [email protected]> On Behalf Of Y Paritcher
>>>> Sent: Sunday, June 7, 2020 11:22 PM
>>>> Cc: [email protected]; [email protected];
>>>> Matthew Garrett; Pali Rohár
>>>> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap 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 <[email protected]>
>>>> ---
>>>> 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
>>>
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 <[email protected]>
> >>>> ---
> >>>> 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
> >>>
On Monday 08 June 2020 20:36:58 [email protected] wrote:
> Can you please comment here how you would like to see events like this should come
> through to userspace?
>
> * Wrong power adapter (you have X and should have Y)
> * You have plugged a dock into the wrong port
> * Fn-lock mode
In my opinion, Fn-lock mode is related to input subsystem and should be
probably reported via input device. For a user, fn-lock is similar like
caps-lock, scroll-lock or num-lock. Also fn-lock is provided by other
laptops and therefore I would expect that kernel provide fn-lock state
for all laptops (thinkpad / latitude / ...) via same interface. And not
via dell-specific interface or general-vendor-message interface.
Wrong power adapter sounds like something related to power subsystem.
Adding Sebastian to the loop, maybe he can provide some useful ideas
about it.
And plugged dock into wrong port. This is probably dell-specific event
and some interface for "vendor" messages from kernel to userspace would
be needed to deliver such things.
Hi,
On 6/9/20 5:36 PM, [email protected] 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
Hi,
On Tue, Jun 09, 2020 at 05:49:38PM +0200, Pali Roh?r wrote:
> On Monday 08 June 2020 20:36:58 [email protected] wrote:
> > Can you please comment here how you would like to see events like this should come
> > through to userspace?
> >
> > * Wrong power adapter (you have X and should have Y)
> > * You have plugged a dock into the wrong port
> > * Fn-lock mode
>
> In my opinion, Fn-lock mode is related to input subsystem and should be
> probably reported via input device. For a user, fn-lock is similar like
> caps-lock, scroll-lock or num-lock. Also fn-lock is provided by other
> laptops and therefore I would expect that kernel provide fn-lock state
> for all laptops (thinkpad / latitude / ...) via same interface. And not
> via dell-specific interface or general-vendor-message interface.
>
> Wrong power adapter sounds like something related to power subsystem.
> Adding Sebastian to the loop, maybe he can provide some useful ideas
> about it.
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.
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 [email protected] wrote:
>>> Can you please comment here how you would like to see events like this should come
>>> through to userspace?
>>>
>>> * Wrong power adapter (you have X and should have Y)
>>> * You have plugged a dock into the wrong port
>>> * Fn-lock mode
>>
>> In my opinion, Fn-lock mode is related to input subsystem and should be
>> probably reported via input device. For a user, fn-lock is similar like
>> caps-lock, scroll-lock or num-lock. Also fn-lock is provided by other
>> laptops and therefore I would expect that kernel provide fn-lock state
>> for all laptops (thinkpad / latitude / ...) via same interface. And not
>> via dell-specific interface or general-vendor-message interface.
>>
>> Wrong power adapter sounds like something related to power subsystem.
>> Adding Sebastian to the loop, maybe he can provide some useful ideas
>> about it.
>
> 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
> 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
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 [email protected] wrote:
> > > > Can you please comment here how you would like to see events like this should come
> > > > through to userspace?
> > > >
> > > > * Wrong power adapter (you have X and should have Y)
> > > > * You have plugged a dock into the wrong port
> > > > * Fn-lock mode
> > >
> > > In my opinion, Fn-lock mode is related to input subsystem and should be
> > > probably reported via input device. For a user, fn-lock is similar like
> > > caps-lock, scroll-lock or num-lock. Also fn-lock is provided by other
> > > laptops and therefore I would expect that kernel provide fn-lock state
> > > for all laptops (thinkpad / latitude / ...) via same interface. And not
> > > via dell-specific interface or general-vendor-message interface.
> > >
> > > Wrong power adapter sounds like something related to power subsystem.
> > > Adding Sebastian to the loop, maybe he can provide some useful ideas
> > > about it.
> >
> > 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
> >
> > 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.