* [PATCH v5 0/4] Add thermal user thresholds support
@ 2024-10-14 9:43 Daniel Lezcano
2024-10-14 9:43 ` [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds Daniel Lezcano
` (4 more replies)
0 siblings, 5 replies; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-14 9:43 UTC (permalink / raw)
To: daniel.lezcano, rafael; +Cc: linux-pm, lukasz.luba, quic_manafm
The trip points are a firmware description of the temperature limits
of a specific thermal zone where we associate an action which is done
by the kernel. The time resolution is low.
The userspace has to deal with a more complex thermal management based
on heuristics from different information coming from different
places. The logic is much more complex but based on a bigger time
resolution, usually one second based.
The purpose of the userspace is to monitor the temperatures from
different places and take actions. However, it can not be constantly
reading the temperature to detect when a temperature threshold has
been reached. This is especially bad for mobile or embedded system as
that will lead to an unacceptable number of wakeup to check the
temperature with nothing to do.
On the other side, the sensors are now most of the time interrupt
driven. That means the thermal framework will use the temperature trip
points to program the sensor to trigger an interrupt when a
temperature limit is crossed.
Unfortunately, the userspace can not benefit this feature and current
solutions found here and there, iow out-of-tree, are to add fake trip
points in the firmware and enable the writable trip points.
This is bad for different reasons, the trip points are for in-kernel
actions, the semantic of their types is used by the thermal framework
and by adding trip points in the device tree is a way to overcome the
current limitation but tampering with how the thermal framework is
supposed to work. The writable trip points is a way to adjust a
temperature limit given a specific platform if the firmware is not
accurate enough and TBH it is more a debug feature from my POV.
The user thresholds mechanism is a way to have the userspace to tell
thermal framework to send a notification when a temperature limit is
crossed. There is no id, no hysteresis, just the temperature and the
direction of the limit crossing. That means we can be notified when a
temperature threshold is crossed the way up only, or the way down only
or both ways. That allows to create hysteresis values if it is needed.
Those thresholds are refered as user thresholds in order to do the
difference with the trip points which are similar.
An user threshold can be added, deleted or flushed. The latter means
all user thresholds belonging to a thermal zone will be deleted.
When one or several user thresholds are crossed, an event is sent to
the userspace.
All aforementioned actions and events lead to a notification to the
userspace.
Along with the kernel changes, the thermal library has been extended
to provide the different API to deal with the new user threshold
netlink events and commands.
In addition, the thermal-engine skeleton uses these new API by
flushing and adding user thresholds as well as getting the
notification about these actions.
Overall the series has been tested with the thermal-engine skeleton
and some selftests which are not part of this series.
Changelog:
V5:
- Added CAP_SYS_ADMIN needed capability when adding, deleting and
flushing a threshold (Rafael)
- Remove the pid information to prevent leaking pid inside
containers. Also the information is not really needed (Rafael)
- Renamed "THERMAL_GENL_ATTR_THRESHOLD_WAY" to
"THERMAL_GENL_ATTR_THRESHOLD_DIRECTION". Did not used '*_DIR' as
suggested initially because it can be ambiguous with 'directory'
(Rafael)
- Renamed 'last_temp' to 'prev_temp' (Rafael)
- Used CLASS constructor/destructor to get / put the thermal
zone's device refcount (Rafael)
- Moved locking inside thermal_thresholds_for_each() (Rafael)
- Reflected the changes above in the thermal library and the
thermal engine skeleton
V4:
- Fix missing stubs when THERMAL_NETLINK=n (kernel test robot)
V3:
- the first patch of the v2 series has been merged
- Modified the description to split the information between the
cover letter and the patch 1 description (Rafael)
- Made the thresholds code as part of the core (Rafael)
- Converted the thresholds into a list and directly declared in
the thermal zone device structure (Rafael)
- Changed the name of the field in the thermal zone device
structure to user_thresholds (Rafael)
- Added #include "thermal_thresholds.h" (Rafael)
- Combined the conditions in the function
__thermal_threshold_is_crossed (Rafael)
- Moved the function thermal_thresholds_flush() before
thermal_thresholds_exit() (Rafael)
- Change thermal_thresholds_handle() to return void (Rafael)
- Move the list field on top the of the structure threshold and
renamed it list_node (Rafael)
- Changed THERMAL_THRESHOLD_* notifications to
THERMAL_TZ_THRESHOLD_* (Rafael)
V2:
- Compute min and max in thermal_zone_device_update() but keep
the loop as it is (Rafael)
- Include slab.h to fix compilation warnings on some architectures
with kmalloc and kfree (kernel test robot)
Daniel Lezcano (4):
thermal/netlink: Add the commands and the events for the thresholds
tools/lib/thermal: Make more generic the command encoding function
tools/lib/thermal: Add the threshold netlink ABI
tools/thermal/thermal-engine: Take into account the thresholds API
drivers/thermal/thermal_netlink.c | 236 +++++++++++++++++-
drivers/thermal/thermal_netlink.h | 34 +++
drivers/thermal/thermal_thresholds.c | 36 +--
drivers/thermal/thermal_thresholds.h | 2 +-
include/uapi/linux/thermal.h | 27 +-
tools/lib/thermal/commands.c | 167 ++++++++++++-
tools/lib/thermal/events.c | 55 +++-
tools/lib/thermal/include/thermal.h | 40 +++
tools/lib/thermal/libthermal.map | 5 +
tools/lib/thermal/thermal.c | 17 ++
tools/thermal/lib/Makefile | 2 +-
tools/thermal/thermal-engine/thermal-engine.c | 105 +++++++-
12 files changed, 662 insertions(+), 64 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-14 9:43 [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
@ 2024-10-14 9:43 ` Daniel Lezcano
2024-10-21 10:58 ` Rafael J. Wysocki
` (2 more replies)
2024-10-14 9:43 ` [PATCH v5 2/4] tools/lib/thermal: Make more generic the command encoding function Daniel Lezcano
` (3 subsequent siblings)
4 siblings, 3 replies; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-14 9:43 UTC (permalink / raw)
To: daniel.lezcano, rafael; +Cc: linux-pm, lukasz.luba, quic_manafm
The thresholds exist but there is no notification neither action code
related to them yet.
These changes implement the netlink for the notifications when the
thresholds are crossed, added, deleted or flushed as well as the
commands which allows to get the list of the thresholds, flush them,
add and delete.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
drivers/thermal/thermal_netlink.c | 236 ++++++++++++++++++++++++++-
drivers/thermal/thermal_netlink.h | 34 ++++
drivers/thermal/thermal_thresholds.c | 36 ++--
drivers/thermal/thermal_thresholds.h | 2 +-
include/uapi/linux/thermal.h | 27 ++-
5 files changed, 307 insertions(+), 28 deletions(-)
diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
index f3c58c708969..978cd46c15e8 100644
--- a/drivers/thermal/thermal_netlink.c
+++ b/drivers/thermal/thermal_netlink.c
@@ -9,6 +9,7 @@
#include <linux/module.h>
#include <linux/notifier.h>
#include <linux/kernel.h>
+#include <net/sock.h>
#include <net/genetlink.h>
#include <uapi/linux/thermal.h>
@@ -49,6 +50,11 @@ static const struct nla_policy thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] =
[THERMAL_GENL_ATTR_CPU_CAPABILITY_ID] = { .type = NLA_U32 },
[THERMAL_GENL_ATTR_CPU_CAPABILITY_PERFORMANCE] = { .type = NLA_U32 },
[THERMAL_GENL_ATTR_CPU_CAPABILITY_EFFICIENCY] = { .type = NLA_U32 },
+
+ /* Thresholds */
+ [THERMAL_GENL_ATTR_THRESHOLD] = { .type = NLA_NESTED },
+ [THERMAL_GENL_ATTR_THRESHOLD_TEMP] = { .type = NLA_U32 },
+ [THERMAL_GENL_ATTR_THRESHOLD_DIRECTION] = { .type = NLA_U32 },
};
struct param {
@@ -62,6 +68,8 @@ struct param {
int trip_type;
int trip_hyst;
int temp;
+ int prev_temp;
+ int direction;
int cdev_state;
int cdev_max_state;
struct thermal_genl_cpu_caps *cpu_capabilities;
@@ -234,6 +242,34 @@ static int thermal_genl_event_cpu_capability_change(struct param *p)
return -EMSGSIZE;
}
+static int thermal_genl_event_threshold_add(struct param *p)
+{
+ if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
+ nla_put_u32(p->msg, THERMAL_GENL_ATTR_THRESHOLD_TEMP, p->temp) ||
+ nla_put_u32(p->msg, THERMAL_GENL_ATTR_THRESHOLD_DIRECTION, p->direction))
+ return -EMSGSIZE;
+
+ return 0;
+}
+
+static int thermal_genl_event_threshold_flush(struct param *p)
+{
+ if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
+ return -EMSGSIZE;
+
+ return 0;
+}
+
+static int thermal_genl_event_threshold_up(struct param *p)
+{
+ if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
+ nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_PREV_TEMP, p->prev_temp) ||
+ nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TEMP, p->temp))
+ return -EMSGSIZE;
+
+ return 0;
+}
+
int thermal_genl_event_tz_delete(struct param *p)
__attribute__((alias("thermal_genl_event_tz")));
@@ -246,6 +282,12 @@ int thermal_genl_event_tz_disable(struct param *p)
int thermal_genl_event_tz_trip_down(struct param *p)
__attribute__((alias("thermal_genl_event_tz_trip_up")));
+int thermal_genl_event_threshold_delete(struct param *p)
+ __attribute__((alias("thermal_genl_event_threshold_add")));
+
+int thermal_genl_event_threshold_down(struct param *p)
+ __attribute__((alias("thermal_genl_event_threshold_up")));
+
static cb_t event_cb[] = {
[THERMAL_GENL_EVENT_TZ_CREATE] = thermal_genl_event_tz_create,
[THERMAL_GENL_EVENT_TZ_DELETE] = thermal_genl_event_tz_delete,
@@ -259,6 +301,11 @@ static cb_t event_cb[] = {
[THERMAL_GENL_EVENT_CDEV_STATE_UPDATE] = thermal_genl_event_cdev_state_update,
[THERMAL_GENL_EVENT_TZ_GOV_CHANGE] = thermal_genl_event_gov_change,
[THERMAL_GENL_EVENT_CPU_CAPABILITY_CHANGE] = thermal_genl_event_cpu_capability_change,
+ [THERMAL_GENL_EVENT_THRESHOLD_ADD] = thermal_genl_event_threshold_add,
+ [THERMAL_GENL_EVENT_THRESHOLD_DELETE] = thermal_genl_event_threshold_delete,
+ [THERMAL_GENL_EVENT_THRESHOLD_FLUSH] = thermal_genl_event_threshold_flush,
+ [THERMAL_GENL_EVENT_THRESHOLD_DOWN] = thermal_genl_event_threshold_down,
+ [THERMAL_GENL_EVENT_THRESHOLD_UP] = thermal_genl_event_threshold_up,
};
/*
@@ -401,6 +448,43 @@ int thermal_genl_cpu_capability_event(int count,
}
EXPORT_SYMBOL_GPL(thermal_genl_cpu_capability_event);
+int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
+ int temperature, int direction)
+{
+ struct param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
+
+ return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_ADD, &p);
+}
+
+int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
+ int temperature, int direction)
+{
+ struct param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
+
+ return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_DELETE, &p);
+}
+
+int thermal_notify_threshold_flush(const struct thermal_zone_device *tz)
+{
+ struct param p = { .tz_id = tz->id };
+
+ return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_FLUSH, &p);
+}
+
+int thermal_notify_threshold_down(const struct thermal_zone_device *tz)
+{
+ struct param p = { .tz_id = tz->id, .temp = tz->temperature, .prev_temp = tz->last_temperature };
+
+ return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_DOWN, &p);
+}
+
+int thermal_notify_threshold_up(const struct thermal_zone_device *tz)
+{
+ struct param p = { .tz_id = tz->id, .temp = tz->temperature, .prev_temp = tz->last_temperature };
+
+ return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_UP, &p);
+}
+
/*************************** Command encoding ********************************/
static int __thermal_genl_cmd_tz_get_id(struct thermal_zone_device *tz,
@@ -572,12 +656,132 @@ static int thermal_genl_cmd_cdev_get(struct param *p)
return ret;
}
+static int __thermal_genl_cmd_threshold_get(struct user_threshold *threshold, void *arg)
+{
+ struct sk_buff *msg = arg;
+
+ if (nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_TEMP, threshold->temperature) ||
+ nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_DIRECTION, threshold->direction))
+ return -1;
+
+ return 0;
+}
+
+static int thermal_genl_cmd_threshold_get(struct param *p)
+{
+ struct sk_buff *msg = p->msg;
+ struct nlattr *start_trip;
+ int id, ret;
+
+ if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
+ return -EINVAL;
+
+ id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
+
+ CLASS(thermal_zone_get_by_id, tz)(id);
+ if (!tz)
+ return -EINVAL;
+
+ start_trip = nla_nest_start(msg, THERMAL_GENL_ATTR_THRESHOLD);
+ if (!start_trip)
+ return -EMSGSIZE;
+
+ ret = thermal_thresholds_for_each(tz, __thermal_genl_cmd_threshold_get, msg);
+ if (ret)
+ return -EMSGSIZE;
+
+ nla_nest_end(msg, start_trip);
+
+ return 0;
+}
+
+static int thermal_genl_cmd_threshold_add(struct param *p)
+{
+ int id, temp, direction, ret = 0;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID] ||
+ !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP] ||
+ !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION])
+ return -EINVAL;
+
+ id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
+ temp = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]);
+ direction = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]);
+
+ CLASS(thermal_zone_get_by_id, tz)(id);
+ if (!tz)
+ return -EINVAL;
+
+ mutex_lock(&tz->lock);
+ ret = thermal_thresholds_add(tz, temp, direction);
+ mutex_unlock(&tz->lock);
+
+ return ret;
+}
+
+static int thermal_genl_cmd_threshold_delete(struct param *p)
+{
+ int id, temp, direction, ret = 0;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID] ||
+ !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP] ||
+ !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION])
+ return -EINVAL;
+
+ id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
+ temp = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]);
+ direction = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]);
+
+ CLASS(thermal_zone_get_by_id, tz)(id);
+ if (!tz)
+ return -EINVAL;
+
+ mutex_lock(&tz->lock);
+ ret = thermal_thresholds_delete(tz, temp, direction);
+ mutex_unlock(&tz->lock);
+
+ return ret;
+}
+
+static int thermal_genl_cmd_threshold_flush(struct param *p)
+{
+ int id;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
+ return -EINVAL;
+
+ id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
+
+ CLASS(thermal_zone_get_by_id, tz)(id);
+ if (!tz)
+ return -EINVAL;
+
+ mutex_lock(&tz->lock);
+ thermal_thresholds_flush(tz);
+ mutex_unlock(&tz->lock);
+
+ return 0;
+}
+
static cb_t cmd_cb[] = {
- [THERMAL_GENL_CMD_TZ_GET_ID] = thermal_genl_cmd_tz_get_id,
- [THERMAL_GENL_CMD_TZ_GET_TRIP] = thermal_genl_cmd_tz_get_trip,
- [THERMAL_GENL_CMD_TZ_GET_TEMP] = thermal_genl_cmd_tz_get_temp,
- [THERMAL_GENL_CMD_TZ_GET_GOV] = thermal_genl_cmd_tz_get_gov,
- [THERMAL_GENL_CMD_CDEV_GET] = thermal_genl_cmd_cdev_get,
+ [THERMAL_GENL_CMD_TZ_GET_ID] = thermal_genl_cmd_tz_get_id,
+ [THERMAL_GENL_CMD_TZ_GET_TRIP] = thermal_genl_cmd_tz_get_trip,
+ [THERMAL_GENL_CMD_TZ_GET_TEMP] = thermal_genl_cmd_tz_get_temp,
+ [THERMAL_GENL_CMD_TZ_GET_GOV] = thermal_genl_cmd_tz_get_gov,
+ [THERMAL_GENL_CMD_CDEV_GET] = thermal_genl_cmd_cdev_get,
+ [THERMAL_GENL_CMD_THRESHOLD_GET] = thermal_genl_cmd_threshold_get,
+ [THERMAL_GENL_CMD_THRESHOLD_ADD] = thermal_genl_cmd_threshold_add,
+ [THERMAL_GENL_CMD_THRESHOLD_DELETE] = thermal_genl_cmd_threshold_delete,
+ [THERMAL_GENL_CMD_THRESHOLD_FLUSH] = thermal_genl_cmd_threshold_flush,
};
static int thermal_genl_cmd_dumpit(struct sk_buff *skb,
@@ -688,6 +892,26 @@ static const struct genl_small_ops thermal_genl_ops[] = {
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
.dumpit = thermal_genl_cmd_dumpit,
},
+ {
+ .cmd = THERMAL_GENL_CMD_THRESHOLD_GET,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+ .doit = thermal_genl_cmd_doit,
+ },
+ {
+ .cmd = THERMAL_GENL_CMD_THRESHOLD_ADD,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+ .doit = thermal_genl_cmd_doit,
+ },
+ {
+ .cmd = THERMAL_GENL_CMD_THRESHOLD_DELETE,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+ .doit = thermal_genl_cmd_doit,
+ },
+ {
+ .cmd = THERMAL_GENL_CMD_THRESHOLD_FLUSH,
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+ .doit = thermal_genl_cmd_doit,
+ },
};
static struct genl_family thermal_genl_family __ro_after_init = {
@@ -700,7 +924,7 @@ static struct genl_family thermal_genl_family __ro_after_init = {
.unbind = thermal_genl_unbind,
.small_ops = thermal_genl_ops,
.n_small_ops = ARRAY_SIZE(thermal_genl_ops),
- .resv_start_op = THERMAL_GENL_CMD_CDEV_GET + 1,
+ .resv_start_op = __THERMAL_GENL_CMD_MAX,
.mcgrps = thermal_genl_mcgrps,
.n_mcgrps = ARRAY_SIZE(thermal_genl_mcgrps),
};
diff --git a/drivers/thermal/thermal_netlink.h b/drivers/thermal/thermal_netlink.h
index e01221e8816b..075e9ae85f3d 100644
--- a/drivers/thermal/thermal_netlink.h
+++ b/drivers/thermal/thermal_netlink.h
@@ -53,6 +53,13 @@ int thermal_notify_tz_gov_change(const struct thermal_zone_device *tz,
int thermal_genl_sampling_temp(int id, int temp);
int thermal_genl_cpu_capability_event(int count,
struct thermal_genl_cpu_caps *caps);
+int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
+ int temperature, int direction);
+int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
+ int temperature, int direction);
+int thermal_notify_threshold_flush(const struct thermal_zone_device *tz);
+int thermal_notify_threshold_down(const struct thermal_zone_device *tz);
+int thermal_notify_threshold_up(const struct thermal_zone_device *tz);
#else
static inline int thermal_netlink_init(void)
{
@@ -139,6 +146,33 @@ static inline int thermal_genl_cpu_capability_event(int count, struct thermal_ge
return 0;
}
+static inline int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
+ int temperature, int direction)
+{
+ return 0;
+}
+
+static inline int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
+ int temperature, int direction)
+{
+ return 0;
+}
+
+static inline int thermal_notify_threshold_flush(const struct thermal_zone_device *tz)
+{
+ return 0;
+}
+
+static inline int thermal_notify_threshold_down(const struct thermal_zone_device *tz)
+{
+ return 0;
+}
+
+static inline int thermal_notify_threshold_up(const struct thermal_zone_device *tz)
+{
+ return 0;
+}
+
static inline void __init thermal_netlink_exit(void) {}
#endif /* CONFIG_THERMAL_NETLINK */
diff --git a/drivers/thermal/thermal_thresholds.c b/drivers/thermal/thermal_thresholds.c
index f33b6d5474d8..ea4aa5a2e86c 100644
--- a/drivers/thermal/thermal_thresholds.c
+++ b/drivers/thermal/thermal_thresholds.c
@@ -32,6 +32,8 @@ void thermal_thresholds_flush(struct thermal_zone_device *tz)
kfree(entry);
}
+ thermal_notify_threshold_flush(tz);
+
__thermal_zone_device_update(tz, THERMAL_TZ_FLUSH_THRESHOLDS);
}
@@ -122,7 +124,6 @@ void thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *hi
int temperature = tz->temperature;
int last_temperature = tz->last_temperature;
- bool notify;
lockdep_assert_held(&tz->lock);
@@ -144,19 +145,19 @@ void thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *hi
* - increased : thresholds are crossed the way up
* - decreased : thresholds are crossed the way down
*/
- if (temperature > last_temperature)
- notify = thermal_thresholds_handle_raising(thresholds, temperature,
- last_temperature, low, high);
- else
- notify = thermal_thresholds_handle_dropping(thresholds, temperature,
- last_temperature, low, high);
-
- if (notify)
- pr_debug("A threshold has been crossed the way %s, with a temperature=%d, last_temperature=%d\n",
- temperature > last_temperature ? "up" : "down", temperature, last_temperature);
+ if (temperature > last_temperature) {
+ if (thermal_thresholds_handle_raising(thresholds, temperature,
+ last_temperature, low, high))
+ thermal_notify_threshold_up(tz);
+ } else {
+ if (thermal_thresholds_handle_dropping(thresholds, temperature,
+ last_temperature, low, high))
+ thermal_notify_threshold_down(tz);
+ }
}
-int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
+int thermal_thresholds_add(struct thermal_zone_device *tz,
+ int temperature, int direction)
{
struct list_head *thresholds = &tz->user_thresholds;
struct user_threshold *t;
@@ -182,12 +183,15 @@ int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int
list_sort(NULL, thresholds, __thermal_thresholds_cmp);
}
+ thermal_notify_threshold_add(tz, temperature, direction);
+
__thermal_zone_device_update(tz, THERMAL_TZ_ADD_THRESHOLD);
return 0;
}
-int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
+int thermal_thresholds_delete(struct thermal_zone_device *tz,
+ int temperature, int direction)
{
struct list_head *thresholds = &tz->user_thresholds;
struct user_threshold *t;
@@ -205,6 +209,8 @@ int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, i
t->direction &= ~direction;
}
+ thermal_notify_threshold_delete(tz, temperature, direction);
+
__thermal_zone_device_update(tz, THERMAL_TZ_DEL_THRESHOLD);
return 0;
@@ -217,7 +223,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
struct user_threshold *entry;
int ret;
- lockdep_assert_held(&tz->lock);
+ mutex_lock(&tz->lock);
list_for_each_entry(entry, thresholds, list_node) {
ret = cb(entry, arg);
@@ -225,5 +231,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
return ret;
}
+ mutex_unlock(&tz->lock);
+
return 0;
}
diff --git a/drivers/thermal/thermal_thresholds.h b/drivers/thermal/thermal_thresholds.h
index 232f4e8089af..cb372659a20d 100644
--- a/drivers/thermal/thermal_thresholds.h
+++ b/drivers/thermal/thermal_thresholds.h
@@ -10,8 +10,8 @@ struct user_threshold {
int thermal_thresholds_init(struct thermal_zone_device *tz);
void thermal_thresholds_exit(struct thermal_zone_device *tz);
-void thermal_thresholds_flush(struct thermal_zone_device *tz);
void thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high);
+void thermal_thresholds_flush(struct thermal_zone_device *tz);
int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction);
int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction);
int thermal_thresholds_for_each(struct thermal_zone_device *tz,
diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
index 2e6f60a36173..ba8604bdf206 100644
--- a/include/uapi/linux/thermal.h
+++ b/include/uapi/linux/thermal.h
@@ -20,7 +20,7 @@ enum thermal_trip_type {
/* Adding event notification support elements */
#define THERMAL_GENL_FAMILY_NAME "thermal"
-#define THERMAL_GENL_VERSION 0x01
+#define THERMAL_GENL_VERSION 0x02
#define THERMAL_GENL_SAMPLING_GROUP_NAME "sampling"
#define THERMAL_GENL_EVENT_GROUP_NAME "event"
@@ -30,6 +30,7 @@ enum thermal_genl_attr {
THERMAL_GENL_ATTR_TZ,
THERMAL_GENL_ATTR_TZ_ID,
THERMAL_GENL_ATTR_TZ_TEMP,
+ THERMAL_GENL_ATTR_TZ_PREV_TEMP,
THERMAL_GENL_ATTR_TZ_TRIP,
THERMAL_GENL_ATTR_TZ_TRIP_ID,
THERMAL_GENL_ATTR_TZ_TRIP_TYPE,
@@ -50,6 +51,9 @@ enum thermal_genl_attr {
THERMAL_GENL_ATTR_CPU_CAPABILITY_ID,
THERMAL_GENL_ATTR_CPU_CAPABILITY_PERFORMANCE,
THERMAL_GENL_ATTR_CPU_CAPABILITY_EFFICIENCY,
+ THERMAL_GENL_ATTR_THRESHOLD,
+ THERMAL_GENL_ATTR_THRESHOLD_TEMP,
+ THERMAL_GENL_ATTR_THRESHOLD_DIRECTION,
__THERMAL_GENL_ATTR_MAX,
};
#define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
@@ -77,6 +81,11 @@ enum thermal_genl_event {
THERMAL_GENL_EVENT_CDEV_STATE_UPDATE, /* Cdev state updated */
THERMAL_GENL_EVENT_TZ_GOV_CHANGE, /* Governor policy changed */
THERMAL_GENL_EVENT_CPU_CAPABILITY_CHANGE, /* CPU capability changed */
+ THERMAL_GENL_EVENT_THRESHOLD_ADD, /* A thresold has been added */
+ THERMAL_GENL_EVENT_THRESHOLD_DELETE, /* A thresold has been deleted */
+ THERMAL_GENL_EVENT_THRESHOLD_FLUSH, /* All thresolds have been deleted */
+ THERMAL_GENL_EVENT_THRESHOLD_UP, /* A thresold has been crossed the way up */
+ THERMAL_GENL_EVENT_THRESHOLD_DOWN, /* A thresold has been crossed the way down */
__THERMAL_GENL_EVENT_MAX,
};
#define THERMAL_GENL_EVENT_MAX (__THERMAL_GENL_EVENT_MAX - 1)
@@ -84,12 +93,16 @@ enum thermal_genl_event {
/* Commands supported by the thermal_genl_family */
enum thermal_genl_cmd {
THERMAL_GENL_CMD_UNSPEC,
- THERMAL_GENL_CMD_TZ_GET_ID, /* List of thermal zones id */
- THERMAL_GENL_CMD_TZ_GET_TRIP, /* List of thermal trips */
- THERMAL_GENL_CMD_TZ_GET_TEMP, /* Get the thermal zone temperature */
- THERMAL_GENL_CMD_TZ_GET_GOV, /* Get the thermal zone governor */
- THERMAL_GENL_CMD_TZ_GET_MODE, /* Get the thermal zone mode */
- THERMAL_GENL_CMD_CDEV_GET, /* List of cdev id */
+ THERMAL_GENL_CMD_TZ_GET_ID, /* List of thermal zones id */
+ THERMAL_GENL_CMD_TZ_GET_TRIP, /* List of thermal trips */
+ THERMAL_GENL_CMD_TZ_GET_TEMP, /* Get the thermal zone temperature */
+ THERMAL_GENL_CMD_TZ_GET_GOV, /* Get the thermal zone governor */
+ THERMAL_GENL_CMD_TZ_GET_MODE, /* Get the thermal zone mode */
+ THERMAL_GENL_CMD_CDEV_GET, /* List of cdev id */
+ THERMAL_GENL_CMD_THRESHOLD_GET, /* List of thresholds */
+ THERMAL_GENL_CMD_THRESHOLD_ADD, /* Add a threshold */
+ THERMAL_GENL_CMD_THRESHOLD_DELETE, /* Delete a threshold */
+ THERMAL_GENL_CMD_THRESHOLD_FLUSH, /* Flush all the thresholds */
__THERMAL_GENL_CMD_MAX,
};
#define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 2/4] tools/lib/thermal: Make more generic the command encoding function
2024-10-14 9:43 [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
2024-10-14 9:43 ` [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds Daniel Lezcano
@ 2024-10-14 9:43 ` Daniel Lezcano
2024-10-21 19:49 ` Lukasz Luba
2024-10-14 9:43 ` [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI Daniel Lezcano
` (2 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-14 9:43 UTC (permalink / raw)
To: daniel.lezcano, rafael; +Cc: linux-pm, lukasz.luba, quic_manafm
The thermal netlink has been extended with more commands which require
an encoding with more information. The generic encoding function puts
the thermal zone id with the command name. It is the unique
parameters.
The next changes will provide more parameters to the command. Set the
scene for those new parameters by making the encoding function more
generic.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
tools/lib/thermal/commands.c | 41 ++++++++++++++++++++++++++++--------
1 file changed, 32 insertions(+), 9 deletions(-)
diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
index 73d4d4e8d6ec..a9223df91dcf 100644
--- a/tools/lib/thermal/commands.c
+++ b/tools/lib/thermal/commands.c
@@ -261,8 +261,23 @@ static struct genl_ops thermal_cmd_ops = {
.o_ncmds = ARRAY_SIZE(thermal_cmds),
};
-static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int cmd,
- int flags, void *arg)
+struct cmd_param {
+ int tz_id;
+};
+
+typedef int (*cmd_cb_t)(struct nl_msg *, struct cmd_param *);
+
+static int thermal_genl_tz_id_encode(struct nl_msg *msg, struct cmd_param *p)
+{
+ if (p->tz_id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
+ return -1;
+
+ return 0;
+}
+
+static thermal_error_t thermal_genl_auto(struct thermal_handler *th, cmd_cb_t cmd_cb,
+ struct cmd_param *param,
+ int cmd, int flags, void *arg)
{
struct nl_msg *msg;
void *hdr;
@@ -276,7 +291,7 @@ static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int
if (!hdr)
return THERMAL_ERROR;
- if (id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id))
+ if (cmd_cb && cmd_cb(msg, param))
return THERMAL_ERROR;
if (nl_send_msg(th->sk_cmd, th->cb_cmd, msg, genl_handle_msg, arg))
@@ -289,30 +304,38 @@ static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int
thermal_error_t thermal_cmd_get_tz(struct thermal_handler *th, struct thermal_zone **tz)
{
- return thermal_genl_auto(th, -1, THERMAL_GENL_CMD_TZ_GET_ID,
+ return thermal_genl_auto(th, NULL, NULL, THERMAL_GENL_CMD_TZ_GET_ID,
NLM_F_DUMP | NLM_F_ACK, tz);
}
thermal_error_t thermal_cmd_get_cdev(struct thermal_handler *th, struct thermal_cdev **tc)
{
- return thermal_genl_auto(th, -1, THERMAL_GENL_CMD_CDEV_GET,
+ return thermal_genl_auto(th, NULL, NULL, THERMAL_GENL_CMD_CDEV_GET,
NLM_F_DUMP | NLM_F_ACK, tc);
}
thermal_error_t thermal_cmd_get_trip(struct thermal_handler *th, struct thermal_zone *tz)
{
- return thermal_genl_auto(th, tz->id, THERMAL_GENL_CMD_TZ_GET_TRIP,
- 0, tz);
+ struct cmd_param p = { .tz_id = tz->id };
+
+ return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
+ THERMAL_GENL_CMD_TZ_GET_TRIP, 0, tz);
}
thermal_error_t thermal_cmd_get_governor(struct thermal_handler *th, struct thermal_zone *tz)
{
- return thermal_genl_auto(th, tz->id, THERMAL_GENL_CMD_TZ_GET_GOV, 0, tz);
+ struct cmd_param p = { .tz_id = tz->id };
+
+ return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
+ THERMAL_GENL_CMD_TZ_GET_GOV, 0, tz);
}
thermal_error_t thermal_cmd_get_temp(struct thermal_handler *th, struct thermal_zone *tz)
{
- return thermal_genl_auto(th, tz->id, THERMAL_GENL_CMD_TZ_GET_TEMP, 0, tz);
+ struct cmd_param p = { .tz_id = tz->id };
+
+ return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
+ THERMAL_GENL_CMD_TZ_GET_TEMP, 0, tz);
}
thermal_error_t thermal_cmd_exit(struct thermal_handler *th)
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI
2024-10-14 9:43 [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
2024-10-14 9:43 ` [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds Daniel Lezcano
2024-10-14 9:43 ` [PATCH v5 2/4] tools/lib/thermal: Make more generic the command encoding function Daniel Lezcano
@ 2024-10-14 9:43 ` Daniel Lezcano
2024-10-21 20:47 ` Lukasz Luba
2024-10-14 9:43 ` [PATCH v5 4/4] tools/thermal/thermal-engine: Take into account the thresholds API Daniel Lezcano
2024-10-21 8:28 ` [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
4 siblings, 1 reply; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-14 9:43 UTC (permalink / raw)
To: daniel.lezcano, rafael; +Cc: linux-pm, lukasz.luba, quic_manafm
The thermal framework supports the thresholds and allows the userspace
to create, delete, flush, get the list of the thresholds as well as
getting the list of the thresholds set for a specific thermal zone.
Add the netlink abstraction in the thermal library to take full
advantage of thresholds for the userspace program.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
tools/lib/thermal/commands.c | 128 +++++++++++++++++++++++++++-
tools/lib/thermal/events.c | 55 +++++++++---
tools/lib/thermal/include/thermal.h | 40 +++++++++
tools/lib/thermal/libthermal.map | 5 ++
tools/lib/thermal/thermal.c | 17 ++++
tools/thermal/lib/Makefile | 2 +-
6 files changed, 232 insertions(+), 15 deletions(-)
diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
index a9223df91dcf..9d5e3e891628 100644
--- a/tools/lib/thermal/commands.c
+++ b/tools/lib/thermal/commands.c
@@ -5,6 +5,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
+#include <limits.h>
#include <thermal.h>
#include "thermal_nl.h"
@@ -33,6 +34,11 @@ static struct nla_policy thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
[THERMAL_GENL_ATTR_CDEV_CUR_STATE] = { .type = NLA_U32 },
[THERMAL_GENL_ATTR_CDEV_MAX_STATE] = { .type = NLA_U32 },
[THERMAL_GENL_ATTR_CDEV_NAME] = { .type = NLA_STRING },
+
+ /* Thresholds */
+ [THERMAL_GENL_ATTR_THRESHOLD] = { .type = NLA_NESTED },
+ [THERMAL_GENL_ATTR_THRESHOLD_TEMP] = { .type = NLA_U32 },
+ [THERMAL_GENL_ATTR_THRESHOLD_DIRECTION] = { .type = NLA_U32 },
};
static int parse_tz_get(struct genl_info *info, struct thermal_zone **tz)
@@ -182,6 +188,38 @@ static int parse_tz_get_gov(struct genl_info *info, struct thermal_zone *tz)
return THERMAL_SUCCESS;
}
+static int parse_threshold_get(struct genl_info *info, struct thermal_zone *tz)
+{
+ struct nlattr *attr;
+ struct thermal_threshold *__tt = NULL;
+ size_t size = 0;
+ int rem;
+
+ nla_for_each_nested(attr, info->attrs[THERMAL_GENL_ATTR_THRESHOLD], rem) {
+
+ if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_TEMP) {
+
+ size++;
+
+ __tt = realloc(__tt, sizeof(*__tt) * (size + 2));
+ if (!__tt)
+ return THERMAL_ERROR;
+
+ __tt[size - 1].temperature = nla_get_u32(attr);
+ }
+
+ if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_DIRECTION)
+ __tt[size - 1].direction = nla_get_u32(attr);
+ }
+
+ if (__tt)
+ __tt[size].temperature = INT_MAX;
+
+ tz->thresholds = __tt;
+
+ return THERMAL_SUCCESS;
+}
+
static int handle_netlink(struct nl_cache_ops *unused,
struct genl_cmd *cmd,
struct genl_info *info, void *arg)
@@ -210,6 +248,10 @@ static int handle_netlink(struct nl_cache_ops *unused,
ret = parse_tz_get_gov(info, arg);
break;
+ case THERMAL_GENL_CMD_THRESHOLD_GET:
+ ret = parse_threshold_get(info, arg);
+ break;
+
default:
return THERMAL_ERROR;
}
@@ -253,6 +295,34 @@ static struct genl_cmd thermal_cmds[] = {
.c_maxattr = THERMAL_GENL_ATTR_MAX,
.c_attr_policy = thermal_genl_policy,
},
+ {
+ .c_id = THERMAL_GENL_CMD_THRESHOLD_GET,
+ .c_name = (char *)"Get thresholds list",
+ .c_msg_parser = handle_netlink,
+ .c_maxattr = THERMAL_GENL_ATTR_MAX,
+ .c_attr_policy = thermal_genl_policy,
+ },
+ {
+ .c_id = THERMAL_GENL_CMD_THRESHOLD_ADD,
+ .c_name = (char *)"Add a threshold",
+ .c_msg_parser = handle_netlink,
+ .c_maxattr = THERMAL_GENL_ATTR_MAX,
+ .c_attr_policy = thermal_genl_policy,
+ },
+ {
+ .c_id = THERMAL_GENL_CMD_THRESHOLD_DELETE,
+ .c_name = (char *)"Delete a threshold",
+ .c_msg_parser = handle_netlink,
+ .c_maxattr = THERMAL_GENL_ATTR_MAX,
+ .c_attr_policy = thermal_genl_policy,
+ },
+ {
+ .c_id = THERMAL_GENL_CMD_THRESHOLD_FLUSH,
+ .c_name = (char *)"Flush the thresholds",
+ .c_msg_parser = handle_netlink,
+ .c_maxattr = THERMAL_GENL_ATTR_MAX,
+ .c_attr_policy = thermal_genl_policy,
+ },
};
static struct genl_ops thermal_cmd_ops = {
@@ -263,13 +333,29 @@ static struct genl_ops thermal_cmd_ops = {
struct cmd_param {
int tz_id;
+ int temp;
+ int direction;
};
typedef int (*cmd_cb_t)(struct nl_msg *, struct cmd_param *);
static int thermal_genl_tz_id_encode(struct nl_msg *msg, struct cmd_param *p)
{
- if (p->tz_id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
+ if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
+ return -1;
+
+ return 0;
+}
+
+static int thermal_genl_threshold_encode(struct nl_msg *msg, struct cmd_param *p)
+{
+ if (thermal_genl_tz_id_encode(msg, p))
+ return -1;
+
+ if (nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_TEMP, p->temp))
+ return -1;
+
+ if (nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_DIRECTION, p->direction))
return -1;
return 0;
@@ -338,6 +424,46 @@ thermal_error_t thermal_cmd_get_temp(struct thermal_handler *th, struct thermal_
THERMAL_GENL_CMD_TZ_GET_TEMP, 0, tz);
}
+thermal_error_t thermal_cmd_threshold_get(struct thermal_handler *th,
+ struct thermal_zone *tz)
+{
+ struct cmd_param p = { .tz_id = tz->id };
+
+ return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
+ THERMAL_GENL_CMD_THRESHOLD_GET, 0, tz);
+}
+
+thermal_error_t thermal_cmd_threshold_add(struct thermal_handler *th,
+ struct thermal_zone *tz,
+ int temperature,
+ int direction)
+{
+ struct cmd_param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
+
+ return thermal_genl_auto(th, thermal_genl_threshold_encode, &p,
+ THERMAL_GENL_CMD_THRESHOLD_ADD, 0, tz);
+}
+
+thermal_error_t thermal_cmd_threshold_delete(struct thermal_handler *th,
+ struct thermal_zone *tz,
+ int temperature,
+ int direction)
+{
+ struct cmd_param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
+
+ return thermal_genl_auto(th, thermal_genl_threshold_encode, &p,
+ THERMAL_GENL_CMD_THRESHOLD_DELETE, 0, tz);
+}
+
+thermal_error_t thermal_cmd_threshold_flush(struct thermal_handler *th,
+ struct thermal_zone *tz)
+{
+ struct cmd_param p = { .tz_id = tz->id };
+
+ return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
+ THERMAL_GENL_CMD_THRESHOLD_FLUSH, 0, tz);
+}
+
thermal_error_t thermal_cmd_exit(struct thermal_handler *th)
{
if (genl_unregister_family(&thermal_cmd_ops))
diff --git a/tools/lib/thermal/events.c b/tools/lib/thermal/events.c
index a7a55d1a0c4c..bd851c869029 100644
--- a/tools/lib/thermal/events.c
+++ b/tools/lib/thermal/events.c
@@ -94,6 +94,30 @@ static int handle_thermal_event(struct nl_msg *n, void *arg)
case THERMAL_GENL_EVENT_TZ_GOV_CHANGE:
return ops->gov_change(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
nla_get_string(attrs[THERMAL_GENL_ATTR_GOV_NAME]), arg);
+
+ case THERMAL_GENL_EVENT_THRESHOLD_ADD:
+ return ops->threshold_add(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
+ nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]),
+ nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]), arg);
+
+ case THERMAL_GENL_EVENT_THRESHOLD_DELETE:
+ return ops->threshold_delete(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
+ nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]),
+ nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]), arg);
+
+ case THERMAL_GENL_EVENT_THRESHOLD_FLUSH:
+ return ops->threshold_flush(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]), arg);
+
+ case THERMAL_GENL_EVENT_THRESHOLD_UP:
+ return ops->threshold_up(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
+ nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_TEMP]),
+ nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_PREV_TEMP]), arg);
+
+ case THERMAL_GENL_EVENT_THRESHOLD_DOWN:
+ return ops->threshold_down(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
+ nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_TEMP]),
+ nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_PREV_TEMP]), arg);
+
default:
return -1;
}
@@ -101,19 +125,24 @@ static int handle_thermal_event(struct nl_msg *n, void *arg)
static void thermal_events_ops_init(struct thermal_events_ops *ops)
{
- enabled_ops[THERMAL_GENL_EVENT_TZ_CREATE] = !!ops->tz_create;
- enabled_ops[THERMAL_GENL_EVENT_TZ_DELETE] = !!ops->tz_delete;
- enabled_ops[THERMAL_GENL_EVENT_TZ_DISABLE] = !!ops->tz_disable;
- enabled_ops[THERMAL_GENL_EVENT_TZ_ENABLE] = !!ops->tz_enable;
- enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_UP] = !!ops->trip_high;
- enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_DOWN] = !!ops->trip_low;
- enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_CHANGE] = !!ops->trip_change;
- enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_ADD] = !!ops->trip_add;
- enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_DELETE] = !!ops->trip_delete;
- enabled_ops[THERMAL_GENL_EVENT_CDEV_ADD] = !!ops->cdev_add;
- enabled_ops[THERMAL_GENL_EVENT_CDEV_DELETE] = !!ops->cdev_delete;
- enabled_ops[THERMAL_GENL_EVENT_CDEV_STATE_UPDATE] = !!ops->cdev_update;
- enabled_ops[THERMAL_GENL_EVENT_TZ_GOV_CHANGE] = !!ops->gov_change;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_CREATE] = !!ops->tz_create;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_DELETE] = !!ops->tz_delete;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_DISABLE] = !!ops->tz_disable;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_ENABLE] = !!ops->tz_enable;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_UP] = !!ops->trip_high;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_DOWN] = !!ops->trip_low;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_CHANGE] = !!ops->trip_change;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_ADD] = !!ops->trip_add;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_DELETE] = !!ops->trip_delete;
+ enabled_ops[THERMAL_GENL_EVENT_CDEV_ADD] = !!ops->cdev_add;
+ enabled_ops[THERMAL_GENL_EVENT_CDEV_DELETE] = !!ops->cdev_delete;
+ enabled_ops[THERMAL_GENL_EVENT_CDEV_STATE_UPDATE] = !!ops->cdev_update;
+ enabled_ops[THERMAL_GENL_EVENT_TZ_GOV_CHANGE] = !!ops->gov_change;
+ enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_ADD] = !!ops->threshold_add;
+ enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_DELETE] = !!ops->threshold_delete;
+ enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_FLUSH] = !!ops->threshold_flush;
+ enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_UP] = !!ops->threshold_up;
+ enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_DOWN] = !!ops->threshold_down;
}
thermal_error_t thermal_events_handle(struct thermal_handler *th, void *arg)
diff --git a/tools/lib/thermal/include/thermal.h b/tools/lib/thermal/include/thermal.h
index 1abc560602cf..818ecdfb46e5 100644
--- a/tools/lib/thermal/include/thermal.h
+++ b/tools/lib/thermal/include/thermal.h
@@ -4,11 +4,20 @@
#define __LIBTHERMAL_H
#include <linux/thermal.h>
+#include <sys/types.h>
#ifndef LIBTHERMAL_API
#define LIBTHERMAL_API __attribute__((visibility("default")))
#endif
+#ifndef THERMAL_THRESHOLD_WAY_UP
+#define THERMAL_THRESHOLD_WAY_UP 0x1
+#endif
+
+#ifndef THERMAL_THRESHOLD_WAY_DOWN
+#define THERMAL_THRESHOLD_WAY_DOWN 0x2
+#endif
+
#ifdef __cplusplus
extern "C" {
#endif
@@ -31,6 +40,11 @@ struct thermal_events_ops {
int (*cdev_delete)(int cdev_id, void *arg);
int (*cdev_update)(int cdev_id, int cur_state, void *arg);
int (*gov_change)(int tz_id, const char *gov_name, void *arg);
+ int (*threshold_add)(int tz_id, int temperature, int direction, void *arg);
+ int (*threshold_delete)(int tz_id, int temperature, int direction, void *arg);
+ int (*threshold_flush)(int tz_id, void *arg);
+ int (*threshold_up)(int tz_id, int temp, int prev_temp, void *arg);
+ int (*threshold_down)(int tz_id, int temp, int prev_temp, void *arg);
};
struct thermal_ops {
@@ -45,12 +59,18 @@ struct thermal_trip {
int hyst;
};
+struct thermal_threshold {
+ int temperature;
+ int direction;
+};
+
struct thermal_zone {
int id;
int temp;
char name[THERMAL_NAME_LENGTH];
char governor[THERMAL_NAME_LENGTH];
struct thermal_trip *trip;
+ struct thermal_threshold *thresholds;
};
struct thermal_cdev {
@@ -74,12 +94,16 @@ typedef int (*cb_tt_t)(struct thermal_trip *, void *);
typedef int (*cb_tc_t)(struct thermal_cdev *, void *);
+typedef int (*cb_th_t)(struct thermal_threshold *, void *);
+
LIBTHERMAL_API int for_each_thermal_zone(struct thermal_zone *tz, cb_tz_t cb, void *arg);
LIBTHERMAL_API int for_each_thermal_trip(struct thermal_trip *tt, cb_tt_t cb, void *arg);
LIBTHERMAL_API int for_each_thermal_cdev(struct thermal_cdev *cdev, cb_tc_t cb, void *arg);
+LIBTHERMAL_API int for_each_thermal_threshold(struct thermal_threshold *th, cb_th_t cb, void *arg);
+
LIBTHERMAL_API struct thermal_zone *thermal_zone_find_by_name(struct thermal_zone *tz,
const char *name);
@@ -124,6 +148,22 @@ LIBTHERMAL_API thermal_error_t thermal_cmd_get_governor(struct thermal_handler *
LIBTHERMAL_API thermal_error_t thermal_cmd_get_temp(struct thermal_handler *th,
struct thermal_zone *tz);
+LIBTHERMAL_API thermal_error_t thermal_cmd_threshold_get(struct thermal_handler *th,
+ struct thermal_zone *tz);
+
+LIBTHERMAL_API thermal_error_t thermal_cmd_threshold_add(struct thermal_handler *th,
+ struct thermal_zone *tz,
+ int temperature,
+ int direction);
+
+LIBTHERMAL_API thermal_error_t thermal_cmd_threshold_delete(struct thermal_handler *th,
+ struct thermal_zone *tz,
+ int temperature,
+ int direction);
+
+LIBTHERMAL_API thermal_error_t thermal_cmd_threshold_flush(struct thermal_handler *th,
+ struct thermal_zone *tz);
+
/*
* Netlink thermal samples
*/
diff --git a/tools/lib/thermal/libthermal.map b/tools/lib/thermal/libthermal.map
index d5e77738c7a4..d657176aa47f 100644
--- a/tools/lib/thermal/libthermal.map
+++ b/tools/lib/thermal/libthermal.map
@@ -4,6 +4,7 @@ LIBTHERMAL_0.0.1 {
for_each_thermal_zone;
for_each_thermal_trip;
for_each_thermal_cdev;
+ for_each_thermal_threshold;
thermal_zone_find_by_name;
thermal_zone_find_by_id;
thermal_zone_discover;
@@ -17,6 +18,10 @@ LIBTHERMAL_0.0.1 {
thermal_cmd_get_trip;
thermal_cmd_get_governor;
thermal_cmd_get_temp;
+ thermal_cmd_threshold_get;
+ thermal_cmd_threshold_add;
+ thermal_cmd_threshold_delete;
+ thermal_cmd_threshold_flush;
thermal_sampling_init;
thermal_sampling_handle;
thermal_sampling_fd;
diff --git a/tools/lib/thermal/thermal.c b/tools/lib/thermal/thermal.c
index 72a76dc205bc..8dddf5cde302 100644
--- a/tools/lib/thermal/thermal.c
+++ b/tools/lib/thermal/thermal.c
@@ -1,10 +1,24 @@
// SPDX-License-Identifier: LGPL-2.1+
// Copyright (C) 2022, Linaro Ltd - Daniel Lezcano <daniel.lezcano@linaro.org>
#include <stdio.h>
+#include <limits.h>
#include <thermal.h>
#include "thermal_nl.h"
+int for_each_thermal_threshold(struct thermal_threshold *th, cb_th_t cb, void *arg)
+{
+ int i, ret = 0;
+
+ if (!th)
+ return 0;
+
+ for (i = 0; th[i].temperature != INT_MAX; i++)
+ ret |= cb(&th[i], arg);
+
+ return ret;
+}
+
int for_each_thermal_cdev(struct thermal_cdev *cdev, cb_tc_t cb, void *arg)
{
int i, ret = 0;
@@ -80,6 +94,9 @@ static int __thermal_zone_discover(struct thermal_zone *tz, void *th)
if (thermal_cmd_get_trip(th, tz) < 0)
return -1;
+ if (thermal_cmd_threshold_get(th, tz) < 0)
+ return -1;
+
if (thermal_cmd_get_governor(th, tz))
return -1;
diff --git a/tools/thermal/lib/Makefile b/tools/thermal/lib/Makefile
index 82db451935c5..f2552f73a64c 100644
--- a/tools/thermal/lib/Makefile
+++ b/tools/thermal/lib/Makefile
@@ -3,7 +3,7 @@
LIBTHERMAL_TOOLS_VERSION = 0
LIBTHERMAL_TOOLS_PATCHLEVEL = 0
-LIBTHERMAL_TOOLS_EXTRAVERSION = 1
+LIBTHERMAL_TOOLS_EXTRAVERSION = 2
MAKEFLAGS += --no-print-directory
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 4/4] tools/thermal/thermal-engine: Take into account the thresholds API
2024-10-14 9:43 [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
` (2 preceding siblings ...)
2024-10-14 9:43 ` [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI Daniel Lezcano
@ 2024-10-14 9:43 ` Daniel Lezcano
2024-10-21 20:10 ` Lukasz Luba
2024-10-21 8:28 ` [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
4 siblings, 1 reply; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-14 9:43 UTC (permalink / raw)
To: daniel.lezcano, rafael; +Cc: linux-pm, lukasz.luba, quic_manafm
Enhance the thermal-engine skeleton with the thresholds added in the
kernel and use the API exported by the thermal library.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
tools/thermal/thermal-engine/thermal-engine.c | 105 +++++++++++++++---
1 file changed, 92 insertions(+), 13 deletions(-)
diff --git a/tools/thermal/thermal-engine/thermal-engine.c b/tools/thermal/thermal-engine/thermal-engine.c
index 9b1476a2680f..0764dc754771 100644
--- a/tools/thermal/thermal-engine/thermal-engine.c
+++ b/tools/thermal/thermal-engine/thermal-engine.c
@@ -38,6 +38,14 @@ struct thermal_data {
struct thermal_handler *th;
};
+static int show_threshold(struct thermal_threshold *th, __maybe_unused void *arg)
+{
+ INFO("threshold temp=%d, direction=%d\n",
+ th->temperature, th->direction);
+
+ return 0;
+}
+
static int show_trip(struct thermal_trip *tt, __maybe_unused void *arg)
{
INFO("trip id=%d, type=%d, temp=%d, hyst=%d\n",
@@ -70,6 +78,8 @@ static int show_tz(struct thermal_zone *tz, __maybe_unused void *arg)
for_each_thermal_trip(tz->trip, show_trip, NULL);
+ for_each_thermal_threshold(tz->thresholds, show_threshold, NULL);
+
show_temp(tz, arg);
show_governor(tz, arg);
@@ -77,6 +87,30 @@ static int show_tz(struct thermal_zone *tz, __maybe_unused void *arg)
return 0;
}
+static int set_threshold(struct thermal_zone *tz, __maybe_unused void *arg)
+{
+ struct thermal_handler *th = arg;
+ int thresholds[] = { 43000, 65000, 49000, 55000, 57000 };
+ size_t i;
+
+ INFO("Setting threshold for thermal zone '%s', id=%d\n", tz->name, tz->id);
+
+ if (thermal_cmd_threshold_flush(th, tz)) {
+ ERROR("Failed to flush all previous thresholds\n");
+ return -1;
+ }
+
+ for (i = 0; i < sizeof(thresholds) / sizeof(thresholds[0]); i++)
+ if (thermal_cmd_threshold_add(th, tz, thresholds[i],
+ THERMAL_THRESHOLD_WAY_UP |
+ THERMAL_THRESHOLD_WAY_DOWN)) {
+ ERROR("Failed to set threshold\n");
+ return -1;
+ }
+
+ return 0;
+}
+
static int tz_create(const char *name, int tz_id, __maybe_unused void *arg)
{
INFO("Thermal zone '%s'/%d created\n", name, tz_id);
@@ -197,20 +231,62 @@ static int gov_change(int tz_id, const char *name, __maybe_unused void *arg)
return 0;
}
+static int threshold_add(int tz_id, int temp, int direction, __maybe_unused void *arg)
+{
+ INFO("Threshold added tz_id=%d: temp=%d, direction=%d\n", tz_id, temp, direction);
+
+ return 0;
+}
+
+static int threshold_delete(int tz_id, int temp, int direction, __maybe_unused void *arg)
+{
+ INFO("Threshold deleted tz_id=%d: temp=%d, direction=%d\n", tz_id, temp, direction);
+
+ return 0;
+}
+
+static int threshold_flush(int tz_id, __maybe_unused void *arg)
+{
+ INFO("Thresholds flushed tz_id=%d\n", tz_id);
+
+ return 0;
+}
+
+static int threshold_up(int tz_id, int temp, int prev_temp, __maybe_unused void *arg)
+{
+ INFO("Threshold crossed way up tz_id=%d: temp=%d, prev_temp=%d\n",
+ tz_id, temp, prev_temp);
+
+ return 0;
+}
+
+static int threshold_down(int tz_id, int temp, int prev_temp, __maybe_unused void *arg)
+{
+ INFO("Threshold crossed way down tz_id=%d: temp=%d, prev_temp=%d\n",
+ tz_id, temp, prev_temp);
+
+ return 0;
+}
+
static struct thermal_ops ops = {
- .events.tz_create = tz_create,
- .events.tz_delete = tz_delete,
- .events.tz_disable = tz_disable,
- .events.tz_enable = tz_enable,
- .events.trip_high = trip_high,
- .events.trip_low = trip_low,
- .events.trip_add = trip_add,
- .events.trip_delete = trip_delete,
- .events.trip_change = trip_change,
- .events.cdev_add = cdev_add,
- .events.cdev_delete = cdev_delete,
- .events.cdev_update = cdev_update,
- .events.gov_change = gov_change
+ .events.tz_create = tz_create,
+ .events.tz_delete = tz_delete,
+ .events.tz_disable = tz_disable,
+ .events.tz_enable = tz_enable,
+ .events.trip_high = trip_high,
+ .events.trip_low = trip_low,
+ .events.trip_add = trip_add,
+ .events.trip_delete = trip_delete,
+ .events.trip_change = trip_change,
+ .events.cdev_add = cdev_add,
+ .events.cdev_delete = cdev_delete,
+ .events.cdev_update = cdev_update,
+ .events.gov_change = gov_change,
+ .events.threshold_add = threshold_add,
+ .events.threshold_delete = threshold_delete,
+ .events.threshold_flush = threshold_flush,
+ .events.threshold_up = threshold_up,
+ .events.threshold_down = threshold_down,
};
static int thermal_event(__maybe_unused int fd, __maybe_unused void *arg)
@@ -280,6 +356,7 @@ enum {
THERMAL_ENGINE_DAEMON_ERROR,
THERMAL_ENGINE_LOG_ERROR,
THERMAL_ENGINE_THERMAL_ERROR,
+ THERMAL_ENGINE_THRESHOLD_ERROR,
THERMAL_ENGINE_MAINLOOP_ERROR,
};
@@ -318,6 +395,8 @@ int main(int argc, char *argv[])
return THERMAL_ENGINE_THERMAL_ERROR;
}
+ for_each_thermal_zone(td.tz, set_threshold, td.th);
+
for_each_thermal_zone(td.tz, show_tz, td.th);
if (mainloop_init()) {
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/4] Add thermal user thresholds support
2024-10-14 9:43 [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
` (3 preceding siblings ...)
2024-10-14 9:43 ` [PATCH v5 4/4] tools/thermal/thermal-engine: Take into account the thresholds API Daniel Lezcano
@ 2024-10-21 8:28 ` Daniel Lezcano
2024-10-21 8:43 ` Lukasz Luba
2024-10-21 11:02 ` Rafael J. Wysocki
4 siblings, 2 replies; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-21 8:28 UTC (permalink / raw)
To: rafael; +Cc: linux-pm, lukasz.luba, quic_manafm
Hi,
as there are no more comments and I think I took into account all the
feedback, is it possible to merge this series so I can get rid of
monitoring its status ?
Thanks
On 14/10/2024 11:43, Daniel Lezcano wrote:
> The trip points are a firmware description of the temperature limits
> of a specific thermal zone where we associate an action which is done
> by the kernel. The time resolution is low.
>
> The userspace has to deal with a more complex thermal management based
> on heuristics from different information coming from different
> places. The logic is much more complex but based on a bigger time
> resolution, usually one second based.
>
> The purpose of the userspace is to monitor the temperatures from
> different places and take actions. However, it can not be constantly
> reading the temperature to detect when a temperature threshold has
> been reached. This is especially bad for mobile or embedded system as
> that will lead to an unacceptable number of wakeup to check the
> temperature with nothing to do.
>
> On the other side, the sensors are now most of the time interrupt
> driven. That means the thermal framework will use the temperature trip
> points to program the sensor to trigger an interrupt when a
> temperature limit is crossed.
>
> Unfortunately, the userspace can not benefit this feature and current
> solutions found here and there, iow out-of-tree, are to add fake trip
> points in the firmware and enable the writable trip points.
>
> This is bad for different reasons, the trip points are for in-kernel
> actions, the semantic of their types is used by the thermal framework
> and by adding trip points in the device tree is a way to overcome the
> current limitation but tampering with how the thermal framework is
> supposed to work. The writable trip points is a way to adjust a
> temperature limit given a specific platform if the firmware is not
> accurate enough and TBH it is more a debug feature from my POV.
>
> The user thresholds mechanism is a way to have the userspace to tell
> thermal framework to send a notification when a temperature limit is
> crossed. There is no id, no hysteresis, just the temperature and the
> direction of the limit crossing. That means we can be notified when a
> temperature threshold is crossed the way up only, or the way down only
> or both ways. That allows to create hysteresis values if it is needed.
>
> Those thresholds are refered as user thresholds in order to do the
> difference with the trip points which are similar.
>
> An user threshold can be added, deleted or flushed. The latter means
> all user thresholds belonging to a thermal zone will be deleted.
>
> When one or several user thresholds are crossed, an event is sent to
> the userspace.
>
> All aforementioned actions and events lead to a notification to the
> userspace.
>
> Along with the kernel changes, the thermal library has been extended
> to provide the different API to deal with the new user threshold
> netlink events and commands.
>
> In addition, the thermal-engine skeleton uses these new API by
> flushing and adding user thresholds as well as getting the
> notification about these actions.
>
> Overall the series has been tested with the thermal-engine skeleton
> and some selftests which are not part of this series.
>
> Changelog:
> V5:
> - Added CAP_SYS_ADMIN needed capability when adding, deleting and
> flushing a threshold (Rafael)
>
> - Remove the pid information to prevent leaking pid inside
> containers. Also the information is not really needed (Rafael)
>
> - Renamed "THERMAL_GENL_ATTR_THRESHOLD_WAY" to
> "THERMAL_GENL_ATTR_THRESHOLD_DIRECTION". Did not used '*_DIR' as
> suggested initially because it can be ambiguous with 'directory'
> (Rafael)
>
> - Renamed 'last_temp' to 'prev_temp' (Rafael)
>
> - Used CLASS constructor/destructor to get / put the thermal
> zone's device refcount (Rafael)
>
> - Moved locking inside thermal_thresholds_for_each() (Rafael)
>
> - Reflected the changes above in the thermal library and the
> thermal engine skeleton
>
>
> V4:
> - Fix missing stubs when THERMAL_NETLINK=n (kernel test robot)
>
> V3:
> - the first patch of the v2 series has been merged
>
> - Modified the description to split the information between the
> cover letter and the patch 1 description (Rafael)
>
> - Made the thresholds code as part of the core (Rafael)
>
> - Converted the thresholds into a list and directly declared in
> the thermal zone device structure (Rafael)
>
> - Changed the name of the field in the thermal zone device
> structure to user_thresholds (Rafael)
>
> - Added #include "thermal_thresholds.h" (Rafael)
>
> - Combined the conditions in the function
> __thermal_threshold_is_crossed (Rafael)
>
> - Moved the function thermal_thresholds_flush() before
> thermal_thresholds_exit() (Rafael)
>
> - Change thermal_thresholds_handle() to return void (Rafael)
>
> - Move the list field on top the of the structure threshold and
> renamed it list_node (Rafael)
>
> - Changed THERMAL_THRESHOLD_* notifications to
> THERMAL_TZ_THRESHOLD_* (Rafael)
>
> V2:
> - Compute min and max in thermal_zone_device_update() but keep
> the loop as it is (Rafael)
>
> - Include slab.h to fix compilation warnings on some architectures
> with kmalloc and kfree (kernel test robot)
>
> Daniel Lezcano (4):
> thermal/netlink: Add the commands and the events for the thresholds
> tools/lib/thermal: Make more generic the command encoding function
> tools/lib/thermal: Add the threshold netlink ABI
> tools/thermal/thermal-engine: Take into account the thresholds API
>
> drivers/thermal/thermal_netlink.c | 236 +++++++++++++++++-
> drivers/thermal/thermal_netlink.h | 34 +++
> drivers/thermal/thermal_thresholds.c | 36 +--
> drivers/thermal/thermal_thresholds.h | 2 +-
> include/uapi/linux/thermal.h | 27 +-
> tools/lib/thermal/commands.c | 167 ++++++++++++-
> tools/lib/thermal/events.c | 55 +++-
> tools/lib/thermal/include/thermal.h | 40 +++
> tools/lib/thermal/libthermal.map | 5 +
> tools/lib/thermal/thermal.c | 17 ++
> tools/thermal/lib/Makefile | 2 +-
> tools/thermal/thermal-engine/thermal-engine.c | 105 +++++++-
> 12 files changed, 662 insertions(+), 64 deletions(-)
>
--
<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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/4] Add thermal user thresholds support
2024-10-21 8:28 ` [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
@ 2024-10-21 8:43 ` Lukasz Luba
2024-10-21 11:02 ` Rafael J. Wysocki
1 sibling, 0 replies; 26+ messages in thread
From: Lukasz Luba @ 2024-10-21 8:43 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-pm, quic_manafm, rafael
Hi Daniel,
On 10/21/24 09:28, Daniel Lezcano wrote:
>
> Hi,
>
> as there are no more comments and I think I took into account all the
> feedback, is it possible to merge this series so I can get rid of
> monitoring its status ?
>
My apologies, give me 1 day. Today I will test it and review.
I've seen some previous version and haven't seen any major issues,
so this one should be good IMO.
Regards,
Lukasz
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-14 9:43 ` [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds Daniel Lezcano
@ 2024-10-21 10:58 ` Rafael J. Wysocki
2024-10-21 19:42 ` Lukasz Luba
2024-10-21 22:02 ` Lukasz Luba
2 siblings, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2024-10-21 10:58 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: rafael, linux-pm, lukasz.luba, quic_manafm
On Mon, Oct 14, 2024 at 11:43 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The thresholds exist but there is no notification neither action code
> related to them yet.
>
> These changes implement the netlink for the notifications when the
> thresholds are crossed, added, deleted or flushed as well as the
> commands which allows to get the list of the thresholds, flush them,
> add and delete.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/thermal/thermal_netlink.c | 236 ++++++++++++++++++++++++++-
> drivers/thermal/thermal_netlink.h | 34 ++++
> drivers/thermal/thermal_thresholds.c | 36 ++--
> drivers/thermal/thermal_thresholds.h | 2 +-
> include/uapi/linux/thermal.h | 27 ++-
> 5 files changed, 307 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
> index f3c58c708969..978cd46c15e8 100644
> --- a/drivers/thermal/thermal_netlink.c
> +++ b/drivers/thermal/thermal_netlink.c
> @@ -9,6 +9,7 @@
> #include <linux/module.h>
> #include <linux/notifier.h>
> #include <linux/kernel.h>
> +#include <net/sock.h>
> #include <net/genetlink.h>
> #include <uapi/linux/thermal.h>
>
> @@ -49,6 +50,11 @@ static const struct nla_policy thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] =
> [THERMAL_GENL_ATTR_CPU_CAPABILITY_ID] = { .type = NLA_U32 },
> [THERMAL_GENL_ATTR_CPU_CAPABILITY_PERFORMANCE] = { .type = NLA_U32 },
> [THERMAL_GENL_ATTR_CPU_CAPABILITY_EFFICIENCY] = { .type = NLA_U32 },
> +
> + /* Thresholds */
> + [THERMAL_GENL_ATTR_THRESHOLD] = { .type = NLA_NESTED },
> + [THERMAL_GENL_ATTR_THRESHOLD_TEMP] = { .type = NLA_U32 },
> + [THERMAL_GENL_ATTR_THRESHOLD_DIRECTION] = { .type = NLA_U32 },
> };
>
> struct param {
> @@ -62,6 +68,8 @@ struct param {
> int trip_type;
> int trip_hyst;
> int temp;
> + int prev_temp;
> + int direction;
> int cdev_state;
> int cdev_max_state;
> struct thermal_genl_cpu_caps *cpu_capabilities;
> @@ -234,6 +242,34 @@ static int thermal_genl_event_cpu_capability_change(struct param *p)
> return -EMSGSIZE;
> }
>
> +static int thermal_genl_event_threshold_add(struct param *p)
> +{
> + if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
> + nla_put_u32(p->msg, THERMAL_GENL_ATTR_THRESHOLD_TEMP, p->temp) ||
> + nla_put_u32(p->msg, THERMAL_GENL_ATTR_THRESHOLD_DIRECTION, p->direction))
> + return -EMSGSIZE;
> +
> + return 0;
> +}
> +
> +static int thermal_genl_event_threshold_flush(struct param *p)
> +{
> + if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
> + return -EMSGSIZE;
> +
> + return 0;
> +}
> +
> +static int thermal_genl_event_threshold_up(struct param *p)
> +{
> + if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
> + nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_PREV_TEMP, p->prev_temp) ||
> + nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TEMP, p->temp))
> + return -EMSGSIZE;
> +
> + return 0;
> +}
> +
> int thermal_genl_event_tz_delete(struct param *p)
> __attribute__((alias("thermal_genl_event_tz")));
>
> @@ -246,6 +282,12 @@ int thermal_genl_event_tz_disable(struct param *p)
> int thermal_genl_event_tz_trip_down(struct param *p)
> __attribute__((alias("thermal_genl_event_tz_trip_up")));
>
> +int thermal_genl_event_threshold_delete(struct param *p)
> + __attribute__((alias("thermal_genl_event_threshold_add")));
> +
> +int thermal_genl_event_threshold_down(struct param *p)
> + __attribute__((alias("thermal_genl_event_threshold_up")));
> +
> static cb_t event_cb[] = {
> [THERMAL_GENL_EVENT_TZ_CREATE] = thermal_genl_event_tz_create,
> [THERMAL_GENL_EVENT_TZ_DELETE] = thermal_genl_event_tz_delete,
> @@ -259,6 +301,11 @@ static cb_t event_cb[] = {
> [THERMAL_GENL_EVENT_CDEV_STATE_UPDATE] = thermal_genl_event_cdev_state_update,
> [THERMAL_GENL_EVENT_TZ_GOV_CHANGE] = thermal_genl_event_gov_change,
> [THERMAL_GENL_EVENT_CPU_CAPABILITY_CHANGE] = thermal_genl_event_cpu_capability_change,
> + [THERMAL_GENL_EVENT_THRESHOLD_ADD] = thermal_genl_event_threshold_add,
> + [THERMAL_GENL_EVENT_THRESHOLD_DELETE] = thermal_genl_event_threshold_delete,
> + [THERMAL_GENL_EVENT_THRESHOLD_FLUSH] = thermal_genl_event_threshold_flush,
> + [THERMAL_GENL_EVENT_THRESHOLD_DOWN] = thermal_genl_event_threshold_down,
> + [THERMAL_GENL_EVENT_THRESHOLD_UP] = thermal_genl_event_threshold_up,
> };
>
> /*
> @@ -401,6 +448,43 @@ int thermal_genl_cpu_capability_event(int count,
> }
> EXPORT_SYMBOL_GPL(thermal_genl_cpu_capability_event);
>
> +int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
> + int temperature, int direction)
> +{
> + struct param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
> +
> + return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_ADD, &p);
> +}
> +
> +int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
> + int temperature, int direction)
> +{
> + struct param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
> +
> + return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_DELETE, &p);
> +}
> +
> +int thermal_notify_threshold_flush(const struct thermal_zone_device *tz)
> +{
> + struct param p = { .tz_id = tz->id };
> +
> + return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_FLUSH, &p);
> +}
> +
> +int thermal_notify_threshold_down(const struct thermal_zone_device *tz)
> +{
> + struct param p = { .tz_id = tz->id, .temp = tz->temperature, .prev_temp = tz->last_temperature };
> +
> + return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_DOWN, &p);
> +}
> +
> +int thermal_notify_threshold_up(const struct thermal_zone_device *tz)
> +{
> + struct param p = { .tz_id = tz->id, .temp = tz->temperature, .prev_temp = tz->last_temperature };
> +
> + return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_UP, &p);
> +}
> +
> /*************************** Command encoding ********************************/
>
> static int __thermal_genl_cmd_tz_get_id(struct thermal_zone_device *tz,
> @@ -572,12 +656,132 @@ static int thermal_genl_cmd_cdev_get(struct param *p)
> return ret;
> }
>
> +static int __thermal_genl_cmd_threshold_get(struct user_threshold *threshold, void *arg)
> +{
> + struct sk_buff *msg = arg;
> +
> + if (nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_TEMP, threshold->temperature) ||
> + nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_DIRECTION, threshold->direction))
> + return -1;
> +
> + return 0;
> +}
> +
> +static int thermal_genl_cmd_threshold_get(struct param *p)
> +{
> + struct sk_buff *msg = p->msg;
> + struct nlattr *start_trip;
> + int id, ret;
> +
> + if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
> + return -EINVAL;
> +
> + id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> +
> + CLASS(thermal_zone_get_by_id, tz)(id);
> + if (!tz)
> + return -EINVAL;
> +
> + start_trip = nla_nest_start(msg, THERMAL_GENL_ATTR_THRESHOLD);
> + if (!start_trip)
> + return -EMSGSIZE;
> +
> + ret = thermal_thresholds_for_each(tz, __thermal_genl_cmd_threshold_get, msg);
> + if (ret)
> + return -EMSGSIZE;
> +
> + nla_nest_end(msg, start_trip);
> +
> + return 0;
> +}
> +
> +static int thermal_genl_cmd_threshold_add(struct param *p)
> +{
> + int id, temp, direction, ret = 0;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID] ||
> + !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP] ||
> + !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION])
> + return -EINVAL;
> +
> + id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> + temp = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]);
> + direction = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]);
> +
> + CLASS(thermal_zone_get_by_id, tz)(id);
> + if (!tz)
> + return -EINVAL;
> +
> + mutex_lock(&tz->lock);
> + ret = thermal_thresholds_add(tz, temp, direction);
> + mutex_unlock(&tz->lock);
> +
> + return ret;
> +}
> +
> +static int thermal_genl_cmd_threshold_delete(struct param *p)
> +{
> + int id, temp, direction, ret = 0;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID] ||
> + !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP] ||
> + !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION])
> + return -EINVAL;
> +
> + id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> + temp = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]);
> + direction = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]);
> +
> + CLASS(thermal_zone_get_by_id, tz)(id);
> + if (!tz)
> + return -EINVAL;
> +
> + mutex_lock(&tz->lock);
> + ret = thermal_thresholds_delete(tz, temp, direction);
> + mutex_unlock(&tz->lock);
> +
> + return ret;
> +}
> +
> +static int thermal_genl_cmd_threshold_flush(struct param *p)
> +{
> + int id;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
> + return -EINVAL;
> +
> + id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> +
> + CLASS(thermal_zone_get_by_id, tz)(id);
> + if (!tz)
> + return -EINVAL;
> +
> + mutex_lock(&tz->lock);
> + thermal_thresholds_flush(tz);
> + mutex_unlock(&tz->lock);
> +
> + return 0;
> +}
> +
> static cb_t cmd_cb[] = {
> - [THERMAL_GENL_CMD_TZ_GET_ID] = thermal_genl_cmd_tz_get_id,
> - [THERMAL_GENL_CMD_TZ_GET_TRIP] = thermal_genl_cmd_tz_get_trip,
> - [THERMAL_GENL_CMD_TZ_GET_TEMP] = thermal_genl_cmd_tz_get_temp,
> - [THERMAL_GENL_CMD_TZ_GET_GOV] = thermal_genl_cmd_tz_get_gov,
> - [THERMAL_GENL_CMD_CDEV_GET] = thermal_genl_cmd_cdev_get,
> + [THERMAL_GENL_CMD_TZ_GET_ID] = thermal_genl_cmd_tz_get_id,
> + [THERMAL_GENL_CMD_TZ_GET_TRIP] = thermal_genl_cmd_tz_get_trip,
> + [THERMAL_GENL_CMD_TZ_GET_TEMP] = thermal_genl_cmd_tz_get_temp,
> + [THERMAL_GENL_CMD_TZ_GET_GOV] = thermal_genl_cmd_tz_get_gov,
> + [THERMAL_GENL_CMD_CDEV_GET] = thermal_genl_cmd_cdev_get,
> + [THERMAL_GENL_CMD_THRESHOLD_GET] = thermal_genl_cmd_threshold_get,
> + [THERMAL_GENL_CMD_THRESHOLD_ADD] = thermal_genl_cmd_threshold_add,
> + [THERMAL_GENL_CMD_THRESHOLD_DELETE] = thermal_genl_cmd_threshold_delete,
> + [THERMAL_GENL_CMD_THRESHOLD_FLUSH] = thermal_genl_cmd_threshold_flush,
> };
>
> static int thermal_genl_cmd_dumpit(struct sk_buff *skb,
> @@ -688,6 +892,26 @@ static const struct genl_small_ops thermal_genl_ops[] = {
> .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> .dumpit = thermal_genl_cmd_dumpit,
> },
> + {
> + .cmd = THERMAL_GENL_CMD_THRESHOLD_GET,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = thermal_genl_cmd_doit,
> + },
> + {
> + .cmd = THERMAL_GENL_CMD_THRESHOLD_ADD,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = thermal_genl_cmd_doit,
> + },
> + {
> + .cmd = THERMAL_GENL_CMD_THRESHOLD_DELETE,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = thermal_genl_cmd_doit,
> + },
> + {
> + .cmd = THERMAL_GENL_CMD_THRESHOLD_FLUSH,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = thermal_genl_cmd_doit,
> + },
> };
>
> static struct genl_family thermal_genl_family __ro_after_init = {
> @@ -700,7 +924,7 @@ static struct genl_family thermal_genl_family __ro_after_init = {
> .unbind = thermal_genl_unbind,
> .small_ops = thermal_genl_ops,
> .n_small_ops = ARRAY_SIZE(thermal_genl_ops),
> - .resv_start_op = THERMAL_GENL_CMD_CDEV_GET + 1,
> + .resv_start_op = __THERMAL_GENL_CMD_MAX,
> .mcgrps = thermal_genl_mcgrps,
> .n_mcgrps = ARRAY_SIZE(thermal_genl_mcgrps),
> };
> diff --git a/drivers/thermal/thermal_netlink.h b/drivers/thermal/thermal_netlink.h
> index e01221e8816b..075e9ae85f3d 100644
> --- a/drivers/thermal/thermal_netlink.h
> +++ b/drivers/thermal/thermal_netlink.h
> @@ -53,6 +53,13 @@ int thermal_notify_tz_gov_change(const struct thermal_zone_device *tz,
> int thermal_genl_sampling_temp(int id, int temp);
> int thermal_genl_cpu_capability_event(int count,
> struct thermal_genl_cpu_caps *caps);
> +int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
> + int temperature, int direction);
> +int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
> + int temperature, int direction);
> +int thermal_notify_threshold_flush(const struct thermal_zone_device *tz);
> +int thermal_notify_threshold_down(const struct thermal_zone_device *tz);
> +int thermal_notify_threshold_up(const struct thermal_zone_device *tz);
> #else
> static inline int thermal_netlink_init(void)
> {
> @@ -139,6 +146,33 @@ static inline int thermal_genl_cpu_capability_event(int count, struct thermal_ge
> return 0;
> }
>
> +static inline int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
> + int temperature, int direction)
> +{
> + return 0;
> +}
> +
> +static inline int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
> + int temperature, int direction)
> +{
> + return 0;
> +}
> +
> +static inline int thermal_notify_threshold_flush(const struct thermal_zone_device *tz)
> +{
> + return 0;
> +}
> +
> +static inline int thermal_notify_threshold_down(const struct thermal_zone_device *tz)
> +{
> + return 0;
> +}
> +
> +static inline int thermal_notify_threshold_up(const struct thermal_zone_device *tz)
> +{
> + return 0;
> +}
> +
> static inline void __init thermal_netlink_exit(void) {}
>
> #endif /* CONFIG_THERMAL_NETLINK */
> diff --git a/drivers/thermal/thermal_thresholds.c b/drivers/thermal/thermal_thresholds.c
> index f33b6d5474d8..ea4aa5a2e86c 100644
> --- a/drivers/thermal/thermal_thresholds.c
> +++ b/drivers/thermal/thermal_thresholds.c
> @@ -32,6 +32,8 @@ void thermal_thresholds_flush(struct thermal_zone_device *tz)
> kfree(entry);
> }
>
> + thermal_notify_threshold_flush(tz);
> +
> __thermal_zone_device_update(tz, THERMAL_TZ_FLUSH_THRESHOLDS);
> }
>
> @@ -122,7 +124,6 @@ void thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *hi
>
> int temperature = tz->temperature;
> int last_temperature = tz->last_temperature;
> - bool notify;
>
> lockdep_assert_held(&tz->lock);
>
> @@ -144,19 +145,19 @@ void thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *hi
> * - increased : thresholds are crossed the way up
> * - decreased : thresholds are crossed the way down
> */
> - if (temperature > last_temperature)
> - notify = thermal_thresholds_handle_raising(thresholds, temperature,
> - last_temperature, low, high);
> - else
> - notify = thermal_thresholds_handle_dropping(thresholds, temperature,
> - last_temperature, low, high);
> -
> - if (notify)
> - pr_debug("A threshold has been crossed the way %s, with a temperature=%d, last_temperature=%d\n",
> - temperature > last_temperature ? "up" : "down", temperature, last_temperature);
> + if (temperature > last_temperature) {
> + if (thermal_thresholds_handle_raising(thresholds, temperature,
> + last_temperature, low, high))
> + thermal_notify_threshold_up(tz);
> + } else {
> + if (thermal_thresholds_handle_dropping(thresholds, temperature,
> + last_temperature, low, high))
> + thermal_notify_threshold_down(tz);
> + }
> }
>
> -int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
> +int thermal_thresholds_add(struct thermal_zone_device *tz,
> + int temperature, int direction)
> {
> struct list_head *thresholds = &tz->user_thresholds;
> struct user_threshold *t;
> @@ -182,12 +183,15 @@ int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int
> list_sort(NULL, thresholds, __thermal_thresholds_cmp);
> }
>
> + thermal_notify_threshold_add(tz, temperature, direction);
> +
> __thermal_zone_device_update(tz, THERMAL_TZ_ADD_THRESHOLD);
>
> return 0;
> }
>
> -int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
> +int thermal_thresholds_delete(struct thermal_zone_device *tz,
> + int temperature, int direction)
> {
> struct list_head *thresholds = &tz->user_thresholds;
> struct user_threshold *t;
> @@ -205,6 +209,8 @@ int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, i
> t->direction &= ~direction;
> }
>
> + thermal_notify_threshold_delete(tz, temperature, direction);
> +
> __thermal_zone_device_update(tz, THERMAL_TZ_DEL_THRESHOLD);
>
> return 0;
> @@ -217,7 +223,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
> struct user_threshold *entry;
> int ret;
>
> - lockdep_assert_held(&tz->lock);
> + mutex_lock(&tz->lock);
>
> list_for_each_entry(entry, thresholds, list_node) {
> ret = cb(entry, arg);
> @@ -225,5 +231,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
> return ret;
The lock needs to be released before returning.
However, I would generally prefer this to go in after
https://lore.kernel.org/linux-pm/4985597.31r3eYUQgx@rjwysocki.net/
so I'll convert it to using guards before applying if you don't mind
and this issue will go away then.
Otherwise the patch looks good to me.
> }
>
> + mutex_unlock(&tz->lock);
> +
> return 0;
> }
> diff --git a/drivers/thermal/thermal_thresholds.h b/drivers/thermal/thermal_thresholds.h
> index 232f4e8089af..cb372659a20d 100644
> --- a/drivers/thermal/thermal_thresholds.h
> +++ b/drivers/thermal/thermal_thresholds.h
> @@ -10,8 +10,8 @@ struct user_threshold {
>
> int thermal_thresholds_init(struct thermal_zone_device *tz);
> void thermal_thresholds_exit(struct thermal_zone_device *tz);
> -void thermal_thresholds_flush(struct thermal_zone_device *tz);
> void thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high);
> +void thermal_thresholds_flush(struct thermal_zone_device *tz);
> int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction);
> int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction);
> int thermal_thresholds_for_each(struct thermal_zone_device *tz,
> diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
> index 2e6f60a36173..ba8604bdf206 100644
> --- a/include/uapi/linux/thermal.h
> +++ b/include/uapi/linux/thermal.h
> @@ -20,7 +20,7 @@ enum thermal_trip_type {
>
> /* Adding event notification support elements */
> #define THERMAL_GENL_FAMILY_NAME "thermal"
> -#define THERMAL_GENL_VERSION 0x01
> +#define THERMAL_GENL_VERSION 0x02
> #define THERMAL_GENL_SAMPLING_GROUP_NAME "sampling"
> #define THERMAL_GENL_EVENT_GROUP_NAME "event"
>
> @@ -30,6 +30,7 @@ enum thermal_genl_attr {
> THERMAL_GENL_ATTR_TZ,
> THERMAL_GENL_ATTR_TZ_ID,
> THERMAL_GENL_ATTR_TZ_TEMP,
> + THERMAL_GENL_ATTR_TZ_PREV_TEMP,
> THERMAL_GENL_ATTR_TZ_TRIP,
> THERMAL_GENL_ATTR_TZ_TRIP_ID,
> THERMAL_GENL_ATTR_TZ_TRIP_TYPE,
> @@ -50,6 +51,9 @@ enum thermal_genl_attr {
> THERMAL_GENL_ATTR_CPU_CAPABILITY_ID,
> THERMAL_GENL_ATTR_CPU_CAPABILITY_PERFORMANCE,
> THERMAL_GENL_ATTR_CPU_CAPABILITY_EFFICIENCY,
> + THERMAL_GENL_ATTR_THRESHOLD,
> + THERMAL_GENL_ATTR_THRESHOLD_TEMP,
> + THERMAL_GENL_ATTR_THRESHOLD_DIRECTION,
> __THERMAL_GENL_ATTR_MAX,
> };
> #define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
> @@ -77,6 +81,11 @@ enum thermal_genl_event {
> THERMAL_GENL_EVENT_CDEV_STATE_UPDATE, /* Cdev state updated */
> THERMAL_GENL_EVENT_TZ_GOV_CHANGE, /* Governor policy changed */
> THERMAL_GENL_EVENT_CPU_CAPABILITY_CHANGE, /* CPU capability changed */
> + THERMAL_GENL_EVENT_THRESHOLD_ADD, /* A thresold has been added */
> + THERMAL_GENL_EVENT_THRESHOLD_DELETE, /* A thresold has been deleted */
> + THERMAL_GENL_EVENT_THRESHOLD_FLUSH, /* All thresolds have been deleted */
> + THERMAL_GENL_EVENT_THRESHOLD_UP, /* A thresold has been crossed the way up */
> + THERMAL_GENL_EVENT_THRESHOLD_DOWN, /* A thresold has been crossed the way down */
> __THERMAL_GENL_EVENT_MAX,
> };
> #define THERMAL_GENL_EVENT_MAX (__THERMAL_GENL_EVENT_MAX - 1)
> @@ -84,12 +93,16 @@ enum thermal_genl_event {
> /* Commands supported by the thermal_genl_family */
> enum thermal_genl_cmd {
> THERMAL_GENL_CMD_UNSPEC,
> - THERMAL_GENL_CMD_TZ_GET_ID, /* List of thermal zones id */
> - THERMAL_GENL_CMD_TZ_GET_TRIP, /* List of thermal trips */
> - THERMAL_GENL_CMD_TZ_GET_TEMP, /* Get the thermal zone temperature */
> - THERMAL_GENL_CMD_TZ_GET_GOV, /* Get the thermal zone governor */
> - THERMAL_GENL_CMD_TZ_GET_MODE, /* Get the thermal zone mode */
> - THERMAL_GENL_CMD_CDEV_GET, /* List of cdev id */
> + THERMAL_GENL_CMD_TZ_GET_ID, /* List of thermal zones id */
> + THERMAL_GENL_CMD_TZ_GET_TRIP, /* List of thermal trips */
> + THERMAL_GENL_CMD_TZ_GET_TEMP, /* Get the thermal zone temperature */
> + THERMAL_GENL_CMD_TZ_GET_GOV, /* Get the thermal zone governor */
> + THERMAL_GENL_CMD_TZ_GET_MODE, /* Get the thermal zone mode */
> + THERMAL_GENL_CMD_CDEV_GET, /* List of cdev id */
> + THERMAL_GENL_CMD_THRESHOLD_GET, /* List of thresholds */
> + THERMAL_GENL_CMD_THRESHOLD_ADD, /* Add a threshold */
> + THERMAL_GENL_CMD_THRESHOLD_DELETE, /* Delete a threshold */
> + THERMAL_GENL_CMD_THRESHOLD_FLUSH, /* Flush all the thresholds */
> __THERMAL_GENL_CMD_MAX,
> };
> #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/4] Add thermal user thresholds support
2024-10-21 8:28 ` [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
2024-10-21 8:43 ` Lukasz Luba
@ 2024-10-21 11:02 ` Rafael J. Wysocki
1 sibling, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2024-10-21 11:02 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: rafael, linux-pm, lukasz.luba, quic_manafm
Hi Daniel,
On Mon, Oct 21, 2024 at 10:28 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi,
>
> as there are no more comments and I think I took into account all the
> feedback, is it possible to merge this series so I can get rid of
> monitoring its status ?
Please see my reply to the first patch.
Apart from a locking issue in thermal_thresholds_for_each() that will
go away when the thermal_zone guard is used in this function, this
looks good to me.
Of course, I'll give Lukasz the time to review and test it before it
gets applied, though.
> On 14/10/2024 11:43, Daniel Lezcano wrote:
> > The trip points are a firmware description of the temperature limits
> > of a specific thermal zone where we associate an action which is done
> > by the kernel. The time resolution is low.
> >
> > The userspace has to deal with a more complex thermal management based
> > on heuristics from different information coming from different
> > places. The logic is much more complex but based on a bigger time
> > resolution, usually one second based.
> >
> > The purpose of the userspace is to monitor the temperatures from
> > different places and take actions. However, it can not be constantly
> > reading the temperature to detect when a temperature threshold has
> > been reached. This is especially bad for mobile or embedded system as
> > that will lead to an unacceptable number of wakeup to check the
> > temperature with nothing to do.
> >
> > On the other side, the sensors are now most of the time interrupt
> > driven. That means the thermal framework will use the temperature trip
> > points to program the sensor to trigger an interrupt when a
> > temperature limit is crossed.
> >
> > Unfortunately, the userspace can not benefit this feature and current
> > solutions found here and there, iow out-of-tree, are to add fake trip
> > points in the firmware and enable the writable trip points.
> >
> > This is bad for different reasons, the trip points are for in-kernel
> > actions, the semantic of their types is used by the thermal framework
> > and by adding trip points in the device tree is a way to overcome the
> > current limitation but tampering with how the thermal framework is
> > supposed to work. The writable trip points is a way to adjust a
> > temperature limit given a specific platform if the firmware is not
> > accurate enough and TBH it is more a debug feature from my POV.
> >
> > The user thresholds mechanism is a way to have the userspace to tell
> > thermal framework to send a notification when a temperature limit is
> > crossed. There is no id, no hysteresis, just the temperature and the
> > direction of the limit crossing. That means we can be notified when a
> > temperature threshold is crossed the way up only, or the way down only
> > or both ways. That allows to create hysteresis values if it is needed.
> >
> > Those thresholds are refered as user thresholds in order to do the
> > difference with the trip points which are similar.
> >
> > An user threshold can be added, deleted or flushed. The latter means
> > all user thresholds belonging to a thermal zone will be deleted.
> >
> > When one or several user thresholds are crossed, an event is sent to
> > the userspace.
> >
> > All aforementioned actions and events lead to a notification to the
> > userspace.
> >
> > Along with the kernel changes, the thermal library has been extended
> > to provide the different API to deal with the new user threshold
> > netlink events and commands.
> >
> > In addition, the thermal-engine skeleton uses these new API by
> > flushing and adding user thresholds as well as getting the
> > notification about these actions.
> >
> > Overall the series has been tested with the thermal-engine skeleton
> > and some selftests which are not part of this series.
> >
> > Changelog:
> > V5:
> > - Added CAP_SYS_ADMIN needed capability when adding, deleting and
> > flushing a threshold (Rafael)
> >
> > - Remove the pid information to prevent leaking pid inside
> > containers. Also the information is not really needed (Rafael)
> >
> > - Renamed "THERMAL_GENL_ATTR_THRESHOLD_WAY" to
> > "THERMAL_GENL_ATTR_THRESHOLD_DIRECTION". Did not used '*_DIR' as
> > suggested initially because it can be ambiguous with 'directory'
> > (Rafael)
> >
> > - Renamed 'last_temp' to 'prev_temp' (Rafael)
> >
> > - Used CLASS constructor/destructor to get / put the thermal
> > zone's device refcount (Rafael)
> >
> > - Moved locking inside thermal_thresholds_for_each() (Rafael)
> >
> > - Reflected the changes above in the thermal library and the
> > thermal engine skeleton
> >
> >
> > V4:
> > - Fix missing stubs when THERMAL_NETLINK=n (kernel test robot)
> >
> > V3:
> > - the first patch of the v2 series has been merged
> >
> > - Modified the description to split the information between the
> > cover letter and the patch 1 description (Rafael)
> >
> > - Made the thresholds code as part of the core (Rafael)
> >
> > - Converted the thresholds into a list and directly declared in
> > the thermal zone device structure (Rafael)
> >
> > - Changed the name of the field in the thermal zone device
> > structure to user_thresholds (Rafael)
> >
> > - Added #include "thermal_thresholds.h" (Rafael)
> >
> > - Combined the conditions in the function
> > __thermal_threshold_is_crossed (Rafael)
> >
> > - Moved the function thermal_thresholds_flush() before
> > thermal_thresholds_exit() (Rafael)
> >
> > - Change thermal_thresholds_handle() to return void (Rafael)
> >
> > - Move the list field on top the of the structure threshold and
> > renamed it list_node (Rafael)
> >
> > - Changed THERMAL_THRESHOLD_* notifications to
> > THERMAL_TZ_THRESHOLD_* (Rafael)
> >
> > V2:
> > - Compute min and max in thermal_zone_device_update() but keep
> > the loop as it is (Rafael)
> >
> > - Include slab.h to fix compilation warnings on some architectures
> > with kmalloc and kfree (kernel test robot)
> >
> > Daniel Lezcano (4):
> > thermal/netlink: Add the commands and the events for the thresholds
> > tools/lib/thermal: Make more generic the command encoding function
> > tools/lib/thermal: Add the threshold netlink ABI
> > tools/thermal/thermal-engine: Take into account the thresholds API
> >
> > drivers/thermal/thermal_netlink.c | 236 +++++++++++++++++-
> > drivers/thermal/thermal_netlink.h | 34 +++
> > drivers/thermal/thermal_thresholds.c | 36 +--
> > drivers/thermal/thermal_thresholds.h | 2 +-
> > include/uapi/linux/thermal.h | 27 +-
> > tools/lib/thermal/commands.c | 167 ++++++++++++-
> > tools/lib/thermal/events.c | 55 +++-
> > tools/lib/thermal/include/thermal.h | 40 +++
> > tools/lib/thermal/libthermal.map | 5 +
> > tools/lib/thermal/thermal.c | 17 ++
> > tools/thermal/lib/Makefile | 2 +-
> > tools/thermal/thermal-engine/thermal-engine.c | 105 +++++++-
> > 12 files changed, 662 insertions(+), 64 deletions(-)
> >
>
>
> --
> <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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-14 9:43 ` [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds Daniel Lezcano
2024-10-21 10:58 ` Rafael J. Wysocki
@ 2024-10-21 19:42 ` Lukasz Luba
2024-10-21 19:47 ` Rafael J. Wysocki
2024-10-21 22:02 ` Lukasz Luba
2 siblings, 1 reply; 26+ messages in thread
From: Lukasz Luba @ 2024-10-21 19:42 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-pm, quic_manafm, rafael
Hi Daniel,
On 10/14/24 10:43, Daniel Lezcano wrote:
> The thresholds exist but there is no notification neither action code
> related to them yet.
>
> These changes implement the netlink for the notifications when the
> thresholds are crossed, added, deleted or flushed as well as the
> commands which allows to get the list of the thresholds, flush them,
> add and delete.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/thermal/thermal_netlink.c | 236 ++++++++++++++++++++++++++-
> drivers/thermal/thermal_netlink.h | 34 ++++
> drivers/thermal/thermal_thresholds.c | 36 ++--
> drivers/thermal/thermal_thresholds.h | 2 +-
> include/uapi/linux/thermal.h | 27 ++-
> 5 files changed, 307 insertions(+), 28 deletions(-)
>
Since Rafael asked to wait a bit for the other
changes to go first, I will skip the detailed review
of that patch. I will do that after you add that
new locking scheme.
In general the code looks good.
Regards,
Lukasz
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-21 19:42 ` Lukasz Luba
@ 2024-10-21 19:47 ` Rafael J. Wysocki
2024-10-21 19:51 ` Lukasz Luba
0 siblings, 1 reply; 26+ messages in thread
From: Rafael J. Wysocki @ 2024-10-21 19:47 UTC (permalink / raw)
To: Lukasz Luba; +Cc: Daniel Lezcano, linux-pm, quic_manafm, rafael
On Mon, Oct 21, 2024 at 9:41 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Daniel,
>
> On 10/14/24 10:43, Daniel Lezcano wrote:
> > The thresholds exist but there is no notification neither action code
> > related to them yet.
> >
> > These changes implement the netlink for the notifications when the
> > thresholds are crossed, added, deleted or flushed as well as the
> > commands which allows to get the list of the thresholds, flush them,
> > add and delete.
> >
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> > drivers/thermal/thermal_netlink.c | 236 ++++++++++++++++++++++++++-
> > drivers/thermal/thermal_netlink.h | 34 ++++
> > drivers/thermal/thermal_thresholds.c | 36 ++--
> > drivers/thermal/thermal_thresholds.h | 2 +-
> > include/uapi/linux/thermal.h | 27 ++-
> > 5 files changed, 307 insertions(+), 28 deletions(-)
> >
>
> Since Rafael asked to wait a bit for the other
> changes to go first, I will skip the detailed review
> of that patch. I will do that after you add that
> new locking scheme.
Well, this is almost orthogonal to my locking changes and can be
easily rebased on top of them, so I don't see a need to respin it.
Please review.
> In general the code looks good.
OK
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/4] tools/lib/thermal: Make more generic the command encoding function
2024-10-14 9:43 ` [PATCH v5 2/4] tools/lib/thermal: Make more generic the command encoding function Daniel Lezcano
@ 2024-10-21 19:49 ` Lukasz Luba
2024-10-22 7:12 ` Daniel Lezcano
0 siblings, 1 reply; 26+ messages in thread
From: Lukasz Luba @ 2024-10-21 19:49 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-pm, quic_manafm, rafael
On 10/14/24 10:43, Daniel Lezcano wrote:
> The thermal netlink has been extended with more commands which require
> an encoding with more information. The generic encoding function puts
> the thermal zone id with the command name. It is the unique
> parameters.
>
> The next changes will provide more parameters to the command. Set the
> scene for those new parameters by making the encoding function more
> generic.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> tools/lib/thermal/commands.c | 41 ++++++++++++++++++++++++++++--------
> 1 file changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
> index 73d4d4e8d6ec..a9223df91dcf 100644
> --- a/tools/lib/thermal/commands.c
> +++ b/tools/lib/thermal/commands.c
> @@ -261,8 +261,23 @@ static struct genl_ops thermal_cmd_ops = {
> .o_ncmds = ARRAY_SIZE(thermal_cmds),
> };
>
> -static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int cmd,
> - int flags, void *arg)
> +struct cmd_param {
> + int tz_id;
> +};
> +
> +typedef int (*cmd_cb_t)(struct nl_msg *, struct cmd_param *);
> +
> +static int thermal_genl_tz_id_encode(struct nl_msg *msg, struct cmd_param *p)
> +{
> + if (p->tz_id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
> + return -1;
> +
> + return 0;
> +}
> +
> +static thermal_error_t thermal_genl_auto(struct thermal_handler *th, cmd_cb_t cmd_cb,
> + struct cmd_param *param,
> + int cmd, int flags, void *arg)
> {
> struct nl_msg *msg;
> void *hdr;
> @@ -276,7 +291,7 @@ static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int
> if (!hdr)
> return THERMAL_ERROR;
>
> - if (id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id))
> + if (cmd_cb && cmd_cb(msg, param))
> return THERMAL_ERROR;
It's not in this code but also in older:
shouldn't we free the nlmsg_free(msg); before returns in this
function?
The rest design looks good
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-21 19:47 ` Rafael J. Wysocki
@ 2024-10-21 19:51 ` Lukasz Luba
0 siblings, 0 replies; 26+ messages in thread
From: Lukasz Luba @ 2024-10-21 19:51 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Daniel Lezcano, linux-pm, quic_manafm
On 10/21/24 20:47, Rafael J. Wysocki wrote:
> On Mon, Oct 21, 2024 at 9:41 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Daniel,
>>
>> On 10/14/24 10:43, Daniel Lezcano wrote:
>>> The thresholds exist but there is no notification neither action code
>>> related to them yet.
>>>
>>> These changes implement the netlink for the notifications when the
>>> thresholds are crossed, added, deleted or flushed as well as the
>>> commands which allows to get the list of the thresholds, flush them,
>>> add and delete.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>> drivers/thermal/thermal_netlink.c | 236 ++++++++++++++++++++++++++-
>>> drivers/thermal/thermal_netlink.h | 34 ++++
>>> drivers/thermal/thermal_thresholds.c | 36 ++--
>>> drivers/thermal/thermal_thresholds.h | 2 +-
>>> include/uapi/linux/thermal.h | 27 ++-
>>> 5 files changed, 307 insertions(+), 28 deletions(-)
>>>
>>
>> Since Rafael asked to wait a bit for the other
>> changes to go first, I will skip the detailed review
>> of that patch. I will do that after you add that
>> new locking scheme.
>
> Well, this is almost orthogonal to my locking changes and can be
> easily rebased on top of them, so I don't see a need to respin it.
>
> Please review.
OK, I will then.
I have an issue with the build process (cross compiling the libthermal)
but I will manage that differently for testing. I suspect some memory
leak in patch 2/4, so testing would help me...
>
>> In general the code looks good.
>
> OK
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 4/4] tools/thermal/thermal-engine: Take into account the thresholds API
2024-10-14 9:43 ` [PATCH v5 4/4] tools/thermal/thermal-engine: Take into account the thresholds API Daniel Lezcano
@ 2024-10-21 20:10 ` Lukasz Luba
2024-10-22 7:52 ` Daniel Lezcano
0 siblings, 1 reply; 26+ messages in thread
From: Lukasz Luba @ 2024-10-21 20:10 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-pm, quic_manafm, rafael
On 10/14/24 10:43, Daniel Lezcano wrote:
> Enhance the thermal-engine skeleton with the thresholds added in the
> kernel and use the API exported by the thermal library.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> tools/thermal/thermal-engine/thermal-engine.c | 105 +++++++++++++++---
> 1 file changed, 92 insertions(+), 13 deletions(-)
>
> diff --git a/tools/thermal/thermal-engine/thermal-engine.c b/tools/thermal/thermal-engine/thermal-engine.c
> index 9b1476a2680f..0764dc754771 100644
> --- a/tools/thermal/thermal-engine/thermal-engine.c
> +++ b/tools/thermal/thermal-engine/thermal-engine.c
> @@ -38,6 +38,14 @@ struct thermal_data {
> struct thermal_handler *th;
> };
>
> +static int show_threshold(struct thermal_threshold *th, __maybe_unused void *arg)
> +{
> + INFO("threshold temp=%d, direction=%d\n",
> + th->temperature, th->direction);
> +
> + return 0;
> +}
> +
> static int show_trip(struct thermal_trip *tt, __maybe_unused void *arg)
> {
> INFO("trip id=%d, type=%d, temp=%d, hyst=%d\n",
> @@ -70,6 +78,8 @@ static int show_tz(struct thermal_zone *tz, __maybe_unused void *arg)
>
> for_each_thermal_trip(tz->trip, show_trip, NULL);
>
> + for_each_thermal_threshold(tz->thresholds, show_threshold, NULL);
> +
> show_temp(tz, arg);
>
> show_governor(tz, arg);
> @@ -77,6 +87,30 @@ static int show_tz(struct thermal_zone *tz, __maybe_unused void *arg)
> return 0;
> }
>
> +static int set_threshold(struct thermal_zone *tz, __maybe_unused void *arg)
> +{
> + struct thermal_handler *th = arg;
> + int thresholds[] = { 43000, 65000, 49000, 55000, 57000 };
> + size_t i;
minor: I would just make it normal 'int'.
> +
> + INFO("Setting threshold for thermal zone '%s', id=%d\n", tz->name, tz->id);
> +
> + if (thermal_cmd_threshold_flush(th, tz)) {
> + ERROR("Failed to flush all previous thresholds\n");
> + return -1;
> + }
> +
> + for (i = 0; i < sizeof(thresholds) / sizeof(thresholds[0]); i++)
> + if (thermal_cmd_threshold_add(th, tz, thresholds[i],
> + THERMAL_THRESHOLD_WAY_UP |
> + THERMAL_THRESHOLD_WAY_DOWN)) {
> + ERROR("Failed to set threshold\n");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> static int tz_create(const char *name, int tz_id, __maybe_unused void *arg)
> {
> INFO("Thermal zone '%s'/%d created\n", name, tz_id);
> @@ -197,20 +231,62 @@ static int gov_change(int tz_id, const char *name, __maybe_unused void *arg)
> return 0;
> }
>
> +static int threshold_add(int tz_id, int temp, int direction, __maybe_unused void *arg)
> +{
> + INFO("Threshold added tz_id=%d: temp=%d, direction=%d\n", tz_id, temp, direction);
> +
> + return 0;
> +}
> +
> +static int threshold_delete(int tz_id, int temp, int direction, __maybe_unused void *arg)
> +{
> + INFO("Threshold deleted tz_id=%d: temp=%d, direction=%d\n", tz_id, temp, direction);
> +
> + return 0;
> +}
> +
> +static int threshold_flush(int tz_id, __maybe_unused void *arg)
> +{
> + INFO("Thresholds flushed tz_id=%d\n", tz_id);
> +
> + return 0;
> +}
> +
> +static int threshold_up(int tz_id, int temp, int prev_temp, __maybe_unused void *arg)
> +{
> + INFO("Threshold crossed way up tz_id=%d: temp=%d, prev_temp=%d\n",
> + tz_id, temp, prev_temp);
> +
> + return 0;
> +}
> +
> +static int threshold_down(int tz_id, int temp, int prev_temp, __maybe_unused void *arg)
> +{
> + INFO("Threshold crossed way down tz_id=%d: temp=%d, prev_temp=%d\n",
> + tz_id, temp, prev_temp);
> +
> + return 0;
> +}
> +
> static struct thermal_ops ops = {
> - .events.tz_create = tz_create,
> - .events.tz_delete = tz_delete,
> - .events.tz_disable = tz_disable,
> - .events.tz_enable = tz_enable,
> - .events.trip_high = trip_high,
> - .events.trip_low = trip_low,
> - .events.trip_add = trip_add,
> - .events.trip_delete = trip_delete,
> - .events.trip_change = trip_change,
> - .events.cdev_add = cdev_add,
> - .events.cdev_delete = cdev_delete,
> - .events.cdev_update = cdev_update,
> - .events.gov_change = gov_change
> + .events.tz_create = tz_create,
> + .events.tz_delete = tz_delete,
> + .events.tz_disable = tz_disable,
> + .events.tz_enable = tz_enable,
> + .events.trip_high = trip_high,
> + .events.trip_low = trip_low,
> + .events.trip_add = trip_add,
> + .events.trip_delete = trip_delete,
> + .events.trip_change = trip_change,
> + .events.cdev_add = cdev_add,
> + .events.cdev_delete = cdev_delete,
> + .events.cdev_update = cdev_update,
> + .events.gov_change = gov_change,
> + .events.threshold_add = threshold_add,
> + .events.threshold_delete = threshold_delete,
> + .events.threshold_flush = threshold_flush,
> + .events.threshold_up = threshold_up,
> + .events.threshold_down = threshold_down,
> };
>
> static int thermal_event(__maybe_unused int fd, __maybe_unused void *arg)
> @@ -280,6 +356,7 @@ enum {
> THERMAL_ENGINE_DAEMON_ERROR,
> THERMAL_ENGINE_LOG_ERROR,
> THERMAL_ENGINE_THERMAL_ERROR,
> + THERMAL_ENGINE_THRESHOLD_ERROR,
> THERMAL_ENGINE_MAINLOOP_ERROR,
> };
>
> @@ -318,6 +395,8 @@ int main(int argc, char *argv[])
> return THERMAL_ENGINE_THERMAL_ERROR;
> }
>
> + for_each_thermal_zone(td.tz, set_threshold, td.th);
> +
> for_each_thermal_zone(td.tz, show_tz, td.th);
>
> if (mainloop_init()) {
LGTM,
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI
2024-10-14 9:43 ` [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI Daniel Lezcano
@ 2024-10-21 20:47 ` Lukasz Luba
2024-10-22 7:49 ` Daniel Lezcano
0 siblings, 1 reply; 26+ messages in thread
From: Lukasz Luba @ 2024-10-21 20:47 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-pm, quic_manafm, rafael
On 10/14/24 10:43, Daniel Lezcano wrote:
> The thermal framework supports the thresholds and allows the userspace
> to create, delete, flush, get the list of the thresholds as well as
> getting the list of the thresholds set for a specific thermal zone.
>
> Add the netlink abstraction in the thermal library to take full
> advantage of thresholds for the userspace program.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> tools/lib/thermal/commands.c | 128 +++++++++++++++++++++++++++-
> tools/lib/thermal/events.c | 55 +++++++++---
> tools/lib/thermal/include/thermal.h | 40 +++++++++
> tools/lib/thermal/libthermal.map | 5 ++
> tools/lib/thermal/thermal.c | 17 ++++
> tools/thermal/lib/Makefile | 2 +-
> 6 files changed, 232 insertions(+), 15 deletions(-)
>
> diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
> index a9223df91dcf..9d5e3e891628 100644
> --- a/tools/lib/thermal/commands.c
> +++ b/tools/lib/thermal/commands.c
> @@ -5,6 +5,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> +#include <limits.h>
>
> #include <thermal.h>
> #include "thermal_nl.h"
> @@ -33,6 +34,11 @@ static struct nla_policy thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
> [THERMAL_GENL_ATTR_CDEV_CUR_STATE] = { .type = NLA_U32 },
> [THERMAL_GENL_ATTR_CDEV_MAX_STATE] = { .type = NLA_U32 },
> [THERMAL_GENL_ATTR_CDEV_NAME] = { .type = NLA_STRING },
> +
> + /* Thresholds */
> + [THERMAL_GENL_ATTR_THRESHOLD] = { .type = NLA_NESTED },
> + [THERMAL_GENL_ATTR_THRESHOLD_TEMP] = { .type = NLA_U32 },
> + [THERMAL_GENL_ATTR_THRESHOLD_DIRECTION] = { .type = NLA_U32 },
> };
>
> static int parse_tz_get(struct genl_info *info, struct thermal_zone **tz)
> @@ -182,6 +188,38 @@ static int parse_tz_get_gov(struct genl_info *info, struct thermal_zone *tz)
> return THERMAL_SUCCESS;
> }
>
> +static int parse_threshold_get(struct genl_info *info, struct thermal_zone *tz)
> +{
> + struct nlattr *attr;
> + struct thermal_threshold *__tt = NULL;
> + size_t size = 0;
why not simply 'int' ?
also: size=0 which has some impact mentioned below.
> + int rem;
I would make them a descending order, those lines.
> +
> + nla_for_each_nested(attr, info->attrs[THERMAL_GENL_ATTR_THRESHOLD], rem) {
> +
> + if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_TEMP) {
> +
> + size++;
> +
> + __tt = realloc(__tt, sizeof(*__tt) * (size + 2));
> + if (!__tt)
> + return THERMAL_ERROR;
> +
> + __tt[size - 1].temperature = nla_get_u32(attr);
> + }
> +
> + if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_DIRECTION)
> + __tt[size - 1].direction = nla_get_u32(attr);
We probably relay on some order here, because the 'size -1' needs to be
done after first 'size++'.
If that the case then maybe it's worth a comment. Or if it wasn't
intended and there are no strong guarantees, then this needs a fix.
> + }
> +
> + if (__tt)
> + __tt[size].temperature = INT_MAX;
> +
> + tz->thresholds = __tt;
I wonder what would happen to the previous 'tz->thresholds' when
we just put new one here... I cannot find other place when it's set.
Since we have '*__tt = NULL' then one of the solutions would be
to simply call:
free(tz->thresholds);
tz->thresholds = __tt;
Am I missing something, when it might be cleaned in different place?
> +
> + return THERMAL_SUCCESS;
> +}
> +
> static int handle_netlink(struct nl_cache_ops *unused,
> struct genl_cmd *cmd,
> struct genl_info *info, void *arg)
> @@ -210,6 +248,10 @@ static int handle_netlink(struct nl_cache_ops *unused,
> ret = parse_tz_get_gov(info, arg);
> break;
>
> + case THERMAL_GENL_CMD_THRESHOLD_GET:
> + ret = parse_threshold_get(info, arg);
> + break;
> +
I can see in the kernel part in the funciton:
thermal_genl_cmd_doit()
that there add, delete, flush also send a response
message. Shouldn't be handled here gently, otherwise
> default:
> return THERMAL_ERROR;
that 'default' might capture them?
> }
> @@ -253,6 +295,34 @@ static struct genl_cmd thermal_cmds[] = {
> .c_maxattr = THERMAL_GENL_ATTR_MAX,
> .c_attr_policy = thermal_genl_policy,
> },
> + {
> + .c_id = THERMAL_GENL_CMD_THRESHOLD_GET,
> + .c_name = (char *)"Get thresholds list",
> + .c_msg_parser = handle_netlink,
> + .c_maxattr = THERMAL_GENL_ATTR_MAX,
> + .c_attr_policy = thermal_genl_policy,
> + },
> + {
> + .c_id = THERMAL_GENL_CMD_THRESHOLD_ADD,
> + .c_name = (char *)"Add a threshold",
> + .c_msg_parser = handle_netlink,
> + .c_maxattr = THERMAL_GENL_ATTR_MAX,
> + .c_attr_policy = thermal_genl_policy,
> + },
> + {
> + .c_id = THERMAL_GENL_CMD_THRESHOLD_DELETE,
> + .c_name = (char *)"Delete a threshold",
> + .c_msg_parser = handle_netlink,
> + .c_maxattr = THERMAL_GENL_ATTR_MAX,
> + .c_attr_policy = thermal_genl_policy,
> + },
> + {
> + .c_id = THERMAL_GENL_CMD_THRESHOLD_FLUSH,
> + .c_name = (char *)"Flush the thresholds",
> + .c_msg_parser = handle_netlink,
> + .c_maxattr = THERMAL_GENL_ATTR_MAX,
> + .c_attr_policy = thermal_genl_policy,
> + },
> };
>
> static struct genl_ops thermal_cmd_ops = {
> @@ -263,13 +333,29 @@ static struct genl_ops thermal_cmd_ops = {
>
> struct cmd_param {
> int tz_id;
> + int temp;
> + int direction;
> };
>
> typedef int (*cmd_cb_t)(struct nl_msg *, struct cmd_param *);
>
> static int thermal_genl_tz_id_encode(struct nl_msg *msg, struct cmd_param *p)
> {
> - if (p->tz_id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
> + if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
> + return -1;
> +
> + return 0;
> +}
> +
> +static int thermal_genl_threshold_encode(struct nl_msg *msg, struct cmd_param *p)
> +{
> + if (thermal_genl_tz_id_encode(msg, p))
> + return -1;
> +
> + if (nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_TEMP, p->temp))
> + return -1;
> +
> + if (nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_DIRECTION, p->direction))
> return -1;
>
> return 0;
> @@ -338,6 +424,46 @@ thermal_error_t thermal_cmd_get_temp(struct thermal_handler *th, struct thermal_
> THERMAL_GENL_CMD_TZ_GET_TEMP, 0, tz);
> }
>
> +thermal_error_t thermal_cmd_threshold_get(struct thermal_handler *th,
> + struct thermal_zone *tz)
> +{
> + struct cmd_param p = { .tz_id = tz->id };
> +
> + return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
> + THERMAL_GENL_CMD_THRESHOLD_GET, 0, tz);
> +}
> +
> +thermal_error_t thermal_cmd_threshold_add(struct thermal_handler *th,
> + struct thermal_zone *tz,
> + int temperature,
> + int direction)
> +{
> + struct cmd_param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
> +
> + return thermal_genl_auto(th, thermal_genl_threshold_encode, &p,
> + THERMAL_GENL_CMD_THRESHOLD_ADD, 0, tz);
> +}
> +
> +thermal_error_t thermal_cmd_threshold_delete(struct thermal_handler *th,
> + struct thermal_zone *tz,
> + int temperature,
> + int direction)
> +{
> + struct cmd_param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
> +
> + return thermal_genl_auto(th, thermal_genl_threshold_encode, &p,
> + THERMAL_GENL_CMD_THRESHOLD_DELETE, 0, tz);
> +}
> +
> +thermal_error_t thermal_cmd_threshold_flush(struct thermal_handler *th,
> + struct thermal_zone *tz)
> +{
> + struct cmd_param p = { .tz_id = tz->id };
> +
> + return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
> + THERMAL_GENL_CMD_THRESHOLD_FLUSH, 0, tz);
> +}
> +
> thermal_error_t thermal_cmd_exit(struct thermal_handler *th)
> {
> if (genl_unregister_family(&thermal_cmd_ops))
> diff --git a/tools/lib/thermal/events.c b/tools/lib/thermal/events.c
> index a7a55d1a0c4c..bd851c869029 100644
> --- a/tools/lib/thermal/events.c
> +++ b/tools/lib/thermal/events.c
> @@ -94,6 +94,30 @@ static int handle_thermal_event(struct nl_msg *n, void *arg)
> case THERMAL_GENL_EVENT_TZ_GOV_CHANGE:
> return ops->gov_change(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
> nla_get_string(attrs[THERMAL_GENL_ATTR_GOV_NAME]), arg);
> +
> + case THERMAL_GENL_EVENT_THRESHOLD_ADD:
> + return ops->threshold_add(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
> + nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]),
> + nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]), arg);
> +
> + case THERMAL_GENL_EVENT_THRESHOLD_DELETE:
> + return ops->threshold_delete(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
> + nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]),
> + nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]), arg);
> +
> + case THERMAL_GENL_EVENT_THRESHOLD_FLUSH:
> + return ops->threshold_flush(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]), arg);
> +
> + case THERMAL_GENL_EVENT_THRESHOLD_UP:
> + return ops->threshold_up(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
> + nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_TEMP]),
> + nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_PREV_TEMP]), arg);
> +
> + case THERMAL_GENL_EVENT_THRESHOLD_DOWN:
> + return ops->threshold_down(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
> + nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_TEMP]),
> + nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_PREV_TEMP]), arg);
> +
> default:
> return -1;
> }
> @@ -101,19 +125,24 @@ static int handle_thermal_event(struct nl_msg *n, void *arg)
>
> static void thermal_events_ops_init(struct thermal_events_ops *ops)
> {
> - enabled_ops[THERMAL_GENL_EVENT_TZ_CREATE] = !!ops->tz_create;
> - enabled_ops[THERMAL_GENL_EVENT_TZ_DELETE] = !!ops->tz_delete;
> - enabled_ops[THERMAL_GENL_EVENT_TZ_DISABLE] = !!ops->tz_disable;
> - enabled_ops[THERMAL_GENL_EVENT_TZ_ENABLE] = !!ops->tz_enable;
> - enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_UP] = !!ops->trip_high;
> - enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_DOWN] = !!ops->trip_low;
> - enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_CHANGE] = !!ops->trip_change;
> - enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_ADD] = !!ops->trip_add;
> - enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_DELETE] = !!ops->trip_delete;
> - enabled_ops[THERMAL_GENL_EVENT_CDEV_ADD] = !!ops->cdev_add;
> - enabled_ops[THERMAL_GENL_EVENT_CDEV_DELETE] = !!ops->cdev_delete;
> - enabled_ops[THERMAL_GENL_EVENT_CDEV_STATE_UPDATE] = !!ops->cdev_update;
> - enabled_ops[THERMAL_GENL_EVENT_TZ_GOV_CHANGE] = !!ops->gov_change;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_CREATE] = !!ops->tz_create;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_DELETE] = !!ops->tz_delete;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_DISABLE] = !!ops->tz_disable;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_ENABLE] = !!ops->tz_enable;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_UP] = !!ops->trip_high;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_DOWN] = !!ops->trip_low;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_CHANGE] = !!ops->trip_change;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_ADD] = !!ops->trip_add;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_TRIP_DELETE] = !!ops->trip_delete;
> + enabled_ops[THERMAL_GENL_EVENT_CDEV_ADD] = !!ops->cdev_add;
> + enabled_ops[THERMAL_GENL_EVENT_CDEV_DELETE] = !!ops->cdev_delete;
> + enabled_ops[THERMAL_GENL_EVENT_CDEV_STATE_UPDATE] = !!ops->cdev_update;
> + enabled_ops[THERMAL_GENL_EVENT_TZ_GOV_CHANGE] = !!ops->gov_change;
> + enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_ADD] = !!ops->threshold_add;
> + enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_DELETE] = !!ops->threshold_delete;
> + enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_FLUSH] = !!ops->threshold_flush;
> + enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_UP] = !!ops->threshold_up;
> + enabled_ops[THERMAL_GENL_EVENT_THRESHOLD_DOWN] = !!ops->threshold_down;
> }
>
> thermal_error_t thermal_events_handle(struct thermal_handler *th, void *arg)
> diff --git a/tools/lib/thermal/include/thermal.h b/tools/lib/thermal/include/thermal.h
> index 1abc560602cf..818ecdfb46e5 100644
> --- a/tools/lib/thermal/include/thermal.h
> +++ b/tools/lib/thermal/include/thermal.h
> @@ -4,11 +4,20 @@
> #define __LIBTHERMAL_H
>
> #include <linux/thermal.h>
> +#include <sys/types.h>
>
> #ifndef LIBTHERMAL_API
> #define LIBTHERMAL_API __attribute__((visibility("default")))
> #endif
>
> +#ifndef THERMAL_THRESHOLD_WAY_UP
> +#define THERMAL_THRESHOLD_WAY_UP 0x1
> +#endif
> +
> +#ifndef THERMAL_THRESHOLD_WAY_DOWN
> +#define THERMAL_THRESHOLD_WAY_DOWN 0x2
> +#endif
> +
> #ifdef __cplusplus
> extern "C" {
> #endif
> @@ -31,6 +40,11 @@ struct thermal_events_ops {
> int (*cdev_delete)(int cdev_id, void *arg);
> int (*cdev_update)(int cdev_id, int cur_state, void *arg);
> int (*gov_change)(int tz_id, const char *gov_name, void *arg);
> + int (*threshold_add)(int tz_id, int temperature, int direction, void *arg);
> + int (*threshold_delete)(int tz_id, int temperature, int direction, void *arg);
> + int (*threshold_flush)(int tz_id, void *arg);
> + int (*threshold_up)(int tz_id, int temp, int prev_temp, void *arg);
> + int (*threshold_down)(int tz_id, int temp, int prev_temp, void *arg);
> };
>
> struct thermal_ops {
> @@ -45,12 +59,18 @@ struct thermal_trip {
> int hyst;
> };
>
> +struct thermal_threshold {
> + int temperature;
> + int direction;
> +};
> +
> struct thermal_zone {
> int id;
> int temp;
> char name[THERMAL_NAME_LENGTH];
> char governor[THERMAL_NAME_LENGTH];
> struct thermal_trip *trip;
> + struct thermal_threshold *thresholds;
> };
>
> struct thermal_cdev {
> @@ -74,12 +94,16 @@ typedef int (*cb_tt_t)(struct thermal_trip *, void *);
>
> typedef int (*cb_tc_t)(struct thermal_cdev *, void *);
>
> +typedef int (*cb_th_t)(struct thermal_threshold *, void *);
> +
> LIBTHERMAL_API int for_each_thermal_zone(struct thermal_zone *tz, cb_tz_t cb, void *arg);
>
> LIBTHERMAL_API int for_each_thermal_trip(struct thermal_trip *tt, cb_tt_t cb, void *arg);
>
> LIBTHERMAL_API int for_each_thermal_cdev(struct thermal_cdev *cdev, cb_tc_t cb, void *arg);
>
> +LIBTHERMAL_API int for_each_thermal_threshold(struct thermal_threshold *th, cb_th_t cb, void *arg);
> +
> LIBTHERMAL_API struct thermal_zone *thermal_zone_find_by_name(struct thermal_zone *tz,
> const char *name);
>
> @@ -124,6 +148,22 @@ LIBTHERMAL_API thermal_error_t thermal_cmd_get_governor(struct thermal_handler *
> LIBTHERMAL_API thermal_error_t thermal_cmd_get_temp(struct thermal_handler *th,
> struct thermal_zone *tz);
>
> +LIBTHERMAL_API thermal_error_t thermal_cmd_threshold_get(struct thermal_handler *th,
> + struct thermal_zone *tz);
> +
> +LIBTHERMAL_API thermal_error_t thermal_cmd_threshold_add(struct thermal_handler *th,
> + struct thermal_zone *tz,
> + int temperature,
> + int direction);
> +
> +LIBTHERMAL_API thermal_error_t thermal_cmd_threshold_delete(struct thermal_handler *th,
> + struct thermal_zone *tz,
> + int temperature,
> + int direction);
> +
> +LIBTHERMAL_API thermal_error_t thermal_cmd_threshold_flush(struct thermal_handler *th,
> + struct thermal_zone *tz);
> +
> /*
> * Netlink thermal samples
> */
> diff --git a/tools/lib/thermal/libthermal.map b/tools/lib/thermal/libthermal.map
> index d5e77738c7a4..d657176aa47f 100644
> --- a/tools/lib/thermal/libthermal.map
> +++ b/tools/lib/thermal/libthermal.map
> @@ -4,6 +4,7 @@ LIBTHERMAL_0.0.1 {
> for_each_thermal_zone;
> for_each_thermal_trip;
> for_each_thermal_cdev;
> + for_each_thermal_threshold;
> thermal_zone_find_by_name;
> thermal_zone_find_by_id;
> thermal_zone_discover;
> @@ -17,6 +18,10 @@ LIBTHERMAL_0.0.1 {
> thermal_cmd_get_trip;
> thermal_cmd_get_governor;
> thermal_cmd_get_temp;
> + thermal_cmd_threshold_get;
> + thermal_cmd_threshold_add;
> + thermal_cmd_threshold_delete;
> + thermal_cmd_threshold_flush;
> thermal_sampling_init;
> thermal_sampling_handle;
> thermal_sampling_fd;
> diff --git a/tools/lib/thermal/thermal.c b/tools/lib/thermal/thermal.c
> index 72a76dc205bc..8dddf5cde302 100644
> --- a/tools/lib/thermal/thermal.c
> +++ b/tools/lib/thermal/thermal.c
> @@ -1,10 +1,24 @@
> // SPDX-License-Identifier: LGPL-2.1+
> // Copyright (C) 2022, Linaro Ltd - Daniel Lezcano <daniel.lezcano@linaro.org>
> #include <stdio.h>
> +#include <limits.h>
> #include <thermal.h>
>
> #include "thermal_nl.h"
>
> +int for_each_thermal_threshold(struct thermal_threshold *th, cb_th_t cb, void *arg)
> +{
> + int i, ret = 0;
> +
> + if (!th)
> + return 0;
> +
> + for (i = 0; th[i].temperature != INT_MAX; i++)
> + ret |= cb(&th[i], arg);
> +
> + return ret;
> +}
> +
> int for_each_thermal_cdev(struct thermal_cdev *cdev, cb_tc_t cb, void *arg)
> {
> int i, ret = 0;
> @@ -80,6 +94,9 @@ static int __thermal_zone_discover(struct thermal_zone *tz, void *th)
> if (thermal_cmd_get_trip(th, tz) < 0)
> return -1;
>
> + if (thermal_cmd_threshold_get(th, tz) < 0)
There are only 2 definitions in the enum thermal_error_t.
I would just simply checked if it's not 0:
if (thermal_cmd_threshold_get(th, tz))
return -1;
Although, it's a minor thing, not affecting the end result.
> + return -1;
> +
> if (thermal_cmd_get_governor(th, tz))
> return -1;
>
> diff --git a/tools/thermal/lib/Makefile b/tools/thermal/lib/Makefile
> index 82db451935c5..f2552f73a64c 100644
> --- a/tools/thermal/lib/Makefile
> +++ b/tools/thermal/lib/Makefile
> @@ -3,7 +3,7 @@
>
> LIBTHERMAL_TOOLS_VERSION = 0
> LIBTHERMAL_TOOLS_PATCHLEVEL = 0
> -LIBTHERMAL_TOOLS_EXTRAVERSION = 1
> +LIBTHERMAL_TOOLS_EXTRAVERSION = 2
>
> MAKEFLAGS += --no-print-directory
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-14 9:43 ` [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds Daniel Lezcano
2024-10-21 10:58 ` Rafael J. Wysocki
2024-10-21 19:42 ` Lukasz Luba
@ 2024-10-21 22:02 ` Lukasz Luba
2024-10-22 7:09 ` Daniel Lezcano
2 siblings, 1 reply; 26+ messages in thread
From: Lukasz Luba @ 2024-10-21 22:02 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-pm, quic_manafm, rafael
On 10/14/24 10:43, Daniel Lezcano wrote:
> The thresholds exist but there is no notification neither action code
> related to them yet.
>
> These changes implement the netlink for the notifications when the
> thresholds are crossed, added, deleted or flushed as well as the
> commands which allows to get the list of the thresholds, flush them,
> add and delete.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/thermal/thermal_netlink.c | 236 ++++++++++++++++++++++++++-
> drivers/thermal/thermal_netlink.h | 34 ++++
> drivers/thermal/thermal_thresholds.c | 36 ++--
> drivers/thermal/thermal_thresholds.h | 2 +-
> include/uapi/linux/thermal.h | 27 ++-
> 5 files changed, 307 insertions(+), 28 deletions(-)
>
[snip]
>
> +static inline int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
> + int temperature, int direction)
> +{
> + return 0;
> +}
> +
> +static inline int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
> + int temperature, int direction)
> +{
> + return 0;
> +}
> +
> +static inline int thermal_notify_threshold_flush(const struct thermal_zone_device *tz)
> +{
> + return 0;
> +}
> +
> +static inline int thermal_notify_threshold_down(const struct thermal_zone_device *tz)
> +{
> + return 0;
> +}
> +
> +static inline int thermal_notify_threshold_up(const struct thermal_zone_device *tz)
> +{
> + return 0;
> +}
These 'return 0' look a bit odd. We usually use 'return -EINVAL' in
not defined. Although, since we don't check the output of those
functions - we are OK. We just have to remember about these zeros,
one day when we would like to add the check of the return.
> +
> static inline void __init thermal_netlink_exit(void) {}
>
> #endif /* CONFIG_THERMAL_NETLINK */
>
> -int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
> +int thermal_thresholds_delete(struct thermal_zone_device *tz,
> + int temperature, int direction)
> {
> struct list_head *thresholds = &tz->user_thresholds;
> struct user_threshold *t;
> @@ -205,6 +209,8 @@ int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, i
> t->direction &= ~direction;
> }
>
> + thermal_notify_threshold_delete(tz, temperature, direction);
> +
> __thermal_zone_device_update(tz, THERMAL_TZ_DEL_THRESHOLD);
>
> return 0;
> @@ -217,7 +223,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
> struct user_threshold *entry;
> int ret;
>
> - lockdep_assert_held(&tz->lock);
> + mutex_lock(&tz->lock);
>
> list_for_each_entry(entry, thresholds, list_node) {
> ret = cb(entry, arg);
> @@ -225,5 +231,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
> return ret;
I agree with Rafael here. The lock should be released before return.
The rest looks good.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-21 22:02 ` Lukasz Luba
@ 2024-10-22 7:09 ` Daniel Lezcano
2024-10-22 9:40 ` Lukasz Luba
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-22 7:09 UTC (permalink / raw)
To: Lukasz Luba; +Cc: linux-pm, quic_manafm, rafael
On 22/10/2024 00:02, Lukasz Luba wrote:
>
>
> On 10/14/24 10:43, Daniel Lezcano wrote:
>> The thresholds exist but there is no notification neither action code
>> related to them yet.
>>
>> These changes implement the netlink for the notifications when the
>> thresholds are crossed, added, deleted or flushed as well as the
>> commands which allows to get the list of the thresholds, flush them,
>> add and delete.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
[ ... ]
>> +static inline int thermal_notify_threshold_up(const struct
>> thermal_zone_device *tz)
>> +{
>> + return 0;
>> +}
>
> These 'return 0' look a bit odd. We usually use 'return -EINVAL' in
> not defined. Although, since we don't check the output of those
> functions - we are OK. We just have to remember about these zeros,
> one day when we would like to add the check of the return.
The return error really depends on the context of the call site. There
are other subsystems returning 0 when the service is not enabled (eg.
cpufreq.h, devfreq.h, device_cgroup.h, etc ...)
--
<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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/4] tools/lib/thermal: Make more generic the command encoding function
2024-10-21 19:49 ` Lukasz Luba
@ 2024-10-22 7:12 ` Daniel Lezcano
2024-10-22 9:43 ` Lukasz Luba
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-22 7:12 UTC (permalink / raw)
To: Lukasz Luba; +Cc: linux-pm, quic_manafm, rafael
On 21/10/2024 21:49, Lukasz Luba wrote:
>
>
> On 10/14/24 10:43, Daniel Lezcano wrote:
>> The thermal netlink has been extended with more commands which require
>> an encoding with more information. The generic encoding function puts
>> the thermal zone id with the command name. It is the unique
>> parameters.
>>
>> The next changes will provide more parameters to the command. Set the
>> scene for those new parameters by making the encoding function more
>> generic.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
[ ... ]
>> +static thermal_error_t thermal_genl_auto(struct thermal_handler *th,
>> cmd_cb_t cmd_cb,
>> + struct cmd_param *param,
>> + int cmd, int flags, void *arg)
>> {
>> struct nl_msg *msg;
>> void *hdr;
>> @@ -276,7 +291,7 @@ static thermal_error_t thermal_genl_auto(struct
>> thermal_handler *th, int id, int
>> if (!hdr)
>> return THERMAL_ERROR;
>> - if (id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id))
>> + if (cmd_cb && cmd_cb(msg, param))
>> return THERMAL_ERROR;
>
> It's not in this code but also in older:
> shouldn't we free the nlmsg_free(msg); before returns in this
> function?
Right, thanks for pointing this out
If it is ok, I will send a patch on top of this series to fix the entire
function
--
<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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI
2024-10-21 20:47 ` Lukasz Luba
@ 2024-10-22 7:49 ` Daniel Lezcano
2024-10-22 9:50 ` Lukasz Luba
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-22 7:49 UTC (permalink / raw)
To: Lukasz Luba; +Cc: linux-pm, quic_manafm, rafael
On 21/10/2024 22:47, Lukasz Luba wrote:
>
>
> On 10/14/24 10:43, Daniel Lezcano wrote:
>> The thermal framework supports the thresholds and allows the userspace
>> to create, delete, flush, get the list of the thresholds as well as
>> getting the list of the thresholds set for a specific thermal zone.
>>
>> Add the netlink abstraction in the thermal library to take full
>> advantage of thresholds for the userspace program.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>> tools/lib/thermal/commands.c | 128 +++++++++++++++++++++++++++-
>> tools/lib/thermal/events.c | 55 +++++++++---
>> tools/lib/thermal/include/thermal.h | 40 +++++++++
>> tools/lib/thermal/libthermal.map | 5 ++
>> tools/lib/thermal/thermal.c | 17 ++++
>> tools/thermal/lib/Makefile | 2 +-
>> 6 files changed, 232 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
>> index a9223df91dcf..9d5e3e891628 100644
>> --- a/tools/lib/thermal/commands.c
>> +++ b/tools/lib/thermal/commands.c
>> @@ -5,6 +5,7 @@
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <unistd.h>
>> +#include <limits.h>
>> #include <thermal.h>
>> #include "thermal_nl.h"
>> @@ -33,6 +34,11 @@ static struct nla_policy
>> thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
>> [THERMAL_GENL_ATTR_CDEV_CUR_STATE] = { .type = NLA_U32 },
>> [THERMAL_GENL_ATTR_CDEV_MAX_STATE] = { .type = NLA_U32 },
>> [THERMAL_GENL_ATTR_CDEV_NAME] = { .type = NLA_STRING },
>> +
>> + /* Thresholds */
>> + [THERMAL_GENL_ATTR_THRESHOLD] = { .type = NLA_NESTED },
>> + [THERMAL_GENL_ATTR_THRESHOLD_TEMP] = { .type = NLA_U32 },
>> + [THERMAL_GENL_ATTR_THRESHOLD_DIRECTION] = { .type = NLA_U32 },
>> };
>> static int parse_tz_get(struct genl_info *info, struct thermal_zone
>> **tz)
>> @@ -182,6 +188,38 @@ static int parse_tz_get_gov(struct genl_info
>> *info, struct thermal_zone *tz)
>> return THERMAL_SUCCESS;
>> }
>> +static int parse_threshold_get(struct genl_info *info, struct
>> thermal_zone *tz)
>> +{
>> + struct nlattr *attr;
>> + struct thermal_threshold *__tt = NULL;
>> + size_t size = 0;
>
> why not simply 'int' ?
Because it is used with realloc where the function definition is:
void *realloc(void *_Nullable ptr, size_t size);
and sizeof() return a size_t type.
So in order to not mix the types, size_t is used which I believe is a
good practice :)
> also: size=0 which has some impact mentioned below.
>
>> + int rem;
>
> I would make them a descending order, those lines.
>
>> +
>> + nla_for_each_nested(attr, info-
>> >attrs[THERMAL_GENL_ATTR_THRESHOLD], rem) {
>> +
>> + if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_TEMP) {
>> +
>> + size++;
>> +
>> + __tt = realloc(__tt, sizeof(*__tt) * (size + 2));
>> + if (!__tt)
>> + return THERMAL_ERROR;
>> +
>> + __tt[size - 1].temperature = nla_get_u32(attr);
>> + }
>> +
>> + if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_DIRECTION)
>> + __tt[size - 1].direction = nla_get_u32(attr);
>
> We probably relay on some order here, because the 'size -1' needs to be
> done after first 'size++'.
> If that the case then maybe it's worth a comment. Or if it wasn't
> intended and there are no strong guarantees, then this needs a fix.
The size contains the size of the array and we want to access the last
element, size - 1
I will add this sentence above as a comment if it is ok for you
>> + }
>> +
>> + if (__tt)
>> + __tt[size].temperature = INT_MAX;
>> +
>> + tz->thresholds = __tt;
>
> I wonder what would happen to the previous 'tz->thresholds' when
> we just put new one here... I cannot find other place when it's set.
>
> Since we have '*__tt = NULL' then one of the solutions would be
> to simply call:
> free(tz->thresholds);
> tz->thresholds = __tt;
>
> Am I missing something, when it might be cleaned in different place?
The caller is supposed to pass a clean empty structure.
Usually, this function is to discover the current configuration, so it
is a one shot call keeping the structure in memory for the libthermal
lifecycle.
The events sends updates of the thermal zones. So with the events and
the initial configuration from the discovery, the userspace is always
up-to-date with the thermal setup.
>> +
>> + return THERMAL_SUCCESS;
>> +}
>> +
>> static int handle_netlink(struct nl_cache_ops *unused,
>> struct genl_cmd *cmd,
>> struct genl_info *info, void *arg)
>> @@ -210,6 +248,10 @@ static int handle_netlink(struct nl_cache_ops
>> *unused,
>> ret = parse_tz_get_gov(info, arg);
>> break;
>> + case THERMAL_GENL_CMD_THRESHOLD_GET:
>> + ret = parse_threshold_get(info, arg);
>> + break;
>> +
>
> I can see in the kernel part in the funciton:
> thermal_genl_cmd_doit()
> that there add, delete, flush also send a response
> message. Shouldn't be handled here gently, otherwise
No, those are commands and the transaction is done at the netlink level
to say if the command was successful or not.
>> default:
>> return THERMAL_ERROR;
>
> that 'default' might capture them?
[ ... ]
>> int for_each_thermal_cdev(struct thermal_cdev *cdev, cb_tc_t cb,
>> void *arg)
>> {
>> int i, ret = 0;
>> @@ -80,6 +94,9 @@ static int __thermal_zone_discover(struct
>> thermal_zone *tz, void *th)
>> if (thermal_cmd_get_trip(th, tz) < 0)
>> return -1;
>> + if (thermal_cmd_threshold_get(th, tz) < 0)
>
> There are only 2 definitions in the enum thermal_error_t.
> I would just simply checked if it's not 0:
Ok
Thanks for the review
> if (thermal_cmd_threshold_get(th, tz))
> return -1;
>
> Although, it's a minor thing, not affecting the end result.
>
>> + return -1;
>> +
>> if (thermal_cmd_get_governor(th, tz))
>> return -1;
>> diff --git a/tools/thermal/lib/Makefile b/tools/thermal/lib/Makefile
>> index 82db451935c5..f2552f73a64c 100644
>> --- a/tools/thermal/lib/Makefile
>> +++ b/tools/thermal/lib/Makefile
>> @@ -3,7 +3,7 @@
>> LIBTHERMAL_TOOLS_VERSION = 0
>> LIBTHERMAL_TOOLS_PATCHLEVEL = 0
>> -LIBTHERMAL_TOOLS_EXTRAVERSION = 1
>> +LIBTHERMAL_TOOLS_EXTRAVERSION = 2
>> MAKEFLAGS += --no-print-directory
--
<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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 4/4] tools/thermal/thermal-engine: Take into account the thresholds API
2024-10-21 20:10 ` Lukasz Luba
@ 2024-10-22 7:52 ` Daniel Lezcano
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-22 7:52 UTC (permalink / raw)
To: Lukasz Luba; +Cc: linux-pm, quic_manafm, rafael
On 21/10/2024 22:10, Lukasz Luba wrote:
>
>
> On 10/14/24 10:43, Daniel Lezcano wrote:
>> Enhance the thermal-engine skeleton with the thresholds added in the
>> kernel and use the API exported by the thermal library.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>> tools/thermal/thermal-engine/thermal-engine.c | 105 +++++++++++++++---
>> 1 file changed, 92 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/thermal/thermal-engine/thermal-engine.c b/tools/
>> thermal/thermal-engine/thermal-engine.c
>> index 9b1476a2680f..0764dc754771 100644
>> --- a/tools/thermal/thermal-engine/thermal-engine.c
>> +++ b/tools/thermal/thermal-engine/thermal-engine.c
>> @@ -38,6 +38,14 @@ struct thermal_data {
>> struct thermal_handler *th;
>> };
>> +static int show_threshold(struct thermal_threshold *th,
>> __maybe_unused void *arg)
>> +{
>> + INFO("threshold temp=%d, direction=%d\n",
>> + th->temperature, th->direction);
>> +
>> + return 0;
>> +}
>> +
>> static int show_trip(struct thermal_trip *tt, __maybe_unused void *arg)
>> {
>> INFO("trip id=%d, type=%d, temp=%d, hyst=%d\n",
>> @@ -70,6 +78,8 @@ static int show_tz(struct thermal_zone *tz,
>> __maybe_unused void *arg)
>> for_each_thermal_trip(tz->trip, show_trip, NULL);
>> + for_each_thermal_threshold(tz->thresholds, show_threshold, NULL);
>> +
>> show_temp(tz, arg);
>> show_governor(tz, arg);
>> @@ -77,6 +87,30 @@ static int show_tz(struct thermal_zone *tz,
>> __maybe_unused void *arg)
>> return 0;
>> }
>> +static int set_threshold(struct thermal_zone *tz, __maybe_unused void
>> *arg)
>> +{
>> + struct thermal_handler *th = arg;
>> + int thresholds[] = { 43000, 65000, 49000, 55000, 57000 };
>> + size_t i;
>
> minor: I would just make it normal 'int'.
Why ? :)
The for loop compares i < sizeof(thresholds) / sizeof(thresholds[0])
sizeof() returns size_t
So we are comparing the same type.
>> +
>> + INFO("Setting threshold for thermal zone '%s', id=%d\n", tz-
>> >name, tz->id);
>> +
[ ... ]
>
> LGTM,
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Thanks for the review
--
<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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-22 7:09 ` Daniel Lezcano
@ 2024-10-22 9:40 ` Lukasz Luba
2024-10-22 10:01 ` Rafael J. Wysocki
0 siblings, 1 reply; 26+ messages in thread
From: Lukasz Luba @ 2024-10-22 9:40 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-pm, quic_manafm, rafael
Hi Daniel,
On 10/22/24 08:09, Daniel Lezcano wrote:
> On 22/10/2024 00:02, Lukasz Luba wrote:
>>
>>
>> On 10/14/24 10:43, Daniel Lezcano wrote:
>>> The thresholds exist but there is no notification neither action code
>>> related to them yet.
>>>
>>> These changes implement the netlink for the notifications when the
>>> thresholds are crossed, added, deleted or flushed as well as the
>>> commands which allows to get the list of the thresholds, flush them,
>>> add and delete.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>
> [ ... ]
>
>>> +static inline int thermal_notify_threshold_up(const struct
>>> thermal_zone_device *tz)
>>> +{
>>> + return 0;
>>> +}
>>
>> These 'return 0' look a bit odd. We usually use 'return -EINVAL' in
>> not defined. Although, since we don't check the output of those
>> functions - we are OK. We just have to remember about these zeros,
>> one day when we would like to add the check of the return.
>
> The return error really depends on the context of the call site. There
> are other subsystems returning 0 when the service is not enabled (eg.
> cpufreq.h, devfreq.h, device_cgroup.h, etc ...)
>
>
Fair enough. As I said, we would just keep them in mind if we one
day decide to add the checks of the returns.
I'm waiting for your next version with the new locking scheme that
Rafael asked and I will add my review tags.
Regards,
Lukasz
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/4] tools/lib/thermal: Make more generic the command encoding function
2024-10-22 7:12 ` Daniel Lezcano
@ 2024-10-22 9:43 ` Lukasz Luba
0 siblings, 0 replies; 26+ messages in thread
From: Lukasz Luba @ 2024-10-22 9:43 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-pm, quic_manafm, rafael
On 10/22/24 08:12, Daniel Lezcano wrote:
> On 21/10/2024 21:49, Lukasz Luba wrote:
>>
>>
>> On 10/14/24 10:43, Daniel Lezcano wrote:
>>> The thermal netlink has been extended with more commands which require
>>> an encoding with more information. The generic encoding function puts
>>> the thermal zone id with the command name. It is the unique
>>> parameters.
>>>
>>> The next changes will provide more parameters to the command. Set the
>>> scene for those new parameters by making the encoding function more
>>> generic.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>
> [ ... ]
>
>>> +static thermal_error_t thermal_genl_auto(struct thermal_handler *th,
>>> cmd_cb_t cmd_cb,
>>> + struct cmd_param *param,
>>> + int cmd, int flags, void *arg)
>>> {
>>> struct nl_msg *msg;
>>> void *hdr;
>>> @@ -276,7 +291,7 @@ static thermal_error_t thermal_genl_auto(struct
>>> thermal_handler *th, int id, int
>>> if (!hdr)
>>> return THERMAL_ERROR;
>>> - if (id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id))
>>> + if (cmd_cb && cmd_cb(msg, param))
>>> return THERMAL_ERROR;
>>
>> It's not in this code but also in older:
>> shouldn't we free the nlmsg_free(msg); before returns in this
>> function?
>
> Right, thanks for pointing this out
>
> If it is ok, I will send a patch on top of this series to fix the entire
> function
>
>
That fine for me, unless Rafael wants a v6 version with those
new locking. I'm OK with both, I will review any version.
BTW I'm still struggling to cross-build or native build that
libthermal, so I cannot give you test results.
I need to sort our the headers and OS packages on dev board...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI
2024-10-22 7:49 ` Daniel Lezcano
@ 2024-10-22 9:50 ` Lukasz Luba
2024-10-22 13:21 ` Daniel Lezcano
0 siblings, 1 reply; 26+ messages in thread
From: Lukasz Luba @ 2024-10-22 9:50 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-pm, quic_manafm, rafael
On 10/22/24 08:49, Daniel Lezcano wrote:
> On 21/10/2024 22:47, Lukasz Luba wrote:
>>
>>
>> On 10/14/24 10:43, Daniel Lezcano wrote:
>>> The thermal framework supports the thresholds and allows the userspace
>>> to create, delete, flush, get the list of the thresholds as well as
>>> getting the list of the thresholds set for a specific thermal zone.
>>>
>>> Add the netlink abstraction in the thermal library to take full
>>> advantage of thresholds for the userspace program.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>> tools/lib/thermal/commands.c | 128 +++++++++++++++++++++++++++-
>>> tools/lib/thermal/events.c | 55 +++++++++---
>>> tools/lib/thermal/include/thermal.h | 40 +++++++++
>>> tools/lib/thermal/libthermal.map | 5 ++
>>> tools/lib/thermal/thermal.c | 17 ++++
>>> tools/thermal/lib/Makefile | 2 +-
>>> 6 files changed, 232 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
>>> index a9223df91dcf..9d5e3e891628 100644
>>> --- a/tools/lib/thermal/commands.c
>>> +++ b/tools/lib/thermal/commands.c
>>> @@ -5,6 +5,7 @@
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <unistd.h>
>>> +#include <limits.h>
>>> #include <thermal.h>
>>> #include "thermal_nl.h"
>>> @@ -33,6 +34,11 @@ static struct nla_policy
>>> thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
>>> [THERMAL_GENL_ATTR_CDEV_CUR_STATE] = { .type = NLA_U32 },
>>> [THERMAL_GENL_ATTR_CDEV_MAX_STATE] = { .type = NLA_U32 },
>>> [THERMAL_GENL_ATTR_CDEV_NAME] = { .type = NLA_STRING },
>>> +
>>> + /* Thresholds */
>>> + [THERMAL_GENL_ATTR_THRESHOLD] = { .type =
>>> NLA_NESTED },
>>> + [THERMAL_GENL_ATTR_THRESHOLD_TEMP] = { .type = NLA_U32 },
>>> + [THERMAL_GENL_ATTR_THRESHOLD_DIRECTION] = { .type = NLA_U32 },
>>> };
>>> static int parse_tz_get(struct genl_info *info, struct thermal_zone
>>> **tz)
>>> @@ -182,6 +188,38 @@ static int parse_tz_get_gov(struct genl_info
>>> *info, struct thermal_zone *tz)
>>> return THERMAL_SUCCESS;
>>> }
>>> +static int parse_threshold_get(struct genl_info *info, struct
>>> thermal_zone *tz)
>>> +{
>>> + struct nlattr *attr;
>>> + struct thermal_threshold *__tt = NULL;
>>> + size_t size = 0;
>>
>> why not simply 'int' ?
>
> Because it is used with realloc where the function definition is:
>
> void *realloc(void *_Nullable ptr, size_t size);
>
> and sizeof() return a size_t type.
>
> So in order to not mix the types, size_t is used which I believe is a
> good practice :)
>
>> also: size=0 which has some impact mentioned below.
>>
>>> + int rem;
>>
>> I would make them a descending order, those lines.
>>
>>> +
>>> + nla_for_each_nested(attr, info-
>>> >attrs[THERMAL_GENL_ATTR_THRESHOLD], rem) {
>>> +
>>> + if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_TEMP) {
>>> +
>>> + size++;
>>> +
>>> + __tt = realloc(__tt, sizeof(*__tt) * (size + 2));
>>> + if (!__tt)
>>> + return THERMAL_ERROR;
>>> +
>>> + __tt[size - 1].temperature = nla_get_u32(attr);
>>> + }
>>> +
>>> + if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_DIRECTION)
>>> + __tt[size - 1].direction = nla_get_u32(attr);
>>
>> We probably relay on some order here, because the 'size -1' needs to be
>> done after first 'size++'.
>> If that the case then maybe it's worth a comment. Or if it wasn't
>> intended and there are no strong guarantees, then this needs a fix.
>
> The size contains the size of the array and we want to access the last
> element, size - 1
>
> I will add this sentence above as a comment if it is ok for you
Yes, please add some comment e.g. that size=0 will be then
first modified by the 1st 'if()' so 'size++' will happen
and there is no way that the 2nd 'if()' will trigger before that.
Those 2 'if()' are kind of independent in the code and it's
not obvious from that part of code, why the 2nd 'if()' won't
run at the beginning. The dangerous situation would be:
'__tt[0 - 1].direction = ' assignment, which is due to
'size=0' init value.
>
>>> + }
>>> +
>>> + if (__tt)
>>> + __tt[size].temperature = INT_MAX;
>>> +
>>> + tz->thresholds = __tt;
>>
>> I wonder what would happen to the previous 'tz->thresholds' when
>> we just put new one here... I cannot find other place when it's set.
>>
>> Since we have '*__tt = NULL' then one of the solutions would be
>> to simply call:
>> free(tz->thresholds);
>> tz->thresholds = __tt;
>>
>> Am I missing something, when it might be cleaned in different place?
>
> The caller is supposed to pass a clean empty structure.
>
> Usually, this function is to discover the current configuration, so it
> is a one shot call keeping the structure in memory for the libthermal
> lifecycle.
>
> The events sends updates of the thermal zones. So with the events and
> the initial configuration from the discovery, the userspace is always
> up-to-date with the thermal setup.
So we cannot receive that we have new thresholds?
I thought we will get that information, even in runtime, so the old
memory should be just freed.
>
>
>>> +
>>> + return THERMAL_SUCCESS;
>>> +}
>>> +
>>> static int handle_netlink(struct nl_cache_ops *unused,
>>> struct genl_cmd *cmd,
>>> struct genl_info *info, void *arg)
>>> @@ -210,6 +248,10 @@ static int handle_netlink(struct nl_cache_ops
>>> *unused,
>>> ret = parse_tz_get_gov(info, arg);
>>> break;
>>> + case THERMAL_GENL_CMD_THRESHOLD_GET:
>>> + ret = parse_threshold_get(info, arg);
>>> + break;
>>> +
>>
>> I can see in the kernel part in the funciton:
>> thermal_genl_cmd_doit()
>> that there add, delete, flush also send a response
>> message. Shouldn't be handled here gently, otherwise
>
> No, those are commands and the transaction is done at the netlink level
> to say if the command was successful or not.
Thanks, I see.
>
>>> default:
>>> return THERMAL_ERROR;
>>
>> that 'default' might capture them?
>
> [ ... ]
>
>>> int for_each_thermal_cdev(struct thermal_cdev *cdev, cb_tc_t cb,
>>> void *arg)
>>> {
>>> int i, ret = 0;
>>> @@ -80,6 +94,9 @@ static int __thermal_zone_discover(struct
>>> thermal_zone *tz, void *th)
>>> if (thermal_cmd_get_trip(th, tz) < 0)
>>> return -1;
>>> + if (thermal_cmd_threshold_get(th, tz) < 0)
>>
>> There are only 2 definitions in the enum thermal_error_t.
>> I would just simply checked if it's not 0:
>
>
> Ok
>
> Thanks for the review
>
We're welcome.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-22 9:40 ` Lukasz Luba
@ 2024-10-22 10:01 ` Rafael J. Wysocki
2024-10-22 10:20 ` Lukasz Luba
0 siblings, 1 reply; 26+ messages in thread
From: Rafael J. Wysocki @ 2024-10-22 10:01 UTC (permalink / raw)
To: Lukasz Luba; +Cc: Daniel Lezcano, linux-pm, quic_manafm, rafael
Hi Lukasz,
On Tue, Oct 22, 2024 at 11:39 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Daniel,
>
> On 10/22/24 08:09, Daniel Lezcano wrote:
> > On 22/10/2024 00:02, Lukasz Luba wrote:
> >>
> >>
> >> On 10/14/24 10:43, Daniel Lezcano wrote:
> >>> The thresholds exist but there is no notification neither action code
> >>> related to them yet.
> >>>
> >>> These changes implement the netlink for the notifications when the
> >>> thresholds are crossed, added, deleted or flushed as well as the
> >>> commands which allows to get the list of the thresholds, flush them,
> >>> add and delete.
> >>>
> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>> ---
> >
> > [ ... ]
> >
> >>> +static inline int thermal_notify_threshold_up(const struct
> >>> thermal_zone_device *tz)
> >>> +{
> >>> + return 0;
> >>> +}
> >>
> >> These 'return 0' look a bit odd. We usually use 'return -EINVAL' in
> >> not defined. Although, since we don't check the output of those
> >> functions - we are OK. We just have to remember about these zeros,
> >> one day when we would like to add the check of the return.
> >
> > The return error really depends on the context of the call site. There
> > are other subsystems returning 0 when the service is not enabled (eg.
> > cpufreq.h, devfreq.h, device_cgroup.h, etc ...)
> >
> >
>
> Fair enough. As I said, we would just keep them in mind if we one
> day decide to add the checks of the returns.
>
> I'm waiting for your next version with the new locking scheme that
> Rafael asked and I will add my review tags.
My plan was to take this patch and replace the open-coded locking with
guards when applying it (that would be a mechanical change AFAICS).
I guess it's OK to add a R-by from you in that case?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds
2024-10-22 10:01 ` Rafael J. Wysocki
@ 2024-10-22 10:20 ` Lukasz Luba
0 siblings, 0 replies; 26+ messages in thread
From: Lukasz Luba @ 2024-10-22 10:20 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Daniel Lezcano, linux-pm, quic_manafm
On 10/22/24 11:01, Rafael J. Wysocki wrote:
> Hi Lukasz,
>
> On Tue, Oct 22, 2024 at 11:39 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Daniel,
>>
>> On 10/22/24 08:09, Daniel Lezcano wrote:
>>> On 22/10/2024 00:02, Lukasz Luba wrote:
>>>>
>>>>
>>>> On 10/14/24 10:43, Daniel Lezcano wrote:
>>>>> The thresholds exist but there is no notification neither action code
>>>>> related to them yet.
>>>>>
>>>>> These changes implement the netlink for the notifications when the
>>>>> thresholds are crossed, added, deleted or flushed as well as the
>>>>> commands which allows to get the list of the thresholds, flush them,
>>>>> add and delete.
>>>>>
>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>> ---
>>>
>>> [ ... ]
>>>
>>>>> +static inline int thermal_notify_threshold_up(const struct
>>>>> thermal_zone_device *tz)
>>>>> +{
>>>>> + return 0;
>>>>> +}
>>>>
>>>> These 'return 0' look a bit odd. We usually use 'return -EINVAL' in
>>>> not defined. Although, since we don't check the output of those
>>>> functions - we are OK. We just have to remember about these zeros,
>>>> one day when we would like to add the check of the return.
>>>
>>> The return error really depends on the context of the call site. There
>>> are other subsystems returning 0 when the service is not enabled (eg.
>>> cpufreq.h, devfreq.h, device_cgroup.h, etc ...)
>>>
>>>
>>
>> Fair enough. As I said, we would just keep them in mind if we one
>> day decide to add the checks of the returns.
>>
>> I'm waiting for your next version with the new locking scheme that
>> Rafael asked and I will add my review tags.
>
> My plan was to take this patch and replace the open-coded locking with
> guards when applying it (that would be a mechanical change AFAICS).
>
> I guess it's OK to add a R-by from you in that case?
Yes, please add. I know what you are going to add there, so:
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI
2024-10-22 9:50 ` Lukasz Luba
@ 2024-10-22 13:21 ` Daniel Lezcano
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2024-10-22 13:21 UTC (permalink / raw)
To: Lukasz Luba; +Cc: linux-pm, quic_manafm, rafael
On 22/10/2024 11:50, Lukasz Luba wrote:
>
>
> On 10/22/24 08:49, Daniel Lezcano wrote:
>> On 21/10/2024 22:47, Lukasz Luba wrote:
>>>
>>>
>>> On 10/14/24 10:43, Daniel Lezcano wrote:
>>>> The thermal framework supports the thresholds and allows the userspace
>>>> to create, delete, flush, get the list of the thresholds as well as
>>>> getting the list of the thresholds set for a specific thermal zone.
>>>>
>>>> Add the netlink abstraction in the thermal library to take full
>>>> advantage of thresholds for the userspace program.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
[ ... ]
>>>> + nla_for_each_nested(attr, info-
>>>> >attrs[THERMAL_GENL_ATTR_THRESHOLD], rem) {
>>>> +
>>>> + if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_TEMP) {
>>>> +
>>>> + size++;
>>>> +
>>>> + __tt = realloc(__tt, sizeof(*__tt) * (size + 2));
>>>> + if (!__tt)
>>>> + return THERMAL_ERROR;
>>>> +
>>>> + __tt[size - 1].temperature = nla_get_u32(attr);
>>>> + }
>>>> +
>>>> + if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_DIRECTION)
>>>> + __tt[size - 1].direction = nla_get_u32(attr);
>>>
>>> We probably relay on some order here, because the 'size -1' needs to be
>>> done after first 'size++'.
>>> If that the case then maybe it's worth a comment. Or if it wasn't
>>> intended and there are no strong guarantees, then this needs a fix.
>>
>> The size contains the size of the array and we want to access the last
>> element, size - 1
>>
>> I will add this sentence above as a comment if it is ok for you
>
> Yes, please add some comment e.g. that size=0 will be then
> first modified by the 1st 'if()' so 'size++' will happen
> and there is no way that the 2nd 'if()' will trigger before that.
> Those 2 'if()' are kind of independent in the code and it's
> not obvious from that part of code, why the 2nd 'if()' won't
> run at the beginning. The dangerous situation would be:
> '__tt[0 - 1].direction = ' assignment, which is due to
> 'size=0' init value.
>
>>
>>>> + }
>>>> +
>>>> + if (__tt)
>>>> + __tt[size].temperature = INT_MAX;
>>>> +
>>>> + tz->thresholds = __tt;
>>>
>>> I wonder what would happen to the previous 'tz->thresholds' when
>>> we just put new one here... I cannot find other place when it's set.
>>>
>>> Since we have '*__tt = NULL' then one of the solutions would be
>>> to simply call:
>>> free(tz->thresholds);
>>> tz->thresholds = __tt;
>>>
>>> Am I missing something, when it might be cleaned in different place?
>>
>> The caller is supposed to pass a clean empty structure.
>>
>> Usually, this function is to discover the current configuration, so it
>> is a one shot call keeping the structure in memory for the libthermal
>> lifecycle.
>>
>> The events sends updates of the thermal zones. So with the events and
>> the initial configuration from the discovery, the userspace is always
>> up-to-date with the thermal setup.
>
> So we cannot receive that we have new thresholds?
> I thought we will get that information, even in runtime, so the old
> memory should be just freed.
Sorry may be I was unclear. I meant we will have the list of thresholds
and we will then receive event when they are created, deleted, etc ...
--
<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
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2024-10-22 13:21 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-14 9:43 [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
2024-10-14 9:43 ` [PATCH v5 1/4] thermal/netlink: Add the commands and the events for the thresholds Daniel Lezcano
2024-10-21 10:58 ` Rafael J. Wysocki
2024-10-21 19:42 ` Lukasz Luba
2024-10-21 19:47 ` Rafael J. Wysocki
2024-10-21 19:51 ` Lukasz Luba
2024-10-21 22:02 ` Lukasz Luba
2024-10-22 7:09 ` Daniel Lezcano
2024-10-22 9:40 ` Lukasz Luba
2024-10-22 10:01 ` Rafael J. Wysocki
2024-10-22 10:20 ` Lukasz Luba
2024-10-14 9:43 ` [PATCH v5 2/4] tools/lib/thermal: Make more generic the command encoding function Daniel Lezcano
2024-10-21 19:49 ` Lukasz Luba
2024-10-22 7:12 ` Daniel Lezcano
2024-10-22 9:43 ` Lukasz Luba
2024-10-14 9:43 ` [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI Daniel Lezcano
2024-10-21 20:47 ` Lukasz Luba
2024-10-22 7:49 ` Daniel Lezcano
2024-10-22 9:50 ` Lukasz Luba
2024-10-22 13:21 ` Daniel Lezcano
2024-10-14 9:43 ` [PATCH v5 4/4] tools/thermal/thermal-engine: Take into account the thresholds API Daniel Lezcano
2024-10-21 20:10 ` Lukasz Luba
2024-10-22 7:52 ` Daniel Lezcano
2024-10-21 8:28 ` [PATCH v5 0/4] Add thermal user thresholds support Daniel Lezcano
2024-10-21 8:43 ` Lukasz Luba
2024-10-21 11:02 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox