thermal: remove power allocator from list of default governors

As it currently stands the power allocator governor can not handle thermal zones that are not specifically crafted and therefore can not be used as a default governor.

Users need to explicitly enable this governor for thermal zones that do have enough information for its operation.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/thermal/Kconfig | 8 --------
 1 file changed, 8 deletions(-)

drivers/thermal/Kconfig +0 -0

index 0390044..34d05d3 100644
@@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE
 	  Select this if you want to let the user space manage the

Quoted 18 lines · Dmitry Torokhov

As it currently stands the power allocator governor can not handle thermal zones that are not specifically crafted and therefore can not be used as a default governor.

Users need to explicitly enable this governor for thermal zones that do have enough information for its operation.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/thermal/Kconfig | 8 --------
 1 file changed, 8 deletions(-)

drivers/thermal/Kconfig +0 -0

index 0390044..34d05d3 100644
@@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE
 	  Select this if you want to let the user space manage the

drivers/thermal/Kconfig config THERMAL_DEFAULT_GOV_USER_SPACE · line 92

Quoted 10 lines

drivers/thermal/Kconfig +0 -8

 	  platform thermals.
 
-config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR
-	bool "power_allocator"
-	select THERMAL_GOV_POWER_ALLOCATOR
-	help
-	  Select this if you want to control temperature based on
-	  system and device power allocation. This governor can only
-	  operate on cooling devices that implement the power API.
-

Currently the only way we have for a thermal zone configured from device tree to use a governor from the kernel boot is by using THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't have a workable thermal framework until userspace is up and running.

Would you rather have the power allocator governor accept every thermal zone?

Cheers,
Javi
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 20 lines · Dmitry Torokhov

As it currently stands the power allocator governor can not handle thermal zones that are not specifically crafted and therefore can not be used as a default governor.

Users need to explicitly enable this governor for thermal zones that do have enough information for its operation.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/thermal/Kconfig | 8 --------
 1 file changed, 8 deletions(-)

drivers/thermal/Kconfig +0 -0

index 0390044..34d05d3 100644
@@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE
 	  Select this if you want to let the user space manage the
 	  platform thermals.
 

drivers/thermal/Kconfig config THERMAL_DEFAULT_GOV_USER_SPACE · line 92

Quoted 8 lines

drivers/thermal/Kconfig +0 -8

-config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR
-	bool "power_allocator"
-	select THERMAL_GOV_POWER_ALLOCATOR
-	help
-	  Select this if you want to control temperature based on
-	  system and device power allocation. This governor can only
-	  operate on cooling devices that implement the power API.
-

Currently the only way we have for a thermal zone configured from device tree to use a governor from the kernel boot is by using THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't have a workable thermal framework until userspace is up and running.

Would you rather have the power allocator governor accept every thermal zone?

Quoted 21 lines · Javi Merino

Quoted 20 lines · Dmitry Torokhov

As it currently stands the power allocator governor can not handle thermal zones that are not specifically crafted and therefore can not be used as a default governor.

Users need to explicitly enable this governor for thermal zones that do have enough information for its operation.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/thermal/Kconfig | 8 --------
 1 file changed, 8 deletions(-)

drivers/thermal/Kconfig +0 -0

index 0390044..34d05d3 100644
@@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE
 	  Select this if you want to let the user space manage the
 	  platform thermals.
 

Quoted 13 lines

drivers/thermal/Kconfig config THERMAL_DEFAULT_GOV_USER_SPACE · line 92

Quoted 8 lines

drivers/thermal/Kconfig +0 -8

-config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR
-	bool "power_allocator"
-	select THERMAL_GOV_POWER_ALLOCATOR
-	help
-	  Select this if you want to control temperature based on
-	  system and device power allocation. This governor can only
-	  operate on cooling devices that implement the power API.
-

Currently the only way we have for a thermal zone configured from device tree to use a governor from the kernel boot is by using THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't have a workable thermal framework until userspace is up and running.

Why would step wise, or fair share be not workable (even if not optimal) thermal frameworks? It doe snot take that long to get to [early] userspace. Half of the boot time the thermal framework is not working anyway because half of the devices that can act as colling devices are not yet logically there.

Quoted 1 line

Quoted 2 lines

Would you rather have the power allocator governor accept every thermal zone?

If it is to be one of default governors then yes, it needs to be able to manage all thermal zones, the same way as the other 3 governors can, as far as I know.

Thanks.

Signature · 1 line

Dmitry

Quoted 43 lines · Dmitry Torokhov

Quoted 21 lines · Javi Merino

Quoted 20 lines · Dmitry Torokhov

As it currently stands the power allocator governor can not handle thermal zones that are not specifically crafted and therefore can not be used as a default governor.

Users need to explicitly enable this governor for thermal zones that do have enough information for its operation.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/thermal/Kconfig | 8 --------
 1 file changed, 8 deletions(-)

drivers/thermal/Kconfig +0 -0

index 0390044..34d05d3 100644
@@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE
 	  Select this if you want to let the user space manage the
 	  platform thermals.
 

Quoted 13 lines

drivers/thermal/Kconfig config THERMAL_DEFAULT_GOV_USER_SPACE · line 92

Quoted 8 lines

drivers/thermal/Kconfig +0 -8

-config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR
-	bool "power_allocator"
-	select THERMAL_GOV_POWER_ALLOCATOR
-	help
-	  Select this if you want to control temperature based on
-	  system and device power allocation. This governor can only
-	  operate on cooling devices that implement the power API.
-

Currently the only way we have for a thermal zone configured from device tree to use a governor from the kernel boot is by using THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't have a workable thermal framework until userspace is up and running.

