[PATCH v5 0/4] Fix SW_TABLET_MODE detection method

[PATCH v5 0/4] 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 v5 part 2 and 3 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. 


Description of changes between version 4 and version 5
------------------------------------------------------

v5 patch 1: Fix hp_wmi_read_int() reporting error (0x05)
	-Moved hp_wmi_read_int() to a separate patch.
	-Initially part of v4 patch 1
	
v5 patch 2: Fix SW_TABLET_MODE detection method
	-Remaining part are changes included in v4 patch 1
	-Replaced return values with -ENODEV errors
	-Updated how the closing return value is calculated.
	-Patch 2 is dependent of patch 1 in order for tablet 
	 mode detection to work properly
	

v5 patch 3: Fix 0x05 error code reported by several WMI calls
	-No new changes were introduced.
	-Patch is identical to v4 patch 2
	
v5 patch 4: Changing bios_args.data to be dynamically allocated
	-Replace sizeof() with struct_size() and flex_array_size() helpers
	-Added ret variable value when the output buffer is zero


Jorge Lopez (4):
  Fix hp_wmi_read_int() reporting error (0x05)
  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 | 159 ++++++++++++++++++++++------------
 1 file changed, 106 insertions(+), 53 deletions(-)

-- 
2.25.1

[PATCH v5 1/4] Fix hp_wmi_read_int() reporting error (0x05)

[Jorge Lopez]

The purpose of this patch is to introduce a fix to hp_wmi_read_int()
and eliminate failure error (0x05). Several WMI queries leverage
hp_wmi_read_int() to read their data and were failing with error 0x05.

HPWMI_DISPLAY_QUERY
HPWMI_HDDTEMP_QUERY
HPWMI_ALS_QUERY
HPWMI_HARDWARE_QUERY
HPWMI_WIRELESS_QUERY
HPWMI_POSTCODEERROR_QUERY

The failure occurs because hp_wmi_read_int() calls
hp_wmi_perform_query() with input parameter of size greater than zero.
Invoking those WMI commands with an input buffer size greater than
zero causes the command to be rejected and error 0x05 be returned.

All changes were validated on a HP ZBook Workstation notebook,
HP EliteBook x360, and HP EliteBook 850 G8.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 48a46466f086..103f56399ed0 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -337,7 +337,7 @@ static int hp_wmi_read_int(int query)
 	int val = 0, ret;
 
 	ret = hp_wmi_perform_query(query, HPWMI_READ, &val,
-				   sizeof(val), sizeof(val));
+				   0, sizeof(val));
 
 	if (ret)
 		return ret < 0 ? ret : -EINVAL;
-- 
2.25.1

Re: [PATCH v5 1/4] Fix hp_wmi_read_int() reporting error (0x05)

[Hans de Goede]

Hi,

On 3/10/22 22:08, Jorge Lopez wrote:
> The purpose of this patch is to introduce a fix to hp_wmi_read_int()
> and eliminate failure error (0x05). Several WMI queries leverage
> hp_wmi_read_int() to read their data and were failing with error 0x05.
> 
> HPWMI_DISPLAY_QUERY
> HPWMI_HDDTEMP_QUERY
> HPWMI_ALS_QUERY
> HPWMI_HARDWARE_QUERY
> HPWMI_WIRELESS_QUERY
> HPWMI_POSTCODEERROR_QUERY
> 
> The failure occurs because hp_wmi_read_int() calls
> hp_wmi_perform_query() with input parameter of size greater than zero.
> Invoking those WMI commands with an input buffer size greater than
> zero causes the command to be rejected and error 0x05 be returned.
> 
> All changes were validated on a HP ZBook Workstation notebook,
> HP EliteBook x360, and HP EliteBook 850 G8.
> 
> Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note since your using a different email address then Signed-off-by to
submit the patches, I've changed the From / Author of the patch to
match the Signed-off-by while applying this patch (and the same for
the other patches in the series).

The patch will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> 
> ---
> Based on the latest platform-drivers-x86.git/for-next
> ---
>  drivers/platform/x86/hp-wmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 48a46466f086..103f56399ed0 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -337,7 +337,7 @@ static int hp_wmi_read_int(int query)
>  	int val = 0, ret;
>  
>  	ret = hp_wmi_perform_query(query, HPWMI_READ, &val,
> -				   sizeof(val), sizeof(val));
> +				   0, sizeof(val));
>  
>  	if (ret)
>  		return ret < 0 ? ret : -EINVAL;

[PATCH v5 2/4] 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 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.

The latest changes use SMBIOS Type 3 (chassis type) and WMI Command
0x40 (device_mode_status) information to determine if the device is
in tablet mode or not.

hp_wmi_hw_state function was split into two functions;
hp_wmi_get_dock_state and hp_wmi_get_tablet_mode.  The new functions
separate how dock_state and tablet_mode is handled in a cleaner
manner.

All changes were validated on a HP ZBook Workstation notebook,
HP EliteBook x360, and HP EliteBook 850 G8.

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

---
Based on the latest platform-drivers-x86.git/for-next

This patch requires  patch "Fix hp_wmi_read_int() reporting
error (0x05)" in order to work correctly.
---
 drivers/platform/x86/hp-wmi.c | 71 +++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 103f56399ed0..e9aa05c26a40 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -35,10 +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"
 #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
@@ -107,6 +103,7 @@ enum hp_wmi_commandtype {
 	HPWMI_FEATURE2_QUERY		= 0x0d,
 	HPWMI_WIRELESS2_QUERY		= 0x1b,
 	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
+	HPWMI_SYSTEM_DEVICE_MODE	= 0x40,
 	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
 };
 
