[PATCH 1/5] thermal/core: Fix unregistering netlink at thermal init time

[PATCH 1/5] thermal/core: Fix unregistering netlink at thermal init time

[Daniel Lezcano]

The thermal subsystem initialization miss an netlink unregistering
function in the error. Add it.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c    | 4 +++-
 drivers/thermal/thermal_netlink.c | 5 +++++
 drivers/thermal/thermal_netlink.h | 3 +++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9a3d9566d73..fddafcee5e6f 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1606,7 +1606,7 @@ static int __init thermal_init(void)
 
 	result = thermal_register_governors();
 	if (result)
-		goto error;
+		goto unregister_netlink;
 
 	result = class_register(&thermal_class);
 	if (result)
@@ -1621,6 +1621,8 @@ static int __init thermal_init(void)
 
 unregister_governors:
 	thermal_unregister_governors();
+unregister_netlink:
+	thermal_netlink_exit();
 error:
 	ida_destroy(&thermal_tz_ida);
 	ida_destroy(&thermal_cdev_ida);
diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
index 75943b06dbe7..08bc46c3ec7b 100644
--- a/drivers/thermal/thermal_netlink.c
+++ b/drivers/thermal/thermal_netlink.c
@@ -699,3 +699,8 @@ int __init thermal_netlink_init(void)
 {
 	return genl_register_family(&thermal_gnl_family);
 }
+
+void __init thermal_netlink_exit(void)
+{
+	genl_unregister_family(&thermal_gnl_family);
+}
diff --git a/drivers/thermal/thermal_netlink.h b/drivers/thermal/thermal_netlink.h
index 1052f523188d..0a9987c3bc57 100644
--- a/drivers/thermal/thermal_netlink.h
+++ b/drivers/thermal/thermal_netlink.h
@@ -13,6 +13,7 @@ struct thermal_genl_cpu_caps {
 /* Netlink notification function */
 #ifdef CONFIG_THERMAL_NETLINK
 int __init thermal_netlink_init(void);
+void __init thermal_netlink_exit(void);
 int thermal_notify_tz_create(int tz_id, const char *name);
 int thermal_notify_tz_delete(int tz_id);
 int thermal_notify_tz_enable(int tz_id);
@@ -115,4 +116,6 @@ static inline int thermal_genl_cpu_capability_event(int count, struct thermal_ge
 	return 0;
 }
 
+static inline void __init thermal_netlink_exit(void) {}
+
 #endif /* CONFIG_THERMAL_NETLINK */
-- 
2.34.1

[PATCH 2/5] thermal/core: Remove unneeded ida_destroy()

[Daniel Lezcano]

As per documentation for the ida_destroy() function: "If the IDA is
already empty, there is no need to call this function."

The thermal framework is in the init sequence, so the ida was not yet
used and consequently it is empty in case of error.

There is no need to call ida_destroy(), let's remove the calls.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index fddafcee5e6f..fad0c4a07d16 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1624,8 +1624,6 @@ static int __init thermal_init(void)
 unregister_netlink:
 	thermal_netlink_exit();
 error:
-	ida_destroy(&thermal_tz_ida);
-	ida_destroy(&thermal_cdev_ida);
 	mutex_destroy(&thermal_list_lock);
 	mutex_destroy(&thermal_governor_lock);
 	return result;
-- 
2.34.1

[PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Daniel Lezcano]

If the thermal framework fails to initialize, the mutex can be used by
the different functions registering a thermal zone anyway. We should
not destroy the mutexes as other components may use them.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index fad0c4a07d16..ea78c93277be 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1602,7 +1602,7 @@ static int __init thermal_init(void)
 
 	result = thermal_netlink_init();
 	if (result)
-		goto error;
+		return result;
 
 	result = thermal_register_governors();
 	if (result)
@@ -1623,9 +1623,7 @@ static int __init thermal_init(void)
 	thermal_unregister_governors();
 unregister_netlink:
 	thermal_netlink_exit();
-error:
-	mutex_destroy(&thermal_list_lock);
-	mutex_destroy(&thermal_governor_lock);
+
 	return result;
 }
 postcore_initcall(thermal_init);
-- 
2.34.1

[PATCH 4/5] thermal/core: Move the thermal trip code to a dedicated file

[Daniel Lezcano]

The thermal_core.c files contains a lot of functions handling
different thermal components like the governors, the trip points, the
cooling device, the OF cooling device, etc ...

This organization does not help to migrate to a more sane code where
there is a better self-encapsulation as all the components' internals
can be directly accessed from a single file.

For the sake of clarity, let's move the thermal trip points code in a
dedicated thermal_trip.c file and add a function to browse all the
trip points like we do with the thermal zones, the govenors and the
cooling devices.

The same can be done for the cooling devices and the governor code but
that will come later as the current work in the thermal framework is
to fix the trip point handling and use a generic trip point structure.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/Makefile          |   4 +-
 drivers/thermal/thermal_core.c    |  87 --------------
 drivers/thermal/thermal_helpers.c |  62 ----------
 drivers/thermal/thermal_trip.c    | 181 ++++++++++++++++++++++++++++++
 4 files changed, 183 insertions(+), 151 deletions(-)
 create mode 100644 drivers/thermal/thermal_trip.c

diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 60f0dfa9aae2..2f0db88240b5 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -4,8 +4,8 @@
 #
 
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
-thermal_sys-y			+= thermal_core.o thermal_sysfs.o \
-					thermal_helpers.o
+thermal_sys-y			+= thermal_core.o thermal_sysfs.o	
+thermal_sys-y			+= thermal_trip.o thermal_helpers.o
 
 # netlink interface to manage the thermal framework
 thermal_sys-$(CONFIG_THERMAL_NETLINK)		+= thermal_netlink.o
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index ea78c93277be..d0577685085a 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1155,12 +1155,6 @@ static void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms
 		*delay_jiffies = round_jiffies(*delay_jiffies);
 }
 
-int thermal_zone_get_num_trips(struct thermal_zone_device *tz)
-{
-	return tz->num_trips;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips);
-
 int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp)
 {
 	int i, ret = -EINVAL;
@@ -1187,87 +1181,6 @@ int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp)
 }
 EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
 
-int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
-			    struct thermal_trip *trip)
-{
-	int ret;
-
-	if (!tz || trip_id < 0 || trip_id >= tz->num_trips || !trip)
-		return -EINVAL;
-
-	if (tz->trips) {
-		*trip = tz->trips[trip_id];
-		return 0;
-	}
-
-	if (tz->ops->get_trip_hyst) {
-		ret = tz->ops->get_trip_hyst(tz, trip_id, &trip->hysteresis);
-		if (ret)
-			return ret;
-	} else {
-		trip->hysteresis = 0;
-	}
-
-	ret = tz->ops->get_trip_temp(tz, trip_id, &trip->temperature);
-	if (ret)
-		return ret;
-
-	return tz->ops->get_trip_type(tz, trip_id, &trip->type);
-}
-EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
-
-int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
-			  struct thermal_trip *trip)
-{
-	int ret;
-
-	mutex_lock(&tz->lock);
-	ret = __thermal_zone_get_trip(tz, trip_id, trip);
-	mutex_unlock(&tz->lock);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
-
-int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
-			  const struct thermal_trip *trip)
-{
-	struct thermal_trip t;
-	int ret;
-
-	if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz->trips)
-		return -EINVAL;
-
-	ret = __thermal_zone_get_trip(tz, trip_id, &t);
-	if (ret)
-		return ret;
-
-	if (t.type != trip->type)
-		return -EINVAL;
-
-	if (t.temperature != trip->temperature && tz->ops->set_trip_temp) {
-		ret = tz->ops->set_trip_temp(tz, trip_id, trip->temperature);
-		if (ret)
-			return ret;
-	}
-
-	if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) {
-		ret = tz->ops->set_trip_hyst(tz, trip_id, trip->hysteresis);
-		if (ret)
-			return ret;
-	}
-
-	if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
-		tz->trips[trip_id] = *trip;
-
-	thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
-				      trip->temperature, trip->hysteresis);
-
-	__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
-	
-	return 0;
-}
-
 /**
  * thermal_zone_device_register_with_trips() - register a new thermal zone device
  * @type:	the thermal zone device type
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 8977d5ddc23c..0f648131b0b5 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -146,68 +146,6 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
 }
 EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
 
-/**
- * __thermal_zone_set_trips - Computes the next trip points for the driver
- * @tz: a pointer to a thermal zone device structure
- *
- * The function computes the next temperature boundaries by browsing
- * the trip points. The result is the closer low and high trip points
- * to the current temperature. These values are passed to the backend
- * driver to let it set its own notification mechanism (usually an
- * interrupt).
- *
- * This function must be called with tz->lock held. Both tz and tz->ops
- * must be valid pointers.
- *
- * It does not return a value
- */
-void __thermal_zone_set_trips(struct thermal_zone_device *tz)
-{
-	struct thermal_trip trip;
-	int low = -INT_MAX, high = INT_MAX;
-	int i, ret;
-
-	lockdep_assert_held(&tz->lock);
-
-	if (!tz->ops->set_trips)
-		return;
-
-	for (i = 0; i < tz->num_trips; i++) {
-		int trip_low;
-
-		ret = __thermal_zone_get_trip(tz, i , &trip);
-		if (ret)
-			return;
-
-		trip_low = trip.temperature - trip.hysteresis;
-
-		if (trip_low < tz->temperature && trip_low > low)
-			low = trip_low;
-
-		if (trip.temperature > tz->temperature &&
-		    trip.temperature < high)
-			high = trip.temperature;
-	}
-
-	/* No need to change trip points */
-	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
-		return;
-
-	tz->prev_low_trip = low;
-	tz->prev_high_trip = high;
-
-	dev_dbg(&tz->device,
-		"new temperature boundaries: %d < x < %d\n", low, high);
-
-	/*
-	 * Set a temperature window. When this window is left the driver
-	 * must inform the thermal core via thermal_zone_device_update.
-	 */
-	ret = tz->ops->set_trips(tz, low, high);
-	if (ret)
-		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
-}
-
 static void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev,
 				       int target)
 {
diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
new file mode 100644
index 000000000000..2ef61ff7ffc3
--- /dev/null
+++ b/drivers/thermal/thermal_trip.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  Copyright (C) 2008 Intel Corp
+ *  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
+ *  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
+ *  Copyright 2022 Linaro Limited
+ *
+ * Thermal trips handling
+ */
+#include "thermal_core.h"
+
+int __for_each_thermal_trip(struct thermal_zone_device *tz,
+			    int (*cb)(struct thermal_trip *,
+				      int trip_id, void *),
+			    void *data)
+{
+	int i, ret;
+	struct thermal_trip trip;
+
+	for (i = 0; i < tz->num_trips; i++) {
+
+		ret = __thermal_zone_get_trip(tz, i, &trip);
+		if (ret)
+			return ret;
+		
+		ret = cb(&trip, i, data);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+int thermal_zone_get_num_trips(struct thermal_zone_device *tz)
+{
+	return tz->num_trips;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips);
+
+/**
+ * __thermal_zone_set_trips - Computes the next trip points for the driver
+ * @tz: a pointer to a thermal zone device structure
+ *
+ * The function computes the next temperature boundaries by browsing
+ * the trip points. The result is the closer low and high trip points
+ * to the current temperature. These values are passed to the backend
+ * driver to let it set its own notification mechanism (usually an
+ * interrupt).
+ *
+ * This function must be called with tz->lock held. Both tz and tz->ops
+ * must be valid pointers.
+ *
+ * It does not return a value
+ */
+void __thermal_zone_set_trips(struct thermal_zone_device *tz)
+{
+	struct thermal_trip trip;
+	int low = -INT_MAX, high = INT_MAX;
+	int i, ret;
+
+	lockdep_assert_held(&tz->lock);
+
+	if (!tz->ops->set_trips)
+		return;
+
+	for (i = 0; i < tz->num_trips; i++) {
+		int trip_low;
+
+		ret = __thermal_zone_get_trip(tz, i , &trip);
+		if (ret)
+			return;
+
+		trip_low = trip.temperature - trip.hysteresis;
+
+		if (trip_low < tz->temperature && trip_low > low)
+			low = trip_low;
+
+		if (trip.temperature > tz->temperature &&
+		    trip.temperature < high)
+			high = trip.temperature;
+	}
+
+	/* No need to change trip points */
+	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
+		return;
+
+	tz->prev_low_trip = low;
+	tz->prev_high_trip = high;
+
+	dev_dbg(&tz->device,
+		"new temperature boundaries: %d < x < %d\n", low, high);
+
+	/*
+	 * Set a temperature window. When this window is left the driver
+	 * must inform the thermal core via thermal_zone_device_update.
+	 */
+	ret = tz->ops->set_trips(tz, low, high);
+	if (ret)
+		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
+}
+
+int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
+			    struct thermal_trip *trip)
+{
+	int ret;
+
+	if (!tz || trip_id < 0 || trip_id >= tz->num_trips || !trip)
+		return -EINVAL;
+
+	if (tz->trips) {
+		*trip = tz->trips[trip_id];
+		return 0;
+	}
+
+	if (tz->ops->get_trip_hyst) {
+		ret = tz->ops->get_trip_hyst(tz, trip_id, &trip->hysteresis);
+		if (ret)
+			return ret;
+	} else {
+		trip->hysteresis = 0;
+	}
+
+	ret = tz->ops->get_trip_temp(tz, trip_id, &trip->temperature);
+	if (ret)
+		return ret;
+
+	return tz->ops->get_trip_type(tz, trip_id, &trip->type);
+}
+EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
+
+int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
+			  struct thermal_trip *trip)
+{
+	int ret;
+
+	mutex_lock(&tz->lock);
+	ret = __thermal_zone_get_trip(tz, trip_id, trip);
+	mutex_unlock(&tz->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
+
+int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
+			  const struct thermal_trip *trip)
+{
+	struct thermal_trip t;
+	int ret;
+
+	if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz->trips)
+		return -EINVAL;
+
+	ret = __thermal_zone_get_trip(tz, trip_id, &t);
+	if (ret)
+		return ret;
+
+	if (t.type != trip->type)
+		return -EINVAL;
+
+	if (t.temperature != trip->temperature && tz->ops->set_trip_temp) {
+		ret = tz->ops->set_trip_temp(tz, trip_id, trip->temperature);
+		if (ret)
+			return ret;
+	}
+
+	if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) {
+		ret = tz->ops->set_trip_hyst(tz, trip_id, trip->hysteresis);
+		if (ret)
+			return ret;
+	}
+
+	if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
+		tz->trips[trip_id] = *trip;
+
+	thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
+				      trip->temperature, trip->hysteresis);
+
+	__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
+	
+	return 0;
+}
-- 
2.34.1

[PATCH 5/5] thermal/core: Sort the trip points when registering a thermal zone

[Daniel Lezcano]

Most of the drivers are converted to use the generic thermal trip
points. They register a thermal zone with an array of trip points.

However, we don't have the guarantee the trip points are ordered. The
main goal of moving to the generic trip points is to provide a common
structure, ordered, so we can fix sanely how the trip points are
crossed. As a reminder, the detection is fuzzy when the trip points
are defined with hysteresis values, we can have duplicated or
inconsistent trip events.

This change sorts the trip points array when it is registered with the
thermal zone. The direction of the ordering is descending because when
we browse the trip points, we want to check the highest trip points
first as they are higher in temperature, thus higher in priority.

A pr_info() trace tells the trip points have been ordered if it
appears they are not sorted initially.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c |  3 +++
 drivers/thermal/thermal_core.h |  1 +
 drivers/thermal/thermal_trip.c | 28 ++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d0577685085a..394770591771 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1255,6 +1255,9 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 	if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
 		return ERR_PTR(-EINVAL);
 
+	if (trips && thermal_trip_sort(trips, num_trips))
+		pr_info("Thermal trips sorted for thermal zone '%s'\n", type);
+	
 	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
 	if (!tz)
 		return ERR_PTR(-ENOMEM);
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 26350206a98d..4688107fda1d 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -117,6 +117,7 @@ void __thermal_zone_set_trips(struct thermal_zone_device *tz);
 int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
 			    struct thermal_trip *trip);
 int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
+int thermal_trip_sort(struct thermal_trip *trips, int num_trips);
 
 /* sysfs I/F */
 int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
index 2ef61ff7ffc3..924998f09a5a 100644
--- a/drivers/thermal/thermal_trip.c
+++ b/drivers/thermal/thermal_trip.c
@@ -9,6 +9,34 @@
  */
 #include "thermal_core.h"
 
