[LTP] [PATCH v4] thermal: add new test group

[LTP] [PATCH v4] thermal: add new test group

[Piotr Kubaj]

This is a new test for checking thermal interrupt events.
stress-ng is used because genload doesn't seem to generate enough load.

In particular, this version replaces use of tr and cut where awk is
already used.

Signed-off-by: Piotr Kubaj <piotr.kubaj@intel.com>
---
 runtest/thermal                       |   3 +
 scenario_groups/default               |   1 +
 testcases/kernel/Makefile             |   1 +
 testcases/kernel/thermal/Makefile     |  11 +++
 testcases/kernel/thermal/thermal01.sh | 100 ++++++++++++++++++++++++++
 5 files changed, 116 insertions(+)
 create mode 100644 runtest/thermal
 create mode 100644 testcases/kernel/thermal/Makefile
 create mode 100755 testcases/kernel/thermal/thermal01.sh

diff --git a/runtest/thermal b/runtest/thermal
new file mode 100644
index 000000000..823d422da
--- /dev/null
+++ b/runtest/thermal
@@ -0,0 +1,3 @@
+# Thermal driver API
+# https://docs.kernel.org/driver-api/thermal/
+thermal_interrupt_events thermal01.sh
diff --git a/scenario_groups/default b/scenario_groups/default
index 0e76b2bee..ffdd7ff25 100644
--- a/scenario_groups/default
+++ b/scenario_groups/default
@@ -26,3 +26,4 @@ crypto
 kernel_misc
 uevent
 watchqueue
+thermal
diff --git a/testcases/kernel/Makefile b/testcases/kernel/Makefile
index 98fd45a9d..ac816e4e8 100644
--- a/testcases/kernel/Makefile
+++ b/testcases/kernel/Makefile
@@ -36,6 +36,7 @@ SUBDIRS			+= connectors \
 			   sched \
 			   security \
 			   sound \
+			   thermal \
 			   tracing \
 			   uevents \
 			   watchqueue \
diff --git a/testcases/kernel/thermal/Makefile b/testcases/kernel/thermal/Makefile
new file mode 100644
index 000000000..edd338605
--- /dev/null
+++ b/testcases/kernel/thermal/Makefile
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2025, Intel Corporation. All rights reserved.
+# Author:Piotr Kubaj <piotr.kubaj@intel.com>
+
+top_srcdir             ?= ../../..
+
+include $(top_srcdir)/include/mk/env_pre.mk
+
+INSTALL_TARGETS                := *.sh
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/thermal/thermal01.sh b/testcases/kernel/thermal/thermal01.sh
new file mode 100755
index 000000000..95adaf04b
--- /dev/null
+++ b/testcases/kernel/thermal/thermal01.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (C) 2025 Intel - http://www.intel.com/
+#
+# ---
+# doc
+# Tests the CPU package thermal sensor interface for Intel platforms.
+#
+# It works by checking the initial count of thermal interrupts. Then it
+# decreases the threshold for sending a thermal interrupt to just above
+# the current temperature and runs a workload on the CPU. Finally, it restores
+# the original thermal threshold and checks whether the number of thermal
+# interrupts increased.
+# ---
+#
+# ---
+# env
+# {
+#  "needs_root": true,
+#  "supported_archs": ["x86", "x86_64"],
+#  "needs_cmds": ["stress-ng"],
+#  "min_runtime": 180
+# }
+# ---
+
+. tst_loader.sh
+
+tst_test()
+{
+	local thermal_zone_numbers=""
+	local temp
+	local temp_high=""
+	local status=0
+
+	local interrupt_array_init=$(awk -F'[^0-9]*' '/Thermal event interrupts/ {$1=$1;print}' /proc/interrupts)
+	if [ $? -eq 0 ]; then
+		tst_res TDEBUG "Initial values of thermal interrupt counters: $interrupt_array_init"
+		local num=$(tst_getconf _NPROCESSORS_ONLN)
+		tst_res TDEBUG "Number of logical cores: $num"
+	else
+		tst_brk TCONF "Thermal event interrupts is not found"
+	fi
+
+	# Below we check for the thermal_zone which uses x86_pkg_temp driver
+	local thermal_zone_numbers=$(grep -l x86_pkg_temp /sys/class/thermal/thermal_zone*/type | xargs dirname)
+	tst_res TINFO "x86_pkg_temp thermal zones: $thermal_zone_numbers"
+
+	if [ -z $thermal_zone_numbers ]; then
+		tst_brk TCONF "No x86_pkg_temp thermal zones found"
+	fi
+	for i in $thermal_zone_numbers; do
+		tst_res TINFO "Currently testing x86_pkg_temp $i"
+		local TEMP="$i/temp"
+		local temp=$(cat "$TEMP")
+		tst_res TDEBUG "$i's current temperature is $temp"
+		case $temp in
+		[0-9]*) ;;
+		*)
+			tst_brk TBROK "Unexpected zone temperature value $temp";;
+		esac
+		local trip=$(cat $i/trip_point_1_temp)
+		# Setting trip_point_1_temp for $i to $temp + 10 (0.001°C)
+		local temp_high=$(( temp + 10 ))
+		echo "$temp_high" > $i/trip_point_1_temp
+		local run_time=30
+		local sleep_time=10
+		while [ $sleep_time -gt 0 ]; do
+			ROD stress-ng --matrix 0 --timeout $run_time --quiet
+			local temp_cur=$(cat "$TEMP")
+			tst_res TDEBUG "temp_cur: $temp_cur"
+			[ $temp_cur -gt $temp_high ] && break
+			tst_sleep $sleep_time
+			run_time=$(( run_time - 3 ))
+			sleep_time=$(( sleep_time - 1 ))
+		done
+		[ $temp_cur -gt $temp_high ] || tst_res TFAIL "Zone temperature is not rising as expected"
+
+		# Restore the original trip_point_1_temp value
+		echo "$trip" > $i/trip_point_1_temp
+
+		# Check whether thermal interrupts count actually increased
+		local interrupt_array_later=$(awk -F'[^0-9]*' '/Thermal event interrupts/ {$1=$1;print}' /proc/interrupts)
+		tst_res TDEBUG "Current values of thermal interrupt counters: $interrupt_array_later"
+		for j in $(seq 1 "$num"); do
+			local interrupt_later=$(echo "$interrupt_array_later" | awk -v j=$j '{print $j}')
+			local interrupt_init=$(echo "$interrupt_array_init" | awk -v j=$j '{print $j}')
+			if [ $interrupt_later -le $interrupt_init ]; then
+				status=1
+			fi
+		done
+	done
+
+	if [ $status -eq 0 ]; then
+		tst_res TPASS "x86 package thermal interrupt triggered"
+	else
+		tst_res TFAIL "x86 package thermal interrupt did not trigger"
+	fi
+}
+
+. tst_run.sh
-- 
2.47.3

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4] thermal: add new test group