@@ -217,6 +214,18 @@ struct rfkill2_device {
 static int rfkill2_count;
 static struct rfkill2_device rfkill2[HPWMI_MAX_RFKILL2_DEVICES];
 
+/* Chassis Types values were obtained from SMBIOS reference
+ * specification version 3.00. A complete list of system enclosures
+ * and chassis types is available on Table 17. 
+ */
+static const char * const tablet_chassis_types[] = {
+	"30", /* Tablet*/
+	"31", /* Convertible */
+	"32"  /* Detachable */
+};
+
+#define DEVICE_MODE_TABLET	0x06
+
 /* map output size to the corresponding WMI method id */
 static inline int encode_outsize_for_pvsz(int outsize)
 {
@@ -345,14 +354,40 @@ static int hp_wmi_read_int(int query)
 	return val;
 }
 
-static int hp_wmi_hw_state(int mask)
+static int hp_wmi_get_dock_state(void)
 {
 	int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
 
 	if (state < 0)
 		return state;
 
-	return !!(state & mask);
+	return !!(state & HPWMI_DOCK_MASK);
+}
+
+static int hp_wmi_get_tablet_mode(void)
+{
+	char system_device_mode[4] = { 0 };
+	int ret;
+	bool tablet_found = false;
+
+	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
+
+	if (!chassis_type)
+		return -ENODEV;
+
+	tablet_found = match_string(tablet_chassis_types,
+			    ARRAY_SIZE(tablet_chassis_types),
+			    chassis_type) >= 0;
+	if (!tablet_found)
+		return -ENODEV;
+
+	ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
+				       system_device_mode, 0, sizeof(system_device_mode));
+
+	if (ret < 0)
+		return ret;
+
+	return system_device_mode[0] == DEVICE_MODE_TABLET;
 }
 
 static int omen_thermal_profile_set(int mode)
@@ -568,7 +603,7 @@ static ssize_t als_show(struct device *dev, struct device_attribute *attr,
 static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
-	int value = hp_wmi_hw_state(HPWMI_DOCK_MASK);
+	int value = hp_wmi_get_dock_state();
 	if (value < 0)
 		return value;
 	return sprintf(buf, "%d\n", value);
@@ -577,7 +612,7 @@ static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
 static ssize_t tablet_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
-	int value = hp_wmi_hw_state(HPWMI_TABLET_MASK);
+	int value = hp_wmi_get_tablet_mode();
 	if (value < 0)
 		return value;
 	return sprintf(buf, "%d\n", value);
@@ -699,10 +734,10 @@ static void hp_wmi_notify(u32 value, void *context)
 	case HPWMI_DOCK_EVENT:
 		if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
 			input_report_switch(hp_wmi_input_dev, SW_DOCK,
-					    hp_wmi_hw_state(HPWMI_DOCK_MASK));
+					    hp_wmi_get_dock_state());
 		if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
 			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
-					    hp_wmi_hw_state(HPWMI_TABLET_MASK));
+					    hp_wmi_get_tablet_mode());
 		input_sync(hp_wmi_input_dev);
 		break;
 	case HPWMI_PARK_HDD:
@@ -780,19 +815,17 @@ static int __init hp_wmi_input_setup(void)
 	__set_bit(EV_SW, hp_wmi_input_dev->evbit);
 
 	/* Dock */
-	val = hp_wmi_hw_state(HPWMI_DOCK_MASK);
+	val = hp_wmi_get_dock_state();
 	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);
-			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
-		}
+	val = hp_wmi_get_tablet_mode();
+	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);
@@ -1227,10 +1260,10 @@ static int hp_wmi_resume_handler(struct device *device)
 	if (hp_wmi_input_dev) {
 		if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
 			input_report_switch(hp_wmi_input_dev, SW_DOCK,
-					    hp_wmi_hw_state(HPWMI_DOCK_MASK));
+					    hp_wmi_get_dock_state());
 		if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
 			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
-					    hp_wmi_hw_state(HPWMI_TABLET_MASK));
+					    hp_wmi_get_tablet_mode());
 		input_sync(hp_wmi_input_dev);
 	}
 
-- 
2.25.1

Re: [PATCH v5 2/4] Fix SW_TABLET_MODE detection method

[Hans de Goede]

Hi,

On 3/10/22 22:08, 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 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.
> 
> The latest changes use SMBIOS Type 3 (chassis type) and WMI Command
> 0x40 (device_mode_status) information to determine if the device is
> in tablet mode or not.
> 
> hp_wmi_hw_state function was split into two functions;
> hp_wmi_get_dock_state and hp_wmi_get_tablet_mode.  The new functions
> separate how dock_state and tablet_mode is handled in a cleaner
> manner.
> 
> All changes were validated on a HP ZBook Workstation notebook,
> HP EliteBook x360, and HP EliteBook 850 G8.
> 
> Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>
> 
> ---
> Based on the latest platform-drivers-x86.git/for-next
> 
> This patch requires  patch "Fix hp_wmi_read_int() reporting
> error (0x05)" in order to work correctly.
> ---
>  drivers/platform/x86/hp-wmi.c | 71 +++++++++++++++++++++++++----------
>  1 file changed, 52 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 103f56399ed0..e9aa05c26a40 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -35,10 +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"
>  #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
> @@ -107,6 +103,7 @@ enum hp_wmi_commandtype {
>  	HPWMI_FEATURE2_QUERY		= 0x0d,
>  	HPWMI_WIRELESS2_QUERY		= 0x1b,
>  	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
> +	HPWMI_SYSTEM_DEVICE_MODE	= 0x40,
>  	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
>  };
>  
> @@ -217,6 +214,18 @@ struct rfkill2_device {
>  static int rfkill2_count;
>  static struct rfkill2_device rfkill2[HPWMI_MAX_RFKILL2_DEVICES];
>  
> +/* Chassis Types values were obtained from SMBIOS reference
> + * specification version 3.00. A complete list of system enclosures
> + * and chassis types is available on Table 17. 
> + */
> +static const char * const tablet_chassis_types[] = {
> +	"30", /* Tablet*/
> +	"31", /* Convertible */
> +	"32"  /* Detachable */
> +};
> +
> +#define DEVICE_MODE_TABLET	0x06
> +
>  /* map output size to the corresponding WMI method id */
>  static inline int encode_outsize_for_pvsz(int outsize)
>  {
> @@ -345,14 +354,40 @@ static int hp_wmi_read_int(int query)
>  	return val;
>  }
>  
> -static int hp_wmi_hw_state(int mask)
> +static int hp_wmi_get_dock_state(void)
>  {
>  	int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
>  
>  	if (state < 0)
>  		return state;
>  
> -	return !!(state & mask);
> +	return !!(state & HPWMI_DOCK_MASK);
> +}
> +
> +static int hp_wmi_get_tablet_mode(void)
> +{
> +	char system_device_mode[4] = { 0 };
> +	int ret;
> +	bool tablet_found = false;
> +
> +	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);

This new-line in the mids of the variable declaration block looks 
weird. Also we usually put variable declarations at the top of
a function in reverse-christmas tree order (longest variable
declarations first).

I've fixed this up to look like this:

	char system_device_mode[4] = { 0 };
	const char *chassis_type;
	bool tablet_found;
	int ret;

	chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
	if (!chassis_type)
		return -ENODEV;

(no functional changes).

> +	tablet_found = match_string(tablet_chassis_types,
> +			    ARRAY_SIZE(tablet_chassis_types),
> +			    chassis_type) >= 0;
> +	if (!tablet_found)
> +		return -ENODEV;
> +
> +	ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
> +				       system_device_mode, 0, sizeof(system_device_mode));
> +

The newline here between the call + check looks weird, I've dropped this while
merging the patch:

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> +	if (ret < 0)
> +		return ret;
> +
> +	return system_device_mode[0] == DEVICE_MODE_TABLET;
>  }
>  
>  static int omen_thermal_profile_set(int mode)
> @@ -568,7 +603,7 @@ static ssize_t als_show(struct device *dev, struct device_attribute *attr,
>  static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> -	int value = hp_wmi_hw_state(HPWMI_DOCK_MASK);
> +	int value = hp_wmi_get_dock_state();
>  	if (value < 0)
>  		return value;
>  	return sprintf(buf, "%d\n", value);
> @@ -577,7 +612,7 @@ static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
>  static ssize_t tablet_show(struct device *dev, struct device_attribute *attr,
>  			   char *buf)
>  {
> -	int value = hp_wmi_hw_state(HPWMI_TABLET_MASK);
> +	int value = hp_wmi_get_tablet_mode();
>  	if (value < 0)
>  		return value;
>  	return sprintf(buf, "%d\n", value);
> @@ -699,10 +734,10 @@ static void hp_wmi_notify(u32 value, void *context)
>  	case HPWMI_DOCK_EVENT:
>  		if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
>  			input_report_switch(hp_wmi_input_dev, SW_DOCK,
> -					    hp_wmi_hw_state(HPWMI_DOCK_MASK));
> +					    hp_wmi_get_dock_state());
>  		if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
>  			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
> -					    hp_wmi_hw_state(HPWMI_TABLET_MASK));
> +					    hp_wmi_get_tablet_mode());
>  		input_sync(hp_wmi_input_dev);
>  		break;
>  	case HPWMI_PARK_HDD:
> @@ -780,19 +815,17 @@ static int __init hp_wmi_input_setup(void)
>  	__set_bit(EV_SW, hp_wmi_input_dev->evbit);
>  
>  	/* Dock */
> -	val = hp_wmi_hw_state(HPWMI_DOCK_MASK);
> +	val = hp_wmi_get_dock_state();
>  	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);
> -			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
> -		}
> +	val = hp_wmi_get_tablet_mode();
> +	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);
> @@ -1227,10 +1260,10 @@ static int hp_wmi_resume_handler(struct device *device)
>  	if (hp_wmi_input_dev) {
>  		if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
>  			input_report_switch(hp_wmi_input_dev, SW_DOCK,
> -					    hp_wmi_hw_state(HPWMI_DOCK_MASK));
> +					    hp_wmi_get_dock_state());
>  		if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
>  			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
> -					    hp_wmi_hw_state(HPWMI_TABLET_MASK));
> +					    hp_wmi_get_tablet_mode());
>  		input_sync(hp_wmi_input_dev);
>  	}
>  

Re: [PATCH v5 2/4] Fix SW_TABLET_MODE detection method

[Stefan Sichler]

Hi,
this patch (which is now committed to the kernel as commit
520ee4ea1cc60251a6e3c911cf0336278aa52634 since v5.18-rc1) unfortunately
introduced a regression on my HP EliteBook 2760p Convertible:

Tablet mode is no longer detected.

It worked flawlessly before (when enable_tablet_mode_sw module param was
set to 1).

Debugging showed that on this device, two problems prevent the table
mode detection from working:

   - Chassis Type is reported as 0x10 (= Lunch Box)

   - the query of HPWMI_SYSTEM_DEVICE_MODE does not report tablet state
at all

Note that the chassis type of this device (switch to tablet mode by
screen *rotation*) actually differs from the newer HP models (switch to
tablet mode by screen *flipping*).


I suggest fixing this by re-adding the removed module parameter
"enable_tablet_mode_sw", but change its behavior to work in the
following way:

   - when left at default -1 (auto): no change to current (new)
implementation

   - when set to 0: unconditionally disable table mode reporting at all

   - when set to 1: ignore Chassis type and use old-skool
hp_wmi_hw_state(HPWMI_TABLET_MASK) query method to determine tablet mode
in addition to new hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE...) method


I prepared a patch based on commit
520ee4ea1cc60251a6e3c911cf0336278aa52634, and tested it successfully on
my device.
See below.

Regards,
Stefan

--- hp-wmi.c.orig	2024-03-10 21:38:09.000000000 +0100
+++ hp-wmi.c	2024-08-08 09:23:29.509113900 +0200
@@ -35,6 +35,10 @@
  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"
  #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
