Re: [PATCH 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

["Pali Rohár" <pali.rohar@gmail.com>]

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