On Mon, 2019-09-30 at 11:18 -0400, Prarit Bhargava wrote: > > On 9/26/19 4:21 PM, Srinivas Pandruvada wrote: > > On Thu, 2019-09-26 at 08:54 -0400, Prarit Bhargava wrote: > > > The current code structure has similar but separate command > > > functions > > > for > > > the enable and disable operations. This can be improved by > > > adding an > > > int > > > argument to the command function structure, and interpreting 1 as > > > enable > > > and 0 as disable. This change results in the removal of the > > > disable > > > command functions. > > > > > > Add int argument to the command function structure. > > > > Does this brings in any significant reduction in data or code size? > > It reduces code size. Right now you have separate functions for > enable & > disable. These are unified into single functions. It reduce by 300 bytes. My logic is the command processor functions are responsible for doing what is required. After 5 years someone adding a command, don't need to understand the meaning of additional argument. IMO let's first focus on adding CLX-N support, this is I guess the aim of this series. Then we will work on some cleanup. I can't apply these patches for test. If you can use http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/review-andy as the baseline. Thanks, Srinivas > > P. > > > My focus is to add features first which helps users. > > > > Thanks, > > Srinivas > > > > > > > > > > Signed-off-by: Prarit Bhargava <prarit@xxxxxxxxxx> > > > Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> > > > --- > > > .../x86/intel-speed-select/isst-config.c | 184 +++++++----- > > > ---- > > > -- > > > 1 file changed, 69 insertions(+), 115 deletions(-) > > > > > > diff --git a/tools/power/x86/intel-speed-select/isst-config.c > > > b/tools/power/x86/intel-speed-select/isst-config.c > > > index 2a9890c8395a..9f2798caead9 100644 > > > --- a/tools/power/x86/intel-speed-select/isst-config.c > > > +++ b/tools/power/x86/intel-speed-select/isst-config.c > > > @@ -11,7 +11,8 @@ > > > struct process_cmd_struct { > > > char *feature; > > > char *command; > > > - void (*process_fn)(void); > > > + void (*process_fn)(int arg); > > > + int arg; > > > }; > > > > > > static const char *version_str = "v1.0"; > > > @@ -678,7 +679,7 @@ static void exec_on_get_ctdp_cpu(int cpu, > > > void > > > *arg1, void *arg2, void *arg3, > > > } > > > > > > #define _get_tdp_level(desc, suffix, object, > > > help) \ > > > - static void > > > get_tdp_##object(void) \ > > > + static void get_tdp_##object(int > > > arg) \ > > > { > > > \ > > > struct isst_pkg_ctdp > > > ctdp; \ > > > \ > > > @@ -724,7 +725,7 @@ static void dump_isst_config_for_cpu(int cpu, > > > void *arg1, void *arg2, > > > } > > > } > > > > > > -static void dump_isst_config(void) > > > +static void dump_isst_config(int arg) > > > { > > > if (cmd_help) { > > > fprintf(stderr, > > > @@ -787,7 +788,7 @@ static void set_tdp_level_for_cpu(int cpu, > > > void > > > *arg1, void *arg2, void *arg3, > > > } > > > } > > > > > > -static void set_tdp_level(void) > > > +static void set_tdp_level(int arg) > > > { > > > if (cmd_help) { > > > fprintf(stderr, "Set Config TDP level\n"); > > > @@ -827,7 +828,7 @@ static void dump_pbf_config_for_cpu(int cpu, > > > void > > > *arg1, void *arg2, void *arg3, > > > } > > > } > > > > > > -static void dump_pbf_config(void) > > > +static void dump_pbf_config(int arg) > > > { > > > if (cmd_help) { > > > fprintf(stderr, > > > @@ -871,43 +872,27 @@ static void set_pbf_for_cpu(int cpu, void > > > *arg1, void *arg2, void *arg3, > > > } > > > } > > > > > > -static void set_pbf_enable(void) > > > -{ > > > - int status = 1; > > > - > > > - if (cmd_help) { > > > - fprintf(stderr, > > > - "Enable Intel Speed Select Technology base > > > frequency feature [No command arguments are required]\n"); > > > - exit(0); > > > - } > > > - > > > - isst_ctdp_display_information_start(outf); > > > - if (max_target_cpus) > > > - for_each_online_target_cpu_in_set(set_pbf_for_cpu, > > > NULL, NULL, > > > - NULL, &status); > > > - else > > > - for_each_online_package_in_set(set_pbf_for_cpu, NULL, > > > NULL, > > > - NULL, &status); > > > - isst_ctdp_display_information_end(outf); > > > -} > > > - > > > -static void set_pbf_disable(void) > > > +static void set_pbf_enable(int arg) > > > { > > > - int status = 0; > > > + int enable = arg; > > > > > > if (cmd_help) { > > > - fprintf(stderr, > > > - "Disable Intel Speed Select Technology base > > > frequency feature [No command arguments are required]\n"); > > > + if (enable) > > > + fprintf(stderr, > > > + "Enable Intel Speed Select Technology > > > base frequency feature [No command arguments are required]\n"); > > > + else > > > + fprintf(stderr, > > > + "Disable Intel Speed Select Technology > > > base frequency feature [No command arguments are required]\n"); > > > exit(0); > > > } > > > > > > isst_ctdp_display_information_start(outf); > > > if (max_target_cpus) > > > for_each_online_target_cpu_in_set(set_pbf_for_cpu, > > > NULL, NULL, > > > - NULL, &status); > > > + NULL, &enable); > > > else > > > for_each_online_package_in_set(set_pbf_for_cpu, NULL, > > > NULL, > > > - NULL, &status); > > > + NULL, &enable); > > > isst_ctdp_display_information_end(outf); > > > } > > > > > > @@ -925,7 +910,7 @@ static void dump_fact_config_for_cpu(int cpu, > > > void *arg1, void *arg2, > > > fact_avx, &fact_info); > > > } > > > > > > -static void dump_fact_config(void) > > > +static void dump_fact_config(int arg) > > > { > > > if (cmd_help) { > > > fprintf(stderr, > > > @@ -985,35 +970,17 @@ static void set_fact_for_cpu(int cpu, void > > > *arg1, void *arg2, void *arg3, > > > } > > > } > > > > > > -static void set_fact_enable(void) > > > +static void set_fact_enable(int arg) > > > { > > > - int status = 1; > > > + int enable = arg; > > > > > > if (cmd_help) { > > > - fprintf(stderr, > > > - "Enable Intel Speed Select Technology Turbo > > > frequency feature\n"); > > > - fprintf(stderr, > > > - "Optional: -t|--trl : Specify turbo ratio > > > limit\n"); > > > - exit(0); > > > - } > > > - > > > - isst_ctdp_display_information_start(outf); > > > - if (max_target_cpus) > > > - for_each_online_target_cpu_in_set(set_fact_for_cpu, > > > NULL, NULL, > > > - NULL, &status); > > > - else > > > - for_each_online_package_in_set(set_fact_for_cpu, NULL, > > > NULL, > > > - NULL, &status); > > > - isst_ctdp_display_information_end(outf); > > > -} > > > - > > > -static void set_fact_disable(void) > > > -{ > > > - int status = 0; > > > - > > > - if (cmd_help) { > > > - fprintf(stderr, > > > - "Disable Intel Speed Select Technology turbo > > > frequency feature\n"); > > > + if (enable) > > > + fprintf(stderr, > > > + "Enable Intel Speed Select Technology > > > Turbo frequency feature\n"); > > > + else > > > + fprintf(stderr, > > > + "Disable Intel Speed Select Technology > > > turbo frequency feature\n"); > > > fprintf(stderr, > > > "Optional: -t|--trl : Specify turbo ratio > > > limit\n"); > > > exit(0); > > > @@ -1022,10 +989,10 @@ static void set_fact_disable(void) > > > isst_ctdp_display_information_start(outf); > > > if (max_target_cpus) > > > for_each_online_target_cpu_in_set(set_fact_for_cpu, > > > NULL, NULL, > > > - NULL, &status); > > > + NULL, &enable); > > > else > > > for_each_online_package_in_set(set_fact_for_cpu, NULL, > > > NULL, > > > - NULL, &status); > > > + NULL, &enable); > > > isst_ctdp_display_information_end(outf); > > > } > > > > > > @@ -1048,19 +1015,25 @@ static void enable_clos_qos_config(int > > > cpu, > > > void *arg1, void *arg2, void *arg3, > > > } > > > } > > > > > > -static void set_clos_enable(void) > > > +static void set_clos_enable(int arg) > > > { > > > - int status = 1; > > > + int enable = arg; > > > > > > if (cmd_help) { > > > - fprintf(stderr, "Enable core-power for a > > > package/die\n"); > > > - fprintf(stderr, > > > - "\tClos Enable: Specify priority type with [ > > > --priority|-p]\n"); > > > - fprintf(stderr, "\t\t 0: Proportional, 1: Ordered\n"); > > > + if (enable) { > > > + fprintf(stderr, > > > + "Enable core-power for a > > > package/die\n"); > > > + fprintf(stderr, > > > + "\tClos Enable: Specify priority type > > > with [--priority|-p]\n"); > > > + fprintf(stderr, "\t\t 0: Proportional, 1: > > > Ordered\n"); > > > + } else { > > > + fprintf(stderr, > > > + "Disable core-power: [No command > > > arguments are required]\n"); > > > + } > > > exit(0); > > > } > > > > > > - if (cpufreq_sysfs_present()) { > > > + if (enable && cpufreq_sysfs_present()) { > > > fprintf(stderr, > > > "cpufreq subsystem and core-power enable will > > > interfere with each other!\n"); > > > } > > > @@ -1068,30 +1041,10 @@ static void set_clos_enable(void) > > > isst_ctdp_display_information_start(outf); > > > if (max_target_cpus) > > > for_each_online_target_cpu_in_set(enable_clos_qos_confi > > > g, NULL, > > > - NULL, NULL, &status); > > > - else > > > - for_each_online_package_in_set(enable_clos_qos_config, > > > NULL, > > > - NULL, NULL, &status); > > > - isst_ctdp_display_information_end(outf); > > > -} > > > - > > > -static void set_clos_disable(void) > > > -{ > > > - int status = 0; > > > - > > > - if (cmd_help) { > > > - fprintf(stderr, > > > - "Disable core-power: [No command arguments are > > > required]\n"); > > > - exit(0); > > > - } > > > - > > > - isst_ctdp_display_information_start(outf); > > > - if (max_target_cpus) > > > - for_each_online_target_cpu_in_set(enable_clos_qos_confi > > > g, NULL, > > > - NULL, NULL, &status); > > > + NULL, NULL, &enable); > > > else > > > for_each_online_package_in_set(enable_clos_qos_config, > > > NULL, > > > - NULL, NULL, &status); > > > + NULL, NULL, &enable); > > > isst_ctdp_display_information_end(outf); > > > } > > > > > > @@ -1109,7 +1062,7 @@ static void dump_clos_config_for_cpu(int > > > cpu, > > > void *arg1, void *arg2, > > > &clos_config); > > > } > > > > > > -static void dump_clos_config(void) > > > +static void dump_clos_config(int arg) > > > { > > > if (cmd_help) { > > > fprintf(stderr, > > > @@ -1145,7 +1098,7 @@ static void get_clos_info_for_cpu(int cpu, > > > void > > > *arg1, void *arg2, void *arg3, > > > isst_clos_display_clos_information(cpu, outf, enable, > > > prio_type); > > > } > > > > > > -static void dump_clos_info(void) > > > +static void dump_clos_info(int arg) > > > { > > > if (cmd_help) { > > > fprintf(stderr, > > > @@ -1188,7 +1141,7 @@ static void set_clos_config_for_cpu(int > > > cpu, > > > void *arg1, void *arg2, void *arg3, > > > isst_display_result(cpu, outf, "core-power", "config", > > > ret); > > > } > > > > > > -static void set_clos_config(void) > > > +static void set_clos_config(int arg) > > > { > > > if (cmd_help) { > > > fprintf(stderr, > > > @@ -1252,7 +1205,7 @@ static void set_clos_assoc_for_cpu(int cpu, > > > void *arg1, void *arg2, void *arg3, > > > isst_display_result(cpu, outf, "core-power", "assoc", > > > ret); > > > } > > > > > > -static void set_clos_assoc(void) > > > +static void set_clos_assoc(int arg) > > > { > > > if (cmd_help) { > > > fprintf(stderr, "Associate a clos id to a CPU\n"); > > > @@ -1286,7 +1239,7 @@ static void get_clos_assoc_for_cpu(int cpu, > > > void *arg1, void *arg2, void *arg3, > > > isst_clos_display_assoc_information(cpu, outf, clos); > > > } > > > > > > -static void get_clos_assoc(void) > > > +static void get_clos_assoc(int arg) > > > { > > > if (cmd_help) { > > > fprintf(stderr, "Get associate clos id to a CPU\n"); > > > @@ -1307,26 +1260,27 @@ static void get_clos_assoc(void) > > > } > > > > > > static struct process_cmd_struct isst_cmds[] = { > > > - { "perf-profile", "get-lock-status", get_tdp_locked }, > > > - { "perf-profile", "get-config-levels", get_tdp_levels }, > > > - { "perf-profile", "get-config-version", get_tdp_version }, > > > - { "perf-profile", "get-config-enabled", get_tdp_enabled }, > > > - { "perf-profile", "get-config-current-level", > > > get_tdp_current_level }, > > > - { "perf-profile", "set-config-level", set_tdp_level }, > > > - { "perf-profile", "info", dump_isst_config }, > > > - { "base-freq", "info", dump_pbf_config }, > > > - { "base-freq", "enable", set_pbf_enable }, > > > - { "base-freq", "disable", set_pbf_disable }, > > > - { "turbo-freq", "info", dump_fact_config }, > > > - { "turbo-freq", "enable", set_fact_enable }, > > > - { "turbo-freq", "disable", set_fact_disable }, > > > - { "core-power", "info", dump_clos_info }, > > > - { "core-power", "enable", set_clos_enable }, > > > - { "core-power", "disable", set_clos_disable }, > > > - { "core-power", "config", set_clos_config }, > > > - { "core-power", "get-config", dump_clos_config }, > > > - { "core-power", "assoc", set_clos_assoc }, > > > - { "core-power", "get-assoc", get_clos_assoc }, > > > + { "perf-profile", "get-lock-status", get_tdp_locked, 0 }, > > > + { "perf-profile", "get-config-levels", get_tdp_levels, 0 }, > > > + { "perf-profile", "get-config-version", get_tdp_version, 0 }, > > > + { "perf-profile", "get-config-enabled", get_tdp_enabled, 0 }, > > > + { "perf-profile", "get-config-current-level", > > > get_tdp_current_level, > > > + 0 }, > > > + { "perf-profile", "set-config-level", set_tdp_level, 0 }, > > > + { "perf-profile", "info", dump_isst_config, 0 }, > > > + { "base-freq", "info", dump_pbf_config, 0 }, > > > + { "base-freq", "enable", set_pbf_enable, 1 }, > > > + { "base-freq", "disable", set_pbf_enable, 0 }, > > > + { "turbo-freq", "info", dump_fact_config, 0 }, > > > + { "turbo-freq", "enable", set_fact_enable, 1 }, > > > + { "turbo-freq", "disable", set_fact_enable, 0 }, > > > + { "core-power", "info", dump_clos_info, 0 }, > > > + { "core-power", "enable", set_clos_enable, 1 }, > > > + { "core-power", "disable", set_clos_enable, 0 }, > > > + { "core-power", "config", set_clos_config, 0 }, > > > + { "core-power", "get-config", dump_clos_config, 0 }, > > > + { "core-power", "assoc", set_clos_assoc, 0 }, > > > + { "core-power", "get-assoc", get_clos_assoc, 0 }, > > > { NULL, NULL, NULL } > > > }; > > > > > > @@ -1571,7 +1525,7 @@ void process_command(int argc, char **argv) > > > if (!strcmp(isst_cmds[i].feature, feature) && > > > !strcmp(isst_cmds[i].command, cmd)) { > > > parse_cmd_args(argc, optind + 1, argv); > > > - isst_cmds[i].process_fn(); > > > + isst_cmds[i].process_fn(isst_cmds[i].arg); > > > matched = 1; > > > break; > > > }
