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-21-2010, 11:30 AM
Eric Miao
 
Default UBUNTU: SAUCE: Allow registration of handler to multiple WMI events with same GUID

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
",
> - * * * * * * * * * * * * * * * 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
>
--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 11-23-2010, 06:09 PM
Tim Gardner
 
Default UBUNTU: SAUCE: Allow registration of handler to multiple WMI events with same GUID

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

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 11-24-2010, 12:03 PM
Colin Ian King
 
Default UBUNTU: SAUCE: Allow registration of handler to multiple WMI events with same GUID

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
",
> >> - 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:13 PM
Tim Gardner
 
Default UBUNTU: SAUCE: Allow registration of handler to multiple WMI events with same GUID

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.

> @@ -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
",
> - 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 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 05:52 AM.

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