[Petr Vorel]

Hi Piotr,

> This is a new test for checking thermal interrupt events.
> stress-ng is used because genload doesn't seem to generate enough load.

> In particular, this version replaces use of tr and cut where awk is
> already used.

> --- /dev/null
> +++ b/runtest/thermal
> @@ -0,0 +1,3 @@
> +# Thermal driver API
> +# https://docs.kernel.org/driver-api/thermal/
> +thermal_interrupt_events thermal01.sh

I would not mind if the test itself was named thermal_interrupt_events.sh (more
descriptive), but that's a minor detail (which can be ignored or changed before
merge).

...
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/thermal/thermal01.sh b/testcases/kernel/thermal/thermal01.sh
> new file mode 100755
> index 000000000..95adaf04b
> --- /dev/null
> +++ b/testcases/kernel/thermal/thermal01.sh
> @@ -0,0 +1,100 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2025 Intel - http://www.intel.com/
> +#
> +# ---
> +# doc
> +# Tests the CPU package thermal sensor interface for Intel platforms.
> +#
> +# It works by checking the initial count of thermal interrupts. Then it
> +# decreases the threshold for sending a thermal interrupt to just above
> +# the current temperature and runs a workload on the CPU. Finally, it restores
> +# the original thermal threshold and checks whether the number of thermal
> +# interrupts increased.
> +# ---
> +#
> +# ---
> +# env
> +# {
> +#  "needs_root": true,
> +#  "supported_archs": ["x86", "x86_64"],
> +#  "needs_cmds": ["stress-ng"],
> +#  "min_runtime": 180
> +# }
> +# ---
> +
> +. tst_loader.sh
> +
> +tst_test()
> +{
> +	local thermal_zone_numbers=""
> +	local temp
> +	local temp_high=""

This could be just:
local thermal_zone_numbers temp temp_high

(empty shell variable is the same as ="").

> +	local status=0
> +
> +	local interrupt_array_init=$(awk -F'[^0-9]*' '/Thermal event interrupts/ {$1=$1;print}' /proc/interrupts)
> +	if [ $? -eq 0 ]; then
> +		tst_res TDEBUG "Initial values of thermal interrupt counters: $interrupt_array_init"
> +		local num=$(tst_getconf _NPROCESSORS_ONLN)
> +		tst_res TDEBUG "Number of logical cores: $num"
> +	else
> +		tst_brk TCONF "Thermal event interrupts is not found"
> +	fi
> +
> +	# Below we check for the thermal_zone which uses x86_pkg_temp driver
> +	local thermal_zone_numbers=$(grep -l x86_pkg_temp /sys/class/thermal/thermal_zone*/type | xargs dirname)
> +	tst_res TINFO "x86_pkg_temp thermal zones: $thermal_zone_numbers"
> +
> +	if [ -z $thermal_zone_numbers ]; then
> +		tst_brk TCONF "No x86_pkg_temp thermal zones found"
> +	fi
> +	for i in $thermal_zone_numbers; do
> +		tst_res TINFO "Currently testing x86_pkg_temp $i"
> +		local TEMP="$i/temp"
> +		local temp=$(cat "$TEMP")
> +		tst_res TDEBUG "$i's current temperature is $temp"
> +		case $temp in
> +		[0-9]*) ;;
> +		*)
> +			tst_brk TBROK "Unexpected zone temperature value $temp";;
> +		esac
> +		local trip=$(cat $i/trip_point_1_temp)
> +		# Setting trip_point_1_temp for $i to $temp + 10 (0.001°C)
> +		local temp_high=$(( temp + 10 ))
> +		echo "$temp_high" > $i/trip_point_1_temp
> +		local run_time=30
> +		local sleep_time=10
> +		while [ $sleep_time -gt 0 ]; do
> +			ROD stress-ng --matrix 0 --timeout $run_time --quiet
> +			local temp_cur=$(cat "$TEMP")
> +			tst_res TDEBUG "temp_cur: $temp_cur"
> +			[ $temp_cur -gt $temp_high ] && break
> +			tst_sleep $sleep_time
> +			run_time=$(( run_time - 3 ))
> +			sleep_time=$(( sleep_time - 1 ))
> +		done
> +		[ $temp_cur -gt $temp_high ] || tst_res TFAIL "Zone temperature is not rising as expected"
> +
> +		# Restore the original trip_point_1_temp value
> +		echo "$trip" > $i/trip_point_1_temp

FYI this is in /sys, otherwise for creating temporary file we would need in the
env.

#  "needs_tmpdir": true,

Thanks for your patch. I'm still not happy we introduce shell tests (sooner or
later somebody will invest his or his company time to rewrite that into C), but
I don't want to block this.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

NOTE: this needs to wait till my patch which adds ROD() is merged.

https://lore.kernel.org/ltp/20251120161957.331580-1-pvorel@suse.cz/

Kind regards,
Petr

> +
> +		# Check whether thermal interrupts count actually increased
> +		local interrupt_array_later=$(awk -F'[^0-9]*' '/Thermal event interrupts/ {$1=$1;print}' /proc/interrupts)
> +		tst_res TDEBUG "Current values of thermal interrupt counters: $interrupt_array_later"
> +		for j in $(seq 1 "$num"); do
> +			local interrupt_later=$(echo "$interrupt_array_later" | awk -v j=$j '{print $j}')
> +			local interrupt_init=$(echo "$interrupt_array_init" | awk -v j=$j '{print $j}')
> +			if [ $interrupt_later -le $interrupt_init ]; then
> +				status=1
> +			fi
> +		done
> +	done
> +
> +	if [ $status -eq 0 ]; then
> +		tst_res TPASS "x86 package thermal interrupt triggered"
> +	else
> +		tst_res TFAIL "x86 package thermal interrupt did not trigger"
> +	fi
> +}
> +
> +. tst_run.sh

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4] thermal: add new test group

[Piotr Kubaj]

This is a new test for checking thermal interrupt events.
Corrects issues pointed out by Petr Vorel for v3.

Signed-off-by: Piotr Kubaj <piotr.kubaj@intel.com>
---
 runtest/thermal                               |   3 +
 scenario_groups/default                       |   1 +
 testcases/kernel/Makefile                     |   1 +
 testcases/kernel/thermal/Makefile             |   9 +
 .../kernel/thermal/thermal_interrupt_events.c | 204 ++++++++++++++++++
 5 files changed, 218 insertions(+)
 create mode 100644 runtest/thermal
 create mode 100644 testcases/kernel/thermal/Makefile
 create mode 100644 testcases/kernel/thermal/thermal_interrupt_events.c

diff --git a/runtest/thermal b/runtest/thermal
new file mode 100644
index 000000000..57e3d29f8
--- /dev/null
+++ b/runtest/thermal
@@ -0,0 +1,3 @@
+# Thermal driver API
+# https://docs.kernel.org/driver-api/thermal/
+thermal_interrupt_events thermal_interrupt_events
diff --git a/scenario_groups/default b/scenario_groups/default
index 0e76b2bee..ffdd7ff25 100644
--- a/scenario_groups/default
+++ b/scenario_groups/default
@@ -26,3 +26,4 @@ crypto
 kernel_misc
 uevent
 watchqueue
+thermal
diff --git a/testcases/kernel/Makefile b/testcases/kernel/Makefile
index 98fd45a9d..ac816e4e8 100644
--- a/testcases/kernel/Makefile
+++ b/testcases/kernel/Makefile
@@ -36,6 +36,7 @@ SUBDIRS			+= connectors \
 			   sched \
 			   security \
 			   sound \
+			   thermal \
 			   tracing \
 			   uevents \
 			   watchqueue \
diff --git a/testcases/kernel/thermal/Makefile b/testcases/kernel/thermal/Makefile
new file mode 100644
index 000000000..4657c3fb3
--- /dev/null
+++ b/testcases/kernel/thermal/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2025, Intel Corporation. All rights reserved.
+# Author:Piotr Kubaj <piotr.kubaj@intel.com>
+
+top_srcdir             ?= ../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/thermal/thermal_interrupt_events.c b/testcases/kernel/thermal/thermal_interrupt_events.c
new file mode 100644
index 000000000..010487c9c
--- /dev/null
+++ b/testcases/kernel/thermal/thermal_interrupt_events.c
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Copyright (C) 2025-2026 Intel - http://www.intel.com/
+ */
+
+/*\
+ * Tests the CPU package thermal sensor interface for Intel platforms.
+
+ * Works by checking the initial count of thermal interrupts. Then it
+ * decreases the threshold for sending a thermal interrupt to just above
+ * the current temperature and runs a workload on the CPU. Finally, it restores
+ * the original thermal threshold and checks whether the number of thermal
+ * interrupts increased.
+ */
+
+#include "tst_safe_stdio.h"
+#include "tst_test.h"
+#define	PATH_LEN	69
+#define RUNTIME		30
+#define SLEEP		10
+#define	STRING_LEN	23
+#define TEMP_INCREMENT	10
+
+static char trip_path[PATH_LEN];
+static int nproc, trip;
+
+static void setup(void)
+{
+	nproc = tst_ncpus();
+	tst_res(TDEBUG, "Number of logical cores: %d", nproc);
+}
+
+static void cleanup(void)
+{
+	tst_res(TDEBUG, "Restoring original trip_point_1_temp value: %d", trip);
+	SAFE_FILE_PRINTF(trip_path, "%d", trip);
+}
+
+static void *cpu_workload(double run_time)
+{
+	time_t start_time = time(NULL);
+	int num = 2;
+
+	while (difftime(time(NULL), start_time) < run_time) {
+		for (int i = 2; i * i <= num; i++) {
+			if (num % i == 0)
+				break;
+		}
+		num++;
+	}
+	return NULL;
+}
+
+static void read_interrupts(uint64_t *interrupt_array, const int nproc)
+{
+	bool interrupts_found = 0;
+	char line[8192];
+
+	memset(interrupt_array, 0, nproc * sizeof(*interrupt_array));
+	FILE *fp = SAFE_FOPEN("/proc/interrupts", "r");
+
+	while (fgets(line, sizeof(line), fp)) {
+		if (strstr(line, "Thermal event interrupts")) {
+			interrupts_found = 1;
+			char *token = strtok(line, " ");
+
+			token = strtok(NULL, " ");
+			int i = 0;
+
+			while (!!strncmp(token, "Thermal", 7)) {
+				interrupt_array[i++] = atoll(token);
+				token = strtok(NULL, " ");
+				tst_res(TDEBUG, "Current value of interrupt_array[%d]: %ld", i - 1, interrupt_array[i - 1]);
+			}
+		}
+	}
+	SAFE_FCLOSE(fp);
+	if (!interrupts_found)
+		tst_brk(TCONF, "No Thermal event interrupts line in /proc/interrupts");
+}
+
+static void run(void)
+{
+	bool status = 1;
+	char line[8192];
+	uint64_t interrupt_init[nproc], interrupt_later[nproc];
+
+	read_interrupts(interrupt_init, nproc);
+
+	DIR *dir = SAFE_OPENDIR("/sys/class/thermal/");
+	struct dirent *entry;
+	int tz_counter = 0;
+
+	while ((entry = SAFE_READDIR(dir))) {
+		if ((strncmp(entry->d_name, "thermal_zone", sizeof("thermal_zone"))) > 0)
+			tz_counter++;
+	}
+	SAFE_CLOSEDIR(dir);
+	tst_res(TDEBUG, "Found %d thermal zone(s)", tz_counter);
+
+	bool x86_pkg_temp_tz[tz_counter], x86_pkg_temp_tz_found = 0;
+
+	memset(x86_pkg_temp_tz, 0, sizeof(x86_pkg_temp_tz));
+
+	for (int i = 0; i < tz_counter; i++) {
+		char path[PATH_LEN];
+
+		snprintf(path, PATH_LEN, "/sys/class/thermal/thermal_zone%d/type", i);
+		tst_res(TDEBUG, "Checking whether %s is x86_pkg_temp", path);
+
+		SAFE_FILE_SCANF(path, "%s", line);
+		if (strstr(line, "x86_pkg_temp")) {
+			tst_res(TDEBUG, "Thermal zone %d uses x86_pkg_temp", i);
+			x86_pkg_temp_tz[i] = 1;
+			x86_pkg_temp_tz_found = 1;
+		}
+	}
+	if (!x86_pkg_temp_tz_found) {
+		tst_res(TINFO, "No thermal zone uses x86_pkg_temp");
+		status = 0;
+	}
+
+	for (int i = 0; i < tz_counter; i++) {
+		if (x86_pkg_temp_tz[i]) {
+			char path[PATH_LEN], temp_path[PATH_LEN];
+			int sleep_time = SLEEP, temp_high, temp;
+			double run_time = RUNTIME;
+
+			snprintf(path, PATH_LEN, "/sys/class/thermal/thermal_zone%d/", i);
+			strncpy(temp_path, path, PATH_LEN);
+			strncat(temp_path, "temp", 4);
+			tst_res(TINFO, "Testing %s", temp_path);
+			SAFE_FILE_SCANF(temp_path, "%d", &temp);
+			if (temp < 0) {
+				tst_brk(TINFO, "Unexpected zone temperature value %d", temp);
+				status = 0;
+			}
+			tst_res(TDEBUG, "Current temperature for %s: %d", path, temp);
+
+			temp_high = temp + TEMP_INCREMENT;
+
+			strncpy(trip_path, path, PATH_LEN);
+			strncat(trip_path, "trip_point_1_temp", 17);
+
+			tst_res(TDEBUG, "Setting new trip_point_1_temp value: %d", temp_high);
+			SAFE_FILE_SCANF(trip_path, "%d", &trip);
+			SAFE_FILE_PRINTF(trip_path, "%d", temp_high);
+
+			while (sleep_time > 0) {
+				tst_res(TDEBUG, "Running for %f seconds, then sleeping for %d seconds", run_time, sleep_time);
+
+				for (int j = 0; j < nproc; j++) {
+					if (!SAFE_FORK()) {
+						cpu_workload(run_time);
+						exit(0);
+					}
+				}
+
+				for (int j = 0; j < nproc; j++)
+					tst_reap_children();
+
+				SAFE_FILE_SCANF(temp_path, "%d", &temp);
+				tst_res(TDEBUG, "Temperature for %s after a test: %d", path, temp);
+
+				if (temp > temp_high)
+					break;
+				sleep(sleep_time--);
+				run_time -= 3;
+			}
+			if (temp <= temp_high) {
+				tst_res(TINFO, "Zone temperature is not rising as expected");
+				status = 0;
+			}
+		}
+	}
+	read_interrupts(interrupt_later, nproc);
+
+	for (int i = 0; i < nproc; i++) {
+		if (interrupt_later[i] < interrupt_init[i]) {
+			tst_res(TINFO, "For CPU %d interrupt counter is currently %ld, while it was %ld before the test", i, interrupt_later[i], interrupt_init[i]);
+			status = 0;
+		}
+	}
+
+	if (status)
+		tst_res(TPASS, "x86 package thermal interrupt triggered");
+	else
+		tst_res(TFAIL, "x86 package thermal interrupt did not trigger");
+}
+
+static struct tst_test test = {
+	.cleanup = cleanup,
+	.forks_child = 1,
+	.min_runtime = 180,
+	.needs_root = 1,
+	.setup = setup,
+	.supported_archs = (const char *const []) {
+		"x86",
+		"x86_64",
+		NULL
+	},
+	.test_all = run
+};
-- 
2.47.3

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4] thermal: add new test group

