[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 4 Apr 2024 23:08:52 +0100
From: Lukasz Luba <lukasz.luba@....com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: LKML <linux-kernel@...r.kernel.org>, Linux PM <linux-pm@...r.kernel.org>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v3 1/6] thermal: core: Move threshold out of struct
thermal_trip
On 4/2/24 19:56, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> The threshold field in struct thermal_trip is only used internally by
> the thermal core and it is better to prevent drivers from misusing it.
> It also takes some space unnecessarily in the trip tables passed by
> drivers to the core during thermal zone registration.
>
> For this reason, introduce struct thermal_trip_desc as a wrapper around
> struct thermal_trip, move the threshold field directly into it and make
> the thermal core store struct thermal_trip_desc objects in the internal
> thermal zone trip tables. Adjust all of the code using trip tables in
> the thermal core accordingly.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>
> v2 -> v3: Rebase on top of 6.9-rc2, minor changelog update.
>
> v1 -> v2: No changes.
>
> ---
> drivers/thermal/gov_fair_share.c | 7 +++--
> drivers/thermal/gov_power_allocator.c | 6 ++--
> drivers/thermal/thermal_core.c | 46 +++++++++++++++++++---------------
> drivers/thermal/thermal_core.h | 7 +++--
> drivers/thermal/thermal_debugfs.c | 6 ++--
> drivers/thermal/thermal_helpers.c | 8 +++--
> drivers/thermal/thermal_netlink.c | 6 ++--
> drivers/thermal/thermal_sysfs.c | 20 +++++++-------
> drivers/thermal/thermal_trip.c | 15 +++++------
> include/linux/thermal.h | 9 ++++--
> 10 files changed, 78 insertions(+), 52 deletions(-)
>
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -61,7 +61,6 @@ enum thermal_notify_event {
> * struct thermal_trip - representation of a point in temperature domain
> * @temperature: temperature value in miliCelsius
> * @hysteresis: relative hysteresis in miliCelsius
> - * @threshold: trip crossing notification threshold miliCelsius
> * @type: trip point type
> * @priv: pointer to driver data associated with this trip
> * @flags: flags representing binary properties of the trip
> @@ -69,12 +68,16 @@ enum thermal_notify_event {
> struct thermal_trip {
> int temperature;
> int hysteresis;
> - int threshold;
> enum thermal_trip_type type;
> u8 flags;
> void *priv;
> };
>
> +struct thermal_trip_desc {
> + struct thermal_trip trip;
> + int threshold;
> +};
> +
> #define THERMAL_TRIP_FLAG_RW_TEMP BIT(0)
> #define THERMAL_TRIP_FLAG_RW_HYST BIT(1)
>
> @@ -203,7 +206,7 @@ struct thermal_zone_device {
> #ifdef CONFIG_THERMAL_DEBUGFS
> struct thermal_debugfs *debugfs;
> #endif
> - struct thermal_trip trips[] __counted_by(num_trips);
> + struct thermal_trip_desc trips[] __counted_by(num_trips);
> };
>
> /**
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -361,17 +361,19 @@ static void handle_critical_trips(struct
> }
>
> static void handle_thermal_trip(struct thermal_zone_device *tz,
> - struct thermal_trip *trip)
> + struct thermal_trip_desc *td)
> {
> + const struct thermal_trip *trip = &td->trip;
> +
> if (trip->temperature == THERMAL_TEMP_INVALID)
> return;
>
> if (tz->last_temperature == THERMAL_TEMP_INVALID) {
> /* Initialization. */
> - trip->threshold = trip->temperature;
> - if (tz->temperature >= trip->threshold)
> - trip->threshold -= trip->hysteresis;
> - } else if (tz->last_temperature < trip->threshold) {
> + td->threshold = trip->temperature;
> + if (tz->temperature >= td->threshold)
> + td->threshold -= trip->hysteresis;
> + } else if (tz->last_temperature < td->threshold) {
> /*
> * The trip threshold is equal to the trip temperature, unless
> * the latter has changed in the meantime. In either case,
> @@ -382,9 +384,9 @@ static void handle_thermal_trip(struct t
> if (tz->temperature >= trip->temperature) {
> thermal_notify_tz_trip_up(tz, trip);
> thermal_debug_tz_trip_up(tz, trip);
> - trip->threshold = trip->temperature - trip->hysteresis;
> + td->threshold = trip->temperature - trip->hysteresis;
> } else {
> - trip->threshold = trip->temperature;
> + td->threshold = trip->temperature;
> }
> } else {
> /*
> @@ -400,9 +402,9 @@ static void handle_thermal_trip(struct t
> if (tz->temperature < trip->temperature - trip->hysteresis) {
> thermal_notify_tz_trip_down(tz, trip);
> thermal_debug_tz_trip_down(tz, trip);
> - trip->threshold = trip->temperature;
> + td->threshold = trip->temperature;
> } else {
> - trip->threshold = trip->temperature - trip->hysteresis;
> + td->threshold = trip->temperature - trip->hysteresis;
> }
> }
>
> @@ -458,7 +460,7 @@ static void thermal_zone_device_init(str
> void __thermal_zone_device_update(struct thermal_zone_device *tz,
> enum thermal_notify_event event)
> {
> - struct thermal_trip *trip;
> + struct thermal_trip_desc *td;
>
> if (tz->suspended)
> return;
> @@ -472,8 +474,8 @@ void __thermal_zone_device_update(struct
>
> tz->notify_event = event;
>
> - for_each_trip(tz, trip)
> - handle_thermal_trip(tz, trip);
> + for_each_trip_desc(tz, td)
> + handle_thermal_trip(tz, td);
>
> monitor_thermal_zone(tz);
> }
> @@ -766,7 +768,7 @@ int thermal_zone_bind_cooling_device(str
> if (trip_index < 0 || trip_index >= tz->num_trips)
> return -EINVAL;
>
> - return thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index], cdev,
> + return thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index].trip, cdev,
> upper, lower, weight);
> }
> EXPORT_SYMBOL_GPL(thermal_zone_bind_cooling_device);
> @@ -825,7 +827,7 @@ int thermal_zone_unbind_cooling_device(s
> if (trip_index < 0 || trip_index >= tz->num_trips)
> return -EINVAL;
>
> - return thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index], cdev);
> + return thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index].trip, cdev);
> }
> EXPORT_SYMBOL_GPL(thermal_zone_unbind_cooling_device);
>
> @@ -1221,16 +1223,19 @@ static void thermal_set_delay_jiffies(un
>
> int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp)
> {
> - int i, ret = -EINVAL;
> + const struct thermal_trip_desc *td;
> + int ret = -EINVAL;
>
> if (tz->ops.get_crit_temp)
> return tz->ops.get_crit_temp(tz, temp);
>
> mutex_lock(&tz->lock);
>
> - for (i = 0; i < tz->num_trips; i++) {
> - if (tz->trips[i].type == THERMAL_TRIP_CRITICAL) {
> - *temp = tz->trips[i].temperature;
> + for_each_trip_desc(tz, td) {
> + const struct thermal_trip *trip = &td->trip;
> +
> + if (trip->type == THERMAL_TRIP_CRITICAL) {
> + *temp = trip->temperature;
> ret = 0;
> break;
> }
> @@ -1274,7 +1279,9 @@ thermal_zone_device_register_with_trips(
> const struct thermal_zone_params *tzp,
> int passive_delay, int polling_delay)
> {
> + const struct thermal_trip *trip = trips;
> struct thermal_zone_device *tz;
> + struct thermal_trip_desc *td;
> int id;
> int result;
> struct thermal_governor *governor;
> @@ -1339,7 +1346,8 @@ thermal_zone_device_register_with_trips(
> tz->device.class = thermal_class;
> tz->devdata = devdata;
> tz->num_trips = num_trips;
> - memcpy(tz->trips, trips, num_trips * sizeof(*trips));
> + for_each_trip_desc(tz, td)
> + td->trip = *trip++;
>
> thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
> thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -120,8 +120,11 @@ void thermal_governor_update_tz(struct t
> enum thermal_notify_event reason);
>
> /* Helpers */
> -#define for_each_trip(__tz, __trip) \
> - for (__trip = __tz->trips; __trip - __tz->trips < __tz->num_trips; __trip++)
> +#define for_each_trip_desc(__tz, __td) \
> + for (__td = __tz->trips; __td - __tz->trips < __tz->num_trips; __td++)
> +
> +#define trip_to_trip_desc(__trip) \
> + container_of(__trip, struct thermal_trip_desc, trip)
>
> void __thermal_zone_set_trips(struct thermal_zone_device *tz);
> int thermal_zone_trip_id(const struct thermal_zone_device *tz,
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -88,7 +88,7 @@ trip_point_type_show(struct device *dev,
> if (sscanf(attr->attr.name, "trip_point_%d_type", &trip_id) != 1)
> return -EINVAL;
>
> - switch (tz->trips[trip_id].type) {
> + switch (tz->trips[trip_id].trip.type) {
This could be a bit shorter, with some helper variable, but that's
minor cosmetic.
> case THERMAL_TRIP_CRITICAL:
> return sprintf(buf, "critical\n");
> case THERMAL_TRIP_HOT:
> @@ -120,7 +120,7 @@ trip_point_temp_store(struct device *dev
>
> mutex_lock(&tz->lock);
>
> - trip = &tz->trips[trip_id];
> + trip = &tz->trips[trip_id].trip;
>
> if (temp != trip->temperature) {
> if (tz->ops.set_trip_temp) {
> @@ -150,7 +150,7 @@ trip_point_temp_show(struct device *dev,
> if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
> return -EINVAL;
>
> - return sprintf(buf, "%d\n", tz->trips[trip_id].temperature);
> + return sprintf(buf, "%d\n", tz->trips[trip_id].trip.temperature);
> }
>
> static ssize_t
> @@ -171,7 +171,7 @@ trip_point_hyst_store(struct device *dev
>
> mutex_lock(&tz->lock);
>
> - trip = &tz->trips[trip_id];
> + trip = &tz->trips[trip_id].trip;
>
> if (hyst != trip->hysteresis) {
> trip->hysteresis = hyst;
> @@ -194,7 +194,7 @@ trip_point_hyst_show(struct device *dev,
> if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
> return -EINVAL;
>
> - return sprintf(buf, "%d\n", tz->trips[trip_id].hysteresis);
> + return sprintf(buf, "%d\n", tz->trips[trip_id].trip.hysteresis);
> }
>
> static ssize_t
> @@ -393,7 +393,7 @@ static const struct attribute_group *the
> */
> static int create_trip_attrs(struct thermal_zone_device *tz)
> {
> - const struct thermal_trip *trip;
> + const struct thermal_trip_desc *td;
> struct attribute **attrs;
>
> /* This function works only for zones with at least one trip */
> @@ -429,8 +429,8 @@ static int create_trip_attrs(struct ther
> return -ENOMEM;
> }
>
> - for_each_trip(tz, trip) {
> - int indx = thermal_zone_trip_id(tz, trip);
> + for_each_trip_desc(tz, td) {
> + int indx = thermal_zone_trip_id(tz, &td->trip);
>
> /* create trip type attribute */
> snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
> @@ -452,7 +452,7 @@ static int create_trip_attrs(struct ther
> tz->trip_temp_attrs[indx].name;
> tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
> tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
> - if (trip->flags & THERMAL_TRIP_FLAG_RW_TEMP) {
> + if (td->trip.flags & THERMAL_TRIP_FLAG_RW_TEMP) {
> tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
> tz->trip_temp_attrs[indx].attr.store =
> trip_point_temp_store;
> @@ -467,7 +467,7 @@ static int create_trip_attrs(struct ther
> tz->trip_hyst_attrs[indx].name;
> tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
> tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
> - if (trip->flags & THERMAL_TRIP_FLAG_RW_HYST) {
> + if (td->trip.flags & THERMAL_TRIP_FLAG_RW_HYST) {
> tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
> tz->trip_hyst_attrs[indx].attr.store =
> trip_point_hyst_store;
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -744,7 +744,7 @@ static void tze_seq_stop(struct seq_file
> static int tze_seq_show(struct seq_file *s, void *v)
> {
> struct thermal_zone_device *tz = s->private;
> - struct thermal_trip *trip;
> + struct thermal_trip_desc *td;
> struct tz_episode *tze;
> const char *type;
> int trip_id;
> @@ -757,7 +757,9 @@ static int tze_seq_show(struct seq_file
>
> seq_printf(s, "| trip | type | temp(°mC) | hyst(°mC) | duration | avg(°mC) | min(°mC) | max(°mC) |\n");
>
> - for_each_trip(tz, trip) {
> + for_each_trip_desc(tz, td) {
> + const struct thermal_trip *trip = &td->trip;
> +
> /*
> * There is no possible mitigation happening at the
> * critical trip point, so the stats will be always
> Index: linux-pm/drivers/thermal/thermal_netlink.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_netlink.c
> +++ linux-pm/drivers/thermal/thermal_netlink.c
> @@ -442,7 +442,7 @@ out_cancel_nest:
> static int thermal_genl_cmd_tz_get_trip(struct param *p)
> {
> struct sk_buff *msg = p->msg;
> - const struct thermal_trip *trip;
> + const struct thermal_trip_desc *td;
> struct thermal_zone_device *tz;
> struct nlattr *start_trip;
> int id;
> @@ -462,7 +462,9 @@ static int thermal_genl_cmd_tz_get_trip(
>
> mutex_lock(&tz->lock);
>
> - for_each_trip(tz, trip) {
> + for_each_trip_desc(tz, td) {
> + const struct thermal_trip *trip = &td->trip;
> +
> if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_ID,
> thermal_zone_trip_id(tz, trip)) ||
> nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_TYPE, trip->type) ||
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -13,11 +13,11 @@ int for_each_thermal_trip(struct thermal
> int (*cb)(struct thermal_trip *, void *),
> void *data)
> {
> - struct thermal_trip *trip;
> + struct thermal_trip_desc *td;
> int ret;
>
> - for_each_trip(tz, trip) {
> - ret = cb(trip, data);
> + for_each_trip_desc(tz, td) {
> + ret = cb(&td->trip, data);
> if (ret)
> return ret;
> }
> @@ -63,7 +63,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_num_t
> */
> void __thermal_zone_set_trips(struct thermal_zone_device *tz)
> {
> - const struct thermal_trip *trip;
> + const struct thermal_trip_desc *td;
> int low = -INT_MAX, high = INT_MAX;
> int ret;
>
> @@ -72,7 +72,8 @@ void __thermal_zone_set_trips(struct the
> if (!tz->ops.set_trips)
> return;
>
> - for_each_trip(tz, trip) {
> + for_each_trip_desc(tz, td) {
> + const struct thermal_trip *trip = &td->trip;
> int trip_low;
>
> trip_low = trip->temperature - trip->hysteresis;
> @@ -110,7 +111,7 @@ int __thermal_zone_get_trip(struct therm
> if (!tz || trip_id < 0 || trip_id >= tz->num_trips || !trip)
> return -EINVAL;
>
> - *trip = tz->trips[trip_id];
> + *trip = tz->trips[trip_id].trip;
> return 0;
> }
> EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
> @@ -135,7 +136,7 @@ int thermal_zone_trip_id(const struct th
> * Assume the trip to be located within the bounds of the thermal
> * zone's trips[] table.
> */
> - return trip - tz->trips;
> + return trip_to_trip_desc(trip) - tz->trips;
> }
> void thermal_zone_trip_updated(struct thermal_zone_device *tz,
> const struct thermal_trip *trip)
> Index: linux-pm/drivers/thermal/gov_fair_share.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_fair_share.c
> +++ linux-pm/drivers/thermal/gov_fair_share.c
> @@ -17,10 +17,13 @@
>
> static int get_trip_level(struct thermal_zone_device *tz)
> {
> - const struct thermal_trip *trip, *level_trip = NULL;
> + const struct thermal_trip *level_trip = NULL;
> + const struct thermal_trip_desc *td;
> int trip_level = -1;
>
> - for_each_trip(tz, trip) {
> + for_each_trip_desc(tz, td) {
> + const struct thermal_trip *trip = &td->trip;
> +
> if (trip->temperature >= tz->temperature)
> continue;
>
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -496,9 +496,11 @@ static void get_governor_trips(struct th
> const struct thermal_trip *first_passive = NULL;
> const struct thermal_trip *last_passive = NULL;
> const struct thermal_trip *last_active = NULL;
> - const struct thermal_trip *trip;
> + const struct thermal_trip_desc *td;
> +
> + for_each_trip_desc(tz, td) {
> + const struct thermal_trip *trip = &td->trip;
>
> - for_each_trip(tz, trip) {
> switch (trip->type) {
> case THERMAL_TRIP_PASSIVE:
> if (!first_passive) {
> Index: linux-pm/drivers/thermal/thermal_helpers.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_helpers.c
> +++ linux-pm/drivers/thermal/thermal_helpers.c
> @@ -50,7 +50,7 @@ get_thermal_instance(struct thermal_zone
> mutex_lock(&tz->lock);
> mutex_lock(&cdev->lock);
>
> - trip = &tz->trips[trip_index];
> + trip = &tz->trips[trip_index].trip;
>
> list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
> if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
> @@ -82,7 +82,7 @@ EXPORT_SYMBOL(get_thermal_instance);
> */
> int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
> {
> - const struct thermal_trip *trip;
> + const struct thermal_trip_desc *td;
> int crit_temp = INT_MAX;
> int ret = -EINVAL;
>
> @@ -91,7 +91,9 @@ int __thermal_zone_get_temp(struct therm
> ret = tz->ops.get_temp(tz, temp);
>
> if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
> - for_each_trip(tz, trip) {
> + for_each_trip_desc(tz, td) {
> + const struct thermal_trip *trip = &td->trip;
> +
> if (trip->type == THERMAL_TRIP_CRITICAL) {
> crit_temp = trip->temperature;
> break;
>
>
>
LGTM,
Reviewed-by: Lukasz Luba <lukasz.luba@....com>
Powered by blists - more mailing lists