Cleanup and update hp-wmi to match the latest API

Cleanup and update hp-wmi to match the latest API

[Thomas Renninger]

The first 6 patches are not that intrusive and it would be great
to see them pushed soon.

The last one needs discussing.

Thanks,

     Thomas

[PATCH 1/7] x86 platform drivers: hp-wmi Reorder event id processing

[Thomas Renninger]

Event id 0x4 defines the hotkey event.
No need (or even wrong) to query HPWMI_HOTKEY_QUERY if event id is != 0x4.

Reorder the eventcode conditionals and use switch case instead of if/else.
Use an enum for the event ids cases.


Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg@redhat.com
CC: linux-acpi@vger.kernel.org
CC: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/hp-wmi.c |   51 +++++++++++++++++++++++++---------------
 1 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 51c07a0..f1c1862 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -58,6 +58,12 @@ enum hp_wmi_radio {
 	HPWMI_WWAN = 2,
 };
 
+enum hp_wmi_event_ids {
+	HPWMI_DOCK_EVENT = 1,
+	HPWMI_BEZEL_BUTTON = 4,
+	HPWMI_WIRELESS = 5,
+};
+
 static int __devinit hp_wmi_bios_setup(struct platform_device *device);
 static int __exit hp_wmi_bios_remove(struct platform_device *device);
 static int hp_wmi_resume_handler(struct device *device);
@@ -338,7 +344,7 @@ static void hp_wmi_notify(u32 value, void *context)
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
 	static struct key_entry *key;
 	union acpi_object *obj;
-	int eventcode;
+	int eventcode, key_code;
 	acpi_status status;
 
 	status = wmi_get_event_data(value, &response);
@@ -357,28 +363,32 @@ static void hp_wmi_notify(u32 value, void *context)
 
 	eventcode = *((u8 *) obj->buffer.pointer);
 	kfree(obj);
-	if (eventcode == 0x4)
-		eventcode = hp_wmi_perform_query(HPWMI_HOTKEY_QUERY, 0,
-						0);
-	key = hp_wmi_get_entry_by_scancode(eventcode);
-	if (key) {
-		switch (key->type) {
-		case KE_KEY:
-			input_report_key(hp_wmi_input_dev,
-					 key->keycode, 1);
-			input_sync(hp_wmi_input_dev);
-			input_report_key(hp_wmi_input_dev,
-					 key->keycode, 0);
-			input_sync(hp_wmi_input_dev);
-			break;
-		}
-	} else if (eventcode == 0x1) {
+	switch (eventcode) {
+	case HPWMI_DOCK_EVENT:
 		input_report_switch(hp_wmi_input_dev, SW_DOCK,
 				    hp_wmi_dock_state());
 		input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
 				    hp_wmi_tablet_state());
 		input_sync(hp_wmi_input_dev);
-	} else if (eventcode == 0x5) {
+		break;
+	case HPWMI_BEZEL_BUTTON:
+		key_code = hp_wmi_perform_query(HPWMI_HOTKEY_QUERY, 0,
+						 0);
+		key = hp_wmi_get_entry_by_scancode(key_code);
+		if (key) {
+			switch (key->type) {
+			case KE_KEY:
+				input_report_key(hp_wmi_input_dev,
+						 key->keycode, 1);
+				input_sync(hp_wmi_input_dev);
+				input_report_key(hp_wmi_input_dev,
+						 key->keycode, 0);
+				input_sync(hp_wmi_input_dev);
+				break;
+			}
+		}
+		break;
+	case HPWMI_WIRELESS:
 		if (wifi_rfkill)
 			rfkill_set_states(wifi_rfkill,
 					  hp_wmi_get_sw_state(HPWMI_WIFI),
@@ -391,9 +401,12 @@ static void hp_wmi_notify(u32 value, void *context)
 			rfkill_set_states(wwan_rfkill,
 					  hp_wmi_get_sw_state(HPWMI_WWAN),
 					  hp_wmi_get_hw_state(HPWMI_WWAN));
-	} else
+		break;
+	default:
 		printk(KERN_INFO "HP WMI: Unknown key pressed - %x\n",
 			eventcode);
+		break;
+	}
 }
 
 static int __init hp_wmi_input_setup(void)
-- 
1.6.3

[PATCH 2/7] x86 platform drivers: hp-wmi Catch and log unkown event and key codes correctly

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg@redhat.com
CC: linux-acpi@vger.kernel.org
CC: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/hp-wmi.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index f1c1862..7b086dd 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -386,7 +386,9 @@ static void hp_wmi_notify(u32 value, void *context)
 				input_sync(hp_wmi_input_dev);
 				break;
 			}
-		}
+		} else
+			printk(KERN_INFO "HP WMI: Unknown key code - 0x%x\n",
+			       key_code);
 		break;
 	case HPWMI_WIRELESS:
 		if (wifi_rfkill)
@@ -403,8 +405,8 @@ static void hp_wmi_notify(u32 value, void *context)
 					  hp_wmi_get_hw_state(HPWMI_WWAN));
 		break;
 	default:
