[PATCH 0/6] thermal/intel: Introduce intel-tcc lib and enhance tjmax handling

[PATCH 0/6] thermal/intel: Introduce intel-tcc lib and enhance tjmax handling

[Zhang Rui]

Currently, all the thermal drivers that access the Intel CPU TCC (thermal
control circuitry) MSRs have two problems,
1. they have their own implementation for the same TCC functionalities,
   including getting the tjmax/temperature, getting/setting the TCC offset.
   This introduces a lot of duplicate code.
2. they get TjMax value once and always use the cached value later.
   But Tjmax value retrieved from MSR_IA32_TEMPERATURE_TARGET can be changed
   at runtime for cases like the Intel SST-PP (Intel Speed Select Technology-
   Performance Profile) level change.

The intel-tcc library is introduced in patch 1/6 for cleaning up the duplicate
code among these drivers, and it also ensures the temperature is alway got
based on the updated tjmax value.

Patch 2 ~ 5 cleans up the drivers by using the new Intel TCC lib APIs.

Patch 6/6 enhances the x86_pkg_temp driver to handle dynamic tjmax when
progamming the thermal interrupt threshold.
Actually, the thermal interrupt threshold programming can also be part of the
TCC library, but I didn't do it in this version because x86_pkg_temp is the
only user for now.

thanks,
rui

----------------------------------------------------------------
Zhang Rui (6):
      thermal/intel: Introduce Intel TCC library
      thermal/x86_pkg_temp_thermal: Use Intel TCC library
      thermal/int340x/processor_thermal: Use Intel TCC library
      thermal/intel/intel_soc_dts_iosf: Use Intel TCC library
      thermal/intel/intel_tcc_cooling: Use Intel TCC library
      thermal/x86_pkg_temp_thermal: Add support for handling dynamic tjmax

 drivers/thermal/intel/Kconfig                      |   7 ++
 drivers/thermal/intel/Makefile                     |   1 +
 drivers/thermal/intel/int340x_thermal/Kconfig      |   1 +
 .../int340x_thermal/processor_thermal_device.c     | 123 ++++---------------
 drivers/thermal/intel/intel_soc_dts_iosf.c         |  33 +-----
 drivers/thermal/intel/intel_tcc.c                  | 132 +++++++++++++++++++++
 drivers/thermal/intel/intel_tcc_cooling.c          |  37 +-----
 drivers/thermal/intel/x86_pkg_temp_thermal.c       |  62 ++++------
 include/linux/intel_tcc.h                          |  18 +++
 9 files changed, 219 insertions(+), 195 deletions(-)
 create mode 100644 drivers/thermal/intel/intel_tcc.c
 create mode 100644 include/linux/intel_tcc.h

[PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Zhang Rui]

There are several different drivers that accesses the Intel TCC
(thermal control circuitry) MSRs, and each of them has its own
implementation for the same functionalities, e.g. getting the current
temperature, getting the tj_max, and getting/setting the tj_max offset.

Introduce a library to unify the code for Intel CPU TCC MSR access.

At the same time, ensure the temperature is got based on the updated
tjmax value because tjmax can be changed at runtime for cases like
the Intel SST-PP (Intel Speed Select Technology - Performance Profile)
level change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/intel/Kconfig     |   4 +
 drivers/thermal/intel/Makefile    |   1 +
 drivers/thermal/intel/intel_tcc.c | 131 ++++++++++++++++++++++++++++++
 include/linux/intel_tcc.h         |  18 ++++
 4 files changed, 154 insertions(+)
 create mode 100644 drivers/thermal/intel/intel_tcc.c
 create mode 100644 include/linux/intel_tcc.h

diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
index f0c845679250..6b938c040d6e 100644
--- a/drivers/thermal/intel/Kconfig
+++ b/drivers/thermal/intel/Kconfig
@@ -12,6 +12,10 @@ config X86_THERMAL_VECTOR
 	def_bool y
 	depends on X86 && CPU_SUP_INTEL && X86_LOCAL_APIC
 
+config INTEL_TCC
+	bool
+	depends on X86
+
 config X86_PKG_TEMP_THERMAL
 	tristate "X86 package temperature thermal driver"
 	depends on X86_THERMAL_VECTOR
diff --git a/drivers/thermal/intel/Makefile b/drivers/thermal/intel/Makefile
index 9a8d8054f316..5d8833c82ab6 100644
--- a/drivers/thermal/intel/Makefile
+++ b/drivers/thermal/intel/Makefile
@@ -2,6 +2,7 @@
 #
 # Makefile for various Intel thermal drivers.
 
+obj-$(CONFIG_INTEL_TCC)	+= intel_tcc.o
 obj-$(CONFIG_INTEL_POWERCLAMP)	+= intel_powerclamp.o
 obj-$(CONFIG_X86_PKG_TEMP_THERMAL)	+= x86_pkg_temp_thermal.o
 obj-$(CONFIG_INTEL_SOC_DTS_IOSF_CORE)	+= intel_soc_dts_iosf.o
diff --git a/drivers/thermal/intel/intel_tcc.c b/drivers/thermal/intel/intel_tcc.c
new file mode 100644
index 000000000000..74b434914975
--- /dev/null
+++ b/drivers/thermal/intel/intel_tcc.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * intel_tcc.c - Library for Intel TCC (thermal control circuitry) MSR access
+ * Copyright (c) 2022, Intel Corporation.
+ */
+
+#include <linux/errno.h>
+#include <linux/intel_tcc.h>
+#include <asm/msr.h>
+
+/**
+ * intel_tcc_get_tjmax() - returns the default TCC activation Temperature
+ * @cpu: cpu that the MSR should be run on.
+ * @tjmax: a valid pointer to where to store the Tjmax value
+ *
+ * Get the TjMax value, which is the default thermal throttling or TCC
+ * activation temperature in degrees C.
+ *
+ * Return: On success returns 0, an error code otherwise
+ */
+
+int intel_tcc_get_tjmax(int cpu, int *tjmax)
+{
+	u32 eax, edx;
+	int err;
+
+	err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
+					&eax, &edx);
+	if (err)
+		return err;
+
+	*tjmax = (eax >> 16) & 0xff;
+
+	return *tjmax ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_NS_GPL(intel_tcc_get_tjmax, INTEL_TCC);
+
+/**
+ * intel_tcc_get_offset() - returns the TCC Offset value to Tjmax
+ * @cpu: cpu that the MSR should be run on.
+ * @offset: a valid pointer to where to store the offset value
+ *
+ * Get the TCC offset value to Tjmax. The effective thermal throttling or TCC
+ * activation temperature equals "Tjmax" - "TCC Offset", in degrees C.
+ *
+ * Return: On success returns 0, an error code otherwise
+ */
+
+int intel_tcc_get_offset(int cpu, int *offset)
+{
+	u32 eax, edx;
+	int err;
+
+	err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
+					&eax, &edx);
+	if (err)
+		return err;
+
+	*offset = (eax >> 24) & 0x3f;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
+
+/**
+ * intel_tcc_set_offset() - set the TCC offset value to Tjmax
+ * @cpu: cpu that the MSR should be run on.
+ * @offset: TCC offset value in degree C
+ *
+ * Set the TCC Offset value to Tjmax. The effective thermal throttling or TCC
+ * activation temperature equals "Tjmax" - "TCC Offset", in degree C.
+ *
+ * Return: On success returns 0, an error code otherwise
+ */
+
+int intel_tcc_set_offset(int cpu, int offset)
+{
+	u32 eax, edx;
+	int err;
+
+	if (offset > 0x3f)
+		return -EINVAL;
+
+	err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
+					&eax, &edx);
+	if (err)
+		return err;
+
+	if (eax & BIT(31))
+		return -EPERM;
+
+	eax &= ~(0x3f << 24);
+	eax |= (offset << 24);
+
+	return wrmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET, eax, edx);
+}
+EXPORT_SYMBOL_NS_GPL(intel_tcc_set_offset, INTEL_TCC);
+
+/**
+ * intel_tcc_get_temp() - returns the current temperature
+ * @cpu: cpu that the MSR should be run on.
+ * @pkg: true: Package Thermal Sensor. false: Core Thermal Sensor.
+ * @temp: a valid pointer to where to store the resulting temperature
+ *
+ * Get the current temperature returned by the CPU core/package level
+ * thermal sensor, in degrees C.
+ *
+ * Return: On success returns 0, an error code otherwise
+ */
+int intel_tcc_get_temp(int cpu, bool pkg, int *temp)
+{
+	u32 eax, edx;
+	u32 msr = pkg ? MSR_IA32_PACKAGE_THERM_STATUS : MSR_IA32_THERM_STATUS;
+	int tjmax, err;
+
+	err = intel_tcc_get_tjmax(cpu, &tjmax);
+	if (err)
+		return err;
+
+	err = rdmsr_safe_on_cpu(cpu, msr, &eax, &edx);
+	if (err)
+		return err;
+
+	if (eax & 0x80000000) {
+		*temp = tjmax - ((eax >> 16) & 0x7f);
+		return 0;
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL_NS_GPL(intel_tcc_get_temp, INTEL_TCC);
+
diff --git a/include/linux/intel_tcc.h b/include/linux/intel_tcc.h
new file mode 100644
index 000000000000..94f8ceab5dd0
--- /dev/null
+++ b/include/linux/intel_tcc.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  header for Intel TCC (thermal control circuitry) library
+ *
+ *  Copyright (C) 2022  Intel Corporation.
+ */
+
+#ifndef __INTEL_TCC_H__
+#define __INTEL_TCC_H__
+
+#include <linux/types.h>
+
+int intel_tcc_get_tjmax(int cpu, int *tjmax);
+int intel_tcc_get_offset(int cpu, int *offset);
+int intel_tcc_set_offset(int cpu, int offset);
+int intel_tcc_get_temp(int cpu, bool pkg, int *temp);
+
+#endif /* __INTEL_TCC_H__ */
-- 
2.25.1

[PATCH 2/6] thermal/int340x/processor_thermal: Use Intel TCC library

[Zhang Rui]

Cleanup the code by using Intel TCC library for TCC (Thermal Control
Circuitry) MSR access.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/intel/int340x_thermal/Kconfig |   1 +
 .../processor_thermal_device.c                | 123 ++++--------------
 2 files changed, 25 insertions(+), 99 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/Kconfig b/drivers/thermal/intel/int340x_thermal/Kconfig
index 5d046de96a5d..0f511917e0e1 100644
--- a/drivers/thermal/intel/int340x_thermal/Kconfig
+++ b/drivers/thermal/intel/int340x_thermal/Kconfig
@@ -10,6 +10,7 @@ config INT340X_THERMAL
 	select ACPI_THERMAL_REL
 	select ACPI_FAN
 	select INTEL_SOC_DTS_IOSF_CORE
+	select INTEL_TCC
 	select PROC_THERMAL_MMIO_RAPL if POWERCAP
 	help
 	  Newer laptops and tablets that use ACPI may have thermal sensors and
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
index a8d98f1bd6c6..a9e08dddb773 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2014, Intel Corporation.
  */
 #include <linux/acpi.h>
+#include <linux/intel_tcc.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -68,54 +69,17 @@ static const struct attribute_group power_limit_attribute_group = {
 	.name = "power_limits"
 };
 
-static int tcc_get_offset(void)
-{
-	u64 val;
-	int err;
-
-	err = rdmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, &val);
-	if (err)
-		return err;
-
-	return (val >> 24) & 0x3f;
-}
-
 static ssize_t tcc_offset_degree_celsius_show(struct device *dev,
 					      struct device_attribute *attr,
 					      char *buf)
 {
-	int tcc;
-
-	tcc = tcc_get_offset();
-	if (tcc < 0)
-		return tcc;
-
-	return sprintf(buf, "%d\n", tcc);
-}
-
-static int tcc_offset_update(unsigned int tcc)
-{
-	u64 val;
-	int err;
+	int offset, ret;
 
-	if (tcc > 63)
-		return -EINVAL;
-
-	err = rdmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, &val);
-	if (err)
-		return err;
-
-	if (val & BIT(31))
-		return -EPERM;
-
-	val &= ~GENMASK_ULL(29, 24);
-	val |= (tcc & 0x3f) << 24;
-
-	err = wrmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, val);
-	if (err)
-		return err;
+	ret = intel_tcc_get_offset(cpumask_any(cpu_online_mask), &offset);
+	if (ret < 0)
+		return ret;
 
