[PATCH v1 09/17] thermal: ACPI: Use the .should_bind() thermal zone callback

[PATCH v1 09/17] thermal: ACPI: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

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

Make the ACPI thermal zone driver use the .should_bind() thermal zone
callback to provide the thermal core with the information on whether or
not to bind the given cooling device to the given trip point in the
given thermal zone.  If it returns 'true', the thermal core will bind
the cooling device to the trip and the corresponding unbinding will be
taken care of automatically by the core on the removal of the involved
thermal zone or cooling device.

This replaces the .bind() and .unbind() thermal zone callbacks which
allows the code to be simplified quite significantly while providing
the same functionality.

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

Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -558,77 +558,31 @@ static void acpi_thermal_zone_device_cri
 	thermal_zone_device_critical(thermal);
 }
 
-struct acpi_thermal_bind_data {
-	struct thermal_zone_device *thermal;
-	struct thermal_cooling_device *cdev;
-	bool bind;
-};
-
-static int bind_unbind_cdev_cb(struct thermal_trip *trip, void *arg)
+static bool acpi_thermal_should_bind_cdev(struct thermal_zone_device *thermal,
+					  const struct thermal_trip *trip,
+					  struct thermal_cooling_device *cdev,
+					  struct cooling_spec *c)
 {
 	struct acpi_thermal_trip *acpi_trip = trip->priv;
-	struct acpi_thermal_bind_data *bd = arg;
-	struct thermal_zone_device *thermal = bd->thermal;
-	struct thermal_cooling_device *cdev = bd->cdev;
 	struct acpi_device *cdev_adev = cdev->devdata;
 	int i;
 
 	/* Skip critical and hot trips. */
 	if (!acpi_trip)
-		return 0;
+		return false;
 
 	for (i = 0; i < acpi_trip->devices.count; i++) {
 		acpi_handle handle = acpi_trip->devices.handles[i];
-		struct acpi_device *adev = acpi_fetch_acpi_dev(handle);
-
-		if (adev != cdev_adev)
-			continue;
 
-		if (bd->bind) {
-			int ret;
-
-			ret = thermal_bind_cdev_to_trip(thermal, trip, cdev,
-							THERMAL_NO_LIMIT,
-							THERMAL_NO_LIMIT,
-							THERMAL_WEIGHT_DEFAULT);
-			if (ret)
-				return ret;
-		} else {
-			thermal_unbind_cdev_from_trip(thermal, trip, cdev);
-		}
+		if (acpi_fetch_acpi_dev(handle) == cdev_adev)
+			return true;
 	}
 
-	return 0;
-}
-
-static int acpi_thermal_bind_unbind_cdev(struct thermal_zone_device *thermal,
-					 struct thermal_cooling_device *cdev,
-					 bool bind)
-{
-	struct acpi_thermal_bind_data bd = {
-		.thermal = thermal, .cdev = cdev, .bind = bind
-	};
-
-	return thermal_zone_for_each_trip(thermal, bind_unbind_cdev_cb, &bd);
-}
-
-static int
-acpi_thermal_bind_cooling_device(struct thermal_zone_device *thermal,
-				 struct thermal_cooling_device *cdev)
-{
-	return acpi_thermal_bind_unbind_cdev(thermal, cdev, true);
-}
-
-static int
-acpi_thermal_unbind_cooling_device(struct thermal_zone_device *thermal,
-				   struct thermal_cooling_device *cdev)
-{
-	return acpi_thermal_bind_unbind_cdev(thermal, cdev, false);
+	return false;
 }
 
 static const struct thermal_zone_device_ops acpi_thermal_zone_ops = {
-	.bind = acpi_thermal_bind_cooling_device,
-	.unbind	= acpi_thermal_unbind_cooling_device,
+	.should_bind = acpi_thermal_should_bind_cdev,
 	.get_temp = thermal_get_temp,
 	.get_trend = thermal_get_trend,
 	.hot = acpi_thermal_zone_device_hot,

[PATCH v1 12/17] platform/x86: acerhdf: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

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

Make the acerhdf driver use the .should_bind() thermal zone
callback to provide the thermal core with the information on whether or
not to bind the given cooling device to the given trip point in the
given thermal zone.  If it returns 'true', the thermal core will bind
the cooling device to the trip and the corresponding unbinding will be
taken care of automatically by the core on the removal of the involved
thermal zone or cooling device.

The previously existing acerhdf_bind() function bound cooling devices
to thermal trip point 0 only, so the new callback needs to return 'true'
for trip point 0.  However, it is straightforward to observe that trip
point 0 is an active trip point and the only other trip point in the
driver's thermal zone is a critical one, so it is sufficient to return
'true' from that callback if the type of the given trip point is
THERMAL_TRIP_ACTIVE.

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

This patch only depends on patch [09/17].

---
 drivers/platform/x86/acerhdf.c |   33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)

Index: linux-pm/drivers/platform/x86/acerhdf.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/acerhdf.c
+++ linux-pm/drivers/platform/x86/acerhdf.c
@@ -378,33 +378,13 @@ static int acerhdf_get_ec_temp(struct th
 	return 0;
 }
 
-static int acerhdf_bind(struct thermal_zone_device *thermal,
-			struct thermal_cooling_device *cdev)
+static bool acerhdf_should_bind(struct thermal_zone_device *thermal,
+				const struct thermal_trip *trip,
+				struct thermal_cooling_device *cdev,
+				struct cooling_spec *c)
 {
 	/* if the cooling device is the one from acerhdf bind it */
-	if (cdev != cl_dev)
-		return 0;
-
-	if (thermal_zone_bind_cooling_device(thermal, 0, cdev,
-			THERMAL_NO_LIMIT, THERMAL_NO_LIMIT,
-			THERMAL_WEIGHT_DEFAULT)) {
-		pr_err("error binding cooling dev\n");
-		return -EINVAL;
-	}
-	return 0;
-}
-
-static int acerhdf_unbind(struct thermal_zone_device *thermal,
-			  struct thermal_cooling_device *cdev)
-{
-	if (cdev != cl_dev)
-		return 0;
-
-	if (thermal_zone_unbind_cooling_device(thermal, 0, cdev)) {
-		pr_err("error unbinding cooling dev\n");
-		return -EINVAL;
-	}
-	return 0;
+	return cdev == cl_dev && trip->type == THERMAL_TRIP_ACTIVE;
 }
 
 static inline void acerhdf_revert_to_bios_mode(void)
