[PATCH v1 0/4] thermal: gov_bang_bang: Prevent cooling devices from getting stuck in the "on" state

[PATCH v1 0/4] thermal: gov_bang_bang: Prevent cooling devices from getting stuck in the "on" state

[Rafael J. Wysocki]

Hi Everyone,

After changes made in 6.10, the Bang-bang governor has an initialization problem
on systems where cooling devices start in the "on" state, but the thermal zone
temperature stays below the corresponding trip points.

Namely, the Bang-bang governor only implements a .trip_crossed() callback which
only runs when a trip point is crossed.  If the zone temperature is always below
the trip point, that callback will never be invoked.  Now, if a cooling device
bound to that trip point starts in the "on" state, the governor has no chance
to change its state to "off".

This currently happens in the acerhdf driver, but it may as well happen elsewhere,
so I think that it needs to be addressed in the thermal subsystem.

It can be addressed by adding a .manage() callback to the Bang-bang governor,
which is done in patch [3/4].  That callback will be invoked every time
__thermal_zone_device_update() runs, not just when a trip is crossed, so it
can adjust the states of the cooling devices to the thermal zone temperature.
However, after running once, it becomes a pure needless overhead because the
states of cooling devices only need to be fixed up once (modulo some special
situations like system resume).

That's addressed in patch [4/4] which uses governor_data to store the information
on whether or not the states of the cooling devices will need to be adjusted.

Patches [1-2/4] are preliminary, but IMV it is better to make these changes
separately for clarity, but also in case they turn out to have a functional
effect which is not expected.

Overall, this series is a fix candidate for 6.11-rc because the change in
behavior addressed by it can be regarded as a regression with respect to 6.9.

Unfortunately, it affects this series:

https://lore.kernel.org/linux-pm/114901234.nniJfEyVGO@rjwysocki.net/

which will need to be reordered and rebased (slightly), but because I've dropped
one broken patch from it already, it will need to be changed anyway.

Thanks!

[PATCH v1 1/4] thermal: gov_bang_bang: Call __thermal_cdev_update() directly

[Rafael J. Wysocki]

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

Instead of clearing the "updated" flag for each cooling device
affected by the trip point crossing in bang_bang_control() and
walking all thermal instances to run thermal_cdev_update() for all
of the affected cooling devices, call __thermal_cdev_update()
directly for each of them.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c |    5 +----
 1 file changed, 1 insertion(+), 4 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
@@ -71,12 +71,9 @@ static void bang_bang_control(struct the
 		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
 
 		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false; /* cdev needs update */
+		__thermal_cdev_update(instance->cdev);
 		mutex_unlock(&instance->cdev->lock);
 	}
-
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-		thermal_cdev_update(instance->cdev);
 }
 
 static struct thermal_governor thermal_gov_bang_bang = {

Re: [PATCH v1 1/4] thermal: gov_bang_bang: Call __thermal_cdev_update() directly

[Peter Kästle]

On 13.08.24 16:25, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Instead of clearing the "updated" flag for each cooling device
> affected by the trip point crossing in bang_bang_control() and
> walking all thermal instances to run thermal_cdev_update() for all
> of the affected cooling devices, call __thermal_cdev_update()
> directly for each of them.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Peter Kästle <peter@piie.net>

> ---
>   drivers/thermal/gov_bang_bang.c |    5 +----
>   1 file changed, 1 insertion(+), 4 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
> @@ -71,12 +71,9 @@ static void bang_bang_control(struct the
>   		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
>   
>   		mutex_lock(&instance->cdev->lock);
> -		instance->cdev->updated = false; /* cdev needs update */
> +		__thermal_cdev_update(instance->cdev);
>   		mutex_unlock(&instance->cdev->lock);
>   	}
> -
> -	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> -		thermal_cdev_update(instance->cdev);
>   }
>   
>   static struct thermal_governor thermal_gov_bang_bang = {
> 
> 
> 

Re: [PATCH v1 1/4] thermal: gov_bang_bang: Call __thermal_cdev_update() directly

[Zhang, Rui]

On Tue, 2024-08-13 at 16:25 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Instead of clearing the "updated" flag for each cooling device
> affected by the trip point crossing in bang_bang_control() and
> walking all thermal instances to run thermal_cdev_update() for all
> of the affected cooling devices, call __thermal_cdev_update()
> directly for each of them.

with this change, we may invoke thermal_cdev_set_cur_state() for
multiple times instead of one, in one bang_bang_control() run.

So this effectively changes the notifications and statistics.

If this is not a problem, maybe better to mention this change in the
changelog?

thanks,
rui
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/thermal/gov_bang_bang.c |    5 +----
>  1 file changed, 1 insertion(+), 4 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
> @@ -71,12 +71,9 @@ static void bang_bang_control(struct the
>                 dev_dbg(&instance->cdev->device, "target=%ld\n",
> instance->target);
>  
>                 mutex_lock(&instance->cdev->lock);
> -               instance->cdev->updated = false; /* cdev needs update
> */
> +               __thermal_cdev_update(instance->cdev);
>                 mutex_unlock(&instance->cdev->lock);
>         }
> -
> -       list_for_each_entry(instance, &tz->thermal_instances,
> tz_node)
> -               thermal_cdev_update(instance->cdev);
>  }
>  
>  static struct thermal_governor thermal_gov_bang_bang = {
> 
> 
> 

Re: [PATCH v1 1/4] thermal: gov_bang_bang: Call __thermal_cdev_update() directly

[Rafael J. Wysocki]

On Wed, Aug 14, 2024 at 8:18 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Tue, 2024-08-13 at 16:25 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Instead of clearing the "updated" flag for each cooling device
> > affected by the trip point crossing in bang_bang_control() and
> > walking all thermal instances to run thermal_cdev_update() for all
> > of the affected cooling devices, call __thermal_cdev_update()
> > directly for each of them.
>
> with this change, we may invoke thermal_cdev_set_cur_state() for
> multiple times instead of one, in one bang_bang_control() run.

No, this cannot happen AFAICS because one cooling device cannot be
bound to the same trip point more than once.

Since bang_bang_control() only checks thermal instances for one trip
point, all cooling devices pointed to by them are guaranteed to be
different.

> So this effectively changes the notifications and statistics.
>
> If this is not a problem, maybe better to mention this change in the
> changelog?
>
> thanks,
> rui
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/thermal/gov_bang_bang.c |    5 +----
> >  1 file changed, 1 insertion(+), 4 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
> > @@ -71,12 +71,9 @@ static void bang_bang_control(struct the
> >                 dev_dbg(&instance->cdev->device, "target=%ld\n",
> > instance->target);
> >
> >                 mutex_lock(&instance->cdev->lock);
> > -               instance->cdev->updated = false; /* cdev needs update
> > */
> > +               __thermal_cdev_update(instance->cdev);
> >                 mutex_unlock(&instance->cdev->lock);
> >         }
> > -
> > -       list_for_each_entry(instance, &tz->thermal_instances,
> > tz_node)
> > -               thermal_cdev_update(instance->cdev);
> >  }
> >
> >  static struct thermal_governor thermal_gov_bang_bang = {
> >
> >
> >
>

Re: [PATCH v1 1/4] thermal: gov_bang_bang: Call __thermal_cdev_update() directly

[Zhang, Rui]

On Tue, 2024-08-13 at 16:25 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Instead of clearing the "updated" flag for each cooling device
> affected by the trip point crossing in bang_bang_control() and
> walking all thermal instances to run thermal_cdev_update() for all
> of the affected cooling devices, call __thermal_cdev_update()
> directly for each of them.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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

thanks,
rui

> ---
>  drivers/thermal/gov_bang_bang.c |    5 +----
>  1 file changed, 1 insertion(+), 4 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
> @@ -71,12 +71,9 @@ static void bang_bang_control(struct the
>                 dev_dbg(&instance->cdev->device, "target=%ld\n",
> instance->target);
>  
>                 mutex_lock(&instance->cdev->lock);
> -               instance->cdev->updated = false; /* cdev needs update
> */
> +               __thermal_cdev_update(instance->cdev);
>                 mutex_unlock(&instance->cdev->lock);
>         }
> -
> -       list_for_each_entry(instance, &tz->thermal_instances,
> tz_node)
> -               thermal_cdev_update(instance->cdev);
>  }
>  
>  static struct thermal_governor thermal_gov_bang_bang = {
> 
> 
> 

[PATCH v1 2/4] thermal: gov_bang_bang: Split bang_bang_control()

[Rafael J. Wysocki]

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

Move the setting of the thermal instance target state from
bang_bang_control() into a separate function that will be also called
in a different place going forward.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c |   42 +++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 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,6 +13,27 @@
 
 #include "thermal_core.h"
 
+static void bang_bang_set_instance_target(struct thermal_instance *instance,
+					  unsigned int target)
+{
+	if (instance->target != 0 && instance->target != 1 &&
+	    instance->target != THERMAL_NO_TARGET)
+		pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
+			 instance->target, instance->name);
+
+	/*
+	 * Enable the fan when the trip is crossed on the way up and disable it
+	 * when the trip is crossed on the way down.
+	 */
+	instance->target = target;
+
+	dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
+
+	mutex_lock(&instance->cdev->lock);
+	__thermal_cdev_update(instance->cdev);
+	mutex_unlock(&instance->cdev->lock);
+}
+
 /**
  * bang_bang_control - controls devices associated with the given zone
  * @tz: thermal_zone_device
@@ -54,25 +75,8 @@ static void bang_bang_control(struct the
 		tz->temperature, trip->hysteresis);
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (instance->trip != trip)
-			continue;
-
-		if (instance->target != 0 && instance->target != 1 &&
-		    instance->target != THERMAL_NO_TARGET)
-			pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
-				 instance->target, instance->name);
-
-		/*
-		 * Enable the fan when the trip is crossed on the way up and
-		 * disable it when the trip is crossed on the way down.
-		 */
-		instance->target = crossed_up;
-
-		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
-
-		mutex_lock(&instance->cdev->lock);
-		__thermal_cdev_update(instance->cdev);
-		mutex_unlock(&instance->cdev->lock);
+		if (instance->trip == trip)
+			bang_bang_set_instance_target(instance, crossed_up);
 	}
 }
 

Re: [PATCH v1 2/4] thermal: gov_bang_bang: Split bang_bang_control()

[Peter Kästle]

On 13.08.24 16:26, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Move the setting of the thermal instance target state from
> bang_bang_control() into a separate function that will be also called
> in a different place going forward.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Peter Kästle <peter@piie.net>

> ---
>   drivers/thermal/gov_bang_bang.c |   42 +++++++++++++++++++++-------------------
>   1 file changed, 23 insertions(+), 19 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,6 +13,27 @@
>   
>   #include "thermal_core.h"
>   
> +static void bang_bang_set_instance_target(struct thermal_instance *instance,
> +					  unsigned int target)
> +{
> +	if (instance->target != 0 && instance->target != 1 &&
> +	    instance->target != THERMAL_NO_TARGET)
> +		pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
> +			 instance->target, instance->name);
> +
> +	/*
> +	 * Enable the fan when the trip is crossed on the way up and disable it
> +	 * when the trip is crossed on the way down.
> +	 */
> +	instance->target = target;
> +
> +	dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
> +
> +	mutex_lock(&instance->cdev->lock);
> +	__thermal_cdev_update(instance->cdev);
> +	mutex_unlock(&instance->cdev->lock);
> +}
> +
>   /**
>    * bang_bang_control - controls devices associated with the given zone
>    * @tz: thermal_zone_device
> @@ -54,25 +75,8 @@ static void bang_bang_control(struct the
>   		tz->temperature, trip->hysteresis);
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> -		if (instance->trip != trip)
> -			continue;
> -
> -		if (instance->target != 0 && instance->target != 1 &&
> -		    instance->target != THERMAL_NO_TARGET)
> -			pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
> -				 instance->target, instance->name);
> -
> -		/*
> -		 * Enable the fan when the trip is crossed on the way up and
> -		 * disable it when the trip is crossed on the way down.
> -		 */
> -		instance->target = crossed_up;
> -
> -		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
> -
> -		mutex_lock(&instance->cdev->lock);
> -		__thermal_cdev_update(instance->cdev);
> -		mutex_unlock(&instance->cdev->lock);
> +		if (instance->trip == trip)
> +			bang_bang_set_instance_target(instance, crossed_up);
>   	}
>   }
>   
> 
> 
> 

Re: [PATCH v1 2/4] thermal: gov_bang_bang: Split bang_bang_control()

[Zhang, Rui]

On Tue, 2024-08-13 at 16:26 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Move the setting of the thermal instance target state from
> bang_bang_control() into a separate function that will be also called
> in a different place going forward.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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

thanks,
rui

> ---
>  drivers/thermal/gov_bang_bang.c |   42 +++++++++++++++++++++--------
> -----------
>  1 file changed, 23 insertions(+), 19 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,6 +13,27 @@
>  
>  #include "thermal_core.h"
>  
> +static void bang_bang_set_instance_target(struct thermal_instance
> *instance,
> +                                         unsigned int target)
> +{
> +       if (instance->target != 0 && instance->target != 1 &&
> +           instance->target != THERMAL_NO_TARGET)
> +               pr_debug("Unexpected state %ld of thermal instance %s
> in bang-bang\n",
> +                        instance->target, instance->name);
> +
> +       /*
> +        * Enable the fan when the trip is crossed on the way up and
> disable it
> +        * when the trip is crossed on the way down.
> +        */
> +       instance->target = target;
> +
> +       dev_dbg(&instance->cdev->device, "target=%ld\n", instance-
> >target);
> +
> +       mutex_lock(&instance->cdev->lock);
> +       __thermal_cdev_update(instance->cdev);
> +       mutex_unlock(&instance->cdev->lock);
> +}
> +
>  /**
>   * bang_bang_control - controls devices associated with the given
> zone
>   * @tz: thermal_zone_device
> @@ -54,25 +75,8 @@ static void bang_bang_control(struct the
>                 tz->temperature, trip->hysteresis);
>  
>         list_for_each_entry(instance, &tz->thermal_instances,
> tz_node) {
> -               if (instance->trip != trip)
> -                       continue;
> -
> -               if (instance->target != 0 && instance->target != 1 &&
> -                   instance->target != THERMAL_NO_TARGET)
> -                       pr_debug("Unexpected state %ld of thermal
> instance %s in bang-bang\n",
> -                                instance->target, instance->name);
> -
> -               /*
> -                * Enable the fan when the trip is crossed on the way
> up and
> -                * disable it when the trip is crossed on the way
> down.
> -                */
> -               instance->target = crossed_up;
> -
> -               dev_dbg(&instance->cdev->device, "target=%ld\n",
> instance->target);
> -
> -               mutex_lock(&instance->cdev->lock);
> -               __thermal_cdev_update(instance->cdev);
> -               mutex_unlock(&instance->cdev->lock);
> +               if (instance->trip == trip)
> +                       bang_bang_set_instance_target(instance,
> crossed_up);
>         }
>  }
>  
> 
> 
> 

[PATCH v1 3/4] thermal: gov_bang_bang: Add .manage() callback

[Rafael J. Wysocki]

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

After recent changes, the Bang-bang governor may not adjust the
initial configuration of cooling devices to the actual situation.

Namely, if a cooling device bound to a certain trip point starts in
the "on" state and the thermal zone temperature is below the threshold
of that trip point, the trip point may never be crossed on the way up
in which case the state of the cooling device will never be adjusted
because the thermal core will never invoke the governor's
.trip_crossed() callback.  [Note that there is no issue if the zone
temperature is at the trip threshold or above it to start with because
.trip_crossed() will be invoked then to indicate the start of thermal
mitigation for the given trip.]

To address this, add a .manage() callback to the Bang-bang governor
and use it to ensure that all of the thermal instances managed by the
governor have been initialized properly and the states of all of the
cooling devices involved have been adjusted to the current zone
temperature as appropriate.

Fixes: 530c932bdf75 ("thermal: gov_bang_bang: Use .trip_crossed() instead of .throttle()")
Link: https://lore.kernel.org/linux-pm/1bfbbae5-42b0-4c7d-9544-e98855715294@piie.net/
Cc: 6.10+ <stable@vger.kernel.org> # 6.10+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

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
@@ -26,6 +26,7 @@ static void bang_bang_set_instance_targe
 	 * when the trip is crossed on the way down.
 	 */
 	instance->target = target;
+	instance->initialized = true;
 
 	dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
 
@@ -80,8 +81,37 @@ static void bang_bang_control(struct the
 	}
 }
 
+static void bang_bang_manage(struct thermal_zone_device *tz)
+{
+	const struct thermal_trip_desc *td;
+	struct thermal_instance *instance;
+
+	for_each_trip_desc(tz, td) {
+		const struct thermal_trip *trip = &td->trip;
+
+		if (tz->temperature >= td->threshold ||
+		    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.
+		 */
+		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+			if (!instance->initialized && instance->trip == trip)
+				bang_bang_set_instance_target(instance, 0);
+		}
+	}
+}
+
 static struct thermal_governor thermal_gov_bang_bang = {
 	.name		= "bang_bang",
 	.trip_crossed	= bang_bang_control,
+	.manage		= bang_bang_manage,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);

Re: [PATCH v1 3/4] thermal: gov_bang_bang: Add .manage() callback

[Peter Kästle]

On 13.08.24 16:27, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After recent changes, the Bang-bang governor may not adjust the
> initial configuration of cooling devices to the actual situation.
> 
> Namely, if a cooling device bound to a certain trip point starts in
> the "on" state and the thermal zone temperature is below the threshold
> of that trip point, the trip point may never be crossed on the way up
> in which case the state of the cooling device will never be adjusted
> because the thermal core will never invoke the governor's
> .trip_crossed() callback.  [Note that there is no issue if the zone
> temperature is at the trip threshold or above it to start with because
> .trip_crossed() will be invoked then to indicate the start of thermal
> mitigation for the given trip.]
> 
> To address this, add a .manage() callback to the Bang-bang governor
> and use it to ensure that all of the thermal instances managed by the
> governor have been initialized properly and the states of all of the
> cooling devices involved have been adjusted to the current zone
> temperature as appropriate.
> 
> Fixes: 530c932bdf75 ("thermal: gov_bang_bang: Use .trip_crossed() instead of .throttle()")
> Link: https://lore.kernel.org/linux-pm/1bfbbae5-42b0-4c7d-9544-e98855715294@piie.net/
> Cc: 6.10+ <stable@vger.kernel.org> # 6.10+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Peter Kästle <peter@piie.net>

> ---
>   drivers/thermal/gov_bang_bang.c |   30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> 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
> @@ -26,6 +26,7 @@ static void bang_bang_set_instance_targe
>   	 * when the trip is crossed on the way down.
>   	 */
>   	instance->target = target;
> +	instance->initialized = true;
>   
>   	dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
>   
> @@ -80,8 +81,37 @@ static void bang_bang_control(struct the
>   	}
>   }
>   
> +static void bang_bang_manage(struct thermal_zone_device *tz)
> +{
> +	const struct thermal_trip_desc *td;
> +	struct thermal_instance *instance;
> +
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
> +
> +		if (tz->temperature >= td->threshold ||
> +		    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.
> +		 */
> +		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +			if (!instance->initialized && instance->trip == trip)
> +				bang_bang_set_instance_target(instance, 0);
> +		}
> +	}
> +}
> +
>   static struct thermal_governor thermal_gov_bang_bang = {
>   	.name		= "bang_bang",
>   	.trip_crossed	= bang_bang_control,
> +	.manage		= bang_bang_manage,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> 
> 
> 

Re: [PATCH v1 3/4] thermal: gov_bang_bang: Add .manage() callback

[Peter Kästle]

On 13.08.24 16:27, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After recent changes, the Bang-bang governor may not adjust the
> initial configuration of cooling devices to the actual situation.
> 
> Namely, if a cooling device bound to a certain trip point starts in
> the "on" state and the thermal zone temperature is below the threshold
> of that trip point, the trip point may never be crossed on the way up
> in which case the state of the cooling device will never be adjusted
> because the thermal core will never invoke the governor's
> .trip_crossed() callback.  [Note that there is no issue if the zone
> temperature is at the trip threshold or above it to start with because
> .trip_crossed() will be invoked then to indicate the start of thermal
> mitigation for the given trip.]
> 
> To address this, add a .manage() callback to the Bang-bang governor
> and use it to ensure that all of the thermal instances managed by the
> governor have been initialized properly and the states of all of the
> cooling devices involved have been adjusted to the current zone
> temperature as appropriate.
> 
> Fixes: 530c932bdf75 ("thermal: gov_bang_bang: Use .trip_crossed() instead of .throttle()")
> Link: https://lore.kernel.org/linux-pm/1bfbbae5-42b0-4c7d-9544-e98855715294@piie.net/
> Cc: 6.10+ <stable@vger.kernel.org> # 6.10+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

oops, previously sent from wrong email address, here again from correct one...
Acked-by: Peter Kästle <peter@piie.net>

> ---
>   drivers/thermal/gov_bang_bang.c |   30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> 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
> @@ -26,6 +26,7 @@ static void bang_bang_set_instance_targe
>   	 * when the trip is crossed on the way down.
>   	 */
>   	instance->target = target;
> +	instance->initialized = true;
>   
>   	dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
>   
> @@ -80,8 +81,37 @@ static void bang_bang_control(struct the
>   	}
>   }
>   
> +static void bang_bang_manage(struct thermal_zone_device *tz)
> +{
> +	const struct thermal_trip_desc *td;
> +	struct thermal_instance *instance;
> +
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
> +
> +		if (tz->temperature >= td->threshold ||
> +		    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.
> +		 */
> +		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +			if (!instance->initialized && instance->trip == trip)
> +				bang_bang_set_instance_target(instance, 0);
> +		}
> +	}
> +}
> +
>   static struct thermal_governor thermal_gov_bang_bang = {
>   	.name		= "bang_bang",
>   	.trip_crossed	= bang_bang_control,
> +	.manage		= bang_bang_manage,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> 
> 
> 

Re: [PATCH v1 3/4] thermal: gov_bang_bang: Add .manage() callback

[Rafael J. Wysocki]

On Wed, Aug 14, 2024 at 12:50 AM Peter Kästle <peter@piie.net> wrote:
>
> On 13.08.24 16:27, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > After recent changes, the Bang-bang governor may not adjust the
> > initial configuration of cooling devices to the actual situation.
> >
> > Namely, if a cooling device bound to a certain trip point starts in
> > the "on" state and the thermal zone temperature is below the threshold
> > of that trip point, the trip point may never be crossed on the way up
> > in which case the state of the cooling device will never be adjusted
> > because the thermal core will never invoke the governor's
> > .trip_crossed() callback.  [Note that there is no issue if the zone
> > temperature is at the trip threshold or above it to start with because
> > .trip_crossed() will be invoked then to indicate the start of thermal
> > mitigation for the given trip.]
> >
> > To address this, add a .manage() callback to the Bang-bang governor
> > and use it to ensure that all of the thermal instances managed by the
> > governor have been initialized properly and the states of all of the
> > cooling devices involved have been adjusted to the current zone
> > temperature as appropriate.
> >
> > Fixes: 530c932bdf75 ("thermal: gov_bang_bang: Use .trip_crossed() instead of .throttle()")
> > Link: https://lore.kernel.org/linux-pm/1bfbbae5-42b0-4c7d-9544-e98855715294@piie.net/
> > Cc: 6.10+ <stable@vger.kernel.org> # 6.10+
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> oops, previously sent from wrong email address, here again from correct one...
> Acked-by: Peter Kästle <peter@piie.net>

No worries and thanks for the ACKs!

I gather that they mean that the changes work for you.

> > ---
> >   drivers/thermal/gov_bang_bang.c |   30 ++++++++++++++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> >
> > 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
> > @@ -26,6 +26,7 @@ static void bang_bang_set_instance_targe
> >        * when the trip is crossed on the way down.
> >        */
> >       instance->target = target;
> > +     instance->initialized = true;
> >
> >       dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
> >
> > @@ -80,8 +81,37 @@ static void bang_bang_control(struct the
> >       }
> >   }
> >
> > +static void bang_bang_manage(struct thermal_zone_device *tz)
> > +{
> > +     const struct thermal_trip_desc *td;
> > +     struct thermal_instance *instance;
> > +
> > +     for_each_trip_desc(tz, td) {
> > +             const struct thermal_trip *trip = &td->trip;
> > +
> > +             if (tz->temperature >= td->threshold ||
> > +                 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.
> > +              */
> > +             list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > +                     if (!instance->initialized && instance->trip == trip)
> > +                             bang_bang_set_instance_target(instance, 0);
> > +             }
> > +     }
> > +}
> > +
> >   static struct thermal_governor thermal_gov_bang_bang = {
> >       .name           = "bang_bang",
> >       .trip_crossed   = bang_bang_control,
> > +     .manage         = bang_bang_manage,
> >   };
> >   THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> >
> >
> >
>

