diff mbox series

cpufreq: qcom-hw: drop devm_xxx() calls from init/exit hooks

Message ID 20210118130603.16176-1-shawn.guo@linaro.org (mailing list archive)
State New, archived
Delegated to: viresh kumar
Headers show
Series cpufreq: qcom-hw: drop devm_xxx() calls from init/exit hooks | expand

Commit Message

Shawn Guo Jan. 18, 2021, 1:06 p.m. UTC
Commit f17b3e44320b ("cpufreq: qcom-hw: Use
devm_platform_ioremap_resource() to simplify code") introduces
a regression on platforms using the driver, by failing to initialise
a policy, when one is created post hotplug.

When all the CPUs of a policy are hoptplugged out, the call to .exit()
and later to devm_iounmap() does not release the memory region that was
requested during devm_platform_ioremap_resource().  Therefore,
a subsequent call to .init() will result in the following error, which
will prevent a new policy to be initialised:

[ 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]

With that being said, the original code was tricky and skipping memory
region request intentionally to hide this issue.  The true cause is that
those devm_xxx() device managed functions shouldn't be used for cpufreq
init/exit hooks, because &pdev->dev is alive across the hooks and will
not trigger auto resource free-up.  Let's drop the use of device managed
functions and manually allocate/free resources, so that the issue can be
fixed properly.

Fixes: f17b3e44320b ("cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code")
Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---

I took some of the commit log from Ionela.

 drivers/cpufreq/qcom-cpufreq-hw.c | 43 ++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 10 deletions(-)

Comments

Ionela Voinescu Jan. 18, 2021, 3:38 p.m. UTC | #1
Hi,

On Monday 18 Jan 2021 at 21:06:03 (+0800), Shawn Guo wrote:
> Commit f17b3e44320b ("cpufreq: qcom-hw: Use
> devm_platform_ioremap_resource() to simplify code") introduces
> a regression on platforms using the driver, by failing to initialise
> a policy, when one is created post hotplug.
> 
> When all the CPUs of a policy are hoptplugged out, the call to .exit()
> and later to devm_iounmap() does not release the memory region that was
> requested during devm_platform_ioremap_resource().  Therefore,
> a subsequent call to .init() will result in the following error, which
> will prevent a new policy to be initialised:
> 
> [ 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]
> 
> With that being said, the original code was tricky and skipping memory
> region request intentionally to hide this issue.  The true cause is that
> those devm_xxx() device managed functions shouldn't be used for cpufreq
> init/exit hooks, because &pdev->dev is alive across the hooks and will
> not trigger auto resource free-up.  Let's drop the use of device managed
> functions and manually allocate/free resources, so that the issue can be
> fixed properly.
> 
> Fixes: f17b3e44320b ("cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code")
> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> 
> I took some of the commit log from Ionela.
> 
>  drivers/cpufreq/qcom-cpufreq-hw.c | 43 ++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 9ed5341dc515..b529b49649e0 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,18 +305,33 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  
>  	index = args.args[0];
>  
> -	base = devm_platform_ioremap_resource(pdev, index);
> -	if (IS_ERR(base))
> -		return PTR_ERR(base);
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
>  

Nit: you could move this allocation after all resource reservation and
mapping below, possibly to avoid doing it unless the base address and
the memory resource is actually valid. Or you can keep it here and
remove the use of the local variables, especially the "base" variable.

> -	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> -	if (!data) {
> -		ret = -ENOMEM;
> -		goto error;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +	if (!res) {
> +		dev_err(dev, "failed to get mem resource %d\n", index);
> +		ret = -ENODEV;
> +		goto free_data;
> +	}
> +
> +	if (!request_mem_region(res->start, resource_size(res), res->name)) {
> +		dev_err(dev, "failed to request resource %pR\n", res);
> +		ret = -EBUSY;
> +		goto free_data;
> +	}
> +
> +	base = ioremap(res->start, resource_size(res));
> +	if (IS_ERR(base)) {
> +		dev_err(dev, "failed to map resource %pR\n", res);
> +		ret = PTR_ERR(base);
> +		goto release_region;
>  	}
>  
>  	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)) {
> @@ -349,7 +366,11 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  
>  	return 0;
>  error:
> -	devm_iounmap(dev, base);
> +	iounmap(data->base);
> +release_region:
> +	release_mem_region(res->start, resource_size(res));
> +free_data:
> +	kfree(data);
>  	return ret;
>  }
>  
> @@ -357,12 +378,14 @@ 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);
> +	iounmap(data->base);
> +	release_mem_region(res->start, resource_size(res));
> +	kfree(data);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 


I only mentioned a small nit above. Otherwise it looks good to me and it
works nicely on DB845c.

Many thanks,
Ionela.
Shawn Guo Jan. 19, 2021, 1:56 a.m. UTC | #2
On Mon, Jan 18, 2021 at 03:38:23PM +0000, Ionela Voinescu wrote:
> Hi,
> 
> On Monday 18 Jan 2021 at 21:06:03 (+0800), Shawn Guo wrote:
> > Commit f17b3e44320b ("cpufreq: qcom-hw: Use
> > devm_platform_ioremap_resource() to simplify code") introduces
> > a regression on platforms using the driver, by failing to initialise
> > a policy, when one is created post hotplug.
> > 
> > When all the CPUs of a policy are hoptplugged out, the call to .exit()
> > and later to devm_iounmap() does not release the memory region that was
> > requested during devm_platform_ioremap_resource().  Therefore,
> > a subsequent call to .init() will result in the following error, which
> > will prevent a new policy to be initialised:
> > 
> > [ 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]
> > 
> > With that being said, the original code was tricky and skipping memory
> > region request intentionally to hide this issue.  The true cause is that
> > those devm_xxx() device managed functions shouldn't be used for cpufreq
> > init/exit hooks, because &pdev->dev is alive across the hooks and will
> > not trigger auto resource free-up.  Let's drop the use of device managed
> > functions and manually allocate/free resources, so that the issue can be
> > fixed properly.
> > 
> > Fixes: f17b3e44320b ("cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code")
> > Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> > 
> > I took some of the commit log from Ionela.
> > 
> >  drivers/cpufreq/qcom-cpufreq-hw.c | 43 ++++++++++++++++++++++++-------
> >  1 file changed, 33 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index 9ed5341dc515..b529b49649e0 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,18 +305,33 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >  
> >  	index = args.args[0];
> >  
> > -	base = devm_platform_ioremap_resource(pdev, index);
> > -	if (IS_ERR(base))
> > -		return PTR_ERR(base);
> > +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> >  
> 
> Nit: you could move this allocation after all resource reservation and
> mapping below, possibly to avoid doing it unless the base address and
> the memory resource is actually valid. Or you can keep it here and
> remove the use of the local variables, especially the "base" variable.

It's a reasonable suggestion.  I will send a new version to kill `base`
variable, but still want to keep `res` as it saves some two level
indirection.

Shawn
Shawn Guo Jan. 19, 2021, 2:05 a.m. UTC | #3
On Tue, Jan 19, 2021 at 9:56 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > @@ -303,18 +305,33 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> > >
> > >     index = args.args[0];
> > >
> > > -   base = devm_platform_ioremap_resource(pdev, index);
> > > -   if (IS_ERR(base))
> > > -           return PTR_ERR(base);
> > > +   data = kzalloc(sizeof(*data), GFP_KERNEL);
> > > +   if (!data)
> > > +           return -ENOMEM;
> > >
> >
> > Nit: you could move this allocation after all resource reservation and
> > mapping below, possibly to avoid doing it unless the base address and
> > the memory resource is actually valid. Or you can keep it here and
> > remove the use of the local variables, especially the "base" variable.
>
> It's a reasonable suggestion.  I will send a new version to kill `base`
> variable, but still want to keep `res` as it saves some two level
> indirection.

I'm changing my mind :)  I will move kzalloc() to respect the original code.

Shawn
diff mbox series

Patch

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 9ed5341dc515..b529b49649e0 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,18 +305,33 @@  static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 
 	index = args.args[0];
 
-	base = devm_platform_ioremap_resource(pdev, index);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
 
-	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
-	if (!data) {
-		ret = -ENOMEM;
-		goto error;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
+	if (!res) {
+		dev_err(dev, "failed to get mem resource %d\n", index);
+		ret = -ENODEV;
+		goto free_data;
+	}
+
+	if (!request_mem_region(res->start, resource_size(res), res->name)) {
+		dev_err(dev, "failed to request resource %pR\n", res);
+		ret = -EBUSY;
+		goto free_data;
+	}
+
+	base = ioremap(res->start, resource_size(res));
+	if (IS_ERR(base)) {
+		dev_err(dev, "failed to map resource %pR\n", res);
+		ret = PTR_ERR(base);
+		goto release_region;
 	}
 
 	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)) {
@@ -349,7 +366,11 @@  static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 
 	return 0;
 error:
-	devm_iounmap(dev, base);
+	iounmap(data->base);
+release_region:
+	release_mem_region(res->start, resource_size(res));
+free_data:
+	kfree(data);
 	return ret;
 }
 
@@ -357,12 +378,14 @@  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);
+	iounmap(data->base);
+	release_mem_region(res->start, resource_size(res));
+	kfree(data);
 
 	return 0;
 }