Re: [PATCH v2 1/8] thermal: core: Add governor callback for thermal zone change

[Rafael J. Wysocki]

On Wed, Dec 20, 2023 at 5:16 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
>
>
>
> On 12/20/23 13:51, Rafael J. Wysocki wrote:
> > On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
> >>
> >> Add a new callback which can update governors when there is a change in
> >> the thermal zone internals, e.g. thermal cooling instance list changed.
> >
> > I would say what struct type the callback is going to be added to.
>
> OK, I'll add that.
>
> >
> >> That makes possible to move some heavy operations like memory allocations
> >> related to the number of cooling instances out of the throttle() callback.
> >>
> >> Reuse the 'enum thermal_notify_event' and extend it with a new event:
> >> THERMAL_INSTANCE_LIST_UPDATE.
> >
> > I think that this is a bit too low-level (see below).
>
> Yes, I agree (based on below).
>
> >
> >> Both callback code paths (throttle() and update_tz()) are protected with
> >> the same thermal zone lock, which guaranties the consistency.
> >>
> >> Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> >> ---
> >>   drivers/thermal/thermal_core.c | 13 +++++++++++++
> >>   include/linux/thermal.h        |  5 +++++
> >>   2 files changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> >> index 625ba07cbe2f..592c956f6fd5 100644
> >> --- a/drivers/thermal/thermal_core.c
> >> +++ b/drivers/thermal/thermal_core.c
> >> @@ -314,6 +314,14 @@ static void handle_non_critical_trips(struct thermal_zone_device *tz,
> >>                         def_governor->throttle(tz, trip);
> >>   }
> >>
> >
> > I needed a bit more time to think about this.
>
> OK.
>
> >
> >> +static void handle_instances_list_update(struct thermal_zone_device *tz)
> >> +{
> >> +       if (!tz->governor || !tz->governor->update_tz)
> >> +               return;
> >> +
> >> +       tz->governor->update_tz(tz, THERMAL_INSTANCE_LIST_UPDATE);
> >> +}
> >
> > So I would call the above something more generic, like
> > thermal_governor_update_tz() and I would pass the "reason" argument to
> > it.
>
> That sounds better, I agree.
>
> >
> >> +
> >>   void thermal_zone_device_critical(struct thermal_zone_device *tz)
> >>   {
> >>          /*
> >> @@ -723,6 +731,8 @@ int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
> >>                  list_add_tail(&dev->tz_node, &tz->thermal_instances);
> >>                  list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
> >>                  atomic_set(&tz->need_update, 1);
> >> +
> >> +               handle_instances_list_update(tz);
> >
> > In particular for this, I would use a special "reason" value, like
> > THERMAL_TZ_BIND_CDEV.
> >
> > Yes, the list of instances will change as a result of the binding, but
> > that is an internal detail specific to the current implementation.
>
> I see. With that new more generic thermal_governor_update_tz() would
> be better then, right?

I think so, IIUC.