Re: [PATCH v1 3/4] thermal: gov_bang_bang: Add .manage() callback

[Peter Kästle]

Hi Rafael,

On 14.08.24 19:18, Rafael J. Wysocki wrote:
> On Wed, Aug 14, 2024 at 12:50 AM Peter Kästle <peter@piie.net> wrote:
>>
>> On 13.08.24 16:27, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> After recent changes, the Bang-bang governor may not adjust the
>>> initial configuration of cooling devices to the actual situation.
>>>
>>> Namely, if a cooling device bound to a certain trip point starts in
>>> the "on" state and the thermal zone temperature is below the threshold
>>> of that trip point, the trip point may never be crossed on the way up
>>> in which case the state of the cooling device will never be adjusted
>>> because the thermal core will never invoke the governor's
>>> .trip_crossed() callback.  [Note that there is no issue if the zone
>>> temperature is at the trip threshold or above it to start with because
>>> .trip_crossed() will be invoked then to indicate the start of thermal
>>> mitigation for the given trip.]
>>>
>>> To address this, add a .manage() callback to the Bang-bang governor
>>> and use it to ensure that all of the thermal instances managed by the
>>> governor have been initialized properly and the states of all of the
>>> cooling devices involved have been adjusted to the current zone
>>> temperature as appropriate.
>>>
>>> Fixes: 530c932bdf75 ("thermal: gov_bang_bang: Use .trip_crossed() instead of .throttle()")
>>> Link: https://lore.kernel.org/linux-pm/1bfbbae5-42b0-4c7d-9544-e98855715294@piie.net/
>>> Cc: 6.10+ <stable@vger.kernel.org> # 6.10+
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> oops, previously sent from wrong email address, here again from correct one...
>> Acked-by: Peter Kästle <peter@piie.net>
> 
> No worries and thanks for the ACKs!
> 
> I gather that they mean that the changes work for you.

Yes, successfully tested all my use cases and reviewed the code.  Thanks.
Still working on the acerhdf cleanup, but this will be another patch series.

> 
>>> ---
>>>    drivers/thermal/gov_bang_bang.c |   30 ++++++++++++++++++++++++++++++
>>>    1 file changed, 30 insertions(+)
>>>
>>> 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
>>> @@ -26,6 +26,7 @@ static void bang_bang_set_instance_targe
>>>         * when the trip is crossed on the way down.
>>>         */
>>>        instance->target = target;
>>> +     instance->initialized = true;
>>>
>>>        dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
>>>
>>> @@ -80,8 +81,37 @@ static void bang_bang_control(struct the
>>>        }
>>>    }
>>>
>>> +static void bang_bang_manage(struct thermal_zone_device *tz)
>>> +{
>>> +     const struct thermal_trip_desc *td;
>>> +     struct thermal_instance *instance;
>>> +
>>> +     for_each_trip_desc(tz, td) {
>>> +             const struct thermal_trip *trip = &td->trip;
>>> +
>>> +             if (tz->temperature >= td->threshold ||
>>> +                 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.
>>> +              */
>>> +             list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>>> +                     if (!instance->initialized && instance->trip == trip)
>>> +                             bang_bang_set_instance_target(instance, 0);
>>> +             }
>>> +     }
>>> +}
>>> +
>>>    static struct thermal_governor thermal_gov_bang_bang = {
>>>        .name           = "bang_bang",
>>>        .trip_crossed   = bang_bang_control,
>>> +     .manage         = bang_bang_manage,
>>>    };
>>>    THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
>>>
>>>
>>>
>>

Re: [PATCH v1 3/4] thermal: gov_bang_bang: Add .manage() callback

[Zhang, Rui]

