I presented this idea during LPC 2018.
Refer to the following link:
https://linuxplumbersconf.org/event/2/contributions/182/attachments/38/45/LPC_2018_Thermal-Srinivas-Rui.pdf
There was broad agreement about this.
At that time, there was no use case for non x86 systems. But now there is
interest from Linaro community. But this doesn't meet all the requirement for
Linaro use cases. So I think, Daniel Lezcano <[email protected]> is working
on a follow up series.
Thermal actions can be categorized in three types:
1. Urgent actions in micro to less than milli second range
2. Actions at milli seconds scale
3. Non urgent actions but improves thermal management, which improves long
term reliability of the system and also allow to meet safety standards
The first category of actions usually handled by the hardware and firmware to
meet the time scale. Some examples are sudden spike of CPU temperature which
needs immediate action.
Second category of actions are done at OS kernel level. Like graceful shutdown
of a system, when a critical temperature is reached. Also a good place to take
takes actions where there is a direct correlation from a thermal source to a
cooling action (one to one relationship). The Linux thermal subsystem has in
built governors to bind thermal zones to cooling devices.
The third category acts at a scale of several seconds. This types of actions
primarily focused for non CPU components, which takes time to heat up but
once hot will take long time to cool. Like skin temperature or board
temperature. Also there is no one to one relationship. For example skin can be
hot, not only by CPU processing, but GPU, displays, networking, charger and
ambient temperature etc. There are some thermally constraint laptops with 15+
sensors and complex thermal relationship tables tying thermal hotspots to
groups of devices. Also these relationships and threshold changes are context
aware. This is done primary from user space thermal management programs. These
programs primarily relies on the thermal sysfs for getting temperature samples
and to get other notifications of change.
There are several limitations of the thermal subsystem, which makes the user
space management inefficient and complex.
- Temperature needs to be polled from sysfs
There is no way to set thermal thresholds and get notifications. We are getting
around by using the passive trips as threshold and using user space governor to
send uevents. The uevent is used for other non thermal events also. We have
special strings to notify temperature samples.
- We have platforms with 15+ zones, where we need to traverse each directory read
"temp" string and convert to integer to process every second in some cases
- For hotpluggable devices the zones can appear and disappear on fly. We have
to again listen to uevents to find out that.
- Based on the context, firmware updates temperature trips, again combination of
user space governor and uevents are used as workaround.
To be more flexible here /dev/thermal_notify device is created, where user
space can wait for notifications and read events and data. A standard structure
for notifications is defined. Based on the zone, additional thermal sysfs
attributes are added to specify thresholds. More events and attributes can be
added in future based on need. If the kernel config is not defined, there are
are no additional thermal sysfs attributes.
This patchset contains
- A new config for creation of the /dev interface
- Use of the new interface in the core for zone creation, deletion and
temperature samples
- Use of the framework in int340x drivers. More will be added in the next series
This series is based on 4.7-rc1
Srinivas Pandruvada (5):
thermal: Add support for /dev/thermal_notify
thermal: Add notification for zone creation and deletion
thermal: Add support for setting notification thresholds
thermal: Add support for setting polling interval
thermal: int340x: Use new device interface
drivers/thermal/Kconfig | 9 +
drivers/thermal/Makefile | 3 +
.../intel/int340x_thermal/int3403_thermal.c | 3 +
.../int340x_thermal/int340x_thermal_zone.c | 29 +++
.../int340x_thermal/int340x_thermal_zone.h | 7 +
.../processor_thermal_device.c | 1 +
drivers/thermal/thermal_core.c | 11 +
drivers/thermal/thermal_dev_if.c | 195 ++++++++++++++++++
drivers/thermal/thermal_sysfs.c | 168 ++++++++++++++-
include/linux/thermal.h | 33 +++
10 files changed, 457 insertions(+), 2 deletions(-)
create mode 100644 drivers/thermal/thermal_dev_if.c
--
2.25.4
Use the new framework to send notifications for:
- Setting temperature threshold for notification to avoid polling
- Send THERMAL_TRIP_REACHED event on reaching threshold
- Send THERMAL_TRIP_UPDATE when firmware change the the existing trip
temperature
Signed-off-by: Srinivas Pandruvada <[email protected]>
---
.../intel/int340x_thermal/int3403_thermal.c | 3 ++
.../int340x_thermal/int340x_thermal_zone.c | 29 +++++++++++++++++++
.../int340x_thermal/int340x_thermal_zone.h | 7 +++++
.../processor_thermal_device.c | 1 +
4 files changed, 40 insertions(+)
diff --git a/drivers/thermal/intel/int340x_thermal/int3403_thermal.c b/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
index f86cbb125e2f..77c014a113a4 100644
--- a/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
@@ -63,15 +63,18 @@ static void int3403_notify(acpi_handle handle,
switch (event) {
case INT3403_PERF_CHANGED_EVENT:
+ int340x_thermal_send_user_event(obj->int340x_zone, THERMAL_PERF_CHANGED, 0);
break;
case INT3403_THERMAL_EVENT:
int340x_thermal_zone_device_update(obj->int340x_zone,
THERMAL_TRIP_VIOLATED);
+ int340x_thermal_send_user_event(obj->int340x_zone, THERMAL_TRIP_REACHED, 0);
break;
case INT3403_PERF_TRIP_POINT_CHANGED:
int340x_thermal_read_trips(obj->int340x_zone);
int340x_thermal_zone_device_update(obj->int340x_zone,
THERMAL_TRIP_CHANGED);
+ int340x_thermal_send_user_event(obj->int340x_zone, THERMAL_TRIP_UPDATE, 0);
break;
default:
dev_err(&priv->pdev->dev, "Unsupported event [0x%x]\n", event);
diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
index 432213272f1e..9568a2db7afd 100644
--- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
+++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
@@ -146,12 +146,41 @@ static int int340x_thermal_get_trip_hyst(struct thermal_zone_device *zone,
return 0;
}
+static int int340x_thermal_get_thres_low(struct thermal_zone_device *zone, int *temp)
+{
+ struct int34x_thermal_zone *d = zone->devdata;
+
+ *temp = d->aux_trips[0];
+
+ return 0;
+}
+
+static int int340x_thermal_set_thres_low(struct thermal_zone_device *zone, int temp)
+{
+ struct int34x_thermal_zone *d = zone->devdata;
+ acpi_status status;
+
+ if (d->override_ops && d->override_ops->set_trip_temp)
+ return d->override_ops->set_trip_temp(zone, 0, temp);
+
+ status = acpi_execute_simple_method(d->adev->handle, "PAT0",
+ millicelsius_to_deci_kelvin(temp));
+ if (ACPI_FAILURE(status))
+ return -EIO;
+
+ d->aux_trips[0] = temp;
+
+ return 0;
+}
+
static struct thermal_zone_device_ops int340x_thermal_zone_ops = {
.get_temp = int340x_thermal_get_zone_temp,
.get_trip_temp = int340x_thermal_get_trip_temp,
.get_trip_type = int340x_thermal_get_trip_type,
.set_trip_temp = int340x_thermal_set_trip_temp,
.get_trip_hyst = int340x_thermal_get_trip_hyst,
+ .set_temp_thres_low = int340x_thermal_set_thres_low,
+ .get_temp_thres_low = int340x_thermal_get_thres_low,
};
static int int340x_thermal_get_trip_config(acpi_handle handle, char *name,
diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
index 3b4971df1b33..142027e4955f 100644
--- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
+++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
@@ -58,4 +58,11 @@ static inline void int340x_thermal_zone_device_update(
thermal_zone_device_update(tzone->zone, event);
}
+static inline void int340x_thermal_send_user_event(
+ struct int34x_thermal_zone *tzone,
+ enum thermal_device_events event,
+ u64 data)
+{
+ thermal_dev_send_event(tzone->zone->id, event, data);
+}
#endif
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
index 297db1d2d960..e25f01948d33 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
@@ -340,6 +340,7 @@ static void proc_thermal_notify(acpi_handle handle, u32 event, void *data)
proc_thermal_read_ppcc(proc_priv);
int340x_thermal_zone_device_update(proc_priv->int340x_zone,
THERMAL_DEVICE_POWER_CAPABILITY_CHANGED);
+ int340x_thermal_send_user_event(proc_priv->int340x_zone, THERMAL_PERF_CHANGED, 0);
break;
default:
dev_dbg(proc_priv->dev, "Unsupported event [0x%x]\n", event);
--
2.25.4
Add new attributes in thermal syfs when a thermal drivers provides
callbacks for them and CONFIG_THERMAL_USER_EVENT_INTERFACE is defined.
These attribute allow user space to stop polling for temperature.
These attributes are:
- temp_thres_low: Specify a notification temperature for a low
temperature threshold event.
temp_thres_high: Specify a notification temperature for a high
temperature threshold event.
temp_thres_hyst: Specify a change in temperature to send notification
again.
This is implemented by adding additional sysfs attribute group. The
changes in this patch are trivial to add new attributes in thermal
sysfs as done for other attributes.
Signed-off-by: Srinivas Pandruvada <[email protected]>
---
drivers/thermal/thermal_sysfs.c | 136 +++++++++++++++++++++++++++++++-
include/linux/thermal.h | 10 ++-
2 files changed, 143 insertions(+), 3 deletions(-)
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index aa99edb4dff7..aa85424c3ac4 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -215,6 +215,125 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr,
return ret ? ret : sprintf(buf, "%d\n", temperature);
}
+#if IS_ENABLED(CONFIG_THERMAL_USER_EVENT_INTERFACE)
+
+#define create_thres_attr(name) \
+ static ssize_t \
+ name##_show(struct device *dev, struct device_attribute *devattr, \
+ char *buf) \
+ { \
+ struct thermal_zone_device *tz = to_thermal_zone(dev); \
+ int temperature, ret; \
+ \
+ ret = tz->ops->get_##name(tz, &temperature); \
+ \
+ return ret ? ret : sprintf(buf, "%d\n", temperature); \
+ } \
+ \
+ static ssize_t \
+ name##_store(struct device *dev, struct device_attribute *devattr, \
+ const char *buf, size_t count) \
+ { \
+ struct thermal_zone_device *tz = to_thermal_zone(dev); \
+ int temperature, ret; \
+ \
+ if (kstrtoint(buf, 10, &temperature)) \
+ return -EINVAL; \
+ \
+ ret = tz->ops->set_##name(tz, temperature); \
+ return ret ? ret : count; \
+ }
+
+create_thres_attr(temp_thres_low);
+create_thres_attr(temp_thres_high);
+create_thres_attr(temp_thres_hyst);
+
+static int create_user_events_attrs(struct thermal_zone_device *tz)
+{
+ struct attribute **attrs;
+ int index = 0;
+
+ if (tz->ops->get_temp_thres_low)
+ ++index;
+ if (tz->ops->get_temp_thres_high)
+ ++index;
+ if (tz->ops->get_temp_thres_high)
+ ++index;
+
+ /* One additional space for NULL */
+ attrs = kcalloc(index + 1, sizeof(*attrs), GFP_KERNEL);
+ if (!attrs)
+ return -ENOMEM;
+
+ tz->threshold_attrs = kcalloc(index, sizeof(*tz->threshold_attrs), GFP_KERNEL);
+ if (!tz->threshold_attrs) {
+ kfree(attrs);
+ return -ENOMEM;
+ }
+
+ index = 0;
+
+ if (tz->ops->get_temp_thres_low) {
+ snprintf(tz->threshold_attrs[index].name, THERMAL_NAME_LENGTH,
+ "temp_thres_low");
+
+ sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
+ tz->threshold_attrs[index].attr.attr.name =
+ tz->threshold_attrs[index].name;
+ tz->threshold_attrs[index].attr.attr.mode = S_IWUSR | S_IRUGO;
+ tz->threshold_attrs[index].attr.show = temp_thres_low_show;
+ tz->threshold_attrs[index].attr.store = temp_thres_low_store;
+ attrs[index] = &tz->threshold_attrs[index].attr.attr;
+ ++index;
+ }
+ if (tz->ops->get_temp_thres_high) {
+ snprintf(tz->threshold_attrs[index].name, THERMAL_NAME_LENGTH,
+ "temp_thres_high");
+
+ sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
+ tz->threshold_attrs[index].attr.attr.name =
+ tz->threshold_attrs[index].name;
+ tz->threshold_attrs[index].attr.attr.mode = S_IWUSR | S_IRUGO;
+ tz->threshold_attrs[index].attr.show = temp_thres_high_show;
+ tz->threshold_attrs[index].attr.store = temp_thres_high_store;
+ attrs[index] = &tz->threshold_attrs[index].attr.attr;
+ ++index;
+ }
+ if (tz->ops->get_temp_thres_hyst) {
+ snprintf(tz->threshold_attrs[index].name, THERMAL_NAME_LENGTH,
+ "temp_thres_hyst");
+
+ sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
+ tz->threshold_attrs[index].attr.attr.name =
+ tz->threshold_attrs[index].name;
+ tz->threshold_attrs[index].attr.attr.mode = S_IWUSR | S_IRUGO;
+ tz->threshold_attrs[index].attr.show = temp_thres_hyst_show;
+ tz->threshold_attrs[index].attr.store = temp_thres_hyst_store;
+ attrs[index] = &tz->threshold_attrs[index].attr.attr;
+ ++index;
+ }
+ attrs[index] = NULL;
+ tz->threshold_attribute_group.attrs = attrs;
+
+ return 0;
+}
+
+static void delete_user_events_attrs(struct thermal_zone_device *tz)
+{
+ kfree(tz->threshold_attrs);
+ kfree(tz->threshold_attribute_group.attrs);
+}
+#else
+static int create_user_events_attrs(struct thermal_zone_device *tz)
+{
+ return -EINVAL;
+}
+
+static void delete_user_events_attrs(struct thermal_zone_device *tz)
+{
+}
+#endif
+
static ssize_t
passive_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
@@ -625,16 +744,27 @@ int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
{
const struct attribute_group **groups;
int i, size, result;
+ int start = 0;
/* we need one extra for trips and the NULL to terminate the array */
size = ARRAY_SIZE(thermal_zone_attribute_groups) + 2;
+
+ result = create_user_events_attrs(tz);
+ if (!result) {
+ ++size;
+ ++start;
+ }
+
/* This also takes care of API requirement to be NULL terminated */
groups = kcalloc(size, sizeof(*groups), GFP_KERNEL);
if (!groups)
return -ENOMEM;
- for (i = 0; i < size - 2; i++)
- groups[i] = thermal_zone_attribute_groups[i];
+ if (start)
+ groups[0] = &tz->threshold_attribute_group;
+
+ for (i = 0; i < size - 2 - start; i++)
+ groups[i + start] = thermal_zone_attribute_groups[i];
if (tz->trips) {
result = create_trip_attrs(tz, mask);
@@ -660,6 +790,8 @@ void thermal_zone_destroy_device_groups(struct thermal_zone_device *tz)
if (tz->trips)
destroy_trip_attrs(tz);
+ delete_user_events_attrs(tz);
+
kfree(tz->device.groups);
}
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index f5e1e7c6a9a2..ee9d79ace7ce 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -102,6 +102,12 @@ struct thermal_zone_device_ops {
enum thermal_trend *);
int (*notify) (struct thermal_zone_device *, int,
enum thermal_trip_type);
+ int (*set_temp_thres_low)(struct thermal_zone_device *, int);
+ int (*set_temp_thres_high)(struct thermal_zone_device *, int);
+ int (*set_temp_thres_hyst)(struct thermal_zone_device *, int);
+ int (*get_temp_thres_low)(struct thermal_zone_device *, int *);
+ int (*get_temp_thres_high)(struct thermal_zone_device *, int *);
+ int (*get_temp_thres_hyst)(struct thermal_zone_device *, int *);
};
struct thermal_cooling_device_ops {
@@ -208,6 +214,8 @@ struct thermal_zone_device {
struct list_head node;
struct delayed_work poll_queue;
enum thermal_notify_event notify_event;
+ struct attribute_group threshold_attribute_group;
+ struct thermal_attr *threshold_attrs;
};
/**
@@ -558,7 +566,7 @@ enum thermal_device_events {
THERMAL_PERF_CHANGED,
};
-#ifdef CONFIG_THERMAL_USER_EVENT_INTERFACE
+#if IS_ENABLED(CONFIG_THERMAL_USER_EVENT_INTERFACE)
int thermal_dev_send_event(int zone_id, enum thermal_device_events event, u64 event_data);
#else
int thermal_dev_send_event(int zone_id, enum thermal_device_events event, u64 event_data)
--
2.25.4
On 04/05/2020 20:16, Srinivas Pandruvada wrote:
> Add new attributes in thermal syfs when a thermal drivers provides
> callbacks for them and CONFIG_THERMAL_USER_EVENT_INTERFACE is defined.
>
> These attribute allow user space to stop polling for temperature.
>
> These attributes are:
> - temp_thres_low: Specify a notification temperature for a low
> temperature threshold event.
> temp_thres_high: Specify a notification temperature for a high
> temperature threshold event.
> temp_thres_hyst: Specify a change in temperature to send notification
> again.
>
> This is implemented by adding additional sysfs attribute group. The
> changes in this patch are trivial to add new attributes in thermal
> sysfs as done for other attributes.
Isn't it duplicate with the trip point?
> Signed-off-by: Srinivas Pandruvada <[email protected]>
> ---
> drivers/thermal/thermal_sysfs.c | 136 +++++++++++++++++++++++++++++++-
> include/linux/thermal.h | 10 ++-
> 2 files changed, 143 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index aa99edb4dff7..aa85424c3ac4 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -215,6 +215,125 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr,
> return ret ? ret : sprintf(buf, "%d\n", temperature);
> }
>
> +#if IS_ENABLED(CONFIG_THERMAL_USER_EVENT_INTERFACE)
> +
> +#define create_thres_attr(name) \
> + static ssize_t \
> + name##_show(struct device *dev, struct device_attribute *devattr, \
> + char *buf) \
> + { \
> + struct thermal_zone_device *tz = to_thermal_zone(dev); \
> + int temperature, ret; \
> + \
> + ret = tz->ops->get_##name(tz, &temperature); \
> + \
> + return ret ? ret : sprintf(buf, "%d\n", temperature); \
> + } \
> + \
> + static ssize_t \
> + name##_store(struct device *dev, struct device_attribute *devattr, \
> + const char *buf, size_t count) \
> + { \
> + struct thermal_zone_device *tz = to_thermal_zone(dev); \
> + int temperature, ret; \
> + \
> + if (kstrtoint(buf, 10, &temperature)) \
> + return -EINVAL; \
> + \
> + ret = tz->ops->set_##name(tz, temperature); \
> + return ret ? ret : count; \
> + }
> +
> +create_thres_attr(temp_thres_low);
> +create_thres_attr(temp_thres_high);
> +create_thres_attr(temp_thres_hyst);
> +
> +static int create_user_events_attrs(struct thermal_zone_device *tz)
> +{
> + struct attribute **attrs;
> + int index = 0;
> +
> + if (tz->ops->get_temp_thres_low)
> + ++index;
> + if (tz->ops->get_temp_thres_high)
> + ++index;
> + if (tz->ops->get_temp_thres_high)
> + ++index;
> +
> + /* One additional space for NULL */
> + attrs = kcalloc(index + 1, sizeof(*attrs), GFP_KERNEL);
> + if (!attrs)
> + return -ENOMEM;
> +
> + tz->threshold_attrs = kcalloc(index, sizeof(*tz->threshold_attrs), GFP_KERNEL);
> + if (!tz->threshold_attrs) {
> + kfree(attrs);
> + return -ENOMEM;
> + }
> +
> + index = 0;
> +
> + if (tz->ops->get_temp_thres_low) {
> + snprintf(tz->threshold_attrs[index].name, THERMAL_NAME_LENGTH,
> + "temp_thres_low");
> +
> + sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
> + tz->threshold_attrs[index].attr.attr.name =
> + tz->threshold_attrs[index].name;
> + tz->threshold_attrs[index].attr.attr.mode = S_IWUSR | S_IRUGO;
> + tz->threshold_attrs[index].attr.show = temp_thres_low_show;
> + tz->threshold_attrs[index].attr.store = temp_thres_low_store;
> + attrs[index] = &tz->threshold_attrs[index].attr.attr;
> + ++index;
> + }
> + if (tz->ops->get_temp_thres_high) {
> + snprintf(tz->threshold_attrs[index].name, THERMAL_NAME_LENGTH,
> + "temp_thres_high");
> +
> + sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
> + tz->threshold_attrs[index].attr.attr.name =
> + tz->threshold_attrs[index].name;
> + tz->threshold_attrs[index].attr.attr.mode = S_IWUSR | S_IRUGO;
> + tz->threshold_attrs[index].attr.show = temp_thres_high_show;
> + tz->threshold_attrs[index].attr.store = temp_thres_high_store;
> + attrs[index] = &tz->threshold_attrs[index].attr.attr;
> + ++index;
> + }
> + if (tz->ops->get_temp_thres_hyst) {
> + snprintf(tz->threshold_attrs[index].name, THERMAL_NAME_LENGTH,
> + "temp_thres_hyst");
> +
> + sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
> + tz->threshold_attrs[index].attr.attr.name =
> + tz->threshold_attrs[index].name;
> + tz->threshold_attrs[index].attr.attr.mode = S_IWUSR | S_IRUGO;
> + tz->threshold_attrs[index].attr.show = temp_thres_hyst_show;
> + tz->threshold_attrs[index].attr.store = temp_thres_hyst_store;
> + attrs[index] = &tz->threshold_attrs[index].attr.attr;
> + ++index;
> + }
> + attrs[index] = NULL;
> + tz->threshold_attribute_group.attrs = attrs;
> +
> + return 0;
> +}
> +
> +static void delete_user_events_attrs(struct thermal_zone_device *tz)
> +{
> + kfree(tz->threshold_attrs);
> + kfree(tz->threshold_attribute_group.attrs);
> +}
> +#else
> +static int create_user_events_attrs(struct thermal_zone_device *tz)
> +{
> + return -EINVAL;
> +}
> +
> +static void delete_user_events_attrs(struct thermal_zone_device *tz)
> +{
> +}
> +#endif
> +
> static ssize_t
> passive_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> @@ -625,16 +744,27 @@ int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
> {
> const struct attribute_group **groups;
> int i, size, result;
> + int start = 0;
>
> /* we need one extra for trips and the NULL to terminate the array */
> size = ARRAY_SIZE(thermal_zone_attribute_groups) + 2;
> +
> + result = create_user_events_attrs(tz);
> + if (!result) {
> + ++size;
> + ++start;
> + }
> +
> /* This also takes care of API requirement to be NULL terminated */
> groups = kcalloc(size, sizeof(*groups), GFP_KERNEL);
> if (!groups)
> return -ENOMEM;
>
> - for (i = 0; i < size - 2; i++)
> - groups[i] = thermal_zone_attribute_groups[i];
> + if (start)
> + groups[0] = &tz->threshold_attribute_group;
> +
> + for (i = 0; i < size - 2 - start; i++)
> + groups[i + start] = thermal_zone_attribute_groups[i];
>
> if (tz->trips) {
> result = create_trip_attrs(tz, mask);
> @@ -660,6 +790,8 @@ void thermal_zone_destroy_device_groups(struct thermal_zone_device *tz)
> if (tz->trips)
> destroy_trip_attrs(tz);
>
> + delete_user_events_attrs(tz);
> +
> kfree(tz->device.groups);
> }
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index f5e1e7c6a9a2..ee9d79ace7ce 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -102,6 +102,12 @@ struct thermal_zone_device_ops {
> enum thermal_trend *);
> int (*notify) (struct thermal_zone_device *, int,
> enum thermal_trip_type);
> + int (*set_temp_thres_low)(struct thermal_zone_device *, int);
> + int (*set_temp_thres_high)(struct thermal_zone_device *, int);
> + int (*set_temp_thres_hyst)(struct thermal_zone_device *, int);
> + int (*get_temp_thres_low)(struct thermal_zone_device *, int *);
> + int (*get_temp_thres_high)(struct thermal_zone_device *, int *);
> + int (*get_temp_thres_hyst)(struct thermal_zone_device *, int *);
> };
>
> struct thermal_cooling_device_ops {
> @@ -208,6 +214,8 @@ struct thermal_zone_device {
> struct list_head node;
> struct delayed_work poll_queue;
> enum thermal_notify_event notify_event;
> + struct attribute_group threshold_attribute_group;
> + struct thermal_attr *threshold_attrs;
> };
>
> /**
> @@ -558,7 +566,7 @@ enum thermal_device_events {
> THERMAL_PERF_CHANGED,
> };
>
> -#ifdef CONFIG_THERMAL_USER_EVENT_INTERFACE
> +#if IS_ENABLED(CONFIG_THERMAL_USER_EVENT_INTERFACE)
> int thermal_dev_send_event(int zone_id, enum thermal_device_events event, u64 event_data);
> #else
> int thermal_dev_send_event(int zone_id, enum thermal_device_events event, u64 event_data)
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Mon, 2020-05-18 at 18:37 +0200, Daniel Lezcano wrote:
> On 04/05/2020 20:16, Srinivas Pandruvada wrote:
> > Add new attributes in thermal syfs when a thermal drivers provides
> > callbacks for them and CONFIG_THERMAL_USER_EVENT_INTERFACE is
> > defined.
> >
> > These attribute allow user space to stop polling for temperature.
> >
> > These attributes are:
> > - temp_thres_low: Specify a notification temperature for a low
> > temperature threshold event.
> > temp_thres_high: Specify a notification temperature for a high
> > temperature threshold event.
> > temp_thres_hyst: Specify a change in temperature to send
> > notification
> > again.
> >
> > This is implemented by adding additional sysfs attribute group. The
> > changes in this patch are trivial to add new attributes in thermal
> > sysfs as done for other attributes.
>
> Isn't it duplicate with the trip point?
A trip point is where an in-kernel governor takes some action. This is
not same as a notification temperature. For example at trip point
configured by ACPI at 85C, the thermal governor may start aggressive
throttling.
But a user space can set a notification threshold at 80C and start some
active controls like activate some fan to reduce the impact of passive
control on performance.
We need a way to distinguish between temperature notification threshold
and actual trip point. Changing a trip point means that user wants
kernel to throttle at temperature.
Thanks,
Srinivas
>
>
>
>
> > Signed-off-by: Srinivas Pandruvada <
> > [email protected]>
> > ---
> > drivers/thermal/thermal_sysfs.c | 136
> > +++++++++++++++++++++++++++++++-
> > include/linux/thermal.h | 10 ++-
> > 2 files changed, 143 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_sysfs.c
> > b/drivers/thermal/thermal_sysfs.c
> > index aa99edb4dff7..aa85424c3ac4 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -215,6 +215,125 @@ trip_point_hyst_show(struct device *dev,
> > struct device_attribute *attr,
> > return ret ? ret : sprintf(buf, "%d\n", temperature);
> > }
> >
> > +#if IS_ENABLED(CONFIG_THERMAL_USER_EVENT_INTERFACE)
> > +
> > +#define create_thres_attr(name)
> > \
> > + static ssize_t
> > \
> > + name##_show(struct device *dev, struct device_attribute
> > *devattr, \
> > + char *buf) \
> > + { \
> > + struct thermal_zone_device *tz = to_thermal_zone(dev); \
> > + int temperature, ret;
> > \
> > +
> > \
> > + ret = tz->ops->get_##name(tz, &temperature);
> > \
> > +
> > \
> > + return ret ? ret : sprintf(buf, "%d\n", temperature); \
> > + } \
> > +
> > \
> > + static ssize_t
> > \
> > + name##_store(struct device *dev, struct device_attribute
> > *devattr, \
> > + const char *buf, size_t count)
> > \
> > + { \
> > + struct thermal_zone_device *tz = to_thermal_zone(dev);
> > \
> > + int temperature, ret;
> > \
> > +
> > \
> > + if (kstrtoint(buf, 10, &temperature))
> > \
> > + return -EINVAL; \
> > +
> > \
> > + ret = tz->ops->set_##name(tz, temperature); \
> > + return ret ? ret : count; \
> > + }
> > +
> > +create_thres_attr(temp_thres_low);
> > +create_thres_attr(temp_thres_high);
> > +create_thres_attr(temp_thres_hyst);
> > +
> > +static int create_user_events_attrs(struct thermal_zone_device
> > *tz)
> > +{
> > + struct attribute **attrs;
> > + int index = 0;
> > +
> > + if (tz->ops->get_temp_thres_low)
> > + ++index;
> > + if (tz->ops->get_temp_thres_high)
> > + ++index;
> > + if (tz->ops->get_temp_thres_high)
> > + ++index;
> > +
> > + /* One additional space for NULL */
> > + attrs = kcalloc(index + 1, sizeof(*attrs), GFP_KERNEL);
> > + if (!attrs)
> > + return -ENOMEM;
> > +
> > + tz->threshold_attrs = kcalloc(index, sizeof(*tz-
> > >threshold_attrs), GFP_KERNEL);
> > + if (!tz->threshold_attrs) {
> > + kfree(attrs);
> > + return -ENOMEM;
> > + }
> > +
> > + index = 0;
> > +
> > + if (tz->ops->get_temp_thres_low) {
> > + snprintf(tz->threshold_attrs[index].name,
> > THERMAL_NAME_LENGTH,
> > + "temp_thres_low");
> > +
> > + sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
> > + tz->threshold_attrs[index].attr.attr.name =
> > + tz-
> > >threshold_attrs[index].name;
> > + tz->threshold_attrs[index].attr.attr.mode = S_IWUSR |
> > S_IRUGO;
> > + tz->threshold_attrs[index].attr.show =
> > temp_thres_low_show;
> > + tz->threshold_attrs[index].attr.store =
> > temp_thres_low_store;
> > + attrs[index] = &tz->threshold_attrs[index].attr.attr;
> > + ++index;
> > + }
> > + if (tz->ops->get_temp_thres_high) {
> > + snprintf(tz->threshold_attrs[index].name,
> > THERMAL_NAME_LENGTH,
> > + "temp_thres_high");
> > +
> > + sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
> > + tz->threshold_attrs[index].attr.attr.name =
> > + tz-
> > >threshold_attrs[index].name;
> > + tz->threshold_attrs[index].attr.attr.mode = S_IWUSR |
> > S_IRUGO;
> > + tz->threshold_attrs[index].attr.show =
> > temp_thres_high_show;
> > + tz->threshold_attrs[index].attr.store =
> > temp_thres_high_store;
> > + attrs[index] = &tz->threshold_attrs[index].attr.attr;
> > + ++index;
> > + }
> > + if (tz->ops->get_temp_thres_hyst) {
> > + snprintf(tz->threshold_attrs[index].name,
> > THERMAL_NAME_LENGTH,
> > + "temp_thres_hyst");
> > +
> > + sysfs_attr_init(&tz->threshold_attrs[index].attr.attr);
> > + tz->threshold_attrs[index].attr.attr.name =
> > + tz-
> > >threshold_attrs[index].name;
> > + tz->threshold_attrs[index].attr.attr.mode = S_IWUSR |
> > S_IRUGO;
> > + tz->threshold_attrs[index].attr.show =
> > temp_thres_hyst_show;
> > + tz->threshold_attrs[index].attr.store =
> > temp_thres_hyst_store;
> > + attrs[index] = &tz->threshold_attrs[index].attr.attr;
> > + ++index;
> > + }
> > + attrs[index] = NULL;
> > + tz->threshold_attribute_group.attrs = attrs;
> > +
> > + return 0;
> > +}
> > +
> > +static void delete_user_events_attrs(struct thermal_zone_device
> > *tz)
> > +{
> > + kfree(tz->threshold_attrs);
> > + kfree(tz->threshold_attribute_group.attrs);
> > +}
> > +#else
> > +static int create_user_events_attrs(struct thermal_zone_device
> > *tz)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static void delete_user_events_attrs(struct thermal_zone_device
> > *tz)
> > +{
> > +}
> > +#endif
> > +
> > static ssize_t
> > passive_store(struct device *dev, struct device_attribute *attr,
> > const char *buf, size_t count)
> > @@ -625,16 +744,27 @@ int thermal_zone_create_device_groups(struct
> > thermal_zone_device *tz,
> > {
> > const struct attribute_group **groups;
> > int i, size, result;
> > + int start = 0;
> >
> > /* we need one extra for trips and the NULL to terminate the
> > array */
> > size = ARRAY_SIZE(thermal_zone_attribute_groups) + 2;
> > +
> > + result = create_user_events_attrs(tz);
> > + if (!result) {
> > + ++size;
> > + ++start;
> > + }
> > +
> > /* This also takes care of API requirement to be NULL
> > terminated */
> > groups = kcalloc(size, sizeof(*groups), GFP_KERNEL);
> > if (!groups)
> > return -ENOMEM;
> >
> > - for (i = 0; i < size - 2; i++)
> > - groups[i] = thermal_zone_attribute_groups[i];
> > + if (start)
> > + groups[0] = &tz->threshold_attribute_group;
> > +
> > + for (i = 0; i < size - 2 - start; i++)
> > + groups[i + start] = thermal_zone_attribute_groups[i];
> >
> > if (tz->trips) {
> > result = create_trip_attrs(tz, mask);
> > @@ -660,6 +790,8 @@ void thermal_zone_destroy_device_groups(struct
> > thermal_zone_device *tz)
> > if (tz->trips)
> > destroy_trip_attrs(tz);
> >
> > + delete_user_events_attrs(tz);
> > +
> > kfree(tz->device.groups);
> > }
> >
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index f5e1e7c6a9a2..ee9d79ace7ce 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -102,6 +102,12 @@ struct thermal_zone_device_ops {
> > enum thermal_trend *);
> > int (*notify) (struct thermal_zone_device *, int,
> > enum thermal_trip_type);
> > + int (*set_temp_thres_low)(struct thermal_zone_device *, int);
> > + int (*set_temp_thres_high)(struct thermal_zone_device *, int);
> > + int (*set_temp_thres_hyst)(struct thermal_zone_device *, int);
> > + int (*get_temp_thres_low)(struct thermal_zone_device *, int *);
> > + int (*get_temp_thres_high)(struct thermal_zone_device *, int
> > *);
> > + int (*get_temp_thres_hyst)(struct thermal_zone_device *, int
> > *);
> > };
> >
> > struct thermal_cooling_device_ops {
> > @@ -208,6 +214,8 @@ struct thermal_zone_device {
> > struct list_head node;
> > struct delayed_work poll_queue;
> > enum thermal_notify_event notify_event;
> > + struct attribute_group threshold_attribute_group;
> > + struct thermal_attr *threshold_attrs;
> > };
> >
> > /**
> > @@ -558,7 +566,7 @@ enum thermal_device_events {
> > THERMAL_PERF_CHANGED,
> > };
> >
> > -#ifdef CONFIG_THERMAL_USER_EVENT_INTERFACE
> > +#if IS_ENABLED(CONFIG_THERMAL_USER_EVENT_INTERFACE)
> > int thermal_dev_send_event(int zone_id, enum thermal_device_events
> > event, u64 event_data);
> > #else
> > int thermal_dev_send_event(int zone_id, enum thermal_device_events
> > event, u64 event_data)
> >
>
>
On Tue, May 19, 2020 at 5:10 AM Srinivas Pandruvada
<[email protected]> wrote:
>
> On Mon, 2020-05-18 at 18:37 +0200, Daniel Lezcano wrote:
> > On 04/05/2020 20:16, Srinivas Pandruvada wrote:
> > > Add new attributes in thermal syfs when a thermal drivers provides
> > > callbacks for them and CONFIG_THERMAL_USER_EVENT_INTERFACE is
> > > defined.
> > >
> > > These attribute allow user space to stop polling for temperature.
> > >
> > > These attributes are:
> > > - temp_thres_low: Specify a notification temperature for a low
> > > temperature threshold event.
> > > temp_thres_high: Specify a notification temperature for a high
> > > temperature threshold event.
> > > temp_thres_hyst: Specify a change in temperature to send
> > > notification
> > > again.
> > >
> > > This is implemented by adding additional sysfs attribute group. The
> > > changes in this patch are trivial to add new attributes in thermal
> > > sysfs as done for other attributes.
> >
> > Isn't it duplicate with the trip point?
> A trip point is where an in-kernel governor takes some action. This is
> not same as a notification temperature. For example at trip point
> configured by ACPI at 85C, the thermal governor may start aggressive
> throttling.
> But a user space can set a notification threshold at 80C and start some
> active controls like activate some fan to reduce the impact of passive
> control on performance.
Then what is the use of thermal trip type "ACTIVE" ?
> We need a way to distinguish between temperature notification threshold
> and actual trip point. Changing a trip point means that user wants
> kernel to throttle at temperature.
On Mon, May 4, 2020 at 11:47 PM Srinivas Pandruvada
<[email protected]> wrote:
>
> Use the new framework to send notifications for:
> - Setting temperature threshold for notification to avoid polling
> - Send THERMAL_TRIP_REACHED event on reaching threshold
> - Send THERMAL_TRIP_UPDATE when firmware change the the existing trip
> temperature
I am a little confused here. I would've expected the thermal core to
send the THERMAL_TRIP_* notifications, not platform drivers. Why
shouldn't this be done in thermal core?
>
> Signed-off-by: Srinivas Pandruvada <[email protected]>
> ---
> .../intel/int340x_thermal/int3403_thermal.c | 3 ++
> .../int340x_thermal/int340x_thermal_zone.c | 29 +++++++++++++++++++
> .../int340x_thermal/int340x_thermal_zone.h | 7 +++++
> .../processor_thermal_device.c | 1 +
> 4 files changed, 40 insertions(+)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/int3403_thermal.c b/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
> index f86cbb125e2f..77c014a113a4 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
> @@ -63,15 +63,18 @@ static void int3403_notify(acpi_handle handle,
>
> switch (event) {
> case INT3403_PERF_CHANGED_EVENT:
> + int340x_thermal_send_user_event(obj->int340x_zone, THERMAL_PERF_CHANGED, 0);
> break;
> case INT3403_THERMAL_EVENT:
> int340x_thermal_zone_device_update(obj->int340x_zone,
> THERMAL_TRIP_VIOLATED);
> + int340x_thermal_send_user_event(obj->int340x_zone, THERMAL_TRIP_REACHED, 0);
> break;
> case INT3403_PERF_TRIP_POINT_CHANGED:
> int340x_thermal_read_trips(obj->int340x_zone);
> int340x_thermal_zone_device_update(obj->int340x_zone,
> THERMAL_TRIP_CHANGED);
> + int340x_thermal_send_user_event(obj->int340x_zone, THERMAL_TRIP_UPDATE, 0);
> break;
> default:
> dev_err(&priv->pdev->dev, "Unsupported event [0x%x]\n", event);
> diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> index 432213272f1e..9568a2db7afd 100644
> --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> @@ -146,12 +146,41 @@ static int int340x_thermal_get_trip_hyst(struct thermal_zone_device *zone,
> return 0;
> }
>
> +static int int340x_thermal_get_thres_low(struct thermal_zone_device *zone, int *temp)
> +{
> + struct int34x_thermal_zone *d = zone->devdata;
> +
> + *temp = d->aux_trips[0];
> +
> + return 0;
> +}
> +
> +static int int340x_thermal_set_thres_low(struct thermal_zone_device *zone, int temp)
> +{
> + struct int34x_thermal_zone *d = zone->devdata;
> + acpi_status status;
> +
> + if (d->override_ops && d->override_ops->set_trip_temp)
> + return d->override_ops->set_trip_temp(zone, 0, temp);
> +
> + status = acpi_execute_simple_method(d->adev->handle, "PAT0",
> + millicelsius_to_deci_kelvin(temp));
> + if (ACPI_FAILURE(status))
> + return -EIO;
> +
> + d->aux_trips[0] = temp;
> +
> + return 0;
> +}
> +
> static struct thermal_zone_device_ops int340x_thermal_zone_ops = {
> .get_temp = int340x_thermal_get_zone_temp,
> .get_trip_temp = int340x_thermal_get_trip_temp,
> .get_trip_type = int340x_thermal_get_trip_type,
> .set_trip_temp = int340x_thermal_set_trip_temp,
> .get_trip_hyst = int340x_thermal_get_trip_hyst,
> + .set_temp_thres_low = int340x_thermal_set_thres_low,
> + .get_temp_thres_low = int340x_thermal_get_thres_low,
> };
>
> static int int340x_thermal_get_trip_config(acpi_handle handle, char *name,
> diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> index 3b4971df1b33..142027e4955f 100644
> --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> @@ -58,4 +58,11 @@ static inline void int340x_thermal_zone_device_update(
> thermal_zone_device_update(tzone->zone, event);
> }
>
> +static inline void int340x_thermal_send_user_event(
> + struct int34x_thermal_zone *tzone,
> + enum thermal_device_events event,
> + u64 data)
> +{
> + thermal_dev_send_event(tzone->zone->id, event, data);
> +}
> #endif
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> index 297db1d2d960..e25f01948d33 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> @@ -340,6 +340,7 @@ static void proc_thermal_notify(acpi_handle handle, u32 event, void *data)
> proc_thermal_read_ppcc(proc_priv);
> int340x_thermal_zone_device_update(proc_priv->int340x_zone,
> THERMAL_DEVICE_POWER_CAPABILITY_CHANGED);
> + int340x_thermal_send_user_event(proc_priv->int340x_zone, THERMAL_PERF_CHANGED, 0);
> break;
> default:
> dev_dbg(proc_priv->dev, "Unsupported event [0x%x]\n", event);
> --
> 2.25.4
>
On Wed, 2020-05-20 at 09:58 +0530, Amit Kucheria wrote:
> On Tue, May 19, 2020 at 5:10 AM Srinivas Pandruvada
> <[email protected]> wrote:
> > On Mon, 2020-05-18 at 18:37 +0200, Daniel Lezcano wrote:
> > > On 04/05/2020 20:16, Srinivas Pandruvada wrote:
> > > > Add new attributes in thermal syfs when a thermal drivers
> > > > provides
> > > > callbacks for them and CONFIG_THERMAL_USER_EVENT_INTERFACE is
> > > > defined.
> > > >
> > > > These attribute allow user space to stop polling for
> > > > temperature.
> > > >
> > > > These attributes are:
> > > > - temp_thres_low: Specify a notification temperature for a low
> > > > temperature threshold event.
> > > > temp_thres_high: Specify a notification temperature for a high
> > > > temperature threshold event.
> > > > temp_thres_hyst: Specify a change in temperature to send
> > > > notification
> > > > again.
> > > >
> > > > This is implemented by adding additional sysfs attribute group.
> > > > The
> > > > changes in this patch are trivial to add new attributes in
> > > > thermal
> > > > sysfs as done for other attributes.
> > >
> > > Isn't it duplicate with the trip point?
> > A trip point is where an in-kernel governor takes some action. This
> > is
> > not same as a notification temperature. For example at trip point
> > configured by ACPI at 85C, the thermal governor may start
> > aggressive
> > throttling.
> > But a user space can set a notification threshold at 80C and start
> > some
> > active controls like activate some fan to reduce the impact of
> > passive
> > control on performance.
>
> Then what is the use of thermal trip type "ACTIVE" ?
This is an example.
The defaults are set by the OEMs via ACPI. User can't modify that if
they want to optimize for their usage on Linux. There are fan control
daemon's which user use on top.
Thanks,
Srinivas
>
> > We need a way to distinguish between temperature notification
> > threshold
> > and actual trip point. Changing a trip point means that user wants
> > kernel to throttle at temperature.
Hi Srinivas,
On Wed, May 20, 2020 at 11:46 PM Srinivas Pandruvada
<[email protected]> wrote:
>
> On Wed, 2020-05-20 at 09:58 +0530, Amit Kucheria wrote:
> > On Tue, May 19, 2020 at 5:10 AM Srinivas Pandruvada
> > <[email protected]> wrote:
> > > On Mon, 2020-05-18 at 18:37 +0200, Daniel Lezcano wrote:
> > > > On 04/05/2020 20:16, Srinivas Pandruvada wrote:
> > > > > Add new attributes in thermal syfs when a thermal drivers
> > > > > provides
> > > > > callbacks for them and CONFIG_THERMAL_USER_EVENT_INTERFACE is
> > > > > defined.
> > > > >
> > > > > These attribute allow user space to stop polling for
> > > > > temperature.
> > > > >
> > > > > These attributes are:
> > > > > - temp_thres_low: Specify a notification temperature for a low
> > > > > temperature threshold event.
> > > > > temp_thres_high: Specify a notification temperature for a high
> > > > > temperature threshold event.
> > > > > temp_thres_hyst: Specify a change in temperature to send
> > > > > notification
> > > > > again.
> > > > >
> > > > > This is implemented by adding additional sysfs attribute group.
> > > > > The
> > > > > changes in this patch are trivial to add new attributes in
> > > > > thermal
> > > > > sysfs as done for other attributes.
> > > >
> > > > Isn't it duplicate with the trip point?
> > > A trip point is where an in-kernel governor takes some action. This
> > > is
> > > not same as a notification temperature. For example at trip point
> > > configured by ACPI at 85C, the thermal governor may start
> > > aggressive
> > > throttling.
> > > But a user space can set a notification threshold at 80C and start
> > > some
> > > active controls like activate some fan to reduce the impact of
> > > passive
> > > control on performance.
> >
> > Then what is the use of thermal trip type "ACTIVE" ?
> This is an example.
> The defaults are set by the OEMs via ACPI. User can't modify that if
> they want to optimize for their usage on Linux. There are fan control
> daemon's which user use on top.
-ENOPARSE. Are you saying users "can" modify these?
In any case, how is what you described earlier not possible with an
ACTIVE trip point directly wired to the fan as a cooling device or
with a HOT trip point that causes the platform driver to send
notification to userspace where a fan control daemon can do what it
needs to?
Basically, I think the issue of polling is orthogonal to the
introduction of the new attributes introduced in this patch and I
don't understand the reason for these attributes from your commit
description.
> > > We need a way to distinguish between temperature notification
> > > threshold
> > > and actual trip point. Changing a trip point means that user wants
> > > kernel to throttle at temperature.
>
Hi Amit,
On Thu, 2020-05-21 at 10:41 +0530, Amit Kucheria wrote:
> Hi Srinivas,
>
> On Wed, May 20, 2020 at 11:46 PM Srinivas Pandruvada
> <[email protected]> wrote:
> > On Wed, 2020-05-20 at 09:58 +0530, Amit Kucheria wrote:
> > > On Tue, May 19, 2020 at 5:10 AM Srinivas Pandruvada
> > > <[email protected]> wrote:
> > > > On Mon, 2020-05-18 at 18:37 +0200, Daniel Lezcano wrote:
> > > > > On 04/05/2020 20:16, Srinivas Pandruvada wrote:
> > > > > > Add new attributes in thermal syfs when a thermal drivers
> > > > > > provides
> > > > > > callbacks for them and CONFIG_THERMAL_USER_EVENT_INTERFACE
> > > > > > is
> > > > > > defined.
> > > > > >
> > > > > > These attribute allow user space to stop polling for
> > > > > > temperature.
> > > > > >
> > > > > > These attributes are:
> > > > > > - temp_thres_low: Specify a notification temperature for a
> > > > > > low
> > > > > > temperature threshold event.
> > > > > > temp_thres_high: Specify a notification temperature for a
> > > > > > high
> > > > > > temperature threshold event.
> > > > > > temp_thres_hyst: Specify a change in temperature to send
> > > > > > notification
> > > > > > again.
> > > > > >
> > > > > > This is implemented by adding additional sysfs attribute
> > > > > > group.
> > > > > > The
> > > > > > changes in this patch are trivial to add new attributes in
> > > > > > thermal
> > > > > > sysfs as done for other attributes.
> > > > >
> > > > > Isn't it duplicate with the trip point?
> > > > A trip point is where an in-kernel governor takes some action.
> > > > This
> > > > is
> > > > not same as a notification temperature. For example at trip
> > > > point
> > > > configured by ACPI at 85C, the thermal governor may start
> > > > aggressive
> > > > throttling.
> > > > But a user space can set a notification threshold at 80C and
> > > > start
> > > > some
> > > > active controls like activate some fan to reduce the impact of
> > > > passive
> > > > control on performance.
> > >
> > > Then what is the use of thermal trip type "ACTIVE" ?
> > This is an example.
> > The defaults are set by the OEMs via ACPI. User can't modify that
> > if
> > they want to optimize for their usage on Linux. There are fan
> > control
> > daemon's which user use on top.
>
> -ENOPARSE. Are you saying users "can" modify these?
Most of the x86 laptops will not have an active trip as the fan control
is done by embedded controller. This is a safety and regulatory issue.
Even when you have an active trip it will be read only and also ACPI
fan cooling device will have few fix states to control.
There are fine grain controls on top are available outside of thermal
drivers via hwmon or others.
https://wiki.archlinux.org/index.php/Fan_speed_control#ThinkPad_laptops
Like in thermald we have XML config, which can be used to set different
speed levels at different temperatures. Instead of polling of
temperature, these attributes allow notification of temperature
threshold. We currently mimic this behavior via adding a RW passive
trip (The RW passive trips has a well defined usage different than what
we are using for).
There can be already existing RO passive/active trips in that zone
already bound to some cooling device. So from user space we search for
some RW passive trip and hope this is will give notifications. This I
believe is a hack to use a fake trip point for notifications for
temperature thresholds.
Thanks,
Srinivas
>
> In any case, how is what you described earlier not possible with an
> ACTIVE trip point directly wired to the fan as a cooling device or
> with a HOT trip point that causes the platform driver to send
> notification to userspace where a fan control daemon can do what it
> needs to?
>
> Basically, I think the issue of polling is orthogonal to the
> introduction of the new attributes introduced in this patch and I
> don't understand the reason for these attributes from your commit
> description.
>
> > > > We need a way to distinguish between temperature notification
> > > > threshold
> > > > and actual trip point. Changing a trip point means that user
> > > > wants
> > > > kernel to throttle at temperature.