| Message ID | 5919a74b74f466e803e07f70136517119dcd4560.1550661235.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | cpufreq: kyro: Reduce frame-size of qcom_cpufreq_kryo_probe() | expand |
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>
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 >
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);
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
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.
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.
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; }
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(-)