[RFC PATCH 0/1] platform: x86: tuxi: Implement TUXEDO TUXI ACPI TFAN as thermal subsystem

[RFC PATCH 0/1] platform: x86: tuxi: Implement TUXEDO TUXI ACPI TFAN as thermal subsystem

[Werner Sembach]

Hi,

Following up to https://lore.kernel.org/all/172b7acd-4313-4924-bcbc-41b73b39ada0@tuxedocomputers.com/ and https://lore.kernel.org/all/f26d867e-f247-43bb-a78b-be0bce35c973@roeck-us.net/ I experimented with the thermal subsystem and these are my results so far, but I'm hitting a bit of a wall:

As far as I can tell to implement "2. As long as GTMP is > 80°C fan speed must be at least 30%." I would need to add a new gevenor, lets call it "user_space_with_safeguards". I would be nice when the temp <-> fanspeed relation could be passed via the thermal_trip structure. And safeguarding the hardware from userspace only works when I can restrict userspace from just selecting the preexisting "user_space" govenor.

So my ideas/questions:
- Add a field "min_fanspeed_percent" to the thermal_trip struct that will only be used by the "user_space_with_safeguards" govenor
- Add a "user_space_with_safeguards" govenor that is the same as the "user_space" govenor, but on trip, a minimum speed is applied
- How can i ensure that on further speed updates the min speed is applied to? I could just implement it directly in the cdev, but that would be spagetti coding around the govenor.
- Can I somehow restrict userspace from using certain govenors?
- I'm a litte bit confused about the thermal zone "mode" sysfs switch, here it says deactivate for userspace control: https://elixir.bootlin.com/linux/v6.12/source/Documentation/ABI/testing/sysfs-class-thermal#L20, but what about the user_space govenor then?

Best regards,
Werner

Werner Sembach (1):
  platform: x86: tuxi: Implement TUXEDO TUXI ACPI TFAN as thermal
    subsystem

 drivers/platform/x86/Makefile                 |   3 +
 drivers/platform/x86/tuxedo/Kbuild            |   6 +
 drivers/platform/x86/tuxedo/nbxx/Kbuild       |   8 +
 drivers/platform/x86/tuxedo/nbxx/Kconfig      |   9 +
 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c  |  96 +++++++
 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h  |  84 ++++++
 .../x86/tuxedo/nbxx/acpi_tuxi_thermal.c       | 241 ++++++++++++++++++
 .../x86/tuxedo/nbxx/acpi_tuxi_thermal.h       |  14 +
 8 files changed, 461 insertions(+)
 create mode 100644 drivers/platform/x86/tuxedo/Kbuild
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h

-- 
2.43.0

[RFC PATCH 1/1] platform: x86: tuxi: Implement TUXEDO TUXI ACPI TFAN as thermal subsystem

[Werner Sembach]

The TUXEDO Sirius 16 Gen1 & Gen2 have the custom TUXEDO Interface (TUXI)
ACPI interface which currently consists of the TFAN device. This has ACPI
functions to control the built in fans and monitor fan speeds and CPU and
GPU temprature.

This driver implements this TFAN device via the thermal subsystem to allow
userspace controlled, but hardware safe, custom fan curves.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/platform/x86/Makefile                 |   3 +
 drivers/platform/x86/tuxedo/Kbuild            |   6 +
 drivers/platform/x86/tuxedo/nbxx/Kbuild       |   8 +
 drivers/platform/x86/tuxedo/nbxx/Kconfig      |   9 +
 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c  |  96 +++++++
 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h  |  84 ++++++
 .../x86/tuxedo/nbxx/acpi_tuxi_thermal.c       | 241 ++++++++++++++++++
 .../x86/tuxedo/nbxx/acpi_tuxi_thermal.h       |  14 +
 8 files changed, 461 insertions(+)
 create mode 100644 drivers/platform/x86/tuxedo/Kbuild
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h

diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e1b1429470674..1562dcd7ad9a5 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
 
 # SEL
 obj-$(CONFIG_SEL3350_PLATFORM)		+= sel3350-platform.o
+
+# TUXEDO
+obj-y					+= tuxedo/
diff --git a/drivers/platform/x86/tuxedo/Kbuild b/drivers/platform/x86/tuxedo/Kbuild
new file mode 100644
index 0000000000000..16396a8923432
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/Kbuild
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# TUXEDO X86 Platform Specific Drivers
+#
+
+obj-y	+= nbxx/
diff --git a/drivers/platform/x86/tuxedo/nbxx/Kbuild b/drivers/platform/x86/tuxedo/nbxx/Kbuild
new file mode 100644
index 0000000000000..aa4d6d66b785f
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/Kbuild
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# TUXEDO X86 Platform Specific Drivers
+#
+
+tuxedo_nbxx_acpi_tuxi-y			:= acpi_tuxi.o
+tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_thermal.o
+obj-$(CONFIG_TUXEDO_NBXX_ACPI_TUXI)	+= tuxedo_nbxx_acpi_tuxi.o
diff --git a/drivers/platform/x86/tuxedo/nbxx/Kconfig b/drivers/platform/x86/tuxedo/nbxx/Kconfig
new file mode 100644
index 0000000000000..93f7f86e803da
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/Kconfig
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# TUXEDO X86 Platform Specific Drivers
+#
+
+config TUXEDO_NBXX_ACPI_TUXI
+	tristate "TUXEDO TUXI"
+	help
+	  TUXEDO TUXI driver
diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
new file mode 100644
index 0000000000000..6031cf731b449
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Werner Sembach wse@tuxedocomputers.com
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include "acpi_tuxi_thermal.h"
+
+#include "acpi_tuxi.h"
+
+static int eval_intparams(acpi_handle handle, acpi_string pathname,
+			  unsigned long long *params, u32 params_count,
+			  unsigned long long *retval)
+{
+	struct acpi_object_list arguments;
+	unsigned long long data;
+	acpi_status status;
+
+	pr_debug("Eval %s\n", pathname);
+
+	if (params_count > 0) {
+		pr_debug("Params:\n");
+
+		arguments.count = params_count;
+		arguments.pointer = kzalloc(
+			params_count * sizeof(*arguments.pointer), GFP_KERNEL);
+		for (int i = 0; i < params_count; ++i) {
+			pr_debug("%llu\n", params[i]);
+
+			arguments.pointer[i].type = ACPI_TYPE_INTEGER;
+			arguments.pointer[i].integer.value = params[i];
+		}
+		status = acpi_evaluate_integer(handle, pathname, &arguments,
+					       &data);
+		kfree(arguments.pointer);
+	} else {
+		status = acpi_evaluate_integer(handle, pathname, NULL, &data);
+	}
+	if (ACPI_FAILURE(status))
+		return_ACPI_STATUS(status);
+
+	if (retval)
+		*retval = data;
+
+	pr_debug("RetVal: %llu\n", *retval);
+
+	return 0;
+}
+
+static int add(struct acpi_device *device) {
+	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
+	acpi_status status;
+
+	driver_data = devm_kzalloc(&device->dev, sizeof(*driver_data),
+				   GFP_KERNEL);
+	if (!driver_data)
+		return -ENOMEM;
+
+	// Find subdevices
+	status = acpi_get_handle(device->handle, "TFAN",
+				 &driver_data->tfan_handle);
+	if (ACPI_FAILURE(status))
+		return_ACPI_STATUS(status);
+
+	driver_data->eval_intparams = &eval_intparams;
+
+	dev_set_drvdata(&device->dev, driver_data);
+
+	return tuxedo_nbxx_acpi_tuxi_thermal_add_devices(device);
+}
+
+static void remove(struct acpi_device *device) {
+	tuxedo_nbxx_acpi_tuxi_thermal_remove_devices(device);
+}
+
+static const struct acpi_device_id acpi_device_ids[] = {
+	{"TUXI0000", 0},
+	{"", 0}
+};
+MODULE_DEVICE_TABLE(acpi, acpi_device_ids);
+
+static struct acpi_driver acpi_driver = {
+	.name = "tuxedo_nbxx_acpi_tuxi",
+	.ids = acpi_device_ids,
+	.ops = {
+		.add = add,
+		.remove = remove,
+	},
+};
+
+module_acpi_driver(acpi_driver);
+
+MODULE_DESCRIPTION("TUXEDO TUXI");
+MODULE_AUTHOR("Werner Sembach <wse@tuxedocomputers.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h
new file mode 100644
index 0000000000000..7ffda8e0dd68a
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h
@@ -0,0 +1,84 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024 Werner Sembach wse@tuxedocomputers.com
+ */
+
+#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_H__
+#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_H__
+
+#include <linux/thermal.h>
+#include <linux/acpi.h>
+
+/*
+ * Set fan speed target
+ *
+ * Set a specific fan speed (needs manual mode)
+ *
+ * Arg0: Fan index
+ * Arg1: Fan speed as a fraction of maximum speed (0-255)
+ */
+#define TUXI_TFAN_METHOD_SET_FAN_SPEED		"SSPD"
+
+/*
+ * Get fan speed target
+ *
+ * Arg0: Fan index
+ * Returns: Current fan speed target a fraction of maximum speed (0-255)
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_SPEED		"GSPD"
+
+/*
+ * Get fans count
+ *
+ * Returns: Number of individually controllable fans
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_COUNT		"GCNT"
+
+/*
+ * Set fans mode
+ *
+ * Arg0: 0 = auto, 1 = manual
+ */
+#define TUXI_TFAN_METHOD_SET_FAN_MODE		"SMOD"
+
+/*
+ * Get fans mode
+ *
+ * Returns: 0 = auto, 1 = manual
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_MODE		"GMOD"
+
+/*
+ * Get fan type/what the fan is pointed at
+ *
+ * Arg0: Fan index
+ * Returns: 0 = general, 1 = CPU, 2 = GPU
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_TYPE		"GTYP"
+
+/*
+ * Get fan temperature/temperature of what the fan is pointed at
+ *
+ * Arg0: Fan index
+ * Returns: Temperature sensor value in 10ths of degrees kelvin
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE	"GTMP"
+
+/*
+ * Get actual fan speed in RPM
+ *
+ * Arg0: Fan index
+ * Returns: Speed sensor value in revolutions per minute
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_RPM		"GRPM"
+
+struct tuxedo_nbxx_acpi_tuxi_driver_data_t {
+	acpi_handle tfan_handle;
+	int (*eval_intparams)(acpi_handle, acpi_string,
+				  unsigned long long *, u32,
+				  unsigned long long *);
+	struct thermal_zone_device **tzdevs;
+	size_t tzdevs_count;
+};
+
+#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_H__
diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c
new file mode 100644
index 0000000000000..557e3fac15779
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Werner Sembach wse@tuxedocomputers.com
+ */
+
+#include <linux/thermal.h>
+#include <linux/minmax.h>
+#include <linux/export.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include "acpi_tuxi.h"
+
+#include "acpi_tuxi_thermal.h"
+
+static struct thermal_trip tzdtrips[] = {
+	{
+		.type		= THERMAL_TRIP_ACTIVE,
+		.temperature	= 80000,
+		.hysteresis	= 5000,
+	},
+	{
+		.type		= THERMAL_TRIP_ACTIVE,
+		.temperature	= 90000,
+		.hysteresis	= 5000,
+	},
+	{
+		.type		= THERMAL_TRIP_HOT,
+		.temperature	= 100000,
+		.hysteresis	= 5000,
+	},
+	{
+		.type		= THERMAL_TRIP_CRITICAL,
+		.temperature	= 110000,
+	},
+};
+
+struct tz_devdata_t {
+	struct acpi_device *parent;
+	struct thermal_cooling_device *cdev;
+	size_t index;
+};
+
+static bool should_bind(struct thermal_zone_device *tzdev,
+			const struct thermal_trip __always_unused *trip,
+			struct thermal_cooling_device *cdev,
+			struct cooling_spec __always_unused *cspec)
+{
+	return cdev->devdata == tzdev;
+}
+
+static int get_temp(struct thermal_zone_device *tzdev, int *temp)
+{
+	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
+	struct tz_devdata_t *tz_devdata;
+	unsigned long long data;
+	int ret;
+
+	tz_devdata = thermal_zone_device_priv(tzdev);
+	driver_data = dev_get_drvdata(&tz_devdata->parent->dev);
+
+	unsigned long long params[] = { tz_devdata->index };
+	ret = driver_data->eval_intparams(driver_data->tfan_handle,
+					  TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
+					  params, ARRAY_SIZE(params), &data);
+	if (ret)
+		return ret;
+
+	*temp = data * 100 - 272000;
+
+	return 0;
+}
+
+static struct thermal_zone_device_ops tzdops = {
+	.should_bind = should_bind,
+	.get_temp = get_temp,
+};
+
+static struct thermal_zone_params tzdparams = {
+	.governor_name = "user_space",
+};
+
+static int get_max_state(struct thermal_cooling_device __always_unused *cdev,
+		  unsigned long *state)
+{
+	*state = 255;
+
+	return 0;
+}
+
+static int get_cur_state(struct thermal_cooling_device *cdev,
+			 unsigned long *state)
+{
+	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
+	struct thermal_zone_device *tzdev;
+	struct tz_devdata_t *tz_devdata;
+	unsigned long long data;
+	int ret;
+
+	tzdev = cdev->devdata;
+	tz_devdata = thermal_zone_device_priv(tzdev);
+	driver_data = dev_get_drvdata(&tz_devdata->parent->dev);
+
+	unsigned long long params[] = { tz_devdata->index };
+	ret = driver_data->eval_intparams(driver_data->tfan_handle,
+					  TUXI_TFAN_METHOD_GET_FAN_SPEED,
+					  params, ARRAY_SIZE(params), &data);
+	if (ret)
+		return ret;
+
+	*state = data;
+
+	return 0;
+}
+
+static int set_cur_state(struct thermal_cooling_device *cdev,
+			 unsigned long state) {
+	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
+	struct thermal_zone_device *tzdev;
+	struct tz_devdata_t *tz_devdata;
+	unsigned long long data;
+	int ret;
+
+	tzdev = cdev->devdata;
+	tz_devdata = thermal_zone_device_priv(tzdev);
+	driver_data = dev_get_drvdata(&tz_devdata->parent->dev);
+
+	unsigned long long params[] = { tz_devdata->index, state };
+	ret = driver_data->eval_intparams(driver_data->tfan_handle,
+					  TUXI_TFAN_METHOD_SET_FAN_SPEED,
+					  params, ARRAY_SIZE(params), &data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct thermal_cooling_device_ops tcdops = {
+	.get_max_state = get_max_state,
+	.get_cur_state = get_cur_state,
+	.set_cur_state = set_cur_state,
+};
+
+int tuxedo_nbxx_acpi_tuxi_thermal_add_devices(struct acpi_device *parent)
+{
+	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
+	struct tz_devdata_t *tz_devdata;
+	struct thermal_zone_device **tzdevs;
+	unsigned long long data;
+	size_t tzdevs_count;
+	char *type;
+	int ret;
+
+	driver_data = dev_get_drvdata(&parent->dev);
+
+	ret = driver_data->eval_intparams(driver_data->tfan_handle,
+					  TUXI_TFAN_METHOD_GET_FAN_COUNT, NULL,
+					  0, &data);
+	if (ret)
+		return ret;
+
+	tzdevs_count = data;
+	tzdevs = devm_kzalloc(&parent->dev, tzdevs_count * sizeof(*tzdevs),
+			      GFP_KERNEL);
+	if (!tzdevs)
+			return -ENOMEM;
+
+	driver_data->tzdevs = tzdevs;
+	driver_data->tzdevs_count = tzdevs_count;
+
+	for (size_t i = 0; i < tzdevs_count; ++i) {
+		tz_devdata = devm_kzalloc(&parent->dev, sizeof(*tz_devdata),
+					  GFP_KERNEL);
+		if (!tz_devdata)
+			return -ENOMEM;
+
+		tz_devdata->parent = parent;
+		tz_devdata->index = i;
+
+		unsigned long long params[] = { i };
+		ret = driver_data->eval_intparams(driver_data->tfan_handle,
+						  TUXI_TFAN_METHOD_GET_FAN_TYPE,
+						  params, ARRAY_SIZE(params),
+						  &data);
+		if (ret)
+			return ret;
+
+		switch(data) {
+		case 0:
+			type = "tuxedo_general";
+			break;
+		case 1:
+			type = "tuxedo_cpu";
+			break;
+		case 2:
+			type = "tuxedo_gpu";
+			break;
+		default:
+			return -EIO;
+		}
+
+		tzdevs[i] = thermal_zone_device_register_with_trips(
+			type, tzdtrips, ARRAY_SIZE(tzdtrips), tz_devdata, &tzdops,
+			&tzdparams, 1000, 1000);
+		if (IS_ERR(tzdevs[i]))
+			return PTR_ERR(tzdevs[i]);
+
+		tz_devdata->cdev = thermal_cooling_device_register(type,
+								   tzdevs[i],
+								   &tcdops);
+		if (IS_ERR(tz_devdata->cdev))
+			return PTR_ERR(tz_devdata->cdev);
+	}
+
+	unsigned long long params[] = { 1 };
+	driver_data->eval_intparams(driver_data->tfan_handle,
+				    TUXI_TFAN_METHOD_SET_FAN_MODE,
+				    params, ARRAY_SIZE(params), &data);
+
+	return 0;
+}
+
+void tuxedo_nbxx_acpi_tuxi_thermal_remove_devices(struct acpi_device *parent) {
+	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
+	struct tz_devdata_t *tz_devdata;
+	unsigned long long data;
+
+	driver_data = dev_get_drvdata(&parent->dev);
+
+	unsigned long long params[] = { 0 };
+	driver_data->eval_intparams(driver_data->tfan_handle,
+				    TUXI_TFAN_METHOD_SET_FAN_MODE,
+				    params, ARRAY_SIZE(params), &data);
+
+	for (int i = 0; i < driver_data->tzdevs_count; ++i) {
+		tz_devdata = thermal_zone_device_priv(driver_data->tzdevs[i]);
+
+		thermal_cooling_device_unregister(tz_devdata->cdev);
+		thermal_zone_device_unregister(driver_data->tzdevs[i]);
+	}
+}
diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h
new file mode 100644
index 0000000000000..2193fa6db0ea1
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024 Werner Sembach wse@tuxedocomputers.com
+ */
+
+#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_THERMAL_H__
+#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_THERMAL_H__
+
+#include <linux/acpi.h>
+
+int tuxedo_nbxx_acpi_tuxi_thermal_add_devices(struct acpi_device *parent);
+void tuxedo_nbxx_acpi_tuxi_thermal_remove_devices(struct acpi_device *parent);
+
+#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_THERMAL_H__
-- 
2.43.0

Re: [RFC PATCH 1/1] platform: x86: tuxi: Implement TUXEDO TUXI ACPI TFAN as thermal subsystem

[Armin Wolf]

Am 27.11.24 um 12:21 schrieb Werner Sembach:

> The TUXEDO Sirius 16 Gen1 & Gen2 have the custom TUXEDO Interface (TUXI)
> ACPI interface which currently consists of the TFAN device. This has ACPI
> functions to control the built in fans and monitor fan speeds and CPU and
> GPU temprature.
>
> This driver implements this TFAN device via the thermal subsystem to allow
> userspace controlled, but hardware safe, custom fan curves.
>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   drivers/platform/x86/Makefile                 |   3 +
>   drivers/platform/x86/tuxedo/Kbuild            |   6 +
>   drivers/platform/x86/tuxedo/nbxx/Kbuild       |   8 +
>   drivers/platform/x86/tuxedo/nbxx/Kconfig      |   9 +
>   drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c  |  96 +++++++
>   drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h  |  84 ++++++
>   .../x86/tuxedo/nbxx/acpi_tuxi_thermal.c       | 241 ++++++++++++++++++
>   .../x86/tuxedo/nbxx/acpi_tuxi_thermal.h       |  14 +
>   8 files changed, 461 insertions(+)
>   create mode 100644 drivers/platform/x86/tuxedo/Kbuild
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h
>
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index e1b1429470674..1562dcd7ad9a5 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
>
>   # SEL
>   obj-$(CONFIG_SEL3350_PLATFORM)		+= sel3350-platform.o
> +
> +# TUXEDO
> +obj-y					+= tuxedo/
> diff --git a/drivers/platform/x86/tuxedo/Kbuild b/drivers/platform/x86/tuxedo/Kbuild
> new file mode 100644
> index 0000000000000..16396a8923432
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/Kbuild
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +obj-y	+= nbxx/

Hi,

please give this directory a more descriptive name, maybe call it "tuxi"?

> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kbuild b/drivers/platform/x86/tuxedo/nbxx/Kbuild
> new file mode 100644
> index 0000000000000..aa4d6d66b785f
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/Kbuild
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +tuxedo_nbxx_acpi_tuxi-y			:= acpi_tuxi.o
> +tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_thermal.o
> +obj-$(CONFIG_TUXEDO_NBXX_ACPI_TUXI)	+= tuxedo_nbxx_acpi_tuxi.o
> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kconfig b/drivers/platform/x86/tuxedo/nbxx/Kconfig
> new file mode 100644
> index 0000000000000..93f7f86e803da
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/Kconfig
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +config TUXEDO_NBXX_ACPI_TUXI
> +	tristate "TUXEDO TUXI"
> +	help
> +	  TUXEDO TUXI driver

Please add any dependencies and a more descriptive help text.

> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
> new file mode 100644
> index 0000000000000..6031cf731b449
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 Werner Sembach wse@tuxedocomputers.com
> + */
> +
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include "acpi_tuxi_thermal.h"
> +
> +#include "acpi_tuxi.h"
> +
> +static int eval_intparams(acpi_handle handle, acpi_string pathname,
> +			  unsigned long long *params, u32 params_count,
> +			  unsigned long long *retval)
> +{
> +	struct acpi_object_list arguments;
> +	unsigned long long data;
> +	acpi_status status;
> +
> +	pr_debug("Eval %s\n", pathname);
> +
> +	if (params_count > 0) {
> +		pr_debug("Params:\n");
> +
> +		arguments.count = params_count;
> +		arguments.pointer = kzalloc(
> +			params_count * sizeof(*arguments.pointer), GFP_KERNEL);
> +		for (int i = 0; i < params_count; ++i) {
> +			pr_debug("%llu\n", params[i]);
> +
> +			arguments.pointer[i].type = ACPI_TYPE_INTEGER;
> +			arguments.pointer[i].integer.value = params[i];
> +		}
> +		status = acpi_evaluate_integer(handle, pathname, &arguments,
> +					       &data);
> +		kfree(arguments.pointer);
> +	} else {
> +		status = acpi_evaluate_integer(handle, pathname, NULL, &data);
> +	}
> +	if (ACPI_FAILURE(status))
> +		return_ACPI_STATUS(status);

This is an internal function of ACPICA, please just return -EIO.

> +
> +	if (retval)
> +		*retval = data;
> +
> +	pr_debug("RetVal: %llu\n", *retval);
> +
> +	return 0;
> +}
> +
> +static int add(struct acpi_device *device) {
> +	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
> +	acpi_status status;
> +
> +	driver_data = devm_kzalloc(&device->dev, sizeof(*driver_data),
> +				   GFP_KERNEL);
> +	if (!driver_data)
> +		return -ENOMEM;
> +
> +	// Find subdevices
> +	status = acpi_get_handle(device->handle, "TFAN",
> +				 &driver_data->tfan_handle);
> +	if (ACPI_FAILURE(status))
> +		return_ACPI_STATUS(status);
> +
> +	driver_data->eval_intparams = &eval_intparams;
> +
> +	dev_set_drvdata(&device->dev, driver_data);
> +
> +	return tuxedo_nbxx_acpi_tuxi_thermal_add_devices(device);
> +}
> +
> +static void remove(struct acpi_device *device) {
> +	tuxedo_nbxx_acpi_tuxi_thermal_remove_devices(device);
> +}
> +
> +static const struct acpi_device_id acpi_device_ids[] = {
> +	{"TUXI0000", 0},
> +	{"", 0}
> +};
> +MODULE_DEVICE_TABLE(acpi, acpi_device_ids);
> +
> +static struct acpi_driver acpi_driver = {
> +	.name = "tuxedo_nbxx_acpi_tuxi",
> +	.ids = acpi_device_ids,
> +	.ops = {
> +		.add = add,
> +		.remove = remove,
> +	},
> +};
> +
> +module_acpi_driver(acpi_driver);

Please implement your driver as an platform driver.

> +
> +MODULE_DESCRIPTION("TUXEDO TUXI");
> +MODULE_AUTHOR("Werner Sembach <wse@tuxedocomputers.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h
> new file mode 100644
> index 0000000000000..7ffda8e0dd68a
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024 Werner Sembach wse@tuxedocomputers.com
> + */
> +
> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_H__
> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_H__
> +
> +#include <linux/thermal.h>
> +#include <linux/acpi.h>
> +
> +/*
> + * Set fan speed target
> + *
> + * Set a specific fan speed (needs manual mode)
> + *
> + * Arg0: Fan index
> + * Arg1: Fan speed as a fraction of maximum speed (0-255)
> + */
> +#define TUXI_TFAN_METHOD_SET_FAN_SPEED		"SSPD"
> +
> +/*
> + * Get fan speed target
> + *
> + * Arg0: Fan index
> + * Returns: Current fan speed target a fraction of maximum speed (0-255)
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_SPEED		"GSPD"
> +
> +/*
> + * Get fans count
> + *
> + * Returns: Number of individually controllable fans
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_COUNT		"GCNT"
> +
> +/*
> + * Set fans mode
> + *
> + * Arg0: 0 = auto, 1 = manual
> + */
> +#define TUXI_TFAN_METHOD_SET_FAN_MODE		"SMOD"
> +
> +/*
> + * Get fans mode
> + *
> + * Returns: 0 = auto, 1 = manual
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_MODE		"GMOD"
> +
> +/*
> + * Get fan type/what the fan is pointed at
> + *
> + * Arg0: Fan index
> + * Returns: 0 = general, 1 = CPU, 2 = GPU
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_TYPE		"GTYP"
> +
> +/*
> + * Get fan temperature/temperature of what the fan is pointed at
> + *
> + * Arg0: Fan index
> + * Returns: Temperature sensor value in 10ths of degrees kelvin
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE	"GTMP"
> +
> +/*
> + * Get actual fan speed in RPM
> + *
> + * Arg0: Fan index
> + * Returns: Speed sensor value in revolutions per minute
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_RPM		"GRPM"
> +
> +struct tuxedo_nbxx_acpi_tuxi_driver_data_t {
> +	acpi_handle tfan_handle;
> +	int (*eval_intparams)(acpi_handle, acpi_string,
> +				  unsigned long long *, u32,
> +				  unsigned long long *);
> +	struct thermal_zone_device **tzdevs;
> +	size_t tzdevs_count;
> +};

Do you plan for this driver to have multiple backends? If not then
please remove this unnecessary indirection.

> +
> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_H__
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c
> new file mode 100644
> index 0000000000000..557e3fac15779
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 Werner Sembach wse@tuxedocomputers.com
> + */
> +
> +#include <linux/thermal.h>
> +#include <linux/minmax.h>
> +#include <linux/export.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/acpi.h>
> +#include "acpi_tuxi.h"
> +
> +#include "acpi_tuxi_thermal.h"
> +
> +static struct thermal_trip tzdtrips[] = {
> +	{
> +		.type		= THERMAL_TRIP_ACTIVE,
> +		.temperature	= 80000,
> +		.hysteresis	= 5000,
> +	},
> +	{
> +		.type		= THERMAL_TRIP_ACTIVE,
> +		.temperature	= 90000,
> +		.hysteresis	= 5000,
> +	},
> +	{
> +		.type		= THERMAL_TRIP_HOT,
> +		.temperature	= 100000,
> +		.hysteresis	= 5000,
> +	},
> +	{
> +		.type		= THERMAL_TRIP_CRITICAL,
> +		.temperature	= 110000,
> +	},
> +};
> +
> +struct tz_devdata_t {
> +	struct acpi_device *parent;
> +	struct thermal_cooling_device *cdev;
> +	size_t index;
> +};
> +
> +static bool should_bind(struct thermal_zone_device *tzdev,
> +			const struct thermal_trip __always_unused *trip,
> +			struct thermal_cooling_device *cdev,
> +			struct cooling_spec __always_unused *cspec)
> +{
> +	return cdev->devdata == tzdev;
> +}
> +
> +static int get_temp(struct thermal_zone_device *tzdev, int *temp)
> +{
> +	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
> +	struct tz_devdata_t *tz_devdata;
> +	unsigned long long data;
> +	int ret;
> +
> +	tz_devdata = thermal_zone_device_priv(tzdev);
> +	driver_data = dev_get_drvdata(&tz_devdata->parent->dev);
> +
> +	unsigned long long params[] = { tz_devdata->index };
> +	ret = driver_data->eval_intparams(driver_data->tfan_handle,
> +					  TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
> +					  params, ARRAY_SIZE(params), &data);
> +	if (ret)
> +		return ret;
> +
> +	*temp = data * 100 - 272000;

Please check if you can use units.h for that.

> +
> +	return 0;
> +}
> +
> +static struct thermal_zone_device_ops tzdops = {
> +	.should_bind = should_bind,
> +	.get_temp = get_temp,
> +};
> +
> +static struct thermal_zone_params tzdparams = {
> +	.governor_name = "user_space",
> +};
> +
> +static int get_max_state(struct thermal_cooling_device __always_unused *cdev,
> +		  unsigned long *state)
> +{
> +	*state = 255;
> +
> +	return 0;
> +}
> +
> +static int get_cur_state(struct thermal_cooling_device *cdev,
> +			 unsigned long *state)
> +{
> +	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
> +	struct thermal_zone_device *tzdev;
> +	struct tz_devdata_t *tz_devdata;
> +	unsigned long long data;
> +	int ret;
> +
> +	tzdev = cdev->devdata;
> +	tz_devdata = thermal_zone_device_priv(tzdev);
> +	driver_data = dev_get_drvdata(&tz_devdata->parent->dev);
> +
> +	unsigned long long params[] = { tz_devdata->index };
> +	ret = driver_data->eval_intparams(driver_data->tfan_handle,
> +					  TUXI_TFAN_METHOD_GET_FAN_SPEED,
> +					  params, ARRAY_SIZE(params), &data);
> +	if (ret)
> +		return ret;
> +
> +	*state = data;
> +
> +	return 0;
> +}
> +
> +static int set_cur_state(struct thermal_cooling_device *cdev,
> +			 unsigned long state) {
> +	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
> +	struct thermal_zone_device *tzdev;
> +	struct tz_devdata_t *tz_devdata;
> +	unsigned long long data;
> +	int ret;
> +
> +	tzdev = cdev->devdata;
> +	tz_devdata = thermal_zone_device_priv(tzdev);
> +	driver_data = dev_get_drvdata(&tz_devdata->parent->dev);
> +
> +	unsigned long long params[] = { tz_devdata->index, state };

Please check if "state" is greater than 255 and return -EINVAL in such a case.

> +	ret = driver_data->eval_intparams(driver_data->tfan_handle,
> +					  TUXI_TFAN_METHOD_SET_FAN_SPEED,
> +					  params, ARRAY_SIZE(params), &data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static struct thermal_cooling_device_ops tcdops = {
> +	.get_max_state = get_max_state,
> +	.get_cur_state = get_cur_state,
> +	.set_cur_state = set_cur_state,
> +};
> +
> +int tuxedo_nbxx_acpi_tuxi_thermal_add_devices(struct acpi_device *parent)
> +{
> +	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
> +	struct tz_devdata_t *tz_devdata;
> +	struct thermal_zone_device **tzdevs;
> +	unsigned long long data;
> +	size_t tzdevs_count;
> +	char *type;
> +	int ret;
> +
> +	driver_data = dev_get_drvdata(&parent->dev);
> +
> +	ret = driver_data->eval_intparams(driver_data->tfan_handle,
> +					  TUXI_TFAN_METHOD_GET_FAN_COUNT, NULL,
> +					  0, &data);
> +	if (ret)
> +		return ret;
> +
> +	tzdevs_count = data;
> +	tzdevs = devm_kzalloc(&parent->dev, tzdevs_count * sizeof(*tzdevs),
> +			      GFP_KERNEL);
> +	if (!tzdevs)
> +			return -ENOMEM;
> +
> +	driver_data->tzdevs = tzdevs;
> +	driver_data->tzdevs_count = tzdevs_count;
> +
> +	for (size_t i = 0; i < tzdevs_count; ++i) {
> +		tz_devdata = devm_kzalloc(&parent->dev, sizeof(*tz_devdata),
> +					  GFP_KERNEL);
> +		if (!tz_devdata)
> +			return -ENOMEM;
> +
> +		tz_devdata->parent = parent;
> +		tz_devdata->index = i;
> +
> +		unsigned long long params[] = { i };
> +		ret = driver_data->eval_intparams(driver_data->tfan_handle,
> +						  TUXI_TFAN_METHOD_GET_FAN_TYPE,
> +						  params, ARRAY_SIZE(params),
> +						  &data);
> +		if (ret)
> +			return ret;
> +
> +		switch(data) {
> +		case 0:
> +			type = "tuxedo_general";
> +			break;
> +		case 1:
> +			type = "tuxedo_cpu";
> +			break;
> +		case 2:
> +			type = "tuxedo_gpu";
> +			break;
> +		default:
> +			return -EIO;
> +		}
> +
> +		tzdevs[i] = thermal_zone_device_register_with_trips(
> +			type, tzdtrips, ARRAY_SIZE(tzdtrips), tz_devdata, &tzdops,
> +			&tzdparams, 1000, 1000);
> +		if (IS_ERR(tzdevs[i]))
> +			return PTR_ERR(tzdevs[i]);
> +
> +		tz_devdata->cdev = thermal_cooling_device_register(type,
> +								   tzdevs[i],
> +								   &tcdops);
> +		if (IS_ERR(tz_devdata->cdev))
> +			return PTR_ERR(tz_devdata->cdev);

Where do you clean up those thermal zones and cooling devices should an error occur? Better use devres for that.

Thanks,
Armin Wolf

> +	}
> +
> +	unsigned long long params[] = { 1 };
> +	driver_data->eval_intparams(driver_data->tfan_handle,
> +				    TUXI_TFAN_METHOD_SET_FAN_MODE,
> +				    params, ARRAY_SIZE(params), &data);
> +
> +	return 0;
> +}
> +
> +void tuxedo_nbxx_acpi_tuxi_thermal_remove_devices(struct acpi_device *parent) {
> +	struct tuxedo_nbxx_acpi_tuxi_driver_data_t *driver_data;
> +	struct tz_devdata_t *tz_devdata;
> +	unsigned long long data;
> +
> +	driver_data = dev_get_drvdata(&parent->dev);
> +
> +	unsigned long long params[] = { 0 };
> +	driver_data->eval_intparams(driver_data->tfan_handle,
> +				    TUXI_TFAN_METHOD_SET_FAN_MODE,
> +				    params, ARRAY_SIZE(params), &data);
> +
> +	for (int i = 0; i < driver_data->tzdevs_count; ++i) {
> +		tz_devdata = thermal_zone_device_priv(driver_data->tzdevs[i]);
> +
> +		thermal_cooling_device_unregister(tz_devdata->cdev);
> +		thermal_zone_device_unregister(driver_data->tzdevs[i]);
> +	}
> +}
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h
> new file mode 100644
> index 0000000000000..2193fa6db0ea1
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024 Werner Sembach wse@tuxedocomputers.com
> + */
> +
> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_THERMAL_H__
> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_THERMAL_H__
> +
> +#include <linux/acpi.h>
> +
> +int tuxedo_nbxx_acpi_tuxi_thermal_add_devices(struct acpi_device *parent);
> +void tuxedo_nbxx_acpi_tuxi_thermal_remove_devices(struct acpi_device *parent);
> +
> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_THERMAL_H__

Re: [RFC PATCH 0/1] platform: x86: tuxi: Implement TUXEDO TUXI ACPI TFAN as thermal subsystem

[Armin Wolf]

Am 27.11.24 um 12:21 schrieb Werner Sembach:

> Hi,
>
> Following up to https://lore.kernel.org/all/172b7acd-4313-4924-bcbc-41b73b39ada0@tuxedocomputers.com/ and https://lore.kernel.org/all/f26d867e-f247-43bb-a78b-be0bce35c973@roeck-us.net/ I experimented with the thermal subsystem and these are my results so far, but I'm hitting a bit of a wall:
>
> As far as I can tell to implement "2. As long as GTMP is > 80°C fan speed must be at least 30%." I would need to add a new gevenor, lets call it "user_space_with_safeguards". I would be nice when the temp <-> fanspeed relation could be passed via the thermal_trip structure. And safeguarding the hardware from userspace only works when I can restrict userspace from just selecting the preexisting "user_space" govenor.
>
> So my ideas/questions:
> - Add a field "min_fanspeed_percent" to the thermal_trip struct that will only be used by the "user_space_with_safeguards" govenor
> - Add a "user_space_with_safeguards" govenor that is the same as the "user_space" govenor, but on trip, a minimum speed is applied
> - How can i ensure that on further speed updates the min speed is applied to? I could just implement it directly in the cdev, but that would be spagetti coding around the govenor.
> - Can I somehow restrict userspace from using certain govenors?
> - I'm a litte bit confused about the thermal zone "mode" sysfs switch, here it says deactivate for userspace control: https://elixir.bootlin.com/linux/v6.12/source/Documentation/ABI/testing/sysfs-class-thermal#L20, but what about the user_space govenor then?

Hi,

i am having little experience with the thermal subsystem, but i suggest that policy decisions like "min_fanspeed_percent" should either:

- come from the hardware/firmware itself
- come from userspace

Effectively this driver tries to enforce a Tuxedo-specific policy that is not directly based on hardware limits. The book "Linux Device Drivers"
says:

	"the role of a device driver is providing///mechanism/, not/policy/."

Furthermore:

	"When/writing/ drivers, a programmer should pay particular attention to this fundamental concept: write kernel code to access the hardware,
	 but don't force particular policies on the user, since different users have different needs. The driver should deal with making the hardware
	 available, leaving all the issues about/how/ to use the hardware to the applications. A driver, then, is flexible if it offers access to the
	 hardware capabilities without adding constraints."

The issue is that the Tuxedo-specific policy is not directly connected with the hardware. Some hardware might need a bigger minimum fan speed than
other hardware for example.

The hardware (or rather firmware in this case) should communicate those constraints to the linux driver so that we do not need to rely on random
temperatures for hardware protection.

This ACPI interface however basically provides us with a hwmon interface and Tuxedo now wants the kernel to enforce their policy on it. I suspect that
happens for warranty reasons, right?

Maybe there is a way to enforce this policy through userspace applications?

Thanks,
Armin Wolf

> Best regards,
> Werner
>
> Werner Sembach (1):
>    platform: x86: tuxi: Implement TUXEDO TUXI ACPI TFAN as thermal
>      subsystem
>
>   drivers/platform/x86/Makefile                 |   3 +
>   drivers/platform/x86/tuxedo/Kbuild            |   6 +
>   drivers/platform/x86/tuxedo/nbxx/Kbuild       |   8 +
>   drivers/platform/x86/tuxedo/nbxx/Kconfig      |   9 +
>   drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c  |  96 +++++++
>   drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h  |  84 ++++++
>   .../x86/tuxedo/nbxx/acpi_tuxi_thermal.c       | 241 ++++++++++++++++++
>   .../x86/tuxedo/nbxx/acpi_tuxi_thermal.h       |  14 +
>   8 files changed, 461 insertions(+)
>   create mode 100644 drivers/platform/x86/tuxedo/Kbuild
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.h
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.c
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_thermal.h
>

Re: [RFC PATCH 0/1] platform: x86: tuxi: Implement TUXEDO TUXI ACPI TFAN as thermal subsystem

[Hans de Goede]

Hi Armin, Werner,

On 1-Dec-24 6:58 PM, Armin Wolf wrote:
> Am 27.11.24 um 12:21 schrieb Werner Sembach:
> 
>> Hi,
>>
>> Following up to https://lore.kernel.org/all/172b7acd-4313-4924-bcbc-41b73b39ada0@tuxedocomputers.com/ and https://lore.kernel.org/all/f26d867e-f247-43bb-a78b-be0bce35c973@roeck-us.net/ I experimented with the thermal subsystem and these are my results so far, but I'm hitting a bit of a wall:
>>
>> As far as I can tell to implement "2. As long as GTMP is > 80°C fan speed must be at least 30%." I would need to add a new gevenor, lets call it "user_space_with_safeguards". I would be nice when the temp <-> fanspeed relation could be passed via the thermal_trip structure. And safeguarding the hardware from userspace only works when I can restrict userspace from just selecting the preexisting "user_space" govenor.
>>
>> So my ideas/questions:
>> - Add a field "min_fanspeed_percent" to the thermal_trip struct that will only be used by the "user_space_with_safeguards" govenor
>> - Add a "user_space_with_safeguards" govenor that is the same as the "user_space" govenor, but on trip, a minimum speed is applied
>> - How can i ensure that on further speed updates the min speed is applied to? I could just implement it directly in the cdev, but that would be spagetti coding around the govenor.
>> - Can I somehow restrict userspace from using certain govenors?
>> - I'm a litte bit confused about the thermal zone "mode" sysfs switch, here it says deactivate for userspace control: https://elixir.bootlin.com/linux/v6.12/source/Documentation/ABI/testing/sysfs-class-thermal#L20, but what about the user_space govenor then?
> 
> Hi,
> 
> i am having little experience with the thermal subsystem, but i suggest that policy decisions like "min_fanspeed_percent" should either:
> 
> - come from the hardware/firmware itself
> - come from userspace
> 
> Effectively this driver tries to enforce a Tuxedo-specific policy that is not directly based on hardware limits. The book "Linux Device Drivers"
> says:
> 
>     "the role of a device driver is providing///mechanism/, not/policy/."
> 
> Furthermore:
> 
>     "When/writing/ drivers, a programmer should pay particular attention to this fundamental concept: write kernel code to access the hardware,
>      but don't force particular policies on the user, since different users have different needs. The driver should deal with making the hardware
>      available, leaving all the issues about/how/ to use the hardware to the applications. A driver, then, is flexible if it offers access to the
>      hardware capabilities without adding constraints."
> 
> The issue is that the Tuxedo-specific policy is not directly connected with the hardware. Some hardware might need a bigger minimum fan speed than
> other hardware for example.
> 
> The hardware (or rather firmware in this case) should communicate those constraints to the linux driver so that we do not need to rely on random
> temperatures for hardware protection.
> 
> This ACPI interface however basically provides us with a hwmon interface and Tuxedo now wants the kernel to enforce their policy on it. I suspect that
> happens for warranty reasons, right?
> 
> Maybe there is a way to enforce this policy through userspace applications?

An important role of device-drivers is also to avoid driving hardware outside
its valid specifications. E.g. charger drivers in the power_supply subsystem
have constant_charge_current and constant_charge_voltage settings which
give a max charging speed in Amperes and a max charging Voltage (typically
4.2V for single Li-xxx cell batteries).

Setting these too high can be very dangerous leading to batteries catching
fire / exploding. so naturally the drivers also have and enforce
constant_charge_current_max and constant_charge_voltage_max sysfs attributes
and the values there do not reflect the maximum what the charger-chip can
handle, but the maximum which the manufacturer has specified for the battery.

This info can e.g. come from devicetree but if not present then the values
applied by the firmware at boot should be read back and used not only
as active, but also as max values.

I believe what Werner wants is pretty much the same thing, allow userspace
to provide its own fan management, but have the kernel monitor the temperature
and if the temperature goes say above 80° Celsius, enforce a min fan-speed of
30% and at 90° enforce a fan-speed of say 70%.

Yes the exact values are a for of policy, but this is a policy to protect
hw from getting damaged and as such is fine to have in the kernel.

As for how to implement this with the thermal subsystem (which indeed looks
like it might be the way to go) I'm not familiar enough with the thermal
subsystem either.

Werner, I suggest that you send an email to the thermal subsystem maintainers
and ask them about this. It would be good to start with explaining your
problem before asking the questions. I would leave out details about the ACPI
interface. Just say you have a temp-sensor + pwm to control fan speed combination
where you want to allow userspace control while having the kernel monitor
things and have the kernel overriding userspace's fan speed setting when
things get too hot, to deal with e.g. the userspace process crashing at
a low fan speed setting.

Regards,

Hans

Re: [RFC PATCH 0/1] platform: x86: tuxi: Implement TUXEDO TUXI ACPI TFAN as thermal subsystem

[Werner Sembach]

Hi Hans, hi Armin,

Am 02.12.24 um 10:40 schrieb Hans de Goede:
> Hi Armin, Werner,
>
> On 1-Dec-24 6:58 PM, Armin Wolf wrote:
>> Am 27.11.24 um 12:21 schrieb Werner Sembach:
>>
>>> Hi,
>>>
>>> Following up to https://lore.kernel.org/all/172b7acd-4313-4924-bcbc-41b73b39ada0@tuxedocomputers.com/ and https://lore.kernel.org/all/f26d867e-f247-43bb-a78b-be0bce35c973@roeck-us.net/ I experimented with the thermal subsystem and these are my results so far, but I'm hitting a bit of a wall:
>>>
>>> As far as I can tell to implement "2. As long as GTMP is > 80°C fan speed must be at least 30%." I would need to add a new gevenor, lets call it "user_space_with_safeguards". I would be nice when the temp <-> fanspeed relation could be passed via the thermal_trip structure. And safeguarding the hardware from userspace only works when I can restrict userspace from just selecting the preexisting "user_space" govenor.
>>>
>>> So my ideas/questions:
>>> - Add a field "min_fanspeed_percent" to the thermal_trip struct that will only be used by the "user_space_with_safeguards" govenor
>>> - Add a "user_space_with_safeguards" govenor that is the same as the "user_space" govenor, but on trip, a minimum speed is applied
>>> - How can i ensure that on further speed updates the min speed is applied to? I could just implement it directly in the cdev, but that would be spagetti coding around the govenor.
>>> - Can I somehow restrict userspace from using certain govenors?
>>> - I'm a litte bit confused about the thermal zone "mode" sysfs switch, here it says deactivate for userspace control: https://elixir.bootlin.com/linux/v6.12/source/Documentation/ABI/testing/sysfs-class-thermal#L20, but what about the user_space govenor then?
>> Hi,
>>
>> i am having little experience with the thermal subsystem, but i suggest that policy decisions like "min_fanspeed_percent" should either:
>>
>> - come from the hardware/firmware itself
>> - come from userspace
>>
>> Effectively this driver tries to enforce a Tuxedo-specific policy that is not directly based on hardware limits. The book "Linux Device Drivers"
>> says:
>>
>>      "the role of a device driver is providing///mechanism/, not/policy/."

But it is a hardware limit, as the hardware will wear and might break if not run 
within its limits.

As this driver is for already released hardware, a firmware fix is not possible.

>>
>> Furthermore:
>>
>>      "When/writing/ drivers, a programmer should pay particular attention to this fundamental concept: write kernel code to access the hardware,
>>       but don't force particular policies on the user, since different users have different needs. The driver should deal with making the hardware
>>       available, leaving all the issues about/how/ to use the hardware to the applications. A driver, then, is flexible if it offers access to the
>>       hardware capabilities without adding constraints."
>>
>> The issue is that the Tuxedo-specific policy is not directly connected with the hardware. Some hardware might need a bigger minimum fan speed than
>> other hardware for example.

Currently this driver is for exactly 2 TUXEDO devices (Sirius 16 gen 1 & 2).

Future devices can have different limits provided by the ACPI or a list in the 
driver.

>>
>> The hardware (or rather firmware in this case) should communicate those constraints to the linux driver so that we do not need to rely on random
>> temperatures for hardware protection.
But then the driver still needs to enforce them.
>>
>> This ACPI interface however basically provides us with a hwmon interface and Tuxedo now wants the kernel to enforce their policy on it. I suspect that
>> happens for warranty reasons, right?

The ACPI interface is only present on TUXEDO devices (TUXI stands for TUXEDO 
Interface and TFAN for TUXEDO FAN). And we want the policy for the devices to 
live longer, not only for warranty.

TUXI meant as a simple getter and setter interface for the EC. We don't have 
access to the EC firmware source so more complex stuff needs to be done 
somewhere else, i.e. the driver.

>>
>> Maybe there is a way to enforce this policy through userspace applications?
That's no enforcement as userspace can just write the sysfs directly.
> An important role of device-drivers is also to avoid driving hardware outside
> its valid specifications. E.g. charger drivers in the power_supply subsystem
> have constant_charge_current and constant_charge_voltage settings which
> give a max charging speed in Amperes and a max charging Voltage (typically
> 4.2V for single Li-xxx cell batteries).
>
> Setting these too high can be very dangerous leading to batteries catching
> fire / exploding. so naturally the drivers also have and enforce
> constant_charge_current_max and constant_charge_voltage_max sysfs attributes
> and the values there do not reflect the maximum what the charger-chip can
> handle, but the maximum which the manufacturer has specified for the battery.
>
> This info can e.g. come from devicetree but if not present then the values
> applied by the firmware at boot should be read back and used not only
> as active, but also as max values.
>
> I believe what Werner wants is pretty much the same thing, allow userspace
> to provide its own fan management, but have the kernel monitor the temperature
> and if the temperature goes say above 80° Celsius, enforce a min fan-speed of
> 30% and at 90° enforce a fan-speed of say 70%.
>
> Yes the exact values are a for of policy, but this is a policy to protect
> hw from getting damaged and as such is fine to have in the kernel.
>
> As for how to implement this with the thermal subsystem (which indeed looks
> like it might be the way to go) I'm not familiar enough with the thermal
> subsystem either.
>
> Werner, I suggest that you send an email to the thermal subsystem maintainers
> and ask them about this. It would be good to start with explaining your
> problem before asking the questions. I would leave out details about the ACPI
> interface. Just say you have a temp-sensor + pwm to control fan speed combination
> where you want to allow userspace control while having the kernel monitor
> things and have the kernel overriding userspace's fan speed setting when
> things get too hot, to deal with e.g. the userspace process crashing at
> a low fan speed setting.
Ack
>
> Regards,
>
> Hans
>
Best regards,

Werner