From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 74197A0C48; Thu, 8 Jul 2021 15:33:48 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3871240696; Thu, 8 Jul 2021 15:33:48 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mails.dpdk.org (Postfix) with ESMTP id 837784014F for ; Thu, 8 Jul 2021 15:33:46 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10038"; a="209546691" X-IronPort-AV: E=Sophos;i="5.84,222,1620716400"; d="scan'208";a="209546691" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jul 2021 06:33:22 -0700 X-IronPort-AV: E=Sophos;i="5.84,222,1620716400"; d="scan'208";a="498426833" Received: from dhunt5-mobl5.ger.corp.intel.com (HELO [10.252.25.85]) ([10.252.25.85]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jul 2021 06:33:21 -0700 To: David Marchand , "Burakov, Anatoly" Cc: dev , Richael.Zhuang@arm.com, Stephen Hemminger , "Pattan, Reshma" , nd References: <20210622125853.2798-1-david.hunt@intel.com> <20210623120342.36321-1-david.hunt@intel.com> <20210623120342.36321-2-david.hunt@intel.com> From: David Hunt Message-ID: Date: Thu, 8 Jul 2021 14:33:19 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [dpdk-dev] [PATCH v6 2/2] power: refactor pstate and acpi code X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 8/7/2021 1:49 PM, David Marchand wrote: > On Wed, Jun 23, 2021 at 2:04 PM David Hunt wrote: >> From: Anatoly Burakov >> >> 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 >> Signed-off-by: David Hunt >> >> --- >> 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. > > >> sources = files( >> 'guest_channel.c', >> 'power_acpi_cpufreq.c', > [snip] > >> 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 >> +#include >> #include >> #include >> >> +#include >> +#include >> + >> #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? > > >> + >> +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 > > +#include > + > #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)). Thanks, David, good suggestions.  I'll make those changes and respin. Rgds, Dave. >