diff mbox

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

Message ID 1290194207-13792-2-git-send-email-colin.king@canonical.com
State Rejected
Delegated to: Tim Gardner
Headers show

Commit Message

Colin Ian King Nov. 19, 2010, 7:16 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

BugLink: http://bugs.launchpad.net/bugs/676997

WMI data blocks can contain WMI events with the same GUID but with

Comments

Eric Miao Nov. 21, 2010, 12:30 p.m. UTC | #1 
On Sat, Nov 20, 2010 at 3:16 AM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@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@canonical.com>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Colin,

Thanks for the patch, it is very straight-forward and clean, much more
carefully considered in overall.

Acked.

> ---
>  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) {
> --
> 1.7.0.4
>
>
> --
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
Tim Gardner Nov. 23, 2010, 7:09 p.m. UTC | #2 
On 11/19/2010 12:16 PM, Colin King wrote:
> From: Colin Ian King<colin.king@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@canonical.com>
> Signed-off-by: Colin Ian King<colin.king@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.
Brad Figg Nov. 23, 2010, 7:51 p.m. UTC | #3 
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@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@canonical.com>
>> Signed-off-by: Colin Ian King<colin.king@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
Colin Ian King Nov. 24, 2010, 1:03 p.m. UTC | #4 
Comments taken. Will re-work it. Thanks for the input.

Colin
On Tue, 2010-11-23 at 11:51 -0800, Brad Figg wrote:
> 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@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@canonical.com>
> >> Signed-off-by: Colin Ian King<colin.king@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@canonical.com http://www.canonical.com
>
diff mbox

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 |  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) {