-	return 0;
+	return sprintf(buf, "%d\n", offset);
 }
 
 static ssize_t tcc_offset_degree_celsius_store(struct device *dev,
@@ -136,7 +100,7 @@ static ssize_t tcc_offset_degree_celsius_store(struct device *dev,
 	if (kstrtouint(buf, 0, &tcc))
 		return -EINVAL;
 
-	err = tcc_offset_update(tcc);
+	err = intel_tcc_set_offset(cpumask_any(cpu_online_mask), tcc);
 	if (err)
 		return err;
 
@@ -145,66 +109,26 @@ static ssize_t tcc_offset_degree_celsius_store(struct device *dev,
 
 static DEVICE_ATTR_RW(tcc_offset_degree_celsius);
 
-static int stored_tjmax; /* since it is fixed, we can have local storage */
-
-static int get_tjmax(void)
-{
-	u32 eax, edx;
-	u32 val;
-	int err;
-
-	err = rdmsr_safe(MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
-	if (err)
-		return err;
-
-	val = (eax >> 16) & 0xff;
-	if (val)
-		return val;
-
-	return -EINVAL;
-}
-
-static int read_temp_msr(int *temp)
+static int proc_thermal_get_zone_temp(struct thermal_zone_device *zone,
+					 int *temp)
 {
 	int cpu;
-	u32 eax, edx;
 	int err;
-	unsigned long curr_temp_off = 0;
+	int curr_temp_off;
 
 	*temp = 0;
 
 	for_each_online_cpu(cpu) {
-		err = rdmsr_safe_on_cpu(cpu, MSR_IA32_THERM_STATUS, &eax,
-					&edx);
+		err = intel_tcc_get_temp(cpu, false, &curr_temp_off);
 		if (err)
-			goto err_ret;
-		else {
-			if (eax & 0x80000000) {
-				curr_temp_off = (eax >> 16) & 0x7f;
-				if (!*temp || curr_temp_off < *temp)
-					*temp = curr_temp_off;
-			} else {
-				err = -EINVAL;
-				goto err_ret;
-			}
-		}
+			return err;
+		if (!*temp || curr_temp_off > *temp)
+			*temp = curr_temp_off;
 	}
 
-	return 0;
-err_ret:
-	return err;
-}
+	*temp *= 1000;
 
-static int proc_thermal_get_zone_temp(struct thermal_zone_device *zone,
-					 int *temp)
-{
-	int ret;
-
-	ret = read_temp_msr(temp);
-	if (!ret)
-		*temp = (stored_tjmax - *temp) * 1000;
-
-	return ret;
+	return 0;
 }
 
 static struct thermal_zone_device_ops proc_thermal_local_ops = {
@@ -286,7 +210,7 @@ int proc_thermal_add(struct device *dev, struct proc_thermal_device *proc_priv)
 	acpi_status status;
 	unsigned long long tmp;
 	struct thermal_zone_device_ops *ops = NULL;
-	int ret;
+	int tjmax, ret;
 
 	adev = ACPI_COMPANION(dev);
 	if (!adev)
@@ -302,8 +226,7 @@ int proc_thermal_add(struct device *dev, struct proc_thermal_device *proc_priv)
 	status = acpi_evaluate_integer(adev->handle, "_TMP", NULL, &tmp);
 	if (ACPI_FAILURE(status)) {
 		/* there is no _TMP method, add local method */
-		stored_tjmax = get_tjmax();
-		if (stored_tjmax > 0)
+		if (!intel_tcc_get_tjmax(cpumask_any(cpu_online_mask), &tjmax))
 			ops = &proc_thermal_local_ops;
 	}
 
@@ -356,9 +279,10 @@ static int tcc_offset_save = -1;
 
 int proc_thermal_suspend(struct device *dev)
 {
-	tcc_offset_save = tcc_get_offset();
-	if (tcc_offset_save < 0)
-		dev_warn(dev, "failed to save offset (%d)\n", tcc_offset_save);
+	if (intel_tcc_get_offset(cpumask_any(cpu_online_mask), &tcc_offset_save)) {
+		dev_warn(dev, "failed to save offset\n");
+		tcc_offset_save = -1;
+	}
 
 	return 0;
 }
@@ -373,7 +297,7 @@ int proc_thermal_resume(struct device *dev)
 
 	/* Do not update if saving failed */
 	if (tcc_offset_save >= 0)
-		tcc_offset_update(tcc_offset_save);
+		intel_tcc_set_offset(cpumask_any(cpu_online_mask), tcc_offset_save);
 
 	return 0;
 }
@@ -460,6 +384,7 @@ void proc_thermal_mmio_remove(struct pci_dev *pdev, struct proc_thermal_device *
 }
 EXPORT_SYMBOL_GPL(proc_thermal_mmio_remove);
 
+MODULE_IMPORT_NS(INTEL_TCC);
 MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>");
 MODULE_DESCRIPTION("Processor Thermal Reporting Device Driver");
 MODULE_LICENSE("GPL v2");
-- 
2.25.1

[PATCH 3/6] thermal/intel/intel_soc_dts_iosf: Use Intel TCC library

[Zhang Rui]

Cleanup the code by using Intel TCC library for TCC (Thermal Control
Circuitry) MSR access.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/intel/Kconfig              |  1 +
 drivers/thermal/intel/intel_soc_dts_iosf.c | 33 ++++------------------
 2 files changed, 6 insertions(+), 28 deletions(-)

diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
index 6b938c040d6e..329c0ee934c4 100644
--- a/drivers/thermal/intel/Kconfig
+++ b/drivers/thermal/intel/Kconfig
@@ -32,6 +32,7 @@ config INTEL_SOC_DTS_IOSF_CORE
 	tristate
 	depends on X86 && PCI
 	select IOSF_MBI
+	select INTEL_TCC
 	help
 	  This is becoming a common feature for Intel SoCs to expose the additional
 	  digital temperature sensors (DTSs) using side band interface (IOSF). This
diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
index 342b0bb5a56d..87fc45b8f8c6 100644
--- a/drivers/thermal/intel/intel_soc_dts_iosf.c
+++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
@@ -7,6 +7,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/bitops.h>
+#include <linux/intel_tcc.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
@@ -45,32 +46,6 @@
 /* DTS0 and DTS 1 */
 #define SOC_MAX_DTS_SENSORS		2
 
-static int get_tj_max(u32 *tj_max)
-{
-	u32 eax, edx;
-	u32 val;
-	int err;
-
-	err = rdmsr_safe(MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
-	if (err)
-		goto err_ret;
-	else {
-		val = (eax >> 16) & 0xff;
-		if (val)
-			*tj_max = val * 1000;
-		else {
-			err = -EINVAL;
-			goto err_ret;
-		}
-	}
-
-	return 0;
-err_ret:
-	*tj_max = 0;
-
-	return err;
-}
-
 static int sys_get_trip_temp(struct thermal_zone_device *tzd, int trip,
 			     int *temp)
 {
@@ -415,8 +390,9 @@ struct intel_soc_dts_sensors *intel_soc_dts_iosf_init(
 	if (!trip_count || read_only_trip_count > trip_count)
 		return ERR_PTR(-EINVAL);
 
-	if (get_tj_max(&tj_max))
-		return ERR_PTR(-EINVAL);
+	ret = intel_tcc_get_tjmax(cpumask_any(cpu_online_mask), &tj_max);
+	if (ret)
+		return ERR_PTR(ret);
 
 	sensors = kzalloc(sizeof(*sensors), GFP_KERNEL);
 	if (!sensors)
@@ -475,4 +451,5 @@ void intel_soc_dts_iosf_exit(struct intel_soc_dts_sensors *sensors)
 }
 EXPORT_SYMBOL_GPL(intel_soc_dts_iosf_exit);
 
+MODULE_IMPORT_NS(INTEL_TCC);
 MODULE_LICENSE("GPL v2");
-- 
2.25.1

[PATCH 4/6] thermal/intel/intel_tcc_cooling: Use Intel TCC library

[Zhang Rui]

Cleanup the code by using Intel TCC library for TCC (Thermal Control
Circuitry) MSR access.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/intel/Kconfig             |  1 +
 drivers/thermal/intel/intel_tcc_cooling.c | 37 +++--------------------
 2 files changed, 6 insertions(+), 32 deletions(-)

diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
index 329c0ee934c4..dafdb3dd3fc7 100644
--- a/drivers/thermal/intel/Kconfig
+++ b/drivers/thermal/intel/Kconfig
@@ -88,6 +88,7 @@ config INTEL_PCH_THERMAL
 config INTEL_TCC_COOLING
 	tristate "Intel TCC offset cooling Driver"
 	depends on X86
+	select INTEL_TCC
 	help
 	  Enable this to support system cooling by adjusting the effective TCC
 	  activation temperature via the TCC Offset register, which is widely
diff --git a/drivers/thermal/intel/intel_tcc_cooling.c b/drivers/thermal/intel/intel_tcc_cooling.c
index 95adac427b6f..8819afb7f23c 100644
--- a/drivers/thermal/intel/intel_tcc_cooling.c
+++ b/drivers/thermal/intel/intel_tcc_cooling.c
@@ -7,12 +7,11 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/device.h>
+#include <linux/intel_tcc.h>
 #include <linux/module.h>
 #include <linux/thermal.h>
 #include <asm/cpu_device_id.h>
 
-#define TCC_SHIFT 24
-#define TCC_MASK	(0x3fULL<<24)
 #define TCC_PROGRAMMABLE	BIT(30)
 
 static struct thermal_cooling_device *tcc_cdev;
@@ -20,47 +19,20 @@ static struct thermal_cooling_device *tcc_cdev;
 static int tcc_get_max_state(struct thermal_cooling_device *cdev, unsigned long
 			     *state)
 {
-	*state = TCC_MASK >> TCC_SHIFT;
-	return 0;
-}
-
-static int tcc_offset_update(int tcc)
-{
-	u64 val;
-	int err;
-
-	err = rdmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, &val);
-	if (err)
-		return err;
-
-	val &= ~TCC_MASK;
-	val |= tcc << TCC_SHIFT;
-
-	err = wrmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, val);
-	if (err)
-		return err;
-
+	*state = 0x3f;
 	return 0;
 }
 
 static int tcc_get_cur_state(struct thermal_cooling_device *cdev, unsigned long
 			     *state)
 {
-	u64 val;
-	int err;
-
-	err = rdmsrl_safe(MSR_IA32_TEMPERATURE_TARGET, &val);
-	if (err)
-		return err;
-
-	*state = (val & TCC_MASK) >> TCC_SHIFT;
-	return 0;
+	return intel_tcc_get_offset(cpumask_any(cpu_online_mask), (int *)state);
 }
 
 static int tcc_set_cur_state(struct thermal_cooling_device *cdev, unsigned long
 			     state)
 {
-	return tcc_offset_update(state);
+	return intel_tcc_set_offset(cpumask_any(cpu_online_mask), (int)state);
 }
 
 static const struct thermal_cooling_device_ops tcc_cooling_ops = {
@@ -129,6 +101,7 @@ static void __exit tcc_cooling_exit(void)
 
 module_exit(tcc_cooling_exit)
 
+MODULE_IMPORT_NS(INTEL_TCC);
 MODULE_DESCRIPTION("TCC offset cooling device Driver");
 MODULE_AUTHOR("Zhang Rui <rui.zhang@intel.com>");
 MODULE_LICENSE("GPL v2");
-- 
2.25.1

[PATCH 5/6] thermal/x86_pkg_temp_thermal: Use Intel TCC library

[Zhang Rui]

Cleanup the code by using Intel TCC library for TCC (Thermal Control
Circuitry) MSR access.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/intel/Kconfig                |  1 +
 drivers/thermal/intel/x86_pkg_temp_thermal.c | 40 ++++++--------------
 2 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
index dafdb3dd3fc7..fd41c810629b 100644
--- a/drivers/thermal/intel/Kconfig
+++ b/drivers/thermal/intel/Kconfig
@@ -21,6 +21,7 @@ config X86_PKG_TEMP_THERMAL
 	depends on X86_THERMAL_VECTOR
 	select THERMAL_GOV_USER_SPACE
 	select THERMAL_WRITABLE_TRIPS
+	select INTEL_TCC
 	default m
 	help
 	  Enable this to register CPU digital sensor for package temperature as
diff --git a/drivers/thermal/intel/x86_pkg_temp_thermal.c b/drivers/thermal/intel/x86_pkg_temp_thermal.c
index a0e234fce71a..cfe905735c62 100644
--- a/drivers/thermal/intel/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/intel/x86_pkg_temp_thermal.c
@@ -7,6 +7,7 @@
 
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/intel_tcc.h>
 #include <linux/err.h>
 #include <linux/param.h>
 #include <linux/device.h>
@@ -104,38 +105,18 @@ static struct zone_device *pkg_temp_thermal_get_dev(unsigned int cpu)
 	return NULL;
 }
 
-/*
-* tj-max is interesting because threshold is set relative to this
-* temperature.
-*/
-static int get_tj_max(int cpu, u32 *tj_max)
-{
-	u32 eax, edx, val;
-	int err;
-
-	err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
-	if (err)
-		return err;
-
-	val = (eax >> 16) & 0xff;
-	*tj_max = val * 1000;
-
-	return val ? 0 : -EINVAL;
-}
-
 static int sys_get_curr_temp(struct thermal_zone_device *tzd, int *temp)
 {
 	struct zone_device *zonedev = tzd->devdata;
-	u32 eax, edx;
+	int ret;
 
-	rdmsr_on_cpu(zonedev->cpu, MSR_IA32_PACKAGE_THERM_STATUS,
-			&eax, &edx);
-	if (eax & 0x80000000) {
-		*temp = zonedev->tj_max - ((eax >> 16) & 0x7f) * 1000;
-		pr_debug("sys_get_curr_temp %d\n", *temp);
-		return 0;
-	}
-	return -EINVAL;
+	ret = intel_tcc_get_temp(zonedev->cpu, true, temp);
+	if (ret)
+		return ret;
+
+	*temp *= 1000;
+	pr_debug("sys_get_curr_temp %d\n", *temp);
+	return 0;
 }
 
 static int sys_get_trip_temp(struct thermal_zone_device *tzd,
@@ -345,7 +326,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
 
 	thres_count = clamp_val(thres_count, 0, MAX_NUMBER_OF_TRIPS);
 
-	err = get_tj_max(cpu, &tj_max);
+	err = intel_tcc_get_tjmax(cpu, &tj_max);
 	if (err)
 		return err;
 
@@ -536,6 +517,7 @@ static void __exit pkg_temp_thermal_exit(void)
 }
 module_exit(pkg_temp_thermal_exit)
 
+MODULE_IMPORT_NS(INTEL_TCC);
 MODULE_DESCRIPTION("X86 PKG TEMP Thermal Driver");
 MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>");
 MODULE_LICENSE("GPL v2");
-- 
2.25.1

[PATCH 6/6] thermal/x86_pkg_temp_thermal: Add support for handling dynamic tjmax

[Zhang Rui]

Tjmax value retrieved from MSR_IA32_TEMPERATURE_TARGET can be changed at
runtime when the Intel SST-PP (Intel Speed Select Technology -
Performance Profile) level is changed.

Enhance the code to use updated tjmax when programming the thermal
interrupt thresholds.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/intel/x86_pkg_temp_thermal.c | 22 +++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/intel/x86_pkg_temp_thermal.c b/drivers/thermal/intel/x86_pkg_temp_thermal.c
index cfe905735c62..9a9866a602ec 100644
--- a/drivers/thermal/intel/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/intel/x86_pkg_temp_thermal.c
@@ -49,7 +49,6 @@ MODULE_PARM_DESC(notify_delay_ms,
 struct zone_device {
 	int				cpu;
 	bool				work_scheduled;
-	u32				tj_max;
 	u32				msr_pkg_therm_low;
 	u32				msr_pkg_therm_high;
 	struct delayed_work		work;
@@ -125,7 +124,7 @@ static int sys_get_trip_temp(struct thermal_zone_device *tzd,
 	struct zone_device *zonedev = tzd->devdata;
 	unsigned long thres_reg_value;
 	u32 mask, shift, eax, edx;
-	int ret;
+	int tj_max, ret;
 
 	if (trip >= MAX_NUMBER_OF_TRIPS)
 		return -EINVAL;
@@ -138,6 +137,11 @@ static int sys_get_trip_temp(struct thermal_zone_device *tzd,
 		shift = THERM_SHIFT_THRESHOLD0;
 	}
 
+	ret = intel_tcc_get_tjmax(zonedev->cpu, &tj_max);
+	if (ret)
+		return ret;
+	tj_max *= 1000;
+
 	ret = rdmsr_on_cpu(zonedev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
 			   &eax, &edx);
 	if (ret < 0)
@@ -145,7 +149,7 @@ static int sys_get_trip_temp(struct thermal_zone_device *tzd,
 
 	thres_reg_value = (eax & mask) >> shift;
 	if (thres_reg_value)
-		*temp = zonedev->tj_max - thres_reg_value * 1000;
+		*temp = tj_max - thres_reg_value * 1000;
 	else
 		*temp = THERMAL_TEMP_INVALID;
 	pr_debug("sys_get_trip_temp %d\n", *temp);
@@ -158,9 +162,14 @@ sys_set_trip_temp(struct thermal_zone_device *tzd, int trip, int temp)
 {
 	struct zone_device *zonedev = tzd->devdata;
 	u32 l, h, mask, shift, intr;
-	int ret;
+	int tj_max, ret;
+
+	ret = intel_tcc_get_tjmax(zonedev->cpu, &tj_max);
+	if (ret)
+		return ret;
+	tj_max *= 1000;
 
-	if (trip >= MAX_NUMBER_OF_TRIPS || temp >= zonedev->tj_max)
+	if (trip >= MAX_NUMBER_OF_TRIPS || temp >= tj_max)
 		return -EINVAL;
 
 	ret = rdmsr_on_cpu(zonedev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
@@ -185,7 +194,7 @@ sys_set_trip_temp(struct thermal_zone_device *tzd, int trip, int temp)
 	if (!temp) {
 		l &= ~intr;
 	} else {
-		l |= (zonedev->tj_max - temp)/1000 << shift;
+		l |= (tj_max - temp)/1000 << shift;
 		l |= intr;
 	}
 
@@ -336,7 +345,6 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
 
 	INIT_DELAYED_WORK(&zonedev->work, pkg_temp_thermal_threshold_work_fn);
 	zonedev->cpu = cpu;
-	zonedev->tj_max = tj_max;
 	zonedev->tzone = thermal_zone_device_register("x86_pkg_temp",
 			thres_count,
 			(thres_count == MAX_NUMBER_OF_TRIPS) ? 0x03 : 0x01,
-- 
2.25.1

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Rafael J. Wysocki]

On Tue, Nov 8, 2022 at 4:31 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> There are several different drivers that accesses the Intel TCC
> (thermal control circuitry) MSRs, and each of them has its own
> implementation for the same functionalities, e.g. getting the current
> temperature, getting the tj_max, and getting/setting the tj_max offset.
>
> Introduce a library to unify the code for Intel CPU TCC MSR access.
>
> At the same time, ensure the temperature is got based on the updated
> tjmax value because tjmax can be changed at runtime for cases like
> the Intel SST-PP (Intel Speed Select Technology - Performance Profile)
> level change.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Nice series, overall I like it a lot, but I have some comments
regarding this particular patch (below).  Some of them are arguably
minor, but at least one thing is more serious.

> ---
>  drivers/thermal/intel/Kconfig     |   4 +
>  drivers/thermal/intel/Makefile    |   1 +
>  drivers/thermal/intel/intel_tcc.c | 131 ++++++++++++++++++++++++++++++
>  include/linux/intel_tcc.h         |  18 ++++
>  4 files changed, 154 insertions(+)
>  create mode 100644 drivers/thermal/intel/intel_tcc.c
>  create mode 100644 include/linux/intel_tcc.h
>
> diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
> index f0c845679250..6b938c040d6e 100644
> --- a/drivers/thermal/intel/Kconfig
> +++ b/drivers/thermal/intel/Kconfig
> @@ -12,6 +12,10 @@ config X86_THERMAL_VECTOR
>         def_bool y
>         depends on X86 && CPU_SUP_INTEL && X86_LOCAL_APIC
>
> +config INTEL_TCC
> +       bool
> +       depends on X86
> +
>  config X86_PKG_TEMP_THERMAL
>         tristate "X86 package temperature thermal driver"
>         depends on X86_THERMAL_VECTOR
> diff --git a/drivers/thermal/intel/Makefile b/drivers/thermal/intel/Makefile
> index 9a8d8054f316..5d8833c82ab6 100644
> --- a/drivers/thermal/intel/Makefile
> +++ b/drivers/thermal/intel/Makefile
> @@ -2,6 +2,7 @@
>  #
>  # Makefile for various Intel thermal drivers.
>
> +obj-$(CONFIG_INTEL_TCC)        += intel_tcc.o
>  obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o
>  obj-$(CONFIG_X86_PKG_TEMP_THERMAL)     += x86_pkg_temp_thermal.o
>  obj-$(CONFIG_INTEL_SOC_DTS_IOSF_CORE)  += intel_soc_dts_iosf.o
> diff --git a/drivers/thermal/intel/intel_tcc.c b/drivers/thermal/intel/intel_tcc.c
> new file mode 100644
> index 000000000000..74b434914975
> --- /dev/null
> +++ b/drivers/thermal/intel/intel_tcc.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * intel_tcc.c - Library for Intel TCC (thermal control circuitry) MSR access
> + * Copyright (c) 2022, Intel Corporation.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/intel_tcc.h>
> +#include <asm/msr.h>
> +
> +/**
> + * intel_tcc_get_tjmax() - returns the default TCC activation Temperature
> + * @cpu: cpu that the MSR should be run on.
> + * @tjmax: a valid pointer to where to store the Tjmax value
> + *
> + * Get the TjMax value, which is the default thermal throttling or TCC
> + * activation temperature in degrees C.
> + *
> + * Return: On success returns 0, an error code otherwise
> + */
> +

This extra empty line is not needed (and not desirable even).  And same below.

> +int intel_tcc_get_tjmax(int cpu, int *tjmax)
> +{
> +       u32 eax, edx;
> +       int err;
> +
> +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> +                                       &eax, &edx);

The current trend is to align the arguments after a line break with
the first one, but I would just put them all on the same line (and
below too).

> +       if (err)
> +               return err;
> +
> +       *tjmax = (eax >> 16) & 0xff;

This means that the tjmax value cannot be negative.

> +
> +       return *tjmax ? 0 : -EINVAL;

So the return value of this function could be tjmax if positive or a
negative error code otherwise.  No return pointers needed.

And why do you want to return -EINVAL (rather than any other error
code) if tjmax turns out to be 0?

> +}
> +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_tjmax, INTEL_TCC);
> +
> +/**
> + * intel_tcc_get_offset() - returns the TCC Offset value to Tjmax
> + * @cpu: cpu that the MSR should be run on.
> + * @offset: a valid pointer to where to store the offset value
> + *
> + * Get the TCC offset value to Tjmax. The effective thermal throttling or TCC
> + * activation temperature equals "Tjmax" - "TCC Offset", in degrees C.
> + *
> + * Return: On success returns 0, an error code otherwise
> + */
> +
> +int intel_tcc_get_offset(int cpu, int *offset)
> +{
> +       u32 eax, edx;
> +       int err;
> +
> +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> +                                       &eax, &edx);
> +       if (err)
> +               return err;
> +
> +       *offset = (eax >> 24) & 0x3f;

Well, offset cannot be negative here, so (again) the return value of
this function could be interpreted as the offsent (if non-negative) or
a negative error code on failure.

> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> +
> +/**
> + * intel_tcc_set_offset() - set the TCC offset value to Tjmax
> + * @cpu: cpu that the MSR should be run on.
> + * @offset: TCC offset value in degree C

I think that this cannot be negative, so maybe say "in K" instead of
"in degree C"?

And maybe it's better to pass u8 here?

> + *
> + * Set the TCC Offset value to Tjmax. The effective thermal throttling or TCC
> + * activation temperature equals "Tjmax" - "TCC Offset", in degree C.
> + *
> + * Return: On success returns 0, an error code otherwise
> + */
> +
> +int intel_tcc_set_offset(int cpu, int offset)
> +{
> +       u32 eax, edx;
> +       int err;
> +
> +       if (offset > 0x3f)
> +               return -EINVAL;
> +
> +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> +                                       &eax, &edx);
> +       if (err)
> +               return err;
> +
> +       if (eax & BIT(31))
> +               return -EPERM;

Why -EPERM?

> +
> +       eax &= ~(0x3f << 24);
> +       eax |= (offset << 24);

The parens are not needed AFAICS.

> +
> +       return wrmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET, eax, edx);

So is any protection against concurrent access needed here?  Like what
if two different callers invoke this function at the same time for the
same CPU?

> +}
> +EXPORT_SYMBOL_NS_GPL(intel_tcc_set_offset, INTEL_TCC);
> +
> +/**
> + * intel_tcc_get_temp() - returns the current temperature
> + * @cpu: cpu that the MSR should be run on.
> + * @pkg: true: Package Thermal Sensor. false: Core Thermal Sensor.
> + * @temp: a valid pointer to where to store the resulting temperature
> + *
> + * Get the current temperature returned by the CPU core/package level
> + * thermal sensor, in degrees C.
> + *
> + * Return: On success returns 0, an error code otherwise
> + */
> +int intel_tcc_get_temp(int cpu, bool pkg, int *temp)
> +{
> +       u32 eax, edx;
> +       u32 msr = pkg ? MSR_IA32_PACKAGE_THERM_STATUS : MSR_IA32_THERM_STATUS;
> +       int tjmax, err;
> +
> +       err = intel_tcc_get_tjmax(cpu, &tjmax);
> +       if (err)
> +               return err;

Well, what if somebody change tjmax on this cpu while this function is running?

> +
> +       err = rdmsr_safe_on_cpu(cpu, msr, &eax, &edx);
> +       if (err)
> +               return err;
> +
> +       if (eax & 0x80000000) {
> +               *temp = tjmax - ((eax >> 16) & 0x7f);
> +               return 0;
> +       }
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_temp, INTEL_TCC);
> +
> diff --git a/include/linux/intel_tcc.h b/include/linux/intel_tcc.h
> new file mode 100644
> index 000000000000..94f8ceab5dd0
> --- /dev/null
> +++ b/include/linux/intel_tcc.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *  header for Intel TCC (thermal control circuitry) library
> + *
> + *  Copyright (C) 2022  Intel Corporation.
> + */
> +
> +#ifndef __INTEL_TCC_H__
> +#define __INTEL_TCC_H__
> +
> +#include <linux/types.h>
> +
> +int intel_tcc_get_tjmax(int cpu, int *tjmax);
> +int intel_tcc_get_offset(int cpu, int *offset);
> +int intel_tcc_set_offset(int cpu, int offset);
> +int intel_tcc_get_temp(int cpu, bool pkg, int *temp);
> +
> +#endif /* __INTEL_TCC_H__ */
> --

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Rafael J. Wysocki]

On Thu, Dec 8, 2022 at 2:49 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Nov 8, 2022 at 4:31 AM Zhang Rui <rui.zhang@intel.com> wrote:
> >
> > There are several different drivers that accesses the Intel TCC
> > (thermal control circuitry) MSRs, and each of them has its own
> > implementation for the same functionalities, e.g. getting the current
> > temperature, getting the tj_max, and getting/setting the tj_max offset.
> >
> > Introduce a library to unify the code for Intel CPU TCC MSR access.
> >
> > At the same time, ensure the temperature is got based on the updated
> > tjmax value because tjmax can be changed at runtime for cases like
> > the Intel SST-PP (Intel Speed Select Technology - Performance Profile)
> > level change.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>
> Nice series, overall I like it a lot, but I have some comments
> regarding this particular patch (below).  Some of them are arguably
> minor, but at least one thing is more serious.
>
> > ---
> >  drivers/thermal/intel/Kconfig     |   4 +
> >  drivers/thermal/intel/Makefile    |   1 +
> >  drivers/thermal/intel/intel_tcc.c | 131 ++++++++++++++++++++++++++++++
> >  include/linux/intel_tcc.h         |  18 ++++
> >  4 files changed, 154 insertions(+)
> >  create mode 100644 drivers/thermal/intel/intel_tcc.c
> >  create mode 100644 include/linux/intel_tcc.h
> >
> > diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
> > index f0c845679250..6b938c040d6e 100644
> > --- a/drivers/thermal/intel/Kconfig
> > +++ b/drivers/thermal/intel/Kconfig
> > @@ -12,6 +12,10 @@ config X86_THERMAL_VECTOR
> >         def_bool y
> >         depends on X86 && CPU_SUP_INTEL && X86_LOCAL_APIC
> >
> > +config INTEL_TCC
> > +       bool
> > +       depends on X86
> > +
> >  config X86_PKG_TEMP_THERMAL
> >         tristate "X86 package temperature thermal driver"
> >         depends on X86_THERMAL_VECTOR
> > diff --git a/drivers/thermal/intel/Makefile b/drivers/thermal/intel/Makefile
> > index 9a8d8054f316..5d8833c82ab6 100644
> > --- a/drivers/thermal/intel/Makefile
> > +++ b/drivers/thermal/intel/Makefile
> > @@ -2,6 +2,7 @@
> >  #
> >  # Makefile for various Intel thermal drivers.
> >
> > +obj-$(CONFIG_INTEL_TCC)        += intel_tcc.o
> >  obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o
> >  obj-$(CONFIG_X86_PKG_TEMP_THERMAL)     += x86_pkg_temp_thermal.o
> >  obj-$(CONFIG_INTEL_SOC_DTS_IOSF_CORE)  += intel_soc_dts_iosf.o
> > diff --git a/drivers/thermal/intel/intel_tcc.c b/drivers/thermal/intel/intel_tcc.c
> > new file mode 100644
> > index 000000000000..74b434914975
> > --- /dev/null
> > +++ b/drivers/thermal/intel/intel_tcc.c
> > @@ -0,0 +1,131 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * intel_tcc.c - Library for Intel TCC (thermal control circuitry) MSR access
> > + * Copyright (c) 2022, Intel Corporation.
> > + */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/intel_tcc.h>
> > +#include <asm/msr.h>
> > +
> > +/**
> > + * intel_tcc_get_tjmax() - returns the default TCC activation Temperature
> > + * @cpu: cpu that the MSR should be run on.
> > + * @tjmax: a valid pointer to where to store the Tjmax value
> > + *
> > + * Get the TjMax value, which is the default thermal throttling or TCC
> > + * activation temperature in degrees C.
> > + *
> > + * Return: On success returns 0, an error code otherwise
> > + */
> > +
>
> This extra empty line is not needed (and not desirable even).  And same below.
>
> > +int intel_tcc_get_tjmax(int cpu, int *tjmax)
> > +{
> > +       u32 eax, edx;
> > +       int err;
> > +
> > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > +                                       &eax, &edx);
>
> The current trend is to align the arguments after a line break with
> the first one, but I would just put them all on the same line (and
> below too).
>
> > +       if (err)
> > +               return err;
> > +
> > +       *tjmax = (eax >> 16) & 0xff;
>
> This means that the tjmax value cannot be negative.
>
> > +
> > +       return *tjmax ? 0 : -EINVAL;
>
> So the return value of this function could be tjmax if positive or a
> negative error code otherwise.  No return pointers needed.
>
> And why do you want to return -EINVAL (rather than any other error
> code) if tjmax turns out to be 0?
>
> > +}
> > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_tjmax, INTEL_TCC);
> > +
> > +/**
> > + * intel_tcc_get_offset() - returns the TCC Offset value to Tjmax
> > + * @cpu: cpu that the MSR should be run on.
> > + * @offset: a valid pointer to where to store the offset value
> > + *
> > + * Get the TCC offset value to Tjmax. The effective thermal throttling or TCC
> > + * activation temperature equals "Tjmax" - "TCC Offset", in degrees C.
> > + *
> > + * Return: On success returns 0, an error code otherwise
> > + */
> > +
> > +int intel_tcc_get_offset(int cpu, int *offset)
> > +{
> > +       u32 eax, edx;
> > +       int err;
> > +
> > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > +                                       &eax, &edx);
> > +       if (err)
> > +               return err;
> > +
> > +       *offset = (eax >> 24) & 0x3f;
>
> Well, offset cannot be negative here, so (again) the return value of
> this function could be interpreted as the offsent (if non-negative) or
> a negative error code on failure.
>
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> > +
> > +/**
> > + * intel_tcc_set_offset() - set the TCC offset value to Tjmax
> > + * @cpu: cpu that the MSR should be run on.
> > + * @offset: TCC offset value in degree C
>
> I think that this cannot be negative, so maybe say "in K" instead of
> "in degree C"?
>
> And maybe it's better to pass u8 here?
>
> > + *
> > + * Set the TCC Offset value to Tjmax. The effective thermal throttling or TCC
> > + * activation temperature equals "Tjmax" - "TCC Offset", in degree C.
> > + *
> > + * Return: On success returns 0, an error code otherwise
> > + */
> > +
> > +int intel_tcc_set_offset(int cpu, int offset)
> > +{
> > +       u32 eax, edx;
> > +       int err;
> > +
> > +       if (offset > 0x3f)
> > +               return -EINVAL;
> > +
> > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > +                                       &eax, &edx);
> > +       if (err)
> > +               return err;
> > +
> > +       if (eax & BIT(31))
> > +               return -EPERM;
>
> Why -EPERM?
>
> > +
> > +       eax &= ~(0x3f << 24);
> > +       eax |= (offset << 24);
>
> The parens are not needed AFAICS.
>
> > +
> > +       return wrmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET, eax, edx);
>
> So is any protection against concurrent access needed here?  Like what
> if two different callers invoke this function at the same time for the
> same CPU?
>
> > +}
> > +EXPORT_SYMBOL_NS_GPL(intel_tcc_set_offset, INTEL_TCC);
> > +
> > +/**
> > + * intel_tcc_get_temp() - returns the current temperature
> > + * @cpu: cpu that the MSR should be run on.
> > + * @pkg: true: Package Thermal Sensor. false: Core Thermal Sensor.
> > + * @temp: a valid pointer to where to store the resulting temperature
> > + *
> > + * Get the current temperature returned by the CPU core/package level
> > + * thermal sensor, in degrees C.
> > + *
> > + * Return: On success returns 0, an error code otherwise
> > + */
> > +int intel_tcc_get_temp(int cpu, bool pkg, int *temp)
> > +{
> > +       u32 eax, edx;
> > +       u32 msr = pkg ? MSR_IA32_PACKAGE_THERM_STATUS : MSR_IA32_THERM_STATUS;
> > +       int tjmax, err;
> > +
> > +       err = intel_tcc_get_tjmax(cpu, &tjmax);
> > +       if (err)
> > +               return err;
>
> Well, what if somebody change tjmax on this cpu while this function is running?

Sorry, scratch this.  The offset can be updated, not tjmax.

But can tjmax change anyway or is it just a property of the hardware?

> > +
> > +       err = rdmsr_safe_on_cpu(cpu, msr, &eax, &edx);
> > +       if (err)
> > +               return err;
> > +
> > +       if (eax & 0x80000000) {
> > +               *temp = tjmax - ((eax >> 16) & 0x7f);
> > +               return 0;
> > +       }
> > +       return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_temp, INTEL_TCC);

And one more thing, but rather for the future.

Reading from these MSRs can be costly, especially when done on a
remote CPU, so would it make sense to cache the retrieved values in
case they are read relatively often?

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Zhang Rui]

