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

[Darren Hart]

On Sat, Jul 04, 2015 at 07:06:48PM +0200, 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@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]).
> 

Without documentation for the older as well as newer laptops, it's possible the
driver evolved to support newer ones while breaking older models. As you
mentioned to me, getting some current documentation would be ideal.

Without it, the best approach is a functional patch which we can ask users to
help us test.

-- 
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/