x86_energy_perf_policy cleanups

PATCH Superseded Len Brown 2 participants 43 messages Oct 19 – Oct 25

Share & tools

Len Brown

author

2026-10-19 16:52 UTC #0 lore raw

cover x86_energy_perf_policy cleanups

This batch is mostly the result of an experiment to see if AI is good enough to find issues, then fix and document them.

Turned out that AI has its strengths, and its weaknesses, and figuring out which is which can be tedious.

Bonus: If you find issues in this series,
you can claim you are smarter than AI. (Andy Shevchenko is:-)

thanks,
Len Brown
Intel Open Source Technology Center

[PATCH 01/18] x86_energy_perf_policy: Create msr_path helper [PATCH 02/18] x86_energy_perf_policy: Enhance MSR probe [PATCH 03/18] x86_energy_perf_policy: Prevent unlikely buffer [PATCH 04/18] x86_energy_perf_policy: Cleanup: Harden with strtol() [PATCH 05/18] x86_energy_perf_policy: Cleanup: Use snprintf() for [PATCH 06/18] x86_energy_perf_policy: Fix optarg keyword matching [PATCH 07/18] x86_energy_perf_policy: Cleanup: Fix potential ratio [PATCH 08/18] x86_energy_perf_policy: Bound package iteration [PATCH 09/18] x86_energy_perf_policy: Cleanup: Define HWP window max [PATCH 10/18] x86_energy_perf_policy: Cleanup: Define BCLK_MHZ [PATCH 11/18] x86_energy_perf_policy: Cleanup: Check fseek() in VM [PATCH 12/18] x86_energy_perf_policy: Document MSRHEADER [PATCH 13/18] x86_energy_perf_policy: Cleanup: Drop unused signal.h [PATCH 14/18] x86_energy_perf_policy: Cleanup: Constify profile name [PATCH 15/18] x86_energy_perf_policy: Cleanup: Constify HWP strings [PATCH 16/18] x86_energy_perf_policy: Fix use-after-free in [PATCH 17/18] x86_energy_perf_policy: Fix write_sysfs() return type [PATCH 18/18] MAINTAINERS: Add x86_energy_perf_policy

Len Brown

author

2026-10-19 16:52 UTC #1 lore raw

PATCH 1/18 x86_energy_perf_policy: Create msr_path helper

From: Len Brown <[email protected]>

Factor out code that creates msr_path[].

No functional change.

Reviewed-by Andy Shevchenko <[email protected]>

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../x86_energy_perf_policy.c                  | 38 +++++++++++--------
 1 file changed, 23 insertions(+), 15 deletions(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +23 ~~-15~~

index 0dc959e30076..a963c0430794 100644
@@ -98,6 +98,7 @@ unsigned char update_platform_profile;
 int soc_slider_balance;
 int soc_slider_offset;
 char platform_profile[64];
+char msr_path[64];
 
 #define PATH_TO_CPU "/sys/devices/system/cpu/"
 #define SYSFS_PATH_MAX 255

@@ -799,21 +800,31 @@ void err_on_hypervisor(void)
 		err(-1, "not supported on this virtual machine");
 }
 
+/*
+ * The Linux MSR driver uses offsets into /dev/cpu/{cpu#}/msr
+ * Some Android versions uses an incompatible path: /dev/msr{cpu#}
+ * Probe and prefer the Linux path, use Android if Linux does not work.
+ */
+static void set_msr_path(int cpu)
+{
+	if (snprintf(msr_path, sizeof(msr_path), use_android_msr_path ? "/dev/msr%d" : "/dev/cpu/%d/msr", cpu) >= (int)sizeof(msr_path))
+		err(-1, "MSR path buffer overflow for cpu%d", cpu);
+}
+
 int get_msr(int cpu, int offset, unsigned long long *msr)
 {
 	int retval;
-	char pathname[32];
 	int fd;
 
-	sprintf(pathname, use_android_msr_path ? "/dev/msr%d" : "/dev/cpu/%d/msr", cpu);
-	fd = open(pathname, O_RDONLY);
+	set_msr_path(cpu);
+	fd = open(msr_path, O_RDONLY);
 	if (fd < 0)
-		err(-1, "%s open failed, try chown or chmod +r %s, or run as root", pathname, use_android_msr_path ? "/dev/msr*" : "/dev/cpu/*/msr");
+		err(-1, "%s open failed, try chown or chmod +r, or run as root", msr_path);
 
 	retval = pread(fd, msr, sizeof(*msr), offset);
 	if (retval != sizeof(*msr)) {
 		err_on_hypervisor();
-		err(-1, "%s offset 0x%llx read failed", pathname, (unsigned long long)offset);
+		err(-1, "%s offset 0x%llx read failed", msr_path, (unsigned long long)offset);
 	}
 
 	if (debug > 1)

@@ -825,14 +836,13 @@ int get_msr(int cpu, int offset, unsigned long long *msr)
 
 int put_msr(int cpu, int offset, unsigned long long new_msr)
 {
-	char pathname[32];
 	int retval;
 	int fd;
 
-	sprintf(pathname, use_android_msr_path ? "/dev/msr%d" : "/dev/cpu/%d/msr", cpu);
-	fd = open(pathname, O_RDWR);
+	set_msr_path(cpu);
+	fd = open(msr_path, O_RDWR);
 	if (fd < 0)
-		err(-1, "%s open failed, try chown or chmod +r %s, or run as root", pathname, use_android_msr_path ? "/dev/msr*" : "/dev/cpu/*/msr");
+		err(-1, "%s open failed, try chown or chmod +r, or run as root", msr_path);
 
 	retval = pwrite(fd, &new_msr, sizeof(new_msr), offset);
 	if (retval != sizeof(new_msr))

@@ -1604,22 +1614,20 @@ void set_base_cpu(void)
 static void probe_android_msr_path(void)
 {
 	struct stat sb;
-	char test_path[32];
 
-	sprintf(test_path, "/dev/msr%d", base_cpu);
-	if (stat(test_path, &sb) == 0)
+	set_msr_path(base_cpu);	/* called with use_android_path = 0, to probe Linux path */
+	if (stat(msr_path, &sb))
 		use_android_msr_path = 1;
 }
 
 void probe_dev_msr(void)
 {
 	struct stat sb;
-	char pathname[32];
 
 	probe_android_msr_path();
 
-	sprintf(pathname, use_android_msr_path ? "/dev/msr%d" : "/dev/cpu/%d/msr", base_cpu);
-	if (stat(pathname, &sb)) {
+	set_msr_path(base_cpu);
+	if (stat(msr_path, &sb)) {
 		if (system("/sbin/modprobe msr > /dev/null 2>&1")) {
 			if (use_android_msr_path)
 				err(-5, "no /dev/msr0, Try \"# modprobe msr\" ");

Signature · 1 line

2.45.2

Len Brown

author

2026-10-19 16:52 UTC #2 lore raw

PATCH 2/18 x86_energy_perf_policy: Enhance MSR probe

From: Len Brown <[email protected]>

If Linux MSR driver isn't running,
modprobe and try the Linux path again.
If still no joy, try Android path.

Signed-off-by Len Brown <[email protected]>

---
 .../x86_energy_perf_policy.c                  | 40 ++++++++++++-------
 1 file changed, 26 insertions(+), 14 deletions(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +26 ~~-14~~

index a963c0430794..abd3cf2fdd39 100644
@@ -15,6 +15,7 @@
 #include <sys/types.h>
 #include <sched.h>
 #include <sys/stat.h>
+#include <sys/wait.h>
 #include <sys/resource.h>
 #include <getopt.h>
 #include <err.h>

@@ -1611,30 +1612,41 @@ void set_base_cpu(void)
 		err(-ENODEV, "No valid cpus found");
 }
 
-static void probe_android_msr_path(void)
+/* return 0 if path exists */
+static int check_msr_path(void)
 {
 	struct stat sb;
 
-	set_msr_path(base_cpu);	/* called with use_android_path = 0, to probe Linux path */
-	if (stat(msr_path, &sb))
-		use_android_msr_path = 1;
+	return stat(msr_path, &sb);
 }
 
+/*
+ * Check Linux MSR path
+ * If it fails, try modprobe and check it again
+ * If still no joy, enable Android path and try again
+ */
 void probe_dev_msr(void)
 {
-	struct stat sb;
+	int retval;
+
+	use_android_msr_path = 0;
+	set_msr_path(base_cpu);
+	if (check_msr_path() == 0)
+		return;
 
-	probe_android_msr_path();
+	retval = system("/sbin/modprobe msr > /dev/null 2>&1");
 
+	if (check_msr_path() == 0)
+		return;
+
+	use_android_msr_path = 1;
 	set_msr_path(base_cpu);
-	if (stat(msr_path, &sb)) {
-		if (system("/sbin/modprobe msr > /dev/null 2>&1")) {
-			if (use_android_msr_path)
-				err(-5, "no /dev/msr0, Try \"# modprobe msr\" ");
-			else
-				err(-5, "no /dev/cpu/0/msr, Try \"# modprobe msr\" ");
-		}
-	}
+	if (check_msr_path() == 0)
+		return;
+
+	if (WIFEXITED(retval))
+		retval = WEXITSTATUS(retval);
+	err(-1, "\"msr\" driver support required, modprobe msr returned %d", retval);
 }
 
 static void get_cpuid_or_exit(unsigned int leaf, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx)

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 2/18

2026-10-20 09:45 UTC #3 lore raw

Re: [PATCH 02/18] x86_energy_perf_policy: Enhance MSR probe

Quoted 3 lines · Len Brown

If Linux MSR driver isn't running,
modprobe and try the Linux path again.
If still no joy, try Android path.

Isn't it too condensed? We have more room on each line...

Quoted 1 line

Signed-off-by Len Brown <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

(One idea below, but it's up to you.)

...

Quoted 23 lines

void probe_dev_msr(void)
{

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +11 -7

-	struct stat sb;
+	int retval;
+
+	use_android_msr_path = 0;
+	set_msr_path(base_cpu);
+	if (check_msr_path() == 0)
+		return;
 
-	probe_android_msr_path();
+	retval = system("/sbin/modprobe msr > /dev/null 2>&1");
 
+	if (check_msr_path() == 0)
+		return;
+
+	use_android_msr_path = 1;
 	set_msr_path(base_cpu);
-	if (stat(msr_path, &sb)) {
-		if (system("/sbin/modprobe msr > /dev/null 2>&1")) {
-			if (use_android_msr_path)
-				err(-5, "no /dev/msr0, Try \"# modprobe msr\" ");
-			else

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c void set_base_cpu(void) · line 1650

Quoted 10 lines

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +6 -3

-				err(-5, "no /dev/cpu/0/msr, Try \"# modprobe msr\" ");
-		}
-	}
+	if (check_msr_path() == 0)
+		return;
+
+	if (WIFEXITED(retval))
+		retval = WEXITSTATUS(retval);
+	err(-1, "\"msr\" driver support required, modprobe msr returned %d", retval);
 }

There is a potential in having a helper

static int try_probe_dev_msr(int android)

{
	use_android_msr_path = android;
	set_msr_path(base_cpu);
	return check_msr_path();
}

void probe_dev_msr(void)

{
	int retval;

	if (try_probe_dev_msr(0) == 0)
		return;

	retval = system("/sbin/modprobe msr > /dev/null 2>&1");

	if (check_msr_path() == 0)
		return;

	if (try_probe_dev_msr(1) == 0)
		return;

	if (WIFEXITED(retval))
		retval = WEXITSTATUS(retval);
	err(-1, "\"msr\" driver support required, modprobe msr returned %d", retval);
}

But original is shorter.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #4 lore raw

PATCH 3/18 x86_energy_perf_policy: Prevent unlikely buffer overflow

In reply to Len Brown

From: Len Brown <[email protected]>

Protect update_cpufreq_scaling_freq() from buffer overflow if somehow it was called with a huge CPU number.

Fixes f1c35c73191b ("tools/power x86_energy_perf_policy: Version 2026.04.25")

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +2 -2

index abd3cf2fdd39..3c4e376f6c77 100644
@@ -1166,12 +1166,12 @@ int ratio_2_sysfs_khz(int ratio)
  */
 void update_cpufreq_scaling_freq(int is_max, int cpu, unsigned int ratio)
 {
-	char pathname[64];
+	char pathname[96];
 	FILE *fp;
 	int retval;
 	int khz;
 
-	sprintf(pathname, "/sys/devices/system/cpu/cpu%d/cpufreq/scaling_%s_freq", cpu, is_max ? "max" : "min");
+	snprintf(pathname, sizeof(pathname), "/sys/devices/system/cpu/cpu%d/cpufreq/scaling_%s_freq", cpu, is_max ? "max" : "min");
 
 	fp = fopen(pathname, "w");
 	if (!fp) {

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 3/18

2026-10-20 09:47 UTC #5 lore raw

Re: [PATCH 03/18] x86_energy_perf_policy: Prevent unlikely buffer overflow

Quoted 2 lines · Len Brown

Protect update_cpufreq_scaling_freq() from buffer overflow if somehow it was called with a huge CPU number.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #6 lore raw

PATCH 4/18 x86_energy_perf_policy: Cleanup: Harden with strtol()

In reply to Len Brown

From: Len Brown <[email protected]>

Replace atoi() with strtol() and proper error checking when parsing the --hwp-use-pkg command-line argument value, and other spots.

Also add explicit check that value is in valid range [0, 1].

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../x86_energy_perf_policy.c                  | 46 +++++++++++--------
 1 file changed, 26 insertions(+), 20 deletions(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +26 ~~-20~~

index 3c4e376f6c77..98e5aa9aec51 100644
@@ -191,7 +191,7 @@ int parse_cmdline_epb(int i)
 		return ENERGY_PERF_BIAS_PERFORMANCE;
 	}
 	if (i < 0 || i > ENERGY_PERF_BIAS_POWERSAVE)
-		errx(1, "--epb must be from 0 to 15");
+		errx(1, "--epb must be 0 through 15");
 	return i;
 }

@@ -305,8 +305,8 @@ int parse_cmdline_hwp_epp(int i)
 	case OPTARG_PERFORMANCE:
 		return HWP_EPP_PERFORMANCE;
 	}
-	if (i < 0 || i > 0xff) {
-		fprintf(stderr, "--hwp-epp must be from 0 to 0xff\n");
+	if (i < 0 || i > 255) {
+		fprintf(stderr, "--hwp-epp must be 0 through 255\n");
 		usage();
 	}
 	return i;

@@ -355,19 +355,13 @@ int parse_optarg_string(char *s)
 	if (!strncmp(s, "performance", 11))
 		return OPTARG_PERFORMANCE;
 
+	errno = 0;
 	i = strtol(s, &endptr, 0);
-	if (s == endptr) {
-		fprintf(stderr, "no digits in \"%s\"\n", s);
+	if (endptr == s || errno == ERANGE || i < 0 || i > 256) {
+		warnx("bad value \"%s\"\n", s);
 		usage();
 	}
-	if (i == LONG_MIN || i == LONG_MAX)
-		errx(-1, "%s", s);
 
-	if (i > 0xFF)
-		errx(-1, "%d (0x%x) must be < 256", i, i);
-
-	if (i < 0)
-		errx(-1, "%d (0x%x) must be >= 0", i, i);
 	return i;
 }

@@ -425,7 +419,7 @@ void parse_cmdline_cpu(char *s)
 
 			startp++;
 			end_cpu = strtol(startp, &endp, 10);
-			if (startp == endp)
+			if (endp == startp)
 				continue;
 
 			while (cpu <= end_cpu) {

@@ -469,11 +463,11 @@ void parse_cmdline_cpu(char *s)
 				break;
 		}
 
+		errno = 0;
 		cpu = strtol(startp, &endp, 10);
-		if (startp == endp)
+		if (endp == startp || errno == ERANGE || cpu < 0 || cpu > max_cpu_num)
 			errx(1, "--cpu cpu-set: confused by '%s'", startp);
-		if (cpu > max_cpu_num)
-			errx(1, "Requested cpu%d exceeds max cpu%d", cpu, max_cpu_num);
+
 		CPU_SET_S(cpu, cpu_setsize, cpu_selected_set);
 		startp = endp;
 	}

@@ -505,7 +499,7 @@ void parse_cmdline_pkg(char *s)
 
 			startp++;
 			end_pkg = strtol(startp, &endp, 10);
-			if (startp == endp)
+			if (endp == startp)
 				continue;
 
 			while (pkg <= end_pkg) {

@@ -526,6 +520,7 @@ void parse_cmdline_pkg(char *s)
 		pkg = strtol(startp, &endp, 10);
 		if (pkg > max_pkg_num)
 			errx(1, "Requested pkg%d Exceeds max pkg%d", pkg, max_pkg_num);
+
 		pkg_selected_set |= 1 << pkg;
 		startp = endp;
 	}

@@ -546,6 +541,7 @@ static int parse_cmdline_int(const char *s, int *out)
 	char *endp;
 	long val;
 
+	errno = 0;
 	val = strtol(s, &endp, 0);
 	if (endp == s || errno == ERANGE)
 		return -1;

@@ -716,7 +712,15 @@ void cmdline(int argc, char **argv)
 			break;
 		case 'u':
 			update_hwp_use_pkg++;
-			if (atoi(optarg) == 0)
+			char *endp;
+			long val;
+
+			errno = 0;
+			val = strtol(optarg, &endp, 10);
+			if (endp == optarg || errno == ERANGE || val < 0 || val > 1)
+				errx(1, "--hwp-use-pkg: must be 0 or 1");
+
+			if (val == 0)
 				req_update.hwp_use_pkg = 0;
 			else
 				req_update.hwp_use_pkg = 1;

@@ -1015,8 +1019,9 @@ static int get_epb_sysfs(int cpu)
 	if (!read_sysfs(path, linebuf, 3))
 		return -1;
 
+	errno = 0;
 	val = strtol(linebuf, &endp, 0);
-	if (endp == linebuf || errno == ERANGE)
+	if (endp == linebuf || errno == ERANGE || val < 0 || val > 15)
 		return -1;
 
 	return (int)val;

@@ -1039,8 +1044,9 @@ static int set_epb_sysfs(int cpu, int val)
 	if (ret <= 0)
 		return -1;
 
+	errno = 0;
 	val = strtol(linebuf, &endp, 0);
-	if (endp == linebuf || errno == ERANGE)
+	if (endp == linebuf || errno == ERANGE || val < 0 || val > 15)
 		return -1;
 
 	return (int)val;

Signature · 1 line

2.45.2

Andy Shevchenko

2026-10-21 15:56 UTC #7 lore raw

Re: [PATCH 04/18] x86_energy_perf_policy: Cleanup: Harden with strtol()

Quoted 4 lines · Len Brown

Replace atoi() with strtol() and proper error checking when parsing the --hwp-use-pkg command-line argument value, and other spots.
Also add explicit check that value is in valid range [0, 1].

...

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c int parse_optarg_string(char *s) · line 360

Quoted 5 lines

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +2 -2

+	errno = 0;
 	i = strtol(s, &endptr, 0);
-	if (s == endptr) {
-		fprintf(stderr, "no digits in \"%s\"\n", s);
+	if (endptr == s || errno == ERANGE || i < 0 || i > 256) {

Do I read it wrongly or should it be i > 255 (as checked below)?

Quoted 12 lines

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +1 -7

+		warnx("bad value \"%s\"\n", s);
 		usage();
 	}
-	if (i == LONG_MIN || i == LONG_MAX)
-		errx(-1, "%s", s);
 
-	if (i > 0xFF)
-		errx(-1, "%d (0x%x) must be < 256", i, i);
-
-	if (i < 0)
-		errx(-1, "%d (0x%x) must be >= 0", i, i);
 	return i;

...

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c void cmdline(int argc, char **argv) · line 716

Quoted 7 lines

void cmdline(int argc, char **argv)
tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +2 -1
 		case 'u':
 			update_hwp_use_pkg++;
-			if (atoi(optarg) == 0)
+			char *endp;
+			long val;

Doesn't this require {} scope in the switch-case?

Quoted 9 lines

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +6 -0

+			errno = 0;
+			val = strtol(optarg, &endp, 10);
+			if (endp == optarg || errno == ERANGE || val < 0 || val > 1)
+				errx(1, "--hwp-use-pkg: must be 0 or 1");
+
+			if (val == 0)
 				req_update.hwp_use_pkg = 0;
 			else
 				req_update.hwp_use_pkg = 1;

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-24 17:53 UTC #8 lore raw

Re: [PATCH 04/18] x86_energy_perf_policy: Cleanup: Harden with strtol()

Quoted 4 lines · Andy Shevchenko

Quoted 2 lines
+ if (endptr == s || errno == ERANGE || i < 0 || i > 256) {
Do I read it wrongly or should it be i > 255 (as checked below)?

yes, 255, thx.

Quoted 10 lines

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c void cmdline(int argc, char **argv) · line 716
Quoted 8 lines
void cmdline(int argc, char **argv)
              case 'u':
                      update_hwp_use_pkg++;
tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +2 -1
-                     if (atoi(optarg) == 0)
+                     char *endp;
+                     long val;
Doesn't this require {} scope in the switch-case?

added. thx.

(C99 lets you get away with this, but braces clarify scope.)

Len Brown, Intel Open Source Technology Center

Andy Shevchenko

2026-10-25 21:33 UTC #9 lore raw

Re: [PATCH 04/18] x86_energy_perf_policy: Cleanup: Harden with strtol()

Quoted 2 lines · Len Brown

On Wed, Oct 21, 2026 at 11:56 AM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 8 lines

Quoted 7 lines

void cmdline(int argc, char **argv)

              case 'u':
                      update_hwp_use_pkg++;

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +2 -1

-                     if (atoi(optarg) == 0)
+                     char *endp;
+                     long val;

Quoted 5 lines

Quoted 1 line
Doesn't this require {} scope in the switch-case?
added. thx.
(C99 lets you get away with this, but braces clarify scope.)

Right, as well as with the definitions in the middle of the code. Maybe define the variables on top? (I admit I haven't checked the style for this in use in this tool, but if it's already uses the C99 approach, I'm fine. Otherwise I'm all for consistent style.)

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #10 lore raw

PATCH 5/18 x86_energy_perf_policy: Cleanup: Use snprintf() for platform-profile

In reply to Len Brown

From: Len Brown <[email protected]>

Better hygiene to use snprintf() instead of strcpy().

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +1 -1

index 98e5aa9aec51..8ce47d46eedf 100644
@@ -675,7 +675,7 @@ void cmdline(int argc, char **argv)
 				errx(1, "--platform-profile: value too long");
 			if (!platform_profile_access(W_OK))
 				errx(1, "Can not update platform-profile in '%s'", PATH_PLATFORM_PROFILE);
-			strcpy(platform_profile, optarg);
+			snprintf(platform_profile, sizeof(platform_profile), "%s", optarg);
 			update_platform_profile = 1;
 			break;
 		case 'm':

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 5/18

2026-10-21 15:57 UTC #11 lore raw

Re: [PATCH 05/18] x86_energy_perf_policy: Cleanup: Use snprintf() for platform-profile

Quoted 1 line · Len Brown

Better hygiene to use snprintf() instead of strcpy().

Yeah, since we don't have strscpy() 2-arg available in the user space tools.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #12 lore raw

PATCH 6/18 x86_energy_perf_policy: Fix optarg keyword matching

In reply to Len Brown

From: Len Brown <[email protected]>

Replace strncmp() with strcmp() for exact string matching in parse_optarg_string(). Exact keyword matching is safer and clearer. This prevents accidental matches with longer strings
like "balance-power-extra".

Fixes 4beec1d75196 ("tools/power x86_energy_perf_policy: support HWP.EPP")

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../x86_energy_perf_policy/x86_energy_perf_policy.c  | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +6 -6

index 8ce47d46eedf..5afff02bf9e7 100644
@@ -337,22 +337,22 @@ int parse_optarg_string(char *s)
 	int i;
 	char *endptr;
 
-	if (!strncmp(s, "default", 7))
+	if (strcmp(s, "default") == 0)
 		return OPTARG_NORMAL;
 
-	if (!strncmp(s, "normal", 6))
+	if (strcmp(s, "normal") == 0)
 		return OPTARG_NORMAL;
 
-	if (!strncmp(s, "power", 9))
+	if (strcmp(s, "power") == 0)
 		return OPTARG_POWER;
 
-	if (!strncmp(s, "balance-power", 17))
+	if (strcmp(s, "balance-power") == 0)
 		return OPTARG_BALANCE_POWER;
 
-	if (!strncmp(s, "balance-performance", 19))
+	if (strcmp(s, "balance-performance") == 0)
 		return OPTARG_BALANCE_PERFORMANCE;
 
-	if (!strncmp(s, "performance", 11))
+	if (strcmp(s, "performance") == 0)
 		return OPTARG_PERFORMANCE;
 
 	errno = 0;

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 6/18

2026-10-21 15:58 UTC #13 lore raw

Re: [PATCH 06/18] x86_energy_perf_policy: Fix optarg keyword matching

Quoted 4 lines · Len Brown

Replace strncmp() with strcmp() for exact string matching in parse_optarg_string(). Exact keyword matching is safer and clearer. This prevents accidental matches with longer strings
like "balance-power-extra".

Right.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #14 lore raw

PATCH 7/18 x86_energy_perf_policy: Cleanup: Fix potential ratio overflow

In reply to Len Brown

From: Len Brown <[email protected]>

Prevent integer overflow in ratio_2_msr_perf() by using 64-bit arithmetic for the intermediate multiplication calculation.

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +3 -1

index 5afff02bf9e7..6b0079a0b36c 100644
@@ -141,11 +141,13 @@ void usage(void)
 int ratio_2_msr_perf(int ratio)
 {
 	int msr_perf;
+	unsigned long long temp;
 
 	if (!bdx_highest_ratio)
 		return ratio;
 
-	msr_perf = ratio * 255 / bdx_highest_ratio;
+	temp = (unsigned long long)ratio * 255;
+	msr_perf = temp / bdx_highest_ratio;
 
 	if (debug)
 		fprintf(stderr, "%d = ratio_to_msr_perf(%d)\n", msr_perf, ratio);

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 7/18

2026-10-21 16:06 UTC #15 lore raw

Re: [PATCH 07/18] x86_energy_perf_policy: Cleanup: Fix potential ratio overflow

Quoted 2 lines · Len Brown

Prevent integer overflow in ratio_2_msr_perf() by using 64-bit arithmetic for the intermediate multiplication calculation.

I assume this even compiles on 32-bit due to __udivsi3 present in the compiler libraries.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #16 lore raw

PATCH 8/18 x86_energy_perf_policy: Bound package iteration

In reply to Len Brown

From: Len Brown <[email protected]>

Add explicit bounds checking against MAX_PACKAGES in for_packages() to prevent potential out-of-bounds bit shift operations.

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +1 -1

index 6b0079a0b36c..5c54cf9e0fcf 100644
@@ -532,7 +532,7 @@ void for_packages(unsigned long long pkg_set, int (func) (int))
 {
 	int pkg_num;
 
-	for (pkg_num = 0; pkg_num <= max_pkg_num; ++pkg_num) {
+	for (pkg_num = 0; pkg_num <= max_pkg_num && pkg_num < MAX_PACKAGES; ++pkg_num) {
 		if (pkg_set & (1UL << pkg_num))
 			func(pkg_num);
 	}

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 8/18

2026-10-21 16:08 UTC #17 lore raw

Re: [PATCH 08/18] x86_energy_perf_policy: Bound package iteration

Quoted 2 lines · Len Brown

Add explicit bounds checking against MAX_PACKAGES in for_packages() to prevent potential out-of-bounds bit shift operations.

Can be done differently, but I believe it's not a hot-path or something that needs (micro)optimisations.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #18 lore raw

PATCH 9/18 x86_energy_perf_policy: Cleanup: Define HWP window max

In reply to Len Brown

From: Len Brown <[email protected]>

Replace all occurrences of the hardcoded magic number 1270000000 with a named symbolic constant HWP_WINDOW_MAX_USEC to improve code readability and maintainability.

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../x86/x86_energy_perf_policy/x86_energy_perf_policy.c    | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +5 -2

index 5c54cf9e0fcf..1a0c23cd4cc4 100644
@@ -34,6 +34,9 @@
 #define	OPTARG_BALANCE_PERFORMANCE	(INT_MAX - 4)
 #define	OPTARG_PERFORMANCE		(INT_MAX - 5)
 
+/* Maximum HWP activity window duration in microseconds */
+#define HWP_WINDOW_MAX_USEC 1270000000U
+
 struct msr_hwp_cap {
 	unsigned char highest;
 	unsigned char guaranteed;

@@ -273,8 +276,8 @@ int parse_cmdline_hwp_window(int i)
 	case OPTARG_PERFORMANCE:
 		return 0;
 	}
-	if (i < 0 || i > 1270000000) {
-		fprintf(stderr, "--hwp-window: 0 for auto; 1 - 1270000000 usec for window duration\n");
+	if (i < 0 || i > HWP_WINDOW_MAX_USEC) {
+		fprintf(stderr, "--hwp-window: 0 for auto; 1 - %u usec for window duration\n", HWP_WINDOW_MAX_USEC);
 		usage();
 	}
 	for (exponent = 0;; ++exponent) {

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 9/18

2026-10-21 16:11 UTC #19 lore raw

Re: [PATCH 09/18] x86_energy_perf_policy: Cleanup: Define HWP window max

Quoted 3 lines · Len Brown

Replace all occurrences of the hardcoded magic number 1270000000 with a named symbolic constant HWP_WINDOW_MAX_USEC to improve code readability and maintainability.

Yep, good one!

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #20 lore raw

PATCH 10/18 x86_energy_perf_policy: Cleanup: Define BCLK_MHZ

In reply to Len Brown

From: Len Brown <[email protected]>

Replace the hardcoded value 100 with a named symbolic constant BCLK_MHZ in ratio_2_sysfs_khz() to document the assumed bus clock frequency.

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../x86/x86_energy_perf_policy/x86_energy_perf_policy.c      | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +4 -1

index 1a0c23cd4cc4..cfd9d8df6ec1 100644
@@ -37,6 +37,9 @@
 /* Maximum HWP activity window duration in microseconds */
 #define HWP_WINDOW_MAX_USEC 1270000000U
 
+/* Nominal bus clock frequency in MHz - assumed for all HWP systems */
+#define BCLK_MHZ 100
+
 struct msr_hwp_cap {
 	unsigned char highest;
 	unsigned char guaranteed;

@@ -1161,7 +1164,7 @@ int print_pkg_msrs(int pkg)
  */
 int ratio_2_sysfs_khz(int ratio)
 {
-	int bclk_khz = 100 * 1000;	/* 100,000 KHz = 100 MHz */
+	int bclk_khz = BCLK_MHZ * 1000; /* Convert MHz to KHz */
 
 	return ratio * bclk_khz;
 }

Signature · 1 line

2.45.2

Andy Shevchenko

2026-10-21 16:16 UTC #21 lore raw

Re: [PATCH 10/18] x86_energy_perf_policy: Cleanup: Define BCLK_MHZ

Quoted 2 lines · Len Brown

Replace the hardcoded value 100 with a named symbolic constant BCLK_MHZ in ratio_2_sysfs_khz() to document the assumed bus clock frequency.

...

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c line 41

Quoted 2 lines

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +2 -0
+/* Nominal bus clock frequency in MHz - assumed for all HWP systems */
+#define BCLK_MHZ 100

Perhaps also

#define KHZ_PER_MHZ 1000

...

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c int print_pkg_msrs(int pkg) · line 1169

Quoted 5 lines

int ratio_2_sysfs_khz(int ratio)
{
tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +1 -1
-	int bclk_khz = 100 * 1000;	/* 100,000 KHz = 100 MHz */
+	int bclk_khz = BCLK_MHZ * 1000; /* Convert MHz to KHz */
 	return ratio * bclk_khz;

	return ratio * BCLK_MHZ * HZ_PER_MHZ;

Quoted 1 line

}

...

Also not sure why not define it directly in KHz?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-24 18:22 UTC #22 lore raw

Re: [PATCH 10/18] x86_energy_perf_policy: Cleanup: Define BCLK_MHZ

Quoted 1 line · Andy Shevchenko

#define KHZ_PER_MHZ 1000

sure.

Quoted 1 line

Also not sure why not define it directly in KHz?

The origin of this tool is that you can actually use it to manage HWP even if Linux has no cpufreq sub-system (cpufreq.off=1) And so its user interface took frequency in 100MHz ratio, to reflect the hardware units.

Then people wanted to use it with cpufreq enabled, and cpufreq has Khz in sysfs. Then we had to deal with not scribbling on MSRs behind sysfs/driver's back, else risk confusing a driver.
Then hybrid was invented and HWP.REQ was defined to be performance that is not necessarily 100MHz units -- and for that we need sysfs/intel_pstate to translate...

Len Brown, Intel Open Source Technology Center

Andy Shevchenko

2026-10-25 21:30 UTC #23 lore raw

Re: [PATCH 10/18] x86_energy_perf_policy: Cleanup: Define BCLK_MHZ

Quoted 14 lines · Len Brown

Quoted 1 line · Andy Shevchenko
#define KHZ_PER_MHZ 1000
sure.
Quoted 1 line
Also not sure why not define it directly in KHz?
The origin of this tool is that you can actually use it to manage HWP even if Linux has no cpufreq sub-system (cpufreq.off=1) And so its user interface took frequency in 100MHz ratio, to reflect the hardware units.

Quoted 6 lines

Then people wanted to use it with cpufreq enabled, and cpufreq has Khz in sysfs. Then we had to deal with not scribbling on MSRs behind sysfs/driver's back, else risk confusing a driver.
Then hybrid was invented and HWP.REQ was defined to be performance that is not necessarily 100MHz units -- and for that we need sysfs/intel_pstate to translate...

Oh, I see, true good legacy and history :-)

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #24 lore raw

PATCH 11/18 x86_energy_perf_policy: Cleanup: Check fseek() in VM path

In reply to Len Brown

From: Len Brown <[email protected]>

Add return value validation for fseek() and ensure consistent resource cleanup on all error paths in err_on_hypervisor().

Fixes 0936cdfbb527 ("tools/power x86_energy_perf_policy: Input/output error in a VM")

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../x86/x86_energy_perf_policy/x86_energy_perf_policy.c     | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +5 -1

index cfd9d8df6ec1..1343966b1f07 100644
@@ -797,7 +797,11 @@ void err_on_hypervisor(void)
 		err(1, "Failed to find 'flags' in /proc/cpuinfo");
 	}
 	rewind(cpuinfo);
-	fseek(cpuinfo, flags - buffer, SEEK_SET);
+	if (fseek(cpuinfo, flags - buffer, SEEK_SET) != 0) {
+		fclose(cpuinfo);
+		free(buffer);
+		err(1, "Failed to seek in /proc/cpuinfo");
+	}
 	if (!fgets(buffer, 4096, cpuinfo)) {
 		fclose(cpuinfo);
 		free(buffer);

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 11/18

2026-10-21 16:19 UTC #25 lore raw

Re: [PATCH 11/18] x86_energy_perf_policy: Cleanup: Check fseek() in VM path

Quoted 2 lines · Len Brown

Add return value validation for fseek() and ensure consistent resource cleanup on all error paths in err_on_hypervisor().

Makes sense.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #26 lore raw

PATCH 12/18 x86_energy_perf_policy: Document MSRHEADER

In reply to Len Brown

From: Len Brown <[email protected]>

Add comprehensive comments documenting the MSRHEADER macro usage and expected build system configuration.

The MSRHEADER macro is not defined in the source file and must be provided by the build system (typically as -DMSRHEADER=\"msr-index.h\" or similar). This can be confusing for developers building the code independently.

Adding documentation clarifies:
- That MSRHEADER is a macro expected to be defined externally
- What types of headers are typically used
- Example build flags showing how to define it

This prevents build failures and confusion when compiling without the proper build system context.

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../x86/x86_energy_perf_policy/x86_energy_perf_policy.c    | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +6 -1

index 1343966b1f07..fcfdb0075408 100644
@@ -9,7 +9,12 @@
  */
 
 #define _GNU_SOURCE
-#include MSRHEADER
+/*
+ * MSRHEADER macro should be defined by the build system to include the
+ * appropriate MSR definitions header file. Typical usage:
+ * gcc -DMSRHEADER=\"msr-index.h\" or similar platform-specific MSR header
+ */
+#include MSRHEADER  /* e.g., msr-index.h with MSR_* constant definitions */
 #include <stdio.h>
 #include <unistd.h>
 #include <sys/types.h>

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 12/18

2026-10-21 16:31 UTC #27 lore raw

Re: [PATCH 12/18] x86_energy_perf_policy: Document MSRHEADER

Quoted 8 lines · Len Brown

Add comprehensive comments documenting the MSRHEADER macro usage and expected build system configuration.
The MSRHEADER macro is not defined in the source file and must be provided by the build system (typically as -DMSRHEADER=\"msr-index.h\" or similar). This can be confusing for developers building the code independently.

Quoted 7 lines

Adding documentation clarifies:
- That MSRHEADER is a macro expected to be defined externally
- What types of headers are typically used
- Example build flags showing how to define it
This prevents build failures and confusion when compiling without the proper build system context.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #28 lore raw

PATCH 13/18 x86_energy_perf_policy: Cleanup: Drop unused signal.h

In reply to Len Brown

From: Len Brown <[email protected]>

Remove unused #include <signal.h>

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c | 1 -
 1 file changed, 1 deletion(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +0 -1

index fcfdb0075408..e26a28fc7edd 100644
@@ -25,7 +25,6 @@
 #include <getopt.h>
 #include <err.h>
 #include <fcntl.h>
-#include <signal.h>
 #include <sys/time.h>
 #include <limits.h>
 #include <stdlib.h>

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 13/18

2026-10-21 16:38 UTC #29 lore raw

Re: [PATCH 13/18] x86_energy_perf_policy: Cleanup: Drop unused signal.h

Quoted 1 line · Len Brown

Remove unused #include <signal.h>

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #30 lore raw

PATCH 14/18 x86_energy_perf_policy: Cleanup: Constify profile name arg

In reply to Len Brown

From: Len Brown <[email protected]>

Add const qualifier to the name parameter of
platform_profile_name_is() which is read-only.

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +1 -1

index e26a28fc7edd..ffdc5df27ef1 100644
@@ -582,7 +582,7 @@ static int platform_profile_access(int mode)
 	return 1;
 }
 
-static int platform_profile_name_is(char *name)
+static int platform_profile_name_is(const char *name)
 {
 	char buf[64];

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 14/18

2026-10-21 16:39 UTC #31 lore raw

Re: [PATCH 14/18] x86_energy_perf_policy: Cleanup: Constify profile name arg

Quoted 3 lines · Len Brown

Add const qualifier to the name parameter of
platform_profile_name_is() which is read-only.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #32 lore raw

PATCH 15/18 x86_energy_perf_policy: Cleanup: Constify HWP strings

In reply to Len Brown

From: Len Brown <[email protected]>

Add const qualifiers to the string parameters of parse_optarg_string(), print_hwp_request(), and print_hwp_request_pkg() which are read-only.

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../x86/x86_energy_perf_policy/x86_energy_perf_policy.c     | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +3 -3

index ffdc5df27ef1..70d889c3e0dc 100644
@@ -344,7 +344,7 @@ int parse_cmdline_turbo(int i)
 	return i;
 }
 
-int parse_optarg_string(char *s)
+int parse_optarg_string(const char *s)
 {
 	int i;
 	char *endptr;

@@ -965,7 +965,7 @@ void read_hwp_cap(int cpu, struct msr_hwp_cap *cap, unsigned int msr_offset)
 	cap->lowest = msr_perf_2_ratio(HWP_LOWEST_PERF(msr));
 }
 
-void print_hwp_request(int cpu, struct msr_hwp_request *h, char *str)
+void print_hwp_request(int cpu, struct msr_hwp_request *h, const char *str)
 {
 	if (cpu != -1)
 		printf("cpu%d: ", cpu);

@@ -977,7 +977,7 @@ void print_hwp_request(int cpu, struct msr_hwp_request *h, char *str)
 	       h->hwp_min, h->hwp_max, h->hwp_desired, h->hwp_epp, h->hwp_window, h->hwp_window & 0x7F, (h->hwp_window >> 7) & 0x7, h->hwp_use_pkg);
 }
 
-void print_hwp_request_pkg(int pkg, struct msr_hwp_request *h, char *str)
+void print_hwp_request_pkg(int pkg, struct msr_hwp_request *h, const char *str)
 {
 	printf("pkg%d: ", pkg);

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 15/18

2026-10-21 16:39 UTC #33 lore raw

Re: [PATCH 15/18] x86_energy_perf_policy: Cleanup: Constify HWP strings

Quoted 3 lines · Len Brown

Add const qualifiers to the string parameters of parse_optarg_string(), print_hwp_request(), and print_hwp_request_pkg() which are read-only.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #34 lore raw

PATCH 16/18 x86_energy_perf_policy: Fix use-after-free in err_on_hypervisor()

In reply to Len Brown

From: Len Brown <[email protected]>

Avoid testing a pointer into a freed buffer.

Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

Signed-off-by Len Brown <[email protected]>

---
 .../x86/x86_energy_perf_policy/x86_energy_perf_policy.c  | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +4 -5

index 70d889c3e0dc..f8ab9f4465ce 100644
@@ -782,10 +782,10 @@ void err_on_hypervisor(void)
 	/* On VMs /proc/cpuinfo contains a "flags" entry for hypervisor */
 	cpuinfo = fopen_or_die("/proc/cpuinfo", "r");
 
-	buffer = malloc(4096);
+	buffer = calloc(4096, 1);
 	if (!buffer) {
 		fclose(cpuinfo);
-		err(-ENOMEM, "buffer malloc fail");
+		err(-ENOMEM, "buffer calloc fail");
 	}
 
 	if (!fread(buffer, 1024, 1, cpuinfo)) {

@@ -814,11 +814,10 @@ void err_on_hypervisor(void)
 	fclose(cpuinfo);
 
 	hypervisor = strstr(buffer, "hypervisor");
-
-	free(buffer);
-
 	if (hypervisor)
 		err(-1, "not supported on this virtual machine");
+
+	free(buffer);
 }
 
 /*

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 16/18

2026-10-21 16:42 UTC #35 lore raw

Re: [PATCH 16/18] x86_energy_perf_policy: Fix use-after-free in err_on_hypervisor()

Quoted 1 line · Len Brown

Avoid testing a pointer into a freed buffer.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-19 16:52 UTC #36 lore raw

PATCH 17/18 x86_energy_perf_policy: Fix write_sysfs() return type

In reply to Len Brown

From: Amarjeet <[email protected]>

write_sysfs() returns an unsigned int even though it returns -1 when write() fails. This converts the error to UINT_MAX and can cause callers that treat a nonzero return value as success to miss the failure.

Use ssize_t to match write(), update the receiving variable accordingly, and explicitly require a positive result in sysfs_write_string().

Signed-off-by Amarjeet <[email protected]>

Signed-off-by Len Brown <[email protected]>

---
 .../x86/x86_energy_perf_policy/x86_energy_perf_policy.c   | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +4 -4

index f8ab9f4465ce..89850cbc6d90 100644
@@ -897,7 +897,7 @@ static unsigned int read_sysfs(const char *path, char *buf, size_t buflen)
 	return (unsigned int)numread;
 }
 
-static unsigned int write_sysfs(const char *path, char *buf, size_t buflen)
+static ssize_t write_sysfs(const char *path, char *buf, size_t buflen)
 {
 	ssize_t numwritten;
 	int fd;

@@ -916,7 +916,7 @@ static unsigned int write_sysfs(const char *path, char *buf, size_t buflen)
 
 	close(fd);
 
-	return (unsigned int)numwritten;
+	return numwritten;
 }
 
 static int sysfs_read_string(const char *path, char *buf, size_t buflen)

@@ -941,7 +941,7 @@ static int sysfs_write_string(const char *path, const char *buf)
 	len = snprintf(tmp, sizeof(tmp), "%s\n", buf);
 	if (len < 0 || len >= (int)sizeof(tmp))
 		return -1;
-	return write_sysfs(path, tmp, (size_t)len + 1) ? 0 : -1;
+	return write_sysfs(path, tmp, (size_t)len + 1) > 0 ? 0 : -1;
 }
 
 void print_hwp_cap(int cpu, struct msr_hwp_cap *cap, char *str)

@@ -1047,7 +1047,7 @@ static int set_epb_sysfs(int cpu, int val)
 	char path[SYSFS_PATH_MAX];
 	char linebuf[3];
 	char *endp;
-	int ret;
+	ssize_t ret;
 
 	if (!has_epb)
 		return -1;

Signature · 1 line

2.45.2

Andy Shevchenko

2026-10-21 16:45 UTC #37 lore raw

Re: [PATCH 17/18] x86_energy_perf_policy: Fix write_sysfs() return type

Quoted 6 lines · Len Brown

write_sysfs() returns an unsigned int even though it returns -1 when write() fails. This converts the error to UINT_MAX and can cause callers that treat a nonzero return value as success to miss the failure.
Use ssize_t to match write(), update the receiving variable accordingly, and explicitly require a positive result in sysfs_write_string().

...

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c static int sysfs_write_string(const char *path, const char *buf) · line 944

Quoted 7 lines

static int sysfs_write_string(const char *path, const char *buf)

tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +1 -1

 	len = snprintf(tmp, sizeof(tmp), "%s\n", buf);
 	if (len < 0 || len >= (int)sizeof(tmp))
 		return -1;
-	return write_sysfs(path, tmp, (size_t)len + 1) ? 0 : -1;
+	return write_sysfs(path, tmp, (size_t)len + 1) > 0 ? 0 : -1;

Wouldn't it be better (but I haven't checked callers of this) to have other way around?

	int ret;
	...
	ret = write_sysfs(path, tmp, (size_t)len + 1);
	if (ret < 0)
		return ret;

	return 0;

Quoted 1 line

}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Len Brown

author

2026-10-24 17:38 UTC #38 lore raw

Re: [PATCH 17/18] x86_energy_perf_policy: Fix write_sysfs() return type

Quoted 10 lines

Wouldn't it be better (but I haven't checked callers of this) to have other way around?
        int ret;
        ...
        ret = write_sysfs(path, tmp, (size_t)len + 1);
        if (ret < 0)
                return ret;

        return 0;

I too prefer plain code over ternary operators.

Your comment made me look closer at this patch and re-write it. Turns out there was a bad error path return value hiding in plain sight. And the callers don't care about num_written, they just care about success/fail -- so we'll return just that.

thanks,
Len Brown, Intel Open Source Technology Center

Len Brown

author

2026-10-19 16:52 UTC #39 lore raw

PATCH 18/18 MAINTAINERS: Add x86_energy_perf_policy

In reply to Len Brown

From: Len Brown <[email protected]>

This entry should help direct patches to linux-pm. (rather than to lkml)

Signed-off-by Len Brown <[email protected]>

---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

MAINTAINERS +9 -0

index 782389bd5cd3..3958e66188ec 100644
@@ -29577,6 +29577,15 @@ W:	https://x86-cpuid.org
 F:	arch/x86/include/asm/cpuid/leaf_types.h
 F:	tools/arch/x86/kcpuid/
 
+X86 ENERGY PERF POLICY UTILITY
+M:	Len Brown <[email protected]>
+L:	[email protected]
+S:	Supported
+Q:	https://patchwork.kernel.org/project/linux-pm/list/
+B:	https://bugzilla.kernel.org
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git x86_energy_perf_policy
+F:	tools/power/x86/x86_energy_perf_policy/
+
 X86 ENTRY CODE
 M:	Andy Lutomirski <[email protected]>
 L:	[email protected]

Signature · 1 line

2.45.2

Andy Shevchenko

Reviewed-by on v1 18/18

2026-10-21 16:46 UTC #40 lore raw

Re: [PATCH 18/18] MAINTAINERS: Add x86_energy_perf_policy

Quoted 2 lines · Len Brown

This entry should help direct patches to linux-pm. (rather than to lkml)

Indeed!

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2026-10-19 18:33 UTC #41 lore raw

Re: [PATCH 01/18] x86_energy_perf_policy: Create msr_path helper

In reply to Len Brown

Quoted 7 lines · Len Brown

Factor out code that creates msr_path[].
No functional change.
Suggested-by Andy Shevchenko <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
Assisted-by GitHub-Copilot:Claude-Sonnet-4.6

In July IIRC the kernel documentation was updated to make the

Assisted-by LLM

be a standard for the kernel contributions independently on the model(s) in use. Yeah, I only learnt like a month ago about this development.

Quoted 1 line

Signed-off-by Len Brown <[email protected]>

The rest I have reviewed (as the tag above suggests).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2026-10-20 09:35 UTC #42 lore raw

Re: [PATCH 01/18] x86_energy_perf_policy: Create msr_path helper

Quoted 1 line · Andy Shevchenko

On Mon, Oct 19, 2026 at 12:43:22PM -0400, Len Brown wrote:

...

Quoted 8 lines

Quoted 1 line
Assisted-by GitHub-Copilot:Claude-Sonnet-4.6
In July IIRC the kernel documentation was updated to make the
Assisted-by LLM
be a standard for the kernel contributions independently on the model(s) in use. Yeah, I only learnt like a month ago about this development.

Actually it gets a new development!
1e035297ffda ("docs: remove Assisted-by references") I believe as you also consider that the tag is redundant.

Signature · 2 lines

With Best Regards,
Andy Shevchenko