[V2,1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER

Commit Message

Viresh Kumar Aug. 24, 2020, 9:09 a.m. UTC

From: Stephan Gerhold <[e-mail]>

The OPP core manages various resources, e.g. clocks or interconnect paths.
These resources are looked up when the OPP table is allocated once
dev_pm_opp_get_opp_table() is called the first time (either directly
or indirectly through one of the many helper functions).

At this point, the resources may not be available yet, i.e. looking them
up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
is currently unable to propagate this error code since it only returns
the allocated OPP table or NULL.

This means that all consumers of the OPP core are required to make sure
that all necessary resources are available. Usually this happens by
requesting them, checking the result and releasing them immediately after.

For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to
several drivers now just to make sure the interconnect providers are
ready before the OPP table is allocated. If this call is missing,
the OPP core will only warn about this and then attempt to continue
without interconnect. This will eventually fail horribly, e.g.:

    cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517
    ... later ...
    of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0)
    cpu cpu0: _opp_add_static_v2: opp key field not found
    cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22

This example happens when trying to use interconnects for a CPU OPP
table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls
dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table
early. To fix the problem with the current approach we would need to add
yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL).
But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects...

This commit attempts to make this more robust by allowing
dev_pm_opp_get_opp_table() to return an error pointer. Fixing all
the usages is trivial because the function is usually used indirectly
through another helper (e.g. dev_pm_opp_set_supported_hw() above).
These other helpers already return an error pointer.

The example above then works correctly because set_supported_hw() will
return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that
error. It should also be possible to remove the remaining usages of
"dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well.

Note that this commit currently only handles -EPROBE_DEFER for the
clock/interconnects within _allocate_opp_table(). Other errors are just
ignored as before. Eventually those should be propagated as well.

Signed-off-by: Stephan Gerhold <[e-mail]>
[ Viresh: skip checking return value of dev_pm_opp_get_opp_table() for
	  EPROBE_DEFER in domain.c, fix NULL return value and reorder
	  code a bit in core.c, and update exynos-asv.c ]
Signed-off-by: Viresh Kumar <[e-mail]>
---
Stephan, I have made some changes to the code. Please try it again and
lemme know if it works fine.

 drivers/base/power/domain.c      | 14 +++++----
 drivers/opp/core.c               | 53 +++++++++++++++++++-------------
 drivers/opp/of.c                 |  8 ++---
 drivers/soc/samsung/exynos-asv.c |  2 +-
 4 files changed, 44 insertions(+), 33 deletions(-)

Comments

Krzysztof Kozlowski Aug. 24, 2020, 9:17 a.m. UTC | #1

On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> From: Stephan Gerhold <[e-mail]>
> 
> The OPP core manages various resources, e.g. clocks or interconnect paths.
> These resources are looked up when the OPP table is allocated once
> dev_pm_opp_get_opp_table() is called the first time (either directly
> or indirectly through one of the many helper functions).
> 
> At this point, the resources may not be available yet, i.e. looking them
> up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
> is currently unable to propagate this error code since it only returns
> the allocated OPP table or NULL.
> 
> This means that all consumers of the OPP core are required to make sure
> that all necessary resources are available. Usually this happens by
> requesting them, checking the result and releasing them immediately after.
> 
> For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to
> several drivers now just to make sure the interconnect providers are
> ready before the OPP table is allocated. If this call is missing,
> the OPP core will only warn about this and then attempt to continue
> without interconnect. This will eventually fail horribly, e.g.:
> 
>     cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517
>     ... later ...
>     of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0)
>     cpu cpu0: _opp_add_static_v2: opp key field not found
>     cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
> 
> This example happens when trying to use interconnects for a CPU OPP
> table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls
> dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table
> early. To fix the problem with the current approach we would need to add
> yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL).
> But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects...
> 
> This commit attempts to make this more robust by allowing
> dev_pm_opp_get_opp_table() to return an error pointer. Fixing all
> the usages is trivial because the function is usually used indirectly
> through another helper (e.g. dev_pm_opp_set_supported_hw() above).
> These other helpers already return an error pointer.
> 
> The example above then works correctly because set_supported_hw() will
> return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that
> error. It should also be possible to remove the remaining usages of
> "dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well.
> 
> Note that this commit currently only handles -EPROBE_DEFER for the
> clock/interconnects within _allocate_opp_table(). Other errors are just
> ignored as before. Eventually those should be propagated as well.
> 
> Signed-off-by: Stephan Gerhold <[e-mail]>
> [ Viresh: skip checking return value of dev_pm_opp_get_opp_table() for
> 	  EPROBE_DEFER in domain.c, fix NULL return value and reorder
> 	  code a bit in core.c, and update exynos-asv.c ]
> Signed-off-by: Viresh Kumar <[e-mail]>
> ---
> Stephan, I have made some changes to the code. Please try it again and
> lemme know if it works fine.
> 
>  drivers/base/power/domain.c      | 14 +++++----
>  drivers/opp/core.c               | 53 +++++++++++++++++++-------------
>  drivers/opp/of.c                 |  8 ++---
>  drivers/soc/samsung/exynos-asv.c |  2 +-
>  4 files changed, 44 insertions(+), 33 deletions(-)

For Samsung:
Acked-by: Krzysztof Kozlowski <[e-mail]>

Best regards,
Krzysztof

Ulf Hansson Aug. 24, 2020, 11:08 a.m. UTC | #2

On Mon, 24 Aug 2020 at 11:09, Viresh Kumar <[e-mail]> wrote:
>
> From: Stephan Gerhold <[e-mail]>
>
> The OPP core manages various resources, e.g. clocks or interconnect paths.
> These resources are looked up when the OPP table is allocated once
> dev_pm_opp_get_opp_table() is called the first time (either directly
> or indirectly through one of the many helper functions).
>
> At this point, the resources may not be available yet, i.e. looking them
> up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
> is currently unable to propagate this error code since it only returns
> the allocated OPP table or NULL.
>
> This means that all consumers of the OPP core are required to make sure
> that all necessary resources are available. Usually this happens by
> requesting them, checking the result and releasing them immediately after.
>
> For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to
> several drivers now just to make sure the interconnect providers are
> ready before the OPP table is allocated. If this call is missing,
> the OPP core will only warn about this and then attempt to continue
> without interconnect. This will eventually fail horribly, e.g.:
>
>     cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517
>     ... later ...
>     of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0)
>     cpu cpu0: _opp_add_static_v2: opp key field not found
>     cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
>
> This example happens when trying to use interconnects for a CPU OPP
> table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls
> dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table
> early. To fix the problem with the current approach we would need to add
> yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL).
> But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects...
>
> This commit attempts to make this more robust by allowing
> dev_pm_opp_get_opp_table() to return an error pointer. Fixing all
> the usages is trivial because the function is usually used indirectly
> through another helper (e.g. dev_pm_opp_set_supported_hw() above).
> These other helpers already return an error pointer.
>
> The example above then works correctly because set_supported_hw() will
> return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that
> error. It should also be possible to remove the remaining usages of
> "dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well.
>
> Note that this commit currently only handles -EPROBE_DEFER for the
> clock/interconnects within _allocate_opp_table(). Other errors are just
> ignored as before. Eventually those should be propagated as well.
>
> Signed-off-by: Stephan Gerhold <[e-mail]>

