linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] hwmon: add HP WMI Sensors driver
@ 2023-04-06 15:23 James Seo
  2023-04-06 16:04 ` Armin Wolf
  0 siblings, 1 reply; 5+ messages in thread
From: James Seo @ 2023-04-06 15:23 UTC (permalink / raw)
  Cc: Jean Delvare, Guenter Roeck, linux-hwmon, platform-driver-x86,
	linux-kernel, James Seo

Hewlett-Packard business-class computers report hardware monitoring
information via WMI. This driver exposes that information to hwmon.
Initial support is provided for temperature, voltage, current, and
fan speed sensor types.

HP's WMI implementation permits many other types of numeric sensors.
Therefore, a debugfs interface is also provided to enumerate and
inspect all numeric sensors visible on the WMI side. This should
facilitate adding support for other sensor types in the future.

Tested on a HP Z420 and a HP EliteOne 800 G1.

Note that these models only have temperature and fan speed sensors,
so other sensor types have not been tested working yet. However, the
driver should still work as expected. A 2005 HP whitepaper describes
the relevant MOF definition and sensor value calculation, and both
this driver and the official HP Performance Advisor utility comply
with them (confirmed in the latter case by reverse engineering).

Link: https://h20331.www2.hp.com/hpsub/downloads/cmi_whitepaper.pdf
Signed-off-by: James Seo <james@equiv.tech>

---

Changes in v2:
* Reword commit message
* Reword documentation
* Remove stray #include
* Use DIV_ROUND_CLOSEST in F to C conversion
* Consolidate uses of mutex_lock()/mutex_unlock()
* Process sensors in forward order by type

History:
v1: https://lore.kernel.org/linux-hwmon/20230403084859.26286-1-james@equiv.tech/
---
 Documentation/hwmon/hp-wmi-sensors.rst |   95 +++
 Documentation/hwmon/index.rst          |    1 +
 MAINTAINERS                            |    7 +
 drivers/hwmon/Kconfig                  |   11 +
 drivers/hwmon/Makefile                 |    1 +
 drivers/hwmon/hp-wmi-sensors.c         | 1034 ++++++++++++++++++++++++
 6 files changed, 1149 insertions(+)
 create mode 100644 Documentation/hwmon/hp-wmi-sensors.rst
 create mode 100644 drivers/hwmon/hp-wmi-sensors.c

diff --git a/Documentation/hwmon/hp-wmi-sensors.rst b/Documentation/hwmon/hp-wmi-sensors.rst
new file mode 100644
index 000000000000..71b1391d5072
--- /dev/null
+++ b/Documentation/hwmon/hp-wmi-sensors.rst
@@ -0,0 +1,95 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+.. include:: <isonum.txt>
+
+Linux HP WMI Sensors Driver
+===========================
+
+:Copyright: |copy| 2023 James Seo <james@equiv.tech>
+
+Description
+-----------
+
+Hewlett-Packard business-class computers report hardware monitoring information
+via Windows Management Instrumentation (WMI). This driver exposes that
+information to the Linux ``hwmon`` subsystem, allowing familiar userspace
+utilities like ``sensors`` to gather numeric sensor readings.
+
+``sysfs`` interface
+-------------------
+
+When the driver is loaded, it discovers the sensors available on the current
+system and creates the following read-only ``sysfs`` attributes as appropriate
+within ``/sys/class/hwmon/hwmonX``:
+
+================ ==================================================
+Name		 Description
+================ ==================================================
+curr[X]_input    Current in milliamperes (mA).
+curr[X]_label    Current sensor label.
+fan[X]_input     Fan speed in RPM.
+fan[X]_label     Fan sensor label.
+fan[X]_fault     Fan sensor fault indicator.
+in[X]_input      Voltage in millivolts (mV).
+in[X]_label      Voltage sensor label.
+temp[X]_input    Temperature in millidegrees Celsius (m\ |deg|\ C).
+temp[X]_label    Temperature sensor label.
+temp[X]_fault    Temperature sensor fault indicator.
+================ ==================================================
+
+Here, ``X`` is some number that depends on other available sensors and on other
+system hardware components.
+
+``fault`` attributes
+  Reading ``1`` instead of ``0`` as the ``fault`` attribute for a sensor
+  indicates that the sensor has encountered some issue during operation such
+  that measurements from it should no longer be trusted.
+
+``debugfs`` interface
+---------------------
+
+The standard ``hwmon`` interface in ``sysfs`` exposes sensors of several common
+types that are connected and operating normally as of driver initialization.
+However, there are usually other sensors on the WMI side that do not meet these
+criteria. This driver therefore provides a ``debugfs`` interface in
+``/sys/kernel/debug/hp-wmi-sensors-X`` that allows read-only access to *all* HP
+WMI sensors on the current system.
+
+.. warning:: The ``debugfs`` interface is only available when the kernel is
+             compiled with option ``CONFIG_DEBUG_FS``, and its implementation
+             is subject to change without notice at any time.
+
+One numbered entry is created per sensor with the following attributes:
+
+=============================== ==========================================
+Name				Example
+=============================== ==========================================
+name                            ``CPU0 Fan``
+description                     ``Reports CPU0 fan speed``
+sensor_type                     ``12``
+other_sensor_type               ``(null)``
+operational_status              ``2``
+current_state                   ``Normal``
+possible_states                 ``Normal\nCaution\nCritical\nNot Present``
+base_units                      ``19``
+unit_modifier                   ``0``
+current_reading                 ``1008``
+=============================== ==========================================
+
+These represent the properties of the underlying ``HP_BIOSNumericSensor`` WMI
+object. Contents may vary somewhat between systems. See [#]_ for more details.
+
+Known issues and limitations
+----------------------------
+
+- Non-numeric HP sensor types such as intrusion sensors that belong to the
+  ``HP_BIOSStateSensor`` WMI object type are not supported.
+- HP's WMI implementation permits sensors to claim to be of any type.
+  However, only sensor types already known to hwmon can be supported.
+
+References
+----------
+
+.. [#] Hewlett-Packard Development Company, L.P.,
+       "HP Client Management Interface Technical White Paper", 2005. [Online].
+       Available: https://h20331.www2.hp.com/hpsub/downloads/cmi_whitepaper.pdf
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index f1fe75f596a5..f8f3c0bef6ed 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -77,6 +77,7 @@ Hardware Monitoring Kernel Drivers
    gl518sm
    gxp-fan-ctrl
    hih6130
+   hp-wmi-sensors
    ibmaem
    ibm-cffps
    ibmpowernv
diff --git a/MAINTAINERS b/MAINTAINERS
index 90abe83c02f3..264960e4c77e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9373,6 +9373,13 @@ L:	platform-driver-x86@vger.kernel.org
 S:	Orphan
 F:	drivers/platform/x86/hp/tc1100-wmi.c
 
+HP WMI HARDWARE MONITOR DRIVER
+M:	James Seo <james@equiv.tech>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/hp-wmi-sensors.rst
+F:	drivers/hwmon/hp-wmi-sensors.c
+
 HPET:	High Precision Event Timers driver
 M:	Clemens Ladisch <clemens@ladisch.de>
 S:	Maintained
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5b3b76477b0e..78798e0ed5e6 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2399,6 +2399,17 @@ config SENSORS_ASUS_EC
 	  This driver can also be built as a module. If so, the module
 	  will be called asus_ec_sensors.
 
+config SENSORS_HP_WMI
+	tristate "HP WMI Sensors"
+	depends on ACPI_WMI
+	help
+	  If you say yes here you get support for the ACPI hardware monitoring
+	  interface found in HP business-class computers. Temperature, voltage,
+	  current, and fan speed sensor types are supported if present.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called hp_wmi_sensors.
+
 endif # ACPI
 
 endif # HWMON
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 88712b5031c8..05cce16f37f6 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_ACPI_POWER) += acpi_power_meter.o
 obj-$(CONFIG_SENSORS_ATK0110)	+= asus_atk0110.o
 obj-$(CONFIG_SENSORS_ASUS_EC)	+= asus-ec-sensors.o
 obj-$(CONFIG_SENSORS_ASUS_WMI)	+= asus_wmi_sensors.o
+obj-$(CONFIG_SENSORS_HP_WMI)	+= hp-wmi-sensors.o
 
 # Native drivers
 # asb100, then w83781d go first, as they can override other drivers' addresses.
diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
new file mode 100644
index 000000000000..8c1f09fcdfce
--- /dev/null
+++ b/drivers/hwmon/hp-wmi-sensors.c
@@ -0,0 +1,1034 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * hwmon driver for HP business-class computers that report numeric
+ * sensor data via Windows Management Instrumentation (WMI).
+ *
+ * Copyright (C) 2023 James Seo <james@equiv.tech>
+ *
+ * References:
+ * [1] Hewlett-Packard Development Company, L.P.,
+ *     "HP Client Management Interface Technical White Paper", 2005. [Online].
+ *     Available: https://h20331.www2.hp.com/hpsub/downloads/cmi_whitepaper.pdf
+ */
+
+#include <linux/acpi.h>
+#include <linux/debugfs.h>
+#include <linux/hwmon.h>
+#include <linux/jiffies.h>
+#include <linux/mutex.h>
+#include <linux/units.h>
+#include <linux/wmi.h>
+
+/*
+ * MOF definition of the HP_BIOSNumericSensor WMI object [1]:
+ *
+ *   #pragma namespace("\\\\.\\root\\HP\\InstrumentedBIOS");
+ *
+ *   [abstract]
+ *   class HP_BIOSSensor
+ *   {
+ *     [read] string Name;
+ *     [read] string Description;
+ *     [read, ValueMap {"0","1","2","3","4","5","6","7","8","9",
+ *      "10","11","12"}, Values {"Unknown","Other","Temperature",
+ *      "Voltage","Current","Tachometer","Counter","Switch","Lock",
+ *      "Humidity","Smoke Detection","Presence","Air Flow"}]
+ *     uint32 SensorType;
+ *     [read] string OtherSensorType;
+ *     [read, ValueMap {"0","1","2","3","4","5","6","7","8","9",
+ *      "10","11","12","13","14","15","16","17","18","..",
+ *      "0x8000.."}, Values {"Unknown","Other","OK","Degraded",
+ *      "Stressed","Predictive Failure","Error",
+ *      "Non-Recoverable Error","Starting","Stopping","Stopped",
+ *      "In Service","No Contact","Lost Communication","Aborted",
+ *      "Dormant","Supporting Entity in Error","Completed",
+ *      "Power Mode","DMTF Reserved","Vendor Reserved"}]
+ *     uint32 OperationalStatus;
+ *     [read] string CurrentState;
+ *     [read] string PossibleStates[];
+ *   };
+ *
+ *   class HP_BIOSNumericSensor : HP_BIOSSensor
+ *   {
+ *      [read, ValueMap {"0","1","2","3","4","5","6","7","8","9",
+ *       "10","11","12","13","14","15","16","17","18","19","20",
+ *       "21","22","23","24","25","26","27","28","29","30","31",
+ *       "32","33","34","35","36","37","38","39","40","41","42",
+ *       "43","44","45","46","47","48","49","50","51","52","53",
+ *       "54","55","56","57","58","59","60","61","62","63","64",
+ *       "65"}, Values {"Unknown","Other","Degrees C","Degrees F",
+ *       "Degrees K","Volts","Amps","Watts","Joules","Coulombs",
+ *       "VA","Nits","Lumens","Lux","Candelas","kPa","PSI",
+ *       "Newtons","CFM","RPM","Hertz","Seconds","Minutes",
+ *       "Hours","Days","Weeks","Mils","Inches","Feet",
+ *       "Cubic Inches","Cubic Feet","Meters","Cubic Centimeters",
+ *       "Cubic Meters","Liters","Fluid Ounces","Radians",
+ *       "Steradians","Revolutions","Cycles","Gravities","Ounces",
+ *       "Pounds","Foot-Pounds","Ounce-Inches","Gauss","Gilberts",
+ *       "Henries","Farads","Ohms","Siemens","Moles","Becquerels",
+ *       "PPM (parts/million)","Decibels","DbA","DbC","Grays",
+ *       "Sieverts","Color Temperature Degrees K","Bits","Bytes",
+ *       "Words (data)","DoubleWords","QuadWords","Percentage"}]
+ *     uint32 BaseUnits;
+ *     [read] sint32 UnitModifier;
+ *     [read] uint32 CurrentReading;
+ *   };
+ *
+ */
+
+#define HP_WMI_BIOS_GUID	   "5FB7F034-2C63-45E9-BE91-3D44E2C707E4"
+#define HP_WMI_NUMERIC_SENSOR_GUID "8F1F6435-9F42-42C8-BADC-0E9424F20C9A"
+
+/* These limits are arbitrary. The WMI implementation may vary by model. */
+
+#define HP_WMI_MAX_STR_SIZE	   128U
+#define HP_WMI_MAX_PROPERTIES	   32U
+#define HP_WMI_MAX_INSTANCES	   32U
+
+enum hp_wmi_type {
+	HP_WMI_TYPE_OTHER			   = 1,
+	HP_WMI_TYPE_TEMPERATURE			   = 2,
+	HP_WMI_TYPE_VOLTAGE			   = 3,
+	HP_WMI_TYPE_CURRENT			   = 4,
+	HP_WMI_TYPE_AIR_FLOW			   = 12,
+};
+
+enum hp_wmi_status {
+	HP_WMI_STATUS_OK			   = 2,
+};
+
+enum hp_wmi_units {
+	HP_WMI_UNITS_DEGREES_C			   = 2,
+	HP_WMI_UNITS_DEGREES_F			   = 3,
+	HP_WMI_UNITS_DEGREES_K			   = 4,
+	HP_WMI_UNITS_VOLTS			   = 5,
+	HP_WMI_UNITS_AMPS			   = 6,
+	HP_WMI_UNITS_RPM			   = 19,
+};
+
+enum hp_wmi_property {
+	HP_WMI_PROPERTY_NAME			   = 0,
+	HP_WMI_PROPERTY_DESCRIPTION		   = 1,
+	HP_WMI_PROPERTY_SENSOR_TYPE		   = 2,
+	HP_WMI_PROPERTY_OTHER_SENSOR_TYPE	   = 3,
+	HP_WMI_PROPERTY_OPERATIONAL_STATUS	   = 4,
+	HP_WMI_PROPERTY_CURRENT_STATE		   = 5,
+	HP_WMI_PROPERTY_POSSIBLE_STATES		   = 6,
+	HP_WMI_PROPERTY_BASE_UNITS		   = 7,
+	HP_WMI_PROPERTY_UNIT_MODIFIER		   = 8,
+	HP_WMI_PROPERTY_CURRENT_READING		   = 9,
+};
+
+static const acpi_object_type hp_wmi_property_map[] = {
+	[HP_WMI_PROPERTY_NAME]			   = ACPI_TYPE_STRING,
+	[HP_WMI_PROPERTY_DESCRIPTION]		   = ACPI_TYPE_STRING,
+	[HP_WMI_PROPERTY_SENSOR_TYPE]		   = ACPI_TYPE_INTEGER,
+	[HP_WMI_PROPERTY_OTHER_SENSOR_TYPE]	   = ACPI_TYPE_STRING,
+	[HP_WMI_PROPERTY_OPERATIONAL_STATUS]	   = ACPI_TYPE_INTEGER,
+	[HP_WMI_PROPERTY_CURRENT_STATE]		   = ACPI_TYPE_STRING,
+	[HP_WMI_PROPERTY_POSSIBLE_STATES]	   = ACPI_TYPE_STRING,
+	[HP_WMI_PROPERTY_BASE_UNITS]		   = ACPI_TYPE_INTEGER,
+	[HP_WMI_PROPERTY_UNIT_MODIFIER]		   = ACPI_TYPE_INTEGER,
+	[HP_WMI_PROPERTY_CURRENT_READING]	   = ACPI_TYPE_INTEGER,
+};
+
+static const enum hwmon_sensor_types hp_wmi_hwmon_type_map[] = {
+	[HP_WMI_TYPE_TEMPERATURE]		   = hwmon_temp,
+	[HP_WMI_TYPE_VOLTAGE]			   = hwmon_in,
+	[HP_WMI_TYPE_CURRENT]			   = hwmon_curr,
+	[HP_WMI_TYPE_AIR_FLOW]			   = hwmon_fan,
+};
+
+static const u32 hp_wmi_hwmon_attributes[hwmon_max] = {
+	[hwmon_chip] = HWMON_C_REGISTER_TZ,
+	[hwmon_temp] = HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_FAULT,
+	[hwmon_in]   = HWMON_I_INPUT | HWMON_I_LABEL,
+	[hwmon_curr] = HWMON_C_INPUT | HWMON_C_LABEL,
+	[hwmon_fan]  = HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_FAULT,
+};
+
+/*
+ * struct hp_wmi_numeric_sensor - a HP_BIOSNumericSensor instance
+ *
+ * Contains WMI object instance properties. See MOF definition [1].
+ */
+struct hp_wmi_numeric_sensor {
+	const char *name;
+	const char *description;
+	u32 sensor_type;
+	const char *other_sensor_type; /* Explains "Other" SensorType. */
+	u32 operational_status;
+	const char *current_state;
+	const char **possible_states;  /* Count may vary. */
+	u32 base_units;
+	s32 unit_modifier;
+	u32 current_reading;
+
+	u8 possible_states_count;
+};
+
+/*
+ * struct hp_wmi_info - sensor info
+ * @nsensor: numeric sensor properties
+ * @instance: its WMI instance number
+ * @is_active: whether the following fields are valid
+ * @type: its hwmon sensor type
+ * @cached_val: current sensor reading value, scaled for hwmon
+ * @last_updated: when these readings were last updated
+ */
+struct hp_wmi_info {
+	struct hp_wmi_numeric_sensor nsensor;
+	u8 instance;
+
+	bool is_active;
+	enum hwmon_sensor_types type;
+	long cached_val;
+	unsigned long last_updated; /* in jiffies */
+};
+
+/*
+ * struct hp_wmi_sensors - driver state
+ * @wdev: pointer to the parent WMI device
+ * @debugfs: root directory in debugfs
+ * @info: sensor info structs for all sensors visible in WMI
+ * @info_map: access info structs by hwmon type and channel number
+ * @count: count of all sensors visible in WMI
+ * @channel_count: count of hwmon channels by hwmon type
+ * @lock: mutex to lock polling WMI and changes to driver state
+ */
+struct hp_wmi_sensors {
+	struct wmi_device *wdev;
+	struct dentry *debugfs;
+	struct hp_wmi_info info[HP_WMI_MAX_INSTANCES];
+	struct hp_wmi_info **info_map[hwmon_max];
+	u8 count;
+	u8 channel_count[hwmon_max];
+
+	struct mutex lock; /* lock polling WMI, driver state changes */
+};
+
+/* hp_wmi_strdup - devm_kstrdup, but length-limited */
+static char *hp_wmi_strdup(struct device *dev, const char *src, u32 len)
+{
+	char *dst;
+
+	len = min(len, HP_WMI_MAX_STR_SIZE - 1);
+
+	dst = devm_kmalloc(dev, (len + 1) * sizeof(*dst), GFP_KERNEL);
+	if (!dst)
+		return NULL;
+
+	strscpy(dst, src, len + 1);
+
+	return dst;
+}
+
+/*
+ * hp_wmi_get_wobj - poll WMI for a HP_BIOSNumericSensor object instance
+ * @state: pointer to driver state
+ * @instance: WMI object instance number
+ *
+ * Returns a new WMI object instance on success, or NULL on error.
+ * Caller must kfree the result.
+ */
+static union acpi_object *hp_wmi_get_wobj(struct hp_wmi_sensors *state,
+					  u8 instance)
+{
+	struct wmi_device *wdev = state->wdev;
+
+	return wmidev_block_query(wdev, instance);
+}
+
+/*
+ * check_wobj - validate a HP_BIOSNumericSensor WMI object instance
+ * @wobj: pointer to WMI object instance to check
+ * @possible_states_count: out pointer to count of possible states
+ *
+ * Returns 0 on success, or a negative error code on error.
+ */
+static int check_wobj(const union acpi_object *wobj, u8 *possible_states_count)
+{
+	acpi_object_type type = wobj->type;
+	int prop = HP_WMI_PROPERTY_NAME;
+	acpi_object_type valid_type;
+	union acpi_object *elements;
+	u32 elem_count;
+	u8 count = 0;
+	u32 i;
+
+	if (type != ACPI_TYPE_PACKAGE)
+		return -EINVAL;
+
+	elem_count = wobj->package.count;
+	if (elem_count > HP_WMI_MAX_PROPERTIES)
+		return -EINVAL;
+
+	elements = wobj->package.elements;
+	for (i = 0; i < elem_count; i++, prop++) {
+		if (prop > HP_WMI_PROPERTY_CURRENT_READING)
+			return -EINVAL;
+
+		type = elements[i].type;
+		valid_type = hp_wmi_property_map[prop];
+		if (type != valid_type)
+			return -EINVAL;
+
+		/*
+		 * elements is a variable-length array of ACPI objects, one for
+		 * each property of the WMI object instance, except that the
+		 * strs in PossibleStates[] are flattened into this array, and
+		 * their count is found in the WMI BMOF. We don't decode the
+		 * BMOF, so find the count by finding the next int.
+		 */
+
+		if (prop == HP_WMI_PROPERTY_CURRENT_STATE) {
+			prop = HP_WMI_PROPERTY_POSSIBLE_STATES;
+			valid_type = hp_wmi_property_map[prop];
+			for (; i + 1 < elem_count; i++, count++) {
+				type = elements[i + 1].type;
+				if (type != valid_type)
+					break;
+			}
+		}
+	}
+
+	if (!count || prop <= HP_WMI_PROPERTY_CURRENT_READING)
+		return -EINVAL;
+
+	*possible_states_count = count;
+
+	return 0;
+}
+
+static int numeric_sensor_has_fault(const struct hp_wmi_numeric_sensor *nsensor)
+{
+	u32 operational_status = nsensor->operational_status;
+	u32 current_reading = nsensor->current_reading;
+
+	return operational_status != HP_WMI_STATUS_OK || !current_reading;
+}
+
+/* scale_numeric_sensor - scale sensor reading for hwmon */
+static long scale_numeric_sensor(const struct hp_wmi_numeric_sensor *nsensor)
+{
+	u32 current_reading = nsensor->current_reading;
+	s32 unit_modifier = nsensor->unit_modifier;
+	u32 sensor_type = nsensor->sensor_type;
+	u32 base_units = nsensor->base_units;
+	s32 target_modifier;
+	long val;
+
+	/* Fan readings are in RPM units; others are in milliunits. */
+	target_modifier = sensor_type == HP_WMI_TYPE_AIR_FLOW ? 0 : -3;
+
+	val = current_reading;
+
+	for (; unit_modifier < target_modifier; unit_modifier++)
+		val = DIV_ROUND_CLOSEST(val, 10);
+
+	for (; unit_modifier > target_modifier; unit_modifier--) {
+		if (val > LONG_MAX / 10) {
+			val = LONG_MAX;
+			break;
+		}
+		val *= 10;
+	}
+
+	if (sensor_type == HP_WMI_TYPE_TEMPERATURE) {
+		switch (base_units) {
+		case HP_WMI_UNITS_DEGREES_F:
+			val -= MILLI * 32;
+			val = val <= LONG_MAX / 5 ?
+				      DIV_ROUND_CLOSEST(val * 5, 9) :
+				      DIV_ROUND_CLOSEST(val, 9) * 5;
+			break;
+
+		case HP_WMI_UNITS_DEGREES_K:
+			val = milli_kelvin_to_millicelsius(val);
+			break;
+		}
+	}
+
+	return val;
+}
+
+/*
+ * classify_numeric_sensor - classify a numeric sensor
+ * @nsensor: pointer to numeric sensor struct
+ *
+ * Returns an enum hp_wmi_type value on success,
+ * or a negative value if the sensor type is unsupported.
+ */
+static int classify_numeric_sensor(const struct hp_wmi_numeric_sensor *nsensor)
+{
+	u32 sensor_type = nsensor->sensor_type;
+	u32 base_units = nsensor->base_units;
+
+	switch (sensor_type) {
+	case HP_WMI_TYPE_TEMPERATURE:
+		if (base_units == HP_WMI_UNITS_DEGREES_C ||
+		    base_units == HP_WMI_UNITS_DEGREES_F ||
+		    base_units == HP_WMI_UNITS_DEGREES_K)
+			return HP_WMI_TYPE_TEMPERATURE;
+		break;
+
+	case HP_WMI_TYPE_VOLTAGE:
+		if (base_units == HP_WMI_UNITS_VOLTS)
+			return HP_WMI_TYPE_VOLTAGE;
+		break;
+
+	case HP_WMI_TYPE_CURRENT:
+		if (base_units == HP_WMI_UNITS_AMPS)
+			return HP_WMI_TYPE_CURRENT;
+		break;
+
+	case HP_WMI_TYPE_AIR_FLOW:
+		if (base_units == HP_WMI_UNITS_RPM)
+			return HP_WMI_TYPE_AIR_FLOW;
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int
+populate_numeric_sensor_from_wobj(struct device *dev,
+				  struct hp_wmi_numeric_sensor *nsensor,
+				  const union acpi_object *wobj)
+{
+	const union acpi_object *element;
+	const char **possible_states;
+	u8 possible_states_count;
+	acpi_object_type type;
+	const char *string;
+	u32 value;
+	int prop;
+	int err;
+
+	err = check_wobj(wobj, &possible_states_count);
+	if (err)
+		return err;
+
+	possible_states = devm_kcalloc(dev, possible_states_count,
+				       sizeof(*possible_states),
+				       GFP_KERNEL);
+	if (!possible_states)
+		return -ENOMEM;
+
+	element = wobj->package.elements;
+	nsensor->possible_states = possible_states;
+	nsensor->possible_states_count = possible_states_count;
+
+	for (prop = 0; prop <= HP_WMI_PROPERTY_CURRENT_READING; prop++) {
+		type = hp_wmi_property_map[prop];
+
+		switch (type) {
+		case ACPI_TYPE_INTEGER:
+			value = element->integer.value;
+			break;
+
+		case ACPI_TYPE_STRING:
+			string = hp_wmi_strdup(dev, element->string.pointer,
+					       element->string.length);
+			if (!string)
+				return -ENOMEM;
+			break;
+
+		default:
+			return -EINVAL;
+		}
+
+		element++;
+
+		switch (prop) {
+		case HP_WMI_PROPERTY_NAME:
+			nsensor->name = string;
+			break;
+
+		case HP_WMI_PROPERTY_DESCRIPTION:
+			nsensor->description = string;
+			break;
+
+		case HP_WMI_PROPERTY_SENSOR_TYPE:
+			if (value > HP_WMI_TYPE_AIR_FLOW)
+				return -EINVAL;
+
+			nsensor->sensor_type = value;
+
+			/* Skip OtherSensorType if it will be meaningless. */
+			if (value != HP_WMI_TYPE_OTHER) {
+				element++;
+				prop++;
+			}
+
+			break;
+
+		case HP_WMI_PROPERTY_OTHER_SENSOR_TYPE:
+			nsensor->other_sensor_type = string;
+			break;
+
+		case HP_WMI_PROPERTY_OPERATIONAL_STATUS:
+			nsensor->operational_status = value;
+			break;
+
+		case HP_WMI_PROPERTY_CURRENT_STATE:
+			nsensor->current_state = string;
+			break;
+
+		case HP_WMI_PROPERTY_POSSIBLE_STATES:
+			*possible_states++ = string;
+			if (--possible_states_count)
+				prop--;
+			break;
+
+		case HP_WMI_PROPERTY_BASE_UNITS:
+			nsensor->base_units = value;
+			break;
+
+		case HP_WMI_PROPERTY_UNIT_MODIFIER:
+			/* UnitModifier is signed. */
+			nsensor->unit_modifier = (s32)value;
+			break;
+
+		case HP_WMI_PROPERTY_CURRENT_READING:
+			nsensor->current_reading = value;
+			break;
+
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+/* update_numeric_sensor_from_wobj - update fungible sensor properties */
+static void
+update_numeric_sensor_from_wobj(struct device *dev,
+				struct hp_wmi_numeric_sensor *nsensor,
+				const union acpi_object *wobj)
+{
+	const union acpi_object *elements;
+	const union acpi_object *element;
+	const char *string;
+	u32 length;
+	u8 offset;
+
+	elements = wobj->package.elements;
+
+	element = &elements[HP_WMI_PROPERTY_OPERATIONAL_STATUS];
+	nsensor->operational_status = element->integer.value;
+
+	element = &elements[HP_WMI_PROPERTY_CURRENT_STATE];
+	string = element->string.pointer;
+
+	if (strcmp(string, nsensor->current_state)) {
+		length = element->string.length;
+		devm_kfree(dev, nsensor->current_state);
+		nsensor->current_state = hp_wmi_strdup(dev, string, length);
+	}
+
+	/* Offset reads into the elements array after PossibleStates[0]. */
+	offset = nsensor->possible_states_count - 1;
+
+	element = &elements[HP_WMI_PROPERTY_UNIT_MODIFIER + offset];
+	nsensor->unit_modifier = (s32)element->integer.value;
+
+	element = &elements[HP_WMI_PROPERTY_CURRENT_READING + offset];
+	nsensor->current_reading = element->integer.value;
+}
+
+/*
+ * interpret_info - interpret sensor for hwmon
+ * @info: pointer to sensor info struct
+ *
+ * Should be called after the numeric sensor member has been updated.
+ */
+static void interpret_info(struct hp_wmi_info *info)
+{
+	const struct hp_wmi_numeric_sensor *nsensor = &info->nsensor;
+
+	info->cached_val = scale_numeric_sensor(nsensor);
+	info->last_updated = jiffies;
+}
+
+/*
+ * hp_wmi_update_info - poll WMI to update sensor info
+ * @state: pointer to driver state
+ * @info: pointer to sensor info struct
+ *
+ * Returns 0 on success, or a negative error code on error.
+ */
+static int hp_wmi_update_info(struct hp_wmi_sensors *state,
+			      struct hp_wmi_info *info)
+{
+	struct hp_wmi_numeric_sensor *nsensor = &info->nsensor;
+	struct device *dev = &state->wdev->dev;
+	const union acpi_object *wobj;
+	u8 instance = info->instance;
+	int ret = 0;
+
+	if (time_after(jiffies, info->last_updated + HZ)) {
+		mutex_lock(&state->lock);
+
+		wobj = hp_wmi_get_wobj(state, instance);
+		if (!wobj) {
+			ret = -EIO;
+			goto out_free_wobj;
+		}
+
+		update_numeric_sensor_from_wobj(dev, nsensor, wobj);
+
+		interpret_info(info);
+
+out_free_wobj:
+		kfree(wobj);
+
+		mutex_unlock(&state->lock);
+	}
+
+	return ret;
+}
+
+#if CONFIG_DEBUG_FS
+
+static int basic_string_show(struct seq_file *seqf, void *ignored)
+{
+	const char *str = seqf->private;
+
+	seq_printf(seqf, "%s\n", str);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(basic_string);
+
+static int fungible_show(struct seq_file *seqf, enum hp_wmi_property prop)
+{
+	struct hp_wmi_numeric_sensor *nsensor;
+	struct hp_wmi_sensors *state;
+	struct hp_wmi_info *info;
+	int err;
+
+	switch (prop) {
+	case HP_WMI_PROPERTY_OPERATIONAL_STATUS:
+		nsensor = container_of(seqf->private,
+				       struct hp_wmi_numeric_sensor,
+				       operational_status);
+		break;
+
+	case HP_WMI_PROPERTY_CURRENT_STATE:
+		nsensor = container_of(seqf->private,
+				       struct hp_wmi_numeric_sensor,
+				       current_state);
+		break;
+
+	case HP_WMI_PROPERTY_UNIT_MODIFIER:
+		nsensor = container_of(seqf->private,
+				       struct hp_wmi_numeric_sensor,
+				       unit_modifier);
+		break;
+
+	case HP_WMI_PROPERTY_CURRENT_READING:
+		nsensor = container_of(seqf->private,
+				       struct hp_wmi_numeric_sensor,
+				       current_reading);
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	info = container_of(nsensor, struct hp_wmi_info, nsensor);
+	state = container_of(info, struct hp_wmi_sensors, info[info->instance]);
+
+	err = hp_wmi_update_info(state, info);
+	if (err)
+		return err;
+
+	switch (prop) {
+	case HP_WMI_PROPERTY_OPERATIONAL_STATUS:
+		seq_printf(seqf, "%u\n", nsensor->operational_status);
+		break;
+
+	case HP_WMI_PROPERTY_CURRENT_STATE:
+		seq_printf(seqf, "%s\n", nsensor->current_state);
+		break;
+
+	case HP_WMI_PROPERTY_UNIT_MODIFIER:
+		seq_printf(seqf, "%d\n", nsensor->unit_modifier);
+		break;
+
+	case HP_WMI_PROPERTY_CURRENT_READING:
+		seq_printf(seqf, "%u\n", nsensor->current_reading);
+		break;
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int operational_status_show(struct seq_file *seqf, void *ignored)
+{
+	return fungible_show(seqf, HP_WMI_PROPERTY_OPERATIONAL_STATUS);
+}
+DEFINE_SHOW_ATTRIBUTE(operational_status);
+
+static int current_state_show(struct seq_file *seqf, void *ignored)
+{
+	return fungible_show(seqf, HP_WMI_PROPERTY_CURRENT_STATE);
+}
+DEFINE_SHOW_ATTRIBUTE(current_state);
+
+static int possible_states_show(struct seq_file *seqf, void *ignored)
+{
+	struct hp_wmi_numeric_sensor *nsensor = seqf->private;
+	u8 i;
+
+	for (i = 0; i < nsensor->possible_states_count; i++)
+		seq_printf(seqf, "%s\n", nsensor->possible_states[i]);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(possible_states);
+
+static int unit_modifier_show(struct seq_file *seqf, void *ignored)
+{
+	return fungible_show(seqf, HP_WMI_PROPERTY_UNIT_MODIFIER);
+}
+DEFINE_SHOW_ATTRIBUTE(unit_modifier);
+
+static int current_reading_show(struct seq_file *seqf, void *ignored)
+{
+	return fungible_show(seqf, HP_WMI_PROPERTY_CURRENT_READING);
+}
+DEFINE_SHOW_ATTRIBUTE(current_reading);
+
+/* hp_wmi_devm_debugfs_remove - devm callback for debugfs cleanup */
+static void hp_wmi_devm_debugfs_remove(void *res)
+{
+	debugfs_remove_recursive(res);
+}
+
+/* hp_wmi_debugfs_init - create and populate debugfs directory tree */
+static void hp_wmi_debugfs_init(struct hp_wmi_sensors *state)
+{
+	struct device *dev = &state->wdev->dev;
+	struct hp_wmi_info *info = state->info;
+	struct hp_wmi_numeric_sensor *nsensor;
+	char buf[HP_WMI_MAX_STR_SIZE];
+	struct dentry *dir;
+	int err;
+	u8 i;
+
+	/* dev_name() gives a not-very-friendly GUID for WMI devices. */
+	scnprintf(buf, sizeof(buf), "%s-%u", "hp-wmi-sensors", dev->id);
+
+	state->debugfs = debugfs_create_dir(buf, NULL);
+	if (IS_ERR(state->debugfs))
+		return;
+
+	err = devm_add_action(dev, hp_wmi_devm_debugfs_remove, state->debugfs);
+	if (err) {
+		debugfs_remove(state->debugfs);
+		return;
+	}
+
+	for (i = 0; i < state->count; i++, info++) {
+		nsensor = &info->nsensor;
+
+		scnprintf(buf, sizeof(buf), "%u", i);
+		dir = debugfs_create_dir(buf, state->debugfs);
+
+		debugfs_create_file("name", 0444, dir,
+				    (void *)nsensor->name,
+				    &basic_string_fops);
+
+		debugfs_create_file("description", 0444, dir,
+				    (void *)nsensor->description,
+				    &basic_string_fops);
+
+		debugfs_create_u32("sensor_type", 0444, dir,
+				   &nsensor->sensor_type);
+
+		debugfs_create_file("other_sensor_type", 0444, dir,
+				    (void *)nsensor->other_sensor_type,
+				    &basic_string_fops);
+
+		debugfs_create_file("operational_status", 0444, dir,
+				    &nsensor->operational_status,
+				    &operational_status_fops);
+
+		debugfs_create_file("current_state", 0444, dir,
+				    (void *)&nsensor->current_state,
+				    &current_state_fops);
+
+		debugfs_create_file("possible_states", 0444, dir,
+				    nsensor, &possible_states_fops);
+
+		debugfs_create_u32("base_units", 0444, dir,
+				   &nsensor->base_units);
+
+		debugfs_create_file("unit_modifier", 0444, dir,
+				    &nsensor->unit_modifier,
+				    &unit_modifier_fops);
+
+		debugfs_create_file("current_reading", 0444, dir,
+				    &nsensor->current_reading,
+				    &current_reading_fops);
+	}
+}
+
+#else
+
+static void hp_wmi_debugfs_init(struct hp_wmi_sensors *state)
+{
+}
+
+#endif
+
+static umode_t hp_wmi_hwmon_is_visible(const void *drvdata,
+				       enum hwmon_sensor_types type,
+				       u32 attr, int channel)
+{
+	const struct hp_wmi_sensors *state = drvdata;
+
+	if (!state->info_map[type] || !state->info_map[type][channel])
+		return 0;
+
+	return 0444;
+}
+
+static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			     u32 attr, int channel, long *val)
+{
+	struct hp_wmi_sensors *state = dev_get_drvdata(dev);
+	const struct hp_wmi_numeric_sensor *nsensor;
+	struct hp_wmi_info *info;
+	int err;
+
+	info = state->info_map[type][channel];
+	nsensor = &info->nsensor;
+
+	err = hp_wmi_update_info(state, info);
+	if (err)
+		return err;
+
+	else if ((type == hwmon_temp && attr == hwmon_temp_fault) ||
+		 (type == hwmon_fan  && attr == hwmon_fan_fault))
+		*val = numeric_sensor_has_fault(nsensor);
+
+	else
+		*val = info->cached_val;
+
+	return 0;
+}
+
+static int hp_wmi_hwmon_read_string(struct device *dev,
+				    enum hwmon_sensor_types type, u32 attr,
+				    int channel, const char **str)
+{
+	const struct hp_wmi_sensors *state = dev_get_drvdata(dev);
+	const struct hp_wmi_info *info;
+
+	info = state->info_map[type][channel];
+	*str = info->nsensor.name;
+
+	return 0;
+}
+
+static int add_channel_info(struct device *dev,
+			    struct hwmon_channel_info *channel_info,
+			    u8 count, enum hwmon_sensor_types type)
+{
+	u32 attr = hp_wmi_hwmon_attributes[type];
+	u32 *config;
+
+	config = devm_kcalloc(dev, count + 1, sizeof(*config), GFP_KERNEL);
+	if (!config)
+		return -ENOMEM;
+
+	channel_info->type = type;
+	channel_info->config = config;
+	memset32(config, attr, count);
+
+	return 0;
+}
+
+static const struct hwmon_ops hp_wmi_hwmon_ops = {
+	.is_visible  = hp_wmi_hwmon_is_visible,
+	.read	     = hp_wmi_hwmon_read,
+	.read_string = hp_wmi_hwmon_read_string,
+};
+
+static struct hwmon_chip_info hp_wmi_chip_info = {
+	.ops         = &hp_wmi_hwmon_ops,
+	.info        = NULL,
+};
+
+static int hp_wmi_sensors_init(struct hp_wmi_sensors *state)
+{
+	struct hp_wmi_info *active_info[HP_WMI_MAX_INSTANCES];
+	const struct hwmon_channel_info **ptr_channel_info;
+	struct hwmon_channel_info *channel_info;
+	struct device *dev = &state->wdev->dev;
+	struct hp_wmi_info *info = state->info;
+	struct hp_wmi_numeric_sensor *nsensor;
+	u8 channel_count[hwmon_max] = {};
+	enum hwmon_sensor_types type;
+	union acpi_object *wobj;
+	struct device *hwdev;
+	u8 type_count = 0;
+	u8 channel;
+	int wtype;
+	int err;
+	u8 i;
+
+	for (i = 0, channel = 0; i < HP_WMI_MAX_INSTANCES; i++, info++) {
+		wobj = hp_wmi_get_wobj(state, i);
+		if (!wobj)
+			break;
+
+		info->instance = i;
+		nsensor = &info->nsensor;
+
+		err = populate_numeric_sensor_from_wobj(dev, nsensor, wobj);
+		if (err)
+			goto out_free_wobj;
+
+		if (numeric_sensor_has_fault(nsensor))
+			goto out_free_wobj;
+
+		wtype = classify_numeric_sensor(nsensor);
+		if (wtype < 0)
+			goto out_free_wobj;
+
+		type = hp_wmi_hwmon_type_map[wtype];
+		if (!channel_count[type])
+			type_count++;
+		channel_count[type]++;
+
+		info->is_active = true;
+		info->type = type;
+
+		interpret_info(info);
+
+		active_info[channel++] = info;
+
+out_free_wobj:
+		kfree(wobj);
+
+		if (err)
+			return err;
+	}
+
+	dev_dbg(dev, "Found %u sensors (%u active, %u types)\n",
+		i, channel, type_count);
+
+	state->count = i;
+	if (!state->count)
+		return -ENODATA;
+
+	hp_wmi_debugfs_init(state);
+
+	if (!channel)
+		return 0; /* Not an error, but debugfs only. */
+
+	if (channel_count[hwmon_temp]) {
+		channel_count[hwmon_chip] = 1;
+		type_count++;
+	}
+
+	memcpy(state->channel_count, channel_count, sizeof(channel_count));
+
+	channel_info = devm_kcalloc(dev, type_count,
+				    sizeof(*channel_info),
+				    GFP_KERNEL);
+	if (!channel_info)
+		return -ENOMEM;
+
+	ptr_channel_info = devm_kcalloc(dev, type_count + 1,
+					sizeof(*ptr_channel_info),
+					GFP_KERNEL);
+	if (!ptr_channel_info)
+		return -ENOMEM;
+
+	hp_wmi_chip_info.info = ptr_channel_info;
+
+	for (type = hwmon_chip; type < hwmon_max; type++) {
+		if (!channel_count[type])
+			continue;
+
+		err = add_channel_info(dev, channel_info,
+				       channel_count[type], type);
+		if (err)
+			return err;
+
+		*ptr_channel_info++ = channel_info++;
+
+		state->info_map[type] = devm_kcalloc(dev, channel_count[type],
+						     sizeof(*state->info_map),
+						     GFP_KERNEL);
+		if (!state->info_map[type])
+			return -ENOMEM;
+	}
+
+	while (channel > 0) {
+		type = active_info[--channel]->type;
+		i = --channel_count[type];
+		state->info_map[type][i] = active_info[channel];
+	}
+
+	hwdev = devm_hwmon_device_register_with_info(dev, "hp_wmi_sensors",
+						     state, &hp_wmi_chip_info,
+						     NULL);
+	return PTR_ERR_OR_ZERO(hwdev);
+}
+
+static int hp_wmi_sensors_probe(struct wmi_device *wdev, const void *context)
+{
+	struct device *dev = &wdev->dev;
+	struct hp_wmi_sensors *state;
+	int err;
+
+	/* Sanity check. */
+	if (!wmi_has_guid(HP_WMI_NUMERIC_SENSOR_GUID) ||
+	    !wmi_has_guid(HP_WMI_BIOS_GUID)) {
+		err = -ENODEV;
+		goto out_err;
+	}
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	state->wdev = wdev;
+
+	mutex_init(&state->lock);
+
+	dev_set_drvdata(dev, state);
+
+	err = hp_wmi_sensors_init(state);
+
+out_err:
+	return err;
+}
+
+static const struct wmi_device_id hp_wmi_sensors_id_table[] = {
+	{ HP_WMI_NUMERIC_SENSOR_GUID, NULL },
+	{},
+};
+
+static struct wmi_driver hp_wmi_sensors_driver = {
+	.driver   = { .name = "hp-wmi-sensors" },
+	.id_table = hp_wmi_sensors_id_table,
+	.probe    = hp_wmi_sensors_probe,
+};
+module_wmi_driver(hp_wmi_sensors_driver);
+
+MODULE_AUTHOR("James Seo <james@equiv.tech>");
+MODULE_DESCRIPTION("HP WMI Sensors driver");
+MODULE_LICENSE("GPL");

base-commit: 76f598ba7d8e2bfb4855b5298caedd5af0c374a8
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] hwmon: add HP WMI Sensors driver
  2023-04-06 15:23 [PATCH v2] hwmon: add HP WMI Sensors driver James Seo
@ 2023-04-06 16:04 ` Armin Wolf
  2023-04-07  5:39   ` James Seo
  0 siblings, 1 reply; 5+ messages in thread
From: Armin Wolf @ 2023-04-06 16:04 UTC (permalink / raw)
  To: James Seo
  Cc: Jean Delvare, Guenter Roeck, linux-hwmon, platform-driver-x86,
	linux-kernel

Am 06.04.23 um 17:23 schrieb James Seo:

> Hewlett-Packard business-class computers report hardware monitoring
> information via WMI. This driver exposes that information to hwmon.
> Initial support is provided for temperature, voltage, current, and
> fan speed sensor types.
>
> HP's WMI implementation permits many other types of numeric sensors.
> Therefore, a debugfs interface is also provided to enumerate and
> inspect all numeric sensors visible on the WMI side. This should
> facilitate adding support for other sensor types in the future.
>
> Tested on a HP Z420 and a HP EliteOne 800 G1.
>
> Note that these models only have temperature and fan speed sensors,
> so other sensor types have not been tested working yet. However, the
> driver should still work as expected. A 2005 HP whitepaper describes
> the relevant MOF definition and sensor value calculation, and both
> this driver and the official HP Performance Advisor utility comply
> with them (confirmed in the latter case by reverse engineering).
>
> Link: https://h20331.www2.hp.com/hpsub/downloads/cmi_whitepaper.pdf
> Signed-off-by: James Seo <james@equiv.tech>
>
> ---
>
> Changes in v2:
> * Reword commit message
> * Reword documentation
> * Remove stray #include
> * Use DIV_ROUND_CLOSEST in F to C conversion
> * Consolidate uses of mutex_lock()/mutex_unlock()
> * Process sensors in forward order by type
>
> History:
> v1: https://lore.kernel.org/linux-hwmon/20230403084859.26286-1-james@equiv.tech/
> ---
>   Documentation/hwmon/hp-wmi-sensors.rst |   95 +++
>   Documentation/hwmon/index.rst          |    1 +
>   MAINTAINERS                            |    7 +
>   drivers/hwmon/Kconfig                  |   11 +
>   drivers/hwmon/Makefile                 |    1 +
>   drivers/hwmon/hp-wmi-sensors.c         | 1034 ++++++++++++++++++++++++
>   6 files changed, 1149 insertions(+)
>   create mode 100644 Documentation/hwmon/hp-wmi-sensors.rst
>   create mode 100644 drivers/hwmon/hp-wmi-sensors.c
>
> diff --git a/Documentation/hwmon/hp-wmi-sensors.rst b/Documentation/hwmon/hp-wmi-sensors.rst
> new file mode 100644
> index 000000000000..71b1391d5072
> --- /dev/null
> +++ b/Documentation/hwmon/hp-wmi-sensors.rst
> @@ -0,0 +1,95 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +.. include:: <isonum.txt>
> +
> +Linux HP WMI Sensors Driver
> +===========================
> +
> +:Copyright: |copy| 2023 James Seo <james@equiv.tech>
> +
> +Description
> +-----------
> +
> +Hewlett-Packard business-class computers report hardware monitoring information
> +via Windows Management Instrumentation (WMI). This driver exposes that
> +information to the Linux ``hwmon`` subsystem, allowing familiar userspace
> +utilities like ``sensors`` to gather numeric sensor readings.
> +
> +``sysfs`` interface
> +-------------------
> +
> +When the driver is loaded, it discovers the sensors available on the current
> +system and creates the following read-only ``sysfs`` attributes as appropriate
> +within ``/sys/class/hwmon/hwmonX``:
> +
> +================ ==================================================
> +Name		 Description
> +================ ==================================================
> +curr[X]_input    Current in milliamperes (mA).
> +curr[X]_label    Current sensor label.
> +fan[X]_input     Fan speed in RPM.
> +fan[X]_label     Fan sensor label.
> +fan[X]_fault     Fan sensor fault indicator.
> +in[X]_input      Voltage in millivolts (mV).
> +in[X]_label      Voltage sensor label.
> +temp[X]_input    Temperature in millidegrees Celsius (m\ |deg|\ C).
> +temp[X]_label    Temperature sensor label.
> +temp[X]_fault    Temperature sensor fault indicator.
> +================ ==================================================
> +
> +Here, ``X`` is some number that depends on other available sensors and on other
> +system hardware components.
> +
> +``fault`` attributes
> +  Reading ``1`` instead of ``0`` as the ``fault`` attribute for a sensor
> +  indicates that the sensor has encountered some issue during operation such
> +  that measurements from it should no longer be trusted.
> +
> +``debugfs`` interface
> +---------------------
> +
> +The standard ``hwmon`` interface in ``sysfs`` exposes sensors of several common
> +types that are connected and operating normally as of driver initialization.
> +However, there are usually other sensors on the WMI side that do not meet these
> +criteria. This driver therefore provides a ``debugfs`` interface in
> +``/sys/kernel/debug/hp-wmi-sensors-X`` that allows read-only access to *all* HP
> +WMI sensors on the current system.
> +
> +.. warning:: The ``debugfs`` interface is only available when the kernel is
> +             compiled with option ``CONFIG_DEBUG_FS``, and its implementation
> +             is subject to change without notice at any time.
> +
> +One numbered entry is created per sensor with the following attributes:
> +
> +=============================== ==========================================
> +Name				Example
> +=============================== ==========================================
> +name                            ``CPU0 Fan``
> +description                     ``Reports CPU0 fan speed``
> +sensor_type                     ``12``
> +other_sensor_type               ``(null)``
> +operational_status              ``2``
> +current_state                   ``Normal``
> +possible_states                 ``Normal\nCaution\nCritical\nNot Present``
> +base_units                      ``19``
> +unit_modifier                   ``0``
> +current_reading                 ``1008``
> +=============================== ==========================================
> +
> +These represent the properties of the underlying ``HP_BIOSNumericSensor`` WMI
> +object. Contents may vary somewhat between systems. See [#]_ for more details.
> +
> +Known issues and limitations
> +----------------------------
> +
> +- Non-numeric HP sensor types such as intrusion sensors that belong to the
> +  ``HP_BIOSStateSensor`` WMI object type are not supported.
> +- HP's WMI implementation permits sensors to claim to be of any type.
> +  However, only sensor types already known to hwmon can be supported.
> +
> +References
> +----------
> +
> +.. [#] Hewlett-Packard Development Company, L.P.,
> +       "HP Client Management Interface Technical White Paper", 2005. [Online].
> +       Available: https://h20331.www2.hp.com/hpsub/downloads/cmi_whitepaper.pdf
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index f1fe75f596a5..f8f3c0bef6ed 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -77,6 +77,7 @@ Hardware Monitoring Kernel Drivers
>      gl518sm
>      gxp-fan-ctrl
>      hih6130
> +   hp-wmi-sensors
>      ibmaem
>      ibm-cffps
>      ibmpowernv
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90abe83c02f3..264960e4c77e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9373,6 +9373,13 @@ L:	platform-driver-x86@vger.kernel.org
>   S:	Orphan
>   F:	drivers/platform/x86/hp/tc1100-wmi.c
>
> +HP WMI HARDWARE MONITOR DRIVER
> +M:	James Seo <james@equiv.tech>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/hp-wmi-sensors.rst
> +F:	drivers/hwmon/hp-wmi-sensors.c
> +
>   HPET:	High Precision Event Timers driver
>   M:	Clemens Ladisch <clemens@ladisch.de>
>   S:	Maintained
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 5b3b76477b0e..78798e0ed5e6 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2399,6 +2399,17 @@ config SENSORS_ASUS_EC
>   	  This driver can also be built as a module. If so, the module
>   	  will be called asus_ec_sensors.
>
> +config SENSORS_HP_WMI
> +	tristate "HP WMI Sensors"
> +	depends on ACPI_WMI
> +	help
> +	  If you say yes here you get support for the ACPI hardware monitoring
> +	  interface found in HP business-class computers. Temperature, voltage,
> +	  current, and fan speed sensor types are supported if present.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called hp_wmi_sensors.
> +
>   endif # ACPI
>
>   endif # HWMON
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 88712b5031c8..05cce16f37f6 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_ACPI_POWER) += acpi_power_meter.o
>   obj-$(CONFIG_SENSORS_ATK0110)	+= asus_atk0110.o
>   obj-$(CONFIG_SENSORS_ASUS_EC)	+= asus-ec-sensors.o
>   obj-$(CONFIG_SENSORS_ASUS_WMI)	+= asus_wmi_sensors.o
> +obj-$(CONFIG_SENSORS_HP_WMI)	+= hp-wmi-sensors.o
>
>   # Native drivers
>   # asb100, then w83781d go first, as they can override other drivers' addresses.
> diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
> new file mode 100644
> index 000000000000..8c1f09fcdfce
> --- /dev/null
> +++ b/drivers/hwmon/hp-wmi-sensors.c
> @@ -0,0 +1,1034 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * hwmon driver for HP business-class computers that report numeric
> + * sensor data via Windows Management Instrumentation (WMI).
> + *
> + * Copyright (C) 2023 James Seo <james@equiv.tech>
> + *
> + * References:
> + * [1] Hewlett-Packard Development Company, L.P.,
> + *     "HP Client Management Interface Technical White Paper", 2005. [Online].
> + *     Available: https://h20331.www2.hp.com/hpsub/downloads/cmi_whitepaper.pdf
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/debugfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/jiffies.h>
> +#include <linux/mutex.h>
> +#include <linux/units.h>
> +#include <linux/wmi.h>
> +
> +/*
> + * MOF definition of the HP_BIOSNumericSensor WMI object [1]:
> + *
> + *   #pragma namespace("\\\\.\\root\\HP\\InstrumentedBIOS");
> + *
> + *   [abstract]
> + *   class HP_BIOSSensor
> + *   {
> + *     [read] string Name;
> + *     [read] string Description;
> + *     [read, ValueMap {"0","1","2","3","4","5","6","7","8","9",
> + *      "10","11","12"}, Values {"Unknown","Other","Temperature",
> + *      "Voltage","Current","Tachometer","Counter","Switch","Lock",
> + *      "Humidity","Smoke Detection","Presence","Air Flow"}]
> + *     uint32 SensorType;
> + *     [read] string OtherSensorType;
> + *     [read, ValueMap {"0","1","2","3","4","5","6","7","8","9",
> + *      "10","11","12","13","14","15","16","17","18","..",
> + *      "0x8000.."}, Values {"Unknown","Other","OK","Degraded",
> + *      "Stressed","Predictive Failure","Error",
> + *      "Non-Recoverable Error","Starting","Stopping","Stopped",
> + *      "In Service","No Contact","Lost Communication","Aborted",
> + *      "Dormant","Supporting Entity in Error","Completed",
> + *      "Power Mode","DMTF Reserved","Vendor Reserved"}]
> + *     uint32 OperationalStatus;
> + *     [read] string CurrentState;
> + *     [read] string PossibleStates[];
> + *   };
> + *
> + *   class HP_BIOSNumericSensor : HP_BIOSSensor
> + *   {
> + *      [read, ValueMap {"0","1","2","3","4","5","6","7","8","9",
> + *       "10","11","12","13","14","15","16","17","18","19","20",
> + *       "21","22","23","24","25","26","27","28","29","30","31",
> + *       "32","33","34","35","36","37","38","39","40","41","42",
> + *       "43","44","45","46","47","48","49","50","51","52","53",
> + *       "54","55","56","57","58","59","60","61","62","63","64",
> + *       "65"}, Values {"Unknown","Other","Degrees C","Degrees F",
> + *       "Degrees K","Volts","Amps","Watts","Joules","Coulombs",
> + *       "VA","Nits","Lumens","Lux","Candelas","kPa","PSI",
> + *       "Newtons","CFM","RPM","Hertz","Seconds","Minutes",
> + *       "Hours","Days","Weeks","Mils","Inches","Feet",
> + *       "Cubic Inches","Cubic Feet","Meters","Cubic Centimeters",
> + *       "Cubic Meters","Liters","Fluid Ounces","Radians",
> + *       "Steradians","Revolutions","Cycles","Gravities","Ounces",
> + *       "Pounds","Foot-Pounds","Ounce-Inches","Gauss","Gilberts",
> + *       "Henries","Farads","Ohms","Siemens","Moles","Becquerels",
> + *       "PPM (parts/million)","Decibels","DbA","DbC","Grays",
> + *       "Sieverts","Color Temperature Degrees K","Bits","Bytes",
> + *       "Words (data)","DoubleWords","QuadWords","Percentage"}]
> + *     uint32 BaseUnits;
> + *     [read] sint32 UnitModifier;
> + *     [read] uint32 CurrentReading;
> + *   };
> + *
> + */
> +
> +#define HP_WMI_BIOS_GUID	   "5FB7F034-2C63-45E9-BE91-3D44E2C707E4"
> +#define HP_WMI_NUMERIC_SENSOR_GUID "8F1F6435-9F42-42C8-BADC-0E9424F20C9A"
> +
> +/* These limits are arbitrary. The WMI implementation may vary by model. */
> +
> +#define HP_WMI_MAX_STR_SIZE	   128U
> +#define HP_WMI_MAX_PROPERTIES	   32U
> +#define HP_WMI_MAX_INSTANCES	   32U
> +
> +enum hp_wmi_type {
> +	HP_WMI_TYPE_OTHER			   = 1,
> +	HP_WMI_TYPE_TEMPERATURE			   = 2,
> +	HP_WMI_TYPE_VOLTAGE			   = 3,
> +	HP_WMI_TYPE_CURRENT			   = 4,
> +	HP_WMI_TYPE_AIR_FLOW			   = 12,
> +};
> +
> +enum hp_wmi_status {
> +	HP_WMI_STATUS_OK			   = 2,
> +};
> +
> +enum hp_wmi_units {
> +	HP_WMI_UNITS_DEGREES_C			   = 2,
> +	HP_WMI_UNITS_DEGREES_F			   = 3,
> +	HP_WMI_UNITS_DEGREES_K			   = 4,
> +	HP_WMI_UNITS_VOLTS			   = 5,
> +	HP_WMI_UNITS_AMPS			   = 6,
> +	HP_WMI_UNITS_RPM			   = 19,
> +};
> +
> +enum hp_wmi_property {
> +	HP_WMI_PROPERTY_NAME			   = 0,
> +	HP_WMI_PROPERTY_DESCRIPTION		   = 1,
> +	HP_WMI_PROPERTY_SENSOR_TYPE		   = 2,
> +	HP_WMI_PROPERTY_OTHER_SENSOR_TYPE	   = 3,
> +	HP_WMI_PROPERTY_OPERATIONAL_STATUS	   = 4,
> +	HP_WMI_PROPERTY_CURRENT_STATE		   = 5,
> +	HP_WMI_PROPERTY_POSSIBLE_STATES		   = 6,
> +	HP_WMI_PROPERTY_BASE_UNITS		   = 7,
> +	HP_WMI_PROPERTY_UNIT_MODIFIER		   = 8,
> +	HP_WMI_PROPERTY_CURRENT_READING		   = 9,
> +};
> +
> +static const acpi_object_type hp_wmi_property_map[] = {
> +	[HP_WMI_PROPERTY_NAME]			   = ACPI_TYPE_STRING,
> +	[HP_WMI_PROPERTY_DESCRIPTION]		   = ACPI_TYPE_STRING,
> +	[HP_WMI_PROPERTY_SENSOR_TYPE]		   = ACPI_TYPE_INTEGER,
> +	[HP_WMI_PROPERTY_OTHER_SENSOR_TYPE]	   = ACPI_TYPE_STRING,
> +	[HP_WMI_PROPERTY_OPERATIONAL_STATUS]	   = ACPI_TYPE_INTEGER,
> +	[HP_WMI_PROPERTY_CURRENT_STATE]		   = ACPI_TYPE_STRING,
> +	[HP_WMI_PROPERTY_POSSIBLE_STATES]	   = ACPI_TYPE_STRING,
> +	[HP_WMI_PROPERTY_BASE_UNITS]		   = ACPI_TYPE_INTEGER,
> +	[HP_WMI_PROPERTY_UNIT_MODIFIER]		   = ACPI_TYPE_INTEGER,
> +	[HP_WMI_PROPERTY_CURRENT_READING]	   = ACPI_TYPE_INTEGER,
> +};
> +
> +static const enum hwmon_sensor_types hp_wmi_hwmon_type_map[] = {
> +	[HP_WMI_TYPE_TEMPERATURE]		   = hwmon_temp,
> +	[HP_WMI_TYPE_VOLTAGE]			   = hwmon_in,
> +	[HP_WMI_TYPE_CURRENT]			   = hwmon_curr,
> +	[HP_WMI_TYPE_AIR_FLOW]			   = hwmon_fan,
> +};
> +
> +static const u32 hp_wmi_hwmon_attributes[hwmon_max] = {
> +	[hwmon_chip] = HWMON_C_REGISTER_TZ,
> +	[hwmon_temp] = HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_FAULT,
> +	[hwmon_in]   = HWMON_I_INPUT | HWMON_I_LABEL,
> +	[hwmon_curr] = HWMON_C_INPUT | HWMON_C_LABEL,
> +	[hwmon_fan]  = HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_FAULT,
> +};
> +
> +/*
> + * struct hp_wmi_numeric_sensor - a HP_BIOSNumericSensor instance
> + *
> + * Contains WMI object instance properties. See MOF definition [1].
> + */
> +struct hp_wmi_numeric_sensor {
> +	const char *name;
> +	const char *description;
> +	u32 sensor_type;
> +	const char *other_sensor_type; /* Explains "Other" SensorType. */
> +	u32 operational_status;
> +	const char *current_state;
> +	const char **possible_states;  /* Count may vary. */
> +	u32 base_units;
> +	s32 unit_modifier;
> +	u32 current_reading;
> +
> +	u8 possible_states_count;
> +};
> +
> +/*
> + * struct hp_wmi_info - sensor info
> + * @nsensor: numeric sensor properties
> + * @instance: its WMI instance number
> + * @is_active: whether the following fields are valid
> + * @type: its hwmon sensor type
> + * @cached_val: current sensor reading value, scaled for hwmon
> + * @last_updated: when these readings were last updated
> + */
> +struct hp_wmi_info {
> +	struct hp_wmi_numeric_sensor nsensor;
> +	u8 instance;
> +
> +	bool is_active;
> +	enum hwmon_sensor_types type;
> +	long cached_val;
> +	unsigned long last_updated; /* in jiffies */
> +};
> +
> +/*
> + * struct hp_wmi_sensors - driver state
> + * @wdev: pointer to the parent WMI device
> + * @debugfs: root directory in debugfs
> + * @info: sensor info structs for all sensors visible in WMI
> + * @info_map: access info structs by hwmon type and channel number
> + * @count: count of all sensors visible in WMI
> + * @channel_count: count of hwmon channels by hwmon type
> + * @lock: mutex to lock polling WMI and changes to driver state
> + */
> +struct hp_wmi_sensors {
> +	struct wmi_device *wdev;
> +	struct dentry *debugfs;
> +	struct hp_wmi_info info[HP_WMI_MAX_INSTANCES];
> +	struct hp_wmi_info **info_map[hwmon_max];
> +	u8 count;
> +	u8 channel_count[hwmon_max];
> +
> +	struct mutex lock; /* lock polling WMI, driver state changes */
> +};
> +
> +/* hp_wmi_strdup - devm_kstrdup, but length-limited */
> +static char *hp_wmi_strdup(struct device *dev, const char *src, u32 len)
> +{
> +	char *dst;
> +
> +	len = min(len, HP_WMI_MAX_STR_SIZE - 1);
> +
> +	dst = devm_kmalloc(dev, (len + 1) * sizeof(*dst), GFP_KERNEL);
> +	if (!dst)
> +		return NULL;
> +
> +	strscpy(dst, src, len + 1);
> +
> +	return dst;
> +}
> +
> +/*
> + * hp_wmi_get_wobj - poll WMI for a HP_BIOSNumericSensor object instance
> + * @state: pointer to driver state
> + * @instance: WMI object instance number
> + *
> + * Returns a new WMI object instance on success, or NULL on error.
> + * Caller must kfree the result.
> + */
> +static union acpi_object *hp_wmi_get_wobj(struct hp_wmi_sensors *state,
> +					  u8 instance)
> +{
> +	struct wmi_device *wdev = state->wdev;
> +
> +	return wmidev_block_query(wdev, instance);
> +}
> +
> +/*
> + * check_wobj - validate a HP_BIOSNumericSensor WMI object instance
> + * @wobj: pointer to WMI object instance to check
> + * @possible_states_count: out pointer to count of possible states
> + *
> + * Returns 0 on success, or a negative error code on error.
> + */
> +static int check_wobj(const union acpi_object *wobj, u8 *possible_states_count)
> +{
> +	acpi_object_type type = wobj->type;
> +	int prop = HP_WMI_PROPERTY_NAME;
> +	acpi_object_type valid_type;
> +	union acpi_object *elements;
> +	u32 elem_count;
> +	u8 count = 0;
> +	u32 i;
> +
> +	if (type != ACPI_TYPE_PACKAGE)
> +		return -EINVAL;
> +
> +	elem_count = wobj->package.count;
> +	if (elem_count > HP_WMI_MAX_PROPERTIES)
> +		return -EINVAL;
> +
> +	elements = wobj->package.elements;
> +	for (i = 0; i < elem_count; i++, prop++) {
> +		if (prop > HP_WMI_PROPERTY_CURRENT_READING)
> +			return -EINVAL;
> +
> +		type = elements[i].type;
> +		valid_type = hp_wmi_property_map[prop];
> +		if (type != valid_type)
> +			return -EINVAL;
> +
> +		/*
> +		 * elements is a variable-length array of ACPI objects, one for
> +		 * each property of the WMI object instance, except that the
> +		 * strs in PossibleStates[] are flattened into this array, and
> +		 * their count is found in the WMI BMOF. We don't decode the
> +		 * BMOF, so find the count by finding the next int.
> +		 */
> +
> +		if (prop == HP_WMI_PROPERTY_CURRENT_STATE) {
> +			prop = HP_WMI_PROPERTY_POSSIBLE_STATES;
> +			valid_type = hp_wmi_property_map[prop];
> +			for (; i + 1 < elem_count; i++, count++) {
> +				type = elements[i + 1].type;
> +				if (type != valid_type)
> +					break;
> +			}
> +		}
> +	}
> +
> +	if (!count || prop <= HP_WMI_PROPERTY_CURRENT_READING)
> +		return -EINVAL;
> +
> +	*possible_states_count = count;
> +
> +	return 0;
> +}
> +
> +static int numeric_sensor_has_fault(const struct hp_wmi_numeric_sensor *nsensor)
> +{
> +	u32 operational_status = nsensor->operational_status;
> +	u32 current_reading = nsensor->current_reading;
> +
> +	return operational_status != HP_WMI_STATUS_OK || !current_reading;
> +}
> +
> +/* scale_numeric_sensor - scale sensor reading for hwmon */
> +static long scale_numeric_sensor(const struct hp_wmi_numeric_sensor *nsensor)
> +{
> +	u32 current_reading = nsensor->current_reading;
> +	s32 unit_modifier = nsensor->unit_modifier;
> +	u32 sensor_type = nsensor->sensor_type;
> +	u32 base_units = nsensor->base_units;
> +	s32 target_modifier;
> +	long val;
> +
> +	/* Fan readings are in RPM units; others are in milliunits. */
> +	target_modifier = sensor_type == HP_WMI_TYPE_AIR_FLOW ? 0 : -3;
> +
> +	val = current_reading;
> +
> +	for (; unit_modifier < target_modifier; unit_modifier++)
> +		val = DIV_ROUND_CLOSEST(val, 10);
> +
> +	for (; unit_modifier > target_modifier; unit_modifier--) {
> +		if (val > LONG_MAX / 10) {
> +			val = LONG_MAX;
> +			break;
> +		}
> +		val *= 10;
> +	}
> +
> +	if (sensor_type == HP_WMI_TYPE_TEMPERATURE) {
> +		switch (base_units) {
> +		case HP_WMI_UNITS_DEGREES_F:
> +			val -= MILLI * 32;
> +			val = val <= LONG_MAX / 5 ?
> +				      DIV_ROUND_CLOSEST(val * 5, 9) :
> +				      DIV_ROUND_CLOSEST(val, 9) * 5;
> +			break;
> +
> +		case HP_WMI_UNITS_DEGREES_K:
> +			val = milli_kelvin_to_millicelsius(val);
> +			break;
> +		}
> +	}
> +
> +	return val;
> +}
> +
> +/*
> + * classify_numeric_sensor - classify a numeric sensor
> + * @nsensor: pointer to numeric sensor struct
> + *
> + * Returns an enum hp_wmi_type value on success,
> + * or a negative value if the sensor type is unsupported.
> + */
> +static int classify_numeric_sensor(const struct hp_wmi_numeric_sensor *nsensor)
> +{
> +	u32 sensor_type = nsensor->sensor_type;
> +	u32 base_units = nsensor->base_units;
> +
> +	switch (sensor_type) {
> +	case HP_WMI_TYPE_TEMPERATURE:
> +		if (base_units == HP_WMI_UNITS_DEGREES_C ||
> +		    base_units == HP_WMI_UNITS_DEGREES_F ||
> +		    base_units == HP_WMI_UNITS_DEGREES_K)
> +			return HP_WMI_TYPE_TEMPERATURE;
> +		break;
> +
> +	case HP_WMI_TYPE_VOLTAGE:
> +		if (base_units == HP_WMI_UNITS_VOLTS)
> +			return HP_WMI_TYPE_VOLTAGE;
> +		break;
> +
> +	case HP_WMI_TYPE_CURRENT:
> +		if (base_units == HP_WMI_UNITS_AMPS)
> +			return HP_WMI_TYPE_CURRENT;
> +		break;
> +
> +	case HP_WMI_TYPE_AIR_FLOW:
> +		if (base_units == HP_WMI_UNITS_RPM)
> +			return HP_WMI_TYPE_AIR_FLOW;
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int
> +populate_numeric_sensor_from_wobj(struct device *dev,
> +				  struct hp_wmi_numeric_sensor *nsensor,
> +				  const union acpi_object *wobj)
> +{
> +	const union acpi_object *element;
> +	const char **possible_states;
> +	u8 possible_states_count;
> +	acpi_object_type type;
> +	const char *string;
> +	u32 value;
> +	int prop;
> +	int err;
> +
> +	err = check_wobj(wobj, &possible_states_count);
> +	if (err)
> +		return err;
> +
> +	possible_states = devm_kcalloc(dev, possible_states_count,
> +				       sizeof(*possible_states),
> +				       GFP_KERNEL);
> +	if (!possible_states)
> +		return -ENOMEM;
> +
> +	element = wobj->package.elements;
> +	nsensor->possible_states = possible_states;
> +	nsensor->possible_states_count = possible_states_count;
> +
> +	for (prop = 0; prop <= HP_WMI_PROPERTY_CURRENT_READING; prop++) {
> +		type = hp_wmi_property_map[prop];
> +
> +		switch (type) {
> +		case ACPI_TYPE_INTEGER:
> +			value = element->integer.value;
> +			break;
> +
> +		case ACPI_TYPE_STRING:
> +			string = hp_wmi_strdup(dev, element->string.pointer,
> +					       element->string.length);
> +			if (!string)
> +				return -ENOMEM;
> +			break;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		element++;
> +
> +		switch (prop) {
> +		case HP_WMI_PROPERTY_NAME:
> +			nsensor->name = string;
> +			break;
> +
> +		case HP_WMI_PROPERTY_DESCRIPTION:
> +			nsensor->description = string;
> +			break;
> +
> +		case HP_WMI_PROPERTY_SENSOR_TYPE:
> +			if (value > HP_WMI_TYPE_AIR_FLOW)
> +				return -EINVAL;
> +
> +			nsensor->sensor_type = value;
> +
> +			/* Skip OtherSensorType if it will be meaningless. */
> +			if (value != HP_WMI_TYPE_OTHER) {
> +				element++;
> +				prop++;
> +			}
> +
> +			break;
> +
> +		case HP_WMI_PROPERTY_OTHER_SENSOR_TYPE:
> +			nsensor->other_sensor_type = string;
> +			break;
> +
> +		case HP_WMI_PROPERTY_OPERATIONAL_STATUS:
> +			nsensor->operational_status = value;
> +			break;
> +
> +		case HP_WMI_PROPERTY_CURRENT_STATE:
> +			nsensor->current_state = string;
> +			break;
> +
> +		case HP_WMI_PROPERTY_POSSIBLE_STATES:
> +			*possible_states++ = string;
> +			if (--possible_states_count)
> +				prop--;
> +			break;
> +
> +		case HP_WMI_PROPERTY_BASE_UNITS:
> +			nsensor->base_units = value;
> +			break;
> +
> +		case HP_WMI_PROPERTY_UNIT_MODIFIER:
> +			/* UnitModifier is signed. */
> +			nsensor->unit_modifier = (s32)value;
> +			break;
> +
> +		case HP_WMI_PROPERTY_CURRENT_READING:
> +			nsensor->current_reading = value;
> +			break;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/* update_numeric_sensor_from_wobj - update fungible sensor properties */
> +static void
> +update_numeric_sensor_from_wobj(struct device *dev,
> +				struct hp_wmi_numeric_sensor *nsensor,
> +				const union acpi_object *wobj)
> +{
> +	const union acpi_object *elements;
> +	const union acpi_object *element;
> +	const char *string;
> +	u32 length;
> +	u8 offset;
> +
> +	elements = wobj->package.elements;
> +
> +	element = &elements[HP_WMI_PROPERTY_OPERATIONAL_STATUS];
> +	nsensor->operational_status = element->integer.value;
> +
> +	element = &elements[HP_WMI_PROPERTY_CURRENT_STATE];
> +	string = element->string.pointer;
> +
> +	if (strcmp(string, nsensor->current_state)) {
> +		length = element->string.length;
> +		devm_kfree(dev, nsensor->current_state);
> +		nsensor->current_state = hp_wmi_strdup(dev, string, length);
> +	}
> +
> +	/* Offset reads into the elements array after PossibleStates[0]. */
> +	offset = nsensor->possible_states_count - 1;
> +
> +	element = &elements[HP_WMI_PROPERTY_UNIT_MODIFIER + offset];
> +	nsensor->unit_modifier = (s32)element->integer.value;
> +
> +	element = &elements[HP_WMI_PROPERTY_CURRENT_READING + offset];
> +	nsensor->current_reading = element->integer.value;
> +}
> +
> +/*
> + * interpret_info - interpret sensor for hwmon
> + * @info: pointer to sensor info struct
> + *
> + * Should be called after the numeric sensor member has been updated.
> + */
> +static void interpret_info(struct hp_wmi_info *info)
> +{
> +	const struct hp_wmi_numeric_sensor *nsensor = &info->nsensor;
> +
> +	info->cached_val = scale_numeric_sensor(nsensor);
> +	info->last_updated = jiffies;
> +}
> +
> +/*
> + * hp_wmi_update_info - poll WMI to update sensor info
> + * @state: pointer to driver state
> + * @info: pointer to sensor info struct
> + *
> + * Returns 0 on success, or a negative error code on error.
> + */
> +static int hp_wmi_update_info(struct hp_wmi_sensors *state,
> +			      struct hp_wmi_info *info)
> +{
> +	struct hp_wmi_numeric_sensor *nsensor = &info->nsensor;
> +	struct device *dev = &state->wdev->dev;
> +	const union acpi_object *wobj;
> +	u8 instance = info->instance;
> +	int ret = 0;
> +
> +	if (time_after(jiffies, info->last_updated + HZ)) {
> +		mutex_lock(&state->lock);
> +
> +		wobj = hp_wmi_get_wobj(state, instance);
> +		if (!wobj) {
> +			ret = -EIO;
> +			goto out_free_wobj;
> +		}
> +
> +		update_numeric_sensor_from_wobj(dev, nsensor, wobj);
> +
> +		interpret_info(info);
> +
> +out_free_wobj:
> +		kfree(wobj);
> +
> +		mutex_unlock(&state->lock);
> +	}
> +
> +	return ret;
> +}
> +
> +#if CONFIG_DEBUG_FS
> +
> +static int basic_string_show(struct seq_file *seqf, void *ignored)
> +{
> +	const char *str = seqf->private;
> +
> +	seq_printf(seqf, "%s\n", str);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(basic_string);
> +
> +static int fungible_show(struct seq_file *seqf, enum hp_wmi_property prop)
> +{
> +	struct hp_wmi_numeric_sensor *nsensor;
> +	struct hp_wmi_sensors *state;
> +	struct hp_wmi_info *info;
> +	int err;
> +
> +	switch (prop) {
> +	case HP_WMI_PROPERTY_OPERATIONAL_STATUS:
> +		nsensor = container_of(seqf->private,
> +				       struct hp_wmi_numeric_sensor,
> +				       operational_status);
> +		break;
> +
> +	case HP_WMI_PROPERTY_CURRENT_STATE:
> +		nsensor = container_of(seqf->private,
> +				       struct hp_wmi_numeric_sensor,
> +				       current_state);
> +		break;
> +
> +	case HP_WMI_PROPERTY_UNIT_MODIFIER:
> +		nsensor = container_of(seqf->private,
> +				       struct hp_wmi_numeric_sensor,
> +				       unit_modifier);
> +		break;
> +
> +	case HP_WMI_PROPERTY_CURRENT_READING:
> +		nsensor = container_of(seqf->private,
> +				       struct hp_wmi_numeric_sensor,
> +				       current_reading);
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	info = container_of(nsensor, struct hp_wmi_info, nsensor);
> +	state = container_of(info, struct hp_wmi_sensors, info[info->instance]);
> +
> +	err = hp_wmi_update_info(state, info);
> +	if (err)
> +		return err;
> +
> +	switch (prop) {
> +	case HP_WMI_PROPERTY_OPERATIONAL_STATUS:
> +		seq_printf(seqf, "%u\n", nsensor->operational_status);
> +		break;
> +
> +	case HP_WMI_PROPERTY_CURRENT_STATE:
> +		seq_printf(seqf, "%s\n", nsensor->current_state);
> +		break;
> +
> +	case HP_WMI_PROPERTY_UNIT_MODIFIER:
> +		seq_printf(seqf, "%d\n", nsensor->unit_modifier);
> +		break;
> +
> +	case HP_WMI_PROPERTY_CURRENT_READING:
> +		seq_printf(seqf, "%u\n", nsensor->current_reading);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int operational_status_show(struct seq_file *seqf, void *ignored)
> +{
> +	return fungible_show(seqf, HP_WMI_PROPERTY_OPERATIONAL_STATUS);
> +}
> +DEFINE_SHOW_ATTRIBUTE(operational_status);
> +
> +static int current_state_show(struct seq_file *seqf, void *ignored)
> +{
> +	return fungible_show(seqf, HP_WMI_PROPERTY_CURRENT_STATE);
> +}
> +DEFINE_SHOW_ATTRIBUTE(current_state);
> +
> +static int possible_states_show(struct seq_file *seqf, void *ignored)
> +{
> +	struct hp_wmi_numeric_sensor *nsensor = seqf->private;
> +	u8 i;
> +
> +	for (i = 0; i < nsensor->possible_states_count; i++)
> +		seq_printf(seqf, "%s\n", nsensor->possible_states[i]);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(possible_states);
> +
> +static int unit_modifier_show(struct seq_file *seqf, void *ignored)
> +{
> +	return fungible_show(seqf, HP_WMI_PROPERTY_UNIT_MODIFIER);
> +}
> +DEFINE_SHOW_ATTRIBUTE(unit_modifier);
> +
> +static int current_reading_show(struct seq_file *seqf, void *ignored)
> +{
> +	return fungible_show(seqf, HP_WMI_PROPERTY_CURRENT_READING);
> +}
> +DEFINE_SHOW_ATTRIBUTE(current_reading);
> +
> +/* hp_wmi_devm_debugfs_remove - devm callback for debugfs cleanup */
> +static void hp_wmi_devm_debugfs_remove(void *res)
> +{
> +	debugfs_remove_recursive(res);
> +}
> +
> +/* hp_wmi_debugfs_init - create and populate debugfs directory tree */
> +static void hp_wmi_debugfs_init(struct hp_wmi_sensors *state)
> +{
> +	struct device *dev = &state->wdev->dev;
> +	struct hp_wmi_info *info = state->info;
> +	struct hp_wmi_numeric_sensor *nsensor;
> +	char buf[HP_WMI_MAX_STR_SIZE];
> +	struct dentry *dir;
> +	int err;
> +	u8 i;
> +
> +	/* dev_name() gives a not-very-friendly GUID for WMI devices. */
> +	scnprintf(buf, sizeof(buf), "%s-%u", "hp-wmi-sensors", dev->id);
> +
> +	state->debugfs = debugfs_create_dir(buf, NULL);
> +	if (IS_ERR(state->debugfs))
> +		return;
> +
> +	err = devm_add_action(dev, hp_wmi_devm_debugfs_remove, state->debugfs);
> +	if (err) {
> +		debugfs_remove(state->debugfs);
> +		return;
> +	}
> +
> +	for (i = 0; i < state->count; i++, info++) {
> +		nsensor = &info->nsensor;
> +
> +		scnprintf(buf, sizeof(buf), "%u", i);
> +		dir = debugfs_create_dir(buf, state->debugfs);
> +
> +		debugfs_create_file("name", 0444, dir,
> +				    (void *)nsensor->name,
> +				    &basic_string_fops);
> +
> +		debugfs_create_file("description", 0444, dir,
> +				    (void *)nsensor->description,
> +				    &basic_string_fops);
> +
> +		debugfs_create_u32("sensor_type", 0444, dir,
> +				   &nsensor->sensor_type);
> +
> +		debugfs_create_file("other_sensor_type", 0444, dir,
> +				    (void *)nsensor->other_sensor_type,
> +				    &basic_string_fops);
> +
> +		debugfs_create_file("operational_status", 0444, dir,
> +				    &nsensor->operational_status,
> +				    &operational_status_fops);
> +
> +		debugfs_create_file("current_state", 0444, dir,
> +				    (void *)&nsensor->current_state,
> +				    &current_state_fops);
> +
> +		debugfs_create_file("possible_states", 0444, dir,
> +				    nsensor, &possible_states_fops);
> +
> +		debugfs_create_u32("base_units", 0444, dir,
> +				   &nsensor->base_units);
> +
> +		debugfs_create_file("unit_modifier", 0444, dir,
> +				    &nsensor->unit_modifier,
> +				    &unit_modifier_fops);
> +
> +		debugfs_create_file("current_reading", 0444, dir,
> +				    &nsensor->current_reading,
> +				    &current_reading_fops);
> +	}
> +}
> +
> +#else
> +
> +static void hp_wmi_debugfs_init(struct hp_wmi_sensors *state)
> +{
> +}
> +
> +#endif
> +
> +static umode_t hp_wmi_hwmon_is_visible(const void *drvdata,
> +				       enum hwmon_sensor_types type,
> +				       u32 attr, int channel)
> +{
> +	const struct hp_wmi_sensors *state = drvdata;
> +
> +	if (!state->info_map[type] || !state->info_map[type][channel])
> +		return 0;
> +
> +	return 0444;
> +}
> +
> +static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			     u32 attr, int channel, long *val)
> +{
> +	struct hp_wmi_sensors *state = dev_get_drvdata(dev);
> +	const struct hp_wmi_numeric_sensor *nsensor;
> +	struct hp_wmi_info *info;
> +	int err;
> +
> +	info = state->info_map[type][channel];
> +	nsensor = &info->nsensor;
> +
> +	err = hp_wmi_update_info(state, info);
> +	if (err)
> +		return err;
> +
> +	else if ((type == hwmon_temp && attr == hwmon_temp_fault) ||
> +		 (type == hwmon_fan  && attr == hwmon_fan_fault))
> +		*val = numeric_sensor_has_fault(nsensor);
> +
> +	else
> +		*val = info->cached_val;
> +
> +	return 0;
> +}
> +
> +static int hp_wmi_hwmon_read_string(struct device *dev,
> +				    enum hwmon_sensor_types type, u32 attr,
> +				    int channel, const char **str)
> +{
> +	const struct hp_wmi_sensors *state = dev_get_drvdata(dev);
> +	const struct hp_wmi_info *info;
> +
> +	info = state->info_map[type][channel];
> +	*str = info->nsensor.name;
> +
> +	return 0;
> +}
> +
> +static int add_channel_info(struct device *dev,
> +			    struct hwmon_channel_info *channel_info,
> +			    u8 count, enum hwmon_sensor_types type)
> +{
> +	u32 attr = hp_wmi_hwmon_attributes[type];
> +	u32 *config;
> +
> +	config = devm_kcalloc(dev, count + 1, sizeof(*config), GFP_KERNEL);
> +	if (!config)
> +		return -ENOMEM;
> +
> +	channel_info->type = type;
> +	channel_info->config = config;
> +	memset32(config, attr, count);
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_ops hp_wmi_hwmon_ops = {
> +	.is_visible  = hp_wmi_hwmon_is_visible,
> +	.read	     = hp_wmi_hwmon_read,
> +	.read_string = hp_wmi_hwmon_read_string,
> +};
> +
> +static struct hwmon_chip_info hp_wmi_chip_info = {
> +	.ops         = &hp_wmi_hwmon_ops,
> +	.info        = NULL,
> +};
> +
> +static int hp_wmi_sensors_init(struct hp_wmi_sensors *state)
> +{
> +	struct hp_wmi_info *active_info[HP_WMI_MAX_INSTANCES];
> +	const struct hwmon_channel_info **ptr_channel_info;
> +	struct hwmon_channel_info *channel_info;
> +	struct device *dev = &state->wdev->dev;
> +	struct hp_wmi_info *info = state->info;
> +	struct hp_wmi_numeric_sensor *nsensor;
> +	u8 channel_count[hwmon_max] = {};
> +	enum hwmon_sensor_types type;
> +	union acpi_object *wobj;
> +	struct device *hwdev;
> +	u8 type_count = 0;
> +	u8 channel;
> +	int wtype;
> +	int err;
> +	u8 i;
> +
> +	for (i = 0, channel = 0; i < HP_WMI_MAX_INSTANCES; i++, info++) {
> +		wobj = hp_wmi_get_wobj(state, i);
> +		if (!wobj)
> +			break;
> +
> +		info->instance = i;
> +		nsensor = &info->nsensor;
> +
> +		err = populate_numeric_sensor_from_wobj(dev, nsensor, wobj);
> +		if (err)
> +			goto out_free_wobj;
> +
> +		if (numeric_sensor_has_fault(nsensor))
> +			goto out_free_wobj;
> +

Hi,

is it guaranteed that faulty sensors wont become operational later?
Also filtering out such sensors would make the support for the hwmon_temp_fault and
hwmon_fan_fault attributes meaningless.

> +		wtype = classify_numeric_sensor(nsensor);
> +		if (wtype < 0)
> +			goto out_free_wobj;
> +
> +		type = hp_wmi_hwmon_type_map[wtype];
> +		if (!channel_count[type])
> +			type_count++;
> +		channel_count[type]++;
> +
> +		info->is_active = true;
> +		info->type = type;
> +
> +		interpret_info(info);
> +
> +		active_info[channel++] = info;
> +
> +out_free_wobj:
> +		kfree(wobj);
> +
> +		if (err)
> +			return err;
> +	}
> +
> +	dev_dbg(dev, "Found %u sensors (%u active, %u types)\n",
> +		i, channel, type_count);
> +
> +	state->count = i;
> +	if (!state->count)
> +		return -ENODATA;
> +
> +	hp_wmi_debugfs_init(state);
> +
> +	if (!channel)
> +		return 0; /* Not an error, but debugfs only. */
> +
> +	if (channel_count[hwmon_temp]) {
> +		channel_count[hwmon_chip] = 1;
> +		type_count++;
> +	}
> +
> +	memcpy(state->channel_count, channel_count, sizeof(channel_count));
> +
> +	channel_info = devm_kcalloc(dev, type_count,
> +				    sizeof(*channel_info),
> +				    GFP_KERNEL);
> +	if (!channel_info)
> +		return -ENOMEM;
> +
> +	ptr_channel_info = devm_kcalloc(dev, type_count + 1,
> +					sizeof(*ptr_channel_info),
> +					GFP_KERNEL);
> +	if (!ptr_channel_info)
> +		return -ENOMEM;
> +
> +	hp_wmi_chip_info.info = ptr_channel_info;
> +
> +	for (type = hwmon_chip; type < hwmon_max; type++) {
> +		if (!channel_count[type])
> +			continue;
> +
> +		err = add_channel_info(dev, channel_info,
> +				       channel_count[type], type);
> +		if (err)
> +			return err;
> +
> +		*ptr_channel_info++ = channel_info++;
> +
> +		state->info_map[type] = devm_kcalloc(dev, channel_count[type],
> +						     sizeof(*state->info_map),
> +						     GFP_KERNEL);
> +		if (!state->info_map[type])
> +			return -ENOMEM;
> +	}
> +
> +	while (channel > 0) {
> +		type = active_info[--channel]->type;
> +		i = --channel_count[type];
> +		state->info_map[type][i] = active_info[channel];
> +	}
> +
> +	hwdev = devm_hwmon_device_register_with_info(dev, "hp_wmi_sensors",
> +						     state, &hp_wmi_chip_info,
> +						     NULL);
> +	return PTR_ERR_OR_ZERO(hwdev);
> +}
> +
> +static int hp_wmi_sensors_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct device *dev = &wdev->dev;
> +	struct hp_wmi_sensors *state;
> +	int err;
> +
> +	/* Sanity check. */
> +	if (!wmi_has_guid(HP_WMI_NUMERIC_SENSOR_GUID) ||
> +	    !wmi_has_guid(HP_WMI_BIOS_GUID)) {
> +		err = -ENODEV;
> +		goto out_err;
> +	}
> +

The sanity check for HP_WMI_NUMERIC_SENSOR_GUID is unnecessary, the WMI driver core already makes sure that your driver
is only matched with WMI devices containing HP_WMI_NUMERIC_SENSOR_GUID.
As for the sanity check regarding HP_WMI_BIOS_GUID: this WMI GUID is not used inside the driver. Since WMI GUIDs are expected
to be unique, checking for HP_WMI_BIOS_GUID (which AFAIK is used by the HP-BIOSCFG driver) without intending to use it is
meaningless.

Armin Wolf

> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> +	if (!state) {
> +		err = -ENOMEM;
> +		goto out_err;
> +	}
> +
> +	state->wdev = wdev;
> +
> +	mutex_init(&state->lock);
> +
> +	dev_set_drvdata(dev, state);
> +
> +	err = hp_wmi_sensors_init(state);
> +
> +out_err:
> +	return err;
> +}
> +
> +static const struct wmi_device_id hp_wmi_sensors_id_table[] = {
> +	{ HP_WMI_NUMERIC_SENSOR_GUID, NULL },
> +	{},
> +};
> +
> +static struct wmi_driver hp_wmi_sensors_driver = {
> +	.driver   = { .name = "hp-wmi-sensors" },
> +	.id_table = hp_wmi_sensors_id_table,
> +	.probe    = hp_wmi_sensors_probe,
> +};
> +module_wmi_driver(hp_wmi_sensors_driver);
> +
> +MODULE_AUTHOR("James Seo <james@equiv.tech>");
> +MODULE_DESCRIPTION("HP WMI Sensors driver");
> +MODULE_LICENSE("GPL");
>
> base-commit: 76f598ba7d8e2bfb4855b5298caedd5af0c374a8

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] hwmon: add HP WMI Sensors driver
  2023-04-06 16:04 ` Armin Wolf
