Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

From: Amit Kucheria
Date: Wed Jul 01 2020 - 05:26:18 EST


On Wed, Jul 1, 2020 at 1:27 PM Zhang Rui <rui.zhang@xxxxxxxxx> wrote:
>
> On Wed, 2020-07-01 at 09:35 +0200, Daniel Lezcano wrote:
> > On 30/06/2020 17:09, Zhang Rui wrote:
> > > Hi, Daniel,
> > >
> > > seems that you forgot to cc linux-pm mailing list.
> > >
> > > On Tue, 2020-06-30 at 17:16 +0530, Amit Kucheria wrote:
> > > > On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> > > > <daniel.lezcano@xxxxxxxxxx> wrote:
> > > > >
> > > > > The cdev, tz and governor list, as well as their respective
> > > > > locks
> > > > > are
> > > > > statically defined in the thermal_core.c file.
> > > > >
> > > > > In order to give a sane access to these list, like browsing all
> > > > > the
> > > > > thermal zones or all the cooling devices, let's define a set of
> > > > > helpers where we pass a callback as a parameter to be called
> > > > > for
> > > > > each
> > > > > thermal entity.
> > > > >
> > > > > We keep the self-encapsulation and ensure the locks are
> > > > > correctly
> > > > > taken when looking at the list.
> > > > >
> > > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > > > > ---
> > > > > drivers/thermal/thermal_core.c | 51
> > > > > ++++++++++++++++++++++++++++++++++
> > > >
> > > > Is the idea to not use thermal_helpers.c from now on? It fits
> > > > perfectly with a patch I have to merge all its contents to
> > > > thermal_core.c :-)
> > >
> > > I agree these changes should be in thermal_helper.c
> >
> > Oh, actually I remind put those functions in the thermal_core.c file
> > because they need the locks which are statically defined in there.
> >
> > If the functions are moved to thermal_helper.c that will imply to
> > export
> > the locks outside of the file, thus breaking the self-encapsulation.
> >
> > Do you want to move them out?
>
> Then no. I don't have any objection of removing thermal_helper.c, so
> you can just leave these functions in thermal_core.c

In that case, Daniel, please find attached a patch to move the
contents of thermal_helpers into thermal_core.c

Feel free to include it at the top of this series and modify as you see fit.

Regards,
Amit
From e09befc6c6717c3e4554b0688c7d83b0696d2554 Mon Sep 17 00:00:00 2001
Message-Id: <e09befc6c6717c3e4554b0688c7d83b0696d2554.1593595413.git.amit.kucheria@xxxxxxxxxx>
From: Amit Kucheria <amit.kucheria@xxxxxxxxxx>
Date: Wed, 1 Jul 2020 14:48:47 +0530
Subject: [PATCH] thermal/core: Merge thermal_helpers.c into thermal_core.c

Moving several helper functions to this file require unnecessarily
exposing locks outside thermal_core.c.

It is better to simply move the few functions in there directly into
thermal_core.c.

Signed-off-by: Amit Kucheria <amit.kucheria@xxxxxxxxxx>
---
drivers/thermal/Makefile | 3 +-
drivers/thermal/thermal_core.c | 216 +++++++++++++++++++++++++++
drivers/thermal/thermal_helpers.c | 238 ------------------------------
3 files changed, 217 insertions(+), 240 deletions(-)
delete mode 100644 drivers/thermal/thermal_helpers.c

diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 0c8b84a09b9aa..ed2fc83fe2128 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -4,8 +4,7 @@
#

obj-$(CONFIG_THERMAL) += thermal_sys.o
-thermal_sys-y += thermal_core.o thermal_sysfs.o \
- thermal_helpers.o
+thermal_sys-y += thermal_core.o thermal_sysfs.o

# interface to/from other layers providing sensors
thermal_sys-$(CONFIG_THERMAL_HWMON) += thermal_hwmon.o
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index b71196eaf90e8..a9e927f7224e9 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -43,6 +43,222 @@ static bool power_off_triggered;

static struct thermal_governor *def_governor;

