Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Darren Hart]

On Tue, Feb 16, 2016 at 03:50:28PM +0100, MichaÅ KÄpieÅ wrote:
> On some laptop models (e.g. Dell Vostro V131), WMI events are not
> generated until a specific SMBIOS request is issued to register an event
> listener [1].  As there seems to be no ACPI method or SMBIOS request to
> determine without possible side effects whether a given machine needs to
> issue this SMBIOS request in order to receive WMI events, DMI matching
> is used to whitelist the models which need it.
> 
> [1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html
> 
> Signed-off-by: MichaÅ KÄpieÅ <kernel@xxxxxxxxxx>
> ---
>  drivers/platform/x86/Kconfig    |    1 +
>  drivers/platform/x86/dell-wmi.c |   49 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 3e4d9c3..5ceb53a 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -122,6 +122,7 @@ config DELL_WMI
>  	depends on ACPI_WMI
>  	depends on INPUT
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> +	depends on DELL_SMBIOS
>  	select INPUT_SPARSEKMAP
>  	---help---
>  	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 368e193..ca8233a 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -37,6 +37,7 @@
>  #include <linux/string.h>
>  #include <linux/dmi.h>
>  #include <acpi/video.h>
> +#include "dell-smbios.h"
>  
>  MODULE_AUTHOR("Matthew Garrett <mjg@xxxxxxxxxx>");
>  MODULE_AUTHOR("Pali RohÃr <pali.rohar@xxxxxxxxx>");
> @@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
>  #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
>  
>  static u32 dell_wmi_interface_version;
> +static bool wmi_requires_smbios_request;
>  
>  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>  MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
>  
> +static int __init dmi_matched(const struct dmi_system_id *dmi)
> +{
> +	wmi_requires_smbios_request = 1;
> +	return 1;
> +}
> +
> +static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
> +	{
> +		.callback = dmi_matched,
> +		.ident = "Dell Vostro V131",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> +		},
> +	},
> +	{ }
> +};
> +
>  /*
>   * Certain keys are flagged as KE_IGNORE. All of these are either
>   * notifications (rather than requests for change) or are also sent
> @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
>  {
>  	int err;
>  	acpi_status status;
> +	struct calling_interface_buffer *buffer;

Please place the longest line first, and move int err to the last declaration.
When changing declarations of local variables, please use "Reverse Christmas
Tree" order (longest line to shortest line) wherever possible.

>  
>  	if (!wmi_has_guid(DELL_EVENT_GUID) ||
>  	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
> @@ -538,12 +559,40 @@ static int __init dell_wmi_init(void)
>  		return -ENODEV;
>  	}
>  
> +	dmi_check_system(dell_wmi_smbios_list);
> +
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		buffer->input[3] = 0x1;
> +		dell_smbios_send_request(17, 3);
> +		err = buffer->output[0];
> +		dell_smbios_release_buffer();
> +		if (err) {
> +			pr_err("Failed to enable WMI (error %d)\n", err);
> +			wmi_remove_notify_handler(DELL_EVENT_GUID);
> +			dell_wmi_input_destroy();
> +			return dell_smbios_error(err);
> +		}
> +	}
> +
>  	return 0;
>  }
>  module_init(dell_wmi_init);
>  
>  static void __exit dell_wmi_exit(void)
>  {
> +	struct calling_interface_buffer *buffer;
> +
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		dell_smbios_send_request(17, 3);
> +		dell_smbios_release_buffer();
> +	}
> +

Pali's point about documenting the hardcoded values and eliminating the code
duplication with a function (inline) is a good one.

Otherwise, this series looks good to me. Looking forward to merging v4.

-- 
Darren Hart
Intel Open Source Technology Center