[PATCH V4] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[PATCH V4] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Viresh Kumar]

From: Stephen Boyd <sboyd@codeaurora.org>

Joonyoung Shim reported an interesting problem on his ARM octa-core
Odoroid-XU3 platform. During system suspend, dev_pm_opp_put_regulator()
was failing for a struct device for which dev_pm_opp_set_regulator() is
called earlier.

This happened because an earlier call to
dev_pm_opp_of_cpumask_remove_table() function (from cpufreq-dt.c file)
removed all the entries from opp_table->dev_list apart from the last CPU
device in the cpumask of CPUs sharing the OPP.

But both dev_pm_opp_set_regulator() and dev_pm_opp_put_regulator()
routines get CPU device for the first CPU in the cpumask. And so the OPP
core failed to find the OPP table for the struct device.

In order to fix that up properly, we need to revisit APIs like
dev_pm_opp_set_regulator() and make them talk in terms of cookies
provided by the OPP core. But such a solution will be hard to backport
to stable kernels.

This patch attempts to fix this problem by returning a pointer to the
opp_table from dev_pm_opp_set_regulator() and using that as the
parameter to dev_pm_opp_put_regulator(). This ensures that the
dev_pm_opp_put_regulator() doesn't fail to find the opp table.

Note that similar design problem also exists with other
dev_pm_opp_put_*() APIs, but those aren't used currently by anyone and
so we don't need to update them for now.

[Viresh]: Written commit log, minor improvements in the patch and tested
on exynos 5250.

Cc: # v4.4+ <stable@vger.kernel.org>
Reported-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V3->V4:
- Completely different approach, suggested earlier by Stephen.
- Can be merged safely now as both /me and Stephen agree to this one.

@Joonyoung: Can you please test this last patch please ?

 drivers/base/power/opp/core.c | 37 +++++++++++++------------------------
 drivers/cpufreq/cpufreq-dt.c  | 12 ++++++++----
 include/linux/pm_opp.h        | 11 ++++++-----
 3 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 4c7c6da7a989..de27562d9ced 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1316,58 +1316,57 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
  * that this function is *NOT* called under RCU protection or in contexts where
  * mutex cannot be locked.
  */
-int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name)
 {
 	struct opp_table *opp_table;
 	struct regulator *reg;
-	int ret;
 
 	mutex_lock(&opp_table_lock);
 
 	opp_table = _add_opp_table(dev);
 	if (!opp_table) {
-		ret = -ENOMEM;
+		opp_table = ERR_PTR(-ENOMEM);
 		goto unlock;
 	}
 
 	/* This should be called before OPPs are initialized */
 	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
-		ret = -EBUSY;
+		opp_table = ERR_PTR(-EBUSY);
 		goto err;
 	}
 
 	/* Already have a regulator set */
 	if (WARN_ON(!IS_ERR(opp_table->regulator))) {
-		ret = -EBUSY;
+		opp_table = ERR_PTR(-EBUSY);
 		goto err;
 	}
 	/* Allocate the regulator */
 	reg = regulator_get_optional(dev, name);
 	if (IS_ERR(reg)) {
-		ret = PTR_ERR(reg);
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "%s: no regulator (%s) found: %d\n",
-				__func__, name, ret);
+		opp_table = (struct opp_table *)reg;
+		if (PTR_ERR(reg) != -EPROBE_DEFER)
+			dev_err(dev, "%s: no regulator (%s) found: %ld\n",
+				__func__, name, PTR_ERR(reg));
 		goto err;
 	}
 
 	opp_table->regulator = reg;
 
 	mutex_unlock(&opp_table_lock);
-	return 0;
+	return opp_table;
 
 err:
 	_remove_opp_table(opp_table);
 unlock:
 	mutex_unlock(&opp_table_lock);
 
-	return ret;
+	return opp_table;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
 
 /**
  * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
- * @dev: Device for which regulator was set.
+ * @opp_table: opp_table returned from dev_pm_opp_set_regulator
  *
  * Locking: The internal opp_table and opp structures are RCU protected.
  * Hence this function internally uses RCU updater strategy with mutex locks
@@ -1375,22 +1374,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
  * that this function is *NOT* called under RCU protection or in contexts where
  * mutex cannot be locked.
  */
