diff mbox series

cpufreq: kyro: Reduce frame-size of qcom_cpufreq_kryo_probe()

Message ID 5919a74b74f466e803e07f70136517119dcd4560.1550661235.git.viresh.kumar@linaro.org
State New
Headers show
Series cpufreq: kyro: Reduce frame-size of qcom_cpufreq_kryo_probe() | expand

Commit Message

Viresh Kumar Feb. 20, 2019, 11:14 a.m. UTC 
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@linaro.org>

---
 drivers/cpufreq/qcom-cpufreq-kryo.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.21.0.rc0.269.g1a574e7a288b

Comments

Arnd Bergmann Feb. 20, 2019, 12:30 p.m. UTC | #1 
On Wed, Feb 20, 2019 at 12:14 PM Viresh Kumar <viresh.kumar@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.kumar@linaro.org>


Looks good to me, thanks for the fix!

Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Amit Kucheria Feb. 20, 2019, 4:26 p.m. UTC | #2 
On Wed, Feb 20, 2019 at 4:44 PM Viresh Kumar <viresh.kumar@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.kumar@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. :-)

Otherwise,

Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>


>         for_each_possible_cpu(cpu) {

>                 cpu_dev = get_cpu_device(cpu);

>                 if (NULL == cpu_dev) {

> @@ -149,6 +153,8 @@ static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)

>                 }

>         }

>

> +       kfree(opp_tables);

> +

>         cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,

>                                                           NULL, 0);

>         if (!IS_ERR(cpufreq_dt_pdev))

> @@ -163,6 +169,7 @@ static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)

>                         break;

>                 dev_pm_opp_put_supported_hw(opp_tables[cpu]);

>         }

> +       kfree(opp_tables);

>

>         return ret;

>  }

> --

> 2.21.0.rc0.269.g1a574e7a288b

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

        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
Amit Kucheria Feb. 21, 2019, 4:32 a.m. UTC | #4 
On Thu, Feb 21, 2019 at 9:15 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> On 20-02-19, 21:56, Amit Kucheria wrote:

> > On Wed, Feb 20, 2019 at 4:44 PM Viresh Kumar <viresh.kumar@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.kumar@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
Viresh Kumar Feb. 21, 2019, 4:44 a.m. UTC | #5 
On 21-02-19, 10:02, Amit Kucheria wrote:
> 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.


I think we are fine without any comments here :)

Thanks for your reviews Amit.

-- 
viresh
Viresh Kumar Feb. 27, 2019, 7:52 a.m. UTC | #6 
On 20-02-19, 16:44, Viresh Kumar 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@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;

> +

>  	for_each_possible_cpu(cpu) {

>  		cpu_dev = get_cpu_device(cpu);

>  		if (NULL == cpu_dev) {

> @@ -149,6 +153,8 @@ static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)

>  		}

>  	}

>  

> +	kfree(opp_tables);

> +

>  	cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,

>  							  NULL, 0);

>  	if (!IS_ERR(cpufreq_dt_pdev))


We can fail here and then we will try to use opp_tables which is already freed.

I wonder how this stupid patch made it through the reviews :)

> @@ -163,6 +169,7 @@ static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)

>  			break;

>  		dev_pm_opp_put_supported_hw(opp_tables[cpu]);

>  	}

> +	kfree(opp_tables);

>  

>  	return ret;

>  }


Having said that, there is another problem which I just noticed in the remove()
path where we don't call dev_pm_opp_put_supported_hw(). That will create
problems if we try to remove module of this driver and then reinstall it as the
OPP table was never freed. The fix for that problem needs to go into stable
kernels past 4.18 and so I will abandon this patch and send a new fix which will
fix the issues of $subject patch as well.

-- 
viresh
diff mbox series

Patch

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;
+
 	for_each_possible_cpu(cpu) {
 		cpu_dev = get_cpu_device(cpu);
 		if (NULL == cpu_dev) {
@@ -149,6 +153,8 @@  static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)
 		}
 	}
 
+	kfree(opp_tables);
+
 	cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
 							  NULL, 0);
 	if (!IS_ERR(cpufreq_dt_pdev))
@@ -163,6 +169,7 @@  static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)
 			break;
 		dev_pm_opp_put_supported_hw(opp_tables[cpu]);
 	}
+	kfree(opp_tables);
 
 	return ret;
 }