Re: [PATCH v3 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Zhang Rui <rui.zhang@intel.com>]

On Fri, 2022-07-15 at 23:09 +0200, Daniel Lezcano wrote:
> By convention the trips points are declared in the ascending
> temperature order. However, no specification for the device tree,
> ACPI
> or documentation tells the trip points must be ordered this way.
> 
> In the other hand, we need those to be ordered to browse them at the
> thermal events. But if we assume they are ordered and change the code
> based on this assumption, any platform with shuffled trip points
> description will be broken (if they exist).
> 
> Instead of taking the risk of breaking the existing platforms, use an
> array of temperature ordered trip identifiers and make it available
> for the code needing to browse the trip points in an ordered way.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 62 +++++++++++++++++++++++++++-----
> --
>  include/linux/thermal.h        |  2 ++
>  2 files changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index f66036b3daae..f02f38b66445 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -355,7 +355,8 @@ static void handle_critical_trips(struct
> thermal_zone_device *tz,
>  }
>  
>  static void handle_thermal_trip_crossed(struct thermal_zone_device
> *tz, int trip,
> -                                       int trip_temp, int trip_hyst,
> enum thermal_trip_type trip_type)
> +                                       int trip_temp, int trip_hyst,
> +                                       enum thermal_trip_type
> trip_type)
>  {
>         if (tz->last_temperature == THERMAL_TEMP_INVALID)
>                 return;
> @@ -1165,6 +1166,46 @@ static void bind_tz(struct thermal_zone_device
> *tz)
>         mutex_unlock(&thermal_list_lock);
>  }
>  
> +static void sort_trips_indexes(struct thermal_zone_device *tz)
> +{
> +       int i, j;
> +
> +       for (i = 0; i < tz->trips; i++)
> +               tz->trips_indexes[i] = i;
> + 
> +       for (i = 0; i < tz->trips; i++) {
> +               for (j = i + 1; j < tz->trips; j++) {
> +                       int t1, t2;
> +
> +                       tz->ops->get_trip_temp(tz, tz-
> >trips_indexes[i], &t1);

This line can be moved to the upper loop.

> +                       tz->ops->get_trip_temp(tz, tz-
> >trips_indexes[j], &t2);
> +

what about the disabled trip points?

we should ignore those trip points and check the return value to make
sure we're comparing the valid trip_temp values.

thanks,
rui

> +                       if (t1 > t2)
> +                               swap(tz->trips_indexes[i], tz-
> >trips_indexes[j]);
> +               }
> +       }
> +}
> +
> +static int thermal_zone_device_trip_init(struct thermal_zone_device
> *tz)
> +{
> +       enum thermal_trip_type trip_type;
> +       int trip_temp, i;
> +       
> +       tz->trips_indexes = kzalloc(tz->trips * sizeof(int),
> GFP_KERNEL);
> +       if (!tz->trips_indexes)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < tz->trips; i++) {
> +               if (tz->ops->get_trip_type(tz, i, &trip_type) ||
> +                   tz->ops->get_trip_temp(tz, i, &trip_temp) ||
> !trip_temp)
> +                       set_bit(i, &tz->trips_disabled);
> +       }
> +
> +       sort_trips_indexes(tz);
> +
> +       return 0;
> +}
> +
>  /**
>   * thermal_zone_device_register() - register a new thermal zone
> device
>   * @type:      the thermal zone device type
> @@ -1196,11 +1237,8 @@ thermal_zone_device_register(const char *type,
> int trips, int mask,
>                              int polling_delay)
>  {
>         struct thermal_zone_device *tz;
> -       enum thermal_trip_type trip_type;
> -       int trip_temp;
>         int id;
>         int result;
> -       int count;
>         struct thermal_governor *governor;
>  
>         if (!type || strlen(type) == 0) {
> @@ -1272,12 +1310,9 @@ thermal_zone_device_register(const char *type,
> int trips, int mask,
>         if (result)
>                 goto release_device;
>  
> -       for (count = 0; count < trips; count++) {
> -               if (tz->ops->get_trip_type(tz, count, &trip_type) ||
> -                   tz->ops->get_trip_temp(tz, count, &trip_temp) ||
> -                   !trip_temp)
> -                       set_bit(count, &tz->trips_disabled);
> -       }
> +       result = thermal_zone_device_trip_init(tz);
> +       if (result)
> +               goto unregister;
>  
>         /* Update 'this' zone's governor information */
>         mutex_lock(&thermal_governor_lock);
> @@ -1290,7 +1325,7 @@ thermal_zone_device_register(const char *type,
> int trips, int mask,
>         result = thermal_set_governor(tz, governor);
>         if (result) {
>                 mutex_unlock(&thermal_governor_lock);
> -               goto unregister;
> +               goto kfree_indexes;
>         }
>  
>         mutex_unlock(&thermal_governor_lock);
> @@ -1298,7 +1333,7 @@ thermal_zone_device_register(const char *type,
> int trips, int mask,
>         if (!tz->tzp || !tz->tzp->no_hwmon) {
>                 result = thermal_add_hwmon_sysfs(tz);
>                 if (result)
> -                       goto unregister;
> +                       goto kfree_indexes;
>         }
>  
>         mutex_lock(&thermal_list_lock);
> @@ -1319,6 +1354,8 @@ thermal_zone_device_register(const char *type,
> int trips, int mask,
>  
>         return tz;
>  
> +kfree_indexes:
> +       kfree(tz->trips_indexes);
>  unregister:
>         device_del(&tz->device);
>  release_device:
> @@ -1387,6 +1424,7 @@ void thermal_zone_device_unregister(struct
> thermal_zone_device *tz)
>         thermal_remove_hwmon_sysfs(tz);
>         ida_simple_remove(&thermal_tz_ida, tz->id);
>         ida_destroy(&tz->ida);
> +       kfree(tz->trips_indexes);
>         mutex_destroy(&tz->lock);
>         device_unregister(&tz->device);
>  
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 231bac2768fb..4c3b72536772 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -112,6 +112,7 @@ struct thermal_cooling_device {
>   * @mode:              current mode of this thermal zone
>   * @devdata:   private pointer for device private data
>   * @trips:     number of trip points the thermal zone supports
> + * @trips_indexes:     an array of sorted trip points indexes
>   * @trips_disabled;    bitmap for disabled trips
>   * @passive_delay_jiffies: number of jiffies to wait between polls
> when
>   *                     performing passive cooling.
> @@ -152,6 +153,7 @@ struct thermal_zone_device {
>         enum thermal_device_mode mode;
>         void *devdata;
>         int trips;
> +       int *trips_indexes;
>         unsigned long trips_disabled;   /* bitmap for disabled trips
> */
>         unsigned long passive_delay_jiffies;
>         unsigned long polling_delay_jiffies;