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

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3948 bytes --]

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]).

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Pali Rohár]

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]).
> 

BUMP!

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

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

[Pali Rohár]

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

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@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]).
> 

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