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@xxxxxxxxxx>
> Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
> ---
>
> 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.
