Received-SPF: pass (google.com: domain of linux-kernel+bounces-7277-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <4e5f7d1b-1534-432b-92c1-880c863d79a2@arm.com>
Date: Wed, 20 Dec 2023 16:17:26 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/8] thermal: core: Add governor callback for thermal
 zone change
Content-Language: en-US
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: linux-kernel@vger.kernel.org, daniel.lezcano@linaro.org,
 linux-pm@vger.kernel.org, rui.zhang@intel.com
References: <20231212134844.1213381-1-lukasz.luba@arm.com>
 <20231212134844.1213381-2-lukasz.luba@arm.com>
 <CAJZ5v0iFOrgtN82pqUqDhE1jMA4wjhH19DFhzPP3yYO05O03=g@mail.gmail.com>
From: Lukasz Luba <lukasz.luba@arm.com>
In-Reply-To: <CAJZ5v0iFOrgtN82pqUqDhE1jMA4wjhH19DFhzPP3yYO05O03=g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 12/20/23 13:51, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> 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@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 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?

> 
>>          }
>>          mutex_unlock(&cdev->lock);
>>          mutex_unlock(&tz->lock);
>> @@ -781,6 +791,9 @@ int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
>>                  if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
>>                          list_del(&pos->tz_node);
>>                          list_del(&pos->cdev_node);
>> +
>> +                       handle_instances_list_update(tz);
> 
> Analogously, I'd use something like THERMAL_TZ_UNBIND_CDEV here.

OK

> 
>> +
>>                          mutex_unlock(&cdev->lock);
>>                          mutex_unlock(&tz->lock);
>>                          goto unbind;
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index c7190e2dfcb4..9fd0d3fb234a 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -51,6 +51,7 @@ enum thermal_notify_event {
>>          THERMAL_DEVICE_POWER_CAPABILITY_CHANGED, /* power capability changed */
>>          THERMAL_TABLE_CHANGED, /* Thermal table(s) changed */
>>          THERMAL_EVENT_KEEP_ALIVE, /* Request for user space handler to respond */
>> +       THERMAL_INSTANCE_LIST_UPDATE, /* List of thermal instances changed */
> 
> So I would add THERMAL_TZ_BIND_CDEV and THERMAL_TZ_UNBIND_CDEV to the list.

OK

> 
>>   };
>>
>>   /**
>> @@ -195,6 +196,8 @@ struct thermal_zone_device {
>>    *                     thermal zone.
>>    * @throttle:  callback called for every trip point even if temperature is
>>    *             below the trip point temperature
>> + * @update_tz: callback called when thermal zone internals have changed, e.g.
>> + *             thermal cooling instance was added/removed
>>    * @governor_list:     node in thermal_governor_list (in thermal_core.c)
>>    */
>>   struct thermal_governor {
>> @@ -203,6 +206,8 @@ struct thermal_governor {
>>          void (*unbind_from_tz)(struct thermal_zone_device *tz);
>>          int (*throttle)(struct thermal_zone_device *tz,
>>                          const struct thermal_trip *trip);
>> +       void (*update_tz)(struct thermal_zone_device *tz,
>> +                         enum thermal_notify_event reason);
>>          struct list_head        governor_list;
>>   };
>>
>> --