| Message ID | 20210112095236.20515-1-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: qcom-hw: add missing devm_release_mem_region() call | expand |
On Tue 12 Jan 03:52 CST 2021, Shawn Guo wrote: > On SDM845/850, running the following commands to put all cores in > freq-domain1 offline and then get one core back online, there will be > a request region error seen from qcom-hw driver. > > $ echo 0 > /sys/devices/system/cpu/cpu4/online > $ echo 0 > /sys/devices/system/cpu/cpu5/online > $ echo 0 > /sys/devices/system/cpu/cpu6/online > $ echo 0 > /sys/devices/system/cpu/cpu7/online > $ echo 1 > /sys/devices/system/cpu/cpu4/online > > [ 3395.915416] CPU4: shutdown > [ 3395.938185] psci: CPU4 killed (polled 0 ms) > [ 3399.071424] CPU5: shutdown > [ 3399.094316] psci: CPU5 killed (polled 0 ms) > [ 3402.139358] CPU6: shutdown > [ 3402.161705] psci: CPU6 killed (polled 0 ms) > [ 3404.742939] CPU7: shutdown > [ 3404.765592] psci: CPU7 killed (polled 0 ms) > [ 3411.492274] Detected VIPT I-cache on CPU4 > [ 3411.492337] GICv3: CPU4: found redistributor 400 region 0:0x0000000017ae0000 > [ 3411.492448] CPU4: Booted secondary processor 0x0000000400 [0x516f802d] > [ 3411.503654] qcom-cpufreq-hw 17d43000.cpufreq: can't request region for resource [mem 0x17d45800-0x17d46bff] > > The cause is that the memory region requested in .init hook doesn't get > released in .exit hook, and the subsequent call to .init will always fail > on this error. Let's break down the devm_platform_ioremap_resource() > call a bit, so that we can have the resource pointer to release memory > region from .exit hook. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > drivers/cpufreq/qcom-cpufreq-hw.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > index 9ed5341dc515..315ee987d2d3 100644 > --- a/drivers/cpufreq/qcom-cpufreq-hw.c > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > @@ -32,6 +32,7 @@ struct qcom_cpufreq_soc_data { > > struct qcom_cpufreq_data { > void __iomem *base; > + struct resource *res; > const struct qcom_cpufreq_soc_data *soc_data; > }; > > @@ -280,6 +281,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > struct of_phandle_args args; > struct device_node *cpu_np; > struct device *cpu_dev; > + struct resource *res; > void __iomem *base; > struct qcom_cpufreq_data *data; > int ret, index; > @@ -303,7 +305,8 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > > index = args.args[0]; > > - base = devm_platform_ioremap_resource(pdev, index); > + res = platform_get_resource(pdev, IORESOURCE_MEM, index); > + base = devm_ioremap_resource(dev, res); > if (IS_ERR(base)) > return PTR_ERR(base); > > @@ -315,6 +318,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > > data->soc_data = of_device_get_match_data(&pdev->dev); > data->base = base; > + data->res = res; > > /* HW should be in enabled state to proceed */ > if (!(readl_relaxed(base + data->soc_data->reg_enable) & 0x1)) { > @@ -358,11 +362,13 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy) > struct device *cpu_dev = get_cpu_device(policy->cpu); > struct qcom_cpufreq_data *data = policy->driver_data; > struct platform_device *pdev = cpufreq_get_driver_data(); > + struct resource *res = data->res; > > dev_pm_opp_remove_all_dynamic(cpu_dev); > dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); > kfree(policy->freq_table); > devm_iounmap(&pdev->dev, data->base); > + devm_release_mem_region(&pdev->dev, res->start, resource_size(res)); Intuitively I feel that resources allocated in cpufreq_driver->init() should be explicitly freed in cpufreq_driver->exit() and should thereby not use devm to track the allocations. Further more, the fact that one needs to explicitly perform the release_mem_region explicitly is a good sign that one shouldn't manually unmap things that was mapped by devm_ioremap_resource(). But afaict when qcom_cpufreq_hw_driver_remove() calls cpufreq_unregister_driver() to end up in cpufreq_remove_dev() it will only call cpufreq_driver->exit() iff cpufreq_driver->offline() is implemented - which it isn't in our case. So without using devm to track this we would leak the memory - which also implies that we're leaking the "freq_table" when this happens. But isn't that simply a typo in cpufreq_remove_dev()? And can't we just use ioremap()/iounmap() here instead? Regards, Bjorn > > return 0; > } > -- > 2.17.1 >
On 12-01-21, 08:44, Bjorn Andersson wrote: > Intuitively I feel that resources allocated in cpufreq_driver->init() > should be explicitly freed in cpufreq_driver->exit() and should thereby > not use devm to track the allocations. I agree. > But afaict when qcom_cpufreq_hw_driver_remove() calls > cpufreq_unregister_driver() to end up in cpufreq_remove_dev() it will > only call cpufreq_driver->exit() iff cpufreq_driver->offline() is > implemented - which it isn't in our case. cpufreq_offline() calls exit() in your case. So no memory leak here. > So without using devm to track > this we would leak the memory - which also implies that we're leaking > the "freq_table" when this happens. > > But isn't that simply a typo in cpufreq_remove_dev()? And can't we just > use ioremap()/iounmap() here instead?
On 12-01-21, 22:59, Bjorn Andersson wrote:
> But that said, why are the ioremap done at init and not at probe time?
These are some hardware registers per cpufreq policy I believe, and so
they did it from policy init instead.
And yes I agree that we shouldn't use devm_ from init() for the cases
where we need to put the resources in exit() as well. But things like
devm_kzalloc() are fine there.
Ionela, since you were the first one to post a patch about this, can
you send a fix for this by dropping the devm_ thing altogether for the
ioremap thing ? Mark it suggested by Bjorn. Thanks.
--
viresh
Il 13/01/21 06:06, Viresh Kumar ha scritto: > On 12-01-21, 22:59, Bjorn Andersson wrote: >> But that said, why are the ioremap done at init and not at probe time? > > These are some hardware registers per cpufreq policy I believe, and so > they did it from policy init instead. > > And yes I agree that we shouldn't use devm_ from init() for the cases > where we need to put the resources in exit() as well. But things like > devm_kzalloc() are fine there. > > Ionela, since you were the first one to post a patch about this, can > you send a fix for this by dropping the devm_ thing altogether for the > ioremap thing ? Mark it suggested by Bjorn. Thanks. > Sorry, are you sure that the eventual fix shouldn't be rebased on top of my change (12014503) [1] that is enabling CPU scaling for all of the platforms that aren't getting the OSM set-up entirely by the TZ/bootloader? It's a pretty big series, that I've rebased 3 times already... [1]: https://patchwork.kernel.org/project/linux-arm-msm/patch/20210112182052.481888-15-angelogioacchino.delregno@somainline.org/
On 13-01-21, 23:12, AngeloGioacchino Del Regno wrote: > Sorry, are you sure that the eventual fix shouldn't be rebased on top of my > change (12014503) [1] that is enabling CPU scaling for all of the platforms > that aren't getting the OSM set-up entirely by the TZ/bootloader? > It's a pretty big series, that I've rebased 3 times already... > > [1]: https://patchwork.kernel.org/project/linux-arm-msm/patch/20210112182052.481888-15-angelogioacchino.delregno@somainline.org/ I am waiting for someone from Qcom background to review the stuff, perhaps Bjorn or someone else as it is a big change. Second, the fixes never get rebased on new stuff as they also need to make it to stable kernels and current Linux release instead of the next one. So, this fix will go first irrespective of the timeframe when it was posted. Thanks Angelo. -- viresh
On Wed 13 Jan 16:12 CST 2021, AngeloGioacchino Del Regno wrote: > Il 13/01/21 06:06, Viresh Kumar ha scritto: > > On 12-01-21, 22:59, Bjorn Andersson wrote: > > > But that said, why are the ioremap done at init and not at probe time? > > > > These are some hardware registers per cpufreq policy I believe, and so > > they did it from policy init instead. > > > > And yes I agree that we shouldn't use devm_ from init() for the cases > > where we need to put the resources in exit() as well. But things like > > devm_kzalloc() are fine there. > > > > Ionela, since you were the first one to post a patch about this, can > > you send a fix for this by dropping the devm_ thing altogether for the > > ioremap thing ? Mark it suggested by Bjorn. Thanks. > > > > Sorry, are you sure that the eventual fix shouldn't be rebased on top of my > change (12014503) [1] that is enabling CPU scaling for all of the platforms > that aren't getting the OSM set-up entirely by the TZ/bootloader? > It's a pretty big series, that I've rebased 3 times already... > I hear you and I love the work you're doing, so I am definitely trying to find time to review your patches properly. Regarding the size of the series, my suggestion is that you have shown the whole picture, so going forward it's better to split it up in individual series based on how they will be merged by different maintainers. Thank you, Bjorn > [1]: https://patchwork.kernel.org/project/linux-arm-msm/patch/20210112182052.481888-15-angelogioacchino.delregno@somainline.org/
Hi, On Monday 18 Jan 2021 at 14:54:11 (+0800), Shawn Guo wrote: > On Mon, Jan 18, 2021 at 2:43 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 13-01-21, 10:36, Viresh Kumar wrote: > > > On 12-01-21, 22:59, Bjorn Andersson wrote: > > > > But that said, why are the ioremap done at init and not at probe time? > > > > > > These are some hardware registers per cpufreq policy I believe, and so > > > they did it from policy init instead. > > > > > > And yes I agree that we shouldn't use devm_ from init() for the cases > > > where we need to put the resources in exit() as well. But things like > > > devm_kzalloc() are fine there. > > > > > > Ionela, since you were the first one to post a patch about this, can > > > you send a fix for this by dropping the devm_ thing altogether for the > > > ioremap thing ? Mark it suggested by Bjorn. Thanks. > > > > Ionela, I hope you are working on this so we can get it fixed quickly > > ? > > Let me take it over. I will try to send a patch out today. > I did not get any cycles for this until today. I'm happy for you Shawn to take it over if you'd like, and I'm happy to review and test. Thanks, Ionela. > Shawn
On Wed, Jan 13, 2021 at 10:36:51AM +0530, Viresh Kumar wrote: > On 12-01-21, 22:59, Bjorn Andersson wrote: > > But that said, why are the ioremap done at init and not at probe time? > > These are some hardware registers per cpufreq policy I believe, and so > they did it from policy init instead. > > And yes I agree that we shouldn't use devm_ from init() for the cases > where we need to put the resources in exit() as well. But things like > devm_kzalloc() are fine there. I'm not sure why devm_kzalloc() is fine there. IIUIC, the memory allocated by devm_kzalloc() in init() is not freed up from exit(), as &pdev->dev is alive across init/exit cycles and will not trigger devres auto free-up. Shawn
On 18-01-21, 20:17, Shawn Guo wrote: > On Wed, Jan 13, 2021 at 10:36:51AM +0530, Viresh Kumar wrote: > > On 12-01-21, 22:59, Bjorn Andersson wrote: > > > But that said, why are the ioremap done at init and not at probe time? > > > > These are some hardware registers per cpufreq policy I believe, and so > > they did it from policy init instead. > > > > And yes I agree that we shouldn't use devm_ from init() for the cases > > where we need to put the resources in exit() as well. But things like > > devm_kzalloc() are fine there. > > I'm not sure why devm_kzalloc() is fine there. IIUIC, the memory > allocated by devm_kzalloc() in init() is not freed up from exit(), as > &pdev->dev is alive across init/exit cycles and will not trigger devres > auto free-up. Yes, but reallocating it again if ->init() get called again isn't a bug and will only block a part of memory for sometime, i.e. until the time driver isn't removed. Though it is better I think to get rid of it as well. -- viresh
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c index 9ed5341dc515..315ee987d2d3 100644 --- a/drivers/cpufreq/qcom-cpufreq-hw.c +++ b/drivers/cpufreq/qcom-cpufreq-hw.c @@ -32,6 +32,7 @@ struct qcom_cpufreq_soc_data { struct qcom_cpufreq_data { void __iomem *base; + struct resource *res; const struct qcom_cpufreq_soc_data *soc_data; }; @@ -280,6 +281,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) struct of_phandle_args args; struct device_node *cpu_np; struct device *cpu_dev; + struct resource *res; void __iomem *base; struct qcom_cpufreq_data *data; int ret, index; @@ -303,7 +305,8 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) index = args.args[0]; - base = devm_platform_ioremap_resource(pdev, index); + res = platform_get_resource(pdev, IORESOURCE_MEM, index); + base = devm_ioremap_resource(dev, res); if (IS_ERR(base)) return PTR_ERR(base); @@ -315,6 +318,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) data->soc_data = of_device_get_match_data(&pdev->dev); data->base = base; + data->res = res; /* HW should be in enabled state to proceed */ if (!(readl_relaxed(base + data->soc_data->reg_enable) & 0x1)) { @@ -358,11 +362,13 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy) struct device *cpu_dev = get_cpu_device(policy->cpu); struct qcom_cpufreq_data *data = policy->driver_data; struct platform_device *pdev = cpufreq_get_driver_data(); + struct resource *res = data->res; dev_pm_opp_remove_all_dynamic(cpu_dev); dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); kfree(policy->freq_table); devm_iounmap(&pdev->dev, data->base); + devm_release_mem_region(&pdev->dev, res->start, resource_size(res)); return 0; }
On SDM845/850, running the following commands to put all cores in freq-domain1 offline and then get one core back online, there will be a request region error seen from qcom-hw driver. $ echo 0 > /sys/devices/system/cpu/cpu4/online $ echo 0 > /sys/devices/system/cpu/cpu5/online $ echo 0 > /sys/devices/system/cpu/cpu6/online $ echo 0 > /sys/devices/system/cpu/cpu7/online $ echo 1 > /sys/devices/system/cpu/cpu4/online [ 3395.915416] CPU4: shutdown [ 3395.938185] psci: CPU4 killed (polled 0 ms) [ 3399.071424] CPU5: shutdown [ 3399.094316] psci: CPU5 killed (polled 0 ms) [ 3402.139358] CPU6: shutdown [ 3402.161705] psci: CPU6 killed (polled 0 ms) [ 3404.742939] CPU7: shutdown [ 3404.765592] psci: CPU7 killed (polled 0 ms) [ 3411.492274] Detected VIPT I-cache on CPU4 [ 3411.492337] GICv3: CPU4: found redistributor 400 region 0:0x0000000017ae0000 [ 3411.492448] CPU4: Booted secondary processor 0x0000000400 [0x516f802d] [ 3411.503654] qcom-cpufreq-hw 17d43000.cpufreq: can't request region for resource [mem 0x17d45800-0x17d46bff] The cause is that the memory region requested in .init hook doesn't get released in .exit hook, and the subsequent call to .init will always fail on this error. Let's break down the devm_platform_ioremap_resource() call a bit, so that we can have the resource pointer to release memory region from .exit hook. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/cpufreq/qcom-cpufreq-hw.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)