Re: [PATCH 2/2] opp: Don't create an OPP table from dev_pm_opp_get_opp_table()

[Ulf Hansson]

On Fri, 6 Nov 2020 at 07:25, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> It has been found that some users (like cpufreq-dt and others on LKML)
> have abused the helper dev_pm_opp_get_opp_table() to create the OPP
> table instead of just finding it, which is the wrong thing to do. This
> routine was meant for OPP core's internal working and exposed the whole
> functionality by mistake.
>
> Change the scope of dev_pm_opp_get_opp_table() to only finding the
> table. The internal helpers _opp_get_opp_table*() are thus renamed to
> _add_opp_table*(), dev_pm_opp_get_opp_table_indexed() is removed (as we
> don't need the index field for finding the OPP table) and so the only
> user, genpd, is updated.
>
> Note that the prototype of _add_opp_table() was already left in opp.h by
> mistake when it was removed earlier and so we weren't required to add it
> now.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

Acked-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c |  2 +-
>  drivers/opp/core.c          | 27 +++++++++++++--------------
>  drivers/opp/of.c            |  4 ++--
>  drivers/opp/opp.h           |  1 +
>  include/linux/pm_opp.h      |  1 -
>  5 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 743268996336..92b750b865d5 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2249,7 +2249,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                          * Save table for faster processing while setting
>                          * performance state.
>                          */
> -                       genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i);
> +                       genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev);
>                         WARN_ON(IS_ERR(genpd->opp_table));
>                 }
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 9915e8487f0b..b24f685823ae 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1138,7 +1138,7 @@ void _get_opp_table_kref(struct opp_table *opp_table)
>   * uses the opp_tables_busy flag to indicate if another creator is in the middle
>   * of adding an OPP table and others should wait for it to finish.
>   */
> -static struct opp_table *_opp_get_opp_table(struct device *dev, int index)
> +struct opp_table *_add_opp_table_indexed(struct device *dev, int index)
>  {
>         struct opp_table *opp_table;
>
> @@ -1188,17 +1188,16 @@ static struct opp_table *_opp_get_opp_table(struct device *dev, int index)
>         return opp_table;
>  }
>
> -struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
> +struct opp_table *_add_opp_table(struct device *dev)
>  {
> -       return _opp_get_opp_table(dev, 0);
> +       return _add_opp_table_indexed(dev, 0);
>  }
> -EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_table);
>
> -struct opp_table *dev_pm_opp_get_opp_table_indexed(struct device *dev,
> -                                                  int index)
> +struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
>  {
> -       return _opp_get_opp_table(dev, index);
> +       return _find_opp_table(dev);
>  }
> +EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_table);
>
>  static void _opp_table_kref_release(struct kref *kref)
>  {
> @@ -1627,7 +1626,7 @@ struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev,
>  {
>         struct opp_table *opp_table;
>
> -       opp_table = dev_pm_opp_get_opp_table(dev);
> +       opp_table = _add_opp_table(dev);
>         if (IS_ERR(opp_table))
>                 return opp_table;
>
> @@ -1686,7 +1685,7 @@ struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
>  {
>         struct opp_table *opp_table;
>
> -       opp_table = dev_pm_opp_get_opp_table(dev);
> +       opp_table = _add_opp_table(dev);
>         if (IS_ERR(opp_table))
>                 return opp_table;
>
> @@ -1779,7 +1778,7 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>         struct regulator *reg;
>         int ret, i;
>
> -       opp_table = dev_pm_opp_get_opp_table(dev);
> +       opp_table = _add_opp_table(dev);
>         if (IS_ERR(opp_table))
>                 return opp_table;
>
> @@ -1887,7 +1886,7 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
>         struct opp_table *opp_table;
>         int ret;
>
> -       opp_table = dev_pm_opp_get_opp_table(dev);
> +       opp_table = _add_opp_table(dev);
>         if (IS_ERR(opp_table))
>                 return opp_table;
>
> @@ -1955,7 +1954,7 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
>         if (!set_opp)
>                 return ERR_PTR(-EINVAL);
>
> -       opp_table = dev_pm_opp_get_opp_table(dev);
> +       opp_table = _add_opp_table(dev);
>         if (IS_ERR(opp_table))
>                 return opp_table;
>
> @@ -2039,7 +2038,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>         int index = 0, ret = -EINVAL;
>         const char **name = names;
>
> -       opp_table = dev_pm_opp_get_opp_table(dev);
> +       opp_table = _add_opp_table(dev);
>         if (IS_ERR(opp_table))
>                 return opp_table;
>
> @@ -2204,7 +2203,7 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>         struct opp_table *opp_table;
>         int ret;
>
> -       opp_table = dev_pm_opp_get_opp_table(dev);
> +       opp_table = _add_opp_table(dev);
>         if (IS_ERR(opp_table))
>                 return PTR_ERR(opp_table);
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 9faeb83e4b32..c718092757d9 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -974,7 +974,7 @@ int dev_pm_opp_of_add_table(struct device *dev)
>         struct opp_table *opp_table;
>         int ret;
>
> -       opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0);
> +       opp_table = _add_opp_table_indexed(dev, 0);
>         if (IS_ERR(opp_table))
>                 return PTR_ERR(opp_table);
>
> @@ -1029,7 +1029,7 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
>                         index = 0;
>         }
>
> -       opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
> +       opp_table = _add_opp_table_indexed(dev, index);
>         if (IS_ERR(opp_table))
>                 return PTR_ERR(opp_table);
>
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index ebd930e0b3ca..4ced7ffa8158 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -224,6 +224,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *o
>  int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
>  void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);
>  struct opp_table *_add_opp_table(struct device *dev);
> +struct opp_table *_add_opp_table_indexed(struct device *dev, int index);
>  void _put_opp_list_kref(struct opp_table *opp_table);
>
>  #ifdef CONFIG_OF
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index dbb484524f82..1435c054016a 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -90,7 +90,6 @@ struct dev_pm_set_opp_data {
>  #if defined(CONFIG_PM_OPP)
>
>  struct opp_table *dev_pm_opp_get_opp_table(struct device *dev);
> -struct opp_table *dev_pm_opp_get_opp_table_indexed(struct device *dev, int index);
>  void dev_pm_opp_put_opp_table(struct opp_table *opp_table);
>
>  unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
> --
> 2.25.0.rc1.19.g042ed3e048af
>