@ 2023-04-07  5:39   ` James Seo
  2023-04-07 12:54     ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: James Seo @ 2023-04-07  5:39 UTC (permalink / raw)
  To: Armin Wolf
  Cc: Jean Delvare, Guenter Roeck, linux-hwmon, platform-driver-x86,
	linux-kernel

Hi,

> is it guaranteed that faulty sensors wont become operational later?
> Also filtering out such sensors would make the support for the hwmon_temp_fault and
> hwmon_fan_fault attributes meaningless.

Good point. I can't be certain, but the MOF does seem to imply that
sensors can indeed be faulty on just a temporary basis.

I'll filter out only the sensors that are "Not Connected" at probe
time. My thinking is, even if these might turn into connected sensors
later, that would mean the user is e.g. hot-plugging a fan (!), and
keeping them could result in a large number (~10 on my Z420) of
pointless extra channels. And this would also match the behavior of
HP's official utility.

Does that seem reasonable? Or did you mean that I shouldn't filter,
and leave disconnected sensors in like some other hwmon drivers do?

> The sanity check for HP_WMI_NUMERIC_SENSOR_GUID is unnecessary, the WMI driver core already makes sure that your driver
> is only matched with WMI devices containing HP_WMI_NUMERIC_SENSOR_GUID.
> As for the sanity check regarding HP_WMI_BIOS_GUID: this WMI GUID is not used inside the driver. Since WMI GUIDs are expected
> to be unique, checking for HP_WMI_BIOS_GUID (which AFAIK is used by the HP-BIOSCFG driver) without intending to use it is
> meaningless.

In that case, I'll gladly remove the checks. I was following the
example of the platform/x86/hp-wmi driver, which checks for that GUID
and another at module load.

Thanks for reviewing.

James

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] hwmon: add HP WMI Sensors driver
  2023-04-07  5:39   ` James Seo
