[PATCH v2 1/5] thermal/core: Update cooling device during thermal zone unregistration

[PATCH v2 1/5] thermal/core: Update cooling device during thermal zone unregistration

[Zhang Rui]

When unregistering a thermal zone device, update all cooling devices
bound to the thermal zone device.

This fixes a problem that the frequency of ACPI processors are still
limited after unloading ACPI thermal driver while ACPI passive cooling
is activated.

Cc: stable@vger.kernel.org
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
v1 -> v2
	Changelog update.
	Rearrange the code to elimiate an "iterator used outside loop"
warning.
---
 drivers/thermal/thermal_core.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index cfd4c1afeae7..30ff39154598 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1497,9 +1497,24 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 
 	/* Unbind all cdevs associated with 'this' thermal zone */
 	list_for_each_entry(cdev, &thermal_cdev_list, node) {
+		struct thermal_instance *ti;
+
+		mutex_lock(&tz->lock);
+		list_for_each_entry(ti, &tz->thermal_instances, tz_node) {
+			if (ti->cdev == cdev) {
+				mutex_unlock(&tz->lock);
+				goto unbind;
+			}
+		}
+
+		/* The cooling device is not bound to current thermal zone */
+		mutex_unlock(&tz->lock);
+		continue;
+
+unbind:
 		if (tz->ops->unbind) {
 			tz->ops->unbind(tz, cdev);
-			continue;
+			goto deactivate;
 		}
 
 		if (!tzp || !tzp->tbp)
@@ -1511,6 +1526,16 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 				tzp->tbp[i].cdev = NULL;
 			}
 		}
+
+deactivate:
+		/*
+		 * The thermal instances for current thermal zone has been
+		 * removed. Update the cooling device in case it is activated
+		 * by current thermal zone device.
+		 */
+		mutex_lock(&cdev->lock);
+		__thermal_cdev_update(cdev);
+		mutex_unlock(&cdev->lock);
 	}
 
 	mutex_unlock(&thermal_list_lock);
-- 
2.25.1

[PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration

[Zhang Rui]

When unregistering a cooling device, it is possible that the cooling
device has been activated. And once the cooling device is unregistered,
no one will deactivate it anymore.

Reset cooling state during cooling device unregistration.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
In theory, this problem that this patch fixes can be triggered on a
platform with ACPI Active cooling, by
1. overheat the system to trigger ACPI active cooling
2. unload ACPI fan driver
3. check if the fan is still spinning
But I don't have such a system so I didn't trigger then problem and I
only did build & boot test.
---
 drivers/thermal/thermal_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 30ff39154598..fd54e6c10b60 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1192,6 +1192,10 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 		}
 	}
 
+	mutex_lock(&cdev->lock);
+	cdev->ops->set_cur_state(cdev, 0);
+	mutex_unlock(&cdev->lock);
+
 	mutex_unlock(&thermal_list_lock);
 
 	device_unregister(&cdev->device);
-- 
2.25.1

[PATCH 3/5] thermal/core: Remove thermal_bind_params structure

[Zhang Rui]

Remove struct thermal_bind_params because no one is using it for thermal
binding now.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 .../driver-api/thermal/sysfs-api.rst          |  40 ------
 drivers/thermal/thermal_core.c                | 134 ++----------------
 include/linux/thermal.h                       |  38 -----
 3 files changed, 14 insertions(+), 198 deletions(-)

diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
index 2e0f79a9e2ee..6c1175c6afba 100644
--- a/Documentation/driver-api/thermal/sysfs-api.rst
+++ b/Documentation/driver-api/thermal/sysfs-api.rst
@@ -304,42 +304,6 @@ temperature) and throttle appropriate devices.
 1.4 Thermal Zone Parameters
 ---------------------------
 
-    ::
-
-	struct thermal_bind_params
-
-    This structure defines the following parameters that are used to bind
-    a zone with a cooling device for a particular trip point.
-
-    .cdev:
-	     The cooling device pointer
-    .weight:
-	     The 'influence' of a particular cooling device on this
-	     zone. This is relative to the rest of the cooling
-	     devices. For example, if all cooling devices have a
-	     weight of 1, then they all contribute the same. You can
-	     use percentages if you want, but it's not mandatory. A
-	     weight of 0 means that this cooling device doesn't
-	     contribute to the cooling of this zone unless all cooling
-	     devices have a weight of 0. If all weights are 0, then
-	     they all contribute the same.
-    .trip_mask:
-	       This is a bit mask that gives the binding relation between
-	       this thermal zone and cdev, for a particular trip point.
-	       If nth bit is set, then the cdev and thermal zone are bound
-	       for trip point n.
-    .binding_limits:
-		     This is an array of cooling state limits. Must have
-		     exactly 2 * thermal_zone.number_of_trip_points. It is an
-		     array consisting of tuples <lower-state upper-state> of
-		     state limits. Each trip will be associated with one state
-		     limit tuple when binding. A NULL pointer means
-		     <THERMAL_NO_LIMITS THERMAL_NO_LIMITS> on all trips.
-		     These limits are used when binding a cdev to a trip point.
-    .match:
-	    This call back returns success(0) if the 'tz and cdev' need to
-	    be bound, as per platform data.
-
     ::
 
 	struct thermal_zone_params
@@ -357,10 +321,6 @@ temperature) and throttle appropriate devices.
 	       will be created. when no_hwmon == true, nothing will be done.
 	       In case the thermal_zone_params is NULL, the hwmon interface
 	       will be created (for backward compatibility).
-    .num_tbps:
-	       Number of thermal_bind_params entries for this zone
-    .tbp:
-	       thermal_bind_params entries
 
 2. sysfs attributes structure
 =============================
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index fd54e6c10b60..5225d65fb0e0 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -794,65 +794,20 @@ void print_bind_err_msg(struct thermal_zone_device *tz,
 		tz->type, cdev->type, ret);
 }
 
-static void __bind(struct thermal_zone_device *tz, int mask,
-		   struct thermal_cooling_device *cdev,
-		   unsigned long *limits,
-		   unsigned int weight)
-{
-	int i, ret;
-
-	for (i = 0; i < tz->num_trips; i++) {
-		if (mask & (1 << i)) {
-			unsigned long upper, lower;
-
-			upper = THERMAL_NO_LIMIT;
-			lower = THERMAL_NO_LIMIT;
-			if (limits) {
-				lower = limits[i * 2];
-				upper = limits[i * 2 + 1];
-			}
-			ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-							       upper, lower,
-							       weight);
-			if (ret)
-				print_bind_err_msg(tz, cdev, ret);
-		}
-	}
-}
-
 static void bind_cdev(struct thermal_cooling_device *cdev)
 {
-	int i, ret;
-	const struct thermal_zone_params *tzp;
+	int ret;
 	struct thermal_zone_device *pos = NULL;
 
 	mutex_lock(&thermal_list_lock);
 
 	list_for_each_entry(pos, &thermal_tz_list, node) {
-		if (!pos->tzp && !pos->ops->bind)
+		if (!pos->ops->bind)
 			continue;
 
-		if (pos->ops->bind) {
-			ret = pos->ops->bind(pos, cdev);
-			if (ret)
-				print_bind_err_msg(pos, cdev, ret);
-			continue;
-		}
-
-		tzp = pos->tzp;
-		if (!tzp || !tzp->tbp)
-			continue;
-
-		for (i = 0; i < tzp->num_tbps; i++) {
-			if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
-				continue;
-			if (tzp->tbp[i].match(pos, cdev))
-				continue;
-			tzp->tbp[i].cdev = cdev;
-			__bind(pos, tzp->tbp[i].trip_mask, cdev,
-			       tzp->tbp[i].binding_limits,
-			       tzp->tbp[i].weight);
-		}
+		ret = pos->ops->bind(pos, cdev);
+		if (ret)
+			print_bind_err_msg(pos, cdev, ret);
 	}
 
 	mutex_unlock(&thermal_list_lock);
@@ -1138,16 +1093,6 @@ void thermal_cooling_device_update(struct thermal_cooling_device *cdev)
 }
 EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
 
