[PATCH v2 0/7] Add thermal thresholds support

[PATCH v2 0/7] Add thermal thresholds support

[Daniel Lezcano]

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 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
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.

A threshold can be added, deleted or flushed. The latter means all
thresholds belonging to a thermal zone will be deleted.
    
When a threshold is added:

 - if the same threshold (temperature and direction) exists, an error
   is returned

 - if a threshold is specified with the same temperature but a
   different direction, the specified direction is added

 - if there is no threshold with the same temperature then it is
   created

When a threshold is deleted:

 - if the same threshold (temperature and direction) exists, it is
   deleted

 - if a threshold is specified with the same temperature but a
   different direction, the specified direction is removed

 - if there is no threshold with the same temperature, then an error
   is returned

When the threshold are flushed:

 - All thresholds related to a thermal zone are deleted

When a threshold is crossed:

 - the userspace does not need to know which threshold(s) have been
   crossed, it will be notified with the thermal zone identifier, the
   current temperature and the previous temperature

 - if multiple thresholds have been crossed between two updates only
   one notification will be send to the userspace, it is pointless to
   send a notification per thresholds crossed as the userspace can
   handle that easily when it has the temperature delta information

All aforementioned actions and events lead to a notification to the
userspace. A threshold change (add, delete and flush) is notified to
the userspace with the process id responsible of the action.

Along with the kernel changes, the thermal library has been extended
to provide the different API to deal with the new threshold netlink
events and commands.

In addition, the thermal-engine skeleton uses these new API by
flushing and adding 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:
  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 (7):
  thermal/core: Compute low and high boundaries in
    thermal_zone_device_update()
  thermal/core: Add thresholds support
  thermal/core: Connect the threshold with the core
  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/Kconfig                       |  15 ++
 drivers/thermal/Makefile                      |   3 +
 drivers/thermal/thermal_core.c                |  21 +-
 drivers/thermal/thermal_core.h                |   6 +-
 drivers/thermal/thermal_netlink.c             | 239 ++++++++++++++++-
 drivers/thermal/thermal_netlink.h             |   7 +
 drivers/thermal/thermal_thresholds.c          | 247 ++++++++++++++++++
 drivers/thermal/thermal_thresholds.h          |  54 ++++
 drivers/thermal/thermal_trip.c                |  27 +-
 include/linux/thermal.h                       |   3 +
 include/uapi/linux/thermal.h                  |  30 ++-
 tools/lib/thermal/commands.c                  | 167 +++++++++++-
 tools/lib/thermal/events.c                    |  58 +++-
 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 | 109 +++++++-
 18 files changed, 972 insertions(+), 78 deletions(-)
 create mode 100644 drivers/thermal/thermal_thresholds.c
 create mode 100644 drivers/thermal/thermal_thresholds.h

-- 
2.43.0

[PATCH v2 1/7] thermal/core: Compute low and high boundaries in thermal_zone_device_update()

[Daniel Lezcano]

In order to set the scene for the thresholds support which have to
manipulate the low and high temperature boundaries for the interrupt
support, we must pass the low and high values to the incoming
thresholds routine.

The variables are set from the thermal_zone_set_trips() where the
function loops the thermal trips to figure out the next and the
previous temperatures to set the interrupt to be triggered when they
are crossed.

These variables will be needed by the function in charge of handling
the thresholds in the incoming changes but they are local to the
aforementioned function thermal_zone_set_trips().

Move the low and high boundaries computation out of the function in
thermal_zone_device_update() so they are accessible from there.

The positive side effect is they are computed in the same loop as
handle_thermal_trip(), so we remove one loop.

Co-developed-by: Rafael J. Wysocki <rjw@rjwysocki.net>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 12 ++++++++++--
 drivers/thermal/thermal_core.h |  2 +-
 drivers/thermal/thermal_trip.c | 27 +--------------------------
 3 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 95c399f94744..166f48071487 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -547,6 +547,7 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz,
 	struct thermal_trip_desc *td;
 	LIST_HEAD(way_down_list);
 	LIST_HEAD(way_up_list);
+	int low = -INT_MAX, high = INT_MAX;
 	int temp, ret;
 
 	if (tz->suspended)
@@ -580,10 +581,17 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz,
 
 	tz->notify_event = event;
 
-	for_each_trip_desc(tz, td)
+	for_each_trip_desc(tz, td) {
 		handle_thermal_trip(tz, td, &way_up_list, &way_down_list);
 
-	thermal_zone_set_trips(tz);
+		if (td->threshold <= tz->temperature && td->threshold > low)
+			low = td->threshold;
+
+		if (td->threshold >= tz->temperature && td->threshold < high)
+			high = td->threshold;
+	}
+
+	thermal_zone_set_trips(tz, low, high);
 
 	list_sort(NULL, &way_up_list, thermal_trip_notify_cmp);
 	list_for_each_entry(td, &way_up_list, notify_list_node)
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 4cf2b7230d04..67a09f90eb95 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -259,7 +259,7 @@ void thermal_governor_update_tz(struct thermal_zone_device *tz,
 
 const char *thermal_trip_type_name(enum thermal_trip_type trip_type);
 
-void thermal_zone_set_trips(struct thermal_zone_device *tz);
+void thermal_zone_set_trips(struct thermal_zone_device *tz, int low, int high);
 int thermal_zone_trip_id(const struct thermal_zone_device *tz,
 			 const struct thermal_trip *trip);
 void thermal_zone_trip_updated(struct thermal_zone_device *tz,
diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
index 06a0554ddc38..1d43ab52e86a 100644
--- a/drivers/thermal/thermal_trip.c
+++ b/drivers/thermal/thermal_trip.c
@@ -61,25 +61,8 @@ int thermal_zone_get_num_trips(struct thermal_zone_device *tz)
 }
 EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips);
 
-/**
- * thermal_zone_set_trips - Computes the next trip points for the driver
- * @tz: a pointer to a thermal zone device structure
- *
- * The function computes the next temperature boundaries by browsing
- * the trip points. The result is the closer low and high trip points
- * to the current temperature. These values are passed to the backend
- * driver to let it set its own notification mechanism (usually an
- * interrupt).
- *
- * This function must be called with tz->lock held. Both tz and tz->ops
- * must be valid pointers.
- *
- * It does not return a value
- */
-void thermal_zone_set_trips(struct thermal_zone_device *tz)
+void thermal_zone_set_trips(struct thermal_zone_device *tz, int low, int high)
 {
-	const struct thermal_trip_desc *td;
-	int low = -INT_MAX, high = INT_MAX;
 	int ret;
 
 	lockdep_assert_held(&tz->lock);
@@ -87,14 +70,6 @@ void thermal_zone_set_trips(struct thermal_zone_device *tz)
 	if (!tz->ops.set_trips)
 		return;
 
-	for_each_trip_desc(tz, td) {
-		if (td->threshold <= tz->temperature && td->threshold > low)
-			low = td->threshold;
-
-		if (td->threshold >= tz->temperature && td->threshold < high)
-			high = td->threshold;
-	}
-
 	/* No need to change trip points */
 	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
 		return;
-- 
2.43.0

Re: [PATCH v2 1/7] thermal/core: Compute low and high boundaries in thermal_zone_device_update()

[Rafael J. Wysocki]

On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> In order to set the scene for the thresholds support which have to
> manipulate the low and high temperature boundaries for the interrupt
> support, we must pass the low and high values to the incoming
> thresholds routine.
>
> The variables are set from the thermal_zone_set_trips() where the
> function loops the thermal trips to figure out the next and the
> previous temperatures to set the interrupt to be triggered when they
> are crossed.
>
> These variables will be needed by the function in charge of handling
> the thresholds in the incoming changes but they are local to the
> aforementioned function thermal_zone_set_trips().
>
> Move the low and high boundaries computation out of the function in
> thermal_zone_device_update() so they are accessible from there.
>
> The positive side effect is they are computed in the same loop as
> handle_thermal_trip(), so we remove one loop.
>
> Co-developed-by: Rafael J. Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Looks good to me and I'd like to apply it earlier separately as I have
material depending on it in the works.

> ---
>  drivers/thermal/thermal_core.c | 12 ++++++++++--
>  drivers/thermal/thermal_core.h |  2 +-
>  drivers/thermal/thermal_trip.c | 27 +--------------------------
>  3 files changed, 12 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 95c399f94744..166f48071487 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -547,6 +547,7 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz,
>         struct thermal_trip_desc *td;
>         LIST_HEAD(way_down_list);
>         LIST_HEAD(way_up_list);
> +       int low = -INT_MAX, high = INT_MAX;
>         int temp, ret;
>
>         if (tz->suspended)
> @@ -580,10 +581,17 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz,
>
>         tz->notify_event = event;
>
> -       for_each_trip_desc(tz, td)
> +       for_each_trip_desc(tz, td) {
>                 handle_thermal_trip(tz, td, &way_up_list, &way_down_list);
>
> -       thermal_zone_set_trips(tz);
> +               if (td->threshold <= tz->temperature && td->threshold > low)
> +                       low = td->threshold;
> +
> +               if (td->threshold >= tz->temperature && td->threshold < high)
> +                       high = td->threshold;
> +       }
> +
> +       thermal_zone_set_trips(tz, low, high);
>
>         list_sort(NULL, &way_up_list, thermal_trip_notify_cmp);
>         list_for_each_entry(td, &way_up_list, notify_list_node)
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 4cf2b7230d04..67a09f90eb95 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -259,7 +259,7 @@ void thermal_governor_update_tz(struct thermal_zone_device *tz,
>
>  const char *thermal_trip_type_name(enum thermal_trip_type trip_type);
>
> -void thermal_zone_set_trips(struct thermal_zone_device *tz);
> +void thermal_zone_set_trips(struct thermal_zone_device *tz, int low, int high);
>  int thermal_zone_trip_id(const struct thermal_zone_device *tz,
>                          const struct thermal_trip *trip);
>  void thermal_zone_trip_updated(struct thermal_zone_device *tz,
> diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> index 06a0554ddc38..1d43ab52e86a 100644
> --- a/drivers/thermal/thermal_trip.c
> +++ b/drivers/thermal/thermal_trip.c
> @@ -61,25 +61,8 @@ int thermal_zone_get_num_trips(struct thermal_zone_device *tz)
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips);
>
> -/**
> - * thermal_zone_set_trips - Computes the next trip points for the driver
> - * @tz: a pointer to a thermal zone device structure
> - *
> - * The function computes the next temperature boundaries by browsing
> - * the trip points. The result is the closer low and high trip points
> - * to the current temperature. These values are passed to the backend
> - * driver to let it set its own notification mechanism (usually an
> - * interrupt).
> - *
> - * This function must be called with tz->lock held. Both tz and tz->ops
> - * must be valid pointers.
> - *
> - * It does not return a value
> - */
> -void thermal_zone_set_trips(struct thermal_zone_device *tz)
> +void thermal_zone_set_trips(struct thermal_zone_device *tz, int low, int high)
>  {
> -       const struct thermal_trip_desc *td;
> -       int low = -INT_MAX, high = INT_MAX;
>         int ret;
>
>         lockdep_assert_held(&tz->lock);
> @@ -87,14 +70,6 @@ void thermal_zone_set_trips(struct thermal_zone_device *tz)
>         if (!tz->ops.set_trips)
>                 return;
>
> -       for_each_trip_desc(tz, td) {
> -               if (td->threshold <= tz->temperature && td->threshold > low)
> -                       low = td->threshold;
> -
> -               if (td->threshold >= tz->temperature && td->threshold < high)
> -                       high = td->threshold;
> -       }
> -
>         /* No need to change trip points */
>         if (tz->prev_low_trip == low && tz->prev_high_trip == high)
>                 return;
> --
> 2.43.0
>

Re: [PATCH v2 1/7] thermal/core: Compute low and high boundaries in thermal_zone_device_update()

[Daniel Lezcano]

On 16/08/2024 13:34, Rafael J. Wysocki wrote:
> On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> In order to set the scene for the thresholds support which have to
>> manipulate the low and high temperature boundaries for the interrupt
>> support, we must pass the low and high values to the incoming
>> thresholds routine.
>>
>> The variables are set from the thermal_zone_set_trips() where the
>> function loops the thermal trips to figure out the next and the
>> previous temperatures to set the interrupt to be triggered when they
>> are crossed.
>>
>> These variables will be needed by the function in charge of handling
>> the thresholds in the incoming changes but they are local to the
>> aforementioned function thermal_zone_set_trips().
>>
>> Move the low and high boundaries computation out of the function in
>> thermal_zone_device_update() so they are accessible from there.
>>
>> The positive side effect is they are computed in the same loop as
>> handle_thermal_trip(), so we remove one loop.
>>
>> Co-developed-by: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Looks good to me and I'd like to apply it earlier separately as I have
> material depending on it in the works.

Sure

-- 
<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

Re: [PATCH v2 1/7] thermal/core: Compute low and high boundaries in thermal_zone_device_update()

[Rafael J. Wysocki]

On Fri, Aug 16, 2024 at 2:06 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 16/08/2024 13:34, Rafael J. Wysocki wrote:
> > On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> In order to set the scene for the thresholds support which have to
> >> manipulate the low and high temperature boundaries for the interrupt
> >> support, we must pass the low and high values to the incoming
> >> thresholds routine.
> >>
> >> The variables are set from the thermal_zone_set_trips() where the
> >> function loops the thermal trips to figure out the next and the
> >> previous temperatures to set the interrupt to be triggered when they
> >> are crossed.
> >>
> >> These variables will be needed by the function in charge of handling
> >> the thresholds in the incoming changes but they are local to the
> >> aforementioned function thermal_zone_set_trips().
> >>
> >> Move the low and high boundaries computation out of the function in
> >> thermal_zone_device_update() so they are accessible from there.
> >>
> >> The positive side effect is they are computed in the same loop as
> >> handle_thermal_trip(), so we remove one loop.
> >>
> >> Co-developed-by: Rafael J. Wysocki <rjw@rjwysocki.net>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >
> > Looks good to me and I'd like to apply it earlier separately as I have
> > material depending on it in the works.
>
> Sure

Applied as 6.12 material, thanks!

[PATCH v2 2/7] thermal/core: Add thresholds support

[Daniel Lezcano]

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 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
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.

A threshold can be added, deleted or flushed. The latter means all
thresholds belonging to a thermal zone will be deleted.

When a threshold is added:

 - if the same threshold (temperature and direction) exists, an error
   is returned

 - if a threshold is specified with the same temperature but a
   different direction, the specified direction is added

 - if there is no threshold with the same temperature then it is
   created

When a threshold is deleted:

 - if the same threshold (temperature and direction) exists, it is
   deleted

 - if a threshold is specified with the same temperature but a
   different direction, the specified direction is removed

 - if there is no threshold with the same temperature, then an error
   is returned

When the threshold are flushed:

 - All thresholds related to a thermal zone are deleted

When a threshold is crossed:

 - the userspace does not need to know which threshold(s) have been
   crossed, it will be notified with the current temperature and the
   previous temperature

 - if multiple thresholds have been crossed between two updates only
   one notification will be send to the userspace, it is pointless to
   send a notification per thresholds crossed as the userspace can
   handle that easily when it has the temperature delta information

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/Kconfig              |  15 ++
 drivers/thermal/Makefile             |   3 +
 drivers/thermal/thermal_core.h       |   4 +
 drivers/thermal/thermal_thresholds.c | 241 +++++++++++++++++++++++++++
 drivers/thermal/thermal_thresholds.h |  57 +++++++
 include/linux/thermal.h              |   3 +
 6 files changed, 323 insertions(+)
 create mode 100644 drivers/thermal/thermal_thresholds.c
 create mode 100644 drivers/thermal/thermal_thresholds.h

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index ed16897584b4..84f9643678d6 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -40,6 +40,21 @@ config THERMAL_DEBUGFS
 	  Say Y to allow the thermal subsystem to collect diagnostic
 	  information that can be accessed via debugfs.
 
+config THERMAL_THRESHOLDS
+	bool "Thermal thresholds notification mechanism"
+	depends on THERMAL_NETLINK
+	help
+	  The userspace implements thermal engines which needs to get
+	  notified when temperature thresholds are crossed the way up
+	  and down. These notification allow them to analyze the
+	  thermal situation of the platform and take decision to
+	  fulfill specific thermal profile like 'balanced',
+	  'performance' or 'power saving'. In addition, the
+	  temperature of the skin sensor is very important in this
+	  case and must be monitored as well.
+
+	  If in doubt, say Y
+
 config THERMAL_EMERGENCY_POWEROFF_DELAY_MS
 	int "Emergency poweroff delay in milli-seconds"
 	default 0
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index ce7a4752ef52..3b991b1a7db4 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -7,6 +7,9 @@ obj-$(CONFIG_THERMAL)		+= thermal_sys.o
 thermal_sys-y			+= thermal_core.o thermal_sysfs.o
 thermal_sys-y			+= thermal_trip.o thermal_helpers.o
 
+# thermal thresholds
+thermal_sys-$(CONFIG_THERMAL_THRESHOLDS)	+= thermal_thresholds.o
+
 # netlink interface to manage the thermal framework
 thermal_sys-$(CONFIG_THERMAL_NETLINK)		+= thermal_netlink.o
 
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 67a09f90eb95..0742c0f03d46 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -13,6 +13,7 @@
 #include <linux/thermal.h>
 
 #include "thermal_netlink.h"
+#include "thermal_thresholds.h"
 #include "thermal_debugfs.h"
 
 struct thermal_trip_desc {
@@ -132,6 +133,9 @@ struct thermal_zone_device {
 	bool resuming;
 #ifdef CONFIG_THERMAL_DEBUGFS
 	struct thermal_debugfs *debugfs;
+#endif
+#ifdef CONFIG_THERMAL_THRESHOLDS
+	struct thresholds *thresholds;
 #endif
 	struct thermal_trip_desc trips[] __counted_by(num_trips);
 };
diff --git a/drivers/thermal/thermal_thresholds.c b/drivers/thermal/thermal_thresholds.c
new file mode 100644
index 000000000000..0241b468cfbd
--- /dev/null
+++ b/drivers/thermal/thermal_thresholds.c
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2024 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * Thermal thresholds
+ */
+#include <linux/list.h>
+#include <linux/list_sort.h>
+#include <linux/slab.h>
+
+#include "thermal_core.h"
+
+struct thresholds {
+	struct list_head list;
+};
+
+int thermal_thresholds_init(struct thermal_zone_device *tz)
+{
+	struct thresholds *thresholds;
+
+	thresholds = kmalloc(sizeof(*thresholds), GFP_KERNEL);
+	if (!thresholds)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&thresholds->list);
+	tz->thresholds = thresholds;
+
+	return 0;
+}
+
+void thermal_thresholds_exit(struct thermal_zone_device *tz)
+{
+	thermal_thresholds_flush(tz);
+	kfree(tz->thresholds);
+	tz->thresholds = NULL;
+}
+
+static int __thermal_thresholds_cmp(void *data,
+				    const struct list_head *l1,
+				    const struct list_head *l2)
+{
+	struct threshold *t1 = container_of(l1, struct threshold, list);
+	struct threshold *t2 = container_of(l2, struct threshold, list);
+
+	return t1->temperature - t2->temperature;
+}
+
+static struct threshold *__thermal_thresholds_find(const struct thresholds *thresholds, int temperature)
+{
+	struct threshold *t;
+
+	list_for_each_entry(t, &thresholds->list, list)
+		if (t->temperature == temperature)
+			return t;
+
+	return NULL;
+}
+
+static bool __thermal_threshold_is_crossed(struct threshold *threshold, int temperature,
+					   int last_temperature, int direction,
+					   int *low, int *high)
+{
+	if (temperature > threshold->temperature && threshold->temperature > *low &&
+	    (THERMAL_THRESHOLD_WAY_DOWN & threshold->direction))
+		*low = threshold->temperature;
+
+	if (temperature < threshold->temperature && threshold->temperature < *high &&
+	    (THERMAL_THRESHOLD_WAY_UP & threshold->direction))
+		*high = threshold->temperature;
+
+	if (temperature < threshold->temperature &&
+	    last_temperature >= threshold->temperature &&
+	    (threshold->direction & direction))
+		return true;
+
+	if (temperature >= threshold->temperature &&
+	    last_temperature < threshold->temperature &&
+	    (threshold->direction & direction))
+		return true;
+
+	return false;
+}
+
+static bool thermal_thresholds_handle_raising(struct thresholds *thresholds, int temperature,
+					      int last_temperature, int *low, int *high)
+{
+	struct threshold *t;
+
+	list_for_each_entry(t, &thresholds->list, list) {
+		if (__thermal_threshold_is_crossed(t, temperature, last_temperature,
+						   THERMAL_THRESHOLD_WAY_UP, low, high))
+			return true;
+	}
+
+	return false;
+}
+
+static bool thermal_thresholds_handle_dropping(struct thresholds *thresholds, int temperature,
+					       int last_temperature, int *low, int *high)
+{
+	struct threshold *t;
+
+	list_for_each_entry_reverse(t, &thresholds->list, list) {
+		if (__thermal_threshold_is_crossed(t, temperature, last_temperature,
+						   THERMAL_THRESHOLD_WAY_DOWN, low, high))
+			return true;
+	}
+
+	return false;
+}
+
+void thermal_thresholds_flush(struct thermal_zone_device *tz)
+{
+	struct thresholds *thresholds = tz->thresholds;
+	struct threshold *entry, *tmp;
+
+	lockdep_assert_held(&tz->lock);
+
+	list_for_each_entry_safe(entry, tmp, &thresholds->list, list) {
+		list_del(&entry->list);
+		kfree(entry);
+	}
+
+	__thermal_zone_device_update(tz, THERMAL_THRESHOLD_FLUSHED);
+}
+
+int thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high)
+{
+	struct thresholds *thresholds = tz->thresholds;
+
+	int temperature = tz->temperature;
+	int last_temperature = tz->last_temperature;
+	bool notify;
+
+	lockdep_assert_held(&tz->lock);
+
+	/*
+	 * We need a second update in order to detect a threshold being crossed
+	 */
+	if (last_temperature == THERMAL_TEMP_INVALID)
+		return 0;
+
+	/*
+	 * The temperature is stable, so obviously we can not have
+	 * crossed a threshold.
+	 */
+	if (last_temperature == temperature)
+		return 0;
+
+	/*
+	 * Since last update the temperature:
+	 * - 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);
+
+	return 0;
+}
+
+int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
+{
+	struct thresholds *thresholds = tz->thresholds;
+	struct threshold *t;
+
+	lockdep_assert_held(&tz->lock);
+
+	t = __thermal_thresholds_find(thresholds, temperature);
+	if (t) {
+		if (t->direction == direction)
+			return -EEXIST;
+
+		t->direction |= direction;
+	} else {
+
+		t = kmalloc(sizeof(*t), GFP_KERNEL);
+		if (!t)
+			return -ENOMEM;
+
+		INIT_LIST_HEAD(&t->list);
+		t->temperature = temperature;
+		t->direction = direction;
+		list_add(&t->list, &thresholds->list);
+		list_sort(NULL, &thresholds->list, __thermal_thresholds_cmp);
+	}
+
+	__thermal_zone_device_update(tz, THERMAL_THRESHOLD_ADDED);
+
+	return 0;
+}
+
+int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
+{
+	struct thresholds *thresholds = tz->thresholds;
+	struct threshold *t;
+
+	lockdep_assert_held(&tz->lock);
+
+	t = __thermal_thresholds_find(thresholds, temperature);
+	if (!t)
+		return -ENOENT;
+
+	if (t->direction == direction) {
+		list_del(&t->list);
+		kfree(t);
+	} else {
+		t->direction &= ~direction;
+	}
+
+	__thermal_zone_device_update(tz, THERMAL_THRESHOLD_DELETED);
+
+	return 0;
+}
+
+int thermal_thresholds_for_each(struct thermal_zone_device *tz,
+				int (*cb)(struct threshold *, void *arg), void *arg)
+{
+	struct thresholds *thresholds = tz->thresholds;
+	struct threshold *entry;
+	int ret;
+
+	lockdep_assert_held(&tz->lock);
+
+	list_for_each_entry(entry, &thresholds->list, list) {
+		ret = cb(entry, arg);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/thermal/thermal_thresholds.h b/drivers/thermal/thermal_thresholds.h
new file mode 100644
index 000000000000..7c8ce150d6d0
--- /dev/null
+++ b/drivers/thermal/thermal_thresholds.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#define THERMAL_THRESHOLD_WAY_UP   BIT(0)
+#define THERMAL_THRESHOLD_WAY_DOWN BIT(1)
+
+struct threshold {
+	int temperature;
+	int direction;
+	struct list_head list;
+};
+
+#ifdef CONFIG_THERMAL_THRESHOLDS
+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);
+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_handle(struct thermal_zone_device *tz, int *low, int *high);
+int thermal_thresholds_for_each(struct thermal_zone_device *tz,
+				int (*cb)(struct threshold *, void *arg), void *arg);
+#else
+static inline int thermal_thresholds_init(struct thermal_zone_device *tz)
+{
+	return 0;
+}
+
+static inline void thermal_thresholds_exit(struct thermal_zone_device *tz)
+{
+	;
+}
+
+static inline void thermal_thresholds_flush(struct thermal_zone_device *tz)
+{
+	;
+}
+
+static inline int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
+{
+	return 0;
+}
+
+static inline int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
+{
+	return 0;
+}
+
+static inline int thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high)
+{
+	return 0;
+}
+
+static inline int thermal_thresholds_for_each(struct thermal_zone_device *tz,
+					      int (*cb)(struct threshold *, void *arg), void *arg)
+{
+	return 0;
+}
+#endif
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 25fbf960b474..bf0b4a8218f6 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -55,6 +55,9 @@ enum thermal_notify_event {
 	THERMAL_TZ_BIND_CDEV, /* Cooling dev is bind to the thermal zone */
 	THERMAL_TZ_UNBIND_CDEV, /* Cooling dev is unbind from the thermal zone */
 	THERMAL_INSTANCE_WEIGHT_CHANGED, /* Thermal instance weight changed */
+	THERMAL_THRESHOLD_ADDED, /* Threshold added */
+	THERMAL_THRESHOLD_DELETED, /* Threshold deleted */
+	THERMAL_THRESHOLD_FLUSHED, /* All thresholds deleted */
 };
 
 /**
-- 
2.43.0

Re: [PATCH v2 2/7] thermal/core: Add thresholds support

[Rafael J. Wysocki]

On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
<daniel.lezcano@linaro.org> 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 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
> 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.
>
> A threshold can be added, deleted or flushed. The latter means all
> thresholds belonging to a thermal zone will be deleted.
>
> When a threshold is added:
>
>  - if the same threshold (temperature and direction) exists, an error
>    is returned
>
>  - if a threshold is specified with the same temperature but a
>    different direction, the specified direction is added
>
>  - if there is no threshold with the same temperature then it is
>    created
>
> When a threshold is deleted:
>
>  - if the same threshold (temperature and direction) exists, it is
>    deleted
>
>  - if a threshold is specified with the same temperature but a
>    different direction, the specified direction is removed
>
>  - if there is no threshold with the same temperature, then an error
>    is returned
>
> When the threshold are flushed:
>
>  - All thresholds related to a thermal zone are deleted
>
> When a threshold is crossed:
>
>  - the userspace does not need to know which threshold(s) have been
>    crossed, it will be notified with the current temperature and the
>    previous temperature
>
>  - if multiple thresholds have been crossed between two updates only
>    one notification will be send to the userspace, it is pointless to
>    send a notification per thresholds crossed as the userspace can
>    handle that easily when it has the temperature delta information

The above seems to be an exact copy of the first part of the cover letter.

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/Kconfig              |  15 ++
>  drivers/thermal/Makefile             |   3 +
>  drivers/thermal/thermal_core.h       |   4 +
>  drivers/thermal/thermal_thresholds.c | 241 +++++++++++++++++++++++++++
>  drivers/thermal/thermal_thresholds.h |  57 +++++++
>  include/linux/thermal.h              |   3 +
>  6 files changed, 323 insertions(+)
>  create mode 100644 drivers/thermal/thermal_thresholds.c
>  create mode 100644 drivers/thermal/thermal_thresholds.h
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index ed16897584b4..84f9643678d6 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -40,6 +40,21 @@ config THERMAL_DEBUGFS
>           Say Y to allow the thermal subsystem to collect diagnostic
>           information that can be accessed via debugfs.
>
> +config THERMAL_THRESHOLDS
> +       bool "Thermal thresholds notification mechanism"
> +       depends on THERMAL_NETLINK
> +       help
> +         The userspace implements thermal engines which needs to get
> +         notified when temperature thresholds are crossed the way up
> +         and down. These notification allow them to analyze the
> +         thermal situation of the platform and take decision to
> +         fulfill specific thermal profile like 'balanced',
> +         'performance' or 'power saving'. In addition, the
> +         temperature of the skin sensor is very important in this
> +         case and must be monitored as well.
> +
> +         If in doubt, say Y
> +

I'm not sure if this needs an additional user-selectable Kconfig
option.  It is not modular anyway and not so big, and distros don't
like user-selectable Kconfig options.

>  config THERMAL_EMERGENCY_POWEROFF_DELAY_MS
>         int "Emergency poweroff delay in milli-seconds"
>         default 0
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index ce7a4752ef52..3b991b1a7db4 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -7,6 +7,9 @@ obj-$(CONFIG_THERMAL)           += thermal_sys.o
>  thermal_sys-y                  += thermal_core.o thermal_sysfs.o
>  thermal_sys-y                  += thermal_trip.o thermal_helpers.o
>
> +# thermal thresholds
> +thermal_sys-$(CONFIG_THERMAL_THRESHOLDS)       += thermal_thresholds.o
> +
>  # netlink interface to manage the thermal framework
>  thermal_sys-$(CONFIG_THERMAL_NETLINK)          += thermal_netlink.o
>
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 67a09f90eb95..0742c0f03d46 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -13,6 +13,7 @@
>  #include <linux/thermal.h>
>
>  #include "thermal_netlink.h"
> +#include "thermal_thresholds.h"
>  #include "thermal_debugfs.h"
>
>  struct thermal_trip_desc {
> @@ -132,6 +133,9 @@ struct thermal_zone_device {
>         bool resuming;
>  #ifdef CONFIG_THERMAL_DEBUGFS
>         struct thermal_debugfs *debugfs;
> +#endif
> +#ifdef CONFIG_THERMAL_THRESHOLDS
> +       struct thresholds *thresholds;

Why does it need to be a pointer?

I would just use a plain struct list_head for it anyway.

Also, as stated in my reply to the cover letter, I would prefer to
clearly distinguish these thresholds from trip thresholds, so I would
call this user_thresholds.

>  #endif
>         struct thermal_trip_desc trips[] __counted_by(num_trips);
>  };
> diff --git a/drivers/thermal/thermal_thresholds.c b/drivers/thermal/thermal_thresholds.c
> new file mode 100644
> index 000000000000..0241b468cfbd
> --- /dev/null
> +++ b/drivers/thermal/thermal_thresholds.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2024 Linaro Limited
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * Thermal thresholds
> + */
> +#include <linux/list.h>
> +#include <linux/list_sort.h>
> +#include <linux/slab.h>
> +
> +#include "thermal_core.h"

+#include "thermal_thresholds.h"

> +
> +struct thresholds {
> +       struct list_head list;
> +};

This duplicates the definition in the header file.

Besides, why is the wrapper struct needed?

> +
> +int thermal_thresholds_init(struct thermal_zone_device *tz)
> +{
> +       struct thresholds *thresholds;
> +
> +       thresholds = kmalloc(sizeof(*thresholds), GFP_KERNEL);
> +       if (!thresholds)
> +               return -ENOMEM;
> +
> +       INIT_LIST_HEAD(&thresholds->list);
> +       tz->thresholds = thresholds;
> +
> +       return 0;
> +}

I'd rather embed "thresholds" in struct thermal_zone_device and avoid
allocating memory separately for it.  Less code, less complexity.

> +
> +void thermal_thresholds_exit(struct thermal_zone_device *tz)
> +{
> +       thermal_thresholds_flush(tz);
> +       kfree(tz->thresholds);
> +       tz->thresholds = NULL;
> +}
> +
> +static int __thermal_thresholds_cmp(void *data,
> +                                   const struct list_head *l1,
> +                                   const struct list_head *l2)
> +{
> +       struct threshold *t1 = container_of(l1, struct threshold, list);
> +       struct threshold *t2 = container_of(l2, struct threshold, list);
> +
> +       return t1->temperature - t2->temperature;
> +}
> +
> +static struct threshold *__thermal_thresholds_find(const struct thresholds *thresholds, int temperature)
> +{
> +       struct threshold *t;
> +
> +       list_for_each_entry(t, &thresholds->list, list)
> +               if (t->temperature == temperature)
> +                       return t;
> +
> +       return NULL;
> +}
> +
> +static bool __thermal_threshold_is_crossed(struct threshold *threshold, int temperature,
> +                                          int last_temperature, int direction,
> +                                          int *low, int *high)
> +{
> +       if (temperature > threshold->temperature && threshold->temperature > *low &&
> +           (THERMAL_THRESHOLD_WAY_DOWN & threshold->direction))
> +               *low = threshold->temperature;
> +
> +       if (temperature < threshold->temperature && threshold->temperature < *high &&
> +           (THERMAL_THRESHOLD_WAY_UP & threshold->direction))
> +               *high = threshold->temperature;
> +
> +       if (temperature < threshold->temperature &&
> +           last_temperature >= threshold->temperature &&
> +           (threshold->direction & direction))
> +               return true;
> +
> +       if (temperature >= threshold->temperature &&
> +           last_temperature < threshold->temperature &&
> +           (threshold->direction & direction))
> +               return true;

I would combine the checks, so something like this

if (temperature >= threshold->temperature) {
        if (threshold->temperature > *low &&
THERMAL_THRESHOLD_WAY_DOWN & threshold->direction)
                *low = threshold->temperature;

        if (last_temperature < threshold->temperature &&
threshold->direction & direction)
                return true;
} else {
        if (threshold->temperature < *high && THERMAL_THRESHOLD_WAY_UP
& threshold->direction)
              *high = threshold->temperature;

        if (last_temperature >= threshold->temperature &&
threshold->direction & direction)
                return true;
}

> +
> +       return false;
> +}
> +
> +static bool thermal_thresholds_handle_raising(struct thresholds *thresholds, int temperature,
> +                                             int last_temperature, int *low, int *high)
> +{
> +       struct threshold *t;
> +
> +       list_for_each_entry(t, &thresholds->list, list) {
> +               if (__thermal_threshold_is_crossed(t, temperature, last_temperature,
> +                                                  THERMAL_THRESHOLD_WAY_UP, low, high))
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
> +static bool thermal_thresholds_handle_dropping(struct thresholds *thresholds, int temperature,
> +                                              int last_temperature, int *low, int *high)
> +{
> +       struct threshold *t;
> +
> +       list_for_each_entry_reverse(t, &thresholds->list, list) {
> +               if (__thermal_threshold_is_crossed(t, temperature, last_temperature,
> +                                                  THERMAL_THRESHOLD_WAY_DOWN, low, high))
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
> +void thermal_thresholds_flush(struct thermal_zone_device *tz)
> +{
> +       struct thresholds *thresholds = tz->thresholds;
> +       struct threshold *entry, *tmp;
> +
> +       lockdep_assert_held(&tz->lock);
> +
> +       list_for_each_entry_safe(entry, tmp, &thresholds->list, list) {
> +               list_del(&entry->list);
> +               kfree(entry);
> +       }
> +
> +       __thermal_zone_device_update(tz, THERMAL_THRESHOLD_FLUSHED);
> +}

I'd move the function above before thermal_thresholds_exit() which
uses it.  Having it here is somewhat confusing.

> +
> +int thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high)

This function doesn't return anything other than 0 AFAICS.  Make it void?

> +{
> +       struct thresholds *thresholds = tz->thresholds;
> +
> +       int temperature = tz->temperature;
> +       int last_temperature = tz->last_temperature;
> +       bool notify;
> +
> +       lockdep_assert_held(&tz->lock);
> +
> +       /*
> +        * We need a second update in order to detect a threshold being crossed
> +        */
> +       if (last_temperature == THERMAL_TEMP_INVALID)
> +               return 0;

So user space won't get notified when tz->temperature is above some
thresholds the first time this runs.  Fair enough, but won't they be
confused by subsequent notifications that will not cover some
thresholds?

> +
> +       /*
> +        * The temperature is stable, so obviously we can not have
> +        * crossed a threshold.
> +        */
> +       if (last_temperature == temperature)
> +               return 0;
> +
> +       /*
> +        * Since last update the temperature:
> +        * - 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);
> +
> +       return 0;
> +}
> +
> +int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
> +{
> +       struct thresholds *thresholds = tz->thresholds;

So IMO it would be cleaner to just put "thresholds" into struct
thermal_zone_device directly as a struct list_head because the above
wouldn't be needed then.

> +       struct threshold *t;
> +
> +       lockdep_assert_held(&tz->lock);
> +
> +       t = __thermal_thresholds_find(thresholds, temperature);
> +       if (t) {
> +               if (t->direction == direction)
> +                       return -EEXIST;

Why is it useful to return an error here?

> +
> +               t->direction |= direction;
> +       } else {
> +
> +               t = kmalloc(sizeof(*t), GFP_KERNEL);
> +               if (!t)
> +                       return -ENOMEM;
> +
> +               INIT_LIST_HEAD(&t->list);
> +               t->temperature = temperature;
> +               t->direction = direction;
> +               list_add(&t->list, &thresholds->list);
> +               list_sort(NULL, &thresholds->list, __thermal_thresholds_cmp);

And the above would become

+        list_add(&t->list, &tz->thresholds);
+        list_sort(NULL, &tz->thresholdst, __thermal_thresholds_cmp);

And analogously below.

> +       }
> +
> +       __thermal_zone_device_update(tz, THERMAL_THRESHOLD_ADDED);
> +
> +       return 0;
> +}
> +
> +int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
> +{
> +       struct thresholds *thresholds = tz->thresholds;
> +       struct threshold *t;
> +
> +       lockdep_assert_held(&tz->lock);
> +
> +       t = __thermal_thresholds_find(thresholds, temperature);
> +       if (!t)
> +               return -ENOENT;
> +
> +       if (t->direction == direction) {
> +               list_del(&t->list);
> +               kfree(t);
> +       } else {
> +               t->direction &= ~direction;
> +       }
> +
> +       __thermal_zone_device_update(tz, THERMAL_THRESHOLD_DELETED);
> +
> +       return 0;
> +}
> +
> +int thermal_thresholds_for_each(struct thermal_zone_device *tz,
> +                               int (*cb)(struct threshold *, void *arg), void *arg)
> +{
> +       struct thresholds *thresholds = tz->thresholds;
> +       struct threshold *entry;
> +       int ret;
> +
> +       lockdep_assert_held(&tz->lock);
> +
> +       list_for_each_entry(entry, &thresholds->list, list) {
> +               ret = cb(entry, arg);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> diff --git a/drivers/thermal/thermal_thresholds.h b/drivers/thermal/thermal_thresholds.h
> new file mode 100644
> index 000000000000..7c8ce150d6d0
> --- /dev/null
> +++ b/drivers/thermal/thermal_thresholds.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#define THERMAL_THRESHOLD_WAY_UP   BIT(0)
> +#define THERMAL_THRESHOLD_WAY_DOWN BIT(1)
> +
> +struct threshold {
> +       int temperature;
> +       int direction;
> +       struct list_head list;
> +};

IMO it would be better to put the list field at the top for better
alignment and such.  I would also call it something like list_node.

And I'd call this struct user_threshold as a whole (as per the
previous remarks about the naming).

> +
> +#ifdef CONFIG_THERMAL_THRESHOLDS
> +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);
> +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_handle(struct thermal_zone_device *tz, int *low, int *high);
> +int thermal_thresholds_for_each(struct thermal_zone_device *tz,
> +                               int (*cb)(struct threshold *, void *arg), void *arg);
> +#else
> +static inline int thermal_thresholds_init(struct thermal_zone_device *tz)
> +{
> +       return 0;
> +}
> +
> +static inline void thermal_thresholds_exit(struct thermal_zone_device *tz)
> +{
> +       ;
> +}
> +
> +static inline void thermal_thresholds_flush(struct thermal_zone_device *tz)
> +{
> +       ;
> +}
> +
> +static inline int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
> +{
> +       return 0;
> +}
> +
> +static inline int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
> +{
> +       return 0;
> +}
> +
> +static inline int thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high)
> +{
> +       return 0;
> +}
> +
> +static inline int thermal_thresholds_for_each(struct thermal_zone_device *tz,
> +                                             int (*cb)(struct threshold *, void *arg), void *arg)
> +{
> +       return 0;
> +}
> +#endif
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 25fbf960b474..bf0b4a8218f6 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -55,6 +55,9 @@ enum thermal_notify_event {
>         THERMAL_TZ_BIND_CDEV, /* Cooling dev is bind to the thermal zone */
>         THERMAL_TZ_UNBIND_CDEV, /* Cooling dev is unbind from the thermal zone */
>         THERMAL_INSTANCE_WEIGHT_CHANGED, /* Thermal instance weight changed */

There is an additional item here in the mainline (THERMAL_TZ_RESUME).

> +       THERMAL_THRESHOLD_ADDED, /* Threshold added */
> +       THERMAL_THRESHOLD_DELETED, /* Threshold deleted */
> +       THERMAL_THRESHOLD_FLUSHED, /* All thresholds deleted */

I'd add "TZ" to these names, eg. THERMAL_TZ_THRESHOLD_ADDED, or even
THERMAL_TZ_ADD_THRESHOLD in analogy with the cdev events above.

And THERMAL_TZ_FLUSH_THRESHOLDS sounds more like proper English to me. ;-)

>  };
>
>  /**
> --

Re: [PATCH v2 2/7] thermal/core: Add thresholds support

[Rafael J. Wysocki]

On Wed, Aug 21, 2024 at 10:05 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:

[cut]

> > --- /dev/null
> > +++ b/drivers/thermal/thermal_thresholds.c
> > @@ -0,0 +1,241 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2024 Linaro Limited
> > + *
> > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> > + *
> > + * Thermal thresholds
> > + */
> > +#include <linux/list.h>
> > +#include <linux/list_sort.h>
> > +#include <linux/slab.h>
> > +
> > +#include "thermal_core.h"
>
> +#include "thermal_thresholds.h"
>
> > +
> > +struct thresholds {
> > +       struct list_head list;
> > +};
>
> This duplicates the definition in the header file.
>
> Besides, why is the wrapper struct needed?

On second thought, it can hold a pointer to the first threshold that
was strictly above the zone temperature when
thermal_thresholds_handle() ran last time, so something like:

struct thresholds {
      struct list_head list;
      struct user_threhold *first_above;
};

and first_above == NULL would mean that the zone temperature was above
all of the threshold or at the highest one.

Then thermal_thresholds_handle() could do something like:

tr = tz->user_thresholds.first_above;

if (tr && tz->temperature >= tr->temperature) {
    do {
        if (tr->direction & THERMAL_THRESHOLD_WAY_UP)
            notify = true;

        if (tr->list_node.next != &tz->user_thresholds.list) {
            tr = list_next_entry(tr, list_node);
        else
            tr = NULL;
    } while (tr && tz->temperature >= tr->temperature);
} else {
    if (!tr)
        tr = list_last_entry(&tz->user_thresholds.list,
                     struct user_threshold, list_node);

    while (tz->temperature < tr->temperature)
        if (tr->direction & THERMAL_THRESHOLD_WAY_DOWN)
            notify = true;

        if (tr->list_node.prev != &tz->user_thresholds.list) {
            tr = list_prev_entry(tr, list_node);
        else
            break;
    }
    if (tz->temperature >= tr->temperature)
        tr = NULL;
}
tz->user_thresholds.first_above = tr;

which is a bit simpler than the code in the current patch.

> > +
> > +int thermal_thresholds_init(struct thermal_zone_device *tz)
> > +{
> > +       struct thresholds *thresholds;
> > +
> > +       thresholds = kmalloc(sizeof(*thresholds), GFP_KERNEL);
> > +       if (!thresholds)
> > +               return -ENOMEM;
> > +
> > +       INIT_LIST_HEAD(&thresholds->list);
> > +       tz->thresholds = thresholds;
> > +
> > +       return 0;
> > +}
>
> I'd rather embed "thresholds" in struct thermal_zone_device and avoid
> allocating memory separately for it.  Less code, less complexity.
>
> > +
> > +void thermal_thresholds_exit(struct thermal_zone_device *tz)
> > +{
> > +       thermal_thresholds_flush(tz);
> > +       kfree(tz->thresholds);
> > +       tz->thresholds = NULL;
> > +}
> > +
> > +static int __thermal_thresholds_cmp(void *data,
> > +                                   const struct list_head *l1,
> > +                                   const struct list_head *l2)
> > +{
> > +       struct threshold *t1 = container_of(l1, struct threshold, list);
> > +       struct threshold *t2 = container_of(l2, struct threshold, list);
> > +
> > +       return t1->temperature - t2->temperature;
> > +}
> > +
> > +static struct threshold *__thermal_thresholds_find(const struct thresholds *thresholds, int temperature)
> > +{
> > +       struct threshold *t;
> > +
> > +       list_for_each_entry(t, &thresholds->list, list)
> > +               if (t->temperature == temperature)
> > +                       return t;
> > +
> > +       return NULL;
> > +}
> > +
> > +static bool __thermal_threshold_is_crossed(struct threshold *threshold, int temperature,
> > +                                          int last_temperature, int direction,
> > +                                          int *low, int *high)
> > +{
> > +       if (temperature > threshold->temperature && threshold->temperature > *low &&
> > +           (THERMAL_THRESHOLD_WAY_DOWN & threshold->direction))
> > +               *low = threshold->temperature;
> > +
> > +       if (temperature < threshold->temperature && threshold->temperature < *high &&
> > +           (THERMAL_THRESHOLD_WAY_UP & threshold->direction))
> > +               *high = threshold->temperature;
> > +
> > +       if (temperature < threshold->temperature &&
> > +           last_temperature >= threshold->temperature &&
> > +           (threshold->direction & direction))
> > +               return true;
> > +
> > +       if (temperature >= threshold->temperature &&
> > +           last_temperature < threshold->temperature &&
> > +           (threshold->direction & direction))
> > +               return true;
>
> I would combine the checks, so something like this
>
> if (temperature >= threshold->temperature) {
>         if (threshold->temperature > *low &&
> THERMAL_THRESHOLD_WAY_DOWN & threshold->direction)
>                 *low = threshold->temperature;
>
>         if (last_temperature < threshold->temperature &&
> threshold->direction & direction)
>                 return true;
> } else {
>         if (threshold->temperature < *high && THERMAL_THRESHOLD_WAY_UP
> & threshold->direction)
>               *high = threshold->temperature;
>
>         if (last_temperature >= threshold->temperature &&
> threshold->direction & direction)
>                 return true;
> }

Also, I'm not sure why "high" and "low" are needed at all.

The current and last zone temperature could be included in the
notification just fine and user space should be able to figure out
which thresholds are affected.

> > +
> > +       return false;
> > +}
> > +

Re: [PATCH v2 2/7] thermal/core: Add thresholds support

[Daniel Lezcano]

On 22/08/2024 13:30, Rafael J. Wysocki wrote:
> On Wed, Aug 21, 2024 at 10:05 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
> 
> [cut]
> 
>>> --- /dev/null
>>> +++ b/drivers/thermal/thermal_thresholds.c
>>> @@ -0,0 +1,241 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright 2024 Linaro Limited
>>> + *
>>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> + *
>>> + * Thermal thresholds
>>> + */
>>> +#include <linux/list.h>
>>> +#include <linux/list_sort.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#include "thermal_core.h"
>>
>> +#include "thermal_thresholds.h"
>>
>>> +
>>> +struct thresholds {
>>> +       struct list_head list;
>>> +};
>>
>> This duplicates the definition in the header file.
>>
>> Besides, why is the wrapper struct needed?
> 
> On second thought, it can hold a pointer to the first threshold that
> was strictly above the zone temperature when
> thermal_thresholds_handle() ran last time, so something like:
> 
> struct thresholds {
>        struct list_head list;
>        struct user_threhold *first_above;
> };
> 
> and first_above == NULL would mean that the zone temperature was above
> all of the threshold or at the highest one.
> 
> Then thermal_thresholds_handle() could do something like:
> 
> tr = tz->user_thresholds.first_above;
> 
> if (tr && tz->temperature >= tr->temperature) {
>      do {
>          if (tr->direction & THERMAL_THRESHOLD_WAY_UP)
>              notify = true;
> 
>          if (tr->list_node.next != &tz->user_thresholds.list) {
>              tr = list_next_entry(tr, list_node);
>          else
>              tr = NULL;
>      } while (tr && tz->temperature >= tr->temperature);
> } else {
>      if (!tr)
>          tr = list_last_entry(&tz->user_thresholds.list,
>                       struct user_threshold, list_node);
> 
>      while (tz->temperature < tr->temperature)
>          if (tr->direction & THERMAL_THRESHOLD_WAY_DOWN)
>              notify = true;
> 
>          if (tr->list_node.prev != &tz->user_thresholds.list) {
>              tr = list_prev_entry(tr, list_node);
>          else
>              break;
>      }
>      if (tz->temperature >= tr->temperature)
>          tr = NULL;
> }
> tz->user_thresholds.first_above = tr;
> 
> which is a bit simpler than the code in the current patch.

TBH, it is probably a matter of taste but I don't see how it is simpler, 
especially if someone wants to to understand or add something in the 
code. I would prefer to keep the initial routine which was already tested.

[ ... ]

> 
> Also, I'm not sure why "high" and "low" are needed at all.
> 
> The current and last zone temperature could be included in the
> notification just fine and user space should be able to figure out
> which thresholds are affected.

I'm not sure to get the question. high and low are needed because the 
thermal core will call set_trips(). The trips and the thresholds are 
combined to find out the high and low temperature to program the driver 
to trigger an interrupt.


-- 
<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

Re: [PATCH v2 2/7] thermal/core: Add thresholds support

[Daniel Lezcano]

On 21/08/2024 22:05, Rafael J. Wysocki wrote:
> On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> 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 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
>> 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.
>>
>> A threshold can be added, deleted or flushed. The latter means all
>> thresholds belonging to a thermal zone will be deleted.
>>
>> When a threshold is added:
>>
>>   - if the same threshold (temperature and direction) exists, an error
>>     is returned
>>
>>   - if a threshold is specified with the same temperature but a
>>     different direction, the specified direction is added
>>
>>   - if there is no threshold with the same temperature then it is
>>     created
>>
>> When a threshold is deleted:
>>
>>   - if the same threshold (temperature and direction) exists, it is
>>     deleted
>>
>>   - if a threshold is specified with the same temperature but a
>>     different direction, the specified direction is removed
>>
>>   - if there is no threshold with the same temperature, then an error
>>     is returned
>>
>> When the threshold are flushed:
>>
>>   - All thresholds related to a thermal zone are deleted
>>
>> When a threshold is crossed:
>>
>>   - the userspace does not need to know which threshold(s) have been
>>     crossed, it will be notified with the current temperature and the
>>     previous temperature
>>
>>   - if multiple thresholds have been crossed between two updates only
>>     one notification will be send to the userspace, it is pointless to
>>     send a notification per thresholds crossed as the userspace can
>>     handle that easily when it has the temperature delta information
> 
> The above seems to be an exact copy of the first part of the cover letter.

Yes, it is done on purpose as it is the commit bringing the feature. I 
thought it is convenient for the developer to read the description of 
the commit introducing the feature.

[ ... ]

>> +config THERMAL_THRESHOLDS
>> +       bool "Thermal thresholds notification mechanism"
>> +       depends on THERMAL_NETLINK
>> +       help
>> +         The userspace implements thermal engines which needs to get
>> +         notified when temperature thresholds are crossed the way up
>> +         and down. These notification allow them to analyze the
>> +         thermal situation of the platform and take decision to
>> +         fulfill specific thermal profile like 'balanced',
>> +         'performance' or 'power saving'. In addition, the
>> +         temperature of the skin sensor is very important in this
>> +         case and must be monitored as well.
>> +
>> +         If in doubt, say Y
>> +
> 
> I'm not sure if this needs an additional user-selectable Kconfig
> option.  It is not modular anyway and not so big, and distros don't
> like user-selectable Kconfig options.

Ok, I can drop the user selectable option.

[ ... ]

>> +#endif
>> +#ifdef CONFIG_THERMAL_THRESHOLDS
>> +       struct thresholds *thresholds;
> 
> Why does it need to be a pointer?
> 
> I would just use a plain struct list_head for it anyway.
> 
> Also, as stated in my reply to the cover letter, I would prefer to
> clearly distinguish these thresholds from trip thresholds, so I would
> call this user_thresholds.
> 
>>   #endif
>>          struct thermal_trip_desc trips[] __counted_by(num_trips);
>>   };
>> diff --git a/drivers/thermal/thermal_thresholds.c b/drivers/thermal/thermal_thresholds.c
>> new file mode 100644
>> index 000000000000..0241b468cfbd
>> --- /dev/null
>> +++ b/drivers/thermal/thermal_thresholds.c
>> @@ -0,0 +1,241 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2024 Linaro Limited
>> + *
>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>> + *
>> + * Thermal thresholds
>> + */
>> +#include <linux/list.h>
>> +#include <linux/list_sort.h>
>> +#include <linux/slab.h>
>> +
>> +#include "thermal_core.h"
> 
> +#include "thermal_thresholds.h"
> 
>> +
>> +struct thresholds {
>> +       struct list_head list;
>> +};
> 
> This duplicates the definition in the header file.

No actually it is plural.

struct threshold;
struct thresholds;

> Besides, why is the wrapper struct needed?

Because I think the threshold will continue to evolve. So instead of 
dealing with lists, we use a dedicated structure. In case we add new 
fields or we change the list by something else the functions prototypes 
are untouched. It is my way of coding in order to always set the scene 
for future changes.

>> +int thermal_thresholds_init(struct thermal_zone_device *tz)
>> +{
>> +       struct thresholds *thresholds;
>> +
>> +       thresholds = kmalloc(sizeof(*thresholds), GFP_KERNEL);
>> +       if (!thresholds)
>> +               return -ENOMEM;
>> +
>> +       INIT_LIST_HEAD(&thresholds->list);
>> +       tz->thresholds = thresholds;
>> +
>> +       return 0;
>> +}
> 
> I'd rather embed "thresholds" in struct thermal_zone_device and avoid
> allocating memory separately for it.  Less code, less complexity.

Ok

>> +static bool __thermal_threshold_is_crossed(struct threshold *threshold, int temperature,
>> +                                          int last_temperature, int direction,
>> +                                          int *low, int *high)
>> +{
>> +       if (temperature > threshold->temperature && threshold->temperature > *low &&
>> +           (THERMAL_THRESHOLD_WAY_DOWN & threshold->direction))
>> +               *low = threshold->temperature;
>> +
>> +       if (temperature < threshold->temperature && threshold->temperature < *high &&
>> +           (THERMAL_THRESHOLD_WAY_UP & threshold->direction))
>> +               *high = threshold->temperature;
>> +
>> +       if (temperature < threshold->temperature &&
>> +           last_temperature >= threshold->temperature &&
>> +           (threshold->direction & direction))
>> +               return true;
>> +
>> +       if (temperature >= threshold->temperature &&
>> +           last_temperature < threshold->temperature &&
>> +           (threshold->direction & direction))
>> +               return true;
> 
> I would combine the checks, so something like this
> 
> if (temperature >= threshold->temperature) {
>          if (threshold->temperature > *low &&
> THERMAL_THRESHOLD_WAY_DOWN & threshold->direction)
>                  *low = threshold->temperature;
>          if (last_temperature < threshold->temperature &&
> threshold->direction & direction)
>                  return true;
> } else {
>          if (threshold->temperature < *high && THERMAL_THRESHOLD_WAY_UP
> & threshold->direction)
>                *high = threshold->temperature;
> 
>          if (last_temperature >= threshold->temperature &&
> threshold->direction & direction)
>                  return true;
> }

Ok

[ ... ]

>> +void thermal_thresholds_flush(struct thermal_zone_device *tz)
>> +{
>> +       struct thresholds *thresholds = tz->thresholds;
>> +       struct threshold *entry, *tmp;
>> +
>> +       lockdep_assert_held(&tz->lock);
>> +
>> +       list_for_each_entry_safe(entry, tmp, &thresholds->list, list) {
>> +               list_del(&entry->list);
>> +               kfree(entry);
>> +       }
>> +
>> +       __thermal_zone_device_update(tz, THERMAL_THRESHOLD_FLUSHED);
>> +}
> 
> I'd move the function above before thermal_thresholds_exit() which
> uses it.  Having it here is somewhat confusing.

Sure

>> +
>> +int thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high)
> 
> This function doesn't return anything other than 0 AFAICS.  Make it void?

Ok

>> +{
>> +       struct thresholds *thresholds = tz->thresholds;
>> +
>> +       int temperature = tz->temperature;
>> +       int last_temperature = tz->last_temperature;
>> +       bool notify;
>> +
>> +       lockdep_assert_held(&tz->lock);
>> +
>> +       /*
>> +        * We need a second update in order to detect a threshold being crossed
>> +        */
>> +       if (last_temperature == THERMAL_TEMP_INVALID)
>> +               return 0;
> 
> So user space won't get notified when tz->temperature is above some
> thresholds the first time this runs.  Fair enough, but won't they be
> confused by subsequent notifications that will not cover some
> thresholds?

It is unlikely to happen.

When the thermal zone is created and enabled, it updates the temperature 
of the thermal zone so tz->temperature is no longer THERMAL_TEMP_INVALID.

At this step, there is no userspace set yet.

Assuming there is zero update until we create a threshold. When this one 
is created then the thermal zone is updated, the temperature is read, 
the tz->last_temperature = tz->temperature and tz->temperature has the 
new value. Then handle_thresholds is called and finally set_trips

If the temperature is above or below a threshold then it is detected and 
notified because tz->last_temperature is not equal to THERMAL_TEMP_INVALID.

Others situations are IMO resulting from a bogus driver/sensor and 
should be handled at the sensor level, not the core code.

[ ... ]

>> +int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
>> +{
>> +       struct thresholds *thresholds = tz->thresholds;
> 
> So IMO it would be cleaner to just put "thresholds" into struct
> thermal_zone_device directly as a struct list_head because the above
> wouldn't be needed then.

I can change that to list directly but if we add anything (which can 
probably happen) then the API change as well as anything below again.

>> +       struct threshold *t;
>> +
>> +       lockdep_assert_held(&tz->lock);
>> +
>> +       t = __thermal_thresholds_find(thresholds, temperature);
>> +       if (t) {
>> +               if (t->direction == direction)
>> +                       return -EEXIST;
> 
> Why is it useful to return an error here?

I was expecting this comment :)

We have the choice between :
  * we do nothing
  * we return an error

Let's assume the userspace is misbehaving and because of an internal bug 
of a thermal engine it creates multiple times the same threshold (eg. 
index not incremented, etc ...). If the kernel reports nothing, then the 
user space will never detect this problem. If it reports the error the 
user space can choose to ignore it or follow it up.

The kernel is strict and it is up to the user space to ignore it or not.

>> +
>> +               t->direction |= direction;
>> +       } else {
>> +
>> +               t = kmalloc(sizeof(*t), GFP_KERNEL);
>> +               if (!t)
>> +                       return -ENOMEM;
>> +
>> +               INIT_LIST_HEAD(&t->list);
>> +               t->temperature = temperature;
>> +               t->direction = direction;
>> +               list_add(&t->list, &thresholds->list);
>> +               list_sort(NULL, &thresholds->list, __thermal_thresholds_cmp);
> 
> And the above would become
> 
> +        list_add(&t->list, &tz->thresholds);
> +        list_sort(NULL, &tz->thresholdst, __thermal_thresholds_cmp);
> 
> And analogously below.
> 
>> +       }
>> +
>> +       __thermal_zone_device_update(tz, THERMAL_THRESHOLD_ADDED);
>> +
>> +       return 0;
>> +}

[ ... ]

>> +struct threshold {
>> +       int temperature;
>> +       int direction;
>> +       struct list_head list;
>> +};
> 
> IMO it would be better to put the list field at the top for better
> alignment and such.  I would also call it something like list_node.
> 
> And I'd call this struct user_threshold as a whole (as per the
> previous remarks about the naming).

Ok

[ ... ]

>> @@ -55,6 +55,9 @@ enum thermal_notify_event {
>>          THERMAL_TZ_BIND_CDEV, /* Cooling dev is bind to the thermal zone */
>>          THERMAL_TZ_UNBIND_CDEV, /* Cooling dev is unbind from the thermal zone */
>>          THERMAL_INSTANCE_WEIGHT_CHANGED, /* Thermal instance weight changed */
> 
> There is an additional item here in the mainline (THERMAL_TZ_RESUME).
> 
>> +       THERMAL_THRESHOLD_ADDED, /* Threshold added */
>> +       THERMAL_THRESHOLD_DELETED, /* Threshold deleted */
>> +       THERMAL_THRESHOLD_FLUSHED, /* All thresholds deleted */
> 
> I'd add "TZ" to these names, eg. THERMAL_TZ_THRESHOLD_ADDED, or even
> THERMAL_TZ_ADD_THRESHOLD in analogy with the cdev events above.
> 
> And THERMAL_TZ_FLUSH_THRESHOLDS sounds more like proper English to me. ;-)

Ok, will do the change



-- 
<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

Re: [PATCH v2 2/7] thermal/core: Add thresholds support

[Rafael J. Wysocki]

On Thu, Aug 22, 2024 at 7:20 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 21/08/2024 22:05, Rafael J. Wysocki wrote:
> > On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> 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 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
> >> 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.
> >>
> >> A threshold can be added, deleted or flushed. The latter means all
> >> thresholds belonging to a thermal zone will be deleted.
> >>
> >> When a threshold is added:
> >>
> >>   - if the same threshold (temperature and direction) exists, an error
> >>     is returned
> >>
> >>   - if a threshold is specified with the same temperature but a
> >>     different direction, the specified direction is added
> >>
> >>   - if there is no threshold with the same temperature then it is
> >>     created
> >>
> >> When a threshold is deleted:
> >>
> >>   - if the same threshold (temperature and direction) exists, it is
> >>     deleted
> >>
> >>   - if a threshold is specified with the same temperature but a
> >>     different direction, the specified direction is removed
> >>
> >>   - if there is no threshold with the same temperature, then an error
> >>     is returned
> >>
> >> When the threshold are flushed:
> >>
> >>   - All thresholds related to a thermal zone are deleted
> >>
> >> When a threshold is crossed:
> >>
> >>   - the userspace does not need to know which threshold(s) have been
> >>     crossed, it will be notified with the current temperature and the
> >>     previous temperature
> >>
> >>   - if multiple thresholds have been crossed between two updates only
> >>     one notification will be send to the userspace, it is pointless to
> >>     send a notification per thresholds crossed as the userspace can
> >>     handle that easily when it has the temperature delta information
> >
> > The above seems to be an exact copy of the first part of the cover letter.
>
> Yes, it is done on purpose as it is the commit bringing the feature. I
> thought it is convenient for the developer to read the description of
> the commit introducing the feature.

I agree, but then you could just say "refer to the changelog of the
first patch for details" in the cover letter.

> [ ... ]
>
> >> +config THERMAL_THRESHOLDS
> >> +       bool "Thermal thresholds notification mechanism"
> >> +       depends on THERMAL_NETLINK
> >> +       help
> >> +         The userspace implements thermal engines which needs to get
> >> +         notified when temperature thresholds are crossed the way up
> >> +         and down. These notification allow them to analyze the
> >> +         thermal situation of the platform and take decision to
> >> +         fulfill specific thermal profile like 'balanced',
> >> +         'performance' or 'power saving'. In addition, the
> >> +         temperature of the skin sensor is very important in this
> >> +         case and must be monitored as well.
> >> +
> >> +         If in doubt, say Y
> >> +
> >
> > I'm not sure if this needs an additional user-selectable Kconfig
> > option.  It is not modular anyway and not so big, and distros don't
> > like user-selectable Kconfig options.
>
> Ok, I can drop the user selectable option.
>
> [ ... ]
>
> >> +#endif
> >> +#ifdef CONFIG_THERMAL_THRESHOLDS
> >> +       struct thresholds *thresholds;
> >
> > Why does it need to be a pointer?
> >
> > I would just use a plain struct list_head for it anyway.
> >
> > Also, as stated in my reply to the cover letter, I would prefer to
> > clearly distinguish these thresholds from trip thresholds, so I would
> > call this user_thresholds.
> >
> >>   #endif
> >>          struct thermal_trip_desc trips[] __counted_by(num_trips);
> >>   };
> >> diff --git a/drivers/thermal/thermal_thresholds.c b/drivers/thermal/thermal_thresholds.c
> >> new file mode 100644
> >> index 000000000000..0241b468cfbd
> >> --- /dev/null
> >> +++ b/drivers/thermal/thermal_thresholds.c
> >> @@ -0,0 +1,241 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright 2024 Linaro Limited
> >> + *
> >> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> + *
> >> + * Thermal thresholds
> >> + */
> >> +#include <linux/list.h>
> >> +#include <linux/list_sort.h>
> >> +#include <linux/slab.h>
> >> +
> >> +#include "thermal_core.h"
> >
> > +#include "thermal_thresholds.h"
> >
> >> +
> >> +struct thresholds {
> >> +       struct list_head list;
> >> +};
> >
> > This duplicates the definition in the header file.
>
> No actually it is plural.
>
> struct threshold;
> struct thresholds;

I saw the first one, but for some reason I thought the other was there
too, sorry.

But this means that it is missing from thermal_core.h where it is used
in the struct thermal_zone_device definition.

> > Besides, why is the wrapper struct needed?
>
> Because I think the threshold will continue to evolve.

Fair enough, like I said here:

https://lore.kernel.org/linux-pm/CAJZ5v0h=DgBSiFbdmnzSFjEJd6sdBffCODspxmM-G92FN2HGiA@mail.gmail.com/

> So instead of
> dealing with lists, we use a dedicated structure. In case we add new
> fields or we change the list by something else the functions prototypes
> are untouched. It is my way of coding in order to always set the scene
> for future changes.
>
> >> +int thermal_thresholds_init(struct thermal_zone_device *tz)
> >> +{
> >> +       struct thresholds *thresholds;
> >> +
> >> +       thresholds = kmalloc(sizeof(*thresholds), GFP_KERNEL);
> >> +       if (!thresholds)
> >> +               return -ENOMEM;
> >> +
> >> +       INIT_LIST_HEAD(&thresholds->list);
> >> +       tz->thresholds = thresholds;
> >> +
> >> +       return 0;
> >> +}
> >
> > I'd rather embed "thresholds" in struct thermal_zone_device and avoid
> > allocating memory separately for it.  Less code, less complexity.
>
> Ok
>
> >> +static bool __thermal_threshold_is_crossed(struct threshold *threshold, int temperature,
> >> +                                          int last_temperature, int direction,
> >> +                                          int *low, int *high)
> >> +{
> >> +       if (temperature > threshold->temperature && threshold->temperature > *low &&
> >> +           (THERMAL_THRESHOLD_WAY_DOWN & threshold->direction))
> >> +               *low = threshold->temperature;
> >> +
> >> +       if (temperature < threshold->temperature && threshold->temperature < *high &&
> >> +           (THERMAL_THRESHOLD_WAY_UP & threshold->direction))
> >> +               *high = threshold->temperature;
> >> +
> >> +       if (temperature < threshold->temperature &&
> >> +           last_temperature >= threshold->temperature &&
> >> +           (threshold->direction & direction))
> >> +               return true;
> >> +
> >> +       if (temperature >= threshold->temperature &&
> >> +           last_temperature < threshold->temperature &&
> >> +           (threshold->direction & direction))
> >> +               return true;
> >
> > I would combine the checks, so something like this
> >
> > if (temperature >= threshold->temperature) {
> >          if (threshold->temperature > *low &&
> > THERMAL_THRESHOLD_WAY_DOWN & threshold->direction)
> >                  *low = threshold->temperature;
> >          if (last_temperature < threshold->temperature &&
> > threshold->direction & direction)
> >                  return true;
> > } else {
> >          if (threshold->temperature < *high && THERMAL_THRESHOLD_WAY_UP
> > & threshold->direction)
> >                *high = threshold->temperature;
> >
> >          if (last_temperature >= threshold->temperature &&
> > threshold->direction & direction)
> >                  return true;
> > }
>
> Ok
>
> [ ... ]
>
> >> +void thermal_thresholds_flush(struct thermal_zone_device *tz)
> >> +{
> >> +       struct thresholds *thresholds = tz->thresholds;
> >> +       struct threshold *entry, *tmp;
> >> +
> >> +       lockdep_assert_held(&tz->lock);
> >> +
> >> +       list_for_each_entry_safe(entry, tmp, &thresholds->list, list) {
> >> +               list_del(&entry->list);
> >> +               kfree(entry);
> >> +       }
> >> +
> >> +       __thermal_zone_device_update(tz, THERMAL_THRESHOLD_FLUSHED);
> >> +}
> >
> > I'd move the function above before thermal_thresholds_exit() which
> > uses it.  Having it here is somewhat confusing.
>
> Sure
>
> >> +
> >> +int thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high)
> >
> > This function doesn't return anything other than 0 AFAICS.  Make it void?
>
> Ok
>
> >> +{
> >> +       struct thresholds *thresholds = tz->thresholds;
> >> +
> >> +       int temperature = tz->temperature;
> >> +       int last_temperature = tz->last_temperature;
> >> +       bool notify;
> >> +
> >> +       lockdep_assert_held(&tz->lock);
> >> +
> >> +       /*
> >> +        * We need a second update in order to detect a threshold being crossed
> >> +        */
> >> +       if (last_temperature == THERMAL_TEMP_INVALID)
> >> +               return 0;
> >
> > So user space won't get notified when tz->temperature is above some
> > thresholds the first time this runs.  Fair enough, but won't they be
> > confused by subsequent notifications that will not cover some
> > thresholds?
>
> It is unlikely to happen.
>
> When the thermal zone is created and enabled, it updates the temperature
> of the thermal zone so tz->temperature is no longer THERMAL_TEMP_INVALID.
>
> At this step, there is no userspace set yet.
>
> Assuming there is zero update until we create a threshold. When this one
> is created then the thermal zone is updated, the temperature is read,
> the tz->last_temperature = tz->temperature and tz->temperature has the
> new value. Then handle_thresholds is called and finally set_trips
>
> If the temperature is above or below a threshold then it is detected and
> notified because tz->last_temperature is not equal to THERMAL_TEMP_INVALID.
>
> Others situations are IMO resulting from a bogus driver/sensor and
> should be handled at the sensor level, not the core code.
>
> [ ... ]
>
> >> +int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
> >> +{
> >> +       struct thresholds *thresholds = tz->thresholds;
> >
> > So IMO it would be cleaner to just put "thresholds" into struct
> > thermal_zone_device directly as a struct list_head because the above
> > wouldn't be needed then.
>
> I can change that to list directly but if we add anything (which can
> probably happen) then the API change as well as anything below again.

But it can be

 #ifdef CONFIG_THERMAL_DEBUGFS
     struct thermal_debugfs *debugfs;
 #endif
+   struct thresholds thresholds;
     struct thermal_trip_desc trips[] __counted_by(num_trips);
 };

> >> +       struct threshold *t;
> >> +
> >> +       lockdep_assert_held(&tz->lock);
> >> +
> >> +       t = __thermal_thresholds_find(thresholds, temperature);
> >> +       if (t) {
> >> +               if (t->direction == direction)
> >> +                       return -EEXIST;
> >
> > Why is it useful to return an error here?
>
> I was expecting this comment :)
>
> We have the choice between :
>   * we do nothing
>   * we return an error
>
> Let's assume the userspace is misbehaving and because of an internal bug
> of a thermal engine it creates multiple times the same threshold (eg.
> index not incremented, etc ...). If the kernel reports nothing, then the
> user space will never detect this problem. If it reports the error the
> user space can choose to ignore it or follow it up.
>
> The kernel is strict and it is up to the user space to ignore it or not.

Well, I'm not sure.

IMO it will just cause user space code to be more complex in some
valid use cases, but whatever.

[PATCH v2 3/7] thermal/core: Connect the threshold with the core

[Daniel Lezcano]

Initialize, de-initialize and handle the threshold in the same place
than the trip points.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 166f48071487..0a4d7f6e5cf5 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -591,6 +591,8 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz,
 			high = td->threshold;
 	}
 
+	thermal_thresholds_handle(tz, &low, &high);
+
 	thermal_zone_set_trips(tz, low, high);
 
 	list_sort(NULL, &way_up_list, thermal_trip_notify_cmp);
@@ -1561,6 +1563,10 @@ thermal_zone_device_register_with_trips(const char *type,
 			goto unregister;
 	}
 
+	result = thermal_thresholds_init(tz);
+	if (result)
+		goto remove_hwmon;
+
 	mutex_lock(&thermal_list_lock);
 	mutex_lock(&tz->lock);
 	list_add_tail(&tz->node, &thermal_tz_list);
@@ -1581,6 +1587,8 @@ thermal_zone_device_register_with_trips(const char *type,
 
 	return tz;
 
+remove_hwmon:
+	thermal_remove_hwmon_sysfs(tz);
 unregister:
 	device_del(&tz->device);
 release_device:
@@ -1669,6 +1677,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 
 	thermal_set_governor(tz, NULL);
 
+	thermal_thresholds_exit(tz);
 	thermal_remove_hwmon_sysfs(tz);
 	ida_free(&thermal_tz_ida, tz->id);
 	ida_destroy(&tz->ida);
-- 
2.43.0

[PATCH v2 4/7] thermal/netlink: Add the commands and the events for the thresholds

[Daniel Lezcano]

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.

As different processes in userspace can interact with the thresholds,
the process id responsible of the action (add, delete or flush) will
be added in the notification. This way a thermal engine is able to
detect if another process is interfering with the thresholds. A
process id of zero is the kernel as it is by convention usually.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_netlink.c    | 239 ++++++++++++++++++++++++++-
 drivers/thermal/thermal_netlink.h    |   7 +
 drivers/thermal/thermal_thresholds.c |  36 ++--
 drivers/thermal/thermal_thresholds.h |  15 +-
 include/uapi/linux/thermal.h         |  30 +++-
 5 files changed, 290 insertions(+), 37 deletions(-)

diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
index 97157c453630..3febb119a7f0 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,12 +50,19 @@ 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_WAY]	= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_THRESHOLD_PID]	= { .type = NLA_U32 },
 };
 
 struct param {
 	struct nlattr **attrs;
 	struct sk_buff *msg;
 	const char *name;
+	pid_t pid;
 	int tz_id;
 	int cdev_id;
 	int trip_id;
@@ -62,6 +70,8 @@ struct param {
 	int trip_type;
 	int trip_hyst;
 	int temp;
+	int last_temp;
+	int direction;
 	int cdev_state;
 	int cdev_max_state;
 	struct thermal_genl_cpu_caps *cpu_capabilities;
@@ -234,6 +244,36 @@ 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_WAY, p->direction) ||
+	    nla_put_u32(p->msg, THERMAL_GENL_ATTR_THRESHOLD_PID, p->pid))
+		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),
+	    nla_put_u32(p->msg, THERMAL_GENL_ATTR_THRESHOLD_PID, p->pid))
+		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_LAST_TEMP, p->last_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 +286,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 +305,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 +452,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, int pid)
+{
+	struct param p = { .tz_id = tz->id, .temp = temperature, .direction = direction, .pid = pid };
+
+	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, int pid)
+{
+	struct param p = { .tz_id = tz->id, .temp = temperature, .direction = direction, .pid = pid };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_DELETE, &p);
+}
+
+int thermal_notify_threshold_flush(const struct thermal_zone_device *tz, int pid)
+{
+	struct param p = { .tz_id = tz->id, .pid = pid };
+
+	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, .last_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, .last_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,
@@ -575,12 +663,130 @@ static int thermal_genl_cmd_cdev_get(struct param *p)
 	return ret;
 }
 
