On Thu, Feb 21, 2019 at 9:15 AM Viresh Kumar <viresh.ku...@linaro.org> wrote:
>
> On 20-02-19, 21:56, Amit Kucheria wrote:
> > On Wed, Feb 20, 2019 at 4:44 PM Viresh Kumar <viresh.ku...@linaro.org>
> > 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 <viresh.ku...@linaro.org>
> > > ---
> > > 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 :)
Perhaps I was just unfamiliar with the dev_pm_opp_set_supported_hw()
API where the actual allocation happens 3 levels deep. Maybe the
comment should apply to dev_pm_opp_set_supported_hw(). I leave it to
you to decide.
> 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