On Thu, 2022-12-08 at 14:49 +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 8, 2022 at 4:31 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > There are several different drivers that accesses the Intel TCC
> > (thermal control circuitry) MSRs, and each of them has its own
> > implementation for the same functionalities, e.g. getting the
> > current
> > temperature, getting the tj_max, and getting/setting the tj_max
> > offset.
> > 
> > Introduce a library to unify the code for Intel CPU TCC MSR access.
> > 
> > At the same time, ensure the temperature is got based on the
> > updated
> > tjmax value because tjmax can be changed at runtime for cases like
> > the Intel SST-PP (Intel Speed Select Technology - Performance
> > Profile)
> > level change.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> Nice series, overall I like it a lot, but I have some comments
> regarding this particular patch (below).  Some of them are arguably
> minor, but at least one thing is more serious.

Hi, Rafael,

Thanks for reviewing the patch set.
> 
> > ---
> >  drivers/thermal/intel/Kconfig     |   4 +
> >  drivers/thermal/intel/Makefile    |   1 +
> >  drivers/thermal/intel/intel_tcc.c | 131
> > ++++++++++++++++++++++++++++++
> >  include/linux/intel_tcc.h         |  18 ++++
> >  4 files changed, 154 insertions(+)
> >  create mode 100644 drivers/thermal/intel/intel_tcc.c
> >  create mode 100644 include/linux/intel_tcc.h
> > 
> > diff --git a/drivers/thermal/intel/Kconfig
> > b/drivers/thermal/intel/Kconfig
> > index f0c845679250..6b938c040d6e 100644
> > --- a/drivers/thermal/intel/Kconfig
> > +++ b/drivers/thermal/intel/Kconfig
> > @@ -12,6 +12,10 @@ config X86_THERMAL_VECTOR
> >         def_bool y
> >         depends on X86 && CPU_SUP_INTEL && X86_LOCAL_APIC
> > 
> > +config INTEL_TCC
> > +       bool
> > +       depends on X86
> > +
> >  config X86_PKG_TEMP_THERMAL
> >         tristate "X86 package temperature thermal driver"
> >         depends on X86_THERMAL_VECTOR
> > diff --git a/drivers/thermal/intel/Makefile
> > b/drivers/thermal/intel/Makefile
> > index 9a8d8054f316..5d8833c82ab6 100644
> > --- a/drivers/thermal/intel/Makefile
> > +++ b/drivers/thermal/intel/Makefile
> > @@ -2,6 +2,7 @@
> >  #
> >  # Makefile for various Intel thermal drivers.
> > 
> > +obj-$(CONFIG_INTEL_TCC)        += intel_tcc.o
> >  obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o
> >  obj-$(CONFIG_X86_PKG_TEMP_THERMAL)     += x86_pkg_temp_thermal.o
> >  obj-$(CONFIG_INTEL_SOC_DTS_IOSF_CORE)  += intel_soc_dts_iosf.o
> > diff --git a/drivers/thermal/intel/intel_tcc.c
> > b/drivers/thermal/intel/intel_tcc.c
> > new file mode 100644
> > index 000000000000..74b434914975
> > --- /dev/null
> > +++ b/drivers/thermal/intel/intel_tcc.c
> > @@ -0,0 +1,131 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * intel_tcc.c - Library for Intel TCC (thermal control circuitry)
> > MSR access
> > + * Copyright (c) 2022, Intel Corporation.
> > + */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/intel_tcc.h>
> > +#include <asm/msr.h>
> > +
> > +/**
> > + * intel_tcc_get_tjmax() - returns the default TCC activation
> > Temperature
> > + * @cpu: cpu that the MSR should be run on.
> > + * @tjmax: a valid pointer to where to store the Tjmax value
> > + *
> > + * Get the TjMax value, which is the default thermal throttling or
> > TCC
> > + * activation temperature in degrees C.
> > + *
> > + * Return: On success returns 0, an error code otherwise
> > + */
> > +
> 
> This extra empty line is not needed (and not desirable even).  And
> same below.

