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

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

[Viresh Kumar]

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@vger.kernel.org> # v4.20
Reported-by: Valentin Schneider <valentin.schneider@arm.com>
Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()")
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 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

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@linaro.org> 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@vger.kernel.org> # v4.20
> Reported-by: Valentin Schneider <valentin.schneider@arm.com>
> Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()")
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

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
>

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

[Viresh Kumar]

On 04-01-19, 11:10, Rafael J. Wysocki wrote:
> On Fri, Jan 4, 2019 at 10:44 AM Viresh Kumar <viresh.kumar@linaro.org> 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@vger.kernel.org> # v4.20
> > Reported-by: Valentin Schneider <valentin.schneider@arm.com>
> > Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()")
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> I guess I'll pick it up by hand.

Sure.

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

Yes, but I had to fake few dynamic OPPs and ignore the static ones
coming from DT. Lets wait for Sudeep or Valentin to test this, who
have real hardware to fix with this patch.

-- 
viresh

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

[Valentin Schneider]

Hi,

On 04/01/2019 09:44, Viresh Kumar 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@vger.kernel.org> # v4.20
> Reported-by: Valentin Schneider <valentin.schneider@arm.com>
> Fixes: 2a4eb7358aba ("OPP: Don't remove dynamic OPPs from _dev_pm_opp_remove_table()")
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Gave it a spin on my Juno, and I don't see the refcount warning anymore so:

Tested-by: Valentin Schneider <valentin.schneider@arm.com>

Thanks for having a look at this.

[...]

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

[Sudeep Holla]

On Fri, Jan 04, 2019 at 03:14:33PM +0530, Viresh Kumar 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.
>

I did test it but since I couldn't reproduce the original issue, my
tested-by is not that worth. Anyways the changes look fine to me.

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

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

[Rafael J. Wysocki]

On Friday, January 4, 2019 12:01:42 PM CET Sudeep Holla wrote:
> On Fri, Jan 04, 2019 at 03:14:33PM +0530, Viresh Kumar 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.
> >
> 
> I did test it but since I couldn't reproduce the original issue, my
> tested-by is not that worth. Anyways the changes look fine to me.
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Applied and pushed to Linus, thanks!