From 001d50c9a14f1fab94b329161a1db9235b4e60da Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Thu, 30 Nov 2017 12:54:28 +0100 Subject: PM / Domains: Remove obsolete "samsung,power-domain" check Currently the generic PM Domain code code checks for the presence of both (generic) "power-domains" and (Samsung Exynos legacy) "samsung,power-domain" properties in all device tree nodes representing devices. There are two issues with this: 1. This imposes a small boot-time penalty on all platforms using DT, 2. Platform-specific checks do not really belong in core framework code. Remove the platform-specific check, as the last user of "samsung,power-domain" was removed in commit 46dcf0ff0de35da8 ("ARM: dts: exynos: Remove exynos4415.dtsi"). All other users were converted before in commit 0da6587041363033 ("ARM: dts: convert to generic power domain bindings for exynos DT"). Signed-off-by: Geert Uytterhoeven Acked-by: Ulf Hansson Acked-by: Krzysztof Kozlowski Signed-off-by: Rafael J. Wysocki --- drivers/base/power/domain.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 0c80bea05bcb..f9dcc981b6b9 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -2199,20 +2199,8 @@ int genpd_dev_pm_attach(struct device *dev) ret = of_parse_phandle_with_args(dev->of_node, "power-domains", "#power-domain-cells", 0, &pd_args); - if (ret < 0) { - if (ret != -ENOENT) - return ret; - - /* - * Try legacy Samsung-specific bindings - * (for backwards compatibility of DT ABI) - */ - pd_args.args_count = 0; - pd_args.np = of_parse_phandle(dev->of_node, - "samsung,power-domain", 0); - if (!pd_args.np) - return -ENOENT; - } + if (ret < 0) + return ret; mutex_lock(&gpd_list_lock); pd = genpd_get_from_provider(&pd_args); -- cgit v1.2.3-58-ga151 From a935424bb658f9ca37eb5e94119b857998341356 Mon Sep 17 00:00:00 2001 From: Ulf Hansson Date: Wed, 10 Jan 2018 21:31:56 +0100 Subject: PM / domains: Don't skip driver's ->suspend|resume_noirq() callbacks Commit 10da65423fdb (PM / Domains: Call driver's noirq callbacks) started to respect driver's noirq callbacks, but while doing that it also introduced a few potential problems. More precisely, in genpd_finish_suspend() and genpd_resume_noirq() the noirq callbacks at the driver level should be invoked, no matter of whether dev->power.wakeup_path is set or not. Additionally, the commit in question also made genpd_resume_noirq() to ignore the return value from pm_runtime_force_resume(). Let's fix both these issues! Fixes: 10da65423fdb (PM / Domains: Call driver's noirq callbacks) Signed-off-by: Ulf Hansson Signed-off-by: Rafael J. Wysocki --- drivers/base/power/domain.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index f9dcc981b6b9..48255ce7c0ad 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1032,15 +1032,12 @@ static int genpd_prepare(struct device *dev) static int genpd_finish_suspend(struct device *dev, bool poweroff) { struct generic_pm_domain *genpd; - int ret; + int ret = 0; genpd = dev_to_genpd(dev); if (IS_ERR(genpd)) return -EINVAL; - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) - return 0; - if (poweroff) ret = pm_generic_poweroff_noirq(dev); else @@ -1048,10 +1045,18 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) if (ret) return ret; + if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) + return 0; + if (genpd->dev_ops.stop && genpd->dev_ops.start) { ret = pm_runtime_force_suspend(dev); - if (ret) + if (ret) { + if (poweroff) + pm_generic_restore_noirq(dev); + else + pm_generic_resume_noirq(dev); return ret; + } } genpd_lock(genpd); @@ -1085,7 +1090,7 @@ static int genpd_suspend_noirq(struct device *dev) static int genpd_resume_noirq(struct device *dev) { struct generic_pm_domain *genpd; - int ret = 0; + int ret; dev_dbg(dev, "%s()\n", __func__); @@ -1094,21 +1099,20 @@ static int genpd_resume_noirq(struct device *dev) return -EINVAL; if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) - return 0; + return pm_generic_resume_noirq(dev); genpd_lock(genpd); genpd_sync_power_on(genpd, true, 0); genpd->suspended_count--; genpd_unlock(genpd); - if (genpd->dev_ops.stop && genpd->dev_ops.start) + if (genpd->dev_ops.stop && genpd->dev_ops.start) { ret = pm_runtime_force_resume(dev); + if (ret) + return ret; + } - ret = pm_generic_resume_noirq(dev); - if (ret) - return ret; - - return ret; + return pm_generic_resume_noirq(dev); } /** -- cgit v1.2.3-58-ga151 From 17218e0092f8c7b7edce7ff08c8b23212eac7271 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Fri, 12 Jan 2018 14:10:38 +0100 Subject: PM / genpd: Stop/start devices without pm_runtime_force_suspend/resume() There are problems with calling pm_runtime_force_suspend/resume() to "stop" and "start" devices in genpd_finish_suspend() and genpd_resume_noirq() (and in analogous hibernation-specific genpd callbacks) after commit 122a22377a3d (PM / Domains: Stop/start devices during system PM suspend/resume in genpd) as those routines do much more than just "stopping" and "starting" devices (which was the stated purpose of that commit) unnecessarily and may not play well with system-wide PM driver callbacks. First, consider the pm_runtime_force_suspend() in genpd_finish_suspend(). If the current runtime PM status of the device is "suspended", that function most likely does the right thing by ignoring the device, because it should have been "stopped" already and whatever needed to be done to deactivate it shoud have been done. In turn, if the runtime PM status of the device is "active", genpd_runtime_suspend() is called for it (indirectly) and (1) runs the ->runtime_suspend callback provided by the device's driver (assuming no bus type with ->runtime_suspend of its own), (2) "stops" the device and (3) checks if the domain can be powered down, and then (4) the device's runtime PM status is changed to "suspended". Out of the four actions above (1) is not necessary and it may be outright harmful, (3) is pointless and (4) is questionable. The only operation that needs to be carried out here is (2). The reason why (1) is not necessary is because the system-wide PM callbacks provided by the device driver for the transition in question have been run and they should have taken care of the driver's part of device suspend already. Moreover, it may be harmful, because the ->runtime_suspend callback may want to access the device which is partially suspended at that point and may not be responsive. Also, system-wide PM callbacks may have been run already (in the previous phases of the system transition under way) for the device's parent or for its supplier devices (if any) and the device may not be accessible because of that. There also is no reason to do (3), because genpd_finish_suspend() will repeat it anyway, and (4) potentially causes confusion to ensue during the subsequent system transition to the working state. Consider pm_runtime_force_resume() in genpd_resume_noirq() now. It runs genpd_runtime_resume() for all devices with runtime PM status set to "suspended", which includes all of the devices whose runtime PM status was changed by pm_runtime_force_suspend() before and may include some devices already suspended when the pm_runtime_force_suspend() was running, which may be confusing. The genpd_runtime_resume() first tries to power up the domain, which (again) is pointless, because genpd_resume_noirq() has done that already. Then, it "starts" the device and runs the ->runtime_resume callback (from the driver, say) for it. If all is well, the device is left with the runtime PM status set to "active". Unfortunately, running the driver's ->runtime_resume callback before its system-wide PM callbacks and possibly before some system-wide PM callbacks of the parent device's driver (let alone supplier drivers) is asking for trouble, especially if the device had been suspended before pm_runtime_force_suspend() ran previously or if the callbacks in question expect to be run back-to-back with their suspend-side counterparts. It also should not be necessary, because the system-wide PM driver callbacks that will be invoked for the device subsequently should take care of resuming it just fine. [Running the driver's ->runtime_resume callback in the "noirq" phase of the transition to the working state may be problematic even for devices whose drivers do use pm_runtime_force_resume() in (or as) their system-wide PM callbacks if they have suppliers other than their parents, because it may cause the supplier to be resumed after the consumer in some cases.] Because of the above, modify genpd as follows: 1. Change genpd_finish_suspend() to only "stop" devices with runtime PM status set to "active" (without invoking runtime PM callbacks for them, changing their runtime PM status and so on). That doesn't change the handling of devices whose drivers use pm_runtime_force_suspend/resume() in (or as) their system-wide PM callbacks and addresses the issues described above for the other devices. 2. Change genpd_resume_noirq() to only "start" devices with runtime PM status set to "active" (without invoking runtime PM callbacks for them, changing their runtime PM status and so on). Again, that doesn't change the handling of devices whose drivers use pm_runtime_force_suspend/resume() in (or as) their system-wide PM callbacks and addresses the described issues for the other devices. Devices with runtime PM status set to "suspended" are not started with the assumption that they will be resumed later, either by pm_runtime_force_resume() or via runtime PM. 3. Change genpd_restore_noirq() to follow genpd_resume_noirq(). That causes devices already suspended before hibernation to be left alone (which also is the case without the change) and avoids running the ->runtime_resume driver callback too early for the other devices. 4. Change genpd_freeze_noirq() and genpd_thaw_noirq() in accordance with the above modifications. Fixes: 122a22377a3d (PM / Domains: Stop/start devices during system PM suspend/resume in genpd) Signed-off-by: Rafael J. Wysocki Acked-by: Ulf Hansson --- drivers/base/power/domain.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 48255ce7c0ad..528b24149bc7 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1048,8 +1048,9 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) return 0; - if (genpd->dev_ops.stop && genpd->dev_ops.start) { - ret = pm_runtime_force_suspend(dev); + if (genpd->dev_ops.stop && genpd->dev_ops.start && + !pm_runtime_status_suspended(dev)) { + ret = genpd_stop_dev(genpd, dev); if (ret) { if (poweroff) pm_generic_restore_noirq(dev); @@ -1106,8 +1107,9 @@ static int genpd_resume_noirq(struct device *dev) genpd->suspended_count--; genpd_unlock(genpd); - if (genpd->dev_ops.stop && genpd->dev_ops.start) { - ret = pm_runtime_force_resume(dev); + if (genpd->dev_ops.stop && genpd->dev_ops.start && + !pm_runtime_status_suspended(dev)) { + ret = genpd_start_dev(genpd, dev); if (ret) return ret; } @@ -1139,8 +1141,9 @@ static int genpd_freeze_noirq(struct device *dev) if (ret) return ret; - if (genpd->dev_ops.stop && genpd->dev_ops.start) - ret = pm_runtime_force_suspend(dev); + if (genpd->dev_ops.stop && genpd->dev_ops.start && + !pm_runtime_status_suspended(dev)) + ret = genpd_stop_dev(genpd, dev); return ret; } @@ -1163,8 +1166,9 @@ static int genpd_thaw_noirq(struct device *dev) if (IS_ERR(genpd)) return -EINVAL; - if (genpd->dev_ops.stop && genpd->dev_ops.start) { - ret = pm_runtime_force_resume(dev); + if (genpd->dev_ops.stop && genpd->dev_ops.start && + !pm_runtime_status_suspended(dev)) { + ret = genpd_start_dev(genpd, dev); if (ret) return ret; } @@ -1221,8 +1225,9 @@ static int genpd_restore_noirq(struct device *dev) genpd_sync_power_on(genpd, true, 0); genpd_unlock(genpd); - if (genpd->dev_ops.stop && genpd->dev_ops.start) { - ret = pm_runtime_force_resume(dev); + if (genpd->dev_ops.stop && genpd->dev_ops.start && + !pm_runtime_status_suspended(dev)) { + ret = genpd_start_dev(genpd, dev); if (ret) return ret; } -- cgit v1.2.3-58-ga151