-static void __unbind(struct thermal_zone_device *tz, int mask,
-		     struct thermal_cooling_device *cdev)
-{
-	int i;
-
-	for (i = 0; i < tz->num_trips; i++)
-		if (mask & (1 << i))
-			thermal_zone_unbind_cooling_device(tz, i, cdev);
-}
-
 /**
  * thermal_cooling_device_unregister - removes a thermal cooling device
  * @cdev:	the thermal cooling device to remove.
@@ -1157,8 +1102,6 @@ static void __unbind(struct thermal_zone_device *tz, int mask,
  */
 void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 {
-	int i;
-	const struct thermal_zone_params *tzp;
 	struct thermal_zone_device *tz;
 
 	if (!cdev)
@@ -1175,21 +1118,8 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 
 	/* Unbind all thermal zones associated with 'this' cdev */
 	list_for_each_entry(tz, &thermal_tz_list, node) {
-		if (tz->ops->unbind) {
+		if (tz->ops->unbind)
 			tz->ops->unbind(tz, cdev);
-			continue;
-		}
-
-		if (!tz->tzp || !tz->tzp->tbp)
-			continue;
-
-		tzp = tz->tzp;
-		for (i = 0; i < tzp->num_tbps; i++) {
-			if (tzp->tbp[i].cdev == cdev) {
-				__unbind(tz, tzp->tbp[i].trip_mask, cdev);
-				tzp->tbp[i].cdev = NULL;
-			}
-		}
 	}
 
 	mutex_lock(&cdev->lock);
@@ -1204,41 +1134,20 @@ EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
 
 static void bind_tz(struct thermal_zone_device *tz)
 {
-	int i, ret;
+	int ret;
 	struct thermal_cooling_device *pos = NULL;
-	const struct thermal_zone_params *tzp = tz->tzp;
 
-	if (!tzp && !tz->ops->bind)
+	if (!tz->ops->bind)
 		return;
 
 	mutex_lock(&thermal_list_lock);
 
-	/* If there is ops->bind, try to use ops->bind */
-	if (tz->ops->bind) {
-		list_for_each_entry(pos, &thermal_cdev_list, node) {
-			ret = tz->ops->bind(tz, pos);
-			if (ret)
-				print_bind_err_msg(tz, pos, ret);
-		}
-		goto exit;
-	}
-
-	if (!tzp || !tzp->tbp)
-		goto exit;
-
 	list_for_each_entry(pos, &thermal_cdev_list, node) {
-		for (i = 0; i < tzp->num_tbps; i++) {
-			if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
-				continue;
-			if (tzp->tbp[i].match(tz, pos))
-				continue;
-			tzp->tbp[i].cdev = pos;
-			__bind(tz, tzp->tbp[i].trip_mask, pos,
-			       tzp->tbp[i].binding_limits,
-			       tzp->tbp[i].weight);
-		}
+		ret = tz->ops->bind(tz, pos);
+		if (ret)
+			print_bind_err_msg(tz, pos, ret);
 	}
-exit:
+
 	mutex_unlock(&thermal_list_lock);
 }
 
@@ -1477,15 +1386,13 @@ EXPORT_SYMBOL_GPL(thermal_zone_device_register);
  */
 void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 {
-	int i, tz_id;
-	const struct thermal_zone_params *tzp;
+	int tz_id;
 	struct thermal_cooling_device *cdev;
 	struct thermal_zone_device *pos = NULL;
 
 	if (!tz)
 		return;
 
-	tzp = tz->tzp;
 	tz_id = tz->id;
 
 	mutex_lock(&thermal_list_lock);
@@ -1516,22 +1423,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 		continue;
 
 unbind:
-		if (tz->ops->unbind) {
+		if (tz->ops->unbind)
 			tz->ops->unbind(tz, cdev);
-			goto deactivate;
-		}
-
-		if (!tzp || !tzp->tbp)
-			break;
-
-		for (i = 0; i < tzp->num_tbps; i++) {
-			if (tzp->tbp[i].cdev == cdev) {
-				__unbind(tz, tzp->tbp[i].trip_mask, cdev);
-				tzp->tbp[i].cdev = NULL;
-			}
-		}
 
-deactivate:
 		/*
 		 * The thermal instances for current thermal zone has been
 		 * removed. Update the cooling device in case it is activated
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 13c6aaed18df..481417d3b9b7 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -207,41 +207,6 @@ struct thermal_governor {
 	struct list_head	governor_list;
 };
 
-/* Structure that holds binding parameters for a zone */
-struct thermal_bind_params {
-	struct thermal_cooling_device *cdev;
-
-	/*
-	 * This is a measure of 'how effectively these devices can
-	 * cool 'this' thermal zone. It shall be determined by
-	 * platform characterization. This value is relative to the
-	 * rest of the weights so a cooling device whose weight is
-	 * double that of another cooling device is twice as
-	 * effective. See Documentation/driver-api/thermal/sysfs-api.rst for more
-	 * information.
-	 */
-	int weight;
-
-	/*
-	 * This is a bit mask that gives the binding relation between this
-	 * thermal zone and cdev, for a particular trip point.
-	 * See Documentation/driver-api/thermal/sysfs-api.rst for more information.
-	 */
-	int trip_mask;
-
-	/*
-	 * This is an array of cooling state limits. Must have exactly
-	 * 2 * thermal_zone.number_of_trip_points. It is an array consisting
-	 * of tuples <lower-state upper-state> of state limits. Each trip
-	 * will be associated with one state limit tuple when binding.
-	 * A NULL pointer means <THERMAL_NO_LIMITS THERMAL_NO_LIMITS>
-	 * on all trips.
-	 */
-	unsigned long *binding_limits;
-	int (*match) (struct thermal_zone_device *tz,
-			struct thermal_cooling_device *cdev);
-};
-
 /* Structure to define Thermal Zone parameters */
 struct thermal_zone_params {
 	char governor_name[THERMAL_NAME_LENGTH];
@@ -253,9 +218,6 @@ struct thermal_zone_params {
 	 */
 	bool no_hwmon;
 
-	int num_tbps;	/* Number of tbp entries */
-	struct thermal_bind_params *tbp;
-
 	/*
 	 * Sustainable power (heat) that this thermal zone can dissipate in
 	 * mW
-- 
2.25.1

[PATCH 4/5] thermal/core: Enforce paired .bind/.unbind callbacks

[Zhang Rui]

The .bind/.unbind callbacks are designed to allow the thermal zone
device to bind to/unbind from a matched cooling device, with thermal
instances created/deleted.

In this sense, .bind/.unbind callbacks must exist in pairs.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 5225d65fb0e0..9c447f22cb39 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1258,6 +1258,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 	if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
 		return ERR_PTR(-EINVAL);
 
+	if ((ops->bind && !ops->unbind) || (!ops->bind && ops->unbind)) {
+		pr_err("Thermal zone device .bind/.unbind not paired\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (!thermal_class)
 		return ERR_PTR(-ENODEV);
 
-- 
2.25.1

[PATCH 5/5] thermal/core: Avoid unbind for thermal zones without .unbind()

[Zhang Rui]

The ops->bind()/unbind() callback is the only way to bind/unbind a
cooling device to/from a thermal zone.

Optimize the code to avoid trying unbind when .unbind() callback doesn't
exist.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 9c447f22cb39..1af36450f13e 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1411,6 +1411,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	}
 	list_del(&tz->node);
 
+	if (!tz->ops->unbind)
+		goto unbind_done;
+
 	/* Unbind all cdevs associated with 'this' thermal zone */
 	list_for_each_entry(cdev, &thermal_cdev_list, node) {
 		struct thermal_instance *ti;
@@ -1428,8 +1431,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 		continue;
 
 unbind:
-		if (tz->ops->unbind)
-			tz->ops->unbind(tz, cdev);
+		tz->ops->unbind(tz, cdev);
 
 		/*
 		 * The thermal instances for current thermal zone has been
@@ -1441,6 +1443,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 		mutex_unlock(&cdev->lock);
 	}
 
+unbind_done:
 	mutex_unlock(&thermal_list_lock);
 
 	cancel_delayed_work_sync(&tz->poll_queue);
-- 
2.25.1

Re: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration

[Rafael J. Wysocki]

On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> When unregistering a cooling device, it is possible that the cooling
> device has been activated. And once the cooling device is unregistered,
> no one will deactivate it anymore.
>
> Reset cooling state during cooling device unregistration.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
> In theory, this problem that this patch fixes can be triggered on a
> platform with ACPI Active cooling, by
> 1. overheat the system to trigger ACPI active cooling
> 2. unload ACPI fan driver
> 3. check if the fan is still spinning
> But I don't have such a system so I didn't trigger then problem and I
> only did build & boot test.

So I'm not sure if this change is actually safe.

In the example above, the system will still need the fan to spin after
the ACPI fan driver is unloaded in order to cool down, won't it?

> ---
>  drivers/thermal/thermal_core.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 30ff39154598..fd54e6c10b60 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1192,6 +1192,10 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
>                 }
>         }
>
> +       mutex_lock(&cdev->lock);
> +       cdev->ops->set_cur_state(cdev, 0);
> +       mutex_unlock(&cdev->lock);
> +
>         mutex_unlock(&thermal_list_lock);
>
>         device_unregister(&cdev->device);
> --
> 2.25.1
>

Re: [PATCH 4/5] thermal/core: Enforce paired .bind/.unbind callbacks

[Rafael J. Wysocki]

On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> The .bind/.unbind callbacks are designed to allow the thermal zone
> device to bind to/unbind from a matched cooling device, with thermal
> instances created/deleted.
>
> In this sense, .bind/.unbind callbacks must exist in pairs.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/thermal_core.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 5225d65fb0e0..9c447f22cb39 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1258,6 +1258,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>         if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
>                 return ERR_PTR(-EINVAL);
>
> +       if ((ops->bind && !ops->unbind) || (!ops->bind && ops->unbind)) {

This can be written as

        if (!!ops->bind != !!ops->unbind) {

> +               pr_err("Thermal zone device .bind/.unbind not paired\n");

And surely none of the existing drivers do that?  Because it would be
a functional regression if they did.

> +               return ERR_PTR(-EINVAL);
> +       }
> +
>         if (!thermal_class)
>                 return ERR_PTR(-ENODEV);
>
> --

Re: [PATCH 4/5] thermal/core: Enforce paired .bind/.unbind callbacks

[Rafael J. Wysocki]

On Fri, Mar 24, 2023 at 2:24 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com> wrote:
> >
> > The .bind/.unbind callbacks are designed to allow the thermal zone
> > device to bind to/unbind from a matched cooling device, with thermal
> > instances created/deleted.
> >
> > In this sense, .bind/.unbind callbacks must exist in pairs.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/thermal_core.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> > index 5225d65fb0e0..9c447f22cb39 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -1258,6 +1258,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
> >         if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
> >                 return ERR_PTR(-EINVAL);
> >
> > +       if ((ops->bind && !ops->unbind) || (!ops->bind && ops->unbind)) {
>
> This can be written as
>
>         if (!!ops->bind != !!ops->unbind) {

Or even

        if (!ops->bind != !ops->unbind) {

for that matter.

>
> > +               pr_err("Thermal zone device .bind/.unbind not paired\n");
>
> And surely none of the existing drivers do that?  Because it would be
> a functional regression if they did.
>
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> >         if (!thermal_class)
> >                 return ERR_PTR(-ENODEV);
> >
> > --

Re: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration

[Zhang, Rui]

On Fri, 2023-03-24 at 14:19 +0100, Rafael J. Wysocki wrote:
> On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > When unregistering a cooling device, it is possible that the
> > cooling
> > device has been activated. And once the cooling device is
> > unregistered,
> > no one will deactivate it anymore.
> > 
> > Reset cooling state during cooling device unregistration.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> > In theory, this problem that this patch fixes can be triggered on a
> > platform with ACPI Active cooling, by
> > 1. overheat the system to trigger ACPI active cooling
> > 2. unload ACPI fan driver
> > 3. check if the fan is still spinning
> > But I don't have such a system so I didn't trigger then problem and
> > I
> > only did build & boot test.
> 
> So I'm not sure if this change is actually safe.
> 
> In the example above, the system will still need the fan to spin
> after
> the ACPI fan driver is unloaded in order to cool down, won't it?

Then we can argue that the ACPI fan driver should not be unloaded in
this case.

Actually, this is the same situation as patch 1/5.
Patch 1/5 fixes the problem that cooling state not restored to 0 when
unloading the thermal driver, and this fixes the same problem when
unloading the cooling device driver.

thanks,
rui

> 
> > ---
> >  drivers/thermal/thermal_core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c
> > index 30ff39154598..fd54e6c10b60 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -1192,6 +1192,10 @@ void
> > thermal_cooling_device_unregister(struct thermal_cooling_device
> > *cdev)
> >                 }
> >         }
> > 
> > +       mutex_lock(&cdev->lock);
> > +       cdev->ops->set_cur_state(cdev, 0);
> > +       mutex_unlock(&cdev->lock);
> > +
> >         mutex_unlock(&thermal_list_lock);
> > 
> >         device_unregister(&cdev->device);
> > --
> > 2.25.1
> > 

Re: [PATCH 4/5] thermal/core: Enforce paired .bind/.unbind callbacks

[Zhang, Rui]

On Fri, 2023-03-24 at 14:24 +0100, Rafael J. Wysocki wrote:
> On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > The .bind/.unbind callbacks are designed to allow the thermal zone
> > device to bind to/unbind from a matched cooling device, with
> > thermal
> > instances created/deleted.
> > 
> > In this sense, .bind/.unbind callbacks must exist in pairs.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/thermal_core.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c
> > index 5225d65fb0e0..9c447f22cb39 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -1258,6 +1258,11 @@
> > thermal_zone_device_register_with_trips(const char *type, struct
> > thermal_trip *t
> >         if (num_trips > 0 && (!ops->get_trip_type || !ops-
> > >get_trip_temp) && !trips)
> >                 return ERR_PTR(-EINVAL);
> > 
> > +       if ((ops->bind && !ops->unbind) || (!ops->bind && ops-
> > >unbind)) {
> 
> This can be written as
> 
>         if (!!ops->bind != !!ops->unbind) {
> 
> > +               pr_err("Thermal zone device .bind/.unbind not
> > paired\n");
> 
> And surely none of the existing drivers do that?  Because it would be
> a functional regression if they did.

Yeah, I did a check and all drivers provide .bind/.unbind callbacks in
pairs.

Hi, Daniel,
I know you're dealing with various of thermal drivers recently, are you
aware of any exceptions?

thanks,
rui

Re: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration

[Rafael J. Wysocki]

On Mon, Mar 27, 2023 at 4:50 PM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Fri, 2023-03-24 at 14:19 +0100, Rafael J. Wysocki wrote:
> > On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > > When unregistering a cooling device, it is possible that the
> > > cooling
> > > device has been activated. And once the cooling device is
> > > unregistered,
> > > no one will deactivate it anymore.
> > >
> > > Reset cooling state during cooling device unregistration.
> > >
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > > In theory, this problem that this patch fixes can be triggered on a
> > > platform with ACPI Active cooling, by
> > > 1. overheat the system to trigger ACPI active cooling
> > > 2. unload ACPI fan driver
> > > 3. check if the fan is still spinning
> > > But I don't have such a system so I didn't trigger then problem and
> > > I
> > > only did build & boot test.
> >
> > So I'm not sure if this change is actually safe.
> >
> > In the example above, the system will still need the fan to spin
> > after
> > the ACPI fan driver is unloaded in order to cool down, won't it?
>
> Then we can argue that the ACPI fan driver should not be unloaded in
> this case.

I don't think that whether or not the driver is expected to be
unloaded at a given time has any bearing on how it should behave when
actually unloaded.

Leaving the cooling device in its current state is "safe" from the
thermal control perspective, but it may affect the general user
experience (which may include performance too) going forward, so there
is a tradeoff.

You can argue that even if the cooling device is reset on the driver
removal, there should be another thermal control mechanism in place
that will take care of the overheat condition instead of it, but that
mechanism may be an emergency system shutdown.

What do the other cooling device drivers do in general when they get removed?

> Actually, this is the same situation as patch 1/5.
> Patch 1/5 fixes the problem that cooling state not restored to 0 when
> unloading the thermal driver, and this fixes the same problem when
> unloading the cooling device driver.

Right, it is analogous.

Re: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration

[Zhang, Rui]

On Mon, 2023-03-27 at 17:13 +0200, Rafael J. Wysocki wrote:
> On Mon, Mar 27, 2023 at 4:50 PM Zhang, Rui <rui.zhang@intel.com>
> wrote:
> > On Fri, 2023-03-24 at 14:19 +0100, Rafael J. Wysocki wrote:
> > > On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > When unregistering a cooling device, it is possible that the
> > > > cooling
> > > > device has been activated. And once the cooling device is
> > > > unregistered,
> > > > no one will deactivate it anymore.
> > > > 
> > > > Reset cooling state during cooling device unregistration.
> > > > 
> > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > ---
> > > > In theory, this problem that this patch fixes can be triggered
> > > > on a
> > > > platform with ACPI Active cooling, by
> > > > 1. overheat the system to trigger ACPI active cooling
> > > > 2. unload ACPI fan driver
> > > > 3. check if the fan is still spinning
> > > > But I don't have such a system so I didn't trigger then problem
> > > > and
> > > > I
> > > > only did build & boot test.
> > > 
> > > So I'm not sure if this change is actually safe.
> > > 
> > > In the example above, the system will still need the fan to spin
> > > after
> > > the ACPI fan driver is unloaded in order to cool down, won't it?
> > 
> > Then we can argue that the ACPI fan driver should not be unloaded
> > in
> > this case.
> 
> I don't think that whether or not the driver is expected to be
> unloaded at a given time has any bearing on how it should behave when
> actually unloaded.
> 
> Leaving the cooling device in its current state is "safe" from the
> thermal control perspective, but it may affect the general user
> experience (which may include performance too) going forward, so
> there
> is a tradeoff.

Right.
If we don't have a third choice, then the question is simple.
"thermal safety" vs. "user experience"?

I'd vote for "thermal safety" and drop this patch series.
> 
> What do the other cooling device drivers do in general when they get
> removed?

No cooling device driver has extra handling after cdev unregistration.

thanks,
rui

Re: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration

[Rafael J. Wysocki]

On Tue, Mar 28, 2023 at 4:46 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Mon, 2023-03-27 at 17:13 +0200, Rafael J. Wysocki wrote:
> > On Mon, Mar 27, 2023 at 4:50 PM Zhang, Rui <rui.zhang@intel.com>
> > wrote:
> > > On Fri, 2023-03-24 at 14:19 +0100, Rafael J. Wysocki wrote:
> > > > On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com>
> > > > wrote:
> > > > > When unregistering a cooling device, it is possible that the
> > > > > cooling
> > > > > device has been activated. And once the cooling device is
> > > > > unregistered,
> > > > > no one will deactivate it anymore.
> > > > >
> > > > > Reset cooling state during cooling device unregistration.
> > > > >
> > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > > ---
> > > > > In theory, this problem that this patch fixes can be triggered
> > > > > on a
> > > > > platform with ACPI Active cooling, by
> > > > > 1. overheat the system to trigger ACPI active cooling
> > > > > 2. unload ACPI fan driver
> > > > > 3. check if the fan is still spinning
> > > > > But I don't have such a system so I didn't trigger then problem
> > > > > and
> > > > > I
> > > > > only did build & boot test.
> > > >
> > > > So I'm not sure if this change is actually safe.
> > > >
> > > > In the example above, the system will still need the fan to spin
> > > > after
> > > > the ACPI fan driver is unloaded in order to cool down, won't it?
> > >
> > > Then we can argue that the ACPI fan driver should not be unloaded
> > > in
> > > this case.
> >
> > I don't think that whether or not the driver is expected to be
> > unloaded at a given time has any bearing on how it should behave when
> > actually unloaded.
> >
> > Leaving the cooling device in its current state is "safe" from the
> > thermal control perspective, but it may affect the general user
> > experience (which may include performance too) going forward, so
> > there
> > is a tradeoff.
>
> Right.
> If we don't have a third choice, then the question is simple.
> "thermal safety" vs. "user experience"?
>
> I'd vote for "thermal safety" and drop this patch series.

Works for me.

> > What do the other cooling device drivers do in general when they get
> > removed?
>
> No cooling device driver has extra handling after cdev unregistration.

However, the question regarding what to do when the driver of a
cooling device in use is being removed is a valid one.

One possible approach that comes to mind could be to defer the driver
removal until the overheat condition goes away, but anyway it would be
better to do that in the core IMV.

Re: [PATCH 2/5] thermal/core: Reset cooling state during cooling device unregistration

[Zhang, Rui]

On Tue, 2023-03-28 at 19:54 +0200, Rafael J. Wysocki wrote:
> > > What do the other cooling device drivers do in general when they
> > > get
> > > removed?
> > 
> > No cooling device driver has extra handling after cdev
> > unregistration.
> 
> However, the question regarding what to do when the driver of a
> cooling device in use is being removed is a valid one.
> 
> One possible approach that comes to mind could be to defer the driver
> removal until the overheat condition goes away, but anyway it would
> be
> better to do that in the core IMV.

In this case, we should guarantee that the thermal zone driver is still
functional. i.e. it still can get temperature change notifications and
update the thermal zone. I doubt if current thermal zone drivers can
guarantee this.

Given that this is a rare case, and the current behavior is not perfect
but still acceptable, maybe we can leave this low priority for now.

thanks,
rui

Re: [PATCH 3/5] thermal/core: Remove thermal_bind_params structure

[Rafael J. Wysocki]

On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> Remove struct thermal_bind_params because no one is using it for thermal
> binding now.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

I think that this patch is useful by itself.  Does it depend on the
rest of the series?

> ---
>  .../driver-api/thermal/sysfs-api.rst          |  40 ------
>  drivers/thermal/thermal_core.c                | 134 ++----------------
>  include/linux/thermal.h                       |  38 -----
>  3 files changed, 14 insertions(+), 198 deletions(-)
>
> diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> index 2e0f79a9e2ee..6c1175c6afba 100644
> --- a/Documentation/driver-api/thermal/sysfs-api.rst
> +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> @@ -304,42 +304,6 @@ temperature) and throttle appropriate devices.
>  1.4 Thermal Zone Parameters
>  ---------------------------
>
> -    ::
> -
> -       struct thermal_bind_params
> -
> -    This structure defines the following parameters that are used to bind
> -    a zone with a cooling device for a particular trip point.
> -
> -    .cdev:
> -            The cooling device pointer
> -    .weight:
> -            The 'influence' of a particular cooling device on this
> -            zone. This is relative to the rest of the cooling
> -            devices. For example, if all cooling devices have a
> -            weight of 1, then they all contribute the same. You can
> -            use percentages if you want, but it's not mandatory. A
> -            weight of 0 means that this cooling device doesn't
> -            contribute to the cooling of this zone unless all cooling
> -            devices have a weight of 0. If all weights are 0, then
> -            they all contribute the same.
> -    .trip_mask:
> -              This is a bit mask that gives the binding relation between
> -              this thermal zone and cdev, for a particular trip point.
> -              If nth bit is set, then the cdev and thermal zone are bound
> -              for trip point n.
> -    .binding_limits:
> -                    This is an array of cooling state limits. Must have
> -                    exactly 2 * thermal_zone.number_of_trip_points. It is an
> -                    array consisting of tuples <lower-state upper-state> of
> -                    state limits. Each trip will be associated with one state
> -                    limit tuple when binding. A NULL pointer means
> -                    <THERMAL_NO_LIMITS THERMAL_NO_LIMITS> on all trips.
> -                    These limits are used when binding a cdev to a trip point.
> -    .match:
> -           This call back returns success(0) if the 'tz and cdev' need to
> -           be bound, as per platform data.
> -
>      ::
>
>         struct thermal_zone_params
> @@ -357,10 +321,6 @@ temperature) and throttle appropriate devices.
>                will be created. when no_hwmon == true, nothing will be done.
>                In case the thermal_zone_params is NULL, the hwmon interface
>                will be created (for backward compatibility).
> -    .num_tbps:
> -              Number of thermal_bind_params entries for this zone
> -    .tbp:
> -              thermal_bind_params entries
>
>  2. sysfs attributes structure
>  =============================
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index fd54e6c10b60..5225d65fb0e0 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -794,65 +794,20 @@ void print_bind_err_msg(struct thermal_zone_device *tz,
>                 tz->type, cdev->type, ret);
>  }
>
> -static void __bind(struct thermal_zone_device *tz, int mask,
> -                  struct thermal_cooling_device *cdev,
> -                  unsigned long *limits,
> -                  unsigned int weight)
> -{
> -       int i, ret;
> -
> -       for (i = 0; i < tz->num_trips; i++) {
> -               if (mask & (1 << i)) {
> -                       unsigned long upper, lower;
> -
> -                       upper = THERMAL_NO_LIMIT;
> -                       lower = THERMAL_NO_LIMIT;
> -                       if (limits) {
> -                               lower = limits[i * 2];
> -                               upper = limits[i * 2 + 1];
> -                       }
> -                       ret = thermal_zone_bind_cooling_device(tz, i, cdev,
> -                                                              upper, lower,
> -                                                              weight);
> -                       if (ret)
> -                               print_bind_err_msg(tz, cdev, ret);
> -               }
> -       }
> -}
> -
>  static void bind_cdev(struct thermal_cooling_device *cdev)
>  {
> -       int i, ret;
> -       const struct thermal_zone_params *tzp;
> +       int ret;
>         struct thermal_zone_device *pos = NULL;
>
>         mutex_lock(&thermal_list_lock);
>
>         list_for_each_entry(pos, &thermal_tz_list, node) {
> -               if (!pos->tzp && !pos->ops->bind)
> +               if (!pos->ops->bind)
>                         continue;
>
> -               if (pos->ops->bind) {
> -                       ret = pos->ops->bind(pos, cdev);
> -                       if (ret)
> -                               print_bind_err_msg(pos, cdev, ret);
> -                       continue;
> -               }
> -
> -               tzp = pos->tzp;
> -               if (!tzp || !tzp->tbp)
> -                       continue;
> -
> -               for (i = 0; i < tzp->num_tbps; i++) {
> -                       if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
> -                               continue;
> -                       if (tzp->tbp[i].match(pos, cdev))
> -                               continue;
> -                       tzp->tbp[i].cdev = cdev;
> -                       __bind(pos, tzp->tbp[i].trip_mask, cdev,
> -                              tzp->tbp[i].binding_limits,
> -                              tzp->tbp[i].weight);
> -               }
> +               ret = pos->ops->bind(pos, cdev);
> +               if (ret)
> +                       print_bind_err_msg(pos, cdev, ret);
>         }
>
>         mutex_unlock(&thermal_list_lock);
> @@ -1138,16 +1093,6 @@ void thermal_cooling_device_update(struct thermal_cooling_device *cdev)
>  }
>  EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
>
> -static void __unbind(struct thermal_zone_device *tz, int mask,
> -                    struct thermal_cooling_device *cdev)
> -{
> -       int i;
> -
> -       for (i = 0; i < tz->num_trips; i++)
> -               if (mask & (1 << i))
> -                       thermal_zone_unbind_cooling_device(tz, i, cdev);
> -}
> -
>  /**
>   * thermal_cooling_device_unregister - removes a thermal cooling device
>   * @cdev:      the thermal cooling device to remove.
> @@ -1157,8 +1102,6 @@ static void __unbind(struct thermal_zone_device *tz, int mask,
>   */
>  void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
>  {
> -       int i;
> -       const struct thermal_zone_params *tzp;
>         struct thermal_zone_device *tz;
>
>         if (!cdev)
> @@ -1175,21 +1118,8 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
>
>         /* Unbind all thermal zones associated with 'this' cdev */
>         list_for_each_entry(tz, &thermal_tz_list, node) {
> -               if (tz->ops->unbind) {
> +               if (tz->ops->unbind)
>                         tz->ops->unbind(tz, cdev);
> -                       continue;
> -               }
> -
> -               if (!tz->tzp || !tz->tzp->tbp)
> -                       continue;
> -
> -               tzp = tz->tzp;
> -               for (i = 0; i < tzp->num_tbps; i++) {
> -                       if (tzp->tbp[i].cdev == cdev) {
> -                               __unbind(tz, tzp->tbp[i].trip_mask, cdev);
> -                               tzp->tbp[i].cdev = NULL;
> -                       }
> -               }
>         }
>
>         mutex_lock(&cdev->lock);
> @@ -1204,41 +1134,20 @@ EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
>
>  static void bind_tz(struct thermal_zone_device *tz)
>  {
> -       int i, ret;
> +       int ret;
>         struct thermal_cooling_device *pos = NULL;
> -       const struct thermal_zone_params *tzp = tz->tzp;
>
> -       if (!tzp && !tz->ops->bind)
> +       if (!tz->ops->bind)
>                 return;
>
>         mutex_lock(&thermal_list_lock);
>
> -       /* If there is ops->bind, try to use ops->bind */
> -       if (tz->ops->bind) {
> -               list_for_each_entry(pos, &thermal_cdev_list, node) {
> -                       ret = tz->ops->bind(tz, pos);
> -                       if (ret)
> -                               print_bind_err_msg(tz, pos, ret);
> -               }
> -               goto exit;
> -       }
> -
> -       if (!tzp || !tzp->tbp)
> -               goto exit;
> -
>         list_for_each_entry(pos, &thermal_cdev_list, node) {
> -               for (i = 0; i < tzp->num_tbps; i++) {
> -                       if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
> -                               continue;
> -                       if (tzp->tbp[i].match(tz, pos))
> -                               continue;
> -                       tzp->tbp[i].cdev = pos;
> -                       __bind(tz, tzp->tbp[i].trip_mask, pos,
> -                              tzp->tbp[i].binding_limits,
> -                              tzp->tbp[i].weight);
> -               }
> +               ret = tz->ops->bind(tz, pos);
> +               if (ret)
> +                       print_bind_err_msg(tz, pos, ret);
>         }
> -exit:
> +
>         mutex_unlock(&thermal_list_lock);
>  }
>
> @@ -1477,15 +1386,13 @@ EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>   */
>  void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  {
> -       int i, tz_id;
> -       const struct thermal_zone_params *tzp;
> +       int tz_id;
>         struct thermal_cooling_device *cdev;
>         struct thermal_zone_device *pos = NULL;
>
>         if (!tz)
>                 return;
>
> -       tzp = tz->tzp;
>         tz_id = tz->id;
>
>         mutex_lock(&thermal_list_lock);
> @@ -1516,22 +1423,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>                 continue;
>
>  unbind:
> -               if (tz->ops->unbind) {
> +               if (tz->ops->unbind)
>                         tz->ops->unbind(tz, cdev);
> -                       goto deactivate;
> -               }
> -
> -               if (!tzp || !tzp->tbp)
> -                       break;
> -
> -               for (i = 0; i < tzp->num_tbps; i++) {
> -                       if (tzp->tbp[i].cdev == cdev) {
> -                               __unbind(tz, tzp->tbp[i].trip_mask, cdev);
> -                               tzp->tbp[i].cdev = NULL;
> -                       }
> -               }
>
> -deactivate:
>                 /*
>                  * The thermal instances for current thermal zone has been
>                  * removed. Update the cooling device in case it is activated
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 13c6aaed18df..481417d3b9b7 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -207,41 +207,6 @@ struct thermal_governor {
>         struct list_head        governor_list;
>  };
>
> -/* Structure that holds binding parameters for a zone */
> -struct thermal_bind_params {
> -       struct thermal_cooling_device *cdev;
> -
> -       /*
> -        * This is a measure of 'how effectively these devices can
> -        * cool 'this' thermal zone. It shall be determined by
> -        * platform characterization. This value is relative to the
> -        * rest of the weights so a cooling device whose weight is
> -        * double that of another cooling device is twice as
> -        * effective. See Documentation/driver-api/thermal/sysfs-api.rst for more
> -        * information.
> -        */
> -       int weight;
> -
> -       /*
> -        * This is a bit mask that gives the binding relation between this
> -        * thermal zone and cdev, for a particular trip point.
> -        * See Documentation/driver-api/thermal/sysfs-api.rst for more information.
> -        */
> -       int trip_mask;
> -
> -       /*
> -        * This is an array of cooling state limits. Must have exactly
> -        * 2 * thermal_zone.number_of_trip_points. It is an array consisting
> -        * of tuples <lower-state upper-state> of state limits. Each trip
> -        * will be associated with one state limit tuple when binding.
> -        * A NULL pointer means <THERMAL_NO_LIMITS THERMAL_NO_LIMITS>
> -        * on all trips.
> -        */
> -       unsigned long *binding_limits;
> -       int (*match) (struct thermal_zone_device *tz,
> -                       struct thermal_cooling_device *cdev);
> -};
> -
>  /* Structure to define Thermal Zone parameters */
>  struct thermal_zone_params {
>         char governor_name[THERMAL_NAME_LENGTH];
> @@ -253,9 +218,6 @@ struct thermal_zone_params {
>          */
>         bool no_hwmon;
>
> -       int num_tbps;   /* Number of tbp entries */
> -       struct thermal_bind_params *tbp;
> -
>         /*
>          * Sustainable power (heat) that this thermal zone can dissipate in
>          * mW
> --
> 2.25.1
>

Re: [PATCH 3/5] thermal/core: Remove thermal_bind_params structure

[Zhang, Rui]

On Wed, 2023-03-29 at 20:39 +0200, Rafael J. Wysocki wrote:
> On Fri, Mar 24, 2023 at 8:08 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > Remove struct thermal_bind_params because no one is using it for
> > thermal
> > binding now.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> I think that this patch is useful by itself.  Does it depend on the
> rest of the series?

Yes. But I have refreshed it and will submit soon.

thanks,
rui
> 
> > ---
> >  .../driver-api/thermal/sysfs-api.rst          |  40 ------
> >  drivers/thermal/thermal_core.c                | 134 ++----------
> > ------
> >  include/linux/thermal.h                       |  38 -----
> >  3 files changed, 14 insertions(+), 198 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/thermal/sysfs-api.rst
> > b/Documentation/driver-api/thermal/sysfs-api.rst
> > index 2e0f79a9e2ee..6c1175c6afba 100644
> > --- a/Documentation/driver-api/thermal/sysfs-api.rst
> > +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> > @@ -304,42 +304,6 @@ temperature) and throttle appropriate devices.
> >  1.4 Thermal Zone Parameters
> >  ---------------------------
> > 
> > -    ::
> > -
> > -       struct thermal_bind_params
> > -
> > -    This structure defines the following parameters that are used
> > to bind
> > -    a zone with a cooling device for a particular trip point.
> > -
> > -    .cdev:
> > -            The cooling device pointer
> > -    .weight:
> > -            The 'influence' of a particular cooling device on this
> > -            zone. This is relative to the rest of the cooling
> > -            devices. For example, if all cooling devices have a
> > -            weight of 1, then they all contribute the same. You
> > can
> > -            use percentages if you want, but it's not mandatory. A
> > -            weight of 0 means that this cooling device doesn't
> > -            contribute to the cooling of this zone unless all
> > cooling
> > -            devices have a weight of 0. If all weights are 0, then
> > -            they all contribute the same.
> > -    .trip_mask:
> > -              This is a bit mask that gives the binding relation
> > between
> > -              this thermal zone and cdev, for a particular trip
> > point.
> > -              If nth bit is set, then the cdev and thermal zone
> > are bound
> > -              for trip point n.
> > -    .binding_limits:
> > -                    This is an array of cooling state limits. Must
> > have
> > -                    exactly 2 *
> > thermal_zone.number_of_trip_points. It is an
> > -                    array consisting of tuples <lower-state upper-
> > state> of
> > -                    state limits. Each trip will be associated
> > with one state
> > -                    limit tuple when binding. A NULL pointer means
> > -                    <THERMAL_NO_LIMITS THERMAL_NO_LIMITS> on all
> > trips.
> > -                    These limits are used when binding a cdev to a
> > trip point.
> > -    .match:
> > -           This call back returns success(0) if the 'tz and cdev'
> > need to
> > -           be bound, as per platform data.
> > -
> >      ::
> > 
> >         struct thermal_zone_params
> > @@ -357,10 +321,6 @@ temperature) and throttle appropriate devices.
> >                will be created. when no_hwmon == true, nothing will
> > be done.
> >                In case the thermal_zone_params is NULL, the hwmon
> > interface
> >                will be created (for backward compatibility).
> > -    .num_tbps:
> > -              Number of thermal_bind_params entries for this zone
> > -    .tbp:
> > -              thermal_bind_params entries
> > 
> >  2. sysfs attributes structure
> >  =============================
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c
> > index fd54e6c10b60..5225d65fb0e0 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -794,65 +794,20 @@ void print_bind_err_msg(struct
> > thermal_zone_device *tz,
> >                 tz->type, cdev->type, ret);
> >  }
> > 
> > -static void __bind(struct thermal_zone_device *tz, int mask,
> > -                  struct thermal_cooling_device *cdev,
> > -                  unsigned long *limits,
> > -                  unsigned int weight)
> > -{
> > -       int i, ret;
> > -
> > -       for (i = 0; i < tz->num_trips; i++) {
> > -               if (mask & (1 << i)) {
> > -                       unsigned long upper, lower;
> > -
> > -                       upper = THERMAL_NO_LIMIT;
> > -                       lower = THERMAL_NO_LIMIT;
> > -                       if (limits) {
> > -                               lower = limits[i * 2];
> > -                               upper = limits[i * 2 + 1];
> > -                       }
> > -                       ret = thermal_zone_bind_cooling_device(tz,
> > i, cdev,
> > -                                                              uppe
> > r, lower,
> > -                                                              weig
> > ht);
> > -                       if (ret)
> > -                               print_bind_err_msg(tz, cdev, ret);
> > -               }
> > -       }
> > -}
> > -
> >  static void bind_cdev(struct thermal_cooling_device *cdev)
> >  {
> > -       int i, ret;
> > -       const struct thermal_zone_params *tzp;
> > +       int ret;
> >         struct thermal_zone_device *pos = NULL;
> > 
> >         mutex_lock(&thermal_list_lock);
> > 
> >         list_for_each_entry(pos, &thermal_tz_list, node) {
> > -               if (!pos->tzp && !pos->ops->bind)
> > +               if (!pos->ops->bind)
> >                         continue;
> > 
> > -               if (pos->ops->bind) {
> > -                       ret = pos->ops->bind(pos, cdev);
> > -                       if (ret)
> > -                               print_bind_err_msg(pos, cdev, ret);
> > -                       continue;
> > -               }
> > -
> > -               tzp = pos->tzp;
> > -               if (!tzp || !tzp->tbp)
> > -                       continue;
> > -
> > -               for (i = 0; i < tzp->num_tbps; i++) {
> > -                       if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
> > -                               continue;
> > -                       if (tzp->tbp[i].match(pos, cdev))
> > -                               continue;
> > -                       tzp->tbp[i].cdev = cdev;
> > -                       __bind(pos, tzp->tbp[i].trip_mask, cdev,
> > -                              tzp->tbp[i].binding_limits,
> > -                              tzp->tbp[i].weight);
> > -               }
> > +               ret = pos->ops->bind(pos, cdev);
> > +               if (ret)
> > +                       print_bind_err_msg(pos, cdev, ret);
> >         }
> > 
> >         mutex_unlock(&thermal_list_lock);
> > @@ -1138,16 +1093,6 @@ void thermal_cooling_device_update(struct
> > thermal_cooling_device *cdev)
> >  }
> >  EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
> > 
> > -static void __unbind(struct thermal_zone_device *tz, int mask,
> > -                    struct thermal_cooling_device *cdev)
> > -{
> > -       int i;
> > -
> > -       for (i = 0; i < tz->num_trips; i++)
> > -               if (mask & (1 << i))
> > -                       thermal_zone_unbind_cooling_device(tz, i,
> > cdev);
> > -}
> > -
> >  /**
> >   * thermal_cooling_device_unregister - removes a thermal cooling
> > device
> >   * @cdev:      the thermal cooling device to remove.
> > @@ -1157,8 +1102,6 @@ static void __unbind(struct
> > thermal_zone_device *tz, int mask,
> >   */
> >  void thermal_cooling_device_unregister(struct
> > thermal_cooling_device *cdev)
> >  {
> > -       int i;
> > -       const struct thermal_zone_params *tzp;
> >         struct thermal_zone_device *tz;
> > 
> >         if (!cdev)
> > @@ -1175,21 +1118,8 @@ void
> > thermal_cooling_device_unregister(struct thermal_cooling_device
> > *cdev)
> > 
> >         /* Unbind all thermal zones associated with 'this' cdev */
> >         list_for_each_entry(tz, &thermal_tz_list, node) {
> > -               if (tz->ops->unbind) {
> > +               if (tz->ops->unbind)
> >                         tz->ops->unbind(tz, cdev);
> > -                       continue;
> > -               }
> > -
> > -               if (!tz->tzp || !tz->tzp->tbp)
> > -                       continue;
> > -
> > -               tzp = tz->tzp;
> > -               for (i = 0; i < tzp->num_tbps; i++) {
> > -                       if (tzp->tbp[i].cdev == cdev) {
> > -                               __unbind(tz, tzp->tbp[i].trip_mask, 
> > cdev);
> > -                               tzp->tbp[i].cdev = NULL;
> > -                       }
> > -               }
> >         }
> > 
> >         mutex_lock(&cdev->lock);
> > @@ -1204,41 +1134,20 @@
> > EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
> > 
> >  static void bind_tz(struct thermal_zone_device *tz)
> >  {
> > -       int i, ret;
> > +       int ret;
> >         struct thermal_cooling_device *pos = NULL;
> > -       const struct thermal_zone_params *tzp = tz->tzp;
> > 
> > -       if (!tzp && !tz->ops->bind)
> > +       if (!tz->ops->bind)
> >                 return;
> > 
> >         mutex_lock(&thermal_list_lock);
> > 
> > -       /* If there is ops->bind, try to use ops->bind */
> > -       if (tz->ops->bind) {
> > -               list_for_each_entry(pos, &thermal_cdev_list, node)
> > {
> > -                       ret = tz->ops->bind(tz, pos);
> > -                       if (ret)
> > -                               print_bind_err_msg(tz, pos, ret);
> > -               }
> > -               goto exit;
> > -       }
> > -
> > -       if (!tzp || !tzp->tbp)
> > -               goto exit;
> > -
> >         list_for_each_entry(pos, &thermal_cdev_list, node) {
> > -               for (i = 0; i < tzp->num_tbps; i++) {
> > -                       if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
> > -                               continue;
> > -                       if (tzp->tbp[i].match(tz, pos))
> > -                               continue;
> > -                       tzp->tbp[i].cdev = pos;
> > -                       __bind(tz, tzp->tbp[i].trip_mask, pos,
> > -                              tzp->tbp[i].binding_limits,
> > -                              tzp->tbp[i].weight);
> > -               }
> > +               ret = tz->ops->bind(tz, pos);
> > +               if (ret)
> > +                       print_bind_err_msg(tz, pos, ret);
> >         }
> > -exit:
> > +
> >         mutex_unlock(&thermal_list_lock);
> >  }
> > 
> > @@ -1477,15 +1386,13 @@
> > EXPORT_SYMBOL_GPL(thermal_zone_device_register);
> >   */
> >  void thermal_zone_device_unregister(struct thermal_zone_device
> > *tz)
> >  {
> > -       int i, tz_id;
> > -       const struct thermal_zone_params *tzp;
> > +       int tz_id;
> >         struct thermal_cooling_device *cdev;
> >         struct thermal_zone_device *pos = NULL;
> > 
> >         if (!tz)
> >                 return;
> > 
> > -       tzp = tz->tzp;
> >         tz_id = tz->id;
> > 
> >         mutex_lock(&thermal_list_lock);
> > @@ -1516,22 +1423,9 @@ void thermal_zone_device_unregister(struct
> > thermal_zone_device *tz)
> >                 continue;
> > 
> >  unbind:
> > -               if (tz->ops->unbind) {
> > +               if (tz->ops->unbind)
> >                         tz->ops->unbind(tz, cdev);
> > -                       goto deactivate;
> > -               }
> > -
> > -               if (!tzp || !tzp->tbp)
> > -                       break;
> > -
> > -               for (i = 0; i < tzp->num_tbps; i++) {
> > -                       if (tzp->tbp[i].cdev == cdev) {
> > -                               __unbind(tz, tzp->tbp[i].trip_mask, 
> > cdev);
> > -                               tzp->tbp[i].cdev = NULL;
> > -                       }
> > -               }
> > 
> > -deactivate:
> >                 /*
> >                  * The thermal instances for current thermal zone
> > has been
> >                  * removed. Update the cooling device in case it is
> > activated
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 13c6aaed18df..481417d3b9b7 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -207,41 +207,6 @@ struct thermal_governor {
> >         struct list_head        governor_list;
> >  };
> > 
> > -/* Structure that holds binding parameters for a zone */
> > -struct thermal_bind_params {
> > -       struct thermal_cooling_device *cdev;
> > -
> > -       /*
> > -        * This is a measure of 'how effectively these devices can
> > -        * cool 'this' thermal zone. It shall be determined by
> > -        * platform characterization. This value is relative to the
> > -        * rest of the weights so a cooling device whose weight is
> > -        * double that of another cooling device is twice as
> > -        * effective. See Documentation/driver-api/thermal/sysfs-
> > api.rst for more
> > -        * information.
> > -        */
> > -       int weight;
> > -
> > -       /*
> > -        * This is a bit mask that gives the binding relation
> > between this
> > -        * thermal zone and cdev, for a particular trip point.
> > -        * See Documentation/driver-api/thermal/sysfs-api.rst for
> > more information.
> > -        */
> > -       int trip_mask;
> > -
> > -       /*
> > -        * This is an array of cooling state limits. Must have
> > exactly
> > -        * 2 * thermal_zone.number_of_trip_points. It is an array
> > consisting
> > -        * of tuples <lower-state upper-state> of state limits.
> > Each trip
> > -        * will be associated with one state limit tuple when
> > binding.
> > -        * A NULL pointer means <THERMAL_NO_LIMITS
> > THERMAL_NO_LIMITS>
> > -        * on all trips.
> > -        */
> > -       unsigned long *binding_limits;
> > -       int (*match) (struct thermal_zone_device *tz,
> > -                       struct thermal_cooling_device *cdev);
> > -};
> > -
> >  /* Structure to define Thermal Zone parameters */
> >  struct thermal_zone_params {
> >         char governor_name[THERMAL_NAME_LENGTH];
> > @@ -253,9 +218,6 @@ struct thermal_zone_params {
> >          */
> >         bool no_hwmon;
> > 
> > -       int num_tbps;   /* Number of tbp entries */
> > -       struct thermal_bind_params *tbp;
> > -
> >         /*
> >          * Sustainable power (heat) that this thermal zone can
> > dissipate in
> >          * mW
> > --
> > 2.25.1
> >