-		printk(KERN_INFO "HP WMI: Unknown key pressed - %x\n",
-			eventcode);
+		printk(KERN_INFO "HP WMI: Unknown eventcode - %d\n",
+		       eventcode);
 		break;
 	}
 }
-- 
1.6.3

[PATCH 3/7] x86 platform drivers: hp-wmi Use consistent prefix string for messages.

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg@redhat.com
CC: linux-acpi@vger.kernel.org
CC: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/hp-wmi.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 7b086dd..e04715a 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -52,6 +52,8 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
 #define HPWMI_WIRELESS_QUERY 0x5
 #define HPWMI_HOTKEY_QUERY 0xc
 
+#define PREFIX "HP WMI: "
+
 enum hp_wmi_radio {
 	HPWMI_WIFI = 0,
 	HPWMI_BLUETOOTH = 1,
@@ -349,14 +351,14 @@ static void hp_wmi_notify(u32 value, void *context)
 
 	status = wmi_get_event_data(value, &response);
 	if (status != AE_OK) {
-		printk(KERN_INFO "hp-wmi: bad event status 0x%x\n", status);
+		printk(KERN_INFO PREFIX "bad event status 0x%x\n", status);
 		return;
 	}
 
 	obj = (union acpi_object *)response.pointer;
 
 	if (!obj || obj->type != ACPI_TYPE_BUFFER || obj->buffer.length != 8) {
-		printk(KERN_INFO "HP WMI: Unknown response received\n");
+		printk(KERN_INFO PREFIX "Unknown response received\n");
 		kfree(obj);
 		return;
 	}
@@ -387,7 +389,7 @@ static void hp_wmi_notify(u32 value, void *context)
 				break;
 			}
 		} else
-			printk(KERN_INFO "HP WMI: Unknown key code - 0x%x\n",
+			printk(KERN_INFO PREFIX "Unknown key code - 0x%x\n",
 			       key_code);
 		break;
 	case HPWMI_WIRELESS:
@@ -405,7 +407,7 @@ static void hp_wmi_notify(u32 value, void *context)
 					  hp_wmi_get_hw_state(HPWMI_WWAN));
 		break;
 	default:
-		printk(KERN_INFO "HP WMI: Unknown eventcode - %d\n",
+		printk(KERN_INFO PREFIX "Unknown eventcode - %d\n",
 		       eventcode);
 		break;
 	}
-- 
1.6.3

Re: [PATCH 3/7] x86 platform drivers: hp-wmi Use consistent prefix string for messages.

[Dmitry Torokhov]

On Friday 21 May 2010 07:18:11 am Thomas Renninger wrote:
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: mjg@redhat.com
> CC: linux-acpi@vger.kernel.org
> CC: platform-driver-x86@vger.kernel.org
> ---
>  drivers/platform/x86/hp-wmi.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 7b086dd..e04715a 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -52,6 +52,8 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
>  #define HPWMI_WIRELESS_QUERY 0x5
>  #define HPWMI_HOTKEY_QUERY 0xc
> 
> +#define PREFIX "HP WMI: "


Better yet, define pr_fmt() and  switch to using pr_info() and friends.
dev_xxx() is another option too.

-- 
Dmitry

Re: [PATCH 3/7] x86 platform drivers: hp-wmi Use consistent prefix string for messages.

[Thomas Renninger]

On Friday 21 May 2010 08:14:53 pm Dmitry Torokhov wrote:
> On Friday 21 May 2010 07:18:11 am Thomas Renninger wrote:
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > CC: mjg@redhat.com
> > CC: linux-acpi@vger.kernel.org
> > CC: platform-driver-x86@vger.kernel.org
> > ---
> >  drivers/platform/x86/hp-wmi.c |   10 ++++++----
> >  1 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/hp-wmi.c
> > b/drivers/platform/x86/hp-wmi.c index 7b086dd..e04715a 100644
> > --- a/drivers/platform/x86/hp-wmi.c
> > +++ b/drivers/platform/x86/hp-wmi.c
> > @@ -52,6 +52,8 @@
> > MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4"); #define
> > HPWMI_WIRELESS_QUERY 0x5
> >  #define HPWMI_HOTKEY_QUERY 0xc
> >
> > +#define PREFIX "HP WMI: "
>
> Better yet, define pr_fmt() and  switch to using pr_info() and friends.
> dev_xxx() is another option too.

I would have to rediff the other patches...
As printk was already used, I'd appreciate if these can be merged and I can 
also fix this on top.

Thanks,

      Thomas

