On 20-02-19, 21:56, Amit Kucheria wrote:
> On Wed, Feb 20, 2019 at 4:44 PM Viresh Kumar <[email protected]> wrote:
> >
> > With the introduction of commit 846a415bf440 ("arm64: default NR_CPUS to
> > 256"), we have started getting following compilation warning:
> >
> > qcom-cpufreq-kryo.c:168:1: warning: the frame size of 2160 bytes is larger
> > than 2048 bytes [-Wframe-larger-than=]
> >
> > Fix that by dynamically allocating opp_tables and freeing it later.
> >
> > Compile tested only.
> >
> > Signed-off-by: Viresh Kumar <[email protected]>
> > ---
> > drivers/cpufreq/qcom-cpufreq-kryo.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/qcom-cpufreq-kryo.c
> > b/drivers/cpufreq/qcom-cpufreq-kryo.c
> > index 1c8583cc06a2..6888cb6db2ef 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-kryo.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-kryo.c
> > @@ -75,7 +75,7 @@ static enum _msm8996_version
> > qcom_cpufreq_kryo_get_msm_id(void)
> >
> > static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)
> > {
> > - struct opp_table *opp_tables[NR_CPUS] = {0};
> > + struct opp_table **opp_tables;
> > enum _msm8996_version msm8996_version;
> > struct nvmem_cell *speedbin_nvmem;
> > struct device_node *np;
> > @@ -133,6 +133,10 @@ static int qcom_cpufreq_kryo_probe(struct
> > platform_device *pdev)
> > }
> > kfree(speedbin);
> >
> > + opp_tables = kcalloc(num_possible_cpus(), sizeof(*opp_tables),
> > GFP_KERNEL);
> > + if (!opp_tables)
> > + return -ENOMEM;
> > +
>
> Perhaps add a comment above that that actual opp_table is allocated in
> the loop below because of dev_pm_opp_set_supported_hw?
>
> I was staring at this for a few minutes wondering why you needed this
> kcalloc before I realised that opp_tables (missed the 's') is a
> temporary array of pointers. :-)
I feel that you got confused because this patch didn't had the diff
where the opp_tables thing is getting used. When we see the .c file
itself, it is pretty much clear on what is going on and I believe the
comment would be totally unnecessary and redundant.
This is how it looks now, please lemme know if you still prefer the
comment :)
opp_tables = kcalloc(num_possible_cpus(), sizeof(*opp_tables),
GFP_KERNEL);
if (!opp_tables)
return -ENOMEM;
for_each_possible_cpu(cpu) {
cpu_dev = get_cpu_device(cpu);
if (NULL == cpu_dev) {
ret = -ENODEV;
goto free_opp;
}
opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
&versions, 1);
if (IS_ERR(opp_tables[cpu])) {
ret = PTR_ERR(opp_tables[cpu]);
dev_err(cpu_dev, "Failed to set supported hardware\n");
goto free_opp;
}
}
kfree(opp_tables);
--
viresh
