DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 21.08] power: refactor pstate sysfs handling
@ 2021-04-01 15:05 Anatoly Burakov
  2021-04-02  9:27 ` [dpdk-dev] [21.08 v2] " Anatoly Burakov
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Anatoly Burakov @ 2021-04-01 15:05 UTC (permalink / raw)
  To: dev; +Cc: anatoly.burakov

Currently, pstate sysfs handling code is a bit of an unmaintainable
mess, which has contributed to various errors leading to bugs. Refactor
the code in a way that makes it more maintainable and less error prone.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_power/meson.build            |   7 +
 lib/librte_power/power_pstate_cpufreq.c | 355 +++++++++++++-----------
 2 files changed, 193 insertions(+), 169 deletions(-)

diff --git a/lib/librte_power/meson.build b/lib/librte_power/meson.build
index 9a2dcbfc7a..fd408ffd4c 100644
--- a/lib/librte_power/meson.build
+++ b/lib/librte_power/meson.build
@@ -5,6 +5,13 @@ if not is_linux
 	build = false
 	reason = 'only supported on Linux'
 endif
+
+# we do some snprintf magic so silence format-nonliteral
+flag_nonliteral = '-Wno-format-nonliteral'
+if cc.has_argument(flag_nonliteral)
+	cflags += flag_nonliteral
+endif
+
 sources = files('rte_power.c', 'power_acpi_cpufreq.c',
 		'power_kvm_vm.c', 'guest_channel.c',
 		'rte_power_empty_poll.c',
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 3a5face4f0..db7856dadc 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -37,6 +37,13 @@
 		} \
 } while (0)
 
+#define FOPEN_OR_ERR_GOTO(f, label) do { \
+		if ((f) == NULL) { \
+			RTE_LOG(ERR, POWER, "File not opened\n"); \
+			goto label; \
+		} \
+} while (0)
+
 #define FOPS_OR_NULL_GOTO(ret, label) do { \
 		if ((ret) == NULL) { \
 			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
@@ -148,93 +155,147 @@ out:	close(fd);
 	return ret;
 }
 
+static int
+open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
+		FILE **f)
+{
+	char fullpath[PATH_MAX];
+	FILE *tmpf;
+
+	/* silenced -Wformat-nonliteral here */
+	snprintf(fullpath, sizeof(fullpath), template, core);
+	tmpf = fopen(fullpath, mode);
+	if (tmpf == NULL)
+		return -1;
+	*f = tmpf;
+
+	return 0;
+}
+
+static int
+read_core_sysfs_u32(FILE *f, uint32_t *val)
+{
+	char buf[BUFSIZ];
+	uint32_t fval;
+	char *s;
+
+	s = fgets(buf, sizeof(buf), f);
+	if (s == NULL)
+		return -1;
+
+	/* fgets puts null terminator in, but do this just in case */
+	buf[BUFSIZ - 1] = '\0';
+
+	/* strip off any terminating newlines */
+	if (strlen(buf))
+		strtok(buf, "\n");
+
+	fval = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
+
+	/* write the value */
+	*val = fval;
+
+	return 0;
+}
+
+static int
+read_core_sysfs_s(FILE *f, char *buf, unsigned int len)
+{
+	char *s;
+
+	s = fgets(buf, len, f);
+	if (s == NULL)
+		return -1;
+
+	/* fgets puts null terminator in, but do this just in case */
+	buf[len - 1] = '\0';
+
+	/* strip off any terminating newlines */
+	if (strlen(buf))
+		strtok(buf, "\n");
+
+	return 0;
+}
+
+static int
+write_core_sysfs_s(FILE *f, const char *str)
+{
+	int ret;
+
+	ret = fseek(f, 0, SEEK_SET);
+	if (ret != 0)
+		return -1;
+
+	ret = fputs(str, f);
+	if (ret != 0)
+		return -1;
+
+	/* flush the output */
+	ret = fflush(f);
+	if (ret != 0)
+		return -1;
+
+	return 0;
+}
+
 /**
  * It is to fopen the sys file for the future setting the lcore frequency.
  */
 static int
 power_init_for_setting_freq(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max, *f_base = NULL, *f_base_max;
-	char fullpath_min[PATH_MAX];
-	char fullpath_max[PATH_MAX];
-	char fullpath_base[PATH_MAX];
-	char fullpath_base_max[PATH_MAX];
-	char buf_base[BUFSIZ];
-	char *s_base;
-	char *s_base_max;
-	uint32_t base_ratio = 0;
-	uint32_t base_max_ratio = 0;
-	uint64_t max_non_turbo = 0;
-	int  ret_val = 0;
-
-	snprintf(fullpath_base_max,
-			sizeof(fullpath_base_max),
-			POWER_SYSFILE_BASE_MAX_FREQ,
-			pi->lcore_id);
-	f_base_max = fopen(fullpath_base_max, "r");
-	if (f_base_max != NULL) {
-		s_base_max = fgets(buf_base, sizeof(buf_base), f_base_max);
-		FOPS_OR_NULL_GOTO(s_base_max, out);
-
-		buf_base[BUFSIZ-1] = '\0';
-		if (strlen(buf_base))
-			/* Strip off terminating '\n' */
-			strtok(buf_base, "\n");
-
-		base_max_ratio =
-			strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
-				/ BUS_FREQ;
-		fclose(f_base_max);
-	}
-
-	snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
-			pi->lcore_id);
-	f_min = fopen(fullpath_min, "rw+");
-	FOPEN_OR_ERR_RET(f_min, -1);
-
-	snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ,
-			pi->lcore_id);
-	f_max = fopen(fullpath_max, "rw+");
-	if (f_max == NULL)
-		fclose(f_min);
-	FOPEN_OR_ERR_RET(f_max, -1);
-
-	pi->f_cur_min = f_min;
-	pi->f_cur_max = f_max;
-
-	snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ,
-			pi->lcore_id);
-
-	f_base = fopen(fullpath_base, "r");
-	FOPEN_OR_ERR_RET(f_base, -1);
-	if (f_base == NULL) {
-		/* No sysfs base_frequency, that's OK, continue without */
-		base_ratio = 0;
+	FILE *f_base = NULL, *f_base_max = NULL, *f_min = NULL, *f_max = NULL;
+	uint32_t base_ratio, base_max_ratio;
+	uint64_t max_non_turbo;
+	int ret;
+
+	/* open all files we expect to have open */
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+			&f_base_max);
+	FOPEN_OR_ERR_GOTO(f_base_max, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_MIN_FREQ, pi->lcore_id, "rw+",
+			&f_min);
+	FOPEN_OR_ERR_GOTO(f_min, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_MAX_FREQ, pi->lcore_id, "rw+",
+			&f_max);
+	FOPEN_OR_ERR_GOTO(f_max, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_BASE_FREQ, pi->lcore_id, "r",
+			&f_base);
+	/* base ratio file may not exist in some kernels, so no error check */
+
+	/* read base max ratio */
+	ret = read_core_sysfs_u32(f_base_max, &base_max_ratio);
+	FOPS_OR_ERR_GOTO(ret, err);
+
+	/* base ratio may not exist */
+	if (f_base != NULL) {
+		ret = read_core_sysfs_u32(f_base, &base_ratio);
+		FOPS_OR_ERR_GOTO(ret, err);
 	} else {
-		s_base = fgets(buf_base, sizeof(buf_base), f_base);
-		FOPS_OR_NULL_GOTO(s_base, out);
-
-		buf_base[BUFSIZ-1] = '\0';
-		if (strlen(buf_base))
-			/* Strip off terminating '\n' */
-			strtok(buf_base, "\n");
-
-		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
-				/ BUS_FREQ;
+		base_ratio = 0;
 	}
 
 	/* Add MSR read to detect turbo status */
+	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
+		goto err;
+	/* no errors after this point */
 
-	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0) {
-		ret_val = -1;
-		goto out;
-	}
+	/* convert ratios to bins */
+	base_max_ratio /= BUS_FREQ;
+	base_ratio /= BUS_FREQ;
+
+	/* assign file handles */
+	pi->f_cur_min = f_min;
+	pi->f_cur_max = f_max;
 
 	max_non_turbo = (max_non_turbo&NON_TURBO_MASK)>>NON_TURBO_OFFSET;
 
 	POWER_DEBUG_TRACE("no turbo perf %"PRIu64"\n", max_non_turbo);
 
-	pi->non_turbo_max_ratio = max_non_turbo;
+	pi->non_turbo_max_ratio = (uint32_t)max_non_turbo;
 
 	/*
 	 * If base_frequency is reported as greater than the maximum
@@ -260,7 +321,20 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 out:
 	if (f_base != NULL)
 		fclose(f_base);
-	return ret_val;
+	fclose(f_base_max);
+	/* f_min and f_max are stored, no need to close */
+	return 0;
+
+err:
+	if (f_base != NULL)
+		fclose(f_base);
+	if (f_base_max != NULL)
+		fclose(f_base_max);
+	if (f_min != NULL)
+		fclose(f_min);
+	if (f_max != NULL)
+		fclose(f_max);
+	return -1;
 }
 
 static int
@@ -365,22 +439,16 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
 static int
 power_set_governor_performance(struct pstate_power_info *pi)
 {
-	FILE *f;
+	FILE *f_governor = NULL;
 	int ret = -1;
 	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
 
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+			&f_governor);
+	FOPEN_OR_ERR_GOTO(f_governor, out);
 
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
-	/* Strip off terminating '\n' */
-	strtok(buf, "\n");
+	ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	/* Save the original governor */
 	rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
@@ -395,21 +463,15 @@ power_set_governor_performance(struct pstate_power_info *pi)
 	}
 
 	/* Write 'performance' to the governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(POWER_GOVERNOR_PERF, f);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	/* We need to flush to see if the fputs succeeds */
-	val = fflush(f);
-	FOPS_OR_ERR_GOTO(val, out);
+	ret = write_core_sysfs_s(f_governor, POWER_GOVERNOR_PERF);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	ret = 0;
 	RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
 			"set to performance successfully\n", pi->lcore_id);
 out:
-	fclose(f);
+	if (f_governor != NULL)
+		fclose(f_governor);
 
 	return ret;
 }
@@ -421,20 +483,16 @@ power_set_governor_performance(struct pstate_power_info *pi)
 static int
 power_set_governor_original(struct pstate_power_info *pi)
 {
-	FILE *f;
+	FILE *f_governor = NULL;
 	int ret = -1;
 	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
 
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+			&f_governor);
+	FOPEN_OR_ERR_GOTO(f_governor, out);
 
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
+	ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	/* Check if the governor to be set is the same as current */
 	if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
@@ -446,19 +504,16 @@ power_set_governor_original(struct pstate_power_info *pi)
 	}
 
 	/* Write back the original governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(pi->governor_ori, f);
-	FOPS_OR_ERR_GOTO(val, out);
+	ret = write_core_sysfs_s(f_governor, pi->governor_ori);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	ret = 0;
 	RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
 			"has been set back to %s successfully\n",
 			pi->lcore_id, pi->governor_ori);
 out:
-	fclose(f);
-
+	if (f_governor != NULL)
+		fclose(f_governor);
 	return ret;
 }
 
@@ -469,51 +524,26 @@ power_set_governor_original(struct pstate_power_info *pi)
 static int
 power_get_available_freqs(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max;
+	FILE *f_min = NULL, *f_max = NULL;
 	int ret = -1;
-	char *p_min, *p_max;
-	char buf_min[BUFSIZ];
-	char buf_max[BUFSIZ];
-	char fullpath_min[PATH_MAX];
-	char fullpath_max[PATH_MAX];
-	char *s_min, *s_max;
 	uint32_t sys_min_freq = 0, sys_max_freq = 0, base_max_freq = 0;
 	uint32_t i, num_freqs = 0;
 
-	snprintf(fullpath_max, sizeof(fullpath_max),
-			POWER_SYSFILE_BASE_MAX_FREQ,
-			pi->lcore_id);
-	snprintf(fullpath_min, sizeof(fullpath_min),
-			POWER_SYSFILE_BASE_MIN_FREQ,
-			pi->lcore_id);
+	/* open all files */
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+			&f_max);
+	FOPEN_OR_ERR_GOTO(f_max, out);
 
-	f_min = fopen(fullpath_min, "r");
-	FOPEN_OR_ERR_RET(f_min, ret);
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MIN_FREQ, pi->lcore_id, "r",
+			     &f_min);
+	FOPEN_OR_ERR_GOTO(f_max, out);
 
-	f_max = fopen(fullpath_max, "r");
-	if (f_max == NULL)
-		fclose(f_min);
+	/* read base ratios */
+	ret = read_core_sysfs_u32(f_max, &sys_max_freq);
+	FOPS_OR_ERR_GOTO(ret, out);
 
-	FOPEN_OR_ERR_RET(f_max, ret);
-
-	s_min = fgets(buf_min, sizeof(buf_min), f_min);
-	FOPS_OR_NULL_GOTO(s_min, out);
-
-	s_max = fgets(buf_max, sizeof(buf_max), f_max);
-	FOPS_OR_NULL_GOTO(s_max, out);
-
-
-	/* Strip the line break if there is */
-	p_min = strchr(buf_min, '\n');
-	if (p_min != NULL)
-		*p_min = 0;
-
-	p_max = strchr(buf_max, '\n');
-	if (p_max != NULL)
-		*p_max = 0;
-
-	sys_min_freq = strtoul(buf_min, &p_min, POWER_CONVERT_TO_DECIMAL);
-	sys_max_freq = strtoul(buf_max, &p_max, POWER_CONVERT_TO_DECIMAL);
+	ret = read_core_sysfs_u32(f_min, &sys_min_freq);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	if (sys_max_freq < sys_min_freq)
 		goto out;
@@ -572,27 +602,14 @@ power_get_cur_idx(struct pstate_power_info *pi)
 {
 	FILE *f_cur;
 	int ret = -1;
-	char *p_cur;
-	char buf_cur[BUFSIZ];
-	char fullpath_cur[PATH_MAX];
-	char *s_cur;
 	uint32_t sys_cur_freq = 0;
 	unsigned int i;
 
-	snprintf(fullpath_cur, sizeof(fullpath_cur),
-			POWER_SYSFILE_CUR_FREQ,
-			pi->lcore_id);
-	f_cur = fopen(fullpath_cur, "r");
-	FOPEN_OR_ERR_RET(f_cur, ret);
+	open_core_sysfs_file(POWER_SYSFILE_CUR_FREQ, pi->lcore_id, "r", &f_cur);
+	FOPEN_OR_ERR_GOTO(f_cur, fail);
 
-	/* initialize the cur_idx to matching current frequency freq index */
-	s_cur = fgets(buf_cur, sizeof(buf_cur), f_cur);
-	FOPS_OR_NULL_GOTO(s_cur, fail);
-
-	p_cur = strchr(buf_cur, '\n');
-	if (p_cur != NULL)
-		*p_cur = 0;
-	sys_cur_freq = strtoul(buf_cur, &p_cur, POWER_CONVERT_TO_DECIMAL);
+	ret = read_core_sysfs_u32(f_cur, &sys_cur_freq);
+	FOPS_OR_ERR_GOTO(ret, fail);
 
 	/* convert the frequency to nearest 100000 value
 	 * Ex: if sys_cur_freq=1396789 then freq_conv=1400000
@@ -611,10 +628,10 @@ power_get_cur_idx(struct pstate_power_info *pi)
 		}
 	}
 
-	fclose(f_cur);
-	return 0;
+	ret = 0;
 fail:
-	fclose(f_cur);
+	if (f_cur != NULL)
+		fclose(f_cur);
 	return ret;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [21.08 v2] power: refactor pstate sysfs handling
  2021-04-01 15:05 [dpdk-dev] [PATCH 21.08] power: refactor pstate sysfs handling Anatoly Burakov
@ 2021-04-02  9:27 ` Anatoly Burakov
  2021-04-02 17:45 ` [dpdk-dev] [PATCH 21.08] " Stephen Hemminger
  2021-04-02 17:46 ` Stephen Hemminger
  2 siblings, 0 replies; 12+ messages in thread
