[PATCH 1/3] UBUNTU: SAUCE: Allow registration of handler to multiple WMI events with same GUID
Colin Ian King
colin.king at canonical.com
Wed Nov 24 13:03:34 UTC 2010
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 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