[PATCH v1 00/16] thermal: core: Redesign the governor interface

[PATCH v1 00/16] thermal: core: Redesign the governor interface

[Rafael J. Wysocki]

Hi Everyone,

First off, apologies for the size of the series, but I couldn't resist
throwing a few cleanups in for a good measure.

To the point, this is based on two observations: (1) that the existing
.throttle() governor callback is not really suitable or convenient for
any governors in the tree and (2) that the majority of governors in the
tree do not take trip hysteresis into account, although arguably they
should do that.

Point (1) is addressed by replacing the .throttle() callbacks with two
new governor callbacks, .trip_crossed() and .manage().

The first one will be invoked whenever a trip is crossed at the time when
the netlink messages are sent (so the ordering of these calls will be the
same as for the netlink messages).  This is suitable for governors that
only care about trip crossing events, like Bang-Bang.

The other one will be invoked once per thermal zone update, after all of
the trips have been processed by the core, so the governors using it will
have to walk the trips by themselves, but they may also skip the trip
walk entirely if they don't need it, like the Power Allocator.

The .trip_crossed() callback is introduced in the first patch.  Next, the
Banb-Bang governor is switched over to using it (instead of .throttle())
and cleaned up.

The .manage() callback is introduced subsequently (in patch [05/16]) and
the Power Allocator, Step-Wise and Fair-Share governors are made use it
instead of .throttle(), in this order.  The latter two are then modified to
use trip point thresholds instead of trip temperatures, which they should
be doing.  Still, all of the patches changing one governor are grouped
together.

Finally, the User Space governor is modified to use the .trip_crossed()
callback instead of .throttle() and the latter is dropped from the code
because it has no more users.

In addition, the handling of critical and hot trip points is relocated to
eliminate a redundant check.

Please refer to the individual patch changelogs for details.

The series is on top of the linux-next branch in linux-pm.git, which by now
should have been included into linux-next.

There is also a git branch where it can be found:

https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=thermal-core-testing

Thanks!

[PATCH v1 02/16] thermal: gov_bang_bang: Use .trip_crossed() instead of .throttle()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The Bang-Bang governor really is only concerned about trip point
crossing, so it can use the new .trip_crossed() callback instead of
.throttle() that is not particularly suitable for it.

Modify it to do so which also takes trip hysteresis into account, so the
governor does not need to use it directly any more.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/thermal/gov_bang_bang.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_bang_bang.c
+++ linux-pm/drivers/thermal/gov_bang_bang.c
@@ -13,8 +13,9 @@
 
 #include "thermal_core.h"
 
-static int thermal_zone_trip_update(struct thermal_zone_device *tz,
-				    const struct thermal_trip *trip)
+static void thermal_zone_trip_update(struct thermal_zone_device *tz,
+				     const struct thermal_trip *trip,
+				     bool crossed_up)
 {
 	int trip_index = thermal_zone_trip_id(tz, trip);
 	struct thermal_instance *instance;
@@ -43,13 +44,12 @@ static int thermal_zone_trip_update(stru
 		}
 
 		/*
-		 * enable fan when temperature exceeds trip_temp and disable
-		 * the fan in case it falls below trip_temp minus hysteresis
+		 * Enable the fan when the trip is crossed on the way up and
+		 * disable it when the trip is crossed on the way down.
 		 */
-		if (instance->target == 0 && tz->temperature >= trip->temperature)
+		if (instance->target == 0 && crossed_up)
 			instance->target = 1;
-		else if (instance->target == 1 &&
-			 tz->temperature < trip->temperature - trip->hysteresis)
+		else if (instance->target == 1 && !crossed_up)
 			instance->target = 0;
 
 		dev_dbg(&instance->cdev->device, "target=%d\n",
@@ -59,14 +59,13 @@ static int thermal_zone_trip_update(stru
 		instance->cdev->updated = false; /* cdev needs update */
 		mutex_unlock(&instance->cdev->lock);
 	}
-
-	return 0;
 }
 
 /**
  * bang_bang_control - controls devices associated with the given zone
  * @tz: thermal_zone_device
  * @trip: the trip point
+ * @crossed_up: whether or not the trip has been crossed on the way up
  *
  * Regulation Logic: a two point regulation, deliver cooling state depending
  * on the previous state shown in this diagram:
@@ -90,26 +89,22 @@ static int thermal_zone_trip_update(stru
  *     (trip_temp - hyst) so that the fan gets turned off again.
  *
  */
-static int bang_bang_control(struct thermal_zone_device *tz,
-			     const struct thermal_trip *trip)
+static void bang_bang_control(struct thermal_zone_device *tz,
+			      const struct thermal_trip *trip,
+			      bool crossed_up)
 {
 	struct thermal_instance *instance;
-	int ret;
 
 	lockdep_assert_held(&tz->lock);
 
-	ret = thermal_zone_trip_update(tz, trip);
-	if (ret)
-		return ret;
+	thermal_zone_trip_update(tz, trip, crossed_up);
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
 		thermal_cdev_update(instance->cdev);
-
-	return 0;
 }
 
 static struct thermal_governor thermal_gov_bang_bang = {
 	.name		= "bang_bang",
-	.throttle	= bang_bang_control,
+	.trip_crossed	= bang_bang_control,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);

[PATCH v1 03/16] thermal: gov_bang_bang: Clean up thermal_zone_trip_update()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Do the following cleanups in thermal_zone_trip_update():

 * Drop the useless "zero hysteresis" message.
 * Eliminate the trip_index local variable that is redundant.
 * Drop 2 comments that are not useful.
 * Downgrade a diagnostic message from pr_warn() to pr_debug().
 * Use consistent field formatting in diagnostic messages.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c |   19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/thermal/gov_bang_bang.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_bang_bang.c
+++ linux-pm/drivers/thermal/gov_bang_bang.c
@@ -17,29 +17,23 @@ static void thermal_zone_trip_update(str
 				     const struct thermal_trip *trip,
 				     bool crossed_up)
 {
-	int trip_index = thermal_zone_trip_id(tz, trip);
 	struct thermal_instance *instance;
 
-	if (!trip->hysteresis)
-		dev_info_once(&tz->device,
-			      "Zero hysteresis value for thermal zone %s\n", tz->type);
-
 	dev_dbg(&tz->device, "Trip%d[temp=%d]:temp=%d:hyst=%d\n",
-				trip_index, trip->temperature, tz->temperature,
-				trip->hysteresis);
+		thermal_zone_trip_id(tz, trip), trip->temperature,
+		tz->temperature, trip->hysteresis);
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		if (instance->trip != trip)
 			continue;
 
-		/* in case fan is in initial state, switch the fan off */
 		if (instance->target == THERMAL_NO_TARGET)
 			instance->target = 0;
 
-		/* in case fan is neither on nor off set the fan to active */
 		if (instance->target != 0 && instance->target != 1) {
-			pr_warn("Thermal instance %s controlled by bang-bang has unexpected state: %ld\n",
-					instance->name, instance->target);
+			pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
+				 instance->target, instance->name);
+
 			instance->target = 1;
 		}
 
@@ -52,8 +46,7 @@ static void thermal_zone_trip_update(str
 		else if (instance->target == 1 && !crossed_up)
 			instance->target = 0;
 
-		dev_dbg(&instance->cdev->device, "target=%d\n",
-					(int)instance->target);
+		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
 
 		mutex_lock(&instance->cdev->lock);
 		instance->cdev->updated = false; /* cdev needs update */

[PATCH v1 04/16] thermal: gov_bang_bang: Fold thermal_zone_trip_update() into its caller

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Fold thermal_zone_trip_update() into bang_bang_control() which is the
only caller of it to reduce code size and make it easier to follow.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c |   75 +++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 42 deletions(-)

Index: linux-pm/drivers/thermal/gov_bang_bang.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_bang_bang.c
+++ linux-pm/drivers/thermal/gov_bang_bang.c
@@ -13,47 +13,6 @@
 
 #include "thermal_core.h"
 
-static void thermal_zone_trip_update(struct thermal_zone_device *tz,
-				     const struct thermal_trip *trip,
-				     bool crossed_up)
-{
-	struct thermal_instance *instance;
-
-	dev_dbg(&tz->device, "Trip%d[temp=%d]:temp=%d:hyst=%d\n",
-		thermal_zone_trip_id(tz, trip), trip->temperature,
-		tz->temperature, trip->hysteresis);
-
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (instance->trip != trip)
-			continue;
-
-		if (instance->target == THERMAL_NO_TARGET)
-			instance->target = 0;
-
-		if (instance->target != 0 && instance->target != 1) {
-			pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
-				 instance->target, instance->name);
-
-			instance->target = 1;
-		}
-
-		/*
-		 * Enable the fan when the trip is crossed on the way up and
-		 * disable it when the trip is crossed on the way down.
-		 */
-		if (instance->target == 0 && crossed_up)
-			instance->target = 1;
-		else if (instance->target == 1 && !crossed_up)
-			instance->target = 0;
-
-		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
-
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false; /* cdev needs update */
-		mutex_unlock(&instance->cdev->lock);
-	}
-}
-
 /**
  * bang_bang_control - controls devices associated with the given zone
  * @tz: thermal_zone_device
@@ -90,7 +49,39 @@ static void bang_bang_control(struct the
 
 	lockdep_assert_held(&tz->lock);
 
-	thermal_zone_trip_update(tz, trip, crossed_up);
+	dev_dbg(&tz->device, "Trip%d[temp=%d]:temp=%d:hyst=%d\n",
+		thermal_zone_trip_id(tz, trip), trip->temperature,
+		tz->temperature, trip->hysteresis);
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		if (instance->trip != trip)
+			continue;
+
+		if (instance->target == THERMAL_NO_TARGET)
+			instance->target = 0;
+
+		if (instance->target != 0 && instance->target != 1) {
+			pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
+				 instance->target, instance->name);
+
+			instance->target = 1;
+		}
+
+		/*
+		 * Enable the fan when the trip is crossed on the way up and
+		 * disable it when the trip is crossed on the way down.
+		 */
+		if (instance->target == 0 && crossed_up)
+			instance->target = 1;
+		else if (instance->target == 1 && !crossed_up)
+			instance->target = 0;
+
+		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
+
+		mutex_lock(&instance->cdev->lock);
+		instance->cdev->updated = false; /* cdev needs update */
+		mutex_unlock(&instance->cdev->lock);
+	}
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
 		thermal_cdev_update(instance->cdev);

[PATCH v1 05/16] thermal: core: Introduce .manage() callback for thermal governors

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a new thermal governor callback called .manage() that will be
invoked once per thermal zone update after processing all of the trip
points in the core.

This will allow governors that look at multiple trip points together
to check all of them in a consistent configuration, so they don't need
to play tricks with skipping .throttle() invocations that they are not
interested in and they can avoid carrying out the same computations for
multiple times in one cycle.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.c |    3 +++
 drivers/thermal/thermal_core.h |    2 ++
 2 files changed, 5 insertions(+)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -516,6 +516,9 @@ void __thermal_zone_device_update(struct
 			governor->trip_crossed(tz, &td->trip, false);
 	}
 