+/*
+ * The trip points must be ordered in the descending order so when we
+ * browse the trip points we will hit the critical, hot and then the
+ * passive/active trip points. The critical trip point being the first
+ * one to be handled.
+ */
+int thermal_trip_sort(struct thermal_trip *trips, int num_trips)
+{
+	struct thermal_trip tt;
+	int sorted = 0;
+	int i, j;
+
+	for (i = 0; i < num_trips; i++) {
+
+		for (j = i + 1; j < num_trips; j++) {
+
+			if (trips[i].temperature < trips[j].temperature) {
+				tt = trips[i];
+				trips[i] = trips[j];
+				trips[j] = tt;
+				sorted++;
+			}
+		}
+ 	}
+
+	return sorted;
+}
+
 int __for_each_thermal_trip(struct thermal_zone_device *tz,
 			    int (*cb)(struct thermal_trip *,
 				      int trip_id, void *),
-- 
2.34.1

Re: [PATCH 4/5] thermal/core: Move the thermal trip code to a dedicated file

[kernel test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on rafael-pm/thermal]
[also build test WARNING on next-20230118]
[cannot apply to linus/master v6.2-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Lezcano/thermal-core-Remove-unneeded-ida_destroy/20230119-051422
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git thermal
patch link:    https://lore.kernel.org/r/20230118211123.111493-4-daniel.lezcano%40linaro.org
patch subject: [PATCH 4/5] thermal/core: Move the thermal trip code to a dedicated file
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230119/202301191017.G3eBi85j-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/039f53e8f2c0e0711f85537055e169fb216a29a9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Daniel-Lezcano/thermal-core-Remove-unneeded-ida_destroy/20230119-051422
        git checkout 039f53e8f2c0e0711f85537055e169fb216a29a9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/thermal/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/thermal/thermal_trip.c:12:5: warning: no previous prototype for '__for_each_thermal_trip' [-Wmissing-prototypes]
      12 | int __for_each_thermal_trip(struct thermal_zone_device *tz,
         |     ^~~~~~~~~~~~~~~~~~~~~~~


vim +/__for_each_thermal_trip +12 drivers/thermal/thermal_trip.c

    11	
  > 12	int __for_each_thermal_trip(struct thermal_zone_device *tz,
    13				    int (*cb)(struct thermal_trip *,
    14					      int trip_id, void *),
    15				    void *data)
    16	{
    17		int i, ret;
    18		struct thermal_trip trip;
    19	
    20		for (i = 0; i < tz->num_trips; i++) {
    21	
    22			ret = __thermal_zone_get_trip(tz, i, &trip);
    23			if (ret)
    24				return ret;
    25			
    26			ret = cb(&trip, i, data);
    27			if (ret)
    28				return ret;
    29		}
    30	
    31		return 0;
    32	}
    33	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 5/5] thermal/core: Sort the trip points when registering a thermal zone

[Zhang, Rui]

On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> Most of the drivers are converted to use the generic thermal trip
> points. They register a thermal zone with an array of trip points.
> 
> However, we don't have the guarantee the trip points are ordered. The
> main goal of moving to the generic trip points is to provide a common
> structure, ordered, so we can fix sanely how the trip points are
> crossed. As a reminder, the detection is fuzzy when the trip points
> are defined with hysteresis values, we can have duplicated or
> inconsistent trip events.
> 
> This change sorts the trip points array when it is registered with
> the
> thermal zone. The direction of the ordering is descending because
> when
> we browse the trip points, we want to check the highest trip points
> first as they are higher in temperature, thus higher in priority.
> 
> A pr_info() trace tells the trip points have been ordered if it
> appears they are not sorted initially.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c |  3 +++
>  drivers/thermal/thermal_core.h |  1 +
>  drivers/thermal/thermal_trip.c | 28 ++++++++++++++++++++++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index d0577685085a..394770591771 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1255,6 +1255,9 @@ thermal_zone_device_register_with_trips(const
> char *type, struct thermal_trip *t
>  	if (num_trips > 0 && (!ops->get_trip_type || !ops-
> >get_trip_temp) && !trips)
>  		return ERR_PTR(-EINVAL);
>  
> +	if (trips && thermal_trip_sort(trips, num_trips))
> +		pr_info("Thermal trips sorted for thermal zone '%s'\n",
> type);
> +	
>  	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
>  	if (!tz)
>  		return ERR_PTR(-ENOMEM);
> diff --git a/drivers/thermal/thermal_core.h
> b/drivers/thermal/thermal_core.h
> index 26350206a98d..4688107fda1d 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -117,6 +117,7 @@ void __thermal_zone_set_trips(struct
> thermal_zone_device *tz);
>  int __thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
>  			    struct thermal_trip *trip);
>  int __thermal_zone_get_temp(struct thermal_zone_device *tz, int
> *temp);
> +int thermal_trip_sort(struct thermal_trip *trips, int num_trips);
>  
>  /* sysfs I/F */
>  int thermal_zone_create_device_groups(struct thermal_zone_device *,
> int);
> diff --git a/drivers/thermal/thermal_trip.c
> b/drivers/thermal/thermal_trip.c
> index 2ef61ff7ffc3..924998f09a5a 100644
> --- a/drivers/thermal/thermal_trip.c
> +++ b/drivers/thermal/thermal_trip.c
> @@ -9,6 +9,34 @@
>   */
>  #include "thermal_core.h"
>  
> +/*
> + * The trip points must be ordered in the descending order so when
> we
> + * browse the trip points we will hit the critical, hot and then the
> + * passive/active trip points. The critical trip point being the
> first
> + * one to be handled.
> + */
> +int thermal_trip_sort(struct thermal_trip *trips, int num_trips)
> +{
> +	struct thermal_trip tt;
> +	int sorted = 0;
> +	int i, j;
> +
> +	for (i = 0; i < num_trips; i++) {
> +
> +		for (j = i + 1; j < num_trips; j++) {
> +
> +			if (trips[i].temperature <
> trips[j].temperature) {
> +				tt = trips[i];
> +				trips[i] = trips[j];
> +				trips[j] = tt;
> +				sorted++;
> +			}
> +		}
> + 	}
> +
> +	return sorted;
> +}
> +
When this happens, the index(trip_id) of each trip is changed, but we
pass the new trip_id to .get_trip_temp()/.set_trip_temp() callbacks.

This will confuse the drivers and update the wrong trips, right?

IMO, we need a map between thermal core trips and unsorted driver
trips.

thanks,
rui

>  int __for_each_thermal_trip(struct thermal_zone_device *tz,
>  			    int (*cb)(struct thermal_trip *,
>  				      int trip_id, void *),

Re: [PATCH 4/5] thermal/core: Move the thermal trip code to a dedicated file

[Zhang, Rui]

Hi, Daniel,

Just one minor comment below.

On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> The thermal_core.c files contains a lot of functions handling
> different thermal components like the governors, the trip points, the
> cooling device, the OF cooling device, etc ...
> 
> This organization does not help to migrate to a more sane code where
> there is a better self-encapsulation as all the components' internals
> can be directly accessed from a single file.
> 
> For the sake of clarity, let's move the thermal trip points code in a
> dedicated thermal_trip.c file and add a function to browse all the
> trip points like we do with the thermal zones, the govenors and the
> cooling devices.
> 
> The same can be done for the cooling devices and the governor code
> but
> that will come later as the current work in the thermal framework is
> to fix the trip point handling and use a generic trip point
> structure.

"No functional changes intended."

thanks,
rui

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/Makefile          |   4 +-
>  drivers/thermal/thermal_core.c    |  87 --------------
>  drivers/thermal/thermal_helpers.c |  62 ----------
>  drivers/thermal/thermal_trip.c    | 181
> ++++++++++++++++++++++++++++++
>  4 files changed, 183 insertions(+), 151 deletions(-)
>  create mode 100644 drivers/thermal/thermal_trip.c
> 
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 60f0dfa9aae2..2f0db88240b5 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -4,8 +4,8 @@
>  #
>  
>  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
> -thermal_sys-y			+= thermal_core.o
> thermal_sysfs.o \
> -					thermal_helpers.o
> +thermal_sys-y			+= thermal_core.o
> thermal_sysfs.o	
> +thermal_sys-y			+= thermal_trip.o
> thermal_helpers.o
>  
>  # netlink interface to manage the thermal framework
>  thermal_sys-$(CONFIG_THERMAL_NETLINK)		+=
> thermal_netlink.o
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index ea78c93277be..d0577685085a 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1155,12 +1155,6 @@ static void thermal_set_delay_jiffies(unsigned
> long *delay_jiffies, int delay_ms
>  		*delay_jiffies = round_jiffies(*delay_jiffies);
>  }
>  
> -int thermal_zone_get_num_trips(struct thermal_zone_device *tz)
> -{
> -	return tz->num_trips;
> -}
> -EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips);
> -
>  int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int
> *temp)
>  {
>  	int i, ret = -EINVAL;
> @@ -1187,87 +1181,6 @@ int thermal_zone_get_crit_temp(struct
> thermal_zone_device *tz, int *temp)
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>  
> -int __thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
> -			    struct thermal_trip *trip)
> -{
> -	int ret;
> -
> -	if (!tz || trip_id < 0 || trip_id >= tz->num_trips || !trip)
> -		return -EINVAL;
> -
> -	if (tz->trips) {
> -		*trip = tz->trips[trip_id];
> -		return 0;
> -	}
> -
> -	if (tz->ops->get_trip_hyst) {
> -		ret = tz->ops->get_trip_hyst(tz, trip_id, &trip-
> >hysteresis);
> -		if (ret)
> -			return ret;
> -	} else {
> -		trip->hysteresis = 0;
> -	}
> -
> -	ret = tz->ops->get_trip_temp(tz, trip_id, &trip->temperature);
> -	if (ret)
> -		return ret;
> -
> -	return tz->ops->get_trip_type(tz, trip_id, &trip->type);
> -}
> -EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
> -
> -int thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
> -			  struct thermal_trip *trip)
> -{
> -	int ret;
> -
> -	mutex_lock(&tz->lock);
> -	ret = __thermal_zone_get_trip(tz, trip_id, trip);
> -	mutex_unlock(&tz->lock);
> -
> -	return ret;
> -}
> -EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
> -
> -int thermal_zone_set_trip(struct thermal_zone_device *tz, int
> trip_id,
> -			  const struct thermal_trip *trip)
> -{
> -	struct thermal_trip t;
> -	int ret;
> -
> -	if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz-
> >trips)
> -		return -EINVAL;
> -
> -	ret = __thermal_zone_get_trip(tz, trip_id, &t);
> -	if (ret)
> -		return ret;
> -
> -	if (t.type != trip->type)
> -		return -EINVAL;
> -
> -	if (t.temperature != trip->temperature && tz->ops-
> >set_trip_temp) {
> -		ret = tz->ops->set_trip_temp(tz, trip_id, trip-
> >temperature);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) 
> {
> -		ret = tz->ops->set_trip_hyst(tz, trip_id, trip-
> >hysteresis);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	if (tz->trips && (t.temperature != trip->temperature ||
> t.hysteresis != trip->hysteresis))
> -		tz->trips[trip_id] = *trip;
> -
> -	thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
> -				      trip->temperature, trip-
> >hysteresis);
> -
> -	__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
> -	
> -	return 0;
> -}
> -
>  /**
>   * thermal_zone_device_register_with_trips() - register a new
> thermal zone device
>   * @type:	the thermal zone device type
> diff --git a/drivers/thermal/thermal_helpers.c
> b/drivers/thermal/thermal_helpers.c
> index 8977d5ddc23c..0f648131b0b5 100644
> --- a/drivers/thermal/thermal_helpers.c
> +++ b/drivers/thermal/thermal_helpers.c
> @@ -146,68 +146,6 @@ int thermal_zone_get_temp(struct
> thermal_zone_device *tz, int *temp)
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>  
> -/**
> - * __thermal_zone_set_trips - Computes the next trip points for the
> driver
> - * @tz: a pointer to a thermal zone device structure
> - *
> - * The function computes the next temperature boundaries by browsing
> - * the trip points. The result is the closer low and high trip
> points
> - * to the current temperature. These values are passed to the
> backend
> - * driver to let it set its own notification mechanism (usually an
> - * interrupt).
> - *
> - * This function must be called with tz->lock held. Both tz and tz-
> >ops
> - * must be valid pointers.
> - *
> - * It does not return a value
> - */
> -void __thermal_zone_set_trips(struct thermal_zone_device *tz)
> -{
> -	struct thermal_trip trip;
> -	int low = -INT_MAX, high = INT_MAX;
> -	int i, ret;
> -
> -	lockdep_assert_held(&tz->lock);
> -
> -	if (!tz->ops->set_trips)
> -		return;
> -
> -	for (i = 0; i < tz->num_trips; i++) {
> -		int trip_low;
> -
> -		ret = __thermal_zone_get_trip(tz, i , &trip);
> -		if (ret)
> -			return;
> -
> -		trip_low = trip.temperature - trip.hysteresis;
> -
> -		if (trip_low < tz->temperature && trip_low > low)
> -			low = trip_low;
> -
> -		if (trip.temperature > tz->temperature &&
> -		    trip.temperature < high)
> -			high = trip.temperature;
> -	}
> -
> -	/* No need to change trip points */
> -	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> -		return;
> -
> -	tz->prev_low_trip = low;
> -	tz->prev_high_trip = high;
> -
> -	dev_dbg(&tz->device,
> -		"new temperature boundaries: %d < x < %d\n", low,
> high);
> -
> -	/*
> -	 * Set a temperature window. When this window is left the
> driver
> -	 * must inform the thermal core via thermal_zone_device_update.
> -	 */
> -	ret = tz->ops->set_trips(tz, low, high);
> -	if (ret)
> -		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
> -}
> -
>  static void thermal_cdev_set_cur_state(struct thermal_cooling_device
> *cdev,
>  				       int target)
>  {
> diff --git a/drivers/thermal/thermal_trip.c
> b/drivers/thermal/thermal_trip.c
> new file mode 100644
> index 000000000000..2ef61ff7ffc3
> --- /dev/null
> +++ b/drivers/thermal/thermal_trip.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  Copyright (C) 2008 Intel Corp
> + *  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
> + *  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
> + *  Copyright 2022 Linaro Limited
> + *
> + * Thermal trips handling
> + */
> +#include "thermal_core.h"
> +
> +int __for_each_thermal_trip(struct thermal_zone_device *tz,
> +			    int (*cb)(struct thermal_trip *,
> +				      int trip_id, void *),
> +			    void *data)
> +{
> +	int i, ret;
> +	struct thermal_trip trip;
> +
> +	for (i = 0; i < tz->num_trips; i++) {
> +
> +		ret = __thermal_zone_get_trip(tz, i, &trip);
> +		if (ret)
> +			return ret;
> +		
> +		ret = cb(&trip, i, data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int thermal_zone_get_num_trips(struct thermal_zone_device *tz)
> +{
> +	return tz->num_trips;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips);
> +
> +/**
> + * __thermal_zone_set_trips - Computes the next trip points for the
> driver
> + * @tz: a pointer to a thermal zone device structure
> + *
> + * The function computes the next temperature boundaries by browsing
> + * the trip points. The result is the closer low and high trip
> points
> + * to the current temperature. These values are passed to the
> backend
> + * driver to let it set its own notification mechanism (usually an
> + * interrupt).
> + *
> + * This function must be called with tz->lock held. Both tz and tz-
> >ops
> + * must be valid pointers.
> + *
> + * It does not return a value
> + */
> +void __thermal_zone_set_trips(struct thermal_zone_device *tz)
> +{
> +	struct thermal_trip trip;
> +	int low = -INT_MAX, high = INT_MAX;
> +	int i, ret;
> +
> +	lockdep_assert_held(&tz->lock);
> +
> +	if (!tz->ops->set_trips)
> +		return;
> +
> +	for (i = 0; i < tz->num_trips; i++) {
> +		int trip_low;
> +
> +		ret = __thermal_zone_get_trip(tz, i , &trip);
> +		if (ret)
> +			return;
> +
> +		trip_low = trip.temperature - trip.hysteresis;
> +
> +		if (trip_low < tz->temperature && trip_low > low)
> +			low = trip_low;
> +
> +		if (trip.temperature > tz->temperature &&
> +		    trip.temperature < high)
> +			high = trip.temperature;
> +	}
> +
> +	/* No need to change trip points */
> +	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> +		return;
> +
> +	tz->prev_low_trip = low;
> +	tz->prev_high_trip = high;
> +
> +	dev_dbg(&tz->device,
> +		"new temperature boundaries: %d < x < %d\n", low,
> high);
> +
> +	/*
> +	 * Set a temperature window. When this window is left the
> driver
> +	 * must inform the thermal core via thermal_zone_device_update.
> +	 */
> +	ret = tz->ops->set_trips(tz, low, high);
> +	if (ret)
> +		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
> +}
> +
> +int __thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
> +			    struct thermal_trip *trip)
> +{
> +	int ret;
> +
> +	if (!tz || trip_id < 0 || trip_id >= tz->num_trips || !trip)
> +		return -EINVAL;
> +
> +	if (tz->trips) {
> +		*trip = tz->trips[trip_id];
> +		return 0;
> +	}
> +
> +	if (tz->ops->get_trip_hyst) {
> +		ret = tz->ops->get_trip_hyst(tz, trip_id, &trip-
> >hysteresis);
> +		if (ret)
> +			return ret;
> +	} else {
> +		trip->hysteresis = 0;
> +	}
> +
> +	ret = tz->ops->get_trip_temp(tz, trip_id, &trip->temperature);
> +	if (ret)
> +		return ret;
> +
> +	return tz->ops->get_trip_type(tz, trip_id, &trip->type);
> +}
> +EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
> +
> +int thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
> +			  struct thermal_trip *trip)
> +{
> +	int ret;
> +
> +	mutex_lock(&tz->lock);
> +	ret = __thermal_zone_get_trip(tz, trip_id, trip);
> +	mutex_unlock(&tz->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
> +
> +int thermal_zone_set_trip(struct thermal_zone_device *tz, int
> trip_id,
> +			  const struct thermal_trip *trip)
> +{
> +	struct thermal_trip t;
> +	int ret;
> +
> +	if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz-
> >trips)
> +		return -EINVAL;
> +
> +	ret = __thermal_zone_get_trip(tz, trip_id, &t);
> +	if (ret)
> +		return ret;
> +
> +	if (t.type != trip->type)
> +		return -EINVAL;
> +
> +	if (t.temperature != trip->temperature && tz->ops-
> >set_trip_temp) {
> +		ret = tz->ops->set_trip_temp(tz, trip_id, trip-
> >temperature);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) 
> {
> +		ret = tz->ops->set_trip_hyst(tz, trip_id, trip-
> >hysteresis);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (tz->trips && (t.temperature != trip->temperature ||
> t.hysteresis != trip->hysteresis))
> +		tz->trips[trip_id] = *trip;
> +
> +	thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
> +				      trip->temperature, trip-
> >hysteresis);
> +
> +	__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
> +	
> +	return 0;
> +}

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Zhang, Rui]

On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> If the thermal framework fails to initialize, the mutex can be used
> by
> the different functions registering a thermal zone anyway.

Hmm, even with no governors and unregistered thermal sysfs class?

IMO, thermal APIs for registering a thermal_zone/cooling_device should
yield early if thermal_init fails.
For other APIs that relies on a valid
thermal_zone_device/thermal_cooling_device pointer, nothing needs to
be changed.

what do you think?

thanks,
rui


>  We should
> not destroy the mutexes as other components may use them.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index fad0c4a07d16..ea78c93277be 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1602,7 +1602,7 @@ static int __init thermal_init(void)
>  
>  	result = thermal_netlink_init();
>  	if (result)
> -		goto error;
> +		return result;
>  
>  	result = thermal_register_governors();
>  	if (result)
> @@ -1623,9 +1623,7 @@ static int __init thermal_init(void)
>  	thermal_unregister_governors();
>  unregister_netlink:
>  	thermal_netlink_exit();
> -error:
> -	mutex_destroy(&thermal_list_lock);
> -	mutex_destroy(&thermal_governor_lock);
> +
>  	return result;
>  }
>  postcore_initcall(thermal_init);

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Daniel Lezcano]

