Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230525140135.3589917-1-daniel.lezcano@linaro.org> <20230525140135.3589917-5-daniel.lezcano@linaro.org>
In-Reply-To: <20230525140135.3589917-5-daniel.lezcano@linaro.org>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 20 Jun 2023 13:28:47 +0200
Message-ID: <CAJZ5v0iv6HkxmV08JyUO3K1YMPXerEb5qNobVkUNv2zW+qVh+w@mail.gmail.com>
Subject: Re: [PATCH 4/8] thermal/core: Update the generic trip points
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     rafael@kernel.org, linux-pm@vger.kernel.org,
        thierry.reding@gmail.com, Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Thu, May 25, 2023 at 4:02 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> At this point, the generic trip points rework allows to create a
> thermal zone with a fixed number of trip points. This usage satisfy
> almost all of the existing drivers.
>
> A few remaining drivers have a mechanism where the firmware updates
> the trip points. But there is no such update mechanism for the generic
> trip points, thus those drivers can not be converted to the generic
> approach.
>
> This patch provides a function 'thermal_zone_trips_update()' allowing
> to change the trip points of a thermal zone.
>
> At the same time, with the logic the trip points array is passed as a
> parameter to the thermal zone at creation time, we make our own
> private trip points array by copying the one passed as parameter.

So the design seems to require the caller to create a new array of
trip points and pass it to thermal_zone_trips_update(), so it can
replace the zone's trips array with it.

If only one trip point changes and there are multiple defined, this is
rather not efficient.

Do you want to prevent the core from using stale trip points this way?
 If so, it should be stated here.

Moreover, at least in the cases when num_trips doesn't change, it
might be more efficient to walk the new trips[] array and only copy
the ones that have changed over their old versions.

I am also not sure if this interface is going to be convenient from
the user's perspective, especially if the trips get sorted by the core
(in the future).  They would need to recreate the entire trips array
every time from scratch, even if only one trip point changes, which
means quite a bit of churn for thermal zones with many trip points.

It might be better to allow them to update trips in place and notify
the core about the change, all under the zone lock to prevent the core
from using trips simultaneously.

And arguably, changing num_trips would be questionable due to the
sysfs consistency reasons mentioned below.

> Note, no code has been found where the trip points update leads to a
> refresh of the trip points in sysfs, so it is very unlikey the number
> of trip points changes. However, for the sake of consistency it would
> be nicer to have the trip points being refreshed in sysfs also, but
> that could be done in a separate set of changes.

So at this point user space has already enumerated the trip points, so
it may fail if some of them go away or it may not be able to use any
new trip points appearing in sysfs.

