diff mbox series

[RFC,v2] hwmon: (hp-wmi-sensors) Fix failure to load on EliteDesk 800 G6

Message ID 20231105192054.24833-1-james@equiv.tech (mailing list archive)
State Changes Requested
Headers show
Series [RFC,v2] hwmon: (hp-wmi-sensors) Fix failure to load on EliteDesk 800 G6 | expand

Commit Message

James Seo Nov. 5, 2023, 7:20 p.m. UTC 
The EliteDesk 800 G6 stores a raw WMI string within the ACPI object in its
BIOS corresponding to one instance of HPBIOS_PlatformEvents.Name. This is
evidently a valid way of representing a WMI data item as far as the Microsoft
ACPI-WMI mapper is concerned, but is preventing the driver from loading.

This seems quite rare, but add support for such strings. Treating this as a
quirk pretty much means adding that support anyway.

Also clean up an oversight in update_numeric_sensor_from_wobj() in which the
result of hp_wmi_strdup() was being used without error checking.

Reported-by: Lukasz Stelmach <l.stelmach@samsung.com>
Closes: https://lore.kernel.org/linux-hwmon/7850a0bd-60e7-88f8-1d6c-0bb0e3234fdc@roeck-us.net/
Signed-off-by: James Seo <james@equiv.tech>
---

Changes v1->v2:
* Remove DMI-based workaround logic
* Add full support for raw WMI strings
  - Improve UTF-16 validation and conversion for the general case
  - Support such strings if they occur in HPBIOS_BIOSEvent objects
* Only use the result of hp_wmi_strdup() in update_numeric_sensor_from_wobj()
  if the call succeeded

History:
v1: https://lore.kernel.org/linux-hwmon/20231103181931.677796-1-james@equiv.tech/

---

 drivers/hwmon/hp-wmi-sensors.c | 127 ++++++++++++++++++++++++++++-----
 1 file changed, 111 insertions(+), 16 deletions(-)


base-commit: 0f564130e5c76f1e5cf0008924f6a6cd138929d9

Comments

Lukasz Stelmach Nov. 13, 2023, 11:41 a.m. UTC | #1 
It was <2023-11-05 nie 11:20>, when James Seo wrote:
> The EliteDesk 800 G6 stores a raw WMI string within the ACPI object in its
> BIOS corresponding to one instance of HPBIOS_PlatformEvents.Name. This is
> evidently a valid way of representing a WMI data item as far as the Microsoft
> ACPI-WMI mapper is concerned, but is preventing the driver from loading.
>
> This seems quite rare, but add support for such strings. Treating this as a
> quirk pretty much means adding that support anyway.
>
> Also clean up an oversight in update_numeric_sensor_from_wobj() in which the
> result of hp_wmi_strdup() was being used without error checking.
>
> Reported-by: Lukasz Stelmach <l.stelmach@samsung.com>
> Closes: https://lore.kernel.org/linux-hwmon/7850a0bd-60e7-88f8-1d6c-0bb0e3234fdc@roeck-us.net/
> Signed-off-by: James Seo <james@equiv.tech>
> ---
>
> Changes v1->v2:
> * Remove DMI-based workaround logic
> * Add full support for raw WMI strings
>   - Improve UTF-16 validation and conversion for the general case
>   - Support such strings if they occur in HPBIOS_BIOSEvent objects
> * Only use the result of hp_wmi_strdup() in update_numeric_sensor_from_wobj()
>   if the call succeeded
>
> History:
> v1: https://lore.kernel.org/linux-hwmon/20231103181931.677796-1-james@equiv.tech/
>
> ---
>
>  drivers/hwmon/hp-wmi-sensors.c | 127 ++++++++++++++++++++++++++++-----
>  1 file changed, 111 insertions(+), 16 deletions(-)
>

Tested-by: Lukasz Stelmach <l.stelmach@samsung.com>

> diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
> index 17ae62f88bbf..bdd7ca163593 100644
> --- a/drivers/hwmon/hp-wmi-sensors.c
> +++ b/drivers/hwmon/hp-wmi-sensors.c
> @@ -17,6 +17,8 @@
>   *     Available: https://github.com/linuxhw/ACPI
>   * [4] P. Rohár, "bmfdec - Decompile binary MOF file (BMF) from WMI buffer",
>   *     2017. [Online]. Available: https://github.com/pali/bmfdec
> + * [5] Microsoft Corporation, "Driver-Defined WMI Data Items", 2017. [Online].
> + *     Available: https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/driver-defined-wmi-data-items
>   */
>  
>  #include <linux/acpi.h>
> @@ -24,6 +26,7 @@
>  #include <linux/hwmon.h>
>  #include <linux/jiffies.h>
>  #include <linux/mutex.h>
> +#include <linux/nls.h>
>  #include <linux/units.h>
>  #include <linux/wmi.h>
>  
> @@ -395,6 +398,50 @@ struct hp_wmi_sensors {
>  	struct mutex lock;	/* Lock polling WMI and driver state changes. */
>  };
>  
> +static bool is_raw_wmi_string(const u8 *pointer, u32 length)
> +{
> +	const u16 *ptr;
> +	u16 len;
> +
> +	/* WMI strings are length-prefixed UTF-16 [5]. */
> +	if (length <= sizeof(*ptr))
> +		return false;
> +
> +	length -= sizeof(*ptr);
> +	ptr = (const u16 *)pointer;
> +	len = *ptr;
> +
> +	return len <= length && !(len & 1);
> +}
> +
> +static char *convert_raw_wmi_string(const u8 *buf)
> +{
> +	const wchar_t *src;
> +	unsigned cps;
> +	unsigned len;
> +	char *dst;
> +	int i;
> +
> +	src = (const wchar_t *)buf;
> +
> +	/* Count UTF-16 code points. Exclude trailing null padding. */
> +	cps = *src / sizeof(*src);
> +	while (cps && !src[cps])
> +		cps--;
> +
> +	/* Each code point becomes up to 3 UTF-8 characters. */
> +	len = min(cps * 3, HP_WMI_MAX_STR_SIZE - 1);
> +
> +	dst = kmalloc((len + 1) * sizeof(*dst), GFP_KERNEL);
> +	if (!dst)
> +		return NULL;
> +
> +	i = utf16s_to_utf8s(++src, cps, UTF16_LITTLE_ENDIAN, dst, len);
> +	dst[i] = '\0';
> +
> +	return dst;
> +}
> +
>  /* hp_wmi_strdup - devm_kstrdup, but length-limited */
>  static char *hp_wmi_strdup(struct device *dev, const char *src)
>  {
> @@ -412,6 +459,23 @@ static char *hp_wmi_strdup(struct device *dev, const char *src)
>  	return dst;
>  }
>  
> +/* hp_wmi_wstrdup - hp_wmi_strdup, but for a raw WMI string */
> +static char *hp_wmi_wstrdup(struct device *dev, const u8 *buf)
> +{
> +	char *src;
> +	char *dst;
> +
> +	src = convert_raw_wmi_string(buf);
> +	if (!src)
> +		return NULL;
> +
> +	dst = hp_wmi_strdup(dev, strim(src));	/* Note: Copy is trimmed. */
> +
> +	kfree(src);
> +
> +	return dst;
> +}
> +
>  /*
>   * hp_wmi_get_wobj - poll WMI for a WMI object instance
>   * @guid: WMI object GUID
> @@ -462,8 +526,14 @@ static int check_wobj(const union acpi_object *wobj,
>  	for (prop = 0; prop <= last_prop; prop++) {
>  		type = elements[prop].type;
>  		valid_type = property_map[prop];
> -		if (type != valid_type)
> +		if (type != valid_type) {
> +			if (type == ACPI_TYPE_BUFFER &&
> +			    valid_type == ACPI_TYPE_STRING &&
> +			    is_raw_wmi_string(elements[prop].buffer.pointer,
> +					      elements[prop].buffer.length))
> +				continue;
>  			return -EINVAL;
> +		}
>  	}
>  
>  	return 0;
> @@ -480,7 +550,9 @@ static int extract_acpi_value(struct device *dev,
>  		break;
>  
>  	case ACPI_TYPE_STRING:
> -		*out_string = hp_wmi_strdup(dev, strim(element->string.pointer));
> +		*out_string = element->type == ACPI_TYPE_BUFFER ?
> +			hp_wmi_wstrdup(dev, element->buffer.pointer) :
> +			hp_wmi_strdup(dev, strim(element->string.pointer));
>  		if (!*out_string)
>  			return -ENOMEM;
>  		break;
> @@ -861,7 +933,9 @@ update_numeric_sensor_from_wobj(struct device *dev,
>  {
>  	const union acpi_object *elements;
>  	const union acpi_object *element;
> -	const char *string;
> +	const char *new_string;
> +	char *trimmed;
> +	char *string;
>  	bool is_new;
>  	int offset;
>  	u8 size;
> @@ -885,11 +959,21 @@ update_numeric_sensor_from_wobj(struct device *dev,
>  	offset = is_new ? size - 1 : -2;
>  
>  	element = &elements[HP_WMI_PROPERTY_CURRENT_STATE + offset];
> -	string = strim(element->string.pointer);
> -
> -	if (strcmp(string, nsensor->current_state)) {
> -		devm_kfree(dev, nsensor->current_state);
> -		nsensor->current_state = hp_wmi_strdup(dev, string);
> +	string = element->type == ACPI_TYPE_BUFFER ?
> +		convert_raw_wmi_string(element->buffer.pointer) :
> +		element->string.pointer;
> +
> +	if (string) {
> +		trimmed = strim(string);
> +		if (strcmp(trimmed, nsensor->current_state)) {
> +			new_string = hp_wmi_strdup(dev, trimmed);
> +			if (new_string) {
> +				devm_kfree(dev, nsensor->current_state);
> +				nsensor->current_state = new_string;
> +			}
> +		}
> +		if (element->type == ACPI_TYPE_BUFFER)
> +			kfree(string);
>  	}
>  
>  	/* Old variant: -2 (not -1) because it lacks the Size property. */
> @@ -996,11 +1080,15 @@ static int check_event_wobj(const union acpi_object *wobj)
>  			  HP_WMI_EVENT_PROPERTY_STATUS);
>  }
>  
> -static int populate_event_from_wobj(struct hp_wmi_event *event,
> +static int populate_event_from_wobj(struct device *dev,
> +				    struct hp_wmi_event *event,
>  				    union acpi_object *wobj)
>  {
>  	int prop = HP_WMI_EVENT_PROPERTY_NAME;
>  	union acpi_object *element;
> +	acpi_object_type type;
> +	char *string;
> +	u32 value;
>  	int err;
>  
>  	err = check_event_wobj(wobj);
> @@ -1009,20 +1097,24 @@ static int populate_event_from_wobj(struct hp_wmi_event *event,
>  
>  	element = wobj->package.elements;
>  
> -	/* Extracted strings are NOT device-managed copies. */
> -
>  	for (; prop <= HP_WMI_EVENT_PROPERTY_CATEGORY; prop++, element++) {
> +		type = hp_wmi_event_property_map[prop];
> +
> +		err = extract_acpi_value(dev, element, type, &value, &string);
> +		if (err)
> +			return err;
> +
>  		switch (prop) {
>  		case HP_WMI_EVENT_PROPERTY_NAME:
> -			event->name = strim(element->string.pointer);
> +			event->name = string;
>  			break;
>  
>  		case HP_WMI_EVENT_PROPERTY_DESCRIPTION:
> -			event->description = strim(element->string.pointer);
> +			event->description = string;
>  			break;
>  
>  		case HP_WMI_EVENT_PROPERTY_CATEGORY:
> -			event->category = element->integer.value;
> +			event->category = value;
>  			break;
>  
>  		default:
> @@ -1511,8 +1603,8 @@ static void hp_wmi_notify(u32 value, void *context)
>  	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
>  	struct hp_wmi_sensors *state = context;
>  	struct device *dev = &state->wdev->dev;
> +	struct hp_wmi_event event = {};
>  	struct hp_wmi_info *fan_info;
> -	struct hp_wmi_event event;
>  	union acpi_object *wobj;
>  	acpi_status err;
>  	int event_type;
> @@ -1546,7 +1638,7 @@ static void hp_wmi_notify(u32 value, void *context)
>  
>  	wobj = out.pointer;
>  
> -	err = populate_event_from_wobj(&event, wobj);
> +	err = populate_event_from_wobj(dev, &event, wobj);
>  	if (err) {
>  		dev_warn(dev, "Bad event data (ACPI type %d)\n", wobj->type);
>  		goto out_free_wobj;
> @@ -1577,6 +1669,9 @@ static void hp_wmi_notify(u32 value, void *context)
>  out_free_wobj:
>  	kfree(wobj);
>  
> +	devm_kfree(dev, event.name);
> +	devm_kfree(dev, event.description);
> +
>  out_unlock:
>  	mutex_unlock(&state->lock);
>  }
>
> base-commit: 0f564130e5c76f1e5cf0008924f6a6cd138929d9
Guenter Roeck Nov. 21, 2023, 4:02 p.m. UTC | #2 
On Sun, Nov 05, 2023 at 11:20:54AM -0800, James Seo wrote:
> The EliteDesk 800 G6 stores a raw WMI string within the ACPI object in its
> BIOS corresponding to one instance of HPBIOS_PlatformEvents.Name. This is
> evidently a valid way of representing a WMI data item as far as the Microsoft
> ACPI-WMI mapper is concerned, but is preventing the driver from loading.
> 
> This seems quite rare, but add support for such strings. Treating this as a
> quirk pretty much means adding that support anyway.
> 
> Also clean up an oversight in update_numeric_sensor_from_wobj() in which the
> result of hp_wmi_strdup() was being used without error checking.
> 
> Reported-by: Lukasz Stelmach <l.stelmach@samsung.com>
> Closes: https://lore.kernel.org/linux-hwmon/7850a0bd-60e7-88f8-1d6c-0bb0e3234fdc@roeck-us.net/
> Signed-off-by: James Seo <james@equiv.tech>
> Tested-by: Lukasz Stelmach <l.stelmach@samsung.com>

I get several checkpatch warnings when trying to apply this patch.
Please run checkpatch --strict and fix what it reports.

Thanks,
Guenter
diff mbox series

Patch

diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
index 17ae62f88bbf..bdd7ca163593 100644
--- a/drivers/hwmon/hp-wmi-sensors.c
+++ b/drivers/hwmon/hp-wmi-sensors.c
@@ -17,6 +17,8 @@ 
  *     Available: https://github.com/linuxhw/ACPI
  * [4] P. Rohár, "bmfdec - Decompile binary MOF file (BMF) from WMI buffer",
  *     2017. [Online]. Available: https://github.com/pali/bmfdec
+ * [5] Microsoft Corporation, "Driver-Defined WMI Data Items", 2017. [Online].
+ *     Available: https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/driver-defined-wmi-data-items
  */
 
 #include <linux/acpi.h>
@@ -24,6 +26,7 @@ 
 #include <linux/hwmon.h>
 #include <linux/jiffies.h>
 #include <linux/mutex.h>
+#include <linux/nls.h>
 #include <linux/units.h>
 #include <linux/wmi.h>
 
@@ -395,6 +398,50 @@  struct hp_wmi_sensors {
 	struct mutex lock;	/* Lock polling WMI and driver state changes. */
 };
 
+static bool is_raw_wmi_string(const u8 *pointer, u32 length)
+{
+	const u16 *ptr;
+	u16 len;
+
+	/* WMI strings are length-prefixed UTF-16 [5]. */
+	if (length <= sizeof(*ptr))
+		return false;
+
+	length -= sizeof(*ptr);
+	ptr = (const u16 *)pointer;
+	len = *ptr;
+
+	return len <= length && !(len & 1);
+}
+
+static char *convert_raw_wmi_string(const u8 *buf)
+{
+	const wchar_t *src;
+	unsigned cps;
+	unsigned len;
+	char *dst;
+	int i;
+
+	src = (const wchar_t *)buf;
+
+	/* Count UTF-16 code points. Exclude trailing null padding. */
+	cps = *src / sizeof(*src);
+	while (cps && !src[cps])
+		cps--;
+
+	/* Each code point becomes up to 3 UTF-8 characters. */
+	len = min(cps * 3, HP_WMI_MAX_STR_SIZE - 1);
+
+	dst = kmalloc((len + 1) * sizeof(*dst), GFP_KERNEL);
+	if (!dst)
+		return NULL;
+
+	i = utf16s_to_utf8s(++src, cps, UTF16_LITTLE_ENDIAN, dst, len);
+	dst[i] = '\0';
+
+	return dst;
+}
+
 /* hp_wmi_strdup - devm_kstrdup, but length-limited */
 static char *hp_wmi_strdup(struct device *dev, const char *src)
 {
@@ -412,6 +459,23 @@  static char *hp_wmi_strdup(struct device *dev, const char *src)
 	return dst;
 }
 
+/* hp_wmi_wstrdup - hp_wmi_strdup, but for a raw WMI string */
+static char *hp_wmi_wstrdup(struct device *dev, const u8 *buf)
+{
+	char *src;
+	char *dst;
+
+	src = convert_raw_wmi_string(buf);
+	if (!src)
+		return NULL;
+
+	dst = hp_wmi_strdup(dev, strim(src));	/* Note: Copy is trimmed. */
+
+	kfree(src);
+
+	return dst;
+}
+
 /*
  * hp_wmi_get_wobj - poll WMI for a WMI object instance
  * @guid: WMI object GUID
@@ -462,8 +526,14 @@  static int check_wobj(const union acpi_object *wobj,
 	for (prop = 0; prop <= last_prop; prop++) {
 		type = elements[prop].type;
 		valid_type = property_map[prop];
-		if (type != valid_type)
+		if (type != valid_type) {
+			if (type == ACPI_TYPE_BUFFER &&
+			    valid_type == ACPI_TYPE_STRING &&
+			    is_raw_wmi_string(elements[prop].buffer.pointer,
+					      elements[prop].buffer.length))
+				continue;
 			return -EINVAL;
+		}
 	}
 
 	return 0;
@@ -480,7 +550,9 @@  static int extract_acpi_value(struct device *dev,
 		break;
 
 	case ACPI_TYPE_STRING:
-		*out_string = hp_wmi_strdup(dev, strim(element->string.pointer));
+		*out_string = element->type == ACPI_TYPE_BUFFER ?
+			hp_wmi_wstrdup(dev, element->buffer.pointer) :
+			hp_wmi_strdup(dev, strim(element->string.pointer));
 		if (!*out_string)
 			return -ENOMEM;
 		break;
@@ -861,7 +933,9 @@  update_numeric_sensor_from_wobj(struct device *dev,
 {
 	const union acpi_object *elements;
 	const union acpi_object *element;
-	const char *string;
+	const char *new_string;
+	char *trimmed;
+	char *string;
 	bool is_new;
 	int offset;
 	u8 size;
@@ -885,11 +959,21 @@  update_numeric_sensor_from_wobj(struct device *dev,
 	offset = is_new ? size - 1 : -2;
 
 	element = &elements[HP_WMI_PROPERTY_CURRENT_STATE + offset];
-	string = strim(element->string.pointer);
-
-	if (strcmp(string, nsensor->current_state)) {
-		devm_kfree(dev, nsensor->current_state);
-		nsensor->current_state = hp_wmi_strdup(dev, string);
+	string = element->type == ACPI_TYPE_BUFFER ?
+		convert_raw_wmi_string(element->buffer.pointer) :
+		element->string.pointer;
+
+	if (string) {
+		trimmed = strim(string);
+		if (strcmp(trimmed, nsensor->current_state)) {
+			new_string = hp_wmi_strdup(dev, trimmed);
+			if (new_string) {
+				devm_kfree(dev, nsensor->current_state);
+				nsensor->current_state = new_string;
+			}
+		}
+		if (element->type == ACPI_TYPE_BUFFER)
+			kfree(string);
 	}
 
 	/* Old variant: -2 (not -1) because it lacks the Size property. */
@@ -996,11 +1080,15 @@  static int check_event_wobj(const union acpi_object *wobj)
 			  HP_WMI_EVENT_PROPERTY_STATUS);
 }
 
-static int populate_event_from_wobj(struct hp_wmi_event *event,
+static int populate_event_from_wobj(struct device *dev,
+				    struct hp_wmi_event *event,
 				    union acpi_object *wobj)
 {
 	int prop = HP_WMI_EVENT_PROPERTY_NAME;
 	union acpi_object *element;
+	acpi_object_type type;
+	char *string;
+	u32 value;
 	int err;
 
 	err = check_event_wobj(wobj);
@@ -1009,20 +1097,24 @@  static int populate_event_from_wobj(struct hp_wmi_event *event,
 
 	element = wobj->package.elements;
 
-	/* Extracted strings are NOT device-managed copies. */
-
 	for (; prop <= HP_WMI_EVENT_PROPERTY_CATEGORY; prop++, element++) {
+		type = hp_wmi_event_property_map[prop];
+
+		err = extract_acpi_value(dev, element, type, &value, &string);
+		if (err)
+			return err;
+
 		switch (prop) {
 		case HP_WMI_EVENT_PROPERTY_NAME:
-			event->name = strim(element->string.pointer);
+			event->name = string;
 			break;
 
 		case HP_WMI_EVENT_PROPERTY_DESCRIPTION:
-			event->description = strim(element->string.pointer);
+			event->description = string;
 			break;
 
 		case HP_WMI_EVENT_PROPERTY_CATEGORY:
-			event->category = element->integer.value;
+			event->category = value;
 			break;
 
 		default:
@@ -1511,8 +1603,8 @@  static void hp_wmi_notify(u32 value, void *context)
 	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
 	struct hp_wmi_sensors *state = context;
 	struct device *dev = &state->wdev->dev;
+	struct hp_wmi_event event = {};
 	struct hp_wmi_info *fan_info;
-	struct hp_wmi_event event;
 	union acpi_object *wobj;
 	acpi_status err;
 	int event_type;
@@ -1546,7 +1638,7 @@  static void hp_wmi_notify(u32 value, void *context)
 
 	wobj = out.pointer;
 
-	err = populate_event_from_wobj(&event, wobj);
+	err = populate_event_from_wobj(dev, &event, wobj);
 	if (err) {
 		dev_warn(dev, "Bad event data (ACPI type %d)\n", wobj->type);
 		goto out_free_wobj;
@@ -1577,6 +1669,9 @@  static void hp_wmi_notify(u32 value, void *context)
 out_free_wobj:
 	kfree(wobj);
 
+	devm_kfree(dev, event.name);
+	devm_kfree(dev, event.description);
+
 out_unlock:
 	mutex_unlock(&state->lock);
 }