linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared
@ 2016-06-16  6:33 Viresh Kumar
  2016-06-16  6:55 ` Alexandre Courbot
  2016-06-16 12:25 ` Rafael J. Wysocki
  0 siblings, 2 replies; 6+ messages in thread
From: Viresh Kumar @ 2016-06-16  6:33 UTC (permalink / raw)
  To: Rafael Wysocki, acourbot, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, gnurou, linux-tegra, Viresh Kumar

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@nvidia.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
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;
 		}
 	}
 
@@ -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;
 
 	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
+	unsigned int shared_opp;
 	struct dev_pm_opp *suspend_opp;
 
 	unsigned int *supported_hw;
-- 
2.7.1.410.g6faf27b

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared
  2016-06-16  6:33 [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared Viresh Kumar
@ 2016-06-16  6:55 ` Alexandre Courbot
  2016-06-16  6:57   ` Viresh Kumar
  2016-06-16 12:25 ` Rafael J. Wysocki
  1 sibling, 1 reply; 6+ messages in thread
From: Alexandre Courbot @ 2016-06-16  6:55 UTC (permalink / raw)
  To: Viresh Kumar, Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, gnurou, linux-tegra

On 06/16/2016 03:33 PM, Viresh Kumar 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@nvidia.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Hi Alexandre,
>
> This is untested, can you please confirm if this fixes it for you?

Yep, with this cpufreq_init() takes the fallback path and cpufreq 
behaves as expected thereafter.

Thanks for reacting so quickly! Can this go into 4.7 fixes?

Tested-by: Alexandre Courbot <acourbot@nvidia.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared
  2016-06-16  6:55 ` Alexandre Courbot
@ 2016-06-16  6:57   ` Viresh Kumar
  0 siblings, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2016-06-16  6:57 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	linaro-kernel, linux-pm, linux-kernel, gnurou, linux-tegra

On 16-06-16, 15:55, Alexandre Courbot wrote:
> On 06/16/2016 03:33 PM, Viresh Kumar 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@nvidia.com>
> >Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >---
> >Hi Alexandre,
> >
> >This is untested, can you please confirm if this fixes it for you?
> 
> Yep, with this cpufreq_init() takes the fallback path and cpufreq behaves as
> expected thereafter.
> 
> Thanks for reacting so quickly! Can this go into 4.7 fixes?

This will :)

> Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks.

-- 
viresh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared
  2016-06-16  6:33 [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared Viresh Kumar
  2016-06-16  6:55 ` Alexandre Courbot
@ 2016-06-16 12:25 ` Rafael J. Wysocki
  2016-06-16 12:48   ` Viresh Kumar
  1 sibling, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2016-06-16 12:25 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, acourbot, Viresh Kumar, Nishanth Menon,
	Stephen Boyd, Lists linaro-kernel, linux-pm,
	Linux Kernel Mailing List, Alexandre Courbot, linux-tegra

On Thu, Jun 16, 2016 at 8:33 AM, Viresh Kumar <viresh.kumar@linaro.org> 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@nvidia.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared
  2016-06-16 12:25 ` Rafael J. Wysocki
@ 2016-06-16 12:48   ` Viresh Kumar
  2016-06-16 13:26     ` Rafael J. Wysocki
  0 siblings, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2016-06-16 12:48 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael Wysocki, acourbot, Viresh Kumar, Nishanth Menon,
	Stephen Boyd, Lists linaro-kernel, linux-pm,
	Linux Kernel Mailing List, Alexandre Courbot, linux-tegra

On 16-06-16, 14:25, Rafael J. Wysocki wrote:
> On Thu, Jun 16, 2016 at 8:33 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +                       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;
> 
> >                 }
> >         }
> >

> > +       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;

Conditional statement for both these cases is getting very long and
if/else looks much more readable. And so I would like to stick with
that, if you allow.

> > +#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.

Sure. This can be done.

-- 
viresh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared
  2016-06-16 12:48   ` Viresh Kumar
@ 2016-06-16 13:26     ` Rafael J. Wysocki
  0 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2016-06-16 13:26 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Rafael Wysocki, acourbot, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Lists linaro-kernel, linux-pm,
	Linux Kernel Mailing List, Alexandre Courbot, linux-tegra

On Thu, Jun 16, 2016 at 2:48 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16-06-16, 14:25, Rafael J. Wysocki wrote:
>> On Thu, Jun 16, 2016 at 8:33 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > +                       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;
>>
>> >                 }
>> >         }
>> >
>
>> > +       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;
>
> Conditional statement for both these cases is getting very long and
> if/else looks much more readable. And so I would like to stick with
> that, if you allow.

Fair enough.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-06-16 13:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-16  6:33 [PATCH] PM / OPP: 'UNKNOWN' status of opp-table->shared Viresh Kumar
2016-06-16  6:55 ` Alexandre Courbot
2016-06-16  6:57   ` Viresh Kumar
2016-06-16 12:25 ` Rafael J. Wysocki
2016-06-16 12:48   ` Viresh Kumar
2016-06-16 13:26     ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).