FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 11-23-2010, 06:51 PM
Brad Figg
 
Default UBUNTU: SAUCE: Allow registration of handler to multiple WMI events with same GUID

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
",
>> - 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

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 11-24-2010, 06:20 PM
Tim Gardner
 
Default UBUNTU: SAUCE: Allow registration of handler to multiple WMI events with same GUID

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.li st, 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
--
Tim Gardner tim.gardner@canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 

Thread Tools




All times are GMT. The time now is 10:05 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org