@@ -447,8 +427,7 @@ static int acerhdf_get_crit_temp(struct
 
 /* bind callback functions to thermalzone */
 static struct thermal_zone_device_ops acerhdf_dev_ops = {
-	.bind = acerhdf_bind,
-	.unbind = acerhdf_unbind,
+	.should_bind = acerhdf_should_bind,
 	.get_temp = acerhdf_get_ec_temp,
 	.change_mode = acerhdf_change_mode,
 	.get_crit_temp = acerhdf_get_crit_temp,

Re: [PATCH v1 12/17] platform/x86: acerhdf: Use the .should_bind() thermal zone callback

[Peter Kästle]

Hi Rafael,

On 30.07.24 20:33, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make the acerhdf driver use the .should_bind() thermal zone
> callback to provide the thermal core with the information on whether or
> not to bind the given cooling device to the given trip point in the
> given thermal zone.  If it returns 'true', the thermal core will bind
> the cooling device to the trip and the corresponding unbinding will be
> taken care of automatically by the core on the removal of the involved
> thermal zone or cooling device.
> 
> The previously existing acerhdf_bind() function bound cooling devices
> to thermal trip point 0 only, so the new callback needs to return 'true'
> for trip point 0.  However, it is straightforward to observe that trip
> point 0 is an active trip point and the only other trip point in the
> driver's thermal zone is a critical one, so it is sufficient to return
> 'true' from that callback if the type of the given trip point is
> THERMAL_TRIP_ACTIVE.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks for including me on the review.
I'm working on it, but unfortunately the refactoring of the thermal layer
around gov_bang_bang.c earlier this year broke acerhdf.
This needs some debugging and refactoring.  I think I can finish it on
upcoming weekend.

-- 
--peter;

Re: [PATCH v1 12/17] platform/x86: acerhdf: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

Hi Peter,

On Wed, Jul 31, 2024 at 10:50 PM Peter Kästle <xypiie@gmail.com> wrote:
>
> Hi Rafael,
>
> On 30.07.24 20:33, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Make the acerhdf driver use the .should_bind() thermal zone
> > callback to provide the thermal core with the information on whether or
> > not to bind the given cooling device to the given trip point in the
> > given thermal zone.  If it returns 'true', the thermal core will bind
> > the cooling device to the trip and the corresponding unbinding will be
> > taken care of automatically by the core on the removal of the involved
> > thermal zone or cooling device.
> >
> > The previously existing acerhdf_bind() function bound cooling devices
> > to thermal trip point 0 only, so the new callback needs to return 'true'
> > for trip point 0.  However, it is straightforward to observe that trip
> > point 0 is an active trip point and the only other trip point in the
> > driver's thermal zone is a critical one, so it is sufficient to return
> > 'true' from that callback if the type of the given trip point is
> > THERMAL_TRIP_ACTIVE.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Thanks for including me on the review.
> I'm working on it, but unfortunately the refactoring of the thermal layer
> around gov_bang_bang.c earlier this year broke acerhdf.

Well, sorry about that.

> This needs some debugging and refactoring.  I think I can finish it on
> upcoming weekend.

Thank you!

I'll be offline next week, so I'll go back to this material in two
weeks or so anyway.

Re: [PATCH v1 12/17] platform/x86: acerhdf: Use the .should_bind() thermal zone callback

[Peter Kästle]

Hi Rafael,

On 01.08.24 12:14, Rafael J. Wysocki wrote:
> Hi Peter,
> 
> On Wed, Jul 31, 2024 at 10:50 PM Peter Kästle <xypiie@gmail.com> wrote:
>>
>> Hi Rafael,
>>
>> On 30.07.24 20:33, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Make the acerhdf driver use the .should_bind() thermal zone
>>> callback to provide the thermal core with the information on whether or
>>> not to bind the given cooling device to the given trip point in the
>>> given thermal zone.  If it returns 'true', the thermal core will bind
>>> the cooling device to the trip and the corresponding unbinding will be
>>> taken care of automatically by the core on the removal of the involved
>>> thermal zone or cooling device.
>>>
>>> The previously existing acerhdf_bind() function bound cooling devices
>>> to thermal trip point 0 only, so the new callback needs to return 'true'
>>> for trip point 0.  However, it is straightforward to observe that trip
>>> point 0 is an active trip point and the only other trip point in the
>>> driver's thermal zone is a critical one, so it is sufficient to return
>>> 'true' from that callback if the type of the given trip point is
>>> THERMAL_TRIP_ACTIVE.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Thanks for including me on the review.
>> I'm working on it, but unfortunately the refactoring of the thermal layer
>> around gov_bang_bang.c earlier this year broke acerhdf.
> 
> Well, sorry about that.

I already fixed the main problem, which caused full malfunction of acerhdf:

The new functionality of .trip_crossed in the gov_bang_bang is missing an
initial check, whether the temperature is below the fanoff temperature
(trip_point.temperature - hysteresis) and by that it did not turn the
fan off.  This then caused that the system will never heat up as much to
cross the upper temperature. As a consequence it could never cross the
lower temperature to turn the fan off. -> Fan was locked always on.
And that's obviously not what we want.
As I didn't find any API call, to ask the governor doing that for me, I
added an "acerhdf_init_fan()" functionality into acerhdf init function right
after registering the thermal zone (and on resume from suspend) which turns
the fan off if the temperature is lower than the fanoff parameter.
Probably not the nicest solution, but maybe the most pragmatic one without
touching the thermal layer.

>> This needs some debugging and refactoring.  I think I can finish it on
>> upcoming weekend.
> 
> Thank you!

I'll need some more time to check why other features of acerhdf broke:
* interval cannot be changed to longer than one second.
   No idea yet, do you have any idea?  Maybe I'll simply drop this
   functionality, as there's no big overhead by polling every second.
* changing /sys/module/acerhdf/parameters/{fanon,fanoff} at runtime
   to change the trip point settings stopped working.  This needs some
   restructuring using module_param_cb callbacks.

> I'll be offline next week, so I'll go back to this material in two
> weeks or so anyway.

I still need some time to fix the remaining part anyhow.  Once this is
done, I'll check your latest patch series and send my acerhdf rework for
review / submission.

-- 
regards,
--peter;

Re: [PATCH v1 12/17] platform/x86: acerhdf: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

Hi Peter,

On Tue, Aug 6, 2024 at 12:10 AM Peter Kästle <peter@piie.net> wrote:
>
> Hi Rafael,
>
> On 01.08.24 12:14, Rafael J. Wysocki wrote:
> > Hi Peter,
> >
> > On Wed, Jul 31, 2024 at 10:50 PM Peter Kästle <xypiie@gmail.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> On 30.07.24 20:33, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> Make the acerhdf driver use the .should_bind() thermal zone
> >>> callback to provide the thermal core with the information on whether or
> >>> not to bind the given cooling device to the given trip point in the
> >>> given thermal zone.  If it returns 'true', the thermal core will bind
> >>> the cooling device to the trip and the corresponding unbinding will be
> >>> taken care of automatically by the core on the removal of the involved
> >>> thermal zone or cooling device.
> >>>
> >>> The previously existing acerhdf_bind() function bound cooling devices
> >>> to thermal trip point 0 only, so the new callback needs to return 'true'
> >>> for trip point 0.  However, it is straightforward to observe that trip
> >>> point 0 is an active trip point and the only other trip point in the
> >>> driver's thermal zone is a critical one, so it is sufficient to return
> >>> 'true' from that callback if the type of the given trip point is
> >>> THERMAL_TRIP_ACTIVE.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> Thanks for including me on the review.
> >> I'm working on it, but unfortunately the refactoring of the thermal layer
> >> around gov_bang_bang.c earlier this year broke acerhdf.
> >
> > Well, sorry about that.
>
> I already fixed the main problem, which caused full malfunction of acerhdf:
>
> The new functionality of .trip_crossed in the gov_bang_bang is missing an
> initial check, whether the temperature is below the fanoff temperature
> (trip_point.temperature - hysteresis) and by that it did not turn the
> fan off.

So IIUC this is when the fan starts in the "on" state and the thermal
core is expected to turn it off when the zone temperature is not in
fact above the trip point low temperature.

> This then caused that the system will never heat up as much to
> cross the upper temperature. As a consequence it could never cross the
> lower temperature to turn the fan off. -> Fan was locked always on.
> And that's obviously not what we want.

Sure.

> As I didn't find any API call, to ask the governor doing that for me, I
> added an "acerhdf_init_fan()" functionality into acerhdf init function right
> after registering the thermal zone (and on resume from suspend) which turns
> the fan off if the temperature is lower than the fanoff parameter.
> Probably not the nicest solution, but maybe the most pragmatic one without
> touching the thermal layer.

Well, this issue may not be limited to the acerhdf case, so it may be
good to address it in the thermal core.  There is kind of a
chicken-and-egg situation in there, however, because the cooling
device state is determined by the governor which only runs when it is
called, but the core doesn't know that the governor should be invoked
when there are no trip point crossing events.

It may just be a matter of adding an ->update_tz() callback to the
bang-bang governor, let me see.

> >> This needs some debugging and refactoring.  I think I can finish it on
> >> upcoming weekend.
> >
> > Thank you!
>
> I'll need some more time to check why other features of acerhdf broke:
> * interval cannot be changed to longer than one second.
>    No idea yet, do you have any idea?

No, I don't, but I'll have a look.

>  Maybe I'll simply drop this
>    functionality, as there's no big overhead by polling every second.

No, there isn't, but anyway it would be good to know why this does not
work as expected any more.

> * changing /sys/module/acerhdf/parameters/{fanon,fanoff} at runtime
>    to change the trip point settings stopped working.  This needs some
>    restructuring using module_param_cb callbacks.

I'll have a look at this too.

> > I'll be offline next week, so I'll go back to this material in two
> > weeks or so anyway.
>
> I still need some time to fix the remaining part anyhow.  Once this is
> done, I'll check your latest patch series and send my acerhdf rework for
> review / submission.

Sure.

In the meantime, I have resent the series including the acerhdf
change, but it is the same as before.

Thanks!

Re: [PATCH v1 12/17] platform/x86: acerhdf: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 5864 bytes --]

Hi Peter,

On Mon, Aug 12, 2024 at 4:56 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Aug 6, 2024 at 12:10 AM Peter Kästle <peter@piie.net> wrote:
> >
> > Hi Rafael,
> >
> > On 01.08.24 12:14, Rafael J. Wysocki wrote:
> > > Hi Peter,
> > >
> > > On Wed, Jul 31, 2024 at 10:50 PM Peter Kästle <xypiie@gmail.com> wrote:
> > >>
> > >> Hi Rafael,
> > >>
> > >> On 30.07.24 20:33, Rafael J. Wysocki wrote:
> > >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >>>
> > >>> Make the acerhdf driver use the .should_bind() thermal zone
> > >>> callback to provide the thermal core with the information on whether or
> > >>> not to bind the given cooling device to the given trip point in the
> > >>> given thermal zone.  If it returns 'true', the thermal core will bind
> > >>> the cooling device to the trip and the corresponding unbinding will be
> > >>> taken care of automatically by the core on the removal of the involved
> > >>> thermal zone or cooling device.
> > >>>
> > >>> The previously existing acerhdf_bind() function bound cooling devices
> > >>> to thermal trip point 0 only, so the new callback needs to return 'true'
> > >>> for trip point 0.  However, it is straightforward to observe that trip
> > >>> point 0 is an active trip point and the only other trip point in the
> > >>> driver's thermal zone is a critical one, so it is sufficient to return
> > >>> 'true' from that callback if the type of the given trip point is
> > >>> THERMAL_TRIP_ACTIVE.
> > >>>
> > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >>
> > >> Thanks for including me on the review.
> > >> I'm working on it, but unfortunately the refactoring of the thermal layer
> > >> around gov_bang_bang.c earlier this year broke acerhdf.
> > >
> > > Well, sorry about that.
> >
> > I already fixed the main problem, which caused full malfunction of acerhdf:
> >
> > The new functionality of .trip_crossed in the gov_bang_bang is missing an
> > initial check, whether the temperature is below the fanoff temperature
> > (trip_point.temperature - hysteresis) and by that it did not turn the
> > fan off.
>
> So IIUC this is when the fan starts in the "on" state and the thermal
> core is expected to turn it off when the zone temperature is not in
> fact above the trip point low temperature.
>
> > This then caused that the system will never heat up as much to
> > cross the upper temperature. As a consequence it could never cross the
> > lower temperature to turn the fan off. -> Fan was locked always on.
> > And that's obviously not what we want.
>
> Sure.
>
> > As I didn't find any API call, to ask the governor doing that for me, I
> > added an "acerhdf_init_fan()" functionality into acerhdf init function right
> > after registering the thermal zone (and on resume from suspend) which turns
> > the fan off if the temperature is lower than the fanoff parameter.
> > Probably not the nicest solution, but maybe the most pragmatic one without
> > touching the thermal layer.
>
> Well, this issue may not be limited to the acerhdf case, so it may be
> good to address it in the thermal core.  There is kind of a
> chicken-and-egg situation in there, however, because the cooling
> device state is determined by the governor which only runs when it is
> called, but the core doesn't know that the governor should be invoked
> when there are no trip point crossing events.
>
> It may just be a matter of adding an ->update_tz() callback to the
> bang-bang governor, let me see.

Attached is a patch doing that - and one more change to invoke this
callback during thermal zone resume.

It should apply to 6.11-rc3 (without any of the thermal core changes
in linux-next or on the list).

If I'm not mistaken, this should address the issue above.

> > >> This needs some debugging and refactoring.  I think I can finish it on
> > >> upcoming weekend.
> > >
> > > Thank you!
> >
> > I'll need some more time to check why other features of acerhdf broke:
> > * interval cannot be changed to longer than one second.
> >    No idea yet, do you have any idea?
>
> No, I don't, but I'll have a look.

This may be related to the way in which round_jiffies() is used by
thermal_set_delay_jiffies() in the thermal core.  I think it should
call round_jiffies_relative() instead, so you may try to replace
round_jiffies() with round_jiffies_relative() in
thermal_set_delay_jiffies() and see if that helps.

> >  Maybe I'll simply drop this
> >    functionality, as there's no big overhead by polling every second.
>
> No, there isn't, but anyway it would be good to know why this does not
> work as expected any more.
>
> > * changing /sys/module/acerhdf/parameters/{fanon,fanoff} at runtime
> >    to change the trip point settings stopped working.  This needs some
> >    restructuring using module_param_cb callbacks.
>
> I'll have a look at this too.

So acerhdf_check_param() updates the trips table, but it is only
called from interval_set_uint(), so it will only trigger when the
interval module param is updated AFAICS.

Apart from this, it is not sufficient to update the original trips
table passed to register_thermal_zone_device_with_trips() because the
core makes a copy of it and does not use the original one any more
after zone registration.

What you need is to either mark the trip point as writable (set
THERMAL_TRIP_FLAG_RW_TEMP and THERMAL_TRIP_FLAG_RW_HYST for it in
flags), but that will not engage the fanon/fanoff sanity check in the
driver, or add callbacks for setting trip temperature and hysteresis
to the thermal zone operations and hook them up to the module
parameters.  However, you'd need to restore .set_trip_hyst() for this
which was removed because it was unused.

Thanks!

[-- Attachment #2: thermal-gov_bamg_bang-update_tz.patch --]
[-- Type: text/x-patch, Size: 2825 bytes --]

---
 drivers/thermal/gov_bang_bang.c |   34 ++++++++++++++++++++++++++++++++++
 drivers/thermal/thermal_core.c  |    3 ++-
 include/linux/thermal.h         |    1 +
 3 files changed, 37 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
@@ -79,8 +79,42 @@ static void bang_bang_control(struct the
 		thermal_cdev_update(instance->cdev);
 }
 
+static void bang_bang_update_tz(struct thermal_zone_device *tz,
+				enum thermal_notify_event reason)
+{
+	struct thermal_instance *instance;
+
+	lockdep_assert_held(&tz->lock);
+
+	if (reason != THERMAL_TZ_BIND_CDEV && reason != THERMAL_TZ_RESUME)
+		return;
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		const struct thermal_trip *trip = instance->trip;
+
+		if (instance->initialized)
+			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.
+		 */
+		if (tz->temperature < trip->temperature)
+			bang_bang_control(tz, trip, 0);
+
+		instance->initialized = true;
+	}
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
+		thermal_cdev_update(instance->cdev);
+}
+
 static struct thermal_governor thermal_gov_bang_bang = {
 	.name		= "bang_bang",
 	.trip_crossed	= bang_bang_control,
+	.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
@@ -1711,7 +1711,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 resume after system sleep */
 };
 
 /**

Re: [PATCH v1 12/17] platform/x86: acerhdf: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 4371 bytes --]

Hi Peter,

On Mon, Aug 12, 2024 at 6:15 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Aug 12, 2024 at 4:56 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Aug 6, 2024 at 12:10 AM Peter Kästle <peter@piie.net> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On 01.08.24 12:14, Rafael J. Wysocki wrote:
> > > > Hi Peter,
> > > >
> > > > On Wed, Jul 31, 2024 at 10:50 PM Peter Kästle <xypiie@gmail.com> wrote:
> > > >>
> > > >> Hi Rafael,
> > > >>
> > > >> On 30.07.24 20:33, Rafael J. Wysocki wrote:
> > > >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >>>
> > > >>> Make the acerhdf driver use the .should_bind() thermal zone
> > > >>> callback to provide the thermal core with the information on whether or
> > > >>> not to bind the given cooling device to the given trip point in the
> > > >>> given thermal zone.  If it returns 'true', the thermal core will bind
> > > >>> the cooling device to the trip and the corresponding unbinding will be
> > > >>> taken care of automatically by the core on the removal of the involved
> > > >>> thermal zone or cooling device.
> > > >>>
> > > >>> The previously existing acerhdf_bind() function bound cooling devices
> > > >>> to thermal trip point 0 only, so the new callback needs to return 'true'
> > > >>> for trip point 0.  However, it is straightforward to observe that trip
> > > >>> point 0 is an active trip point and the only other trip point in the
> > > >>> driver's thermal zone is a critical one, so it is sufficient to return
> > > >>> 'true' from that callback if the type of the given trip point is
> > > >>> THERMAL_TRIP_ACTIVE.
> > > >>>
> > > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >>
> > > >> Thanks for including me on the review.
> > > >> I'm working on it, but unfortunately the refactoring of the thermal layer
> > > >> around gov_bang_bang.c earlier this year broke acerhdf.
> > > >
> > > > Well, sorry about that.
> > >
> > > I already fixed the main problem, which caused full malfunction of acerhdf:
> > >
> > > The new functionality of .trip_crossed in the gov_bang_bang is missing an
> > > initial check, whether the temperature is below the fanoff temperature
> > > (trip_point.temperature - hysteresis) and by that it did not turn the
> > > fan off.
> >
> > So IIUC this is when the fan starts in the "on" state and the thermal
> > core is expected to turn it off when the zone temperature is not in
> > fact above the trip point low temperature.
> >
> > > This then caused that the system will never heat up as much to
> > > cross the upper temperature. As a consequence it could never cross the
> > > lower temperature to turn the fan off. -> Fan was locked always on.
> > > And that's obviously not what we want.
> >
> > Sure.
> >
> > > As I didn't find any API call, to ask the governor doing that for me, I
> > > added an "acerhdf_init_fan()" functionality into acerhdf init function right
> > > after registering the thermal zone (and on resume from suspend) which turns
> > > the fan off if the temperature is lower than the fanoff parameter.
> > > Probably not the nicest solution, but maybe the most pragmatic one without
> > > touching the thermal layer.
> >
> > Well, this issue may not be limited to the acerhdf case, so it may be
> > good to address it in the thermal core.  There is kind of a
> > chicken-and-egg situation in there, however, because the cooling
> > device state is determined by the governor which only runs when it is
> > called, but the core doesn't know that the governor should be invoked
> > when there are no trip point crossing events.
> >
> > It may just be a matter of adding an ->update_tz() callback to the
> > bang-bang governor, let me see.

This isn't the right approach because .update_tz() is called before
checking the zone temperature for the first time, but to remedy the
issue at hand, code needs to run when the zone temperature is known to
the thermal core.

This means that the Bang-bang governor needs a .manage() callback in
addition to the .trip_crossed() one, but that callback will only need
to check if the states of cooling devices bound to the trip points
below the zone temperature need to be adjusted, and just once.

So something like in the attached patch.

[-- Attachment #2: thermal-gov_bang_bang-manage.patch --]
[-- Type: text/x-patch, Size: 3421 bytes --]

---
 drivers/thermal/gov_bang_bang.c |   64 ++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 15 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,28 @@
 
 #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;
+	instance->initialized = true;
+
+	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
@@ -54,25 +76,36 @@ 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->trip == trip)
+			bang_bang_set_instance_target(instance, crossed_up);
+	}
+}
 
-		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);
-		instance->cdev->updated = false; /* cdev needs update */
-		mutex_unlock(&instance->cdev->lock);
+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);
+		}
 	}
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
@@ -82,5 +115,6 @@ static void bang_bang_control(struct the
 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);