[PATCH 4/7] x86 platform drivers: hp-wmi Add media key 0x20e8

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg@redhat.com
CC: linux-acpi@vger.kernel.org
CC: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/hp-wmi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index e04715a..f3ae911 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -96,6 +96,7 @@ static struct key_entry hp_wmi_keymap[] = {
 	{KE_KEY, 0x02, KEY_BRIGHTNESSUP},
 	{KE_KEY, 0x03, KEY_BRIGHTNESSDOWN},
 	{KE_KEY, 0x20e6, KEY_PROG1},
+	{KE_KEY, 0x20e8, KEY_MEDIA},
 	{KE_KEY, 0x2142, KEY_MEDIA},
 	{KE_KEY, 0x213b, KEY_INFO},
 	{KE_KEY, 0x2169, KEY_DIRECTION},
-- 
1.6.3

[PATCH 5/7] x86 platform drivers: hp-wmi Set placeholder for unimplemented events

[Thomas Renninger]

And tell the user that those have happened.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg@redhat.com
CC: linux-acpi@vger.kernel.org
CC: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/hp-wmi.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index f3ae911..f869fae 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -53,6 +53,7 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
 #define HPWMI_HOTKEY_QUERY 0xc
 
 #define PREFIX "HP WMI: "
+#define UNIMP "Unimplemented "
 
 enum hp_wmi_radio {
 	HPWMI_WIFI = 0,
@@ -62,8 +63,12 @@ enum hp_wmi_radio {
 
 enum hp_wmi_event_ids {
 	HPWMI_DOCK_EVENT = 1,
+	HPWMI_PARK_HDD = 2,
+	HPWMI_SMART_ADAPTER = 3,
 	HPWMI_BEZEL_BUTTON = 4,
 	HPWMI_WIRELESS = 5,
+	HPWMI_CPU_BATTERY_THROTTLE = 6,
+	HPWMI_LOCK_SWITCH = 7,
 };
 
 static int __devinit hp_wmi_bios_setup(struct platform_device *device);
@@ -374,6 +379,12 @@ static void hp_wmi_notify(u32 value, void *context)
 				    hp_wmi_tablet_state());
 		input_sync(hp_wmi_input_dev);
 		break;
+	case HPWMI_PARK_HDD:
+		printk(KERN_INFO PREFIX UNIMP "HDD park event detected\n");
+		break;
+	case HPWMI_SMART_ADAPTER:
+		printk(KERN_INFO PREFIX UNIMP "Smart adapter event detected\n");
+		break;
 	case HPWMI_BEZEL_BUTTON:
 		key_code = hp_wmi_perform_query(HPWMI_HOTKEY_QUERY, 0,
 						 0);
@@ -407,6 +418,13 @@ static void hp_wmi_notify(u32 value, void *context)
 					  hp_wmi_get_sw_state(HPWMI_WWAN),
 					  hp_wmi_get_hw_state(HPWMI_WWAN));
 		break;
+	case HPWMI_CPU_BATTERY_THROTTLE:
+		printk(KERN_INFO PREFIX UNIMP "CPU throttle because of 3 Cell"
+		       " battery event detected\n");
+		break;
+	case HPWMI_LOCK_SWITCH:
+		printk(KERN_INFO PREFIX UNIMP "Lock switch event detected\n");
+		break;
 	default:
 		printk(KERN_INFO PREFIX "Unknown eventcode - %d\n",
 		       eventcode);
-- 
1.6.3

Re: [PATCH 5/7] x86 platform drivers: hp-wmi Set placeholder for unimplemented events

[Matthew Garrett]

On Fri, May 21, 2010 at 04:18:13PM +0200, Thomas Renninger wrote:
> And tell the user that those have happened.

Marking these as unimplemented seems fine, but I think the message is 
too much - my system generates the smart charger events, and I'd rather 
not have even more spew when I plug/unplug. Mind if I merge this without 
the messages?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 5/7] x86 platform drivers: hp-wmi Set placeholder for unimplemented events

[Thomas Renninger]

On Friday 28 May 2010 15:33:14 Matthew Garrett wrote:
> On Fri, May 21, 2010 at 04:18:13PM +0200, Thomas Renninger wrote:
> > And tell the user that those have happened.
> 
> Marking these as unimplemented seems fine, but I think the message is 
> too much - my system generates the smart charger events, and I'd
> rather not have even more spew when I plug/unplug. Mind if I merge
> this without the messages?
Fine with me, thanks.

     Thomas

[PATCH 6/7] x86 platform drivers: hp-wmi fix buffer size depending on ACPI version

[Thomas Renninger]

Depending on ACPI version (1.0 -> 32 bit) an integer could be
32 or 64 bit long. _WED internal concatenates two integers and
the return value will be 8 byte (2* 32 bit) or 16 byte (2* 64 bit)
long, depending on the ACPI version.

Also the data send with the WMI event is defined to be splitted into:
  - Event ID -> 4 bytes
  - Event Data -> 4 bytes

This gets messed up with new ACPI versions.
But it's a HP BIOS bug that may get fixed in the future
-> Support both, 16 and 8 byte _WED buffers.