-void dev_pm_opp_put_regulator(struct device *dev)
+void dev_pm_opp_put_regulator(struct opp_table *opp_table)
 {
-	struct opp_table *opp_table;
-
 	mutex_lock(&opp_table_lock);
 
-	/* Check for existing table for 'dev' first */
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		dev_err(dev, "Failed to find opp_table: %ld\n",
-			PTR_ERR(opp_table));
-		goto unlock;
-	}
-
 	if (IS_ERR(opp_table->regulator)) {
-		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+		pr_err("%s: Doesn't have regulator set\n", __func__);
 		goto unlock;
 	}
 
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 5c07ae05d69a..4d3ec92cbabf 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -28,6 +28,7 @@
 #include "cpufreq-dt.h"
 
 struct private_data {
+	struct opp_table *opp_table;
 	struct device *cpu_dev;
 	struct thermal_cooling_device *cdev;
 	const char *reg_name;
@@ -143,6 +144,7 @@ static int resources_available(void)
 static int cpufreq_init(struct cpufreq_policy *policy)
 {
 	struct cpufreq_frequency_table *freq_table;
+	struct opp_table *opp_table = NULL;
 	struct private_data *priv;
 	struct device *cpu_dev;
 	struct clk *cpu_clk;
@@ -186,8 +188,9 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	 */
 	name = find_supply_name(cpu_dev);
 	if (name) {
-		ret = dev_pm_opp_set_regulator(cpu_dev, name);
-		if (ret) {
+		opp_table = dev_pm_opp_set_regulator(cpu_dev, name);
+		if (IS_ERR(opp_table)) {
+			ret = PTR_ERR(opp_table);
 			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
 				policy->cpu, ret);
 			goto out_put_clk;
@@ -237,6 +240,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	priv->reg_name = name;
+	priv->opp_table = opp_table;
 
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
@@ -285,7 +289,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 out_free_opp:
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (name)
-		dev_pm_opp_put_regulator(cpu_dev);
+		dev_pm_opp_put_regulator(opp_table);
 out_put_clk:
 	clk_put(cpu_clk);
 
@@ -300,7 +304,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
 	if (priv->reg_name)
-		dev_pm_opp_put_regulator(priv->cpu_dev);
+		dev_pm_opp_put_regulator(priv->opp_table);
 
 	clk_put(policy->clk);
 	kfree(priv);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index bca26157f5b6..f6bc76501912 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -19,6 +19,7 @@
 
 struct dev_pm_opp;
 struct device;
+struct opp_table;
 
 enum dev_pm_opp_event {
 	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
@@ -62,8 +63,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
 void dev_pm_opp_put_supported_hw(struct device *dev);
 int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
-int dev_pm_opp_set_regulator(struct device *dev, const char *name);
-void dev_pm_opp_put_regulator(struct device *dev);
+struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name);
+void dev_pm_opp_put_regulator(struct opp_table *opp_table);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -170,12 +171,12 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 
 static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
 
-static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+static inline struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name)
 {
-	return -ENOTSUPP;
+	return ERR_PTR(-ENOTSUPP);
 }
 
-static inline void dev_pm_opp_put_regulator(struct device *dev) {}
+static inline void dev_pm_opp_put_regulator(struct opp_table *opp_table) {}
 
 static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
-- 
2.7.1.410.g6faf27b

Re: [PATCH V4] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Joonyoung Shim]

Hi Viresh,

On 11/30/2016 12:59 PM, Viresh Kumar wrote:
> From: Stephen Boyd <sboyd@codeaurora.org>
> 
> Joonyoung Shim reported an interesting problem on his ARM octa-core
> Odoroid-XU3 platform. During system suspend, dev_pm_opp_put_regulator()
> was failing for a struct device for which dev_pm_opp_set_regulator() is
> called earlier.
> 
> This happened because an earlier call to
> dev_pm_opp_of_cpumask_remove_table() function (from cpufreq-dt.c file)
> removed all the entries from opp_table->dev_list apart from the last CPU
> device in the cpumask of CPUs sharing the OPP.
> 
> But both dev_pm_opp_set_regulator() and dev_pm_opp_put_regulator()
> routines get CPU device for the first CPU in the cpumask. And so the OPP
> core failed to find the OPP table for the struct device.
> 
> In order to fix that up properly, we need to revisit APIs like
> dev_pm_opp_set_regulator() and make them talk in terms of cookies
> provided by the OPP core. But such a solution will be hard to backport
> to stable kernels.
> 
> This patch attempts to fix this problem by returning a pointer to the
> opp_table from dev_pm_opp_set_regulator() and using that as the
> parameter to dev_pm_opp_put_regulator(). This ensures that the
> dev_pm_opp_put_regulator() doesn't fail to find the opp table.
> 
> Note that similar design problem also exists with other
> dev_pm_opp_put_*() APIs, but those aren't used currently by anyone and
> so we don't need to update them for now.
> 
> [Viresh]: Written commit log, minor improvements in the patch and tested
> on exynos 5250.
> 
> Cc: # v4.4+ <stable@vger.kernel.org>
> Reported-by: Joonyoung Shim <jy0922.shim@samsung.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V3->V4:
> - Completely different approach, suggested earlier by Stephen.
> - Can be merged safely now as both /me and Stephen agree to this one.
> 
> @Joonyoung: Can you please test this last patch please ?
> 

Just system suspend/resume is working but i was missing below test case
that you inform when i test for prior patches on my Odroid-XU3 board.

- offline CPU 4
- suspend the system

With this test case, now all patches posted have the problem that is
failed to get clk: -2.

If all CPUs are online, cpufreq_driver->init(policy) is called by
cpufreq_online() only for CPU0 and CPU4 during system resume but it is
called for CPU5, CPU6 and CPU7 during system resume after CPU4 is
offline, and causes error.

Please refer below logs.

# echo 0 > /sys/devices/system/cpu/cpu4/online 
[  145.438931] IRQ54 no longer affine to CPU4
[  145.439931] CPU4: shutdown
#
#
# rtcwake -m mem -s 3
wakeup from "mem" at Mon Apr  9 11:04:19 2001
[  148.708773] PM: Syncing filesystems ... done.
[  148.718591] Freezing user space processes ... (elapsed 0.005 seconds) done.
[  148.727749] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[  148.753406] wake enabled for irq 135
[  148.754006] smsc95xx 1-1.1:1.0 eth0: entering SUSPEND2 mode
[  148.844402] PM: suspend of devices complete after 100.196 msecs
[  148.869158] PM: late suspend of devices complete after 19.793 msecs
[  148.891948] PM: noirq suspend of devices complete after 17.803 msecs
[  148.897067] Disabling non-boot CPUs ...
[  148.920201] IRQ51 no longer affine to CPU1
[  148.921414] CPU1: shutdown
[  148.990114] IRQ52 no longer affine to CPU2
[  148.991158] CPU2: shutdown
[  149.049416] IRQ53 no longer affine to CPU3
[  149.050477] CPU3: shutdown
[  149.114297] IRQ55 no longer affine to CPU5
[  149.115318] CPU5: shutdown
[  149.184539] IRQ56 no longer affine to CPU6
[  149.185421] CPU6: shutdown
[  149.257561] IRQ57 no longer affine to CPU7
[  149.258458] CPU7: shutdown
exynos_suspend_enter: suspending the system...
exynos_suspend_enter: wakeup masks: fffffffd,ffffffef
UART[f7020000]: ULCON=0003, UCON=f3c5, UFCON=0131, UBRDIV=001b
[  149.295339] Enabling non-boot CPUs ...
[  149.314454] CPU1 is up
[  149.329717] CPU2 is up
[  149.350140] CPU3 is up
[  149.370365] cpu cpu5: cpufreq_init: failed to get clk: -2
[  149.374803] IRQ55 no longer affine to CPU5
[  149.375126] CPU5: shutdown
[  149.399737] Error taking CPU5 up: -2
[  149.425424] cpu cpu6: cpufreq_init: failed to get clk: -2
[  149.429886] IRQ56 no longer affine to CPU6
[  149.430242] CPU6: shutdown
[  149.454745] Error taking CPU6 up: -2
[  149.480397] cpu cpu7: cpufreq_init: failed to get clk: -2
[  149.484869] IRQ57 no longer affine to CPU7
[  149.485186] CPU7: shutdown
[  149.509718] Error taking CPU7 up: -2
[  149.512392] s3c-i2c 12c60000.i2c: slave address 0x00
[  149.516787] s3c-i2c 12c60000.i2c: bus frequency set to 65 KHz
[  149.522554] s3c-i2c 12c80000.i2c: slave address 0x00
[  149.527461] s3c-i2c 12c80000.i2c: bus frequency set to 65 KHz
[  149.535818] PM: noirq resume of devices complete after 23.970 msecs
[  149.543853] PM: early resume of devices complete after 3.020 msecs
[  149.571356] s3c2410-wdt 101d0000.watchdog: watchdog disabled
[  149.575811] usb usb1: root hub lost power or was reset
[  149.641026] usb usb2: root hub lost power or was reset
[  149.646353] exynos-tmu 10060000.tmu: More trip points than supported by this TMU.
[  149.652405] exynos-tmu 10060000.tmu: 2 trip points should be configured in polling mode.
[  149.663667] wake disabled for irq 135
[  149.680093] Suspended for 2.879 seconds
[  149.715816] usb usb3: root hub lost power or was reset
[  149.719482] usb usb4: root hub lost power or was reset
[  149.728432] s3c-rtc 101e0000.rtc: rtc disabled, re-enabling
[  149.932034] usb 1-1: reset high-speed USB device number 2 using exynos-ehci
[  150.387017] usb 1-1.1: reset high-speed USB device number 3 using exynos-ehci
[  150.566376] PM: resume of devices complete after 995.669 msecs
[  150.575402] Restarting tasks ... done.

Thanks.

Re: [PATCH V4] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Viresh Kumar]

On 30-11-16, 15:19, Joonyoung Shim wrote:
> Hi Viresh,
> 
> On 11/30/2016 12:59 PM, Viresh Kumar wrote:
> > From: Stephen Boyd <sboyd@codeaurora.org>
> > 
> > Joonyoung Shim reported an interesting problem on his ARM octa-core
> > Odoroid-XU3 platform. During system suspend, dev_pm_opp_put_regulator()
> > was failing for a struct device for which dev_pm_opp_set_regulator() is
> > called earlier.
> > 
> > This happened because an earlier call to
> > dev_pm_opp_of_cpumask_remove_table() function (from cpufreq-dt.c file)
> > removed all the entries from opp_table->dev_list apart from the last CPU
> > device in the cpumask of CPUs sharing the OPP.
> > 
> > But both dev_pm_opp_set_regulator() and dev_pm_opp_put_regulator()
> > routines get CPU device for the first CPU in the cpumask. And so the OPP
> > core failed to find the OPP table for the struct device.
> > 
> > In order to fix that up properly, we need to revisit APIs like
> > dev_pm_opp_set_regulator() and make them talk in terms of cookies
> > provided by the OPP core. But such a solution will be hard to backport
> > to stable kernels.
> > 
> > This patch attempts to fix this problem by returning a pointer to the
> > opp_table from dev_pm_opp_set_regulator() and using that as the
> > parameter to dev_pm_opp_put_regulator(). This ensures that the
> > dev_pm_opp_put_regulator() doesn't fail to find the opp table.
> > 
> > Note that similar design problem also exists with other
> > dev_pm_opp_put_*() APIs, but those aren't used currently by anyone and
> > so we don't need to update them for now.
> > 
> > [Viresh]: Written commit log, minor improvements in the patch and tested
> > on exynos 5250.
> > 
> > Cc: # v4.4+ <stable@vger.kernel.org>
> > Reported-by: Joonyoung Shim <jy0922.shim@samsung.com>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > V3->V4:
> > - Completely different approach, suggested earlier by Stephen.
> > - Can be merged safely now as both /me and Stephen agree to this one.
> > 
> > @Joonyoung: Can you please test this last patch please ?
> > 
> 
> Just system suspend/resume is working

Should I consider that as a Tested-by from you for the problem you
reported at least ?

> but i was missing below test case
> that you inform when i test for prior patches on my Odroid-XU3 board.
> 
> - offline CPU 4
> - suspend the system
> 
> With this test case, now all patches posted have the problem that is
> failed to get clk: -2.

That probably happens because your DT isn't good enough. Following DT
change may fix it for you:

diff --git a/arch/arm/boot/dts/exynos5422-cpus.dtsi b/arch/arm/boot/dts/exynos5422-cpus.dtsi
index bf3c6f1ec4ee..998a7dad95fc 100644
--- a/arch/arm/boot/dts/exynos5422-cpus.dtsi
+++ b/arch/arm/boot/dts/exynos5422-cpus.dtsi
@@ -41,6 +41,7 @@
                        device_type = "cpu";
                        compatible = "arm,cortex-a7";
                        reg = <0x101>;
+                       clocks = <&clock CLK_KFC_CLK>;
                        clock-frequency = <1000000000>;
                        cci-control-port = <&cci_control0>;
                        operating-points-v2 = <&cluster_a7_opp_table>;
@@ -53,6 +54,7 @@
                        device_type = "cpu";
                        compatible = "arm,cortex-a7";
                        reg = <0x102>;
+                       clocks = <&clock CLK_KFC_CLK>;
                        clock-frequency = <1000000000>;
                        cci-control-port = <&cci_control0>;
                        operating-points-v2 = <&cluster_a7_opp_table>;
@@ -65,6 +67,7 @@
                        device_type = "cpu";
                        compatible = "arm,cortex-a7";
                        reg = <0x103>;
+                       clocks = <&clock CLK_KFC_CLK>;
                        clock-frequency = <1000000000>;
                        cci-control-port = <&cci_control0>;
                        operating-points-v2 = <&cluster_a7_opp_table>;
@@ -89,6 +92,7 @@
                cpu5: cpu@1 {
                        device_type = "cpu";
                        compatible = "arm,cortex-a15";
+                       clocks = <&clock CLK_ARM_CLK>;
                        reg = <0x1>;
                        clock-frequency = <1800000000>;
                        cci-control-port = <&cci_control1>;
@@ -101,6 +105,7 @@
                cpu6: cpu@2 {
                        device_type = "cpu";
                        compatible = "arm,cortex-a15";
+                       clocks = <&clock CLK_ARM_CLK>;
                        reg = <0x2>;
                        clock-frequency = <1800000000>;
                        cci-control-port = <&cci_control1>;
@@ -113,6 +118,7 @@
                cpu7: cpu@3 {
                        device_type = "cpu";
                        compatible = "arm,cortex-a15";
+                       clocks = <&clock CLK_ARM_CLK>;
                        reg = <0x3>;
                        clock-frequency = <1800000000>;
                        cci-control-port = <&cci_control1>;

-- 
viresh

Re: [PATCH V4] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Joonyoung Shim]

On 11/30/2016 05:05 PM, Viresh Kumar wrote:
> On 30-11-16, 15:19, Joonyoung Shim wrote:
>> Hi Viresh,
>>
>> On 11/30/2016 12:59 PM, Viresh Kumar wrote:
>>> From: Stephen Boyd <sboyd@codeaurora.org>
>>>
>>> Joonyoung Shim reported an interesting problem on his ARM octa-core
>>> Odoroid-XU3 platform. During system suspend, dev_pm_opp_put_regulator()
>>> was failing for a struct device for which dev_pm_opp_set_regulator() is
>>> called earlier.
>>>
>>> This happened because an earlier call to
>>> dev_pm_opp_of_cpumask_remove_table() function (from cpufreq-dt.c file)
>>> removed all the entries from opp_table->dev_list apart from the last CPU
>>> device in the cpumask of CPUs sharing the OPP.
>>>
>>> But both dev_pm_opp_set_regulator() and dev_pm_opp_put_regulator()
>>> routines get CPU device for the first CPU in the cpumask. And so the OPP
>>> core failed to find the OPP table for the struct device.
>>>
>>> In order to fix that up properly, we need to revisit APIs like
>>> dev_pm_opp_set_regulator() and make them talk in terms of cookies
>>> provided by the OPP core. But such a solution will be hard to backport
>>> to stable kernels.
>>>
>>> This patch attempts to fix this problem by returning a pointer to the
>>> opp_table from dev_pm_opp_set_regulator() and using that as the
>>> parameter to dev_pm_opp_put_regulator(). This ensures that the
>>> dev_pm_opp_put_regulator() doesn't fail to find the opp table.
>>>
>>> Note that similar design problem also exists with other
>>> dev_pm_opp_put_*() APIs, but those aren't used currently by anyone and
>>> so we don't need to update them for now.
>>>
>>> [Viresh]: Written commit log, minor improvements in the patch and tested
>>> on exynos 5250.
>>>
>>> Cc: # v4.4+ <stable@vger.kernel.org>
>>> Reported-by: Joonyoung Shim <jy0922.shim@samsung.com>
>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>> V3->V4:
>>> - Completely different approach, suggested earlier by Stephen.
>>> - Can be merged safely now as both /me and Stephen agree to this one.
>>>
>>> @Joonyoung: Can you please test this last patch please ?
>>>
>>
>> Just system suspend/resume is working
> 
> Should I consider that as a Tested-by from you for the problem you
> reported at least ?
> 

My Tested-by is ok about the original problem reported by me.

>> but i was missing below test case
>> that you inform when i test for prior patches on my Odroid-XU3 board.
>>
>> - offline CPU 4
>> - suspend the system
>>
>> With this test case, now all patches posted have the problem that is
>> failed to get clk: -2.
> 
> That probably happens because your DT isn't good enough. Following DT
> change may fix it for you:
> 

Already i tried and the error was gone but sometimes system resume is
halt. I'm not sure that it has any effect so i need to more dig.

Thanks.

Re: [PATCH V4] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Viresh Kumar]

On 01-12-16, 10:45, Joonyoung Shim wrote:
> Already i tried and the error was gone but sometimes system resume is
> halt.

That looks to be a different problem which may not be related to
cpufreq at all.

> I'm not sure that it has any effect so i need to more dig.

Right.

-- 
viresh

Re: [PATCH V4] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Viresh Kumar]

On 30-11-16, 09:29, Viresh Kumar wrote:
> +struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name)
>  {
>  	struct opp_table *opp_table;
>  	struct regulator *reg;
> -	int ret;
>  
>  	mutex_lock(&opp_table_lock);
>  
>  	opp_table = _add_opp_table(dev);
>  	if (!opp_table) {
> -		ret = -ENOMEM;
> +		opp_table = ERR_PTR(-ENOMEM);
>  		goto unlock;
>  	}
>  
>  	/* This should be called before OPPs are initialized */
>  	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> -		ret = -EBUSY;
> +		opp_table = ERR_PTR(-EBUSY);

The pointer opp_table shouldn't be overwritten here as it will be used
in the error path. Will resend the patch shortly.

-- 
viresh

[PATCH V5] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Viresh Kumar]

