Incorrect key code parsing in dell-wmi.c since 5ea2559

[Pali RohÃr]

Hello,

I found another problem in dell-wmi.c code which is still partially in
mainline kernel since commit 5ea2559726b786283236835dc2905c23b36ac91c:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5ea2559726b786283236835dc2905c23b36ac91c

===============================================
commit 5ea2559726b786283236835dc2905c23b36ac91c
Author: Rezwanul Kabir <Rezwanul_Kabir@xxxxxxxx>
Date:   Mon Nov 2 12:00:42 2009 -0500

    dell-wmi: Add support for new Dell systems
    
    Newer Dell systems support HotKey features differently from legacy
    systems.  A new vendor specifc HotKey SMBIOS table (Type 0xB2) is
    defined. This table contains a mapping between scancode and the
    corresponding predefined keyfunction ( i.e. keycode).. Also, a new
    ACPI-WMI event type (called KeyIDList) with a value of 0x0010 is
    defined. Any BIOS containing 0xB2 table will send hotkey notifications
    using KeyIDList event.
    
    This is Rezwanul's patch, updated to ensure that brightness events are
    not sent if the backlight is controlled via ACPI and with the default
    keycode for the display output switching altered to match desktop
    expectations.
    
    Signed-off-by: Rezwanul Kabir <Rezwanul_Kabir@xxxxxxxx>
    Signed-off-by: Matthew Garrett <mjg@xxxxxxxxxx>
    Signed-off-by: Len Brown <len.brown@xxxxxxxxx>
===============================================

Before this commit WMI event buffer was parsed as:

  int *buffer = (int *)obj->buffer.pointer;
  key = dell_wmi_get_entry_by_scancode(buffer[1] & 0xFFFF);

So basically 4th-7th bytes are parsed.

After this commit WMI event buffer is parsed as:

  u16 *buffer_entry = (u16 *)obj->buffer.pointer;
  reported_key = (int)buffer_entry[1] & 0xffff;
  key = dell_wmi_get_entry_by_scancode(reported_key);

Which means that 2nd and 3rd bytes are parsed.

Apparently this commit changed what kernel parse as keycode. And I bet
this is some copy-paste error and not correct code. Variable buffer was
changed from (int*) to (u16*) and which change could be "hidden" at time
of code review.

Next there is commit which somehow is trying to fix user problems:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d5164dbf1f651d1e955b158fb70a9c844cc91cd1

===============================================
commit d5164dbf1f651d1e955b158fb70a9c844cc91cd1
Author: Islam Amer <pharon@xxxxxxxxx>
Date:   Thu Jun 24 13:39:47 2010 -0400

    dell-wmi: Add support for eject key on Dell Studio 1555
    
    Fixes pressing the eject key on Dell Studio 1555 does not work and produces
    message :
    
    dell-wmi: Unknown key 0 pressed
    
    Signed-off-by: Islam Amer <pharon@xxxxxxxxx>
===============================================

It changes parsing WMI event buffer to:

  u16 *buffer_entry = (u16 *)obj->buffer.pointer;
  if (buffer_entry[1] == 0x0)
    reported_key = (int)buffer_entry[2] & 0xffff;
  else
    reported_key = (int)buffer_entry[1] & 0xffff;
  key = dell_wmi_get_entry_by_scancode(reported_key);

My idea is that Islam Amer tried to fix problem introduced in commit
5ea2559726b786283236835dc2905c23b36ac91c.

According to some available very-very old Dell ACPI-WMI documentation at

http://vpsservice1.sampo.com.tw/sampo_update/document/jimmy/ACPI-WMI%20.pdf

and also from information from commit message 5ea2559 format of WMI is
(u16*)[length, type, data, ...].

Because type for hotkey is 0x0000 then it expected that kernel reported
always key 0x0000...

So I would propose to drop parsing "buffer_entry[1]" as key code and
rewrite dell_wmi_notify function to always process WMI buffer as
[length, type, data...].

What do you think about it? It also simplify notify code as there is one
branch for dell_new_hk_type and one for old parsing (with that entry[1]).

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.