Also the wrong assumption that from the event data sent, only the
first byte is relevant got cleaned up that it fits event_id/event_data
as described above.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: robert.moore@intel.com
CC: mjg@redhat.com
CC: platform-driver-x86@vger.kernel.org
CC: linux-acpi@vger.kernel.org
---
 drivers/platform/x86/hp-wmi.c |   38 ++++++++++++++++++++++++++++++--------
 1 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index f869fae..0ebe5de 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -352,7 +352,9 @@ static void hp_wmi_notify(u32 value, void *context)
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
 	static struct key_entry *key;
 	union acpi_object *obj;
-	int eventcode, key_code;
+	u32 event_id, event_data;
+	int key_code;
+	u32 *location;
 	acpi_status status;
 
 	status = wmi_get_event_data(value, &response);
@@ -363,15 +365,34 @@ static void hp_wmi_notify(u32 value, void *context)
 
 	obj = (union acpi_object *)response.pointer;
 
-	if (!obj || obj->type != ACPI_TYPE_BUFFER || obj->buffer.length != 8) {
-		printk(KERN_INFO PREFIX "Unknown response received\n");
+        if (obj || obj->type != ACPI_TYPE_BUFFER) {
+                printk(KERN_INFO "hp-wmi: Unknown response received %d\n",
+		       obj->type);
 		kfree(obj);
 		return;
 	}
 
-	eventcode = *((u8 *) obj->buffer.pointer);
+	/*
+	 * Depending on ACPI version the concatenation of id and event data
+	 * inside _WED function will result in a 8 or 16 byte buffer.
+	 */
+	location = (u32*)obj->buffer.pointer;
+	if (obj->buffer.length == 8) {
+		event_id = *location;
+		event_data = *(location + 1);
+	}
+	else if (obj->buffer.length == 16) {
+		event_id = *location;
+		event_data = *(location + 2);
+	} else {
+		printk(KERN_INFO "hp-wmi: Unknown buffer length %d\n",
+		       obj->buffer.length);
+                kfree(obj);
+                return;
+        }
 	kfree(obj);
-	switch (eventcode) {
+
+	switch (event_id) {
 	case HPWMI_DOCK_EVENT:
 		input_report_switch(hp_wmi_input_dev, SW_DOCK,
 				    hp_wmi_dock_state());
@@ -380,7 +401,8 @@ static void hp_wmi_notify(u32 value, void *context)
 		input_sync(hp_wmi_input_dev);
 		break;
 	case HPWMI_PARK_HDD:
-		printk(KERN_INFO PREFIX UNIMP "HDD park event detected\n");
+		printk(KERN_INFO PREFIX UNIMP "HDD park event detected -"
+		       " 0x%x\n", event_data);
 		break;
 	case HPWMI_SMART_ADAPTER:
 		printk(KERN_INFO PREFIX UNIMP "Smart adapter event detected\n");
@@ -426,8 +448,8 @@ static void hp_wmi_notify(u32 value, void *context)
 		printk(KERN_INFO PREFIX UNIMP "Lock switch event detected\n");
 		break;
 	default:
-		printk(KERN_INFO PREFIX "Unknown eventcode - %d\n",
-		       eventcode);
+		printk(KERN_INFO PREFIX "Unknown event_id - %d - 0x%x\n",
+		       event_id, event_data);
 		break;
 	}
 }
-- 
1.6.3

[PATCH] x86 platform drivers: hp-wmi fix buffer size depending on ACPI version

[Thomas Renninger]

Depending on ACPI version (1.0 -> 32 bit) an integer could be
32 or 64 bit long. _WED internal concatenates two integers and
the return value will be 8 byte (2* 32 bit) or 16 byte (2* 64 bit)
long, depending on the ACPI version.

Also the data send with the WMI event is defined to be splitted into:
  - Event ID -> 4 bytes
  - Event Data -> 4 bytes

This gets messed up with new ACPI versions.
But it's a HP BIOS bug that may get fixed in the future
-> Support both, 16 and 8 byte _WED buffers.

Also the wrong assumption that from the event data sent, only the
first byte is relevant got cleaned up that it fits event_id/event_data
as described above.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: robert.moore@intel.com
CC: mjg@redhat.com
CC: platform-driver-x86@vger.kernel.org
CC: linux-acpi@vger.kernel.org
---
 drivers/platform/x86/hp-wmi.c |   37 +++++++++++++++++++++++++++++--------
 1 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index f869fae..0ba7015 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -352,7 +352,9 @@ static void hp_wmi_notify(u32 value, void *context)
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
 	static struct key_entry *key;
 	union acpi_object *obj;
-	int eventcode, key_code;
+	u32 event_id, event_data;
+	int key_code;
+	u32 *location;
 	acpi_status status;
 
 	status = wmi_get_event_data(value, &response);
@@ -363,15 +365,33 @@ static void hp_wmi_notify(u32 value, void *context)
 
 	obj = (union acpi_object *)response.pointer;
 
-	if (!obj || obj->type != ACPI_TYPE_BUFFER || obj->buffer.length != 8) {
-		printk(KERN_INFO PREFIX "Unknown response received\n");
+	if (obj || obj->type != ACPI_TYPE_BUFFER) {
+		printk(KERN_INFO "hp-wmi: Unknown response received %d\n",
+		       obj->type);
 		kfree(obj);
 		return;
 	}
 
-	eventcode = *((u8 *) obj->buffer.pointer);
+	/*
+	 * Depending on ACPI version the concatenation of id and event data
+	 * inside _WED function will result in a 8 or 16 byte buffer.
+	 */
+	location = (u32 *)obj->buffer.pointer;
+	if (obj->buffer.length == 8) {
+		event_id = *location;
+		event_data = *(location + 1);
+	} else if (obj->buffer.length == 16) {
+		event_id = *location;
+		event_data = *(location + 2);
+	} else {
+		printk(KERN_INFO "hp-wmi: Unknown buffer length %d\n",
+		       obj->buffer.length);
+		kfree(obj);
+		return;
+	}
 	kfree(obj);
-	switch (eventcode) {
+
+	switch (event_id) {
 	case HPWMI_DOCK_EVENT:
 		input_report_switch(hp_wmi_input_dev, SW_DOCK,
 				    hp_wmi_dock_state());
@@ -380,7 +400,8 @@ static void hp_wmi_notify(u32 value, void *context)
 		input_sync(hp_wmi_input_dev);
 		break;
 	case HPWMI_PARK_HDD:
-		printk(KERN_INFO PREFIX UNIMP "HDD park event detected\n");
+		printk(KERN_INFO PREFIX UNIMP "HDD park event detected -"
+		       " 0x%x\n", event_data);
 		break;
 	case HPWMI_SMART_ADAPTER:
 		printk(KERN_INFO PREFIX UNIMP "Smart adapter event detected\n");
@@ -426,8 +447,8 @@ static void hp_wmi_notify(u32 value, void *context)
 		printk(KERN_INFO PREFIX UNIMP "Lock switch event detected\n");
 		break;
 	default:
-		printk(KERN_INFO PREFIX "Unknown eventcode - %d\n",
-		       eventcode);
+		printk(KERN_INFO PREFIX "Unknown event_id - %d - 0x%x\n",
+		       event_id, event_data);
 		break;
 	}
 }
-- 
1.6.3

Re: [PATCH] x86 platform drivers: hp-wmi fix buffer size depending on ACPI version

[Thomas Renninger]

On Friday 21 May 2010 16:41:43 Thomas Renninger wrote:
> Depending on ACPI version (1.0 -> 32 bit) an integer could be
checkpatch cleaned up version of 6/7 and 7/7 sent.
The others are fine.
Sorry about that.

    Thomas

[PATCH 7/7] X86 platform: hp-wmi Better match the HP WMI query interface

[Thomas Renninger]

- Improve error handling, by explictly return zero for success, error otherwise
- WMI query command can have arbitrary input sized params
- WMI query command can have specific output sized params (0, 4, 128,..) byte

I like to go on here, but this is a rather intrusive change that should
be looked at first. I am sure the one or other thing can be done better or
there might be typo/bug somewhere.

This did not get any testing yet, only compile tested.

Next steps could be:
  - Eventually introduce hp_wmi_perform_{read,write}_query macros
  - Introduce new wireless query interface (0x1B)
  - more

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg@redhat.com
CC: linux-acpi@vger.kernel.org
CC: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/hp-wmi.c |  135 ++++++++++++++++++++++++++++++++---------
 1 files changed, 105 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 0ebe5de..2b8c504 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -80,13 +80,12 @@ struct bios_args {
 	u32 command;
 	u32 commandtype;
 	u32 datasize;
-	u32 data;
+	char *data;
 };
 
 struct bios_return {
 	u32 sigpass;
 	u32 return_code;
-	u32 value;
 };
 
 struct key_entry {
@@ -131,7 +130,26 @@ static struct platform_driver hp_wmi_driver = {
 	.remove = hp_wmi_bios_remove,
 };
 
-static int hp_wmi_perform_query(int query, int write, int value)
+/*
+ * hp_wmi_perform_query
+ *
+ * query:	The commandtype -> What should be queried
+ * write:	The command -> 0 read, 1 write, 3 ODM specific
+ * buffer:	Buffer used as input and/or output
+ * buffersize:	Size of buffer
+ *
+ * returns zero on success
+ *         an HP WMI query specific error code (which is positive)
+ *         -EINVAL if the query was not successful at all
+ *         -EINVAL if the output buffer size exceeds buffersize
+ *
+ * Note: The buffersize must at least be the maximum of the input and output
+ *       size. E.g. Battery info query (0x7) is defined to have 1 byte input
+ *       and 128 byte output. The caller would do:
+ *       buffer = kzalloc(128, GFP_KERNEL);
+ *       ret = hp_wmi_perform_query(0x7, 0, buffer, 128)
+ */
+static int hp_wmi_perform_query(int query, int write, char *buffer, int buffersize)
 {
 	struct bios_return bios_return;
 	acpi_status status;
@@ -140,8 +158,8 @@ static int hp_wmi_perform_query(int query, int write, int value)
 		.signature = 0x55434553,
 		.command = write ? 0x2 : 0x1,
 		.commandtype = query,
-		.datasize = write ? 0x4 : 0,
-		.data = value,
+		.datasize = buffersize,
+		.data = buffer,
 	};
 	struct acpi_buffer input = { sizeof(struct bios_args), &args };
 	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -158,54 +176,89 @@ static int hp_wmi_perform_query(int query, int write, int value)
 	}
 
 	bios_return = *((struct bios_return *)obj->buffer.pointer);
+
+	if (bios_return.return_code) {
+		printk(KERN_WARNING PREFIX "Query %d returned %d\n", query,
+		       bios_return.return_code);
+		kfree(obj);
+		return bios_return.return_code;
+	}
+	if (obj->buffer.length - sizeof(bios_return) > buffersize) {
+		kfree(obj);
+		return -EINVAL;
+	}
+
+	memset(buffer, 0, buffersize);
+	memcpy(buffer, ((char*)obj->buffer.pointer) + sizeof(struct bios_return),
+	       obj->buffer.length - sizeof(bios_return));
 	kfree(obj);
-	if (bios_return.return_code > 0)
-		return bios_return.return_code * -1;
-	else
-		return bios_return.value;
+	return 0;
 }
 
 static int hp_wmi_display_state(void)
 {
-	return hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, 0, 0);
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, 0, (char*)&state,
+				       sizeof(state));
+	if (ret)
+		return -EINVAL;
+	return state;	
 }
 
 static int hp_wmi_hddtemp_state(void)
 {
-	return hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, 0, 0);
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, 0, (char*)&state,
+				       sizeof(state));
+	if (ret)
+		return -EINVAL;
+	return state;	
 }
 
 static int hp_wmi_als_state(void)
 {
-	return hp_wmi_perform_query(HPWMI_ALS_QUERY, 0, 0);
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, 0, (char*)&state,
+				       sizeof(state));
+	if (ret)
+		return -EINVAL;
+	return state;	
 }
 
 static int hp_wmi_dock_state(void)
 {
-	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, 0);
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, (char*)&state,
+				       sizeof(state));
 