From: Anatoly Burakov @ 2021-04-02  9:27 UTC (permalink / raw)
  To: dev

Currently, pstate sysfs handling code is a bit of an unmaintainable
mess, which has contributed to various errors leading to bugs. Refactor
the code in a way that makes it more maintainable and less error prone.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_power/meson.build            |   7 +
 lib/librte_power/power_pstate_cpufreq.c | 355 +++++++++++++-----------
 2 files changed, 193 insertions(+), 169 deletions(-)

diff --git a/lib/librte_power/meson.build b/lib/librte_power/meson.build
index 9a2dcbfc7a..fd408ffd4c 100644
--- a/lib/librte_power/meson.build
+++ b/lib/librte_power/meson.build
@@ -5,6 +5,13 @@ if not is_linux
 	build = false
 	reason = 'only supported on Linux'
 endif
+
+# we do some snprintf magic so silence format-nonliteral
+flag_nonliteral = '-Wno-format-nonliteral'
+if cc.has_argument(flag_nonliteral)
+	cflags += flag_nonliteral
+endif
+
 sources = files('rte_power.c', 'power_acpi_cpufreq.c',
 		'power_kvm_vm.c', 'guest_channel.c',
 		'rte_power_empty_poll.c',
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 1cb0e4d917..db7856dadc 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -37,6 +37,13 @@
 		} \
 } while (0)
 
+#define FOPEN_OR_ERR_GOTO(f, label) do { \
+		if ((f) == NULL) { \
+			RTE_LOG(ERR, POWER, "File not opened\n"); \
+			goto label; \
+		} \
+} while (0)
+
 #define FOPS_OR_NULL_GOTO(ret, label) do { \
 		if ((ret) == NULL) { \
 			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
@@ -148,93 +155,147 @@ out:	close(fd);
 	return ret;
 }
 
+static int
+open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
+		FILE **f)
+{
+	char fullpath[PATH_MAX];
+	FILE *tmpf;
+
+	/* silenced -Wformat-nonliteral here */
+	snprintf(fullpath, sizeof(fullpath), template, core);
+	tmpf = fopen(fullpath, mode);
+	if (tmpf == NULL)
+		return -1;
+	*f = tmpf;
+
+	return 0;
+}
+
+static int
+read_core_sysfs_u32(FILE *f, uint32_t *val)
+{
+	char buf[BUFSIZ];
+	uint32_t fval;
+	char *s;
+
+	s = fgets(buf, sizeof(buf), f);
+	if (s == NULL)
+		return -1;
+
+	/* fgets puts null terminator in, but do this just in case */
+	buf[BUFSIZ - 1] = '\0';
+
+	/* strip off any terminating newlines */
+	if (strlen(buf))
+		strtok(buf, "\n");
+
+	fval = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
+
+	/* write the value */
+	*val = fval;
+
+	return 0;
+}
+
+static int
+read_core_sysfs_s(FILE *f, char *buf, unsigned int len)
+{
+	char *s;
+
+	s = fgets(buf, len, f);
+	if (s == NULL)
+		return -1;
+
+	/* fgets puts null terminator in, but do this just in case */
+	buf[len - 1] = '\0';
+
+	/* strip off any terminating newlines */
+	if (strlen(buf))
+		strtok(buf, "\n");
+
+	return 0;
+}
+
+static int
+write_core_sysfs_s(FILE *f, const char *str)
+{
+	int ret;
+
+	ret = fseek(f, 0, SEEK_SET);
+	if (ret != 0)
+		return -1;
+
+	ret = fputs(str, f);
+	if (ret != 0)
+		return -1;
+
+	/* flush the output */
+	ret = fflush(f);
+	if (ret != 0)
+		return -1;
+
+	return 0;
+}
+
 /**
  * It is to fopen the sys file for the future setting the lcore frequency.
  */
 static int
 power_init_for_setting_freq(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max, *f_base = NULL, *f_base_max;
-	char fullpath_min[PATH_MAX];
-	char fullpath_max[PATH_MAX];
-	char fullpath_base[PATH_MAX];
-	char fullpath_base_max[PATH_MAX];
-	char buf_base[BUFSIZ];
-	char *s_base;
-	char *s_base_max;
-	uint32_t base_ratio = 0;
-	uint32_t base_max_ratio = 0;
-	uint64_t max_non_turbo = 0;
-	int  ret_val = 0;
-
-	snprintf(fullpath_base_max,
-			sizeof(fullpath_base_max),
-			POWER_SYSFILE_BASE_MAX_FREQ,
-			pi->lcore_id);
-	f_base_max = fopen(fullpath_base_max, "r");
-	FOPEN_OR_ERR_RET(f_base_max, -1);
-	if (f_base_max != NULL) {
-		s_base_max = fgets(buf_base, sizeof(buf_base), f_base_max);
-		FOPS_OR_NULL_GOTO(s_base_max, out);
-
-		buf_base[BUFSIZ-1] = '\0';
-		if (strlen(buf_base))
-			/* Strip off terminating '\n' */
-			strtok(buf_base, "\n");
-
-		base_max_ratio =
-			strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
-				/ BUS_FREQ;
-	}
-
-	snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
-			pi->lcore_id);
-	f_min = fopen(fullpath_min, "rw+");
-	FOPEN_OR_ERR_RET(f_min, -1);
-
-	snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ,
-			pi->lcore_id);
-	f_max = fopen(fullpath_max, "rw+");
-	if (f_max == NULL)
-		fclose(f_min);
-	FOPEN_OR_ERR_RET(f_max, -1);
-
-	pi->f_cur_min = f_min;
-	pi->f_cur_max = f_max;
-
-	snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ,
-			pi->lcore_id);
-
-	f_base = fopen(fullpath_base, "r");
-	if (f_base == NULL) {
-		/* No sysfs base_frequency, that's OK, continue without */
-		base_ratio = 0;
+	FILE *f_base = NULL, *f_base_max = NULL, *f_min = NULL, *f_max = NULL;
+	uint32_t base_ratio, base_max_ratio;
+	uint64_t max_non_turbo;
+	int ret;
+
+	/* open all files we expect to have open */
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+			&f_base_max);
+	FOPEN_OR_ERR_GOTO(f_base_max, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_MIN_FREQ, pi->lcore_id, "rw+",
+			&f_min);
+	FOPEN_OR_ERR_GOTO(f_min, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_MAX_FREQ, pi->lcore_id, "rw+",
+			&f_max);
+	FOPEN_OR_ERR_GOTO(f_max, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_BASE_FREQ, pi->lcore_id, "r",
+			&f_base);
+	/* base ratio file may not exist in some kernels, so no error check */
+
+	/* read base max ratio */
+	ret = read_core_sysfs_u32(f_base_max, &base_max_ratio);
+	FOPS_OR_ERR_GOTO(ret, err);
+
+	/* base ratio may not exist */
+	if (f_base != NULL) {
+		ret = read_core_sysfs_u32(f_base, &base_ratio);
+		FOPS_OR_ERR_GOTO(ret, err);
 	} else {
-		s_base = fgets(buf_base, sizeof(buf_base), f_base);
-		FOPS_OR_NULL_GOTO(s_base, out);
-
-		buf_base[BUFSIZ-1] = '\0';
-		if (strlen(buf_base))
-			/* Strip off terminating '\n' */
-			strtok(buf_base, "\n");
-
-		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
-				/ BUS_FREQ;
-		fclose(f_base);
+		base_ratio = 0;
 	}
 
 	/* Add MSR read to detect turbo status */
+	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
+		goto err;
+	/* no errors after this point */
 
-	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0) {
-		ret_val = -1;
-		goto out;
-	}
+	/* convert ratios to bins */
+	base_max_ratio /= BUS_FREQ;
+	base_ratio /= BUS_FREQ;
+
+	/* assign file handles */
+	pi->f_cur_min = f_min;
+	pi->f_cur_max = f_max;
 
 	max_non_turbo = (max_non_turbo&NON_TURBO_MASK)>>NON_TURBO_OFFSET;
 
 	POWER_DEBUG_TRACE("no turbo perf %"PRIu64"\n", max_non_turbo);
 
-	pi->non_turbo_max_ratio = max_non_turbo;
+	pi->non_turbo_max_ratio = (uint32_t)max_non_turbo;
 
 	/*
 	 * If base_frequency is reported as greater than the maximum
@@ -260,7 +321,20 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 out:
 	if (f_base != NULL)
 		fclose(f_base);
-	return ret_val;
+	fclose(f_base_max);
+	/* f_min and f_max are stored, no need to close */
+	return 0;
+
+err:
+	if (f_base != NULL)
+		fclose(f_base);
+	if (f_base_max != NULL)
+		fclose(f_base_max);
+	if (f_min != NULL)
+		fclose(f_min);
+	if (f_max != NULL)
+		fclose(f_max);
+	return -1;
 }
 
 static int
@@ -365,22 +439,16 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
 static int
 power_set_governor_performance(struct pstate_power_info *pi)
 {
-	FILE *f;
+	FILE *f_governor = NULL;
 	int ret = -1;
 	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
 
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+			&f_governor);
+	FOPEN_OR_ERR_GOTO(f_governor, out);
 
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
-	/* Strip off terminating '\n' */
-	strtok(buf, "\n");
+	ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	/* Save the original governor */
 	rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
@@ -395,21 +463,15 @@ power_set_governor_performance(struct pstate_power_info *pi)
 	}
 
 	/* Write 'performance' to the governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(POWER_GOVERNOR_PERF, f);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	/* We need to flush to see if the fputs succeeds */
-	val = fflush(f);
-	FOPS_OR_ERR_GOTO(val, out);
+	ret = write_core_sysfs_s(f_governor, POWER_GOVERNOR_PERF);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	ret = 0;
 	RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
 			"set to performance successfully\n", pi->lcore_id);
 out:
-	fclose(f);
+	if (f_governor != NULL)
+		fclose(f_governor);
 
 	return ret;
 }
@@ -421,20 +483,16 @@ power_set_governor_performance(struct pstate_power_info *pi)
 static int
 power_set_governor_original(struct pstate_power_info *pi)
 {
-	FILE *f;
+	FILE *f_governor = NULL;
 	int ret = -1;
 	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
 
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+			&f_governor);
+	FOPEN_OR_ERR_GOTO(f_governor, out);
 
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
+	ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	/* Check if the governor to be set is the same as current */
 	if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
@@ -446,19 +504,16 @@ power_set_governor_original(struct pstate_power_info *pi)
 	}
 
 	/* Write back the original governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(pi->governor_ori, f);
-	FOPS_OR_ERR_GOTO(val, out);
+	ret = write_core_sysfs_s(f_governor, pi->governor_ori);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	ret = 0;
 	RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
 			"has been set back to %s successfully\n",
 			pi->lcore_id, pi->governor_ori);
 out:
-	fclose(f);
-
+	if (f_governor != NULL)
+		fclose(f_governor);
 	return ret;
 }
 
@@ -469,51 +524,26 @@ power_set_governor_original(struct pstate_power_info *pi)
 static int
 power_get_available_freqs(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max;
+	FILE *f_min = NULL, *f_max = NULL;
 	int ret = -1;
-	char *p_min, *p_max;
-	char buf_min[BUFSIZ];
-	char buf_max[BUFSIZ];
-	char fullpath_min[PATH_MAX];
-	char fullpath_max[PATH_MAX];
-	char *s_min, *s_max;
 	uint32_t sys_min_freq = 0, sys_max_freq = 0, base_max_freq = 0;
 	uint32_t i, num_freqs = 0;
 
-	snprintf(fullpath_max, sizeof(fullpath_max),
-			POWER_SYSFILE_BASE_MAX_FREQ,
-			pi->lcore_id);
-	snprintf(fullpath_min, sizeof(fullpath_min),
-			POWER_SYSFILE_BASE_MIN_FREQ,
-			pi->lcore_id);
+	/* open all files */
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+			&f_max);
+	FOPEN_OR_ERR_GOTO(f_max, out);
 
-	f_min = fopen(fullpath_min, "r");
-	FOPEN_OR_ERR_RET(f_min, ret);
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MIN_FREQ, pi->lcore_id, "r",
+			     &f_min);
+	FOPEN_OR_ERR_GOTO(f_max, out);
 
-	f_max = fopen(fullpath_max, "r");
-	if (f_max == NULL)
-		fclose(f_min);
+	/* read base ratios */
+	ret = read_core_sysfs_u32(f_max, &sys_max_freq);
+	FOPS_OR_ERR_GOTO(ret, out);
 
-	FOPEN_OR_ERR_RET(f_max, ret);
-
-	s_min = fgets(buf_min, sizeof(buf_min), f_min);
-	FOPS_OR_NULL_GOTO(s_min, out);
-
-	s_max = fgets(buf_max, sizeof(buf_max), f_max);
-	FOPS_OR_NULL_GOTO(s_max, out);
-
-
-	/* Strip the line break if there is */
-	p_min = strchr(buf_min, '\n');
-	if (p_min != NULL)
-		*p_min = 0;
-
-	p_max = strchr(buf_max, '\n');
-	if (p_max != NULL)
-		*p_max = 0;
-
-	sys_min_freq = strtoul(buf_min, &p_min, POWER_CONVERT_TO_DECIMAL);
-	sys_max_freq = strtoul(buf_max, &p_max, POWER_CONVERT_TO_DECIMAL);
+	ret = read_core_sysfs_u32(f_min, &sys_min_freq);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	if (sys_max_freq < sys_min_freq)
 		goto out;
