diff mbox series

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

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

Commit Message

James Seo Nov. 3, 2023, 6:19 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.

As this seems quite rare, add a machine-limited workaround for now.

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>
---
Was converting to UTF-8 a good idea? There's also not much UTF-16 validation,
because at the moment it's only being done on one machine with a known input.
---
 drivers/hwmon/hp-wmi-sensors.c | 57 ++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)


base-commit: 0f564130e5c76f1e5cf0008924f6a6cd138929d9

Comments

Guenter Roeck Nov. 3, 2023, 7:36 p.m. UTC | #1 
On 11/3/23 11:19, 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.
> 
> As this seems quite rare, add a machine-limited workaround for now.
> 
> 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>
> ---
[ ... ]

> +static bool is_raw_wmi_string(const acpi_object_type property_map[], int prop)
> +{
> +	const char *board_name;
> +
> +	if (property_map != hp_wmi_platform_events_property_map ||
> +	    prop != HP_WMI_PLATFORM_EVENTS_PROPERTY_NAME)
> +		return false;
> +
> +	board_name = dmi_get_system_info(DMI_BOARD_NAME);
> +	if (!board_name)
> +		return false;
> +
> +	return !strcmp(board_name, HP_WMI_BOARD_NAME_ELITEDESK_800_G6);

Would it be possible to use a dmi table and dmi_check_system() ?
That would make it easier to add more platforms later on if needed.

Thanks,
Guenter
James Seo Nov. 4, 2023, 4:07 p.m. UTC | #2 
On Fri, Nov 03, 2023 at 12:36:49PM -0700, Guenter Roeck wrote:
> On 11/3/23 11:19, James Seo wrote:
>> +static bool is_raw_wmi_string(const acpi_object_type property_map[], int prop)
>> +{
>> +	const char *board_name;
>> +
>> +	if (property_map != hp_wmi_platform_events_property_map ||
>> +	    prop != HP_WMI_PLATFORM_EVENTS_PROPERTY_NAME)
>> +		return false;
>> +
>> +	board_name = dmi_get_system_info(DMI_BOARD_NAME);
>> +	if (!board_name)
>> +		return false;
>> +
>> +	return !strcmp(board_name, HP_WMI_BOARD_NAME_ELITEDESK_800_G6);
> 
> Would it be possible to use a dmi table and dmi_check_system() ?
> That would make it easier to add more platforms later on if needed.
> 
> Thanks,
> Guenter
>

Hi Guenter,

Sure, I can do something like this:


#define HP_WMI_WSTR_INFO(name, wids) {					\
	.matches = {							\
		DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Hewlett-Packard"),	\
		DMI_EXACT_MATCH(DMI_BOARD_NAME, (name)),		\
	},								\
	.driver_data = (void *)(wids),					\
}

struct hp_wmi_wstr_id {
	const acpi_object_type *property_map;
	int prop;
};

static const struct hp_wmi_wstr_id elitedesk_800_g6_wstr_ids[] = {
	{
		.property_map = hp_wmi_platform_events_property_map,
		.prop = HP_WMI_PLATFORM_EVENTS_PROPERTY_NAME,
	},
	{ },
};

static const struct dmi_system_id hp_wmi_dmi_wstr_table[] = {
	HP_WMI_WSTR_INFO("870C", elitedesk_800_g6_wstr_ids),
	{ },
};

static bool is_raw_wmi_string(const acpi_object_type property_map[], int prop)
{
	const struct hp_wmi_wstr_id *wstr_id;
	const struct dmi_system_id *id;

	id = dmi_first_match(hp_wmi_dmi_wstr_table);
	if (!id)
		return false;

	wstr_id = id->driver_data;
	for (; wstr_id->property_map; wstr_id++)
		if (property_map == wstr_id->property_map &&
		    prop == wstr_id->prop)
			return true;

	return false;
}


Out of curiosity, how would you feel about just adding full raw WMI string
support now? It wouldn't take much more work and for various small reasons
it's starting to look like a better idea to me.

James
Guenter Roeck Nov. 4, 2023, 4:29 p.m. UTC | #3 
On 11/4/23 09:07, James Seo wrote:
> On Fri, Nov 03, 2023 at 12:36:49PM -0700, Guenter Roeck wrote:
>> On 11/3/23 11:19, James Seo wrote:
>>> +static bool is_raw_wmi_string(const acpi_object_type property_map[], int prop)
>>> +{
>>> +	const char *board_name;
>>> +
>>> +	if (property_map != hp_wmi_platform_events_property_map ||
>>> +	    prop != HP_WMI_PLATFORM_EVENTS_PROPERTY_NAME)
>>> +		return false;
>>> +
>>> +	board_name = dmi_get_system_info(DMI_BOARD_NAME);
>>> +	if (!board_name)
>>> +		return false;
>>> +
>>> +	return !strcmp(board_name, HP_WMI_BOARD_NAME_ELITEDESK_800_G6);
>>
>> Would it be possible to use a dmi table and dmi_check_system() ?
>> That would make it easier to add more platforms later on if needed.
>>
>> Thanks,
>> Guenter
>>
> 
> Hi Guenter,
> 
> Sure, I can do something like this:
> 
> 
> #define HP_WMI_WSTR_INFO(name, wids) {					\
> 	.matches = {							\
> 		DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Hewlett-Packard"),	\
> 		DMI_EXACT_MATCH(DMI_BOARD_NAME, (name)),		\
> 	},								\
> 	.driver_data = (void *)(wids),					\
> }
> 

Quite frankly, I dislike multi-line macros because they make it (more)
difficult to understand the code. If that is where you want to go,
I'd rather keep the current code (or wait until someone else maintains
the hwmon subsystem).

> struct hp_wmi_wstr_id {
> 	const acpi_object_type *property_map;
> 	int prop;
> };
> 
> static const struct hp_wmi_wstr_id elitedesk_800_g6_wstr_ids[] = {
> 	{
> 		.property_map = hp_wmi_platform_events_property_map,
> 		.prop = HP_WMI_PLATFORM_EVENTS_PROPERTY_NAME,
> 	},
> 	{ },
> };
> 
> static const struct dmi_system_id hp_wmi_dmi_wstr_table[] = {
> 	HP_WMI_WSTR_INFO("870C", elitedesk_800_g6_wstr_ids),
> 	{ },
> };
> 
> static bool is_raw_wmi_string(const acpi_object_type property_map[], int prop)
> {
> 	const struct hp_wmi_wstr_id *wstr_id;
> 	const struct dmi_system_id *id;
> 
> 	id = dmi_first_match(hp_wmi_dmi_wstr_table);
> 	if (!id)
> 		return false;
> 
> 	wstr_id = id->driver_data;
> 	for (; wstr_id->property_map; wstr_id++)
> 		if (property_map == wstr_id->property_map &&
> 		    prop == wstr_id->prop)
> 			return true;
> 
> 	return false;
> }
> 
> 
> Out of curiosity, how would you feel about just adding full raw WMI string
> support now? It wouldn't take much more work and for various small reasons
> it's starting to look like a better idea to me.
> 

I don't know; I would have to see the code.

Guenter
James Seo Nov. 5, 2023, 6:51 p.m. UTC | #4 
On Sat, Nov 04, 2023 at 09:29:43AM -0700, Guenter Roeck wrote:
> On 11/4/23 09:07, James Seo wrote:
>> 
>> #define HP_WMI_WSTR_INFO(name, wids) {					\
>> 	.matches = {							\
>> 		DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Hewlett-Packard"),	\
>> 		DMI_EXACT_MATCH(DMI_BOARD_NAME, (name)),		\
>> 	},								\
>> 	.driver_data = (void *)(wids),					\
>> }
>> 
> 
> Quite frankly, I dislike multi-line macros because they make it (more)
> difficult to understand the code. If that is where you want to go,
> I'd rather keep the current code (or wait until someone else maintains
> the hwmon subsystem).

OK, I'll remove that macro if I end up using a DMI table for this. 

>> Out of curiosity, how would you feel about just adding full raw WMI string
>> support now? It wouldn't take much more work and for various small reasons
>> it's starting to look like a better idea to me.
>> 
> 
> I don't know; I would have to see the code.

I'll submit something in a bit.
diff mbox series

Patch

diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
index 17ae62f88bbf..c82a9bbf16ca 100644
--- a/drivers/hwmon/hp-wmi-sensors.c
+++ b/drivers/hwmon/hp-wmi-sensors.c
@@ -17,13 +17,17 @@ 
  *     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>
 #include <linux/debugfs.h>
+#include <linux/dmi.h>
 #include <linux/hwmon.h>
 #include <linux/jiffies.h>
 #include <linux/mutex.h>
+#include <linux/nls.h>
 #include <linux/units.h>
 #include <linux/wmi.h>
 
@@ -52,6 +56,10 @@ 
 #define HP_WMI_MAX_PROPERTIES		32U
 #define HP_WMI_MAX_INSTANCES		32U
 
+/* DMI board names for machines requiring workarounds. */
+
+#define HP_WMI_BOARD_NAME_ELITEDESK_800_G6	"870C"
+
 enum hp_wmi_type {
 	HP_WMI_TYPE_OTHER			= 1,
 	HP_WMI_TYPE_TEMPERATURE			= 2,
@@ -412,6 +420,30 @@  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)
+{
+	const wchar_t *src;
+	size_t len;
+	char *dst;
+	int i;
+
+	/* WMI strings are length-prefixed UTF-16. See [5]. */
+	src = (wchar_t *)buf;
+	len = min(*src++ / sizeof(*src), (size_t)HP_WMI_MAX_STR_SIZE - 1);
+	while (len && !src[len - 1])
+		len--;
+
+	dst = devm_kmalloc(dev, (len + 1) * sizeof(*dst), GFP_KERNEL);
+	if (!dst)
+		return NULL;
+
+	i = utf16s_to_utf8s(src, len, UTF16_LITTLE_ENDIAN, dst, len);
+	dst[i] = '\0';
+
+	return strim(dst);
+}
+
 /*
  * hp_wmi_get_wobj - poll WMI for a WMI object instance
  * @guid: WMI object GUID
@@ -442,6 +474,21 @@  static u8 hp_wmi_wobj_instance_count(const char *guid)
 	return clamp(count, 0, (int)HP_WMI_MAX_INSTANCES);
 }
 
+static bool is_raw_wmi_string(const acpi_object_type property_map[], int prop)
+{
+	const char *board_name;
+
+	if (property_map != hp_wmi_platform_events_property_map ||
+	    prop != HP_WMI_PLATFORM_EVENTS_PROPERTY_NAME)
+		return false;
+
+	board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	if (!board_name)
+		return false;
+
+	return !strcmp(board_name, HP_WMI_BOARD_NAME_ELITEDESK_800_G6);
+}
+
 static int check_wobj(const union acpi_object *wobj,
 		      const acpi_object_type property_map[], int last_prop)
 {
@@ -462,8 +509,12 @@  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 &&
+			    is_raw_wmi_string(property_map, prop))
+				continue;
 			return -EINVAL;
+		}
 	}
 
 	return 0;
@@ -480,7 +531,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;