[PATCH v2 0/3] Fix SW_TABLET_MODE detection method

[PATCH v2 0/3] Fix SW_TABLET_MODE detection method

[Jorge Lopez]

The intension for this patch was to address SW_TABLET_MODE detection 
problem.  It is during the initial investigation; two other issues were 
identified and are related to the initial task.  

First, several WMI queries were reporting error 0x05 including 
HPWMI_HARDWARE_QUERY that is responsible for returning dock and table 
modes values. See patch v2 part 2 comments for list of WMI queries 
affected.  The driver now reports the appropriate states and values 
correctly.

Lastly, a limiting data size restriction was discovered. 
struct bios_args data member size limits all possible WMI commands 
to those requiring buffer size of 128 bytes or less.  Several WMI 
commands and queries require a buffer size larger than 128 bytes 
hence limiting current and new feature supported by the driver. 
hp_wmi_perform_query function changed to handle the memory 
allocation and release of any required buffer size. 


Jorge Lopez (3):

  Fix SW_TABLET_MODE detection method
  Fix 0x05 error code reported by several WMI calls
  Changing bios_args.data to be dynamically allocated

 drivers/platform/x86/hp-wmi.c | 160 ++++++++++++++++++++++------------
 1 file changed, 104 insertions(+), 56 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] Fix SW_TABLET_MODE detection method

[Jorge Lopez]

The purpose of this patch is to introduce a fix and removal
of the current hack when determining tablet mode status.

Determining the tablet mode status requires reading Byte 0 bit 2 and 3
as reported by HPWMI_HARDWARE_QUERY.  The investigation identified the
failure was rooted in two areas; HPWMI_HARDWARE_QUERY failure (0x05)
and reading Byte 0, bit 2 only to determine the table mode status.
HPWMI_HARDWARE_QUERY WMI failure also rendered the dock state value invalid.

All changes were validated on a ZBook Workstation notebook.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next
---
 drivers/platform/x86/hp-wmi.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 48a46466f086..544fce906ce7 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -35,9 +35,6 @@ MODULE_LICENSE("GPL");
 MODULE_ALIAS("wmi:95F24279-4D7B-4334-9387-ACCDC67EF61C");
 MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
 
-static int enable_tablet_mode_sw = -1;
-module_param(enable_tablet_mode_sw, int, 0444);
-MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=auto, 0=no, 1=yes)");
 
 #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
 #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
