[PATCH v1 17/17] thermal: code: Pass trip descriptors to trip bind/unbind functions

[PATCH v1 17/17] thermal: code: Pass trip descriptors to trip bind/unbind functions

[Rafael J. Wysocki]

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

The code is somewhat cleaner if struct thermal_trip_desc pointers are
passed to thermal_bind_cdev_to_trip(), thermal_unbind_cdev_from_trip(),
and print_bind_err_msg() instead of struct thermal_trip pointers, so
modify it accordingly.

No intentional functional impact.

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

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -759,9 +759,9 @@ struct thermal_zone_device *thermal_zone
 /**
  * thermal_bind_cdev_to_trip - bind a cooling device to a thermal zone
  * @tz:		pointer to struct thermal_zone_device
- * @trip:	trip point the cooling devices is associated with in this zone.
+ * @td:		descriptor of the trip point to bind @cdev to
  * @cdev:	pointer to struct thermal_cooling_device
- * @c:		cooling specification for @trip and @cdev
+ * @c:		cooling specification for the trip point and @cdev
  *
  * This interface function bind a thermal cooling device to the certain trip
  * point of a thermal zone device.
@@ -770,11 +770,10 @@ struct thermal_zone_device *thermal_zone
  * Return: 0 on success, the proper error value otherwise.
  */
 static int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
-				     struct thermal_trip *trip,
+				     struct thermal_trip_desc *td,
 				     struct thermal_cooling_device *cdev,
 				     struct cooling_spec *c)
 {
-	struct thermal_trip_desc *td = trip_to_trip_desc(trip);
 	struct thermal_instance *dev, *instance;
 	bool upper_no_limit;
 	int result;
@@ -805,7 +804,7 @@ static int thermal_bind_cdev_to_trip(str
 
 	dev->tz = tz;
 	dev->cdev = cdev;
-	dev->trip = trip;
+	dev->trip = &td->trip;
 	dev->upper = c->upper;
 	dev->upper_no_limit = upper_no_limit;
 	dev->lower = c->lower;
@@ -878,7 +877,7 @@ free_mem:
 /**
  * thermal_unbind_cdev_from_trip - unbind a cooling device from a thermal zone.
  * @tz:		pointer to a struct thermal_zone_device.
- * @trip:	trip point the cooling devices is associated with in this zone.
+ * @td:		descriptor of the trip point to unbind @cdev from
  * @cdev:	pointer to a struct thermal_cooling_device.
  *
  * This interface function unbind a thermal cooling device from the certain
@@ -886,10 +885,9 @@ free_mem:
  * This function is usually called in the thermal zone device .unbind callback.
  */
 static void thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
-					  struct thermal_trip *trip,
+					  struct thermal_trip_desc *td,
 					  struct thermal_cooling_device *cdev)
 {
-	struct thermal_trip_desc *td = trip_to_trip_desc(trip);
 	struct thermal_instance *pos, *next;
 
 	lockdep_assert_held(&tz->lock);
@@ -945,11 +943,11 @@ static struct class *thermal_class;
 
 static inline
 void print_bind_err_msg(struct thermal_zone_device *tz,
-			const struct thermal_trip *trip,
+			const struct thermal_trip_desc *td,
 			struct thermal_cooling_device *cdev, int ret)
 {
 	dev_err(&tz->device, "binding cdev %s to trip %d failed: %d\n",
-		cdev->type, thermal_zone_trip_id(tz, trip), ret);
+		cdev->type, thermal_zone_trip_id(tz, &td->trip), ret);
 }
 
 static void thermal_zone_cdev_binding(struct thermal_zone_device *tz,
@@ -963,7 +961,6 @@ static void thermal_zone_cdev_binding(st
 	mutex_lock(&tz->lock);
 
 	for_each_trip_desc(tz, td) {
-		struct thermal_trip *trip = &td->trip;
 		struct cooling_spec c = {
 			.upper = THERMAL_NO_LIMIT,
 			.lower = THERMAL_NO_LIMIT,
@@ -971,12 +968,12 @@ static void thermal_zone_cdev_binding(st
 		};
 		int ret;
 
-		if (!tz->ops.should_bind(tz, trip, cdev, &c))
+		if (!tz->ops.should_bind(tz, &td->trip, cdev, &c))
 			continue;
 
-		ret = thermal_bind_cdev_to_trip(tz, trip, cdev, &c);
+		ret = thermal_bind_cdev_to_trip(tz, td, cdev, &c);
 		if (ret)
-			print_bind_err_msg(tz, trip, cdev, ret);
+			print_bind_err_msg(tz, td, cdev, ret);
 	}
 
 	mutex_unlock(&tz->lock);
@@ -1287,7 +1284,7 @@ static void thermal_zone_cdev_unbinding(
 	mutex_lock(&tz->lock);
 
 	for_each_trip_desc(tz, td)
-		thermal_unbind_cdev_from_trip(tz, &td->trip, cdev);
+		thermal_unbind_cdev_from_trip(tz, td, cdev);
 
 	mutex_unlock(&tz->lock);
 }

[PATCH v1 01/17] thermal: core: Fold two functions into their respective callers

[Rafael J. Wysocki]

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

Fold bind_cdev() into __thermal_cooling_device_register() and bind_tz()
into thermal_zone_device_register_with_trips() to reduce code bloat and
make it somewhat easier to follow the code flow.

No intentional functional impact.

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

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -991,20 +991,6 @@ void print_bind_err_msg(struct thermal_z
 		tz->type, cdev->type, ret);
 }
 
-static void bind_cdev(struct thermal_cooling_device *cdev)
-{
-	int ret;
-	struct thermal_zone_device *pos = NULL;
-
-	list_for_each_entry(pos, &thermal_tz_list, node) {
-		if (pos->ops.bind) {
-			ret = pos->ops.bind(pos, cdev);
-			if (ret)
-				print_bind_err_msg(pos, cdev, ret);
-		}
-	}
-}
-
 /**
  * __thermal_cooling_device_register() - register a new thermal cooling device
  * @np:		a pointer to a device tree node.
@@ -1100,7 +1086,13 @@ __thermal_cooling_device_register(struct
 	list_add(&cdev->node, &thermal_cdev_list);
 
 	/* Update binding information for 'this' new cdev */
-	bind_cdev(cdev);
+	list_for_each_entry(pos, &thermal_tz_list, node) {
+		if (pos->ops.bind) {
+			ret = pos->ops.bind(pos, cdev);
+			if (ret)
+				print_bind_err_msg(pos, cdev, ret);
+		}
+	}
 
 	list_for_each_entry(pos, &thermal_tz_list, node)
 		if (atomic_cmpxchg(&pos->need_update, 1, 0))
@@ -1338,25 +1330,6 @@ void thermal_cooling_device_unregister(s
 }
 EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
 
-static void bind_tz(struct thermal_zone_device *tz)
-{
-	int ret;
-	struct thermal_cooling_device *pos = NULL;
-
-	if (!tz->ops.bind)
-		return;
-
-	mutex_lock(&thermal_list_lock);
-
-	list_for_each_entry(pos, &thermal_cdev_list, node) {
-		ret = tz->ops.bind(tz, pos);
-		if (ret)
-			print_bind_err_msg(tz, pos, ret);
-	}
-
-	mutex_unlock(&thermal_list_lock);
-}
-
 static void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms)
 {
 	*delay_jiffies = msecs_to_jiffies(delay_ms);
@@ -1554,13 +1527,23 @@ thermal_zone_device_register_with_trips(
 	}
 
 	mutex_lock(&thermal_list_lock);
+
 	mutex_lock(&tz->lock);
 	list_add_tail(&tz->node, &thermal_tz_list);
 	mutex_unlock(&tz->lock);
-	mutex_unlock(&thermal_list_lock);
 
 	/* Bind cooling devices for this zone */
-	bind_tz(tz);
+	if (tz->ops.bind) {
+		struct thermal_cooling_device *cdev;
+
+		list_for_each_entry(cdev, &thermal_cdev_list, node) {
+			result = tz->ops.bind(tz, cdev);
+			if (result)
+				print_bind_err_msg(tz, cdev, result);
+		}
+	}
+
+	mutex_unlock(&thermal_list_lock);
 
 	thermal_zone_device_init(tz);
 	/* Update the new thermal zone and mark it as already updated. */

[PATCH v1 03/17] thermal: core: Drop redundant thermal instance checks

[Rafael J. Wysocki]

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

Because the trip and cdev pointers are sufficient to idendify a thermal
instance holding them unambiguously, drop the additional thermal zone
checks from two loops walking the list of thermal instances in a
thermal zone.

No intentional functional impact.

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

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -850,7 +850,7 @@ int thermal_bind_cdev_to_trip(struct the
 	mutex_lock(&tz->lock);
 	mutex_lock(&cdev->lock);
 	list_for_each_entry(pos, &tz->thermal_instances, tz_node)
-		if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
+		if (pos->trip == trip && pos->cdev == cdev) {
 			result = -EEXIST;
 			break;
 		}
@@ -915,7 +915,7 @@ int thermal_unbind_cdev_from_trip(struct
 	mutex_lock(&tz->lock);
 	mutex_lock(&cdev->lock);
 	list_for_each_entry_safe(pos, next, &tz->thermal_instances, tz_node) {
-		if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
+		if (pos->trip == trip && pos->cdev == cdev) {
 			list_del(&pos->tz_node);
 			list_del(&pos->cdev_node);
 

[PATCH v1 04/17] thermal: core: Clean up cdev binding/unbinding functions

[Rafael J. Wysocki]

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

Add a new label to thermal_bind_cdev_to_trip() and use it to eliminate
two redundant !result check from that function, rename a label in
thermal_unbind_cdev_from_trip() to reflect its actual purpose and
adjust some white space in these functions.

No intentional functional impact.

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

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -806,6 +806,7 @@ int thermal_bind_cdev_to_trip(struct the
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
+
 	dev->tz = tz;
 	dev->cdev = cdev;
 	dev->trip = trip;
@@ -821,8 +822,7 @@ int thermal_bind_cdev_to_trip(struct the
 
 	dev->id = result;
 	sprintf(dev->name, "cdev%d", dev->id);
-	result =
-	    sysfs_create_link(&tz->device.kobj, &cdev->device.kobj, dev->name);
+	result = sysfs_create_link(&tz->device.kobj, &cdev->device.kobj, dev->name);
 	if (result)
 		goto release_ida;
 
@@ -849,24 +849,26 @@ int thermal_bind_cdev_to_trip(struct the
 
 	mutex_lock(&tz->lock);
 	mutex_lock(&cdev->lock);
-	list_for_each_entry(pos, &tz->thermal_instances, tz_node)
+
+	list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
 		if (pos->trip == trip && pos->cdev == cdev) {
 			result = -EEXIST;
-			break;
+			goto remove_weight_file;
 		}
-	if (!result) {
-		list_add_tail(&dev->tz_node, &tz->thermal_instances);
-		list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
-		atomic_set(&tz->need_update, 1);
-
-		thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
 	}
+
+	list_add_tail(&dev->tz_node, &tz->thermal_instances);
+	list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
+	atomic_set(&tz->need_update, 1);
+
+	thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
+
 	mutex_unlock(&cdev->lock);
 	mutex_unlock(&tz->lock);
 
-	if (!result)
-		return 0;
+	return 0;
 
+remove_weight_file:
 	device_remove_file(&tz->device, &dev->weight_attr);
 remove_trip_file:
 	device_remove_file(&tz->device, &dev->attr);
@@ -914,6 +916,7 @@ int thermal_unbind_cdev_from_trip(struct
 
 	mutex_lock(&tz->lock);
 	mutex_lock(&cdev->lock);
+
 	list_for_each_entry_safe(pos, next, &tz->thermal_instances, tz_node) {
 		if (pos->trip == trip && pos->cdev == cdev) {
 			list_del(&pos->tz_node);
@@ -923,15 +926,16 @@ int thermal_unbind_cdev_from_trip(struct
 
 			mutex_unlock(&cdev->lock);
 			mutex_unlock(&tz->lock);
-			goto unbind;
+			goto free;
 		}
 	}
+
 	mutex_unlock(&cdev->lock);
 	mutex_unlock(&tz->lock);
 
 	return -ENODEV;
 
-unbind:
+free:
 	device_remove_file(&tz->device, &pos->weight_attr);
 	device_remove_file(&tz->device, &pos->attr);
 	sysfs_remove_link(&tz->device.kobj, pos->name);

[PATCH v1 06/17] thermal: sysfs: Use the dev argument in instance-related show/store

[Rafael J. Wysocki]

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

Two sysfs show/store functions for attributes representing thermal
instances, trip_point_show() and weight_store(), retrieve the thermal
zone pointer from the instance object at hand, but they may also get
it from their dev argument, which is more consistent with what the
other thermal sysfs functions do, so make them do so.

No intentional functional impact.

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

Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -836,13 +836,12 @@ void thermal_cooling_device_stats_reinit
 ssize_t
 trip_point_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
 	struct thermal_instance *instance;
 
-	instance =
-	    container_of(attr, struct thermal_instance, attr);
+	instance = container_of(attr, struct thermal_instance, attr);
 
-	return sprintf(buf, "%d\n",
-		       thermal_zone_trip_id(instance->tz, instance->trip));
+	return sprintf(buf, "%d\n", thermal_zone_trip_id(tz, instance->trip));
 }
 
 ssize_t
@@ -858,6 +857,7 @@ weight_show(struct device *dev, struct d
 ssize_t weight_store(struct device *dev, struct device_attribute *attr,
 		     const char *buf, size_t count)
 {
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
 	struct thermal_instance *instance;
 	int ret, weight;
 
@@ -868,14 +868,13 @@ ssize_t weight_store(struct device *dev,
 	instance = container_of(attr, struct thermal_instance, weight_attr);
 
 	/* Don't race with governors using the 'weight' value */
-	mutex_lock(&instance->tz->lock);
+	mutex_lock(&tz->lock);
 
 	instance->weight = weight;
 
-	thermal_governor_update_tz(instance->tz,
-				   THERMAL_INSTANCE_WEIGHT_CHANGED);
+	thermal_governor_update_tz(tz, THERMAL_INSTANCE_WEIGHT_CHANGED);
 
-	mutex_unlock(&instance->tz->lock);
+	mutex_unlock(&tz->lock);
 
 	return count;
 }

[PATCH v1 07/17] thermal: core: Move thermal zone locking out of bind/unbind functions

[Rafael J. Wysocki]

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

Since thermal_bind_cdev_to_trip() and thermal_unbind_cdev_from_trip()
acquire the thermal zone lock, the locking rules for their callers get
complicated.  In particular, the thermal zone lock cannot be acquired
in any code path leading to one of these functions even though it might
be useful to do so.

To address this, remove the thermal zone locking from both these
functions, add lockdep assertions for the thermal zone lock to both
of them and make their callers acquire the thermal zone lock instead.

No intentional functional impact.

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

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -789,6 +789,7 @@ int thermal_bind_cdev_to_trip(struct the
 	int result;
 
 	lockdep_assert_held(&thermal_list_lock);
+	lockdep_assert_held(&tz->lock);
 
 	if (list_empty(&tz->node) || list_empty(&cdev->node))
 		return -EINVAL;
@@ -851,7 +852,6 @@ int thermal_bind_cdev_to_trip(struct the
 	if (result)
 		goto remove_trip_file;
 
-	mutex_lock(&tz->lock);
 	mutex_lock(&cdev->lock);
 
 	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
@@ -868,7 +868,6 @@ int thermal_bind_cdev_to_trip(struct the
 	thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
 
 	mutex_unlock(&cdev->lock);
-	mutex_unlock(&tz->lock);
 
 	return 0;
 
@@ -892,11 +891,19 @@ int thermal_zone_bind_cooling_device(str
 				     unsigned long upper, unsigned long lower,
 				     unsigned int weight)
 {
+	int ret;
+
 	if (trip_index < 0 || trip_index >= tz->num_trips)
 		return -EINVAL;
 
-	return thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index].trip, cdev,
-					 upper, lower, weight);
+	mutex_lock(&tz->lock);
+
+	ret = thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index].trip, cdev,
+					upper, lower, weight);
+
+	mutex_unlock(&tz->lock);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(thermal_zone_bind_cooling_device);
 
@@ -919,7 +926,8 @@ int thermal_unbind_cdev_from_trip(struct
 	struct thermal_trip_desc *td = trip_to_trip_desc(trip);
 	struct thermal_instance *pos, *next;
 
-	mutex_lock(&tz->lock);
+	lockdep_assert_held(&tz->lock);
+
 	mutex_lock(&cdev->lock);
 
 	list_for_each_entry_safe(pos, next, &td->thermal_instances, trip_node) {
@@ -930,13 +938,11 @@ int thermal_unbind_cdev_from_trip(struct
 			thermal_governor_update_tz(tz, THERMAL_TZ_UNBIND_CDEV);
 
 			mutex_unlock(&cdev->lock);
-			mutex_unlock(&tz->lock);
 			goto free;
 		}
 	}
 
 	mutex_unlock(&cdev->lock);
-	mutex_unlock(&tz->lock);
 
 	return -ENODEV;
 
@@ -954,10 +960,18 @@ int thermal_zone_unbind_cooling_device(s
 				       int trip_index,
 				       struct thermal_cooling_device *cdev)
 {
+	int ret;
+
 	if (trip_index < 0 || trip_index >= tz->num_trips)
 		return -EINVAL;
 
-	return thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index].trip, cdev);
+	mutex_lock(&tz->lock);
+
+	ret = thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index].trip, cdev);
+
+	mutex_unlock(&tz->lock);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(thermal_zone_unbind_cooling_device);
 
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -609,7 +609,7 @@ static int acpi_thermal_bind_unbind_cdev
 		.thermal = thermal, .cdev = cdev, .bind = bind
 	};
 
-	return for_each_thermal_trip(thermal, bind_unbind_cdev_cb, &bd);
+	return thermal_zone_for_each_trip(thermal, bind_unbind_cdev_cb, &bd);
 }
 
 static int

[PATCH v1 08/17] thermal: core: Introduce .should_bind() thermal zone callback

[Rafael J. Wysocki]

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

The current design of the code binding cooling devices to trip points in
thermal zones is convoluted and hard to follow.

Namely, a driver that registers a thermal zone can provide .bind()
and .unbind() operations for it, which are required to call either
thermal_bind_cdev_to_trip() and thermal_unbind_cdev_from_trip(),
respectively, or thermal_zone_bind_cooling_device() and
thermal_zone_unbind_cooling_device(), respectively, for every relevant
trip point and the given cooling device.  Moreover, if .bind() is
provided and .unbind() is not, the cleanup necessary during the removal
of a thermal zone or a cooling device may not be carried out.

In other words, the core relies on the thermal zone owners to do the
right thing, which is error prone and far from obvious, even though all
of that is not really necessary.  Specifically, if the core could ask
the thermal zone owner, through a special thermal zone callback, whether
or not a given cooling device should be bound to a given trip point in
the given thermal zone, it might as well carry out all of the binding
and unbinding by itself.  In particular, the unbinding can be done
automatically without involving the thermal zone owner at all because
all of the thermal instances associated with a thermal zone or cooling
device going away must be deleted regardless.

Accordingly, introduce a new thermal zone operation, .should_bind(),
that can be invoked by the thermal core for a given thermal zone,
trip point and cooling device combination in order to check whether
or not the cooling device should be bound to the trip point at hand.
It takes an additional cooling_spec argument allowing the thermal
zone owner to specify the highest and lowest cooling states of the
cooling device and its weight for the given trip point binding.

Make the thermal core use this operation, if present, in the absence of
.bind() and .unbind().  Note that .should_bind() will be called under
the thermal zone lock.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.c |  106 +++++++++++++++++++++++++++++++----------
 include/linux/thermal.h        |   10 +++
 2 files changed, 92 insertions(+), 24 deletions(-)

Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -84,11 +84,21 @@ struct thermal_trip {
 
 struct thermal_zone_device;
 
+struct cooling_spec {
+	unsigned long upper;	/* Highest cooling state  */
+	unsigned long lower;	/* Lowest cooling state  */
+	unsigned int weight;	/* Cooling device weight */
+};
+
 struct thermal_zone_device_ops {
 	int (*bind) (struct thermal_zone_device *,
 		     struct thermal_cooling_device *);
 	int (*unbind) (struct thermal_zone_device *,
 		       struct thermal_cooling_device *);
+	bool (*should_bind) (struct thermal_zone_device *,
+			     const struct thermal_trip *,
+			     struct thermal_cooling_device *,
+			     struct cooling_spec *);
 	int (*get_temp) (struct thermal_zone_device *, int *);
 	int (*set_trips) (struct thermal_zone_device *, int, int);
 	int (*change_mode) (struct thermal_zone_device *,
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1000,12 +1000,61 @@ static struct class *thermal_class;
 
 static inline
 void print_bind_err_msg(struct thermal_zone_device *tz,
+			const struct thermal_trip *trip,
 			struct thermal_cooling_device *cdev, int ret)
 {
+	if (trip) {
+		dev_err(&tz->device, "binding cdev %s to trip %d failed: %d\n",
+			cdev->type, thermal_zone_trip_id(tz, trip), ret);
+		return;
+	}
+
 	dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
 		tz->type, cdev->type, ret);
 }
 
+static void thermal_zone_cdev_binding(struct thermal_zone_device *tz,
+				      struct thermal_cooling_device *cdev)
+{
+	struct thermal_trip_desc *td;
+	int ret;
+
+	/*
+	 * Old-style binding. The .bind() callback is expected to call
+	 * thermal_bind_cdev_to_trip() under the thermal zone lock.
+	 */
+	if (tz->ops.bind) {
+		ret = tz->ops.bind(tz, cdev);
+		if (ret)
+			print_bind_err_msg(tz, NULL, cdev, ret);
+
+		return;
+	}
+
+	if (!tz->ops.should_bind)
+		return;
+
+	mutex_lock(&tz->lock);
+
+	for_each_trip_desc(tz, td) {
+		struct thermal_trip *trip = &td->trip;
+		struct cooling_spec c = {
+			.upper = THERMAL_NO_LIMIT,
+			.lower = THERMAL_NO_LIMIT,
+			.weight = THERMAL_WEIGHT_DEFAULT
+		};
+
+		if (tz->ops.should_bind(tz, trip, cdev, &c)) {
+			ret = thermal_bind_cdev_to_trip(tz, trip, cdev, c.upper,
+							c.lower, c.weight);
+			if (ret)
+				print_bind_err_msg(tz, trip, cdev, ret);
+		}
+	}
+
+	mutex_unlock(&tz->lock);
+}
+
 /**
  * __thermal_cooling_device_register() - register a new thermal cooling device
  * @np:		a pointer to a device tree node.
@@ -1101,13 +1150,8 @@ __thermal_cooling_device_register(struct
 	list_add(&cdev->node, &thermal_cdev_list);
 
 	/* Update binding information for 'this' new cdev */
-	list_for_each_entry(pos, &thermal_tz_list, node) {
-		if (pos->ops.bind) {
-			ret = pos->ops.bind(pos, cdev);
-			if (ret)
-				print_bind_err_msg(pos, cdev, ret);
-		}
-	}
+	list_for_each_entry(pos, &thermal_tz_list, node)
+		thermal_zone_cdev_binding(pos, cdev);
 
 	list_for_each_entry(pos, &thermal_tz_list, node)
 		if (atomic_cmpxchg(&pos->need_update, 1, 0))
@@ -1308,6 +1352,28 @@ unlock_list:
 }
 EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
 
+static void thermal_zone_cdev_unbinding(struct thermal_zone_device *tz,
+					struct thermal_cooling_device *cdev)
+{
+	struct thermal_trip_desc *td;
+
+	/*
+	 * Old-style unbinding.  The .unbind callback is expected to call
+	 * thermal_unbind_cdev_from_trip() under the thermal zone lock.
+	 */
+	if (tz->ops.unbind) {
+		tz->ops.unbind(tz, cdev);
+		return;
+	}
+
+	mutex_lock(&tz->lock);
+
+	for_each_trip_desc(tz, td)
+		thermal_unbind_cdev_from_trip(tz, &td->trip, cdev);
+
+	mutex_unlock(&tz->lock);
+}
+
 /**
  * thermal_cooling_device_unregister - removes a thermal cooling device
  * @cdev:	the thermal cooling device to remove.
@@ -1334,10 +1400,8 @@ void thermal_cooling_device_unregister(s
 	list_del(&cdev->node);
 
 	/* Unbind all thermal zones associated with 'this' cdev */
-	list_for_each_entry(tz, &thermal_tz_list, node) {
-		if (tz->ops.unbind)
-			tz->ops.unbind(tz, cdev);
-	}
+	list_for_each_entry(tz, &thermal_tz_list, node)
+		thermal_zone_cdev_unbinding(tz, cdev);
 
 	mutex_unlock(&thermal_list_lock);
 
@@ -1412,6 +1476,7 @@ thermal_zone_device_register_with_trips(
 					unsigned int polling_delay)
 {
 	const struct thermal_trip *trip = trips;
+	struct thermal_cooling_device *cdev;
 	struct thermal_zone_device *tz;
 	struct thermal_trip_desc *td;
 	int id;
@@ -1434,8 +1499,9 @@ thermal_zone_device_register_with_trips(
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (!ops || !ops->get_temp) {
-		pr_err("Thermal zone device ops not defined\n");
+	if (!ops || !ops->get_temp || (ops->should_bind && ops->bind) ||
+	    (ops->should_bind && ops->unbind)) {
+		pr_err("Thermal zone device ops not defined or invalid\n");
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -1548,15 +1614,8 @@ thermal_zone_device_register_with_trips(
 	mutex_unlock(&tz->lock);
 
 	/* Bind cooling devices for this zone */
-	if (tz->ops.bind) {
-		struct thermal_cooling_device *cdev;
-
-		list_for_each_entry(cdev, &thermal_cdev_list, node) {
-			result = tz->ops.bind(tz, cdev);
-			if (result)
-				print_bind_err_msg(tz, cdev, result);
-		}
-	}
+	list_for_each_entry(cdev, &thermal_cdev_list, node)
+		thermal_zone_cdev_binding(tz, cdev);
 
 	mutex_unlock(&thermal_list_lock);
 
@@ -1650,8 +1709,7 @@ void thermal_zone_device_unregister(stru
 
 	/* Unbind all cdevs associated with 'this' thermal zone */
 	list_for_each_entry(cdev, &thermal_cdev_list, node)
-		if (tz->ops.unbind)
-			tz->ops.unbind(tz, cdev);
+		thermal_zone_cdev_unbinding(tz, cdev);
 
 	mutex_unlock(&thermal_list_lock);
 

[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 11/17] thermal: imx: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

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

Make the imx_thermal 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.

In the imx_thermal case, it only needs to return 'true' for the passive
trip point and it will match any cooling device passed to it, in
analogy with the old-style imx_bind() callback function.

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

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

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

Index: linux-pm/drivers/thermal/imx_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/imx_thermal.c
+++ linux-pm/drivers/thermal/imx_thermal.c
@@ -353,24 +353,16 @@ static int imx_set_trip_temp(struct ther
 	return 0;
 }
 
-static int imx_bind(struct thermal_zone_device *tz,
-		    struct thermal_cooling_device *cdev)
+static bool imx_should_bind(struct thermal_zone_device *tz,
+			    const struct thermal_trip *trip,
+			    struct thermal_cooling_device *cdev,
+			    struct cooling_spec *c)
 {
-	return thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
-						THERMAL_NO_LIMIT,
-						THERMAL_NO_LIMIT,
-						THERMAL_WEIGHT_DEFAULT);
-}
-
-static int imx_unbind(struct thermal_zone_device *tz,
-		      struct thermal_cooling_device *cdev)
-{
-	return thermal_zone_unbind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev);
+	return trip->type == THERMAL_TRIP_PASSIVE;
 }
 
 static struct thermal_zone_device_ops imx_tz_ops = {
-	.bind = imx_bind,
-	.unbind = imx_unbind,
+	.should_bind = imx_should_bind,
 	.get_temp = imx_get_temp,
 	.change_mode = imx_change_mode,
 	.set_trip_temp = imx_set_trip_temp,

[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);

[PATCH v1 13/17] mlxsw: core_thermal: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

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

Make the mlxsw core_thermal 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.

It replaces the .bind() and .unbind() thermal zone callbacks (in 3
places) which assumed the same trip points ordering in the driver
and in the thermal core (that may not be true any more in the
future).  The .bind() callbacks used loops over trip point indices
to call thermal_zone_bind_cooling_device() for the same cdev (once
it had been verified) and all of the trip points, but they passed
different 'upper' and 'lower' values to it for each trip.

To retain the original functionality, the .should_bind() callbacks
need to use the same 'upper' and 'lower' values that would be used
by the corresponding .bind() callbacks when they are about about to
return 'true'.  To that end, the 'priv' field of each trip is set
during the thermal zone initialization to point to the corresponding
'state' object containing the maximum and minimum cooling states of
the cooling device.

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

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

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c |  121 +++++----------------
 1 file changed, 34 insertions(+), 87 deletions(-)

Index: linux-pm/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
===================================================================
--- linux-pm.orig/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
+++ linux-pm/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
@@ -165,52 +165,22 @@ static int mlxsw_get_cooling_device_idx(
 	return -ENODEV;
 }
 
-static int mlxsw_thermal_bind(struct thermal_zone_device *tzdev,
-			      struct thermal_cooling_device *cdev)
+static bool mlxsw_thermal_should_bind(struct thermal_zone_device *tzdev,
+				      const struct thermal_trip *trip,
+				      struct thermal_cooling_device *cdev,
+				      struct cooling_spec *c)
 {
 	struct mlxsw_thermal *thermal = thermal_zone_device_priv(tzdev);
-	struct device *dev = thermal->bus_info->dev;
-	int i, err;
+	const struct mlxsw_cooling_states *state = trip->priv;
 
 	/* If the cooling device is one of ours bind it */
 	if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
-		return 0;
+		return false;
 
-	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
-		const struct mlxsw_cooling_states *state = &thermal->cooling_states[i];
+	c->upper = state->max_state;
+	c->lower = state->min_state;
 
-		err = thermal_zone_bind_cooling_device(tzdev, i, cdev,
-						       state->max_state,
-						       state->min_state,
-						       THERMAL_WEIGHT_DEFAULT);
-		if (err < 0) {
-			dev_err(dev, "Failed to bind cooling device to trip %d\n", i);
-			return err;
-		}
-	}
-	return 0;
-}
-
-static int mlxsw_thermal_unbind(struct thermal_zone_device *tzdev,
-				struct thermal_cooling_device *cdev)
-{
-	struct mlxsw_thermal *thermal = thermal_zone_device_priv(tzdev);
-	struct device *dev = thermal->bus_info->dev;
-	int i;
-	int err;
-
-	/* If the cooling device is our one unbind it */
-	if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
-		return 0;
-
-	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
-		err = thermal_zone_unbind_cooling_device(tzdev, i, cdev);
-		if (err < 0) {
-			dev_err(dev, "Failed to unbind cooling device\n");
-			return err;
-		}
-	}
-	return 0;
+	return true;
 }
 
 static int mlxsw_thermal_get_temp(struct thermal_zone_device *tzdev,
@@ -239,59 +209,29 @@ static struct thermal_zone_params mlxsw_
 	.no_hwmon = true,
 };
 
-static struct thermal_zone_device_ops mlxsw_thermal_ops = {
-	.bind = mlxsw_thermal_bind,
-	.unbind = mlxsw_thermal_unbind,
-	.get_temp = mlxsw_thermal_get_temp,
-};
-
-static int mlxsw_thermal_module_bind(struct thermal_zone_device *tzdev,
-				     struct thermal_cooling_device *cdev)
+static bool mlxsw_thermal_module_should_bind(struct thermal_zone_device *tzdev,
+					     const struct thermal_trip *trip,
+					     struct thermal_cooling_device *cdev,
+					     struct cooling_spec *c)
 {
 	struct mlxsw_thermal_module *tz = thermal_zone_device_priv(tzdev);
 	struct mlxsw_thermal *thermal = tz->parent;
-	int i, j, err;
+	const struct mlxsw_cooling_states *state = trip->priv;
 
 	/* If the cooling device is one of ours bind it */
 	if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
-		return 0;
+		return false;
 
-	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
-		const struct mlxsw_cooling_states *state = &tz->cooling_states[i];
+	c->upper = state->max_state;
+	c->lower = state->min_state;
 
-		err = thermal_zone_bind_cooling_device(tzdev, i, cdev,
-						       state->max_state,
-						       state->min_state,
-						       THERMAL_WEIGHT_DEFAULT);
-		if (err < 0)
-			goto err_thermal_zone_bind_cooling_device;
-	}
-	return 0;
-
-err_thermal_zone_bind_cooling_device:
-	for (j = i - 1; j >= 0; j--)
-		thermal_zone_unbind_cooling_device(tzdev, j, cdev);
-	return err;
+	return true;
 }
 
-static int mlxsw_thermal_module_unbind(struct thermal_zone_device *tzdev,
-				       struct thermal_cooling_device *cdev)
-{
-	struct mlxsw_thermal_module *tz = thermal_zone_device_priv(tzdev);
-	struct mlxsw_thermal *thermal = tz->parent;
-	int i;
-	int err;
-
-	/* If the cooling device is one of ours unbind it */
-	if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
-		return 0;
-
-	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
-		err = thermal_zone_unbind_cooling_device(tzdev, i, cdev);
-		WARN_ON(err);
-	}
-	return err;
-}
+static struct thermal_zone_device_ops mlxsw_thermal_ops = {
+	.should_bind = mlxsw_thermal_should_bind,
+	.get_temp = mlxsw_thermal_get_temp,
+};
 
 static int mlxsw_thermal_module_temp_get(struct thermal_zone_device *tzdev,
 					 int *p_temp)
@@ -313,8 +253,7 @@ static int mlxsw_thermal_module_temp_get
 }
 
 static struct thermal_zone_device_ops mlxsw_thermal_module_ops = {
-	.bind		= mlxsw_thermal_module_bind,
-	.unbind		= mlxsw_thermal_module_unbind,
+	.should_bind	= mlxsw_thermal_module_should_bind,
 	.get_temp	= mlxsw_thermal_module_temp_get,
 };
 
@@ -342,8 +281,7 @@ static int mlxsw_thermal_gearbox_temp_ge
 }
 
 static struct thermal_zone_device_ops mlxsw_thermal_gearbox_ops = {
-	.bind		= mlxsw_thermal_module_bind,
-	.unbind		= mlxsw_thermal_module_unbind,
+	.should_bind	= mlxsw_thermal_module_should_bind,
 	.get_temp	= mlxsw_thermal_gearbox_temp_get,
 };
 
@@ -451,6 +389,7 @@ mlxsw_thermal_module_init(struct device
 			  struct mlxsw_thermal_area *area, u8 module)
 {
 	struct mlxsw_thermal_module *module_tz;
+	int i;
 
 	module_tz = &area->tz_module_arr[module];
 	/* Skip if parent is already set (case of port split). */
@@ -465,6 +404,8 @@ mlxsw_thermal_module_init(struct device
 	       sizeof(thermal->trips));
 	memcpy(module_tz->cooling_states, default_cooling_states,
 	       sizeof(thermal->cooling_states));
+	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++)
+		module_tz->trips[i].priv = &module_tz->cooling_states[i];
 }
 
 static void mlxsw_thermal_module_fini(struct mlxsw_thermal_module *module_tz)
@@ -579,7 +520,7 @@ mlxsw_thermal_gearboxes_init(struct devi
 	struct mlxsw_thermal_module *gearbox_tz;
 	char mgpir_pl[MLXSW_REG_MGPIR_LEN];
 	u8 gbox_num;
-	int i;
+	int i, j;
 	int err;
 
 	mlxsw_reg_mgpir_pack(mgpir_pl, area->slot_index);
@@ -606,6 +547,9 @@ mlxsw_thermal_gearboxes_init(struct devi
 		       sizeof(thermal->trips));
 		memcpy(gearbox_tz->cooling_states, default_cooling_states,
 		       sizeof(thermal->cooling_states));
+		for (j = 0; j < MLXSW_THERMAL_NUM_TRIPS; j++)
+			gearbox_tz->trips[j].priv = &gearbox_tz->cooling_states[j];
+
 		gearbox_tz->module = i;
 		gearbox_tz->parent = thermal;
 		gearbox_tz->slot_index = area->slot_index;
@@ -722,6 +666,9 @@ int mlxsw_thermal_init(struct mlxsw_core
 	thermal->bus_info = bus_info;
 	memcpy(thermal->trips, default_thermal_trips, sizeof(thermal->trips));
 	memcpy(thermal->cooling_states, default_cooling_states, sizeof(thermal->cooling_states));
+	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++)
+		thermal->trips[i].priv = &thermal->cooling_states[i];
+
 	thermal->line_cards[0].slot_index = 0;
 
 	err = mlxsw_reg_query(thermal->core, MLXSW_REG(mfcr), mfcr_pl);

Re: [PATCH v1 13/17] mlxsw: core_thermal: Use the .should_bind() thermal zone callback

[Ido Schimmel]

On Tue, Jul 30, 2024 at 08:34:45PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make the mlxsw core_thermal 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.
> 
> It replaces the .bind() and .unbind() thermal zone callbacks (in 3
> places) which assumed the same trip points ordering in the driver
> and in the thermal core (that may not be true any more in the
> future).  The .bind() callbacks used loops over trip point indices
> to call thermal_zone_bind_cooling_device() for the same cdev (once
> it had been verified) and all of the trip points, but they passed
> different 'upper' and 'lower' values to it for each trip.
> 
> To retain the original functionality, the .should_bind() callbacks
> need to use the same 'upper' and 'lower' values that would be used
> by the corresponding .bind() callbacks when they are about about to

Nit: s/about about/about/

> return 'true'.  To that end, the 'priv' field of each trip is set
> during the thermal zone initialization to point to the corresponding
> 'state' object containing the maximum and minimum cooling states of
> the cooling device.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Please see more comments below, but this patch is going to conflict with
the series at [1] which is currently under review. How do you want to
handle that?

https://lore.kernel.org/netdev/cover.1722345311.git.petrm@nvidia.com/

> ---
> 
> This patch only depends on patch [09/17].
> 
> ---
>  drivers/net/ethernet/mellanox/mlxsw/core_thermal.c |  121 +++++----------------
>  1 file changed, 34 insertions(+), 87 deletions(-)
> 
> Index: linux-pm/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
> +++ linux-pm/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
> @@ -165,52 +165,22 @@ static int mlxsw_get_cooling_device_idx(
>  	return -ENODEV;
>  }
>  
> -static int mlxsw_thermal_bind(struct thermal_zone_device *tzdev,
> -			      struct thermal_cooling_device *cdev)
> +static bool mlxsw_thermal_should_bind(struct thermal_zone_device *tzdev,
> +				      const struct thermal_trip *trip,
> +				      struct thermal_cooling_device *cdev,
> +				      struct cooling_spec *c)
>  {
>  	struct mlxsw_thermal *thermal = thermal_zone_device_priv(tzdev);
> -	struct device *dev = thermal->bus_info->dev;
> -	int i, err;
> +	const struct mlxsw_cooling_states *state = trip->priv;
>  
>  	/* If the cooling device is one of ours bind it */
>  	if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> -		return 0;
> +		return false;
>  
> -	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> -		const struct mlxsw_cooling_states *state = &thermal->cooling_states[i];
> +	c->upper = state->max_state;
> +	c->lower = state->min_state;
>  
> -		err = thermal_zone_bind_cooling_device(tzdev, i, cdev,
> -						       state->max_state,
> -						       state->min_state,
> -						       THERMAL_WEIGHT_DEFAULT);
> -		if (err < 0) {
> -			dev_err(dev, "Failed to bind cooling device to trip %d\n", i);
> -			return err;
> -		}
> -	}
> -	return 0;
> -}
> -
> -static int mlxsw_thermal_unbind(struct thermal_zone_device *tzdev,
> -				struct thermal_cooling_device *cdev)
> -{
> -	struct mlxsw_thermal *thermal = thermal_zone_device_priv(tzdev);
> -	struct device *dev = thermal->bus_info->dev;
> -	int i;
> -	int err;
> -
> -	/* If the cooling device is our one unbind it */
> -	if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> -		return 0;
> -
> -	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> -		err = thermal_zone_unbind_cooling_device(tzdev, i, cdev);
> -		if (err < 0) {
> -			dev_err(dev, "Failed to unbind cooling device\n");
> -			return err;
> -		}
> -	}
> -	return 0;
> +	return true;
>  }
>  
>  static int mlxsw_thermal_get_temp(struct thermal_zone_device *tzdev,
> @@ -239,59 +209,29 @@ static struct thermal_zone_params mlxsw_
>  	.no_hwmon = true,
>  };
>  
> -static struct thermal_zone_device_ops mlxsw_thermal_ops = {
> -	.bind = mlxsw_thermal_bind,
> -	.unbind = mlxsw_thermal_unbind,
> -	.get_temp = mlxsw_thermal_get_temp,
> -};

Is there a reason to move 'mlxsw_thermal_ops' below?

> -
> -static int mlxsw_thermal_module_bind(struct thermal_zone_device *tzdev,
> -				     struct thermal_cooling_device *cdev)
> +static bool mlxsw_thermal_module_should_bind(struct thermal_zone_device *tzdev,
> +					     const struct thermal_trip *trip,
> +					     struct thermal_cooling_device *cdev,
> +					     struct cooling_spec *c)
>  {
>  	struct mlxsw_thermal_module *tz = thermal_zone_device_priv(tzdev);
>  	struct mlxsw_thermal *thermal = tz->parent;
> -	int i, j, err;
> +	const struct mlxsw_cooling_states *state = trip->priv;

Please place it between 'tz' and 'thermal'. Networking code tries to
maintain reverse xmas tree ordering for local variables.

>  
>  	/* If the cooling device is one of ours bind it */
>  	if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> -		return 0;
> +		return false;
>  
> -	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> -		const struct mlxsw_cooling_states *state = &tz->cooling_states[i];
> +	c->upper = state->max_state;
> +	c->lower = state->min_state;
>  
> -		err = thermal_zone_bind_cooling_device(tzdev, i, cdev,
> -						       state->max_state,
> -						       state->min_state,
> -						       THERMAL_WEIGHT_DEFAULT);
> -		if (err < 0)
> -			goto err_thermal_zone_bind_cooling_device;
> -	}
> -	return 0;
> -
> -err_thermal_zone_bind_cooling_device:
> -	for (j = i - 1; j >= 0; j--)
> -		thermal_zone_unbind_cooling_device(tzdev, j, cdev);
> -	return err;
> +	return true;
>  }
>  
> -static int mlxsw_thermal_module_unbind(struct thermal_zone_device *tzdev,
> -				       struct thermal_cooling_device *cdev)
> -{
> -	struct mlxsw_thermal_module *tz = thermal_zone_device_priv(tzdev);
> -	struct mlxsw_thermal *thermal = tz->parent;
> -	int i;
> -	int err;
> -
> -	/* If the cooling device is one of ours unbind it */
> -	if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> -		return 0;
> -
> -	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> -		err = thermal_zone_unbind_cooling_device(tzdev, i, cdev);
> -		WARN_ON(err);
> -	}
> -	return err;
> -}
> +static struct thermal_zone_device_ops mlxsw_thermal_ops = {
> +	.should_bind = mlxsw_thermal_should_bind,
> +	.get_temp = mlxsw_thermal_get_temp,
> +};
>  
>  static int mlxsw_thermal_module_temp_get(struct thermal_zone_device *tzdev,
>  					 int *p_temp)
> @@ -313,8 +253,7 @@ static int mlxsw_thermal_module_temp_get
>  }
>  
>  static struct thermal_zone_device_ops mlxsw_thermal_module_ops = {
> -	.bind		= mlxsw_thermal_module_bind,
> -	.unbind		= mlxsw_thermal_module_unbind,
> +	.should_bind	= mlxsw_thermal_module_should_bind,
>  	.get_temp	= mlxsw_thermal_module_temp_get,
>  };
>  
> @@ -342,8 +281,7 @@ static int mlxsw_thermal_gearbox_temp_ge
>  }
>  
>  static struct thermal_zone_device_ops mlxsw_thermal_gearbox_ops = {
> -	.bind		= mlxsw_thermal_module_bind,
> -	.unbind		= mlxsw_thermal_module_unbind,
> +	.should_bind	= mlxsw_thermal_module_should_bind,
>  	.get_temp	= mlxsw_thermal_gearbox_temp_get,
>  };
>  
> @@ -451,6 +389,7 @@ mlxsw_thermal_module_init(struct device
>  			  struct mlxsw_thermal_area *area, u8 module)
>  {
>  	struct mlxsw_thermal_module *module_tz;
> +	int i;
>  
>  	module_tz = &area->tz_module_arr[module];
>  	/* Skip if parent is already set (case of port split). */
> @@ -465,6 +404,8 @@ mlxsw_thermal_module_init(struct device
>  	       sizeof(thermal->trips));
>  	memcpy(module_tz->cooling_states, default_cooling_states,
>  	       sizeof(thermal->cooling_states));
> +	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++)
> +		module_tz->trips[i].priv = &module_tz->cooling_states[i];
>  }
>  
>  static void mlxsw_thermal_module_fini(struct mlxsw_thermal_module *module_tz)
> @@ -579,7 +520,7 @@ mlxsw_thermal_gearboxes_init(struct devi
>  	struct mlxsw_thermal_module *gearbox_tz;
>  	char mgpir_pl[MLXSW_REG_MGPIR_LEN];
>  	u8 gbox_num;
> -	int i;
> +	int i, j;
>  	int err;
>  
>  	mlxsw_reg_mgpir_pack(mgpir_pl, area->slot_index);
> @@ -606,6 +547,9 @@ mlxsw_thermal_gearboxes_init(struct devi
>  		       sizeof(thermal->trips));
>  		memcpy(gearbox_tz->cooling_states, default_cooling_states,
>  		       sizeof(thermal->cooling_states));
> +		for (j = 0; j < MLXSW_THERMAL_NUM_TRIPS; j++)
> +			gearbox_tz->trips[j].priv = &gearbox_tz->cooling_states[j];
> +
>  		gearbox_tz->module = i;
>  		gearbox_tz->parent = thermal;
>  		gearbox_tz->slot_index = area->slot_index;
> @@ -722,6 +666,9 @@ int mlxsw_thermal_init(struct mlxsw_core
>  	thermal->bus_info = bus_info;
>  	memcpy(thermal->trips, default_thermal_trips, sizeof(thermal->trips));
>  	memcpy(thermal->cooling_states, default_cooling_states, sizeof(thermal->cooling_states));
> +	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++)
> +		thermal->trips[i].priv = &thermal->cooling_states[i];
> +
>  	thermal->line_cards[0].slot_index = 0;
>  
>  	err = mlxsw_reg_query(thermal->core, MLXSW_REG(mfcr), mfcr_pl);
> 
> 
> 

Re: [PATCH v1 13/17] mlxsw: core_thermal: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

On Wed, Jul 31, 2024 at 2:43 PM Ido Schimmel <idosch@nvidia.com> wrote:
>
> On Tue, Jul 30, 2024 at 08:34:45PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Make the mlxsw core_thermal 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.
> >
> > It replaces the .bind() and .unbind() thermal zone callbacks (in 3
> > places) which assumed the same trip points ordering in the driver
> > and in the thermal core (that may not be true any more in the
> > future).  The .bind() callbacks used loops over trip point indices
> > to call thermal_zone_bind_cooling_device() for the same cdev (once
> > it had been verified) and all of the trip points, but they passed
> > different 'upper' and 'lower' values to it for each trip.
> >
> > To retain the original functionality, the .should_bind() callbacks
> > need to use the same 'upper' and 'lower' values that would be used
> > by the corresponding .bind() callbacks when they are about about to
>
> Nit: s/about about/about/

Yes, thanks!

> > return 'true'.  To that end, the 'priv' field of each trip is set
> > during the thermal zone initialization to point to the corresponding
> > 'state' object containing the maximum and minimum cooling states of
> > the cooling device.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Please see more comments below, but this patch is going to conflict with
> the series at [1] which is currently under review. How do you want to
> handle that?
>
> https://lore.kernel.org/netdev/cover.1722345311.git.petrm@nvidia.com/

I may be missing something, but I don't see conflicts between this
patch and the series above that would be hard to resolve at merge
time.

Anyway, I'll try to apply the above series locally and merge it with
this patch, thanks for the heads up!

> > ---
> >
> > This patch only depends on patch [09/17].
> >
> > ---
> >  drivers/net/ethernet/mellanox/mlxsw/core_thermal.c |  121 +++++----------------
> >  1 file changed, 34 insertions(+), 87 deletions(-)
> >
> > Index: linux-pm/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
> > ===================================================================
> > --- linux-pm.orig/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
> > +++ linux-pm/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
> > @@ -165,52 +165,22 @@ static int mlxsw_get_cooling_device_idx(
> >       return -ENODEV;
> >  }
> >
> > -static int mlxsw_thermal_bind(struct thermal_zone_device *tzdev,
> > -                           struct thermal_cooling_device *cdev)
> > +static bool mlxsw_thermal_should_bind(struct thermal_zone_device *tzdev,
> > +                                   const struct thermal_trip *trip,
> > +                                   struct thermal_cooling_device *cdev,
> > +                                   struct cooling_spec *c)
> >  {
> >       struct mlxsw_thermal *thermal = thermal_zone_device_priv(tzdev);
> > -     struct device *dev = thermal->bus_info->dev;
> > -     int i, err;
> > +     const struct mlxsw_cooling_states *state = trip->priv;
> >
> >       /* If the cooling device is one of ours bind it */
> >       if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> > -             return 0;
> > +             return false;
> >
> > -     for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> > -             const struct mlxsw_cooling_states *state = &thermal->cooling_states[i];
> > +     c->upper = state->max_state;
> > +     c->lower = state->min_state;
> >
> > -             err = thermal_zone_bind_cooling_device(tzdev, i, cdev,
> > -                                                    state->max_state,
> > -                                                    state->min_state,
> > -                                                    THERMAL_WEIGHT_DEFAULT);
> > -             if (err < 0) {
> > -                     dev_err(dev, "Failed to bind cooling device to trip %d\n", i);
> > -                     return err;
> > -             }
> > -     }
> > -     return 0;
> > -}
> > -
> > -static int mlxsw_thermal_unbind(struct thermal_zone_device *tzdev,
> > -                             struct thermal_cooling_device *cdev)
> > -{
> > -     struct mlxsw_thermal *thermal = thermal_zone_device_priv(tzdev);
> > -     struct device *dev = thermal->bus_info->dev;
> > -     int i;
> > -     int err;
> > -
> > -     /* If the cooling device is our one unbind it */
> > -     if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> > -             return 0;
> > -
> > -     for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> > -             err = thermal_zone_unbind_cooling_device(tzdev, i, cdev);
> > -             if (err < 0) {
> > -                     dev_err(dev, "Failed to unbind cooling device\n");
> > -                     return err;
> > -             }
> > -     }
> > -     return 0;
> > +     return true;
> >  }
> >
> >  static int mlxsw_thermal_get_temp(struct thermal_zone_device *tzdev,
> > @@ -239,59 +209,29 @@ static struct thermal_zone_params mlxsw_
> >       .no_hwmon = true,
> >  };
> >
> > -static struct thermal_zone_device_ops mlxsw_thermal_ops = {
> > -     .bind = mlxsw_thermal_bind,
> > -     .unbind = mlxsw_thermal_unbind,
> > -     .get_temp = mlxsw_thermal_get_temp,
> > -};
>
> Is there a reason to move 'mlxsw_thermal_ops' below?

Not really, it can stay here.

> > -
> > -static int mlxsw_thermal_module_bind(struct thermal_zone_device *tzdev,
> > -                                  struct thermal_cooling_device *cdev)
> > +static bool mlxsw_thermal_module_should_bind(struct thermal_zone_device *tzdev,
> > +                                          const struct thermal_trip *trip,
> > +                                          struct thermal_cooling_device *cdev,
> > +                                          struct cooling_spec *c)
> >  {
> >       struct mlxsw_thermal_module *tz = thermal_zone_device_priv(tzdev);
> >       struct mlxsw_thermal *thermal = tz->parent;
> > -     int i, j, err;
> > +     const struct mlxsw_cooling_states *state = trip->priv;
>
> Please place it between 'tz' and 'thermal'. Networking code tries to
> maintain reverse xmas tree ordering for local variables.

I will.

Thanks for the comments!

Re: [PATCH v1 13/17] mlxsw: core_thermal: Use the .should_bind() thermal zone callback

[Rafael J. Wysocki]

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

On Wed, Jul 31, 2024 at 3:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jul 31, 2024 at 2:43 PM Ido Schimmel <idosch@nvidia.com> wrote:
> >
> > On Tue, Jul 30, 2024 at 08:34:45PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Make the mlxsw core_thermal 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.
> > >
> > > It replaces the .bind() and .unbind() thermal zone callbacks (in 3
> > > places) which assumed the same trip points ordering in the driver
> > > and in the thermal core (that may not be true any more in the
> > > future).  The .bind() callbacks used loops over trip point indices
> > > to call thermal_zone_bind_cooling_device() for the same cdev (once
> > > it had been verified) and all of the trip points, but they passed
> > > different 'upper' and 'lower' values to it for each trip.
> > >
> > > To retain the original functionality, the .should_bind() callbacks
> > > need to use the same 'upper' and 'lower' values that would be used
> > > by the corresponding .bind() callbacks when they are about about to
> >
> > Nit: s/about about/about/
>
> Yes, thanks!
>
> > > return 'true'.  To that end, the 'priv' field of each trip is set
> > > during the thermal zone initialization to point to the corresponding
> > > 'state' object containing the maximum and minimum cooling states of
> > > the cooling device.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Please see more comments below, but this patch is going to conflict with
> > the series at [1] which is currently under review. How do you want to
> > handle that?
> >
> > https://lore.kernel.org/netdev/cover.1722345311.git.petrm@nvidia.com/
>
> I may be missing something, but I don't see conflicts between this
> patch and the series above that would be hard to resolve at merge
> time.
>
> Anyway, I'll try to apply the above series locally and merge it with
> this patch, thanks for the heads up!

So there is only one merge conflict that's straightforward to resolve
(as far as I'm concerned).  My resolution of it is attached, FWIW.

In my view the changes in the series above and this patch are mostly
independent of each other.

Thanks!

[-- Attachment #2: merge.patch --]
[-- Type: text/x-patch, Size: 1076 bytes --]

diff --cc drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
index 303d2ce4dc1e,0c50a0cc316d..e746cd9c68ed
--- a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
@@@ -450,8 -389,12 +388,9 @@@ mlxsw_thermal_module_init(struct mlxsw_
  			  struct mlxsw_thermal_area *area, u8 module)
  {
  	struct mlxsw_thermal_module *module_tz;
+ 	int i;
  
  	module_tz = &area->tz_module_arr[module];
 -	/* Skip if parent is already set (case of port split). */
 -	if (module_tz->parent)
 -		return;
  	module_tz->module = module;
  	module_tz->slot_index = area->slot_index;
  	module_tz->parent = thermal;
@@@ -461,8 -404,8 +400,10 @@@
  	       sizeof(thermal->trips));
  	memcpy(module_tz->cooling_states, default_cooling_states,
  	       sizeof(thermal->cooling_states));
+ 	for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++)
+ 		module_tz->trips[i].priv = &module_tz->cooling_states[i];
 +
 +	return mlxsw_thermal_module_tz_init(module_tz);
  }
  
  static void mlxsw_thermal_module_fini(struct mlxsw_thermal_module *module_tz)

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

[Rafael J. Wysocki]

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

Make the thermal_of 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
assumed the same trip points ordering in the driver and in the thermal
core (that may not be true any more in the future).  The .bind()
callback would walk the given thermal zone's cooling maps to find all
of the valid trip point combinations with the given cooling device and
it would call thermal_zone_bind_cooling_device() for all of them using
trip point indices reflecting the ordering of the trips in the DT.

The .should_bind() callback still walks the thermal zone's cooling maps,
but it can use the trip object passed to it by the thermal core to find
the trip in question in the first place and then it uses the
corresponding 'cooling-device' entries to look up the given cooling
device.  To be able to match the trip object provided by the thermal
core to a specific device node, the driver sets the 'priv' field of each
trip to the corresponding device node pointer during initialization.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_of.c |  171 ++++++++++---------------------------------
 1 file changed, 41 insertions(+), 130 deletions(-)

Index: linux-pm/drivers/thermal/thermal_of.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_of.c
+++ linux-pm/drivers/thermal/thermal_of.c
@@ -20,37 +20,6 @@
 
 /***   functions parsing device tree nodes   ***/
 
-static int of_find_trip_id(struct device_node *np, struct device_node *trip)
-{
-	struct device_node *trips;
-	struct device_node *t;
-	int i = 0;
-
-	trips = of_get_child_by_name(np, "trips");
-	if (!trips) {
-		pr_err("Failed to find 'trips' node\n");
-		return -EINVAL;
-	}
-
-	/*
-	 * Find the trip id point associated with the cooling device map
-	 */
-	for_each_child_of_node(trips, t) {
-
-		if (t == trip) {
-			of_node_put(t);
-			goto out;
-		}
-		i++;
-	}
-
-	i = -ENXIO;
-out:
-	of_node_put(trips);
-
-	return i;
-}
-
 /*
  * It maps 'enum thermal_trip_type' found in include/linux/thermal.h
  * into the device tree binding of 'trip', property type.
@@ -119,6 +88,8 @@ static int thermal_of_populate_trip(stru
 
 	trip->flags = THERMAL_TRIP_FLAG_RW_TEMP;
 
+	trip->priv = np;
+
 	return 0;
 }
 
@@ -290,39 +261,9 @@ static struct device_node *thermal_of_zo
 	return tz_np;
 }
 
-static int __thermal_of_unbind(struct device_node *map_np, int index, int trip_id,
-			       struct thermal_zone_device *tz, struct thermal_cooling_device *cdev)
-{
-	struct of_phandle_args cooling_spec;
-	int ret;
-
-	ret = of_parse_phandle_with_args(map_np, "cooling-device", "#cooling-cells",
-					 index, &cooling_spec);
-
-	if (ret < 0) {
-		pr_err("Invalid cooling-device entry\n");
-		return ret;
-	}
-
-	of_node_put(cooling_spec.np);
-
-	if (cooling_spec.args_count < 2) {
-		pr_err("wrong reference to cooling device, missing limits\n");
-		return -EINVAL;
-	}
-
-	if (cooling_spec.np != cdev->np)
-		return 0;
-
-	ret = thermal_zone_unbind_cooling_device(tz, trip_id, cdev);
-	if (ret)
-		pr_err("Failed to unbind '%s' with '%s': %d\n", tz->type, cdev->type, ret);
-
-	return ret;
-}
-
-static int __thermal_of_bind(struct device_node *map_np, int index, int trip_id,
-			     struct thermal_zone_device *tz, struct thermal_cooling_device *cdev)
+static bool thermal_of_get_cooling_spec(struct device_node *map_np, int index,
+					struct thermal_cooling_device *cdev,
+					struct cooling_spec *c)
 {
 	struct of_phandle_args cooling_spec;
 	int ret, weight = THERMAL_WEIGHT_DEFAULT;
@@ -334,104 +275,75 @@ static int __thermal_of_bind(struct devi
 
 	if (ret < 0) {
 		pr_err("Invalid cooling-device entry\n");
-		return ret;
+		return false;
 	}
 
 	of_node_put(cooling_spec.np);
 
 	if (cooling_spec.args_count < 2) {
 		pr_err("wrong reference to cooling device, missing limits\n");
-		return -EINVAL;
+		return false;
 	}
 
 	if (cooling_spec.np != cdev->np)
-		return 0;
-
-	ret = thermal_zone_bind_cooling_device(tz, trip_id, cdev, cooling_spec.args[1],
-					       cooling_spec.args[0],
-					       weight);
-	if (ret)
-		pr_err("Failed to bind '%s' with '%s': %d\n", tz->type, cdev->type, ret);
-
-	return ret;
-}
-
-static int thermal_of_for_each_cooling_device(struct device_node *tz_np, struct device_node *map_np,
-					      struct thermal_zone_device *tz, struct thermal_cooling_device *cdev,
-					      int (*action)(struct device_node *, int, int,
-							    struct thermal_zone_device *, struct thermal_cooling_device *))
-{
-	struct device_node *tr_np;
-	int count, i, trip_id;
-
-	tr_np = of_parse_phandle(map_np, "trip", 0);
-	if (!tr_np)
-		return -ENODEV;
-
-	trip_id = of_find_trip_id(tz_np, tr_np);
-	if (trip_id < 0)
-		return trip_id;
-
-	count = of_count_phandle_with_args(map_np, "cooling-device", "#cooling-cells");
-	if (count <= 0) {
-		pr_err("Add a cooling_device property with at least one device\n");
-		return -ENOENT;
-	}
+		return false;
 
-	/*
-	 * At this point, we don't want to bail out when there is an
-	 * error, we will try to bind/unbind as many as possible
-	 * cooling devices
-	 */
-	for (i = 0; i < count; i++)
-		action(map_np, i, trip_id, tz, cdev);
+	c->lower = cooling_spec.args[0];
+	c->upper = cooling_spec.args[1];
+	c->weight = weight;
 
-	return 0;
+	return true;
 }
 
-static int thermal_of_for_each_cooling_maps(struct thermal_zone_device *tz,
-					    struct thermal_cooling_device *cdev,
-					    int (*action)(struct device_node *, int, int,
-							  struct thermal_zone_device *, struct thermal_cooling_device *))
+static bool thermal_of_should_bind(struct thermal_zone_device *tz,
+				   const struct thermal_trip *trip,
+				   struct thermal_cooling_device *cdev,
+				   struct cooling_spec *c)
 {
 	struct device_node *tz_np, *cm_np, *child;
-	int ret = 0;
+	bool result = false;
 
 	tz_np = thermal_of_zone_get_by_name(tz);
 	if (IS_ERR(tz_np)) {
 		pr_err("Failed to get node tz by name\n");
-		return PTR_ERR(tz_np);
+		return false;
 	}
 
 	cm_np = of_get_child_by_name(tz_np, "cooling-maps");
 	if (!cm_np)
 		goto out;
 
+	/* Look up the trip and the cdev in the cooling maps. */
 	for_each_child_of_node(cm_np, child) {
-		ret = thermal_of_for_each_cooling_device(tz_np, child, tz, cdev, action);
-		if (ret) {
+		struct device_node *tr_np;
+		int count, i;
+
+		tr_np = of_parse_phandle(map_np, "trip", 0);
+		if (tr_np != trip->priv) {
 			of_node_put(child);
-			break;
+			continue;
+		}
+
+		/* The trip has been found, look up the cdev. */
+		count = of_count_phandle_with_args(child, "cooling-device", "#cooling-cells");
+		if (count <= 0)
+			pr_err("Add a cooling_device property with at least one device\n");
+
+		for (i = 0; i < count; i++) {
+			result = thermal_of_get_cooling_spec(child, i, cdev, c);
+			if (result)
+				break;
 		}
+
+		of_node_put(child);
+		break;
 	}
 
 	of_node_put(cm_np);
 out:
 	of_node_put(tz_np);
 
-	return ret;
-}
-
-static int thermal_of_bind(struct thermal_zone_device *tz,
-			   struct thermal_cooling_device *cdev)
-{
-	return thermal_of_for_each_cooling_maps(tz, cdev, __thermal_of_bind);
-}
-
-static int thermal_of_unbind(struct thermal_zone_device *tz,
-			     struct thermal_cooling_device *cdev)
-{
-	return thermal_of_for_each_cooling_maps(tz, cdev, __thermal_of_unbind);
+	return result;
 }
 
 /**
@@ -502,8 +414,7 @@ static struct thermal_zone_device *therm
 
 	thermal_of_parameters_init(np, &tzp);
 
-	of_ops.bind = thermal_of_bind;
-	of_ops.unbind = thermal_of_unbind;
+	of_ops.should_bind = thermal_of_should_bind;
 
 	ret = of_property_read_string(np, "critical-action", &action);
 	if (!ret)

[PATCH v1 16/17] thermal: code: Clean up trip bind/unbind functions

[Rafael J. Wysocki]

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

Make thermal_bind_cdev_to_trip() take a struct cooling_spec pointer
to reduce the number of its arguments, change the return type of
thermal_unbind_cdev_from_trip() to void and rearrange the code in
thermal_zone_cdev_binding() to reduce the indentation level.

No intentional functional impact.

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

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -761,15 +761,7 @@ struct thermal_zone_device *thermal_zone
  * @tz:		pointer to struct thermal_zone_device
  * @trip:	trip point the cooling devices is associated with in this zone.
  * @cdev:	pointer to struct thermal_cooling_device
- * @upper:	the Maximum cooling state for this trip point.
- *		THERMAL_NO_LIMIT means no upper limit,
- *		and the cooling device can be in max_state.
- * @lower:	the Minimum cooling state can be used for this trip point.
- *		THERMAL_NO_LIMIT means no lower limit,
- *		and the cooling device can be in cooling state 0.
- * @weight:	The weight of the cooling device to be bound to the
- *		thermal zone. Use THERMAL_WEIGHT_DEFAULT for the
- *		default value
+ * @c:		cooling specification for @trip and @cdev
  *
  * This interface function bind a thermal cooling device to the certain trip
  * point of a thermal zone device.
@@ -780,8 +772,7 @@ struct thermal_zone_device *thermal_zone
 static int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
 				     struct thermal_trip *trip,
 				     struct thermal_cooling_device *cdev,
-				     unsigned long upper, unsigned long lower,
-				     unsigned int weight)
+				     struct cooling_spec *c)
 {
 	struct thermal_trip_desc *td = trip_to_trip_desc(trip);
 	struct thermal_instance *dev, *instance;
@@ -795,17 +786,17 @@ static int thermal_bind_cdev_to_trip(str
 		return -EINVAL;
 
 	/* lower default 0, upper default max_state */
-	if (lower == THERMAL_NO_LIMIT)
-		lower = 0;
+	if (c->lower == THERMAL_NO_LIMIT)
+		c->lower = 0;
 
-	if (upper == THERMAL_NO_LIMIT) {
-		upper = cdev->max_state;
+	if (c->upper == THERMAL_NO_LIMIT) {
+		c->upper = cdev->max_state;
 		upper_no_limit = true;
 	} else {
 		upper_no_limit = false;
 	}
 
-	if (lower > upper || upper > cdev->max_state)
+	if (c->lower > c->upper || c->upper > cdev->max_state)
 		return -EINVAL;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
@@ -815,11 +806,11 @@ static int thermal_bind_cdev_to_trip(str
 	dev->tz = tz;
 	dev->cdev = cdev;
 	dev->trip = trip;
-	dev->upper = upper;
+	dev->upper = c->upper;
 	dev->upper_no_limit = upper_no_limit;
-	dev->lower = lower;
+	dev->lower = c->lower;
 	dev->target = THERMAL_NO_TARGET;
-	dev->weight = weight;
+	dev->weight = c->weight;
 
 	result = ida_alloc(&tz->ida, GFP_KERNEL);
 	if (result < 0)
@@ -893,12 +884,10 @@ free_mem:
  * This interface function unbind a thermal cooling device from the certain
  * trip point of a thermal zone device.
  * This function is usually called in the thermal zone device .unbind callback.
- *
- * Return: 0 on success, the proper error value otherwise.
  */
-static int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
-					 struct thermal_trip *trip,
-					 struct thermal_cooling_device *cdev)
+static void thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
+					  struct thermal_trip *trip,
+					  struct thermal_cooling_device *cdev)
 {
 	struct thermal_trip_desc *td = trip_to_trip_desc(trip);
 	struct thermal_instance *pos, *next;
@@ -921,7 +910,7 @@ static int thermal_unbind_cdev_from_trip
 
 	mutex_unlock(&cdev->lock);
 
-	return -ENODEV;
+	return;
 
 free:
 	device_remove_file(&tz->device, &pos->weight_attr);
@@ -929,7 +918,6 @@ free:
 	sysfs_remove_link(&tz->device.kobj, pos->name);
 	ida_free(&tz->ida, pos->id);
 	kfree(pos);
-	return 0;
 }
 
 static void thermal_release(struct device *dev)
@@ -968,7 +956,6 @@ static void thermal_zone_cdev_binding(st
 				      struct thermal_cooling_device *cdev)
 {
 	struct thermal_trip_desc *td;
-	int ret;
 
 	if (!tz->ops.should_bind)
 		return;
@@ -982,13 +969,14 @@ static void thermal_zone_cdev_binding(st
 			.lower = THERMAL_NO_LIMIT,
 			.weight = THERMAL_WEIGHT_DEFAULT
 		};
+		int ret;
 
-		if (tz->ops.should_bind(tz, trip, cdev, &c)) {
-			ret = thermal_bind_cdev_to_trip(tz, trip, cdev, c.upper,
-							c.lower, c.weight);
-			if (ret)
-				print_bind_err_msg(tz, trip, cdev, ret);
-		}
+		if (!tz->ops.should_bind(tz, trip, cdev, &c))
+			continue;
+
+		ret = thermal_bind_cdev_to_trip(tz, trip, cdev, &c);
+		if (ret)
+			print_bind_err_msg(tz, trip, cdev, ret);
 	}
 
 	mutex_unlock(&tz->lock);

[PATCH v1 02/17] thermal: core: Rearrange checks in thermal_bind_cdev_to_trip()

[Rafael J. Wysocki]

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

It is not necessary to look up the thermal zone and the cooling device
in the respective global lists to check whether or not they are
registered.  It is sufficient to check whether or not their respective
list nodes are empty for this purpose.

Use the above observation to simplify thermal_bind_cdev_to_trip().  In
addition, eliminate an unnecessary ternary operator from it.

Moreover, add lockdep_assert_held() for thermal_list_lock to it because
that lock must be held by its callers when it is running.

No intentional functional impact.

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

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -781,25 +781,17 @@ int thermal_bind_cdev_to_trip(struct the
 {
 	struct thermal_instance *dev;
 	struct thermal_instance *pos;
-	struct thermal_zone_device *pos1;
-	struct thermal_cooling_device *pos2;
 	bool upper_no_limit;
 	int result;
 
-	list_for_each_entry(pos1, &thermal_tz_list, node) {
-		if (pos1 == tz)
-			break;
-	}
-	list_for_each_entry(pos2, &thermal_cdev_list, node) {
-		if (pos2 == cdev)
-			break;
-	}
+	lockdep_assert_held(&thermal_list_lock);
 
-	if (tz != pos1 || cdev != pos2)
+	if (list_empty(&tz->node) || list_empty(&cdev->node))
 		return -EINVAL;
 
 	/* lower default 0, upper default max_state */
-	lower = lower == THERMAL_NO_LIMIT ? 0 : lower;
+	if (lower == THERMAL_NO_LIMIT)
+		lower = 0;
 
 	if (upper == THERMAL_NO_LIMIT) {
 		upper = cdev->max_state;

[PATCH v1 10/17] thermal: core: Unexport thermal_bind_cdev_to_trip() and thermal_unbind_cdev_from_trip()

[Rafael J. Wysocki]

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

Since thermal_bind_cdev_to_trip() and thermal_unbind_cdev_from_trip()
are only called locally in the thermal core now, they can be static,
so change their definitions accordingly and drop their headers from
the global thermal header file.

No intentional functional impact.

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

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -777,7 +777,7 @@ struct thermal_zone_device *thermal_zone
  *
  * Return: 0 on success, the proper error value otherwise.
  */
-int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
+static int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
 				     struct thermal_trip *trip,
 				     struct thermal_cooling_device *cdev,
 				     unsigned long upper, unsigned long lower,
@@ -883,7 +883,6 @@ free_mem:
 	kfree(dev);
 	return result;
 }
-EXPORT_SYMBOL_GPL(thermal_bind_cdev_to_trip);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 				     int trip_index,
@@ -919,9 +918,9 @@ EXPORT_SYMBOL_GPL(thermal_zone_bind_cool
  *
  * Return: 0 on success, the proper error value otherwise.
  */
-int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
-				  struct thermal_trip *trip,
-				  struct thermal_cooling_device *cdev)
+static int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
+					 struct thermal_trip *trip,
+					 struct thermal_cooling_device *cdev)
 {
 	struct thermal_trip_desc *td = trip_to_trip_desc(trip);
 	struct thermal_instance *pos, *next;
@@ -954,7 +953,6 @@ free:
 	kfree(pos);
 	return 0;
 }
-EXPORT_SYMBOL_GPL(thermal_unbind_cdev_from_trip);
 
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
 				       int trip_index,
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -246,18 +246,10 @@ const char *thermal_zone_device_type(str
 int thermal_zone_device_id(struct thermal_zone_device *tzd);
 struct device *thermal_zone_device(struct thermal_zone_device *tzd);
 
-int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
-			      struct thermal_trip *trip,
-			      struct thermal_cooling_device *cdev,
-			      unsigned long upper, unsigned long lower,
-			      unsigned int weight);
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
 				     struct thermal_cooling_device *,
 				     unsigned long, unsigned long,
 				     unsigned int);
-int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
-				  struct thermal_trip *trip,
-				  struct thermal_cooling_device *cdev);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
 				       struct thermal_cooling_device *);
 void thermal_zone_device_update(struct thermal_zone_device *,

[PATCH v1 15/17] thermal: core: Drop unused bind/unbind functions and callbacks

[Rafael J. Wysocki]

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

There are no more callers of thermal_zone_bind_cooling_device() and
thermal_zone_unbind_cooling_device(), so drop them along with all of
the corresponding headers, code and documentation.

Moreover, because the .bind() and .unbind() thermal zone callbacks would
only be used when the above functions, respectively, were called, drop
them as well along with all of the code related to them.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/driver-api/thermal/sysfs-api.rst |   52 -----------------
 drivers/thermal/thermal_core.c                 |   75 +------------------------
 include/linux/thermal.h                        |   10 ---
 3 files changed, 3 insertions(+), 134 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -884,28 +884,6 @@ free_mem:
 	return result;
 }
 
-int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
-				     int trip_index,
-				     struct thermal_cooling_device *cdev,
-				     unsigned long upper, unsigned long lower,
-				     unsigned int weight)
-{
-	int ret;
-
-	if (trip_index < 0 || trip_index >= tz->num_trips)
-		return -EINVAL;
-
-	mutex_lock(&tz->lock);
-
-	ret = thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index].trip, cdev,
-					upper, lower, weight);
-
-	mutex_unlock(&tz->lock);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_bind_cooling_device);
-
 /**
  * thermal_unbind_cdev_from_trip - unbind a cooling device from a thermal zone.
  * @tz:		pointer to a struct thermal_zone_device.
@@ -954,25 +932,6 @@ free:
 	return 0;
 }
 
-int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
-				       int trip_index,
-				       struct thermal_cooling_device *cdev)
-{
-	int ret;
-
-	if (trip_index < 0 || trip_index >= tz->num_trips)
-		return -EINVAL;
-
-	mutex_lock(&tz->lock);
-
-	ret = thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index].trip, cdev);
-
-	mutex_unlock(&tz->lock);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_unbind_cooling_device);
-
 static void thermal_release(struct device *dev)
 {
 	struct thermal_zone_device *tz;
@@ -1001,14 +960,8 @@ void print_bind_err_msg(struct thermal_z
 			const struct thermal_trip *trip,
 			struct thermal_cooling_device *cdev, int ret)
 {
-	if (trip) {
-		dev_err(&tz->device, "binding cdev %s to trip %d failed: %d\n",
-			cdev->type, thermal_zone_trip_id(tz, trip), ret);
-		return;
-	}
-
-	dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
-		tz->type, cdev->type, ret);
+	dev_err(&tz->device, "binding cdev %s to trip %d failed: %d\n",
+		cdev->type, thermal_zone_trip_id(tz, trip), ret);
 }
 
 static void thermal_zone_cdev_binding(struct thermal_zone_device *tz,
@@ -1017,18 +970,6 @@ static void thermal_zone_cdev_binding(st
 	struct thermal_trip_desc *td;
 	int ret;
 
-	/*
-	 * Old-style binding. The .bind() callback is expected to call
-	 * thermal_bind_cdev_to_trip() under the thermal zone lock.
-	 */
-	if (tz->ops.bind) {
-		ret = tz->ops.bind(tz, cdev);
-		if (ret)
-			print_bind_err_msg(tz, NULL, cdev, ret);
-
-		return;
-	}
-
 	if (!tz->ops.should_bind)
 		return;
 
@@ -1355,15 +1296,6 @@ static void thermal_zone_cdev_unbinding(
 {
 	struct thermal_trip_desc *td;
 
-	/*
-	 * Old-style unbinding.  The .unbind callback is expected to call
-	 * thermal_unbind_cdev_from_trip() under the thermal zone lock.
-	 */
-	if (tz->ops.unbind) {
-		tz->ops.unbind(tz, cdev);
-		return;
-	}
-
 	mutex_lock(&tz->lock);
 
 	for_each_trip_desc(tz, td)
@@ -1497,8 +1429,7 @@ thermal_zone_device_register_with_trips(
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (!ops || !ops->get_temp || (ops->should_bind && ops->bind) ||
-	    (ops->should_bind && ops->unbind)) {
+	if (!ops || !ops->get_temp) {
 		pr_err("Thermal zone device ops not defined or invalid\n");
 		return ERR_PTR(-EINVAL);
 	}
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -91,10 +91,6 @@ struct cooling_spec {
 };
 
 struct thermal_zone_device_ops {
-	int (*bind) (struct thermal_zone_device *,
-		     struct thermal_cooling_device *);
-	int (*unbind) (struct thermal_zone_device *,
-		       struct thermal_cooling_device *);
 	bool (*should_bind) (struct thermal_zone_device *,
 			     const struct thermal_trip *,
 			     struct thermal_cooling_device *,
@@ -246,12 +242,6 @@ const char *thermal_zone_device_type(str
 int thermal_zone_device_id(struct thermal_zone_device *tzd);
 struct device *thermal_zone_device(struct thermal_zone_device *tzd);
 
-int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
-				     struct thermal_cooling_device *,
-				     unsigned long, unsigned long,
-				     unsigned int);
-int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
-				       struct thermal_cooling_device *);
 void thermal_zone_device_update(struct thermal_zone_device *,
 				enum thermal_notify_event);
 
Index: linux-pm/Documentation/driver-api/thermal/sysfs-api.rst
===================================================================
--- linux-pm.orig/Documentation/driver-api/thermal/sysfs-api.rst
+++ linux-pm/Documentation/driver-api/thermal/sysfs-api.rst
@@ -251,56 +251,6 @@ temperature) and throttle appropriate de
     It deletes the corresponding entry from /sys/class/thermal folder and
     unbinds itself from all the thermal zone devices using it.
 
-1.3 interface for binding a thermal zone device with a thermal cooling device
------------------------------------------------------------------------------
-
-    ::
-
-	int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
-		int trip, struct thermal_cooling_device *cdev,
-		unsigned long upper, unsigned long lower, unsigned int weight);
-
-    This interface function binds a thermal cooling device to a particular trip
-    point of a thermal zone device.
-
-    This function is usually called in the thermal zone device .bind callback.
-
-    tz:
-	  the thermal zone device
-    cdev:
-	  thermal cooling device
-    trip:
-	  indicates which trip point in this thermal zone the cooling device
-	  is associated with.
-    upper:
-	  the Maximum cooling state for this trip point.
-	  THERMAL_NO_LIMIT means no upper limit,
-	  and the cooling device can be in max_state.
-    lower:
-	  the Minimum cooling state can be used for this trip point.
-	  THERMAL_NO_LIMIT means no lower limit,
-	  and the cooling device can be in cooling state 0.
-    weight:
-	  the influence of this cooling device in this thermal
-	  zone.  See 1.4.1 below for more information.
-
-    ::
-
-	int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
-				int trip, struct thermal_cooling_device *cdev);
-
-    This interface function unbinds a thermal cooling device from a particular
-    trip point of a thermal zone device. This function is usually called in
-    the thermal zone device .unbind callback.
-
-    tz:
-	the thermal zone device
-    cdev:
-	thermal cooling device
-    trip:
-	indicates which trip point in this thermal zone the cooling device
-	is associated with.
-
 1.4 Thermal Zone Parameters
 ---------------------------
 
@@ -371,8 +321,6 @@ Thermal cooling device sys I/F, created
 
 Then next two dynamic attributes are created/removed in pairs. They represent
 the relationship between a thermal zone and its associated cooling device.
-They are created/removed for each successful execution of
-thermal_zone_bind_cooling_device/thermal_zone_unbind_cooling_device.
 
 ::
 

[PATCH v1 05/17] thermal: core: Move lists of thermal instances to trip descriptors

[Rafael J. Wysocki]

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

In almost all places where a thermal zone's list of thermal instances
is walked, there is a check to match a specific trip point and it is
walked in vain whenever there are no cooling devices associated with
the given trip.

To address this, store the lists of thermal instances in trip point
descriptors instead of storing them in thermal zones and adjust all
code using those lists accordingly.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_bang_bang.c       |    8 ++----
 drivers/thermal/gov_fair_share.c      |   16 ++++---------
 drivers/thermal/gov_power_allocator.c |   41 ++++++++++++++--------------------
 drivers/thermal/gov_step_wise.c       |   16 ++++++-------
 drivers/thermal/thermal_core.c        |   33 +++++++++++++++------------
 drivers/thermal/thermal_core.h        |    5 +---
 drivers/thermal/thermal_helpers.c     |    5 ++--
 include/linux/thermal.h               |    4 +--
 8 files changed, 60 insertions(+), 68 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -491,7 +491,7 @@ static void thermal_zone_device_check(st
 
 static void thermal_zone_device_init(struct thermal_zone_device *tz)
 {
-	struct thermal_instance *pos;
+	struct thermal_trip_desc *td;
 
 	INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
 
@@ -499,8 +499,12 @@ static void thermal_zone_device_init(str
 	tz->passive = 0;
 	tz->prev_low_trip = -INT_MAX;
 	tz->prev_high_trip = INT_MAX;
-	list_for_each_entry(pos, &tz->thermal_instances, tz_node)
-		pos->initialized = false;
+	for_each_trip_desc(tz, td) {
+		struct thermal_instance *instance;
+
+		list_for_each_entry(instance, &td->thermal_instances, trip_node)
+			instance->initialized = false;
+	}
 }
 
 static void thermal_governor_trip_crossed(struct thermal_governor *governor,
@@ -774,13 +778,13 @@ struct thermal_zone_device *thermal_zone
  * Return: 0 on success, the proper error value otherwise.
  */
 int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
-				     const struct thermal_trip *trip,
+				     struct thermal_trip *trip,
 				     struct thermal_cooling_device *cdev,
 				     unsigned long upper, unsigned long lower,
 				     unsigned int weight)
 {
-	struct thermal_instance *dev;
-	struct thermal_instance *pos;
+	struct thermal_trip_desc *td = trip_to_trip_desc(trip);
+	struct thermal_instance *dev, *instance;
 	bool upper_no_limit;
 	int result;
 
@@ -850,14 +854,14 @@ int thermal_bind_cdev_to_trip(struct the
 	mutex_lock(&tz->lock);
 	mutex_lock(&cdev->lock);
 
-	list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
-		if (pos->trip == trip && pos->cdev == cdev) {
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
+		if (instance->cdev == cdev) {
 			result = -EEXIST;
 			goto remove_weight_file;
 		}
 	}
 
-	list_add_tail(&dev->tz_node, &tz->thermal_instances);
+	list_add_tail(&dev->trip_node, &td->thermal_instances);
 	list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
 	atomic_set(&tz->need_update, 1);
 
@@ -909,17 +913,18 @@ EXPORT_SYMBOL_GPL(thermal_zone_bind_cool
  * Return: 0 on success, the proper error value otherwise.
  */
 int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
-				  const struct thermal_trip *trip,
+				  struct thermal_trip *trip,
 				  struct thermal_cooling_device *cdev)
 {
+	struct thermal_trip_desc *td = trip_to_trip_desc(trip);
 	struct thermal_instance *pos, *next;
 
 	mutex_lock(&tz->lock);
 	mutex_lock(&cdev->lock);
 
-	list_for_each_entry_safe(pos, next, &tz->thermal_instances, tz_node) {
-		if (pos->trip == trip && pos->cdev == cdev) {
-			list_del(&pos->tz_node);
+	list_for_each_entry_safe(pos, next, &td->thermal_instances, trip_node) {
+		if (pos->cdev == cdev) {
+			list_del(&pos->trip_node);
 			list_del(&pos->cdev_node);
 
 			thermal_governor_update_tz(tz, THERMAL_TZ_UNBIND_CDEV);
@@ -1446,7 +1451,6 @@ thermal_zone_device_register_with_trips(
 		}
 	}
 
-	INIT_LIST_HEAD(&tz->thermal_instances);
 	INIT_LIST_HEAD(&tz->node);
 	ida_init(&tz->ida);
 	mutex_init(&tz->lock);
@@ -1470,6 +1474,7 @@ thermal_zone_device_register_with_trips(
 	tz->num_trips = num_trips;
 	for_each_trip_desc(tz, td) {
 		td->trip = *trip++;
+		INIT_LIST_HEAD(&td->thermal_instances);
 		/*
 		 * Mark all thresholds as invalid to start with even though
 		 * this only matters for the trips that start as invalid and
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -30,6 +30,7 @@ struct thermal_trip_desc {
 	struct thermal_trip trip;
 	struct thermal_trip_attrs trip_attrs;
 	struct list_head notify_list_node;
+	struct list_head thermal_instances;
 	int notify_temp;
 	int threshold;
 };
@@ -93,7 +94,6 @@ struct thermal_governor {
  * @tzp:	thermal zone parameters
  * @governor:	pointer to the governor for this thermal zone
  * @governor_data:	private pointer for governor data
- * @thermal_instances:	list of &struct thermal_instance of this thermal zone
  * @ida:	&struct ida to generate unique id for this zone's cooling
  *		devices
  * @lock:	lock to protect thermal_instances list
@@ -128,7 +128,6 @@ struct thermal_zone_device {
 	struct thermal_zone_params *tzp;
 	struct thermal_governor *governor;
 	void *governor_data;
-	struct list_head thermal_instances;
 	struct ida ida;
 	struct mutex lock;
 	struct list_head node;
@@ -224,7 +223,7 @@ struct thermal_instance {
 	struct device_attribute attr;
 	char weight_attr_name[THERMAL_NAME_LENGTH];
 	struct device_attribute weight_attr;
-	struct list_head tz_node; /* node in tz->thermal_instances */
+	struct list_head trip_node; /* node in trip->thermal_instances */
 	struct list_head cdev_node; /* node in cdev->thermal_instances */
 	unsigned int weight; /* The weight of the cooling device */
 	bool upper_no_limit;
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
@@ -45,6 +45,7 @@ static void bang_bang_control(struct the
 			      const struct thermal_trip *trip,
 			      bool crossed_up)
 {
+	const struct thermal_trip_desc *td = trip_to_trip_desc(trip);
 	struct thermal_instance *instance;
 
 	lockdep_assert_held(&tz->lock);
@@ -53,10 +54,7 @@ static void bang_bang_control(struct the
 		thermal_zone_trip_id(tz, trip), trip->temperature,
 		tz->temperature, trip->hysteresis);
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (instance->trip != trip)
-			continue;
-
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
 		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",
@@ -75,7 +73,7 @@ static void bang_bang_control(struct the
 		mutex_unlock(&instance->cdev->lock);
 	}
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
+	list_for_each_entry(instance, &td->thermal_instances, trip_node)
 		thermal_cdev_update(instance->cdev);
 }
 
Index: linux-pm/drivers/thermal/gov_fair_share.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_fair_share.c
+++ linux-pm/drivers/thermal/gov_fair_share.c
@@ -44,7 +44,7 @@ static int get_trip_level(struct thermal
 /**
  * fair_share_throttle - throttles devices associated with the given zone
  * @tz: thermal_zone_device
- * @trip: trip point
+ * @td: trip point descriptor
  * @trip_level: number of trips crossed by the zone temperature
  *
  * Throttling Logic: This uses three parameters to calculate the new
@@ -61,29 +61,23 @@ static int get_trip_level(struct thermal
  * new_state of cooling device = P3 * P2 * P1
  */
 static void fair_share_throttle(struct thermal_zone_device *tz,
-				const struct thermal_trip *trip,
+				const struct thermal_trip_desc *td,
 				int trip_level)
 {
 	struct thermal_instance *instance;
 	int total_weight = 0;
 	int nr_instances = 0;
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (instance->trip != trip)
-			continue;
-
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
 		total_weight += instance->weight;
 		nr_instances++;
 	}
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
 		struct thermal_cooling_device *cdev = instance->cdev;
 		u64 dividend;
 		u32 divisor;
 
-		if (instance->trip != trip)
-			continue;
-
 		dividend = trip_level;
 		dividend *= cdev->max_state;
 		divisor = tz->num_trips;
@@ -116,7 +110,7 @@ static void fair_share_manage(struct the
 		    trip->type == THERMAL_TRIP_HOT)
 			continue;
 
-		fair_share_throttle(tz, trip, trip_level);
+		fair_share_throttle(tz, td, trip_level);
 	}
 }
 
Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -97,13 +97,6 @@ struct power_allocator_params {
 	struct power_actor *power;
 };
 
-static bool power_actor_is_valid(struct power_allocator_params *params,
-				 struct thermal_instance *instance)
-{
-	return (instance->trip == params->trip_max &&
-		 cdev_is_power_actor(instance->cdev));
-}
-
 /**
  * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
  * @tz: thermal zone we are operating in
@@ -118,13 +111,14 @@ static bool power_actor_is_valid(struct
 static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
 {
 	struct power_allocator_params *params = tz->governor_data;
+	const struct thermal_trip_desc *td = trip_to_trip_desc(params->trip_max);
 	struct thermal_cooling_device *cdev;
 	struct thermal_instance *instance;
 	u32 sustainable_power = 0;
 	u32 min_power;
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (!power_actor_is_valid(params, instance))
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
+		if (!cdev_is_power_actor(instance->cdev))
 			continue;
 
 		cdev = instance->cdev;
@@ -400,6 +394,7 @@ static void divvy_up_power(struct power_
 static void allocate_power(struct thermal_zone_device *tz, int control_temp)
 {
 	struct power_allocator_params *params = tz->governor_data;
+	const struct thermal_trip_desc *td = trip_to_trip_desc(params->trip_max);
 	unsigned int num_actors = params->num_actors;
 	struct power_actor *power = params->power;
 	struct thermal_cooling_device *cdev;
@@ -417,10 +412,10 @@ static void allocate_power(struct therma
 	/* Clean all buffers for new power estimations */
 	memset(power, 0, params->buffer_size);
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
 		struct power_actor *pa = &power[i];
 
-		if (!power_actor_is_valid(params, instance))
+		if (!cdev_is_power_actor(instance->cdev))
 			continue;
 
 		cdev = instance->cdev;
@@ -454,10 +449,10 @@ static void allocate_power(struct therma
 		       power_range);
 
 	i = 0;
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
 		struct power_actor *pa = &power[i];
 
-		if (!power_actor_is_valid(params, instance))
+		if (!cdev_is_power_actor(instance->cdev))
 			continue;
 
 		power_actor_set_power(instance->cdev, instance,
@@ -538,12 +533,13 @@ static void reset_pid_controller(struct
 static void allow_maximum_power(struct thermal_zone_device *tz)
 {
 	struct power_allocator_params *params = tz->governor_data;
+	const struct thermal_trip_desc *td = trip_to_trip_desc(params->trip_max);
 	struct thermal_cooling_device *cdev;
 	struct thermal_instance *instance;
 	u32 req_power;
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (!power_actor_is_valid(params, instance))
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
+		if (!cdev_is_power_actor(instance->cdev))
 			continue;
 
 		cdev = instance->cdev;
@@ -581,13 +577,11 @@ static void allow_maximum_power(struct t
 static int check_power_actors(struct thermal_zone_device *tz,
 			      struct power_allocator_params *params)
 {
+	const struct thermal_trip_desc *td = trip_to_trip_desc(params->trip_max);
 	struct thermal_instance *instance;
 	int ret = 0;
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (instance->trip != params->trip_max)
-			continue;
-
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
 		if (!cdev_is_power_actor(instance->cdev)) {
 			dev_warn(&tz->device, "power_allocator: %s is not a power actor\n",
 				 instance->cdev->type);
@@ -635,14 +629,15 @@ static void power_allocator_update_tz(st
 				      enum thermal_notify_event reason)
 {
 	struct power_allocator_params *params = tz->governor_data;
+	const struct thermal_trip_desc *td = trip_to_trip_desc(params->trip_max);
 	struct thermal_instance *instance;
 	int num_actors = 0;
 
 	switch (reason) {
 	case THERMAL_TZ_BIND_CDEV:
 	case THERMAL_TZ_UNBIND_CDEV:
-		list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-			if (power_actor_is_valid(params, instance))
+		list_for_each_entry(instance, &td->thermal_instances, trip_node)
+			if (cdev_is_power_actor(instance->cdev))
 				num_actors++;
 
 		if (num_actors == params->num_actors)
@@ -652,8 +647,8 @@ static void power_allocator_update_tz(st
 		break;
 	case THERMAL_INSTANCE_WEIGHT_CHANGED:
 		params->total_weight = 0;
-		list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-			if (power_actor_is_valid(params, instance))
+		list_for_each_entry(instance, &td->thermal_instances, trip_node)
+			if (cdev_is_power_actor(instance->cdev))
 				params->total_weight += instance->weight;
 		break;
 	default:
Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -66,9 +66,10 @@ static unsigned long get_target_state(st
 }
 
 static void thermal_zone_trip_update(struct thermal_zone_device *tz,
-				     const struct thermal_trip *trip,
+				     const struct thermal_trip_desc *td,
 				     int trip_threshold)
 {
+	const struct thermal_trip *trip = &td->trip;
 	enum thermal_trend trend = get_tz_trend(tz, trip);
 	int trip_id = thermal_zone_trip_id(tz, trip);
 	struct thermal_instance *instance;
@@ -82,12 +83,9 @@ static void thermal_zone_trip_update(str
 	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
 		trip_id, trip->type, trip_threshold, trend, throttle);
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+	list_for_each_entry(instance, &td->thermal_instances, trip_node) {
 		int old_target;
 
-		if (instance->trip != trip)
-			continue;
-
 		old_target = instance->target;
 		instance->target = get_target_state(instance, trend, throttle);
 
@@ -127,11 +125,13 @@ static void step_wise_manage(struct ther
 		    trip->type == THERMAL_TRIP_HOT)
 			continue;
 
-		thermal_zone_trip_update(tz, trip, td->threshold);
+		thermal_zone_trip_update(tz, td, td->threshold);
 	}
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-		thermal_cdev_update(instance->cdev);
+	for_each_trip_desc(tz, td) {
+		list_for_each_entry(instance, &td->thermal_instances, trip_node)
+			thermal_cdev_update(instance->cdev);
+	}
 }
 
 static struct thermal_governor thermal_gov_step_wise = {
Index: linux-pm/drivers/thermal/thermal_helpers.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_helpers.c
+++ linux-pm/drivers/thermal/thermal_helpers.c
@@ -43,10 +43,11 @@ static bool thermal_instance_present(str
 				     struct thermal_cooling_device *cdev,
 				     const struct thermal_trip *trip)
 {
+	const struct thermal_trip_desc *td = trip_to_trip_desc(trip);
 	struct thermal_instance *ti;
 
-	list_for_each_entry(ti, &tz->thermal_instances, tz_node) {
-		if (ti->trip == trip && ti->cdev == cdev)
+	list_for_each_entry(ti, &td->thermal_instances, trip_node) {
+		if (ti->cdev == cdev)
 			return true;
 	}
 
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -237,7 +237,7 @@ int thermal_zone_device_id(struct therma
 struct device *thermal_zone_device(struct thermal_zone_device *tzd);
 
 int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
-			      const struct thermal_trip *trip,
+			      struct thermal_trip *trip,
 			      struct thermal_cooling_device *cdev,
 			      unsigned long upper, unsigned long lower,
 			      unsigned int weight);
@@ -246,7 +246,7 @@ int thermal_zone_bind_cooling_device(str
 				     unsigned long, unsigned long,
 				     unsigned int);
 int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
-				  const struct thermal_trip *trip,
+				  struct thermal_trip *trip,
 				  struct thermal_cooling_device *cdev);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
 				       struct thermal_cooling_device *);