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

["Pali Rohár" <pali.rohar@gmail.com>]

On Saturday 04 July 2015 19:06:48 Pali Rohár wrote:
> 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@dell.com>
> 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@dell.com>
>     Signed-off-by: Matthew Garrett <mjg@redhat.com>
>     Signed-off-by: Len Brown <len.brown@intel.com>
> ===============================================
> 
> 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@gmail.com>
> 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@gmail.com>
> ===============================================
> 
> 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]).
> 

Hello! What do you think about it? Any objections?

-- 
Pali Rohár
pali.rohar@gmail.com