| Message ID | 480ae6e161788d338fb1637aa2615a75588ac3c6.1480478081.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {