[PATCH] dell-wmi: Update code for processing WMI events

[Pali Rohár]

WMI buffer can contains more events. First value in buffer is length of=
event
followed by data of specified length. After that is next length and nex=
t data.
When length is zero then there is no more events in bufffer.

This patch adds support for processing all events in buffer (not only f=
irst)
and parse more event types (not only hotkey events). Because of variabl=
e length
of events sometimes BIOS fills more hotkeys (or other values) into sing=
le WMI
event. In this case this patch process also these multiple hotkeys (and=
not
only first one).

Some event types are just ignored because kernel is not interested for =
them
(e.g. NIC Link status, battery unplug, ...).

This patch is based on DSDT table from Dell Latitude E6440. Code should=
be
backward compatible so will process other events of old types same as b=
efore
this patch.

This patch also fixes problem when in kernel log are written messages a=
bout
unknown WMI events. Now all know events are parsed and those which are =
not
interesting for kernel are dropped without unknown message.

Signed-off-by: Pali Roh=C3=A1r <***@gmail.com>
Tested-by: Pali Roh=C3=A1r <***@gmail.com>
---
drivers/platform/x86/dell-wmi.c | 157 +++++++++++++++++++++++++++++++=
--------
1 file changed, 127 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/del=
l-wmi.c
index 25721bf..3d15949 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -145,11 +145,35 @@ static const u16 bios_to_linux_keycode[256] __ini=
tconst =3D {
=20
static struct input_dev *dell_wmi_input_dev;
=20
+static void dell_wmi_process_key(int reported_key)
+{
+ const struct key_entry *key;
+
+ key =3D sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
+ reported_key);
+ if (!key) {
+ pr_info("Unknown key %x pressed\n", reported_key);
+ return;
+ }
+
+ pr_debug("Key %x pressed\n", reported_key);
+
+ /* Don't report brightness notifications that will also come via ACPI=
*/
+ if ((key->keycode =3D=3D KEY_BRIGHTNESSUP ||
+ key->keycode =3D=3D KEY_BRIGHTNESSDOWN) && acpi_video)
+ return;
+
+ sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
+}
+
static void dell_wmi_notify(u32 value, void *context)
{
struct acpi_buffer response =3D { ACPI_ALLOCATE_BUFFER, NULL };
union acpi_object *obj;
acpi_status status;
+ acpi_size buffer_size;
+ u16 *buffer_entry, *buffer_end;
+ int len, i;
=20
status =3D wmi_get_event_data(value, &response);
if (status !=3D AE_OK) {
@@ -158,44 +182,117 @@ static void dell_wmi_notify(u32 value, void *con=
text)
}
=20
obj =3D (union acpi_object *)response.pointer;
+ if (!obj) {
+ pr_info("no response\n");
+ return;
+ }
=20
- if (obj && obj->type =3D=3D ACPI_TYPE_BUFFER) {
- const struct key_entry *key;
- int reported_key;
- u16 *buffer_entry =3D (u16 *)obj->buffer.pointer;
- int buffer_size =3D obj->buffer.length/2;
-
- if (buffer_size >=3D 2 && dell_new_hk_type && buffer_entry[1] !=3D 0=
x10) {
- pr_info("Received unknown WMI event (0x%x)\n",
- buffer_entry[1]);
- kfree(obj);
- return;
- }
+ if (obj->type !=3D ACPI_TYPE_BUFFER) {
+ pr_info("bad response type %x\n", obj->type);
+ kfree(obj);
+ return;
+ }
+
+ pr_debug("Received WMI event (%*ph)\n",
+ obj->buffer.length, obj->buffer.pointer);
=20
- if (buffer_size >=3D 3 && (dell_new_hk_type || buffer_entry[1] =3D=3D=
0x0))
- reported_key =3D (int)buffer_entry[2];
+ buffer_entry =3D (u16 *)obj->buffer.pointer;
+ buffer_size =3D obj->buffer.length/2;
+
+ if (!dell_new_hk_type) {
+ if (buffer_size >=3D 3 && buffer_entry[1] =3D=3D 0x0)
+ dell_wmi_process_key(buffer_entry[2]);
else if (buffer_size >=3D 2)
- reported_key =3D (int)buffer_entry[1] & 0xffff;
- else {
+ dell_wmi_process_key(buffer_entry[1]);
+ else
pr_info("Received unknown WMI event\n");
- kfree(obj);
- return;
+ kfree(obj);
+ return;
+ }
+
+ buffer_end =3D buffer_entry + buffer_size;
+
+ while (buffer_entry < buffer_end) {
+
+ len =3D buffer_entry[0];
+ if (len =3D=3D 0)
+ break;
+
+ len++;
+
+ if (buffer_entry+len > buffer_end) {
+ pr_info("Invalid length of WMI event\n");
+ break;
}
=20
- key =3D sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
- reported_key);
- if (!key) {
- pr_info("Unknown key %x pressed\n", reported_key);
- } else if ((key->keycode =3D=3D KEY_BRIGHTNESSUP ||
- key->keycode =3D=3D KEY_BRIGHTNESSDOWN) && acpi_video) {
- /* Don't report brightness notifications that will also
- * come via ACPI */
- ;
- } else {
- sparse_keymap_report_entry(dell_wmi_input_dev, key,
- 1, true);
+ pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
+
+ switch (buffer_entry[1]) {
+ case 0x00:
+ for (i =3D 2; i < len; ++i) {
+ switch (buffer_entry[i]) {
+ case 0xe043:
+ /* NIC Link is Up */
+ pr_debug("NIC Link is Up\n");
+ break;
+ case 0xe044:
+ /* NIC Link is Down */
+ pr_debug("NIC Link is Down\n");
+ break;
+ case 0xe045:
+ /* Unknown event but defined in DSDT */
+ default:
+ /* Unknown event */
+ pr_info("Unknown WMI event type 0x00: "
+ "0x%x\n", (int)buffer_entry[i]);
+ break;
+ }
+ }
+ break;
+ case 0x10:
+ /* Keys pressed */
+ for (i =3D 2; i < len; ++i)
+ dell_wmi_process_key(buffer_entry[i]);
+ break;
+ case 0x11:
+ for (i =3D 2; i < len; ++i) {
+ switch (buffer_entry[i]) {
+ case 0xfff0:
+ /* Battery unplugged */
+ pr_debug("Battery unplugged\n");
+ break;
+ case 0xfff1:
+ /* Battery inserted */
+ pr_debug("Battery inserted\n");
+ break;
+ case 0x01e1:
+ case 0x02ea:
+ case 0x02eb:
+ case 0x02ec:
+ case 0x02f6:
+ /* Keyboard backlight level changed */
+ pr_debug("Keyboard backlight level "
+ "changed\n");
+ break;
+ default:
+ /* Unknown event */
+ pr_info("Unknown WMI event type 0x11: "
+ "0x%x\n", (int)buffer_entry[i]);
+ break;
+ }
+ }
+ break;
+ default:
+ /* Unknown event */
+ pr_info("Unknown WMI event type 0x%x\n",
+ (int)buffer_entry[1]);
+ break;
}
+
+ buffer_entry +=3D len;
+
}
+
kfree(obj);
}
=20
--=20
1.7.9.5

[Darren Hart]

WMI buffer can contains more events. First value in buffer is length =

of event

followed by data of specified length. After that is next length and n=

ext data.

When length is zero then there is no more events in bufffer.
=20
This patch adds support for processing all events in buffer (not only=

first)

and parse more event types (not only hotkey events). Because of varia=

ble length

of events sometimes BIOS fills more hotkeys (or other values) into si=

ngle WMI

event. In this case this patch process also these multiple hotkeys (a=

nd not

only first one).
=20
Some event types are just ignored because kernel is not interested fo=

r them

(e.g. NIC Link status, battery unplug, ...).
=20
This patch is based on DSDT table from Dell Latitude E6440. Code shou=

ld be

backward compatible so will process other events of old types same as=

before

this patch.
=20
This patch also fixes problem when in kernel log are written messages=

about

unknown WMI events. Now all know events are parsed and those which ar=

e not

interesting for kernel are dropped without unknown message.

This should probably be done in a separate patch.

=20

Well yes, I should hope so ;-)

---
drivers/platform/x86/dell-wmi.c | 157 +++++++++++++++++++++++++++++=

++--------

1 file changed, 127 insertions(+), 30 deletions(-)
=20
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/d=

ell-wmi.c

index 25721bf..3d15949 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -145,11 +145,35 @@ static const u16 bios_to_linux_keycode[256] __i=

nitconst =3D {

=20
static struct input_dev *dell_wmi_input_dev;
=20
+static void dell_wmi_process_key(int reported_key)
+{
+ const struct key_entry *key;
+
+ key =3D sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
+ reported_key);
+ if (!key) {
+ pr_info("Unknown key %x pressed\n", reported_key);
+ return;
+ }
+
+ pr_debug("Key %x pressed\n", reported_key);
+
+ /* Don't report brightness notifications that will also come via AC=

PI */

+ if ((key->keycode =3D=3D KEY_BRIGHTNESSUP ||
+ key->keycode =3D=3D KEY_BRIGHTNESSDOWN) && acpi_video)
+ return;
+
+ sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
+}
+
static void dell_wmi_notify(u32 value, void *context)
{
struct acpi_buffer response =3D { ACPI_ALLOCATE_BUFFER, NULL };
union acpi_object *obj;
acpi_status status;
+ acpi_size buffer_size;
+ u16 *buffer_entry, *buffer_end;
+ int len, i;
=20
status =3D wmi_get_event_data(value, &response);
if (status !=3D AE_OK) {
@@ -158,44 +182,117 @@ static void dell_wmi_notify(u32 value, void *c=

ontext)

}
=20
obj =3D (union acpi_object *)response.pointer;
+ if (!obj) {
+ pr_info("no response\n");
+ return;
+ }

If you intend to print this, it should probably be a bit more informati=
ve. Is
"info" the right level here? I would imagine either WARN if this was a =
bad
thing, or DEBUG if this is more for debugging the driver.

- if (obj && obj->type =3D=3D ACPI_TYPE_BUFFER) {
- const struct key_entry *key;
- int reported_key;
- u16 *buffer_entry =3D (u16 *)obj->buffer.pointer;
- int buffer_size =3D obj->buffer.length/2;
-
- if (buffer_size >=3D 2 && dell_new_hk_type && buffer_entry[1] !=3D=

0x10) {

- pr_info("Received unknown WMI event (0x%x)\n",
- buffer_entry[1]);
- kfree(obj);
- return;
- }
+ if (obj->type !=3D ACPI_TYPE_BUFFER) {
+ pr_info("bad response type %x\n", obj->type);
+ kfree(obj);
+ return;
+ }
+
+ pr_debug("Received WMI event (%*ph)\n",
+ obj->buffer.length, obj->buffer.pointer);
=20
- if (buffer_size >=3D 3 && (dell_new_hk_type || buffer_entry[1] =3D=

=3D 0x0))

- reported_key =3D (int)buffer_entry[2];
+ buffer_entry =3D (u16 *)obj->buffer.pointer;
+ buffer_size =3D obj->buffer.length/2;
+
+ if (!dell_new_hk_type) {
+ if (buffer_size >=3D 3 && buffer_entry[1] =3D=3D 0x0)
+ dell_wmi_process_key(buffer_entry[2]);
else if (buffer_size >=3D 2)
- reported_key =3D (int)buffer_entry[1] & 0xffff;
- else {
+ dell_wmi_process_key(buffer_entry[1]);

Why can we drop the 0xffff mask now?

+ else
pr_info("Received unknown WMI event\n");
- kfree(obj);
- return;
+ kfree(obj);
+ return;
+ }
+
+ buffer_end =3D buffer_entry + buffer_size;
+
+ while (buffer_entry < buffer_end) {
+
+ len =3D buffer_entry[0];
+ if (len =3D=3D 0)
+ break;
+
+ len++;
+

Why increment len here? Are you trying to avoid a "len + 1" in the comp=
arisons
below? If so, is using "len * 2" in the debug message below correct? Pl=
ease
clarify.

+ if (buffer_entry+len > buffer_end) {

See coding style documentation on operators. Please run patches through
checkpatch.

+ pr_info("Invalid length of WMI event\n");

info doesn't see correct here either.

+ break;
}
=20
- key =3D sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
- reported_key);
- if (!key) {
- pr_info("Unknown key %x pressed\n", reported_key);
- } else if ((key->keycode =3D=3D KEY_BRIGHTNESSUP ||
- key->keycode =3D=3D KEY_BRIGHTNESSDOWN) && acpi_video) {
- /* Don't report brightness notifications that will also
- * come via ACPI */
- ;
- } else {
- sparse_keymap_report_entry(dell_wmi_input_dev, key,
- 1, true);
+ pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
+
+ switch (buffer_entry[1]) {
+ for (i =3D 2; i < len; ++i) {
+ switch (buffer_entry[i]) {
+ /* NIC Link is Up */
+ pr_debug("NIC Link is Up\n");
+ break;
+ /* NIC Link is Down */
+ pr_debug("NIC Link is Down\n");
+ break;
+ /* Unknown event but defined in DSDT */
+ /* Unknown event */
+ pr_info("Unknown WMI event type 0x00: "
+ "0x%x\n", (int)buffer_entry[i]);
+ break;
+ }
+ }
+ break;
+ /* Keys pressed */
+ for (i =3D 2; i < len; ++i)
+ dell_wmi_process_key(buffer_entry[i]);
+ break;
+ for (i =3D 2; i < len; ++i) {
+ switch (buffer_entry[i]) {
+ /* Battery unplugged */
+ pr_debug("Battery unplugged\n");
+ break;
+ /* Battery inserted */
+ pr_debug("Battery inserted\n");
+ break;
+ /* Keyboard backlight level changed */
+ pr_debug("Keyboard backlight level "
+ "changed\n");
+ break;
+ /* Unknown event */
+ pr_info("Unknown WMI event type 0x11: "
+ "0x%x\n", (int)buffer_entry[i]);
+ break;
+ }
+ }
+ break;
+ /* Unknown event */
+ pr_info("Unknown WMI event type 0x%x\n",
+ (int)buffer_entry[1]);
+ break;
}
+
+ buffer_entry +=3D len;
+
}
+
kfree(obj);
}
=20
--=20
1.7.9.5
=20
=20

--=20
Darren Hart
Intel Open Source Technology Center

[Pali Rohár]

Post by Darren Hart

Post by Pali Rohár
WMI buffer can contains more events. First value in buffer
is length of event followed by data of specified length.
After that is next length and next data. When length is
zero then there is no more events in bufffer.
This patch adds support for processing all events in buffer
(not only first) and parse more event types (not only
hotkey events). Because of variable length of events
sometimes BIOS fills more hotkeys (or other values) into
single WMI event. In this case this patch process also
these multiple hotkeys (and not only first one).
Some event types are just ignored because kernel is not
interested for them (e.g. NIC Link status, battery unplug,
...).
This patch is based on DSDT table from Dell Latitude E6440.
Code should be backward compatible so will process other
events of old types same as before this patch.
This patch also fixes problem when in kernel log are written
messages about unknown WMI events. Now all know events are
parsed and those which are not interesting for kernel are
dropped without unknown message.

This should probably be done in a separate patch.

It is not possible, because my patch rewrite code for handling
events. Kernel does not print "unknown event" messages when it
parse WMI event and understand specified part.

Post by Darren Hart
Well yes, I should hope so ;-)

Post by Pali Rohár
---
drivers/platform/x86/dell-wmi.c | 157
+++++++++++++++++++++++++++++++-------- 1 file changed,
127 insertions(+), 30 deletions(-)
diff --git a/drivers/platform/x86/dell-wmi.c
b/drivers/platform/x86/dell-wmi.c index 25721bf..3d15949
100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -145,11 +145,35 @@ static const u16
bios_to_linux_keycode[256] __initconst = {
static struct input_dev *dell_wmi_input_dev;
+static void dell_wmi_process_key(int reported_key)
+{
+ const struct key_entry *key;
+
+ key =
sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
+ reported_key);
+ if (!key) {
+ pr_info("Unknown key %x pressed\n", reported_key);
+ return;
+ }
+
+ pr_debug("Key %x pressed\n", reported_key);
+
+ /* Don't report brightness notifications that will also
come via ACPI */ + if ((key->keycode == KEY_BRIGHTNESSUP ||
+ key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
+ return;
+
+ sparse_keymap_report_entry(dell_wmi_input_dev, key, 1,
true); +}
+
static void dell_wmi_notify(u32 value, void *context)
{
struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL
}; union acpi_object *obj;
acpi_status status;
+ acpi_size buffer_size;
+ u16 *buffer_entry, *buffer_end;
+ int len, i;
status = wmi_get_event_data(value, &response);
if (status != AE_OK) {
@@ -158,44 +182,117 @@ static void dell_wmi_notify(u32
value, void *context)
}
obj = (union acpi_object *)response.pointer;
+ if (!obj) {
+ pr_info("no response\n");
+ return;
+ }

If you intend to print this, it should probably be a bit more
informative. Is "info" the right level here? I would imagine
either WARN if this was a bad thing, or DEBUG if this is more
for debugging the driver.

So what you (or somebody else) prefer? warn or debug?

Post by Darren Hart

Post by Pali Rohár
- if (obj && obj->type == ACPI_TYPE_BUFFER) {
- const struct key_entry *key;
- int reported_key;
- u16 *buffer_entry = (u16 *)obj->buffer.pointer;
- int buffer_size = obj->buffer.length/2;
-
- if (buffer_size >= 2 && dell_new_hk_type &&
buffer_entry[1] != 0x10) { - pr_info("Received

unknown

Post by Darren Hart

Post by Pali Rohár
WMI event (0x%x)\n",
- buffer_entry[1]);
- kfree(obj);
- return;
- }
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ pr_info("bad response type %x\n", obj->type);
+ kfree(obj);
+ return;
+ }
+
+ pr_debug("Received WMI event (%*ph)\n",
+ obj->buffer.length, obj->buffer.pointer);
- if (buffer_size >= 3 && (dell_new_hk_type ||
buffer_entry[1] == 0x0)) - reported_key =
(int)buffer_entry[2];
+ buffer_entry = (u16 *)obj->buffer.pointer;
+ buffer_size = obj->buffer.length/2;
+
+ if (!dell_new_hk_type) {
+ if (buffer_size >= 3 && buffer_entry[1] == 0x0)
+ dell_wmi_process_key(buffer_entry[2]);
else if (buffer_size >= 2)
- reported_key = (int)buffer_entry[1] & 0xffff;
- else {
+ dell_wmi_process_key(buffer_entry[1]);

Why can we drop the 0xffff mask now?

Because it is useless (or correct me if not!). Variable
buffer_entry has type u16* so operation "AND 0xFFFF" on 16bit
integer do nothing.

Post by Darren Hart

Post by Pali Rohár
+ else
pr_info("Received unknown WMI event\n");
- kfree(obj);
- return;
+ kfree(obj);
+ return;
+ }
+
+ buffer_end = buffer_entry + buffer_size;
+
+ while (buffer_entry < buffer_end) {
+
+ len = buffer_entry[0];
+ if (len == 0)
+ break;
+
+ len++;
+

Why increment len here? Are you trying to avoid a "len + 1" in
the comparisons below? If so, is using "len * 2" in the debug
message below correct? Please clarify.

in buffer_entry[0] (16 bit integer) is stored length of event (in
16bit units) without first (length) value. And "%*ph" takes size
in bytes (u8). So length in bytes (u8) units is 2 * length in u16
units.

Post by Darren Hart

Post by Pali Rohár
+ if (buffer_entry+len > buffer_end) {

See coding style documentation on operators. Please run
patches through checkpatch.

checkpatch.pl does not show any problem for these lines.

Post by Darren Hart

Post by Pali Rohár
+ pr_info("Invalid length of WMI event\n");

info doesn't see correct here either.

debug or warn?

Post by Darren Hart

Post by Pali Rohár
+ break;
}
- key =
sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
- reported_key);
- if (!key) {
- pr_info("Unknown key %x pressed\n", reported_key);
- } else if ((key->keycode == KEY_BRIGHTNESSUP ||
- key->keycode == KEY_BRIGHTNESSDOWN) &&

acpi_video) {

Post by Darren Hart

Post by Pali Rohár
- /* Don't report brightness notifications that will

also

Post by Darren Hart

Post by Pali Rohár
- * come via ACPI */
- ;
- } else {
- sparse_keymap_report_entry(dell_wmi_input_dev,

key,

Post by Darren Hart

Post by Pali Rohár
- 1, true);
+ pr_debug("Process buffer (%*ph)\n", len*2,

buffer_entry);

Post by Darren Hart

Post by Pali Rohár
+
+ switch (buffer_entry[1]) {
+ for (i = 2; i < len; ++i) {
+ switch (buffer_entry[i]) {
+ /* NIC Link is Up */
+ pr_debug("NIC Link is Up\n");
+ break;
+ /* NIC Link is Down */
+ pr_debug("NIC Link is Down\n");
+ break;
+ /* Unknown event but defined in DSDT */
+ /* Unknown event */
+ pr_info("Unknown WMI event type 0x00: "
+ "0x%x\n", (int)buffer_entry[i]);
+ break;
+ }
+ }
+ break;
+ /* Keys pressed */
+ for (i = 2; i < len; ++i)
+ dell_wmi_process_key(buffer_entry[i]);
+ break;
+ for (i = 2; i < len; ++i) {
+ switch (buffer_entry[i]) {
+ /* Battery unplugged */
+ pr_debug("Battery unplugged\n");
+ break;
+ /* Battery inserted */
+ pr_debug("Battery inserted\n");
+ break;
+ /* Keyboard backlight level changed */
+ pr_debug("Keyboard backlight level "
+ "changed\n");
+ break;
+ /* Unknown event */
+ pr_info("Unknown WMI event type 0x11: "
+ "0x%x\n", (int)buffer_entry[i]);
+ break;
+ }
+ }
+ break;
+ /* Unknown event */
+ pr_info("Unknown WMI event type 0x%x\n",
+ (int)buffer_entry[1]);
+ break;
}
+
+ buffer_entry += len;
+
}
+
kfree(obj);
}