2023-12-12 13:48:25

by Lukasz Luba

Subject: [PATCH v2 4/8] thermal: gov_power_allocator: Simplify checks for valid power actor

There is a need to check if the cooling device in the thermal zone
supports IPA callback and is set for control trip point.
Refactor the code which validates the power actor capabilities and
make it more consistent in all places.

No intentional functional impact.

Signed-off-by: Lukasz Luba <[email protected]>
---
drivers/thermal/gov_power_allocator.c | 41 +++++++++++----------------
1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 3328c3ec71f2..1a605fd9c8c6 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -85,6 +85,13 @@ struct power_allocator_params {
u32 *weighted_req_power;
};

+static bool power_actor_is_valid(struct power_allocator_params *params,
+ struct thermal_instance *instance)
+{
+ return ((instance->trip == params->trip_max) &&
+ cdev_is_power_actor(instance->cdev));
+}
+
/**
* estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
* @tz: thermal zone we are operating in
@@ -105,14 +112,10 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
u32 min_power;

list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
- cdev = instance->cdev;
-
- if (instance->trip != params->trip_max)
- continue;
-
- if (!cdev_is_power_actor(cdev))
+ if (!power_actor_is_valid(params, instance))
continue;

+ cdev = instance->cdev;
if (cdev->ops->state2power(cdev, instance->upper, &min_power))
continue;

@@ -407,8 +410,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
return -ENODEV;

list_for_each_entry(instance, &tz->thermal_instances, tz_node)
- if ((instance->trip == params->trip_max) &&
- cdev_is_power_actor(instance->cdev))
+ if (power_actor_is_valid(params, instance))
total_weight += instance->weight;

/* Clean all buffers for new power estimations */
@@ -421,14 +423,10 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
weighted_req_power = params->weighted_req_power;

list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
- cdev = instance->cdev;
-
- if (instance->trip != params->trip_max)
- continue;
-
- if (!cdev_is_power_actor(cdev))
+ if (!power_actor_is_valid(params, instance))
continue;

+ cdev = instance->cdev;
if (cdev->ops->get_requested_power(cdev, &req_power[i]))
continue;

@@ -458,10 +456,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)

i = 0;
list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
- if (instance->trip != params->trip_max)
- continue;
-
- if (!cdev_is_power_actor(instance->cdev))
+ if (!power_actor_is_valid(params, instance))
continue;

power_actor_set_power(instance->cdev, instance,
@@ -546,12 +541,11 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
u32 req_power;

list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
- cdev = instance->cdev;
-
- if (instance->trip != params->trip_max ||
- !cdev_is_power_actor(instance->cdev))
+ if (!power_actor_is_valid(params, instance))
continue;

+ cdev = instance->cdev;
+
instance->target = 0;
mutex_lock(&cdev->lock);
/*
@@ -664,8 +658,7 @@ static void power_allocator_update_tz(struct thermal_zone_device *tz,
switch (reason) {
case THERMAL_INSTANCE_LIST_UPDATE:
list_for_each_entry(instance, &tz->thermal_instances, tz_node)
- if ((instance->trip == params->trip_max) &&
- cdev_is_power_actor(instance->cdev))
+ if (power_actor_is_valid(params, instance))
num_actors++;

if (num_actors == params->num_actors)
--
2.25.1

2023-12-20 14:40:26

by Rafael J. Wysocki

Subject: Re: [PATCH v2 4/8] thermal: gov_power_allocator: Simplify checks for valid power actor

On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <[email protected]> wrote:
>
> There is a need to check if the cooling device in the thermal zone
> supports IPA callback and is set for control trip point.
> Refactor the code which validates the power actor capabilities and
> make it more consistent in all places.

This really is about reducing code duplication which is worth
mentioning, so I would say

"Add a helper to check if a given cooling device in a thermal zone
supports the IPA callback and is bound to the control trip point and
use it wherever that check is needed to reduce code duplication."

>
> No intentional functional impact.
>
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
> drivers/thermal/gov_power_allocator.c | 41 +++++++++++----------------
> 1 file changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 3328c3ec71f2..1a605fd9c8c6 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -85,6 +85,13 @@ struct power_allocator_params {
> u32 *weighted_req_power;
> };
>
> +static bool power_actor_is_valid(struct power_allocator_params *params,
> + struct thermal_instance *instance)
> +{
> + return ((instance->trip == params->trip_max) &&

The inner parens are redundant.

> + cdev_is_power_actor(instance->cdev));
> +}

The part below LGTM.

2023-12-20 16:25:17

by Lukasz Luba

Subject: Re: [PATCH v2 4/8] thermal: gov_power_allocator: Simplify checks for valid power actor

On 12/20/23 14:40, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <[email protected]> wrote:
>>
>> There is a need to check if the cooling device in the thermal zone
>> supports IPA callback and is set for control trip point.
>> Refactor the code which validates the power actor capabilities and
>> make it more consistent in all places.
>
> This really is about reducing code duplication which is worth
> mentioning, so I would say
>
> "Add a helper to check if a given cooling device in a thermal zone
> supports the IPA callback and is bound to the control trip point and
> use it wherever that check is needed to reduce code duplication."
>

Thanks, I'll use it.

>>
>> No intentional functional impact.
>>
>> Signed-off-by: Lukasz Luba <[email protected]>
>> ---
>> drivers/thermal/gov_power_allocator.c | 41 +++++++++++----------------
>> 1 file changed, 17 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>> index 3328c3ec71f2..1a605fd9c8c6 100644
>> --- a/drivers/thermal/gov_power_allocator.c
>> +++ b/drivers/thermal/gov_power_allocator.c
>> @@ -85,6 +85,13 @@ struct power_allocator_params {
>> u32 *weighted_req_power;
>> };
>>
>> +static bool power_actor_is_valid(struct power_allocator_params *params,
>> + struct thermal_instance *instance)
>> +{
>> + return ((instance->trip == params->trip_max) &&
>
> The inner parens are redundant.

OK

>
>> + cdev_is_power_actor(instance->cdev));
>> +}
>
> The part below LGTM.