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

[Rafael J. Wysocki]

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.

> 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).

> 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.

> +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.

> +
>  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.

>         }
>         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.

> +
>                         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.

>  };
>
>  /**
> @@ -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;
>  };
>
> --