Reviewed-by: Ulf Hansson <[e-mail]>

Kind regards
Uffe


> [ Viresh: skip checking return value of dev_pm_opp_get_opp_table() for
>           EPROBE_DEFER in domain.c, fix NULL return value and reorder
>           code a bit in core.c, and update exynos-asv.c ]
> Signed-off-by: Viresh Kumar <[e-mail]>
> ---
> Stephan, I have made some changes to the code. Please try it again and
> lemme know if it works fine.
>
>  drivers/base/power/domain.c      | 14 +++++----
>  drivers/opp/core.c               | 53 +++++++++++++++++++-------------
>  drivers/opp/of.c                 |  8 ++---
>  drivers/soc/samsung/exynos-asv.c |  2 +-
>  4 files changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2cb5e04cf86c..b92bb61550d3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2044,8 +2044,9 @@ int of_genpd_add_provider_simple(struct device_node *np,
>         if (genpd->set_performance_state) {
>                 ret = dev_pm_opp_of_add_table(&genpd->dev);
>                 if (ret) {
> -                       dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> -                               ret);
> +                       if (ret != -EPROBE_DEFER)
> +                               dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> +                                       ret);
>                         goto unlock;
>                 }
>
> @@ -2054,7 +2055,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>                  * state.
>                  */
>                 genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev);
> -               WARN_ON(!genpd->opp_table);
> +               WARN_ON(IS_ERR(genpd->opp_table));
>         }
>
>         ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
> @@ -2111,8 +2112,9 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                 if (genpd->set_performance_state) {
>                         ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
>                         if (ret) {
> -                               dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> -                                       i, ret);
> +                               if (ret != -EPROBE_DEFER)
> +                                       dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> +                                               i, ret);
>                                 goto error;
>                         }
>
> @@ -2121,7 +2123,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                          * performance state.
>                          */
>                         genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i);
> -                       WARN_ON(!genpd->opp_table);
> +                       WARN_ON(IS_ERR(genpd->opp_table));
>                 }
>
>                 genpd->provider = &np->fwnode;
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 6978b9218c6e..8c69a764d0a4 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1068,7 +1068,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>          */
>         opp_table = kzalloc(sizeof(*opp_table), GFP_KERNEL);
>         if (!opp_table)
> -               return NULL;
> +               return ERR_PTR(-ENOMEM);
>
>         mutex_init(&opp_table->lock);
>         mutex_init(&opp_table->genpd_virt_dev_lock);
> @@ -1079,8 +1079,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>
>         opp_dev = _add_opp_dev(dev, opp_table);
>         if (!opp_dev) {
> -               kfree(opp_table);
> -               return NULL;
> +               ret = -ENOMEM;
> +               goto err;
>         }
>
>         _of_init_opp_table(opp_table, dev, index);
> @@ -1089,16 +1089,21 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>         opp_table->clk = clk_get(dev, NULL);
>         if (IS_ERR(opp_table->clk)) {
>                 ret = PTR_ERR(opp_table->clk);
> -               if (ret != -EPROBE_DEFER)
> -                       dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__,
> -                               ret);
> +               if (ret == -EPROBE_DEFER)
> +                       goto err;
> +
> +               dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
>         }
>
>         /* Find interconnect path(s) for the device */
>         ret = dev_pm_opp_of_find_icc_paths(dev, opp_table);
> -       if (ret)
> +       if (ret) {
> +               if (ret == -EPROBE_DEFER)
> +                       goto err;
> +
>                 dev_warn(dev, "%s: Error finding interconnect paths: %d\n",
>                          __func__, ret);
> +       }
>
>         BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
>         INIT_LIST_HEAD(&opp_table->opp_list);
> @@ -1107,6 +1112,10 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>         /* Secure the device table modification */
>         list_add(&opp_table->node, &opp_tables);
>         return opp_table;
> +
> +err:
> +       kfree(opp_table);
> +       return ERR_PTR(ret);
>  }
>
>  void _get_opp_table_kref(struct opp_table *opp_table)
> @@ -1129,7 +1138,7 @@ static struct opp_table *_opp_get_opp_table(struct device *dev, int index)
>         if (opp_table) {
>                 if (!_add_opp_dev_unlocked(dev, opp_table)) {
>                         dev_pm_opp_put_opp_table(opp_table);
> -                       opp_table = NULL;
> +                       opp_table = ERR_PTR(-ENOMEM);
>                 }
>                 goto unlock;
>         }
> @@ -1573,8 +1582,8 @@ 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);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /* Make sure there are no concurrent readers while updating opp_table */
>         WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1632,8 +1641,8 @@ 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);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /* Make sure there are no concurrent readers while updating opp_table */
>         WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1725,8 +1734,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>         int ret, i;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /* This should be called before OPPs are initialized */
>         if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1833,8 +1842,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
>         int ret;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /* This should be called before OPPs are initialized */
>         if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1901,8 +1910,8 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
>                 return ERR_PTR(-EINVAL);
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (!IS_ERR(opp_table))
> +               return opp_table;
>
>         /* This should be called before OPPs are initialized */
>         if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1982,8 +1991,8 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>         const char **name = names;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /*
>          * If the genpd's OPP table isn't already initialized, parsing of the
> @@ -2153,8 +2162,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>         int ret;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return -ENOMEM;
> +       if (IS_ERR(opp_table))
> +               return PTR_ERR(opp_table);
>
>         /* Fix regulator count for dynamic OPPs */
>         opp_table->regulator_count = 1;
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 7d9d4455a59e..e39ddcc779af 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -947,8 +947,8 @@ int dev_pm_opp_of_add_table(struct device *dev)
>         int ret;
>
>         opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0);
> -       if (!opp_table)
> -               return -ENOMEM;
> +       if (IS_ERR(opp_table))
> +               return PTR_ERR(opp_table);
>
>         /*
>          * OPPs have two version of bindings now. Also try the old (v1)
> @@ -1002,8 +1002,8 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
>         }
>
>         opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
> -       if (!opp_table)
> -               return -ENOMEM;
> +       if (IS_ERR(opp_table))
> +               return PTR_ERR(opp_table);
>
>         ret = _of_add_opp_table_v2(dev, opp_table);
>         if (ret)
> diff --git a/drivers/soc/samsung/exynos-asv.c b/drivers/soc/samsung/exynos-asv.c
> index 30bb7b7cc769..8abf4dfaa5c5 100644
> --- a/drivers/soc/samsung/exynos-asv.c
> +++ b/drivers/soc/samsung/exynos-asv.c
> @@ -93,7 +93,7 @@ static int exynos_asv_update_opps(struct exynos_asv *asv)
>                         continue;
>
>                 opp_table = dev_pm_opp_get_opp_table(cpu);
> -               if (IS_ERR_OR_NULL(opp_table))
> +               if (IS_ERR(opp_table))
>                         continue;
>
>                 if (!last_opp_table || opp_table != last_opp_table) {
> --
> 2.25.0.rc1.19.g042ed3e048af
>

Stephan Gerhold Aug. 24, 2020, 11:39 a.m. UTC | #3

On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> From: Stephan Gerhold <[e-mail]>
> 
> The OPP core manages various resources, e.g. clocks or interconnect paths.
> These resources are looked up when the OPP table is allocated once
> dev_pm_opp_get_opp_table() is called the first time (either directly
> or indirectly through one of the many helper functions).
> 
> At this point, the resources may not be available yet, i.e. looking them
> up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
> is currently unable to propagate this error code since it only returns
> the allocated OPP table or NULL.
> 
> This means that all consumers of the OPP core are required to make sure
> that all necessary resources are available. Usually this happens by
> requesting them, checking the result and releasing them immediately after.
> 
> For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to
> several drivers now just to make sure the interconnect providers are
> ready before the OPP table is allocated. If this call is missing,
> the OPP core will only warn about this and then attempt to continue
> without interconnect. This will eventually fail horribly, e.g.:
> 
>     cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517
>     ... later ...
>     of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0)
>     cpu cpu0: _opp_add_static_v2: opp key field not found
>     cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
> 
> This example happens when trying to use interconnects for a CPU OPP
> table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls
> dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table
> early. To fix the problem with the current approach we would need to add
> yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL).
> But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects...
> 
> This commit attempts to make this more robust by allowing
> dev_pm_opp_get_opp_table() to return an error pointer. Fixing all
> the usages is trivial because the function is usually used indirectly
> through another helper (e.g. dev_pm_opp_set_supported_hw() above).
> These other helpers already return an error pointer.
> 
> The example above then works correctly because set_supported_hw() will
> return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that
> error. It should also be possible to remove the remaining usages of
> "dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well.
> 
> Note that this commit currently only handles -EPROBE_DEFER for the
> clock/interconnects within _allocate_opp_table(). Other errors are just
> ignored as before. Eventually those should be propagated as well.
> 
> Signed-off-by: Stephan Gerhold <[e-mail]>
> [ Viresh: skip checking return value of dev_pm_opp_get_opp_table() for
> 	  EPROBE_DEFER in domain.c, fix NULL return value and reorder
> 	  code a bit in core.c, and update exynos-asv.c ]
> Signed-off-by: Viresh Kumar <[e-mail]>
> ---
> Stephan, I have made some changes to the code. Please try it again and
> lemme know if it works fine.
> 

I tested the problem I described in the commit message above
(qcom-cpufreq-nvmem with interconnects) and it works fine
with this patch.

I also tested probe deferal with interconnects with just cpufreq-dt
(second patch), and that works fine as well.

And the changes look good to me too. :)

Thank you!
Stephan

>  drivers/base/power/domain.c      | 14 +++++----
>  drivers/opp/core.c               | 53 +++++++++++++++++++-------------
>  drivers/opp/of.c                 |  8 ++---
>  drivers/soc/samsung/exynos-asv.c |  2 +-
>  4 files changed, 44 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2cb5e04cf86c..b92bb61550d3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2044,8 +2044,9 @@ int of_genpd_add_provider_simple(struct device_node *np,
>  	if (genpd->set_performance_state) {
>  		ret = dev_pm_opp_of_add_table(&genpd->dev);
>  		if (ret) {
> -			dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> -				ret);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> +					ret);
>  			goto unlock;
>  		}
>  
> @@ -2054,7 +2055,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>  		 * state.
>  		 */
>  		genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev);
> -		WARN_ON(!genpd->opp_table);
> +		WARN_ON(IS_ERR(genpd->opp_table));
>  	}
>  
>  	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
> @@ -2111,8 +2112,9 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>  		if (genpd->set_performance_state) {
>  			ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
>  			if (ret) {
> -				dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> -					i, ret);
> +				if (ret != -EPROBE_DEFER)
> +					dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> +						i, ret);
>  				goto error;
>  			}
>  
> @@ -2121,7 +2123,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>  			 * performance state.
>  			 */
>  			genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i);
> -			WARN_ON(!genpd->opp_table);
> +			WARN_ON(IS_ERR(genpd->opp_table));
>  		}
>  
>  		genpd->provider = &np->fwnode;
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 6978b9218c6e..8c69a764d0a4 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1068,7 +1068,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>  	 */
>  	opp_table = kzalloc(sizeof(*opp_table), GFP_KERNEL);
>  	if (!opp_table)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	mutex_init(&opp_table->lock);
>  	mutex_init(&opp_table->genpd_virt_dev_lock);
> @@ -1079,8 +1079,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>  
>  	opp_dev = _add_opp_dev(dev, opp_table);
>  	if (!opp_dev) {
> -		kfree(opp_table);
> -		return NULL;
> +		ret = -ENOMEM;
> +		goto err;
>  	}
>  
>  	_of_init_opp_table(opp_table, dev, index);
> @@ -1089,16 +1089,21 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>  	opp_table->clk = clk_get(dev, NULL);
>  	if (IS_ERR(opp_table->clk)) {
>  		ret = PTR_ERR(opp_table->clk);
> -		if (ret != -EPROBE_DEFER)
> -			dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__,
> -				ret);
> +		if (ret == -EPROBE_DEFER)
> +			goto err;
> +
> +		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
>  	}
>  
>  	/* Find interconnect path(s) for the device */
>  	ret = dev_pm_opp_of_find_icc_paths(dev, opp_table);
> -	if (ret)
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			goto err;
> +
>  		dev_warn(dev, "%s: Error finding interconnect paths: %d\n",
>  			 __func__, ret);
> +	}
>  
>  	BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
>  	INIT_LIST_HEAD(&opp_table->opp_list);
> @@ -1107,6 +1112,10 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>  	/* Secure the device table modification */
>  	list_add(&opp_table->node, &opp_tables);
>  	return opp_table;
> +
> +err:
> +	kfree(opp_table);
> +	return ERR_PTR(ret);
>  }
>  
>  void _get_opp_table_kref(struct opp_table *opp_table)
> @@ -1129,7 +1138,7 @@ static struct opp_table *_opp_get_opp_table(struct device *dev, int index)
>  	if (opp_table) {
>  		if (!_add_opp_dev_unlocked(dev, opp_table)) {
>  			dev_pm_opp_put_opp_table(opp_table);
> -			opp_table = NULL;
> +			opp_table = ERR_PTR(-ENOMEM);
>  		}
>  		goto unlock;
>  	}
> @@ -1573,8 +1582,8 @@ 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);
> -	if (!opp_table)
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(opp_table))
> +		return opp_table;
>  
>  	/* Make sure there are no concurrent readers while updating opp_table */
>  	WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1632,8 +1641,8 @@ 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);
> -	if (!opp_table)
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(opp_table))
> +		return opp_table;
>  
>  	/* Make sure there are no concurrent readers while updating opp_table */
>  	WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1725,8 +1734,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>  	int ret, i;
>  
>  	opp_table = dev_pm_opp_get_opp_table(dev);
> -	if (!opp_table)
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(opp_table))
> +		return opp_table;
>  
>  	/* This should be called before OPPs are initialized */
>  	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1833,8 +1842,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
>  	int ret;
>  
>  	opp_table = dev_pm_opp_get_opp_table(dev);
> -	if (!opp_table)
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(opp_table))
> +		return opp_table;
>  
>  	/* This should be called before OPPs are initialized */
>  	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1901,8 +1910,8 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
>  		return ERR_PTR(-EINVAL);
>  
>  	opp_table = dev_pm_opp_get_opp_table(dev);
> -	if (!opp_table)
> -		return ERR_PTR(-ENOMEM);
> +	if (!IS_ERR(opp_table))
> +		return opp_table;
>  
>  	/* This should be called before OPPs are initialized */
>  	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1982,8 +1991,8 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>  	const char **name = names;
>  
>  	opp_table = dev_pm_opp_get_opp_table(dev);
> -	if (!opp_table)
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(opp_table))
> +		return opp_table;
>  
>  	/*
>  	 * If the genpd's OPP table isn't already initialized, parsing of the
> @@ -2153,8 +2162,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>  	int ret;
>  
>  	opp_table = dev_pm_opp_get_opp_table(dev);
> -	if (!opp_table)
> -		return -ENOMEM;
> +	if (IS_ERR(opp_table))
> +		return PTR_ERR(opp_table);
>  
>  	/* Fix regulator count for dynamic OPPs */
>  	opp_table->regulator_count = 1;
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 7d9d4455a59e..e39ddcc779af 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -947,8 +947,8 @@ int dev_pm_opp_of_add_table(struct device *dev)
>  	int ret;
>  
>  	opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0);
> -	if (!opp_table)
> -		return -ENOMEM;
> +	if (IS_ERR(opp_table))
> +		return PTR_ERR(opp_table);
>  
>  	/*
>  	 * OPPs have two version of bindings now. Also try the old (v1)
> @@ -1002,8 +1002,8 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
>  	}
>  
>  	opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
> -	if (!opp_table)
> -		return -ENOMEM;
> +	if (IS_ERR(opp_table))
> +		return PTR_ERR(opp_table);
>  
>  	ret = _of_add_opp_table_v2(dev, opp_table);
>  	if (ret)
> diff --git a/drivers/soc/samsung/exynos-asv.c b/drivers/soc/samsung/exynos-asv.c
> index 30bb7b7cc769..8abf4dfaa5c5 100644
> --- a/drivers/soc/samsung/exynos-asv.c
> +++ b/drivers/soc/samsung/exynos-asv.c
> @@ -93,7 +93,7 @@ static int exynos_asv_update_opps(struct exynos_asv *asv)
>  			continue;
>  
>  		opp_table = dev_pm_opp_get_opp_table(cpu);
> -		if (IS_ERR_OR_NULL(opp_table))
> +		if (IS_ERR(opp_table))
>  			continue;
>  
>  		if (!last_opp_table || opp_table != last_opp_table) {
> -- 
> 2.25.0.rc1.19.g042ed3e048af
>

Sudeep Holla Oct. 15, 2020, 6:05 p.m. UTC | #4

On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> From: Stephan Gerhold <[e-mail]>
> 
> The OPP core manages various resources, e.g. clocks or interconnect paths.
> These resources are looked up when the OPP table is allocated once
> dev_pm_opp_get_opp_table() is called the first time (either directly
> or indirectly through one of the many helper functions).
> 
> At this point, the resources may not be available yet, i.e. looking them
> up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
> is currently unable to propagate this error code since it only returns
> the allocated OPP table or NULL.
>

OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
directly. Before this change clk_get(dev..) was allowed to fail and
--EPROBE_DEFER was not an error. We use dev_pm_opp_add to add OPPs
read from the firmware and this change is preventing that.

Sorry for checking this so late, but noticed only when this hit mainline.

Viresh Kumar Oct. 16, 2020, 4:24 a.m. UTC | #5

On 15-10-20, 19:05, Sudeep Holla wrote:
> OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> directly. Before this change clk_get(dev..) was allowed to fail and
> --EPROBE_DEFER was not an error.

I think the change in itself is fine. We should be returning from
there if we get EPROBE_DEFER. The question is rather why are you
getting EPROBE_DEFER here ?

> We use dev_pm_opp_add to add OPPs
> read from the firmware and this change is preventing that.
> 
> Sorry for checking this so late, but noticed only when this hit mainline.
> 
> -- 
> Regards,
> Sudeep

Sudeep Holla Oct. 16, 2020, 6 a.m. UTC | #6

On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> On 15-10-20, 19:05, Sudeep Holla wrote:
> > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > directly. Before this change clk_get(dev..) was allowed to fail and
> > --EPROBE_DEFER was not an error.
> 
> I think the change in itself is fine. We should be returning from
> there if we get EPROBE_DEFER. The question is rather why are you
> getting EPROBE_DEFER here ?
> 

Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
this patch and it worked. I need to check it in detail yet.

Sudeep Holla Oct. 16, 2020, 11:12 a.m. UTC | #7

On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > --EPROBE_DEFER was not an error.
> >
> > I think the change in itself is fine. We should be returning from
> > there if we get EPROBE_DEFER. The question is rather why are you
> > getting EPROBE_DEFER here ?
> >
>
> Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> this patch and it worked. I need to check it in detail yet.
>

You confused me earlier. As I said there will be no clock provider
registered for SCMI CPU/Dev DVFS.
	opp_table->clk = clk_get(dev, NULL);
will always return -EPROBE_DEFER as there is no clock provider for dev.
But this change now propagates that error to caller of dev_pm_opp_add
which means we can't add opp to a device if there are no clock providers.
This breaks for DVFS which don't operate separately with clocks and
regulators.

--
Regards,
Sudeep

Stephan Gerhold Oct. 16, 2020, 3:28 p.m. UTC | #8

On Fri, Oct 16, 2020 at 12:12:22PM +0100, Sudeep Holla wrote:
> On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > --EPROBE_DEFER was not an error.
> > >
> > > I think the change in itself is fine. We should be returning from
> > > there if we get EPROBE_DEFER. The question is rather why are you
> > > getting EPROBE_DEFER here ?
> > >
> >
> > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > this patch and it worked. I need to check it in detail yet.
> >
> 
> You confused me earlier. As I said there will be no clock provider
> registered for SCMI CPU/Dev DVFS.
> 	opp_table->clk = clk_get(dev, NULL);
> will always return -EPROBE_DEFER as there is no clock provider for dev.

Shouldn't it return -ENOENT if there is no clock for the SCMI case?
With -EPROBE_DEFER I would expect that it shows up at some point.

I tried to avoid a situation like this by ignoring all errors
!= -EPROBE_DEFER. So if clk_get(dev, NULL) would return -ENOENT
everything should work as expected...

Thanks,
Stephan

Viresh Kumar Oct. 19, 2020, 4:58 a.m. UTC | #9

On 16-10-20, 12:12, Sudeep Holla wrote:
> On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > --EPROBE_DEFER was not an error.
> > >
> > > I think the change in itself is fine. We should be returning from
> > > there if we get EPROBE_DEFER. The question is rather why are you
> > > getting EPROBE_DEFER here ?
> > >
> >
> > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > this patch and it worked. I need to check it in detail yet.
> >
> 
> You confused me earlier. As I said there will be no clock provider
> registered for SCMI CPU/Dev DVFS.
> 	opp_table->clk = clk_get(dev, NULL);
> will always return -EPROBE_DEFER as there is no clock provider for dev.
> But this change now propagates that error to caller of dev_pm_opp_add
> which means we can't add opp to a device if there are no clock providers.
> This breaks for DVFS which don't operate separately with clocks and
> regulators.

The CPUs DT node shouldn't have a clock property in such a case and I
would expect an error instead of EPROBE_DEFER then. Isn't it ?

Sudeep Holla Oct. 19, 2020, 9:17 a.m. UTC | #10

On Mon, Oct 19, 2020 at 10:28:27AM +0530, Viresh Kumar wrote:
> On 16-10-20, 12:12, Sudeep Holla wrote:
> > On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > > --EPROBE_DEFER was not an error.
> > > >
> > > > I think the change in itself is fine. We should be returning from
> > > > there if we get EPROBE_DEFER. The question is rather why are you
> > > > getting EPROBE_DEFER here ?
> > > >
> > >
> > > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > > this patch and it worked. I need to check it in detail yet.
> > >
> > 
> > You confused me earlier. As I said there will be no clock provider
> > registered for SCMI CPU/Dev DVFS.
> > 	opp_table->clk = clk_get(dev, NULL);
> > will always return -EPROBE_DEFER as there is no clock provider for dev.
> > But this change now propagates that error to caller of dev_pm_opp_add
> > which means we can't add opp to a device if there are no clock providers.
> > This breaks for DVFS which don't operate separately with clocks and
> > regulators.
>
> The CPUs DT node shouldn't have a clock property in such a case and I
> would expect an error instead of EPROBE_DEFER then. Isn't it ?

Ideally yes, but for legacy reasons clocks property has been used for
providing OPP/DVFS handle too. While we can change and add new property
for that, it will still break old bindings.

--
Regards,
Sudeep

Viresh Kumar Oct. 19, 2020, 9:24 a.m. UTC | #11

On 19-10-20, 10:17, Sudeep Holla wrote:
> On Mon, Oct 19, 2020 at 10:28:27AM +0530, Viresh Kumar wrote:
> > On 16-10-20, 12:12, Sudeep Holla wrote:
> > > On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > > > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > > > --EPROBE_DEFER was not an error.
> > > > >
> > > > > I think the change in itself is fine. We should be returning from
> > > > > there if we get EPROBE_DEFER. The question is rather why are you
> > > > > getting EPROBE_DEFER here ?
> > > > >
> > > >
> > > > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > > > this patch and it worked. I need to check it in detail yet.
> > > >
> > > 
> > > You confused me earlier. As I said there will be no clock provider
> > > registered for SCMI CPU/Dev DVFS.
> > > 	opp_table->clk = clk_get(dev, NULL);
> > > will always return -EPROBE_DEFER as there is no clock provider for dev.
> > > But this change now propagates that error to caller of dev_pm_opp_add
> > > which means we can't add opp to a device if there are no clock providers.
> > > This breaks for DVFS which don't operate separately with clocks and
> > > regulators.
> >
> > The CPUs DT node shouldn't have a clock property in such a case and I
> > would expect an error instead of EPROBE_DEFER then. Isn't it ?
> 
> Ideally yes, but for legacy reasons clocks property has been used for
> providing OPP/DVFS handle too. While we can change and add new property
> for that, it will still break old bindings.

I am not sure I understood it all. So does your platform have the
clock-names property or not for the CPUs ? And how will something
break here ?

Sudeep Holla Oct. 19, 2020, 10:12 a.m. UTC | #12

On Mon, Oct 19, 2020 at 02:54:11PM +0530, Viresh Kumar wrote:
> On 19-10-20, 10:17, Sudeep Holla wrote:
> > On Mon, Oct 19, 2020 at 10:28:27AM +0530, Viresh Kumar wrote:
> > > On 16-10-20, 12:12, Sudeep Holla wrote:
> > > > On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > > > > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > > > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > > > > --EPROBE_DEFER was not an error.
> > > > > >
> > > > > > I think the change in itself is fine. We should be returning from
> > > > > > there if we get EPROBE_DEFER. The question is rather why are you
> > > > > > getting EPROBE_DEFER here ?
> > > > > >
> > > > >
> > > > > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > > > > this patch and it worked. I need to check it in detail yet.
> > > > >
> > > > 
> > > > You confused me earlier. As I said there will be no clock provider
> > > > registered for SCMI CPU/Dev DVFS.
> > > > 	opp_table->clk = clk_get(dev, NULL);
> > > > will always return -EPROBE_DEFER as there is no clock provider for dev.
> > > > But this change now propagates that error to caller of dev_pm_opp_add
> > > > which means we can't add opp to a device if there are no clock providers.
> > > > This breaks for DVFS which don't operate separately with clocks and
> > > > regulators.
> > >
> > > The CPUs DT node shouldn't have a clock property in such a case and I
> > > would expect an error instead of EPROBE_DEFER then. Isn't it ?
> > 
> > Ideally yes, but for legacy reasons clocks property has been used for
> > providing OPP/DVFS handle too. While we can change and add new property
> > for that, it will still break old bindings.
> 
> I am not sure I understood it all. So does your platform have the
> clock-names property or not for the CPUs ? And how will something
> break here ?
> 

Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
by any clock provider driver. E.g. the issue you will see if "clocks"
property is used instead of "qcom,freq-domain" on Qcom parts.

On SCMI, we have used clocks property to represent perf domain which I
understand is not ideal but it is there 

Viresh Kumar Oct. 19, 2020, 10:35 a.m. UTC | #13

On 19-10-20, 11:12, Sudeep Holla wrote:
> Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> by any clock provider driver. E.g. the issue you will see if "clocks"
> property is used instead of "qcom,freq-domain" on Qcom parts.

Okay, I understand. But what I still don't understand is why it fails
for you. You have a clocks property in DT for the CPU, the OPP core
tries to get it and will get deferred-probed, which will try probing
at a later point of time and it shall work then. Isn't it ?

Sudeep Holla Oct. 19, 2020, 2:10 p.m. UTC | #14

On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> On 19-10-20, 11:12, Sudeep Holla wrote:
> > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > by any clock provider driver. E.g. the issue you will see if "clocks"
> > property is used instead of "qcom,freq-domain" on Qcom parts.
> 
> Okay, I understand. But what I still don't understand is why it fails
> for you. You have a clocks property in DT for the CPU, the OPP core
> tries to get it and will get deferred-probed, which will try probing
> at a later point of time and it shall work then. Isn't it ?
>

Nope unfortunately. We don't have clock provider, so clk_get will
never succeed and always return -EPROBE_DEFER.

Viresh Kumar Oct. 20, 2020, 5:05 a.m. UTC | #15

On 19-10-20, 15:10, Sudeep Holla wrote:
> On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > 
> > Okay, I understand. But what I still don't understand is why it fails
> > for you. You have a clocks property in DT for the CPU, the OPP core
> > tries to get it and will get deferred-probed, which will try probing
> > at a later point of time and it shall work then. Isn't it ?
> >
> 
> Nope unfortunately. We don't have clock provider, so clk_get will
> never succeed and always return -EPROBE_DEFER.

Now this is really bad, you have a fake clocks property, how is the
OPP core supposed to know it ? Damn.

Viresh Kumar Oct. 20, 2020, 5:54 a.m. UTC | #16

On 20-10-20, 10:35, Viresh Kumar wrote:
> On 19-10-20, 15:10, Sudeep Holla wrote:
> > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > > 
> > > Okay, I understand. But what I still don't understand is why it fails
> > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > tries to get it and will get deferred-probed, which will try probing
> > > at a later point of time and it shall work then. Isn't it ?
> > >
> > 
> > Nope unfortunately. We don't have clock provider, so clk_get will
> > never succeed and always return -EPROBE_DEFER.
> 
> Now this is really bad, you have a fake clocks property, how is the
> OPP core supposed to know it ? Damn.

What about instead of fixing the OPP core, which really is doing the
right thing, we fix your driver (as you can't fix the DT) and add a
dummy CPU clk to make it all work ?

Sudeep Holla Oct. 20, 2020, 9:37 a.m. UTC | #17

On Tue, Oct 20, 2020 at 11:24:32AM +0530, Viresh Kumar wrote:
> On 20-10-20, 10:35, Viresh Kumar wrote:
> > On 19-10-20, 15:10, Sudeep Holla wrote:
> > > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > > > 
> > > > Okay, I understand. But what I still don't understand is why it fails
> > > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > > tries to get it and will get deferred-probed, which will try probing
> > > > at a later point of time and it shall work then. Isn't it ?
> > > >
> > > 
> > > Nope unfortunately. We don't have clock provider, so clk_get will
> > > never succeed and always return -EPROBE_DEFER.
> > 
> > Now this is really bad, you have a fake clocks property, how is the
> > OPP core supposed to know it ? Damn.
> 
> What about instead of fixing the OPP core, which really is doing the
> right thing, we fix your driver (as you can't fix the DT) and add a
> dummy CPU clk to make it all work ?
>

I really would avoid that. I would rather change the binding as there is
no single official users of that binding in the upstream tree.

Viresh Kumar Oct. 20, 2020, 9:41 a.m. UTC | #18

On 20-10-20, 10:37, Sudeep Holla wrote:
> On Tue, Oct 20, 2020 at 11:24:32AM +0530, Viresh Kumar wrote:
> > On 20-10-20, 10:35, Viresh Kumar wrote:
> > > On 19-10-20, 15:10, Sudeep Holla wrote:
> > > > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > > > > 
> > > > > Okay, I understand. But what I still don't understand is why it fails
> > > > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > > > tries to get it and will get deferred-probed, which will try probing
> > > > > at a later point of time and it shall work then. Isn't it ?
> > > > >
> > > > 
> > > > Nope unfortunately. We don't have clock provider, so clk_get will
> > > > never succeed and always return -EPROBE_DEFER.
> > > 
> > > Now this is really bad, you have a fake clocks property, how is the
> > > OPP core supposed to know it ? Damn.
> > 
> > What about instead of fixing the OPP core, which really is doing the
> > right thing, we fix your driver (as you can't fix the DT) and add a
> > dummy CPU clk to make it all work ?
> >
> 
> I really would avoid that. I would rather change the binding as there is
> no single official users of that binding in the upstream tree.

But how will you solve backward compatibility thing then ?

Sudeep Holla Oct. 20, 2020, 9:52 a.m. UTC | #19

On Tue, Oct 20, 2020 at 03:11:34PM +0530, Viresh Kumar wrote:
> On 20-10-20, 10:37, Sudeep Holla wrote:
> > On Tue, Oct 20, 2020 at 11:24:32AM +0530, Viresh Kumar wrote:
> > > On 20-10-20, 10:35, Viresh Kumar wrote:
> > > > On 19-10-20, 15:10, Sudeep Holla wrote:
> > > > > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > > > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > > > > > 
> > > > > > Okay, I understand. But what I still don't understand is why it fails
> > > > > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > > > > tries to get it and will get deferred-probed, which will try probing
> > > > > > at a later point of time and it shall work then. Isn't it ?
> > > > > >
> > > > > 
> > > > > Nope unfortunately. We don't have clock provider, so clk_get will
> > > > > never succeed and always return -EPROBE_DEFER.
> > > > 
> > > > Now this is really bad, you have a fake clocks property, how is the
> > > > OPP core supposed to know it ? Damn.
> > > 
> > > What about instead of fixing the OPP core, which really is doing the
> > > right thing, we fix your driver (as you can't fix the DT) and add a
> > > dummy CPU clk to make it all work ?
> > >
> > 
> > I really would avoid that. I would rather change the binding as there is
> > no single official users of that binding in the upstream tree.
> 
> But how will you solve backward compatibility thing then ?
> 

I am just betting on the fact that no users upstream means no backward
compatibility needed. If someone raises issue we need to add backward
compatibility with dummy clk as you suggested.

Viresh Kumar Oct. 20, 2020, 9:59 a.m. UTC | #20

On 20-10-20, 10:52, Sudeep Holla wrote:
> On Tue, Oct 20, 2020 at 03:11:34PM +0530, Viresh Kumar wrote:
> > On 20-10-20, 10:37, Sudeep Holla wrote:
> > > On Tue, Oct 20, 2020 at 11:24:32AM +0530, Viresh Kumar wrote:
> > > > On 20-10-20, 10:35, Viresh Kumar wrote:
> > > > > On 19-10-20, 15:10, Sudeep Holla wrote:
> > > > > > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > > > > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > > > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > > > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > > > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > > > > > > 
> > > > > > > Okay, I understand. But what I still don't understand is why it fails
> > > > > > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > > > > > tries to get it and will get deferred-probed, which will try probing
> > > > > > > at a later point of time and it shall work then. Isn't it ?
> > > > > > >
> > > > > > 
> > > > > > Nope unfortunately. We don't have clock provider, so clk_get will
> > > > > > never succeed and always return -EPROBE_DEFER.
> > > > > 
> > > > > Now this is really bad, you have a fake clocks property, how is the
> > > > > OPP core supposed to know it ? Damn.
> > > > 
> > > > What about instead of fixing the OPP core, which really is doing the
> > > > right thing, we fix your driver (as you can't fix the DT) and add a
> > > > dummy CPU clk to make it all work ?
> > > >
> > > 
> > > I really would avoid that. I would rather change the binding as there is
> > > no single official users of that binding in the upstream tree.
> > 
> > But how will you solve backward compatibility thing then ?
> > 
> 
> I am just betting on the fact that no users upstream means no backward
> compatibility needed. If someone raises issue we need to add backward
> compatibility with dummy clk as you suggested.

Okay. I would have done a change in the OPP core to fix the issue, but
the current code looks correct and we shouldn't change it to satisfy
buggy users. I hope that makes sense.

Guenter Roeck Oct. 27, 2020, 10:24 p.m. UTC | #21

Trying again through different e-mail provider. My previous e-mail
got stuck in spam filters. Apologies if this is received multiple
times.

On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> From: Stephan Gerhold <[e-mail]>
> 
[ ... ]
>  
>  	opp_table = dev_pm_opp_get_opp_table(dev);
> -	if (!opp_table)
> -		return ERR_PTR(-ENOMEM);
> +	if (!IS_ERR(opp_table))
> +		return opp_table;

This should have been

	if (IS_ERR(opp_table))
		return opp_table;

Guenter

Viresh Kumar Oct. 28, 2020, 4:06 a.m. UTC | #22

On 27-10-20, 15:24, Guenter Roeck wrote:
> Trying again through different e-mail provider. My previous e-mail
> got stuck in spam filters. Apologies if this is received multiple
> times.

I received only once :)

> On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> > From: Stephan Gerhold <[e-mail]>
> > 
> [ ... ]

You removed way too much and this lost the context. Keeping the
routine's prototype line would have been useful.

> >  
> >  	opp_table = dev_pm_opp_get_opp_table(dev);
> > -	if (!opp_table)
> > -		return ERR_PTR(-ENOMEM);
> > +	if (!IS_ERR(opp_table))
> > +		return opp_table;
> 
> This should have been
> 
> 	if (IS_ERR(opp_table))
> 		return opp_table;

I believe this is the delta from dev_pm_opp_register_set_opp_helper()
?

A proper fix is already pushed in linux-next for this yesterday.

Guenter Roeck Oct. 28, 2020, 5:29 p.m. UTC | #23

On Wed, Oct 28, 2020 at 09:36:37AM +0530, Viresh Kumar wrote:
> On 27-10-20, 15:24, Guenter Roeck wrote:
> > Trying again through different e-mail provider. My previous e-mail
> > got stuck in spam filters. Apologies if this is received multiple
> > times.
> 
> I received only once :)
> 

I think the original was tagged by gmail as spam because it had too many
recipients. It came back with lots of "rejected by recipient", but I think
they just do that to confuse spammers. So likely no one received it.

> > On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> > > From: Stephan Gerhold <[e-mail]>
> > > 
> > [ ... ]
> 
> You removed way too much and this lost the context. Keeping the
> routine's prototype line would have been useful.
> 
Sorry, the original was a bit more complete.

> > >  
> > >  	opp_table = dev_pm_opp_get_opp_table(dev);
> > > -	if (!opp_table)
> > > -		return ERR_PTR(-ENOMEM);
> > > +	if (!IS_ERR(opp_table))
> > > +		return opp_table;
> > 
> > This should have been
> > 
> > 	if (IS_ERR(opp_table))
> > 		return opp_table;
> 
> I believe this is the delta from dev_pm_opp_register_set_opp_helper()
> ?
> 
Correct.

> A proper fix is already pushed in linux-next for this yesterday.
> 

Great, thanks. I didn't notice because yesterday's build didn't include
the fix, and starting today I dropped the one qemu boot test that reported
it (beaglexm) because that doesn't work anymore for other reasons (due to
commit fb2c599f056, more specifically).

Guenter

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2cb5e04cf86c..b92bb61550d3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2044,8 +2044,9 @@  int of_genpd_add_provider_simple(struct device_node *np,
 	if (genpd->set_performance_state) {
 		ret = dev_pm_opp_of_add_table(&genpd->dev);
 		if (ret) {
-			dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
-				ret);
+			if (ret != -EPROBE_DEFER)
+				dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
+					ret);
 			goto unlock;
 		}
 
@@ -2054,7 +2055,7 @@  int of_genpd_add_provider_simple(struct device_node *np,
 		 * state.
 		 */
 		genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev);
-		WARN_ON(!genpd->opp_table);
+		WARN_ON(IS_ERR(genpd->opp_table));
 	}
 
 	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
@@ -2111,8 +2112,9 @@  int of_genpd_add_provider_onecell(struct device_node *np,
 		if (genpd->set_performance_state) {
 			ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
 			if (ret) {
-				dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
-					i, ret);
+				if (ret != -EPROBE_DEFER)
+					dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
+						i, ret);
 				goto error;
 			}
 
@@ -2121,7 +2123,7 @@  int of_genpd_add_provider_onecell(struct device_node *np,
 			 * performance state.
 			 */
 			genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i);
-			WARN_ON(!genpd->opp_table);
+			WARN_ON(IS_ERR(genpd->opp_table));
 		}
 
 		genpd->provider = &np->fwnode;
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 6978b9218c6e..8c69a764d0a4 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1068,7 +1068,7 @@  static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	 */
 	opp_table = kzalloc(sizeof(*opp_table), GFP_KERNEL);
 	if (!opp_table)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	mutex_init(&opp_table->lock);
 	mutex_init(&opp_table->genpd_virt_dev_lock);
@@ -1079,8 +1079,8 @@  static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 
 	opp_dev = _add_opp_dev(dev, opp_table);
 	if (!opp_dev) {
-		kfree(opp_table);
-		return NULL;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	_of_init_opp_table(opp_table, dev, index);
@@ -1089,16 +1089,21 @@  static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	opp_table->clk = clk_get(dev, NULL);
 	if (IS_ERR(opp_table->clk)) {
 		ret = PTR_ERR(opp_table->clk);
-		if (ret != -EPROBE_DEFER)
-			dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__,
-				ret);
+		if (ret == -EPROBE_DEFER)
+			goto err;
+
+		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
 	}
 
 	/* Find interconnect path(s) for the device */
 	ret = dev_pm_opp_of_find_icc_paths(dev, opp_table);