@@ -428,6 +432,9 @@
  	bool tablet_found;
  	int ret;

+	if (!enable_tablet_mode_sw)
+		return -ENODEV;
+
  	chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
  	if (!chassis_type)
  		return -ENODEV;
@@ -435,16 +442,24 @@
  	tablet_found = match_string(tablet_chassis_types,
  				    ARRAY_SIZE(tablet_chassis_types),
  				    chassis_type) >= 0;
-	if (!tablet_found)
+	if (!tablet_found && enable_tablet_mode_sw < 0 /*auto*/)
  		return -ENODEV;

  	ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
  				   system_device_mode, zero_if_sup(system_device_mode),
  				   sizeof(system_device_mode));
-	if (ret < 0)
-		return ret;
+	if (ret >= 0)
+		ret = (system_device_mode[0] == DEVICE_MODE_TABLET);
+
+	/* workaround for older convertibles */
+	if (ret <= 0 && enable_tablet_mode_sw > 0)
+	{
+		ret = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
+		if (!(ret < 0))
+			ret = !!(ret & HPWMI_TABLET_MASK);
+	}

-	return system_device_mode[0] == DEVICE_MODE_TABLET;
+	return ret;
  }

  static int omen_thermal_profile_set(int mode)

Re: [PATCH v5 2/4] Fix SW_TABLET_MODE detection method

[Ilpo Järvinen]

On Thu, 8 Aug 2024, Stefan Sichler wrote:

> this patch (which is now committed to the kernel as commit
> 520ee4ea1cc60251a6e3c911cf0336278aa52634 since v5.18-rc1) unfortunately
> introduced a regression on my HP EliteBook 2760p Convertible:
> 
> Tablet mode is no longer detected.
> 
> It worked flawlessly before (when enable_tablet_mode_sw module param was
> set to 1).
> 
> Debugging showed that on this device, two problems prevent the table
> mode detection from working:
> 
>   - Chassis Type is reported as 0x10 (= Lunch Box)
> 
>   - the query of HPWMI_SYSTEM_DEVICE_MODE does not report tablet state
> at all
> 
> Note that the chassis type of this device (switch to tablet mode by
> screen *rotation*) actually differs from the newer HP models (switch to
> tablet mode by screen *flipping*).
> 
> 
> I suggest fixing this by re-adding the removed module parameter
> "enable_tablet_mode_sw", but change its behavior to work in the
> following way:
> 
>   - when left at default -1 (auto): no change to current (new)
> implementation
> 
>   - when set to 0: unconditionally disable table mode reporting at all
> 
>   - when set to 1: ignore Chassis type and use old-skool
> hp_wmi_hw_state(HPWMI_TABLET_MASK) query method to determine tablet mode
> in addition to new hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE...) method
> 
> 
> I prepared a patch based on commit
> 520ee4ea1cc60251a6e3c911cf0336278aa52634, and tested it successfully on
> my device.
> See below.
> 
> Regards,
> Stefan
> 
> --- hp-wmi.c.orig	2024-03-10 21:38:09.000000000 +0100
> +++ hp-wmi.c	2024-08-08 09:23:29.509113900 +0200

This submission does not follow the normal patch formatting guidelines,
please see Documentation/process/submitting-patches.rst.

> @@ -35,6 +35,10 @@
>  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"
>  #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
> @@ -428,6 +432,9 @@
>  	bool tablet_found;
>  	int ret;
> 
> +	if (!enable_tablet_mode_sw)
> +		return -ENODEV;
> +
>  	chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
>  	if (!chassis_type)
>  		return -ENODEV;
> @@ -435,16 +442,24 @@
>  	tablet_found = match_string(tablet_chassis_types,
>  				    ARRAY_SIZE(tablet_chassis_types),
>  				    chassis_type) >= 0;
> -	if (!tablet_found)
> +	if (!tablet_found && enable_tablet_mode_sw < 0 /*auto*/)

Having to add a comment like that is a very strong indication you'd want 
to have a named define instead, e.g., HPWMI_TABLET_MODE_AUTO.

>  		return -ENODEV;
> 
>  	ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
>  				   system_device_mode,
> zero_if_sup(system_device_mode),
>  				   sizeof(system_device_mode));
> -	if (ret < 0)
> -		return ret;
> +	if (ret >= 0)
> +		ret = (system_device_mode[0] == DEVICE_MODE_TABLET);
> +
> +	/* workaround for older convertibles */
> +	if (ret <= 0 && enable_tablet_mode_sw > 0)
> +	{
> +		ret = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
> +		if (!(ret < 0))
> +			ret = !!(ret & HPWMI_TABLET_MASK);
> +	}
> 
> -	return system_device_mode[0] == DEVICE_MODE_TABLET;
> +	return ret;

The logic is quite hard to follow. It would be better to return 
early.

	if (ret < 0 && enable_tablet_mode_sw == HPWMI_TABLET_MODE_AUTO)
		return ret;

	if (ret >= 0 && system_device_mode[0] == DEVICE_MODE_TABLET)
		return 1;

	ret = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
	if (ret < 0)
		return ret;

	return !!(ret & HPWMI_TABLET_MASK);


However, automatically detecting this condition over adding the module 
parameter would be the preferred solution.

-- 
 i.

>  }
> 
>  static int omen_thermal_profile_set(int mode)
> 
> 

Re: [PATCH v5 2/4] Fix SW_TABLET_MODE detection method

[Stefan Sichler]

Hi Ilpo,

thank you for your comments.
I added some defines for the values of the module param and re-ordered the
logic a bit. I think it is easier to follow now.

I now also use another return variable "ret2" for the result of
hp_wmi_read_int() to not overwrite the result of original
hp_wmi_perform_query() when hp_wmi_read_int() fails. This was actually a
bug in my first implementation.