On 19/01/2023 08:41, Zhang, Rui wrote:
> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
>> If the thermal framework fails to initialize, the mutex can be used
>> by
>> the different functions registering a thermal zone anyway.
> 
> Hmm, even with no governors and unregistered thermal sysfs class?
> 
> IMO, thermal APIs for registering a thermal_zone/cooling_device should
> yield early if thermal_init fails.
> For other APIs that relies on a valid
> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> be changed.
> 
> what do you think?

I think you are right.

It would be nice if we can check if the thermal class is registered and 
bail out if not. But there is no function to check that AFAICS.

Alternatively we can convert the thermal class static structure to a 
pointer and set it to NULL in case of error in thermal_init() ?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 5/5] thermal/core: Sort the trip points when registering a thermal zone

[Daniel Lezcano]

Hi Rui,

On 19/01/2023 08:22, Zhang, Rui wrote:
> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:

[ ... ]

>> +int thermal_trip_sort(struct thermal_trip *trips, int num_trips)
>> +{
>> +	struct thermal_trip tt;
>> +	int sorted = 0;
>> +	int i, j;
>> +
>> +	for (i = 0; i < num_trips; i++) {
>> +
>> +		for (j = i + 1; j < num_trips; j++) {
>> +
>> +			if (trips[i].temperature <
>> trips[j].temperature) {
>> +				tt = trips[i];
>> +				trips[i] = trips[j];
>> +				trips[j] = tt;
>> +				sorted++;
>> +			}
>> +		}
>> + 	}
>> +
>> +	return sorted;
>> +}
>> +
> When this happens, the index(trip_id) of each trip is changed, but we
> pass the new trip_id to .get_trip_temp()/.set_trip_temp() callbacks.