-	if (ret < 0)
-		return ret;
+	if (ret)
+		return -EINVAL;
 
-	return ret & 0x1;
+	return state & 0x1;
 }
 
 static int hp_wmi_tablet_state(void)
 {
-	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, 0);
-
-	if (ret < 0)
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, (char*)&state,
+				       sizeof(state));
+	if (ret)
 		return ret;
 
-	return (ret & 0x4) ? 1 : 0;
+	return (state & 0x4) ? 1 : 0;
 }
 
 static int hp_wmi_set_block(void *data, bool blocked)
 {
 	enum hp_wmi_radio r = (enum hp_wmi_radio) data;
 	int query = BIT(r + 8) | ((!blocked) << r);
+	int ret;
 
-	return hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 1, query);
+	ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 1,
+				   (char*)&query, sizeof(query));
+	if (ret)
+		return -EINVAL;
+	return 0;	
 }
 
 static const struct rfkill_ops hp_wmi_rfkill_ops = {
@@ -214,8 +267,13 @@ static const struct rfkill_ops hp_wmi_rfkill_ops = {
 
 static bool hp_wmi_get_sw_state(enum hp_wmi_radio r)
 {
-	int wireless = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, 0);
-	int mask = 0x200 << (r * 8);
+	int wireless;
+	int mask;
+	hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0,
+			     (char*)&wireless, sizeof(wireless));
+	/* TBD: Pass error */
+
+	mask = 0x200 << (r * 8);
 
 	if (wireless & mask)
 		return false;
@@ -225,8 +283,13 @@ static bool hp_wmi_get_sw_state(enum hp_wmi_radio r)
 
 static bool hp_wmi_get_hw_state(enum hp_wmi_radio r)
 {
-	int wireless = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, 0);
-	int mask = 0x800 << (r * 8);
+	int wireless;
+	int mask;
+	hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0,
+			     (char*)&wireless, sizeof(wireless));
+	/* TBD: Pass error */
+
+	mask = 0x800 << (r * 8);
 
 	if (wireless & mask)
 		return false;