[Petr Vorel]

Hi Piotr,

> This is a new test for checking thermal interrupt events.
> Corrects issues pointed out by Petr Vorel for v3.

> +++ b/testcases/kernel/thermal/thermal_interrupt_events.c
> @@ -0,0 +1,204 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (C) 2025-2026 Intel - http://www.intel.com/
> + */
> +
> +/*\
> + * Tests the CPU package thermal sensor interface for Intel platforms.
> +
> + * Works by checking the initial count of thermal interrupts. Then it
> + * decreases the threshold for sending a thermal interrupt to just above
> + * the current temperature and runs a workload on the CPU. Finally, it restores
> + * the original thermal threshold and checks whether the number of thermal
> + * interrupts increased.
> + */
> +
> +#include "tst_safe_stdio.h"
> +#include "tst_test.h"
very nit: blank line between includes and defines helps readability
> +#define	PATH_LEN	69
very nit: you mix tabs and spaces after #define.
I'd be ok to use NAME_MAX (255) from <limits.h> than to have an extra constant
just to save little bit of memory.
But maybe worth to define 8192 (/proc/interrupts line), which is used on 2 places.

> +#define RUNTIME		30
> +#define SLEEP		10
> +#define	STRING_LEN	23
You don't use STRING_LEN.

> +#define TEMP_INCREMENT	10
> +
> +static char trip_path[PATH_LEN];
> +static int nproc, trip;
> +
> +static void setup(void)
> +{
> +	nproc = tst_ncpus();
> +	tst_res(TDEBUG, "Number of logical cores: %d", nproc);
> +}
> +
> +static void cleanup(void)
> +{
> +	tst_res(TDEBUG, "Restoring original trip_point_1_temp value: %d", trip);
I don't think this message is useful. It's just a cleanup. Also if it fails,
you'll get message from LTP library.

> +	SAFE_FILE_PRINTF(trip_path, "%d", trip);
> +}
> +
> +static void *cpu_workload(double run_time)
> +{
> +	time_t start_time = time(NULL);
> +	int num = 2;
> +
> +	while (difftime(time(NULL), start_time) < run_time) {
> +		for (int i = 2; i * i <= num; i++) {
> +			if (num % i == 0)
> +				break;
> +		}
> +		num++;
> +	}
> +	return NULL;
> +}
> +
> +static void read_interrupts(uint64_t *interrupt_array, const int nproc)
very nit: maybe just interrupts? Variable names can be meaningful and yet short
enough :).