If we pass the thermal trips to the 
thermal_zone_device_register_with_trips(), .get_trip_temp, 
.get_trip_hyst and .get_trip_type are not used.

.set_trip_temp is called from sysfs, where the trip_id is read from the 
file name. This trip_id will be correct in this case, as the files are 
created after sorting the array.

> This will confuse the drivers and update the wrong trips, right?

No, because at the moment we use the generic trip structure, it is 
handled by the thermal framework.

The drivers do not have to deal with the trip id or assuming its value 
given a trip point after registering the thermal zone. If it does, we 
should fix the driver as the trip_id is a framework internal value.

The trip_id is just an index to be passed around, so whatever the value, 
it should point to the right trip point.

For instance, the device tree describes the trip point and they could be 
in any order, all the DT based drivers are agnostic of the trip_id.

If there is an update of the trip points, we read the trip points 
definition again and do an update of all of them.

> IMO, we need a map between thermal core trips and unsorted driver
> trips.

That what I proposed several months ago but we concluded that would 
another extra level of complexity. So we decided to replace all the 
.get_trip_* by a generic trip point structure handled by the thermal 
framework itself.

Hopefully, soon we will have all .get_trip* gone with a nice cleanup of 
the drivers and the core framework.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Rafael J. Wysocki]

On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/01/2023 08:41, Zhang, Rui wrote:
> > On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> >> If the thermal framework fails to initialize, the mutex can be used
> >> by
> >> the different functions registering a thermal zone anyway.
> >
> > Hmm, even with no governors and unregistered thermal sysfs class?
> >
> > IMO, thermal APIs for registering a thermal_zone/cooling_device should
> > yield early if thermal_init fails.
> > For other APIs that relies on a valid
> > thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> > be changed.
> >
> > what do you think?
>
> I think you are right.
>
> It would be nice if we can check if the thermal class is registered and
> bail out if not. But there is no function to check that AFAICS.
>
> Alternatively we can convert the thermal class static structure to a
> pointer and set it to NULL in case of error in thermal_init() ?

It doesn't matter if this is a NULL pointer or a static object that's
clearly marked as unused.

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Daniel Lezcano]

On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 19/01/2023 08:41, Zhang, Rui wrote:
>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
>>>> If the thermal framework fails to initialize, the mutex can be used
>>>> by
>>>> the different functions registering a thermal zone anyway.
>>>
>>> Hmm, even with no governors and unregistered thermal sysfs class?
>>>
>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
>>> yield early if thermal_init fails.
>>> For other APIs that relies on a valid
>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
>>> be changed.
>>>
>>> what do you think?
>>
>> I think you are right.
>>
>> It would be nice if we can check if the thermal class is registered and
>> bail out if not. But there is no function to check that AFAICS.
>>
>> Alternatively we can convert the thermal class static structure to a
>> pointer and set it to NULL in case of error in thermal_init() ?
> 
> It doesn't matter if this is a NULL pointer or a static object that's
> clearly marked as unused.

Without introducing another global variable, is it possible to know if 
the class is used or not ?

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Rafael J. Wysocki]

On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> > On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 19/01/2023 08:41, Zhang, Rui wrote:
> >>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> >>>> If the thermal framework fails to initialize, the mutex can be used
> >>>> by
> >>>> the different functions registering a thermal zone anyway.
> >>>
> >>> Hmm, even with no governors and unregistered thermal sysfs class?
> >>>
> >>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
> >>> yield early if thermal_init fails.
> >>> For other APIs that relies on a valid
> >>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> >>> be changed.
> >>>
> >>> what do you think?
> >>
> >> I think you are right.
> >>
> >> It would be nice if we can check if the thermal class is registered and
> >> bail out if not. But there is no function to check that AFAICS.
> >>
> >> Alternatively we can convert the thermal class static structure to a
> >> pointer and set it to NULL in case of error in thermal_init() ?
> >
> > It doesn't matter if this is a NULL pointer or a static object that's
> > clearly marked as unused.
>
> Without introducing another global variable, is it possible to know if
> the class is used or not ?

If thermal_class.p is cleared to NULL on class_register() failures in
thermal_init() (unfortunately, the driver core doesn't do that, but
maybe it should - let me cut a patch for that), then it can be used
for that.

Re: [PATCH 1/5] thermal/core: Fix unregistering netlink at thermal init time

[Rafael J. Wysocki]

On Wed, Jan 18, 2023 at 10:11 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The thermal subsystem initialization miss an netlink unregistering
> function in the error. Add it.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

What tree is this series based on?