@@ -283,7 +346,11 @@ static ssize_t set_als(struct device *dev, struct device_attribute *attr,
 		       const char *buf, size_t count)
 {
 	u32 tmp = simple_strtoul(buf, NULL, 10);
-	hp_wmi_perform_query(HPWMI_ALS_QUERY, 1, tmp);
+	int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, 1, (char*)&tmp,
+				       sizeof(tmp));
+	if (ret)
+		return -EINVAL;
+
 	return count;
 }
 
@@ -353,7 +420,7 @@ static void hp_wmi_notify(u32 value, void *context)
 	static struct key_entry *key;
 	union acpi_object *obj;
 	u32 event_id, event_data;
-	int key_code;
+	int key_code, ret;
 	u32 *location;
 	acpi_status status;
 
@@ -408,8 +475,11 @@ static void hp_wmi_notify(u32 value, void *context)
 		printk(KERN_INFO PREFIX UNIMP "Smart adapter event detected\n");
 		break;
 	case HPWMI_BEZEL_BUTTON:
-		key_code = hp_wmi_perform_query(HPWMI_HOTKEY_QUERY, 0,
-						 0);
+		ret = hp_wmi_perform_query(HPWMI_HOTKEY_QUERY, 0,
+					   (char*)&key_code,
+					   sizeof(key_code));
+		if (ret)
+			break;
 		key = hp_wmi_get_entry_by_scancode(key_code);
 		if (key) {
 			switch (key->type) {
@@ -508,7 +578,12 @@ static void cleanup_sysfs(struct platform_device *device)
 static int __devinit hp_wmi_bios_setup(struct platform_device *device)
 {
 	int err;
-	int wireless = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, 0);
+	int wireless;
+
+	err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, (char*)&wireless, 
+				   sizeof(wireless));
+	if (err)
+		return err;
 
 	err = device_create_file(&device->dev, &dev_attr_display);
 	if (err)
-- 
1.6.3

[PATCH] X86 platform: hp-wmi Better match the HP WMI query interface

[Thomas Renninger]

- Improve error handling, by explictly return zero for success, error otherwise
- WMI query command can have arbitrary input sized params
- WMI query command can have specific output sized params (0, 4, 128,..) byte

I like to go on here, but this is a rather intrusive change that should
be looked at first. I am sure the one or other thing can be done better or
there might be typo/bug somewhere.

This did not get any testing yet, only compile tested.

Next steps could be:
  - Eventually introduce hp_wmi_perform_{read,write}_query macros
  - Introduce new wireless query interface (0x1B)
  - more

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg@redhat.com
CC: linux-acpi@vger.kernel.org
CC: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/hp-wmi.c |  137 ++++++++++++++++++++++++++++++++---------
 1 files changed, 107 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 0ba7015..5d79c3f 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -80,13 +80,12 @@ struct bios_args {
 	u32 command;
 	u32 commandtype;
 	u32 datasize;
-	u32 data;
+	char *data;
 };
 
 struct bios_return {
 	u32 sigpass;
 	u32 return_code;
-	u32 value;
 };
 
 struct key_entry {
@@ -131,7 +130,27 @@ static struct platform_driver hp_wmi_driver = {
 	.remove = hp_wmi_bios_remove,
 };
 
-static int hp_wmi_perform_query(int query, int write, int value)
+/*
+ * hp_wmi_perform_query
+ *
+ * query:	The commandtype -> What should be queried
+ * write:	The command -> 0 read, 1 write, 3 ODM specific
+ * buffer:	Buffer used as input and/or output
+ * buffersize:	Size of buffer
+ *
+ * returns zero on success
+ *         an HP WMI query specific error code (which is positive)
+ *         -EINVAL if the query was not successful at all
+ *         -EINVAL if the output buffer size exceeds buffersize
+ *
+ * Note: The buffersize must at least be the maximum of the input and output
+ *       size. E.g. Battery info query (0x7) is defined to have 1 byte input
+ *       and 128 byte output. The caller would do:
+ *       buffer = kzalloc(128, GFP_KERNEL);
+ *       ret = hp_wmi_perform_query(0x7, 0, buffer, 128)
+ */
+static int hp_wmi_perform_query(int query, int write, char *buffer,
+				int buffersize)
 {
 	struct bios_return bios_return;
 	acpi_status status;
@@ -140,8 +159,8 @@ static int hp_wmi_perform_query(int query, int write, int value)
 		.signature = 0x55434553,
 		.command = write ? 0x2 : 0x1,
 		.commandtype = query,
-		.datasize = write ? 0x4 : 0,
-		.data = value,
+		.datasize = buffersize,
+		.data = buffer,
 	};
 	struct acpi_buffer input = { sizeof(struct bios_args), &args };
 	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -158,54 +177,90 @@ static int hp_wmi_perform_query(int query, int write, int value)
 	}
 
 	bios_return = *((struct bios_return *)obj->buffer.pointer);
+
+	if (bios_return.return_code) {
+		printk(KERN_WARNING PREFIX "Query %d returned %d\n", query,
+		       bios_return.return_code);
+		kfree(obj);
+		return bios_return.return_code;
+	}
+	if (obj->buffer.length - sizeof(bios_return) > buffersize) {
+		kfree(obj);
+		return -EINVAL;
+	}
+
+	memset(buffer, 0, buffersize);
+	memcpy(buffer,
+	       ((char *)obj->buffer.pointer) + sizeof(struct bios_return),
+	       obj->buffer.length - sizeof(bios_return));
 	kfree(obj);
-	if (bios_return.return_code > 0)
-		return bios_return.return_code * -1;
-	else
-		return bios_return.value;
+	return 0;
 }
 
 static int hp_wmi_display_state(void)
 {
-	return hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, 0, 0);
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, 0, (char *)&state,
+				       sizeof(state));
+	if (ret)
+		return -EINVAL;
+	return state;
 }
 
 static int hp_wmi_hddtemp_state(void)
 {
-	return hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, 0, 0);
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, 0, (char *)&state,
+				       sizeof(state));
+	if (ret)
+		return -EINVAL;
+	return state;
 }
 
 static int hp_wmi_als_state(void)
 {
-	return hp_wmi_perform_query(HPWMI_ALS_QUERY, 0, 0);
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, 0, (char *)&state,
+				       sizeof(state));
+	if (ret)
+		return -EINVAL;
+	return state;
 }
 
 static int hp_wmi_dock_state(void)
 {
-	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, 0);
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, (char *)&state,
+				       sizeof(state));
 
