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
[email protected]