I still think that re-using the enable_tablet_mode_sw param for that is
a reasonable compromise, because the comment of original
commit 520ee4ea1cc60251a6e3c911cf0336278aa52634 suggests that there was
some problem with hp_wmi_read_int(HPWMI_HARDWARE_QUERY) on newer Tablets.
So, because I cannot test this, I'd prefer to not change current behavior
when the enable_tablet_mode_sw parameter is not set.

Best Regards
Stefan

>>>>>>>>>>>>>>> Actual patch starts here:

From: Stefan Sichler <stsichler@web.de>
Date: Mon, 19 Aug 2024 14:45:57 +0200
Subject: [PATCH] platform/x86: hp-wmi: repair Tablet Mode detection on old
  Convertibles

This fixes a regression introduced by commit
520ee4ea1cc60251a6e3c911cf0336278aa52634
("Fix SW_TABLET_MODE detection method").

Investigation showed that some older Convertibles like HP EliteBook 2760p
do neither report Chassis Type correctly nor report tablet state by
HPWMI_SYSTEM_DEVICE_MODE, so for those, it is still required to fallback
to previous HPWMI_HARDWARE_QUERY method.

Since on pre-5.18 kernels, it was required to set enable_tablet_mode_sw
module param to enable this anyway, we re-add this removed parameter here
and re-use it in the following way:
   - when left at default -1 (auto): no change to current implementation
   - when set to 0 (off): unconditionally disable tablet mode reporting
   - when set to 1 (force on): ignore Chassis type and use old
     hp_wmi_hw_state(HPWMI_TABLET_MASK) query method in addition to new
     hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE...) method

Signed-off-by: Stefan Sichler <stsichler@web.de>

---
  drivers/platform/x86/hp/hp-wmi.c | 32 ++++++++++++++++++++++++++++----
  1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 876e0a97c..cab01308d 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -38,6 +38,13 @@ 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_TABLET_MODE_SW_AUTO -1
+#define HPWMI_TABLET_MODE_SW_OFF 0
+#define HPWMI_TABLET_MODE_SW_FORCE_ON 1
+
  #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
  #define HPWMI_BIOS_GUID "5FB7F034-2C63-45E9-BE91-3D44E2C707E4"

@@ -459,23 +466,40 @@ static int hp_wmi_get_tablet_mode(void)
  	bool tablet_found;
  	int ret;

+	if (enable_tablet_mode_sw == HPWMI_TABLET_MODE_SW_OFF)
+		return -ENODEV;
+
  	chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
  	if (!chassis_type)
  		return -ENODEV;

-	tablet_found = match_string(tablet_chassis_types,
+	tablet_found =
+		(enable_tablet_mode_sw == HPWMI_TABLET_MODE_SW_FORCE_ON)
+		|| (match_string(tablet_chassis_types,
  				    ARRAY_SIZE(tablet_chassis_types),
-				    chassis_type) >= 0;
+				    chassis_type) >= 0);
  	if (!tablet_found)
  		return -ENODEV;

  	ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
  				   system_device_mode, zero_if_sup(system_device_mode),
  				   sizeof(system_device_mode));
-	if (ret < 0)
+	if (ret < 0 && enable_tablet_mode_sw != HPWMI_TABLET_MODE_SW_FORCE_ON)
  		return ret;

-	return system_device_mode[0] == DEVICE_MODE_TABLET;
+	if (ret >= 0)
+		ret = (system_device_mode[0] == DEVICE_MODE_TABLET);
+
+	/* workaround for older convertibles. needs to be actively switched on
+	 * and is only executed when HPWMI_SYSTEM_DEVICE_MODE query failed or
+	 * did not report tablet state, i.e. (ret < 0) or (ret == 0) */
+	if (ret <= 0 && enable_tablet_mode_sw == HPWMI_TABLET_MODE_SW_FORCE_ON) {
+		int ret2 = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
+		if (ret2 >= 0)
+			ret = !!(ret2 & HPWMI_TABLET_MASK);
+	}
+
+	return ret;
  }

  static int omen_thermal_profile_set(int mode)
--
2.43.0

[PATCH v5 3/4] 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 was corrected in a previous patch.
Now, this function invokes hp_wmi_perform_query with input parameter
of size zero and the output buffer of size 4.

WMI commands calling hp_wmi_perform_query with input buffer size value
of zero are listed below.

HPWMI_DISPLAY_QUERY
HPWMI_HDDTEMP_QUERY
HPWMI_ALS_QUERY
HPWMI_HARDWARE_QUERY
HPWMI_WIRELESS_QUERY
HPWMI_BIOS_QUERY
HPWMI_FEATURE_QUERY
HPWMI_HOTKEY_QUERY
HPWMI_FEATURE2_QUERY
HPWMI_WIRELESS2_QUERY
HPWMI_POSTCODEERROR_QUERY
HPWMI_THERMAL_PROFILE_QUERY
HPWMI_FAN_SPEED_MAX_GET_QUERY

Invoking those WMI commands with an input buffer size greater
than zero will cause error 0x05 to be returned.

All WMI commands executed by the driver were reviewed and changes
were made to ensure the expected input and output buffer size match
the WMI specification.

Changes were validated on a HP ZBook Workstation notebook,
HP EliteBook x360, and HP EliteBook 850 G8.  Additional
validation was included in the test process to ensure no other
commands were 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 | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index e9aa05c26a40..e76bd4bef6b5 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -329,7 +329,7 @@ static int hp_wmi_get_fan_speed(int fan)
 	char fan_data[4] = { fan, 0, 0, 0 };
 
 	int ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_GET_QUERY, HPWMI_GM,
-				       &fan_data, sizeof(fan_data),
+				       &fan_data, sizeof(char),
 				       sizeof(fan_data));
 
 	if (ret != 0)
