| Message ID | 20220530081236.40728-1-pierre.gondois@arm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v1] cpufreq: CPPC: Fix unused-function warning | expand |
On 30-05-22, 10:12, Pierre Gondois wrote: > Building the cppc_cpufreq driver with for arm64 with > CONFIG_ENERGY_MODEL=n triggers the following warnings: > drivers/cpufreq/cppc_cpufreq.c:550:12: error: ‘cppc_get_cpu_cost’ defined but not used > [-Werror=unused-function] > 550 | static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, > | ^~~~~~~~~~~~~~~~~ > drivers/cpufreq/cppc_cpufreq.c:481:12: error: ‘cppc_get_cpu_power’ defined but not used > [-Werror=unused-function] > 481 | static int cppc_get_cpu_power(struct device *cpu_dev, > | ^~~~~~~~~~~~~~~~~~ > > Fixes: 740fcdc2c20e ("cpufreq: CPPC: Register EM based on efficiency class information") > Reported-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > --- > drivers/cpufreq/cppc_cpufreq.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index d092c9bb4ba3..ecd0d3ee48c5 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -478,7 +478,7 @@ static inline unsigned long compute_cost(int cpu, int step) > step * CPPC_EM_COST_STEP; > } > > -static int cppc_get_cpu_power(struct device *cpu_dev, > +static __maybe_unused int cppc_get_cpu_power(struct device *cpu_dev, > unsigned long *power, unsigned long *KHz) > { > unsigned long perf_step, perf_prev, perf, perf_check; > @@ -547,8 +547,8 @@ static int cppc_get_cpu_power(struct device *cpu_dev, > return 0; > } > > -static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, > - unsigned long *cost) > +static __maybe_unused int cppc_get_cpu_cost(struct device *cpu_dev, > + unsigned long KHz, unsigned long *cost) > { > unsigned long perf_step, perf_prev; > struct cppc_perf_caps *perf_caps; Should we actually run cppc_cpufreq_register_em() for !CONFIG_ENERGY_MODEL ? Why?
On 5/30/22 10:20, Viresh Kumar wrote: > On 30-05-22, 10:12, Pierre Gondois wrote: >> Building the cppc_cpufreq driver with for arm64 with >> CONFIG_ENERGY_MODEL=n triggers the following warnings: >> drivers/cpufreq/cppc_cpufreq.c:550:12: error: ‘cppc_get_cpu_cost’ defined but not used >> [-Werror=unused-function] >> 550 | static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, >> | ^~~~~~~~~~~~~~~~~ >> drivers/cpufreq/cppc_cpufreq.c:481:12: error: ‘cppc_get_cpu_power’ defined but not used >> [-Werror=unused-function] >> 481 | static int cppc_get_cpu_power(struct device *cpu_dev, >> | ^~~~~~~~~~~~~~~~~~ >> >> Fixes: 740fcdc2c20e ("cpufreq: CPPC: Register EM based on efficiency class information") >> Reported-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >> --- >> drivers/cpufreq/cppc_cpufreq.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >> index d092c9bb4ba3..ecd0d3ee48c5 100644 >> --- a/drivers/cpufreq/cppc_cpufreq.c >> +++ b/drivers/cpufreq/cppc_cpufreq.c >> @@ -478,7 +478,7 @@ static inline unsigned long compute_cost(int cpu, int step) >> step * CPPC_EM_COST_STEP; >> } >> >> -static int cppc_get_cpu_power(struct device *cpu_dev, >> +static __maybe_unused int cppc_get_cpu_power(struct device *cpu_dev, >> unsigned long *power, unsigned long *KHz) >> { >> unsigned long perf_step, perf_prev, perf, perf_check; >> @@ -547,8 +547,8 @@ static int cppc_get_cpu_power(struct device *cpu_dev, >> return 0; >> } >> >> -static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, >> - unsigned long *cost) >> +static __maybe_unused int cppc_get_cpu_cost(struct device *cpu_dev, >> + unsigned long KHz, unsigned long *cost) >> { >> unsigned long perf_step, perf_prev; >> struct cppc_perf_caps *perf_caps; > > Should we actually run cppc_cpufreq_register_em() for > !CONFIG_ENERGY_MODEL ? Why? > Hello Viresh, It seems that when CONFIG_ENERGY_MODEL=n, the compiler is already considering cppc_cpufreq_register_em() as an empty function. Indeed, CONFIG_ENERGY_MODEL=n makes em_dev_register_perf_domain() an empty function, so cppc_cpufreq_register_em() is only made of variable definitions. This compiler optimization also explains why cppc_get_cpu_power() and cppc_get_cpu_cost() trigger the -Wunused-function warning. Putting cppc_cpufreq_register_em() inside an #ifdef CONFIG_ENERGY_MODEL guard seems also valid to me. To avoid too many empty definitions of cppc_cpufreq_register_em(), I guess it should be inside an #if defined(CONFIG_ARM64) && defined(CONFIG_ENERGY_MODEL) guard instead. Please let me know what you prefer. Regards, Pierre
On 30-05-22, 10:44, Pierre Gondois wrote: > > > On 5/30/22 10:20, Viresh Kumar wrote: > > On 30-05-22, 10:12, Pierre Gondois wrote: > > > Building the cppc_cpufreq driver with for arm64 with > > > CONFIG_ENERGY_MODEL=n triggers the following warnings: > > > drivers/cpufreq/cppc_cpufreq.c:550:12: error: ‘cppc_get_cpu_cost’ defined but not used > > > [-Werror=unused-function] > > > 550 | static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, > > > | ^~~~~~~~~~~~~~~~~ > > > drivers/cpufreq/cppc_cpufreq.c:481:12: error: ‘cppc_get_cpu_power’ defined but not used > > > [-Werror=unused-function] > > > 481 | static int cppc_get_cpu_power(struct device *cpu_dev, > > > | ^~~~~~~~~~~~~~~~~~ > > > > > > Fixes: 740fcdc2c20e ("cpufreq: CPPC: Register EM based on efficiency class information") > > > Reported-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > > > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > > > --- > > > drivers/cpufreq/cppc_cpufreq.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > > > index d092c9bb4ba3..ecd0d3ee48c5 100644 > > > --- a/drivers/cpufreq/cppc_cpufreq.c > > > +++ b/drivers/cpufreq/cppc_cpufreq.c > > > @@ -478,7 +478,7 @@ static inline unsigned long compute_cost(int cpu, int step) > > > step * CPPC_EM_COST_STEP; > > > } > > > -static int cppc_get_cpu_power(struct device *cpu_dev, > > > +static __maybe_unused int cppc_get_cpu_power(struct device *cpu_dev, > > > unsigned long *power, unsigned long *KHz) > > > { > > > unsigned long perf_step, perf_prev, perf, perf_check; > > > @@ -547,8 +547,8 @@ static int cppc_get_cpu_power(struct device *cpu_dev, > > > return 0; > > > } > > > -static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, > > > - unsigned long *cost) > > > +static __maybe_unused int cppc_get_cpu_cost(struct device *cpu_dev, > > > + unsigned long KHz, unsigned long *cost) > > > { > > > unsigned long perf_step, perf_prev; > > > struct cppc_perf_caps *perf_caps; > > > > Should we actually run cppc_cpufreq_register_em() for > > !CONFIG_ENERGY_MODEL ? Why? > > > > Hello Viresh, > It seems that when CONFIG_ENERGY_MODEL=n, the compiler is already > considering cppc_cpufreq_register_em() as an empty function. > > Indeed, CONFIG_ENERGY_MODEL=n makes em_dev_register_perf_domain() > an empty function, so cppc_cpufreq_register_em() is only made of > variable definitions. This compiler optimization also explains > why cppc_get_cpu_power() and cppc_get_cpu_cost() trigger the > -Wunused-function warning. > > Putting cppc_cpufreq_register_em() inside an > #ifdef CONFIG_ENERGY_MODEL > guard seems also valid to me. To avoid too many empty definitions > of cppc_cpufreq_register_em(), I guess it should be inside an > #if defined(CONFIG_ARM64) && defined(CONFIG_ENERGY_MODEL) > guard instead. > Please let me know what you prefer. In that case we shouldn't do: cppc_cpufreq_driver.register_em = cppc_cpufreq_register_em; as well, as that is extra work for the cpufreq core, which won't be used at all. So instead of __maybe_unused, lets put all dependent stuff within CONFIG_ENERGY_MODEL ?
On 5/30/22 11:07, Viresh Kumar wrote: > On 30-05-22, 10:44, Pierre Gondois wrote: >> >> >> On 5/30/22 10:20, Viresh Kumar wrote: >>> On 30-05-22, 10:12, Pierre Gondois wrote: >>>> Building the cppc_cpufreq driver with for arm64 with >>>> CONFIG_ENERGY_MODEL=n triggers the following warnings: >>>> drivers/cpufreq/cppc_cpufreq.c:550:12: error: ‘cppc_get_cpu_cost’ defined but not used >>>> [-Werror=unused-function] >>>> 550 | static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, >>>> | ^~~~~~~~~~~~~~~~~ >>>> drivers/cpufreq/cppc_cpufreq.c:481:12: error: ‘cppc_get_cpu_power’ defined but not used >>>> [-Werror=unused-function] >>>> 481 | static int cppc_get_cpu_power(struct device *cpu_dev, >>>> | ^~~~~~~~~~~~~~~~~~ >>>> >>>> Fixes: 740fcdc2c20e ("cpufreq: CPPC: Register EM based on efficiency class information") >>>> Reported-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >>>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >>>> --- >>>> drivers/cpufreq/cppc_cpufreq.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >>>> index d092c9bb4ba3..ecd0d3ee48c5 100644 >>>> --- a/drivers/cpufreq/cppc_cpufreq.c >>>> +++ b/drivers/cpufreq/cppc_cpufreq.c >>>> @@ -478,7 +478,7 @@ static inline unsigned long compute_cost(int cpu, int step) >>>> step * CPPC_EM_COST_STEP; >>>> } >>>> -static int cppc_get_cpu_power(struct device *cpu_dev, >>>> +static __maybe_unused int cppc_get_cpu_power(struct device *cpu_dev, >>>> unsigned long *power, unsigned long *KHz) >>>> { >>>> unsigned long perf_step, perf_prev, perf, perf_check; >>>> @@ -547,8 +547,8 @@ static int cppc_get_cpu_power(struct device *cpu_dev, >>>> return 0; >>>> } >>>> -static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, >>>> - unsigned long *cost) >>>> +static __maybe_unused int cppc_get_cpu_cost(struct device *cpu_dev, >>>> + unsigned long KHz, unsigned long *cost) >>>> { >>>> unsigned long perf_step, perf_prev; >>>> struct cppc_perf_caps *perf_caps; >>> >>> Should we actually run cppc_cpufreq_register_em() for >>> !CONFIG_ENERGY_MODEL ? Why? >>> >> >> Hello Viresh, >> It seems that when CONFIG_ENERGY_MODEL=n, the compiler is already >> considering cppc_cpufreq_register_em() as an empty function. >> >> Indeed, CONFIG_ENERGY_MODEL=n makes em_dev_register_perf_domain() >> an empty function, so cppc_cpufreq_register_em() is only made of >> variable definitions. This compiler optimization also explains >> why cppc_get_cpu_power() and cppc_get_cpu_cost() trigger the >> -Wunused-function warning. >> >> Putting cppc_cpufreq_register_em() inside an >> #ifdef CONFIG_ENERGY_MODEL >> guard seems also valid to me. To avoid too many empty definitions >> of cppc_cpufreq_register_em(), I guess it should be inside an >> #if defined(CONFIG_ARM64) && defined(CONFIG_ENERGY_MODEL) >> guard instead. >> Please let me know what you prefer. > > In that case we shouldn't do: > > cppc_cpufreq_driver.register_em = cppc_cpufreq_register_em; > > as well, as that is extra work for the cpufreq core, which won't be > used at all. > > So instead of __maybe_unused, lets put all dependent stuff within > CONFIG_ENERGY_MODEL ? > Ok yes. Just to be sure and except if disagreed, I will use the following structure: #if CONFIG_ARM64 #else #endif #if defined(CONFIG_ARM64) && defined(CONFIG_ENERGY_MODEL) int populate_efficiency_class(); #else int populate_efficiency_class(); #endif to avoid having multiple empty definitions of populate_efficiency_class() (for eg.) that we would have with: #if CONFIG_ARM64 #if CONFIG_ENERGY_MODEL int populate_efficiency_class(); #else // CONFIG_ENERGY_MODEL int populate_efficiency_class(); #endif // CONFIG_ENERGY_MODEL #else // CONFIG_ARM64 int populate_efficiency_class(); #endif // CONFIG_ARM64
On 30-05-22, 11:42, Pierre Gondois wrote: > Ok yes. Just to be sure and except if disagreed, I will use the > following structure: > #if CONFIG_ARM64 > #else > #endif > > #if defined(CONFIG_ARM64) && defined(CONFIG_ENERGY_MODEL) > int populate_efficiency_class(); > #else > int populate_efficiency_class(); > #endif > > to avoid having multiple empty definitions of > populate_efficiency_class() (for eg.) that we would have with: > #if CONFIG_ARM64 > #if CONFIG_ENERGY_MODEL > int populate_efficiency_class(); > #else // CONFIG_ENERGY_MODEL > int populate_efficiency_class(); > #endif // CONFIG_ENERGY_MODEL > #else // CONFIG_ARM64 > int populate_efficiency_class(); > #endif // CONFIG_ARM64 Look good.
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c index d092c9bb4ba3..ecd0d3ee48c5 100644 --- a/drivers/cpufreq/cppc_cpufreq.c +++ b/drivers/cpufreq/cppc_cpufreq.c @@ -478,7 +478,7 @@ static inline unsigned long compute_cost(int cpu, int step) step * CPPC_EM_COST_STEP; } -static int cppc_get_cpu_power(struct device *cpu_dev, +static __maybe_unused int cppc_get_cpu_power(struct device *cpu_dev, unsigned long *power, unsigned long *KHz) { unsigned long perf_step, perf_prev, perf, perf_check; @@ -547,8 +547,8 @@ static int cppc_get_cpu_power(struct device *cpu_dev, return 0; } -static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, - unsigned long *cost) +static __maybe_unused int cppc_get_cpu_cost(struct device *cpu_dev, + unsigned long KHz, unsigned long *cost) { unsigned long perf_step, perf_prev; struct cppc_perf_caps *perf_caps;
Building the cppc_cpufreq driver with for arm64 with CONFIG_ENERGY_MODEL=n triggers the following warnings: drivers/cpufreq/cppc_cpufreq.c:550:12: error: ‘cppc_get_cpu_cost’ defined but not used [-Werror=unused-function] 550 | static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz, | ^~~~~~~~~~~~~~~~~ drivers/cpufreq/cppc_cpufreq.c:481:12: error: ‘cppc_get_cpu_power’ defined but not used [-Werror=unused-function] 481 | static int cppc_get_cpu_power(struct device *cpu_dev, | ^~~~~~~~~~~~~~~~~~ Fixes: 740fcdc2c20e ("cpufreq: CPPC: Register EM based on efficiency class information") Reported-by: Shaokun Zhang <zhangshaokun@hisilicon.com> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> --- drivers/cpufreq/cppc_cpufreq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)