diff mbox series

cpufreq: qcom-hw: add missing devm_release_mem_region() call

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

Commit Message

Shawn Guo Jan. 12, 2021, 9:52 a.m. UTC
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(-)

Comments

Bjorn Andersson Jan. 12, 2021, 2:44 p.m. UTC | #1
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
>
Viresh Kumar Jan. 13, 2021, 4:31 a.m. UTC | #2
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?
Viresh Kumar Jan. 13, 2021, 5:06 a.m. UTC | #3
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
AngeloGioacchino Del Regno Jan. 13, 2021, 10:12 p.m. UTC | #4
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/
Viresh Kumar Jan. 14, 2021, 4:58 a.m. UTC | #5
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
Bjorn Andersson Jan. 14, 2021, 5:21 a.m. UTC | #6
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/
Ionela Voinescu Jan. 18, 2021, 11:03 a.m. UTC | #7
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
Shawn Guo Jan. 18, 2021, 12:17 p.m. UTC | #8
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
Viresh Kumar Jan. 19, 2021, 3:36 a.m. UTC | #9
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 mbox series

Patch

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;
 }