Re: [PATCH v1] cpufreq: CPPC: Fix unused-function warning

[Pierre Gondois]

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@xxxxxxxxxxxxx>
> > > > Signed-off-by: Pierre Gondois <pierre.gondois@xxxxxxx>
> > > > ---
> > > >    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