+	if (governor->manage)
+		governor->manage(tz);
+
 	monitor_thermal_zone(tz);
 }
 
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -31,6 +31,7 @@ struct thermal_trip_desc {
  * @unbind_from_tz:	callback called when a governor is unbound from a
  *			thermal zone.
  * @trip_crossed:	called for trip points that have just been crossed
+ * @manage:	called on thermal zone temperature updates
  * @throttle:	callback called for every trip point even if temperature is
  *		below the trip point temperature
  * @update_tz:	callback called when thermal zone internals have changed, e.g.
@@ -44,6 +45,7 @@ struct thermal_governor {
 	void (*trip_crossed)(struct thermal_zone_device *tz,
 			     const struct thermal_trip *trip,
 			     bool crossed_up);
+	void (*manage)(struct thermal_zone_device *tz);
 	int (*throttle)(struct thermal_zone_device *tz,
 			const struct thermal_trip *trip);
 	void (*update_tz)(struct thermal_zone_device *tz,

[PATCH v1 06/16] thermal: gov_power_allocator: Use .manage() callback instead of .throttle()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The Power Allocator governor really only wants to be called once per
thermal zone update and it does a special check to skip the extra,
from its perspective, invocations of the .throttle() callback.

Make it use .manage() instead of .throttle().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_power_allocator.c |   24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -395,7 +395,7 @@ static void divvy_up_power(struct power_
 	}
 }
 
-static int allocate_power(struct thermal_zone_device *tz, int control_temp)
+static void allocate_power(struct thermal_zone_device *tz, int control_temp)
 {
 	struct power_allocator_params *params = tz->governor_data;
 	unsigned int num_actors = params->num_actors;
@@ -410,7 +410,7 @@ static int allocate_power(struct thermal
 	int i = 0, ret;
 
 	if (!num_actors)
-		return -ENODEV;
+		return;
 
 	/* Clean all buffers for new power estimations */
 	memset(power, 0, params->buffer_size);
@@ -471,8 +471,6 @@ static int allocate_power(struct thermal
 				      num_actors, power_range,
 				      max_allocatable_power, tz->temperature,
 				      control_temp - tz->temperature);
-
-	return 0;
 }
 
 /**
@@ -745,40 +743,32 @@ static void power_allocator_unbind(struc
 	tz->governor_data = NULL;
 }
 
-static int power_allocator_throttle(struct thermal_zone_device *tz,
-				    const struct thermal_trip *trip)
+static void power_allocator_manage(struct thermal_zone_device *tz)
 {
 	struct power_allocator_params *params = tz->governor_data;
+	const struct thermal_trip *trip = params->trip_switch_on;
 	bool update;
 
 	lockdep_assert_held(&tz->lock);
 
-	/*
-	 * We get called for every trip point but we only need to do
-	 * our calculations once
-	 */
-	if (trip != params->trip_max)
-		return 0;
-
-	trip = params->trip_switch_on;
 	if (trip && tz->temperature < trip->temperature) {
 		update = tz->passive;
 		tz->passive = 0;
 		reset_pid_controller(params);
 		allow_maximum_power(tz, update);
-		return 0;
+		return;
 	}
 
 	tz->passive = 1;
 
-	return allocate_power(tz, params->trip_max->temperature);
+	allocate_power(tz, params->trip_max->temperature);
 }
 
 static struct thermal_governor thermal_gov_power_allocator = {
 	.name		= "power_allocator",
 	.bind_to_tz	= power_allocator_bind,
 	.unbind_from_tz	= power_allocator_unbind,
-	.throttle	= power_allocator_throttle,
+	.manage		= power_allocator_manage,
 	.update_tz	= power_allocator_update_tz,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);

[PATCH v1 01/16] thermal: core: Introduce .trip_crossed() callback for thermal governors

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a new thermal governor callback called .trip_crossed()
that will be invoked whenever a trip point is crossed by the zone
temperature, either on the way up or on the way down.

The trip crossing direction information will be passed to it and if
multiple trips are crossed in the same direction during one thermal zone
update, the new callback will be invoked for them in temperature order,
either ascending or descending, depending on the trip crossing
direction.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.c |   19 +++++++++++++++++--
 drivers/thermal/thermal_core.h |    4 ++++
 2 files changed, 21 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -302,11 +302,21 @@ static void monitor_thermal_zone(struct
 		thermal_zone_device_set_polling(tz, tz->polling_delay_jiffies);
 }
 
+static struct thermal_governor *thermal_get_tz_governor(struct thermal_zone_device *tz)
+{
+	if (tz->governor)
+		return tz->governor;
+
+	return def_governor;
+}
+
 static void handle_non_critical_trips(struct thermal_zone_device *tz,
 				      const struct thermal_trip *trip)
 {
-	tz->governor ? tz->governor->throttle(tz, trip) :
-		       def_governor->throttle(tz, trip);
+	struct thermal_governor *governor = thermal_get_tz_governor(tz);
+
+	if (governor->throttle)
+		governor->throttle(tz, trip);
 }
 
 void thermal_governor_update_tz(struct thermal_zone_device *tz,
@@ -470,6 +480,7 @@ static int thermal_trip_notify_cmp(void
 void __thermal_zone_device_update(struct thermal_zone_device *tz,
 				  enum thermal_notify_event event)
 {
+	struct thermal_governor *governor = thermal_get_tz_governor(tz);
 	struct thermal_trip_desc *td;
 	LIST_HEAD(way_down_list);
 	LIST_HEAD(way_up_list);
@@ -493,12 +504,16 @@ void __thermal_zone_device_update(struct
 	list_for_each_entry(td, &way_up_list, notify_list_node) {
 		thermal_notify_tz_trip_up(tz, &td->trip);
 		thermal_debug_tz_trip_up(tz, &td->trip);
+		if (governor->trip_crossed)
+			governor->trip_crossed(tz, &td->trip, true);
 	}
 
 	list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
 	list_for_each_entry(td, &way_down_list, notify_list_node) {
 		thermal_notify_tz_trip_down(tz, &td->trip);
 		thermal_debug_tz_trip_down(tz, &td->trip);
+		if (governor->trip_crossed)
+			governor->trip_crossed(tz, &td->trip, false);
 	}
 
 	monitor_thermal_zone(tz);
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -30,6 +30,7 @@ struct thermal_trip_desc {
  *		otherwise it fails.
  * @unbind_from_tz:	callback called when a governor is unbound from a
  *			thermal zone.
+ * @trip_crossed:	called for trip points that have just been crossed
  * @throttle:	callback called for every trip point even if temperature is
  *		below the trip point temperature
  * @update_tz:	callback called when thermal zone internals have changed, e.g.
@@ -40,6 +41,9 @@ struct thermal_governor {
 	const char *name;
 	int (*bind_to_tz)(struct thermal_zone_device *tz);
 	void (*unbind_from_tz)(struct thermal_zone_device *tz);
+	void (*trip_crossed)(struct thermal_zone_device *tz,
+			     const struct thermal_trip *trip,
+			     bool crossed_up);
 	int (*throttle)(struct thermal_zone_device *tz,
 			const struct thermal_trip *trip);
 	void (*update_tz)(struct thermal_zone_device *tz,

[PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notice that the passive field in struct thermal_zone_device is not
used by the Power Allocator governor itself and so the ordering of
its updates with respect to allow_maximum_power() or allocate_power()
does not matter.

Accordingly, make power_allocator_manage() update that field right
before returning, which allows the current value of it to be passed
directly to allow_maximum_power() without using the additional update
variable that can be dropped.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_power_allocator.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -747,21 +747,18 @@ static void power_allocator_manage(struc
 {
 	struct power_allocator_params *params = tz->governor_data;
 	const struct thermal_trip *trip = params->trip_switch_on;
-	bool update;
 
 	lockdep_assert_held(&tz->lock);
 
 	if (trip && tz->temperature < trip->temperature) {
-		update = tz->passive;
-		tz->passive = 0;
 		reset_pid_controller(params);
-		allow_maximum_power(tz, update);
+		allow_maximum_power(tz, tz->passive);
+		tz->passive = 0;
 		return;
 	}
 
-	tz->passive = 1;
-
 	allocate_power(tz, params->trip_max->temperature);
+	tz->passive = 1;
 }
 
 static struct thermal_governor thermal_gov_power_allocator = {

[PATCH v1 08/16] thermal: gov_step_wise: Use .manage() callback instead of .throttle()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make the Step-Wise governor use the new .manage() callback instead of
.throttle().

Even though using .throttle() is not particularly problematic for the
Step-Wise governor, using .manage() instead still allows it to reduce
overhead by updating all of the colling devices once after setting
target values for all of the thermal instances.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_step_wise.c |   39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -109,34 +109,37 @@ static void thermal_zone_trip_update(str
 	}
 }
 
-/**
- * step_wise_throttle - throttles devices associated with the given zone
- * @tz: thermal_zone_device
- * @trip: trip point
- *
- * Throttling Logic: This uses the trend of the thermal zone to throttle.
- * If the thermal zone is 'heating up' this throttles all the cooling
- * devices associated with the zone and its particular trip point, by one
- * step. If the zone is 'cooling down' it brings back the performance of
- * the devices by one step.
- */
-static int step_wise_throttle(struct thermal_zone_device *tz,
-			      const struct thermal_trip *trip)
+static void step_wise_manage(struct thermal_zone_device *tz)
 {
+	const struct thermal_trip_desc *td;
 	struct thermal_instance *instance;
 
 	lockdep_assert_held(&tz->lock);
 
-	thermal_zone_trip_update(tz, trip);
+	/*
+	 * Throttling Logic: Use the trend of the thermal zone to throttle.
+	 * If the thermal zone is 'heating up', throttle all of the cooling
+	 * devices associated with each trip point by one step. If the zone
+	 * is 'cooling down', it brings back the performance of the devices
+	 * by one step.
+	 */
+	for_each_trip_desc(tz, td) {
+		const struct thermal_trip *trip = &td->trip;
+
+		if (trip->temperature == THERMAL_TEMP_INVALID ||
+		    trip->type == THERMAL_TRIP_CRITICAL ||
+		    trip->type == THERMAL_TRIP_HOT)
+			continue;
+
+		thermal_zone_trip_update(tz, trip);
+	}
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
 		thermal_cdev_update(instance->cdev);
-
-	return 0;
 }
 
 static struct thermal_governor thermal_gov_step_wise = {
-	.name		= "step_wise",
-	.throttle	= step_wise_throttle,
+	.name	= "step_wise",
+	.manage	= step_wise_manage,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_step_wise);

[PATCH v1 09/16] thermal: gov_step_wise: Use trip thresholds instead of trip temperatures

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In principle, the Step-Wise governor should take trip hystereses into
account.  After all, once a trip has been crossed on the way up,
mitigation is still needed until it is crossed on the way down.

For this reason, make it use trip thresholds that are computed by
the core when trips are crossed, so as to apply mitigations in the
hysteresis rages of trips that were crossed on the way up, but have
not been crossed on the way down yet.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_step_wise.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -62,7 +62,8 @@ static unsigned long get_target_state(st
 }
 
 static void thermal_zone_trip_update(struct thermal_zone_device *tz,
-				     const struct thermal_trip *trip)
+				     const struct thermal_trip *trip,
+				     int trip_threshold)
 {
 	int trip_id = thermal_zone_trip_id(tz, trip);
 	enum thermal_trend trend;
@@ -72,13 +73,13 @@ static void thermal_zone_trip_update(str
 
 	trend = get_tz_trend(tz, trip);
 
-	if (tz->temperature >= trip->temperature) {
+	if (tz->temperature >= trip_threshold) {
 		throttle = true;
 		trace_thermal_zone_trip(tz, trip_id, trip->type);
 	}
 
 	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
-		trip_id, trip->type, trip->temperature, trend, throttle);
+		trip_id, trip->type, trip_threshold, trend, throttle);
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		if (instance->trip != trip)
@@ -131,7 +132,7 @@ static void step_wise_manage(struct ther
 		    trip->type == THERMAL_TRIP_HOT)
 			continue;
 
-		thermal_zone_trip_update(tz, trip);
+		thermal_zone_trip_update(tz, trip, td->threshold);
 	}
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node)

[PATCH v1 10/16] thermal: gov_step_wise: Clean up thermal_zone_trip_update()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Do some assorted cleanups in thermal_zone_trip_update():

 * Compute the trend value upfront.
 * Move old_target definition to the block where it is used.
 * Adjust white space around diagnositc messages and locking.
 * Use suitable field formatting in a message to avoid an explicit
   cast to int.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_step_wise.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -65,13 +65,10 @@ static void thermal_zone_trip_update(str
 				     const struct thermal_trip *trip,
 				     int trip_threshold)
 {
+	enum thermal_trend trend = get_tz_trend(tz, trip);
 	int trip_id = thermal_zone_trip_id(tz, trip);
-	enum thermal_trend trend;
 	struct thermal_instance *instance;
 	bool throttle = false;
-	int old_target;
-
-	trend = get_tz_trend(tz, trip);
 
 	if (tz->temperature >= trip_threshold) {
 		throttle = true;
@@ -82,13 +79,16 @@ static void thermal_zone_trip_update(str
 		trip_id, trip->type, trip_threshold, trend, throttle);
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		int old_target;
+
 		if (instance->trip != trip)
 			continue;
 
 		old_target = instance->target;
 		instance->target = get_target_state(instance, trend, throttle);
-		dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
-					old_target, (int)instance->target);
+
+		dev_dbg(&instance->cdev->device, "old_target=%d, target=%ld\n",
+			old_target, instance->target);
 
 		if (instance->initialized && old_target == instance->target)
 			continue;
@@ -104,6 +104,7 @@ static void thermal_zone_trip_update(str
 		}
 
 		instance->initialized = true;
+
 		mutex_lock(&instance->cdev->lock);
 		instance->cdev->updated = false; /* cdev needs update */
 		mutex_unlock(&instance->cdev->lock);

[PATCH v1 11/16] thermal: gov_fair_share: Use .manage() callback instead of .throttle()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The Fair Share governor tries very hard to be stateless and so it
calls get_trip_level() from fair_share_throttle() every time, even
though the number produced by this function for all of the trips
during a given thermal zone update is actually the same.  Since
get_trip_level() walks all of the trips in the thermal zone every
time it is called, doing this may generate quite a bit of completely
useless overhead.

For this reason, make the governor use the new .manage() callback
instead of .throttle() which allows it to call get_trip_level() just
once and use the value computed by it to handle all of the trips.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_fair_share.c |   37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/thermal/gov_fair_share.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_fair_share.c
+++ linux-pm/drivers/thermal/gov_fair_share.c
@@ -53,6 +53,7 @@ static long get_target_state(struct ther
  * fair_share_throttle - throttles devices associated with the given zone
  * @tz: thermal_zone_device
  * @trip: trip point
+ * @trip_level: number of trips crossed by the zone temperature
  *
  * Throttling Logic: This uses three parameters to calculate the new
  * throttle state of the cooling devices associated with the given zone.
@@ -61,22 +62,19 @@ static long get_target_state(struct ther
  * P1. max_state: Maximum throttle state exposed by the cooling device.
  * P2. percentage[i]/100:
  *	How 'effective' the 'i'th device is, in cooling the given zone.
- * P3. cur_trip_level/max_no_of_trips:
+ * P3. trip_level/max_no_of_trips:
  *	This describes the extent to which the devices should be throttled.
  *	We do not want to throttle too much when we trip a lower temperature,
  *	whereas the throttling is at full swing if we trip critical levels.
- *	(Heavily assumes the trip points are in ascending order)
  * new_state of cooling device = P3 * P2 * P1
  */
-static int fair_share_throttle(struct thermal_zone_device *tz,
-			       const struct thermal_trip *trip)
+static void fair_share_throttle(struct thermal_zone_device *tz,
+				const struct thermal_trip *trip,
+				int trip_level)
 {
 	struct thermal_instance *instance;
 	int total_weight = 0;
 	int total_instance = 0;
-	int cur_trip_level = get_trip_level(tz);
-
-	lockdep_assert_held(&tz->lock);
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		if (instance->trip != trip)
@@ -99,18 +97,35 @@ static int fair_share_throttle(struct th
 			percentage = (instance->weight * 100) / total_weight;
 
 		instance->target = get_target_state(tz, cdev, percentage,
-						    cur_trip_level);
+						    trip_level);
 
 		mutex_lock(&cdev->lock);
 		__thermal_cdev_update(cdev);
 		mutex_unlock(&cdev->lock);
 	}
+}
 
-	return 0;
+static void fair_share_manage(struct thermal_zone_device *tz)
+{
+	int trip_level = get_trip_level(tz);
+	const struct thermal_trip_desc *td;
+
+	lockdep_assert_held(&tz->lock);
+
+	for_each_trip_desc(tz, td) {
+		const struct thermal_trip *trip = &td->trip;
+
+		if (trip->temperature == THERMAL_TEMP_INVALID ||
+		    trip->type == THERMAL_TRIP_CRITICAL ||
+		    trip->type == THERMAL_TRIP_HOT)
+			continue;
+
+		fair_share_throttle(tz, trip, trip_level);
+	}
 }
 
 static struct thermal_governor thermal_gov_fair_share = {
-	.name		= "fair_share",
-	.throttle	= fair_share_throttle,
+	.name	= "fair_share",
+	.manage	= fair_share_manage,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_fair_share);

[PATCH v1 12/16] thermal: gov_fair_share: Use trip thresholds instead of trip temperatures

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In principle, the Fair Share governor should take trip hystereses
into account.  After all, once a trip has been crossed on the way up,
mitigation is still needed until it is crossed on the way down.

For this reason, make it use trip thresholds that are computed by
the core when trips are crossed, so as to apply mitigations if the
zone temperature is in a hysteresis rage of one or more trips that
were crossed on the way up, but have not been crossed on the way
down yet.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_fair_share.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/thermal/gov_fair_share.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_fair_share.c
+++ linux-pm/drivers/thermal/gov_fair_share.c
@@ -17,28 +17,26 @@
 
 static int get_trip_level(struct thermal_zone_device *tz)
 {
-	const struct thermal_trip *level_trip = NULL;
+	const struct thermal_trip_desc *level_td = NULL;
 	const struct thermal_trip_desc *td;
 	int trip_level = -1;
 
 	for_each_trip_desc(tz, td) {
-		const struct thermal_trip *trip = &td->trip;
-
-		if (trip->temperature >= tz->temperature)
+		if (td->threshold > tz->temperature)
 			continue;
 
 		trip_level++;
 
-		if (!level_trip || trip->temperature > level_trip->temperature)
-			level_trip = trip;
+		if (!level_td || td->threshold > level_td->threshold)
+			level_td = td;
 	}
 
 	/*  Bail out if the temperature is not greater than any trips. */
 	if (trip_level < 0)
 		return 0;
 
-	trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, level_trip),
-				level_trip->type);
+	trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, &level_td->trip),
+				level_td->trip.type);
 
 	return trip_level;
 }

[PATCH v1 13/16] thermal: gov_fair_share: Eliminate unnecessary integer divisions

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The computations carried out by fair_share_throttle() for each trip
point include at least one redundant integer division which introduces
superfluous rounding errors.  Also the multiplications by 100 in it are
not really necessary and can be eliminated.

Rearrange fair_share_throttle() to carry out only one integer division per
trip and only as many integer multiplications as necessary and rename one
variable in it (while at it).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_fair_share.c |   32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

Index: linux-pm/drivers/thermal/gov_fair_share.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_fair_share.c
+++ linux-pm/drivers/thermal/gov_fair_share.c
@@ -41,12 +41,6 @@ static int get_trip_level(struct thermal
 	return trip_level;
 }
 
-static long get_target_state(struct thermal_zone_device *tz,
-		struct thermal_cooling_device *cdev, int percentage, int level)
-{
-	return (long)(percentage * level * cdev->max_state) / (100 * tz->num_trips);
-}
-
 /**
  * fair_share_throttle - throttles devices associated with the given zone
  * @tz: thermal_zone_device
@@ -58,7 +52,7 @@ static long get_target_state(struct ther
  *
  * Parameters used for Throttling:
  * P1. max_state: Maximum throttle state exposed by the cooling device.
- * P2. percentage[i]/100:
+ * P2. weight[i]/total_weight:
  *	How 'effective' the 'i'th device is, in cooling the given zone.
  * P3. trip_level/max_no_of_trips:
  *	This describes the extent to which the devices should be throttled.
@@ -72,30 +66,34 @@ static void fair_share_throttle(struct t
 {
 	struct thermal_instance *instance;
 	int total_weight = 0;
-	int total_instance = 0;
+	int nr_instances = 0;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		if (instance->trip != trip)
 			continue;
 
 		total_weight += instance->weight;
-		total_instance++;
+		nr_instances++;
 	}
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		int percentage;
 		struct thermal_cooling_device *cdev = instance->cdev;
+		u64 dividend;
+		u32 divisor;
 
 		if (instance->trip != trip)
 			continue;
 
-		if (!total_weight)
-			percentage = 100 / total_instance;
-		else
-			percentage = (instance->weight * 100) / total_weight;
-
-		instance->target = get_target_state(tz, cdev, percentage,
-						    trip_level);
+		dividend = trip_level;
+		dividend *= cdev->max_state;
+		divisor = tz->num_trips;
+		if (total_weight) {
+			dividend *= instance->weight;
+			divisor *= total_weight;
+		} else {
+			divisor *= nr_instances;
+		}
+		instance->target = div_u64(dividend, divisor);
 
 		mutex_lock(&cdev->lock);
 		__thermal_cdev_update(cdev);

[PATCH v1 14/16] thermal: gov_user_space: Use .trip_crossed() instead of .throttle()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notifying user space about trip points that have not been crossed is
not particuarly useful, so modity the User Space governor to use the
.trip_crossed() callback, which is only invoked for trips that have been
crossed, instead of .throttle() that is invoked for all trips in a
thermal zone every time the zone is updated.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Note: I am not actually sure if there is user space depending on the
current behavior that can be broken by this change.

I can easily imagine trying to implement a complicated governor in user
space that will look at all of the trips in the thermal zone regardless
of whether or not they are crossed, which can be kind of helped by the
current behavior of the user space governor.

However, the total overhead caused by it is considerable and quite
arguably it may not be acceptable at least in some cases.

---
 drivers/thermal/gov_user_space.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/thermal/gov_user_space.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_user_space.c
+++ linux-pm/drivers/thermal/gov_user_space.c
@@ -26,11 +26,13 @@ static int user_space_bind(struct therma
  * notify_user_space - Notifies user space about thermal events
  * @tz: thermal_zone_device
  * @trip: trip point
+ * @crossed_up: whether or not the trip has been crossed on the way up
  *
  * This function notifies the user space through UEvents.
  */
-static int notify_user_space(struct thermal_zone_device *tz,
-			     const struct thermal_trip *trip)
+static void notify_user_space(struct thermal_zone_device *tz,
+			      const struct thermal_trip *trip,
+			      bool crossed_up)
 {
 	char *thermal_prop[5];
 	int i;
@@ -46,13 +48,11 @@ static int notify_user_space(struct ther
 	kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
 	for (i = 0; i < 4; ++i)
 		kfree(thermal_prop[i]);
-
-	return 0;
 }
 
 static struct thermal_governor thermal_gov_user_space = {
 	.name		= "user_space",
-	.throttle	= notify_user_space,
+	.trip_crossed	= notify_user_space,
 	.bind_to_tz	= user_space_bind,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_user_space);

[PATCH v1 15/16] thermal: core: Drop the .throttle() governor callback

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since all of the governors in the tree have been switched over to using
the new callbacks, either .trip_crossed() or .manage(), the .throttle()
governor callback is not used any more, so drop it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.c |   11 -----------
 drivers/thermal/thermal_core.h |    4 ----
 2 files changed, 15 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -310,15 +310,6 @@ static struct thermal_governor *thermal_
 	return def_governor;
 }
 
-static void handle_non_critical_trips(struct thermal_zone_device *tz,
-				      const struct thermal_trip *trip)
-{
-	struct thermal_governor *governor = thermal_get_tz_governor(tz);
-
-	if (governor->throttle)
-		governor->throttle(tz, trip);
-}
-
 void thermal_governor_update_tz(struct thermal_zone_device *tz,
 				enum thermal_notify_event reason)
 {
@@ -418,8 +409,6 @@ static void handle_thermal_trip(struct t
 
 	if (trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT)
 		handle_critical_trips(tz, trip);
-	else
-		handle_non_critical_trips(tz, trip);
 }
 
 static void update_temperature(struct thermal_zone_device *tz)
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -32,8 +32,6 @@ struct thermal_trip_desc {
  *			thermal zone.
  * @trip_crossed:	called for trip points that have just been crossed
  * @manage:	called on thermal zone temperature updates
- * @throttle:	callback called for every trip point even if temperature is
- *		below the trip point temperature
  * @update_tz:	callback called when thermal zone internals have changed, e.g.
  *		thermal cooling instance was added/removed
  * @governor_list:	node in thermal_governor_list (in thermal_core.c)
@@ -46,8 +44,6 @@ struct thermal_governor {
 			     const struct thermal_trip *trip,
 			     bool crossed_up);
 	void (*manage)(struct thermal_zone_device *tz);
-	int (*throttle)(struct thermal_zone_device *tz,
-			const struct thermal_trip *trip);
 	void (*update_tz)(struct thermal_zone_device *tz,
 			  enum thermal_notify_event reason);
 	struct list_head	governor_list;

[PATCH v1 16/16] thermal: core: Relocate critical and hot trip handling

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH v1] 

Modify handle_thermal_trip() to call handle_critical_trips() only after
finding that the trip temperature has been crossed on the way up and
remove the redundant temperature check from the latter.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -350,10 +350,6 @@ void thermal_zone_device_critical_reboot
 static void handle_critical_trips(struct thermal_zone_device *tz,
 				  const struct thermal_trip *trip)
 {
-	/* If we have not crossed the trip_temp, we do not care. */
-	if (trip->temperature <= 0 || tz->temperature < trip->temperature)
-		return;
-
 	trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, trip), trip->type);
 
 	if (trip->type == THERMAL_TRIP_CRITICAL)
@@ -405,10 +401,11 @@ static void handle_thermal_trip(struct t
 		list_add_tail(&td->notify_list_node, way_up_list);
 		td->notify_temp = trip->temperature;
 		td->threshold -= trip->hysteresis;
-	}
 
-	if (trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT)
-		handle_critical_trips(tz, trip);
+		if (trip->type == THERMAL_TRIP_CRITICAL ||
+		    trip->type == THERMAL_TRIP_HOT)
+			handle_critical_trips(tz, trip);
+	}
 }
 
 static void update_temperature(struct thermal_zone_device *tz)

Re: [PATCH v1 01/16] thermal: core: Introduce .trip_crossed() callback for thermal governors

[Lukasz Luba]

On 4/10/24 17:10, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce a new thermal governor callback called .trip_crossed()
> that will be invoked whenever a trip point is crossed by the zone
> temperature, either on the way up or on the way down.
> 
> The trip crossing direction information will be passed to it and if
> multiple trips are crossed in the same direction during one thermal zone
> update, the new callback will be invoked for them in temperature order,
> either ascending or descending, depending on the trip crossing
> direction.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_core.c |   19 +++++++++++++++++--
>   drivers/thermal/thermal_core.h |    4 ++++
>   2 files changed, 21 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -302,11 +302,21 @@ static void monitor_thermal_zone(struct
>   		thermal_zone_device_set_polling(tz, tz->polling_delay_jiffies);
>   }
>   
> +static struct thermal_governor *thermal_get_tz_governor(struct thermal_zone_device *tz)
> +{
> +	if (tz->governor)
> +		return tz->governor;
> +
> +	return def_governor;
> +}
> +
>   static void handle_non_critical_trips(struct thermal_zone_device *tz,
>   				      const struct thermal_trip *trip)
>   {
> -	tz->governor ? tz->governor->throttle(tz, trip) :
> -		       def_governor->throttle(tz, trip);
> +	struct thermal_governor *governor = thermal_get_tz_governor(tz);
> +
> +	if (governor->throttle)
> +		governor->throttle(tz, trip);
>   }
>   
>   void thermal_governor_update_tz(struct thermal_zone_device *tz,
> @@ -470,6 +480,7 @@ static int thermal_trip_notify_cmp(void
>   void __thermal_zone_device_update(struct thermal_zone_device *tz,
>   				  enum thermal_notify_event event)
>   {
> +	struct thermal_governor *governor = thermal_get_tz_governor(tz);
>   	struct thermal_trip_desc *td;
>   	LIST_HEAD(way_down_list);
>   	LIST_HEAD(way_up_list);
> @@ -493,12 +504,16 @@ void __thermal_zone_device_update(struct
>   	list_for_each_entry(td, &way_up_list, notify_list_node) {
>   		thermal_notify_tz_trip_up(tz, &td->trip);
>   		thermal_debug_tz_trip_up(tz, &td->trip);
> +		if (governor->trip_crossed)
> +			governor->trip_crossed(tz, &td->trip, true);
>   	}
>   
>   	list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
>   	list_for_each_entry(td, &way_down_list, notify_list_node) {
>   		thermal_notify_tz_trip_down(tz, &td->trip);
>   		thermal_debug_tz_trip_down(tz, &td->trip);
> +		if (governor->trip_crossed)
> +			governor->trip_crossed(tz, &td->trip, false);
>   	}
>   
>   	monitor_thermal_zone(tz);
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -30,6 +30,7 @@ struct thermal_trip_desc {
>    *		otherwise it fails.
>    * @unbind_from_tz:	callback called when a governor is unbound from a
>    *			thermal zone.
> + * @trip_crossed:	called for trip points that have just been crossed
>    * @throttle:	callback called for every trip point even if temperature is
>    *		below the trip point temperature
>    * @update_tz:	callback called when thermal zone internals have changed, e.g.
> @@ -40,6 +41,9 @@ struct thermal_governor {
>   	const char *name;
>   	int (*bind_to_tz)(struct thermal_zone_device *tz);
>   	void (*unbind_from_tz)(struct thermal_zone_device *tz);
> +	void (*trip_crossed)(struct thermal_zone_device *tz,
> +			     const struct thermal_trip *trip,
> +			     bool crossed_up);
>   	int (*throttle)(struct thermal_zone_device *tz,
>   			const struct thermal_trip *trip);
>   	void (*update_tz)(struct thermal_zone_device *tz,
> 
> 
> 

LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 02/16] thermal: gov_bang_bang: Use .trip_crossed() instead of .throttle()

[Lukasz Luba]

On 4/10/24 17:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The Bang-Bang governor really is only concerned about trip point
> crossing, so it can use the new .trip_crossed() callback instead of
> .throttle() that is not particularly suitable for it.
> 
> Modify it to do so which also takes trip hysteresis into account, so the
> governor does not need to use it directly any more.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_bang_bang.c |   31 +++++++++++++------------------
>   1 file changed, 13 insertions(+), 18 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_bang_bang.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_bang_bang.c
> +++ linux-pm/drivers/thermal/gov_bang_bang.c
> @@ -13,8 +13,9 @@
>   
>   #include "thermal_core.h"
>   
> -static int thermal_zone_trip_update(struct thermal_zone_device *tz,
> -				    const struct thermal_trip *trip)
> +static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> +				     const struct thermal_trip *trip,
> +				     bool crossed_up)
>   {
>   	int trip_index = thermal_zone_trip_id(tz, trip);
>   	struct thermal_instance *instance;
> @@ -43,13 +44,12 @@ static int thermal_zone_trip_update(stru
>   		}
>   
>   		/*
> -		 * enable fan when temperature exceeds trip_temp and disable
> -		 * the fan in case it falls below trip_temp minus hysteresis
> +		 * Enable the fan when the trip is crossed on the way up and
> +		 * disable it when the trip is crossed on the way down.
>   		 */
> -		if (instance->target == 0 && tz->temperature >= trip->temperature)
> +		if (instance->target == 0 && crossed_up)
>   			instance->target = 1;
> -		else if (instance->target == 1 &&
> -			 tz->temperature < trip->temperature - trip->hysteresis)
> +		else if (instance->target == 1 && !crossed_up)
>   			instance->target = 0;
>   
>   		dev_dbg(&instance->cdev->device, "target=%d\n",
> @@ -59,14 +59,13 @@ static int thermal_zone_trip_update(stru
>   		instance->cdev->updated = false; /* cdev needs update */
>   		mutex_unlock(&instance->cdev->lock);
>   	}
> -
> -	return 0;
>   }
>   
>   /**
>    * bang_bang_control - controls devices associated with the given zone
>    * @tz: thermal_zone_device
>    * @trip: the trip point
> + * @crossed_up: whether or not the trip has been crossed on the way up
>    *
>    * Regulation Logic: a two point regulation, deliver cooling state depending
>    * on the previous state shown in this diagram:
> @@ -90,26 +89,22 @@ static int thermal_zone_trip_update(stru
>    *     (trip_temp - hyst) so that the fan gets turned off again.
>    *
>    */
> -static int bang_bang_control(struct thermal_zone_device *tz,
> -			     const struct thermal_trip *trip)
> +static void bang_bang_control(struct thermal_zone_device *tz,
> +			      const struct thermal_trip *trip,
> +			      bool crossed_up)
>   {
>   	struct thermal_instance *instance;
> -	int ret;
>   
>   	lockdep_assert_held(&tz->lock);
>   
> -	ret = thermal_zone_trip_update(tz, trip);
> -	if (ret)
> -		return ret;
> +	thermal_zone_trip_update(tz, trip, crossed_up);
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
>   		thermal_cdev_update(instance->cdev);
> -
> -	return 0;
>   }
>   
>   static struct thermal_governor thermal_gov_bang_bang = {
>   	.name		= "bang_bang",
> -	.throttle	= bang_bang_control,
> +	.trip_crossed	= bang_bang_control,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> 
> 
> 

LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 03/16] thermal: gov_bang_bang: Clean up thermal_zone_trip_update()

[Lukasz Luba]

On 4/10/24 17:05, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Do the following cleanups in thermal_zone_trip_update():
> 
>   * Drop the useless "zero hysteresis" message.
>   * Eliminate the trip_index local variable that is redundant.
>   * Drop 2 comments that are not useful.
>   * Downgrade a diagnostic message from pr_warn() to pr_debug().
>   * Use consistent field formatting in diagnostic messages.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_bang_bang.c |   19 ++++++-------------
>   1 file changed, 6 insertions(+), 13 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_bang_bang.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_bang_bang.c
> +++ linux-pm/drivers/thermal/gov_bang_bang.c
> @@ -17,29 +17,23 @@ static void thermal_zone_trip_update(str
>   				     const struct thermal_trip *trip,
>   				     bool crossed_up)
>   {
> -	int trip_index = thermal_zone_trip_id(tz, trip);
>   	struct thermal_instance *instance;
>   
> -	if (!trip->hysteresis)
> -		dev_info_once(&tz->device,
> -			      "Zero hysteresis value for thermal zone %s\n", tz->type);
> -
>   	dev_dbg(&tz->device, "Trip%d[temp=%d]:temp=%d:hyst=%d\n",
> -				trip_index, trip->temperature, tz->temperature,
> -				trip->hysteresis);
> +		thermal_zone_trip_id(tz, trip), trip->temperature,
> +		tz->temperature, trip->hysteresis);
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   		if (instance->trip != trip)
>   			continue;
>   
> -		/* in case fan is in initial state, switch the fan off */
>   		if (instance->target == THERMAL_NO_TARGET)
>   			instance->target = 0;
>   
> -		/* in case fan is neither on nor off set the fan to active */
>   		if (instance->target != 0 && instance->target != 1) {
> -			pr_warn("Thermal instance %s controlled by bang-bang has unexpected state: %ld\n",
> -					instance->name, instance->target);
> +			pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
> +				 instance->target, instance->name);
> +
>   			instance->target = 1;
>   		}
>   
> @@ -52,8 +46,7 @@ static void thermal_zone_trip_update(str
>   		else if (instance->target == 1 && !crossed_up)
>   			instance->target = 0;
>   
> -		dev_dbg(&instance->cdev->device, "target=%d\n",
> -					(int)instance->target);
> +		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
>   
>   		mutex_lock(&instance->cdev->lock);
>   		instance->cdev->updated = false; /* cdev needs update */
> 
> 
> 

LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 04/16] thermal: gov_bang_bang: Fold thermal_zone_trip_update() into its caller

[Lukasz Luba]

On 4/10/24 17:06, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Fold thermal_zone_trip_update() into bang_bang_control() which is the
> only caller of it to reduce code size and make it easier to follow.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_bang_bang.c |   75 +++++++++++++++++-----------------------
>   1 file changed, 33 insertions(+), 42 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_bang_bang.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_bang_bang.c
> +++ linux-pm/drivers/thermal/gov_bang_bang.c
> @@ -13,47 +13,6 @@
>   
>   #include "thermal_core.h"
>   
> -static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> -				     const struct thermal_trip *trip,
> -				     bool crossed_up)
> -{
> -	struct thermal_instance *instance;
> -
> -	dev_dbg(&tz->device, "Trip%d[temp=%d]:temp=%d:hyst=%d\n",
> -		thermal_zone_trip_id(tz, trip), trip->temperature,
> -		tz->temperature, trip->hysteresis);
> -
> -	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> -		if (instance->trip != trip)
> -			continue;
> -
> -		if (instance->target == THERMAL_NO_TARGET)
> -			instance->target = 0;
> -
> -		if (instance->target != 0 && instance->target != 1) {
> -			pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
> -				 instance->target, instance->name);
> -
> -			instance->target = 1;
> -		}
> -
> -		/*
> -		 * Enable the fan when the trip is crossed on the way up and
> -		 * disable it when the trip is crossed on the way down.
> -		 */
> -		if (instance->target == 0 && crossed_up)
> -			instance->target = 1;
> -		else if (instance->target == 1 && !crossed_up)
> -			instance->target = 0;
> -
> -		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
> -
> -		mutex_lock(&instance->cdev->lock);
> -		instance->cdev->updated = false; /* cdev needs update */
> -		mutex_unlock(&instance->cdev->lock);
> -	}
> -}
> -
>   /**
>    * bang_bang_control - controls devices associated with the given zone
>    * @tz: thermal_zone_device
> @@ -90,7 +49,39 @@ static void bang_bang_control(struct the
>   
>   	lockdep_assert_held(&tz->lock);
>   
> -	thermal_zone_trip_update(tz, trip, crossed_up);
> +	dev_dbg(&tz->device, "Trip%d[temp=%d]:temp=%d:hyst=%d\n",
> +		thermal_zone_trip_id(tz, trip), trip->temperature,
> +		tz->temperature, trip->hysteresis);
> +
> +	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +		if (instance->trip != trip)
> +			continue;
> +
> +		if (instance->target == THERMAL_NO_TARGET)
> +			instance->target = 0;
> +
> +		if (instance->target != 0 && instance->target != 1) {
> +			pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
> +				 instance->target, instance->name);
> +
> +			instance->target = 1;
> +		}
> +
> +		/*
> +		 * Enable the fan when the trip is crossed on the way up and
> +		 * disable it when the trip is crossed on the way down.
> +		 */
> +		if (instance->target == 0 && crossed_up)
> +			instance->target = 1;
> +		else if (instance->target == 1 && !crossed_up)
> +			instance->target = 0;
> +
> +		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
> +
> +		mutex_lock(&instance->cdev->lock);
> +		instance->cdev->updated = false; /* cdev needs update */
> +		mutex_unlock(&instance->cdev->lock);
> +	}
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
>   		thermal_cdev_update(instance->cdev);
> 
> 
> 

LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 05/16] thermal: core: Introduce .manage() callback for thermal governors

[Lukasz Luba]

On 4/10/24 17:08, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce a new thermal governor callback called .manage() that will be
> invoked once per thermal zone update after processing all of the trip
> points in the core.
> 
> This will allow governors that look at multiple trip points together
> to check all of them in a consistent configuration, so they don't need
> to play tricks with skipping .throttle() invocations that they are not
> interested in and they can avoid carrying out the same computations for
> multiple times in one cycle.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_core.c |    3 +++
>   drivers/thermal/thermal_core.h |    2 ++
>   2 files changed, 5 insertions(+)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -516,6 +516,9 @@ void __thermal_zone_device_update(struct
>   			governor->trip_crossed(tz, &td->trip, false);
>   	}
>   
> +	if (governor->manage)
> +		governor->manage(tz);
> +
>   	monitor_thermal_zone(tz);
>   }
>   
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -31,6 +31,7 @@ struct thermal_trip_desc {
>    * @unbind_from_tz:	callback called when a governor is unbound from a
>    *			thermal zone.
>    * @trip_crossed:	called for trip points that have just been crossed
> + * @manage:	called on thermal zone temperature updates
>    * @throttle:	callback called for every trip point even if temperature is
>    *		below the trip point temperature
>    * @update_tz:	callback called when thermal zone internals have changed, e.g.
> @@ -44,6 +45,7 @@ struct thermal_governor {
>   	void (*trip_crossed)(struct thermal_zone_device *tz,
>   			     const struct thermal_trip *trip,
>   			     bool crossed_up);
> +	void (*manage)(struct thermal_zone_device *tz);
>   	int (*throttle)(struct thermal_zone_device *tz,
>   			const struct thermal_trip *trip);
>   	void (*update_tz)(struct thermal_zone_device *tz,
> 
> 
> 

LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 06/16] thermal: gov_power_allocator: Use .manage() callback instead of .throttle()

[Lukasz Luba]

On 4/10/24 17:10, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The Power Allocator governor really only wants to be called once per
> thermal zone update and it does a special check to skip the extra,
> from its perspective, invocations of the .throttle() callback.
> 
> Make it use .manage() instead of .throttle().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_power_allocator.c |   24 +++++++-----------------
>   1 file changed, 7 insertions(+), 17 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -395,7 +395,7 @@ static void divvy_up_power(struct power_
>   	}
>   }
>   
> -static int allocate_power(struct thermal_zone_device *tz, int control_temp)
> +static void allocate_power(struct thermal_zone_device *tz, int control_temp)
>   {
>   	struct power_allocator_params *params = tz->governor_data;
>   	unsigned int num_actors = params->num_actors;
> @@ -410,7 +410,7 @@ static int allocate_power(struct thermal
>   	int i = 0, ret;
>   
>   	if (!num_actors)
> -		return -ENODEV;
> +		return;
>   
>   	/* Clean all buffers for new power estimations */
>   	memset(power, 0, params->buffer_size);
> @@ -471,8 +471,6 @@ static int allocate_power(struct thermal
>   				      num_actors, power_range,
>   				      max_allocatable_power, tz->temperature,
>   				      control_temp - tz->temperature);
> -
> -	return 0;
>   }
>   
>   /**
> @@ -745,40 +743,32 @@ static void power_allocator_unbind(struc
>   	tz->governor_data = NULL;
>   }
>   
> -static int power_allocator_throttle(struct thermal_zone_device *tz,
> -				    const struct thermal_trip *trip)
> +static void power_allocator_manage(struct thermal_zone_device *tz)
>   {
>   	struct power_allocator_params *params = tz->governor_data;
> +	const struct thermal_trip *trip = params->trip_switch_on;
>   	bool update;
>   
>   	lockdep_assert_held(&tz->lock);
>   
> -	/*
> -	 * We get called for every trip point but we only need to do
> -	 * our calculations once
> -	 */
> -	if (trip != params->trip_max)
> -		return 0;
> -
> -	trip = params->trip_switch_on;
>   	if (trip && tz->temperature < trip->temperature) {
>   		update = tz->passive;
>   		tz->passive = 0;
>   		reset_pid_controller(params);
>   		allow_maximum_power(tz, update);
> -		return 0;
> +		return;
>   	}
>   
>   	tz->passive = 1;
>   
> -	return allocate_power(tz, params->trip_max->temperature);
> +	allocate_power(tz, params->trip_max->temperature);
>   }
>   
>   static struct thermal_governor thermal_gov_power_allocator = {
>   	.name		= "power_allocator",
>   	.bind_to_tz	= power_allocator_bind,
>   	.unbind_from_tz	= power_allocator_unbind,
> -	.throttle	= power_allocator_throttle,
> +	.manage		= power_allocator_manage,
>   	.update_tz	= power_allocator_update_tz,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> 
> 
> 

LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable

[Lukasz Luba]

On 4/10/24 17:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notice that the passive field in struct thermal_zone_device is not
> used by the Power Allocator governor itself and so the ordering of
> its updates with respect to allow_maximum_power() or allocate_power()
> does not matter.
> 
> Accordingly, make power_allocator_manage() update that field right
> before returning, which allows the current value of it to be passed
> directly to allow_maximum_power() without using the additional update
> variable that can be dropped.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_power_allocator.c |    9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -747,21 +747,18 @@ static void power_allocator_manage(struc
>   {
>   	struct power_allocator_params *params = tz->governor_data;
>   	const struct thermal_trip *trip = params->trip_switch_on;
> -	bool update;
>   
>   	lockdep_assert_held(&tz->lock);
>   
>   	if (trip && tz->temperature < trip->temperature) {
> -		update = tz->passive;
> -		tz->passive = 0;
>   		reset_pid_controller(params);
> -		allow_maximum_power(tz, update);
> +		allow_maximum_power(tz, tz->passive);
> +		tz->passive = 0;
>   		return;
>   	}
>   
> -	tz->passive = 1;
> -
>   	allocate_power(tz, params->trip_max->temperature);
> +	tz->passive = 1;
>   }
>   
>   static struct thermal_governor thermal_gov_power_allocator = {
> 
> 
> 


LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 08/16] thermal: gov_step_wise: Use .manage() callback instead of .throttle()

[Lukasz Luba]

On 4/10/24 17:13, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make the Step-Wise governor use the new .manage() callback instead of
> .throttle().
> 
> Even though using .throttle() is not particularly problematic for the
> Step-Wise governor, using .manage() instead still allows it to reduce
> overhead by updating all of the colling devices once after setting

s/colling/cooling/

> target values for all of the thermal instances.

Make sense, good observation, it's pointless to call the
thermal_cdev_update() in each trip point throttle() invocation.

> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_step_wise.c |   39 +++++++++++++++++++++------------------
>   1 file changed, 21 insertions(+), 18 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_step_wise.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_step_wise.c
> +++ linux-pm/drivers/thermal/gov_step_wise.c
> @@ -109,34 +109,37 @@ static void thermal_zone_trip_update(str
>   	}
>   }
>   
> -/**
> - * step_wise_throttle - throttles devices associated with the given zone
> - * @tz: thermal_zone_device
> - * @trip: trip point
> - *
> - * Throttling Logic: This uses the trend of the thermal zone to throttle.
> - * If the thermal zone is 'heating up' this throttles all the cooling
> - * devices associated with the zone and its particular trip point, by one
> - * step. If the zone is 'cooling down' it brings back the performance of
> - * the devices by one step.
> - */
> -static int step_wise_throttle(struct thermal_zone_device *tz,
> -			      const struct thermal_trip *trip)
> +static void step_wise_manage(struct thermal_zone_device *tz)
>   {
> +	const struct thermal_trip_desc *td;
>   	struct thermal_instance *instance;
>   
>   	lockdep_assert_held(&tz->lock);
>   
> -	thermal_zone_trip_update(tz, trip);
> +	/*
> +	 * Throttling Logic: Use the trend of the thermal zone to throttle.
> +	 * If the thermal zone is 'heating up', throttle all of the cooling
> +	 * devices associated with each trip point by one step. If the zone
> +	 * is 'cooling down', it brings back the performance of the devices
> +	 * by one step.
> +	 */
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
> +
> +		if (trip->temperature == THERMAL_TEMP_INVALID ||
> +		    trip->type == THERMAL_TRIP_CRITICAL ||
> +		    trip->type == THERMAL_TRIP_HOT)
> +			continue;
> +
> +		thermal_zone_trip_update(tz, trip);
> +	}
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
>   		thermal_cdev_update(instance->cdev);
> -
> -	return 0;
>   }
>   
>   static struct thermal_governor thermal_gov_step_wise = {
> -	.name		= "step_wise",
> -	.throttle	= step_wise_throttle,
> +	.name	= "step_wise",
> +	.manage	= step_wise_manage,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_step_wise);
> 
> 
> 

LGTM w/ that 'cooling' spelling fixed.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 14/16] thermal: gov_user_space: Use .trip_crossed() instead of .throttle()

[Lukasz Luba]

On 4/10/24 18:03, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notifying user space about trip points that have not been crossed is
> not particuarly useful, so modity the User Space governor to use the

s/particuarly/particularly/

s/modity/modify/

> .trip_crossed() callback, which is only invoked for trips that have been
> crossed, instead of .throttle() that is invoked for all trips in a
> thermal zone every time the zone is updated.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Note: I am not actually sure if there is user space depending on the
> current behavior that can be broken by this change.
> 
> I can easily imagine trying to implement a complicated governor in user
> space that will look at all of the trips in the thermal zone regardless
> of whether or not they are crossed, which can be kind of helped by the
> current behavior of the user space governor.
> 
> However, the total overhead caused by it is considerable and quite
> arguably it may not be acceptable at least in some cases.

If there is a such user space governor - it should not rely on
notifications from each trip. It should rather read from sysfs
the list of all trips during setup and just act on those which
have been crossed in runtime, IMO.

Therefore, I agree with the proposed change.

> 
> ---
>   drivers/thermal/gov_user_space.c |   10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_user_space.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_user_space.c
> +++ linux-pm/drivers/thermal/gov_user_space.c
> @@ -26,11 +26,13 @@ static int user_space_bind(struct therma
>    * notify_user_space - Notifies user space about thermal events
>    * @tz: thermal_zone_device
>    * @trip: trip point
> + * @crossed_up: whether or not the trip has been crossed on the way up
>    *
>    * This function notifies the user space through UEvents.
>    */
> -static int notify_user_space(struct thermal_zone_device *tz,
> -			     const struct thermal_trip *trip)
> +static void notify_user_space(struct thermal_zone_device *tz,
> +			      const struct thermal_trip *trip,
> +			      bool crossed_up)
>   {
>   	char *thermal_prop[5];
>   	int i;
> @@ -46,13 +48,11 @@ static int notify_user_space(struct ther
>   	kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
>   	for (i = 0; i < 4; ++i)
>   		kfree(thermal_prop[i]);
> -
> -	return 0;
>   }
>   
>   static struct thermal_governor thermal_gov_user_space = {
>   	.name		= "user_space",
> -	.throttle	= notify_user_space,
> +	.trip_crossed	= notify_user_space,
>   	.bind_to_tz	= user_space_bind,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_user_space);
> 
> 
> 

LGTM w/ the spelling fixes applied

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 10/16] thermal: gov_step_wise: Clean up thermal_zone_trip_update()

[Lukasz Luba]

On 4/10/24 17:44, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Do some assorted cleanups in thermal_zone_trip_update():
> 
>   * Compute the trend value upfront.
>   * Move old_target definition to the block where it is used.
>   * Adjust white space around diagnositc messages and locking.

s/diagnositc/diagnostic/

>   * Use suitable field formatting in a message to avoid an explicit
>     cast to int.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_step_wise.c |   13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_step_wise.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_step_wise.c
> +++ linux-pm/drivers/thermal/gov_step_wise.c
> @@ -65,13 +65,10 @@ static void thermal_zone_trip_update(str
>   				     const struct thermal_trip *trip,
>   				     int trip_threshold)
>   {
> +	enum thermal_trend trend = get_tz_trend(tz, trip);
>   	int trip_id = thermal_zone_trip_id(tz, trip);
> -	enum thermal_trend trend;
>   	struct thermal_instance *instance;
>   	bool throttle = false;
> -	int old_target;
> -
> -	trend = get_tz_trend(tz, trip);
>   
>   	if (tz->temperature >= trip_threshold) {
>   		throttle = true;
> @@ -82,13 +79,16 @@ static void thermal_zone_trip_update(str
>   		trip_id, trip->type, trip_threshold, trend, throttle);
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +		int old_target;
> +
>   		if (instance->trip != trip)
>   			continue;
>   
>   		old_target = instance->target;
>   		instance->target = get_target_state(instance, trend, throttle);
> -		dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
> -					old_target, (int)instance->target);
> +
> +		dev_dbg(&instance->cdev->device, "old_target=%d, target=%ld\n",
> +			old_target, instance->target);
>   
>   		if (instance->initialized && old_target == instance->target)
>   			continue;
> @@ -104,6 +104,7 @@ static void thermal_zone_trip_update(str
>   		}
>   
>   		instance->initialized = true;
> +
>   		mutex_lock(&instance->cdev->lock);
>   		instance->cdev->updated = false; /* cdev needs update */
>   		mutex_unlock(&instance->cdev->lock);
> 
> 
> 

LGTM w/ spelling fixed.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 11/16] thermal: gov_fair_share: Use .manage() callback instead of .throttle()

[Lukasz Luba]

On 4/10/24 17:57, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The Fair Share governor tries very hard to be stateless and so it
> calls get_trip_level() from fair_share_throttle() every time, even
> though the number produced by this function for all of the trips
> during a given thermal zone update is actually the same.  Since
> get_trip_level() walks all of the trips in the thermal zone every
> time it is called, doing this may generate quite a bit of completely
> useless overhead.
> 
> For this reason, make the governor use the new .manage() callback
> instead of .throttle() which allows it to call get_trip_level() just
> once and use the value computed by it to handle all of the trips.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_fair_share.c |   37 ++++++++++++++++++++++++++-----------
>   1 file changed, 26 insertions(+), 11 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_fair_share.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_fair_share.c
> +++ linux-pm/drivers/thermal/gov_fair_share.c
> @@ -53,6 +53,7 @@ static long get_target_state(struct ther
>    * fair_share_throttle - throttles devices associated with the given zone
>    * @tz: thermal_zone_device
>    * @trip: trip point
> + * @trip_level: number of trips crossed by the zone temperature
>    *
>    * Throttling Logic: This uses three parameters to calculate the new
>    * throttle state of the cooling devices associated with the given zone.
> @@ -61,22 +62,19 @@ static long get_target_state(struct ther
>    * P1. max_state: Maximum throttle state exposed by the cooling device.
>    * P2. percentage[i]/100:
>    *	How 'effective' the 'i'th device is, in cooling the given zone.
> - * P3. cur_trip_level/max_no_of_trips:
> + * P3. trip_level/max_no_of_trips:
>    *	This describes the extent to which the devices should be throttled.
>    *	We do not want to throttle too much when we trip a lower temperature,
>    *	whereas the throttling is at full swing if we trip critical levels.
> - *	(Heavily assumes the trip points are in ascending order)
>    * new_state of cooling device = P3 * P2 * P1
>    */
> -static int fair_share_throttle(struct thermal_zone_device *tz,
> -			       const struct thermal_trip *trip)
> +static void fair_share_throttle(struct thermal_zone_device *tz,
> +				const struct thermal_trip *trip,
> +				int trip_level)
>   {
>   	struct thermal_instance *instance;
>   	int total_weight = 0;
>   	int total_instance = 0;
> -	int cur_trip_level = get_trip_level(tz);
> -
> -	lockdep_assert_held(&tz->lock);
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   		if (instance->trip != trip)
> @@ -99,18 +97,35 @@ static int fair_share_throttle(struct th
>   			percentage = (instance->weight * 100) / total_weight;
>   
>   		instance->target = get_target_state(tz, cdev, percentage,
> -						    cur_trip_level);
> +						    trip_level);
>   
>   		mutex_lock(&cdev->lock);
>   		__thermal_cdev_update(cdev);
>   		mutex_unlock(&cdev->lock);
>   	}
> +}
>   
> -	return 0;
> +static void fair_share_manage(struct thermal_zone_device *tz)
> +{
> +	int trip_level = get_trip_level(tz);
> +	const struct thermal_trip_desc *td;
> +
> +	lockdep_assert_held(&tz->lock);
> +
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
> +
> +		if (trip->temperature == THERMAL_TEMP_INVALID ||
> +		    trip->type == THERMAL_TRIP_CRITICAL ||
> +		    trip->type == THERMAL_TRIP_HOT)
> +			continue;
> +
> +		fair_share_throttle(tz, trip, trip_level);
> +	}
>   }
>   
>   static struct thermal_governor thermal_gov_fair_share = {
> -	.name		= "fair_share",
> -	.throttle	= fair_share_throttle,
> +	.name	= "fair_share",
> +	.manage	= fair_share_manage,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_fair_share);
> 
> 
> 

LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 13/16] thermal: gov_fair_share: Eliminate unnecessary integer divisions

[Lukasz Luba]

On 4/10/24 18:00, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The computations carried out by fair_share_throttle() for each trip
> point include at least one redundant integer division which introduces
> superfluous rounding errors.  Also the multiplications by 100 in it are
> not really necessary and can be eliminated.
> 
> Rearrange fair_share_throttle() to carry out only one integer division per
> trip and only as many integer multiplications as necessary and rename one
> variable in it (while at it).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_fair_share.c |   32 +++++++++++++++-----------------
>   1 file changed, 15 insertions(+), 17 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_fair_share.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_fair_share.c
> +++ linux-pm/drivers/thermal/gov_fair_share.c
> @@ -41,12 +41,6 @@ static int get_trip_level(struct thermal
>   	return trip_level;
>   }
>   
> -static long get_target_state(struct thermal_zone_device *tz,
> -		struct thermal_cooling_device *cdev, int percentage, int level)
> -{
> -	return (long)(percentage * level * cdev->max_state) / (100 * tz->num_trips);
> -}
> -
>   /**
>    * fair_share_throttle - throttles devices associated with the given zone
>    * @tz: thermal_zone_device
> @@ -58,7 +52,7 @@ static long get_target_state(struct ther
>    *
>    * Parameters used for Throttling:
>    * P1. max_state: Maximum throttle state exposed by the cooling device.
> - * P2. percentage[i]/100:
> + * P2. weight[i]/total_weight:
>    *	How 'effective' the 'i'th device is, in cooling the given zone.
>    * P3. trip_level/max_no_of_trips:
>    *	This describes the extent to which the devices should be throttled.
> @@ -72,30 +66,34 @@ static void fair_share_throttle(struct t
>   {
>   	struct thermal_instance *instance;
>   	int total_weight = 0;
> -	int total_instance = 0;
> +	int nr_instances = 0;
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   		if (instance->trip != trip)
>   			continue;
>   
>   		total_weight += instance->weight;
> -		total_instance++;
> +		nr_instances++;
>   	}
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> -		int percentage;
>   		struct thermal_cooling_device *cdev = instance->cdev;
> +		u64 dividend;
> +		u32 divisor;
>   
>   		if (instance->trip != trip)
>   			continue;
>   
> -		if (!total_weight)
> -			percentage = 100 / total_instance;
> -		else
> -			percentage = (instance->weight * 100) / total_weight;
> -
> -		instance->target = get_target_state(tz, cdev, percentage,
> -						    trip_level);
> +		dividend = trip_level;
> +		dividend *= cdev->max_state;
> +		divisor = tz->num_trips;
> +		if (total_weight) {
> +			dividend *= instance->weight;
> +			divisor *= total_weight;
> +		} else {
> +			divisor *= nr_instances;
> +		}
> +		instance->target = div_u64(dividend, divisor);
>   
>   		mutex_lock(&cdev->lock);
>   		__thermal_cdev_update(cdev);
> 
> 
> 


Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 15/16] thermal: core: Drop the .throttle() governor callback

[Lukasz Luba]

On 4/10/24 18:42, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since all of the governors in the tree have been switched over to using
> the new callbacks, either .trip_crossed() or .manage(), the .throttle()
> governor callback is not used any more, so drop it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_core.c |   11 -----------
>   drivers/thermal/thermal_core.h |    4 ----
>   2 files changed, 15 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -310,15 +310,6 @@ static struct thermal_governor *thermal_
>   	return def_governor;
>   }
>   
> -static void handle_non_critical_trips(struct thermal_zone_device *tz,
> -				      const struct thermal_trip *trip)
> -{
> -	struct thermal_governor *governor = thermal_get_tz_governor(tz);
> -
> -	if (governor->throttle)
> -		governor->throttle(tz, trip);
> -}
> -
>   void thermal_governor_update_tz(struct thermal_zone_device *tz,
>   				enum thermal_notify_event reason)
>   {
> @@ -418,8 +409,6 @@ static void handle_thermal_trip(struct t
>   
>   	if (trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT)
>   		handle_critical_trips(tz, trip);
> -	else
> -		handle_non_critical_trips(tz, trip);
>   }
>   
>   static void update_temperature(struct thermal_zone_device *tz)
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -32,8 +32,6 @@ struct thermal_trip_desc {
>    *			thermal zone.
>    * @trip_crossed:	called for trip points that have just been crossed
>    * @manage:	called on thermal zone temperature updates
> - * @throttle:	callback called for every trip point even if temperature is
> - *		below the trip point temperature
>    * @update_tz:	callback called when thermal zone internals have changed, e.g.
>    *		thermal cooling instance was added/removed
>    * @governor_list:	node in thermal_governor_list (in thermal_core.c)
> @@ -46,8 +44,6 @@ struct thermal_governor {
>   			     const struct thermal_trip *trip,
>   			     bool crossed_up);
>   	void (*manage)(struct thermal_zone_device *tz);
> -	int (*throttle)(struct thermal_zone_device *tz,
> -			const struct thermal_trip *trip);
>   	void (*update_tz)(struct thermal_zone_device *tz,
>   			  enum thermal_notify_event reason);
>   	struct list_head	governor_list;
> 
> 
> 



Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 16/16] thermal: core: Relocate critical and hot trip handling

[Lukasz Luba]

On 4/10/24 18:44, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH v1]
> 
> Modify handle_thermal_trip() to call handle_critical_trips() only after
> finding that the trip temperature has been crossed on the way up and
> remove the redundant temperature check from the latter.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_core.c |   11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -350,10 +350,6 @@ void thermal_zone_device_critical_reboot
>   static void handle_critical_trips(struct thermal_zone_device *tz,
>   				  const struct thermal_trip *trip)
>   {
> -	/* If we have not crossed the trip_temp, we do not care. */
> -	if (trip->temperature <= 0 || tz->temperature < trip->temperature)
> -		return;
> -
>   	trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, trip), trip->type);
>   
>   	if (trip->type == THERMAL_TRIP_CRITICAL)
> @@ -405,10 +401,11 @@ static void handle_thermal_trip(struct t
>   		list_add_tail(&td->notify_list_node, way_up_list);
>   		td->notify_temp = trip->temperature;
>   		td->threshold -= trip->hysteresis;
> -	}
>   
> -	if (trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT)
> -		handle_critical_trips(tz, trip);
> +		if (trip->type == THERMAL_TRIP_CRITICAL ||
> +		    trip->type == THERMAL_TRIP_HOT)
> +			handle_critical_trips(tz, trip);
> +	}
>   }
>   
>   static void update_temperature(struct thermal_zone_device *tz)
> 
> 
> 

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 09/16] thermal: gov_step_wise: Use trip thresholds instead of trip temperatures