From: Stephen Boyd <sboyd@codeaurora.org>

Joonyoung Shim reported an interesting problem on his ARM octa-core
Odoroid-XU3 platform. During system suspend, dev_pm_opp_put_regulator()
was failing for a struct device for which dev_pm_opp_set_regulator() is
called earlier.

This happened because an earlier call to
dev_pm_opp_of_cpumask_remove_table() function (from cpufreq-dt.c file)
removed all the entries from opp_table->dev_list apart from the last CPU
device in the cpumask of CPUs sharing the OPP.

But both dev_pm_opp_set_regulator() and dev_pm_opp_put_regulator()
routines get CPU device for the first CPU in the cpumask. And so the OPP
core failed to find the OPP table for the struct device.

This patch attempts to fix this problem by returning a pointer to the
opp_table from dev_pm_opp_set_regulator() and using that as the
parameter to dev_pm_opp_put_regulator(). This ensures that the
dev_pm_opp_put_regulator() doesn't fail to find the opp table.

Note that similar design problem also exists with other
dev_pm_opp_put_*() APIs, but those aren't used currently by anyone and
so we don't need to update them for now.

[Viresh]: Written commit log and tested on exynos 5250.

Cc: # v4.4+ <stable@vger.kernel.org>
Reported-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V4->V5
- Preserve the opp_table pointer in the error path, as it will be used
  to free the table.

 drivers/base/power/opp/core.c | 22 ++++++----------------
 drivers/cpufreq/cpufreq-dt.c  | 12 ++++++++----
 include/linux/pm_opp.h        | 11 ++++++-----
 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 4c7c6da7a989..2824d3a5e9f0 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1316,7 +1316,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
  * that this function is *NOT* called under RCU protection or in contexts where
  * mutex cannot be locked.
  */
-int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name)
 {
 	struct opp_table *opp_table;
 	struct regulator *reg;
@@ -1354,20 +1354,20 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
 	opp_table->regulator = reg;
 
 	mutex_unlock(&opp_table_lock);
-	return 0;
+	return opp_table;
 
 err:
 	_remove_opp_table(opp_table);
 unlock:
 	mutex_unlock(&opp_table_lock);
 
-	return ret;
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
 
 /**
  * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
- * @dev: Device for which regulator was set.
+ * @opp_table: OPP table returned from dev_pm_opp_set_regulator().
  *
  * Locking: The internal opp_table and opp structures are RCU protected.
  * Hence this function internally uses RCU updater strategy with mutex locks
@@ -1375,22 +1375,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
  * that this function is *NOT* called under RCU protection or in contexts where
  * mutex cannot be locked.
  */
-void dev_pm_opp_put_regulator(struct device *dev)
+void dev_pm_opp_put_regulator(struct opp_table *opp_table)
 {
-	struct opp_table *opp_table;
-
 	mutex_lock(&opp_table_lock);
 
-	/* Check for existing table for 'dev' first */
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		dev_err(dev, "Failed to find opp_table: %ld\n",
-			PTR_ERR(opp_table));
-		goto unlock;
-	}
-
 	if (IS_ERR(opp_table->regulator)) {
-		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+		pr_err("%s: Doesn't have regulator set\n", __func__);
 		goto unlock;
 	}
 
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 5c07ae05d69a..4d3ec92cbabf 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -28,6 +28,7 @@
 #include "cpufreq-dt.h"
 
 struct private_data {
+	struct opp_table *opp_table;
 	struct device *cpu_dev;
 	struct thermal_cooling_device *cdev;
 	const char *reg_name;
@@ -143,6 +144,7 @@ static int resources_available(void)
 static int cpufreq_init(struct cpufreq_policy *policy)
 {
 	struct cpufreq_frequency_table *freq_table;
+	struct opp_table *opp_table = NULL;
 	struct private_data *priv;
 	struct device *cpu_dev;
 	struct clk *cpu_clk;
@@ -186,8 +188,9 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	 */
 	name = find_supply_name(cpu_dev);
 	if (name) {
-		ret = dev_pm_opp_set_regulator(cpu_dev, name);
-		if (ret) {
+		opp_table = dev_pm_opp_set_regulator(cpu_dev, name);
+		if (IS_ERR(opp_table)) {
+			ret = PTR_ERR(opp_table);
 			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
 				policy->cpu, ret);
 			goto out_put_clk;
@@ -237,6 +240,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	priv->reg_name = name;
+	priv->opp_table = opp_table;
 
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
@@ -285,7 +289,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 out_free_opp:
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (name)
-		dev_pm_opp_put_regulator(cpu_dev);
+		dev_pm_opp_put_regulator(opp_table);
 out_put_clk:
 	clk_put(cpu_clk);
 
@@ -300,7 +304,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
 	if (priv->reg_name)
-		dev_pm_opp_put_regulator(priv->cpu_dev);
+		dev_pm_opp_put_regulator(priv->opp_table);
 
 	clk_put(policy->clk);
 	kfree(priv);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index bca26157f5b6..f6bc76501912 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -19,6 +19,7 @@
 
 struct dev_pm_opp;
 struct device;
+struct opp_table;
 
 enum dev_pm_opp_event {
 	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
@@ -62,8 +63,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
 void dev_pm_opp_put_supported_hw(struct device *dev);
 int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
-int dev_pm_opp_set_regulator(struct device *dev, const char *name);
-void dev_pm_opp_put_regulator(struct device *dev);
+struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name);
+void dev_pm_opp_put_regulator(struct opp_table *opp_table);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -170,12 +171,12 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 
 static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
 
-static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+static inline struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name)
 {
-	return -ENOTSUPP;
+	return ERR_PTR(-ENOTSUPP);
 }
 
-static inline void dev_pm_opp_put_regulator(struct device *dev) {}
+static inline void dev_pm_opp_put_regulator(struct opp_table *opp_table) {}
 
 static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
-- 
2.7.1.410.g6faf27b

Re: [PATCH V5] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Stephen Boyd]

On 11/30, Viresh Kumar wrote:
> From: Stephen Boyd <sboyd@codeaurora.org>
> 
> Joonyoung Shim reported an interesting problem on his ARM octa-core
> Odoroid-XU3 platform. During system suspend, dev_pm_opp_put_regulator()
> was failing for a struct device for which dev_pm_opp_set_regulator() is
> called earlier.
> 
> This happened because an earlier call to
> dev_pm_opp_of_cpumask_remove_table() function (from cpufreq-dt.c file)
> removed all the entries from opp_table->dev_list apart from the last CPU
> device in the cpumask of CPUs sharing the OPP.
> 
> But both dev_pm_opp_set_regulator() and dev_pm_opp_put_regulator()
> routines get CPU device for the first CPU in the cpumask. And so the OPP
> core failed to find the OPP table for the struct device.
> 
> This patch attempts to fix this problem by returning a pointer to the
> opp_table from dev_pm_opp_set_regulator() and using that as the
> parameter to dev_pm_opp_put_regulator(). This ensures that the
> dev_pm_opp_put_regulator() doesn't fail to find the opp table.
> 
> Note that similar design problem also exists with other
> dev_pm_opp_put_*() APIs, but those aren't used currently by anyone and
> so we don't need to update them for now.
> 
> [Viresh]: Written commit log and tested on exynos 5250.
> 
> Cc: # v4.4+ <stable@vger.kernel.org>
> Reported-by: Joonyoung Shim <jy0922.shim@samsung.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

You should have asked for my Signed-off-by instead of just adding
it. Here it is to make things explicit and recorded:

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Thanks for putting everything together and simplifying the error
case.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V5] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Viresh Kumar]

On 30-11-16, 14:00, Stephen Boyd wrote:
> On 11/30, Viresh Kumar wrote:
> > From: Stephen Boyd <sboyd@codeaurora.org>
> > 
> > Joonyoung Shim reported an interesting problem on his ARM octa-core
> > Odoroid-XU3 platform. During system suspend, dev_pm_opp_put_regulator()
> > was failing for a struct device for which dev_pm_opp_set_regulator() is
> > called earlier.
> > 
> > This happened because an earlier call to
> > dev_pm_opp_of_cpumask_remove_table() function (from cpufreq-dt.c file)
> > removed all the entries from opp_table->dev_list apart from the last CPU
> > device in the cpumask of CPUs sharing the OPP.
> > 
> > But both dev_pm_opp_set_regulator() and dev_pm_opp_put_regulator()
> > routines get CPU device for the first CPU in the cpumask. And so the OPP
> > core failed to find the OPP table for the struct device.
> > 
> > This patch attempts to fix this problem by returning a pointer to the
> > opp_table from dev_pm_opp_set_regulator() and using that as the
> > parameter to dev_pm_opp_put_regulator(). This ensures that the
> > dev_pm_opp_put_regulator() doesn't fail to find the opp table.
> > 
> > Note that similar design problem also exists with other
> > dev_pm_opp_put_*() APIs, but those aren't used currently by anyone and
> > so we don't need to update them for now.
> > 
> > [Viresh]: Written commit log and tested on exynos 5250.
> > 
> > Cc: # v4.4+ <stable@vger.kernel.org>
> > Reported-by: Joonyoung Shim <jy0922.shim@samsung.com>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> 
> You should have asked for my Signed-off-by instead of just adding
> it.

I was worried about the 24 hrs that gets wasted because of 12 hrs
difference in our time zones and so added you as the author and added
your sob. :)

> Here it is to make things explicit and recorded:

Thanks.

-- 
viresh

Re: [PATCH V5] PM / OPP: Pass opp_table to dev_pm_opp_put_regulator()

[Rafael J. Wysocki]

On Thursday, December 01, 2016 05:55:48 AM Viresh Kumar wrote:
> On 30-11-16, 14:00, Stephen Boyd wrote:
> > On 11/30, Viresh Kumar wrote:
> > > From: Stephen Boyd <sboyd@codeaurora.org>
> > > 
> > > Joonyoung Shim reported an interesting problem on his ARM octa-core
> > > Odoroid-XU3 platform. During system suspend, dev_pm_opp_put_regulator()
> > > was failing for a struct device for which dev_pm_opp_set_regulator() is
> > > called earlier.
> > > 
> > > This happened because an earlier call to
> > > dev_pm_opp_of_cpumask_remove_table() function (from cpufreq-dt.c file)
> > > removed all the entries from opp_table->dev_list apart from the last CPU
> > > device in the cpumask of CPUs sharing the OPP.
> > > 
> > > But both dev_pm_opp_set_regulator() and dev_pm_opp_put_regulator()
> > > routines get CPU device for the first CPU in the cpumask. And so the OPP
> > > core failed to find the OPP table for the struct device.
> > > 
> > > This patch attempts to fix this problem by returning a pointer to the
> > > opp_table from dev_pm_opp_set_regulator() and using that as the
> > > parameter to dev_pm_opp_put_regulator(). This ensures that the
> > > dev_pm_opp_put_regulator() doesn't fail to find the opp table.
> > > 
> > > Note that similar design problem also exists with other
> > > dev_pm_opp_put_*() APIs, but those aren't used currently by anyone and
> > > so we don't need to update them for now.
> > > 
> > > [Viresh]: Written commit log and tested on exynos 5250.
> > > 
> > > Cc: # v4.4+ <stable@vger.kernel.org>
> > > Reported-by: Joonyoung Shim <jy0922.shim@samsung.com>
> > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > 
> > You should have asked for my Signed-off-by instead of just adding
> > it.
> 
> I was worried about the 24 hrs that gets wasted because of 12 hrs
> difference in our time zones and so added you as the author and added
> your sob. :)
> 
> > Here it is to make things explicit and recorded:

Applied.

Thanks,
Rafael