Sure. will remove it.
> 
> > +int intel_tcc_get_tjmax(int cpu, int *tjmax)
> > +{
> > +       u32 eax, edx;
> > +       int err;
> > +
> > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > +                                       &eax, &edx);
> 
> The current trend is to align the arguments after a line break with
> the first one, but I would just put them all on the same line (and
> below too).

I agree putting them in the same line looks prettier.

> 
> > +       if (err)
> > +               return err;
> > +
> > +       *tjmax = (eax >> 16) & 0xff;
> 
> This means that the tjmax value cannot be negative.
> 
> > +
> > +       return *tjmax ? 0 : -EINVAL;
> 
> So the return value of this function could be tjmax if positive or a
> negative error code otherwise.  No return pointers needed.

I tried both. And I think I chose this solution just because it makes
the following cleanup patches in this series looks prettier.

I will try your suggestion, and if there is any other reason I wrote it
in this way, I will find it out again. :p

> 
> And why do you want to return -EINVAL (rather than any other error
> code) if tjmax turns out to be 0?

Because x86_pkg_temp_thermal driver returns -EINVAL previously.
Any suggestions on this?

> > +}
> > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_tjmax, INTEL_TCC);
> > +
> > +/**
> > + * intel_tcc_get_offset() - returns the TCC Offset value to Tjmax
> > + * @cpu: cpu that the MSR should be run on.
> > + * @offset: a valid pointer to where to store the offset value
> > + *
> > + * Get the TCC offset value to Tjmax. The effective thermal
> > throttling or TCC
> > + * activation temperature equals "Tjmax" - "TCC Offset", in
> > degrees C.
> > + *
> > + * Return: On success returns 0, an error code otherwise
> > + */
> > +
> > +int intel_tcc_get_offset(int cpu, int *offset)
> > +{
> > +       u32 eax, edx;
> > +       int err;
> > +
> > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > +                                       &eax, &edx);
> > +       if (err)
> > +               return err;
> > +
> > +       *offset = (eax >> 24) & 0x3f;
> 
> Well, offset cannot be negative here, so (again) the return value of
> this function could be interpreted as the offsent (if non-negative)
> or
> a negative error code on failure.
> 
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> > +
> > +/**
> > + * intel_tcc_set_offset() - set the TCC offset value to Tjmax
> > + * @cpu: cpu that the MSR should be run on.
> > + * @offset: TCC offset value in degree C
> 
> I think that this cannot be negative, so maybe say "in K" instead of
> "in degree C"?

degree C is the unit used in the Intel SDM, so better to keep this
comment aligned.

> 
> And maybe it's better to pass u8 here?

sounds good, will do in next version.

> 
> > + *
> > + * Set the TCC Offset value to Tjmax. The effective thermal
> > throttling or TCC
> > + * activation temperature equals "Tjmax" - "TCC Offset", in degree
> > C.
> > + *
> > + * Return: On success returns 0, an error code otherwise
> > + */
> > +
> > +int intel_tcc_set_offset(int cpu, int offset)
> > +{
> > +       u32 eax, edx;
> > +       int err;
> > +
> > +       if (offset > 0x3f)
> > +               return -EINVAL;
> > +
> > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > +                                       &eax, &edx);
> > +       if (err)
> > +               return err;
> > +
> > +       if (eax & BIT(31))
> > +               return -EPERM;
> 
> Why -EPERM?

Bit 31 set means the MSR is locked, and os software cannot write it.
should I use -EACCES instead?

> 
> > +
> > +       eax &= ~(0x3f << 24);
> > +       eax |= (offset << 24);
> 
> The parens are not needed AFAICS.
> 
Agreed.

> > +
> > +       return wrmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > eax, edx);
> 
> So is any protection against concurrent access needed here?  Like
> what
> if two different callers invoke this function at the same time for
> the
> same CPU?

Given that the tcc offset is the only field that kernel code writes,
all the other bits won't change, so this is not a problem.

> 
> > +}
> > +EXPORT_SYMBOL_NS_GPL(intel_tcc_set_offset, INTEL_TCC);
> > +
> > +/**
> > + * intel_tcc_get_temp() - returns the current temperature
> > + * @cpu: cpu that the MSR should be run on.
> > + * @pkg: true: Package Thermal Sensor. false: Core Thermal Sensor.
> > + * @temp: a valid pointer to where to store the resulting
> > temperature
> > + *
> > + * Get the current temperature returned by the CPU core/package
> > level
> > + * thermal sensor, in degrees C.
> > + *
> > + * Return: On success returns 0, an error code otherwise
> > + */
> > +int intel_tcc_get_temp(int cpu, bool pkg, int *temp)
> > +{
> > +       u32 eax, edx;
> > +       u32 msr = pkg ? MSR_IA32_PACKAGE_THERM_STATUS :
> > MSR_IA32_THERM_STATUS;
> > +       int tjmax, err;
> > +
> > +       err = intel_tcc_get_tjmax(cpu, &tjmax);
> > +       if (err)
> > +               return err;
> 
> Well, what if somebody change tjmax on this cpu while this function
> is running?

tjmax is read only. Only firmware can change its value at runtime, say
this field is updated when SST-PP level is changed.

thanks,
rui
> 
> > +
> > +       err = rdmsr_safe_on_cpu(cpu, msr, &eax, &edx);
> > +       if (err)
> > +               return err;
> > +
> > +       if (eax & 0x80000000) {
> > +               *temp = tjmax - ((eax >> 16) & 0x7f);
> > +               return 0;
> > +       }
> > +       return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_temp, INTEL_TCC);
> > +
> > diff --git a/include/linux/intel_tcc.h b/include/linux/intel_tcc.h
> > new file mode 100644
> > index 000000000000..94f8ceab5dd0
> > --- /dev/null
> > +++ b/include/linux/intel_tcc.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + *  header for Intel TCC (thermal control circuitry) library
> > + *
> > + *  Copyright (C) 2022  Intel Corporation.
> > + */
> > +
> > +#ifndef __INTEL_TCC_H__
> > +#define __INTEL_TCC_H__
> > +
> > +#include <linux/types.h>
> > +
> > +int intel_tcc_get_tjmax(int cpu, int *tjmax);
> > +int intel_tcc_get_offset(int cpu, int *offset);
> > +int intel_tcc_set_offset(int cpu, int offset);
> > +int intel_tcc_get_temp(int cpu, bool pkg, int *temp);
> > +
> > +#endif /* __INTEL_TCC_H__ */
> > --

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Rafael J. Wysocki]