+static int __thermal_genl_cmd_threshold_get(struct 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_WAY, threshold->direction))
+		return -1;
+
+	return 0;
+}
+
+static int thermal_genl_cmd_threshold_get(struct param *p)
+{
+	struct thermal_zone_device *tz;
+	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]);
+
+	tz = thermal_zone_get_by_id(id);
+	if (!tz)
+		return -EINVAL;
+
+	start_trip = nla_nest_start(msg, THERMAL_GENL_ATTR_THRESHOLD);
+	if (!start_trip)
+		return -EMSGSIZE;
+
+	mutex_lock(&tz->lock);
+	ret = thermal_thresholds_for_each(tz, __thermal_genl_cmd_threshold_get, msg);
+	mutex_unlock(&tz->lock);
+
+	if (ret)
+		return -EMSGSIZE;	
+
+	nla_nest_end(msg, start_trip);
+
+	return 0;
+}
+
+static int thermal_genl_cmd_threshold_add(struct param *p)
+{
+	struct thermal_zone_device *tz;
+	int id, temp, direction, ret = 0;
+
+	if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID] ||
+	    !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP] ||
+	    !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_WAY])
+		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_WAY]);
+
+	tz = thermal_zone_get_by_id(id);
+	if (!tz)
+		return -EINVAL;
+
+	mutex_lock(&tz->lock);
+	ret = thermal_thresholds_add(tz, temp, direction, p->pid);
+	mutex_unlock(&tz->lock);
+
+	return ret;
+}
+
+static int thermal_genl_cmd_threshold_delete(struct param *p)
+{
+	struct thermal_zone_device *tz;
+	int id, temp, direction, ret = 0;
+
+	if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID] ||
+	    !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP] ||
+	    !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_WAY])
+		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_WAY]);
+
+	tz = thermal_zone_get_by_id(id);
+	if (!tz)
+		return -EINVAL;
+
+	mutex_lock(&tz->lock);
+	ret = thermal_thresholds_delete(tz, temp, direction, p->pid);
+	mutex_unlock(&tz->lock);
+
+	return ret;
+}
+
+static int thermal_genl_cmd_threshold_flush(struct param *p)
+{
+	struct thermal_zone_device *tz;
+	int id;
+
+	if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
+		return -EINVAL;
+
+	id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
+
+	tz = thermal_zone_get_by_id(id);
+	if (!tz)
+		return -EINVAL;
+
+	mutex_lock(&tz->lock);
+	thermal_thresholds_flush(tz, p->pid);
+	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,
@@ -623,6 +829,7 @@ static int thermal_genl_cmd_doit(struct sk_buff *skb,
 	if (!msg)
 		return -ENOMEM;
 	p.msg = msg;
+	p.pid = task_tgid_vnr(current);
 
 	hdr = genlmsg_put_reply(msg, info, &thermal_genl_family, 0, cmd);
 	if (!hdr)
@@ -691,6 +898,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 = {
@@ -703,7 +930,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..715acee04f31 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, pid_t pid);
+int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
+				    int temperature, int direction, pid_t pid);
+int thermal_notify_threshold_flush(const struct thermal_zone_device *tz, pid_t pid);
+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)
 {
diff --git a/drivers/thermal/thermal_thresholds.c b/drivers/thermal/thermal_thresholds.c
index 0241b468cfbd..e0cfb4969203 100644
--- a/drivers/thermal/thermal_thresholds.c
+++ b/drivers/thermal/thermal_thresholds.c
@@ -32,7 +32,7 @@ int thermal_thresholds_init(struct thermal_zone_device *tz)
 
 void thermal_thresholds_exit(struct thermal_zone_device *tz)
 {
-	thermal_thresholds_flush(tz);
+	thermal_thresholds_flush(tz, 0);
 	kfree(tz->thresholds);
 	tz->thresholds = NULL;
 }
@@ -111,7 +111,7 @@ static bool thermal_thresholds_handle_dropping(struct thresholds *thresholds, in
 	return false;
 }
 
-void thermal_thresholds_flush(struct thermal_zone_device *tz)
+void thermal_thresholds_flush(struct thermal_zone_device *tz, pid_t pid)
 {
 	struct thresholds *thresholds = tz->thresholds;
 	struct threshold *entry, *tmp;
@@ -123,6 +123,8 @@ void thermal_thresholds_flush(struct thermal_zone_device *tz)
 		kfree(entry);
 	}
 
+	thermal_notify_threshold_flush(tz, pid);
+
 	__thermal_zone_device_update(tz, THERMAL_THRESHOLD_FLUSHED);
 }
 
@@ -132,7 +134,6 @@ int thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *hig
 
 	int temperature = tz->temperature;
 	int last_temperature = tz->last_temperature;
-	bool notify;
 
 	lockdep_assert_held(&tz->lock);
 
@@ -154,21 +155,21 @@ int thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *hig
 	 * - 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);
+	}
 
 	return 0;
 }
 
-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, pid_t pid)
 {
 	struct thresholds *thresholds = tz->thresholds;
 	struct threshold *t;
@@ -194,12 +195,15 @@ int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int
 		list_sort(NULL, &thresholds->list, __thermal_thresholds_cmp);
 	}
 
+	thermal_notify_threshold_add(tz, temperature, direction, pid);
+
 	__thermal_zone_device_update(tz, THERMAL_THRESHOLD_ADDED);
 
 	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, pid_t pid)
 {
 	struct thresholds *thresholds = tz->thresholds;
 	struct threshold *t;
@@ -219,6 +223,8 @@ int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, i
 
 	__thermal_zone_device_update(tz, THERMAL_THRESHOLD_DELETED);
 
+	thermal_notify_threshold_delete(tz, temperature, direction, pid);
+
 	return 0;
 }
 
diff --git a/drivers/thermal/thermal_thresholds.h b/drivers/thermal/thermal_thresholds.h
index 7c8ce150d6d0..b7a149b3400c 100644
--- a/drivers/thermal/thermal_thresholds.h
+++ b/drivers/thermal/thermal_thresholds.h
@@ -1,8 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
-#define THERMAL_THRESHOLD_WAY_UP   BIT(0)
-#define THERMAL_THRESHOLD_WAY_DOWN BIT(1)
-
 struct threshold {
 	int temperature;
 	int direction;
@@ -12,9 +9,9 @@ struct threshold {
 #ifdef CONFIG_THERMAL_THRESHOLDS
 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);
-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);
+void thermal_thresholds_flush(struct thermal_zone_device *tz, pid_t pid);
+int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction, pid_t pid);
+int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction, pid_t pid);
 int thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high);
 int thermal_thresholds_for_each(struct thermal_zone_device *tz,
 				int (*cb)(struct threshold *, void *arg), void *arg);