> +{
> +	bool interrupts_found = 0;
very nit: sure it works, but why not using true and false?

> +	char line[8192];
> +
> +	memset(interrupt_array, 0, nproc * sizeof(*interrupt_array));
> +	FILE *fp = SAFE_FOPEN("/proc/interrupts", "r");
> +
> +	while (fgets(line, sizeof(line), fp)) {
> +		if (strstr(line, "Thermal event interrupts")) {
> +			interrupts_found = 1;
> +			char *token = strtok(line, " ");
> +
> +			token = strtok(NULL, " ");
> +			int i = 0;
> +
> +			while (!!strncmp(token, "Thermal", 7)) {
> +				interrupt_array[i++] = atoll(token);
> +				token = strtok(NULL, " ");
> +				tst_res(TDEBUG, "Current value of interrupt_array[%d]: %ld", i - 1, interrupt_array[i - 1]);
nit: It's just a debug info, why not just (shorten long lines):
				tst_res(TDEBUG, "interrupts[%d]: %ld", i-1, interrupts[i-1]);

> +			}
We don't need processing any other line after TRM: right?

	while (fgets(line, sizeof(line), fp)) {
		if (!strstr(line, "Thermal event interrupts"))
			continue;

		interrupts_found = 1;
		char *token = strtok(line, " ");

		token = strtok(NULL, " ");
		int i = 0;

		while (!!strncmp(token, "Thermal", 7)) {
			interrupt_array[i++] = atoll(token);
			token = strtok(NULL, " ");
			tst_res(TDEBUG, "Current value of interrupt_array[%d]: %ld", i - 1, interrupt_array[i - 1]);
		}
		break;
	}

> +		}
> +	}
> +	SAFE_FCLOSE(fp);
> +	if (!interrupts_found)
> +		tst_brk(TCONF, "No Thermal event interrupts line in /proc/interrupts");
> +}
> +

