diff mbox series

[v1] thermal: gov_bang_bang: Adjust states of all uninitialized instances

Message ID 6103874.lOV4Wx5bFT@rjwysocki.net
State New
Headers show
Series [v1] thermal: gov_bang_bang: Adjust states of all uninitialized instances | expand

Commit Message

Rafael J. Wysocki Aug. 26, 2024, 4:23 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If a cooling device is registered after a thermal zone it should be
bound to and the trip point it should be bound to has already been
crossed by the zone temperature on the way up, the cooling device's
state may need to be adjusted, but the Bang-bang governor will not
do that because its .manage() callback only looks at thermal instances
for trip points whose thresholds are below or at the zone temperature.

Address this by updating bang_bang_manage() to look at all of the
uninitialized thermal instances and setting their target states in
accordance with the position of the zone temperature with respect to
the threshold of the given trip point.

Fixes: 5f64b4a1ab1b ("thermal: gov_bang_bang: Add .manage() callback")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Found by code inspection while considering a related change.

I don't thik it is super-urgent, but it qualifies as 6.12 material IMV.

---
 drivers/thermal/gov_bang_bang.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Lukasz Luba Sept. 5, 2024, 8:33 a.m. UTC | #1
On 8/26/24 17:23, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If a cooling device is registered after a thermal zone it should be
> bound to and the trip point it should be bound to has already been
> crossed by the zone temperature on the way up, the cooling device's
> state may need to be adjusted, but the Bang-bang governor will not
> do that because its .manage() callback only looks at thermal instances
> for trip points whose thresholds are below or at the zone temperature.
> 
> Address this by updating bang_bang_manage() to look at all of the
> uninitialized thermal instances and setting their target states in
> accordance with the position of the zone temperature with respect to
> the threshold of the given trip point.
> 
> Fixes: 5f64b4a1ab1b ("thermal: gov_bang_bang: Add .manage() callback")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Found by code inspection while considering a related change.
> 
> I don't thik it is super-urgent, but it qualifies as 6.12 material IMV.
> 
> ---
>   drivers/thermal/gov_bang_bang.c |   14 ++++++--------
>   1 file changed, 6 insertions(+), 8 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
> @@ -92,23 +92,21 @@ static void bang_bang_manage(struct ther
>   
>   	for_each_trip_desc(tz, td) {
>   		const struct thermal_trip *trip = &td->trip;
> +		bool turn_on;
>   
> -		if (tz->temperature >= td->threshold ||
> -		    trip->temperature == THERMAL_TEMP_INVALID ||
> +		if (trip->temperature == THERMAL_TEMP_INVALID ||
>   		    trip->type == THERMAL_TRIP_CRITICAL ||
>   		    trip->type == THERMAL_TRIP_HOT)
>   			continue;
>   
>   		/*
> -		 * If the initial cooling device state is "on", but the zone
> -		 * temperature is not above the trip point, the core will not
> -		 * call bang_bang_control() until the zone temperature reaches
> -		 * the trip point temperature which may be never.  In those
> -		 * cases, set the initial state of the cooling device to 0.
> +		 * Adjust the target states for uninitialized thermal instances
> +		 * to the thermal zone temperature and the trip point threshold.
>   		 */
> +		turn_on = tz->temperature >= td->threshold;
>   		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   			if (!instance->initialized && instance->trip == trip)
> -				bang_bang_set_instance_target(instance, 0);
> +				bang_bang_set_instance_target(instance, turn_on);
>   		}
>   	}
>   
> 
> 
> 

That makes sense.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Peter Kästle Sept. 15, 2024, 5:07 p.m. UTC | #2
On 26.08.24 18:23, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If a cooling device is registered after a thermal zone it should be
> bound to and the trip point it should be bound to has already been
> crossed by the zone temperature on the way up, the cooling device's
> state may need to be adjusted, but the Bang-bang governor will not
> do that because its .manage() callback only looks at thermal instances
> for trip points whose thresholds are below or at the zone temperature.
> 
> Address this by updating bang_bang_manage() to look at all of the
> uninitialized thermal instances and setting their target states in
> accordance with the position of the zone temperature with respect to
> the threshold of the given trip point.
> 
> Fixes: 5f64b4a1ab1b ("thermal: gov_bang_bang: Add .manage() callback")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Peter Kaestle <peter@piie.net>

> ---
> 
> Found by code inspection while considering a related change.
> 
> I don't thik it is super-urgent, but it qualifies as 6.12 material IMV.
> 
> ---
>   drivers/thermal/gov_bang_bang.c |   14 ++++++--------
>   1 file changed, 6 insertions(+), 8 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
> @@ -92,23 +92,21 @@ static void bang_bang_manage(struct ther
>   
>   	for_each_trip_desc(tz, td) {
>   		const struct thermal_trip *trip = &td->trip;
> +		bool turn_on;
>   
> -		if (tz->temperature >= td->threshold ||
> -		    trip->temperature == THERMAL_TEMP_INVALID ||
> +		if (trip->temperature == THERMAL_TEMP_INVALID ||
>   		    trip->type == THERMAL_TRIP_CRITICAL ||
>   		    trip->type == THERMAL_TRIP_HOT)
>   			continue;
>   
>   		/*
> -		 * If the initial cooling device state is "on", but the zone
> -		 * temperature is not above the trip point, the core will not
> -		 * call bang_bang_control() until the zone temperature reaches
> -		 * the trip point temperature which may be never.  In those
> -		 * cases, set the initial state of the cooling device to 0.
> +		 * Adjust the target states for uninitialized thermal instances
> +		 * to the thermal zone temperature and the trip point threshold.
>   		 */
> +		turn_on = tz->temperature >= td->threshold;
>   		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   			if (!instance->initialized && instance->trip == trip)
> -				bang_bang_set_instance_target(instance, 0);
> +				bang_bang_set_instance_target(instance, turn_on);
>   		}
>   	}
>   
> 
>
diff mbox series

Patch

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
@@ -92,23 +92,21 @@  static void bang_bang_manage(struct ther
 
 	for_each_trip_desc(tz, td) {
 		const struct thermal_trip *trip = &td->trip;
+		bool turn_on;
 
-		if (tz->temperature >= td->threshold ||
-		    trip->temperature == THERMAL_TEMP_INVALID ||
+		if (trip->temperature == THERMAL_TEMP_INVALID ||
 		    trip->type == THERMAL_TRIP_CRITICAL ||
 		    trip->type == THERMAL_TRIP_HOT)
 			continue;
 
 		/*
-		 * If the initial cooling device state is "on", but the zone
-		 * temperature is not above the trip point, the core will not
-		 * call bang_bang_control() until the zone temperature reaches
-		 * the trip point temperature which may be never.  In those
-		 * cases, set the initial state of the cooling device to 0.
+		 * Adjust the target states for uninitialized thermal instances
+		 * to the thermal zone temperature and the trip point threshold.
 		 */
+		turn_on = tz->temperature >= td->threshold;
 		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 			if (!instance->initialized && instance->trip == trip)
-				bang_bang_set_instance_target(instance, 0);
+				bang_bang_set_instance_target(instance, turn_on);
 		}
 	}