-	if (ret)
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			goto err;
+
 		dev_warn(dev, "%s: Error finding interconnect paths: %d\n",
 			 __func__, ret);
+	}
 
 	BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
 	INIT_LIST_HEAD(&opp_table->opp_list);
@@ -1107,6 +1112,10 @@  static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	/* Secure the device table modification */
 	list_add(&opp_table->node, &opp_tables);
 	return opp_table;
+
+err:
+	kfree(opp_table);
+	return ERR_PTR(ret);
 }
 
 void _get_opp_table_kref(struct opp_table *opp_table)
@@ -1129,7 +1138,7 @@  static struct opp_table *_opp_get_opp_table(struct device *dev, int index)
 	if (opp_table) {
 		if (!_add_opp_dev_unlocked(dev, opp_table)) {
 			dev_pm_opp_put_opp_table(opp_table);
-			opp_table = NULL;
+			opp_table = ERR_PTR(-ENOMEM);
 		}
 		goto unlock;
 	}
@@ -1573,8 +1582,8 @@  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);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/* Make sure there are no concurrent readers while updating opp_table */
 	WARN_ON(!list_empty(&opp_table->opp_list));
@@ -1632,8 +1641,8 @@  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);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/* Make sure there are no concurrent readers while updating opp_table */
 	WARN_ON(!list_empty(&opp_table->opp_list));
