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

[Lukasz Luba <lukasz.luba@arm.com>]

On 12/20/23 17:44, Rafael J. Wysocki wrote:
> On Wed, Dec 20, 2023 at 5:16 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> 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?
> 
> I think so, IIUC.

OK, thanks!