From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1422724AbcFGWaT (ORCPT ); Tue, 7 Jun 2016 18:30:19 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33522 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932638AbcFGWaR (ORCPT ); Tue, 7 Jun 2016 18:30:17 -0400 Date: Wed, 8 Jun 2016 00:30:11 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= To: =?utf-8?B?TWljaGHFgiBLxJlwaWXFhA==?= Cc: Matthew Garrett , Darren Hart , Gabriele Mazzotta , Mario Limonciello , Andy Lutomirski , Alex Hung , platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events Message-ID: <20160607223011.GP29844@pali> References: <1463916983-12562-1-git-send-email-pali.rohar@gmail.com> <1463916983-12562-5-git-send-email-pali.rohar@gmail.com> <20160602104211.GC2456@eudyptula.hq.kempniu.pl> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160602104211.GC2456@eudyptula.hq.kempniu.pl> 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 Thursday 02 June 2016 12:42:11 Michał Kępień wrote: > > -static void dell_wmi_process_key(int reported_key) > > +static void dell_wmi_process_key(int type, int code) > > { > > const struct key_entry *key; > > > > key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev, > > - reported_key); > > + (type << 16) | code); > > if (!key) { > > - pr_info("Unknown key with scancode 0x%x pressed\n", > > - reported_key); > > + pr_info("Unknown key with type 0x%x and code 0x%x pressed\n", > > + type, code); > > Since you updated the switch cases below so that each of them now > consists of four digits, maybe it's a good idea to change the format > specifiers for type and code to %04x for coherency? Ok. > > return; > > } > > > > - pr_debug("Key %x pressed\n", reported_key); > > + pr_debug("Key with type 0x%x and code 0x%x pressed\n", type, code); > > The same applies here. Ok. > > + case 0x0000: > > + /* One key pressed or event occurred */ > > + if (len > 2) > > + dell_wmi_process_key(0x0000, buffer_entry[2]); > > I am sure you spent way more time analysing this than me, but is this > documented anywhere? Yes, this is the only documented part. Read my email linked to commit message for more details. > Technically speaking, this is a behavioral change > which causes events to be lost on any Dell model which is capable of > stuffing more than one key code into a single type 0x0000 WMI event (not > that I know of any such model). I do not think. Other values in that buffer are not scan codes of other keys, but additional events. See that table with comments (those comments which you suggested to change). > > + /* Other entries in buffer could contain additional information */ > > This comment exceeds 80 characters, but do you think it is needed at > all? What does the reader gain by reading it? Any additional > information that follows the key code is ignored by kernel code anyway. It is just documentation purpose, to understand why we are processing only buffer_entry[2] and why are ignoring anything else after it. > > break; > > @@ -576,21 +525,71 @@ static int __init dell_wmi_input_setup(void) > > goto err_free_dev; > > } > > > > + keymap = kcalloc(dmi_results.keymap_size + > > + ARRAY_SIZE(dell_wmi_keymap_type_0000) + > > + ARRAY_SIZE(dell_wmi_keymap_type_0010) + > > + ARRAY_SIZE(dell_wmi_keymap_type_0011) + > > + 1, > > + sizeof(struct key_entry), GFP_KERNEL); > > + if (!keymap) { > > + err = -ENOMEM; > > + goto err_free_dev; > > You're potentially leaking dmi_results.keymap here. You are right, I will fix it. > > + } > > + > > + /* Append table with events of type 0x0010 which comes from DMI */ > > if (dmi_results.keymap) { > > - dell_new_hk_type = true; > > + for (i = 0; i < dmi_results.keymap_size; i++) { > > + keymap[pos] = dmi_results.keymap[i]; > > + keymap[pos].code |= (0x0010 << 16); > > + pos++; > > + } > > + kfree(dmi_results.keymap); > > + } > > Nit, but is the enclosing conditional expression needed any more, now > that you got rid of dell_new_hk_type? If the 0xB2 DMI table is not > found then dmi_results.keymap_size will be 0 and thus the for loop above > doesn't do anything and it is okay to pass a null pointer to kfree(). Yes, it is not needed anymore. I will delete it. > > > > - err = sparse_keymap_setup(dell_wmi_input_dev, > > - dmi_results.keymap, NULL); > > + /* Append table with extra events of type 0x0010 which are not in DMI */ > > + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0010); i++) { > > + const struct key_entry *entry = &dell_wmi_keymap_type_0010[i]; > > > > /* > > - * Sparse keymap library makes a copy of keymap so we > > - * don't need the original one that was allocated. > > + * Check if we've already found this scancode. This takes > > + * quadratic time, but it doesn't matter unless the list > > + * of extra keys gets very long. > > */ > > - kfree(dmi_results.keymap); > > - } else { > > - err = sparse_keymap_setup(dell_wmi_input_dev, > > - dell_wmi_legacy_keymap, NULL); > > + if (dmi_results.keymap_size && > > + have_scancode(entry->code | (0x0010 << 16), > > + keymap, dmi_results.keymap_size) > > + ) > > + continue; > > Is the first part of this conditional expression really needed? If > dmi_results.keymap_size is 0 then have_scancode() will simply return > false, so the only disadvantage of removing this check is the overhead > of calling have_scancode() for every iteration of the loop, but I > believe that overhead is negligible as this is not performance-critical > code. It is linear scan of whole table and so above two loops has something like quadratic time complexity. List dell_wmi_keymap_type_0010 is not too long for now, so it is OK. But still it is better not to call have_scancode() everytime if it is not needed. Once we have big list of events we could switch code to use some hash tables... -- Pali Rohár pali.rohar@gmail.com