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

Brad Figg brad.figg at canonical.com
Tue Nov 23 19:51:15 UTC 2010


On 11/23/2010 11:09 AM, Tim Gardner wrote:
> On 11/19/2010 12:16 PM, Colin King wrote:
>> From: Colin Ian King<colin.king at canonical.com>
>>
>> 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 at canonical.com>
>> Signed-off-by: Colin Ian King<colin.king at canonical.com>
>> ---
>>    drivers/platform/x86/wmi.c |  131 ++++++++++++++++++++++++++------------------
>>    1 files changed, 78 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index e4eaa14..70526ca 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;
>> @@ -556,21 +557,34 @@ acpi_status wmi_install_notify_handler(const char *guid,
>>    wmi_notify_handler handler, void *data)
>>    {
>>    	struct wmi_block *block;
>> -	acpi_status status;
>> +	acpi_status status = AE_NOT_EXIST;
>> +	char tmp[16], guid_input[16];
>> +	struct list_head *p;
>>
>>    	if (!guid || !handler)
>>    		return AE_BAD_PARAMETER;
>>
>> -	if (!find_guid(guid,&block))
>> -		return AE_NOT_EXIST;
>> +	wmi_parse_guid(guid, tmp);
>> +	wmi_swap_bytes(tmp, guid_input);
>> +
>> +	list_for_each(p,&wmi_blocks.list) {
>> +		acpi_status wmi_status;
>> +		block = list_entry(p, struct wmi_block, list);
>>
>> -	if (block->handler&&   block->handler != wmi_notify_debug)
>> -		return AE_ALREADY_ACQUIRED;
>> +		if (memcmp(block->gblock.guid, guid_input, 16) == 0) {
>> +			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;
>>    }
>> @@ -584,23 +598,38 @@ 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;
>> +	acpi_status status = AE_NOT_EXIST;
>> +	char tmp[16], guid_input[16];
>> +	struct list_head *p;
>>
>>    	if (!guid)
>>    		return AE_BAD_PARAMETER;
>>
>> -	if (!find_guid(guid,&block))
>> -		return AE_NOT_EXIST;
>> +	wmi_parse_guid(guid, tmp);
>> +	wmi_swap_bytes(tmp, guid_input);
>> +
>> +	list_for_each(p,&wmi_blocks.list) {
>> +		acpi_status wmi_status;
>> +		block = list_entry(p, struct wmi_block, list);
>>
>> -	if (!block->handler || block->handler == wmi_notify_debug)
>> -		return AE_NULL_ENTRY;
>> +		if (memcmp(block->gblock.guid, guid_input, 16) == 0) {
>> +			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 +746,34 @@ 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 +790,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);
>> +		}
>>    	}
>>    }
>>
>> @@ -831,19 +868,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 +875,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) {
>
> This'll work, but its not really in the spirit of the original code
> wherein the mechanics of the list search are isolated within a single
> function. Would it not be a bit cleaner to modify find_guid() such that
> you could call it repeatadly until it fails to find a match? Something like:
>
> p = NULL;
> while ((p=find_guid(guid,&block,&p)) != NULL) {
> 	/* do magic here */
> }
>
> P.S. The patch is also space/tab mangled.

I agree with Tim's comments here. Even if you had to implement an alternate
"find_guid", it would be keeping the other code cleaner.

Brad
-- 
Brad Figg brad.figg at canonical.com http://www.canonical.com




More information about the kernel-team mailing list