On Thu, Dec 8, 2022 at 5:49 PM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Thu, 2022-12-08 at 14:49 +0100, Rafael J. Wysocki wrote:
> > On Tue, Nov 8, 2022 at 4:31 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > > There are several different drivers that accesses the Intel TCC
> > > (thermal control circuitry) MSRs, and each of them has its own
> > > implementation for the same functionalities, e.g. getting the
> > > current
> > > temperature, getting the tj_max, and getting/setting the tj_max
> > > offset.
> > >
> > > Introduce a library to unify the code for Intel CPU TCC MSR access.
> > >
> > > At the same time, ensure the temperature is got based on the
> > > updated
> > > tjmax value because tjmax can be changed at runtime for cases like
> > > the Intel SST-PP (Intel Speed Select Technology - Performance
> > > Profile)
> > > level change.
> > >
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> >
> > Nice series, overall I like it a lot, but I have some comments
> > regarding this particular patch (below).  Some of them are arguably
> > minor, but at least one thing is more serious.
>
> Hi, Rafael,
>
> Thanks for reviewing the patch set.
> >
> > > ---
> > >  drivers/thermal/intel/Kconfig     |   4 +
> > >  drivers/thermal/intel/Makefile    |   1 +
> > >  drivers/thermal/intel/intel_tcc.c | 131
> > > ++++++++++++++++++++++++++++++
> > >  include/linux/intel_tcc.h         |  18 ++++
> > >  4 files changed, 154 insertions(+)
> > >  create mode 100644 drivers/thermal/intel/intel_tcc.c
> > >  create mode 100644 include/linux/intel_tcc.h
> > >
> > > diff --git a/drivers/thermal/intel/Kconfig
> > > b/drivers/thermal/intel/Kconfig
> > > index f0c845679250..6b938c040d6e 100644
> > > --- a/drivers/thermal/intel/Kconfig
> > > +++ b/drivers/thermal/intel/Kconfig
> > > @@ -12,6 +12,10 @@ config X86_THERMAL_VECTOR
> > >         def_bool y
> > >         depends on X86 && CPU_SUP_INTEL && X86_LOCAL_APIC
> > >
> > > +config INTEL_TCC
> > > +       bool
> > > +       depends on X86
> > > +
> > >  config X86_PKG_TEMP_THERMAL
> > >         tristate "X86 package temperature thermal driver"
> > >         depends on X86_THERMAL_VECTOR
> > > diff --git a/drivers/thermal/intel/Makefile
> > > b/drivers/thermal/intel/Makefile
> > > index 9a8d8054f316..5d8833c82ab6 100644
> > > --- a/drivers/thermal/intel/Makefile
> > > +++ b/drivers/thermal/intel/Makefile
> > > @@ -2,6 +2,7 @@
> > >  #
> > >  # Makefile for various Intel thermal drivers.
> > >
> > > +obj-$(CONFIG_INTEL_TCC)        += intel_tcc.o
> > >  obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o
> > >  obj-$(CONFIG_X86_PKG_TEMP_THERMAL)     += x86_pkg_temp_thermal.o
> > >  obj-$(CONFIG_INTEL_SOC_DTS_IOSF_CORE)  += intel_soc_dts_iosf.o
> > > diff --git a/drivers/thermal/intel/intel_tcc.c
> > > b/drivers/thermal/intel/intel_tcc.c
> > > new file mode 100644
> > > index 000000000000..74b434914975
> > > --- /dev/null
> > > +++ b/drivers/thermal/intel/intel_tcc.c
> > > @@ -0,0 +1,131 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * intel_tcc.c - Library for Intel TCC (thermal control circuitry)
> > > MSR access
> > > + * Copyright (c) 2022, Intel Corporation.
> > > + */
> > > +
> > > +#include <linux/errno.h>
> > > +#include <linux/intel_tcc.h>
> > > +#include <asm/msr.h>
> > > +
> > > +/**
> > > + * intel_tcc_get_tjmax() - returns the default TCC activation
> > > Temperature
> > > + * @cpu: cpu that the MSR should be run on.
> > > + * @tjmax: a valid pointer to where to store the Tjmax value
> > > + *
> > > + * Get the TjMax value, which is the default thermal throttling or
> > > TCC
> > > + * activation temperature in degrees C.
> > > + *
> > > + * Return: On success returns 0, an error code otherwise
> > > + */
> > > +
> >
> > This extra empty line is not needed (and not desirable even).  And
> > same below.
>
> Sure. will remove it.
> >
> > > +int intel_tcc_get_tjmax(int cpu, int *tjmax)
> > > +{
> > > +       u32 eax, edx;
> > > +       int err;
> > > +
> > > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > > +                                       &eax, &edx);
> >
> > The current trend is to align the arguments after a line break with
> > the first one, but I would just put them all on the same line (and
> > below too).
>
> I agree putting them in the same line looks prettier.
>
> >
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       *tjmax = (eax >> 16) & 0xff;
> >
> > This means that the tjmax value cannot be negative.
> >
> > > +
> > > +       return *tjmax ? 0 : -EINVAL;
> >
> > So the return value of this function could be tjmax if positive or a
> > negative error code otherwise.  No return pointers needed.
>
> I tried both. And I think I chose this solution just because it makes
> the following cleanup patches in this series looks prettier.
>
> I will try your suggestion, and if there is any other reason I wrote it
> in this way, I will find it out again. :p
>
> >
> > And why do you want to return -EINVAL (rather than any other error
> > code) if tjmax turns out to be 0?
>
> Because x86_pkg_temp_thermal driver returns -EINVAL previously.
> Any suggestions on this?

I would use -ENODATA.

> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_tjmax, INTEL_TCC);
> > > +
> > > +/**
> > > + * intel_tcc_get_offset() - returns the TCC Offset value to Tjmax
> > > + * @cpu: cpu that the MSR should be run on.
> > > + * @offset: a valid pointer to where to store the offset value
> > > + *
> > > + * Get the TCC offset value to Tjmax. The effective thermal
> > > throttling or TCC
> > > + * activation temperature equals "Tjmax" - "TCC Offset", in
> > > degrees C.
> > > + *
> > > + * Return: On success returns 0, an error code otherwise
> > > + */
> > > +
> > > +int intel_tcc_get_offset(int cpu, int *offset)
> > > +{
> > > +       u32 eax, edx;
> > > +       int err;
> > > +
> > > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > > +                                       &eax, &edx);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       *offset = (eax >> 24) & 0x3f;
> >
> > Well, offset cannot be negative here, so (again) the return value of
> > this function could be interpreted as the offsent (if non-negative)
> > or
> > a negative error code on failure.
> >
> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> > > +
> > > +/**
> > > + * intel_tcc_set_offset() - set the TCC offset value to Tjmax
> > > + * @cpu: cpu that the MSR should be run on.
> > > + * @offset: TCC offset value in degree C
> >
> > I think that this cannot be negative, so maybe say "in K" instead of
> > "in degree C"?
>
> degree C is the unit used in the Intel SDM, so better to keep this
> comment aligned.

OK

> >
> > And maybe it's better to pass u8 here?
>
> sounds good, will do in next version.
>
> >
> > > + *
> > > + * Set the TCC Offset value to Tjmax. The effective thermal
> > > throttling or TCC
> > > + * activation temperature equals "Tjmax" - "TCC Offset", in degree
> > > C.
> > > + *
> > > + * Return: On success returns 0, an error code otherwise
> > > + */
> > > +
> > > +int intel_tcc_set_offset(int cpu, int offset)
> > > +{
> > > +       u32 eax, edx;
> > > +       int err;
> > > +
> > > +       if (offset > 0x3f)
> > > +               return -EINVAL;
> > > +
> > > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > > +                                       &eax, &edx);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       if (eax & BIT(31))
> > > +               return -EPERM;
> >
> > Why -EPERM?
>
> Bit 31 set means the MSR is locked, and os software cannot write it.
> should I use -EACCES instead?

No, -EPERM is fine in this case, but a short comment would be useful.

> >
> > > +
> > > +       eax &= ~(0x3f << 24);
> > > +       eax |= (offset << 24);
> >
> > The parens are not needed AFAICS.
> >
> Agreed.
>
> > > +
> > > +       return wrmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > > eax, edx);
> >
> > So is any protection against concurrent access needed here?  Like
> > what
> > if two different callers invoke this function at the same time for
> > the
> > same CPU?
>
> Given that the tcc offset is the only field that kernel code writes,
> all the other bits won't change, so this is not a problem.

OK

> >
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_set_offset, INTEL_TCC);
> > > +
> > > +/**
> > > + * intel_tcc_get_temp() - returns the current temperature
> > > + * @cpu: cpu that the MSR should be run on.
> > > + * @pkg: true: Package Thermal Sensor. false: Core Thermal Sensor.
> > > + * @temp: a valid pointer to where to store the resulting
> > > temperature
> > > + *
> > > + * Get the current temperature returned by the CPU core/package
> > > level
> > > + * thermal sensor, in degrees C.
> > > + *
> > > + * Return: On success returns 0, an error code otherwise
> > > + */
> > > +int intel_tcc_get_temp(int cpu, bool pkg, int *temp)
> > > +{
> > > +       u32 eax, edx;
> > > +       u32 msr = pkg ? MSR_IA32_PACKAGE_THERM_STATUS :
> > > MSR_IA32_THERM_STATUS;
> > > +       int tjmax, err;
> > > +
> > > +       err = intel_tcc_get_tjmax(cpu, &tjmax);
> > > +       if (err)
> > > +               return err;
> >
> > Well, what if somebody change tjmax on this cpu while this function
> > is running?
>
> tjmax is read only. Only firmware can change its value at runtime, say
> this field is updated when SST-PP level is changed.

Do we get any type of notification on tjmax changes, or do we need to
poll for it?

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Zhang Rui]

> > > 
> > > > +
> > > > +       return *tjmax ? 0 : -EINVAL;
> > > 
> > > So the return value of this function could be tjmax if positive
> > > or a
> > > negative error code otherwise.  No return pointers needed.
> > 
> > I tried both. And I think I chose this solution just because it
> > makes
> > the following cleanup patches in this series looks prettier.
> > 
> > I will try your suggestion, and if there is any other reason I
> > wrote it
> > in this way, I will find it out again. :p
> > 
> > > And why do you want to return -EINVAL (rather than any other
> > > error
> > > code) if tjmax turns out to be 0?
> > 
> > Because x86_pkg_temp_thermal driver returns -EINVAL previously.
> > Any suggestions on this?
> 
> I would use -ENODATA.

Sure, will do.

> > > > +}
> > > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_set_offset, INTEL_TCC);
> > > > +
> > > > +/**
> > > > + * intel_tcc_get_temp() - returns the current temperature
> > > > + * @cpu: cpu that the MSR should be run on.
> > > > + * @pkg: true: Package Thermal Sensor. false: Core Thermal
> > > > Sensor.
> > > > + * @temp: a valid pointer to where to store the resulting
> > > > temperature
> > > > + *
> > > > + * Get the current temperature returned by the CPU
> > > > core/package
> > > > level
> > > > + * thermal sensor, in degrees C.
> > > > + *
> > > > + * Return: On success returns 0, an error code otherwise
> > > > + */
> > > > +int intel_tcc_get_temp(int cpu, bool pkg, int *temp)
> > > > +{
> > > > +       u32 eax, edx;
> > > > +       u32 msr = pkg ? MSR_IA32_PACKAGE_THERM_STATUS :
> > > > MSR_IA32_THERM_STATUS;
> > > > +       int tjmax, err;
> > > > +
> > > > +       err = intel_tcc_get_tjmax(cpu, &tjmax);
> > > > +       if (err)
> > > > +               return err;
> > > 
> > > Well, what if somebody change tjmax on this cpu while this
> > > function
> > > is running?
> > 
> > tjmax is read only. Only firmware can change its value at runtime,
> > say
> > this field is updated when SST-PP level is changed.
> 
> Do we get any type of notification on tjmax changes, or do we need to
> poll for it?

I'm not aware of such a notification.
Srinivas, do you know if there is one?

thanks,
rui

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Zhang Rui]

> > > +
> > > +       err = rdmsr_safe_on_cpu(cpu, msr, &eax, &edx);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       if (eax & 0x80000000) {
> > > +               *temp = tjmax - ((eax >> 16) & 0x7f);
> > > +               return 0;
> > > +       }
> > > +       return -EINVAL;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_temp, INTEL_TCC);
> 
> And one more thing, but rather for the future.
> 
> Reading from these MSRs can be costly, especially when done on a
> remote CPU, so would it make sense to cache the retrieved values in
> case they are read relatively often?like the coretemp driver already
> has similar code.

For now, this API is called from sysfs attribute callback so it is okay
to add a unified rate_limit here, say 1 second like coretemp driver.

But for the future, I'm not sure. This really depends on the
expectation of the caller.

BTW, this reminds me that I can improve the code a little bit, to avoid
reading MSR from remote CPU when possible.

thanks,
rui

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[srinivas pandruvada]

On Fri, 2022-12-09 at 21:32 +0800, Zhang Rui wrote:
> > > 
> > > > 

[...]

> > > > Well, what if somebody change tjmax on this cpu while this
> > > > function
> > > > is running?
> > > 
> > > tjmax is read only. Only firmware can change its value at
> > > runtime,
> > > say
> > > this field is updated when SST-PP level is changed.
> > 
> > Do we get any type of notification on tjmax changes, or do we need
> > to
> > poll for it?
> 
> I'm not aware of such a notification.
> Srinivas, do you know if there is one?
There is no explicit notification. You can infer from HWP guaranteed
change interrupt.

Thanks,
Srinivas

> 
> thanks,
> rui
> 
> 

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Zhang Rui]

> > > +       if (err)
> > > +               return err;
> > > +
> > > +       *tjmax = (eax >> 16) & 0xff;
> > 
> > This means that the tjmax value cannot be negative.
> > 
> > > +
> > > +       return *tjmax ? 0 : -EINVAL;
> > 
> > So the return value of this function could be tjmax if positive or
> > a
> > negative error code otherwise.  No return pointers needed.
> 
> I tried both. And I think I chose this solution just because it makes
> the following cleanup patches in this series looks prettier.
> 
> I will try your suggestion, and if there is any other reason I wrote
> it
> in this way, I will find it out again. :p

I see.
Say, we have to use

intel_tcc_get_temp(int cpu, int *temp)

so I keep the same format for all the intel_tcc_get_XXX APIs

intel_tcc_get_tjmax(int cpu, int *tjmax)
intel_tcc_get_offset(int cpu, int *offset)

so that the return value is decoded in the same way. This helps avoid
return value check mistakes for the callers.

surely I can remove the return pointer if you still prefer that. :p

> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> > > +
> > > +/**
> > > + * intel_tcc_set_offset() - set the TCC offset value to Tjmax
> > > + * @cpu: cpu that the MSR should be run on.
> > > + * @offset: TCC offset value in degree C
> > 
> > I think that this cannot be negative, so maybe say "in K" instead
> > of
> > "in degree C"?
> 
> degree C is the unit used in the Intel SDM, so better to keep this
> comment aligned.
> 
> > And maybe it's better to pass u8 here?
> 
> sounds good, will do in next version.
> 
to keep consistent, we can use either
int intel_tcc_get_offset(int cpu)
int intel_tcc_set_offset(int cpu, int offset)
or
int intel_tcc_get_offset(int cpu, u8 *offset)
int intel_tcc_set_offset(int cpu, u8 offset)

or else callers need to declare 'offset' but with different type, when
getting and setting it, which looks strange.

thanks,
rui
> > > + *
> > > + * Set the TCC Offset value to Tjmax. The effective thermal
> > > throttling or TCC
> > > + * activation temperature equals "Tjmax" - "TCC Offset", in
> > > degree
> > > C.
> > > + *
> > > + * Return: On success returns 0, an error code otherwise
> > > + */
> > > +
> > > +int intel_tcc_set_offset(int cpu, int offset)
> > > +{
> > > +       u32 eax, edx;
> > > +       int err;
> > > +
> > > +       if (offset > 0x3f)
> > > +               return -EINVAL;
> > > +
> > > +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > > +                                       &eax, &edx);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       if (eax & BIT(31))
> > > +               return -EPERM;
> > 
> > Why -EPERM?
> 
> Bit 31 set means the MSR is locked, and os software cannot write it.
> should I use -EACCES instead?
> 
> > > +
> > > +       eax &= ~(0x3f << 24);
> > > +       eax |= (offset << 24);
> > 
> > The parens are not needed AFAICS.
> > 
> Agreed.
> 
> > > +
> > > +       return wrmsr_safe_on_cpu(cpu,
> > > MSR_IA32_TEMPERATURE_TARGET,
> > > eax, edx);
> > 
> > So is any protection against concurrent access needed here?  Like
> > what
> > if two different callers invoke this function at the same time for
> > the
> > same CPU?
> 
> Given that the tcc offset is the only field that kernel code writes,
> all the other bits won't change, so this is not a problem.
> 
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_set_offset, INTEL_TCC);
> > > +
> > > +/**
> > > + * intel_tcc_get_temp() - returns the current temperature
> > > + * @cpu: cpu that the MSR should be run on.
> > > + * @pkg: true: Package Thermal Sensor. false: Core Thermal
> > > Sensor.
> > > + * @temp: a valid pointer to where to store the resulting
> > > temperature
> > > + *
> > > + * Get the current temperature returned by the CPU core/package
> > > level
> > > + * thermal sensor, in degrees C.
> > > + *
> > > + * Return: On success returns 0, an error code otherwise
> > > + */
> > > +int intel_tcc_get_temp(int cpu, bool pkg, int *temp)
> > > +{
> > > +       u32 eax, edx;
> > > +       u32 msr = pkg ? MSR_IA32_PACKAGE_THERM_STATUS :
> > > MSR_IA32_THERM_STATUS;
> > > +       int tjmax, err;
> > > +
> > > +       err = intel_tcc_get_tjmax(cpu, &tjmax);
> > > +       if (err)
> > > +               return err;
> > 
> > Well, what if somebody change tjmax on this cpu while this function
> > is running?
> 
> tjmax is read only. Only firmware can change its value at runtime,
> say
> this field is updated when SST-PP level is changed.
> 
> thanks,
> rui
> > > +
> > > +       err = rdmsr_safe_on_cpu(cpu, msr, &eax, &edx);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       if (eax & 0x80000000) {
> > > +               *temp = tjmax - ((eax >> 16) & 0x7f);
> > > +               return 0;
> > > +       }
> > > +       return -EINVAL;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_temp, INTEL_TCC);
> > > +
> > > diff --git a/include/linux/intel_tcc.h
> > > b/include/linux/intel_tcc.h
> > > new file mode 100644
> > > index 000000000000..94f8ceab5dd0
> > > --- /dev/null
> > > +++ b/include/linux/intel_tcc.h
> > > @@ -0,0 +1,18 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + *  header for Intel TCC (thermal control circuitry) library
> > > + *
> > > + *  Copyright (C) 2022  Intel Corporation.
> > > + */
> > > +
> > > +#ifndef __INTEL_TCC_H__
> > > +#define __INTEL_TCC_H__
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +int intel_tcc_get_tjmax(int cpu, int *tjmax);
> > > +int intel_tcc_get_offset(int cpu, int *offset);
> > > +int intel_tcc_set_offset(int cpu, int offset);
> > > +int intel_tcc_get_temp(int cpu, bool pkg, int *temp);
> > > +
> > > +#endif /* __INTEL_TCC_H__ */
> > > --

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Zhang Rui]

On Fri, 2022-12-09 at 08:28 -0800, srinivas pandruvada wrote:
> On Fri, 2022-12-09 at 21:32 +0800, Zhang Rui wrote:
> 
> [...]
> 
> > > > > Well, what if somebody change tjmax on this cpu while this
> > > > > function
> > > > > is running?
> > > > 
> > > > tjmax is read only. Only firmware can change its value at
> > > > runtime,
> > > > say
> > > > this field is updated when SST-PP level is changed.
> > > 
> > > Do we get any type of notification on tjmax changes, or do we
> > > need
> > > to
> > > poll for it?
> > 
> > I'm not aware of such a notification.
> > Srinivas, do you know if there is one?
> There is no explicit notification. You can infer from HWP guaranteed
> change interrupt.

But
1. tjmax may or may not change when HWP guaranteed change interrupt
fires. And it does not change in most cases, right?
2. HWP guaranteed change interrupt is per CPU, so if we update tjmax in
the interrupt handler, we also need to cache the timestamp to avoid
duplicate updates from multiple CPUs in the same package/die.

Given that the TCC lib APIs may or may not be called, depends on the
usersapce behavior, do you think this is a win by adding this extra
cost?

Instead, update the tjmax value only when the API is called, and then
cache it to handle frequent read is sufficient. what do you think?

thanks,
rui

> 
> Thanks,
> Srinivas
> 
> > thanks,
> > rui
> > 
> > 
> 
> 

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Rafael J. Wysocki]

On Sun, Dec 11, 2022 at 8:24 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
>
> > > > +       if (err)
> > > > +               return err;
> > > > +
> > > > +       *tjmax = (eax >> 16) & 0xff;
> > >
> > > This means that the tjmax value cannot be negative.
> > >
> > > > +
> > > > +       return *tjmax ? 0 : -EINVAL;
> > >
> > > So the return value of this function could be tjmax if positive or
> > > a
> > > negative error code otherwise.  No return pointers needed.
> >
> > I tried both. And I think I chose this solution just because it makes
> > the following cleanup patches in this series looks prettier.
> >
> > I will try your suggestion, and if there is any other reason I wrote
> > it
> > in this way, I will find it out again. :p
>
> I see.
> Say, we have to use
>
> intel_tcc_get_temp(int cpu, int *temp)

Do we?

> so I keep the same format for all the intel_tcc_get_XXX APIs
>
> intel_tcc_get_tjmax(int cpu, int *tjmax)
> intel_tcc_get_offset(int cpu, int *offset)
>
> so that the return value is decoded in the same way. This helps avoid
> return value check mistakes for the callers.
>
> surely I can remove the return pointer if you still prefer that. :p

Using a function value directly is very much preferred unless there
really are two values to return.

In these particular cases a negative return value can be clearly
interpreted as an error condition, so using the return value directly
is sufficient.