[Lukasz Luba]

On 4/10/24 17:43, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In principle, the Step-Wise governor should take trip hystereses into

s/hystereses/hysteresis/

> account.  After all, once a trip has been crossed on the way up,
> mitigation is still needed until it is crossed on the way down.
> 
> For this reason, make it use trip thresholds that are computed by
> the core when trips are crossed, so as to apply mitigations in the
> hysteresis rages of trips that were crossed on the way up, but have
> not been crossed on the way down yet.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_step_wise.c |    9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_step_wise.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_step_wise.c
> +++ linux-pm/drivers/thermal/gov_step_wise.c
> @@ -62,7 +62,8 @@ static unsigned long get_target_state(st
>   }
>   
>   static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> -				     const struct thermal_trip *trip)
> +				     const struct thermal_trip *trip,
> +				     int trip_threshold)
>   {
>   	int trip_id = thermal_zone_trip_id(tz, trip);
>   	enum thermal_trend trend;
> @@ -72,13 +73,13 @@ static void thermal_zone_trip_update(str
>   
>   	trend = get_tz_trend(tz, trip);
>   
> -	if (tz->temperature >= trip->temperature) {
> +	if (tz->temperature >= trip_threshold) {

So this value in 'trip_threshold' might be lower than older
'trip->temperature', but not necessarily. Anyway, all good here.

>   		throttle = true;
>   		trace_thermal_zone_trip(tz, trip_id, trip->type);
>   	}
>   
>   	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
> -		trip_id, trip->type, trip->temperature, trend, throttle);
> +		trip_id, trip->type, trip_threshold, trend, throttle);
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   		if (instance->trip != trip)
> @@ -131,7 +132,7 @@ static void step_wise_manage(struct ther
>   		    trip->type == THERMAL_TRIP_HOT)
>   			continue;
>   
> -		thermal_zone_trip_update(tz, trip);
> +		thermal_zone_trip_update(tz, trip, td->threshold);
>   	}
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> 
> 
> 


LGTM w/ spelling fixed.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 12/16] thermal: gov_fair_share: Use trip thresholds instead of trip temperatures

[Lukasz Luba]

On 4/10/24 17:58, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In principle, the Fair Share governor should take trip hystereses

s/hystereses/hysteresis/

> into account.  After all, once a trip has been crossed on the way up,
> mitigation is still needed until it is crossed on the way down.
> 
> For this reason, make it use trip thresholds that are computed by
> the core when trips are crossed, so as to apply mitigations if the
> zone temperature is in a hysteresis rage of one or more trips that
> were crossed on the way up, but have not been crossed on the way
> down yet.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_fair_share.c |   14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_fair_share.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_fair_share.c
> +++ linux-pm/drivers/thermal/gov_fair_share.c
> @@ -17,28 +17,26 @@
>   
>   static int get_trip_level(struct thermal_zone_device *tz)
>   {
> -	const struct thermal_trip *level_trip = NULL;
> +	const struct thermal_trip_desc *level_td = NULL;
>   	const struct thermal_trip_desc *td;
>   	int trip_level = -1;
>   
>   	for_each_trip_desc(tz, td) {
> -		const struct thermal_trip *trip = &td->trip;
> -
> -		if (trip->temperature >= tz->temperature)
> +		if (td->threshold > tz->temperature)
>   			continue;
>   
>   		trip_level++;
>   
> -		if (!level_trip || trip->temperature > level_trip->temperature)
> -			level_trip = trip;
> +		if (!level_td || td->threshold > level_td->threshold)
> +			level_td = td;
>   	}
>   
>   	/*  Bail out if the temperature is not greater than any trips. */
>   	if (trip_level < 0)
>   		return 0;
>   
> -	trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, level_trip),
> -				level_trip->type);
> +	trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, &level_td->trip),
> +				level_td->trip.type);
>   
>   	return trip_level;
>   }
> 
> 
> 


LGTM w/ spelling fixed.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 02/16] thermal: gov_bang_bang: Use .trip_crossed() instead of .throttle()

[Daniel Lezcano]

On 10/04/2024 18:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The Bang-Bang governor really is only concerned about trip point
> crossing, so it can use the new .trip_crossed() callback instead of
> .throttle() that is not particularly suitable for it.
> 
> Modify it to do so which also takes trip hysteresis into account, so the
> governor does not need to use it directly any more.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
<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 v1 03/16] thermal: gov_bang_bang: Clean up thermal_zone_trip_update()

[Daniel Lezcano]

On 10/04/2024 18:05, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Do the following cleanups in thermal_zone_trip_update():
> 
>   * Drop the useless "zero hysteresis" message.
>   * Eliminate the trip_index local variable that is redundant.
>   * Drop 2 comments that are not useful.
>   * Downgrade a diagnostic message from pr_warn() to pr_debug().
>   * Use consistent field formatting in diagnostic messages.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


-- 
<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 v1 04/16] thermal: gov_bang_bang: Fold thermal_zone_trip_update() into its caller

[Daniel Lezcano]

On 10/04/2024 18:06, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Fold thermal_zone_trip_update() into bang_bang_control() which is the
> only caller of it to reduce code size and make it easier to follow.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
<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 v1 05/16] thermal: core: Introduce .manage() callback for thermal governors

[Daniel Lezcano]

On 10/04/2024 18:08, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce a new thermal governor callback called .manage() that will be
> invoked once per thermal zone update after processing all of the trip
> points in the core.
> 
> This will allow governors that look at multiple trip points together
> to check all of them in a consistent configuration, so they don't need
> to play tricks with skipping .throttle() invocations that they are not
> interested in and they can avoid carrying out the same computations for
> multiple times in one cycle.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
<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 v1 01/16] thermal: core: Introduce .trip_crossed() callback for thermal governors

[Daniel Lezcano]

On 10/04/2024 18:10, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce a new thermal governor callback called .trip_crossed()
> that will be invoked whenever a trip point is crossed by the zone
> temperature, either on the way up or on the way down.
> 
> The trip crossing direction information will be passed to it and if
> multiple trips are crossed in the same direction during one thermal zone
> update, the new callback will be invoked for them in temperature order,
> either ascending or descending, depending on the trip crossing
> direction.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_core.c |   19 +++++++++++++++++--
>   drivers/thermal/thermal_core.h |    4 ++++
>   2 files changed, 21 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -302,11 +302,21 @@ static void monitor_thermal_zone(struct
>   		thermal_zone_device_set_polling(tz, tz->polling_delay_jiffies);
>   }
>   
> +static struct thermal_governor *thermal_get_tz_governor(struct thermal_zone_device *tz)
> +{
> +	if (tz->governor)
> +		return tz->governor;
> +
> +	return def_governor;
> +}
> +
>   static void handle_non_critical_trips(struct thermal_zone_device *tz,
>   				      const struct thermal_trip *trip)
>   {
> -	tz->governor ? tz->governor->throttle(tz, trip) :
> -		       def_governor->throttle(tz, trip);
> +	struct thermal_governor *governor = thermal_get_tz_governor(tz);
> +
> +	if (governor->throttle)
> +		governor->throttle(tz, trip);
>   }
>   
>   void thermal_governor_update_tz(struct thermal_zone_device *tz,
> @@ -470,6 +480,7 @@ static int thermal_trip_notify_cmp(void
>   void __thermal_zone_device_update(struct thermal_zone_device *tz,
>   				  enum thermal_notify_event event)
>   {
> +	struct thermal_governor *governor = thermal_get_tz_governor(tz);
>   	struct thermal_trip_desc *td;
>   	LIST_HEAD(way_down_list);
>   	LIST_HEAD(way_up_list);
> @@ -493,12 +504,16 @@ void __thermal_zone_device_update(struct
>   	list_for_each_entry(td, &way_up_list, notify_list_node) {
>   		thermal_notify_tz_trip_up(tz, &td->trip);
>   		thermal_debug_tz_trip_up(tz, &td->trip);
> +		if (governor->trip_crossed)
> +			governor->trip_crossed(tz, &td->trip, true);

Is it possible to wrap this into a function ? So we keep the calls at 
the same level in this block

>   	}
>   
>   	list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
>   	list_for_each_entry(td, &way_down_list, notify_list_node) {
>   		thermal_notify_tz_trip_down(tz, &td->trip);
>   		thermal_debug_tz_trip_down(tz, &td->trip);
> +		if (governor->trip_crossed)
> +			governor->trip_crossed(tz, &td->trip, false);

idem

>   	}
>   
>   	monitor_thermal_zone(tz);
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -30,6 +30,7 @@ struct thermal_trip_desc {
>    *		otherwise it fails.
>    * @unbind_from_tz:	callback called when a governor is unbound from a
>    *			thermal zone.
> + * @trip_crossed:	called for trip points that have just been crossed
>    * @throttle:	callback called for every trip point even if temperature is
>    *		below the trip point temperature
>    * @update_tz:	callback called when thermal zone internals have changed, e.g.
> @@ -40,6 +41,9 @@ struct thermal_governor {
>   	const char *name;
>   	int (*bind_to_tz)(struct thermal_zone_device *tz);
>   	void (*unbind_from_tz)(struct thermal_zone_device *tz);
> +	void (*trip_crossed)(struct thermal_zone_device *tz,
> +			     const struct thermal_trip *trip,
> +			     bool crossed_up);
>   	int (*throttle)(struct thermal_zone_device *tz,
>   			const struct thermal_trip *trip);
>   	void (*update_tz)(struct thermal_zone_device *tz,
> 
> 
> 

-- 
<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 v1 01/16] thermal: core: Introduce .trip_crossed() callback for thermal governors

[Rafael J. Wysocki]

On Tue, Apr 23, 2024 at 7:14 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 10/04/2024 18:10, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Introduce a new thermal governor callback called .trip_crossed()
> > that will be invoked whenever a trip point is crossed by the zone
> > temperature, either on the way up or on the way down.
> >
> > The trip crossing direction information will be passed to it and if
> > multiple trips are crossed in the same direction during one thermal zone
> > update, the new callback will be invoked for them in temperature order,
> > either ascending or descending, depending on the trip crossing
> > direction.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/thermal_core.c |   19 +++++++++++++++++--
> >   drivers/thermal/thermal_core.h |    4 ++++
> >   2 files changed, 21 insertions(+), 2 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.c
> > +++ linux-pm/drivers/thermal/thermal_core.c
> > @@ -302,11 +302,21 @@ static void monitor_thermal_zone(struct
> >               thermal_zone_device_set_polling(tz, tz->polling_delay_jiffies);
> >   }
> >
> > +static struct thermal_governor *thermal_get_tz_governor(struct thermal_zone_device *tz)
> > +{
> > +     if (tz->governor)
> > +             return tz->governor;
> > +
> > +     return def_governor;
> > +}
> > +
> >   static void handle_non_critical_trips(struct thermal_zone_device *tz,
> >                                     const struct thermal_trip *trip)
> >   {
> > -     tz->governor ? tz->governor->throttle(tz, trip) :
> > -                    def_governor->throttle(tz, trip);
> > +     struct thermal_governor *governor = thermal_get_tz_governor(tz);
> > +
> > +     if (governor->throttle)
> > +             governor->throttle(tz, trip);
> >   }
> >
> >   void thermal_governor_update_tz(struct thermal_zone_device *tz,
> > @@ -470,6 +480,7 @@ static int thermal_trip_notify_cmp(void
> >   void __thermal_zone_device_update(struct thermal_zone_device *tz,
> >                                 enum thermal_notify_event event)
> >   {
> > +     struct thermal_governor *governor = thermal_get_tz_governor(tz);
> >       struct thermal_trip_desc *td;
> >       LIST_HEAD(way_down_list);
> >       LIST_HEAD(way_up_list);
> > @@ -493,12 +504,16 @@ void __thermal_zone_device_update(struct
> >       list_for_each_entry(td, &way_up_list, notify_list_node) {
> >               thermal_notify_tz_trip_up(tz, &td->trip);
> >               thermal_debug_tz_trip_up(tz, &td->trip);
> > +             if (governor->trip_crossed)
> > +                     governor->trip_crossed(tz, &td->trip, true);
>
> Is it possible to wrap this into a function ? So we keep the calls at
> the same level in this block

I can send a separate patch for this if you want me to.

Thanks!

Re: [PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable

[Daniel Lezcano]

On 10/04/2024 18:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notice that the passive field in struct thermal_zone_device is not
> used by the Power Allocator governor itself and so the ordering of
> its updates with respect to allow_maximum_power() or allocate_power()
> does not matter.
> 
> Accordingly, make power_allocator_manage() update that field right
> before returning, which allows the current value of it to be passed
> directly to allow_maximum_power() without using the additional update
> variable that can be dropped.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

The step_wise and the power allocator are changing the tz->passive 
values, so telling the core to start and stop the passive mitigation timer.

It looks strange that a plugin controls the core internal and not the 
opposite.

I'm wondering if it would not make sense to have the following ops:

	.start
	.stop

.start is called when the first trip point is crossed the way up
.stop is called when the first trip point is crossed the way down

  - The core is responsible to start and stop the passive mitigation timer.

  - the governors do no longer us tz->passive

The reset of the governor can happen at start or stop, as well as the 
device cooling states.


>   drivers/thermal/gov_power_allocator.c |    9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -747,21 +747,18 @@ static void power_allocator_manage(struc
>   {
>   	struct power_allocator_params *params = tz->governor_data;
>   	const struct thermal_trip *trip = params->trip_switch_on;
> -	bool update;
>   
>   	lockdep_assert_held(&tz->lock);
>   
>   	if (trip && tz->temperature < trip->temperature) {
> -		update = tz->passive;
> -		tz->passive = 0;
>   		reset_pid_controller(params);
> -		allow_maximum_power(tz, update);
> +		allow_maximum_power(tz, tz->passive);
> +		tz->passive = 0;
>   		return;
>   	}
>   
> -	tz->passive = 1;
> -
>   	allocate_power(tz, params->trip_max->temperature);
> +	tz->passive = 1;
>   }
>   
>   static struct thermal_governor thermal_gov_power_allocator = {
> 
> 
> 

-- 
<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 v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable

[Rafael J. Wysocki]

On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Notice that the passive field in struct thermal_zone_device is not
> > used by the Power Allocator governor itself and so the ordering of
> > its updates with respect to allow_maximum_power() or allocate_power()
> > does not matter.
> >
> > Accordingly, make power_allocator_manage() update that field right
> > before returning, which allows the current value of it to be passed
> > directly to allow_maximum_power() without using the additional update
> > variable that can be dropped.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
>
> The step_wise and the power allocator are changing the tz->passive
> values, so telling the core to start and stop the passive mitigation timer.
>
> It looks strange that a plugin controls the core internal and not the
> opposite.
>
> I'm wondering if it would not make sense to have the following ops:
>
>         .start
>         .stop
>
> .start is called when the first trip point is crossed the way up
> .stop is called when the first trip point is crossed the way down
>
>   - The core is responsible to start and stop the passive mitigation timer.
>
>   - the governors do no longer us tz->passive
>
> The reset of the governor can happen at start or stop, as well as the
> device cooling states.

I have a patch that simply increments tz->passive when a passive trip
point is passed on the way up and decrements it when a passive trip
point is crossed on the way down.  It appears to work reasonably well.

Re: [PATCH v1 01/16] thermal: core: Introduce .trip_crossed() callback for thermal governors

[Daniel Lezcano]

On 23/04/2024 19:25, Rafael J. Wysocki wrote:
> On Tue, Apr 23, 2024 at 7:14 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 10/04/2024 18:10, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Introduce a new thermal governor callback called .trip_crossed()
>>> that will be invoked whenever a trip point is crossed by the zone
>>> temperature, either on the way up or on the way down.
>>>
>>> The trip crossing direction information will be passed to it and if
>>> multiple trips are crossed in the same direction during one thermal zone
>>> update, the new callback will be invoked for them in temperature order,
>>> either ascending or descending, depending on the trip crossing
>>> direction.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---

[ ... ]

>>> +             if (governor->trip_crossed)
>>> +                     governor->trip_crossed(tz, &td->trip, true);
>>
>> Is it possible to wrap this into a function ? So we keep the calls at
>> the same level in this block
> 
> I can send a separate patch for this if you want me to.

Yes, 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 v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable

[Daniel Lezcano]

On 23/04/2024 19:54, Rafael J. Wysocki wrote:
> On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Notice that the passive field in struct thermal_zone_device is not
>>> used by the Power Allocator governor itself and so the ordering of
>>> its updates with respect to allow_maximum_power() or allocate_power()
>>> does not matter.
>>>
>>> Accordingly, make power_allocator_manage() update that field right
>>> before returning, which allows the current value of it to be passed
>>> directly to allow_maximum_power() without using the additional update
>>> variable that can be dropped.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>
>> The step_wise and the power allocator are changing the tz->passive
>> values, so telling the core to start and stop the passive mitigation timer.
>>
>> It looks strange that a plugin controls the core internal and not the
>> opposite.
>>
>> I'm wondering if it would not make sense to have the following ops:
>>
>>          .start
>>          .stop
>>
>> .start is called when the first trip point is crossed the way up
>> .stop is called when the first trip point is crossed the way down
>>
>>    - The core is responsible to start and stop the passive mitigation timer.
>>
>>    - the governors do no longer us tz->passive
>>
>> The reset of the governor can happen at start or stop, as well as the
>> device cooling states.
> 
> I have a patch that simply increments tz->passive when a passive trip
> point is passed on the way up and decrements it when a passive trip
> point is crossed on the way down.  It appears to work reasonably well.

Does it make the governors getting ride of it ? Or at least not changing 
its value ?

-- 
<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 v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable

[Rafael J. Wysocki]

On Tue, Apr 23, 2024 at 8:00 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 23/04/2024 19:54, Rafael J. Wysocki wrote:
> > On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> Notice that the passive field in struct thermal_zone_device is not
> >>> used by the Power Allocator governor itself and so the ordering of
> >>> its updates with respect to allow_maximum_power() or allocate_power()
> >>> does not matter.
> >>>
> >>> Accordingly, make power_allocator_manage() update that field right
> >>> before returning, which allows the current value of it to be passed
> >>> directly to allow_maximum_power() without using the additional update
> >>> variable that can be dropped.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> ---
> >>
> >> The step_wise and the power allocator are changing the tz->passive
> >> values, so telling the core to start and stop the passive mitigation timer.
> >>
> >> It looks strange that a plugin controls the core internal and not the
> >> opposite.
> >>
> >> I'm wondering if it would not make sense to have the following ops:
> >>
> >>          .start
> >>          .stop
> >>
> >> .start is called when the first trip point is crossed the way up
> >> .stop is called when the first trip point is crossed the way down
> >>
> >>    - The core is responsible to start and stop the passive mitigation timer.
> >>
> >>    - the governors do no longer us tz->passive
> >>
> >> The reset of the governor can happen at start or stop, as well as the
> >> device cooling states.
> >
> > I have a patch that simply increments tz->passive when a passive trip
> > point is passed on the way up and decrements it when a passive trip
> > point is crossed on the way down.  It appears to work reasonably well.
>
> Does it make the governors getting ride of it ? Or at least not changing
> its value ?

Not yet, but I'm going to update it this way.  The governors should
not mess up with tz->passive IMV.

Re: [PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable

[Daniel Lezcano]

On 23/04/2024 20:05, Rafael J. Wysocki wrote:
> On Tue, Apr 23, 2024 at 8:00 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 23/04/2024 19:54, Rafael J. Wysocki wrote:
>>> On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> Notice that the passive field in struct thermal_zone_device is not
>>>>> used by the Power Allocator governor itself and so the ordering of
>>>>> its updates with respect to allow_maximum_power() or allocate_power()
>>>>> does not matter.
>>>>>
>>>>> Accordingly, make power_allocator_manage() update that field right
>>>>> before returning, which allows the current value of it to be passed
>>>>> directly to allow_maximum_power() without using the additional update
>>>>> variable that can be dropped.
>>>>>
>>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>> ---
>>>>
>>>> The step_wise and the power allocator are changing the tz->passive
>>>> values, so telling the core to start and stop the passive mitigation timer.
>>>>
>>>> It looks strange that a plugin controls the core internal and not the
>>>> opposite.
>>>>
>>>> I'm wondering if it would not make sense to have the following ops:
>>>>
>>>>           .start
>>>>           .stop
>>>>
>>>> .start is called when the first trip point is crossed the way up
>>>> .stop is called when the first trip point is crossed the way down
>>>>
>>>>     - The core is responsible to start and stop the passive mitigation timer.
>>>>
>>>>     - the governors do no longer us tz->passive
>>>>
>>>> The reset of the governor can happen at start or stop, as well as the
>>>> device cooling states.
>>>
>>> I have a patch that simply increments tz->passive when a passive trip
>>> point is passed on the way up and decrements it when a passive trip
>>> point is crossed on the way down.  It appears to work reasonably well.
>>
>> Does it make the governors getting ride of it ? Or at least not changing
>> its value ?
> 
> Not yet, but I'm going to update it this way.  The governors should
> not mess up with tz->passive IMV.

+1

-- 
<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 v1 09/16] thermal: gov_step_wise: Use trip thresholds instead of trip temperatures

[Daniel Lezcano]

On 10/04/2024 18:43, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In principle, the Step-Wise governor should take trip hystereses into
> account.  After all, once a trip has been crossed on the way up,
> mitigation is still needed until it is crossed on the way down.
> 
> For this reason, make it use trip thresholds that are computed by
> the core when trips are crossed, so as to apply mitigations in the
> hysteresis rages of trips that were crossed on the way up, but have
> not been crossed on the way down yet.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
<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 v1 08/16] thermal: gov_step_wise: Use .manage() callback instead of .throttle()

[Daniel Lezcano]

On 10/04/2024 18:13, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make the Step-Wise governor use the new .manage() callback instead of
> .throttle().
> 
> Even though using .throttle() is not particularly problematic for the
> Step-Wise governor, using .manage() instead still allows it to reduce
> overhead by updating all of the colling devices once after setting
> target values for all of the thermal instances.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
<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 v1 15/16] thermal: core: Drop the .throttle() governor callback

[Daniel Lezcano]

On Wed, Apr 10, 2024 at 07:42:35PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since all of the governors in the tree have been switched over to using
> the new callbacks, either .trip_crossed() or .manage(), the .throttle()
> governor callback is not used any more, so drop it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Re: [PATCH v1 14/16] thermal: gov_user_space: Use .trip_crossed() instead of .throttle()

[Daniel Lezcano]

On Wed, Apr 10, 2024 at 07:03:10PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notifying user space about trip points that have not been crossed is
> not particuarly useful, so modity the User Space governor to use the
> .trip_crossed() callback, which is only invoked for trips that have been
> crossed, instead of .throttle() that is invoked for all trips in a
> thermal zone every time the zone is updated.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

I would also consider removing this governor which is pointless now that we
have the netlink notification mechanism

Re: [PATCH v1 16/16] thermal: core: Relocate critical and hot trip handling

[Daniel Lezcano]

On Wed, Apr 10, 2024 at 07:44:34PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH v1] 
> 
> Modify handle_thermal_trip() to call handle_critical_trips() only after
> finding that the trip temperature has been crossed on the way up and
> remove the redundant temperature check from the latter.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 

 <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 v1 14/16] thermal: gov_user_space: Use .trip_crossed() instead of .throttle()

[Srinivas Pandruvada]

On 4/24/24 02:14, Daniel Lezcano wrote:
> On Wed, Apr 10, 2024 at 07:03:10PM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Notifying user space about trip points that have not been crossed is
>> not particuarly useful, so modity the User Space governor to use the
>> .trip_crossed() callback, which is only invoked for trips that have been
>> crossed, instead of .throttle() that is invoked for all trips in a
>> thermal zone every time the zone is updated.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
> I would also consider removing this governor which is pointless now that we
> have the netlink notification mechanism

That is a good goal, But, not there yet to deprecate.

Thanks,

Srinivas

>

Re: [PATCH v1 14/16] thermal: gov_user_space: Use .trip_crossed() instead of .throttle()

[Daniel Lezcano]

On 24/04/2024 13:32, Srinivas Pandruvada wrote:
> 
> On 4/24/24 02:14, Daniel Lezcano wrote:
>> On Wed, Apr 10, 2024 at 07:03:10PM +0200, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Notifying user space about trip points that have not been crossed is
>>> not particuarly useful, so modity the User Space governor to use the
>>> .trip_crossed() callback, which is only invoked for trips that have been
>>> crossed, instead of .throttle() that is invoked for all trips in a
>>> thermal zone every time the zone is updated.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>> I would also consider removing this governor which is pointless now 
>> that we
>> have the netlink notification mechanism
> 
> That is a good goal, But, not there yet to deprecate.

What can be done to deprecate it ?


-- 
<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 v1 14/16] thermal: gov_user_space: Use .trip_crossed() instead of .throttle()

[Srinivas Pandruvada]

On 4/24/24 04:34, Daniel Lezcano wrote:
> On 24/04/2024 13:32, Srinivas Pandruvada wrote:
>>
>> On 4/24/24 02:14, Daniel Lezcano wrote:
>>> On Wed, Apr 10, 2024 at 07:03:10PM +0200, Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> Notifying user space about trip points that have not been crossed is
>>>> not particuarly useful, so modity the User Space governor to use the
>>>> .trip_crossed() callback, which is only invoked for trips that have 
>>>> been
>>>> crossed, instead of .throttle() that is invoked for all trips in a
>>>> thermal zone every time the zone is updated.
>>>>
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> ---
>>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>> I would also consider removing this governor which is pointless now 
>>> that we
>>> have the netlink notification mechanism
>>
>> That is a good goal, But, not there yet to deprecate.
>
> What can be done to deprecate it ?
>
>

First we need to migrate all the existing usage to netlink. We need to 
add some more netlink notifications. That is relatively easy.

The problem is user space changes are much slower to push than kernel. 
Kernels changes gets deployed at faster pace in some distributions.

Thanks,

Srinivas