-	if (ret < 0)
-		return ret;
+	if (ret)
+		return -EINVAL;
 
-	return ret & 0x1;
+	return state & 0x1;
 }
 
 static int hp_wmi_tablet_state(void)
 {
-	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, 0);
-
-	if (ret < 0)
+	int state;
+	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, (char *)&state,
+				       sizeof(state));
+	if (ret)
 		return ret;
 
-	return (ret & 0x4) ? 1 : 0;
+	return (state & 0x4) ? 1 : 0;
 }
 
 static int hp_wmi_set_block(void *data, bool blocked)
 {
 	enum hp_wmi_radio r = (enum hp_wmi_radio) data;
 	int query = BIT(r + 8) | ((!blocked) << r);
+	int ret;
 
-	return hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 1, query);
+	ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 1,
+				   (char *)&query, sizeof(query));
+	if (ret)
+		return -EINVAL;
+	return 0;
 }
 
 static const struct rfkill_ops hp_wmi_rfkill_ops = {
@@ -214,8 +269,13 @@ static const struct rfkill_ops hp_wmi_rfkill_ops = {
 
 static bool hp_wmi_get_sw_state(enum hp_wmi_radio r)
 {
-	int wireless = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, 0);
-	int mask = 0x200 << (r * 8);
+	int wireless;
+	int mask;
+	hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0,
+			     (char *)&wireless, sizeof(wireless));
+	/* TBD: Pass error */
+
+	mask = 0x200 << (r * 8);
 
 	if (wireless & mask)
 		return false;
@@ -225,8 +285,13 @@ static bool hp_wmi_get_sw_state(enum hp_wmi_radio r)
 
 static bool hp_wmi_get_hw_state(enum hp_wmi_radio r)
 {
-	int wireless = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, 0);
-	int mask = 0x800 << (r * 8);
+	int wireless;
+	int mask;
+	hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0,
+			     (char *)&wireless, sizeof(wireless));
+	/* TBD: Pass error */
+
+	mask = 0x800 << (r * 8);
 
 	if (wireless & mask)
 		return false;
