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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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
>