@@ -399,7 +399,7 @@ static int omen_thermal_profile_set(int mode)
 		return -EINVAL;
 
 	ret = hp_wmi_perform_query(HPWMI_SET_PERFORMANCE_MODE, HPWMI_GM,
-				   &buffer, sizeof(buffer), sizeof(buffer));
+				   &buffer, sizeof(buffer), 0);
 
 	if (ret)
 		return ret < 0 ? ret : -EINVAL;
@@ -436,7 +436,7 @@ static int hp_wmi_fan_speed_max_set(int enabled)
 	int ret;
 
 	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_SET_QUERY, HPWMI_GM,
-				   &enabled, sizeof(enabled), sizeof(enabled));
+				   &enabled, sizeof(enabled), 0);
 
 	if (ret)
 		return ret < 0 ? ret : -EINVAL;
@@ -449,7 +449,7 @@ static int hp_wmi_fan_speed_max_get(void)
 	int val = 0, ret;
 
 	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
-				   &val, sizeof(val), sizeof(val));
+				   &val, 0, sizeof(val));
 
 	if (ret)
 		return ret < 0 ? ret : -EINVAL;
@@ -461,7 +461,7 @@ static int __init hp_wmi_bios_2008_later(void)
 {
 	int state = 0;
 	int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, HPWMI_READ, &state,
-				       sizeof(state), sizeof(state));
+				       0, sizeof(state));
 	if (!ret)
 		return 1;
 
@@ -472,7 +472,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;
 
@@ -550,7 +550,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;
 
@@ -639,7 +639,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;
 
@@ -660,9 +660,9 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
 	if (clear == false)
 		return -EINVAL;
 
-	/* Clear the POST error code. It is kept until until cleared. */
+	/* Clear the POST error code. It is kept until cleared. */
 	ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_WRITE, &tmp,
-				       sizeof(tmp), sizeof(tmp));
+				       sizeof(tmp), 0);
 	if (ret)
 		return ret < 0 ? ret : -EINVAL;
 
@@ -952,7 +952,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

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

[Hans de Goede]

Hi,

On 3/10/22 22:08, Jorge Lopez wrote:
> Several WMI queries leverage hp_wmi_read_int function to read their
> data. hp_wmi_read_int function was corrected in a previous patch.
> Now, this function invokes hp_wmi_perform_query with input parameter
> of size zero and the output buffer of size 4.
> 
> WMI commands calling hp_wmi_perform_query with input buffer size value
> of zero are listed below.
> 
> HPWMI_DISPLAY_QUERY
> HPWMI_HDDTEMP_QUERY
> HPWMI_ALS_QUERY
> HPWMI_HARDWARE_QUERY
> HPWMI_WIRELESS_QUERY
> HPWMI_BIOS_QUERY
> HPWMI_FEATURE_QUERY
> HPWMI_HOTKEY_QUERY
> HPWMI_FEATURE2_QUERY
> HPWMI_WIRELESS2_QUERY
> HPWMI_POSTCODEERROR_QUERY
> HPWMI_THERMAL_PROFILE_QUERY
> HPWMI_FAN_SPEED_MAX_GET_QUERY
> 
> Invoking those WMI commands with an input buffer size greater
> than zero will cause error 0x05 to be returned.
> 
> All WMI commands executed by the driver were reviewed and changes
> were made to ensure the expected input and output buffer size match
> the WMI specification.
> 
> Changes were validated on a HP ZBook Workstation notebook,
> HP EliteBook x360, and HP EliteBook 850 G8.  Additional
> validation was included in the test process to ensure no other
> commands were incorrectly handled.
> 
> Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> 
> ---
> Based on the latest platform-drivers-x86.git/for-next
> ---
>  drivers/platform/x86/hp-wmi.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index e9aa05c26a40..e76bd4bef6b5 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -329,7 +329,7 @@ static int hp_wmi_get_fan_speed(int fan)
>  	char fan_data[4] = { fan, 0, 0, 0 };
>  
>  	int ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_GET_QUERY, HPWMI_GM,
> -				       &fan_data, sizeof(fan_data),
> +				       &fan_data, sizeof(char),
>  				       sizeof(fan_data));
>  
>  	if (ret != 0)
> @@ -399,7 +399,7 @@ static int omen_thermal_profile_set(int mode)
>  		return -EINVAL;
>  
>  	ret = hp_wmi_perform_query(HPWMI_SET_PERFORMANCE_MODE, HPWMI_GM,
> -				   &buffer, sizeof(buffer), sizeof(buffer));
> +				   &buffer, sizeof(buffer), 0);
>  
>  	if (ret)
>  		return ret < 0 ? ret : -EINVAL;
> @@ -436,7 +436,7 @@ static int hp_wmi_fan_speed_max_set(int enabled)
>  	int ret;
>  
>  	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_SET_QUERY, HPWMI_GM,
> -				   &enabled, sizeof(enabled), sizeof(enabled));
> +				   &enabled, sizeof(enabled), 0);
>  
>  	if (ret)
>  		return ret < 0 ? ret : -EINVAL;
> @@ -449,7 +449,7 @@ static int hp_wmi_fan_speed_max_get(void)
>  	int val = 0, ret;
>  
>  	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
> -				   &val, sizeof(val), sizeof(val));
> +				   &val, 0, sizeof(val));
>  
>  	if (ret)
>  		return ret < 0 ? ret : -EINVAL;
> @@ -461,7 +461,7 @@ static int __init hp_wmi_bios_2008_later(void)
>  {
>  	int state = 0;
>  	int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, HPWMI_READ, &state,
> -				       sizeof(state), sizeof(state));
> +				       0, sizeof(state));
>  	if (!ret)
>  		return 1;
>  
> @@ -472,7 +472,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;
>  
> @@ -550,7 +550,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;
>  
> @@ -639,7 +639,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;
>  
> @@ -660,9 +660,9 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>  	if (clear == false)
>  		return -EINVAL;
>  
> -	/* Clear the POST error code. It is kept until until cleared. */
> +	/* Clear the POST error code. It is kept until cleared. */
>  	ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_WRITE, &tmp,
> -				       sizeof(tmp), sizeof(tmp));
> +				       sizeof(tmp), 0);
>  	if (ret)
>  		return ret < 0 ? ret : -EINVAL;
>  
> @@ -952,7 +952,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;
>  