@@ -572,27 +602,14 @@ power_get_cur_idx(struct pstate_power_info *pi)
 {
 	FILE *f_cur;
 	int ret = -1;
-	char *p_cur;
-	char buf_cur[BUFSIZ];
-	char fullpath_cur[PATH_MAX];
-	char *s_cur;
 	uint32_t sys_cur_freq = 0;
 	unsigned int i;
 
-	snprintf(fullpath_cur, sizeof(fullpath_cur),
-			POWER_SYSFILE_CUR_FREQ,
-			pi->lcore_id);
-	f_cur = fopen(fullpath_cur, "r");
-	FOPEN_OR_ERR_RET(f_cur, ret);
+	open_core_sysfs_file(POWER_SYSFILE_CUR_FREQ, pi->lcore_id, "r", &f_cur);
+	FOPEN_OR_ERR_GOTO(f_cur, fail);
 
-	/* initialize the cur_idx to matching current frequency freq index */
-	s_cur = fgets(buf_cur, sizeof(buf_cur), f_cur);
-	FOPS_OR_NULL_GOTO(s_cur, fail);
-
-	p_cur = strchr(buf_cur, '\n');
-	if (p_cur != NULL)
-		*p_cur = 0;
-	sys_cur_freq = strtoul(buf_cur, &p_cur, POWER_CONVERT_TO_DECIMAL);
+	ret = read_core_sysfs_u32(f_cur, &sys_cur_freq);
+	FOPS_OR_ERR_GOTO(ret, fail);
 
 	/* convert the frequency to nearest 100000 value
 	 * Ex: if sys_cur_freq=1396789 then freq_conv=1400000
@@ -611,10 +628,10 @@ power_get_cur_idx(struct pstate_power_info *pi)
 		}
 	}
 
-	fclose(f_cur);
-	return 0;
+	ret = 0;
 fail:
-	fclose(f_cur);
+	if (f_cur != NULL)
+		fclose(f_cur);
 	return ret;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 21.08] power: refactor pstate sysfs handling
  2021-04-01 15:05 [dpdk-dev] [PATCH 21.08] power: refactor pstate sysfs handling Anatoly Burakov
  2021-04-02  9:27 ` [dpdk-dev] [21.08 v2] " Anatoly Burakov
@ 2021-04-02 17:45 ` Stephen Hemminger
  2021-04-06 10:06   ` Burakov, Anatoly
  2021-04-02 17:46 ` Stephen Hemminger
  2 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2021-04-02 17:45 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev

On Thu,  1 Apr 2021 15:05:17 +0000
Anatoly Burakov <anatoly.burakov@intel.com> wrote:

>  
> +#define FOPEN_OR_ERR_GOTO(f, label) do { \
> +		if ((f) == NULL) { \
> +			RTE_LOG(ERR, POWER, "File not opened\n"); \
> +			goto label; \
> +		} \
> +} while (0)
> +

I am not a fan of macros like this that have gotos.
It is clearer to just open code it.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 21.08] power: refactor pstate sysfs handling
  2021-04-01 15:05 [dpdk-dev] [PATCH 21.08] power: refactor pstate sysfs handling Anatoly Burakov
  2021-04-02  9:27 ` [dpdk-dev] [21.08 v2] " Anatoly Burakov
  2021-04-02 17:45 ` [dpdk-dev] [PATCH 21.08] " Stephen Hemminger
@ 2021-04-02 17:46 ` Stephen Hemminger
  2021-04-06 10:05   ` Burakov, Anatoly
  2 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2021-04-02 17:46 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev

On Thu,  1 Apr 2021 15:05:17 +0000
Anatoly Burakov <anatoly.burakov@intel.com> wrote:

> +	/* fgets puts null terminator in, but do this just in case */
> +	buf[BUFSIZ - 1] = '\0';
> +
> +	/* strip off any terminating newlines */
> +	if (strlen(buf))
> +		strtok(buf, "\n");

Why not the simpler/shorter
	strchrnul(buf, '\n') = '\0';

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 21.08] power: refactor pstate sysfs handling
  2021-04-02 17:46 ` Stephen Hemminger
@ 2021-04-06 10:05   ` Burakov, Anatoly
  2021-04-22 15:08     ` [dpdk-dev] [21.08 PATCH v3 1/1] " Anatoly Burakov
  0 siblings, 1 reply; 12+ messages in thread
From: Burakov, Anatoly @ 2021-04-06 10:05 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

On 02-Apr-21 6:46 PM, Stephen Hemminger wrote:
> On Thu,  1 Apr 2021 15:05:17 +0000
> Anatoly Burakov <anatoly.burakov@intel.com> wrote:
> 
>> +	/* fgets puts null terminator in, but do this just in case */
>> +	buf[BUFSIZ - 1] = '\0';
>> +
>> +	/* strip off any terminating newlines */
>> +	if (strlen(buf))
>> +		strtok(buf, "\n");
> 
> Why not the simpler/shorter
> 	strchrnul(buf, '\n') = '\0';
> 

How many more string functions are there that i don't know about :D

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 21.08] power: refactor pstate sysfs handling
  2021-04-02 17:45 ` [dpdk-dev] [PATCH 21.08] " Stephen Hemminger
@ 2021-04-06 10:06   ` Burakov, Anatoly
  0 siblings, 0 replies; 12+ messages in thread
From: Burakov, Anatoly @ 2021-04-06 10:06 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

On 02-Apr-21 6:45 PM, Stephen Hemminger wrote:
> On Thu,  1 Apr 2021 15:05:17 +0000
> Anatoly Burakov <anatoly.burakov@intel.com> wrote:
> 
>>   
>> +#define FOPEN_OR_ERR_GOTO(f, label) do { \
>> +		if ((f) == NULL) { \
>> +			RTE_LOG(ERR, POWER, "File not opened\n"); \
>> +			goto label; \
>> +		} \
>> +} while (0)
>> +
> 
> I am not a fan of macros like this that have gotos.
> It is clearer to just open code it.
> 

Well, it's keeping consistent style with the rest of the file. If we are 
to remove these, ideally we should just replace usage of all of them.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [21.08 PATCH v3 1/1] power: refactor pstate sysfs handling
  2021-04-06 10:05   ` Burakov, Anatoly
@ 2021-04-22 15:08     ` Anatoly Burakov
  2021-04-23 11:03       ` [dpdk-dev] [21.08 PATCH v4 1/2] power: don't use rte prefix in internal code Anatoly Burakov
  2021-04-23 11:03       ` [dpdk-dev] [21.08 PATCH v4 2/2] power: refactor pstate and acpi code Anatoly Burakov
  0 siblings, 2 replies; 12+ messages in thread
From: Anatoly Burakov @ 2021-04-22 15:08 UTC (permalink / raw)
  To: dev; +Cc: david.hunt, stephen.hemminger, reshma.pattan

Currently, pstate sysfs handling code is a bit of an unmaintainable
mess, which has contributed to various errors leading to bugs. Refactor
the code in a way that makes it more maintainable and less error prone.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/power/meson.build            |   7 +
 lib/power/power_pstate_cpufreq.c | 357 ++++++++++++++++---------------
 2 files changed, 191 insertions(+), 173 deletions(-)

diff --git a/lib/power/meson.build b/lib/power/meson.build
index a2cc9fe2ef..85324d48d2 100644
--- a/lib/power/meson.build
+++ b/lib/power/meson.build
@@ -5,6 +5,13 @@ if not is_linux
     build = false
     reason = 'only supported on Linux'
 endif
+
+# we do some snprintf magic so silence format-nonliteral
+flag_nonliteral = '-Wno-format-nonliteral'
+if cc.has_argument(flag_nonliteral)
+	cflags += flag_nonliteral
+endif
+
 sources = files(
         'guest_channel.c',
         'power_acpi_cpufreq.c',
diff --git a/lib/power/power_pstate_cpufreq.c b/lib/power/power_pstate_cpufreq.c
index 2cfc54acf3..4357ac4920 100644
--- a/lib/power/power_pstate_cpufreq.c
+++ b/lib/power/power_pstate_cpufreq.c
@@ -37,6 +37,13 @@
 		} \
 } while (0)
 
+#define FOPEN_OR_ERR_GOTO(f, label) do { \
+		if ((f) == NULL) { \
+			RTE_LOG(ERR, POWER, "File not opened\n"); \
+			goto label; \
+		} \
+} while (0)
+
 #define FOPS_OR_NULL_GOTO(ret, label) do { \
 		if ((ret) == NULL) { \
 			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
@@ -148,97 +155,145 @@ out:	close(fd);
 	return ret;
 }
 
+static int
+open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
+		FILE **f)
+{
+	char fullpath[PATH_MAX];
+	FILE *tmpf;
+
+	/* silenced -Wformat-nonliteral here */
+	snprintf(fullpath, sizeof(fullpath), template, core);
+	tmpf = fopen(fullpath, mode);
+	if (tmpf == NULL)
+		return -1;
+	*f = tmpf;
+
+	return 0;
+}
+
+static int
+read_core_sysfs_u32(FILE *f, uint32_t *val)
+{
+	char buf[BUFSIZ];
+	uint32_t fval;
+	char *s;
+
+	s = fgets(buf, sizeof(buf), f);
+	if (s == NULL)
+		return -1;
+
+	/* fgets puts null terminator in, but do this just in case */
+	buf[BUFSIZ - 1] = '\0';
+
+	/* strip off any terminating newlines */
+	*strchrnul(buf, '\n') = '\0';
+
+	fval = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
+
+	/* write the value */
+	*val = fval;
+
+	return 0;
+}
+
+static int
+read_core_sysfs_s(FILE *f, char *buf, unsigned int len)
+{
+	char *s;
+
+	s = fgets(buf, len, f);
+	if (s == NULL)
+		return -1;
+
+	/* fgets puts null terminator in, but do this just in case */
+	buf[len - 1] = '\0';
+
+	/* strip off any terminating newlines */
+	*strchrnul(buf, '\n') = '\0';
+
+	return 0;
+}
+
+static int
+write_core_sysfs_s(FILE *f, const char *str)
+{
+	int ret;
+
+	ret = fseek(f, 0, SEEK_SET);
+	if (ret != 0)
+		return -1;
+
+	ret = fputs(str, f);
+	if (ret != 0)
+		return -1;
+
+	/* flush the output */
+	ret = fflush(f);
+	if (ret != 0)
+		return -1;
+
+	return 0;
+}
+
 /**
  * It is to fopen the sys file for the future setting the lcore frequency.
  */
 static int
 power_init_for_setting_freq(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max, *f_base = NULL, *f_base_max;
-	char fullpath_min[PATH_MAX];
-	char fullpath_max[PATH_MAX];
-	char fullpath_base[PATH_MAX];
-	char fullpath_base_max[PATH_MAX];
-	char buf_base[BUFSIZ];
-	char *s_base;
-	char *s_base_max;
-	uint32_t base_ratio = 0;
-	uint32_t base_max_ratio = 0;
-	uint64_t max_non_turbo = 0;
-	int  ret_val = 0;
-
-	snprintf(fullpath_base_max,
-			sizeof(fullpath_base_max),
-			POWER_SYSFILE_BASE_MAX_FREQ,
-			pi->lcore_id);
-	f_base_max = fopen(fullpath_base_max, "r");
-	FOPEN_OR_ERR_RET(f_base_max, -1);
-	if (f_base_max != NULL) {
-		s_base_max = fgets(buf_base, sizeof(buf_base), f_base_max);
-
-		/* close the file unconditionally */
-		fclose(f_base_max);
-		f_base_max = NULL;
-
-		FOPS_OR_NULL_GOTO(s_base_max, out);
-
-		buf_base[BUFSIZ-1] = '\0';
-		if (strlen(buf_base))
-			/* Strip off terminating '\n' */
-			strtok(buf_base, "\n");
-
-		base_max_ratio =
-			strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
-				/ BUS_FREQ;
-	}
-
-	snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
-			pi->lcore_id);
-	f_min = fopen(fullpath_min, "rw+");
-	FOPEN_OR_ERR_RET(f_min, -1);
-
-	snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ,
-			pi->lcore_id);
-	f_max = fopen(fullpath_max, "rw+");
-	if (f_max == NULL)
-		fclose(f_min);
-	FOPEN_OR_ERR_RET(f_max, -1);
-
-	pi->f_cur_min = f_min;
-	pi->f_cur_max = f_max;
-
-	snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ,
-			pi->lcore_id);
-
-	f_base = fopen(fullpath_base, "r");
-	if (f_base == NULL) {
-		/* No sysfs base_frequency, that's OK, continue without */
-		base_ratio = 0;
+	FILE *f_base = NULL, *f_base_max = NULL, *f_min = NULL, *f_max = NULL;
+	uint32_t base_ratio, base_max_ratio;
+	uint64_t max_non_turbo;
+	int ret;
+
+	/* open all files we expect to have open */
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+			&f_base_max);
+	FOPEN_OR_ERR_GOTO(f_base_max, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_MIN_FREQ, pi->lcore_id, "rw+",
+			&f_min);
+	FOPEN_OR_ERR_GOTO(f_min, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_MAX_FREQ, pi->lcore_id, "rw+",
+			&f_max);
+	FOPEN_OR_ERR_GOTO(f_max, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_BASE_FREQ, pi->lcore_id, "r",
+			&f_base);
+	/* base ratio file may not exist in some kernels, so no error check */
+
+	/* read base max ratio */
+	ret = read_core_sysfs_u32(f_base_max, &base_max_ratio);
+	FOPS_OR_ERR_GOTO(ret, err);
+
+	/* base ratio may not exist */
+	if (f_base != NULL) {
+		ret = read_core_sysfs_u32(f_base, &base_ratio);
+		FOPS_OR_ERR_GOTO(ret, err);
 	} else {
-		s_base = fgets(buf_base, sizeof(buf_base), f_base);
-		FOPS_OR_NULL_GOTO(s_base, out);
-
-		buf_base[BUFSIZ-1] = '\0';
-		if (strlen(buf_base))
-			/* Strip off terminating '\n' */
-			strtok(buf_base, "\n");
-
-		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
-				/ BUS_FREQ;
+		base_ratio = 0;
 	}
 
 	/* Add MSR read to detect turbo status */
+	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
+		goto err;
+	/* no errors after this point */
 
-	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0) {
-		ret_val = -1;
-		goto out;
-	}
+	/* convert ratios to bins */
+	base_max_ratio /= BUS_FREQ;
+	base_ratio /= BUS_FREQ;
+
+	/* assign file handles */
+	pi->f_cur_min = f_min;
+	pi->f_cur_max = f_max;
 
 	max_non_turbo = (max_non_turbo&NON_TURBO_MASK)>>NON_TURBO_OFFSET;
 
 	POWER_DEBUG_TRACE("no turbo perf %"PRIu64"\n", max_non_turbo);
 
-	pi->non_turbo_max_ratio = max_non_turbo;
+	pi->non_turbo_max_ratio = (uint32_t)max_non_turbo;
 
 	/*
 	 * If base_frequency is reported as greater than the maximum
@@ -264,7 +319,20 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 out:
 	if (f_base != NULL)
 		fclose(f_base);
-	return ret_val;
+	fclose(f_base_max);
+	/* f_min and f_max are stored, no need to close */
+	return 0;
+
+err:
+	if (f_base != NULL)
+		fclose(f_base);
+	if (f_base_max != NULL)
+		fclose(f_base_max);
+	if (f_min != NULL)
+		fclose(f_min);
+	if (f_max != NULL)
+		fclose(f_max);
+	return -1;
 }
 
 static int
@@ -369,22 +437,16 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
 static int
 power_set_governor_performance(struct pstate_power_info *pi)
 {
-	FILE *f;
+	FILE *f_governor = NULL;
 	int ret = -1;
 	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
 
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+			&f_governor);
+	FOPEN_OR_ERR_GOTO(f_governor, out);
 
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
-	/* Strip off terminating '\n' */
-	strtok(buf, "\n");
+	ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	/* Save the original governor */
 	rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
@@ -399,21 +461,15 @@ power_set_governor_performance(struct pstate_power_info *pi)
 	}
 
 	/* Write 'performance' to the governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(POWER_GOVERNOR_PERF, f);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	/* We need to flush to see if the fputs succeeds */
-	val = fflush(f);
-	FOPS_OR_ERR_GOTO(val, out);
+	ret = write_core_sysfs_s(f_governor, POWER_GOVERNOR_PERF);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	ret = 0;
 	RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
 			"set to performance successfully\n", pi->lcore_id);
 out:
-	fclose(f);
+	if (f_governor != NULL)
+		fclose(f_governor);
 
 	return ret;
 }
@@ -425,20 +481,16 @@ power_set_governor_performance(struct pstate_power_info *pi)
 static int
 power_set_governor_original(struct pstate_power_info *pi)
 {
-	FILE *f;
+	FILE *f_governor = NULL;
 	int ret = -1;
 	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
 
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+			&f_governor);
+	FOPEN_OR_ERR_GOTO(f_governor, out);
 
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
+	ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	/* Check if the governor to be set is the same as current */
 	if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
@@ -450,19 +502,16 @@ power_set_governor_original(struct pstate_power_info *pi)
 	}
 
 	/* Write back the original governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(pi->governor_ori, f);
-	FOPS_OR_ERR_GOTO(val, out);
+	ret = write_core_sysfs_s(f_governor, pi->governor_ori);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	ret = 0;
 	RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
 			"has been set back to %s successfully\n",
 			pi->lcore_id, pi->governor_ori);
 out:
-	fclose(f);
-
+	if (f_governor != NULL)
+		fclose(f_governor);
 	return ret;
 }
 
@@ -473,51 +522,26 @@ power_set_governor_original(struct pstate_power_info *pi)
 static int
 power_get_available_freqs(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max;
+	FILE *f_min = NULL, *f_max = NULL;
 	int ret = -1;
-	char *p_min, *p_max;
-	char buf_min[BUFSIZ];
-	char buf_max[BUFSIZ];
-	char fullpath_min[PATH_MAX];
-	char fullpath_max[PATH_MAX];
-	char *s_min, *s_max;
 	uint32_t sys_min_freq = 0, sys_max_freq = 0, base_max_freq = 0;
 	uint32_t i, num_freqs = 0;
 
-	snprintf(fullpath_max, sizeof(fullpath_max),
-			POWER_SYSFILE_BASE_MAX_FREQ,
-			pi->lcore_id);
-	snprintf(fullpath_min, sizeof(fullpath_min),
-			POWER_SYSFILE_BASE_MIN_FREQ,
-			pi->lcore_id);
+	/* open all files */
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+			&f_max);
+	FOPEN_OR_ERR_GOTO(f_max, out);
 
-	f_min = fopen(fullpath_min, "r");
-	FOPEN_OR_ERR_RET(f_min, ret);
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MIN_FREQ, pi->lcore_id, "r",
+			     &f_min);
+	FOPEN_OR_ERR_GOTO(f_max, out);
 
-	f_max = fopen(fullpath_max, "r");
-	if (f_max == NULL)
-		fclose(f_min);
+	/* read base ratios */
+	ret = read_core_sysfs_u32(f_max, &sys_max_freq);
+	FOPS_OR_ERR_GOTO(ret, out);
 
-	FOPEN_OR_ERR_RET(f_max, ret);
-
-	s_min = fgets(buf_min, sizeof(buf_min), f_min);
-	FOPS_OR_NULL_GOTO(s_min, out);
-
-	s_max = fgets(buf_max, sizeof(buf_max), f_max);
-	FOPS_OR_NULL_GOTO(s_max, out);
-
-
-	/* Strip the line break if there is */
-	p_min = strchr(buf_min, '\n');
-	if (p_min != NULL)
-		*p_min = 0;
-
-	p_max = strchr(buf_max, '\n');
-	if (p_max != NULL)
-		*p_max = 0;
-
-	sys_min_freq = strtoul(buf_min, &p_min, POWER_CONVERT_TO_DECIMAL);
-	sys_max_freq = strtoul(buf_max, &p_max, POWER_CONVERT_TO_DECIMAL);
+	ret = read_core_sysfs_u32(f_min, &sys_min_freq);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	if (sys_max_freq < sys_min_freq)
 		goto out;
@@ -576,27 +600,14 @@ power_get_cur_idx(struct pstate_power_info *pi)
 {
 	FILE *f_cur;
 	int ret = -1;
-	char *p_cur;
-	char buf_cur[BUFSIZ];
-	char fullpath_cur[PATH_MAX];
-	char *s_cur;
 	uint32_t sys_cur_freq = 0;
 	unsigned int i;
 
-	snprintf(fullpath_cur, sizeof(fullpath_cur),
-			POWER_SYSFILE_CUR_FREQ,
-			pi->lcore_id);
-	f_cur = fopen(fullpath_cur, "r");
-	FOPEN_OR_ERR_RET(f_cur, ret);
+	open_core_sysfs_file(POWER_SYSFILE_CUR_FREQ, pi->lcore_id, "r", &f_cur);
+	FOPEN_OR_ERR_GOTO(f_cur, fail);
 
-	/* initialize the cur_idx to matching current frequency freq index */
-	s_cur = fgets(buf_cur, sizeof(buf_cur), f_cur);
-	FOPS_OR_NULL_GOTO(s_cur, fail);
-
-	p_cur = strchr(buf_cur, '\n');
-	if (p_cur != NULL)
-		*p_cur = 0;
-	sys_cur_freq = strtoul(buf_cur, &p_cur, POWER_CONVERT_TO_DECIMAL);
+	ret = read_core_sysfs_u32(f_cur, &sys_cur_freq);
+	FOPS_OR_ERR_GOTO(ret, fail);
 
 	/* convert the frequency to nearest 100000 value
 	 * Ex: if sys_cur_freq=1396789 then freq_conv=1400000
@@ -615,10 +626,10 @@ power_get_cur_idx(struct pstate_power_info *pi)
 		}
 	}
 
-	fclose(f_cur);
-	return 0;
+	ret = 0;
 fail:
-	fclose(f_cur);
+	if (f_cur != NULL)
+		fclose(f_cur);
 	return ret;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [21.08 PATCH v4 1/2] power: don't use rte prefix in internal code
  2021-04-22 15:08     ` [dpdk-dev] [21.08 PATCH v3 1/1] " Anatoly Burakov
@ 2021-04-23 11:03       ` Anatoly Burakov
  2021-05-31 10:23         ` David Hunt
  2021-04-23 11:03       ` [dpdk-dev] [21.08 PATCH v4 2/2] power: refactor pstate and acpi code Anatoly Burakov
  1 sibling, 1 reply; 12+ messages in thread
From: Anatoly Burakov @ 2021-04-23 11:03 UTC (permalink / raw)
  To: dev; +Cc: stephen, Richael.Zhuang, reshma.pattan, david.hunt

Currently, ACPI code uses rte_power_info as the struct name, which
gives the appearance that this is an externally visible API. Fix to
use internal namespace.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/power/power_acpi_cpufreq.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/lib/power/power_acpi_cpufreq.c b/lib/power/power_acpi_cpufreq.c
index d028a9947f..1b8c69cc8b 100644
--- a/lib/power/power_acpi_cpufreq.c
+++ b/lib/power/power_acpi_cpufreq.c
@@ -78,7 +78,7 @@ enum power_state {
 /**
  * Power info per lcore.
  */
-struct rte_power_info {
+struct acpi_power_info {
 	unsigned int lcore_id;                   /**< Logical core id */
 	uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
 	uint32_t nb_freqs;                   /**< number of available freqs */
@@ -90,14 +90,14 @@ struct rte_power_info {
 	uint16_t turbo_enable;               /**< Turbo Boost enable/disable */
 } __rte_cache_aligned;
 
-static struct rte_power_info lcore_power_info[RTE_MAX_LCORE];
+static struct acpi_power_info lcore_power_info[RTE_MAX_LCORE];
 
 /**
  * It is to set specific freq for specific logical core, according to the index
  * of supported frequencies.
  */
 static int
-set_freq_internal(struct rte_power_info *pi, uint32_t idx)
+set_freq_internal(struct acpi_power_info *pi, uint32_t idx)
 {
 	if (idx >= RTE_MAX_LCORE_FREQS || idx >= pi->nb_freqs) {
 		RTE_LOG(ERR, POWER, "Invalid frequency index %u, which "
@@ -133,7 +133,7 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx)
  * governor will be saved for rolling back.
  */
 static int
-power_set_governor_userspace(struct rte_power_info *pi)
+power_set_governor_userspace(struct acpi_power_info *pi)
 {
 	FILE *f;
 	int ret = -1;
@@ -189,7 +189,7 @@ power_set_governor_userspace(struct rte_power_info *pi)
  * sys file.
  */
 static int
-power_get_available_freqs(struct rte_power_info *pi)
+power_get_available_freqs(struct acpi_power_info *pi)
 {
 	FILE *f;
 	int ret = -1, i, count;
@@ -259,7 +259,7 @@ power_get_available_freqs(struct rte_power_info *pi)
  * It is to fopen the sys file for the future setting the lcore frequency.
  */
 static int
-power_init_for_setting_freq(struct rte_power_info *pi)
+power_init_for_setting_freq(struct acpi_power_info *pi)
 {
 	FILE *f;
 	char fullpath[PATH_MAX];
@@ -299,7 +299,7 @@ power_acpi_cpufreq_check_supported(void)
 int
 power_acpi_cpufreq_init(unsigned int lcore_id)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 	uint32_t exp_state;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
@@ -374,7 +374,7 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
  * needed by writing the sys file.
  */
 static int
-power_set_governor_original(struct rte_power_info *pi)
+power_set_governor_original(struct acpi_power_info *pi)
 {
 	FILE *f;
 	int ret = -1;
@@ -420,7 +420,7 @@ power_set_governor_original(struct rte_power_info *pi)
 int
 power_acpi_cpufreq_exit(unsigned int lcore_id)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 	uint32_t exp_state;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
@@ -475,7 +475,7 @@ power_acpi_cpufreq_exit(unsigned int lcore_id)
 uint32_t
 power_acpi_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
@@ -522,7 +522,7 @@ power_acpi_cpufreq_set_freq(unsigned int lcore_id, uint32_t index)
 int
 power_acpi_cpufreq_freq_down(unsigned int lcore_id)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
@@ -540,7 +540,7 @@ power_acpi_cpufreq_freq_down(unsigned int lcore_id)
 int
 power_acpi_cpufreq_freq_up(unsigned int lcore_id)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
@@ -581,7 +581,7 @@ power_acpi_cpufreq_freq_max(unsigned int lcore_id)
 int
 power_acpi_cpufreq_freq_min(unsigned int lcore_id)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
@@ -598,7 +598,7 @@ power_acpi_cpufreq_freq_min(unsigned int lcore_id)
 int
 power_acpi_turbo_status(unsigned int lcore_id)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
@@ -614,7 +614,7 @@ power_acpi_turbo_status(unsigned int lcore_id)
 int
 power_acpi_enable_turbo(unsigned int lcore_id)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
@@ -647,7 +647,7 @@ power_acpi_enable_turbo(unsigned int lcore_id)
 int
 power_acpi_disable_turbo(unsigned int lcore_id)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
@@ -674,7 +674,7 @@ power_acpi_disable_turbo(unsigned int lcore_id)
 int power_acpi_get_capabilities(unsigned int lcore_id,
 		struct rte_power_core_capabilities *caps)
 {
-	struct rte_power_info *pi;
+	struct acpi_power_info *pi;
 
 	if (lcore_id >= RTE_MAX_LCORE) {
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [21.08 PATCH v4 2/2] power: refactor pstate and acpi code
  2021-04-22 15:08     ` [dpdk-dev] [21.08 PATCH v3 1/1] " Anatoly Burakov
  2021-04-23 11:03       ` [dpdk-dev] [21.08 PATCH v4 1/2] power: don't use rte prefix in internal code Anatoly Burakov
@ 2021-04-23 11:03       ` Anatoly Burakov
  2021-05-07  2:13         ` Richael Zhuang
  1 sibling, 1 reply; 12+ messages in thread
From: Anatoly Burakov @ 2021-04-23 11:03 UTC (permalink / raw)
  To: dev; +Cc: stephen, Richael.Zhuang, reshma.pattan, david.hunt

Currently, ACPI and PSTATE modes have lots of code duplication,
confusing logic, and a bunch of other issues that can, and have, led to
various bugs and resource leaks.

This commit factors out the common parts of sysfs reading/writing for
ACPI and PSTATE drivers.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/power/meson.build            |   7 +
 lib/power/power_acpi_cpufreq.c   | 178 +++--------------
 lib/power/power_common.c         | 133 +++++++++++++
 lib/power/power_common.h         |  46 +++++
 lib/power/power_pstate_cpufreq.c | 332 ++++++++-----------------------
 5 files changed, 293 insertions(+), 403 deletions(-)

diff --git a/lib/power/meson.build b/lib/power/meson.build
index a2cc9fe2ef..85324d48d2 100644
--- a/lib/power/meson.build
+++ b/lib/power/meson.build
@@ -5,6 +5,13 @@ if not is_linux
     build = false
     reason = 'only supported on Linux'
 endif
+
+# we do some snprintf magic so silence format-nonliteral
+flag_nonliteral = '-Wno-format-nonliteral'
+if cc.has_argument(flag_nonliteral)
+	cflags += flag_nonliteral
+endif
+
 sources = files(
         'guest_channel.c',
         'power_acpi_cpufreq.c',
diff --git a/lib/power/power_acpi_cpufreq.c b/lib/power/power_acpi_cpufreq.c
index 1b8c69cc8b..97f1d302c9 100644
--- a/lib/power/power_acpi_cpufreq.c
+++ b/lib/power/power_acpi_cpufreq.c
@@ -19,41 +19,10 @@
 #include "power_acpi_cpufreq.h"
 #include "power_common.h"
 
-#ifdef RTE_LIBRTE_POWER_DEBUG
-#define POWER_DEBUG_TRACE(fmt, args...) do { \
-		RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \
-} while (0)
-#else
-#define POWER_DEBUG_TRACE(fmt, args...)
-#endif
-
-#define FOPEN_OR_ERR_RET(f, retval) do { \
-		if ((f) == NULL) { \
-			RTE_LOG(ERR, POWER, "File not opened\n"); \
-			return retval; \
-		} \
-} while (0)
-
-#define FOPS_OR_NULL_GOTO(ret, label) do { \
-		if ((ret) == NULL) { \
-			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
-			goto label; \
-		} \
-} while (0)
-
-#define FOPS_OR_ERR_GOTO(ret, label) do { \
-		if ((ret) < 0) { \
-			RTE_LOG(ERR, POWER, "File operations failed\n"); \
-			goto label; \
-		} \
-} while (0)
-
 #define STR_SIZE     1024
 #define POWER_CONVERT_TO_DECIMAL 10
 
 #define POWER_GOVERNOR_USERSPACE "userspace"
-#define POWER_SYSFILE_GOVERNOR   \
-		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
 #define POWER_SYSFILE_AVAIL_FREQ \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_frequencies"
 #define POWER_SYSFILE_SETSPEED   \
@@ -135,53 +104,18 @@ set_freq_internal(struct acpi_power_info *pi, uint32_t idx)
 static int
 power_set_governor_userspace(struct acpi_power_info *pi)
 {
-	FILE *f;
-	int ret = -1;
-	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
-
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
-
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
-	/* Strip off terminating '\n' */
-	strtok(buf, "\n");
-
-	/* Save the original governor */
-	rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
-
-	/* Check if current governor is userspace */
-	if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
-			sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
-		ret = 0;
-		POWER_DEBUG_TRACE("Power management governor of lcore %u is "
-				"already userspace\n", pi->lcore_id);
-		goto out;
-	}
-
-	/* Write 'userspace' to the governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(POWER_GOVERNOR_USERSPACE, f);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	/* We need to flush to see if the fputs succeeds */
-	val = fflush(f);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	ret = 0;
-	RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
-			"set to user space successfully\n", pi->lcore_id);
-out:
-	fclose(f);
-
-	return ret;
+	return power_set_governor(pi->lcore_id, POWER_GOVERNOR_USERSPACE,
+			pi->governor_ori, sizeof(pi->governor_ori));
+}
+
+/**
+ * It is to check the governor and then set the original governor back if
+ * needed by writing the sys file.
+ */
+static int
+power_set_governor_original(struct acpi_power_info *pi)
+{
+	return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
 }
 
 /**
@@ -195,22 +129,14 @@ power_get_available_freqs(struct acpi_power_info *pi)
 	int ret = -1, i, count;
 	char *p;
 	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
 	char *freqs[RTE_MAX_LCORE_FREQS];
-	char *s;
 
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ,
-			pi->lcore_id);
-	f = fopen(fullpath, "r");
-	FOPEN_OR_ERR_RET(f, ret);
+	open_core_sysfs_file(POWER_SYSFILE_AVAIL_FREQ, pi->lcore_id, "r",
+			&f);
+	FOPEN_OR_ERR_GOTO(f, out);
 
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
-
-	/* Strip the line break if there is */
-	p = strchr(buf, '\n');
-	if (p != NULL)
-		*p = 0;
+	ret = read_core_sysfs_s(f, buf, sizeof(buf));
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	/* Split string into at most RTE_MAX_LCORE_FREQS frequencies */
 	count = rte_strsplit(buf, sizeof(buf), freqs,
@@ -250,7 +176,8 @@ power_get_available_freqs(struct acpi_power_info *pi)
 	POWER_DEBUG_TRACE("%d frequency(s) of lcore %u are available\n",
 			count, pi->lcore_id);
 out:
-	fclose(f);
+	if (f != NULL)
+		fclose(f);
 
 	return ret;
 }
@@ -262,18 +189,16 @@ static int
 power_init_for_setting_freq(struct acpi_power_info *pi)
 {
 	FILE *f;
-	char fullpath[PATH_MAX];
 	char buf[BUFSIZ];
 	uint32_t i, freq;
-	char *s;
+	int ret;
 
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, -1);
+	open_core_sysfs_file(POWER_SYSFILE_SETSPEED, pi->lcore_id, "r",
+			&f);
+	FOPEN_OR_ERR_GOTO(f, err);
 
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
+	ret = read_core_sysfs_s(f, buf, sizeof(buf));
+	FOPS_OR_ERR_GOTO(ret, err);
 
 	freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
 	for (i = 0; i < pi->nb_freqs; i++) {
@@ -284,8 +209,9 @@ power_init_for_setting_freq(struct acpi_power_info *pi)
 		}
 	}
 
-out:
-	fclose(f);
+err:
+	if (f != NULL)
+		fclose(f);
 
 	return -1;
 }
@@ -369,54 +295,6 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
 	return -1;
 }
 
-/**
- * It is to check the governor and then set the original governor back if
- * needed by writing the sys file.
- */
-static int
-power_set_governor_original(struct acpi_power_info *pi)
-{
-	FILE *f;
-	int ret = -1;
-	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
-
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
-
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
-
-	/* Check if the governor to be set is the same as current */
-	if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
-		ret = 0;
-		POWER_DEBUG_TRACE("Power management governor of lcore %u "
-				"has already been set to %s\n",
-				pi->lcore_id, pi->governor_ori);
-		goto out;
-	}
-
-	/* Write back the original governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(pi->governor_ori, f);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	ret = 0;
-	RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
-			"has been set back to %s successfully\n",
-			pi->lcore_id, pi->governor_ori);
-out:
-	fclose(f);
-
-	return ret;
-}
-
 int
 power_acpi_cpufreq_exit(unsigned int lcore_id)
 {
diff --git a/lib/power/power_common.c b/lib/power/power_common.c
index 67e3318ec7..f06e9e5fa9 100644
--- a/lib/power/power_common.c
+++ b/lib/power/power_common.c
@@ -3,13 +3,20 @@
  */
 
 #include <limits.h>
+#include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
 
+#include <rte_log.h>
+#include <rte_string_fns.h>
+
 #include "power_common.h"
 
 #define POWER_SYSFILE_SCALING_DRIVER   \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
+#define POWER_SYSFILE_GOVERNOR  \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
+#define POWER_CONVERT_TO_DECIMAL 10
 
 int
 cpufreq_check_scaling_driver(const char *driver_name)
@@ -58,3 +65,129 @@ cpufreq_check_scaling_driver(const char *driver_name)
 	 */
 	return 1;
 }
+
+int
+open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
+		FILE **f)
+{
+	char fullpath[PATH_MAX];
+	FILE *tmpf;
+
+	/* silenced -Wformat-nonliteral here */
+	snprintf(fullpath, sizeof(fullpath), template, core);
+	tmpf = fopen(fullpath, mode);
+	if (tmpf == NULL)
+		return -1;
+	*f = tmpf;
+
+	return 0;
+}
+
+int
+read_core_sysfs_u32(FILE *f, uint32_t *val)
+{
+	char buf[BUFSIZ];
+	uint32_t fval;
+	char *s;
+
+	s = fgets(buf, sizeof(buf), f);
+	if (s == NULL)
+		return -1;
+
+	/* fgets puts null terminator in, but do this just in case */
+	buf[BUFSIZ - 1] = '\0';
+
+	/* strip off any terminating newlines */
+	*strchrnul(buf, '\n') = '\0';
+
+	fval = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
+
+	/* write the value */
+	*val = fval;
+
+	return 0;
+}
+
+int
+read_core_sysfs_s(FILE *f, char *buf, unsigned int len)
+{
+	char *s;
+
+	s = fgets(buf, len, f);
+	if (s == NULL)
+		return -1;
+
+	/* fgets puts null terminator in, but do this just in case */
+	buf[len - 1] = '\0';
+
+	/* strip off any terminating newlines */
+	*strchrnul(buf, '\n') = '\0';
+
+	return 0;
+}
+
+int
+write_core_sysfs_s(FILE *f, const char *str)
+{
+	int ret;
+
+	ret = fseek(f, 0, SEEK_SET);
+	if (ret != 0)
+		return -1;
+
+	ret = fputs(str, f);
+	if (ret != 0)
+		return -1;
+
+	/* flush the output */
+	ret = fflush(f);
+	if (ret != 0)
+		return -1;
+
+	return 0;
+}
+
+/**
+ * It is to check the current scaling governor by reading sys file, and then
+ * set it into 'performance' if it is not by writing the sys file. The original
+ * governor will be saved for rolling back.
+ */
+int
+power_set_governor(unsigned int lcore_id, const char *new_governor,
+		char *orig_governor, size_t orig_governor_len)
+{
+	FILE *f_governor = NULL;
+	int ret = -1;
+	char buf[BUFSIZ];
+
+	open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, lcore_id, "rw+", &f_governor);
+	FOPEN_OR_ERR_GOTO(f_governor, out);
+
+	ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+	FOPS_OR_ERR_GOTO(ret, out);
+
+	/* Save the original governor, if it was provided */
+	if (orig_governor)
+		rte_strscpy(orig_governor, buf, orig_governor_len);
+
+	/* Check if current governor is already what we want */
+	if (strcmp(buf, new_governor) == 0) {
+		ret = 0;
+		POWER_DEBUG_TRACE("Power management governor of lcore %u is "
+				"already %s\n", lcore_id, new_governor);
+		goto out;
+	}
+
+	/* Write the new governor */
+	ret = write_core_sysfs_s(f_governor, new_governor);
+	FOPS_OR_ERR_GOTO(ret, out);
+
+	ret = 0;
+	RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
+			"set to '%s' successfully\n", lcore_id, new_governor);
+out:
+	if (f_governor != NULL)
+		fclose(f_governor);
+
+	return ret;
+}
diff --git a/lib/power/power_common.h b/lib/power/power_common.h
index fab3ca995a..729ae8cbf4 100644
--- a/lib/power/power_common.h
+++ b/lib/power/power_common.h
@@ -5,9 +5,55 @@
 #ifndef _POWER_COMMON_H_
 #define _POWER_COMMON_H_
 
+#include <inttypes.h>
+
 #define RTE_POWER_INVALID_FREQ_INDEX (~0)
 
+
+#ifdef RTE_LIBRTE_POWER_DEBUG
+#define POWER_DEBUG_TRACE(fmt, args...) do { \
+		RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \
+} while (0)
+#else
+#define POWER_DEBUG_TRACE(fmt, args...)
+#endif
+
+#define FOPEN_OR_ERR_RET(f, retval) do { \
+		if ((f) == NULL) { \
+			RTE_LOG(ERR, POWER, "File not opened\n"); \
+			return retval; \
+		} \
+} while (0)
+
+#define FOPEN_OR_ERR_GOTO(f, label) do { \
+		if ((f) == NULL) { \
+			RTE_LOG(ERR, POWER, "File not opened\n"); \
+			goto label; \
+		} \
+} while (0)
+
+#define FOPS_OR_NULL_GOTO(ret, label) do { \
+		if ((ret) == NULL) { \
+			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
+			goto label; \
+		} \
+} while (0)
+
+#define FOPS_OR_ERR_GOTO(ret, label) do { \
+		if ((ret) < 0) { \
+			RTE_LOG(ERR, POWER, "File operations failed\n"); \
+			goto label; \
+		} \
+} while (0)
+
 /* check if scaling driver matches one we want */
 int cpufreq_check_scaling_driver(const char *driver);
+int power_set_governor(unsigned int lcore_id, const char *new_governor,
+		char *orig_governor, size_t orig_governor_len);
+int open_core_sysfs_file(const char *template, unsigned int core,
+			 const char *mode, FILE **f);
+int read_core_sysfs_u32(FILE *f, uint32_t *val);
+int read_core_sysfs_s(FILE *f, char *buf, unsigned int len);
+int write_core_sysfs_s(FILE *f, const char *str);
 
 #endif /* _POWER_COMMON_H_ */
diff --git a/lib/power/power_pstate_cpufreq.c b/lib/power/power_pstate_cpufreq.c
index 2cfc54acf3..2a365988a9 100644
--- a/lib/power/power_pstate_cpufreq.c
+++ b/lib/power/power_pstate_cpufreq.c
@@ -21,46 +21,13 @@
 #include "power_pstate_cpufreq.h"
 #include "power_common.h"
 
-
-#ifdef RTE_LIBRTE_POWER_DEBUG
-#define POWER_DEBUG_TRACE(fmt, args...) do { \
-		RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \
-} while (0)
-#else
-#define POWER_DEBUG_TRACE(fmt, args...)
-#endif
-
-#define FOPEN_OR_ERR_RET(f, retval) do { \
-		if ((f) == NULL) { \
-			RTE_LOG(ERR, POWER, "File not opened\n"); \
-			return retval; \
-		} \
-} while (0)
-
-#define FOPS_OR_NULL_GOTO(ret, label) do { \
-		if ((ret) == NULL) { \
-			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
-			goto label; \
-		} \
-} while (0)
-
-#define FOPS_OR_ERR_GOTO(ret, label) do { \
-		if ((ret) < 0) { \
-			RTE_LOG(ERR, POWER, "File operations failed\n"); \
-			goto label; \
-		} \
-} while (0)
-
 /* macros used for rounding frequency to nearest 100000 */
 #define FREQ_ROUNDING_DELTA 50000
 #define ROUND_FREQ_TO_N_100000 100000
 
-#define POWER_CONVERT_TO_DECIMAL 10
 #define BUS_FREQ     100000
 
 #define POWER_GOVERNOR_PERF "performance"
-#define POWER_SYSFILE_GOVERNOR  \
-		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
 #define POWER_SYSFILE_MAX_FREQ \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_max_freq"
 #define POWER_SYSFILE_MIN_FREQ  \
@@ -154,91 +121,58 @@ out:	close(fd);
 static int
 power_init_for_setting_freq(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max, *f_base = NULL, *f_base_max;
-	char fullpath_min[PATH_MAX];
-	char fullpath_max[PATH_MAX];
-	char fullpath_base[PATH_MAX];
-	char fullpath_base_max[PATH_MAX];
-	char buf_base[BUFSIZ];
-	char *s_base;
-	char *s_base_max;
-	uint32_t base_ratio = 0;
-	uint32_t base_max_ratio = 0;
-	uint64_t max_non_turbo = 0;
-	int  ret_val = 0;
-
-	snprintf(fullpath_base_max,
-			sizeof(fullpath_base_max),
-			POWER_SYSFILE_BASE_MAX_FREQ,
-			pi->lcore_id);
-	f_base_max = fopen(fullpath_base_max, "r");
-	FOPEN_OR_ERR_RET(f_base_max, -1);
-	if (f_base_max != NULL) {
-		s_base_max = fgets(buf_base, sizeof(buf_base), f_base_max);
-
-		/* close the file unconditionally */
-		fclose(f_base_max);
-		f_base_max = NULL;
-
-		FOPS_OR_NULL_GOTO(s_base_max, out);
-
-		buf_base[BUFSIZ-1] = '\0';
-		if (strlen(buf_base))
-			/* Strip off terminating '\n' */
-			strtok(buf_base, "\n");
-
-		base_max_ratio =
-			strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
-				/ BUS_FREQ;
-	}
-
-	snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
-			pi->lcore_id);
-	f_min = fopen(fullpath_min, "rw+");
-	FOPEN_OR_ERR_RET(f_min, -1);
-
-	snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ,
-			pi->lcore_id);
-	f_max = fopen(fullpath_max, "rw+");
-	if (f_max == NULL)
-		fclose(f_min);
-	FOPEN_OR_ERR_RET(f_max, -1);
-
-	pi->f_cur_min = f_min;
-	pi->f_cur_max = f_max;
-
-	snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ,
-			pi->lcore_id);
-
-	f_base = fopen(fullpath_base, "r");
-	if (f_base == NULL) {
-		/* No sysfs base_frequency, that's OK, continue without */
-		base_ratio = 0;
+	FILE *f_base = NULL, *f_base_max = NULL, *f_min = NULL, *f_max = NULL;
+	uint32_t base_ratio, base_max_ratio;
+	uint64_t max_non_turbo;
+	int ret;
+
+	/* open all files we expect to have open */
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+			&f_base_max);
+	FOPEN_OR_ERR_GOTO(f_base_max, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_MIN_FREQ, pi->lcore_id, "rw+",
+			&f_min);
+	FOPEN_OR_ERR_GOTO(f_min, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_MAX_FREQ, pi->lcore_id, "rw+",
+			&f_max);
+	FOPEN_OR_ERR_GOTO(f_max, err);
+
+	open_core_sysfs_file(POWER_SYSFILE_BASE_FREQ, pi->lcore_id, "r",
+			&f_base);
+	/* base ratio file may not exist in some kernels, so no error check */
+
+	/* read base max ratio */
+	ret = read_core_sysfs_u32(f_base_max, &base_max_ratio);
+	FOPS_OR_ERR_GOTO(ret, err);
+
+	/* base ratio may not exist */
+	if (f_base != NULL) {
+		ret = read_core_sysfs_u32(f_base, &base_ratio);
+		FOPS_OR_ERR_GOTO(ret, err);
 	} else {
-		s_base = fgets(buf_base, sizeof(buf_base), f_base);
-		FOPS_OR_NULL_GOTO(s_base, out);
-
-		buf_base[BUFSIZ-1] = '\0';
-		if (strlen(buf_base))
-			/* Strip off terminating '\n' */
-			strtok(buf_base, "\n");
-
-		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
-				/ BUS_FREQ;
+		base_ratio = 0;
 	}
 
 	/* Add MSR read to detect turbo status */
+	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
+		goto err;
+	/* no errors after this point */
 
-	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0) {
-		ret_val = -1;
-		goto out;
-	}
+	/* convert ratios to bins */
+	base_max_ratio /= BUS_FREQ;
+	base_ratio /= BUS_FREQ;
+
+	/* assign file handles */
+	pi->f_cur_min = f_min;
+	pi->f_cur_max = f_max;
 
 	max_non_turbo = (max_non_turbo&NON_TURBO_MASK)>>NON_TURBO_OFFSET;
 
 	POWER_DEBUG_TRACE("no turbo perf %"PRIu64"\n", max_non_turbo);
 
-	pi->non_turbo_max_ratio = max_non_turbo;
+	pi->non_turbo_max_ratio = (uint32_t)max_non_turbo;
 
 	/*
 	 * If base_frequency is reported as greater than the maximum
@@ -264,7 +198,20 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 out:
 	if (f_base != NULL)
 		fclose(f_base);
-	return ret_val;
+	fclose(f_base_max);
+	/* f_min and f_max are stored, no need to close */
+	return 0;
+
+err:
+	if (f_base != NULL)
+		fclose(f_base);
+	if (f_base_max != NULL)
+		fclose(f_base_max);
+	if (f_min != NULL)
+		fclose(f_min);
+	if (f_max != NULL)
+		fclose(f_max);
+	return -1;
 }
 
 static int
@@ -369,53 +316,8 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
 static int
 power_set_governor_performance(struct pstate_power_info *pi)
 {
-	FILE *f;
-	int ret = -1;
-	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
-
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
-
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
-	/* Strip off terminating '\n' */
-	strtok(buf, "\n");
-
-	/* Save the original governor */
-	rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
-
-	/* Check if current governor is performance */
-	if (strncmp(buf, POWER_GOVERNOR_PERF,
-			sizeof(POWER_GOVERNOR_PERF)) == 0) {
-		ret = 0;
-		POWER_DEBUG_TRACE("Power management governor of lcore %u is "
-				"already performance\n", pi->lcore_id);
-		goto out;
-	}
-
-	/* Write 'performance' to the governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(POWER_GOVERNOR_PERF, f);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	/* We need to flush to see if the fputs succeeds */
-	val = fflush(f);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	ret = 0;
-	RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
-			"set to performance successfully\n", pi->lcore_id);
-out:
-	fclose(f);
-
-	return ret;
+	return power_set_governor(pi->lcore_id, POWER_GOVERNOR_PERF,
+			pi->governor_ori, sizeof(pi->governor_ori));
 }
 
 /**
@@ -425,45 +327,7 @@ power_set_governor_performance(struct pstate_power_info *pi)
 static int
 power_set_governor_original(struct pstate_power_info *pi)
 {
-	FILE *f;
-	int ret = -1;
-	char buf[BUFSIZ];
-	char fullpath[PATH_MAX];
-	char *s;
-	int val;
-
-	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
-			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
-
-	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
-
-	/* Check if the governor to be set is the same as current */
-	if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
-		ret = 0;
-		POWER_DEBUG_TRACE("Power management governor of lcore %u "
-				"has already been set to %s\n",
-				pi->lcore_id, pi->governor_ori);
-		goto out;
-	}
-
-	/* Write back the original governor */
-	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	val = fputs(pi->governor_ori, f);
-	FOPS_OR_ERR_GOTO(val, out);
-
-	ret = 0;
-	RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
-			"has been set back to %s successfully\n",
-			pi->lcore_id, pi->governor_ori);
-out:
-	fclose(f);
-
-	return ret;
+	return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
 }
 
 /**
@@ -473,51 +337,26 @@ power_set_governor_original(struct pstate_power_info *pi)
 static int
 power_get_available_freqs(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max;
+	FILE *f_min = NULL, *f_max = NULL;
 	int ret = -1;
-	char *p_min, *p_max;
-	char buf_min[BUFSIZ];
-	char buf_max[BUFSIZ];
-	char fullpath_min[PATH_MAX];
-	char fullpath_max[PATH_MAX];
-	char *s_min, *s_max;
 	uint32_t sys_min_freq = 0, sys_max_freq = 0, base_max_freq = 0;
 	uint32_t i, num_freqs = 0;
 
-	snprintf(fullpath_max, sizeof(fullpath_max),
-			POWER_SYSFILE_BASE_MAX_FREQ,
-			pi->lcore_id);
-	snprintf(fullpath_min, sizeof(fullpath_min),
-			POWER_SYSFILE_BASE_MIN_FREQ,
-			pi->lcore_id);
+	/* open all files */
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+			&f_max);
+	FOPEN_OR_ERR_GOTO(f_max, out);
 
-	f_min = fopen(fullpath_min, "r");
-	FOPEN_OR_ERR_RET(f_min, ret);
+	open_core_sysfs_file(POWER_SYSFILE_BASE_MIN_FREQ, pi->lcore_id, "r",
+			     &f_min);
+	FOPEN_OR_ERR_GOTO(f_max, out);
 
-	f_max = fopen(fullpath_max, "r");
-	if (f_max == NULL)
-		fclose(f_min);
+	/* read base ratios */
+	ret = read_core_sysfs_u32(f_max, &sys_max_freq);
+	FOPS_OR_ERR_GOTO(ret, out);
 
-	FOPEN_OR_ERR_RET(f_max, ret);
-
-	s_min = fgets(buf_min, sizeof(buf_min), f_min);
-	FOPS_OR_NULL_GOTO(s_min, out);
-
-	s_max = fgets(buf_max, sizeof(buf_max), f_max);
-	FOPS_OR_NULL_GOTO(s_max, out);
-
-
-	/* Strip the line break if there is */
-	p_min = strchr(buf_min, '\n');
-	if (p_min != NULL)
-		*p_min = 0;
-
-	p_max = strchr(buf_max, '\n');
-	if (p_max != NULL)
-		*p_max = 0;
-
-	sys_min_freq = strtoul(buf_min, &p_min, POWER_CONVERT_TO_DECIMAL);
-	sys_max_freq = strtoul(buf_max, &p_max, POWER_CONVERT_TO_DECIMAL);
+	ret = read_core_sysfs_u32(f_min, &sys_min_freq);
+	FOPS_OR_ERR_GOTO(ret, out);
 
 	if (sys_max_freq < sys_min_freq)
 		goto out;
@@ -576,27 +415,14 @@ power_get_cur_idx(struct pstate_power_info *pi)
 {
 	FILE *f_cur;
 	int ret = -1;
-	char *p_cur;
-	char buf_cur[BUFSIZ];
-	char fullpath_cur[PATH_MAX];
-	char *s_cur;
 	uint32_t sys_cur_freq = 0;
 	unsigned int i;
 
-	snprintf(fullpath_cur, sizeof(fullpath_cur),
-			POWER_SYSFILE_CUR_FREQ,
-			pi->lcore_id);
-	f_cur = fopen(fullpath_cur, "r");
-	FOPEN_OR_ERR_RET(f_cur, ret);
+	open_core_sysfs_file(POWER_SYSFILE_CUR_FREQ, pi->lcore_id, "r", &f_cur);
+	FOPEN_OR_ERR_GOTO(f_cur, fail);
 
-	/* initialize the cur_idx to matching current frequency freq index */
-	s_cur = fgets(buf_cur, sizeof(buf_cur), f_cur);
-	FOPS_OR_NULL_GOTO(s_cur, fail);
-
-	p_cur = strchr(buf_cur, '\n');
-	if (p_cur != NULL)
-		*p_cur = 0;
-	sys_cur_freq = strtoul(buf_cur, &p_cur, POWER_CONVERT_TO_DECIMAL);
+	ret = read_core_sysfs_u32(f_cur, &sys_cur_freq);
+	FOPS_OR_ERR_GOTO(ret, fail);
 
 	/* convert the frequency to nearest 100000 value
 	 * Ex: if sys_cur_freq=1396789 then freq_conv=1400000
@@ -615,10 +441,10 @@ power_get_cur_idx(struct pstate_power_info *pi)
 		}
 	}
 
-	fclose(f_cur);
-	return 0;
+	ret = 0;
 fail:
-	fclose(f_cur);
+	if (f_cur != NULL)
+		fclose(f_cur);
 	return ret;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [21.08 PATCH v4 2/2] power: refactor pstate and acpi code
  2021-04-23 11:03       ` [dpdk-dev] [21.08 PATCH v4 2/2] power: refactor pstate and acpi code Anatoly Burakov
@ 2021-05-07  2:13         ` Richael Zhuang
  2021-05-07  9:49           ` Burakov, Anatoly
  0 siblings, 1 reply; 12+ messages in thread
From: Richael Zhuang @ 2021-05-07  2:13 UTC (permalink / raw)
  To: Anatoly Burakov, dev; +Cc: stephen, reshma.pattan, david.hunt, nd, nd



> -----Original Message-----
> From: Anatoly Burakov <anatoly.burakov@intel.com>
> Sent: Friday, April 23, 2021 7:03 PM
> To: dev@dpdk.org
> Cc: stephen@networkplumber.org; Richael Zhuang
> <Richael.Zhuang@arm.com>; reshma.pattan@intel.com;
> david.hunt@intel.com
> Subject: [21.08 PATCH v4 2/2] power: refactor pstate and acpi code
> 
> Currently, ACPI and PSTATE modes have lots of code duplication, confusing
> logic, and a bunch of other issues that can, and have, led to various bugs and
> resource leaks.
> 
> This commit factors out the common parts of sysfs reading/writing for ACPI
> and PSTATE drivers.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/power/meson.build            |   7 +
>  lib/power/power_acpi_cpufreq.c   | 178 +++--------------
>  lib/power/power_common.c         | 133 +++++++++++++
>  lib/power/power_common.h         |  46 +++++
>  lib/power/power_pstate_cpufreq.c | 332 ++++++++-----------------------
>  5 files changed, 293 insertions(+), 403 deletions(-)
> 
> diff --git a/lib/power/meson.build b/lib/power/meson.build index
> a2cc9fe2ef..85324d48d2 100644
> --- a/lib/power/meson.build
> +++ b/lib/power/meson.build
> @@ -5,6 +5,13 @@ if not is_linux
>      build = false
>      reason = 'only supported on Linux'
>  endif
> +
> +# we do some snprintf magic so silence format-nonliteral
> +flag_nonliteral = '-Wno-format-nonliteral'
> +if cc.has_argument(flag_nonliteral)
> +	cflags += flag_nonliteral
> +endif
> +
>  sources = files(
>          'guest_channel.c',
>          'power_acpi_cpufreq.c',
> diff --git a/lib/power/power_acpi_cpufreq.c
> b/lib/power/power_acpi_cpufreq.c index 1b8c69cc8b..97f1d302c9 100644
> --- a/lib/power/power_acpi_cpufreq.c
> +++ b/lib/power/power_acpi_cpufreq.c
> @@ -19,41 +19,10 @@
>  #include "power_acpi_cpufreq.h"
>  #include "power_common.h"
> 
> -#ifdef RTE_LIBRTE_POWER_DEBUG
> -#define POWER_DEBUG_TRACE(fmt, args...) do { \
> -		RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \
> -} while (0)
> -#else
> -#define POWER_DEBUG_TRACE(fmt, args...) -#endif
> -
> -#define FOPEN_OR_ERR_RET(f, retval) do { \
> -		if ((f) == NULL) { \
> -			RTE_LOG(ERR, POWER, "File not opened\n"); \
> -			return retval; \
> -		} \
> -} while (0)
> -
> -#define FOPS_OR_NULL_GOTO(ret, label) do { \
> -		if ((ret) == NULL) { \
> -			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
> -			goto label; \
> -		} \
> -} while (0)
> -
> -#define FOPS_OR_ERR_GOTO(ret, label) do { \
> -		if ((ret) < 0) { \
> -			RTE_LOG(ERR, POWER, "File operations failed\n"); \
> -			goto label; \
> -		} \
> -} while (0)
> -
>  #define STR_SIZE     1024
>  #define POWER_CONVERT_TO_DECIMAL 10
> 
>  #define POWER_GOVERNOR_USERSPACE "userspace"
> -#define POWER_SYSFILE_GOVERNOR   \
> -		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
>  #define POWER_SYSFILE_AVAIL_FREQ \
> 
> 	"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_frequen
> cies"
>  #define POWER_SYSFILE_SETSPEED   \
> @@ -135,53 +104,18 @@ set_freq_internal(struct acpi_power_info *pi,
> uint32_t idx)  static int  power_set_governor_userspace(struct
> acpi_power_info *pi)  {
> -	FILE *f;
> -	int ret = -1;
> -	char buf[BUFSIZ];
> -	char fullpath[PATH_MAX];
> -	char *s;
> -	int val;
> -
> -	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
> -			pi->lcore_id);
> -	f = fopen(fullpath, "rw+");
> -	FOPEN_OR_ERR_RET(f, ret);
> -
> -	s = fgets(buf, sizeof(buf), f);
> -	FOPS_OR_NULL_GOTO(s, out);
> -	/* Strip off terminating '\n' */
> -	strtok(buf, "\n");
> -
> -	/* Save the original governor */
> -	rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
> -
> -	/* Check if current governor is userspace */
> -	if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
> -			sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
> -		ret = 0;
> -		POWER_DEBUG_TRACE("Power management governor of
> lcore %u is "
> -				"already userspace\n", pi->lcore_id);
> -		goto out;
> -	}
> -
> -	/* Write 'userspace' to the governor */
> -	val = fseek(f, 0, SEEK_SET);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	val = fputs(POWER_GOVERNOR_USERSPACE, f);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	/* We need to flush to see if the fputs succeeds */
> -	val = fflush(f);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	ret = 0;
> -	RTE_LOG(INFO, POWER, "Power management governor of lcore %u
> has been "
> -			"set to user space successfully\n", pi->lcore_id);
> -out:
> -	fclose(f);
> -
> -	return ret;
> +	return power_set_governor(pi->lcore_id,
> POWER_GOVERNOR_USERSPACE,
> +			pi->governor_ori, sizeof(pi->governor_ori)); }
> +
> +/**
> + * It is to check the governor and then set the original governor back
> +if
> + * needed by writing the sys file.
> + */
> +static int
> +power_set_governor_original(struct acpi_power_info *pi) {
> +	return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
>  }
> 
>  /**
> @@ -195,22 +129,14 @@ power_get_available_freqs(struct
> acpi_power_info *pi)
>  	int ret = -1, i, count;
>  	char *p;
>  	char buf[BUFSIZ];
> -	char fullpath[PATH_MAX];
>  	char *freqs[RTE_MAX_LCORE_FREQS];
> -	char *s;
> 
> -	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ,
> -			pi->lcore_id);
> -	f = fopen(fullpath, "r");
> -	FOPEN_OR_ERR_RET(f, ret);
> +	open_core_sysfs_file(POWER_SYSFILE_AVAIL_FREQ, pi->lcore_id, "r",
> +			&f);
> +	FOPEN_OR_ERR_GOTO(f, out);
> 
> -	s = fgets(buf, sizeof(buf), f);
> -	FOPS_OR_NULL_GOTO(s, out);
> -
> -	/* Strip the line break if there is */
> -	p = strchr(buf, '\n');
> -	if (p != NULL)
> -		*p = 0;
> +	ret = read_core_sysfs_s(f, buf, sizeof(buf));
> +	FOPS_OR_ERR_GOTO(ret, out);
> 
>  	/* Split string into at most RTE_MAX_LCORE_FREQS frequencies */
>  	count = rte_strsplit(buf, sizeof(buf), freqs, @@ -250,7 +176,8 @@
> power_get_available_freqs(struct acpi_power_info *pi)
>  	POWER_DEBUG_TRACE("%d frequency(s) of lcore %u are
> available\n",
>  			count, pi->lcore_id);
>  out:
> -	fclose(f);
> +	if (f != NULL)
> +		fclose(f);
> 
>  	return ret;
>  }
> @@ -262,18 +189,16 @@ static int
>  power_init_for_setting_freq(struct acpi_power_info *pi)  {
>  	FILE *f;
> -	char fullpath[PATH_MAX];
>  	char buf[BUFSIZ];
>  	uint32_t i, freq;
> -	char *s;
> +	int ret;
> 
> -	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
> -			pi->lcore_id);
> -	f = fopen(fullpath, "rw+");
> -	FOPEN_OR_ERR_RET(f, -1);
> +	open_core_sysfs_file(POWER_SYSFILE_SETSPEED, pi->lcore_id, "r",
> +			&f);
Hi Anatoly,
I tried to verify your patch on my arm platform,  and I found several bugs.

Here it should be "rw+", for it will write freq to POWER_SYSFILE_SETSPEED later.

Best Regards,
Richael
> +	FOPEN_OR_ERR_GOTO(f, err);
> 
> -	s = fgets(buf, sizeof(buf), f);
> -	FOPS_OR_NULL_GOTO(s, out);
> +	ret = read_core_sysfs_s(f, buf, sizeof(buf));
> +	FOPS_OR_ERR_GOTO(ret, err);
> 
>  	freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
>  	for (i = 0; i < pi->nb_freqs; i++) {
> @@ -284,8 +209,9 @@ power_init_for_setting_freq(struct acpi_power_info
> *pi)
>  		}
>  	}
> 
> -out:
> -	fclose(f);
> +err:
> +	if (f != NULL)
> +		fclose(f);
> 
>  	return -1;
>  }
> @@ -369,54 +295,6 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
>  	return -1;
>  }
> 
> -/**
> - * It is to check the governor and then set the original governor back if
> - * needed by writing the sys file.
> - */
> -static int
> -power_set_governor_original(struct acpi_power_info *pi) -{
> -	FILE *f;
> -	int ret = -1;
> -	char buf[BUFSIZ];
> -	char fullpath[PATH_MAX];
> -	char *s;
> -	int val;
> -
> -	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
> -			pi->lcore_id);
> -	f = fopen(fullpath, "rw+");
> -	FOPEN_OR_ERR_RET(f, ret);
> -
> -	s = fgets(buf, sizeof(buf), f);
> -	FOPS_OR_NULL_GOTO(s, out);
> -
> -	/* Check if the governor to be set is the same as current */
> -	if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
> -		ret = 0;
> -		POWER_DEBUG_TRACE("Power management governor of
> lcore %u "
> -				"has already been set to %s\n",
> -				pi->lcore_id, pi->governor_ori);
> -		goto out;
> -	}
> -
> -	/* Write back the original governor */
> -	val = fseek(f, 0, SEEK_SET);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	val = fputs(pi->governor_ori, f);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	ret = 0;
> -	RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
> -			"has been set back to %s successfully\n",
> -			pi->lcore_id, pi->governor_ori);
> -out:
> -	fclose(f);
> -
> -	return ret;
> -}
> -
>  int
>  power_acpi_cpufreq_exit(unsigned int lcore_id)  { diff --git
> a/lib/power/power_common.c b/lib/power/power_common.c index
> 67e3318ec7..f06e9e5fa9 100644
> --- a/lib/power/power_common.c
> +++ b/lib/power/power_common.c
> @@ -3,13 +3,20 @@
>   */
> 
>  #include <limits.h>
> +#include <stdlib.h>
>  #include <stdio.h>
>  #include <string.h>
> 
> +#include <rte_log.h>
> +#include <rte_string_fns.h>
> +
>  #include "power_common.h"
> 
>  #define POWER_SYSFILE_SCALING_DRIVER   \
>  		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
> +#define POWER_SYSFILE_GOVERNOR  \
> +		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
> +#define POWER_CONVERT_TO_DECIMAL 10
> 
>  int
>  cpufreq_check_scaling_driver(const char *driver_name) @@ -58,3 +65,129
> @@ cpufreq_check_scaling_driver(const char *driver_name)
>  	 */
>  	return 1;
>  }
> +
> +int
> +open_core_sysfs_file(const char *template, unsigned int core, const char
> *mode,
> +		FILE **f)
> +{
> +	char fullpath[PATH_MAX];
> +	FILE *tmpf;
> +
> +	/* silenced -Wformat-nonliteral here */
> +	snprintf(fullpath, sizeof(fullpath), template, core);
> +	tmpf = fopen(fullpath, mode);
> +	if (tmpf == NULL)
> +		return -1;
> +	*f = tmpf;

When the file that open_core_sysfs_file() opens doesn't exist, there's segmentation fault.
Moving *f=tmpf above runs OK.

> +
> +	return 0;
> +}
> +
> +int
> +read_core_sysfs_u32(FILE *f, uint32_t *val) {
> +	char buf[BUFSIZ];
> +	uint32_t fval;
> +	char *s;
> +
> +	s = fgets(buf, sizeof(buf), f);
> +	if (s == NULL)
> +		return -1;
> +
> +	/* fgets puts null terminator in, but do this just in case */
> +	buf[BUFSIZ - 1] = '\0';
> +
> +	/* strip off any terminating newlines */
> +	*strchrnul(buf, '\n') = '\0';
> +
> +	fval = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
> +
> +	/* write the value */
> +	*val = fval;
> +
> +	return 0;
> +}
> +
> +int
> +read_core_sysfs_s(FILE *f, char *buf, unsigned int len) {
> +	char *s;
> +
> +	s = fgets(buf, len, f);
> +	if (s == NULL)
> +		return -1;
> +
> +	/* fgets puts null terminator in, but do this just in case */
> +	buf[len - 1] = '\0';
> +
> +	/* strip off any terminating newlines */
> +	*strchrnul(buf, '\n') = '\0';
> +
> +	return 0;
> +}
> +
> +int
> +write_core_sysfs_s(FILE *f, const char *str) {
> +	int ret;
> +
> +	ret = fseek(f, 0, SEEK_SET);
> +	if (ret != 0)
> +		return -1;
> +
> +	ret = fputs(str, f);
> +	if (ret != 0)

ret >=0 if success, EOF means failure.

> +		return -1;
> +
> +	/* flush the output */
> +	ret = fflush(f);
> +	if (ret != 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +/**
> + * It is to check the current scaling governor by reading sys file, and
> +then
> + * set it into 'performance' if it is not by writing the sys file. The
> +original
> + * governor will be saved for rolling back.
> + */
> +int
> +power_set_governor(unsigned int lcore_id, const char *new_governor,
> +		char *orig_governor, size_t orig_governor_len) {
> +	FILE *f_governor = NULL;
> +	int ret = -1;
> +	char buf[BUFSIZ];
> +
> +	open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, lcore_id, "rw+",
> &f_governor);
> +	FOPEN_OR_ERR_GOTO(f_governor, out);
> +
> +	ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
> +	FOPS_OR_ERR_GOTO(ret, out);
> +
> +	/* Save the original governor, if it was provided */
> +	if (orig_governor)
> +		rte_strscpy(orig_governor, buf, orig_governor_len);
> +
> +	/* Check if current governor is already what we want */
> +	if (strcmp(buf, new_governor) == 0) {
> +		ret = 0;
> +		POWER_DEBUG_TRACE("Power management governor of
> lcore %u is "
> +				"already %s\n", lcore_id, new_governor);
> +		goto out;
> +	}
> +
> +	/* Write the new governor */
> +	ret = write_core_sysfs_s(f_governor, new_governor);
> +	FOPS_OR_ERR_GOTO(ret, out);
> +
> +	ret = 0;
> +	RTE_LOG(INFO, POWER, "Power management governor of lcore %u
> has been "
> +			"set to '%s' successfully\n", lcore_id, new_governor);
> +out:
> +	if (f_governor != NULL)
> +		fclose(f_governor);
> +
> +	return ret;
> +}
> diff --git a/lib/power/power_common.h b/lib/power/power_common.h
> index fab3ca995a..729ae8cbf4 100644
> --- a/lib/power/power_common.h
> +++ b/lib/power/power_common.h
> @@ -5,9 +5,55 @@
>  #ifndef _POWER_COMMON_H_
>  #define _POWER_COMMON_H_
> 
> +#include <inttypes.h>
> +
>  #define RTE_POWER_INVALID_FREQ_INDEX (~0)
> 
> +
> +#ifdef RTE_LIBRTE_POWER_DEBUG
> +#define POWER_DEBUG_TRACE(fmt, args...) do { \
> +		RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \ }
> while (0)
> +#else #define POWER_DEBUG_TRACE(fmt, args...) #endif
> +
> +#define FOPEN_OR_ERR_RET(f, retval) do { \
> +		if ((f) == NULL) { \
> +			RTE_LOG(ERR, POWER, "File not opened\n"); \
> +			return retval; \
> +		} \
> +} while (0)
> +
> +#define FOPEN_OR_ERR_GOTO(f, label) do { \
> +		if ((f) == NULL) { \
> +			RTE_LOG(ERR, POWER, "File not opened\n"); \
> +			goto label; \
> +		} \
> +} while (0)
> +
> +#define FOPS_OR_NULL_GOTO(ret, label) do { \
> +		if ((ret) == NULL) { \
> +			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
> +			goto label; \
> +		} \
> +} while (0)
> +
> +#define FOPS_OR_ERR_GOTO(ret, label) do { \
> +		if ((ret) < 0) { \
> +			RTE_LOG(ERR, POWER, "File operations failed\n"); \
> +			goto label; \
> +		} \
> +} while (0)
> +
>  /* check if scaling driver matches one we want */  int
> cpufreq_check_scaling_driver(const char *driver);
> +int power_set_governor(unsigned int lcore_id, const char *new_governor,
> +		char *orig_governor, size_t orig_governor_len); int
> +open_core_sysfs_file(const char *template, unsigned int core,
> +			 const char *mode, FILE **f);
> +int read_core_sysfs_u32(FILE *f, uint32_t *val); int
> +read_core_sysfs_s(FILE *f, char *buf, unsigned int len); int
> +write_core_sysfs_s(FILE *f, const char *str);
> 
>  #endif /* _POWER_COMMON_H_ */
> diff --git a/lib/power/power_pstate_cpufreq.c
> b/lib/power/power_pstate_cpufreq.c
> index 2cfc54acf3..2a365988a9 100644
> --- a/lib/power/power_pstate_cpufreq.c
> +++ b/lib/power/power_pstate_cpufreq.c
> @@ -21,46 +21,13 @@
>  #include "power_pstate_cpufreq.h"
>  #include "power_common.h"
> 
> -
> -#ifdef RTE_LIBRTE_POWER_DEBUG
> -#define POWER_DEBUG_TRACE(fmt, args...) do { \
> -		RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \
> -} while (0)
> -#else
> -#define POWER_DEBUG_TRACE(fmt, args...) -#endif
> -
> -#define FOPEN_OR_ERR_RET(f, retval) do { \
> -		if ((f) == NULL) { \
> -			RTE_LOG(ERR, POWER, "File not opened\n"); \
> -			return retval; \
> -		} \
> -} while (0)
> -
> -#define FOPS_OR_NULL_GOTO(ret, label) do { \
> -		if ((ret) == NULL) { \
> -			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
> -			goto label; \
> -		} \
> -} while (0)
> -
> -#define FOPS_OR_ERR_GOTO(ret, label) do { \
> -		if ((ret) < 0) { \
> -			RTE_LOG(ERR, POWER, "File operations failed\n"); \
> -			goto label; \
> -		} \
> -} while (0)
> -
>  /* macros used for rounding frequency to nearest 100000 */  #define
> FREQ_ROUNDING_DELTA 50000  #define ROUND_FREQ_TO_N_100000
> 100000
> 
> -#define POWER_CONVERT_TO_DECIMAL 10
>  #define BUS_FREQ     100000
> 
>  #define POWER_GOVERNOR_PERF "performance"
> -#define POWER_SYSFILE_GOVERNOR  \
> -		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
>  #define POWER_SYSFILE_MAX_FREQ \
> 
> 	"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_max_freq"
>  #define POWER_SYSFILE_MIN_FREQ  \
> @@ -154,91 +121,58 @@ out:	close(fd);
>  static int
>  power_init_for_setting_freq(struct pstate_power_info *pi)  {
> -	FILE *f_min, *f_max, *f_base = NULL, *f_base_max;
> -	char fullpath_min[PATH_MAX];
> -	char fullpath_max[PATH_MAX];
> -	char fullpath_base[PATH_MAX];
> -	char fullpath_base_max[PATH_MAX];
> -	char buf_base[BUFSIZ];
> -	char *s_base;
> -	char *s_base_max;
> -	uint32_t base_ratio = 0;
> -	uint32_t base_max_ratio = 0;
> -	uint64_t max_non_turbo = 0;
> -	int  ret_val = 0;
> -
> -	snprintf(fullpath_base_max,
> -			sizeof(fullpath_base_max),
> -			POWER_SYSFILE_BASE_MAX_FREQ,
> -			pi->lcore_id);
> -	f_base_max = fopen(fullpath_base_max, "r");
> -	FOPEN_OR_ERR_RET(f_base_max, -1);
> -	if (f_base_max != NULL) {
> -		s_base_max = fgets(buf_base, sizeof(buf_base),
> f_base_max);
> -
> -		/* close the file unconditionally */
> -		fclose(f_base_max);
> -		f_base_max = NULL;
> -
> -		FOPS_OR_NULL_GOTO(s_base_max, out);
> -
> -		buf_base[BUFSIZ-1] = '\0';
> -		if (strlen(buf_base))
> -			/* Strip off terminating '\n' */
> -			strtok(buf_base, "\n");
> -
> -		base_max_ratio =
> -			strtoul(buf_base, NULL,
> POWER_CONVERT_TO_DECIMAL)
> -				/ BUS_FREQ;
> -	}
> -
> -	snprintf(fullpath_min, sizeof(fullpath_min),
> POWER_SYSFILE_MIN_FREQ,
> -			pi->lcore_id);
> -	f_min = fopen(fullpath_min, "rw+");
> -	FOPEN_OR_ERR_RET(f_min, -1);
> -
> -	snprintf(fullpath_max, sizeof(fullpath_max),
> POWER_SYSFILE_MAX_FREQ,
> -			pi->lcore_id);
> -	f_max = fopen(fullpath_max, "rw+");
> -	if (f_max == NULL)
> -		fclose(f_min);
> -	FOPEN_OR_ERR_RET(f_max, -1);
> -
> -	pi->f_cur_min = f_min;
> -	pi->f_cur_max = f_max;
> -
> -	snprintf(fullpath_base, sizeof(fullpath_base),
> POWER_SYSFILE_BASE_FREQ,
> -			pi->lcore_id);
> -
> -	f_base = fopen(fullpath_base, "r");
> -	if (f_base == NULL) {
> -		/* No sysfs base_frequency, that's OK, continue without */
> -		base_ratio = 0;
> +	FILE *f_base = NULL, *f_base_max = NULL, *f_min = NULL, *f_max =
> NULL;
> +	uint32_t base_ratio, base_max_ratio;
> +	uint64_t max_non_turbo;
> +	int ret;
> +
> +	/* open all files we expect to have open */
> +	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi-
> >lcore_id, "r",
> +			&f_base_max);
> +	FOPEN_OR_ERR_GOTO(f_base_max, err);
> +
> +	open_core_sysfs_file(POWER_SYSFILE_MIN_FREQ, pi->lcore_id,
> "rw+",
> +			&f_min);
> +	FOPEN_OR_ERR_GOTO(f_min, err);
> +
> +	open_core_sysfs_file(POWER_SYSFILE_MAX_FREQ, pi->lcore_id,
> "rw+",
> +			&f_max);
> +	FOPEN_OR_ERR_GOTO(f_max, err);
> +
> +	open_core_sysfs_file(POWER_SYSFILE_BASE_FREQ, pi->lcore_id, "r",
> +			&f_base);
> +	/* base ratio file may not exist in some kernels, so no error check */
> +
> +	/* read base max ratio */
> +	ret = read_core_sysfs_u32(f_base_max, &base_max_ratio);
> +	FOPS_OR_ERR_GOTO(ret, err);
> +
> +	/* base ratio may not exist */
> +	if (f_base != NULL) {
> +		ret = read_core_sysfs_u32(f_base, &base_ratio);
> +		FOPS_OR_ERR_GOTO(ret, err);
>  	} else {
> -		s_base = fgets(buf_base, sizeof(buf_base), f_base);
> -		FOPS_OR_NULL_GOTO(s_base, out);
> -
> -		buf_base[BUFSIZ-1] = '\0';
> -		if (strlen(buf_base))
> -			/* Strip off terminating '\n' */
> -			strtok(buf_base, "\n");
> -
> -		base_ratio = strtoul(buf_base, NULL,
> POWER_CONVERT_TO_DECIMAL)
> -				/ BUS_FREQ;
> +		base_ratio = 0;
>  	}
> 
>  	/* Add MSR read to detect turbo status */
> +	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) <
> 0)
> +		goto err;
> +	/* no errors after this point */
> 
> -	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) <
> 0) {
> -		ret_val = -1;
> -		goto out;
> -	}
> +	/* convert ratios to bins */
> +	base_max_ratio /= BUS_FREQ;
> +	base_ratio /= BUS_FREQ;
> +
> +	/* assign file handles */
> +	pi->f_cur_min = f_min;
> +	pi->f_cur_max = f_max;
> 
>  	max_non_turbo =
> (max_non_turbo&NON_TURBO_MASK)>>NON_TURBO_OFFSET;
> 
>  	POWER_DEBUG_TRACE("no turbo perf %"PRIu64"\n",
> max_non_turbo);
> 
> -	pi->non_turbo_max_ratio = max_non_turbo;
> +	pi->non_turbo_max_ratio = (uint32_t)max_non_turbo;
> 
>  	/*
>  	 * If base_frequency is reported as greater than the maximum @@ -
> 264,7 +198,20 @@ power_init_for_setting_freq(struct pstate_power_info
> *pi)
>  out:
>  	if (f_base != NULL)
>  		fclose(f_base);
> -	return ret_val;
> +	fclose(f_base_max);
> +	/* f_min and f_max are stored, no need to close */
> +	return 0;
> +
> +err:
> +	if (f_base != NULL)
> +		fclose(f_base);
> +	if (f_base_max != NULL)
> +		fclose(f_base_max);
> +	if (f_min != NULL)
> +		fclose(f_min);
> +	if (f_max != NULL)
> +		fclose(f_max);
> +	return -1;
>  }
> 
>  static int
> @@ -369,53 +316,8 @@ set_freq_internal(struct pstate_power_info *pi,
> uint32_t idx)  static int  power_set_governor_performance(struct
> pstate_power_info *pi)  {
> -	FILE *f;
> -	int ret = -1;
> -	char buf[BUFSIZ];
> -	char fullpath[PATH_MAX];
> -	char *s;
> -	int val;
> -
> -	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
> -			pi->lcore_id);
> -	f = fopen(fullpath, "rw+");
> -	FOPEN_OR_ERR_RET(f, ret);
> -
> -	s = fgets(buf, sizeof(buf), f);
> -	FOPS_OR_NULL_GOTO(s, out);
> -	/* Strip off terminating '\n' */
> -	strtok(buf, "\n");
> -
> -	/* Save the original governor */
> -	rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
> -
> -	/* Check if current governor is performance */
> -	if (strncmp(buf, POWER_GOVERNOR_PERF,
> -			sizeof(POWER_GOVERNOR_PERF)) == 0) {
> -		ret = 0;
> -		POWER_DEBUG_TRACE("Power management governor of
> lcore %u is "
> -				"already performance\n", pi->lcore_id);
> -		goto out;
> -	}
> -
> -	/* Write 'performance' to the governor */
> -	val = fseek(f, 0, SEEK_SET);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	val = fputs(POWER_GOVERNOR_PERF, f);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	/* We need to flush to see if the fputs succeeds */
> -	val = fflush(f);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	ret = 0;
> -	RTE_LOG(INFO, POWER, "Power management governor of lcore %u
> has been "
> -			"set to performance successfully\n", pi->lcore_id);
> -out:
> -	fclose(f);
> -
> -	return ret;
> +	return power_set_governor(pi->lcore_id, POWER_GOVERNOR_PERF,
> +			pi->governor_ori, sizeof(pi->governor_ori));
>  }
> 
>  /**
> @@ -425,45 +327,7 @@ power_set_governor_performance(struct
> pstate_power_info *pi)  static int  power_set_governor_original(struct
> pstate_power_info *pi)  {
> -	FILE *f;
> -	int ret = -1;
> -	char buf[BUFSIZ];
> -	char fullpath[PATH_MAX];
> -	char *s;
> -	int val;
> -
> -	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
> -			pi->lcore_id);
> -	f = fopen(fullpath, "rw+");
> -	FOPEN_OR_ERR_RET(f, ret);
> -
> -	s = fgets(buf, sizeof(buf), f);
> -	FOPS_OR_NULL_GOTO(s, out);
> -
> -	/* Check if the governor to be set is the same as current */
> -	if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
> -		ret = 0;
> -		POWER_DEBUG_TRACE("Power management governor of
> lcore %u "
> -				"has already been set to %s\n",
> -				pi->lcore_id, pi->governor_ori);
> -		goto out;
> -	}
> -
> -	/* Write back the original governor */
> -	val = fseek(f, 0, SEEK_SET);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	val = fputs(pi->governor_ori, f);
> -	FOPS_OR_ERR_GOTO(val, out);
> -
> -	ret = 0;
> -	RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
> -			"has been set back to %s successfully\n",
> -			pi->lcore_id, pi->governor_ori);
> -out:
> -	fclose(f);
> -
> -	return ret;
> +	return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
>  }
> 
>  /**
> @@ -473,51 +337,26 @@ power_set_governor_original(struct
> pstate_power_info *pi)  static int  power_get_available_freqs(struct
> pstate_power_info *pi)  {
> -	FILE *f_min, *f_max;
> +	FILE *f_min = NULL, *f_max = NULL;
>  	int ret = -1;
> -	char *p_min, *p_max;
> -	char buf_min[BUFSIZ];
> -	char buf_max[BUFSIZ];
> -	char fullpath_min[PATH_MAX];
> -	char fullpath_max[PATH_MAX];
> -	char *s_min, *s_max;
>  	uint32_t sys_min_freq = 0, sys_max_freq = 0, base_max_freq = 0;
>  	uint32_t i, num_freqs = 0;
> 
> -	snprintf(fullpath_max, sizeof(fullpath_max),
> -			POWER_SYSFILE_BASE_MAX_FREQ,
> -			pi->lcore_id);
> -	snprintf(fullpath_min, sizeof(fullpath_min),
> -			POWER_SYSFILE_BASE_MIN_FREQ,
> -			pi->lcore_id);
> +	/* open all files */
> +	open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi-
> >lcore_id, "r",
> +			&f_max);
> +	FOPEN_OR_ERR_GOTO(f_max, out);
> 
> -	f_min = fopen(fullpath_min, "r");
> -	FOPEN_OR_ERR_RET(f_min, ret);
> +	open_core_sysfs_file(POWER_SYSFILE_BASE_MIN_FREQ, pi->lcore_id,
> "r",
> +			     &f_min);
> +	FOPEN_OR_ERR_GOTO(f_max, out);
> 
> -	f_max = fopen(fullpath_max, "r");
> -	if (f_max == NULL)
> -		fclose(f_min);
> +	/* read base ratios */
> +	ret = read_core_sysfs_u32(f_max, &sys_max_freq);
> +	FOPS_OR_ERR_GOTO(ret, out);
> 
> -	FOPEN_OR_ERR_RET(f_max, ret);
> -
> -	s_min = fgets(buf_min, sizeof(buf_min), f_min);
> -	FOPS_OR_NULL_GOTO(s_min, out);
> -
> -	s_max = fgets(buf_max, sizeof(buf_max), f_max);
> -	FOPS_OR_NULL_GOTO(s_max, out);
> -
> -
> -	/* Strip the line break if there is */
> -	p_min = strchr(buf_min, '\n');
> -	if (p_min != NULL)
> -		*p_min = 0;
> -
> -	p_max = strchr(buf_max, '\n');
> -	if (p_max != NULL)
> -		*p_max = 0;
> -
> -	sys_min_freq = strtoul(buf_min, &p_min,
> POWER_CONVERT_TO_DECIMAL);
> -	sys_max_freq = strtoul(buf_max, &p_max,
> POWER_CONVERT_TO_DECIMAL);
> +	ret = read_core_sysfs_u32(f_min, &sys_min_freq);
> +	FOPS_OR_ERR_GOTO(ret, out);
> 
>  	if (sys_max_freq < sys_min_freq)
>  		goto out;
> @@ -576,27 +415,14 @@ power_get_cur_idx(struct pstate_power_info *pi)
> {
>  	FILE *f_cur;
>  	int ret = -1;
> -	char *p_cur;
> -	char buf_cur[BUFSIZ];
> -	char fullpath_cur[PATH_MAX];
> -	char *s_cur;
>  	uint32_t sys_cur_freq = 0;
>  	unsigned int i;
> 
> -	snprintf(fullpath_cur, sizeof(fullpath_cur),
> -			POWER_SYSFILE_CUR_FREQ,
> -			pi->lcore_id);
> -	f_cur = fopen(fullpath_cur, "r");
> -	FOPEN_OR_ERR_RET(f_cur, ret);
> +	open_core_sysfs_file(POWER_SYSFILE_CUR_FREQ, pi->lcore_id, "r",
> &f_cur);
> +	FOPEN_OR_ERR_GOTO(f_cur, fail);
> 
> -	/* initialize the cur_idx to matching current frequency freq index */
> -	s_cur = fgets(buf_cur, sizeof(buf_cur), f_cur);
> -	FOPS_OR_NULL_GOTO(s_cur, fail);
> -
> -	p_cur = strchr(buf_cur, '\n');
> -	if (p_cur != NULL)
> -		*p_cur = 0;
> -	sys_cur_freq = strtoul(buf_cur, &p_cur,
> POWER_CONVERT_TO_DECIMAL);
> +	ret = read_core_sysfs_u32(f_cur, &sys_cur_freq);
> +	FOPS_OR_ERR_GOTO(ret, fail);
> 
>  	/* convert the frequency to nearest 100000 value
>  	 * Ex: if sys_cur_freq=1396789 then freq_conv=1400000 @@ -615,10
> +441,10 @@ power_get_cur_idx(struct pstate_power_info *pi)
>  		}
>  	}
> 
> -	fclose(f_cur);
> -	return 0;
> +	ret = 0;
>  fail:
> -	fclose(f_cur);
> +	if (f_cur != NULL)
> +		fclose(f_cur);
>  	return ret;
>  }
> 
> --
> 2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [21.08 PATCH v4 2/2] power: refactor pstate and acpi code
  2021-05-07  2:13         ` Richael Zhuang
@ 2021-05-07  9:49           ` Burakov, Anatoly
  0 siblings, 0 replies; 12+ messages in thread
From: Burakov, Anatoly @ 2021-05-07  9:49 UTC (permalink / raw)
  To: Richael Zhuang, dev; +Cc: stephen, reshma.pattan, david.hunt, nd

On 07-May-21 3:13 AM, Richael Zhuang wrote:
>> @@ -262,18 +189,16 @@ static int
>>   power_init_for_setting_freq(struct acpi_power_info *pi)  {
>>   	FILE *f;
>> -	char fullpath[PATH_MAX];
>>   	char buf[BUFSIZ];
>>   	uint32_t i, freq;
>> -	char *s;
>> +	int ret;
>>
>> -	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
>> -			pi->lcore_id);
>> -	f = fopen(fullpath, "rw+");
>> -	FOPEN_OR_ERR_RET(f, -1);
>> +	open_core_sysfs_file(POWER_SYSFILE_SETSPEED, pi->lcore_id, "r",
>> +			&f);
> Hi Anatoly,
> I tried to verify your patch on my arm platform,  and I found several bugs.
> 
> Here it should be "rw+", for it will write freq to POWER_SYSFILE_SETSPEED later.
> 
> Best Regards,
> Richael

<snip>

>>   	return 1;
>>   }
>> +
>> +int
>> +open_core_sysfs_file(const char *template, unsigned int core, const char
>> *mode,
>> +		FILE **f)
>> +{
>> +	char fullpath[PATH_MAX];
>> +	FILE *tmpf;
>> +
>> +	/* silenced -Wformat-nonliteral here */
>> +	snprintf(fullpath, sizeof(fullpath), template, core);
>> +	tmpf = fopen(fullpath, mode);
>> +	if (tmpf == NULL)
>> +		return -1;
>> +	*f = tmpf;
> 
> When the file that open_core_sysfs_file() opens doesn't exist, there's segmentation fault.
> Moving *f=tmpf above runs OK.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +read_core_sysfs_u32(FILE *f, uint32_t *val) {
>> +	char buf[BUFSIZ];
>> +	uint32_t fval;
>> +	char *s;
>> +
>> +	s = fgets(buf, sizeof(buf), f);
>> +	if (s == NULL)
>> +		return -1;
>> +

<snip>

>> +	/* strip off any terminating newlines */
>> +	*strchrnul(buf, '\n') = '\0';
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +write_core_sysfs_s(FILE *f, const char *str) {
>> +	int ret;
>> +
>> +	ret = fseek(f, 0, SEEK_SET);
>> +	if (ret != 0)
>> +		return -1;
>> +
>> +	ret = fputs(str, f);
>> +	if (ret != 0)
> 
> ret >=0 if success, EOF means failure.
> 

Hi Richael,

Thank you very much for testing! I'll address these issues in the next 
revision, and double-check everything else.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [21.08 PATCH v4 1/2] power: don't use rte prefix in internal code
  2021-04-23 11:03       ` [dpdk-dev] [21.08 PATCH v4 1/2] power: don't use rte prefix in internal code Anatoly Burakov
@ 2021-05-31 10:23         ` David Hunt
  0 siblings, 0 replies; 12+ messages in thread
From: David Hunt @ 2021-05-31 10:23 UTC (permalink / raw)
  To: Anatoly Burakov, dev; +Cc: stephen, Richael.Zhuang, reshma.pattan

Hi Anatoly

On 23/4/2021 12:03 PM, Anatoly Burakov wrote:
> Currently, ACPI code uses rte_power_info as the struct name, which
> gives the appearance that this is an externally visible API. Fix to
> use internal namespace.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   lib/power/power_acpi_cpufreq.c | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
>

Looks good. This brings the acpi driver code in line with the pstate 
driver code, which already
has it's struct called ptate_power_info.

Acked-by: David Hunt <david.hunt@intel.com>



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-05-31 10:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01 15:05 [dpdk-dev] [PATCH 21.08] power: refactor pstate sysfs handling Anatoly Burakov
2021-04-02  9:27 ` [dpdk-dev] [21.08 v2] " Anatoly Burakov
2021-04-02 17:45 ` [dpdk-dev] [PATCH 21.08] " Stephen Hemminger
2021-04-06 10:06   ` Burakov, Anatoly
2021-04-02 17:46 ` Stephen Hemminger
2021-04-06 10:05   ` Burakov, Anatoly
2021-04-22 15:08     ` [dpdk-dev] [21.08 PATCH v3 1/1] " Anatoly Burakov
2021-04-23 11:03       ` [dpdk-dev] [21.08 PATCH v4 1/2] power: don't use rte prefix in internal code Anatoly Burakov
2021-05-31 10:23         ` David Hunt
2021-04-23 11:03       ` [dpdk-dev] [21.08 PATCH v4 2/2] power: refactor pstate and acpi code Anatoly Burakov
2021-05-07  2:13         ` Richael Zhuang
2021-05-07  9:49           ` Burakov, Anatoly

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git