@ 2023-04-07 12:54     ` Guenter Roeck
  2023-04-08  3:58       ` James Seo
  0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2023-04-07 12:54 UTC (permalink / raw)
  To: James Seo, Armin Wolf
  Cc: Jean Delvare, linux-hwmon, platform-driver-x86, linux-kernel

On 4/6/23 22:39, James Seo wrote:
> Hi,
> 
>> is it guaranteed that faulty sensors wont become operational later?
>> Also filtering out such sensors would make the support for the hwmon_temp_fault and
>> hwmon_fan_fault attributes meaningless.
> 
> Good point. I can't be certain, but the MOF does seem to imply that
> sensors can indeed be faulty on just a temporary basis.
> 

Your current code would explicitly exclude faulty fans from being listed,
which does not exactly sound like a good idea.

> I'll filter out only the sensors that are "Not Connected" at probe
> time. My thinking is, even if these might turn into connected sensors
> later, that would mean the user is e.g. hot-plugging a fan (!), and
> keeping them could result in a large number (~10 on my Z420) of
> pointless extra channels. And this would also match the behavior of
> HP's official utility.
> 
Ultimately that is an implementation decision. Are the sensors hot-pluggable ?
If so, how does HP's utility handle the insertion or removal of a sensor (fan) ?

Either case, it is ok with me if disconnected sensors are not listed.
Not listing faulty sensors seems like a bad idea, though.

Guenter

> Does that seem reasonable? Or did you mean that I shouldn't filter,
> and leave disconnected sensors in like some other hwmon drivers do?
> 
>> The sanity check for HP_WMI_NUMERIC_SENSOR_GUID is unnecessary, the WMI driver core already makes sure that your driver
>> is only matched with WMI devices containing HP_WMI_NUMERIC_SENSOR_GUID.
>> As for the sanity check regarding HP_WMI_BIOS_GUID: this WMI GUID is not used inside the driver. Since WMI GUIDs are expected
>> to be unique, checking for HP_WMI_BIOS_GUID (which AFAIK is used by the HP-BIOSCFG driver) without intending to use it is
>> meaningless.
> 
> In that case, I'll gladly remove the checks. I was following the
> example of the platform/x86/hp-wmi driver, which checks for that GUID
> and another at module load.
> 
> Thanks for reviewing.
> 
> James


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] hwmon: add HP WMI Sensors driver
  2023-04-07 12:54     ` Guenter Roeck
@ 2023-04-08  3:58       ` James Seo
  0 siblings, 0 replies; 5+ messages in thread
From: James Seo @ 2023-04-08  3:58 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jean Delvare, linux-hwmon, platform-driver-x86, linux-kernel

Greetings,

On Fri, Apr 07, 2023 at 05:54:48AM -0700, Guenter Roeck wrote:
> On 4/6/23 22:39, James Seo wrote:
>> Hi,
>> 
>>> is it guaranteed that faulty sensors wont become operational later?
>>> Also filtering out such sensors would make the support for the hwmon_temp_fault and
>>> hwmon_fan_fault attributes meaningless.
>> 
>> Good point. I can't be certain, but the MOF does seem to imply that
>> sensors can indeed be faulty on just a temporary basis.
>> 
> 
> Your current code would explicitly exclude faulty fans from being listed,
> which does not exactly sound like a good idea.

True enough. I recall my reasoning being that faulty sensors would
still be visible in debugfs. I should have seen the problem then.

>> I'll filter out only the sensors that are "Not Connected" at probe
>> time. My thinking is, even if these might turn into connected sensors
>> later, that would mean the user is e.g. hot-plugging a fan (!), and
>> keeping them could result in a large number (~10 on my Z420) of
>> pointless extra channels. And this would also match the behavior of
>> HP's official utility.
>> 
> Ultimately that is an implementation decision. Are the sensors hot-pluggable ?

HP's WMI object specification allows sensors to be hot-pluggable in
principle. I can't definitively say more than that due to a lack of
test hardware (that whitepaper I referenced is from 2005, after all).

So I think the answer is that it depends on the board and the WMI
implementation. That's also what I meant in my reply to Armin when I
said that I couldn't be certain whether faulty sensors can recover.

But I take your point that the driver should be able to handle it if
the board can.

> If so, how does HP's utility handle the insertion or removal of a sensor (fan) ?

HP's utility just pretty-prints a snapshot of what is in WMI at the
moment when the user clicks a button, and then only for the sensors
that were connected when the utility was first started. It doesn't do
anything special to handle insertion or removal beyond that.

> Either case, it is ok with me if disconnected sensors are not listed.
> Not listing faulty sensors seems like a bad idea, though.
> 
> Guenter

Acknowledged. Faulty sensors will be listed in the next version.

Thanks for reviewing. Further suggestions or concerns from you or
anyone else reading this are both welcome and appreciated.

James

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-04-08  3:58 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-06 15:23 [PATCH v2] hwmon: add HP WMI Sensors driver James Seo
2023-04-06 16:04 ` Armin Wolf
2023-04-07  5:39   ` James Seo
2023-04-07 12:54     ` Guenter Roeck
2023-04-08  3:58       ` James Seo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).