[PATCH v5 4/4] 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 HP ZBook Workstation notebook,
HP EliteBook x360, and HP EliteBook 850 G8.  Additional
validation was included in the test process to ensure no other
commands were 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, 42 insertions(+), 22 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index e76bd4bef6b5..cc5c4f637328 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -82,12 +82,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[];
 };
 
 enum hp_wmi_commandtype {
@@ -268,34 +273,40 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
 	int mid;
 	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 };
+	union acpi_object *obj = NULL;
+	struct bios_args *args = NULL;
+	size_t bios_args_size = struct_size(args, data, insize);
+	
+	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 out_free;
+	}
 
-	obj = output.pointer;
+	memcpy(args->data, buffer, flex_array_size(args, data, 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);
+	if (ret)
+		goto out_free;
+
+	obj = output.pointer;
+	if (!obj) {
 		ret = -EINVAL;
 		goto out_free;
 	}
@@ -310,9 +321,17 @@ 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)
+	if (!outsize) {
+		ret = 0;
 		goto out_free;
+	}
 
 	actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return)));
 	memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize);
@@ -320,6 +339,7 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
 
 out_free:
 	kfree(obj);
+	kfree(args);
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH v5 4/4] Changing bios_args.data to be dynamically allocated

[Hans de Goede]

Hi,

On 3/10/22 22:08, 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 HP ZBook Workstation notebook,
> HP EliteBook x360, and HP EliteBook 850 G8.  Additional
> validation was included in the test process to ensure no other
> commands were 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, 42 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index e76bd4bef6b5..cc5c4f637328 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -82,12 +82,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[];
>  };
>  
>  enum hp_wmi_commandtype {
> @@ -268,34 +273,40 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>  	int mid;
>  	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 };
> +	union acpi_object *obj = NULL;
> +	struct bios_args *args = NULL;
> +	size_t bios_args_size = struct_size(args, data, insize);
> +	
> +	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;

The variable declaration here again looks a bit messy, also
there is no reason to move the block setting + checking mid,
that just makes the diff unnecessarily large.

I've cleaned this up while mergin (no functional changes).


>  
> -	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 out_free;
> +	}
>  
> -	obj = output.pointer;
> +	memcpy(args->data, buffer, flex_array_size(args, data, 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);
> +	if (ret)
> +		goto out_free;
> +
> +	obj = output.pointer;
> +	if (!obj) {
>  		ret = -EINVAL;
>  		goto out_free;
>  	}
> @@ -310,9 +321,17 @@ 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;
> +	}
> +

You have now moved the obj->type != ACPI_TYPE_BUFFER check to after the:
	
	bios_return = (struct bios_return *)obj->buffer.pointer;

Statement, which dereferences the buffer member of the obj union.  That check
MUST be done before looking at the buffer member, so I have moved it back to
its old place, this also makes the diff/patch smaller.  Note this one is
a functional change to your patch.

The changed initial block of the function now looks like this:

static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
				void *buffer, int insize, int outsize)
{
	struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL };
	size_t bios_args_size = struct_size(args, data, insize);
	struct bios_return *bios_return;
	union acpi_object *obj = NULL;
	struct bios_args *args = NULL;
	int mid, actual_outsize, ret;

	mid = encode_outsize_for_pvsz(outsize);
	if (WARN_ON(mid < 0))
		return mid;

	args = kmalloc(bios_args_size, GFP_KERNEL);
	if (!args)
		return -ENOMEM;

	input.length = bios_args_size;
	input.pointer = args;

	args->signature = 0x55434553;
	args->command = command;
	args->commandtype = query;
	args->datasize = insize;
	memcpy(args->data, buffer, flex_array_size(args, data, insize));

	ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
	if (ret)
		goto out_free;

	obj = output.pointer;
	if (!obj) {
		ret = -EINVAL;
		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;
	}

	bios_return = (struct bios_return *)obj->buffer.pointer;


And the new diff for this chunk of the patch now is:

@@ -266,37 +271,42 @@ static inline int encode_outsize_for_pvsz(int outsize)
 static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
 				void *buffer, int insize, int outsize)
 {
-	int mid;
+	struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL };
+	size_t bios_args_size = struct_size(args, data, insize);
 	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 };
-	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
-	int ret = 0;
+	union acpi_object *obj = NULL;
+	struct bios_args *args = NULL;
+	int mid, actual_outsize, ret;
 
 	mid = encode_outsize_for_pvsz(outsize);
 	if (WARN_ON(mid < 0))
 		return mid;
 
-	if (WARN_ON(insize > sizeof(args.data)))
-		return -EINVAL;
-	memcpy(&args.data[0], buffer, insize);
+	args = kmalloc(bios_args_size, GFP_KERNEL);
+	if (!args)
+		return -ENOMEM;
 
-	wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
+	input.length = bios_args_size;
+	input.pointer = args;
 
-	obj = output.pointer;
+	args->signature = 0x55434553;
+	args->command = command;
+	args->commandtype = query;
+	args->datasize = insize;
+	memcpy(args->data, buffer, flex_array_size(args, data, insize));
 
-	if (!obj)
-		return -EINVAL;
+	ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
+	if (ret)
+		goto out_free;
+
+	obj = output.pointer;
+	if (!obj) {
+		ret = -EINVAL;
+		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;
 	}
@@ ...

I've merged this patch with the above changes:

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans








>  	/* Ignore output data of zero size */
> -	if (!outsize)
> +	if (!outsize) {
> +		ret = 0;
>  		goto out_free;
> +	}
>  
>  	actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return)));
>  	memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize);
> @@ -320,6 +339,7 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>  
>  out_free:
>  	kfree(obj);
> +	kfree(args);
>  	return ret;
>  }
>  

