Received-SPF: pass (google.com: domain of linux-kernel+bounces-7362-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20231212134844.1213381-1-lukasz.luba@arm.com> <20231212134844.1213381-2-lukasz.luba@arm.com>
 <CAJZ5v0iFOrgtN82pqUqDhE1jMA4wjhH19DFhzPP3yYO05O03=g@mail.gmail.com> <4e5f7d1b-1534-432b-92c1-880c863d79a2@arm.com>
In-Reply-To: <4e5f7d1b-1534-432b-92c1-880c863d79a2@arm.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Wed, 20 Dec 2023 18:44:31 +0100
Message-ID: <CAJZ5v0ieGnAaBU=mvKWWs1PVnNtr4QOQev_X1SA+5XvpADh-gA@mail.gmail.com>
Subject: Re: [PATCH v2 1/8] thermal: core: Add governor callback for thermal
 zone change
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, linux-kernel@vger.kernel.org, 
	daniel.lezcano@linaro.org, linux-pm@vger.kernel.org, rui.zhang@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Dec 20, 2023 at 5:16=E2=80=AFPM Lukasz Luba <lukasz.luba@arm.com> w=
rote:
>
>
>
> On 12/20/23 13:51, Rafael J. Wysocki wrote:
> > On Tue, Dec 12, 2023 at 2:48=E2=80=AFPM Lukasz Luba <lukasz.luba@arm.co=
m> wrote:
> >>
> >> Add a new callback which can update governors when there is a change i=
n
> >> 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 allocati=
ons
> >> related to the number of cooling instances out of the throttle() callb=
ack.
> >>
> >> 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 wi=
th
> >> the same thermal zone lock, which guaranties the consistency.
> >>
> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> >> ---
> >>   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 ther=
mal_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_instanc=
es);
> >>                  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.