[PATCH v2] thermal: gov_power_allocator: Update total_weight on bind and cdev updates

[PATCH v2] thermal: gov_power_allocator: Update total_weight on bind and cdev updates

[Yu-Che Cheng]

params->total_weight is not initialized during bind and not updated when
the bound cdev changes. The cooling device weight will not be used due
to the uninitialized total_weight, until we trigger an update via sysfs.

The bound cdev update will be triggered during thermal zone registration,
where each cooling device will be bound to the thermal zone one by one.

The power_allocator_bind can be called without additional cdev update
when manually changing the policy of a thermal zone via sysfs.

Add a new function to handle weight update logic, including updating
total_weight, and call it when bind, weight changes, and cdev updates to
ensure total_weight is always correct.

Fixes: a3cd6db4cc2e ("thermal: gov_power_allocator: Support new update callback of weights")
Signed-off-by: Yu-Che Cheng <giver@chromium.org>
---
Changes in v2:
- Move the total_weight update to a new function for clarity.
- Found v1 may cause crash when there are thermal zones without any
  active or passive trip points.
  Check trip_max before accessing its trip_desc. 
- Link to v1: https://lore.kernel.org/r/20250219-fix-power-allocator-weight-v1-1-79b1a99e94eb@chromium.org
---
 drivers/thermal/gov_power_allocator.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 3b644de3292e..126452fb470e 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -641,6 +641,22 @@ static int allocate_actors_buffer(struct power_allocator_params *params,
 	return ret;
 }
 