@@ -283,7 +348,11 @@ static ssize_t set_als(struct device *dev, struct device_attribute *attr,
 		       const char *buf, size_t count)
 {
 	u32 tmp = simple_strtoul(buf, NULL, 10);
-	hp_wmi_perform_query(HPWMI_ALS_QUERY, 1, tmp);
+	int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, 1, (char *)&tmp,
+				       sizeof(tmp));
+	if (ret)
+		return -EINVAL;
+
 	return count;
 }
 
@@ -353,7 +422,7 @@ static void hp_wmi_notify(u32 value, void *context)
 	static struct key_entry *key;
 	union acpi_object *obj;
 	u32 event_id, event_data;
-	int key_code;
+	int key_code, ret;
 	u32 *location;
 	acpi_status status;
 
@@ -407,8 +476,11 @@ static void hp_wmi_notify(u32 value, void *context)
 		printk(KERN_INFO PREFIX UNIMP "Smart adapter event detected\n");
 		break;
 	case HPWMI_BEZEL_BUTTON:
-		key_code = hp_wmi_perform_query(HPWMI_HOTKEY_QUERY, 0,
-						 0);
+		ret = hp_wmi_perform_query(HPWMI_HOTKEY_QUERY, 0,
+					   (char *)&key_code,
+					   sizeof(key_code));
+		if (ret)
+			break;
 		key = hp_wmi_get_entry_by_scancode(key_code);
 		if (key) {
 			switch (key->type) {
@@ -507,7 +579,12 @@ static void cleanup_sysfs(struct platform_device *device)
 static int __devinit hp_wmi_bios_setup(struct platform_device *device)
 {
 	int err;
-	int wireless = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, 0);
+	int wireless;
+
+	err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, (char *)&wireless,
+				   sizeof(wireless));
+	if (err)
+		return err;
 
 	err = device_create_file(&device->dev, &dev_attr_display);
 	if (err)
-- 
1.6.3

Re: Cleanup and update hp-wmi to match the latest API

[Matthew Garrett]

Applied, thanks.

-- 
Matthew Garrett | mjg59@srcf.ucam.org