Re: [PATCH] thermal: gov_power_allocator: Allow binding without cooling devices

[Lukasz Luba]

On 3/28/24 09:12, Lukasz Luba wrote:
> Hi Rafael,
>
> On 3/27/24 15:25, Rafael J. Wysocki wrote:
> > On Thu, Mar 21, 2024 at 3:44 PM Nikita Travkin <nikita@xxxxxxx> wrote:
> > > Commit e83747c2f8e3 ("thermal: gov_power_allocator: Set up trip 
> > > points earlier")
> > > added a check that would fail binding the governer if there is no
> > > cooling devices bound to the thermal zone. Unfortunately this causes
> > > issues in cases when the TZ is bound to the governer before the cooling
> > > devices are attached to it. (I.e. when the tz is registered using
> > > thermal_zone_device_register_with_trips().)
> > >
> > > Additionally, the documentation across gov_power_allocator suggests it's
> > > intended to allow it to be bound to thermal zones without cooling
> > > devices (and thus without passive/active trip points), however the same
> > > change added a check for the trip point to be present, causing those TZ
> > > to fail probing.

This patch description is mixing trips and cooling devices and refers to
a commit which is only for guarding the trip points number to be
not less than 2. In IPA we require 2 trip points.

> > > Those changes cause all thermal zones to fail on some devices (such as
> > > sc7180-acer-aspire1) and prevent the kernel from controlling the cpu/gpu
> > > frequency based on the temperature, as well as losing all the other
> > > "informational" thermal zones if power_allocator is set as default.
> > >
> > > This commit partially reverts the referenced one by dropping the trip
> > > point check and by allowing the TZ to probe even if no actor buffer was
> > > allocated to allow those TZ to probe again.
> > >
> > > Fixes: e83747c2f8e3 ("thermal: gov_power_allocator: Set up trip 
> > > points earlier")

Not that commit.

> > > Signed-off-by: Nikita Travkin <nikita@xxxxxxx>
> > > ---
> > > I've noticed that all thermal zones fail probing with -EINVAL on my
> > > sc7180 based Acer Aspire 1 since 6.8. This commit allows me to bring
> > > them back.
> >
> > Łukasz, any comments?
>
> Let me check this today.
>
> > > ---
> > >   drivers/thermal/gov_power_allocator.c | 14 +++++---------
> > >   1 file changed, 5 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/thermal/gov_power_allocator.c 
> > > b/drivers/thermal/gov_power_allocator.c
> > > index 1b17dc4c219c..4f2d7f3b7508 100644
> > > --- a/drivers/thermal/gov_power_allocator.c
> > > +++ b/drivers/thermal/gov_power_allocator.c
> > > @@ -679,11 +679,6 @@ static int power_allocator_bind(struct 
> > > thermal_zone_device *tz)
> > >                  return -ENOMEM;
> > >
> > >          get_governor_trips(tz, params);
> > > -       if (!params->trip_max) {
> > > -               dev_warn(&tz->device, "power_allocator: missing 
> > > trip_max\n");

This if() guards the binding of TZ with less than 2 trip points,
not the cooling devices.

> > > -               kfree(params);
> > > -               return -EINVAL;
> > > -       }
> > >
> > >          ret = check_power_actors(tz, params);
> > >          if (ret < 0) {
> > > @@ -693,7 +688,7 @@ static int power_allocator_bind(struct 
> > > thermal_zone_device *tz)
> > >          }
> > >
> > >          ret = allocate_actors_buffer(params, ret);
> > > -       if (ret) {

This if() is from different commit.

> > > +       if (ret && ret != -EINVAL) {

This is about 0 cooling devices in the thermal zone, but IPA won't work
so why to even fake and forward binding?

Rafael should we support binding with 0 cooling devices?

> > >                  dev_warn(&tz->device, "power_allocator: allocation 
> > > failed\n");
> > >                  kfree(params);
> > >                  return ret;
> > > @@ -714,9 +709,10 @@ static int power_allocator_bind(struct 
> > > thermal_zone_device *tz)
> > >          else
> > >                  params->sustainable_power = tz->tzp->sustainable_power;
> > >
> > > -       estimate_pid_constants(tz, tz->tzp->sustainable_power,
> > > -                              params->trip_switch_on,
> > > -                              params->trip_max->temperature);
> > > +       if (params->trip_max)

This is not supported, we need those 2 trip points.

> > > +               estimate_pid_constants(tz, tz->tzp->sustainable_power,
> > > +                                      params->trip_switch_on,
> > > +                                      params->trip_max->temperature);
> > >
> > >          reset_pid_controller(params);
> > >
> > >
> > > ---
> > > base-commit: e7528c088874326d3060a46f572252be43755a86
> > > change-id: 20240321-gpa-no-cooling-devs-c79ee3288325
> > >
> > > Best regards,
> > > --
> > > Nikita Travkin <nikita@xxxxxxx>