From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757357AbbIDKcY (ORCPT ); Fri, 4 Sep 2015 06:32:24 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:35979 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752926AbbIDKcW (ORCPT ); Fri, 4 Sep 2015 06:32:22 -0400 Date: Fri, 4 Sep 2015 12:32:19 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= To: Rezwanul Kabir , Matthew Garrett , Len Brown , Islam Amer , linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org, Gabriele Mazzotta , =?utf-8?B?TWljaGHFgiBLxJlwaWXFhA==?= , Darren Hart , Mario Limonciello Subject: Re: Incorrect key code parsing in dell-wmi.c since 5ea2559 Message-ID: <20150904103219.GF32350@pali> References: <201507041906.48648@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <201507041906.48648@pali> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 > Signed-off-by: Matthew Garrett > Signed-off-by: Len Brown > =============================================== > > 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 > 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 > =============================================== > > 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