For this reason, until there is a way to notify user space about the
need to re-enumerate trip points (and user space indicates support for
it), the only trip point property that may change in sysfs is the
temperature.

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 40 ++++++++---------
>  drivers/thermal/thermal_core.h |  3 ++
>  drivers/thermal/thermal_trip.c | 78 ++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h        |  4 ++
>  4 files changed, 102 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index afcd4197babd..3688b06401c8 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1224,32 +1224,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>                 return ERR_PTR(-EINVAL);
>         }
>
> -       /*
> -        * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
> -        * For example, shifting by 32 will result in compiler warning:
> -        * warning: right shift count >= width of type [-Wshift-count- overflow]
> -        *
> -        * Also "mask >> num_trips" will always be true with 32 bit shift.
> -        * E.g. mask = 0x80000000 for trip id 31 to be RW. Then
> -        * mask >> 32 = 0x80000000
> -        * This will result in failure for the below condition.
> -        *
> -        * Check will be true when the bit 31 of the mask is set.
> -        * 32 bit shift will cause overflow of 4 byte integer.
> -        */
> -       if (num_trips > (BITS_PER_TYPE(int) - 1) || num_trips < 0 || mask >> num_trips) {
> -               pr_err("Incorrect number of thermal trips\n");
> -               return ERR_PTR(-EINVAL);
> -       }
> -
>         if (!ops) {
>                 pr_err("Thermal zone device ops not defined\n");
>                 return ERR_PTR(-EINVAL);
>         }
>
> -       if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
> -               return ERR_PTR(-EINVAL);
> -
>         if (!thermal_class)
>                 return ERR_PTR(-ENODEV);
>
> @@ -1283,8 +1262,22 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>         tz->ops = ops;
>         tz->device.class = thermal_class;
>         tz->devdata = devdata;
> -       tz->trips = trips;
> -       tz->num_trips = num_trips;
> +
> +       if (trips) {
> +               result = __thermal_zone_trips_update(tz, trips, num_trips, mask);
> +               if (result)
> +                       goto remove_id;
> +       } else {
> +               /*
> +                * Legacy trip point handling
> +                */
> +               if ((!tz->ops->get_trip_type || !tz->ops->get_trip_temp) && num_trips) {
> +                       result = -EINVAL;
> +                       goto remove_id;
> +               }
> +
> +               tz->num_trips = num_trips;
> +       }
>
>         thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
>         thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
> @@ -1451,6 +1444,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>         mutex_unlock(&tz->lock);
>
>         kfree(tz->tzp);
> +       kfree(tz->trips);
>
>         put_device(&tz->device);
>
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 84ada34ff079..c27a9930f904 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -125,6 +125,9 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz,
>  void __thermal_zone_set_trips(struct thermal_zone_device *tz);
>  int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
>                             struct thermal_trip *trip);
> +int __thermal_zone_trips_update(struct thermal_zone_device *tz,
> +                               struct thermal_trip *trips,
> +                               int num_trips, int mask);
>  int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
>
>  /* sysfs I/F */
> diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> index 61d927c35776..171b1902c01c 100644
> --- a/drivers/thermal/thermal_trip.c
> +++ b/drivers/thermal/thermal_trip.c
> @@ -7,6 +7,8 @@
>   *
>   * Thermal trips handling
>   */
> +#include <linux/slab.h>
> +
>  #define THERMAL_CORE_SUBSYS
>  #include "thermal_core.h"
>
> @@ -181,3 +183,79 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
>
>         return 0;
>  }
> +
> +int __thermal_zone_trips_update(struct thermal_zone_device *tz, struct thermal_trip *trips,
> +                               int num_trips, int mask)
> +{
> +       struct thermal_trip *new_trips;
> +
> +       /*
> +        * Legacy trip point handling is incompatible with this
> +        * function
> +        */
> +       if (tz->ops->get_trip_type || tz->ops->get_trip_temp) {
> +               pr_err("Legacy trip points use incompatible function '%s'\n", __func__);
> +               return -ENOSYS;
> +       }
> +
> +       /*
> +        * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
> +        * For example, shifting by 32 will result in compiler warning:
> +        * warning: right shift count >= width of type [-Wshift-count- overflow]
> +        *
> +        * Also "mask >> num_trips" will always be true with 32 bit shift.
> +        * E.g. mask = 0x80000000 for trip id 31 to be RW. Then
> +        * mask >> 32 = 0x80000000
> +        * This will result in failure for the below condition.
> +        *
> +        * Check will be true when the bit 31 of the mask is set.
> +        * 32 bit shift will cause overflow of 4 byte integer.
> +        */
> +       if (num_trips > (BITS_PER_TYPE(int) - 1) || num_trips < 0 || mask >> num_trips) {
> +               pr_err("Incorrect number of thermal trips\n");
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * New generic trip point handling
> +        */
> +       if (num_trips > 0 && !trips) {
> +               pr_err("Inconsistent parameters\n");
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * Allocate our private trip points array structure
> +        */
> +       new_trips = kmemdup(trips, sizeof(*trips) * num_trips, GFP_KERNEL);
> +       if (!new_trips)
> +               return -ENOMEM;
> +
> +       /*
> +        * Newly allocated thermal zone will have the 'trips' field
> +        * NULL, kfree() is immune against that
> +        */
> +       kfree(tz->trips);
> +       tz->trips = new_trips;
> +       tz->num_trips = num_trips;
> +
> +       return 0;
> +}
> +
> +int thermal_zone_trips_update(struct thermal_zone_device *tz, struct thermal_trip *trips,
> +                             int num_trips, int mask)
> +{
> +       int ret;
> +
> +       mutex_lock(&tz->lock);
> +       ret = __thermal_zone_trips_update(tz, trips, num_trips, mask);
> +       mutex_unlock(&tz->lock);
> +
> +       if (ret)
> +               return ret;
> +
> +       __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);

This is called under tz->lock in other places AFAICS.  Why not here?

> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_trips_update);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 87837094d549..83937256a01c 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -291,6 +291,10 @@ int thermal_zone_get_num_trips(struct thermal_zone_device *tz);
>
>  int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp);
>
> +int thermal_zone_trips_update(struct thermal_zone_device *tz,
> +                             struct thermal_trip *trips,
> +                             int num_trips, int mask);
> +
>  #ifdef CONFIG_THERMAL_ACPI
>  int thermal_acpi_active_trip_temp(struct acpi_device *adev, int id, int *ret_temp);
>  int thermal_acpi_passive_trip_temp(struct acpi_device *adev, int *ret_temp);
> --