linux-kernel - Re: [PATCH] cpufreq: kyro: Reduce frame-size of qcom_cpufreq_kryo

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190221034534.n5jli3bj4mwtoudd@vireshk-i7>
Date:   Thu, 21 Feb 2019 09:15:34 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Amit Kucheria <amit.kucheria@...aro.org>
Cc:     Rafael Wysocki <rjw@...ysocki.net>, Ilia Lin <ilia.lin@...nel.org>,
        Linux PM list <linux-pm@...r.kernel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cpufreq: kyro: Reduce frame-size of
 qcom_cpufreq_kryo_probe()

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

        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