On Tue, 2024-08-13 at 16:27 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After recent changes, the Bang-bang governor may not adjust the
> initial configuration of cooling devices to the actual situation.
> 
> Namely, if a cooling device bound to a certain trip point starts in
> the "on" state and the thermal zone temperature is below the
> threshold
> of that trip point, the trip point may never be crossed on the way up
> in which case the state of the cooling device will never be adjusted
> because the thermal core will never invoke the governor's
> .trip_crossed() callback.  [Note that there is no issue if the zone
> temperature is at the trip threshold or above it to start with
> because
> .trip_crossed() will be invoked then to indicate the start of thermal
> mitigation for the given trip.]
> 
> To address this, add a .manage() callback to the Bang-bang governor
> and use it to ensure that all of the thermal instances managed by the
> governor have been initialized properly and the states of all of the
> cooling devices involved have been adjusted to the current zone
> temperature as appropriate.
> 
> Fixes: 530c932bdf75 ("thermal: gov_bang_bang: Use .trip_crossed()
> instead of .throttle()")
> Link:
> https://lore.kernel.org/linux-pm/1bfbbae5-42b0-4c7d-9544-e98855715294@piie.net/
> Cc: 6.10+ <stable@vger.kernel.org> # 6.10+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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

thanks,
rui

> ---
>  drivers/thermal/gov_bang_bang.c |   30
> ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> 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
> @@ -26,6 +26,7 @@ static void bang_bang_set_instance_targe
>          * when the trip is crossed on the way down.
>          */
>         instance->target = target;
> +       instance->initialized = true;
>  
>         dev_dbg(&instance->cdev->device, "target=%ld\n", instance-
> >target);
>  
> @@ -80,8 +81,37 @@ static void bang_bang_control(struct the
>         }
>  }
>  
> +static void bang_bang_manage(struct thermal_zone_device *tz)
> +{
> +       const struct thermal_trip_desc *td;
> +       struct thermal_instance *instance;
> +
> +       for_each_trip_desc(tz, td) {
> +               const struct thermal_trip *trip = &td->trip;
> +
> +               if (tz->temperature >= td->threshold ||
> +                   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.
> +                */
> +               list_for_each_entry(instance, &tz->thermal_instances,
> tz_node) {
> +                       if (!instance->initialized && instance->trip
> == trip)
> +                               bang_bang_set_instance_target(instanc
> e, 0);
> +               }
> +       }
> +}
> +
>  static struct thermal_governor thermal_gov_bang_bang = {
>         .name           = "bang_bang",
>         .trip_crossed   = bang_bang_control,
> +       .manage         = bang_bang_manage,
>  };
>  THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> 
> 
> 

[PATCH v1 4/4] thermal: gov_bang_bang: Use governor_data to reduce overhead

[Rafael J. Wysocki]

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

After running once, the for_each_trip_desc() loop in
bang_bang_manage() is pure needless overhead because it is not going to
make any changes unless a new cooling device has been bound to one of
the trips in the thermal zone or the system is resuming from sleep.

For this reason, make bang_bang_manage() set governor_data for the
thermal zone and check it upfront to decide whether or not it needs to
do anything.

However, governor_data needs to be reset in some cases to let
bang_bang_manage() know that it should walk the trips again, so add an
.update_tz() callback to the governor and make the core additionally
invoke it during system resume.

To avoid affecting the other users of that callback unnecessarily, add
a special notification reason for system resume, THERMAL_TZ_RESUME, and
also pass it to __thermal_zone_device_update() called during system
resume for consistency.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c |   18 ++++++++++++++++++
 drivers/thermal/thermal_core.c  |    3 ++-
 include/linux/thermal.h         |    1 +
 3 files changed, 21 insertions(+), 1 deletion(-)

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
@@ -86,6 +86,10 @@ static void bang_bang_manage(struct ther
 	const struct thermal_trip_desc *td;
 	struct thermal_instance *instance;
 
+	/* If the code below has run already, nothing needs to be done. */
+	if (tz->governor_data)
+		return;
+
 	for_each_trip_desc(tz, td) {
 		const struct thermal_trip *trip = &td->trip;
 
@@ -107,11 +111,25 @@ static void bang_bang_manage(struct ther
 				bang_bang_set_instance_target(instance, 0);
 		}
 	}
+
+	tz->governor_data = (void *)true;
+}
+
+static void bang_bang_update_tz(struct thermal_zone_device *tz,
+				enum thermal_notify_event reason)
+{
+	/*
+	 * Let bang_bang_manage() know that it needs to walk trips after binding
+	 * a new cdev and after system resume.
+	 */
+	if (reason == THERMAL_TZ_BIND_CDEV || reason == THERMAL_TZ_RESUME)
+		tz->governor_data = NULL;
 }
 
 static struct thermal_governor thermal_gov_bang_bang = {
 	.name		= "bang_bang",
 	.trip_crossed	= bang_bang_control,
 	.manage		= bang_bang_manage,
+	.update_tz	= bang_bang_update_tz,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1692,7 +1692,8 @@ static void thermal_zone_device_resume(s
 
 	thermal_debug_tz_resume(tz);
 	thermal_zone_device_init(tz);
-	__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
+	thermal_governor_update_tz(tz, THERMAL_TZ_RESUME);
+	__thermal_zone_device_update(tz, THERMAL_TZ_RESUME);
 
 	complete(&tz->resume);
 	tz->resuming = false;
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -55,6 +55,7 @@ enum thermal_notify_event {
 	THERMAL_TZ_BIND_CDEV, /* Cooling dev is bind to the thermal zone */
 	THERMAL_TZ_UNBIND_CDEV, /* Cooling dev is unbind from the thermal zone */
 	THERMAL_INSTANCE_WEIGHT_CHANGED, /* Thermal instance weight changed */
+	THERMAL_TZ_RESUME, /* Thermal zone is resuming after system sleep */
 };
 
 /**

Re: [PATCH v1 4/4] thermal: gov_bang_bang: Use governor_data to reduce overhead

[Peter Kästle]

On 13.08.24 16:29, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After running once, the for_each_trip_desc() loop in
> bang_bang_manage() is pure needless overhead because it is not going to
> make any changes unless a new cooling device has been bound to one of
> the trips in the thermal zone or the system is resuming from sleep.
> 
> For this reason, make bang_bang_manage() set governor_data for the
> thermal zone and check it upfront to decide whether or not it needs to
> do anything.
> 
> However, governor_data needs to be reset in some cases to let
> bang_bang_manage() know that it should walk the trips again, so add an
> .update_tz() callback to the governor and make the core additionally
> invoke it during system resume.
> 
> To avoid affecting the other users of that callback unnecessarily, add
> a special notification reason for system resume, THERMAL_TZ_RESUME, and
> also pass it to __thermal_zone_device_update() called during system
> resume for consistency.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Peter Kästle <peter@piie.net>

> ---
>   drivers/thermal/gov_bang_bang.c |   18 ++++++++++++++++++
>   drivers/thermal/thermal_core.c  |    3 ++-
>   include/linux/thermal.h         |    1 +
>   3 files changed, 21 insertions(+), 1 deletion(-)
> 
> 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
> @@ -86,6 +86,10 @@ static void bang_bang_manage(struct ther
>   	const struct thermal_trip_desc *td;
>   	struct thermal_instance *instance;
>   
> +	/* If the code below has run already, nothing needs to be done. */
> +	if (tz->governor_data)
> +		return;
> +
>   	for_each_trip_desc(tz, td) {
>   		const struct thermal_trip *trip = &td->trip;
>   
> @@ -107,11 +111,25 @@ static void bang_bang_manage(struct ther
>   				bang_bang_set_instance_target(instance, 0);
>   		}
>   	}
> +
> +	tz->governor_data = (void *)true;
> +}
> +
> +static void bang_bang_update_tz(struct thermal_zone_device *tz,
> +				enum thermal_notify_event reason)
> +{
> +	/*
> +	 * Let bang_bang_manage() know that it needs to walk trips after binding
> +	 * a new cdev and after system resume.
> +	 */
> +	if (reason == THERMAL_TZ_BIND_CDEV || reason == THERMAL_TZ_RESUME)
> +		tz->governor_data = NULL;
>   }
>   
>   static struct thermal_governor thermal_gov_bang_bang = {
>   	.name		= "bang_bang",
>   	.trip_crossed	= bang_bang_control,
>   	.manage		= bang_bang_manage,
> +	.update_tz	= bang_bang_update_tz,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1692,7 +1692,8 @@ static void thermal_zone_device_resume(s
>   
>   	thermal_debug_tz_resume(tz);
>   	thermal_zone_device_init(tz);
> -	__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> +	thermal_governor_update_tz(tz, THERMAL_TZ_RESUME);
> +	__thermal_zone_device_update(tz, THERMAL_TZ_RESUME);
>   
>   	complete(&tz->resume);
>   	tz->resuming = false;
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -55,6 +55,7 @@ enum thermal_notify_event {
>   	THERMAL_TZ_BIND_CDEV, /* Cooling dev is bind to the thermal zone */
>   	THERMAL_TZ_UNBIND_CDEV, /* Cooling dev is unbind from the thermal zone */
>   	THERMAL_INSTANCE_WEIGHT_CHANGED, /* Thermal instance weight changed */
> +	THERMAL_TZ_RESUME, /* Thermal zone is resuming after system sleep */
>   };
>   
>   /**
> 
> 
> 

Re: [PATCH v1 4/4] thermal: gov_bang_bang: Use governor_data to reduce overhead

[Zhang, Rui]

On Tue, 2024-08-13 at 16:29 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After running once, the for_each_trip_desc() loop in
> bang_bang_manage() is pure needless overhead because it is not going
> to
> make any changes unless a new cooling device has been bound to one of
> the trips in the thermal zone or the system is resuming from sleep.
> 
> For this reason, make bang_bang_manage() set governor_data for the
> thermal zone and check it upfront to decide whether or not it needs
> to
> do anything.
> 
> However, governor_data needs to be reset in some cases to let
> bang_bang_manage() know that it should walk the trips again, so add
> an
> .update_tz() callback to the governor and make the core additionally
> invoke it during system resume.
> 
> To avoid affecting the other users of that callback unnecessarily,
> add
> a special notification reason for system resume, THERMAL_TZ_RESUME,
> and
> also pass it to __thermal_zone_device_update() called during system
> resume for consistency.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/thermal/gov_bang_bang.c |   18 ++++++++++++++++++
>  drivers/thermal/thermal_core.c  |    3 ++-
>  include/linux/thermal.h         |    1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> 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
> @@ -86,6 +86,10 @@ static void bang_bang_manage(struct ther
>         const struct thermal_trip_desc *td;
>         struct thermal_instance *instance;
>  
> +       /* If the code below has run already, nothing needs to be
> done. */
> +       if (tz->governor_data)
> +               return;
> +
>         for_each_trip_desc(tz, td) {
>                 const struct thermal_trip *trip = &td->trip;
>  
> @@ -107,11 +111,25 @@ static void bang_bang_manage(struct ther
>                                 bang_bang_set_instance_target(instanc
> e, 0);
>                 }
>         }
> +
> +       tz->governor_data = (void *)true;
> +}
> +
> +static void bang_bang_update_tz(struct thermal_zone_device *tz,
> +                               enum thermal_notify_event reason)
> +{
> +       /*
> +        * Let bang_bang_manage() know that it needs to walk trips
> after binding
> +        * a new cdev and after system resume.
> +        */
> +       if (reason == THERMAL_TZ_BIND_CDEV || reason ==
> THERMAL_TZ_RESUME)
> +               tz->governor_data = NULL;
>  }

can we do the cdev initialization for BIND_CDEV and RESUME notification
in .update_tz() directly?

Then we don't need .manage() callback. This makes more sense to me
because bang_bang governor cares about trip point crossing only.

thanks,
rui
>  
>  static struct thermal_governor thermal_gov_bang_bang = {
>         .name           = "bang_bang",
>         .trip_crossed   = bang_bang_control,
>         .manage         = bang_bang_manage,
> +       .update_tz      = bang_bang_update_tz,
>  };
>  THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1692,7 +1692,8 @@ static void thermal_zone_device_resume(s
>  
>         thermal_debug_tz_resume(tz);
>         thermal_zone_device_init(tz);
> -       __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> +       thermal_governor_update_tz(tz, THERMAL_TZ_RESUME);
> +       __thermal_zone_device_update(tz, THERMAL_TZ_RESUME);
>  
>         complete(&tz->resume);
>         tz->resuming = false;
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -55,6 +55,7 @@ enum thermal_notify_event {
>         THERMAL_TZ_BIND_CDEV, /* Cooling dev is bind to the thermal
> zone */
>         THERMAL_TZ_UNBIND_CDEV, /* Cooling dev is unbind from the
> thermal zone */
>         THERMAL_INSTANCE_WEIGHT_CHANGED, /* Thermal instance weight
> changed */
> +       THERMAL_TZ_RESUME, /* Thermal zone is resuming after system
> sleep */
>  };
>  
>  /**
> 
> 
> 

Re: [PATCH v1 4/4] thermal: gov_bang_bang: Use governor_data to reduce overhead

[Rafael J. Wysocki]

On Wed, Aug 14, 2024 at 8:09 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Tue, 2024-08-13 at 16:29 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > After running once, the for_each_trip_desc() loop in
> > bang_bang_manage() is pure needless overhead because it is not going
> > to
> > make any changes unless a new cooling device has been bound to one of
> > the trips in the thermal zone or the system is resuming from sleep.
> >
> > For this reason, make bang_bang_manage() set governor_data for the
> > thermal zone and check it upfront to decide whether or not it needs
> > to
> > do anything.
> >
> > However, governor_data needs to be reset in some cases to let
> > bang_bang_manage() know that it should walk the trips again, so add
> > an
> > .update_tz() callback to the governor and make the core additionally
> > invoke it during system resume.
> >
> > To avoid affecting the other users of that callback unnecessarily,
> > add
> > a special notification reason for system resume, THERMAL_TZ_RESUME,
> > and
> > also pass it to __thermal_zone_device_update() called during system
> > resume for consistency.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/thermal/gov_bang_bang.c |   18 ++++++++++++++++++
> >  drivers/thermal/thermal_core.c  |    3 ++-
> >  include/linux/thermal.h         |    1 +
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> >
> > 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
> > @@ -86,6 +86,10 @@ static void bang_bang_manage(struct ther
> >         const struct thermal_trip_desc *td;
> >         struct thermal_instance *instance;
> >
> > +       /* If the code below has run already, nothing needs to be
> > done. */
> > +       if (tz->governor_data)
> > +               return;
> > +
> >         for_each_trip_desc(tz, td) {
> >                 const struct thermal_trip *trip = &td->trip;
> >
> > @@ -107,11 +111,25 @@ static void bang_bang_manage(struct ther
> >                                 bang_bang_set_instance_target(instanc
> > e, 0);
> >                 }
> >         }
> > +
> > +       tz->governor_data = (void *)true;
> > +}
> > +
> > +static void bang_bang_update_tz(struct thermal_zone_device *tz,
> > +                               enum thermal_notify_event reason)
> > +{
> > +       /*
> > +        * Let bang_bang_manage() know that it needs to walk trips
> > after binding
> > +        * a new cdev and after system resume.
> > +        */
> > +       if (reason == THERMAL_TZ_BIND_CDEV || reason ==
> > THERMAL_TZ_RESUME)
> > +               tz->governor_data = NULL;
> >  }
>
> can we do the cdev initialization for BIND_CDEV and RESUME notification
> in .update_tz() directly?

That would be viable if the zone temperature was known at the time
.update_tz() runs, but it isn't.  See this message:

https://lore.kernel.org/linux-pm/CAJZ5v0ji_7Z-24iCO_Xxu4Zm4jgVFmR9jVp8QNiCOxzV9gqSnA@mail.gmail.com/

As long as the zone temperature is not known, it is not clear which
way to initialize the cooling devices.

Interestingly enough, the zone temperature is first checked by the
core when the zone is enabled and not when it is registered.

> Then we don't need .manage() callback. This makes more sense to me
> because bang_bang governor cares about trip point crossing only.

That's true, but this is all about a corner case in which no trip
points are crossed and the cooling devices need to be initialized
properly regardless.

> >  static struct thermal_governor thermal_gov_bang_bang = {
> >         .name           = "bang_bang",
> >         .trip_crossed   = bang_bang_control,
> >         .manage         = bang_bang_manage,
> > +       .update_tz      = bang_bang_update_tz,
> >  };
> >  THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> > Index: linux-pm/drivers/thermal/thermal_core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.c
> > +++ linux-pm/drivers/thermal/thermal_core.c
> > @@ -1692,7 +1692,8 @@ static void thermal_zone_device_resume(s
> >
> >         thermal_debug_tz_resume(tz);
> >         thermal_zone_device_init(tz);
> > -       __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> > +       thermal_governor_update_tz(tz, THERMAL_TZ_RESUME);
> > +       __thermal_zone_device_update(tz, THERMAL_TZ_RESUME);
> >
> >         complete(&tz->resume);
> >         tz->resuming = false;
> > Index: linux-pm/include/linux/thermal.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/thermal.h
> > +++ linux-pm/include/linux/thermal.h
> > @@ -55,6 +55,7 @@ enum thermal_notify_event {
> >         THERMAL_TZ_BIND_CDEV, /* Cooling dev is bind to the thermal
> > zone */
> >         THERMAL_TZ_UNBIND_CDEV, /* Cooling dev is unbind from the
> > thermal zone */
> >         THERMAL_INSTANCE_WEIGHT_CHANGED, /* Thermal instance weight
> > changed */
> > +       THERMAL_TZ_RESUME, /* Thermal zone is resuming after system
> > sleep */
> >  };
> >
> >  /**
> >
> >
> >
>

Re: [PATCH v1 4/4] thermal: gov_bang_bang: Use governor_data to reduce overhead

[Zhang, Rui]

On Wed, 2024-08-14 at 19:26 +0200, Rafael J. Wysocki wrote:
> On Wed, Aug 14, 2024 at 8:09 AM Zhang, Rui <rui.zhang@intel.com>
> wrote:
> > 
> > On Tue, 2024-08-13 at 16:29 +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > After running once, the for_each_trip_desc() loop in
> > > bang_bang_manage() is pure needless overhead because it is not
> > > going
> > > to
> > > make any changes unless a new cooling device has been bound to
> > > one of
> > > the trips in the thermal zone or the system is resuming from
> > > sleep.
> > > 
> > > For this reason, make bang_bang_manage() set governor_data for
> > > the
> > > thermal zone and check it upfront to decide whether or not it
> > > needs
> > > to
> > > do anything.
> > > 
> > > However, governor_data needs to be reset in some cases to let
> > > bang_bang_manage() know that it should walk the trips again, so
> > > add
> > > an
> > > .update_tz() callback to the governor and make the core
> > > additionally
> > > invoke it during system resume.
> > > 
> > > To avoid affecting the other users of that callback
> > > unnecessarily,
> > > add
> > > a special notification reason for system resume,
> > > THERMAL_TZ_RESUME,
> > > and
> > > also pass it to __thermal_zone_device_update() called during
> > > system
> > > resume for consistency.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/thermal/gov_bang_bang.c |   18 ++++++++++++++++++
> > >  drivers/thermal/thermal_core.c  |    3 ++-
> > >  include/linux/thermal.h         |    1 +
> > >  3 files changed, 21 insertions(+), 1 deletion(-)
> > > 
> > > 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
> > > @@ -86,6 +86,10 @@ static void bang_bang_manage(struct ther
> > >         const struct thermal_trip_desc *td;
> > >         struct thermal_instance *instance;
> > > 
> > > +       /* If the code below has run already, nothing needs to be
> > > done. */
> > > +       if (tz->governor_data)
> > > +               return;
> > > +
> > >         for_each_trip_desc(tz, td) {
> > >                 const struct thermal_trip *trip = &td->trip;
> > > 
> > > @@ -107,11 +111,25 @@ static void bang_bang_manage(struct ther
> > >                                
> > > bang_bang_set_instance_target(instanc
> > > e, 0);
> > >                 }
> > >         }
> > > +
> > > +       tz->governor_data = (void *)true;
> > > +}
> > > +
> > > +static void bang_bang_update_tz(struct thermal_zone_device *tz,
> > > +                               enum thermal_notify_event reason)
> > > +{
> > > +       /*
> > > +        * Let bang_bang_manage() know that it needs to walk
> > > trips
> > > after binding
> > > +        * a new cdev and after system resume.
> > > +        */
> > > +       if (reason == THERMAL_TZ_BIND_CDEV || reason ==
> > > THERMAL_TZ_RESUME)
> > > +               tz->governor_data = NULL;
> > >  }
> > 
> > can we do the cdev initialization for BIND_CDEV and RESUME
> > notification
> > in .update_tz() directly?
> 
> That would be viable if the zone temperature was known at the time
> .update_tz() runs, but it isn't.  See this message:
> 
> https://lore.kernel.org/linux-pm/CAJZ5v0ji_7Z-24iCO_Xxu4Zm4jgVFmR9jVp8QNiCOxzV9gqSnA@mail.gmail.com/
> 
> As long as the zone temperature is not known, it is not clear which
> way to initialize the cooling devices.

right. Then the patch LGTM.

BTW, what do you think if we add handling for first temperature read in
handle_thermal_trip(), say, some draft code like below,

if (tz->last_temperature == THERMAL_TEMP_INIT) {
	if (tz->temperature < trip->temperature)
		list_add(&td->notify_list_node, way_down_list);
	else
		list_add_tail(&td->notify_list_node, way_up_list);
}

This should handle both the init and the resume case.

thanks,
rui
> 
> Interestingly enough, the zone temperature is first checked by the
> core when the zone is enabled and not when it is registered.
> 
> > Then we don't need .manage() callback. This makes more sense to me
> > because bang_bang governor cares about trip point crossing only.
> 
> That's true, but this is all about a corner case in which no trip
> points are crossed and the cooling devices need to be initialized
> properly regardless.
> 
> > >  static struct thermal_governor thermal_gov_bang_bang = {
> > >         .name           = "bang_bang",
> > >         .trip_crossed   = bang_bang_control,
> > >         .manage         = bang_bang_manage,
> > > +       .update_tz      = bang_bang_update_tz,
> > >  };
> > >  THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> > > Index: linux-pm/drivers/thermal/thermal_core.c
> > > =================================================================
> > > ==
> > > --- linux-pm.orig/drivers/thermal/thermal_core.c
> > > +++ linux-pm/drivers/thermal/thermal_core.c
> > > @@ -1692,7 +1692,8 @@ static void thermal_zone_device_resume(s
> > > 
> > >         thermal_debug_tz_resume(tz);
> > >         thermal_zone_device_init(tz);
> > > -       __thermal_zone_device_update(tz,
> > > THERMAL_EVENT_UNSPECIFIED);
> > > +       thermal_governor_update_tz(tz, THERMAL_TZ_RESUME);
> > > +       __thermal_zone_device_update(tz, THERMAL_TZ_RESUME);
> > > 
> > >         complete(&tz->resume);
> > >         tz->resuming = false;
> > > Index: linux-pm/include/linux/thermal.h
> > > =================================================================
> > > ==
> > > --- linux-pm.orig/include/linux/thermal.h
> > > +++ linux-pm/include/linux/thermal.h
> > > @@ -55,6 +55,7 @@ enum thermal_notify_event {
> > >         THERMAL_TZ_BIND_CDEV, /* Cooling dev is bind to the
> > > thermal
> > > zone */
> > >         THERMAL_TZ_UNBIND_CDEV, /* Cooling dev is unbind from the
> > > thermal zone */
> > >         THERMAL_INSTANCE_WEIGHT_CHANGED, /* Thermal instance
> > > weight
> > > changed */
> > > +       THERMAL_TZ_RESUME, /* Thermal zone is resuming after
> > > system
> > > sleep */
> > >  };
> > > 
> > >  /**
> > > 
> > > 
> > > 
> > 
> 

Re: [PATCH v1 4/4] thermal: gov_bang_bang: Use governor_data to reduce overhead

[Rafael J. Wysocki]

On Thu, Aug 15, 2024 at 5:26 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Wed, 2024-08-14 at 19:26 +0200, Rafael J. Wysocki wrote:
> > On Wed, Aug 14, 2024 at 8:09 AM Zhang, Rui <rui.zhang@intel.com>
> > wrote:
> > >
> > > On Tue, 2024-08-13 at 16:29 +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > After running once, the for_each_trip_desc() loop in
> > > > bang_bang_manage() is pure needless overhead because it is not
> > > > going
> > > > to
> > > > make any changes unless a new cooling device has been bound to
> > > > one of
> > > > the trips in the thermal zone or the system is resuming from
> > > > sleep.
> > > >
> > > > For this reason, make bang_bang_manage() set governor_data for
> > > > the
> > > > thermal zone and check it upfront to decide whether or not it
> > > > needs
> > > > to
> > > > do anything.
> > > >
> > > > However, governor_data needs to be reset in some cases to let
> > > > bang_bang_manage() know that it should walk the trips again, so
> > > > add
> > > > an
> > > > .update_tz() callback to the governor and make the core
> > > > additionally
> > > > invoke it during system resume.
> > > >
> > > > To avoid affecting the other users of that callback
> > > > unnecessarily,
> > > > add
> > > > a special notification reason for system resume,
> > > > THERMAL_TZ_RESUME,
> > > > and
> > > > also pass it to __thermal_zone_device_update() called during
> > > > system
> > > > resume for consistency.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  drivers/thermal/gov_bang_bang.c |   18 ++++++++++++++++++
> > > >  drivers/thermal/thermal_core.c  |    3 ++-
> > > >  include/linux/thermal.h         |    1 +
> > > >  3 files changed, 21 insertions(+), 1 deletion(-)
> > > >
> > > > 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
> > > > @@ -86,6 +86,10 @@ static void bang_bang_manage(struct ther
> > > >         const struct thermal_trip_desc *td;
> > > >         struct thermal_instance *instance;
> > > >
> > > > +       /* If the code below has run already, nothing needs to be
> > > > done. */
> > > > +       if (tz->governor_data)
> > > > +               return;
> > > > +
> > > >         for_each_trip_desc(tz, td) {
> > > >                 const struct thermal_trip *trip = &td->trip;
> > > >
> > > > @@ -107,11 +111,25 @@ static void bang_bang_manage(struct ther
> > > >
> > > > bang_bang_set_instance_target(instanc
> > > > e, 0);
> > > >                 }
> > > >         }
> > > > +
> > > > +       tz->governor_data = (void *)true;
> > > > +}
> > > > +
> > > > +static void bang_bang_update_tz(struct thermal_zone_device *tz,
> > > > +                               enum thermal_notify_event reason)
> > > > +{
> > > > +       /*
> > > > +        * Let bang_bang_manage() know that it needs to walk
> > > > trips
> > > > after binding
> > > > +        * a new cdev and after system resume.
> > > > +        */
> > > > +       if (reason == THERMAL_TZ_BIND_CDEV || reason ==
> > > > THERMAL_TZ_RESUME)
> > > > +               tz->governor_data = NULL;
> > > >  }
> > >
> > > can we do the cdev initialization for BIND_CDEV and RESUME
> > > notification
> > > in .update_tz() directly?
> >
> > That would be viable if the zone temperature was known at the time
> > .update_tz() runs, but it isn't.  See this message:
> >
> > https://lore.kernel.org/linux-pm/CAJZ5v0ji_7Z-24iCO_Xxu4Zm4jgVFmR9jVp8QNiCOxzV9gqSnA@mail.gmail.com/
> >
> > As long as the zone temperature is not known, it is not clear which
> > way to initialize the cooling devices.
>
> right. Then the patch LGTM.

Great!

> BTW, what do you think if we add handling for first temperature read in
> handle_thermal_trip(), say, some draft code like below,
>
> if (tz->last_temperature == THERMAL_TEMP_INIT) {
>         if (tz->temperature < trip->temperature)
>                 list_add(&td->notify_list_node, way_down_list);
>         else
>                 list_add_tail(&td->notify_list_node, way_up_list);
> }
>
> This should handle both the init and the resume case.

I have considered doing something similar, but there are quite a few
arguments against it.

First, it would cause spurious notifications to be sent to user space.
Second, the debug code would need to be modified to take this case
into account explicitly.  Moreover, this would be extra overhead for
the other governors.

IMV it's better to limit the impact to the Bang-bang governor where
the problem is.

Re: [PATCH v1 4/4] thermal: gov_bang_bang: Use governor_data to reduce overhead

[Zhang, Rui]

On Thu, 2024-08-15 at 14:35 +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 15, 2024 at 5:26 AM Zhang, Rui <rui.zhang@intel.com>
> wrote:
> > 
> > On Wed, 2024-08-14 at 19:26 +0200, Rafael J. Wysocki wrote:
> > > On Wed, Aug 14, 2024 at 8:09 AM Zhang, Rui <rui.zhang@intel.com>
> > > wrote:
> > > > 
> > > > On Tue, 2024-08-13 at 16:29 +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > After running once, the for_each_trip_desc() loop in
> > > > > bang_bang_manage() is pure needless overhead because it is
> > > > > not
> > > > > going
> > > > > to
> > > > > make any changes unless a new cooling device has been bound
> > > > > to
> > > > > one of
> > > > > the trips in the thermal zone or the system is resuming from
> > > > > sleep.
> > > > > 
> > > > > For this reason, make bang_bang_manage() set governor_data
> > > > > for
> > > > > the
> > > > > thermal zone and check it upfront to decide whether or not it
> > > > > needs
> > > > > to
> > > > > do anything.
> > > > > 
> > > > > However, governor_data needs to be reset in some cases to let
> > > > > bang_bang_manage() know that it should walk the trips again,
> > > > > so
> > > > > add
> > > > > an
> > > > > .update_tz() callback to the governor and make the core
> > > > > additionally
> > > > > invoke it during system resume.
> > > > > 
> > > > > To avoid affecting the other users of that callback
> > > > > unnecessarily,
> > > > > add
> > > > > a special notification reason for system resume,
> > > > > THERMAL_TZ_RESUME,
> > > > > and
> > > > > also pass it to __thermal_zone_device_update() called during
> > > > > system
> > > > > resume for consistency.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > ---
> > > > >  drivers/thermal/gov_bang_bang.c |   18 ++++++++++++++++++
> > > > >  drivers/thermal/thermal_core.c  |    3 ++-
> > > > >  include/linux/thermal.h         |    1 +
> > > > >  3 files changed, 21 insertions(+), 1 deletion(-)
> > > > > 
> > > > > 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
> > > > > @@ -86,6 +86,10 @@ static void bang_bang_manage(struct ther
> > > > >         const struct thermal_trip_desc *td;
> > > > >         struct thermal_instance *instance;
> > > > > 
> > > > > +       /* If the code below has run already, nothing needs
> > > > > to be
> > > > > done. */
> > > > > +       if (tz->governor_data)
> > > > > +               return;
> > > > > +
> > > > >         for_each_trip_desc(tz, td) {
> > > > >                 const struct thermal_trip *trip = &td->trip;
> > > > > 
> > > > > @@ -107,11 +111,25 @@ static void bang_bang_manage(struct
> > > > > ther
> > > > > 
> > > > > bang_bang_set_instance_target(instanc
> > > > > e, 0);
> > > > >                 }
> > > > >         }
> > > > > +
> > > > > +       tz->governor_data = (void *)true;
> > > > > +}
> > > > > +
> > > > > +static void bang_bang_update_tz(struct thermal_zone_device
> > > > > *tz,
> > > > > +                               enum thermal_notify_event
> > > > > reason)
> > > > > +{
> > > > > +       /*
> > > > > +        * Let bang_bang_manage() know that it needs to walk
> > > > > trips
> > > > > after binding
> > > > > +        * a new cdev and after system resume.
> > > > > +        */
> > > > > +       if (reason == THERMAL_TZ_BIND_CDEV || reason ==
> > > > > THERMAL_TZ_RESUME)
> > > > > +               tz->governor_data = NULL;
> > > > >  }
> > > > 
> > > > can we do the cdev initialization for BIND_CDEV and RESUME
> > > > notification
> > > > in .update_tz() directly?
> > > 
> > > That would be viable if the zone temperature was known at the
> > > time
> > > .update_tz() runs, but it isn't.  See this message:
> > > 
> > > https://lore.kernel.org/linux-pm/CAJZ5v0ji_7Z-24iCO_Xxu4Zm4jgVFmR9jVp8QNiCOxzV9gqSnA@mail.gmail.com/
> > > 
> > > As long as the zone temperature is not known, it is not clear
> > > which
> > > way to initialize the cooling devices.
> > 
> > right. Then the patch LGTM.
> 
> Great!
> 
> > BTW, what do you think if we add handling for first temperature
> > read in
> > handle_thermal_trip(), say, some draft code like below,
> > 
> > if (tz->last_temperature == THERMAL_TEMP_INIT) {
> >         if (tz->temperature < trip->temperature)
> >                 list_add(&td->notify_list_node, way_down_list);
> >         else
> >                 list_add_tail(&td->notify_list_node, way_up_list);
> > }
> > 
> > This should handle both the init and the resume case.
> 
> I have considered doing something similar, but there are quite a few
> arguments against it.
> 
> First, it would cause spurious notifications to be sent to user
> space.
> Second, the debug code would need to be modified to take this case
> into account explicitly.  Moreover, this would be extra overhead for
> the other governors.
> 
> IMV it's better to limit the impact to the Bang-bang governor where
> the problem is.

Agreed.

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

thanks,
rui