@@ -1725,8 +1734,8 @@  struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
 	int ret, i;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/* This should be called before OPPs are initialized */
 	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
@@ -1833,8 +1842,8 @@  struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
 	int ret;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/* This should be called before OPPs are initialized */
 	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
@@ -1901,8 +1910,8 @@  struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
 		return ERR_PTR(-EINVAL);
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (!IS_ERR(opp_table))
+		return opp_table;
 
 	/* This should be called before OPPs are initialized */
 	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
@@ -1982,8 +1991,8 @@  struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 	const char **name = names;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/*
 	 * If the genpd's OPP table isn't already initialized, parsing of the
@@ -2153,8 +2162,8 @@  int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 	int ret;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return -ENOMEM;
+	if (IS_ERR(opp_table))
+		return PTR_ERR(opp_table);
 
 	/* Fix regulator count for dynamic OPPs */
 	opp_table->regulator_count = 1;
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 7d9d4455a59e..e39ddcc779af 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -947,8 +947,8 @@  int dev_pm_opp_of_add_table(struct device *dev)
 	int ret;
 
 	opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0);
-	if (!opp_table)
-		return -ENOMEM;
+	if (IS_ERR(opp_table))
+		return PTR_ERR(opp_table);
 
 	/*
 	 * OPPs have two version of bindings now. Also try the old (v1)
@@ -1002,8 +1002,8 @@  int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
 	}
 
 	opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
-	if (!opp_table)
-		return -ENOMEM;
+	if (IS_ERR(opp_table))
+		return PTR_ERR(opp_table);
 
 	ret = _of_add_opp_table_v2(dev, opp_table);
 	if (ret)
diff --git a/drivers/soc/samsung/exynos-asv.c b/drivers/soc/samsung/exynos-asv.c
index 30bb7b7cc769..8abf4dfaa5c5 100644
--- a/drivers/soc/samsung/exynos-asv.c
+++ b/drivers/soc/samsung/exynos-asv.c
@@ -93,7 +93,7 @@  static int exynos_asv_update_opps(struct exynos_asv *asv)
 			continue;
 
 		opp_table = dev_pm_opp_get_opp_table(cpu);
-		if (IS_ERR_OR_NULL(opp_table))
+		if (IS_ERR(opp_table))
 			continue;
 
 		if (!last_opp_table || opp_table != last_opp_table) {