Re: [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 16, 2016 at 8:33 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> dev_pm_opp_get_sharing_cpus() returns 0 even in the case where the OPP
> core doesn't know if the table is shared or not. It is working for most
> of the platforms, as the OPP table was never created and we returned
> -ENODEV then.
>
> But in case of one of the platforms (Jetson TK1) at least, the situation
> is a bit different. The OPP table is created (somehow) before
> dev_pm_opp_get_sharing_cpus() is called and so we returned 0. The caller
> of this routine treated that as 'CPUs don't share OPPs' and that had bad
> consequences on performance.
>
> Fix this by converting 'shared_opp' to an integer and have an extra
> value when its state in undefined. dev_pm_opp_get_sharing_cpus() returns
> -EINVAL now in that case, so that the caller can handle it accordingly
> (cpufreq-dt considers that as 'all CPUs share the table').
>
> Fixes: 6f707daa3833 ("PM / OPP: Add dev_pm_opp_get_sharing_cpus()")
> Reported-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
> Hi Alexandre,
>
> This is untested, can you please confirm if this fixes it for you?
>
>  drivers/base/power/opp/core.c |  3 +++
>  drivers/base/power/opp/cpu.c  | 12 +++++++++---
>  drivers/base/power/opp/of.c   | 10 ++++++++--
>  drivers/base/power/opp/opp.h  |  5 ++++-
>  4 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 7c04c87738a6..14d212885098 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -764,6 +764,9 @@ static struct opp_table *_add_opp_table(struct device *dev)
>         /* Set regulator to a non-NULL error value */
>         opp_table->regulator = ERR_PTR(-ENXIO);
>
> +       /* Set sharing information as unknown */
> +       opp_table->shared_opp = OPP_TABLE_SHARED_UNKNOWN;
> +
>         /* Find clk for the device */
>         opp_table->clk = clk_get(dev, NULL);
>         if (IS_ERR(opp_table->clk)) {
> diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c
> index 83d6e7ba1a34..4ca86df8a451 100644
> --- a/drivers/base/power/opp/cpu.c
> +++ b/drivers/base/power/opp/cpu.c
> @@ -211,7 +211,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev,
>                 }
>
>                 /* Mark opp-table as multiple CPUs are sharing it now */
> -               opp_table->shared_opp = true;
> +               opp_table->shared_opp = OPP_TABLE_IS_SHARED;
>         }
>  unlock:
>         mutex_unlock(&opp_table_lock);
> @@ -227,7 +227,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus);
>   *
>   * This updates the @cpumask with CPUs that are sharing OPPs with @cpu_dev.
>   *
> - * Returns -ENODEV if OPP table isn't already present.
> + * Returns -ENODEV if OPP table isn't already present and -EINVAL if the OPP
> + * table's status is shared-unknown.
>   *
>   * Locking: The internal opp_table and opp structures are RCU protected.
>   * Hence this function internally uses RCU updater strategy with mutex locks
> @@ -249,9 +250,14 @@ int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
>                 goto unlock;
>         }
>
> +       if (opp_table->shared_opp == OPP_TABLE_SHARED_UNKNOWN) {
> +               ret = -EINVAL;
> +               goto unlock;
> +       }
> +
>         cpumask_clear(cpumask);
>
> -       if (opp_table->shared_opp) {
> +       if (opp_table->shared_opp == OPP_TABLE_IS_SHARED) {
>                 list_for_each_entry(opp_dev, &opp_table->dev_list, node)
>                         cpumask_set_cpu(opp_dev->dev->id, cpumask);
>         } else {
> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
> index 94d2010558e3..83ad3a6a16f1 100644
> --- a/drivers/base/power/opp/of.c
> +++ b/drivers/base/power/opp/of.c
> @@ -34,7 +34,10 @@ static struct opp_table *_managed_opp(const struct device_node *np)
>                          * But the OPPs will be considered as shared only if the
>                          * OPP table contains a "opp-shared" property.
>                          */
> -                       return opp_table->shared_opp ? opp_table : NULL;
> +                       if (opp_table->shared_opp == OPP_TABLE_IS_SHARED)
> +                               return opp_table;
> +
> +                       return NULL;

That still can be

return opp_table->shared_opp == OPP_TABLE_IS_SHARED ? opp_table : NULL;

>                 }
>         }
>
> @@ -353,7 +356,10 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
>         }
>
>         opp_table->np = opp_np;
> -       opp_table->shared_opp = of_property_read_bool(opp_np, "opp-shared");
> +       if (of_property_read_bool(opp_np, "opp-shared"))
> +               opp_table->shared_opp = OPP_TABLE_IS_SHARED;
> +       else
> +               opp_table->shared_opp = OPP_TABLE_IS_NOT_SHARED;

And here

opp_table->shared_opp = of_property_read_bool(opp_np, "opp-shared") ?
                                            OPP_TABLE_IS_SHARED :
OPP_TABLE_IS_NOT_SHARED;

>
>         mutex_unlock(&opp_table_lock);
>
> diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
> index 20f3be22e060..ffd0b54e7894 100644
> --- a/drivers/base/power/opp/opp.h
> +++ b/drivers/base/power/opp/opp.h
> @@ -166,7 +166,10 @@ struct opp_table {
>         /* For backward compatibility with v1 bindings */
>         unsigned int voltage_tolerance_v1;
>
> -       bool shared_opp;
> +#define OPP_TABLE_IS_NOT_SHARED                0
> +#define OPP_TABLE_IS_SHARED            1
> +#define OPP_TABLE_SHARED_UNKNOWN       UINT_MAX

Please change this into an enum type.

Besides, I'd call them OPP_TABLE_ACCESS_SHARED,
OPP_TABLE_ACCESS_EXCLUSIVE, OPP_TABLE_ACCESS_UNKNOWN or similar, but I
don't care that much either.

> +       unsigned int shared_opp;
>         struct dev_pm_opp *suspend_opp;
>
>         unsigned int *supported_hw;
> --
> 2.7.1.410.g6faf27b
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux