Re: [PATCH] cpufreq: scpi/scmi: Fix freeing of dynamic OPPs

[Rafael J. Wysocki]

On Fri, Jan 4, 2019 at 10:44 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> Since the commit 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from
> _dev_pm_opp_remove_table()"), dynamically created OPP aren't
> automatically removed anymore by dev_pm_opp_cpumask_remove_table(). This
> affects the scpi and scmi cpufreq drivers which no longer free OPPs on
> failures or on invocations of the policy->exit() callback.
>
> Create a generic OPP helper dev_pm_opp_remove_all_dynamic() which can be
> called from these drivers instead of dev_pm_opp_cpumask_remove_table().
>
> In dev_pm_opp_remove_all_dynamic(), we need to make sure that the
> opp_list isn't getting accessed simultaneously from other parts of the
> OPP core while the helper is freeing dynamic OPPs, i.e. we can't drop
> the opp_table->lock while traversing through the OPP list. And to
> accomplish that, this patch also creates _opp_kref_release_unlocked()
> which can be called from this new helper with the opp_table lock already
> held.
>
> Cc: 4.20 <stable@xxxxxxxxxxxxxxx> # v4.20
> Reported-by: Valentin Schneider <valentin.schneider@xxxxxxx>
> Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()")
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

I guess I'll pick it up by hand.

I'm assuming that you have tested it, have you?

> ---
>  drivers/cpufreq/scmi-cpufreq.c |  4 +--
>  drivers/cpufreq/scpi-cpufreq.c |  4 +--
>  drivers/opp/core.c             | 63 +++++++++++++++++++++++++++++++---
>  include/linux/pm_opp.h         |  5 +++
>  4 files changed, 67 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index 50b1551ba894..c2e66528f5ee 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -176,7 +176,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  out_free_priv:
>         kfree(priv);
>  out_free_opp:
> -       dev_pm_opp_cpumask_remove_table(policy->cpus);
> +       dev_pm_opp_remove_all_dynamic(cpu_dev);
>
>         return ret;
>  }
> @@ -188,7 +188,7 @@ static int scmi_cpufreq_exit(struct cpufreq_policy *policy)
>         cpufreq_cooling_unregister(priv->cdev);
>         dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
>         kfree(priv);
> -       dev_pm_opp_cpumask_remove_table(policy->related_cpus);
> +       dev_pm_opp_remove_all_dynamic(priv->cpu_dev);
>
>         return 0;
>  }
> diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
> index 87a98ec77773..99449738faa4 100644
> --- a/drivers/cpufreq/scpi-cpufreq.c
> +++ b/drivers/cpufreq/scpi-cpufreq.c
> @@ -177,7 +177,7 @@ static int scpi_cpufreq_init(struct cpufreq_policy *policy)
>  out_free_priv:
>         kfree(priv);
>  out_free_opp:
> -       dev_pm_opp_cpumask_remove_table(policy->cpus);
> +       dev_pm_opp_remove_all_dynamic(cpu_dev);
>
>         return ret;
>  }
> @@ -190,7 +190,7 @@ static int scpi_cpufreq_exit(struct cpufreq_policy *policy)
>         clk_put(priv->clk);
>         dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
>         kfree(priv);
> -       dev_pm_opp_cpumask_remove_table(policy->related_cpus);
> +       dev_pm_opp_remove_all_dynamic(priv->cpu_dev);
>
>         return 0;
>  }
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e5507add8f04..18f1639dbc4a 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -988,11 +988,9 @@ void _opp_free(struct dev_pm_opp *opp)
>         kfree(opp);
>  }
>
> -static void _opp_kref_release(struct kref *kref)
> +static void _opp_kref_release(struct dev_pm_opp *opp,
> +                             struct opp_table *opp_table)
>  {
> -       struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref);
> -       struct opp_table *opp_table = opp->opp_table;
> -
>         /*
>          * Notify the changes in the availability of the operable
>          * frequency/voltage list.
> @@ -1002,7 +1000,22 @@ static void _opp_kref_release(struct kref *kref)
>         opp_debug_remove_one(opp);
>         list_del(&opp->node);
>         kfree(opp);
> +}
>
> +static void _opp_kref_release_unlocked(struct kref *kref)
> +{
> +       struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref);
> +       struct opp_table *opp_table = opp->opp_table;
> +
> +       _opp_kref_release(opp, opp_table);
> +}
> +
> +static void _opp_kref_release_locked(struct kref *kref)
> +{
> +       struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref);
> +       struct opp_table *opp_table = opp->opp_table;
> +
> +       _opp_kref_release(opp, opp_table);
>         mutex_unlock(&opp_table->lock);
>  }
>
> @@ -1013,10 +1026,16 @@ void dev_pm_opp_get(struct dev_pm_opp *opp)
>
>  void dev_pm_opp_put(struct dev_pm_opp *opp)
>  {
> -       kref_put_mutex(&opp->kref, _opp_kref_release, &opp->opp_table->lock);
> +       kref_put_mutex(&opp->kref, _opp_kref_release_locked,
> +                      &opp->opp_table->lock);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_put);
>
> +static void dev_pm_opp_put_unlocked(struct dev_pm_opp *opp)
> +{
> +       kref_put(&opp->kref, _opp_kref_release_unlocked);
> +}
> +
>  /**
>   * dev_pm_opp_remove()  - Remove an OPP from OPP table
>   * @dev:       device for which we do this operation
> @@ -1060,6 +1079,40 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
>
> +/**
> + * dev_pm_opp_remove_all_dynamic() - Remove all dynamically created OPPs
> + * @dev:       device for which we do this operation
> + *
> + * This function removes all dynamically created OPPs from the opp table.
> + */
> +void dev_pm_opp_remove_all_dynamic(struct device *dev)
> +{
> +       struct opp_table *opp_table;
> +       struct dev_pm_opp *opp, *temp;
> +       int count = 0;
> +
> +       opp_table = _find_opp_table(dev);
> +       if (IS_ERR(opp_table))
> +               return;
> +
> +       mutex_lock(&opp_table->lock);
> +       list_for_each_entry_safe(opp, temp, &opp_table->opp_list, node) {
> +               if (opp->dynamic) {
> +                       dev_pm_opp_put_unlocked(opp);
> +                       count++;
> +               }
> +       }
> +       mutex_unlock(&opp_table->lock);
> +
> +       /* Drop the references taken by dev_pm_opp_add() */
> +       while (count--)
> +               dev_pm_opp_put_opp_table(opp_table);
> +
> +       /* Drop the reference taken by _find_opp_table() */
> +       dev_pm_opp_put_opp_table(opp_table);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_remove_all_dynamic);
> +
>  struct dev_pm_opp *_opp_allocate(struct opp_table *table)
>  {
>         struct dev_pm_opp *opp;
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 0a2a88e5a383..b895f4e79868 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -108,6 +108,7 @@ void dev_pm_opp_put(struct dev_pm_opp *opp);
>  int dev_pm_opp_add(struct device *dev, unsigned long freq,
>                    unsigned long u_volt);
>  void dev_pm_opp_remove(struct device *dev, unsigned long freq);
> +void dev_pm_opp_remove_all_dynamic(struct device *dev);
>
>  int dev_pm_opp_enable(struct device *dev, unsigned long freq);
>
> @@ -217,6 +218,10 @@ static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq)
>  {
>  }
>
> +static inline void dev_pm_opp_remove_all_dynamic(struct device *dev)
> +{
> +}
> +
>  static inline int dev_pm_opp_enable(struct device *dev, unsigned long freq)
>  {
>         return 0;
> --
> 2.19.1.568.g152ad8e3369a
>