> +static void run(void)
> +{
> +	bool status = 1;
> +	char line[8192];
> +	uint64_t interrupt_init[nproc], interrupt_later[nproc];
> +
> +	read_interrupts(interrupt_init, nproc);
> +
> +	DIR *dir = SAFE_OPENDIR("/sys/class/thermal/");
> +	struct dirent *entry;
> +	int tz_counter = 0;
> +
> +	while ((entry = SAFE_READDIR(dir))) {
> +		if ((strncmp(entry->d_name, "thermal_zone", sizeof("thermal_zone"))) > 0)
> +			tz_counter++;
> +	}
> +	SAFE_CLOSEDIR(dir);
> +	tst_res(TDEBUG, "Found %d thermal zone(s)", tz_counter);
As I noted previously, at least this part will not change if you run test more
times, does it? Why not to move it to the setup()?

Imagine running test 1000x iterations:
./thermal_interrupt_events -i 1000

Why to waste time with reading it again?

The only exception might be reading interrupts. I would expect it's ok to have
only the initial state (read in the setup() as well), but maybe (when test run
with more iterations via -i x) it needs to have the updated state (from the
previous iteration).

> +
> +	bool x86_pkg_temp_tz[tz_counter], x86_pkg_temp_tz_found = 0;
> +
> +	memset(x86_pkg_temp_tz, 0, sizeof(x86_pkg_temp_tz));
> +
> +	for (int i = 0; i < tz_counter; i++) {
> +		char path[PATH_LEN];
> +
> +		snprintf(path, PATH_LEN, "/sys/class/thermal/thermal_zone%d/type", i);
> +		tst_res(TDEBUG, "Checking whether %s is x86_pkg_temp", path);
> +
> +		SAFE_FILE_SCANF(path, "%s", line);
> +		if (strstr(line, "x86_pkg_temp")) {
> +			tst_res(TDEBUG, "Thermal zone %d uses x86_pkg_temp", i);
> +			x86_pkg_temp_tz[i] = 1;
> +			x86_pkg_temp_tz_found = 1;
> +		}
> +	}
> +	if (!x86_pkg_temp_tz_found) {
> +		tst_res(TINFO, "No thermal zone uses x86_pkg_temp");
And probably this part will not happen when you run more iterations (-i1000).

> +		status = 0;

If this happen, test fail, right? (you never set status = 1 later). Why to do
the rest of the testing?

I would really expect the whole part of run() up here is in the setup() and test
quits in this case:

if (!x86_pkg_temp_tz_found)
	tst_brk(TCONF, "No thermal zone uses x86_pkg_temp");

> +	}


> +
> +	for (int i = 0; i < tz_counter; i++) {
> +		if (x86_pkg_temp_tz[i]) {

run() is quite long. Maybe move content of of this loop would help.
Something like this (use whatever function name) would help the readability.

	for (int i = 0; i < tz_counter; i++) {
		if (x86_pkg_temp_tz[i])
			test_zone(x86_pkg_temp_tz[i]);
	}
Maybe even split the while part into it's own function.

> +			char path[PATH_LEN], temp_path[PATH_LEN];
> +			int sleep_time = SLEEP, temp_high, temp;
> +			double run_time = RUNTIME;
> +
> +			snprintf(path, PATH_LEN, "/sys/class/thermal/thermal_zone%d/", i);
> +			strncpy(temp_path, path, PATH_LEN);
> +			strncat(temp_path, "temp", 4);
> +			tst_res(TINFO, "Testing %s", temp_path);
> +			SAFE_FILE_SCANF(temp_path, "%d", &temp);
> +			if (temp < 0) {
> +				tst_brk(TINFO, "Unexpected zone temperature value %d", temp);
tst_brk(TINFO, ...) is wrong, because tst_brk() quits testing. TINFO is always
used with tst_res() (normal message).

I guess it should be either tst_brk(TCONF), as wrong temperature looks to me as
a bug.

> +				status = 0;
> +			}
> +			tst_res(TDEBUG, "Current temperature for %s: %d", path, temp);
> +
> +			temp_high = temp + TEMP_INCREMENT;
> +
> +			strncpy(trip_path, path, PATH_LEN);
> +			strncat(trip_path, "trip_point_1_temp", 17);
> +
> +			tst_res(TDEBUG, "Setting new trip_point_1_temp value: %d", temp_high);
> +			SAFE_FILE_SCANF(trip_path, "%d", &trip);
> +			SAFE_FILE_PRINTF(trip_path, "%d", temp_high);
> +
> +			while (sleep_time > 0) {
> +				tst_res(TDEBUG, "Running for %f seconds, then sleeping for %d seconds", run_time, sleep_time);
nit: %f should be %d, right?

				tst_res(TDEBUG, "Running for %d seconds, then sleeping for %d seconds",
					(int)run_time, sleep_time);
> +
> +				for (int j = 0; j < nproc; j++) {
> +					if (!SAFE_FORK()) {
> +						cpu_workload(run_time);
> +						exit(0);
> +					}
> +				}
> +
> +				for (int j = 0; j < nproc; j++)
> +					tst_reap_children();

tst_reap_children() should be called only once (not for all cpus).

Quoting doc:

	The 'tst_reap_children()' function makes the process wait for all of its
	children and exits with 'tst_brk(TBROK, ...)' if any of them returned
	a non zero exit code.

See function itself in lib/tst_test.c and "Test 3" in lib/newlib_tests/tst_checkpoint.c.

> +
> +				SAFE_FILE_SCANF(temp_path, "%d", &temp);
> +				tst_res(TDEBUG, "Temperature for %s after a test: %d", path, temp);
> +
> +				if (temp > temp_high)
> +					break;
> +				sleep(sleep_time--);
> +				run_time -= 3;
> +			}
> +			if (temp <= temp_high) {
> +				tst_res(TINFO, "Zone temperature is not rising as expected");
I'm not the expert on the topic, but IMHO how about have this as TFAIL
and print at the end only TPASS if no error found?

				tst_res(TFAIL, "CPU %d: Zone temperature is not rising as expected", i);

> +				status = 0;
> +			}
> +		}
> +	}
> +	read_interrupts(interrupt_later, nproc);
> +
> +	for (int i = 0; i < nproc; i++) {
> +		if (interrupt_later[i] < interrupt_init[i]) {
> +			tst_res(TINFO, "For CPU %d interrupt counter is currently %ld, while it was %ld before the test", i, interrupt_later[i], interrupt_init[i]);
very nit: this line is really too long.

			tst_res(TFAIL, "CPU %d interrupt counter: %ld (previous: %ld)",
				i, interrupt_later[i], interrupt_init[i]);

> +			status = 0;
> +		}
> +	}
> +
> +	if (status)
> +		tst_res(TPASS, "x86 package thermal interrupt triggered");
> +	else
> +		tst_res(TFAIL, "x86 package thermal interrupt did not trigger");

	if (status)
		tst_res(TPASS, "All interrupts triggered");

(Don't print final TFAIL when errors were printed previously.)
...

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4] thermal: add new test group

[Kubaj, Piotr]

2026-01-23 (金) の 21:25 +0100 に Petr Vorel さんは書きました:
> Hi Piotr,
> 
> > This is a new test for checking thermal interrupt events.
> > Corrects issues pointed out by Petr Vorel for v3.
> 
> > +++ b/testcases/kernel/thermal/thermal_interrupt_events.c
> > @@ -0,0 +1,204 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +/*
> > + * Copyright (C) 2025-2026 Intel - http://www.intel.com/
> > + */
> > +
> > +/*\
> > + * Tests the CPU package thermal sensor interface for Intel
> > platforms.
> > +
> > + * Works by checking the initial count of thermal interrupts. Then
> > it
> > + * decreases the threshold for sending a thermal interrupt to just
> > above
> > + * the current temperature and runs a workload on the CPU.
> > Finally, it restores
> > + * the original thermal threshold and checks whether the number of
> > thermal
> > + * interrupts increased.
> > + */
> > +
> > +#include "tst_safe_stdio.h"
> > +#include "tst_test.h"
> very nit: blank line between includes and defines helps readability
Done.

> > +#define	PATH_LEN	69
> very nit: you mix tabs and spaces after #define.
> I'd be ok to use NAME_MAX (255) from <limits.h> than to have an extra
> constant
> just to save little bit of memory.
> But maybe worth to define 8192 (/proc/interrupts line), which is used
> on 2 places.
I switched to NAME_MAX. Increasing by 32x seems unreasonable to me.

> 
> > +#define RUNTIME		30
> > +#define SLEEP		10
> > +#define	STRING_LEN	23
> You don't use STRING_LEN.
Removed.

> 
> > +#define TEMP_INCREMENT	10
> > +
> > +static char trip_path[PATH_LEN];
> > +static int nproc, trip;
> > +
> > +static void setup(void)
> > +{
> > +	nproc = tst_ncpus();
> > +	tst_res(TDEBUG, "Number of logical cores: %d", nproc);
> > +}
> > +
> > +static void cleanup(void)
> > +{
> > +	tst_res(TDEBUG, "Restoring original trip_point_1_temp
> > value: %d", trip);
> I don't think this message is useful. It's just a cleanup. Also if it
> fails,
> you'll get message from LTP library.
Removed.

> 
> > +	SAFE_FILE_PRINTF(trip_path, "%d", trip);
> > +}
> > +
> > +static void *cpu_workload(double run_time)
> > +{
> > +	time_t start_time = time(NULL);
> > +	int num = 2;
> > +
> > +	while (difftime(time(NULL), start_time) < run_time) {
> > +		for (int i = 2; i * i <= num; i++) {
> > +			if (num % i == 0)
> > +				break;
> > +		}
> > +		num++;
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static void read_interrupts(uint64_t *interrupt_array, const int
> > nproc)
> very nit: maybe just interrupts? Variable names can be meaningful and
> yet short
> enough :).
Done.

> 
> > +{
> > +	bool interrupts_found = 0;
> very nit: sure it works, but why not using true and false?
Done.

> 
> > +	char line[8192];
> > +
> > +	memset(interrupt_array, 0, nproc *
> > sizeof(*interrupt_array));
> > +	FILE *fp = SAFE_FOPEN("/proc/interrupts", "r");
> > +
> > +	while (fgets(line, sizeof(line), fp)) {
> > +		if (strstr(line, "Thermal event interrupts")) {
> > +			interrupts_found = 1;
> > +			char *token = strtok(line, " ");
> > +
> > +			token = strtok(NULL, " ");
> > +			int i = 0;
> > +
> > +			while (!!strncmp(token, "Thermal", 7)) {
> > +				interrupt_array[i++] =
> > atoll(token);
> > +				token = strtok(NULL, " ");
> > +				tst_res(TDEBUG, "Current value of
> > interrupt_array[%d]: %ld", i - 1, interrupt_array[i - 1]);
> nit: It's just a debug info, why not just (shorten long lines):
> 				tst_res(TDEBUG, "interrupts[%d]:
> %ld", i-1, interrupts[i-1]);
Done.

> 
> > +			}
> We don't need processing any other line after TRM: right?
Changed.

> 
> 	while (fgets(line, sizeof(line), fp)) {
> 		if (!strstr(line, "Thermal event interrupts"))
> 			continue;
> 
> 		interrupts_found = 1;
> 		char *token = strtok(line, " ");
> 
> 		token = strtok(NULL, " ");
> 		int i = 0;
> 
> 		while (!!strncmp(token, "Thermal", 7)) {
> 			interrupt_array[i++] = atoll(token);
> 			token = strtok(NULL, " ");
> 			tst_res(TDEBUG, "Current value of
> interrupt_array[%d]: %ld", i - 1, interrupt_array[i - 1]);
> 		}
> 		break;
> 	}
> 
> > +		}
> > +	}
> > +	SAFE_FCLOSE(fp);
> > +	if (!interrupts_found)
> > +		tst_brk(TCONF, "No Thermal event interrupts line
> > in /proc/interrupts");
> > +}
> > +
> 
> > +static void run(void)
> > +{
> > +	bool status = 1;
> > +	char line[8192];
> > +	uint64_t interrupt_init[nproc], interrupt_later[nproc];
> > +
> > +	read_interrupts(interrupt_init, nproc);
> > +
> > +	DIR *dir = SAFE_OPENDIR("/sys/class/thermal/");
> > +	struct dirent *entry;
> > +	int tz_counter = 0;
> > +
> > +	while ((entry = SAFE_READDIR(dir))) {
> > +		if ((strncmp(entry->d_name, "thermal_zone",
> > sizeof("thermal_zone"))) > 0)
> > +			tz_counter++;
> > +	}
> > +	SAFE_CLOSEDIR(dir);
> > +	tst_res(TDEBUG, "Found %d thermal zone(s)", tz_counter);
> As I noted previously, at least this part will not change if you run
> test more
> times, does it? Why not to move it to the setup()?
> 
> Imagine running test 1000x iterations:
> ./thermal_interrupt_events -i 1000
> 
> Why to waste time with reading it again?
> 
> The only exception might be reading interrupts. I would expect it's
> ok to have
> only the initial state (read in the setup() as well), but maybe (when
> test run
> with more iterations via -i x) it needs to have the updated state
> (from the
> previous iteration).
That part is still in consultation with our architect.

> 
> > +
> > +	bool x86_pkg_temp_tz[tz_counter], x86_pkg_temp_tz_found =
> > 0;
> > +
> > +	memset(x86_pkg_temp_tz, 0, sizeof(x86_pkg_temp_tz));
> > +
> > +	for (int i = 0; i < tz_counter; i++) {
> > +		char path[PATH_LEN];
> > +
> > +		snprintf(path, PATH_LEN,
> > "/sys/class/thermal/thermal_zone%d/type", i);
> > +		tst_res(TDEBUG, "Checking whether %s is
> > x86_pkg_temp", path);
> > +
> > +		SAFE_FILE_SCANF(path, "%s", line);
> > +		if (strstr(line, "x86_pkg_temp")) {
> > +			tst_res(TDEBUG, "Thermal zone %d uses
> > x86_pkg_temp", i);
> > +			x86_pkg_temp_tz[i] = 1;
> > +			x86_pkg_temp_tz_found = 1;
> > +		}
> > +	}
> > +	if (!x86_pkg_temp_tz_found) {
> > +		tst_res(TINFO, "No thermal zone uses
> > x86_pkg_temp");
> And probably this part will not happen when you run more iterations
> (-i1000).
Changed.

> 
> > +		status = 0;
> 
> If this happen, test fail, right? (you never set status = 1 later).
> Why to do
> the rest of the testing?
> 
> I would really expect the whole part of run() up here is in the
> setup() and test
> quits in this case:
> 
> if (!x86_pkg_temp_tz_found)
> 	tst_brk(TCONF, "No thermal zone uses x86_pkg_temp");
> 
> > +	}
Changed.

> 
> 
> > +
> > +	for (int i = 0; i < tz_counter; i++) {
> > +		if (x86_pkg_temp_tz[i]) {
> 
> run() is quite long. Maybe move content of of this loop would help.
> Something like this (use whatever function name) would help the
> readability.
> 
> 	for (int i = 0; i < tz_counter; i++) {
> 		if (x86_pkg_temp_tz[i])
> 			test_zone(x86_pkg_temp_tz[i]);
> 	}
> Maybe even split the while part into it's own function.
Changed. I wanted to avoid creating functions that were only used once.

> 
> > +			char path[PATH_LEN], temp_path[PATH_LEN];
> > +			int sleep_time = SLEEP, temp_high, temp;
> > +			double run_time = RUNTIME;
> > +
> > +			snprintf(path, PATH_LEN,
> > "/sys/class/thermal/thermal_zone%d/", i);
> > +			strncpy(temp_path, path, PATH_LEN);
> > +			strncat(temp_path, "temp", 4);
> > +			tst_res(TINFO, "Testing %s", temp_path);
> > +			SAFE_FILE_SCANF(temp_path, "%d", &temp);
> > +			if (temp < 0) {
> > +				tst_brk(TINFO, "Unexpected zone
> > temperature value %d", temp);
> tst_brk(TINFO, ...) is wrong, because tst_brk() quits testing. TINFO
> is always
> used with tst_res() (normal message).
> 
> I guess it should be either tst_brk(TCONF), as wrong temperature
> looks to me as
> a bug.
Changed.

> 
> > +				status = 0;
> > +			}
> > +			tst_res(TDEBUG, "Current temperature for
> > %s: %d", path, temp);
> > +
> > +			temp_high = temp + TEMP_INCREMENT;
> > +
> > +			strncpy(trip_path, path, PATH_LEN);
> > +			strncat(trip_path, "trip_point_1_temp",
> > 17);
> > +
> > +			tst_res(TDEBUG, "Setting new
> > trip_point_1_temp value: %d", temp_high);
> > +			SAFE_FILE_SCANF(trip_path, "%d", &trip);
> > +			SAFE_FILE_PRINTF(trip_path, "%d",
> > temp_high);
> > +
> > +			while (sleep_time > 0) {
> > +				tst_res(TDEBUG, "Running for %f
> > seconds, then sleeping for %d seconds", run_time, sleep_time);
> nit: %f should be %d, right?
run_time is double, because difftime returns double. Switching to %d
causes a warning. If you prefer, I might add casting to int and then
%d.

> 
> 				tst_res(TDEBUG, "Running for %d
> seconds, then sleeping for %d seconds",
> 					(int)run_time, sleep_time);
> > +
> > +				for (int j = 0; j < nproc; j++) {
> > +					if (!SAFE_FORK()) {
> > +						cpu_workload(run_t
> > ime);
> > +						exit(0);
> > +					}
> > +				}
> > +
> > +				for (int j = 0; j < nproc; j++)
> > +					tst_reap_children();
> 
> tst_reap_children() should be called only once (not for all cpus).
> 
> Quoting doc:
> 
> 	The 'tst_reap_children()' function makes the process wait
> for all of its
> 	children and exits with 'tst_brk(TBROK, ...)' if any of them
> returned
> 	a non zero exit code.
> 
> See function itself in lib/tst_test.c and "Test 3" in
> lib/newlib_tests/tst_checkpoint.c.
Fixed.

> 
> > +
> > +				SAFE_FILE_SCANF(temp_path, "%d",
> > &temp);
> > +				tst_res(TDEBUG, "Temperature for
> > %s after a test: %d", path, temp);
> > +
> > +				if (temp > temp_high)
> > +					break;
> > +				sleep(sleep_time--);
> > +				run_time -= 3;
> > +			}
> > +			if (temp <= temp_high) {
> > +				tst_res(TINFO, "Zone temperature
> > is not rising as expected");
> I'm not the expert on the topic, but IMHO how about have this as
> TFAIL
> and print at the end only TPASS if no error found?
> 
> 				tst_res(TFAIL, "CPU %d: Zone
> temperature is not rising as expected", i);
> 
> > +				status = 0;
> > +			}
> > +		}
> > +	}
> > +	read_interrupts(interrupt_later, nproc);
> > +
> > +	for (int i = 0; i < nproc; i++) {
> > +		if (interrupt_later[i] < interrupt_init[i]) {
> > +			tst_res(TINFO, "For CPU %d interrupt
> > counter is currently %ld, while it was %ld before the test", i,
> > interrupt_later[i], interrupt_init[i]);
> very nit: this line is really too long.
> 
> 			tst_res(TFAIL, "CPU %d interrupt counter:
> %ld (previous: %ld)",
> 				i, interrupt_later[i],
> interrupt_init[i]);
Changed.

> 
> > +			status = 0;
> > +		}
> > +	}
> > +
> > +	if (status)
> > +		tst_res(TPASS, "x86 package thermal interrupt
> > triggered");
> > +	else
> > +		tst_res(TFAIL, "x86 package thermal interrupt did
> > not trigger");
> 
> 	if (status)
> 		tst_res(TPASS, "All interrupts triggered");
> 
> (Don't print final TFAIL when errors were printed previously.)
Fixed.

> ...
> 
> Kind regards,
> Petr
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4] thermal: add new test group

[Petr Vorel]

Hi Piotr,

...
> > > +static void run(void)
> > > +{
> > > +	bool status = 1;
> > > +	char line[8192];
> > > +	uint64_t interrupt_init[nproc], interrupt_later[nproc];
> > > +
> > > +	read_interrupts(interrupt_init, nproc);
> > > +
> > > +	DIR *dir = SAFE_OPENDIR("/sys/class/thermal/");
> > > +	struct dirent *entry;
> > > +	int tz_counter = 0;
> > > +
> > > +	while ((entry = SAFE_READDIR(dir))) {
> > > +		if ((strncmp(entry->d_name, "thermal_zone",
> > > sizeof("thermal_zone"))) > 0)
> > > +			tz_counter++;
> > > +	}
> > > +	SAFE_CLOSEDIR(dir);
> > > +	tst_res(TDEBUG, "Found %d thermal zone(s)", tz_counter);
> > As I noted previously, at least this part will not change if you run
> > test more
> > times, does it? Why not to move it to the setup()?

> > Imagine running test 1000x iterations:
> > ./thermal_interrupt_events -i 1000

> > Why to waste time with reading it again?

> > The only exception might be reading interrupts. I would expect it's
> > ok to have
> > only the initial state (read in the setup() as well), but maybe (when
> > test run
> > with more iterations via -i x) it needs to have the updated state
> > (from the
> > previous iteration).
> That part is still in consultation with our architect.

Thank you! Of course, it's ok to keep it if it's needed.

...
> > > +	for (int i = 0; i < tz_counter; i++) {
> > > +		if (x86_pkg_temp_tz[i]) {

> > run() is quite long. Maybe move content of of this loop would help.
> > Something like this (use whatever function name) would help the
> > readability.

> > 	for (int i = 0; i < tz_counter; i++) {
> > 		if (x86_pkg_temp_tz[i])
> > 			test_zone(x86_pkg_temp_tz[i]);
> > 	}
> > Maybe even split the while part into it's own function.
> Changed. I wanted to avoid creating functions that were only used once.

Understand, but there is also code readability which matters.

...
> > > +
> > > +			while (sleep_time > 0) {
> > > +				tst_res(TDEBUG, "Running for %f
> > > seconds, then sleeping for %d seconds", run_time, sleep_time);
> > nit: %f should be %d, right?
> run_time is double, because difftime returns double. Switching to %d
> causes a warning. If you prefer, I might add casting to int and then
> %d.

The output looks better but it's very minor.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp