linux-kernel - Re: [PATCH] thermal: gov_power_allocator: Update total

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKchOA3u9a2UK-xGONcEBbzrGMCGFVFDnNXuLHG+umDT2JA5gA@mail.gmail.com>
Date: Sat, 22 Feb 2025 07:31:27 +0800
From: Yu-Che Cheng <giver@...omium.org>
To: Lukasz Luba <lukasz.luba@....com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Yu-Che Cheng <giver@...omium.org>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Zhang Rui <rui.zhang@...el.com>, 
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Chen-Yu Tsai <wenst@...omium.org>
Subject: Re: [PATCH] thermal: gov_power_allocator: Update total_weight on bind
 and cdev updates

> 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 for the suggestion. Will apply in v2.

On Fri, Feb 21, 2025 at 6:43 PM Lukasz Luba <lukasz.luba@....com> wrote:
>
>
>
> On 2/20/25 19:40, Rafael J. Wysocki wrote:
> > On Wed, Feb 19, 2025 at 8:07 AM Yu-Che Cheng <giver@...omium.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.
>
> 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@...omium.org>
> >> ---
> >>   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.