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) {
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.
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>;
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.
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;
goto unlock; }opp_table = ERR_PTR(-ENOMEM);
/* 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.
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) {
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.
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.
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
linaro-kernel@lists.linaro.org