+static void power_allocator_update_weight(struct power_allocator_params *params)
+{
+	const struct thermal_trip_desc *td;
+	struct thermal_instance *instance;
+
+	if (!params->trip_max)
+		return;
+
+	td = trip_to_trip_desc(params->trip_max);
+
+	params->total_weight = 0;
+	list_for_each_entry(instance, &td->thermal_instances, trip_node)
+		if (power_actor_is_valid(instance))
+			params->total_weight += instance->weight;
+}
+
 static void power_allocator_update_tz(struct thermal_zone_device *tz,
 				      enum thermal_notify_event reason)
 {
@@ -656,16 +672,12 @@ static void power_allocator_update_tz(struct thermal_zone_device *tz,
 			if (power_actor_is_valid(instance))
 				num_actors++;
 
-		if (num_actors == params->num_actors)
-			return;
+		if (num_actors != params->num_actors)
+			allocate_actors_buffer(params, num_actors);
 
-		allocate_actors_buffer(params, num_actors);
-		break;
+		fallthrough;
 	case THERMAL_INSTANCE_WEIGHT_CHANGED:
-		params->total_weight = 0;
-		list_for_each_entry(instance, &td->thermal_instances, trip_node)
-			if (power_actor_is_valid(instance))
-				params->total_weight += instance->weight;
+		power_allocator_update_weight(params);
 		break;
 	default:
 		break;
@@ -731,6 +743,8 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 
 	tz->governor_data = params;
 
+	power_allocator_update_weight(params);
+
 	return 0;
 
 free_params:

---
base-commit: 2408a807bfc3f738850ef5ad5e3fd59d66168996
change-id: 20250218-fix-power-allocator-weight-5dac6d4b5797

Best regards,
-- 
Yu-Che Cheng <giver@chromium.org>

Re: [PATCH v2] thermal: gov_power_allocator: Update total_weight on bind and cdev updates

[Rafael J. Wysocki]

On Sat, Feb 22, 2025 at 4:20 AM Yu-Che Cheng <giver@chromium.org> wrote:
>
> params->total_weight is not initialized during bind and not updated when
> the bound cdev changes. The cooling device weight will not be used due
> to the uninitialized total_weight, until we trigger an update via sysfs.
>
> The bound cdev update will be triggered during thermal zone registration,
> where each cooling device will be bound to the thermal zone one by one.
>
> The power_allocator_bind can be called without additional cdev update
> when manually changing the policy of a thermal zone via sysfs.
>
> Add a new function to handle weight update logic, including updating
> total_weight, and call it when bind, weight changes, and cdev updates to
> ensure total_weight is always correct.
>
> Fixes: a3cd6db4cc2e ("thermal: gov_power_allocator: Support new update callback of weights")
> Signed-off-by: Yu-Che Cheng <giver@chromium.org>

Applied as 6.14-rc material with some minor edits in the changelog.

Lukasz, if you have any objections, please let me know.

Thanks!

> ---
> Changes in v2:
> - Move the total_weight update to a new function for clarity.
> - Found v1 may cause crash when there are thermal zones without any
>   active or passive trip points.
>   Check trip_max before accessing its trip_desc.
> - Link to v1: https://lore.kernel.org/r/20250219-fix-power-allocator-weight-v1-1-79b1a99e94eb@chromium.org
> ---
>  drivers/thermal/gov_power_allocator.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 3b644de3292e..126452fb470e 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -641,6 +641,22 @@ static int allocate_actors_buffer(struct power_allocator_params *params,
>         return ret;
>  }
>
> +static void power_allocator_update_weight(struct power_allocator_params *params)
> +{
> +       const struct thermal_trip_desc *td;
> +       struct thermal_instance *instance;
> +
> +       if (!params->trip_max)
> +               return;
> +
> +       td = trip_to_trip_desc(params->trip_max);
> +
> +       params->total_weight = 0;
> +       list_for_each_entry(instance, &td->thermal_instances, trip_node)
> +               if (power_actor_is_valid(instance))
> +                       params->total_weight += instance->weight;
> +}
> +
>  static void power_allocator_update_tz(struct thermal_zone_device *tz,
>                                       enum thermal_notify_event reason)
>  {
> @@ -656,16 +672,12 @@ static void power_allocator_update_tz(struct thermal_zone_device *tz,
>                         if (power_actor_is_valid(instance))
>                                 num_actors++;
>
> -               if (num_actors == params->num_actors)
> -                       return;
> +               if (num_actors != params->num_actors)
> +                       allocate_actors_buffer(params, num_actors);
>
> -               allocate_actors_buffer(params, num_actors);
> -               break;
> +               fallthrough;
>         case THERMAL_INSTANCE_WEIGHT_CHANGED:
> -               params->total_weight = 0;
> -               list_for_each_entry(instance, &td->thermal_instances, trip_node)
> -                       if (power_actor_is_valid(instance))
> -                               params->total_weight += instance->weight;
> +               power_allocator_update_weight(params);
>                 break;
>         default:
>                 break;
> @@ -731,6 +743,8 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>
>         tz->governor_data = params;
>
> +       power_allocator_update_weight(params);
> +
>         return 0;
>
>  free_params:
>
> ---
> base-commit: 2408a807bfc3f738850ef5ad5e3fd59d66168996
> change-id: 20250218-fix-power-allocator-weight-5dac6d4b5797
>
> Best regards,
> --
> Yu-Che Cheng <giver@chromium.org>
>

Re: [PATCH v2] thermal: gov_power_allocator: Update total_weight on bind and cdev updates

[Lukasz Luba]

On 2/25/25 11:35, Rafael J. Wysocki wrote:
> On Sat, Feb 22, 2025 at 4:20 AM Yu-Che Cheng <giver@chromium.org> wrote:
>>
>> params->total_weight is not initialized during bind and not updated when
>> the bound cdev changes. The cooling device weight will not be used due
>> to the uninitialized total_weight, until we trigger an update via sysfs.
>>
>> The bound cdev update will be triggered during thermal zone registration,
>> where each cooling device will be bound to the thermal zone one by one.
>>
>> The power_allocator_bind can be called without additional cdev update
>> when manually changing the policy of a thermal zone via sysfs.
>>
>> Add a new function to handle weight update logic, including updating
>> total_weight, and call it when bind, weight changes, and cdev updates to
>> ensure total_weight is always correct.
>>
>> Fixes: a3cd6db4cc2e ("thermal: gov_power_allocator: Support new update callback of weights")
>> Signed-off-by: Yu-Che Cheng <giver@chromium.org>
> 
> Applied as 6.14-rc material with some minor edits in the changelog.
> 
> Lukasz, if you have any objections, please let me know.

My apologies for the delay. Thanks Rafael for taking this into your
tree.

No objections, thanks Yu-Che for the fix.

The code looks good and if it's not too late then:

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Regards,
Lukasz