Re: [PATCH v5 4/4] Changing bios_args.data to be dynamically allocated

[Hans de Goede]

Hi,

On 3/14/22 11:51, Hans de Goede wrote:
> Hi,
> 
> On 3/10/22 22:08, 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 HP ZBook Workstation notebook,
>> HP EliteBook x360, and HP EliteBook 850 G8.  Additional
>> validation was included in the test process to ensure no other
>> commands were 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, 42 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
>> index e76bd4bef6b5..cc5c4f637328 100644
>> --- a/drivers/platform/x86/hp-wmi.c
>> +++ b/drivers/platform/x86/hp-wmi.c
>> @@ -82,12 +82,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[];
>>  };
>>  
>>  enum hp_wmi_commandtype {
>> @@ -268,34 +273,40 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>  	int mid;
>>  	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 };
>> +	union acpi_object *obj = NULL;
>> +	struct bios_args *args = NULL;
>> +	size_t bios_args_size = struct_size(args, data, insize);
>> +	
>> +	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;
> 
> The variable declaration here again looks a bit messy, also
> there is no reason to move the block setting + checking mid,
> that just makes the diff unnecessarily large.
> 
> I've cleaned this up while mergin (no functional changes).
> 
> 
>>  
>> -	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 out_free;
>> +	}
>>  
>> -	obj = output.pointer;
>> +	memcpy(args->data, buffer, flex_array_size(args, data, 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);
>> +	if (ret)
>> +		goto out_free;
>> +
>> +	obj = output.pointer;
>> +	if (!obj) {
>>  		ret = -EINVAL;
>>  		goto out_free;
>>  	}
>> @@ -310,9 +321,17 @@ 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;
>> +	}
>> +
> 
> You have now moved the obj->type != ACPI_TYPE_BUFFER check to after the:
> 	
> 	bios_return = (struct bios_return *)obj->buffer.pointer;
> 
> Statement, which dereferences the buffer member of the obj union.  That check
> MUST be done before looking at the buffer member, so I have moved it back to
> its old place, this also makes the diff/patch smaller.  Note this one is
> a functional change to your patch.
> 
> The changed initial block of the function now looks like this:
> 
> static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
> 				void *buffer, int insize, int outsize)
> {
> 	struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL };
> 	size_t bios_args_size = struct_size(args, data, insize);

Ahum, there is a compile error here because args is not declared yet, so the final
version looks slightly different, see:

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=review-hans&id=9f7e383ebdee6712bee02e3a6c2027cf287950fc

Regards,

Hans


> 	struct bios_return *bios_return;
> 	union acpi_object *obj = NULL;
> 	struct bios_args *args = NULL;
> 	int mid, actual_outsize, ret;
> 
> 	mid = encode_outsize_for_pvsz(outsize);
> 	if (WARN_ON(mid < 0))
> 		return mid;
> 
> 	args = kmalloc(bios_args_size, GFP_KERNEL);
> 	if (!args)
> 		return -ENOMEM;
> 
> 	input.length = bios_args_size;
> 	input.pointer = args;
> 
> 	args->signature = 0x55434553;
> 	args->command = command;
> 	args->commandtype = query;
> 	args->datasize = insize;
> 	memcpy(args->data, buffer, flex_array_size(args, data, insize));
> 
> 	ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> 	if (ret)
> 		goto out_free;
> 
> 	obj = output.pointer;
> 	if (!obj) {
> 		ret = -EINVAL;
> 		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;
> 	}
> 
> 	bios_return = (struct bios_return *)obj->buffer.pointer;
> 
> 
> And the new diff for this chunk of the patch now is:
> 
> @@ -266,37 +271,42 @@ static inline int encode_outsize_for_pvsz(int outsize)
>  static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>  				void *buffer, int insize, int outsize)
>  {
> -	int mid;
> +	struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	size_t bios_args_size = struct_size(args, data, insize);
>  	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 };
> -	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> -	int ret = 0;
> +	union acpi_object *obj = NULL;
> +	struct bios_args *args = NULL;
> +	int mid, actual_outsize, ret;
>  
>  	mid = encode_outsize_for_pvsz(outsize);
>  	if (WARN_ON(mid < 0))
>  		return mid;
>  
> -	if (WARN_ON(insize > sizeof(args.data)))
> -		return -EINVAL;
> -	memcpy(&args.data[0], buffer, insize);
> +	args = kmalloc(bios_args_size, GFP_KERNEL);
> +	if (!args)
> +		return -ENOMEM;
>  
> -	wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> +	input.length = bios_args_size;
> +	input.pointer = args;
>  
> -	obj = output.pointer;
> +	args->signature = 0x55434553;
> +	args->command = command;
> +	args->commandtype = query;
> +	args->datasize = insize;
> +	memcpy(args->data, buffer, flex_array_size(args, data, insize));
>  
> -	if (!obj)
> -		return -EINVAL;
> +	ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> +	if (ret)
> +		goto out_free;
> +
> +	obj = output.pointer;
> +	if (!obj) {
> +		ret = -EINVAL;
> +		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;
>  	}
> @@ ...
> 
> I've merged this patch with the above changes:
> 
> Thank you for your patch, I've applied this patch to my review-hans 
> branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> Note it will show up in my review-hans branch once I've pushed my
> local branch there, which might take a while.
> 
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> 
> 
>>  	/* Ignore output data of zero size */
>> -	if (!outsize)
>> +	if (!outsize) {
>> +		ret = 0;
>>  		goto out_free;
>> +	}
>>  
>>  	actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return)));
>>  	memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize);
>> @@ -320,6 +339,7 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>  
>>  out_free:
>>  	kfree(obj);
>> +	kfree(args);
>>  	return ret;
>>  }
>>  
> 
>