| Message ID | 20210112095236.20515-1-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | cpufreq: qcom-hw: add missing devm_release_mem_region() call | expand |
On 12-01-21, 17:52, 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(-) FWIW, Ionela also sent a fix though I like this one better for the obvious reasons. https://lore.kernel.org/lkml/20210108151406.23595-1-ionela.voinescu@arm.com/ > 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; > } > -- > 2.17.1
On Tue, Jan 12, 2021 at 03:44:49PM +0530, Viresh Kumar wrote: > On 12-01-21, 17:52, 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(-) > > FWIW, Ionela also sent a fix though I like this one better for the > obvious reasons. > > https://lore.kernel.org/lkml/20210108151406.23595-1-ionela.voinescu@arm.com/ Ha, thanks for the pointer. So the original code was tricky and skipped the region request call intentionally. Shawn
Hi guys, On Tuesday 12 Jan 2021 at 19:19:29 (+0800), Shawn Guo wrote: > On Tue, Jan 12, 2021 at 03:44:49PM +0530, Viresh Kumar wrote: > > On 12-01-21, 17:52, 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(-) > > > > FWIW, Ionela also sent a fix though I like this one better for the > > obvious reasons. > > > > https://lore.kernel.org/lkml/20210108151406.23595-1-ionela.voinescu@arm.com/ > > Ha, thanks for the pointer. So the original code was tricky and skipped > the region request call intentionally. > As long as the problem is fixed, I'm happy :). For this patch: Tested-by: Ionela Voinescu <ionela.voinescu@arm.com> The patch probably deserves: Fixes: f17b3e44320b ("cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code") Thanks, Ionela. > Shawn
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 Tue 12 Jan 22:31 CST 2021, Viresh Kumar wrote: > 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. > Okay, so cpufreq_offline() calls exit iff there's no offline implemented and therefor cpufreq_remove_dev() need to call exit if the other code path was taken in cpufreq_offline()...that's...clever... Thanks for confirming that exit() will be invoked, then we should stop using the devm versions of the allocators. But that said, why are the ioremap done at init and not at probe time? Regards, Bjorn > > 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? > > -- > viresh
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.
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.
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/
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 ?
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. Shawn
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.
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(-)