Hi All,
The ACPI thermal driver has undergone some significant changes recently, but
there is still room for improvements in it.
First off, it turns out that a small rearrangement of its internal data
structures allows code duplication in it to be reduced quite a bit (patches
[01-04/13].
Next, by changing the way it binds cooling devices to thermal zones (and trips
within them), the use of trip point indices can be eliminated from it (patch
[11/13]) which then allows its internal data structures to be simplified even
further (patch [12/13]).
However, in order to make those latter changes, it is useful to modify struct
thermal_instance to carry a trip pointer instead a trip index (patch [05/13])
which then allows the core to be adjusted to facilitate using trip pointers for
cooling device binding and unbinding (patch [10/13]).
Meanwhile, the modification of struct thermal_instance mentioned above also
helps to reduce the thermal governors overhead related to using
__thermal_zone_get_trip() that carries out bounds checking and copies trip
point data which both are not necessary in the governor code. Some related
cleanups of thermal governors can be done as well (patches [06-09/13].
Finally, it is prudent to visually distinguish the names of structure fields
and variables that carry temperature values in different units, so patch
[13/13] changes the names of those items in the ACPI thermal driver that are
used to store temperature values in deci-Kelvin.
This series is on top of the series of ACPI thermal driver posted last week:
https://patchwork.kernel.org/project/linux-acpi/list/?series=783543
and a couple of recent thermal core patches:
https://patchwork.kernel.org/project/linux-pm/patch/12296181.O9o76ZdvQC@kreacher/
https://patchwork.kernel.org/project/linux-pm/patch/5981326.lOV4Wx5bFT@kreacher/
It will be exposed in a separate git branch for easier access.
Please see the individual patch changelogs for details.
Thanks!
From: Rafael J. Wysocki <[email protected]>
The critical_valid and hot_valid flags in struct acpi_thermal_trips are
only used during initialization and they are only false if the
corresponding trip temperatures are equal to THERMAL_TEMP_INVALID, so
drop them and use THERMAL_TEMP_INVALID checks instead of them where
applicable.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/thermal.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -100,8 +100,6 @@ struct acpi_thermal_active {
struct acpi_thermal_trips {
struct acpi_thermal_passive passive;
struct acpi_thermal_active active[ACPI_THERMAL_MAX_ACTIVE];
- bool critical_valid;
- bool hot_valid;
};
struct acpi_thermal {
@@ -355,13 +353,13 @@ static long acpi_thermal_get_critical_tr
}
if (crt == -1) {
acpi_handle_debug(tz->device->handle, "Critical threshold disabled\n");
- goto fail;
+ return THERMAL_TEMP_INVALID;
}
status = acpi_evaluate_integer(tz->device->handle, "_CRT", NULL, &tmp);
if (ACPI_FAILURE(status)) {
acpi_handle_debug(tz->device->handle, "No critical threshold\n");
- goto fail;
+ return THERMAL_TEMP_INVALID;
}
if (tmp <= 2732) {
/*
@@ -369,17 +367,12 @@ static long acpi_thermal_get_critical_tr
* so discard them as invalid.
*/
pr_info(FW_BUG "Invalid critical threshold (%llu)\n", tmp);
- goto fail;
+ return THERMAL_TEMP_INVALID;
}
set:
- tz->trips.critical_valid = true;
acpi_handle_debug(tz->device->handle, "Critical threshold [%llu]\n", tmp);
return tmp;
-
-fail:
- tz->trips.critical_valid = false;
- return THERMAL_TEMP_INVALID;
}
static long acpi_thermal_get_hot_trip(struct acpi_thermal *tz)
@@ -389,12 +382,10 @@ static long acpi_thermal_get_hot_trip(st
status = acpi_evaluate_integer(tz->device->handle, "_HOT", NULL, &tmp);
if (ACPI_FAILURE(status)) {
- tz->trips.hot_valid = false;
acpi_handle_debug(tz->device->handle, "No hot threshold\n");
return THERMAL_TEMP_INVALID;
}
- tz->trips.hot_valid = true;
acpi_handle_debug(tz->device->handle, "Hot threshold [%llu]\n", tmp);
return tmp;
}
@@ -789,7 +780,7 @@ static void acpi_thermal_aml_dependency_
*/
static void acpi_thermal_guess_offset(struct acpi_thermal *tz, long crit_temp)
{
- if (tz->trips.critical_valid && crit_temp % 5 == 1)
+ if (crit_temp != THERMAL_TEMP_INVALID && crit_temp % 5 == 1)
tz->kelvin_offset = 273100;
else
tz->kelvin_offset = 273200;
@@ -850,11 +841,11 @@ static int acpi_thermal_add(struct acpi_
trip_count = acpi_thermal_get_trip_points(tz);
crit_temp = acpi_thermal_get_critical_trip(tz);
- if (tz->trips.critical_valid)
+ if (crit_temp != THERMAL_TEMP_INVALID)
trip_count++;
hot_temp = acpi_thermal_get_hot_trip(tz);
- if (tz->trips.hot_valid)
+ if (hot_temp != THERMAL_TEMP_INVALID)
trip_count++;
if (!trip_count) {
@@ -886,13 +877,13 @@ static int acpi_thermal_add(struct acpi_
tz->trip_table = trip;
- if (tz->trips.critical_valid) {
+ if (crit_temp != THERMAL_TEMP_INVALID) {
trip->type = THERMAL_TRIP_CRITICAL;
trip->temperature = acpi_thermal_temp(tz, crit_temp);
trip++;
}
- if (tz->trips.hot_valid) {
+ if (hot_temp != THERMAL_TEMP_INVALID) {
trip->type = THERMAL_TRIP_HOT;
trip->temperature = acpi_thermal_temp(tz, hot_temp);
trip++;
From: Rafael J. Wysocki <[email protected]>
Rearrange the ACPI thermal driver's callback functions used for cooling
device binding and unbinding, acpi_thermal_bind_cooling_device() and
acpi_thermal_unbind_cooling_device(), respectively, so that they use trip
pointers instead of trip indices which is more straightforward and allows
the driver to become independent of the ordering of trips in the thermal
zone structure.
The general functionality is not expected to be changed.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/thermal.c | 114 +++++++++++++++++++------------------------------
1 file changed, 46 insertions(+), 68 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -568,94 +568,72 @@ static void acpi_thermal_zone_device_cri
thermal_zone_device_critical(thermal);
}
-static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
- struct thermal_cooling_device *cdev,
- bool bind)
+struct acpi_thermal_bind_data {
+ struct thermal_zone_device *thermal;
+ struct thermal_cooling_device *cdev;
+ bool bind;
+};
+
+static int bind_unbind_cdev_cb(struct thermal_trip *trip, void *arg)
{
- struct acpi_device *device = cdev->devdata;
- struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
- struct acpi_thermal_trip *acpi_trip;
- struct acpi_device *dev;
- acpi_handle handle;
+ struct acpi_thermal_trip *acpi_trip = trip->priv;
+ struct acpi_thermal_bind_data *bd = arg;
+ struct thermal_zone_device *thermal = bd->thermal;
+ struct thermal_cooling_device *cdev = bd->cdev;
+ struct acpi_device *cdev_adev = cdev->devdata;
int i;
- int j;
- int trip = -1;
- int result = 0;
-
- if (tz->trips.critical_valid)
- trip++;
-
- if (tz->trips.hot_valid)
- trip++;
-
- acpi_trip = &tz->trips.passive.trip;
- if (acpi_thermal_trip_valid(acpi_trip)) {
- trip++;
- for (i = 0; i < acpi_trip->devices.count; i++) {
- handle = acpi_trip->devices.handles[i];
- dev = acpi_fetch_acpi_dev(handle);
- if (dev != device)
- continue;
-
- if (bind)
- result = thermal_zone_bind_cooling_device(
- thermal, trip, cdev,
- THERMAL_NO_LIMIT,
- THERMAL_NO_LIMIT,
- THERMAL_WEIGHT_DEFAULT);
- else
- result =
- thermal_zone_unbind_cooling_device(
- thermal, trip, cdev);
- if (result)
- goto failed;
+ /* Skip critical and hot trips. */
+ if (!acpi_trip)
+ return 0;
+
+ for (i = 0; i < acpi_trip->devices.count; i++) {
+ acpi_handle handle = acpi_trip->devices.handles[i];
+ struct acpi_device *adev = acpi_fetch_acpi_dev(handle);
+
+ if (adev != cdev_adev)
+ continue;
+
+ if (bd->bind) {
+ int ret;
+
+ ret = thermal_bind_cdev_to_trip(thermal, trip, cdev,
+ THERMAL_NO_LIMIT,
+ THERMAL_NO_LIMIT,
+ THERMAL_WEIGHT_DEFAULT);
+ if (ret)
+ return ret;
+ } else {
+ thermal_unbind_cdev_from_trip(thermal, trip, cdev);
}
}
- for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
- acpi_trip = &tz->trips.active[i].trip;
- if (!acpi_thermal_trip_valid(acpi_trip))
- break;
-
- trip++;
- for (j = 0; j < acpi_trip->devices.count; j++) {
- handle = acpi_trip->devices.handles[j];
- dev = acpi_fetch_acpi_dev(handle);
- if (dev != device)
- continue;
-
- if (bind)
- result = thermal_zone_bind_cooling_device(
- thermal, trip, cdev,
- THERMAL_NO_LIMIT,
- THERMAL_NO_LIMIT,
- THERMAL_WEIGHT_DEFAULT);
- else
- result = thermal_zone_unbind_cooling_device(
- thermal, trip, cdev);
+ return 0;
+}
- if (result)
- goto failed;
- }
- }
+static int acpi_thermal_bind_unbind_cdev(struct thermal_zone_device *thermal,
+ struct thermal_cooling_device *cdev,
+ bool bind)
+{
+ struct acpi_thermal_bind_data bd = {
+ .thermal = thermal, .cdev = cdev, .bind = bind
+ };
-failed:
- return result;
+ return for_each_thermal_trip(thermal, bind_unbind_cdev_cb, &bd);
}
static int
acpi_thermal_bind_cooling_device(struct thermal_zone_device *thermal,
struct thermal_cooling_device *cdev)
{
- return acpi_thermal_cooling_device_cb(thermal, cdev, true);
+ return acpi_thermal_bind_unbind_cdev(thermal, cdev, true);
}
static int
acpi_thermal_unbind_cooling_device(struct thermal_zone_device *thermal,
struct thermal_cooling_device *cdev)
{
- return acpi_thermal_cooling_device_cb(thermal, cdev, false);
+ return acpi_thermal_bind_unbind_cdev(thermal, cdev, false);
}
static struct thermal_zone_device_ops acpi_thermal_zone_ops = {
From: Rafael J. Wysocki <[email protected]>
Add new helper functions, thermal_bind_cdev_to_trip() and
thermal_unbind_cdev_from_trip(), to allow a trip pointer to be used for
binding a cooling device to a trip point and unbinding it, respectively,
and redefine the existing helpers, thermal_zone_bind_cooling_device()
and thermal_zone_unbind_cooling_device(), as wrappers around the new
ones, respectively.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/thermal_core.c | 54 +++++++++++++++++++++++++----------------
include/linux/thermal.h | 8 ++++++
2 files changed, 42 insertions(+), 20 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -600,10 +600,9 @@ struct thermal_zone_device *thermal_zone
*/
/**
- * thermal_zone_bind_cooling_device() - bind a cooling device to a thermal zone
+ * thermal_bind_cdev_to_trip - bind a cooling device to a thermal zone
* @tz: pointer to struct thermal_zone_device
- * @trip_index: indicates which trip point the cooling devices is
- * associated with in this thermal zone.
+ * @trip: trip point the cooling devices is associated with in this zone.
* @cdev: pointer to struct thermal_cooling_device
* @upper: the Maximum cooling state for this trip point.
* THERMAL_NO_LIMIT means no upper limit,
@@ -621,8 +620,8 @@ struct thermal_zone_device *thermal_zone
*
* Return: 0 on success, the proper error value otherwise.
*/
-int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
- int trip_index,
+int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
+ const struct thermal_trip *trip,
struct thermal_cooling_device *cdev,
unsigned long upper, unsigned long lower,
unsigned int weight)
@@ -631,15 +630,9 @@ int thermal_zone_bind_cooling_device(str
struct thermal_instance *pos;
struct thermal_zone_device *pos1;
struct thermal_cooling_device *pos2;
- const struct thermal_trip *trip;
bool upper_no_limit;
int result;
- if (trip_index >= tz->num_trips || trip_index < 0)
- return -EINVAL;
-
- trip = &tz->trips[trip_index];
-
list_for_each_entry(pos1, &thermal_tz_list, node) {
if (pos1 == tz)
break;
@@ -736,14 +729,26 @@ free_mem:
kfree(dev);
return result;
}
+EXPORT_SYMBOL_GPL(thermal_bind_cdev_to_trip);
+
+int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
+ int trip_index,
+ struct thermal_cooling_device *cdev,
+ unsigned long upper, unsigned long lower,
+ unsigned int weight)
+{
+ if (trip_index < 0 || trip_index >= tz->num_trips)
+ return -EINVAL;
+
+ return thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index], cdev,
+ upper, lower, weight);
+}
EXPORT_SYMBOL_GPL(thermal_zone_bind_cooling_device);
/**
- * thermal_zone_unbind_cooling_device() - unbind a cooling device from a
- * thermal zone.
+ * thermal_unbind_cdev_from_trip - unbind a cooling device from a thermal zone.
* @tz: pointer to a struct thermal_zone_device.
- * @trip_index: indicates which trip point the cooling devices is
- * associated with in this thermal zone.
+ * @trip: trip point the cooling devices is associated with in this zone.
* @cdev: pointer to a struct thermal_cooling_device.
*
* This interface function unbind a thermal cooling device from the certain
@@ -752,16 +757,14 @@ EXPORT_SYMBOL_GPL(thermal_zone_bind_cool
*
* Return: 0 on success, the proper error value otherwise.
*/
-int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
- int trip_index,
- struct thermal_cooling_device *cdev)
+int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
+ const struct thermal_trip *trip,
+ struct thermal_cooling_device *cdev)
{
struct thermal_instance *pos, *next;
- const struct thermal_trip *trip;
mutex_lock(&tz->lock);
mutex_lock(&cdev->lock);
- trip = &tz->trips[trip_index];
list_for_each_entry_safe(pos, next, &tz->thermal_instances, tz_node) {
if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
list_del(&pos->tz_node);
@@ -784,6 +787,17 @@ unbind:
kfree(pos);
return 0;
}
+EXPORT_SYMBOL_GPL(thermal_unbind_cdev_from_trip);
+
+int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
+ int trip_index,
+ struct thermal_cooling_device *cdev)
+{
+ if (trip_index < 0 || trip_index >= tz->num_trips)
+ return -EINVAL;
+
+ return thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index], cdev);
+}
EXPORT_SYMBOL_GPL(thermal_zone_unbind_cooling_device);
static void thermal_release(struct device *dev)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -320,10 +320,18 @@ const char *thermal_zone_device_type(str
int thermal_zone_device_id(struct thermal_zone_device *tzd);
struct device *thermal_zone_device(struct thermal_zone_device *tzd);
+int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
+ const struct thermal_trip *trip,
+ struct thermal_cooling_device *cdev,
+ unsigned long upper, unsigned long lower,
+ unsigned int weight);
int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
struct thermal_cooling_device *,
unsigned long, unsigned long,
unsigned int);
+int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
+ const struct thermal_trip *trip,
+ struct thermal_cooling_device *cdev);
int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
struct thermal_cooling_device *);
void thermal_zone_device_update(struct thermal_zone_device *,
From: Rafael J. Wysocki <[email protected]>
In order to reduce code duplication, merge update_passive_devices() and
update_active_devices() into one function called update_trip_devices()
that will be used for updating both the passive and active trip points.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/thermal.c | 53 ++++++++++++++++++-------------------------------
1 file changed, 20 insertions(+), 33 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -43,6 +43,8 @@
#define ACPI_THERMAL_MAX_ACTIVE 10
#define ACPI_THERMAL_MAX_LIMIT_STR_LEN 65
+#define ACPI_THERMAL_TRIP_PASSIVE (-1)
+
/*
* This exception is thrown out in two cases:
* 1.An invalid trip point becomes invalid or a valid trip point becomes invalid
@@ -202,18 +204,25 @@ static void acpi_thermal_update_passive_
ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
}
-static bool update_passive_devices(struct acpi_thermal *tz, bool compare)
+static bool update_trip_devices(struct acpi_thermal *tz,
+ struct acpi_thermal_trip *acpi_trip,
+ int index, bool compare)
{
- struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
struct acpi_handle_list devices;
+ char method[] = "_PSL";
acpi_status status;
+ if (index != ACPI_THERMAL_TRIP_PASSIVE) {
+ method[1] = 'A';
+ method[2] = 'L';
+ method[3] = '0' + index;
+ }
+
memset(&devices, 0, sizeof(devices));
- status = acpi_evaluate_reference(tz->device->handle, "_PSL", NULL, &devices);
+ status = acpi_evaluate_reference(tz->device->handle, method, NULL, &devices);
if (ACPI_FAILURE(status)) {
- acpi_handle_info(tz->device->handle,
- "Missing device list for passive threshold\n");
+ acpi_handle_info(tz->device->handle, "%s evaluation failure\n", method);
return false;
}
@@ -231,8 +240,9 @@ static void acpi_thermal_update_passive_
if (!acpi_thermal_trip_valid(acpi_trip))
return;
- if (update_passive_devices(tz, true))
+ if (update_trip_devices(tz, acpi_trip, ACPI_THERMAL_TRIP_PASSIVE, true)) {
return;
+ }
acpi_trip->temperature = THERMAL_TEMP_INVALID;
ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
@@ -273,30 +283,6 @@ static void acpi_thermal_update_active_t
ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
}
-static bool update_active_devices(struct acpi_thermal *tz, int index, bool compare)
-{
- char method[] = { '_', 'A', 'L', '0' + index, '\0' };
- struct acpi_thermal_trip *acpi_trip = &tz->trips.active[index].trip;
- struct acpi_handle_list devices;
- acpi_status status;
-
- memset(&devices, 0, sizeof(devices));
-
- status = acpi_evaluate_reference(tz->device->handle, method, NULL, &devices);
- if (ACPI_FAILURE(status)) {
- acpi_handle_info(tz->device->handle,
- "Missing device list for active threshold %d\n",
- index);
- return false;
- }
-
- if (compare && memcmp(&acpi_trip->devices, &devices, sizeof(devices)))
- ACPI_THERMAL_TRIPS_EXCEPTION(tz, "device");
-
- memcpy(&acpi_trip->devices, &devices, sizeof(devices));
- return true;
-}
-
static void acpi_thermal_update_active_devices(struct acpi_thermal *tz, int index)
{
struct acpi_thermal_trip *acpi_trip = &tz->trips.active[index].trip;
@@ -304,7 +290,7 @@ static void acpi_thermal_update_active_d
if (!acpi_thermal_trip_valid(acpi_trip))
return;
- if (update_active_devices(tz, index, true))
+ if (update_trip_devices(tz, acpi_trip, index, true))
return;
acpi_trip->temperature = THERMAL_TEMP_INVALID;
@@ -460,7 +446,8 @@ static bool acpi_thermal_init_passive_tr
tz->trips.passive.tsp = tmp;
- if (!update_passive_devices(tz, false))
+ if (!update_trip_devices(tz, &tz->trips.passive.trip,
+ ACPI_THERMAL_TRIP_PASSIVE, false))
goto fail;
tz->trips.passive.trip.temperature = temp;
@@ -482,7 +469,7 @@ static bool acpi_thermal_init_active_tri
if (temp == THERMAL_TEMP_INVALID)
goto fail;
- if (!update_active_devices(tz, index, false))
+ if (!update_trip_devices(tz, &tz->trips.active[index].trip, index, false))
goto fail;
tz->trips.active[index].trip.temperature = temp;
From: Rafael J. Wysocki <[email protected]>
In order to reduce code duplicationeve further, merge
acpi_thermal_init_passive/active_trip() into one function called
acpi_thermal_init_trip() that will be used for initializing both
the passive and active trip points.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/thermal.c | 64 ++++++++++++++++++++++---------------------------
1 file changed, 30 insertions(+), 34 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -399,72 +399,68 @@ static long acpi_thermal_get_hot_trip(st
return tmp;
}
-static bool acpi_thermal_init_passive_trip(struct acpi_thermal *tz)
+static bool passive_trip_params_init(struct acpi_thermal *tz)
{
unsigned long long tmp;
acpi_status status;
- int temp;
-
- if (psv == -1)
- goto fail;
-
- if (psv > 0) {
- temp = celsius_to_deci_kelvin(psv);
- } else {
- temp = get_passive_temp(tz);
- if (temp == THERMAL_TEMP_INVALID)
- goto fail;
- }
status = acpi_evaluate_integer(tz->device->handle, "_TC1", NULL, &tmp);
if (ACPI_FAILURE(status))
- goto fail;
+ return false;
tz->trips.passive.tc1 = tmp;
status = acpi_evaluate_integer(tz->device->handle, "_TC2", NULL, &tmp);
if (ACPI_FAILURE(status))
- goto fail;
+ return false;
tz->trips.passive.tc2 = tmp;
status = acpi_evaluate_integer(tz->device->handle, "_TSP", NULL, &tmp);
if (ACPI_FAILURE(status))
- goto fail;
+ return false;
tz->trips.passive.tsp = tmp;
- if (!update_trip_devices(tz, &tz->trips.passive.trip,
- ACPI_THERMAL_TRIP_PASSIVE, false))
- goto fail;
-
- tz->trips.passive.trip.temperature = temp;
return true;
-
-fail:
- tz->trips.passive.trip.temperature = THERMAL_TEMP_INVALID;
- return false;
}
-static bool acpi_thermal_init_active_trip(struct acpi_thermal *tz, int index)
+static bool acpi_thermal_init_trip(struct acpi_thermal *tz, int index)
{
+ struct acpi_thermal_trip *acpi_trip;
long temp;
- if (act == -1)
- goto fail;
+ if (index == ACPI_THERMAL_TRIP_PASSIVE) {
+ acpi_trip = &tz->trips.passive.trip;
+
+ if (psv == -1)
+ goto fail;
+
+ if (!passive_trip_params_init(tz))
+ goto fail;
+
+ temp = psv > 0 ? celsius_to_deci_kelvin(psv) :
+ get_passive_temp(tz);
+ } else {
+ acpi_trip = &tz->trips.active[index].trip;
+
+ if (act == -1)
+ goto fail;
+
+ temp = get_active_temp(tz, index);
+ }
- temp = get_active_temp(tz, index);
if (temp == THERMAL_TEMP_INVALID)
goto fail;
- if (!update_trip_devices(tz, &tz->trips.active[index].trip, index, false))
+ if (!update_trip_devices(tz, acpi_trip, index, false))
goto fail;
- tz->trips.active[index].trip.temperature = temp;
+ acpi_trip->temperature = temp;
return true;
fail:
- tz->trips.active[index].trip.temperature = THERMAL_TEMP_INVALID;
+ acpi_trip->temperature = THERMAL_TEMP_INVALID;
return false;
}
@@ -473,11 +469,11 @@ static int acpi_thermal_get_trip_points(
unsigned int count = 0;
int i;
- if (acpi_thermal_init_passive_trip(tz))
+ if (acpi_thermal_init_trip(tz, ACPI_THERMAL_TRIP_PASSIVE))
count++;
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
- if (acpi_thermal_init_active_trip(tz, i))
+ if (acpi_thermal_init_trip(tz, i))
count++;
else
break;
From: Rafael J. Wysocki <[email protected]>
Replace the integer trip number stored in struct thermal_instance with
a pointer to the relevant trip and adjust the code using the structure
in question accordingly.
The main reason for making this change is to allow the trip point to
cooling device binding code more straightforward, as illustrated by
subsequent modifications of the ACPI thermal driver, but it also helps
to clarify the overall design and allows the governor code overhead to
be reduced (through subsequent modifications).
The only case in which it adds complexity is trip_point_show() that
needs to walk the trips[] table to find the index of the given trip
point, but this is not a critical path and the interface that
trip_point_show() belongs to is problematic anyway (for instance, it
doesn't cover the case when the same cooling devices is associated
with multiple trip points).
This is a preliminary change and the affected code will be refined by
a series of subsequent modifications of thermal governors, the core and
the ACPI thermal driver.
The general functionality is not expected to be affected by this change.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/gov_bang_bang.c | 23 ++++++++---------------
drivers/thermal/gov_fair_share.c | 5 +++--
drivers/thermal/gov_power_allocator.c | 11 ++++++++---
drivers/thermal/gov_step_wise.c | 16 +++++++---------
drivers/thermal/thermal_core.c | 15 ++++++++++-----
drivers/thermal/thermal_core.h | 4 +++-
drivers/thermal/thermal_helpers.c | 5 ++++-
drivers/thermal/thermal_sysfs.c | 3 ++-
drivers/thermal/thermal_trip.c | 15 +++++++++++++++
9 files changed, 60 insertions(+), 37 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -87,7 +87,7 @@ struct thermal_instance {
char name[THERMAL_NAME_LENGTH];
struct thermal_zone_device *tz;
struct thermal_cooling_device *cdev;
- int trip;
+ const struct thermal_trip *trip;
bool initialized;
unsigned long upper; /* Highest cooling state for this trip point */
unsigned long lower; /* Lowest cooling state for this trip point */
@@ -119,6 +119,8 @@ void __thermal_zone_device_update(struct
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_trip_id(struct thermal_zone_device *tz,
+ const struct thermal_trip *trip);
int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
/* sysfs I/F */
Index: linux-pm/drivers/thermal/thermal_helpers.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_helpers.c
+++ linux-pm/drivers/thermal/thermal_helpers.c
@@ -42,14 +42,17 @@ int get_tz_trend(struct thermal_zone_dev
struct thermal_instance *
get_thermal_instance(struct thermal_zone_device *tz,
- struct thermal_cooling_device *cdev, int trip)
+ struct thermal_cooling_device *cdev, int trip_index)
{
struct thermal_instance *pos = NULL;
struct thermal_instance *target_instance = NULL;
+ const struct thermal_trip *trip;
mutex_lock(&tz->lock);
mutex_lock(&cdev->lock);
+ trip = &tz->trips[trip_index];
+
list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
target_instance = pos;
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -602,7 +602,7 @@ struct thermal_zone_device *thermal_zone
/**
* thermal_zone_bind_cooling_device() - bind a cooling device to a thermal zone
* @tz: pointer to struct thermal_zone_device
- * @trip: indicates which trip point the cooling devices is
+ * @trip_index: indicates which trip point the cooling devices is
* associated with in this thermal zone.
* @cdev: pointer to struct thermal_cooling_device
* @upper: the Maximum cooling state for this trip point.
@@ -622,7 +622,7 @@ struct thermal_zone_device *thermal_zone
* Return: 0 on success, the proper error value otherwise.
*/
int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
- int trip,
+ int trip_index,
struct thermal_cooling_device *cdev,
unsigned long upper, unsigned long lower,
unsigned int weight)
@@ -631,12 +631,15 @@ int thermal_zone_bind_cooling_device(str
struct thermal_instance *pos;
struct thermal_zone_device *pos1;
struct thermal_cooling_device *pos2;
+ const struct thermal_trip *trip;
bool upper_no_limit;
int result;
- if (trip >= tz->num_trips || trip < 0)
+ if (trip_index >= tz->num_trips || trip_index < 0)
return -EINVAL;
+ trip = &tz->trips[trip_index];
+
list_for_each_entry(pos1, &thermal_tz_list, node) {
if (pos1 == tz)
break;
@@ -739,7 +742,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_bind_cool
* thermal_zone_unbind_cooling_device() - unbind a cooling device from a
* thermal zone.
* @tz: pointer to a struct thermal_zone_device.
- * @trip: indicates which trip point the cooling devices is
+ * @trip_index: indicates which trip point the cooling devices is
* associated with in this thermal zone.
* @cdev: pointer to a struct thermal_cooling_device.
*
@@ -750,13 +753,15 @@ EXPORT_SYMBOL_GPL(thermal_zone_bind_cool
* Return: 0 on success, the proper error value otherwise.
*/
int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
- int trip,
+ int trip_index,
struct thermal_cooling_device *cdev)
{
struct thermal_instance *pos, *next;
+ const struct thermal_trip *trip;
mutex_lock(&tz->lock);
mutex_lock(&cdev->lock);
+ trip = &tz->trips[trip_index];
list_for_each_entry_safe(pos, next, &tz->thermal_instances, tz_node) {
if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
list_del(&pos->tz_node);
Index: linux-pm/drivers/thermal/gov_bang_bang.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_bang_bang.c
+++ linux-pm/drivers/thermal/gov_bang_bang.c
@@ -13,28 +13,21 @@
#include "thermal_core.h"
-static int thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_id)
+static int thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_index)
{
- struct thermal_trip trip;
+ const struct thermal_trip *trip = &tz->trips[trip_index];
struct thermal_instance *instance;
- int ret;
- ret = __thermal_zone_get_trip(tz, trip_id, &trip);
- if (ret) {
- pr_warn_once("Failed to retrieve trip point %d\n", trip_id);
- return ret;
- }
-
- if (!trip.hysteresis)
+ if (!trip->hysteresis)
dev_info_once(&tz->device,
"Zero hysteresis value for thermal zone %s\n", tz->type);
dev_dbg(&tz->device, "Trip%d[temp=%d]:temp=%d:hyst=%d\n",
- trip_id, trip.temperature, tz->temperature,
- trip.hysteresis);
+ trip_index, trip->temperature, tz->temperature,
+ trip->hysteresis);
list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
- if (instance->trip != trip_id)
+ if (instance->trip != trip)
continue;
/* in case fan is in initial state, switch the fan off */
@@ -52,10 +45,10 @@ static int thermal_zone_trip_update(stru
* enable fan when temperature exceeds trip_temp and disable
* the fan in case it falls below trip_temp minus hysteresis
*/
- if (instance->target == 0 && tz->temperature >= trip.temperature)
+ if (instance->target == 0 && tz->temperature >= trip->temperature)
instance->target = 1;
else if (instance->target == 1 &&
- tz->temperature <= trip.temperature - trip.hysteresis)
+ tz->temperature <= trip->temperature - trip->hysteresis)
instance->target = 0;
dev_dbg(&instance->cdev->device, "target=%d\n",
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
@@ -49,7 +49,7 @@ static long get_target_state(struct ther
/**
* fair_share_throttle - throttles devices associated with the given zone
* @tz: thermal_zone_device
- * @trip: trip point index
+ * @trip_index: trip point index
*
* Throttling Logic: This uses three parameters to calculate the new
* throttle state of the cooling devices associated with the given zone.
@@ -65,8 +65,9 @@ static long get_target_state(struct ther
* (Heavily assumes the trip points are in ascending order)
* new_state of cooling device = P3 * P2 * P1
*/
-static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
+static int fair_share_throttle(struct thermal_zone_device *tz, int trip_index)
{
+ const struct thermal_trip *trip = &tz->trips[trip_index];
struct thermal_instance *instance;
int total_weight = 0;
int total_instance = 0;
Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -81,26 +81,24 @@ static void update_passive_instance(stru
static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_id)
{
+ const struct thermal_trip *trip = &tz->trips[trip_id];
enum thermal_trend trend;
struct thermal_instance *instance;
- struct thermal_trip trip;
bool throttle = false;
int old_target;
- __thermal_zone_get_trip(tz, trip_id, &trip);
-
trend = get_tz_trend(tz, trip_id);
- if (tz->temperature >= trip.temperature) {
+ if (tz->temperature >= trip->temperature) {
throttle = true;
- trace_thermal_zone_trip(tz, trip_id, trip.type);
+ trace_thermal_zone_trip(tz, trip_id, trip->type);
}
dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
- trip_id, trip.type, trip.temperature, trend, throttle);
+ trip_id, trip->type, trip->temperature, trend, throttle);
list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
- if (instance->trip != trip_id)
+ if (instance->trip != trip)
continue;
old_target = instance->target;
@@ -114,11 +112,11 @@ static void thermal_zone_trip_update(str
/* Activate a passive thermal instance */
if (old_target == THERMAL_NO_TARGET &&
instance->target != THERMAL_NO_TARGET)
- update_passive_instance(tz, trip.type, 1);
+ update_passive_instance(tz, trip->type, 1);
/* Deactivate a passive thermal instance */
else if (old_target != THERMAL_NO_TARGET &&
instance->target == THERMAL_NO_TARGET)
- update_passive_instance(tz, trip.type, -1);
+ update_passive_instance(tz, trip->type, -1);
instance->initialized = true;
mutex_lock(&instance->cdev->lock);
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -943,7 +943,8 @@ trip_point_show(struct device *dev, stru
instance =
container_of(attr, struct thermal_instance, attr);
- return sprintf(buf, "%d\n", instance->trip);
+ return sprintf(buf, "%d\n",
+ thermal_zone_trip_id(instance->tz, instance->trip));
}
ssize_t
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -157,3 +157,18 @@ int thermal_zone_set_trip(struct thermal
return 0;
}
+
+int thermal_zone_trip_id(struct thermal_zone_device *tz,
+ const struct thermal_trip *trip)
+{
+ int i;
+
+ lockdep_assert_held(&tz->lock);
+
+ for (i = 0; i < tz->num_trips; i++) {
+ if (&tz->trips[i] == trip)
+ return i;
+ }
+
+ return -ENODATA;
+}
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
@@ -90,12 +90,14 @@ static u32 estimate_sustainable_power(st
u32 sustainable_power = 0;
struct thermal_instance *instance;
struct power_allocator_params *params = tz->governor_data;
+ const struct thermal_trip *trip_max_desired_temperature =
+ &tz->trips[params->trip_max_desired_temperature];
list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
struct thermal_cooling_device *cdev = instance->cdev;
u32 min_power;
- if (instance->trip != params->trip_max_desired_temperature)
+ if (instance->trip != trip_max_desired_temperature)
continue;
if (!cdev_is_power_actor(cdev))
@@ -383,12 +385,13 @@ static int allocate_power(struct thermal
{
struct thermal_instance *instance;
struct power_allocator_params *params = tz->governor_data;
+ const struct thermal_trip *trip_max_desired_temperature =
+ &tz->trips[params->trip_max_desired_temperature];
u32 *req_power, *max_power, *granted_power, *extra_actor_power;
u32 *weighted_req_power;
u32 total_req_power, max_allocatable_power, total_weighted_req_power;
u32 total_granted_power, power_range;
int i, num_actors, total_weight, ret = 0;
- int trip_max_desired_temperature = params->trip_max_desired_temperature;
num_actors = 0;
total_weight = 0;
@@ -564,12 +567,14 @@ static void allow_maximum_power(struct t
{
struct thermal_instance *instance;
struct power_allocator_params *params = tz->governor_data;
+ const struct thermal_trip *trip_max_desired_temperature =
+ &tz->trips[params->trip_max_desired_temperature];
u32 req_power;
list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
struct thermal_cooling_device *cdev = instance->cdev;
- if ((instance->trip != params->trip_max_desired_temperature) ||
+ if ((instance->trip != trip_max_desired_temperature) ||
(!cdev_is_power_actor(instance->cdev)))
continue;
From: Rafael J. Wysocki <[email protected]>
Make get_trip_level() access the thermal zone's trip table directly
instead of using __thermal_zone_get_trip() which adds overhead related
to the unnecessary bounds checking and copying the trip point data.
Also rearrange the code in it to make it somewhat easier to follow.
The general functionality is not expected to be changed.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/gov_fair_share.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
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
@@ -21,23 +21,21 @@
*/
static int get_trip_level(struct thermal_zone_device *tz)
{
- struct thermal_trip trip;
- int count;
+ const struct thermal_trip *trip = tz->trips;
+ int i;
- for (count = 0; count < tz->num_trips; count++) {
- __thermal_zone_get_trip(tz, count, &trip);
- if (tz->temperature < trip.temperature)
+ if (tz->temperature < trip->temperature)
+ return 0;
+
+ for (i = 0; i < tz->num_trips - 1; i++) {
+ trip++;
+ if (tz->temperature < trip->temperature)
break;
}
- /*
- * count > 0 only if temperature is greater than first trip
- * point, in which case, trip_point = count - 1
- */
- if (count > 0)
- trace_thermal_zone_trip(tz, count - 1, trip.type);
+ trace_thermal_zone_trip(tz, i, tz->trips[i].type);
- return count;
+ return i;
}
static long get_target_state(struct thermal_zone_device *tz,
From: Rafael J. Wysocki <[email protected]>
Rename function argumets used for passing trip point indices in thermal
governors and in the .throttle() callback to "trip_index" to avoid
confusion with trip pointer names.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/gov_bang_bang.c | 4 ++--
drivers/thermal/gov_step_wise.c | 16 ++++++++--------
drivers/thermal/gov_user_space.c | 6 +++---
include/linux/thermal.h | 2 +-
4 files changed, 14 insertions(+), 14 deletions(-)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -199,7 +199,7 @@ struct thermal_governor {
char name[THERMAL_NAME_LENGTH];
int (*bind_to_tz)(struct thermal_zone_device *tz);
void (*unbind_from_tz)(struct thermal_zone_device *tz);
- int (*throttle)(struct thermal_zone_device *tz, int trip);
+ int (*throttle)(struct thermal_zone_device *tz, int trip_index);
struct list_head governor_list;
};
Index: linux-pm/drivers/thermal/gov_user_space.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_user_space.c
+++ linux-pm/drivers/thermal/gov_user_space.c
@@ -25,11 +25,11 @@ static int user_space_bind(struct therma
/**
* notify_user_space - Notifies user space about thermal events
* @tz: thermal_zone_device
- * @trip: trip point index
+ * @trip_index: trip point index
*
* This function notifies the user space through UEvents.
*/
-static int notify_user_space(struct thermal_zone_device *tz, int trip)
+static int notify_user_space(struct thermal_zone_device *tz, int trip_index)
{
char *thermal_prop[5];
int i;
@@ -38,7 +38,7 @@ static int notify_user_space(struct ther
thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", tz->type);
thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", tz->temperature);
- thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d", trip);
+ thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d", trip_index);
thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", tz->notify_event);
thermal_prop[4] = NULL;
kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
Index: linux-pm/drivers/thermal/gov_bang_bang.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_bang_bang.c
+++ linux-pm/drivers/thermal/gov_bang_bang.c
@@ -89,14 +89,14 @@ static int thermal_zone_trip_update(stru
* (trip_temp - hyst) so that the fan gets turned off again.
*
*/
-static int bang_bang_control(struct thermal_zone_device *tz, int trip)
+static int bang_bang_control(struct thermal_zone_device *tz, int trip_index)
{
struct thermal_instance *instance;
int ret;
lockdep_assert_held(&tz->lock);
- ret = thermal_zone_trip_update(tz, trip);
+ ret = thermal_zone_trip_update(tz, trip_index);
if (ret)
return ret;
Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -68,23 +68,23 @@ static unsigned long get_target_state(st
return next_target;
}
-static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_id)
+static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_index)
{
- const struct thermal_trip *trip = &tz->trips[trip_id];
+ const struct thermal_trip *trip = &tz->trips[trip_index];
enum thermal_trend trend;
struct thermal_instance *instance;
bool throttle = false;
int old_target;
- trend = get_tz_trend(tz, trip_id);
+ trend = get_tz_trend(tz, trip_index);
if (tz->temperature >= trip->temperature) {
throttle = true;
- trace_thermal_zone_trip(tz, trip_id, trip->type);
+ trace_thermal_zone_trip(tz, trip_index, trip->type);
}
dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
- trip_id, trip->type, trip->temperature, trend, throttle);
+ trip_index, trip->type, trip->temperature, trend, throttle);
list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
if (instance->trip != trip)
@@ -120,7 +120,7 @@ static void thermal_zone_trip_update(str
/**
* step_wise_throttle - throttles devices associated with the given zone
* @tz: thermal_zone_device
- * @trip: trip point index
+ * @trip_index: trip point index
*
* Throttling Logic: This uses the trend of the thermal zone to throttle.
* If the thermal zone is 'heating up' this throttles all the cooling
@@ -128,13 +128,13 @@ static void thermal_zone_trip_update(str
* step. If the zone is 'cooling down' it brings back the performance of
* the devices by one step.
*/
-static int step_wise_throttle(struct thermal_zone_device *tz, int trip)
+static int step_wise_throttle(struct thermal_zone_device *tz, int trip_index)
{
struct thermal_instance *instance;
lockdep_assert_held(&tz->lock);
- thermal_zone_trip_update(tz, trip);
+ thermal_zone_trip_update(tz, trip_index);
list_for_each_entry(instance, &tz->thermal_instances, tz_node)
thermal_cdev_update(instance->cdev);
From: Rafael J. Wysocki <[email protected]>
Fold update_passive_instance() into thermal_zone_trip_update() that is
its only caller so as to make the code in question easeir to follow.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/gov_step_wise.c | 28 ++++++++++------------------
1 file changed, 10 insertions(+), 18 deletions(-)
Index: linux-pm/drivers/thermal/gov_step_wise.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_step_wise.c
+++ linux-pm/drivers/thermal/gov_step_wise.c
@@ -68,17 +68,6 @@ static unsigned long get_target_state(st
return next_target;
}
-static void update_passive_instance(struct thermal_zone_device *tz,
- enum thermal_trip_type type, int value)
-{
- /*
- * If value is +1, activate a passive instance.
- * If value is -1, deactivate a passive instance.
- */
- if (type == THERMAL_TRIP_PASSIVE)
- tz->passive += value;
-}
-
static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_id)
{
const struct thermal_trip *trip = &tz->trips[trip_id];
@@ -109,14 +98,17 @@ static void thermal_zone_trip_update(str
if (instance->initialized && old_target == instance->target)
continue;
- /* Activate a passive thermal instance */
if (old_target == THERMAL_NO_TARGET &&
- instance->target != THERMAL_NO_TARGET)
- update_passive_instance(tz, trip->type, 1);
- /* Deactivate a passive thermal instance */
- else if (old_target != THERMAL_NO_TARGET &&
- instance->target == THERMAL_NO_TARGET)
- update_passive_instance(tz, trip->type, -1);
+ instance->target != THERMAL_NO_TARGET) {
+ /* Activate a passive thermal instance */
+ if (trip->type == THERMAL_TRIP_PASSIVE)
+ tz->passive++;
+ } else if (old_target != THERMAL_NO_TARGET &&
+ instance->target == THERMAL_NO_TARGET) {
+ /* Deactivate a passive thermal instance */
+ if (trip->type == THERMAL_TRIP_PASSIVE)
+ tz->passive--;
+ }
instance->initialized = true;
mutex_lock(&instance->cdev->lock);
From: Rafael J. Wysocki <[email protected]>
The device lists present in struct acpi_thermal_passive and struct
acpi_thermal_active can be located in struct acpi_thermal_trip which
then will allow the same code to be used for handling both the passive
and active trip points, so make that change.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/thermal.c | 36 +++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 15 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -81,11 +81,11 @@ static struct workqueue_struct *acpi_the
struct acpi_thermal_trip {
unsigned long temperature;
+ struct acpi_handle_list devices;
};
struct acpi_thermal_passive {
struct acpi_thermal_trip trip;
- struct acpi_handle_list devices;
unsigned long tc1;
unsigned long tc2;
unsigned long tsp;
@@ -93,7 +93,6 @@ struct acpi_thermal_passive {
struct acpi_thermal_active {
struct acpi_thermal_trip trip;
- struct acpi_handle_list devices;
};
struct acpi_thermal_trips {
@@ -205,6 +204,7 @@ static void acpi_thermal_update_passive_
static bool update_passive_devices(struct acpi_thermal *tz, bool compare)
{
+ struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
struct acpi_handle_list devices;
acpi_status status;
@@ -217,10 +217,10 @@ static bool update_passive_devices(struc
return false;
}
- if (compare && memcmp(&tz->trips.passive.devices, &devices, sizeof(devices)))
+ if (compare && memcmp(&acpi_trip->devices, &devices, sizeof(devices)))
ACPI_THERMAL_TRIPS_EXCEPTION(tz, "device");
- memcpy(&tz->trips.passive.devices, &devices, sizeof(devices));
+ memcpy(&acpi_trip->devices, &devices, sizeof(devices));
return true;
}
@@ -276,6 +276,7 @@ static void acpi_thermal_update_active_t
static bool update_active_devices(struct acpi_thermal *tz, int index, bool compare)
{
char method[] = { '_', 'A', 'L', '0' + index, '\0' };
+ struct acpi_thermal_trip *acpi_trip = &tz->trips.active[index].trip;
struct acpi_handle_list devices;
acpi_status status;
@@ -289,10 +290,10 @@ static bool update_active_devices(struct
return false;
}
- if (compare && memcmp(&tz->trips.active[index].devices, &devices, sizeof(devices)))
+ if (compare && memcmp(&acpi_trip->devices, &devices, sizeof(devices)))
ACPI_THERMAL_TRIPS_EXCEPTION(tz, "device");
- memcpy(&tz->trips.active[index].devices, &devices, sizeof(devices));
+ memcpy(&acpi_trip->devices, &devices, sizeof(devices));
return true;
}
@@ -602,6 +603,7 @@ static int acpi_thermal_cooling_device_c
{
struct acpi_device *device = cdev->devdata;
struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
+ struct acpi_thermal_trip *acpi_trip;
struct acpi_device *dev;
acpi_handle handle;
int i;
@@ -615,10 +617,11 @@ static int acpi_thermal_cooling_device_c
if (tz->trips.hot_valid)
trip++;
- if (acpi_thermal_trip_valid(&tz->trips.passive.trip)) {
+ acpi_trip = &tz->trips.passive.trip;
+ if (acpi_thermal_trip_valid(acpi_trip)) {
trip++;
- for (i = 0; i < tz->trips.passive.devices.count; i++) {
- handle = tz->trips.passive.devices.handles[i];
+ for (i = 0; i < acpi_trip->devices.count; i++) {
+ handle = acpi_trip->devices.handles[i];
dev = acpi_fetch_acpi_dev(handle);
if (dev != device)
continue;
@@ -640,12 +643,13 @@ static int acpi_thermal_cooling_device_c
}
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
- if (!acpi_thermal_trip_valid(&tz->trips.active[i].trip))
+ acpi_trip = &tz->trips.active[i].trip;
+ if (!acpi_thermal_trip_valid(acpi_trip))
break;
trip++;
- for (j = 0; j < tz->trips.active[i].devices.count; j++) {
- handle = tz->trips.active[i].devices.handles[j];
+ for (j = 0; j < acpi_trip->devices.count; j++) {
+ handle = acpi_trip->devices.handles[j];
dev = acpi_fetch_acpi_dev(handle);
if (dev != device)
continue;
@@ -1035,11 +1039,13 @@ static int acpi_thermal_resume(struct de
return -EINVAL;
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
- if (!acpi_thermal_trip_valid(&tz->trips.active[i].trip))
+ struct acpi_thermal_trip *acpi_trip = &tz->trips.active[i].trip;
+
+ if (!acpi_thermal_trip_valid(acpi_trip))
break;
- for (j = 0; j < tz->trips.active[i].devices.count; j++) {
- acpi_bus_update_power(tz->trips.active[i].devices.handles[j],
+ for (j = 0; j < acpi_trip->devices.count; j++) {
+ acpi_bus_update_power(acpi_trip->devices.handles[j],
&power_state);
}
}
From: Rafael J. Wysocki <[email protected]>
In order to reduce code duplicationeve further, merge
acpi_thermal_update_passive/active_devices() into one function
called acpi_thermal_update_trip_devices() that will be used for
updating both the passive and active trip points.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/thermal.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -233,14 +233,16 @@ static bool update_trip_devices(struct a
return true;
}
-static void acpi_thermal_update_passive_devices(struct acpi_thermal *tz)
+static void acpi_thermal_update_trip_devices(struct acpi_thermal *tz, int index)
{
- struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
+ struct acpi_thermal_trip *acpi_trip;
+ acpi_trip = index == ACPI_THERMAL_TRIP_PASSIVE ?
+ &tz->trips.passive.trip : &tz->trips.active[index].trip;
if (!acpi_thermal_trip_valid(acpi_trip))
return;
- if (update_trip_devices(tz, acpi_trip, ACPI_THERMAL_TRIP_PASSIVE, true)) {
+ if (update_trip_devices(tz, acpi_trip, index, true)) {
return;
}
@@ -283,20 +285,6 @@ static void acpi_thermal_update_active_t
ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
}
-static void acpi_thermal_update_active_devices(struct acpi_thermal *tz, int index)
-{
- struct acpi_thermal_trip *acpi_trip = &tz->trips.active[index].trip;
-
- if (!acpi_thermal_trip_valid(acpi_trip))
- return;
-
- if (update_trip_devices(tz, acpi_trip, index, true))
- return;
-
- acpi_trip->temperature = THERMAL_TEMP_INVALID;
- ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
-}
-
static int acpi_thermal_adjust_trip(struct thermal_trip *trip, void *data)
{
struct acpi_thermal_trip *acpi_trip = trip->priv;
@@ -324,9 +312,9 @@ static void acpi_thermal_adjust_thermal_
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++)
acpi_thermal_update_active_trip(tz, i);
} else {
- acpi_thermal_update_passive_devices(tz);
+ acpi_thermal_update_trip_devices(tz, ACPI_THERMAL_TRIP_PASSIVE);
for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++)
- acpi_thermal_update_active_devices(tz, i);
+ acpi_thermal_update_trip_devices(tz, i);
}
for_each_thermal_trip(tz->thermal_zone, acpi_thermal_adjust_trip, tz);
From: Rafael J. Wysocki <[email protected]>
Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
overhead (due to pointless bounds checking and copying of trip point
data) from the power allocator thermal governor and generally make it
use trip pointers instead of trip indices where applicable.
The general functionality is not expected to be changed.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/gov_power_allocator.c | 102 ++++++++++++----------------------
1 file changed, 38 insertions(+), 64 deletions(-)
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
@@ -16,8 +16,6 @@
#include "thermal_core.h"
-#define INVALID_TRIP -1
-
#define FRAC_BITS 10
#define int_to_frac(x) ((x) << FRAC_BITS)
#define frac_to_int(x) ((x) >> FRAC_BITS)
@@ -55,23 +53,23 @@ static inline s64 div_frac(s64 x, s64 y)
* @err_integral: accumulated error in the PID controller.
* @prev_err: error in the previous iteration of the PID controller.
* Used to calculate the derivative term.
+ * @sustainable_power: Sustainable power (heat) that this thermal zone can
+ * dissipate
* @trip_switch_on: first passive trip point of the thermal zone. The
* governor switches on when this trip point is crossed.
* If the thermal zone only has one passive trip point,
- * @trip_switch_on should be INVALID_TRIP.
+ * @trip_switch_on should be NULL.
* @trip_max_desired_temperature: last passive trip point of the thermal
* zone. The temperature we are
* controlling for.
- * @sustainable_power: Sustainable power (heat) that this thermal zone can
- * dissipate
*/
struct power_allocator_params {
bool allocated_tzp;
s64 err_integral;
s32 prev_err;
- int trip_switch_on;
- int trip_max_desired_temperature;
u32 sustainable_power;
+ const struct thermal_trip *trip_switch_on;
+ const struct thermal_trip *trip_max_desired_temperature;
};
/**
@@ -90,14 +88,12 @@ static u32 estimate_sustainable_power(st
u32 sustainable_power = 0;
struct thermal_instance *instance;
struct power_allocator_params *params = tz->governor_data;
- const struct thermal_trip *trip_max_desired_temperature =
- &tz->trips[params->trip_max_desired_temperature];
list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
struct thermal_cooling_device *cdev = instance->cdev;
u32 min_power;
- if (instance->trip != trip_max_desired_temperature)
+ if (instance->trip != params->trip_max_desired_temperature)
continue;
if (!cdev_is_power_actor(cdev))
@@ -116,24 +112,23 @@ static u32 estimate_sustainable_power(st
* estimate_pid_constants() - Estimate the constants for the PID controller
* @tz: thermal zone for which to estimate the constants
* @sustainable_power: sustainable power for the thermal zone
- * @trip_switch_on: trip point number for the switch on temperature
+ * @trip_switch_on: trip point for the switch on temperature
* @control_temp: target temperature for the power allocator governor
*
* This function is used to update the estimation of the PID
* controller constants in struct thermal_zone_parameters.
*/
static void estimate_pid_constants(struct thermal_zone_device *tz,
- u32 sustainable_power, int trip_switch_on,
+ u32 sustainable_power,
+ const struct thermal_trip *trip_switch_on,
int control_temp)
{
- struct thermal_trip trip;
u32 temperature_threshold = control_temp;
int ret;
s32 k_i;
- ret = __thermal_zone_get_trip(tz, trip_switch_on, &trip);
- if (!ret)
- temperature_threshold -= trip.temperature;
+ if (trip_switch_on)
+ temperature_threshold -= trip_switch_on->temperature;
/*
* estimate_pid_constants() tries to find appropriate default
@@ -386,7 +381,7 @@ static int allocate_power(struct thermal
struct thermal_instance *instance;
struct power_allocator_params *params = tz->governor_data;
const struct thermal_trip *trip_max_desired_temperature =
- &tz->trips[params->trip_max_desired_temperature];
+ params->trip_max_desired_temperature;
u32 *req_power, *max_power, *granted_power, *extra_actor_power;
u32 *weighted_req_power;
u32 total_req_power, max_allocatable_power, total_weighted_req_power;
@@ -513,46 +508,35 @@ static int allocate_power(struct thermal
static void get_governor_trips(struct thermal_zone_device *tz,
struct power_allocator_params *params)
{
- int i, last_active, last_passive;
- bool found_first_passive;
-
- found_first_passive = false;
- last_active = INVALID_TRIP;
- last_passive = INVALID_TRIP;
+ const struct thermal_trip *last_active = NULL:
+ const struct thermal_trip *last_passive = NULL;
+ bool found_first_passive = false;
+ int i;
for (i = 0; i < tz->num_trips; i++) {
- struct thermal_trip trip;
- int ret;
+ const struct thermal_trip *trip = &tz->trips[i];
- ret = __thermal_zone_get_trip(tz, i, &trip);
- if (ret) {
- dev_warn(&tz->device,
- "Failed to get trip point %d type: %d\n", i,
- ret);
- continue;
- }
-
- if (trip.type == THERMAL_TRIP_PASSIVE) {
+ if (trip->type == THERMAL_TRIP_PASSIVE) {
if (!found_first_passive) {
- params->trip_switch_on = i;
+ params->trip_switch_on = trip;
found_first_passive = true;
} else {
- last_passive = i;
+ last_passive = trip;
}
- } else if (trip.type == THERMAL_TRIP_ACTIVE) {
- last_active = i;
+ } else if (trip->type == THERMAL_TRIP_ACTIVE) {
+ last_active = trip;
} else {
break;
}
}
- if (last_passive != INVALID_TRIP) {
+ if (last_passive) {
params->trip_max_desired_temperature = last_passive;
} else if (found_first_passive) {
params->trip_max_desired_temperature = params->trip_switch_on;
- params->trip_switch_on = INVALID_TRIP;
+ params->trip_switch_on = NULL;
} else {
- params->trip_switch_on = INVALID_TRIP;
+ params->trip_switch_on = NULL;
params->trip_max_desired_temperature = last_active;
}
}
@@ -567,14 +551,12 @@ static void allow_maximum_power(struct t
{
struct thermal_instance *instance;
struct power_allocator_params *params = tz->governor_data;
- const struct thermal_trip *trip_max_desired_temperature =
- &tz->trips[params->trip_max_desired_temperature];
u32 req_power;
list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
struct thermal_cooling_device *cdev = instance->cdev;
- if ((instance->trip != trip_max_desired_temperature) ||
+ if (instance->trip != params->trip_max_desired_temperature ||
(!cdev_is_power_actor(instance->cdev)))
continue;
@@ -636,7 +618,6 @@ static int power_allocator_bind(struct t
{
int ret;
struct power_allocator_params *params;
- struct thermal_trip trip;
ret = check_power_actors(tz);
if (ret)
@@ -662,12 +643,13 @@ static int power_allocator_bind(struct t
get_governor_trips(tz, params);
if (tz->num_trips > 0) {
- ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature,
- &trip);
- if (!ret)
+ const struct thermal_trip *trip;
+
+ trip = params->trip_max_desired_temperature;
+ if (trip)
estimate_pid_constants(tz, tz->tzp->sustainable_power,
params->trip_switch_on,
- trip.temperature);
+ trip->temperature);
}
reset_pid_controller(params);
@@ -697,11 +679,10 @@ static void power_allocator_unbind(struc
tz->governor_data = NULL;
}
-static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
+static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_index)
{
struct power_allocator_params *params = tz->governor_data;
- struct thermal_trip trip;
- int ret;
+ const struct thermal_trip *trip = &tz->trips[trip_index];
bool update;
lockdep_assert_held(&tz->lock);
@@ -710,12 +691,12 @@ static int power_allocator_throttle(stru
* We get called for every trip point but we only need to do
* our calculations once
*/
- if (trip_id != params->trip_max_desired_temperature)
+ if (trip != params->trip_max_desired_temperature)
return 0;
- ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
- if (!ret && (tz->temperature < trip.temperature)) {
- update = (tz->last_temperature >= trip.temperature);
+ trip = params->trip_switch_on;
+ if (trip && tz->temperature < trip->temperature) {
+ update = tz->last_temperature >= trip->temperature;
tz->passive = 0;
reset_pid_controller(params);
allow_maximum_power(tz, update);
@@ -724,14 +705,7 @@ static int power_allocator_throttle(stru
tz->passive = 1;
- ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature, &trip);
- if (ret) {
- dev_warn(&tz->device, "Failed to get the maximum desired temperature: %d\n",
- ret);
- return ret;
- }
-
- return allocate_power(tz, trip.temperature);
+ return allocate_power(tz, params->trip_max_desired_temperature->temperature);
}
static struct thermal_governor thermal_gov_power_allocator = {
On Tue, Sep 26, 2023 at 7:18 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 21/09/2023 19:49, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > In order to reduce code duplication, merge update_passive_devices() and
> > update_active_devices() into one function called update_trip_devices()
> > that will be used for updating both the passive and active trip points.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/acpi/thermal.c | 53 ++++++++++++++++++-------------------------------
> > 1 file changed, 20 insertions(+), 33 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/thermal.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/thermal.c
> > +++ linux-pm/drivers/acpi/thermal.c
> > @@ -43,6 +43,8 @@
> > #define ACPI_THERMAL_MAX_ACTIVE 10
> > #define ACPI_THERMAL_MAX_LIMIT_STR_LEN 65
> >
> > +#define ACPI_THERMAL_TRIP_PASSIVE (-1)
> > +
> > /*
> > * This exception is thrown out in two cases:
> > * 1.An invalid trip point becomes invalid or a valid trip point becomes invalid
> > @@ -202,18 +204,25 @@ static void acpi_thermal_update_passive_
> > ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
> > }
> >
> > -static bool update_passive_devices(struct acpi_thermal *tz, bool compare)
> > +static bool update_trip_devices(struct acpi_thermal *tz,
> > + struct acpi_thermal_trip *acpi_trip,
> > + int index, bool compare)
> > {
> > - struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
> > struct acpi_handle_list devices;
> > + char method[] = "_PSL";
> > acpi_status status;
> >
> > + if (index != ACPI_THERMAL_TRIP_PASSIVE) {
> > + method[1] = 'A';
> > + method[2] = 'L';
> > + method[3] = '0' + index;
> > + }
>
> Could be index > 9 ?
I can add a check, but it will never be called with index > 9 anyway.
On Tue, Sep 26, 2023 at 7:56 PM Rafael J. Wysocki <[email protected]> wrote:
>
> On Tue, Sep 26, 2023 at 7:18 PM Daniel Lezcano
> <[email protected]> wrote:
> >
> > On 21/09/2023 19:49, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > In order to reduce code duplication, merge update_passive_devices() and
> > > update_active_devices() into one function called update_trip_devices()
> > > that will be used for updating both the passive and active trip points.
> > >
> > > No intentional functional impact.
> > >
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > > drivers/acpi/thermal.c | 53 ++++++++++++++++++-------------------------------
> > > 1 file changed, 20 insertions(+), 33 deletions(-)
> > >
> > > Index: linux-pm/drivers/acpi/thermal.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/thermal.c
> > > +++ linux-pm/drivers/acpi/thermal.c
> > > @@ -43,6 +43,8 @@
> > > #define ACPI_THERMAL_MAX_ACTIVE 10
> > > #define ACPI_THERMAL_MAX_LIMIT_STR_LEN 65
> > >
> > > +#define ACPI_THERMAL_TRIP_PASSIVE (-1)
> > > +
> > > /*
> > > * This exception is thrown out in two cases:
> > > * 1.An invalid trip point becomes invalid or a valid trip point becomes invalid
> > > @@ -202,18 +204,25 @@ static void acpi_thermal_update_passive_
> > > ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
> > > }
> > >
> > > -static bool update_passive_devices(struct acpi_thermal *tz, bool compare)
> > > +static bool update_trip_devices(struct acpi_thermal *tz,
> > > + struct acpi_thermal_trip *acpi_trip,
> > > + int index, bool compare)
> > > {
> > > - struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
> > > struct acpi_handle_list devices;
> > > + char method[] = "_PSL";
> > > acpi_status status;
> > >
> > > + if (index != ACPI_THERMAL_TRIP_PASSIVE) {
> > > + method[1] = 'A';
> > > + method[2] = 'L';
> > > + method[3] = '0' + index;
> > > + }
> >
> > Could be index > 9 ?
>
> I can add a check, but it will never be called with index > 9 anyway.
To be more precise, update_trip_devices() is called in two places,
acpi_thermal_init_trip() and acpi_thermal_update_trip_devices().
Both of these are called either with ACPI_THERMAL_TRIP_PASSIVE passed
as index, or from a loop over indices between 0 and
ACPI_THERMAL_MAX_ACTIVE-1 inclusive.
On 21/09/2023 19:49, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> In order to reduce code duplication, merge update_passive_devices() and
> update_active_devices() into one function called update_trip_devices()
> that will be used for updating both the passive and active trip points.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/thermal.c | 53 ++++++++++++++++++-------------------------------
> 1 file changed, 20 insertions(+), 33 deletions(-)
>
> Index: linux-pm/drivers/acpi/thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/thermal.c
> +++ linux-pm/drivers/acpi/thermal.c
> @@ -43,6 +43,8 @@
> #define ACPI_THERMAL_MAX_ACTIVE 10
> #define ACPI_THERMAL_MAX_LIMIT_STR_LEN 65
>
> +#define ACPI_THERMAL_TRIP_PASSIVE (-1)
> +
> /*
> * This exception is thrown out in two cases:
> * 1.An invalid trip point becomes invalid or a valid trip point becomes invalid
> @@ -202,18 +204,25 @@ static void acpi_thermal_update_passive_
> ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
> }
>
> -static bool update_passive_devices(struct acpi_thermal *tz, bool compare)
> +static bool update_trip_devices(struct acpi_thermal *tz,
> + struct acpi_thermal_trip *acpi_trip,
> + int index, bool compare)
> {
> - struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
> struct acpi_handle_list devices;
> + char method[] = "_PSL";
> acpi_status status;
>
> + if (index != ACPI_THERMAL_TRIP_PASSIVE) {
> + method[1] = 'A';
> + method[2] = 'L';
> + method[3] = '0' + index;
> + }
Could be index > 9 ?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 21/09/2023 19:48, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> The device lists present in struct acpi_thermal_passive and struct
> acpi_thermal_active can be located in struct acpi_thermal_trip which
> then will allow the same code to be used for handling both the passive
> and active trip points, so make that change.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 21/09/2023 19:49, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> In order to reduce code duplication, merge update_passive_devices() and
> update_active_devices() into one function called update_trip_devices()
> that will be used for updating both the passive and active trip points.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
Reviewed-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 26/09/2023 20:04, Rafael J. Wysocki wrote:
> On Tue, Sep 26, 2023 at 7:56 PM Rafael J. Wysocki <[email protected]> wrote:
>>
>> On Tue, Sep 26, 2023 at 7:18 PM Daniel Lezcano
>> <[email protected]> wrote:
>>>
>>> On 21/09/2023 19:49, Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki <[email protected]>
>>>>
>>>> In order to reduce code duplication, merge update_passive_devices() and
>>>> update_active_devices() into one function called update_trip_devices()
>>>> that will be used for updating both the passive and active trip points.
>>>>
>>>> No intentional functional impact.
>>>>
>>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>>> ---
>>>> drivers/acpi/thermal.c | 53 ++++++++++++++++++-------------------------------
[ ... ]
>>>> + if (index != ACPI_THERMAL_TRIP_PASSIVE) {
>>>> + method[1] = 'A';
>>>> + method[2] = 'L';
>>>> + method[3] = '0' + index;
>>>> + }
>>>
>>> Could be index > 9 ?
>>
>> I can add a check, but it will never be called with index > 9 anyway.
>
> To be more precise, update_trip_devices() is called in two places,
> acpi_thermal_init_trip() and acpi_thermal_update_trip_devices().
>
> Both of these are called either with ACPI_THERMAL_TRIP_PASSIVE passed
> as index, or from a loop over indices between 0 and
> ACPI_THERMAL_MAX_ACTIVE-1 inclusive.
Ok, thanks for clarifying
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 21/09/2023 19:51, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> In order to reduce code duplicationeve further, merge
> acpi_thermal_init_passive/active_trip() into one function called
> acpi_thermal_init_trip() that will be used for initializing both
> the passive and active trip points.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
Reviewed-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Wed, Sep 27, 2023 at 5:00 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 21/09/2023 19:54, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Make get_trip_level() access the thermal zone's trip table directly
> > instead of using __thermal_zone_get_trip() which adds overhead related
> > to the unnecessary bounds checking and copying the trip point data.
> >
> > Also rearrange the code in it to make it somewhat easier to follow.
> >
> > The general functionality is not expected to be changed.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/thermal/gov_fair_share.c | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > 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
> > @@ -21,23 +21,21 @@
> > */
> > static int get_trip_level(struct thermal_zone_device *tz)
> > {
> > - struct thermal_trip trip;
> > - int count;
> > + const struct thermal_trip *trip = tz->trips;
> > + int i;
> >
> > - for (count = 0; count < tz->num_trips; count++) {
> > - __thermal_zone_get_trip(tz, count, &trip);
> > - if (tz->temperature < trip.temperature)
> > + if (tz->temperature < trip->temperature)
> > + return 0;
> > +
> > + for (i = 0; i < tz->num_trips - 1; i++) {
> > + trip++;
> > + if (tz->temperature < trip->temperature)
> > break;
> > }
>
> Is it possible to use for_each_thermal_trip() instead ? That would make
> the code more self-encapsulate
It is possible in principle, but this is a governor which is regarded
as part of the core, isn't it?
So is an extra overhead related to using a callback (which may be
subject to retpolines and such) really justified in this case?
>
> > - /*
> > - * count > 0 only if temperature is greater than first trip
> > - * point, in which case, trip_point = count - 1
> > - */
> > - if (count > 0)
> > - trace_thermal_zone_trip(tz, count - 1, trip.type);
> > + trace_thermal_zone_trip(tz, i, tz->trips[i].type);
> >
> > - return count;
> > + return i;
> > }
> >
> > static long get_target_state(struct thermal_zone_device *tz,
> >
> >
> >
>
> --
On Wed, Sep 27, 2023 at 5:37 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 27/09/2023 17:06, Rafael J. Wysocki wrote:
> > On Wed, Sep 27, 2023 at 5:00 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >> On 21/09/2023 19:54, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <[email protected]>
> >>>
> >>> Make get_trip_level() access the thermal zone's trip table directly
> >>> instead of using __thermal_zone_get_trip() which adds overhead related
> >>> to the unnecessary bounds checking and copying the trip point data.
> >>>
> >>> Also rearrange the code in it to make it somewhat easier to follow.
> >>>
> >>> The general functionality is not expected to be changed.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <[email protected]>
> >>> ---
> >>> drivers/thermal/gov_fair_share.c | 22 ++++++++++------------
> >>> 1 file changed, 10 insertions(+), 12 deletions(-)
> >>>
> >>> 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
> >>> @@ -21,23 +21,21 @@
> >>> */
> >>> static int get_trip_level(struct thermal_zone_device *tz)
> >>> {
> >>> - struct thermal_trip trip;
> >>> - int count;
> >>> + const struct thermal_trip *trip = tz->trips;
> >>> + int i;
> >>>
> >>> - for (count = 0; count < tz->num_trips; count++) {
> >>> - __thermal_zone_get_trip(tz, count, &trip);
> >>> - if (tz->temperature < trip.temperature)
> >>> + if (tz->temperature < trip->temperature)
> >>> + return 0;
> >>> +
> >>> + for (i = 0; i < tz->num_trips - 1; i++) {
> >>> + trip++;
> >>> + if (tz->temperature < trip->temperature)
> >>> break;
> >>> }
> >>
> >> Is it possible to use for_each_thermal_trip() instead ? That would make
> >> the code more self-encapsulate
> >
> > It is possible in principle, but this is a governor which is regarded
> > as part of the core, isn't it?
> >
> > So is an extra overhead related to using a callback (which may be
> > subject to retpolines and such) really justified in this case?
>
> From my POV, all trip points browsing should be replaced by
> for_each_thermal_trip() so any change in the future in how we go through
> the existing thermal trips will impact one place.
>
> If the routine needs to be optimized, that is something we can do also
> (may be an inline the callback?)
OK
On Wed, Sep 27, 2023 at 5:10 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 21/09/2023 19:55, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
> > overhead (due to pointless bounds checking and copying of trip point
> > data) from the power allocator thermal governor and generally make it
> > use trip pointers instead of trip indices where applicable.
>
> Actually the __thermal_zone_get_trip() change was done on purpose to
> replace the 'throttle' callback index parameter by the trip pointer and
> removing those call to __thermal_zone_get_trip() while the code was
> using the trip pointer.
>
> IMO, the changes should focus on changing the trip_index parameter by
> the trip pointer directly in the throttle ops.
So you would like .throttle() to take a trip pointer argument instead
of an index?
The difficulty here is that the user space governor needs to expose
the index to user space anyway, so it would need to find it if it gets
a trip pointer instead.
Not a big deal I suppose, but a bit of extra overhead.
Also it is easier to switch the governors over to using trip pointers
internally and then change the .throttle() argument on top of that.
> The pointer can be
> retrieved in the handle_thermal_trip() function and passed around for
> the rest of the actions on this trip point
Right, except for the user space governor which needs a trip index.
And the indices are used for tracing too.
On Wed, Sep 27, 2023 at 5:46 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 27/09/2023 17:27, Rafael J. Wysocki wrote:
> > On Wed, Sep 27, 2023 at 5:10 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >> On 21/09/2023 19:55, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <[email protected]>
> >>>
> >>> Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
> >>> overhead (due to pointless bounds checking and copying of trip point
> >>> data) from the power allocator thermal governor and generally make it
> >>> use trip pointers instead of trip indices where applicable.
> >>
> >> Actually the __thermal_zone_get_trip() change was done on purpose to
> >> replace the 'throttle' callback index parameter by the trip pointer and
> >> removing those call to __thermal_zone_get_trip() while the code was
> >> using the trip pointer.
> >>
> >> IMO, the changes should focus on changing the trip_index parameter by
> >> the trip pointer directly in the throttle ops.
> >
> > So you would like .throttle() to take a trip pointer argument instead
> > of an index?
> >
> > The difficulty here is that the user space governor needs to expose
> > the index to user space anyway, so it would need to find it if it gets
> > a trip pointer instead.
> >
> > Not a big deal I suppose, but a bit of extra overhead.
> >
> > Also it is easier to switch the governors over to using trip pointers
> > internally and then change the .throttle() argument on top of that.
> >
> >> The pointer can be
> >> retrieved in the handle_thermal_trip() function and passed around for
> >> the rest of the actions on this trip point
> >
> > Right, except for the user space governor which needs a trip index.
> > And the indices are used for tracing too.
>
> Given the userspace governor is going obsolete and the notifications are
> for the userspace, which is slow, we can retrieve the index from the
> throttling ops
OK
Given that patches [01-05/13] are not controversial, I'll respon the
governor patches into a separate series on top of them.
I would much appreciate it if you could take a look at patch [10/13]
and the remaining ACPi thermal patches in this series [11-13/13].
They don't depend on the governor changes.
On 21/09/2023 19:52, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Replace the integer trip number stored in struct thermal_instance with
> a pointer to the relevant trip and adjust the code using the structure
> in question accordingly.
>
> The main reason for making this change is to allow the trip point to
> cooling device binding code more straightforward, as illustrated by
> subsequent modifications of the ACPI thermal driver, but it also helps
> to clarify the overall design and allows the governor code overhead to
> be reduced (through subsequent modifications).
>
> The only case in which it adds complexity is trip_point_show() that
> needs to walk the trips[] table to find the index of the given trip
> point, but this is not a critical path and the interface that
> trip_point_show() belongs to is problematic anyway (for instance, it
> doesn't cover the case when the same cooling devices is associated
> with multiple trip points).
>
> This is a preliminary change and the affected code will be refined by
> a series of subsequent modifications of thermal governors, the core and
> the ACPI thermal driver.
>
> The general functionality is not expected to be affected by this change.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
Reviewed-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 21/09/2023 19:50, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> In order to reduce code duplicationeve further, merge
> acpi_thermal_update_passive/active_devices() into one function
> called acpi_thermal_update_trip_devices() that will be used for
> updating both the passive and active trip points.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
Reviewed-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 21/09/2023 19:55, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
> overhead (due to pointless bounds checking and copying of trip point
> data) from the power allocator thermal governor and generally make it
> use trip pointers instead of trip indices where applicable.
Actually the __thermal_zone_get_trip() change was done on purpose to
replace the 'throttle' callback index parameter by the trip pointer and
removing those call to __thermal_zone_get_trip() while the code was
using the trip pointer.
IMO, the changes should focus on changing the trip_index parameter by
the trip pointer directly in the throttle ops. The pointer can be
retrieved in the handle_thermal_trip() function and passed around for
the rest of the actions on this trip point
> The general functionality is not expected to be changed.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/thermal/gov_power_allocator.c | 102 ++++++++++++----------------------
> 1 file changed, 38 insertions(+), 64 deletions(-)
>
> 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
> @@ -16,8 +16,6 @@
>
> #include "thermal_core.h"
>
> -#define INVALID_TRIP -1
> -
> #define FRAC_BITS 10
> #define int_to_frac(x) ((x) << FRAC_BITS)
> #define frac_to_int(x) ((x) >> FRAC_BITS)
> @@ -55,23 +53,23 @@ static inline s64 div_frac(s64 x, s64 y)
> * @err_integral: accumulated error in the PID controller.
> * @prev_err: error in the previous iteration of the PID controller.
> * Used to calculate the derivative term.
> + * @sustainable_power: Sustainable power (heat) that this thermal zone can
> + * dissipate
> * @trip_switch_on: first passive trip point of the thermal zone. The
> * governor switches on when this trip point is crossed.
> * If the thermal zone only has one passive trip point,
> - * @trip_switch_on should be INVALID_TRIP.
> + * @trip_switch_on should be NULL.
> * @trip_max_desired_temperature: last passive trip point of the thermal
> * zone. The temperature we are
> * controlling for.
> - * @sustainable_power: Sustainable power (heat) that this thermal zone can
> - * dissipate
> */
> struct power_allocator_params {
> bool allocated_tzp;
> s64 err_integral;
> s32 prev_err;
> - int trip_switch_on;
> - int trip_max_desired_temperature;
> u32 sustainable_power;
> + const struct thermal_trip *trip_switch_on;
> + const struct thermal_trip *trip_max_desired_temperature;
> };
>
> /**
> @@ -90,14 +88,12 @@ static u32 estimate_sustainable_power(st
> u32 sustainable_power = 0;
> struct thermal_instance *instance;
> struct power_allocator_params *params = tz->governor_data;
> - const struct thermal_trip *trip_max_desired_temperature =
> - &tz->trips[params->trip_max_desired_temperature];
>
> list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> struct thermal_cooling_device *cdev = instance->cdev;
> u32 min_power;
>
> - if (instance->trip != trip_max_desired_temperature)
> + if (instance->trip != params->trip_max_desired_temperature)
> continue;
>
> if (!cdev_is_power_actor(cdev))
> @@ -116,24 +112,23 @@ static u32 estimate_sustainable_power(st
> * estimate_pid_constants() - Estimate the constants for the PID controller
> * @tz: thermal zone for which to estimate the constants
> * @sustainable_power: sustainable power for the thermal zone
> - * @trip_switch_on: trip point number for the switch on temperature
> + * @trip_switch_on: trip point for the switch on temperature
> * @control_temp: target temperature for the power allocator governor
> *
> * This function is used to update the estimation of the PID
> * controller constants in struct thermal_zone_parameters.
> */
> static void estimate_pid_constants(struct thermal_zone_device *tz,
> - u32 sustainable_power, int trip_switch_on,
> + u32 sustainable_power,
> + const struct thermal_trip *trip_switch_on,
> int control_temp)
> {
> - struct thermal_trip trip;
> u32 temperature_threshold = control_temp;
> int ret;
> s32 k_i;
>
> - ret = __thermal_zone_get_trip(tz, trip_switch_on, &trip);
> - if (!ret)
> - temperature_threshold -= trip.temperature;
> + if (trip_switch_on)
> + temperature_threshold -= trip_switch_on->temperature;
>
> /*
> * estimate_pid_constants() tries to find appropriate default
> @@ -386,7 +381,7 @@ static int allocate_power(struct thermal
> struct thermal_instance *instance;
> struct power_allocator_params *params = tz->governor_data;
> const struct thermal_trip *trip_max_desired_temperature =
> - &tz->trips[params->trip_max_desired_temperature];
> + params->trip_max_desired_temperature;
> u32 *req_power, *max_power, *granted_power, *extra_actor_power;
> u32 *weighted_req_power;
> u32 total_req_power, max_allocatable_power, total_weighted_req_power;
> @@ -513,46 +508,35 @@ static int allocate_power(struct thermal
> static void get_governor_trips(struct thermal_zone_device *tz,
> struct power_allocator_params *params)
> {
> - int i, last_active, last_passive;
> - bool found_first_passive;
> -
> - found_first_passive = false;
> - last_active = INVALID_TRIP;
> - last_passive = INVALID_TRIP;
> + const struct thermal_trip *last_active = NULL:
> + const struct thermal_trip *last_passive = NULL;
> + bool found_first_passive = false;
> + int i;
>
> for (i = 0; i < tz->num_trips; i++) {
> - struct thermal_trip trip;
> - int ret;
> + const struct thermal_trip *trip = &tz->trips[i];
>
> - ret = __thermal_zone_get_trip(tz, i, &trip);
> - if (ret) {
> - dev_warn(&tz->device,
> - "Failed to get trip point %d type: %d\n", i,
> - ret);
> - continue;
> - }
> -
> - if (trip.type == THERMAL_TRIP_PASSIVE) {
> + if (trip->type == THERMAL_TRIP_PASSIVE) {
> if (!found_first_passive) {
> - params->trip_switch_on = i;
> + params->trip_switch_on = trip;
> found_first_passive = true;
> } else {
> - last_passive = i;
> + last_passive = trip;
> }
> - } else if (trip.type == THERMAL_TRIP_ACTIVE) {
> - last_active = i;
> + } else if (trip->type == THERMAL_TRIP_ACTIVE) {
> + last_active = trip;
> } else {
> break;
> }
> }
>
> - if (last_passive != INVALID_TRIP) {
> + if (last_passive) {
> params->trip_max_desired_temperature = last_passive;
> } else if (found_first_passive) {
> params->trip_max_desired_temperature = params->trip_switch_on;
> - params->trip_switch_on = INVALID_TRIP;
> + params->trip_switch_on = NULL;
> } else {
> - params->trip_switch_on = INVALID_TRIP;
> + params->trip_switch_on = NULL;
> params->trip_max_desired_temperature = last_active;
> }
> }
> @@ -567,14 +551,12 @@ static void allow_maximum_power(struct t
> {
> struct thermal_instance *instance;
> struct power_allocator_params *params = tz->governor_data;
> - const struct thermal_trip *trip_max_desired_temperature =
> - &tz->trips[params->trip_max_desired_temperature];
> u32 req_power;
>
> list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> struct thermal_cooling_device *cdev = instance->cdev;
>
> - if ((instance->trip != trip_max_desired_temperature) ||
> + if (instance->trip != params->trip_max_desired_temperature ||
> (!cdev_is_power_actor(instance->cdev)))
> continue;
>
> @@ -636,7 +618,6 @@ static int power_allocator_bind(struct t
> {
> int ret;
> struct power_allocator_params *params;
> - struct thermal_trip trip;
>
> ret = check_power_actors(tz);
> if (ret)
> @@ -662,12 +643,13 @@ static int power_allocator_bind(struct t
> get_governor_trips(tz, params);
>
> if (tz->num_trips > 0) {
> - ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature,
> - &trip);
> - if (!ret)
> + const struct thermal_trip *trip;
> +
> + trip = params->trip_max_desired_temperature;
> + if (trip)
> estimate_pid_constants(tz, tz->tzp->sustainable_power,
> params->trip_switch_on,
> - trip.temperature);
> + trip->temperature);
> }
>
> reset_pid_controller(params);
> @@ -697,11 +679,10 @@ static void power_allocator_unbind(struc
> tz->governor_data = NULL;
> }
>
> -static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> +static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_index)
> {
> struct power_allocator_params *params = tz->governor_data;
> - struct thermal_trip trip;
> - int ret;
> + const struct thermal_trip *trip = &tz->trips[trip_index];
> bool update;
>
> lockdep_assert_held(&tz->lock);
> @@ -710,12 +691,12 @@ static int power_allocator_throttle(stru
> * We get called for every trip point but we only need to do
> * our calculations once
> */
> - if (trip_id != params->trip_max_desired_temperature)
> + if (trip != params->trip_max_desired_temperature)
> return 0;
>
> - ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
> - if (!ret && (tz->temperature < trip.temperature)) {
> - update = (tz->last_temperature >= trip.temperature);
> + trip = params->trip_switch_on;
> + if (trip && tz->temperature < trip->temperature) {
> + update = tz->last_temperature >= trip->temperature;
> tz->passive = 0;
> reset_pid_controller(params);
> allow_maximum_power(tz, update);
> @@ -724,14 +705,7 @@ static int power_allocator_throttle(stru
>
> tz->passive = 1;
>
> - ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature, &trip);
> - if (ret) {
> - dev_warn(&tz->device, "Failed to get the maximum desired temperature: %d\n",
> - ret);
> - return ret;
> - }
> -
> - return allocate_power(tz, trip.temperature);
> + return allocate_power(tz, params->trip_max_desired_temperature->temperature);
> }
>
> static struct thermal_governor thermal_gov_power_allocator = {
>
>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 21/09/2023 19:54, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Make get_trip_level() access the thermal zone's trip table directly
> instead of using __thermal_zone_get_trip() which adds overhead related
> to the unnecessary bounds checking and copying the trip point data.
>
> Also rearrange the code in it to make it somewhat easier to follow.
>
> The general functionality is not expected to be changed.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/thermal/gov_fair_share.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> 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
> @@ -21,23 +21,21 @@
> */
> static int get_trip_level(struct thermal_zone_device *tz)
> {
> - struct thermal_trip trip;
> - int count;
> + const struct thermal_trip *trip = tz->trips;
> + int i;
>
> - for (count = 0; count < tz->num_trips; count++) {
> - __thermal_zone_get_trip(tz, count, &trip);
> - if (tz->temperature < trip.temperature)
> + if (tz->temperature < trip->temperature)
> + return 0;
> +
> + for (i = 0; i < tz->num_trips - 1; i++) {
> + trip++;
> + if (tz->temperature < trip->temperature)
> break;
> }
Is it possible to use for_each_thermal_trip() instead ? That would make
the code more self-encapsulate
> - /*
> - * count > 0 only if temperature is greater than first trip
> - * point, in which case, trip_point = count - 1
> - */
> - if (count > 0)
> - trace_thermal_zone_trip(tz, count - 1, trip.type);
> + trace_thermal_zone_trip(tz, i, tz->trips[i].type);
>
> - return count;
> + return i;
> }
>
> static long get_target_state(struct thermal_zone_device *tz,
>
>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 27/09/2023 17:06, Rafael J. Wysocki wrote:
> On Wed, Sep 27, 2023 at 5:00 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> On 21/09/2023 19:54, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <[email protected]>
>>>
>>> Make get_trip_level() access the thermal zone's trip table directly
>>> instead of using __thermal_zone_get_trip() which adds overhead related
>>> to the unnecessary bounds checking and copying the trip point data.
>>>
>>> Also rearrange the code in it to make it somewhat easier to follow.
>>>
>>> The general functionality is not expected to be changed.
>>>
>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>> ---
>>> drivers/thermal/gov_fair_share.c | 22 ++++++++++------------
>>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> 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
>>> @@ -21,23 +21,21 @@
>>> */
>>> static int get_trip_level(struct thermal_zone_device *tz)
>>> {
>>> - struct thermal_trip trip;
>>> - int count;
>>> + const struct thermal_trip *trip = tz->trips;
>>> + int i;
>>>
>>> - for (count = 0; count < tz->num_trips; count++) {
>>> - __thermal_zone_get_trip(tz, count, &trip);
>>> - if (tz->temperature < trip.temperature)
>>> + if (tz->temperature < trip->temperature)
>>> + return 0;
>>> +
>>> + for (i = 0; i < tz->num_trips - 1; i++) {
>>> + trip++;
>>> + if (tz->temperature < trip->temperature)
>>> break;
>>> }
>>
>> Is it possible to use for_each_thermal_trip() instead ? That would make
>> the code more self-encapsulate
>
> It is possible in principle, but this is a governor which is regarded
> as part of the core, isn't it?
>
> So is an extra overhead related to using a callback (which may be
> subject to retpolines and such) really justified in this case?
From my POV, all trip points browsing should be replaced by
for_each_thermal_trip() so any change in the future in how we go through
the existing thermal trips will impact one place.
If the routine needs to be optimized, that is something we can do also
(may be an inline the callback?)
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 27/09/2023 17:27, Rafael J. Wysocki wrote:
> On Wed, Sep 27, 2023 at 5:10 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> On 21/09/2023 19:55, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <[email protected]>
>>>
>>> Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
>>> overhead (due to pointless bounds checking and copying of trip point
>>> data) from the power allocator thermal governor and generally make it
>>> use trip pointers instead of trip indices where applicable.
>>
>> Actually the __thermal_zone_get_trip() change was done on purpose to
>> replace the 'throttle' callback index parameter by the trip pointer and
>> removing those call to __thermal_zone_get_trip() while the code was
>> using the trip pointer.
>>
>> IMO, the changes should focus on changing the trip_index parameter by
>> the trip pointer directly in the throttle ops.
>
> So you would like .throttle() to take a trip pointer argument instead
> of an index?
>
> The difficulty here is that the user space governor needs to expose
> the index to user space anyway, so it would need to find it if it gets
> a trip pointer instead.
>
> Not a big deal I suppose, but a bit of extra overhead.
>
> Also it is easier to switch the governors over to using trip pointers
> internally and then change the .throttle() argument on top of that.
>
>> The pointer can be
>> retrieved in the handle_thermal_trip() function and passed around for
>> the rest of the actions on this trip point
>
> Right, except for the user space governor which needs a trip index.
> And the indices are used for tracing too.
Given the userspace governor is going obsolete and the notifications are
for the userspace, which is slow, we can retrieve the index from the
throttling ops
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 21/09/2023 20:02, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Rearrange the ACPI thermal driver's callback functions used for cooling
> device binding and unbinding, acpi_thermal_bind_cooling_device() and
> acpi_thermal_unbind_cooling_device(), respectively, so that they use trip
> pointers instead of trip indices which is more straightforward and allows
> the driver to become independent of the ordering of trips in the thermal
> zone structure.
>
> The general functionality is not expected to be changed.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
Reviewed-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Thu, Sep 28, 2023 at 9:10 AM Daniel Lezcano
<[email protected]> wrote:
>
> On 21/09/2023 20:01, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Add new helper functions, thermal_bind_cdev_to_trip() and
> > thermal_unbind_cdev_from_trip(), to allow a trip pointer to be used for
> > binding a cooling device to a trip point and unbinding it, respectively,
> > and redefine the existing helpers, thermal_zone_bind_cooling_device()
> > and thermal_zone_unbind_cooling_device(), as wrappers around the new
> > ones, respectively.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
>
> Reviewed-by: Daniel Lezcano <[email protected]>
Thanks so much for all of the reviews!
I'll now apply the patches for which you have given tags and respin
the rest (governor changes) as a separate series on top of them.
On 21/09/2023 20:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> The critical_valid and hot_valid flags in struct acpi_thermal_trips are
> only used during initialization and they are only false if the
> corresponding trip temperatures are equal to THERMAL_TEMP_INVALID, so
> drop them and use THERMAL_TEMP_INVALID checks instead of them where
> applicable.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
Reviewed-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 21/09/2023 20:01, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Add new helper functions, thermal_bind_cdev_to_trip() and
> thermal_unbind_cdev_from_trip(), to allow a trip pointer to be used for
> binding a cooling device to a trip point and unbinding it, respectively,
> and redefine the existing helpers, thermal_zone_bind_cooling_device()
> and thermal_zone_unbind_cooling_device(), as wrappers around the new
> ones, respectively.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
Reviewed-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 28/09/2023 12:38, Rafael J. Wysocki wrote:
> On Thu, Sep 28, 2023 at 9:10 AM Daniel Lezcano
> <[email protected]> wrote:
>>
>> On 21/09/2023 20:01, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <[email protected]>
>>>
>>> Add new helper functions, thermal_bind_cdev_to_trip() and
>>> thermal_unbind_cdev_from_trip(), to allow a trip pointer to be used for
>>> binding a cooling device to a trip point and unbinding it, respectively,
>>> and redefine the existing helpers, thermal_zone_bind_cooling_device()
>>> and thermal_zone_unbind_cooling_device(), as wrappers around the new
>>> ones, respectively.
>>>
>>> No intentional functional impact.
>>>
>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>> ---
>>
>> Reviewed-by: Daniel Lezcano <[email protected]>
>
> Thanks so much for all of the reviews!
You are welcome. Thanks for cleaning up the ACPI code
> I'll now apply the patches for which you have given tags and respin
> the rest (governor changes) as a separate series on top of them.
Sounds good, thanks
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog