| Message ID | 20210118130603.16176-1-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: qcom-hw: drop devm_xxx() calls from init/exit hooks | expand |
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
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 --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; }
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(-) -- 2.17.1