Why would step wise, or fair share be not workable (even if not optimal) thermal frameworks? It doe snot take that long to get to [early] userspace. Half of the boot time the thermal framework is not working anyway because half of the devices that can act as colling devices are not yet logically there.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

Would you rather have the power allocator governor accept every thermal zone?

If it is to be one of default governors then yes, it needs to be able to manage all thermal zones, the same way as the other 3 governors can, as far as I know.

I believe fairshare also relies on platform data to be properly set, otherwise, it would not be functional. That's probably why I overlooked this point. That said, I would say, if we follow this logic, a similar patch is needed for fairshare.

Javi, ideally, the governor would need to have a workable default settings for any thermal zone that miss the platform settings. Do you think it would be doable for power allocator?

The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone.

While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---
 drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------
 drivers/thermal/thermal_core.c    | 28 ++++++++++++++++++
 include/linux/thermal.h           |  6 ++++
 3 files changed, 87 insertions(+), 9 deletions(-)

drivers/thermal/power_allocator.c +43 -6

index 63a448f9d93b..f78836c2da26 100644
@@ -73,6 +73,39 @@ struct power_allocator_params {
 };
 
 /**
+ * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
+ * @tz: thermal zone we are operating in
+ *
+ * For thermal zones that don't provide a sustainable_power in their
+ * thermal_zone_params, estimate one.  Calculate it using the minimum
+ * power of all the cooling devices as that gives a valid value that
+ * can give some degree of functionality.  For optimal performance of
+ * this governor, provide a sustainable_power in the thermal zone's
+ * thermal_zone_params.
+ */
+static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
+{
+	u32 sustainable_power = 0;
+	struct thermal_instance *instance;
+	struct power_allocator_params *params = tz->governor_data;
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		struct thermal_cooling_device *cdev = instance->cdev;
+		u32 min_power;
+
+		if (instance->trip != params->trip_max_desired_temperature)
+			continue;
+
+		if (power_actor_get_min_power(cdev, tz, &min_power))
+			continue;
+
+		sustainable_power += min_power;
+	}
+
+	return sustainable_power;
+}
+
+/**
  * pid_controller() - PID controller
  * @tz:	thermal zone we are operating in
  * @current_temp:	the current temperature in millicelsius

@@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 {
 	s64 p, i, d, power_range;
 	s32 err, max_power_frac;
+	u32 sustainable_power;
 	struct power_allocator_params *params = tz->governor_data;
 
 	max_power_frac = int_to_frac(max_allocatable_power);

@@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 
 	power_range = p + i + d;
 
+	sustainable_power = tz->tzp->sustainable_power ?:
+		estimate_sustainable_power(tz);
+
 	/* feed-forward the known sustainable dissipatable power */
-	power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
+	power_range = sustainable_power + frac_to_int(power_range);
 
 	power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
 

@@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	int ret;
 	struct power_allocator_params *params;
 	unsigned long switch_on_temp, control_temp;
-	u32 temperature_threshold;
+	u32 sustainable_power, temperature_threshold;
 
-	if (!tz->tzp || !tz->tzp->sustainable_power) {
-		dev_err(&tz->device,
-			"power_allocator: missing sustainable_power\n");
+	if (!tz->tzp)
 		return -EINVAL;
-	}
 
 	params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
 	if (!params)
 		return -ENOMEM;
 
