Re: [PATCH] dell-wmi: Update code for processing WMI events

[Darren Hart <dvhart@infradead.org>]

On Tue, Oct 21, 2014 at 12:15:24AM +0200, Pali Rohár wrote:
> WMI buffer can contains more events. First value in buffer is length of event
> followed by data of specified length. After that is next length and next data.
> When length is zero then there is no more events in bufffer.
> 
> This patch adds support for processing all events in buffer (not only first)
> and parse more event types (not only hotkey events). Because of variable length
> of events sometimes BIOS fills more hotkeys (or other values) into single WMI
> event. In this case this patch process also these multiple hotkeys (and not
> only first one).
> 
> Some event types are just ignored because kernel is not interested for them
> (e.g. NIC Link status, battery unplug, ...).
> 
> This patch is based on DSDT table from Dell Latitude E6440. Code should be
> backward compatible so will process other events of old types same as before
> this patch.
> 
> This patch also fixes problem when in kernel log are written messages about
> unknown WMI events. Now all know events are parsed and those which are not
> interesting for kernel are dropped without unknown message.

This should probably be done in a separate patch.

> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Tested-by: Pali Rohár <pali.rohar@gmail.com>

Well yes, I should hope so ;-)

> ---
>  drivers/platform/x86/dell-wmi.c |  157 +++++++++++++++++++++++++++++++--------
>  1 file changed, 127 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 25721bf..3d15949 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -145,11 +145,35 @@ static const u16 bios_to_linux_keycode[256] __initconst = {
>  
>  static struct input_dev *dell_wmi_input_dev;
>  
> +static void dell_wmi_process_key(int reported_key)
> +{
> +	const struct key_entry *key;
> +
> +	key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
> +						reported_key);
> +	if (!key) {
> +		pr_info("Unknown key %x pressed\n", reported_key);
> +		return;
> +	}
> +
> +	pr_debug("Key %x pressed\n", reported_key);
> +
> +	/* Don't report brightness notifications that will also come via ACPI */
> +	if ((key->keycode == KEY_BRIGHTNESSUP ||
> +	     key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
> +		return;
> +
> +	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
> +}
> +
>  static void dell_wmi_notify(u32 value, void *context)
>  {
>  	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>  	union acpi_object *obj;
>  	acpi_status status;
> +	acpi_size buffer_size;
> +	u16 *buffer_entry, *buffer_end;
> +	int len, i;
>  
>  	status = wmi_get_event_data(value, &response);
>  	if (status != AE_OK) {
> @@ -158,44 +182,117 @@ static void dell_wmi_notify(u32 value, void *context)
>  	}
>  
>  	obj = (union acpi_object *)response.pointer;
> +	if (!obj) {
> +		pr_info("no response\n");
> +		return;
> +	}


If you intend to print this, it should probably be a bit more informative. Is
"info" the right level here? I would imagine either WARN if this was a bad
thing, or DEBUG if this is more for debugging the driver.


> -	if (obj && obj->type == ACPI_TYPE_BUFFER) {
> -		const struct key_entry *key;
> -		int reported_key;
> -		u16 *buffer_entry = (u16 *)obj->buffer.pointer;
> -		int buffer_size = obj->buffer.length/2;
> -
> -		if (buffer_size >= 2 && dell_new_hk_type && buffer_entry[1] != 0x10) {
> -			pr_info("Received unknown WMI event (0x%x)\n",
> -				buffer_entry[1]);
> -			kfree(obj);
> -			return;
> -		}
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		pr_info("bad response type %x\n", obj->type);
> +		kfree(obj);
> +		return;
> +	}
> +
> +	pr_debug("Received WMI event (%*ph)\n",
> +		obj->buffer.length, obj->buffer.pointer);
>  
> -		if (buffer_size >= 3 && (dell_new_hk_type || buffer_entry[1] == 0x0))
> -			reported_key = (int)buffer_entry[2];
> +	buffer_entry = (u16 *)obj->buffer.pointer;
> +	buffer_size = obj->buffer.length/2;
> +
> +	if (!dell_new_hk_type) {
> +		if (buffer_size >= 3 && buffer_entry[1] == 0x0)
> +			dell_wmi_process_key(buffer_entry[2]);
>  		else if (buffer_size >= 2)
> -			reported_key = (int)buffer_entry[1] & 0xffff;
> -		else {
> +			dell_wmi_process_key(buffer_entry[1]);


Why can we drop the 0xffff mask now?


> +		else
>  			pr_info("Received unknown WMI event\n");
> -			kfree(obj);
> -			return;
> +		kfree(obj);
> +		return;
> +	}
> +
> +	buffer_end = buffer_entry + buffer_size;
> +
> +	while (buffer_entry < buffer_end) {
> +
> +		len = buffer_entry[0];
> +		if (len == 0)
> +			break;
> +
> +		len++;
> +


Why increment len here? Are you trying to avoid a "len + 1" in the comparisons
below? If so, is using "len * 2" in the debug message below correct? Please
clarify.


> +		if (buffer_entry+len > buffer_end) {


See coding style documentation on operators. Please run patches through
checkpatch.


> +			pr_info("Invalid length of WMI event\n");

info doesn't see correct here either.

> +			break;
>  		}
>  
> -		key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
> -							reported_key);
> -		if (!key) {
> -			pr_info("Unknown key %x pressed\n", reported_key);
> -		} else if ((key->keycode == KEY_BRIGHTNESSUP ||
> -			    key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video) {
> -			/* Don't report brightness notifications that will also
> -			 * come via ACPI */
> -			;
> -		} else {
> -			sparse_keymap_report_entry(dell_wmi_input_dev, key,
> -						   1, true);
> +		pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
> +
> +		switch (buffer_entry[1]) {
> +		case 0x00:
> +			for (i = 2; i < len; ++i) {
> +				switch (buffer_entry[i]) {
> +				case 0xe043:
> +					/* NIC Link is Up */
> +					pr_debug("NIC Link is Up\n");
> +					break;
> +				case 0xe044:
> +					/* NIC Link is Down */
> +					pr_debug("NIC Link is Down\n");
> +					break;
> +				case 0xe045:
> +					/* Unknown event but defined in DSDT */
> +				default:
> +					/* Unknown event */
> +					pr_info("Unknown WMI event type 0x00: "
> +						"0x%x\n", (int)buffer_entry[i]);
> +					break;
> +				}
> +			}
> +			break;
> +		case 0x10:
> +			/* Keys pressed */
> +			for (i = 2; i < len; ++i)
> +				dell_wmi_process_key(buffer_entry[i]);
> +			break;
> +		case 0x11:
> +			for (i = 2; i < len; ++i) {
> +				switch (buffer_entry[i]) {
> +				case 0xfff0:
> +					/* Battery unplugged */
> +					pr_debug("Battery unplugged\n");
> +					break;
> +				case 0xfff1:
> +					/* Battery inserted */
> +					pr_debug("Battery inserted\n");
> +					break;
> +				case 0x01e1:
> +				case 0x02ea:
> +				case 0x02eb:
> +				case 0x02ec:
> +				case 0x02f6:
> +					/* Keyboard backlight level changed */
> +					pr_debug("Keyboard backlight level "
> +						 "changed\n");
> +					break;
> +				default:
> +					/* Unknown event */
> +					pr_info("Unknown WMI event type 0x11: "
> +						"0x%x\n", (int)buffer_entry[i]);
> +					break;
> +				}
> +			}
> +			break;
> +		default:
> +			/* Unknown event */
> +			pr_info("Unknown WMI event type 0x%x\n",
> +				(int)buffer_entry[1]);
> +			break;
>  		}
> +
> +		buffer_entry += len;
> +
>  	}
> +
>  	kfree(obj);
>  }
>  
> -- 
> 1.7.9.5
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center