+/* Various helper functions */
+
+int get_tz_trend(struct thermal_zone_device *tz, int trip)
+{
+ enum thermal_trend trend;
+
+ if (tz->emul_temperature || !tz->ops->get_trend ||
+ tz->ops->get_trend(tz, trip, &trend)) {
+ if (tz->temperature > tz->last_temperature)
+ trend = THERMAL_TREND_RAISING;
+ else if (tz->temperature < tz->last_temperature)
+ trend = THERMAL_TREND_DROPPING;
+ else
+ trend = THERMAL_TREND_STABLE;
+ }
+
+ return trend;
+}
+EXPORT_SYMBOL(get_tz_trend);
+
+struct thermal_instance *
+get_thermal_instance(struct thermal_zone_device *tz,
+ struct thermal_cooling_device *cdev, int trip)
+{
+ struct thermal_instance *pos = NULL;
+ struct thermal_instance *target_instance = NULL;
+
+ mutex_lock(&tz->lock);
+ mutex_lock(&cdev->lock);
+
+ list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
+ if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
+ target_instance = pos;
+ break;
+ }
+ }
+
+ mutex_unlock(&cdev->lock);
+ mutex_unlock(&tz->lock);
+
+ return target_instance;
+}
+EXPORT_SYMBOL(get_thermal_instance);
+
+/**
+ * thermal_zone_get_temp() - returns the temperature of a thermal zone
+ * @tz: a valid pointer to a struct thermal_zone_device
+ * @temp: a valid pointer to where to store the resulting temperature.
+ *
+ * When a valid thermal zone reference is passed, it will fetch its
+ * temperature and fill @temp.
+ *
+ * Return: On success returns 0, an error code otherwise
+ */
+int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+ int ret = -EINVAL;
+ int count;
+ int crit_temp = INT_MAX;
+ enum thermal_trip_type type;
+
+ if (!tz || IS_ERR(tz) || !tz->ops->get_temp)
+ goto exit;
+
+ mutex_lock(&tz->lock);
+
+ ret = tz->ops->get_temp(tz, temp);
+
+ if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
+ for (count = 0; count < tz->trips; count++) {
+ ret = tz->ops->get_trip_type(tz, count, &type);
+ if (!ret && type == THERMAL_TRIP_CRITICAL) {
+ ret = tz->ops->get_trip_temp(tz, count,
+ &crit_temp);
+ break;
+ }
+ }
+
+ /*
+ * Only allow emulating a temperature when the real temperature
+ * is below the critical temperature so that the emulation code
+ * cannot hide critical conditions.
+ */
+ if (!ret && *temp < crit_temp)
+ *temp = tz->emul_temperature;
+ }
+
+ mutex_unlock(&tz->lock);
+exit:
+ return ret;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
+
+/**
+ * thermal_zone_set_trips - Computes the next trip points for the driver
+ * @tz: a pointer to a thermal zone device structure
+ *
+ * The function computes the next temperature boundaries by browsing
+ * the trip points. The result is the closer low and high trip points
+ * to the current temperature. These values are passed to the backend
+ * driver to let it set its own notification mechanism (usually an
+ * interrupt).
+ *
+ * It does not return a value
+ */
+void thermal_zone_set_trips(struct thermal_zone_device *tz)
+{
+ int low = -INT_MAX;
+ int high = INT_MAX;
+ int trip_temp, hysteresis;
+ int i, ret;
+
+ mutex_lock(&tz->lock);
+
+ if (!tz->ops->set_trips || !tz->ops->get_trip_hyst)
+ goto exit;
+
+ for (i = 0; i < tz->trips; i++) {
+ int trip_low;
+
+ tz->ops->get_trip_temp(tz, i, &trip_temp);
+ tz->ops->get_trip_hyst(tz, i, &hysteresis);
+
+ trip_low = trip_temp - hysteresis;
+
+ if (trip_low < tz->temperature && trip_low > low)
+ low = trip_low;
+
+ if (trip_temp > tz->temperature && trip_temp < high)
+ high = trip_temp;
+ }
+
+ /* No need to change trip points */
+ if (tz->prev_low_trip == low && tz->prev_high_trip == high)
+ goto exit;
+
+ tz->prev_low_trip = low;
+ tz->prev_high_trip = high;
+
+ dev_dbg(&tz->device,
+ "new temperature boundaries: %d < x < %d\n", low, high);
+
+ /*
+ * Set a temperature window. When this window is left the driver
+ * must inform the thermal core via thermal_zone_device_update.
+ */
+ ret = tz->ops->set_trips(tz, low, high);
+ if (ret)
+ dev_err(&tz->device, "Failed to set trips: %d\n", ret);
+
+exit:
+ mutex_unlock(&tz->lock);
+}
+
+void thermal_cdev_update(struct thermal_cooling_device *cdev)
+{
+ struct thermal_instance *instance;
+ unsigned long target = 0;
+
+ mutex_lock(&cdev->lock);
+ /* cooling device is updated*/
+ if (cdev->updated) {
+ mutex_unlock(&cdev->lock);
+ return;
+ }
+
+ /* Make sure cdev enters the deepest cooling state */
+ list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
+ dev_dbg(&cdev->device, "zone%d->target=%lu\n",
+ instance->tz->id, instance->target);
+ if (instance->target == THERMAL_NO_TARGET)
+ continue;
+ if (instance->target > target)
+ target = instance->target;
+ }
+
+ if (!cdev->ops->set_cur_state(cdev, target))
+ thermal_cooling_device_stats_update(cdev, target);
+
+ cdev->updated = true;
+ mutex_unlock(&cdev->lock);
+ trace_cdev_update(cdev, target);
+ dev_dbg(&cdev->device, "set to state %lu\n", target);
+}
+EXPORT_SYMBOL(thermal_cdev_update);
+
+/**
+ * thermal_zone_get_slope - return the slope attribute of the thermal zone
+ * @tz: thermal zone device with the slope attribute
+ *
+ * Return: If the thermal zone device has a slope attribute, return it, else
+ * return 1.
+ */
+int thermal_zone_get_slope(struct thermal_zone_device *tz)
+{
+ if (tz && tz->tzp)
+ return tz->tzp->slope;
+ return 1;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_get_slope);
+
+/**
+ * thermal_zone_get_offset - return the offset attribute of the thermal zone
+ * @tz: thermal zone device with the offset attribute
+ *
+ * Return: If the thermal zone device has a offset attribute, return it, else
+ * return 0.
+ */
+int thermal_zone_get_offset(struct thermal_zone_device *tz)
+{
+ if (tz && tz->tzp)
+ return tz->tzp->offset;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_get_offset);
+
/*
* Governor section: set of functions to handle thermal governors
*
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
deleted file mode 100644
index 87b1256fa2f20..0000000000000
--- a/drivers/thermal/thermal_helpers.c
+++ /dev/null
@@ -1,238 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * thermal_helpers.c - helper functions to handle thermal devices
- *
- * Copyright (C) 2016 Eduardo Valentin <edubezval@xxxxxxxxx>
- *
- * Highly based on original thermal_core.c
- * Copyright (C) 2008 Intel Corp
- * Copyright (C) 2008 Zhang Rui <rui.zhang@xxxxxxxxx>
- * Copyright (C) 2008 Sujith Thomas <sujith.thomas@xxxxxxxxx>
- */
-
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/device.h>
-#include <linux/err.h>
-#include <linux/export.h>
-#include <linux/slab.h>
-#include <linux/string.h>
-#include <linux/sysfs.h>
-
-#include <trace/events/thermal.h>
-
-#include "thermal_core.h"
-
-int get_tz_trend(struct thermal_zone_device *tz, int trip)
-{
- enum thermal_trend trend;
-
- if (tz->emul_temperature || !tz->ops->get_trend ||
- tz->ops->get_trend(tz, trip, &trend)) {
- if (tz->temperature > tz->last_temperature)
- trend = THERMAL_TREND_RAISING;
- else if (tz->temperature < tz->last_temperature)
- trend = THERMAL_TREND_DROPPING;
- else
- trend = THERMAL_TREND_STABLE;
- }
-
- return trend;
-}
-EXPORT_SYMBOL(get_tz_trend);
-
-struct thermal_instance *
-get_thermal_instance(struct thermal_zone_device *tz,
- struct thermal_cooling_device *cdev, int trip)
-{
- struct thermal_instance *pos = NULL;
- struct thermal_instance *target_instance = NULL;
-
- mutex_lock(&tz->lock);
- mutex_lock(&cdev->lock);
-
- list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
- if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
- target_instance = pos;
- break;
- }
- }
-
- mutex_unlock(&cdev->lock);
- mutex_unlock(&tz->lock);
-
- return target_instance;
-}
-EXPORT_SYMBOL(get_thermal_instance);
-
-/**
- * thermal_zone_get_temp() - returns the temperature of a thermal zone
- * @tz: a valid pointer to a struct thermal_zone_device
- * @temp: a valid pointer to where to store the resulting temperature.
- *
- * When a valid thermal zone reference is passed, it will fetch its
- * temperature and fill @temp.
- *
- * Return: On success returns 0, an error code otherwise
- */
-int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
-{
- int ret = -EINVAL;
- int count;
- int crit_temp = INT_MAX;
- enum thermal_trip_type type;
-
- if (!tz || IS_ERR(tz) || !tz->ops->get_temp)
- goto exit;
-
- mutex_lock(&tz->lock);
-
- ret = tz->ops->get_temp(tz, temp);
-
- if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
- for (count = 0; count < tz->trips; count++) {
- ret = tz->ops->get_trip_type(tz, count, &type);
- if (!ret && type == THERMAL_TRIP_CRITICAL) {
- ret = tz->ops->get_trip_temp(tz, count,
- &crit_temp);
- break;
- }
- }
-
- /*
- * Only allow emulating a temperature when the real temperature
- * is below the critical temperature so that the emulation code
- * cannot hide critical conditions.
- */
- if (!ret && *temp < crit_temp)
- *temp = tz->emul_temperature;
- }
-
- mutex_unlock(&tz->lock);
-exit:
- return ret;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
-
-/**
- * thermal_zone_set_trips - Computes the next trip points for the driver
- * @tz: a pointer to a thermal zone device structure
- *
- * The function computes the next temperature boundaries by browsing
- * the trip points. The result is the closer low and high trip points
- * to the current temperature. These values are passed to the backend
- * driver to let it set its own notification mechanism (usually an
- * interrupt).
- *
- * It does not return a value
- */
-void thermal_zone_set_trips(struct thermal_zone_device *tz)
-{
- int low = -INT_MAX;
- int high = INT_MAX;
- int trip_temp, hysteresis;
- int i, ret;
-
- mutex_lock(&tz->lock);
-
- if (!tz->ops->set_trips || !tz->ops->get_trip_hyst)
- goto exit;
-
- for (i = 0; i < tz->trips; i++) {
- int trip_low;
-
- tz->ops->get_trip_temp(tz, i, &trip_temp);
- tz->ops->get_trip_hyst(tz, i, &hysteresis);
-
- trip_low = trip_temp - hysteresis;
-
- if (trip_low < tz->temperature && trip_low > low)
- low = trip_low;
-
- if (trip_temp > tz->temperature && trip_temp < high)
- high = trip_temp;
- }
-
- /* No need to change trip points */
- if (tz->prev_low_trip == low && tz->prev_high_trip == high)
- goto exit;
-
- tz->prev_low_trip = low;
- tz->prev_high_trip = high;
-
- dev_dbg(&tz->device,
- "new temperature boundaries: %d < x < %d\n", low, high);
-
- /*
- * Set a temperature window. When this window is left the driver
- * must inform the thermal core via thermal_zone_device_update.
- */
- ret = tz->ops->set_trips(tz, low, high);
- if (ret)
- dev_err(&tz->device, "Failed to set trips: %d\n", ret);
-
-exit:
- mutex_unlock(&tz->lock);
-}
-
-void thermal_cdev_update(struct thermal_cooling_device *cdev)
-{
- struct thermal_instance *instance;
- unsigned long target = 0;
-
- mutex_lock(&cdev->lock);
- /* cooling device is updated*/
- if (cdev->updated) {
- mutex_unlock(&cdev->lock);
- return;
- }
-
- /* Make sure cdev enters the deepest cooling state */
- list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
- dev_dbg(&cdev->device, "zone%d->target=%lu\n",
- instance->tz->id, instance->target);
- if (instance->target == THERMAL_NO_TARGET)
- continue;
- if (instance->target > target)
- target = instance->target;
- }
-
- if (!cdev->ops->set_cur_state(cdev, target))
- thermal_cooling_device_stats_update(cdev, target);
-
- cdev->updated = true;
- mutex_unlock(&cdev->lock);
- trace_cdev_update(cdev, target);
- dev_dbg(&cdev->device, "set to state %lu\n", target);
-}
-EXPORT_SYMBOL(thermal_cdev_update);
-
-/**
- * thermal_zone_get_slope - return the slope attribute of the thermal zone
- * @tz: thermal zone device with the slope attribute
- *
- * Return: If the thermal zone device has a slope attribute, return it, else
- * return 1.
- */
-int thermal_zone_get_slope(struct thermal_zone_device *tz)
-{
- if (tz && tz->tzp)
- return tz->tzp->slope;
- return 1;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_get_slope);
-
-/**
- * thermal_zone_get_offset - return the offset attribute of the thermal zone
- * @tz: thermal zone device with the offset attribute
- *
- * Return: If the thermal zone device has a offset attribute, return it, else
- * return 0.
- */
-int thermal_zone_get_offset(struct thermal_zone_device *tz)
-{
- if (tz && tz->tzp)
- return tz->tzp->offset;
- return 0;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_get_offset);
--
2.25.1