+	if (!tz->tzp->sustainable_power)
+		dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
+
 	ret = get_governor_trips(tz, params);
 	if (ret) {
 		dev_err(&tz->device,

drivers/thermal/power_allocator.c +0 -0

 	tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000;
 	/*
 	 * The default for k_d and integral_cutoff is 0, so we can

drivers/thermal/thermal_core.c +28 -0

index 4ca211be4c0f..d26bc9e6f936 100644
@@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev,
 }
 
 /**
+ * power_actor_get_min_power() - get the mainimum power that a cdev can consume
+ * @cdev:	pointer to &thermal_cooling_device
+ * @tz:		a valid thermal zone device pointer
+ * @min_power:	pointer in which to store the minimum power
+ *
+ * Calculate the minimum power consumption in milliwats that the
+ * cooling device can currently consume and store it in @min_power.
+ *
+ * Return: 0 on success, -EINVAL if @cdev doesn't support the
+ * power_actor API or -E* on other error.
+ */
+int power_actor_get_min_power(struct thermal_cooling_device *cdev,
+			      struct thermal_zone_device *tz, u32 *min_power)
+{
+	unsigned long max_state;
+	int ret;
+
+	if (!cdev_is_power_actor(cdev))
+		return -EINVAL;
+
+	ret = cdev->ops->get_max_state(cdev, &max_state);
+	if (ret)
+		return ret;
+
+	return cdev->ops->state2power(cdev, tz, max_state, min_power);
+}
+
+/**
  * power_actor_set_power() - limit the maximum power that a cooling device can consume
  * @cdev:	pointer to &thermal_cooling_device
  * @instance:	thermal instance to update

include/linux/thermal.h +2 -0

index 037e9df2f610..f99d934d373a 100644
@@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
 
 int power_actor_get_max_power(struct thermal_cooling_device *,
 			      struct thermal_zone_device *tz, u32 *max_power);
+int power_actor_get_min_power(struct thermal_cooling_device *,
+			      struct thermal_zone_device *tz, u32 *min_power);
 int power_actor_set_power(struct thermal_cooling_device *,
 			  struct thermal_instance *, u32);
 struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,

include/linux/thermal.h +0 -0

 static inline int power_actor_set_power(struct thermal_cooling_device *cdev,
 			  struct thermal_instance *tz, u32 power)
 { return 0; }

Hi Javi,

A few nitpicks and a comment below.

Quoted 109 lines · Javi Merino

The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone.

While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---
 drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------
 drivers/thermal/thermal_core.c    | 28 ++++++++++++++++++
 include/linux/thermal.h           |  6 ++++
 3 files changed, 87 insertions(+), 9 deletions(-)

drivers/thermal/power_allocator.c +43 -6

index 63a448f9d93b..f78836c2da26 100644
@@ -73,6 +73,39 @@ struct power_allocator_params {
 };
 
 /**
+ * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
+ * @tz: thermal zone we are operating in
+ *
+ * For thermal zones that don't provide a sustainable_power in their
+ * thermal_zone_params, estimate one.  Calculate it using the minimum
+ * power of all the cooling devices as that gives a valid value that
+ * can give some degree of functionality.  For optimal performance of
+ * this governor, provide a sustainable_power in the thermal zone's
+ * thermal_zone_params.
+ */
+static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
+{
+	u32 sustainable_power = 0;
+	struct thermal_instance *instance;
+	struct power_allocator_params *params = tz->governor_data;
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		struct thermal_cooling_device *cdev = instance->cdev;
+		u32 min_power;
+
+		if (instance->trip != params->trip_max_desired_temperature)
+			continue;
+
+		if (power_actor_get_min_power(cdev, tz, &min_power))
+			continue;
+
+		sustainable_power += min_power;
+	}
+
+	return sustainable_power;
+}
+
+/**
  * pid_controller() - PID controller
  * @tz:	thermal zone we are operating in
  * @current_temp:	the current temperature in millicelsius

@@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 {
 	s64 p, i, d, power_range;
 	s32 err, max_power_frac;
+	u32 sustainable_power;
 	struct power_allocator_params *params = tz->governor_data;
 
 	max_power_frac = int_to_frac(max_allocatable_power);

@@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 
 	power_range = p + i + d;
 
+	sustainable_power = tz->tzp->sustainable_power ?:
+		estimate_sustainable_power(tz);
+
 	/* feed-forward the known sustainable dissipatable power */
-	power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
+	power_range = sustainable_power + frac_to_int(power_range);
 
 	power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
 

@@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	int ret;
 	struct power_allocator_params *params;
 	unsigned long switch_on_temp, control_temp;
-	u32 temperature_threshold;
+	u32 sustainable_power, temperature_threshold;
 
-	if (!tz->tzp || !tz->tzp->sustainable_power) {
-		dev_err(&tz->device,
-			"power_allocator: missing sustainable_power\n");
+	if (!tz->tzp)
 		return -EINVAL;
-	}
 
 	params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
 	if (!params)
 		return -ENOMEM;
 
+	if (!tz->tzp->sustainable_power)
+		dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
+
 	ret = get_governor_trips(tz, params);
 	if (ret) {
 		dev_err(&tz->device,

drivers/thermal/power_allocator.c static int power_allocator_bind(struct thermal_zone_device *tz) · line 490

Quoted 6 lines

drivers/thermal/power_allocator.c +3 -0

@@ -448,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	if (ret)
 		goto free;
 
+	/*
+	 * Provide an arbitrary sustainable_power to set the default
+	 * values of k_po and k_pu.  We can estimate sustainable_power

                                        ^
                                        can not

drivers/thermal/power_allocator.c static int power_allocator_bind(struct thermal_zone_device *tz) · line 493

Quoted 3 lines

drivers/thermal/power_allocator.c +3 -0

+	 * at this point because no cooling devices have been
+	 * registered yet.  By providing an arbitrary value we get
+	 * better defaults that setting k_po and k_pu to 0.

                           ^
                           than

Quoted 4 lines

drivers/thermal/power_allocator.c +2 -0

+	 */
+	sustainable_power = tz->tzp->sustainable_power ?: 2500;
 	temperature_threshold = control_temp - switch_on_temp;
 

drivers/thermal/power_allocator.c static int power_allocator_bind(struct thermal_zone_device *tz) · line 501

Quoted 7 lines

drivers/thermal/power_allocator.c +2 -3

 	tz->tzp->k_po = tz->tzp->k_po ?:
-		int_to_frac(tz->tzp->sustainable_power) / temperature_threshold;
+		int_to_frac(sustainable_power) / temperature_threshold;
 	tz->tzp->k_pu = tz->tzp->k_pu ?:
-		int_to_frac(2 * tz->tzp->sustainable_power) /
-		temperature_threshold;
+		int_to_frac(2 * sustainable_power) / temperature_threshold;

As we are being conservative with our estimation of sustainable power (sum of mins) when it is not explicitly specified, should we be conservative here and let the proportional terms, k_po and k_pu be zero as well?

Quoted 58 lines

drivers/thermal/power_allocator.c +0 -0

 	tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000;
 	/*
 	 * The default for k_d and integral_cutoff is 0, so we can

drivers/thermal/thermal_core.c +28 -0

index 4ca211be4c0f..d26bc9e6f936 100644
@@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev,
 }
 
 /**
+ * power_actor_get_min_power() - get the mainimum power that a cdev can consume
+ * @cdev:	pointer to &thermal_cooling_device
+ * @tz:		a valid thermal zone device pointer
+ * @min_power:	pointer in which to store the minimum power
+ *
+ * Calculate the minimum power consumption in milliwats that the
+ * cooling device can currently consume and store it in @min_power.
+ *
+ * Return: 0 on success, -EINVAL if @cdev doesn't support the
+ * power_actor API or -E* on other error.
+ */
+int power_actor_get_min_power(struct thermal_cooling_device *cdev,
+			      struct thermal_zone_device *tz, u32 *min_power)
+{
+	unsigned long max_state;
+	int ret;
+
+	if (!cdev_is_power_actor(cdev))
+		return -EINVAL;
+
+	ret = cdev->ops->get_max_state(cdev, &max_state);
+	if (ret)
+		return ret;
+
+	return cdev->ops->state2power(cdev, tz, max_state, min_power);
+}
+
+/**
  * power_actor_set_power() - limit the maximum power that a cooling device can consume
  * @cdev:	pointer to &thermal_cooling_device
  * @instance:	thermal instance to update

include/linux/thermal.h +2 -0

index 037e9df2f610..f99d934d373a 100644
@@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
 
 int power_actor_get_max_power(struct thermal_cooling_device *,
 			      struct thermal_zone_device *tz, u32 *max_power);
+int power_actor_get_min_power(struct thermal_cooling_device *,
+			      struct thermal_zone_device *tz, u32 *min_power);
 int power_actor_set_power(struct thermal_cooling_device *,
 			  struct thermal_instance *, u32);
 struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,

@@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
 static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev,
 			      struct thermal_zone_device *tz, u32 *max_power)

include/linux/thermal.h static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev) · line 427

Quoted 5 lines

include/linux/thermal.h +4 -0

 { return 0; }
+static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev,
+					    struct thermal_zone_device *tz,
+					    u32 *min_power)
+{ return -ENODEV; }

Perhaps return 0 like power_actor_get_max_power just above for consistency.

The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone.

While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---

The power allocator governor currently requires that the thermal zone has at least two passive trip points. If there aren't, the governor refuses to bind to the thermal zone.

This commit relaxes that requirement. Now the governor will bind to all thermal zones regardless of how many trip points they have.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---
 Documentation/thermal/power_allocator.txt |  2 +-
 drivers/thermal/power_allocator.c         | 91 +++++++++++++++++--------------
 2 files changed, 52 insertions(+), 41 deletions(-)

Documentation/thermal/power_allocator.txt +1 -1

index c3797b529991..a1ce2235f121 100644
@@ -4,7 +4,7 @@ Power allocator governor tunables
 Trip points
 -----------
 
-The governor requires the following two passive trip points:
+The governor works optimally with the following two passive trip points:
 
 1.  "switch on" trip point: temperature above which the governor
     control loop starts operating.  This is the first passive trip

drivers/thermal/power_allocator.c +4 -0

index f78836c2da26..257c9af20f22 100644
@@ -24,6 +24,8 @@
 
 #include "thermal_core.h"
 
+#define INVALID_TRIP -1
+
 #define FRAC_BITS 10
 #define int_to_frac(x) ((x) << FRAC_BITS)
 #define frac_to_int(x) ((x) >> FRAC_BITS)

@@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
  *             Used to calculate the derivative term.
  * @trip_switch_on:    first passive trip point of the thermal zone.  The
  *                     governor switches on when this trip point is crossed.
+ *                     If the thermal zone only has one passive trip point,
+ *                     @trip_switch_on should be INVALID_TRIP.
  * @trip_max_desired_temperature:      last passive trip point of the thermal
  *                                     zone.  The temperature we are
  *                                     controlling for.

Hi Javi,

Quoted 54 lines

The power allocator governor currently requires that the thermal zone has at least two passive trip points. If there aren't, the governor refuses to bind to the thermal zone.

This commit relaxes that requirement. Now the governor will bind to all thermal zones regardless of how many trip points they have.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---
 Documentation/thermal/power_allocator.txt |  2 +-
 drivers/thermal/power_allocator.c         | 91 +++++++++++++++++--------------
 2 files changed, 52 insertions(+), 41 deletions(-)

Documentation/thermal/power_allocator.txt +1 -1

index c3797b529991..a1ce2235f121 100644
@@ -4,7 +4,7 @@ Power allocator governor tunables
 Trip points
 -----------
 
-The governor requires the following two passive trip points:
+The governor works optimally with the following two passive trip points:
 
 1.  "switch on" trip point: temperature above which the governor
     control loop starts operating.  This is the first passive trip

drivers/thermal/power_allocator.c +4 -0

index f78836c2da26..257c9af20f22 100644
@@ -24,6 +24,8 @@
 
 #include "thermal_core.h"
 
+#define INVALID_TRIP -1
+
 #define FRAC_BITS 10
 #define int_to_frac(x) ((x) << FRAC_BITS)
 #define frac_to_int(x) ((x) >> FRAC_BITS)

@@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
  *             Used to calculate the derivative term.
  * @trip_switch_on:    first passive trip point of the thermal zone.  The
  *                     governor switches on when this trip point is crossed.
+ *                     If the thermal zone only has one passive trip point,
+ *                     @trip_switch_on should be INVALID_TRIP.
  * @trip_max_desired_temperature:      last passive trip point of the thermal
  *                                     zone.  The temperature we are
  *                                     controlling for.

@@ -378,43 +382,66 @@ unlock:
        return ret;
 }
 

drivers/thermal/power_allocator.c unlock: · line 388

Quoted 6 lines

drivers/thermal/power_allocator.c +4 -2

-static int get_governor_trips(struct thermal_zone_device *tz,
-                             struct power_allocator_params *params)
+/**
+ * get_governor_trips() - get the number of the two trip points that are key for this governor
+ * @tz:        thermal zone to operate on
+ * @params:    pointer the private data for this governor

^
pointer to private data

drivers/thermal/power_allocator.c unlock: · line 398

Quoted 10 lines

drivers/thermal/power_allocator.c +10 -0

+ *
+ * The power allocator governor works optimally with two trips points:
+ * a "switch on" trip point and a "maximum desired temperature".  These
+ * are defined as the first and last passive trip points.
+ *
+ * If there is only one trip point, then that's considered to be the
+ * "maximum desired temperature" trip point and the governor is always
+ * on.  If there are no passive or active trip points, then the
+ * governor won't do anything.  In fact, its throttle function
+ * shouldn't be called at all.

      ^
      "shouldn't" or "won't" ?

[snip]

Quoted 3 lines

static void power_allocator_unbind(struct thermal_zone_device *tz)

drivers/thermal/power_allocator.c +0 -0

@@ -544,13 +561,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
 

drivers/thermal/power_allocator.c static int power_allocator_throttle(struct thermal_zone_device *tz, int trip) · line 564

Quoted 10 lines

drivers/thermal/power_allocator.c +1 -7

        ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
                                     &switch_on_temp);
-       if (ret) {
-               dev_warn(&tz->device,
-                        "Failed to get switch on temperature: %d\n", ret);
-               return ret;
-       }
-
-       if (current_temp < switch_on_temp) {
+       if ((!ret) && (current_temp < switch_on_temp)) {

nit: I think the inner pair of () are not necessary.

The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone.

While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---
 drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------
 drivers/thermal/thermal_core.c    | 28 ++++++++++++++++++
 include/linux/thermal.h           |  6 ++++
 3 files changed, 87 insertions(+), 9 deletions(-)

drivers/thermal/power_allocator.c +43 -6

index 63a448f9d93b..7ec459780dff 100644
@@ -73,6 +73,39 @@ struct power_allocator_params {
 };
 
 /**
+ * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
+ * @tz: thermal zone we are operating in
+ *
+ * For thermal zones that don't provide a sustainable_power in their
+ * thermal_zone_params, estimate one.  Calculate it using the minimum
+ * power of all the cooling devices as that gives a valid value that
+ * can give some degree of functionality.  For optimal performance of
+ * this governor, provide a sustainable_power in the thermal zone's
+ * thermal_zone_params.
+ */
+static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
+{
+	u32 sustainable_power = 0;
+	struct thermal_instance *instance;
+	struct power_allocator_params *params = tz->governor_data;
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		struct thermal_cooling_device *cdev = instance->cdev;
+		u32 min_power;
+
+		if (instance->trip != params->trip_max_desired_temperature)
+			continue;
+
+		if (power_actor_get_min_power(cdev, tz, &min_power))
+			continue;
+
+		sustainable_power += min_power;
+	}
+
+	return sustainable_power;
+}
+
+/**
  * pid_controller() - PID controller
  * @tz:	thermal zone we are operating in
  * @current_temp:	the current temperature in millicelsius

@@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 {
 	s64 p, i, d, power_range;
 	s32 err, max_power_frac;
+	u32 sustainable_power;
 	struct power_allocator_params *params = tz->governor_data;
 
 	max_power_frac = int_to_frac(max_allocatable_power);

@@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 
 	power_range = p + i + d;
 
+	sustainable_power = tz->tzp->sustainable_power ?:
+		estimate_sustainable_power(tz);
+
 	/* feed-forward the known sustainable dissipatable power */
-	power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
+	power_range = sustainable_power + frac_to_int(power_range);
 
 	power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
 

@@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	int ret;
 	struct power_allocator_params *params;
 	unsigned long switch_on_temp, control_temp;
-	u32 temperature_threshold;
+	u32 sustainable_power, temperature_threshold;
 
-	if (!tz->tzp || !tz->tzp->sustainable_power) {
-		dev_err(&tz->device,
-			"power_allocator: missing sustainable_power\n");
+	if (!tz->tzp)
 		return -EINVAL;
-	}
 
 	params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
 	if (!params)
 		return -ENOMEM;
 
+	if (!tz->tzp->sustainable_power)
+		dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
+
 	ret = get_governor_trips(tz, params);
 	if (ret) {
 		dev_err(&tz->device,

@@ -448,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	if (ret)

drivers/thermal/power_allocator.c +0 -0

 	/*
 	 * The default for k_d and integral_cutoff is 0, so we can

drivers/thermal/thermal_core.c +28 -0

index 4ca211be4c0f..760204f0b63c 100644
@@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev,
 }
 
 /**
+ * power_actor_get_min_power() - get the mainimum power that a cdev can consume
+ * @cdev:	pointer to &thermal_cooling_device
+ * @tz:		a valid thermal zone device pointer
+ * @min_power:	pointer in which to store the minimum power
+ *
+ * Calculate the minimum power consumption in milliwatts that the
+ * cooling device can currently consume and store it in @min_power.
+ *
+ * Return: 0 on success, -EINVAL if @cdev doesn't support the
+ * power_actor API or -E* on other error.
+ */
+int power_actor_get_min_power(struct thermal_cooling_device *cdev,
+			      struct thermal_zone_device *tz, u32 *min_power)
+{
+	unsigned long max_state;
+	int ret;
+
+	if (!cdev_is_power_actor(cdev))
+		return -EINVAL;
+
+	ret = cdev->ops->get_max_state(cdev, &max_state);
+	if (ret)
+		return ret;
+
+	return cdev->ops->state2power(cdev, tz, max_state, min_power);
+}
+
+/**
  * power_actor_set_power() - limit the maximum power that a cooling device can consume
  * @cdev:	pointer to &thermal_cooling_device
  * @instance:	thermal instance to update

include/linux/thermal.h +6 -0

index 037e9df2f610..f99d934d373a 100644
@@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
 
 int power_actor_get_max_power(struct thermal_cooling_device *,
 			      struct thermal_zone_device *tz, u32 *max_power);
+int power_actor_get_min_power(struct thermal_cooling_device *,
+			      struct thermal_zone_device *tz, u32 *min_power);
 int power_actor_set_power(struct thermal_cooling_device *,
 			  struct thermal_instance *, u32);
 struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,

@@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
 static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev,
 			      struct thermal_zone_device *tz, u32 *max_power)
 { return 0; }
+static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev,
+					    struct thermal_zone_device *tz,
+					    u32 *min_power)
+{ return -ENODEV; }
 static inline int power_actor_set_power(struct thermal_cooling_device *cdev,
 			  struct thermal_instance *tz, u32 power)
 { return 0; }

Signature · 1 line

1.9.1

Quoted 110 lines · Javi Merino

The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone.

While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---
 drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------
 drivers/thermal/thermal_core.c    | 28 ++++++++++++++++++
 include/linux/thermal.h           |  6 ++++
 3 files changed, 87 insertions(+), 9 deletions(-)

drivers/thermal/power_allocator.c +43 -6

index 63a448f9d93b..7ec459780dff 100644
@@ -73,6 +73,39 @@ struct power_allocator_params {
 };
 
 /**
+ * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
+ * @tz: thermal zone we are operating in
+ *
+ * For thermal zones that don't provide a sustainable_power in their
+ * thermal_zone_params, estimate one.  Calculate it using the minimum
+ * power of all the cooling devices as that gives a valid value that
+ * can give some degree of functionality.  For optimal performance of
+ * this governor, provide a sustainable_power in the thermal zone's
+ * thermal_zone_params.
+ */
+static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
+{
+	u32 sustainable_power = 0;
+	struct thermal_instance *instance;
+	struct power_allocator_params *params = tz->governor_data;
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		struct thermal_cooling_device *cdev = instance->cdev;
+		u32 min_power;
+
+		if (instance->trip != params->trip_max_desired_temperature)
+			continue;
+
+		if (power_actor_get_min_power(cdev, tz, &min_power))
+			continue;
+
+		sustainable_power += min_power;
+	}
+
+	return sustainable_power;
+}
+
+/**
  * pid_controller() - PID controller
  * @tz:	thermal zone we are operating in
  * @current_temp:	the current temperature in millicelsius

@@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 {
 	s64 p, i, d, power_range;
 	s32 err, max_power_frac;
+	u32 sustainable_power;
 	struct power_allocator_params *params = tz->governor_data;
 
 	max_power_frac = int_to_frac(max_allocatable_power);

@@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 
 	power_range = p + i + d;
 
+	sustainable_power = tz->tzp->sustainable_power ?:
+		estimate_sustainable_power(tz);
+
 	/* feed-forward the known sustainable dissipatable power */
-	power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
+	power_range = sustainable_power + frac_to_int(power_range);
 
 	power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
 

@@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	int ret;
 	struct power_allocator_params *params;
 	unsigned long switch_on_temp, control_temp;
-	u32 temperature_threshold;
+	u32 sustainable_power, temperature_threshold;
 
-	if (!tz->tzp || !tz->tzp->sustainable_power) {
-		dev_err(&tz->device,
-			"power_allocator: missing sustainable_power\n");
+	if (!tz->tzp)
 		return -EINVAL;
-	}
 
 	params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
 	if (!params)
 		return -ENOMEM;
 
+	if (!tz->tzp->sustainable_power)
+		dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
+
 	ret = get_governor_trips(tz, params);
 	if (ret) {
 		dev_err(&tz->device,

@@ -448,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	if (ret)

drivers/thermal/power_allocator.c static int power_allocator_bind(struct thermal_zone_device *tz) · line 495

Quoted 10 lines

drivers/thermal/power_allocator.c +8 -0

 		goto free;
 
+	/*
+	 * Provide an arbitrary sustainable_power to set the default
+	 * values of k_po and k_pu.  We can not estimate sustainable_power
+	 * at this point because no cooling devices have been
+	 * registered yet.  By providing an arbitrary value we get
+	 * better defaults than setting k_po and k_pu to 0.
+	 */
+	sustainable_power = tz->tzp->sustainable_power ?: 2500;

I think having 2500 here may produce constants that are not sane for most thermal zones.

Quoted 2 lines

drivers/thermal/power_allocator.c +0 -0

 	temperature_threshold = control_temp - switch_on_temp;
 

drivers/thermal/power_allocator.c static int power_allocator_bind(struct thermal_zone_device *tz) · line 502

Quoted 8 lines

drivers/thermal/power_allocator.c +2 -3

 	tz->tzp->k_po = tz->tzp->k_po ?:
-		int_to_frac(tz->tzp->sustainable_power) / temperature_threshold;
+		int_to_frac(sustainable_power) / temperature_threshold;
 	tz->tzp->k_pu = tz->tzp->k_pu ?:
-		int_to_frac(2 * tz->tzp->sustainable_power) /
-		temperature_threshold;
+		int_to_frac(2 * sustainable_power) / temperature_threshold;
 	tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000;

I would prefer you move the constants estimations to where you have a sane sustainable_power.

Thermal zones created using thermal_zone_device_create() may not have tzp. As the governor gets its parameters from there, allocate it while the governor is bound to the thermal zone so that it can operate in it. In this case, tzp is freed when the thermal zone switches to another governor.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---

While this would be easier to do by just ignoring the thermal zone if there was no tzp, I think the approach in this patch provides a better behavior.

 drivers/thermal/power_allocator.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

drivers/thermal/power_allocator.c +4 -2

index 2dfb8ade4d1b..85ce0aac9a41 100644
@@ -58,6 +58,8 @@ static inline s64 div_frac(s64 x, s64 y)
 
 /**
  * struct power_allocator_params - parameters for the power allocator governor
+ * @allocated_tzp:     whether we have allocated tzp for this thermal zone and
+ *                     it needs to be freed on unbind
  * @err_integral:      accumulated error in the PID controller.
  * @prev_err:  error in the previous iteration of the PID controller.
  *             Used to calculate the derivative term.

@@ -70,6 +72,7 @@ static inline s64 div_frac(s64 x, s64 y)
  *                                     controlling for.
  */
 struct power_allocator_params {
+       bool allocated_tzp;
        s64 err_integral;
        s32 prev_err;
        int trip_switch_on;

@@ -530,8 +533,7 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
  * Initialize the PID controller parameters and bind it to the thermal
  * zone.
  *
- * Return: 0 on success, -EINVAL if the thermal zone doesn't have tzp or -ENOMEM
- * if we ran out of memory.
+ * Return: 0 on success, or -ENOMEM if we ran out of memory.
  */
 static int power_allocator_bind(struct thermal_zone_device *tz)
 {

@@ -539,13 +541,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
        struct power_allocator_params *params;
        unsigned long control_temp;

drivers/thermal/power_allocator.c +21 -1

+               if (!tz->tzp) {
+                       ret = -ENOMEM;
+                       goto free_params;
+               }
+
+               params->allocated_tzp = true;
+       }
+
        if (!tz->tzp->sustainable_power)
                dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
 

@@ -562,11 +571,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
        tz->governor_data = params;
 
        return 0;
+
+free_params:
+       kfree(params);
+
+       return ret;
 }
 
 static void power_allocator_unbind(struct thermal_zone_device *tz)
 {
+       struct power_allocator_params *params = tz->governor_data;
+
        dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id);
+
+       if (params->allocated_tzp) {
+               kfree(tz->tzp);
+               tz->tzp = NULL;
+       }
+
        kfree(tz->governor_data);
        tz->governor_data = NULL;
 }
--

1.9.1

Quoted 11 lines · Javi Merino

Thermal zones created using thermal_zone_device_create() may not have tzp. As the governor gets its parameters from there, allocate it while the governor is bound to the thermal zone so that it can operate in it. In this case, tzp is freed when the thermal zone switches to another governor.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---

Quoted 3 lines

While this would be easier to do by just ignoring the thermal zone if there was no tzp, I think the approach in this patch provides a better behavior.

Why?
Just ignoring the thermal zone seems reasonable and simpler.

From the developer point of view, I agree that it's simpler. What I

Quoted 37 lines

 drivers/thermal/power_allocator.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

drivers/thermal/power_allocator.c +4 -2

index 2dfb8ade4d1b..85ce0aac9a41 100644
@@ -58,6 +58,8 @@ static inline s64 div_frac(s64 x, s64 y)
 
 /**
  * struct power_allocator_params - parameters for the power allocator governor
+ * @allocated_tzp:     whether we have allocated tzp for this thermal zone and
+ *                     it needs to be freed on unbind
  * @err_integral:      accumulated error in the PID controller.
  * @prev_err:  error in the previous iteration of the PID controller.
  *             Used to calculate the derivative term.

@@ -70,6 +72,7 @@ static inline s64 div_frac(s64 x, s64 y)
  *                                     controlling for.
  */
 struct power_allocator_params {
+       bool allocated_tzp;
        s64 err_integral;
        s32 prev_err;
        int trip_switch_on;

@@ -530,8 +533,7 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
  * Initialize the PID controller parameters and bind it to the thermal
  * zone.
  *
- * Return: 0 on success, -EINVAL if the thermal zone doesn't have tzp or -ENOMEM
- * if we ran out of memory.
+ * Return: 0 on success, or -ENOMEM if we ran out of memory.
  */
 static int power_allocator_bind(struct thermal_zone_device *tz)
 {

@@ -539,13 +541,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
        struct power_allocator_params *params;
        unsigned long control_temp;

drivers/thermal/power_allocator.c static int power_allocator_bind(struct thermal_zone_device *tz) · line 549

Quoted 10 lines

drivers/thermal/power_allocator.c +2 -3

 
-       if (!tz->tzp)
-               return -EINVAL;
-
        params = kzalloc(sizeof(*params), GFP_KERNEL);
        if (!params)
                return -ENOMEM;
 
+       if (!tz->tzp) {
+               tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);

Why bother to allocate this dummy struct?
Can't we just leave tz->tzp as NULL, and do a NULL check where needed?

Quoted 39 lines

drivers/thermal/power_allocator.c +21 -1

+               if (!tz->tzp) {
+                       ret = -ENOMEM;
+                       goto free_params;
+               }
+
+               params->allocated_tzp = true;
+       }
+
        if (!tz->tzp->sustainable_power)
                dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
 

@@ -562,11 +571,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
        tz->governor_data = params;
 
        return 0;
+
+free_params:
+       kfree(params);
+
+       return ret;
 }
 
 static void power_allocator_unbind(struct thermal_zone_device *tz)
 {
+       struct power_allocator_params *params = tz->governor_data;
+
        dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id);
+
+       if (params->allocated_tzp) {
+               kfree(tz->tzp);
+               tz->tzp = NULL;
+       }
+
        kfree(tz->governor_data);
        tz->governor_data = NULL;
 }
--

1.9.1

Quoted 12 lines · Daniel Kurtz

Quoted 11 lines · Javi Merino

Thermal zones created using thermal_zone_device_create() may not have tzp. As the governor gets its parameters from there, allocate it while the governor is bound to the thermal zone so that it can operate in it. In this case, tzp is freed when the thermal zone switches to another governor.

Cc Zhang Rui <[email protected]>

Cc Eduardo Valentin <[email protected]>

Signed-off-by Javi Merino <[email protected]>

---

Quoted 6 lines

Quoted 3 lines

While this would be easier to do by just ignoring the thermal zone if there was no tzp, I think the approach in this patch provides a better behavior.

Why?
Just ignoring the thermal zone seems reasonable and simpler.

From the developer point of view, I agree that it's simpler. What I want to avoid is the system integrator getting different behaviors based on the presence of tzp when the thermal zone was created. If the integrator was to configure this from userspace, they would only be able to do so if the thermal zone was created with tzp. I don't like this distinction, I prefer the consistency from the user point of view that this patch gives.

Cheers,
Javi

Quoted 91 lines

Quoted 37 lines

 drivers/thermal/power_allocator.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

drivers/thermal/power_allocator.c +4 -2

index 2dfb8ade4d1b..85ce0aac9a41 100644
@@ -58,6 +58,8 @@ static inline s64 div_frac(s64 x, s64 y)
 
 /**
  * struct power_allocator_params - parameters for the power allocator governor
+ * @allocated_tzp:     whether we have allocated tzp for this thermal zone and
+ *                     it needs to be freed on unbind
  * @err_integral:      accumulated error in the PID controller.
  * @prev_err:  error in the previous iteration of the PID controller.
  *             Used to calculate the derivative term.

@@ -70,6 +72,7 @@ static inline s64 div_frac(s64 x, s64 y)
  *                                     controlling for.
  */
 struct power_allocator_params {
+       bool allocated_tzp;
        s64 err_integral;
        s32 prev_err;
        int trip_switch_on;

@@ -530,8 +533,7 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
  * Initialize the PID controller parameters and bind it to the thermal
  * zone.
  *
- * Return: 0 on success, -EINVAL if the thermal zone doesn't have tzp or -ENOMEM
- * if we ran out of memory.
+ * Return: 0 on success, or -ENOMEM if we ran out of memory.
  */
 static int power_allocator_bind(struct thermal_zone_device *tz)
 {

@@ -539,13 +541,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
        struct power_allocator_params *params;
        unsigned long control_temp;

drivers/thermal/power_allocator.c static int power_allocator_bind(struct thermal_zone_device *tz) · line 549

Quoted 10 lines

drivers/thermal/power_allocator.c +2 -3

 
-       if (!tz->tzp)
-               return -EINVAL;
-
        params = kzalloc(sizeof(*params), GFP_KERNEL);
        if (!params)
                return -ENOMEM;
 
+       if (!tz->tzp) {
+               tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);

Why bother to allocate this dummy struct?
Can't we just leave tz->tzp as NULL, and do a NULL check where needed?

Quoted 39 lines

drivers/thermal/power_allocator.c +21 -1

+               if (!tz->tzp) {
+                       ret = -ENOMEM;
+                       goto free_params;
+               }
+
+               params->allocated_tzp = true;
+       }
+
        if (!tz->tzp->sustainable_power)
                dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
 

@@ -562,11 +571,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
        tz->governor_data = params;
 
        return 0;
+
+free_params:
+       kfree(params);
+
+       return ret;
 }
 
 static void power_allocator_unbind(struct thermal_zone_device *tz)
 {
+       struct power_allocator_params *params = tz->governor_data;
+
        dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id);
+
+       if (params->allocated_tzp) {
+               kfree(tz->tzp);
+               tz->tzp = NULL;
+       }
+
        kfree(tz->governor_data);
        tz->governor_data = NULL;
 }
--

1.9.1

Relax the thermal governor requirements of sustainable_power and at least two trip points so that it can be bound to any thermal zone. Its behavior won't be optimal, it would be the best it can with the data provided.

Changes since v3:
- Don't hardcode a value for sustainable power and re-estimate the PID controllers every time if no sustainable power is given as suggested by Eduardo Valentin.
- power_actor_get_min_power() moved to a patch of its own.

Changes since v2:
- Typos suggested by Daniel Kurtz

Changes since v1:
- Let the power allocator governor operate if the thermal zone doesn't have tzp as suggested by Chung-yih Wang

Javi Merino (5):
thermal: Add a function to get the minimum power
thermal: power_allocator: relax the requirement of a sustainable_power
in tzp
thermal: power_allocator: relax the requirement of two passive trip
points
thermal: power_allocator: don't require tzp to be present for the
thermal zone
thermal: power_allocator: exit early if there are no cooling devices

 Documentation/thermal/power_allocator.txt |   2 +-
 drivers/thermal/power_allocator.c         | 241 ++++++++++++++++++++++--------
 drivers/thermal/thermal_core.c            |  28 ++++
 include/linux/thermal.h                   |   6 +
 4 files changed, 212 insertions(+), 65 deletions(-)

Relax the thermal governor requirements of sustainable_power and at least two trip points so that it can be bound to any thermal zone. Its behavior won't be optimal, it would be the best possible with the data provided.

Changes since v4:
- Fix crash when a thermal zone with no trip points has no get_trip_point_temp(). Reported by Daniel Kurtz.
- s/estimate_controller_constants()/estimate_pid_constants()/g

Changes since v3:
- Don't hardcode a value for sustainable power and re-estimate the PID controllers every time if no sustainable power is given as suggested by Eduardo Valentin.
- power_actor_get_min_power() moved to a patch of its own.

Changes since v2:
- Typos suggested by Daniel Kurtz

Changes since v1:
- Let the power allocator governor operate if the thermal zone doesn't have tzp as suggested by Chung-yih Wang

Javi Merino (5):
thermal: Add a function to get the minimum power
thermal: power_allocator: relax the requirement of a sustainable_power
in tzp
thermal: power_allocator: relax the requirement of two passive trip
points
thermal: power_allocator: don't require tzp to be present for the
thermal zone
thermal: power_allocator: exit early if there are no cooling devices

 Documentation/thermal/power_allocator.txt |   2 +-
 drivers/thermal/power_allocator.c         | 243 ++++++++++++++++++++++--------
 drivers/thermal/thermal_core.c            |  28 ++++
 include/linux/thermal.h                   |   6 +
 4 files changed, 214 insertions(+), 65 deletions(-)

--
1.9.1