> ---
>  drivers/thermal/thermal_core.c    | 4 +++-
>  drivers/thermal/thermal_netlink.c | 5 +++++
>  drivers/thermal/thermal_netlink.h | 3 +++
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9a3d9566d73..fddafcee5e6f 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1606,7 +1606,7 @@ static int __init thermal_init(void)
>
>         result = thermal_register_governors();
>         if (result)
> -               goto error;
> +               goto unregister_netlink;
>
>         result = class_register(&thermal_class);
>         if (result)
> @@ -1621,6 +1621,8 @@ static int __init thermal_init(void)
>
>  unregister_governors:
>         thermal_unregister_governors();
> +unregister_netlink:
> +       thermal_netlink_exit();
>  error:
>         ida_destroy(&thermal_tz_ida);
>         ida_destroy(&thermal_cdev_ida);
> diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
> index 75943b06dbe7..08bc46c3ec7b 100644
> --- a/drivers/thermal/thermal_netlink.c
> +++ b/drivers/thermal/thermal_netlink.c
> @@ -699,3 +699,8 @@ int __init thermal_netlink_init(void)
>  {
>         return genl_register_family(&thermal_gnl_family);
>  }
> +
> +void __init thermal_netlink_exit(void)
> +{
> +       genl_unregister_family(&thermal_gnl_family);
> +}
> diff --git a/drivers/thermal/thermal_netlink.h b/drivers/thermal/thermal_netlink.h
> index 1052f523188d..0a9987c3bc57 100644
> --- a/drivers/thermal/thermal_netlink.h
> +++ b/drivers/thermal/thermal_netlink.h
> @@ -13,6 +13,7 @@ struct thermal_genl_cpu_caps {
>  /* Netlink notification function */
>  #ifdef CONFIG_THERMAL_NETLINK
>  int __init thermal_netlink_init(void);
> +void __init thermal_netlink_exit(void);
>  int thermal_notify_tz_create(int tz_id, const char *name);
>  int thermal_notify_tz_delete(int tz_id);
>  int thermal_notify_tz_enable(int tz_id);
> @@ -115,4 +116,6 @@ static inline int thermal_genl_cpu_capability_event(int count, struct thermal_ge
>         return 0;
>  }
>
> +static inline void __init thermal_netlink_exit(void) {}
> +
>  #endif /* CONFIG_THERMAL_NETLINK */
> --
> 2.34.1
>

Re: [PATCH 1/5] thermal/core: Fix unregistering netlink at thermal init time

[Daniel Lezcano]

On 19/01/2023 14:28, Rafael J. Wysocki wrote:
> On Wed, Jan 18, 2023 at 10:11 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> The thermal subsystem initialization miss an netlink unregistering
>> function in the error. Add it.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> What tree is this series based on?

It is based on 
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/linux-next



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/5] thermal/core: Fix unregistering netlink at thermal init time

[Rafael J. Wysocki]

On Thu, Jan 19, 2023 at 2:40 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/01/2023 14:28, Rafael J. Wysocki wrote:
> > On Wed, Jan 18, 2023 at 10:11 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> The thermal subsystem initialization miss an netlink unregistering
> >> function in the error. Add it.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >
> > What tree is this series based on?
>
> It is based on
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/linux-next

OK

Can this information be added to the patches or to a cover letter of
the series next time?

Re: [PATCH 1/5] thermal/core: Fix unregistering netlink at thermal init time

[Daniel Lezcano]

On 19/01/2023 14:45, Rafael J. Wysocki wrote:
> On Thu, Jan 19, 2023 at 2:40 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 19/01/2023 14:28, Rafael J. Wysocki wrote:
>>> On Wed, Jan 18, 2023 at 10:11 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> The thermal subsystem initialization miss an netlink unregistering
>>>> function in the error. Add it.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>> What tree is this series based on?
>>
>> It is based on
>> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/linux-next
> 
> OK
> 
> Can this information be added to the patches or to a cover letter of
> the series next time?

Sure

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Daniel Lezcano]

On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
>>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 19/01/2023 08:41, Zhang, Rui wrote:
>>>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
>>>>>> If the thermal framework fails to initialize, the mutex can be used
>>>>>> by
>>>>>> the different functions registering a thermal zone anyway.
>>>>>
>>>>> Hmm, even with no governors and unregistered thermal sysfs class?
>>>>>
>>>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
>>>>> yield early if thermal_init fails.
>>>>> For other APIs that relies on a valid
>>>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
>>>>> be changed.
>>>>>
>>>>> what do you think?
>>>>
>>>> I think you are right.
>>>>
>>>> It would be nice if we can check if the thermal class is registered and
>>>> bail out if not. But there is no function to check that AFAICS.
>>>>
>>>> Alternatively we can convert the thermal class static structure to a
>>>> pointer and set it to NULL in case of error in thermal_init() ?
>>>
>>> It doesn't matter if this is a NULL pointer or a static object that's
>>> clearly marked as unused.
>>
>> Without introducing another global variable, is it possible to know if
>> the class is used or not ?
> 
> If thermal_class.p is cleared to NULL on class_register() failures in
> thermal_init() (unfortunately, the driver core doesn't do that, but
> maybe it should - let me cut a patch for that), then it can be used
> for that.

It should be in class_unregister() too, right ?

And is it possible to add a class_is_registered() ? in order to prevent 
accessing class structure internals ?

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Rafael J. Wysocki]

On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> > On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> >>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> On 19/01/2023 08:41, Zhang, Rui wrote:
> >>>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> >>>>>> If the thermal framework fails to initialize, the mutex can be used
> >>>>>> by
> >>>>>> the different functions registering a thermal zone anyway.
> >>>>>
> >>>>> Hmm, even with no governors and unregistered thermal sysfs class?
> >>>>>
> >>>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
> >>>>> yield early if thermal_init fails.
> >>>>> For other APIs that relies on a valid
> >>>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> >>>>> be changed.
> >>>>>
> >>>>> what do you think?
> >>>>
> >>>> I think you are right.
> >>>>
> >>>> It would be nice if we can check if the thermal class is registered and
> >>>> bail out if not. But there is no function to check that AFAICS.
> >>>>
> >>>> Alternatively we can convert the thermal class static structure to a
> >>>> pointer and set it to NULL in case of error in thermal_init() ?
> >>>
> >>> It doesn't matter if this is a NULL pointer or a static object that's
> >>> clearly marked as unused.
> >>
> >> Without introducing another global variable, is it possible to know if
> >> the class is used or not ?
> >
> > If thermal_class.p is cleared to NULL on class_register() failures in
> > thermal_init() (unfortunately, the driver core doesn't do that, but
> > maybe it should - let me cut a patch for that), then it can be used
> > for that.
>
> It should be in class_unregister() too, right ?
>
> And is it possible to add a class_is_registered() ? in order to prevent
> accessing class structure internals ?

I suppose so.

And we'd like it to be used some places like
thermal_zone_device_register_with_trips(), wouldn't we?

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Daniel Lezcano]

On 19/01/2023 16:05, Rafael J. Wysocki wrote:
> On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 19/01/2023 14:24, Rafael J. Wysocki wrote:
>>> On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
>>>>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>
>>>>>> On 19/01/2023 08:41, Zhang, Rui wrote:
>>>>>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
>>>>>>>> If the thermal framework fails to initialize, the mutex can be used
>>>>>>>> by
>>>>>>>> the different functions registering a thermal zone anyway.
>>>>>>>
>>>>>>> Hmm, even with no governors and unregistered thermal sysfs class?
>>>>>>>
>>>>>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
>>>>>>> yield early if thermal_init fails.
>>>>>>> For other APIs that relies on a valid
>>>>>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
>>>>>>> be changed.
>>>>>>>
>>>>>>> what do you think?
>>>>>>
>>>>>> I think you are right.
>>>>>>
>>>>>> It would be nice if we can check if the thermal class is registered and
>>>>>> bail out if not. But there is no function to check that AFAICS.
>>>>>>
>>>>>> Alternatively we can convert the thermal class static structure to a
>>>>>> pointer and set it to NULL in case of error in thermal_init() ?
>>>>>
>>>>> It doesn't matter if this is a NULL pointer or a static object that's
>>>>> clearly marked as unused.
>>>>
>>>> Without introducing another global variable, is it possible to know if
>>>> the class is used or not ?
>>>
>>> If thermal_class.p is cleared to NULL on class_register() failures in
>>> thermal_init() (unfortunately, the driver core doesn't do that, but
>>> maybe it should - let me cut a patch for that), then it can be used
>>> for that.
>>
>> It should be in class_unregister() too, right ?
>>
>> And is it possible to add a class_is_registered() ? in order to prevent
>> accessing class structure internals ?
> 
> I suppose so.
> 
> And we'd like it to be used some places like
> thermal_zone_device_register_with_trips(), wouldn't we?

Yes, in thermal_zone_device_register_with_trips() and 
thermal_cooling_device_register().



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 5/5] thermal/core: Sort the trip points when registering a thermal zone

[Zhang, Rui]

On Thu, 2023-01-19 at 11:25 +0100, Daniel Lezcano wrote:
> Hi Rui,
> 
> On 19/01/2023 08:22, Zhang, Rui wrote:
> > On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> 
> [ ... ]
> 
> > > +int thermal_trip_sort(struct thermal_trip *trips, int num_trips)
> > > +{
> > > +	struct thermal_trip tt;
> > > +	int sorted = 0;
> > > +	int i, j;
> > > +
> > > +	for (i = 0; i < num_trips; i++) {
> > > +
> > > +		for (j = i + 1; j < num_trips; j++) {
> > > +
> > > +			if (trips[i].temperature <
> > > trips[j].temperature) {
> > > +				tt = trips[i];
> > > +				trips[i] = trips[j];
> > > +				trips[j] = tt;
> > > +				sorted++;
> > > +			}
> > > +		}
> > > + 	}
> > > +
> > > +	return sorted;
> > > +}
> > > +
> > When this happens, the index(trip_id) of each trip is changed, but
> > we
> > pass the new trip_id to .get_trip_temp()/.set_trip_temp()
> > callbacks.
> 
> If we pass the thermal trips to the 
> thermal_zone_device_register_with_trips(), .get_trip_temp, 
> .get_trip_hyst and .get_trip_type are not used.

agreed.

> 
> .set_trip_temp is called from sysfs, where the trip_id is read from
> the 
> file name.

yes.

>  This trip_id will be correct in this case, as the files are 
> created after sorting the array.

yes, the trip_id from sysfs matches its index in tz->trips[].

> 
> > This will confuse the drivers and update the wrong trips, right?
> 
> No, because at the moment we use the generic trip structure, it is 
> handled by the thermal framework.
> 
> The drivers do not have to deal with the trip id or assuming its
> value 
> given a trip point after registering the thermal zone. If it does,
> we 
> should fix the driver as the trip_id is a framework internal value.

I didn't quite follow this.
Please correct me if my understanding is wrong,

Say, driver supports two writable trip point A and B, B has higher
temperature but it is set in trips[0] when the driver registers the
thermal device.

After thermal_trip_sort(), B becomes trips[1], and its sysfs attribute
is shown as trip_point_1_xxx, right?

When setting the trip B temperature, trip_id is 1, and we invoke
	tz->ops->set_trip_temp(tz, trip_id, trip->temperature);

In the driver, the .set_trip_temp() callback updates trip A instead of
trip B because trip_id == 1 stands for trip A from the drivers
perspective of view, right?

You can refer to the .set_trip_temp() callback of x86_pkg_temp_thermal.
c which handles two trip points.

> 
> The trip_id is just an index to be passed around, so whatever the
> value, 
> it should point to the right trip point.
> 
> For instance, the device tree describes the trip point and they could
> be 
> in any order, all the DT based drivers are agnostic of the trip_id.
> 
> If there is an update of the trip points, we read the trip points 
> definition again and do an update of all of them.
> 
> > IMO, we need a map between thermal core trips and unsorted driver
> > trips.
> 
> That what I proposed several months ago but we concluded that would 
> another extra level of complexity. So we decided to replace all the 
> .get_trip_* by a generic trip point structure handled by the thermal 
> framework itself.

If the problem is valid, maybe we can add an 'orig_id' to struct
thermal_trip for the driver to reference?

thanks,
rui

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Rafael J. Wysocki]

On Thursday, January 19, 2023 5:39:29 PM CET Daniel Lezcano wrote:
> On 19/01/2023 16:05, Rafael J. Wysocki wrote:
> > On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> >>> On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> >>>>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> >>>>> <daniel.lezcano@linaro.org> wrote:
> >>>>>>
> >>>>>> On 19/01/2023 08:41, Zhang, Rui wrote:
> >>>>>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> >>>>>>>> If the thermal framework fails to initialize, the mutex can be used
> >>>>>>>> by
> >>>>>>>> the different functions registering a thermal zone anyway.
> >>>>>>>
> >>>>>>> Hmm, even with no governors and unregistered thermal sysfs class?
> >>>>>>>
> >>>>>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
> >>>>>>> yield early if thermal_init fails.
> >>>>>>> For other APIs that relies on a valid
> >>>>>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> >>>>>>> be changed.
> >>>>>>>
> >>>>>>> what do you think?
> >>>>>>
> >>>>>> I think you are right.
> >>>>>>
> >>>>>> It would be nice if we can check if the thermal class is registered and
> >>>>>> bail out if not. But there is no function to check that AFAICS.
> >>>>>>
> >>>>>> Alternatively we can convert the thermal class static structure to a
> >>>>>> pointer and set it to NULL in case of error in thermal_init() ?
> >>>>>
> >>>>> It doesn't matter if this is a NULL pointer or a static object that's
> >>>>> clearly marked as unused.
> >>>>
> >>>> Without introducing another global variable, is it possible to know if
> >>>> the class is used or not ?
> >>>
> >>> If thermal_class.p is cleared to NULL on class_register() failures in
> >>> thermal_init() (unfortunately, the driver core doesn't do that, but
> >>> maybe it should - let me cut a patch for that), then it can be used
> >>> for that.
> >>
> >> It should be in class_unregister() too, right ?
> >>
> >> And is it possible to add a class_is_registered() ? in order to prevent
> >> accessing class structure internals ?
> > 
> > I suppose so.
> > 
> > And we'd like it to be used some places like
> > thermal_zone_device_register_with_trips(), wouldn't we?
> 
> Yes, in thermal_zone_device_register_with_trips() and 
> thermal_cooling_device_register().

Something like the patch below I think, because thermal_cooling_device_register()
is a wrapper around thermal_zone_device_register_with_trips().

It needs to be split into 2 individual patches.

---
 drivers/base/class.c           |   16 +++++++++++-----
 drivers/thermal/thermal_core.c |    3 +++
 include/linux/device/class.h   |    5 +++++
 3 files changed, 19 insertions(+), 5 deletions(-)

Index: linux-pm/include/linux/device/class.h
===================================================================
--- linux-pm.orig/include/linux/device/class.h
+++ linux-pm/include/linux/device/class.h
@@ -82,6 +82,11 @@ struct class_dev_iter {
 	const struct device_type	*type;
 };
 
