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@xxxxxxxxxx>
> ---
> 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
>