@@ -127,6 +124,7 @@ enum hp_wmi_command {
 enum hp_wmi_hardware_mask {
 	HPWMI_DOCK_MASK		= 0x01,
 	HPWMI_TABLET_MASK	= 0x04,
+	HPWMI_DETACHABLE_MASK	= 0x08,
 };
 
 struct bios_return {
@@ -347,12 +345,19 @@ static int hp_wmi_read_int(int query)
 
 static int hp_wmi_hw_state(int mask)
 {
-	int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
+	int state = 0, ret;
 
-	if (state < 0)
-		return state;
+	ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &state,
+				   0, sizeof(state));
 
-	return !!(state & mask);
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
+	/* determine if Detachable mode is enabled */
+	if (HPWMI_TABLET_MASK == mask)
+		state = (state & HPWMI_DETACHABLE_MASK );
+
+	return (state & mask);
 }
 
 static int omen_thermal_profile_set(int mode)
@@ -781,18 +786,16 @@ static int __init hp_wmi_input_setup(void)
 
 	/* Dock */
 	val = hp_wmi_hw_state(HPWMI_DOCK_MASK);
-	if (!(val < 0)) {
+	if (val > 0) {
 		__set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
 		input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
 	}
 
 	/* Tablet mode */
-	if (enable_tablet_mode_sw > 0) {
-		val = hp_wmi_hw_state(HPWMI_TABLET_MASK);
-		if (val >= 0) {
-			__set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
+	val = hp_wmi_hw_state(HPWMI_TABLET_MASK);
+	if (val > 0) {
+		__set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
 			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
-		}
 	}
 
 	err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL);
-- 
2.25.1

[PATCH v2 2/3] Fix 0x05 error code reported by several WMI calls

[Jorge Lopez]

Several WMI queries leverage hp_wmi_read_int function to read their data.
hp_wmi_read_int function returns the appropiate value if the WMI command
requires an input and output buffer size values greater than zero.
WMI queries such HPWMI_HARDWARE_QUERY, HPWMI_WIRELESS2_QUERY,
HPWMI_FEATURE2_QUERY, HPWMI_DISPLAY_QUERY, HPWMI_ALS_QUERY, and
HPWMI_POSTCODEERROR_QUERY requires calling hp_wmi_perform_query function
with input buffer size value of zero.  Any input buffer size greater
than zero will cause error 0x05 to be returned.

All changes were validated on a ZBook Workstation notebook. Additional
validation was included to ensure no other commands were failing or
incorrectly handled.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next
---
 drivers/platform/x86/hp-wmi.c | 64 ++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 544fce906ce7..de715687021a 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -442,7 +442,7 @@ static int __init hp_wmi_bios_2009_later(void)
 {
 	u8 state[128];
 	int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, HPWMI_READ, &state,
-				       sizeof(state), sizeof(state));
+				       0, sizeof(state));
 	if (!ret)
 		return 1;
 
@@ -477,25 +477,37 @@ static const struct rfkill_ops hp_wmi_rfkill_ops = {
 static bool hp_wmi_get_sw_state(enum hp_wmi_radio r)
 {
 	int mask = 0x200 << (r * 8);
+	int ret= 0;
+	int wireless = 0;
+
+	ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless,
+				   0, sizeof(wireless));
 
-	int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
+	if (ret < 0)
+	  return -EINVAL;
 
 	/* TBD: Pass error */
 	WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY");
 
-	return !(wireless & mask);
+	return (wireless & mask);
 }
 
 static bool hp_wmi_get_hw_state(enum hp_wmi_radio r)
 {
 	int mask = 0x800 << (r * 8);
+	int ret= 0;
+	int wireless = 0;
 
-	int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
+	ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless,
+				   0, sizeof(wireless));
+
+	if (ret < 0)
+	  return -EINVAL;
 
 	/* TBD: Pass error */
 	WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY");
 
-	return !(wireless & mask);
+	return (wireless & mask);
 }
 
 static int hp_wmi_rfkill2_set_block(void *data, bool blocked)
@@ -520,7 +532,7 @@ static int hp_wmi_rfkill2_refresh(void)
 	int err, i;
 
 	err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state,
-				   sizeof(state), sizeof(state));
+				   0, sizeof(state));
 	if (err)
 		return err;
 
@@ -546,27 +558,36 @@ static int hp_wmi_rfkill2_refresh(void)
 static ssize_t display_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
-	int value = hp_wmi_read_int(HPWMI_DISPLAY_QUERY);
-	if (value < 0)
-		return value;
+	int value = 0;
+	int ret  = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_READ, &value,
+				   0, sizeof(value));
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
 	return sprintf(buf, "%d\n", value);
 }
 
 static ssize_t hddtemp_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
-	int value = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY);
-	if (value < 0)
-		return value;
+	int value = 0;
+	int ret  = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_READ, &value,
+				   0, sizeof(value));
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
 	return sprintf(buf, "%d\n", value);
 }
 
 static ssize_t als_show(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {
-	int value = hp_wmi_read_int(HPWMI_ALS_QUERY);
-	if (value < 0)
-		return value;
+	int value = 0;
+	int ret  = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_READ, &value,
+				   0, sizeof(value));
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -592,9 +613,12 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
 	/* Get the POST error code of previous boot failure. */
-	int value = hp_wmi_read_int(HPWMI_POSTCODEERROR_QUERY);
-	if (value < 0)
-		return value;
+	int value = 0;
+	int ret  = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_READ, &value,
+				   0, sizeof(value));
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
 	return sprintf(buf, "0x%x\n", value);
 }
 
@@ -609,7 +633,7 @@ static ssize_t als_store(struct device *dev, struct device_attribute *attr,
 		return ret;
 
 	ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_WRITE, &tmp,
-				       sizeof(tmp), sizeof(tmp));
+				   sizeof(tmp), 0);
 	if (ret)
 		return ret < 0 ? ret : -EINVAL;
 
@@ -922,7 +946,7 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
 	int err, i;
 
 	err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state,
-				   sizeof(state), sizeof(state));
+				   0, sizeof(state));
 	if (err)
 		return err < 0 ? err : -EINVAL;
 
