Patchwork [1/3] UBUNTU: SAUCE: Allow registration of handler to multiple WMI events with same GUID

login
register
mail settings
Submitter Colin King
Date Nov. 24, 2010, 6:53 p.m.
Message ID <1290624788-9335-2-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/72940/
State Accepted
Delegated to: Tim Gardner
Headers show

Comments

Colin King - Nov. 24, 2010, 6:53 p.m.
BugLink: http://bugs.launchpad.net/bugs/676997

WMI data blocks can contain WMI events with the same GUID but with
Tim Gardner - Nov. 24, 2010, 7:13 p.m.
On 11/24/2010 11:53 AM, Colin Ian King wrote:
> BugLink: http://bugs.launchpad.net/bugs/676997
>
> WMI data blocks can contain WMI events with the same GUID but with
> different notifiy_ids.  This patch enables a single event handler
> to be registered and unregistered against all events against with
> the same GUID.  The event handler is passed the notify_id of these
> events and hence can differentiate between the differen events. The
> patch also ensures we only register and unregister a device per
> unique GUID.
>
> The original registration implementation just matched on the first
> event with the matching GUID and left the other events with out a
> handler.
>
> Signed-off-by: Eric Miao<eric.miao@canonical.com>
> Signed-off-by: Colin Ian King<colin.king@canonical.com>
> ---
>   drivers/platform/x86/wmi.c |  125 +++++++++++++++++++++++--------------------
>   1 files changed, 67 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index e4eaa14..3312c35 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -68,6 +68,7 @@ struct wmi_block {
>   	wmi_notify_handler handler;
>   	void *handler_data;
>   	struct device *dev;
> +	bool first_instance;
>   };
>
>   static struct wmi_block wmi_blocks;
> @@ -241,13 +242,16 @@ static bool find_guid(const char *guid_string, struct wmi_block **out)
>   	char tmp[16], guid_input[16];
>   	struct wmi_block *wblock;
>   	struct guid_block *block;
> -	struct list_head *p;
>
>   	wmi_parse_guid(guid_string, tmp);
>   	wmi_swap_bytes(tmp, guid_input);
>
> -	list_for_each(p,&wmi_blocks.list) {
> -		wblock = list_entry(p, struct wmi_block, list);
> +	if ((out == NULL) || (*out == NULL))
> +		wblock = list_entry(&wmi_blocks.list, struct wmi_block, list);
> +	else
> +		wblock = *out;
> +
> +	list_for_each_entry_continue(wblock,&wmi_blocks.list, list) {
>   		block =&wblock->gblock;
>
>   		if (memcmp(block->guid, guid_input, 16) == 0) {

I think the code in this clause should be:

			if (out && *out)
				*out = wblock;
			return 1;

Otherwise it is possible to GP fault on a NULL dereference.

> @@ -555,22 +559,26 @@ static void wmi_notify_debug(u32 value, void *context)
>   acpi_status wmi_install_notify_handler(const char *guid,
>   wmi_notify_handler handler, void *data)
>   {
> -	struct wmi_block *block;
> -	acpi_status status;
> +	struct wmi_block *block = NULL;
> +	acpi_status status = AE_NOT_EXIST;
>
>   	if (!guid || !handler)
>   		return AE_BAD_PARAMETER;
>
> -	if (!find_guid(guid,&block))
> -		return AE_NOT_EXIST;
> +	while (find_guid(guid,&block)) {
> +		acpi_status wmi_status;
>
> -	if (block->handler&&  block->handler != wmi_notify_debug)
> -		return AE_ALREADY_ACQUIRED;
> +		if (block->handler&&  block->handler != wmi_notify_debug)
> +			return AE_ALREADY_ACQUIRED;
>
> -	block->handler = handler;
> -	block->handler_data = data;
> +		block->handler = handler;
> +		block->handler_data = data;
>
> -	status = wmi_method_enable(block, 1);
> +		wmi_status = wmi_method_enable(block, 1);
> +		if ((wmi_status != AE_OK) ||
> +		    ((wmi_status == AE_OK)&&  (status == AE_NOT_EXIST)))
> +			status = wmi_status;
> +	}
>
>   	return status;
>   }
> @@ -583,24 +591,29 @@ EXPORT_SYMBOL_GPL(wmi_install_notify_handler);
>    */
>   acpi_status wmi_remove_notify_handler(const char *guid)
>   {
> -	struct wmi_block *block;
> -	acpi_status status = AE_OK;
> +	struct wmi_block *block = NULL;
> +	acpi_status status = AE_NOT_EXIST;
>
>   	if (!guid)
>   		return AE_BAD_PARAMETER;
>
> -	if (!find_guid(guid,&block))
> -		return AE_NOT_EXIST;
> +	while (find_guid(guid,&block)) {
> +		acpi_status wmi_status;
>
> -	if (!block->handler || block->handler == wmi_notify_debug)
> -		return AE_NULL_ENTRY;
> +		if (!block->handler || block->handler == wmi_notify_debug)
> +			return AE_NULL_ENTRY;
>
> -	if (debug_event) {
> -		block->handler = wmi_notify_debug;
> -	} else {
> -		status = wmi_method_enable(block, 0);
> -		block->handler = NULL;
> -		block->handler_data = NULL;
> +		if (debug_event) {
> +			block->handler = wmi_notify_debug;
> +			status = AE_OK;
> +		} else {
> +			wmi_status = wmi_method_enable(block, 0);
> +			block->handler = NULL;
> +			block->handler_data = NULL;
> +			if ((wmi_status != AE_OK) ||
> +			    ((wmi_status == AE_OK)&&  (status == AE_NOT_EXIST)))
> +				status = wmi_status;
> +		}
>   	}
>   	return status;
>   }
> @@ -717,28 +730,35 @@ static int wmi_create_devs(void)
>   	/* Create devices for all the GUIDs */
>   	list_for_each(p,&wmi_blocks.list) {
>   		wblock = list_entry(p, struct wmi_block, list);
> +		/*
> +		 * Only create device on first instance, as subsequent
> +		 * instances share the same GUID and we need to avoid
> +		 * creating multiple devices with the same GUID
> +		 */
> +		if (wblock->first_instance) {
> +			guid_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> +			if (!guid_dev)
> +				return -ENOMEM;
>
> -		guid_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> -		if (!guid_dev)
> -			return -ENOMEM;
> -
> -		wblock->dev = guid_dev;
> +			wblock->dev = guid_dev;
>
> -		guid_dev->class =&wmi_class;
> -		dev_set_drvdata(guid_dev, wblock);
> +			guid_dev->class =&wmi_class;
> +			dev_set_drvdata(guid_dev, wblock);
>
> -		gblock =&wblock->gblock;
> +			gblock =&wblock->gblock;
>
> -		wmi_gtoa(gblock->guid, guid_string);
> -		dev_set_name(guid_dev, guid_string);
> +			wmi_gtoa(gblock->guid, guid_string);
> +			dev_set_name(guid_dev, guid_string);
>
> -		result = device_register(guid_dev);
> -		if (result)
> -			return result;
> +			result = device_register(guid_dev);
> +			if (result)
> +				return result;
>
> -		result = device_create_file(guid_dev,&dev_attr_modalias);
> -		if (result)
> -			return result;
> +			result = device_create_file(guid_dev,
> +						&dev_attr_modalias);
> +			if (result)
> +				return result;
> +		}
>   	}
>
>   	return 0;
> @@ -755,12 +775,14 @@ static void wmi_remove_devs(void)
>   	list_for_each(p,&wmi_blocks.list) {
>   		wblock = list_entry(p, struct wmi_block, list);
>
> -		guid_dev = wblock->dev;
> -		gblock =&wblock->gblock;
> +		if (wblock->first_instance) {
> +			guid_dev = wblock->dev;
> +			gblock =&wblock->gblock;
>
> -		device_remove_file(guid_dev,&dev_attr_modalias);
> +			device_remove_file(guid_dev,&dev_attr_modalias);
>
> -		device_unregister(guid_dev);
> +			device_unregister(guid_dev);
> +		}
>   	}
>   }
>
> @@ -810,7 +832,6 @@ static __init acpi_status parse_wdg(acpi_handle handle)
>   	union acpi_object *obj;
>   	struct guid_block *gblock;
>   	struct wmi_block *wblock;
> -	char guid_string[37];
>   	acpi_status status;
>   	u32 i, total;
>
> @@ -831,19 +852,6 @@ static __init acpi_status parse_wdg(acpi_handle handle)
>   		return AE_NO_MEMORY;
>
>   	for (i = 0; i<  total; i++) {
> -		/*
> -		  Some WMI devices, like those for nVidia hooks, have a
> -		  duplicate GUID. It's not clear what we should do in this
> -		  case yet, so for now, we'll just ignore the duplicate.
> -		  Anyone who wants to add support for that device can come
> -		  up with a better workaround for the mess then.
> -		*/
> -		if (guid_already_parsed(gblock[i].guid) == true) {
> -			wmi_gtoa(gblock[i].guid, guid_string);
> -			printk(KERN_INFO PREFIX "Skipping duplicate GUID %s\n",
> -				guid_string);
> -			continue;
> -		}
>   		if (debug_dump_wdg)
>   			wmi_dump_wdg(&gblock[i]);
>
> @@ -851,6 +859,7 @@ static __init acpi_status parse_wdg(acpi_handle handle)
>   		if (!wblock)
>   			return AE_NO_MEMORY;
>
> +		wblock->first_instance = !guid_already_parsed(gblock[i].guid);
>   		wblock->gblock = gblock[i];
>   		wblock->handle = handle;
>   		if (debug_event) {
Tim Gardner - Nov. 24, 2010, 7:20 p.m.
On 11/24/2010 12:13 PM, Tim Gardner wrote:
> On 11/24/2010 11:53 AM, Colin Ian King wrote:
>> BugLink: http://bugs.launchpad.net/bugs/676997
>>
>> WMI data blocks can contain WMI events with the same GUID but with
>> different notifiy_ids.  This patch enables a single event handler
>> to be registered and unregistered against all events against with
>> the same GUID.  The event handler is passed the notify_id of these
>> events and hence can differentiate between the differen events. The
>> patch also ensures we only register and unregister a device per
>> unique GUID.
>>
>> The original registration implementation just matched on the first
>> event with the matching GUID and left the other events with out a
>> handler.
>>
>> Signed-off-by: Eric Miao<eric.miao@canonical.com>
>> Signed-off-by: Colin Ian King<colin.king@canonical.com>
>> ---
>>    drivers/platform/x86/wmi.c |  125 +++++++++++++++++++++++--------------------
>>    1 files changed, 67 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index e4eaa14..3312c35 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -68,6 +68,7 @@ struct wmi_block {
>>    	wmi_notify_handler handler;
>>    	void *handler_data;
>>    	struct device *dev;
>> +	bool first_instance;
>>    };
>>
>>    static struct wmi_block wmi_blocks;
>> @@ -241,13 +242,16 @@ static bool find_guid(const char *guid_string, struct wmi_block **out)
>>    	char tmp[16], guid_input[16];
>>    	struct wmi_block *wblock;
>>    	struct guid_block *block;
>> -	struct list_head *p;
>>
>>    	wmi_parse_guid(guid_string, tmp);
>>    	wmi_swap_bytes(tmp, guid_input);
>>
>> -	list_for_each(p,&wmi_blocks.list) {
>> -		wblock = list_entry(p, struct wmi_block, list);
>> +	if ((out == NULL) || (*out == NULL))
>> +		wblock = list_entry(&wmi_blocks.list, struct wmi_block, list);
>> +	else
>> +		wblock = *out;
>> +
>> +	list_for_each_entry_continue(wblock,&wmi_blocks.list, list) {
>>    		block =&wblock->gblock;
>>
>>    		if (memcmp(block->guid, guid_input, 16) == 0) {
>
> I think the code in this clause should be:
>
> 			if (out&&  *out)
> 				*out = wblock;
> 			return 1;
>
> Otherwise it is possible to GP fault on a NULL dereference.
>

On the other hand, I did check all of the call sites for this function, 
and all send correctly initialized 'out' pointers. So,

Acked-by: Tim Gardner <tim.gardner@canonical.com>

rtg

Patch

different notifiy_ids.  This patch enables a single event handler
to be registered and unregistered against all events against with
the same GUID.  The event handler is passed the notify_id of these
events and hence can differentiate between the differen events. The
patch also ensures we only register and unregister a device per
unique GUID.

The original registration implementation just matched on the first
event with the matching GUID and left the other events with out a
handler.

Signed-off-by: Eric Miao <eric.miao@canonical.com>
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/platform/x86/wmi.c |  125 +++++++++++++++++++++++--------------------
 1 files changed, 67 insertions(+), 58 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index e4eaa14..3312c35 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -68,6 +68,7 @@  struct wmi_block {
 	wmi_notify_handler handler;
 	void *handler_data;
 	struct device *dev;
+	bool first_instance;
 };
 
 static struct wmi_block wmi_blocks;
@@ -241,13 +242,16 @@  static bool find_guid(const char *guid_string, struct wmi_block **out)
 	char tmp[16], guid_input[16];
 	struct wmi_block *wblock;
 	struct guid_block *block;
-	struct list_head *p;
 
 	wmi_parse_guid(guid_string, tmp);
 	wmi_swap_bytes(tmp, guid_input);
 
-	list_for_each(p, &wmi_blocks.list) {
-		wblock = list_entry(p, struct wmi_block, list);
+	if ((out == NULL) || (*out == NULL))
+		wblock = list_entry(&wmi_blocks.list, struct wmi_block, list);
+	else
+		wblock = *out;
+
+	list_for_each_entry_continue(wblock, &wmi_blocks.list, list) {
 		block = &wblock->gblock;
 
 		if (memcmp(block->guid, guid_input, 16) == 0) {
@@ -555,22 +559,26 @@  static void wmi_notify_debug(u32 value, void *context)
 acpi_status wmi_install_notify_handler(const char *guid,
 wmi_notify_handler handler, void *data)
 {
-	struct wmi_block *block;
-	acpi_status status;
+	struct wmi_block *block = NULL;
+	acpi_status status = AE_NOT_EXIST;
 
 	if (!guid || !handler)
 		return AE_BAD_PARAMETER;
 
-	if (!find_guid(guid, &block))
-		return AE_NOT_EXIST;
+	while (find_guid(guid, &block)) {
+		acpi_status wmi_status;
 
-	if (block->handler && block->handler != wmi_notify_debug)
-		return AE_ALREADY_ACQUIRED;
+		if (block->handler && block->handler != wmi_notify_debug)
+			return AE_ALREADY_ACQUIRED;
 
-	block->handler = handler;
-	block->handler_data = data;
+		block->handler = handler;
+		block->handler_data = data;
 
-	status = wmi_method_enable(block, 1);
+		wmi_status = wmi_method_enable(block, 1);
+		if ((wmi_status != AE_OK) ||
+		    ((wmi_status == AE_OK) && (status == AE_NOT_EXIST)))
+			status = wmi_status;
+	}
 
 	return status;
 }
@@ -583,24 +591,29 @@  EXPORT_SYMBOL_GPL(wmi_install_notify_handler);
  */
 acpi_status wmi_remove_notify_handler(const char *guid)
 {
-	struct wmi_block *block;
-	acpi_status status = AE_OK;
+	struct wmi_block *block = NULL;
+	acpi_status status = AE_NOT_EXIST;
 
 	if (!guid)
 		return AE_BAD_PARAMETER;
 
-	if (!find_guid(guid, &block))
-		return AE_NOT_EXIST;
+	while (find_guid(guid, &block)) {
+		acpi_status wmi_status;
 
-	if (!block->handler || block->handler == wmi_notify_debug)
-		return AE_NULL_ENTRY;
+		if (!block->handler || block->handler == wmi_notify_debug)
+			return AE_NULL_ENTRY;
 
-	if (debug_event) {
-		block->handler = wmi_notify_debug;
-	} else {
-		status = wmi_method_enable(block, 0);
-		block->handler = NULL;
-		block->handler_data = NULL;
+		if (debug_event) {
+			block->handler = wmi_notify_debug;
+			status = AE_OK;
+		} else {
+			wmi_status = wmi_method_enable(block, 0);
+			block->handler = NULL;
+			block->handler_data = NULL;
+			if ((wmi_status != AE_OK) ||
+			    ((wmi_status == AE_OK) && (status == AE_NOT_EXIST)))
+				status = wmi_status;
+		}
 	}
 	return status;
 }
@@ -717,28 +730,35 @@  static int wmi_create_devs(void)
 	/* Create devices for all the GUIDs */
 	list_for_each(p, &wmi_blocks.list) {
 		wblock = list_entry(p, struct wmi_block, list);
+		/*
+		 * Only create device on first instance, as subsequent
+		 * instances share the same GUID and we need to avoid
+		 * creating multiple devices with the same GUID
+		 */
+		if (wblock->first_instance) {
+			guid_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
+			if (!guid_dev)
+				return -ENOMEM;
 
-		guid_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
-		if (!guid_dev)
-			return -ENOMEM;
-
-		wblock->dev = guid_dev;
+			wblock->dev = guid_dev;
 
-		guid_dev->class = &wmi_class;
-		dev_set_drvdata(guid_dev, wblock);
+			guid_dev->class = &wmi_class;
+			dev_set_drvdata(guid_dev, wblock);
 
-		gblock = &wblock->gblock;
+			gblock = &wblock->gblock;
 
-		wmi_gtoa(gblock->guid, guid_string);
-		dev_set_name(guid_dev, guid_string);
+			wmi_gtoa(gblock->guid, guid_string);
+			dev_set_name(guid_dev, guid_string);
 
-		result = device_register(guid_dev);
-		if (result)
-			return result;
+			result = device_register(guid_dev);
+			if (result)
+				return result;
 
-		result = device_create_file(guid_dev, &dev_attr_modalias);
-		if (result)
-			return result;
+			result = device_create_file(guid_dev,
+						    &dev_attr_modalias);
+			if (result)
+				return result;
+		}
 	}
 
 	return 0;
@@ -755,12 +775,14 @@  static void wmi_remove_devs(void)
 	list_for_each(p, &wmi_blocks.list) {
 		wblock = list_entry(p, struct wmi_block, list);
 
-		guid_dev = wblock->dev;
-		gblock = &wblock->gblock;
+		if (wblock->first_instance) {
+			guid_dev = wblock->dev;
+			gblock = &wblock->gblock;
 
-		device_remove_file(guid_dev, &dev_attr_modalias);
+			device_remove_file(guid_dev, &dev_attr_modalias);
 
-		device_unregister(guid_dev);
+			device_unregister(guid_dev);
+		}
 	}
 }
 
@@ -810,7 +832,6 @@  static __init acpi_status parse_wdg(acpi_handle handle)
 	union acpi_object *obj;
 	struct guid_block *gblock;
 	struct wmi_block *wblock;
-	char guid_string[37];
 	acpi_status status;
 	u32 i, total;
 
@@ -831,19 +852,6 @@  static __init acpi_status parse_wdg(acpi_handle handle)
 		return AE_NO_MEMORY;
 
 	for (i = 0; i < total; i++) {
-		/*
-		  Some WMI devices, like those for nVidia hooks, have a
-		  duplicate GUID. It's not clear what we should do in this
-		  case yet, so for now, we'll just ignore the duplicate.
-		  Anyone who wants to add support for that device can come
-		  up with a better workaround for the mess then.
-		*/
-		if (guid_already_parsed(gblock[i].guid) == true) {
-			wmi_gtoa(gblock[i].guid, guid_string);
-			printk(KERN_INFO PREFIX "Skipping duplicate GUID %s\n",
-				guid_string);
-			continue;
-		}
 		if (debug_dump_wdg)
 			wmi_dump_wdg(&gblock[i]);
 
@@ -851,6 +859,7 @@  static __init acpi_status parse_wdg(acpi_handle handle)
 		if (!wblock)
 			return AE_NO_MEMORY;
 
+		wblock->first_instance = !guid_already_parsed(gblock[i].guid);
 		wblock->gblock = gblock[i];
 		wblock->handle = handle;
 		if (debug_event) {