+static inline bool class_is_registered(struct class *class)
+{
+	return !!class->p;
+}
+
 extern struct kobject *sysfs_dev_block_kobj;
 extern struct kobject *sysfs_dev_char_kobj;
 extern int __must_check __class_register(struct class *class,
Index: linux-pm/drivers/base/class.c
===================================================================
--- linux-pm.orig/drivers/base/class.c
+++ linux-pm/drivers/base/class.c
@@ -53,6 +53,8 @@ static void class_release(struct kobject
 
 	pr_debug("class '%s': release.\n", class->name);
 
+	class->p = NULL;
+
 	if (class->class_release)
 		class->class_release(class);
 	else
@@ -186,17 +188,21 @@ int __class_register(struct class *cls,
 	cls->p = cp;
 
 	error = kset_register(&cp->subsys);
-	if (error) {
-		kfree(cp);
-		return error;
-	}
+	if (error)
+		goto err_out;
+
 	error = class_add_groups(class_get(cls), cls->class_groups);
 	class_put(cls);
 	if (error) {
 		kobject_del(&cp->subsys.kobj);
 		kfree_const(cp->subsys.kobj.name);
-		kfree(cp);
+		goto err_out;
 	}
+	return 0;
+
+err_out:
+	cls->p = NULL;
+	kfree(cp);
 	return error;
 }
 EXPORT_SYMBOL_GPL(__class_register);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1342,6 +1342,9 @@ thermal_zone_device_register_with_trips(
 	if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
 		return ERR_PTR(-EINVAL);
 
+	if (!class_is_registered(&thermal_class))
+		return ERR_PTR(-ENODEV);
+
 	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
 	if (!tz)
 		return ERR_PTR(-ENOMEM);

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Zhang, Rui]

On Thu, 2023-01-19 at 18:21 +0100, Rafael J. Wysocki wrote:
> On Thursday, January 19, 2023 5:39:29 PM CET Daniel Lezcano wrote:
> > On 19/01/2023 16:05, Rafael J. Wysocki wrote:
> > > On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
> > > <daniel.lezcano@linaro.org> wrote:
> > > > On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> > > > > On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> > > > > <daniel.lezcano@linaro.org> wrote:
> > > > > > On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> > > > > > > On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> > > > > > > <daniel.lezcano@linaro.org> wrote:
> > > > > > > > On 19/01/2023 08:41, Zhang, Rui wrote:
> > > > > > > > > On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano
> > > > > > > > > wrote:
> > > > > > > > > > If the thermal framework fails to initialize, the
> > > > > > > > > > mutex can be used
> > > > > > > > > > by
> > > > > > > > > > the different functions registering a thermal zone
> > > > > > > > > > anyway.
> > > > > > > > > 
> > > > > > > > > Hmm, even with no governors and unregistered thermal
> > > > > > > > > sysfs class?
> > > > > > > > > 
> > > > > > > > > IMO, thermal APIs for registering a
> > > > > > > > > thermal_zone/cooling_device should
> > > > > > > > > yield early if thermal_init fails.
> > > > > > > > > For other APIs that relies on a valid
> > > > > > > > > thermal_zone_device/thermal_cooling_device pointer,
> > > > > > > > > nothing needs to
> > > > > > > > > be changed.
> > > > > > > > > 
> > > > > > > > > what do you think?
> > > > > > > > 
> > > > > > > > I think you are right.
> > > > > > > > 
> > > > > > > > It would be nice if we can check if the thermal class
> > > > > > > > is registered and
> > > > > > > > bail out if not. But there is no function to check that
> > > > > > > > AFAICS.
> > > > > > > > 
> > > > > > > > Alternatively we can convert the thermal class static
> > > > > > > > structure to a
> > > > > > > > pointer and set it to NULL in case of error in
> > > > > > > > thermal_init() ?
> > > > > > > 
> > > > > > > It doesn't matter if this is a NULL pointer or a static
> > > > > > > object that's
> > > > > > > clearly marked as unused.
> > > > > > 
> > > > > > Without introducing another global variable, is it possible
> > > > > > to know if
> > > > > > the class is used or not ?
> > > > > 
> > > > > If thermal_class.p is cleared to NULL on class_register()
> > > > > failures in
> > > > > thermal_init() (unfortunately, the driver core doesn't do
> > > > > that, but
> > > > > maybe it should - let me cut a patch for that), then it can
> > > > > be used
> > > > > for that.
> > > > 
> > > > It should be in class_unregister() too, right ?
> > > > 
> > > > And is it possible to add a class_is_registered() ? in order to
> > > > prevent
> > > > accessing class structure internals ?
> > > 
> > > I suppose so.
> > > 
> > > And we'd like it to be used some places like
> > > thermal_zone_device_register_with_trips(), wouldn't we?
> > 
> > Yes, in thermal_zone_device_register_with_trips() and 
> > thermal_cooling_device_register().
> 
> Something like the patch below I think, because
> thermal_cooling_device_register()
> is a wrapper around thermal_zone_device_register_with_trips().
> 

thermal_zone_device_register() is a wrapper around
thermal_zone_device_register_with_trips(), but
thermal_cooling_device_register() is not. :)

thermal_cooling_device_register() registers a cooling device to thermal
class so the class_is_registered() check is still needed.

thanks,
rui

> It needs to be split into 2 individual patches.
> 
> ---
>  drivers/base/class.c           |   16 +++++++++++-----
>  drivers/thermal/thermal_core.c |    3 +++
>  include/linux/device/class.h   |    5 +++++
>  3 files changed, 19 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/include/linux/device/class.h
> ===================================================================
> --- linux-pm.orig/include/linux/device/class.h
> +++ linux-pm/include/linux/device/class.h
> @@ -82,6 +82,11 @@ struct class_dev_iter {
>  	const struct device_type	*type;
>  };
>  
> +static inline bool class_is_registered(struct class *class)
> +{
> +	return !!class->p;
> +}
> +
>  extern struct kobject *sysfs_dev_block_kobj;
>  extern struct kobject *sysfs_dev_char_kobj;
>  extern int __must_check __class_register(struct class *class,
> Index: linux-pm/drivers/base/class.c
> ===================================================================
> --- linux-pm.orig/drivers/base/class.c
> +++ linux-pm/drivers/base/class.c
> @@ -53,6 +53,8 @@ static void class_release(struct kobject
>  
>  	pr_debug("class '%s': release.\n", class->name);
>  
> +	class->p = NULL;
> +
>  	if (class->class_release)
>  		class->class_release(class);
>  	else
> @@ -186,17 +188,21 @@ int __class_register(struct class *cls,
>  	cls->p = cp;
>  
>  	error = kset_register(&cp->subsys);
> -	if (error) {
> -		kfree(cp);
> -		return error;
> -	}
> +	if (error)
> +		goto err_out;
> +
>  	error = class_add_groups(class_get(cls), cls->class_groups);
>  	class_put(cls);
>  	if (error) {
>  		kobject_del(&cp->subsys.kobj);
>  		kfree_const(cp->subsys.kobj.name);
> -		kfree(cp);
> +		goto err_out;
>  	}
> +	return 0;
> +
> +err_out:
> +	cls->p = NULL;
> +	kfree(cp);
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(__class_register);
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1342,6 +1342,9 @@ thermal_zone_device_register_with_trips(
>  	if (num_trips > 0 && (!ops->get_trip_type || !ops-
> >get_trip_temp) && !trips)
>  		return ERR_PTR(-EINVAL);
>  
> +	if (!class_is_registered(&thermal_class))
> +		return ERR_PTR(-ENODEV);
> +
>  	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
>  	if (!tz)
>  		return ERR_PTR(-ENOMEM);
> 
> 
> 

Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy()

[Rafael J. Wysocki]

On Fri, Jan 20, 2023 at 3:10 PM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Thu, 2023-01-19 at 18:21 +0100, Rafael J. Wysocki wrote:
> > On Thursday, January 19, 2023 5:39:29 PM CET Daniel Lezcano wrote:
> > > On 19/01/2023 16:05, Rafael J. Wysocki wrote:
> > > > On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
> > > > <daniel.lezcano@linaro.org> wrote:
> > > > > On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> > > > > > On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> > > > > > <daniel.lezcano@linaro.org> wrote:
> > > > > > > On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> > > > > > > > On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> > > > > > > > <daniel.lezcano@linaro.org> wrote:
> > > > > > > > > On 19/01/2023 08:41, Zhang, Rui wrote:
> > > > > > > > > > On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano
> > > > > > > > > > wrote:
> > > > > > > > > > > If the thermal framework fails to initialize, the
> > > > > > > > > > > mutex can be used
> > > > > > > > > > > by
> > > > > > > > > > > the different functions registering a thermal zone
> > > > > > > > > > > anyway.
> > > > > > > > > >
> > > > > > > > > > Hmm, even with no governors and unregistered thermal
> > > > > > > > > > sysfs class?
> > > > > > > > > >
> > > > > > > > > > IMO, thermal APIs for registering a
> > > > > > > > > > thermal_zone/cooling_device should
> > > > > > > > > > yield early if thermal_init fails.
> > > > > > > > > > For other APIs that relies on a valid
> > > > > > > > > > thermal_zone_device/thermal_cooling_device pointer,
> > > > > > > > > > nothing needs to
> > > > > > > > > > be changed.
> > > > > > > > > >
> > > > > > > > > > what do you think?
> > > > > > > > >
> > > > > > > > > I think you are right.
> > > > > > > > >
> > > > > > > > > It would be nice if we can check if the thermal class
> > > > > > > > > is registered and
> > > > > > > > > bail out if not. But there is no function to check that
> > > > > > > > > AFAICS.
> > > > > > > > >
> > > > > > > > > Alternatively we can convert the thermal class static
> > > > > > > > > structure to a
> > > > > > > > > pointer and set it to NULL in case of error in
> > > > > > > > > thermal_init() ?
> > > > > > > >
> > > > > > > > It doesn't matter if this is a NULL pointer or a static
> > > > > > > > object that's
> > > > > > > > clearly marked as unused.
> > > > > > >
> > > > > > > Without introducing another global variable, is it possible
> > > > > > > to know if
> > > > > > > the class is used or not ?
> > > > > >
> > > > > > If thermal_class.p is cleared to NULL on class_register()
> > > > > > failures in
> > > > > > thermal_init() (unfortunately, the driver core doesn't do
> > > > > > that, but
> > > > > > maybe it should - let me cut a patch for that), then it can
> > > > > > be used
> > > > > > for that.
> > > > >
> > > > > It should be in class_unregister() too, right ?
> > > > >
> > > > > And is it possible to add a class_is_registered() ? in order to
> > > > > prevent
> > > > > accessing class structure internals ?
> > > >
> > > > I suppose so.
> > > >
> > > > And we'd like it to be used some places like
> > > > thermal_zone_device_register_with_trips(), wouldn't we?
> > >
> > > Yes, in thermal_zone_device_register_with_trips() and
> > > thermal_cooling_device_register().
> >
> > Something like the patch below I think, because
> > thermal_cooling_device_register()
> > is a wrapper around thermal_zone_device_register_with_trips().
> >
>
> thermal_zone_device_register() is a wrapper around
> thermal_zone_device_register_with_trips(), but
> thermal_cooling_device_register() is not. :)
>
> thermal_cooling_device_register() registers a cooling device to thermal
> class so the class_is_registered() check is still needed.

OK, thanks!