Re: [PATCH v2] cpufreq: apple-soc: Fix OPP table cleanup

[Viresh Kumar]

On 24-06-26, 11:37, Haoxiang Li wrote:
> dev_pm_opp_of_add_table() adds the DT OPP table but not all failure
> paths remove it. The driver also uses dev_pm_opp_remove_all_dynamic(),
> which is not the right cleanup helper for OPPs loaded from firmware.
> 
> Remove the DT OPP table with dev_pm_opp_of_remove_table() on init
> failure paths and from apple_soc_cpufreq_exit().
> 
> Signed-off-by: Haoxiang Li <haoxiang_li2024@xxxxxxx>

Please add Fixes and Stable tags.

> diff --git a/drivers/cpufreq/apple-soc-cpufreq.c b/drivers/cpufreq/apple-soc-cpufreq.c
> @@ -320,10 +320,10 @@ static int apple_soc_cpufreq_init(struct cpufreq_policy *policy)
>  	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
>  out_free_priv:
>  	kfree(priv);
> -out_free_opp:
> -	dev_pm_opp_remove_all_dynamic(cpu_dev);
>  out_iounmap:
>  	iounmap(reg_base);
> +out_remove_opp_table:
> +	dev_pm_opp_of_remove_table(cpu_dev);
>  	return ret;
>  }
>  
> @@ -332,7 +332,7 @@ static void apple_soc_cpufreq_exit(struct cpufreq_policy *policy)
>  	struct apple_cpu_priv *priv = policy->driver_data;
>  
>  	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
> -	dev_pm_opp_remove_all_dynamic(priv->cpu_dev);
> +	dev_pm_opp_of_remove_table(priv->cpu_dev);
>  	iounmap(priv->reg_base);
>  	kfree(priv);

The order of cleanup calls should match in the above two sequences and there are
other missing pieces as well.

I think this would fix it:

diff --git a/drivers/cpufreq/apple-soc-cpufreq.c b/drivers/cpufreq/apple-soc-cpufreq.c
index 9396034167e5..150deae8c31d 100644
--- a/drivers/cpufreq/apple-soc-cpufreq.c
+++ b/drivers/cpufreq/apple-soc-cpufreq.c
@@ -251,21 +251,19 @@ static int apple_soc_cpufreq_init(struct cpufreq_policy *policy)
                return -ENODEV;
        }

-       ret = dev_pm_opp_of_add_table(cpu_dev);
-       if (ret < 0) {
-               dev_err(cpu_dev, "%s: failed to add OPP table: %d\n", __func__, ret);
-               return ret;
-       }
+       priv = kzalloc_obj(*priv);
+       if (!priv)
+               return -ENOMEM;

        ret = apple_soc_cpufreq_find_cluster(policy, &reg_base, &info);
        if (ret) {
                dev_err(cpu_dev, "%s: failed to get cluster info: %d\n", __func__, ret);
-               return ret;
+               goto out_priv;
        }

-       ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
-       if (ret) {
-               dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", __func__, ret);
+       ret = dev_pm_opp_of_cpumask_add_table(policy->cpus);
+       if (ret < 0) {
+               dev_err(cpu_dev, "%s: failed to add OPP table: %d\n", __func__, ret);
                goto out_iounmap;
        }

@@ -273,19 +271,13 @@ static int apple_soc_cpufreq_init(struct cpufreq_policy *policy)
        if (ret <= 0) {
                dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
                ret = -EPROBE_DEFER;
-               goto out_free_opp;
-       }
-
-       priv = kzalloc_obj(*priv);
-       if (!priv) {
-               ret = -ENOMEM;
-               goto out_free_opp;
+               goto out_free_table;
        }

        ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
        if (ret) {
                dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
-               goto out_free_priv;
+               goto out_free_table;
        }

        /* Get OPP levels (p-state indexes) and stash them in driver_data */
@@ -320,12 +312,12 @@ static int apple_soc_cpufreq_init(struct cpufreq_policy *policy)

 out_free_cpufreq_table:
        dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
-out_free_priv:
-       kfree(priv);
-out_free_opp:
-       dev_pm_opp_remove_all_dynamic(cpu_dev);
+out_free_table:
+       dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 out_iounmap:
        iounmap(reg_base);
+out_free_priv:
+       kfree(priv);
        return ret;
 }

@@ -334,7 +326,7 @@ static void apple_soc_cpufreq_exit(struct cpufreq_policy *policy)
        struct apple_cpu_priv *priv = policy->driver_data;

        dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
-       dev_pm_opp_remove_all_dynamic(priv->cpu_dev);
+       dev_pm_opp_of_cpumask_remove_table(policy->cpus);
        iounmap(priv->reg_base);
        kfree(priv);
 }

-- 
viresh