[PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Shawn Guo]

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));
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Viresh Kumar]

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

-- 
viresh

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Shawn Guo]

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

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Ionela Voinescu]

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

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Bjorn Andersson]

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
> 

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Viresh Kumar]

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

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Bjorn Andersson]

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

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Viresh Kumar]

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

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[AngeloGioacchino Del Regno]

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/

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Viresh Kumar]

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

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Bjorn Andersson]

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/

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Viresh Kumar]

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
?

-- 
viresh

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Shawn Guo]

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

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Ionela Voinescu]

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

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Shawn Guo]

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

Re: [PATCH] cpufreq: qcom-hw: add missing devm_release_mem_region() call

[Viresh Kumar]

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