@@ -29,17 +26,17 @@ static inline void thermal_thresholds_exit(struct thermal_zone_device *tz)
 	;
 }
 
-static inline void thermal_thresholds_flush(struct thermal_zone_device *tz)
+static inline void thermal_thresholds_flush(struct thermal_zone_device *tz, pid_t pid)
 {
 	;
 }
 
-static inline int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
+static inline int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction, pid_t pid)
 {
 	return 0;
 }
 
-static inline int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
+static inline int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction, pid_t pid)
 {
 	return 0;
 }
diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
index fc78bf3aead7..bcbaf62a1727 100644
--- a/include/uapi/linux/thermal.h
+++ b/include/uapi/linux/thermal.h
@@ -3,6 +3,8 @@
 #define _UAPI_LINUX_THERMAL_H
 
 #define THERMAL_NAME_LENGTH	20
+#define THERMAL_THRESHOLD_WAY_UP	0x1
+#define THERMAL_THRESHOLD_WAY_DOWN	0x2
 
 enum thermal_device_mode {
 	THERMAL_DEVICE_DISABLED = 0,
@@ -18,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"
 
@@ -28,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_LAST_TEMP,
 	THERMAL_GENL_ATTR_TZ_TRIP,
 	THERMAL_GENL_ATTR_TZ_TRIP_ID,
 	THERMAL_GENL_ATTR_TZ_TRIP_TYPE,
@@ -48,6 +51,10 @@ 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_WAY,
+	THERMAL_GENL_ATTR_THRESHOLD_PID,
 	__THERMAL_GENL_ATTR_MAX,
 };
 #define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
@@ -75,6 +82,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)
@@ -82,12 +94,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

[PATCH v2 5/7] tools/lib/thermal: Make more generic the command encoding function

[Daniel Lezcano]

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

[PATCH v2 6/7] tools/lib/thermal: Add the threshold netlink ABI

[Daniel Lezcano]

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          |  58 ++++++++++---
 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, 235 insertions(+), 15 deletions(-)

diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
index a9223df91dcf..12539a519b81 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_WAY]       = { .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_WAY)
+			__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_WAY, 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..a009381b30ec 100644
--- a/tools/lib/thermal/events.c
+++ b/tools/lib/thermal/events.c
@@ -94,6 +94,33 @@ 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_WAY]),
+					  nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_PID]), 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_WAY]),
+					     nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_PID]), arg);
+
+	case THERMAL_GENL_EVENT_THRESHOLD_FLUSH:
+		return ops->threshold_flush(nla_get_u32(attrs[THERMAL_GENL_ATTR_TZ_ID]),
+					    nla_get_u32(attrs[THERMAL_GENL_ATTR_THRESHOLD_PID]), 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_LAST_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_LAST_TEMP]), arg);
+
 	default:
 		return -1;
 	}
@@ -101,19 +128,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..0571ec0bf438 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, pid_t pid, void *arg);
+	int (*threshold_delete)(int tz_id, int temperature, int direction, pid_t pid, void *arg);
+	int (*threshold_flush)(int tz_id, pid_t pid, void *arg);
+	int (*threshold_up)(int tz_id, int temp, int last_temp, void *arg);
+	int (*threshold_down)(int tz_id, int temp, int last_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..4851744d482e 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

[PATCH v2 7/7] tools/thermal/thermal-engine: Take into account the thresholds API

[Daniel Lezcano]

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 | 109 +++++++++++++++---
 1 file changed, 96 insertions(+), 13 deletions(-)

diff --git a/tools/thermal/thermal-engine/thermal-engine.c b/tools/thermal/thermal-engine/thermal-engine.c
index 9b1476a2680f..ddc30a27acda 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,66 @@ 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,
+			 pid_t pid, __maybe_unused void *arg)
+{
+	INFO("Threshold added by pid=%d, tz_id=%d: temp=%d, direction=%d\n",
+	     pid, tz_id, temp, direction);
+
+	return 0;
+}
+
+static int threshold_delete(int tz_id, int temp, int direction,
+			    pid_t pid, __maybe_unused void *arg)
+{
+	INFO("Threshold deleted by pid=%d, tz_id=%d: temp=%d, direction=%d\n",
+	     pid, tz_id, temp, direction);
+
+	return 0;
+}
+
+static int threshold_flush(int tz_id, pid_t pid, __maybe_unused void *arg)
+{
+	INFO("Thresholds flushed by pid=%d, tz_id=%d\n", pid, tz_id);
+
+	return 0;
+}
+
+static int threshold_up(int tz_id, int temp, int last_temp, __maybe_unused void *arg)
+{
+	INFO("Threshold crossed way up tz_id=%d: temp=%d, last_temp=%d\n",
+	     tz_id, temp, last_temp);
+
+	return 0;
+}
+
+static int threshold_down(int tz_id, int temp, int last_temp, __maybe_unused void *arg)
+{
+	INFO("Threshold crossed way down tz_id=%d: temp=%d, last_temp=%d\n",
+	     tz_id, temp, last_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 +360,7 @@ enum {
 	THERMAL_ENGINE_DAEMON_ERROR,
 	THERMAL_ENGINE_LOG_ERROR,
 	THERMAL_ENGINE_THERMAL_ERROR,
+	THERMAL_ENGINE_THRESHOLD_ERROR,	
 	THERMAL_ENGINE_MAINLOOP_ERROR,
 };
 
@@ -318,6 +399,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

Re: [PATCH v2 0/7] Add thermal thresholds support

[Rafael J. Wysocki]

On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
<daniel.lezcano@linaro.org> 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 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
> 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.

First off, I'd prefer these things to be referred to as "user
thresholds" to avoid confusion with trip thresholds.

> A threshold can be added, deleted or flushed. The latter means all
> thresholds belonging to a thermal zone will be deleted.
>
> When a threshold is added:
>
>  - if the same threshold (temperature and direction) exists, an error
>    is returned

Why is it useful to return an error in this case?  It appears that the
existing threshold could be used just fine.

>  - if a threshold is specified with the same temperature but a
>    different direction, the specified direction is added
>
>  - if there is no threshold with the same temperature then it is
>    created
>
> When a threshold is deleted:
>
>  - if the same threshold (temperature and direction) exists, it is
>    deleted
>
>  - if a threshold is specified with the same temperature but a
>    different direction, the specified direction is removed
>
>  - if there is no threshold with the same temperature, then an error
>    is returned
>
> When the threshold are flushed:
>
>  - All thresholds related to a thermal zone are deleted
>
> When a threshold is crossed:
>
>  - the userspace does not need to know which threshold(s) have been
>    crossed, it will be notified with the thermal zone identifier, the
>    current temperature and the previous temperature

This has a bit of a warm-up issue when the zone temperature is above a
threshold to start with.

Or is it not a problem because thresholds can only be added when the
zone temperature is known?

>  - if multiple thresholds have been crossed between two updates only
>    one notification will be send to the userspace, it is pointless to
>    send a notification per thresholds crossed as the userspace can
>    handle that easily when it has the temperature delta information

That's clever.

> All aforementioned actions and events lead to a notification to the
> userspace. A threshold change (add, delete and flush) is notified to
> the userspace with the process id responsible of the action.
>
> Along with the kernel changes, the thermal library has been extended
> to provide the different API to deal with the new threshold netlink
> events and commands.
>
> In addition, the thermal-engine skeleton uses these new API by
> flushing and adding 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.

Re: [PATCH v2 0/7] Add thermal thresholds support

[Pandruvada, Srinivas]

Hi Daniel,

On Fri, 2024-08-16 at 10:12 +0200, 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 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
> 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.
> 
> A threshold can be added, deleted or flushed. The latter means all
> thresholds belonging to a thermal zone will be deleted.
>     

So you are proposing to add threshold via netlink, not adding any new
sysfs attribute? That is not clear here.

I think you are adding"
THERMAL_GENL_CMD_THRESHOLD_GET
THERMAL_GENL_CMD_THRESHOLD_ADD
THERMAL_GENL_CMD_THRESHOLD_DELETE
THERMAL_GENL_CMD_THRESHOLD_FLUSH

We need to document our netlink messages including old ones.

Also we should add "MODIFY" as we tend to change them quite often.

Also no hysteresis, that is practically we can't use. Temperature
changes so much that that will flood user space. You will get 100s of
events on CPU temperature if you set temperature threshold in CPU.
We have a whole filtering in driver to avoid this.
You need a rate limit here.

There are multiple user processes can add threshold and there is no
ownership. So one process can cause too much noise to others as it is
multicast.

We worked on a change to filer these as discussed during last LPC, but
not posted yet. This will really need this as this will be too many
messages.

Thanks,
Srinivas



> When a threshold is added:
> 
>  - if the same threshold (temperature and direction) exists, an error
>    is returned
> 
>  - if a threshold is specified with the same temperature but a
>    different direction, the specified direction is added
> 
>  - if there is no threshold with the same temperature then it is
>    created
> 
> When a threshold is deleted:
> 
>  - if the same threshold (temperature and direction) exists, it is
>    deleted
> 
>  - if a threshold is specified with the same temperature but a
>    different direction, the specified direction is removed
> 
>  - if there is no threshold with the same temperature, then an error
>    is returned
> 
> When the threshold are flushed:
> 
>  - All thresholds related to a thermal zone are deleted
> 
> When a threshold is crossed:
> 
>  - the userspace does not need to know which threshold(s) have been
>    crossed, it will be notified with the thermal zone identifier, the
>    current temperature and the previous temperature
> 
>  - if multiple thresholds have been crossed between two updates only
>    one notification will be send to the userspace, it is pointless to
>    send a notification per thresholds crossed as the userspace can
>    handle that easily when it has the temperature delta information
> 
> All aforementioned actions and events lead to a notification to the
> userspace. A threshold change (add, delete and flush) is notified to
> the userspace with the process id responsible of the action.
> 
> Along with the kernel changes, the thermal library has been extended
> to provide the different API to deal with the new threshold netlink
> events and commands.
> 
> In addition, the thermal-engine skeleton uses these new API by
> flushing and adding 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:
>   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 (7):
>   thermal/core: Compute low and high boundaries in
>     thermal_zone_device_update()
>   thermal/core: Add thresholds support
>   thermal/core: Connect the threshold with the core
>   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/Kconfig                       |  15 ++
>  drivers/thermal/Makefile                      |   3 +
>  drivers/thermal/thermal_core.c                |  21 +-
>  drivers/thermal/thermal_core.h                |   6 +-
>  drivers/thermal/thermal_netlink.c             | 239
> ++++++++++++++++-
>  drivers/thermal/thermal_netlink.h             |   7 +
>  drivers/thermal/thermal_thresholds.c          | 247
> ++++++++++++++++++
>  drivers/thermal/thermal_thresholds.h          |  54 ++++
>  drivers/thermal/thermal_trip.c                |  27 +-
>  include/linux/thermal.h                       |   3 +
>  include/uapi/linux/thermal.h                  |  30 ++-
>  tools/lib/thermal/commands.c                  | 167 +++++++++++-
>  tools/lib/thermal/events.c                    |  58 +++-
>  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 | 109 +++++++-
>  18 files changed, 972 insertions(+), 78 deletions(-)
>  create mode 100644 drivers/thermal/thermal_thresholds.c
>  create mode 100644 drivers/thermal/thermal_thresholds.h
> 

Re: [PATCH v2 0/7] Add thermal thresholds support

[Rafael J. Wysocki]

Hi Srinivas,

On Wed, Aug 21, 2024 at 10:04 PM Pandruvada, Srinivas
<srinivas.pandruvada@intel.com> wrote:
>
> Hi Daniel,
>
> On Fri, 2024-08-16 at 10:12 +0200, 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 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
> > 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.
> >
> > A threshold can be added, deleted or flushed. The latter means all
> > thresholds belonging to a thermal zone will be deleted.
> >
>
> So you are proposing to add threshold via netlink, not adding any new
> sysfs attribute? That is not clear here.
>
> I think you are adding"
> THERMAL_GENL_CMD_THRESHOLD_GET
> THERMAL_GENL_CMD_THRESHOLD_ADD
> THERMAL_GENL_CMD_THRESHOLD_DELETE
> THERMAL_GENL_CMD_THRESHOLD_FLUSH
>
> We need to document our netlink messages including old ones.
>
> Also we should add "MODIFY" as we tend to change them quite often.
>
> Also no hysteresis, that is practically we can't use.

The direction thing is equivalent to hysteresis though.

Instead of using one threshold with a given hysteresis value, use two
of them with different temperature values and different directions.

> Temperature changes so much that that will flood user space. You will get 100s of
> events on CPU temperature if you set temperature threshold in CPU.

Events only trigger when thresholds are crossed in a specific
direction, but overall you have a point.

> We have a whole filtering in driver to avoid this.
> You need a rate limit here.
>
> There are multiple user processes can add threshold and there is no
> ownership. So one process can cause too much noise to others as it is
> multicast.
>
> We worked on a change to filer these as discussed during last LPC, but
> not posted yet. This will really need this as this will be too many
> messages.

Unless there is a way to limit your subscription to events regarding a
specific thermal zone, for instance.

Anyway, I have to admit ignorance regarding the user space usage model
related to this.  For example, is it expected that there will be one
user space entity managing the thresholds or there can be many.

Re: [PATCH v2 0/7] Add thermal thresholds support

[Pandruvada, Srinivas]

On Wed, 2024-08-21 at 22:20 +0200, Rafael J. Wysocki wrote:
> Hi Srinivas,
> 
> On Wed, Aug 21, 2024 at 10:04 PM Pandruvada, Srinivas
> <srinivas.pandruvada@intel.com> wrote:
> > 
> > Hi Daniel,
> > 
> > On Fri, 2024-08-16 at 10:12 +0200, 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 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
> > > 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.
> > > 
> > > A threshold can be added, deleted or flushed. The latter means
> > > all
> > > thresholds belonging to a thermal zone will be deleted.
> > > 
> > 
> > So you are proposing to add threshold via netlink, not adding any
> > new
> > sysfs attribute? That is not clear here.
> > 
> > I think you are adding"
> > THERMAL_GENL_CMD_THRESHOLD_GET
> > THERMAL_GENL_CMD_THRESHOLD_ADD
> > THERMAL_GENL_CMD_THRESHOLD_DELETE
> > THERMAL_GENL_CMD_THRESHOLD_FLUSH
> > 
> > We need to document our netlink messages including old ones.
> > 
> > Also we should add "MODIFY" as we tend to change them quite often.
> > 
> > Also no hysteresis, that is practically we can't use.
> 
> The direction thing is equivalent to hysteresis though.
> 
> Instead of using one threshold with a given hysteresis value, use two
> of them with different temperature values and different directions.
> 
> > Temperature changes so much that that will flood user space. You
> > will get 100s of
> > events on CPU temperature if you set temperature threshold in CPU.
> 
> Events only trigger when thresholds are crossed in a specific
> direction, but overall you have a point.

This is good enough for some sensor which changes slowly like skin. But
some sensors like CPU and board are not like that. We publish both raw
and filtered event count in debugfs for x86 package temp.
For example, I just ran a sample on a client for one threshold.

Total notification from hardware: 224
Notified to user space: 16



> 
> > We have a whole filtering in driver to avoid this.
> > You need a rate limit here.
> > 
> > There are multiple user processes can add threshold and there is no
> > ownership. So one process can cause too much noise to others as it
> > is
> > multicast.
> > 
> > We worked on a change to filer these as discussed during last LPC,
> > but
> > not posted yet. This will really need this as this will be too many
> > messages.
> 
> Unless there is a way to limit your subscription to events regarding
> a
> specific thermal zone, for instance.
That's what this change does.

> 
> Anyway, I have to admit ignorance regarding the user space usage
> model
> related to this.  For example, is it expected that there will be one
> user space entity managing the thresholds or there can be many.

Good question. That's why we didn't send this change. If there is one
agent this is fine.

But in embedded space, there may be more than one. Hence they needed
netlink multicast instead of just one char device.

Thanks,
Srinivas

Re: [PATCH v2 0/7] Add thermal thresholds support

[Rafael J. Wysocki]

On Thu, Aug 22, 2024 at 12:16 AM Pandruvada, Srinivas
<srinivas.pandruvada@intel.com> wrote:
>
> On Wed, 2024-08-21 at 22:20 +0200, Rafael J. Wysocki wrote:
> > Hi Srinivas,
> >
> > On Wed, Aug 21, 2024 at 10:04 PM Pandruvada, Srinivas
> > <srinivas.pandruvada@intel.com> wrote:
> > >
> > > Hi Daniel,
> > >
> > > On Fri, 2024-08-16 at 10:12 +0200, 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 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
> > > > 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.
> > > >
> > > > A threshold can be added, deleted or flushed. The latter means
> > > > all
> > > > thresholds belonging to a thermal zone will be deleted.
> > > >
> > >
> > > So you are proposing to add threshold via netlink, not adding any
> > > new
> > > sysfs attribute? That is not clear here.
> > >
> > > I think you are adding"
> > > THERMAL_GENL_CMD_THRESHOLD_GET
> > > THERMAL_GENL_CMD_THRESHOLD_ADD
> > > THERMAL_GENL_CMD_THRESHOLD_DELETE
> > > THERMAL_GENL_CMD_THRESHOLD_FLUSH
> > >
> > > We need to document our netlink messages including old ones.
> > >
> > > Also we should add "MODIFY" as we tend to change them quite often.
> > >
> > > Also no hysteresis, that is practically we can't use.
> >
> > The direction thing is equivalent to hysteresis though.
> >
> > Instead of using one threshold with a given hysteresis value, use two
> > of them with different temperature values and different directions.
> >
> > > Temperature changes so much that that will flood user space. You
> > > will get 100s of
> > > events on CPU temperature if you set temperature threshold in CPU.
> >
> > Events only trigger when thresholds are crossed in a specific
> > direction, but overall you have a point.
>
> This is good enough for some sensor which changes slowly like skin.

Right, and I think that this is the driving use case.

> But some sensors like CPU and board are not like that.

Sure, so this interface will not be for them.

> We publish both raw and filtered event count in debugfs for x86 package temp.
> For example, I just ran a sample on a client for one threshold.
>
> Total notification from hardware: 224
> Notified to user space: 16

So did you apply the Daniel's patches and run the test or did you do
something else?

> >
> > > We have a whole filtering in driver to avoid this.
> > > You need a rate limit here.
> > >
> > > There are multiple user processes can add threshold and there is no
> > > ownership. So one process can cause too much noise to others as it
> > > is
> > > multicast.
> > >
> > > We worked on a change to filer these as discussed during last LPC,
> > > but
> > > not posted yet. This will really need this as this will be too many
> > > messages.
> >
> > Unless there is a way to limit your subscription to events regarding
> > a
> > specific thermal zone, for instance.
> That's what this change does.
>
> >
> > Anyway, I have to admit ignorance regarding the user space usage
> > model
> > related to this.  For example, is it expected that there will be one
> > user space entity managing the thresholds or there can be many.
>
> Good question. That's why we didn't send this change. If there is one
> agent this is fine.

I think that the addition and deletion of a user threshold should be
privileged operations.

If that is the case, it all boils down to proper coordination in user space.

> But in embedded space, there may be more than one. Hence they needed
> netlink multicast instead of just one char device.

I see.

Re: [PATCH v2 0/7] Add thermal thresholds support

[Pandruvada, Srinivas]

On Thu, 2024-08-22 at 11:41 +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 22, 2024 at 12:16 AM Pandruvada, Srinivas
> <srinivas.pandruvada@intel.com> wrote:
> > 
> > On Wed, 2024-08-21 at 22:20 +0200, Rafael J. Wysocki wrote:
> > > Hi Srinivas,
> > > 
> > > On Wed, Aug 21, 2024 at 10:04 PM Pandruvada, Srinivas
> > > <srinivas.pandruvada@intel.com> wrote:
> > > > 
> > > > Hi Daniel,
> > > > 
> > > > On Fri, 2024-08-16 at 10:12 +0200, 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 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
> > > > > 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.
> > > > > 
> > > > > A threshold can be added, deleted or flushed. The latter
> > > > > means
> > > > > all
> > > > > thresholds belonging to a thermal zone will be deleted.
> > > > > 
> > > > 
> > > > So you are proposing to add threshold via netlink, not adding
> > > > any
> > > > new
> > > > sysfs attribute? That is not clear here.
> > > > 
> > > > I think you are adding"
> > > > THERMAL_GENL_CMD_THRESHOLD_GET
> > > > THERMAL_GENL_CMD_THRESHOLD_ADD
> > > > THERMAL_GENL_CMD_THRESHOLD_DELETE
> > > > THERMAL_GENL_CMD_THRESHOLD_FLUSH
> > > > 
> > > > We need to document our netlink messages including old ones.
> > > > 
> > > > Also we should add "MODIFY" as we tend to change them quite
> > > > often.
> > > > 
> > > > Also no hysteresis, that is practically we can't use.
> > > 
> > > The direction thing is equivalent to hysteresis though.
> > > 
> > > Instead of using one threshold with a given hysteresis value, use
> > > two
> > > of them with different temperature values and different
> > > directions.
> > > 
> > > > Temperature changes so much that that will flood user space.
> > > > You
> > > > will get 100s of
> > > > events on CPU temperature if you set temperature threshold in
> > > > CPU.
> > > 
> > > Events only trigger when thresholds are crossed in a specific
> > > direction, but overall you have a point.
> > 
> > This is good enough for some sensor which changes slowly like skin.
> 
> Right, and I think that this is the driving use case.
> 
> > But some sensors like CPU and board are not like that.
> 
> Sure, so this interface will not be for them.
But when we design an interface, I think this shouldn't be sensor
dependent.

> 
> > We publish both raw and filtered event count in debugfs for x86
> > package temp.
> > For example, I just ran a sample on a client for one threshold.
> > 
> > Total notification from hardware: 224
> > Notified to user space: 16
> 
> So did you apply the Daniel's patches and run the test or did you do
> something else?
We already use netlink to send notification to user space via writable
trip. So didn't apply any patches. I don't see these patches will do
any different.


> 
> > > 
> > > > We have a whole filtering in driver to avoid this.
> > > > You need a rate limit here.
> > > > 
> > > > There are multiple user processes can add threshold and there
> > > > is no
> > > > ownership. So one process can cause too much noise to others as
> > > > it
> > > > is
> > > > multicast.
> > > > 
> > > > We worked on a change to filer these as discussed during last
> > > > LPC,
> > > > but
> > > > not posted yet. This will really need this as this will be too
> > > > many
> > > > messages.
> > > 
> > > Unless there is a way to limit your subscription to events
> > > regarding
> > > a
> > > specific thermal zone, for instance.
> > That's what this change does.
> > 
> > > 
> > > Anyway, I have to admit ignorance regarding the user space usage
> > > model
> > > related to this.  For example, is it expected that there will be
> > > one
> > > user space entity managing the thresholds or there can be many.
> > 
> > Good question. That's why we didn't send this change. If there is
> > one
> > agent this is fine.
> 
> I think that the addition and deletion of a user threshold should be
> privileged operations.
> 
> If that is the case, it all boils down to proper coordination in user
> space.
> 
Some kernel API use some cookie as parameter. Here also something like
that can be used to match.

> > But in embedded space, there may be more than one. Hence they
> > needed
> > netlink multicast instead of just one char device.
> 
> I see.

Thanks,
Srinivas

Re: [PATCH v2 0/7] Add thermal thresholds support

[Pandruvada, Srinivas]

On Thu, 2024-08-22 at 12:11 +0000, Pandruvada, Srinivas wrote:
> On Thu, 2024-08-22 at 11:41 +0200, Rafael J. Wysocki wrote:
> > On Thu, Aug 22, 2024 at 12:16 AM Pandruvada, Srinivas
> > <srinivas.pandruvada@intel.com> wrote:
> > > 
> > > On Wed, 2024-08-21 at 22:20 +0200, Rafael J. Wysocki wrote:
> > > > Hi Srinivas,
> > > > 
> > > > On Wed, Aug 21, 2024 at 10:04 PM Pandruvada, Srinivas
> > > > <srinivas.pandruvada@intel.com> wrote:
> > > > > 
> > > > > Hi Daniel,
> > > > > 
> > > > > On Fri, 2024-08-16 at 10:12 +0200, Daniel Lezcano wrote:
> > > > > > The trip points are a firmware description of the
> > > > > > temperature
> > > > > > limits
> > > > > > of a specific thermal zone where we associate an action
> > > > > > whichcan't
> > > > > > 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 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
> > > > > > 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.
> > > > > > 
> > > > > > A threshold can be added, deleted or flushed. The latter
> > > > > > means
> > > > > > all
> > > > > > thresholds belonging to a thermal zone will be deleted.
> > > > > > 
> > > > > 
> > > > > So you are proposing to add threshold via netlink, not adding
> > > > > any
> > > > > new
> > > > > sysfs attribute? That is not clear here.
> > > > > 
> > > > > I think you are adding"
> > > > > THERMAL_GENL_CMD_THRESHOLD_GET
> > > > > THERMAL_GENL_CMD_THRESHOLD_ADD
> > > > > THERMAL_GENL_CMD_THRESHOLD_DELETE
> > > > > THERMAL_GENL_CMD_THRESHOLD_FLUSH
> > > > > 
> > > > > We need to document our netlink messages including old ones.
> > > > > 
> > > > > Also we should add "MODIFY" as we tend to change them quite
> > > > > often.
> > > > > 
> > > > > Also no hysteresis, that is practically we can't use.
> > > > 
> > > > The direction thing is equivalent to hysteresis though.
> > > > 
> > > > Instead of using one threshold with a given hysteresis value,
> > > > use
> > > > two
> > > > of them with different temperature values and different
> > > > directions.
> > > > 
> > > > > Temperature changes so much that that will flood user space.
> > > > > You
> > > > > will get 100s of
> > > > > events on CPU temperature if you set temperature threshold in
> > > > > CPU.
> > > > 
> > > > Events only trigger when thresholds are crossed in a specific
> > > > direction, but overall you have a point.
> > > 
> > > This is good enough for some sensor which changes slowly like
> > > skin.
> > 
> > Right, and I think that this is the driving use case.
> > 
> > > But some sensors like CPU and board are not like that.
> > 
> > Sure, so this interface will not be for them.
> But when we design an interface, I think this shouldn't be sensor
> dependent.
> 
> > 
> > > We publish both raw and filtered event count in debugfs for x86
> > > package temp.
> > > For example, I just ran a sample on a client for one threshold.
> > > 
> > > Total notification from hardware: 224
> > > Notified to user space: 16
> > 
> > So did you apply the Daniel's patches and run the test or did you
> > do
> > something else?
> We already use netlink to send notification to user space via
> writable
> trip. So didn't apply any patches. I don't see these patches will do
> any different.

I mean without rate limit here all 224 notifications will be passed to
user space (not 16). Temperature goes up and down over threshold for
100s of times before settling.

I can give  a try with this series to confirm.

Thanks,
Srinivas

> 
> 
> > 
> > > > 
> > > > > We have a whole filtering in driver to avoid this.
> > > > > You need a rate limit here.
> > > > > 
> > > > > There are multiple user processes can add threshold and there
> > > > > is no
> > > > > ownership. So one process can cause too much noise to others
> > > > > as
> > > > > it
> > > > > is
> > > > > multicast.
> > > > > 
> > > > > We worked on a change to filer these as discussed during last
> > > > > LPC,
> > > > > but
> > > > > not posted yet. This will really need this as this will be
> > > > > too
> > > > > many
> > > > > messages.
> > > > 
> > > > Unless there is a way to limit your subscription to events
> > > > regarding
> > > > a
> > > > specific thermal zone, for instance.
> > > That's what this change does.
> > > 
> > > > 
> > > > Anyway, I have to admit ignorance regarding the user space
> > > > usage
> > > > model
> > > > related to this.  For example, is it expected that there will
> > > > be
> > > > one
> > > > user space entity managing the thresholds or there can be many.
> > > 
> > > Good question. That's why we didn't send this change. If there is
> > > one
> > > agent this is fine.
> > 
> > I think that the addition and deletion of a user threshold should
> > be
> > privileged operations.
> > 
> > If that is the case, it all boils down to proper coordination in
> > user
> > space.
> > 
> Some kernel API use some cookie as parameter. Here also something
> like
> that can be used to match.
> 
> > > But in embedded space, there may be more than one. Hence they
> > > needed
> > > netlink multicast instead of just one char device.
> > 
> > I see.
> 
> Thanks,
> Srinivas
> 

Re: [PATCH v2 0/7] Add thermal thresholds support

[Rafael J. Wysocki]

On Thu, Aug 22, 2024 at 2:52 PM Pandruvada, Srinivas
<srinivas.pandruvada@intel.com> wrote:
>
> On Thu, 2024-08-22 at 12:11 +0000, Pandruvada, Srinivas wrote:
> > On Thu, 2024-08-22 at 11:41 +0200, Rafael J. Wysocki wrote:
> > > On Thu, Aug 22, 2024 at 12:16 AM Pandruvada, Srinivas
> > > <srinivas.pandruvada@intel.com> wrote:
> > > >
> > > > On Wed, 2024-08-21 at 22:20 +0200, Rafael J. Wysocki wrote:
> > > > > Hi Srinivas,
> > > > >
> > > > > On Wed, Aug 21, 2024 at 10:04 PM Pandruvada, Srinivas
> > > > > <srinivas.pandruvada@intel.com> wrote:
> > > > > >
> > > > > > Hi Daniel,
> > > > > >
> > > > > > On Fri, 2024-08-16 at 10:12 +0200, Daniel Lezcano wrote:
> > > > > > > The trip points are a firmware description of the
> > > > > > > temperature
> > > > > > > limits
> > > > > > > of a specific thermal zone where we associate an action
> > > > > > > whichcan't
> > > > > > > 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 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
> > > > > > > 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.
> > > > > > >
> > > > > > > A threshold can be added, deleted or flushed. The latter
> > > > > > > means
> > > > > > > all
> > > > > > > thresholds belonging to a thermal zone will be deleted.
> > > > > > >
> > > > > >
> > > > > > So you are proposing to add threshold via netlink, not adding
> > > > > > any
> > > > > > new
> > > > > > sysfs attribute? That is not clear here.
> > > > > >
> > > > > > I think you are adding"
> > > > > > THERMAL_GENL_CMD_THRESHOLD_GET
> > > > > > THERMAL_GENL_CMD_THRESHOLD_ADD
> > > > > > THERMAL_GENL_CMD_THRESHOLD_DELETE
> > > > > > THERMAL_GENL_CMD_THRESHOLD_FLUSH
> > > > > >
> > > > > > We need to document our netlink messages including old ones.
> > > > > >
> > > > > > Also we should add "MODIFY" as we tend to change them quite
> > > > > > often.
> > > > > >
> > > > > > Also no hysteresis, that is practically we can't use.
> > > > >
> > > > > The direction thing is equivalent to hysteresis though.
> > > > >
> > > > > Instead of using one threshold with a given hysteresis value,
> > > > > use
> > > > > two
> > > > > of them with different temperature values and different
> > > > > directions.
> > > > >
> > > > > > Temperature changes so much that that will flood user space.
> > > > > > You
> > > > > > will get 100s of
> > > > > > events on CPU temperature if you set temperature threshold in
> > > > > > CPU.
> > > > >
> > > > > Events only trigger when thresholds are crossed in a specific
> > > > > direction, but overall you have a point.
> > > >
> > > > This is good enough for some sensor which changes slowly like
> > > > skin.
> > >
> > > Right, and I think that this is the driving use case.
> > >
> > > > But some sensors like CPU and board are not like that.
> > >
> > > Sure, so this interface will not be for them.
> > But when we design an interface, I think this shouldn't be sensor
> > dependent.
> >
> > >
> > > > We publish both raw and filtered event count in debugfs for x86
> > > > package temp.
> > > > For example, I just ran a sample on a client for one threshold.
> > > >
> > > > Total notification from hardware: 224
> > > > Notified to user space: 16
> > >
> > > So did you apply the Daniel's patches and run the test or did you
> > > do
> > > something else?
> > We already use netlink to send notification to user space via
> > writable
> > trip. So didn't apply any patches. I don't see these patches will do
> > any different.
>
> I mean without rate limit here all 224 notifications will be passed to
> user space (not 16). Temperature goes up and down over threshold for
> 100s of times before settling.

Yeah, that needs a real trip point with hysteresis.

Or the threshold could be removed once crossed and then added back
when the temperature falls down sufficiently.

> I can give  a try with this series to confirm.

That would be premature at this point.

Re: [PATCH v2 0/7] Add thermal thresholds support

[Daniel Lezcano]

On 22/08/2024 14:11, Pandruvada, Srinivas wrote:
> On Thu, 2024-08-22 at 11:41 +0200, Rafael J. Wysocki wrote:
>> On Thu, Aug 22, 2024 at 12:16 AM Pandruvada, Srinivas
>> <srinivas.pandruvada@intel.com> wrote:

[ ... ]

>> So did you apply the Daniel's patches and run the test or did you do
>> something else?
> We already use netlink to send notification to user space via writable
> trip. So didn't apply any patches. I don't see these patches will do
> any different.

Actually you are missing the point of the thresholds approach.

The goal is to track the temperature easily from userspace without 
constantly polling the temperatures in all the places.

The trip points are a firmware descriptions. Their number is fixed. They 
are designed for in-kernel thermal framework. They have a type. A 
governor is supposed to be tied with it. A cooling device also.

Writable trip points means you should be able to add trip points 
dedicated to the userspace to the firmware which is not possible. Then 
reuse them from userspace which is unrelated to the in-kernel thermal 
management. It is difficult to deal with because of the need of tracking 
the low and high limits from userspace.

The thresholds are there to allow the userspace to have the benefit of 
the interrupt driven temperature monitoring.

Obviously if you set the thresholds to a temperature equal to a 
mitigation trip point then when the limit is reached you will receive 
notifications for the trip point going back and forth as well as 
notifications for the thresholds.





-- 
<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

Re: [PATCH v2 0/7] Add thermal thresholds support

[Pandruvada, Srinivas]

On Thu, 2024-08-22 at 19:08 +0200, Daniel Lezcano wrote:
> On 22/08/2024 14:11, Pandruvada, Srinivas wrote:
> > On Thu, 2024-08-22 at 11:41 +0200, Rafael J. Wysocki wrote:
> > > On Thu, Aug 22, 2024 at 12:16 AM Pandruvada, Srinivas
> > > <srinivas.pandruvada@intel.com> wrote:
> 
> [ ... ]
> 
> > > So did you apply the Daniel's patches and run the test or did you
> > > do
> > > something else?
> > We already use netlink to send notification to user space via
> > writable
> > trip. So didn't apply any patches. I don't see these patches will
> > do
> > any different.
> 
> Actually you are missing the point of the thresholds approach.

I very well understand. I proposed similar approach without netlink
several years back. Also submitted patches to use IIO.


> 
> The goal is to track the temperature easily from userspace without 
> constantly polling the temperatures in all the places.
> 
Exactly.


> The trip points are a firmware descriptions. Their number is fixed.
> They 
> are designed for in-kernel thermal framework. They have a type. A 
> governor is supposed to be tied with it. A cooling device also.
> 
Yes, I understand the whole approach. trips are trip where you want
governors to take action.

> Writable trip points means you should be able to add trip points 
> dedicated to the userspace to the firmware which is not possible.
> Then 
> reuse them from userspace which is unrelated to the in-kernel thermal
> management. It is difficult to deal with because of the need of
> tracking 
> the low and high limits from userspace.
> 
Yes.

> The thresholds are there to allow the userspace to have the benefit
> of 
> the interrupt driven temperature monitoring.
> 
Yes. I am not denying benefits.

> Obviously if you set the thresholds to a temperature equal to a 
> mitigation trip point then when the limit is reached you will receive
> notifications for the trip point going back and forth as well as 
> notifications for the thresholds.
> 
Not correct. It is not at mitigation point. This is how several sensors
several sensors work. I submitted a range of sensor drivers to IIO
framework, several have this issue.

To make it useful, you need to have some rate limiting. Netlink is not
a low overhead user-kernel interface.

Looks like you finalized design and just looking for patch reviews!


> 
> 
> 
> 

Re: [PATCH v2 0/7] Add thermal thresholds support

[Daniel Lezcano]

Hi Srinivas,

On 21/08/2024 22:04, Pandruvada, Srinivas wrote:
> Hi Daniel,
> 
> On Fri, 2024-08-16 at 10:12 +0200, 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 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
>> 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.
>>
>> A threshold can be added, deleted or flushed. The latter means all
>> thresholds belonging to a thermal zone will be deleted.
>>      
> 
> So you are proposing to add threshold via netlink, not adding any new
> sysfs attribute? That is not clear here.
> 
> I think you are adding"
> THERMAL_GENL_CMD_THRESHOLD_GET
> THERMAL_GENL_CMD_THRESHOLD_ADD
> THERMAL_GENL_CMD_THRESHOLD_DELETE
> THERMAL_GENL_CMD_THRESHOLD_FLUSH
> 
> We need to document our netlink messages including old ones.

We can do that but I would prefer to do that in a separate series.

> Also we should add "MODIFY" as we tend to change them quite often.

We can not change a threshold, only delete or add it.

> Also no hysteresis, that is practically we can't use. Temperature
> changes so much that that will flood user space. 

A threshold with a direction allows to create an hysteresis. Instead of 
one threshold, you will have two with different directions and temperature.

> You will get 100s of
> events on CPU temperature if you set temperature threshold in CPU.
> We have a whole filtering in driver to avoid this.
> You need a rate limit here.

I disagree, the user space is not supposed to monitor at a high rate a 
particular thermal zone. If the CPU is sending 100s notifications per 
second that means the threshold is set close to the mitigation trip 
point temperature which is a bad user space configuration.

> There are multiple user processes can add threshold and there is no
> ownership. So one process can cause too much noise to others as it is
> multicast.

ownership is something we may want to handle later.

What about restricting the thermal netlink to the 'root' user only ATM?

> We worked on a change to filer these as discussed during last LPC, but
> not posted yet. This will really need this as this will be too many
> messages.

With a correct configuration, that would not happen because it is 
different from receiving events from the trip points.


-- 
<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