-- 
2.25.1

[PATCH v2 3/3] Changing bios_args.data to be dynamically allocated

[Jorge Lopez]

The purpose of this patch is to remove 128 bytes buffer limitation
imposed in bios_args structure.

A limiting factor discovered during this investigation was the struct
bios_args.data size restriction.  The data member size limits all possible
WMI commands to those requiring buffer size of 128 bytes or less.
Several WMI commands and queries require a buffer size larger than 128
bytes hence limiting current and feature supported by the driver.
It is for this reason, struct bios_args.data changed and is dynamically
allocated.  hp_wmi_perform_query function changed to handle the memory
allocation and release of any required buffer size.

All changes were validated on a ZBook Workstation notebook. Additional
validation was included to ensure no other commands were failing or
incorrectly handled.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next
---
 drivers/platform/x86/hp-wmi.c | 67 +++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index de715687021a..11c8e9b6e64a 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -83,12 +83,17 @@ enum hp_wmi_event_ids {
 	HPWMI_BATTERY_CHARGE_PERIOD	= 0x10,
 };
 
+/**
+ * struct bios_args buffer is dynamically allocated.  New WMI command types
+ * were introduced that exceeds 128-byte data size.  Changes to handle
+ * the data size allocation scheme were kept in hp_wmi_perform_qurey function.
+ */
 struct bios_args {
 	u32 signature;
 	u32 command;
 	u32 commandtype;
 	u32 datasize;
-	u8 data[128];
+	u8 data[0];
 };
 
 enum hp_wmi_commandtype {
@@ -258,39 +263,47 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
 	struct bios_return *bios_return;
 	int actual_outsize;
 	union acpi_object *obj;
-	struct bios_args args = {
-		.signature = 0x55434553,
-		.command = command,
-		.commandtype = query,
-		.datasize = insize,
-		.data = { 0 },
-	};
-	struct acpi_buffer input = { sizeof(struct bios_args), &args };
+	size_t bios_args_size = sizeof(struct bios_args) + insize;
+	struct bios_args *args = NULL;
+	struct acpi_buffer input;
 	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
 	int ret = 0;
 
-	mid = encode_outsize_for_pvsz(outsize);
-	if (WARN_ON(mid < 0))
-		return mid;
+	args = kmalloc(bios_args_size, GFP_KERNEL);
+	if (!args)
+		return -ENOMEM;
 
-	if (WARN_ON(insize > sizeof(args.data)))
-		return -EINVAL;
-	memcpy(&args.data[0], buffer, insize);
+	input.length = bios_args_size;
+	input.pointer = args;
 
-	wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
+	mid = encode_outsize_for_pvsz(outsize);
+	if (WARN_ON(mid < 0)) {
+		ret = mid;
+		goto in_free;
+	}
 
-	obj = output.pointer;
+	/* Avoid unnecessary copy to the data buffer if input buffer size is zero */
+	if (insize > 0)
+		memcpy(args->data, buffer, insize);
 
-	if (!obj)
-		return -EINVAL;
+	args->signature = 0x55434553;
+	args->command = command;
+	args->commandtype = query;
+	args->datasize = insize;
 
-	if (obj->type != ACPI_TYPE_BUFFER) {
+	ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
+
+	obj = output.pointer;
+	if (!obj) {
+		pr_warn("query 0x%x returned a null obj 0x%x\n", query, ret);
 		ret = -EINVAL;
-		goto out_free;
+		goto in_free;
 	}
 
-	bios_return = (struct bios_return *)obj->buffer.pointer;
-	ret = bios_return->return_code;
+	if (!ret && obj->type == ACPI_TYPE_BUFFER) {
+		bios_return = (struct bios_return *)obj->buffer.pointer;
+		ret = bios_return->return_code;
+	}
 
 	if (ret) {
 		if (ret != HPWMI_RET_UNKNOWN_COMMAND &&
@@ -299,6 +312,12 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
 		goto out_free;
 	}
 
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret);
+		ret = -EINVAL;
+		goto out_free;
+	}
+
 	/* Ignore output data of zero size */
 	if (!outsize)
 		goto out_free;
@@ -309,6 +328,8 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
 
 out_free:
 	kfree(obj);
+in_free:
+	kfree(args);
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH v2 1/3] Fix SW_TABLET_MODE detection method

[Hans de Goede]

Hi Jorge,

On 2/18/22 17:09, Jorge Lopez wrote:
> The purpose of this patch is to introduce a fix and removal
> of the current hack when determining tablet mode status.
> 
> Determining the tablet mode status requires reading Byte 0 bit 2 and 3
> as reported by HPWMI_HARDWARE_QUERY.  The investigation identified the
> failure was rooted in two areas; HPWMI_HARDWARE_QUERY failure (0x05)
> and reading Byte 0, bit 2 only to determine the table mode status.
> HPWMI_HARDWARE_QUERY WMI failure also rendered the dock state value invalid.
> 
> All changes were validated on a ZBook Workstation notebook.
> 
> Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>
> 
> ---
> Based on the latest platform-drivers-x86.git/for-next
> ---
>  drivers/platform/x86/hp-wmi.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 48a46466f086..544fce906ce7 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -35,9 +35,6 @@ MODULE_LICENSE("GPL");
>  MODULE_ALIAS("wmi:95F24279-4D7B-4334-9387-ACCDC67EF61C");
>  MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
>  
> -static int enable_tablet_mode_sw = -1;
> -module_param(enable_tablet_mode_sw, int, 0444);
> -MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=auto, 0=no, 1=yes)");
>  
>  #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
>  #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
> @@ -127,6 +124,7 @@ enum hp_wmi_command {
>  enum hp_wmi_hardware_mask {
>  	HPWMI_DOCK_MASK		= 0x01,
>  	HPWMI_TABLET_MASK	= 0x04,
> +	HPWMI_DETACHABLE_MASK	= 0x08,
>  };
>  
>  struct bios_return {
> @@ -347,12 +345,19 @@ static int hp_wmi_read_int(int query)
>  
>  static int hp_wmi_hw_state(int mask)
>  {
> -	int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
> +	int state = 0, ret;
>  
> -	if (state < 0)
> -		return state;
> +	ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &state,
> +				   0, sizeof(state));
>  
> -	return !!(state & mask);
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
> +	/* determine if Detachable mode is enabled */
> +	if (HPWMI_TABLET_MASK == mask)
> +		state = (state & HPWMI_DETACHABLE_MASK );

If you do: "state &= 0x08" as you do here
then after that "state & HPWMI_TABLET_MASK" aka
"state & 0x04" as done below will always be 0
since you have masked out the 0x04 bit when applying
the 0x08 mask.

I think you probably want something like this instead:

	if (HPWMI_TABLET_MASK == mask)
		if (!(state & HPWMI_DETACHABLE_MASK))
			return 0;

So when asked to check the HPWMI_TABLET_MASK then if
the device is not a detachable always return 0,
does that sound right ?

Note it is probable better to just makes this a generic
helper which no longer takes the mask and let the caller
handle all this. Or maybe just remove this function all
together since if you remove the masking I don't think
there will be much left of it.





> +
> +	return (state & mask);
>  }
>  
>  static int omen_thermal_profile_set(int mode)
> @@ -781,18 +786,16 @@ static int __init hp_wmi_input_setup(void)
>  
>  	/* Dock */
>  	val = hp_wmi_hw_state(HPWMI_DOCK_MASK);
> -	if (!(val < 0)) {
> +	if (val > 0) {
>  		__set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
>  		input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
>  	}

I'm not sure why you made this change here, the original code here
actually is correct. This:

		__set_bit(SW_DOCK, hp_wmi_input_dev->swbit);

tells userspace that the input-device has a SW_DOCK switch,
where as this:

  		input_report_switch(hp_wmi_input_dev, SW_DOCK, val);

Set the initial value of the dock-switch.

After your change we will not only do this bit:

		__set_bit(SW_DOCK, hp_wmi_input_dev->swbit);

When the machine is docked at boot, which means that if it
gets docked later we will not report it since SW_DOCK is
never set in hp_wmi_input_dev->swbit in that case
(swbit must be initialized before registering the input-device).


>  
>  	/* Tablet mode */
> -	if (enable_tablet_mode_sw > 0) {
> -		val = hp_wmi_hw_state(HPWMI_TABLET_MASK);
> -		if (val >= 0) {
> -			__set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
> +	val = hp_wmi_hw_state(HPWMI_TABLET_MASK);
> +	if (val > 0) {
> +		__set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
>  			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
> -		}
>  	}

Dropping the module option is fine, but otherwise this needs to be changed.

I think we want something like this here:

	err = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &val, 0, sizeof(val));

	if (!err) {
		__set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
		input_report_switch(hp_wmi_input_dev, SW_DOCK, (val & HPWMI_DOCK_MASK));
	}

	if (!err && (val & HPWMI_DETACHABLE_MASK)) {
		__set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
		input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, (val & HPWMI_TABLET_MASK));
	}

This also avoids unnecessarily doing the HPWMI_HARDWARE_QUERY twice.

And we probably want something similar in hp_wmi_notify() case HPWMI_DOCK_EVENT:
to also avoid unnecessarily doing the HPWMI_HARDWARE_QUERY twice there.

Regards,

Hans


Reg

Re: [PATCH v2 2/3] Fix 0x05 error code reported by several WMI calls

[Hans de Goede]

Hi,

On 2/18/22 17:09, Jorge Lopez wrote:
> Several WMI queries leverage hp_wmi_read_int function to read their data.
> hp_wmi_read_int function returns the appropiate value if the WMI command
> requires an input and output buffer size values greater than zero.
> WMI queries such HPWMI_HARDWARE_QUERY, HPWMI_WIRELESS2_QUERY,
> HPWMI_FEATURE2_QUERY, HPWMI_DISPLAY_QUERY, HPWMI_ALS_QUERY, and
> HPWMI_POSTCODEERROR_QUERY requires calling hp_wmi_perform_query function
> with input buffer size value of zero.  Any input buffer size greater
> than zero will cause error 0x05 to be returned.
> 
> All changes were validated on a ZBook Workstation notebook. Additional
> validation was included to ensure no other commands were failing or
> incorrectly handled.
> 
> Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

So after this the only remaining callers of hp_wmi_read_int() are:

1. hp_wmi_notify() case HPWMI_BEZEL_BUTTON
2. hp_wmi_rfkill_setup()
3. thermal_profile_get()

Where 2. looks like you just forgot to convert it since it
does a hp_wmi_read_int(HPWMI_WIRELESS_QUERY) which you do
convert in the hp_wmi_get_sw_state() and hp_wmi_get_hw_state()
helpers, suggesting that it should be converted in
hp_wmi_rfkill_setup() too.

This leaves just 1 and 3. So IMHO it would be better to
fix hp_wmi_read_int() and if 1. and 3. indeed need the old
behavior convert them to call hp_wmi_perform_query() directly.

Regards,

Hans



> 
> ---
> Based on the latest platform-drivers-x86.git/for-next
> ---
>  drivers/platform/x86/hp-wmi.c | 64 ++++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 544fce906ce7..de715687021a 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -442,7 +442,7 @@ static int __init hp_wmi_bios_2009_later(void)
>  {
>  	u8 state[128];
>  	int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, HPWMI_READ, &state,
> -				       sizeof(state), sizeof(state));
> +				       0, sizeof(state));
>  	if (!ret)
>  		return 1;
>  
> @@ -477,25 +477,37 @@ static const struct rfkill_ops hp_wmi_rfkill_ops = {
>  static bool hp_wmi_get_sw_state(enum hp_wmi_radio r)
>  {
>  	int mask = 0x200 << (r * 8);
> +	int ret= 0;
> +	int wireless = 0;
> +
> +	ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless,
> +				   0, sizeof(wireless));
>  
> -	int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
> +	if (ret < 0)
> +	  return -EINVAL;
>  
>  	/* TBD: Pass error */
>  	WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY");
>  
> -	return !(wireless & mask);
> +	return (wireless & mask);
>  }
>  
>  static bool hp_wmi_get_hw_state(enum hp_wmi_radio r)
>  {
>  	int mask = 0x800 << (r * 8);
> +	int ret= 0;
> +	int wireless = 0;
>  
> -	int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
> +	ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless,
> +				   0, sizeof(wireless));
> +
> +	if (ret < 0)
> +	  return -EINVAL;
>  
>  	/* TBD: Pass error */
>  	WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY");
>  
> -	return !(wireless & mask);
> +	return (wireless & mask);
>  }
>  
>  static int hp_wmi_rfkill2_set_block(void *data, bool blocked)
> @@ -520,7 +532,7 @@ static int hp_wmi_rfkill2_refresh(void)
>  	int err, i;
>  
>  	err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state,
> -				   sizeof(state), sizeof(state));
> +				   0, sizeof(state));
>  	if (err)
>  		return err;
>  
> @@ -546,27 +558,36 @@ static int hp_wmi_rfkill2_refresh(void)
>  static ssize_t display_show(struct device *dev, struct device_attribute *attr,
>  			    char *buf)
>  {
> -	int value = hp_wmi_read_int(HPWMI_DISPLAY_QUERY);
> -	if (value < 0)
> -		return value;
> +	int value = 0;
> +	int ret  = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_READ, &value,
> +				   0, sizeof(value));
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
>  	return sprintf(buf, "%d\n", value);
>  }
>  
>  static ssize_t hddtemp_show(struct device *dev, struct device_attribute *attr,
>  			    char *buf)
>  {
> -	int value = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY);
> -	if (value < 0)
> -		return value;
> +	int value = 0;
> +	int ret  = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_READ, &value,
> +				   0, sizeof(value));
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
>  	return sprintf(buf, "%d\n", value);
>  }
>  
>  static ssize_t als_show(struct device *dev, struct device_attribute *attr,
>  			char *buf)
>  {
> -	int value = hp_wmi_read_int(HPWMI_ALS_QUERY);
> -	if (value < 0)
> -		return value;
> +	int value = 0;
> +	int ret  = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_READ, &value,
> +				   0, sizeof(value));
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
>  	return sprintf(buf, "%d\n", value);
>  }
>  
> @@ -592,9 +613,12 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
>  	/* Get the POST error code of previous boot failure. */
> -	int value = hp_wmi_read_int(HPWMI_POSTCODEERROR_QUERY);
> -	if (value < 0)
> -		return value;
> +	int value = 0;
> +	int ret  = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_READ, &value,
> +				   0, sizeof(value));
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
>  	return sprintf(buf, "0x%x\n", value);
>  }
>  
> @@ -609,7 +633,7 @@ static ssize_t als_store(struct device *dev, struct device_attribute *attr,
>  		return ret;
>  
>  	ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_WRITE, &tmp,
> -				       sizeof(tmp), sizeof(tmp));
> +				   sizeof(tmp), 0);
>  	if (ret)
>  		return ret < 0 ? ret : -EINVAL;
>  
> @@ -922,7 +946,7 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
>  	int err, i;
>  
>  	err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state,
> -				   sizeof(state), sizeof(state));
> +				   0, sizeof(state));
>  	if (err)
>  		return err < 0 ? err : -EINVAL;
>  

Re: [PATCH v2 3/3] Changing bios_args.data to be dynamically allocated

[Hans de Goede]

Hi,

On 2/18/22 17:09, Jorge Lopez wrote:
> The purpose of this patch is to remove 128 bytes buffer limitation
> imposed in bios_args structure.
> 
> A limiting factor discovered during this investigation was the struct
> bios_args.data size restriction.  The data member size limits all possible
> WMI commands to those requiring buffer size of 128 bytes or less.
> Several WMI commands and queries require a buffer size larger than 128
> bytes hence limiting current and feature supported by the driver.
> It is for this reason, struct bios_args.data changed and is dynamically
> allocated.  hp_wmi_perform_query function changed to handle the memory
> allocation and release of any required buffer size.
> 
> All changes were validated on a ZBook Workstation notebook. Additional
> validation was included to ensure no other commands were failing or
> incorrectly handled.
> 
> Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>
> 
> ---
> Based on the latest platform-drivers-x86.git/for-next
> ---
>  drivers/platform/x86/hp-wmi.c | 67 +++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index de715687021a..11c8e9b6e64a 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -83,12 +83,17 @@ enum hp_wmi_event_ids {
>  	HPWMI_BATTERY_CHARGE_PERIOD	= 0x10,
>  };
>  
> +/**
> + * struct bios_args buffer is dynamically allocated.  New WMI command types
> + * were introduced that exceeds 128-byte data size.  Changes to handle
> + * the data size allocation scheme were kept in hp_wmi_perform_qurey function.
> + */
>  struct bios_args {
>  	u32 signature;
>  	u32 command;
>  	u32 commandtype;
>  	u32 datasize;
> -	u8 data[128];
> +	u8 data[0];
>  };
>  
>  enum hp_wmi_commandtype {
> @@ -258,39 +263,47 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>  	struct bios_return *bios_return;
>  	int actual_outsize;
>  	union acpi_object *obj;
> -	struct bios_args args = {
> -		.signature = 0x55434553,
> -		.command = command,
> -		.commandtype = query,
> -		.datasize = insize,
> -		.data = { 0 },
> -	};
> -	struct acpi_buffer input = { sizeof(struct bios_args), &args };
> +	size_t bios_args_size = sizeof(struct bios_args) + insize;
> +	struct bios_args *args = NULL;
> +	struct acpi_buffer input;
>  	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>  	int ret = 0;
>  
> -	mid = encode_outsize_for_pvsz(outsize);
> -	if (WARN_ON(mid < 0))
> -		return mid;
> +	args = kmalloc(bios_args_size, GFP_KERNEL);
> +	if (!args)
> +		return -ENOMEM;
>  
> -	if (WARN_ON(insize > sizeof(args.data)))
> -		return -EINVAL;
> -	memcpy(&args.data[0], buffer, insize);
> +	input.length = bios_args_size;
> +	input.pointer = args;
>  
> -	wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> +	mid = encode_outsize_for_pvsz(outsize);
> +	if (WARN_ON(mid < 0)) {
> +		ret = mid;
> +		goto in_free;
> +	}
>  
> -	obj = output.pointer;
> +	/* Avoid unnecessary copy to the data buffer if input buffer size is zero */
> +	if (insize > 0)

This if is not necessary, memcpy should touch neither dst nor src when
called with size==0.


> +		memcpy(args->data, buffer, insize);
>  
> -	if (!obj)
> -		return -EINVAL;
> +	args->signature = 0x55434553;
> +	args->command = command;
> +	args->commandtype = query;
> +	args->datasize = insize;
>  
> -	if (obj->type != ACPI_TYPE_BUFFER) {
> +	ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);

Add:
	if (ret)
		goto in_free;

Here ? AFAIK if ret is set &output is not touched, so other
wise you end up reading unitialized mem when doing:

	obj = output.pointer;
> +	if (!obj) {
> +		pr_warn("query 0x%x returned a null obj 0x%x\n", query, ret);

Are you sure this can never happen when e.g. querying features on
older models ? I believe it is probably best to just drop this as
it is an unrelated behavior change and if you really think this is
a good idea, put the adding of the pr_warn in a separate commit
with an explanation why this is being added.

>  		ret = -EINVAL;
> -		goto out_free;
> +		goto in_free;
>  	}
>  
> -	bios_return = (struct bios_return *)obj->buffer.pointer;
> -	ret = bios_return->return_code;
> +	if (!ret && obj->type == ACPI_TYPE_BUFFER) {
> +		bios_return = (struct bios_return *)obj->buffer.pointer;
> +		ret = bios_return->return_code;
> +	}

Please keep this in the form of:

	if (unexpected_condition) {
		ret = -EINVAL;
		goto out_free;
	}

	straight-path-statements-here;

This form is consistently used in most kernel code so that the
"straight-path" code is always indented only 1 level which makes
it easier to follow.

And this would also mean that you end up making less changes
in this patch.

>  
>  	if (ret) {
>  		if (ret != HPWMI_RET_UNKNOWN_COMMAND &&
> @@ -299,6 +312,12 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>  		goto out_free;
>  	}
>  
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret);
> +		ret = -EINVAL;
> +		goto out_free;
> +	}
> +
>  	/* Ignore output data of zero size */
>  	if (!outsize)
>  		goto out_free;
> @@ -309,6 +328,8 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>  
>  out_free:
>  	kfree(obj);
> +in_free:
> +	kfree(args);
>  	return ret;

IMHO it would be better to have a single "out" label here and
initialize obj to NULL when declared, kfree(NULL) is a no-op,
so this way you don't need 2 labels and you also avoid potentially
jumping to the wrong label.

Regards,

Hans