On Wed, Jun 23, 2021 at 2:04 PM David Hunt [off-list ref] wrote:

quoted

From: Anatoly Burakov <redacted>

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 <redacted>
Signed-off-by: David Hunt <redacted>

---
changes in v5
* fixed bugs raised by Richael Zhuang in review - open file rw+, etc.
* removed FOPS* and FOPEN* macros, which contained control statements.
* fixed some checkpatch warnings.
changes in v6
* fixed check of fputs return, negative on error.
---
  lib/power/meson.build            |   7 +
  lib/power/power_acpi_cpufreq.c   | 192 ++++------------
  lib/power/power_common.c         | 146 ++++++++++++
  lib/power/power_common.h         |  17 ++
  lib/power/power_pstate_cpufreq.c | 374 ++++++++++---------------------
  5 files changed, 335 insertions(+), 401 deletions(-)

diff --git a/lib/power/meson.build b/lib/power/meson.build
index c1097d32f1..74c5f3a294 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
+

This can be removed with __rte_format_printf tag + API change below.

quoted

  sources = files(
          'guest_channel.c',
          'power_acpi_cpufreq.c',

[snip]

quoted

diff --git a/lib/power/power_common.c b/lib/power/power_common.c
index 67e3318ec7..4deb343dae 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,142 @@ cpufreq_check_scaling_driver(const char *driver_name)
          */
         return 1;
  }

cpufreq_check_scaling_driver can use open_core_sysfs_file, right?

quoted

+
+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);
+       *f = tmpf;
+       if (tmpf == NULL)
+               return -1;
+
+       return 0;
+}

@@ -67,14 +67,15 @@ cpufreq_check_scaling_driver(const char *driver_name)
  }

  int
-open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
-               FILE **f)
+open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
  {
         char fullpath[PATH_MAX];
+       va_list ap;
         FILE *tmpf;

-       /* silenced -Wformat-nonliteral here */
-       snprintf(fullpath, sizeof(fullpath), template, core);
+       va_start(ap, format);
+       vsnprintf(fullpath, sizeof(fullpath), format, ap);
+       va_end(ap);
         tmpf = fopen(fullpath, mode);
         *f = tmpf;
         if (tmpf == NULL)

With declaration in .h as:

@@ -7,6 +7,8 @@

  #include <inttypes.h>

+#include <rte_common.h>
+
  #define RTE_POWER_INVALID_FREQ_INDEX (~0)

@@ -21,8 +23,8 @@
  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 open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
+               __rte_format_printf(3, 4);
  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);

This leaves the possibility to use any kind of formats.
And to be honest, I did not manage to make gcc happy otherwise (even
when passing __rte_format_printf(3, 0)).