Re: [PATCH] cpufreq: kyro: Reduce frame-size of qcom_cpufreq_kryo_probe()

[Viresh Kumar]

On 20-02-19, 21:56, Amit Kucheria wrote:
> On Wed, Feb 20, 2019 at 4:44 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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.kumar@xxxxxxxxxx>
> > ---
> >  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