> > > > +
> > > > +       return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> > > > +
> > > > +/**
> > > > + * intel_tcc_set_offset() - set the TCC offset value to Tjmax
> > > > + * @cpu: cpu that the MSR should be run on.
> > > > + * @offset: TCC offset value in degree C
> > >
> > > I think that this cannot be negative, so maybe say "in K" instead
> > > of
> > > "in degree C"?
> >
> > degree C is the unit used in the Intel SDM, so better to keep this
> > comment aligned.
> >
> > > And maybe it's better to pass u8 here?
> >
> > sounds good, will do in next version.
> >
> to keep consistent, we can use either
> int intel_tcc_get_offset(int cpu)
> int intel_tcc_set_offset(int cpu, int offset)

What about intel_tcc_set_offset(int cpu, u8 offset)?

> or
> int intel_tcc_get_offset(int cpu, u8 *offset)
> int intel_tcc_set_offset(int cpu, u8 offset)
>
> or else callers need to declare 'offset' but with different type, when
> getting and setting it, which looks strange.

Well, one can surely pass an int as a u8 argument and assign a u8
return value to an int variable.

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Rafael J. Wysocki]

On Sun, Dec 11, 2022 at 8:50 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Fri, 2022-12-09 at 08:28 -0800, srinivas pandruvada wrote:
> > On Fri, 2022-12-09 at 21:32 +0800, Zhang Rui wrote:
> >
> > [...]
> >
> > > > > > Well, what if somebody change tjmax on this cpu while this
> > > > > > function
> > > > > > is running?
> > > > >
> > > > > tjmax is read only. Only firmware can change its value at
> > > > > runtime,
> > > > > say
> > > > > this field is updated when SST-PP level is changed.
> > > >
> > > > Do we get any type of notification on tjmax changes, or do we
> > > > need
> > > > to
> > > > poll for it?
> > >
> > > I'm not aware of such a notification.
> > > Srinivas, do you know if there is one?
> > There is no explicit notification. You can infer from HWP guaranteed
> > change interrupt.
>
> But
> 1. tjmax may or may not change when HWP guaranteed change interrupt
> fires. And it does not change in most cases, right?
> 2. HWP guaranteed change interrupt is per CPU, so if we update tjmax in
> the interrupt handler, we also need to cache the timestamp to avoid
> duplicate updates from multiple CPUs in the same package/die.
>
> Given that the TCC lib APIs may or may not be called, depends on the
> usersapce behavior, do you think this is a win by adding this extra
> cost?

Not really.

> Instead, update the tjmax value only when the API is called, and then
> cache it to handle frequent read is sufficient. what do you think?

Yes, that should be sufficient, but I wouldn't do it in this series anyway.

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Zhang Rui]

On Mon, 2022-12-12 at 13:11 +0100, Rafael J. Wysocki wrote:
> On Sun, Dec 11, 2022 at 8:24 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > 
> > > > > +       if (err)
> > > > > +               return err;
> > > > > +
> > > > > +       *tjmax = (eax >> 16) & 0xff;
> > > > 
> > > > This means that the tjmax value cannot be negative.
> > > > 
> > > > > +
> > > > > +       return *tjmax ? 0 : -EINVAL;
> > > > 
> > > > So the return value of this function could be tjmax if positive
> > > > or
> > > > a
> > > > negative error code otherwise.  No return pointers needed.
> > > 
> > > I tried both. And I think I chose this solution just because it
> > > makes
> > > the following cleanup patches in this series looks prettier.
> > > 
> > > I will try your suggestion, and if there is any other reason I
> > > wrote
> > > it
> > > in this way, I will find it out again. :p
> > 
> > I see.
> > Say, we have to use
> > 
> > intel_tcc_get_temp(int cpu, int *temp)
> 
> Do we?

temp = tjmax - digital_readout

tjmax and digital_readout are from MSR and they are both positive
values, but temp can be negative in theory.
so are you suggesting that we should treat the negative CPU temperature
as an error because this won't happen in real world?

thanks,
rui

> 
> > so I keep the same format for all the intel_tcc_get_XXX APIs
> > 
> > intel_tcc_get_tjmax(int cpu, int *tjmax)
> > intel_tcc_get_offset(int cpu, int *offset)
> > 
> > so that the return value is decoded in the same way. This helps
> > avoid
> > return value check mistakes for the callers.
> > 
> > surely I can remove the return pointer if you still prefer that. :p
> 
> Using a function value directly is very much preferred unless there
> really are two values to return.
> 
> In these particular cases a negative return value can be clearly
> interpreted as an error condition, so using the return value directly
> is sufficient.
> 
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> > > > > +
> > > > > +/**
> > > > > + * intel_tcc_set_offset() - set the TCC offset value to
> > > > > Tjmax
> > > > > + * @cpu: cpu that the MSR should be run on.
> > > > > + * @offset: TCC offset value in degree C
> > > > 
> > > > I think that this cannot be negative, so maybe say "in K"
> > > > instead
> > > > of
> > > > "in degree C"?
> > > 
> > > degree C is the unit used in the Intel SDM, so better to keep
> > > this
> > > comment aligned.
> > > 
> > > > And maybe it's better to pass u8 here?
> > > 
> > > sounds good, will do in next version.
> > > 
> > to keep consistent, we can use either
> > int intel_tcc_get_offset(int cpu)
> > int intel_tcc_set_offset(int cpu, int offset)
> 
> What about intel_tcc_set_offset(int cpu, u8 offset)?
> 
> > or
> > int intel_tcc_get_offset(int cpu, u8 *offset)
> > int intel_tcc_set_offset(int cpu, u8 offset)
> > 
> > or else callers need to declare 'offset' but with different type,
> > when
> > getting and setting it, which looks strange.
> 
> Well, one can surely pass an int as a u8 argument and assign a u8
> return value to an int variable.

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Rafael J. Wysocki]

On Tue, Dec 13, 2022 at 2:38 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Mon, 2022-12-12 at 13:11 +0100, Rafael J. Wysocki wrote:
> > On Sun, Dec 11, 2022 at 8:24 AM Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > >
> > > > > > +       if (err)
> > > > > > +               return err;
> > > > > > +
> > > > > > +       *tjmax = (eax >> 16) & 0xff;
> > > > >
> > > > > This means that the tjmax value cannot be negative.
> > > > >
> > > > > > +
> > > > > > +       return *tjmax ? 0 : -EINVAL;
> > > > >
> > > > > So the return value of this function could be tjmax if positive
> > > > > or
> > > > > a
> > > > > negative error code otherwise.  No return pointers needed.
> > > >
> > > > I tried both. And I think I chose this solution just because it
> > > > makes
> > > > the following cleanup patches in this series looks prettier.
> > > >
> > > > I will try your suggestion, and if there is any other reason I
> > > > wrote
> > > > it
> > > > in this way, I will find it out again. :p
> > >
> > > I see.
> > > Say, we have to use
> > >
> > > intel_tcc_get_temp(int cpu, int *temp)
> >
> > Do we?
>
> temp = tjmax - digital_readout
>
> tjmax and digital_readout are from MSR and they are both positive
> values, but temp can be negative in theory.

I know, but is this ever expected to happen?

> so are you suggesting that we should treat the negative CPU temperature
> as an error because this won't happen in real world?

No.

If it's necessary to handle negative temperatures, an int is needed
and the additional return value is useful.

Now there is also the question of what the callers will do with a
negative temperature value.  Do they care?

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Zhang Rui]

On Tue, 2022-12-13 at 16:34 +0100, Rafael J. Wysocki wrote:
> On Tue, Dec 13, 2022 at 2:38 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > On Mon, 2022-12-12 at 13:11 +0100, Rafael J. Wysocki wrote:
> > > On Sun, Dec 11, 2022 at 8:24 AM Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > > > > +       if (err)
> > > > > > > +               return err;
> > > > > > > +
> > > > > > > +       *tjmax = (eax >> 16) & 0xff;
> > > > > > 
> > > > > > This means that the tjmax value cannot be negative.
> > > > > > 
> > > > > > > +
> > > > > > > +       return *tjmax ? 0 : -EINVAL;
> > > > > > 
> > > > > > So the return value of this function could be tjmax if
> > > > > > positive
> > > > > > or
> > > > > > a
> > > > > > negative error code otherwise.  No return pointers needed.
> > > > > 
> > > > > I tried both. And I think I chose this solution just because
> > > > > it
> > > > > makes
> > > > > the following cleanup patches in this series looks prettier.
> > > > > 
> > > > > I will try your suggestion, and if there is any other reason
> > > > > I
> > > > > wrote
> > > > > it
> > > > > in this way, I will find it out again. :p
> > > > 
> > > > I see.
> > > > Say, we have to use
> > > > 
> > > > intel_tcc_get_temp(int cpu, int *temp)
> > > 
> > > Do we?
> > 
> > temp = tjmax - digital_readout
> > 
> > tjmax and digital_readout are from MSR and they are both positive
> > values, but temp can be negative in theory.
> 
> I know, but is this ever expected to happen?
> 
> > so are you suggesting that we should treat the negative CPU
> > temperature
> > as an error because this won't happen in real world?
> 
> No.
> 
> If it's necessary to handle negative temperatures, an int is needed
> and the additional return value is useful.
> 
> Now there is also the question of what the callers will do with a
> negative temperature value.  Do they care?

Currently, all the callers are from sysfs attribute.
for userspace tools, at least thermald ignores negative CPU
temperature.
So I will return an error for negative temperature in next version.

thanks,
rui

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Rafael J. Wysocki]

On Wed, Dec 14, 2022 at 5:15 PM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Tue, 2022-12-13 at 16:34 +0100, Rafael J. Wysocki wrote:
> > On Tue, Dec 13, 2022 at 2:38 AM Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > > On Mon, 2022-12-12 at 13:11 +0100, Rafael J. Wysocki wrote:
> > > > On Sun, Dec 11, 2022 at 8:24 AM Zhang Rui <rui.zhang@intel.com>
> > > > wrote:
> > > > > > > > +       if (err)
> > > > > > > > +               return err;
> > > > > > > > +
> > > > > > > > +       *tjmax = (eax >> 16) & 0xff;
> > > > > > >
> > > > > > > This means that the tjmax value cannot be negative.
> > > > > > >
> > > > > > > > +
> > > > > > > > +       return *tjmax ? 0 : -EINVAL;
> > > > > > >
> > > > > > > So the return value of this function could be tjmax if
> > > > > > > positive
> > > > > > > or
> > > > > > > a
> > > > > > > negative error code otherwise.  No return pointers needed.
> > > > > >
> > > > > > I tried both. And I think I chose this solution just because
> > > > > > it
> > > > > > makes
> > > > > > the following cleanup patches in this series looks prettier.
> > > > > >
> > > > > > I will try your suggestion, and if there is any other reason
> > > > > > I
> > > > > > wrote
> > > > > > it
> > > > > > in this way, I will find it out again. :p
> > > > >
> > > > > I see.
> > > > > Say, we have to use
> > > > >
> > > > > intel_tcc_get_temp(int cpu, int *temp)
> > > >
> > > > Do we?
> > >
> > > temp = tjmax - digital_readout
> > >
> > > tjmax and digital_readout are from MSR and they are both positive
> > > values, but temp can be negative in theory.
> >
> > I know, but is this ever expected to happen?
> >
> > > so are you suggesting that we should treat the negative CPU
> > > temperature
> > > as an error because this won't happen in real world?
> >
> > No.
> >
> > If it's necessary to handle negative temperatures, an int is needed
> > and the additional return value is useful.
> >
> > Now there is also the question of what the callers will do with a
> > negative temperature value.  Do they care?
>
> Currently, all the callers are from sysfs attribute.
> for userspace tools, at least thermald ignores negative CPU
> temperature.
> So I will return an error for negative temperature in next version.

Sounds good.

Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Rafael J. Wysocki]

On Mon, Dec 12, 2022 at 1:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sun, Dec 11, 2022 at 8:24 AM Zhang Rui <rui.zhang@intel.com> wrote:
> >
> >
> > > > > +       if (err)
> > > > > +               return err;
> > > > > +
> > > > > +       *tjmax = (eax >> 16) & 0xff;
> > > >
> > > > This means that the tjmax value cannot be negative.
> > > >
> > > > > +
> > > > > +       return *tjmax ? 0 : -EINVAL;
> > > >
> > > > So the return value of this function could be tjmax if positive or
> > > > a
> > > > negative error code otherwise.  No return pointers needed.
> > >
> > > I tried both. And I think I chose this solution just because it makes
> > > the following cleanup patches in this series looks prettier.
> > >
> > > I will try your suggestion, and if there is any other reason I wrote
> > > it
> > > in this way, I will find it out again. :p
> >
> > I see.
> > Say, we have to use
> >
> > intel_tcc_get_temp(int cpu, int *temp)
>
> Do we?
>
> > so I keep the same format for all the intel_tcc_get_XXX APIs
> >
> > intel_tcc_get_tjmax(int cpu, int *tjmax)
> > intel_tcc_get_offset(int cpu, int *offset)
> >
> > so that the return value is decoded in the same way. This helps avoid
> > return value check mistakes for the callers.
> >
> > surely I can remove the return pointer if you still prefer that. :p
>
> Using a function value directly is very much preferred unless there
> really are two values to return.
>
> In these particular cases a negative return value can be clearly
> interpreted as an error condition, so using the return value directly
> is sufficient.
>
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> > > > > +
> > > > > +/**
> > > > > + * intel_tcc_set_offset() - set the TCC offset value to Tjmax
> > > > > + * @cpu: cpu that the MSR should be run on.
> > > > > + * @offset: TCC offset value in degree C
> > > >
> > > > I think that this cannot be negative, so maybe say "in K" instead
> > > > of
> > > > "in degree C"?
> > >
> > > degree C is the unit used in the Intel SDM, so better to keep this
> > > comment aligned.
> > >
> > > > And maybe it's better to pass u8 here?
> > >
> > > sounds good, will do in next version.
> > >
> > to keep consistent, we can use either
> > int intel_tcc_get_offset(int cpu)
> > int intel_tcc_set_offset(int cpu, int offset)
>
> What about intel_tcc_set_offset(int cpu, u8 offset)?
>
> > or
> > int intel_tcc_get_offset(int cpu, u8 *offset)
> > int intel_tcc_set_offset(int cpu, u8 offset)
> >
> > or else callers need to declare 'offset' but with different type, when
> > getting and setting it, which looks strange.
>
> Well, one can surely pass an int as a u8 argument and assign a u8
> return value to an int variable.

That said, I see your point regarding the consistency, so if you
decide to pass an int argument, that's fine with me too, but in that
case the function needs to check against negative offset values.