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

[Lukasz Luba]

On 2/20/25 19:40, Rafael J. Wysocki wrote:
> On Wed, Feb 19, 2025 at 8:07 AM Yu-Che Cheng <giver@xxxxxxxxxxxx> 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.

Thanks Yu-Che for catching this.

> > Update total_weight when bind and cdev updates to ensure total_weight is
> > correct.
> >
> > Fixes: a3cd6db4cc2e ("thermal: gov_power_allocator: Support new update callback of weights")
> > Signed-off-by: Yu-Che Cheng <giver@xxxxxxxxxxxx>
> > ---
> >   drivers/thermal/gov_power_allocator.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > index 3b644de3292e..600cb0e367c3 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -656,11 +656,10 @@ 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;
>
> This is basically fine, but I would add a new function, say
> power_allocator_update_weight(), for updating the weight.
>
> >          case THERMAL_INSTANCE_WEIGHT_CHANGED:
>
> And I'd call it from here and from power_allocator_bind() below.
>
> >                  params->total_weight = 0;
> >                  list_for_each_entry(instance, &td->thermal_instances, trip_node)
> > @@ -731,6 +730,8 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
> >
> >          tz->governor_data = params;
> >
> > +       power_allocator_update_tz(tz, THERMAL_INSTANCE_WEIGHT_CHANGED);
>
> Because this is kind of confusing (although it will work AFAICS).

Thanks Rafael, I agree with suggestions. It would look cleaner.