[PATCH v2 0/3] Misc thermal cleanup and fixes

[PATCH v2 0/3] Misc thermal cleanup and fixes

[Daniel Lezcano]

This series is based on linux-pm/thermal

It unregisters the netlink generic family for thermal in case the thermal
framework fails to initialize, removes a unneeded ida_destroy() call and moves
the thermal trip code in a separate file.

A couple of changes have been removed from the previous version:

 - Ordering the trip points. Some consolidation is needed in the set_trip_temp
   ops before ordering the trip points, so this change is postpone

 - Remove the mutex destroy call. Even if the call is not needed, the
   justification of the change is not accurate. So it is postponed also.

 V2:
   - Removed ordering the trip points change
   - Removed mutex destroy change
   - Added "No functional change intented" as requested by Rui
   - Added the function declaration for_each_thermal_trip

Daniel Lezcano (3):
  thermal/core: Fix unregistering netlink at thermal init time
  thermal/core: Remove unneeded ida_destroy()
  thermal/core: Move the thermal trip code to a dedicated file

 drivers/thermal/Makefile          |   4 +-
 drivers/thermal/thermal_core.c    |  93 +--------------
 drivers/thermal/thermal_core.h    |   4 +
 drivers/thermal/thermal_helpers.c |  62 ----------
 drivers/thermal/thermal_netlink.c |   5 +
 drivers/thermal/thermal_netlink.h |   3 +
 drivers/thermal/thermal_trip.c    | 182 ++++++++++++++++++++++++++++++
 7 files changed, 199 insertions(+), 154 deletions(-)
 create mode 100644 drivers/thermal/thermal_trip.c

-- 
2.34.1

[PATCH v2 1/3] 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 v2 2/3] 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 v2 3/3] 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.

No functional changes intended.

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

diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 2506c6c8ca83..2faf4651f34a 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 fad0c4a07d16..4ee685043a3e 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_core.h b/drivers/thermal/thermal_core.h
index 26350206a98d..7af54382e915 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -52,6 +52,10 @@ int for_each_thermal_cooling_device(int (*cb)(struct thermal_cooling_device *,
 int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
 			      void *thermal_governor);
 
+int __for_each_thermal_trip(struct thermal_zone_device *,
+			    int (*cb)(struct thermal_trip *, void *),
+			    void *);
+
 struct thermal_zone_device *thermal_zone_get_by_id(int id);
 
 struct thermal_attr {
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..5ce71f1cb1ba
--- /dev/null
+++ b/drivers/thermal/thermal_trip.c
@@ -0,0 +1,182 @@
+// 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 *, void *),
+			    void *data)
+{
+	int i, ret;
+	struct thermal_trip trip;
+
+	lockdep_assert_held(&tz->lock);
+
+	for (i = 0; i < tz->num_trips; i++) {
+
+		ret = __thermal_zone_get_trip(tz, i, &trip);
+		if (ret)
+			return ret;
+		
+		ret = cb(&trip, 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

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

[Zhang, Rui]

On Mon, 2023-01-23 at 16:27 +0100, Daniel Lezcano wrote:
> The thermal subsystem initialization miss an netlink

s/miss an netlink/misses a netlink

other than that, the patch looks good to me.

Reviewed-by: Zhang Rui <rui.zhang@intel.com>

-rui

>  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 */

Re: [PATCH v2 2/3] thermal/core: Remove unneeded ida_destroy()

[Zhang, Rui]

On Mon, 2023-01-23 at 16:27 +0100, Daniel Lezcano wrote:
> 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>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui
> ---
>  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;

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

[Zhang, Rui]

On Mon, 2023-01-23 at 16:27 +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.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
>  drivers/thermal/Makefile          |   4 +-
>  drivers/thermal/thermal_core.c    |  87 --------------
>  drivers/thermal/thermal_core.h    |   4 +
>  drivers/thermal/thermal_helpers.c |  62 ----------
>  drivers/thermal/thermal_trip.c    | 182
> ++++++++++++++++++++++++++++++
>  5 files changed, 188 insertions(+), 151 deletions(-)
>  create mode 100644 drivers/thermal/thermal_trip.c
> 
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 2506c6c8ca83..2faf4651f34a 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 fad0c4a07d16..4ee685043a3e 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_core.h
> b/drivers/thermal/thermal_core.h
> index 26350206a98d..7af54382e915 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -52,6 +52,10 @@ int for_each_thermal_cooling_device(int
> (*cb)(struct thermal_cooling_device *,
>  int for_each_thermal_governor(int (*cb)(struct thermal_governor *,
> void *),
>  			      void *thermal_governor);
>  
> +int __for_each_thermal_trip(struct thermal_zone_device *,
> +			    int (*cb)(struct thermal_trip *, void *),
> +			    void *);
> +
>  struct thermal_zone_device *thermal_zone_get_by_id(int id);
>  
>  struct thermal_attr {
> 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..5ce71f1cb1ba
> --- /dev/null
> +++ b/drivers/thermal/thermal_trip.c
> @@ -0,0 +1,182 @@
> +// 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 *, void *),
> +			    void *data)
> +{
> +	int i, ret;
> +	struct thermal_trip trip;
> +
> +	lockdep_assert_held(&tz->lock);
> +
> +	for (i = 0; i < tz->num_trips; i++) {
> +
> +		ret = __thermal_zone_get_trip(tz, i, &trip);
> +		if (ret)
> +			return ret;
> +		
> +		ret = cb(&trip, 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 v2 0/3] Misc thermal cleanup and fixes

[Daniel Lezcano]

Hi Rafael,

will you take this series on your branch ?


On 23/01/2023 16:27, Daniel Lezcano wrote:
> This series is based on linux-pm/thermal
> 
> It unregisters the netlink generic family for thermal in case the thermal
> framework fails to initialize, removes a unneeded ida_destroy() call and moves
> the thermal trip code in a separate file.
> 
> A couple of changes have been removed from the previous version:
> 
>   - Ordering the trip points. Some consolidation is needed in the set_trip_temp
>     ops before ordering the trip points, so this change is postpone
> 
>   - Remove the mutex destroy call. Even if the call is not needed, the
>     justification of the change is not accurate. So it is postponed also.
> 
>   V2:
>     - Removed ordering the trip points change
>     - Removed mutex destroy change
>     - Added "No functional change intented" as requested by Rui
>     - Added the function declaration for_each_thermal_trip
> 
> Daniel Lezcano (3):
>    thermal/core: Fix unregistering netlink at thermal init time
>    thermal/core: Remove unneeded ida_destroy()
>    thermal/core: Move the thermal trip code to a dedicated file
> 
>   drivers/thermal/Makefile          |   4 +-
>   drivers/thermal/thermal_core.c    |  93 +--------------
>   drivers/thermal/thermal_core.h    |   4 +
>   drivers/thermal/thermal_helpers.c |  62 ----------
>   drivers/thermal/thermal_netlink.c |   5 +
>   drivers/thermal/thermal_netlink.h |   3 +
>   drivers/thermal/thermal_trip.c    | 182 ++++++++++++++++++++++++++++++
>   7 files changed, 199 insertions(+), 154 deletions(-)
>   create mode 100644 drivers/thermal/thermal_trip.c
> 

-- 
<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 v2 0/3] Misc thermal cleanup and fixes

[Rafael J. Wysocki]

On Wed, Jan 25, 2023 at 12:16 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Rafael,
>
> will you take this series on your branch ?

Yes, I'm planning to do that, if I don't find any issues with it (not
likely though).

> On 23/01/2023 16:27, Daniel Lezcano wrote:
> > This series is based on linux-pm/thermal
> >
> > It unregisters the netlink generic family for thermal in case the thermal
> > framework fails to initialize, removes a unneeded ida_destroy() call and moves
> > the thermal trip code in a separate file.
> >
> > A couple of changes have been removed from the previous version:
> >
> >   - Ordering the trip points. Some consolidation is needed in the set_trip_temp
> >     ops before ordering the trip points, so this change is postpone
> >
> >   - Remove the mutex destroy call. Even if the call is not needed, the
> >     justification of the change is not accurate. So it is postponed also.
> >
> >   V2:
> >     - Removed ordering the trip points change
> >     - Removed mutex destroy change
> >     - Added "No functional change intented" as requested by Rui
> >     - Added the function declaration for_each_thermal_trip
> >
> > Daniel Lezcano (3):
> >    thermal/core: Fix unregistering netlink at thermal init time
> >    thermal/core: Remove unneeded ida_destroy()
> >    thermal/core: Move the thermal trip code to a dedicated file
> >
> >   drivers/thermal/Makefile          |   4 +-
> >   drivers/thermal/thermal_core.c    |  93 +--------------
> >   drivers/thermal/thermal_core.h    |   4 +
> >   drivers/thermal/thermal_helpers.c |  62 ----------
> >   drivers/thermal/thermal_netlink.c |   5 +
> >   drivers/thermal/thermal_netlink.h |   3 +
> >   drivers/thermal/thermal_trip.c    | 182 ++++++++++++++++++++++++++++++
> >   7 files changed, 199 insertions(+), 154 deletions(-)
> >   create mode 100644 drivers/thermal/thermal_trip.c
> >
>
> --

Re: [PATCH v2 0/3] Misc thermal cleanup and fixes

[Rafael J. Wysocki]

On Wed, Jan 25, 2023 at 3:03 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jan 25, 2023 at 12:16 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> >
> > Hi Rafael,
> >
> > will you take this series on your branch ?
>
> Yes, I'm planning to do that, if I don't find any issues with it (not
> likely though).

I've just queued it up for 6.3.

> > On 23/01/2023 16:27, Daniel Lezcano wrote:
> > > This series is based on linux-pm/thermal
> > >
> > > It unregisters the netlink generic family for thermal in case the thermal
> > > framework fails to initialize, removes a unneeded ida_destroy() call and moves
> > > the thermal trip code in a separate file.
> > >
> > > A couple of changes have been removed from the previous version:
> > >
> > >   - Ordering the trip points. Some consolidation is needed in the set_trip_temp
> > >     ops before ordering the trip points, so this change is postpone
> > >
> > >   - Remove the mutex destroy call. Even if the call is not needed, the
> > >     justification of the change is not accurate. So it is postponed also.
> > >
> > >   V2:
> > >     - Removed ordering the trip points change
> > >     - Removed mutex destroy change
> > >     - Added "No functional change intented" as requested by Rui
> > >     - Added the function declaration for_each_thermal_trip
> > >
> > > Daniel Lezcano (3):
> > >    thermal/core: Fix unregistering netlink at thermal init time
> > >    thermal/core: Remove unneeded ida_destroy()
> > >    thermal/core: Move the thermal trip code to a dedicated file
> > >
> > >   drivers/thermal/Makefile          |   4 +-
> > >   drivers/thermal/thermal_core.c    |  93 +--------------
> > >   drivers/thermal/thermal_core.h    |   4 +
> > >   drivers/thermal/thermal_helpers.c |  62 ----------
> > >   drivers/thermal/thermal_netlink.c |   5 +
> > >   drivers/thermal/thermal_netlink.h |   3 +
> > >   drivers/thermal/thermal_trip.c    | 182 ++++++++++++++++++++++++++++++
> > >   7 files changed, 199 insertions(+), 154 deletions(-)
> > >   create mode 100644 drivers/thermal/thermal_trip.c
> > >
> >
> > --