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

[Lukasz Luba]

On 12/20/23 17:44, Rafael J. Wysocki wrote:
> 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.

OK, thanks!