From 86e12eac1f7f84b03512ecfa110c48b9204e6286 Mon Sep 17 00:00:00 2001 From: Ulf Hansson Date: Thu, 8 Dec 2016 14:45:20 +0100 Subject: [PATCH 1/9] PM / Domains: Rename functions in genpd for power on/off Currently the mix of genpd_poweron(), genpd_power_on(), genpd_sync_poweron() and the ->power_on() callback, makes a bit difficult to follow the path of execution. The similar applies to the functions dealing with power off. In a way to improve this understanding, let's do the following renaming: genpd_power_on() -> _genpd_power_on() genpd_poweron() -> genpd_power_on() genpd_sync_poweron() -> genpd_sync_power_on() genpd_power_off() -> _genpd_power_off() genpd_poweroff() -> genpd_power_off() genpd_sync_poweroff() -> genpd_sync_power_off() genpd_poweroff_unused() -> genpd_power_off_unused() Signed-off-by: Ulf Hansson Signed-off-by: Rafael J. Wysocki --- drivers/base/power/domain.c | 72 ++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 2997026b4dfb..fd2e3e1eac65 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -201,7 +201,7 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd) smp_mb__after_atomic(); } -static int genpd_power_on(struct generic_pm_domain *genpd, bool timed) +static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) { unsigned int state_idx = genpd->state_idx; ktime_t time_start; @@ -231,7 +231,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, bool timed) return ret; } -static int genpd_power_off(struct generic_pm_domain *genpd, bool timed) +static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed) { unsigned int state_idx = genpd->state_idx; ktime_t time_start; @@ -262,10 +262,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool timed) } /** - * genpd_queue_power_off_work - Queue up the execution of genpd_poweroff(). + * genpd_queue_power_off_work - Queue up the execution of genpd_power_off(). * @genpd: PM domain to power off. * - * Queue up the execution of genpd_poweroff() unless it's already been done + * Queue up the execution of genpd_power_off() unless it's already been done * before. */ static void genpd_queue_power_off_work(struct generic_pm_domain *genpd) @@ -274,14 +274,14 @@ static void genpd_queue_power_off_work(struct generic_pm_domain *genpd) } /** - * genpd_poweron - Restore power to a given PM domain and its masters. + * genpd_power_on - Restore power to a given PM domain and its masters. * @genpd: PM domain to power up. * @depth: nesting count for lockdep. * * Restore power to @genpd and all of its masters so that it is possible to * resume a device belonging to it. */ -static int genpd_poweron(struct generic_pm_domain *genpd, unsigned int depth) +static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth) { struct gpd_link *link; int ret = 0; @@ -300,7 +300,7 @@ static int genpd_poweron(struct generic_pm_domain *genpd, unsigned int depth) genpd_sd_counter_inc(master); genpd_lock_nested(master, depth + 1); - ret = genpd_poweron(master, depth + 1); + ret = genpd_power_on(master, depth + 1); genpd_unlock(master); if (ret) { @@ -309,7 +309,7 @@ static int genpd_poweron(struct generic_pm_domain *genpd, unsigned int depth) } } - ret = genpd_power_on(genpd, true); + ret = _genpd_power_on(genpd, true); if (ret) goto err; @@ -368,14 +368,14 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, } /** - * genpd_poweroff - Remove power from a given PM domain. + * genpd_power_off - Remove power from a given PM domain. * @genpd: PM domain to power down. * @is_async: PM domain is powered down from a scheduled work * * If all of the @genpd's devices have been suspended and all of its subdomains * have been powered down, remove power from @genpd. */ -static int genpd_poweroff(struct generic_pm_domain *genpd, bool is_async) +static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async) { struct pm_domain_data *pdd; struct gpd_link *link; @@ -427,13 +427,13 @@ static int genpd_poweroff(struct generic_pm_domain *genpd, bool is_async) /* * If sd_count > 0 at this point, one of the subdomains hasn't - * managed to call genpd_poweron() for the master yet after - * incrementing it. In that case genpd_poweron() will wait + * managed to call genpd_power_on() for the master yet after + * incrementing it. In that case genpd_power_on() will wait * for us to drop the lock, so we can call .power_off() and let - * the genpd_poweron() restore power for us (this shouldn't + * the genpd_power_on() restore power for us (this shouldn't * happen very often). */ - ret = genpd_power_off(genpd, true); + ret = _genpd_power_off(genpd, true); if (ret) return ret; } @@ -459,7 +459,7 @@ static void genpd_power_off_work_fn(struct work_struct *work) genpd = container_of(work, struct generic_pm_domain, power_off_work); genpd_lock(genpd); - genpd_poweroff(genpd, true); + genpd_power_off(genpd, true); genpd_unlock(genpd); } @@ -578,7 +578,7 @@ static int genpd_runtime_suspend(struct device *dev) return 0; genpd_lock(genpd); - genpd_poweroff(genpd, false); + genpd_power_off(genpd, false); genpd_unlock(genpd); return 0; @@ -618,7 +618,7 @@ static int genpd_runtime_resume(struct device *dev) } genpd_lock(genpd); - ret = genpd_poweron(genpd, 0); + ret = genpd_power_on(genpd, 0); genpd_unlock(genpd); if (ret) @@ -658,7 +658,7 @@ err_poweroff: if (!pm_runtime_is_irq_safe(dev) || (pm_runtime_is_irq_safe(dev) && genpd_is_irq_safe(genpd))) { genpd_lock(genpd); - genpd_poweroff(genpd, 0); + genpd_power_off(genpd, 0); genpd_unlock(genpd); } @@ -674,9 +674,9 @@ static int __init pd_ignore_unused_setup(char *__unused) __setup("pd_ignore_unused", pd_ignore_unused_setup); /** - * genpd_poweroff_unused - Power off all PM domains with no devices in use. + * genpd_power_off_unused - Power off all PM domains with no devices in use. */ -static int __init genpd_poweroff_unused(void) +static int __init genpd_power_off_unused(void) { struct generic_pm_domain *genpd; @@ -694,7 +694,7 @@ static int __init genpd_poweroff_unused(void) return 0; } -late_initcall(genpd_poweroff_unused); +late_initcall(genpd_power_off_unused); #if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_GENERIC_DOMAINS_OF) @@ -727,7 +727,7 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd, } /** - * genpd_sync_poweroff - Synchronously power off a PM domain and its masters. + * genpd_sync_power_off - Synchronously power off a PM domain and its masters. * @genpd: PM domain to power off, if possible. * * Check if the given PM domain can be powered off (during system suspend or @@ -738,7 +738,7 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd, * executed sequentially, so it is guaranteed that it will never run twice in * parallel). */ -static void genpd_sync_poweroff(struct generic_pm_domain *genpd) +static void genpd_sync_power_off(struct generic_pm_domain *genpd) { struct gpd_link *link; @@ -751,18 +751,18 @@ static void genpd_sync_poweroff(struct generic_pm_domain *genpd) /* Choose the deepest state when suspending */ genpd->state_idx = genpd->state_count - 1; - genpd_power_off(genpd, false); + _genpd_power_off(genpd, false); genpd->status = GPD_STATE_POWER_OFF; list_for_each_entry(link, &genpd->slave_links, slave_node) { genpd_sd_counter_dec(link->master); - genpd_sync_poweroff(link->master); + genpd_sync_power_off(link->master); } } /** - * genpd_sync_poweron - Synchronously power on a PM domain and its masters. + * genpd_sync_power_on - Synchronously power on a PM domain and its masters. * @genpd: PM domain to power on. * * This function is only called in "noirq" and "syscore" stages of system power @@ -770,7 +770,7 @@ static void genpd_sync_poweroff(struct generic_pm_domain *genpd) * executed sequentially, so it is guaranteed that it will never run twice in * parallel). */ -static void genpd_sync_poweron(struct generic_pm_domain *genpd) +static void genpd_sync_power_on(struct generic_pm_domain *genpd) { struct gpd_link *link; @@ -778,11 +778,11 @@ static void genpd_sync_poweron(struct generic_pm_domain *genpd) return; list_for_each_entry(link, &genpd->slave_links, slave_node) { - genpd_sync_poweron(link->master); + genpd_sync_power_on(link->master); genpd_sd_counter_inc(link->master); } - genpd_power_on(genpd, false); + _genpd_power_on(genpd, false); genpd->status = GPD_STATE_ACTIVE; } @@ -894,7 +894,7 @@ static int pm_genpd_suspend_noirq(struct device *dev) * the same PM domain, so it is not necessary to use locking here. */ genpd->suspended_count++; - genpd_sync_poweroff(genpd); + genpd_sync_power_off(genpd); return 0; } @@ -924,7 +924,7 @@ static int pm_genpd_resume_noirq(struct device *dev) * guaranteed that this function will never run twice in parallel for * the same PM domain, so it is not necessary to use locking here. */ - genpd_sync_poweron(genpd); + genpd_sync_power_on(genpd); genpd->suspended_count--; if (genpd->dev_ops.stop && genpd->dev_ops.start) @@ -1012,12 +1012,12 @@ static int pm_genpd_restore_noirq(struct device *dev) if (genpd->suspended_count++ == 0) /* * The boot kernel might put the domain into arbitrary state, - * so make it appear as powered off to genpd_sync_poweron(), + * so make it appear as powered off to genpd_sync_power_on(), * so that it tries to power it on in case it was really off. */ genpd->status = GPD_STATE_POWER_OFF; - genpd_sync_poweron(genpd); + genpd_sync_power_on(genpd); if (genpd->dev_ops.stop && genpd->dev_ops.start) ret = pm_runtime_force_resume(dev); @@ -1072,9 +1072,9 @@ static void genpd_syscore_switch(struct device *dev, bool suspend) if (suspend) { genpd->suspended_count++; - genpd_sync_poweroff(genpd); + genpd_sync_power_off(genpd); } else { - genpd_sync_poweron(genpd); + genpd_sync_power_on(genpd); genpd->suspended_count--; } } @@ -2043,7 +2043,7 @@ int genpd_dev_pm_attach(struct device *dev) dev->pm_domain->sync = genpd_dev_pm_sync; genpd_lock(pd); - ret = genpd_poweron(pd, 0); + ret = genpd_power_on(pd, 0); genpd_unlock(pd); out: return ret ? -EPROBE_DEFER : 0; From 4f497a4310646f14ab518ab6108dbef07f4606af Mon Sep 17 00:00:00 2001 From: Corentin LABBE Date: Thu, 15 Dec 2016 11:15:25 +0100 Subject: [PATCH 2/9] PM / QoS: Remove unneeded linux/miscdevice.h include pm_qos.h does not use any miscdevice, so this patch remove this unnecessary inclusion. Signed-off-by: Corentin Labbe Signed-off-by: Rafael J. Wysocki --- include/linux/pm_qos.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h index 0f65d36c2a75..d4d34791e463 100644 --- a/include/linux/pm_qos.h +++ b/include/linux/pm_qos.h @@ -6,7 +6,6 @@ */ #include #include -#include #include #include From f3c826ac26766f82769319db68f5b4337d6efc24 Mon Sep 17 00:00:00 2001 From: Ulf Hansson Date: Tue, 31 Jan 2017 17:01:03 +0100 Subject: [PATCH 3/9] PM / Domains: Correct comment in irq_safe_dev_in_no_sleep_domain() The earlier comment stated that the dev_warn_once() was going to be printed once per device. Let's fix that, as dev_warn_once() is printed only once, no matter of the device. Reported-by: Geert Uytterhoeven Signed-off-by: Ulf Hansson Acked-by: Lina Iyer Signed-off-by: Rafael J. Wysocki --- drivers/base/power/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index fd2e3e1eac65..b04c14f2b440 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -130,7 +130,7 @@ static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev, ret = pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd); - /* Warn once for each IRQ safe dev in no sleep domain */ + /* Warn once if IRQ safe dev in no sleep domain */ if (ret) dev_warn_once(dev, "PM domain %s will not be powered off\n", genpd->name); From 0883ac038be12c4dba1a68a05030730f10442bc2 Mon Sep 17 00:00:00 2001 From: Ulf Hansson Date: Wed, 8 Feb 2017 13:39:00 +0100 Subject: [PATCH 4/9] PM / Domains: Fix asynchronous execution of *noirq() callbacks As the PM core may invoke the *noirq() callbacks asynchronously, the current lock-less approach in genpd doesn't work. The consequence is that we may find concurrent operations racing to power on/off the PM domain. As of now, no immediate errors has been reported, but it's probably only a matter time. Therefor let's fix the problem now before this becomes a real issue, by deploying the locking scheme to the relevant functions. Reported-by: Brian Norris Signed-off-by: Ulf Hansson Tested-by: Geert Uytterhoeven Signed-off-by: Rafael J. Wysocki --- drivers/base/power/domain.c | 68 +++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index b04c14f2b440..3a75fb1b4126 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -729,16 +729,18 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd, /** * genpd_sync_power_off - Synchronously power off a PM domain and its masters. * @genpd: PM domain to power off, if possible. + * @use_lock: use the lock. + * @depth: nesting count for lockdep. * * Check if the given PM domain can be powered off (during system suspend or * hibernation) and do that if so. Also, in that case propagate to its masters. * * This function is only called in "noirq" and "syscore" stages of system power - * transitions, so it need not acquire locks (all of the "noirq" callbacks are - * executed sequentially, so it is guaranteed that it will never run twice in - * parallel). + * transitions. The "noirq" callbacks may be executed asynchronously, thus in + * these cases the lock must be held. */ -static void genpd_sync_power_off(struct generic_pm_domain *genpd) +static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, + unsigned int depth) { struct gpd_link *link; @@ -757,20 +759,29 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd) list_for_each_entry(link, &genpd->slave_links, slave_node) { genpd_sd_counter_dec(link->master); - genpd_sync_power_off(link->master); + + if (use_lock) + genpd_lock_nested(link->master, depth + 1); + + genpd_sync_power_off(link->master, use_lock, depth + 1); + + if (use_lock) + genpd_unlock(link->master); } } /** * genpd_sync_power_on - Synchronously power on a PM domain and its masters. * @genpd: PM domain to power on. + * @use_lock: use the lock. + * @depth: nesting count for lockdep. * * This function is only called in "noirq" and "syscore" stages of system power - * transitions, so it need not acquire locks (all of the "noirq" callbacks are - * executed sequentially, so it is guaranteed that it will never run twice in - * parallel). + * transitions. The "noirq" callbacks may be executed asynchronously, thus in + * these cases the lock must be held. */ -static void genpd_sync_power_on(struct generic_pm_domain *genpd) +static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, + unsigned int depth) { struct gpd_link *link; @@ -778,8 +789,15 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd) return; list_for_each_entry(link, &genpd->slave_links, slave_node) { - genpd_sync_power_on(link->master); genpd_sd_counter_inc(link->master); + + if (use_lock) + genpd_lock_nested(link->master, depth + 1); + + genpd_sync_power_on(link->master, use_lock, depth + 1); + + if (use_lock) + genpd_unlock(link->master); } _genpd_power_on(genpd, false); @@ -888,13 +906,10 @@ static int pm_genpd_suspend_noirq(struct device *dev) return ret; } - /* - * Since all of the "noirq" callbacks are executed sequentially, it is - * guaranteed that this function will never run twice in parallel for - * the same PM domain, so it is not necessary to use locking here. - */ + genpd_lock(genpd); genpd->suspended_count++; - genpd_sync_power_off(genpd); + genpd_sync_power_off(genpd, true, 0); + genpd_unlock(genpd); return 0; } @@ -919,13 +934,10 @@ static int pm_genpd_resume_noirq(struct device *dev) if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)) return 0; - /* - * Since all of the "noirq" callbacks are executed sequentially, it is - * guaranteed that this function will never run twice in parallel for - * the same PM domain, so it is not necessary to use locking here. - */ - genpd_sync_power_on(genpd); + 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) ret = pm_runtime_force_resume(dev); @@ -1002,13 +1014,10 @@ static int pm_genpd_restore_noirq(struct device *dev) return -EINVAL; /* - * Since all of the "noirq" callbacks are executed sequentially, it is - * guaranteed that this function will never run twice in parallel for - * the same PM domain, so it is not necessary to use locking here. - * * At this point suspended_count == 0 means we are being run for the * first time for the given domain in the present cycle. */ + genpd_lock(genpd); if (genpd->suspended_count++ == 0) /* * The boot kernel might put the domain into arbitrary state, @@ -1017,7 +1026,8 @@ static int pm_genpd_restore_noirq(struct device *dev) */ genpd->status = GPD_STATE_POWER_OFF; - genpd_sync_power_on(genpd); + 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); @@ -1072,9 +1082,9 @@ static void genpd_syscore_switch(struct device *dev, bool suspend) if (suspend) { genpd->suspended_count++; - genpd_sync_power_off(genpd); + genpd_sync_power_off(genpd, false, 0); } else { - genpd_sync_power_on(genpd); + genpd_sync_power_on(genpd, false, 0); genpd->suspended_count--; } } From 16d0bf1882109da84b4b90d7ba4fc7ba18db0d67 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Wed, 8 Feb 2017 19:37:55 +0100 Subject: [PATCH 5/9] PM / Domains: Provide dummy governors if CONFIG_PM_GENERIC_DOMAINS=n This allows to compile-test drivers that refer to governors (always by reference) when CONFIG_PM_GENERIC_DOMAINS=n. Signed-off-by: Geert Uytterhoeven Acked-by: Ulf Hansson Signed-off-by: Rafael J. Wysocki --- include/linux/pm_domain.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 81ece61075df..5339ed5bd6f9 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -182,6 +182,9 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd) { return -ENOTSUPP; } + +#define simple_qos_governor (*(struct dev_power_governor *)(NULL)) +#define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) #endif static inline int pm_genpd_add_device(struct generic_pm_domain *genpd, From c84345597558349474f55be2b7d4093256e42884 Mon Sep 17 00:00:00 2001 From: Grygorii Strashko Date: Fri, 10 Feb 2017 14:25:00 -0800 Subject: [PATCH 6/9] PM / wakeirq: Enable dedicated wakeirq for suspend We currently rely on runtime PM to enable dedicated wakeirq for suspend. This assumption fails in the following two cases: 1. If the consumer driver does not have runtime PM implemented, the dedicated wakeirq never gets enabled for suspend 2. If the consumer driver has runtime PM implemented, but does not idle in suspend Let's fix the issue by always enabling the dedicated wakeirq during suspend. Depends-on: bed570307ed7 (PM / wakeirq: Fix dedicated wakeirq for drivers not using autosuspend) Fixes: 4990d4fe327b (PM / Wakeirq: Add automated device wake IRQ handling) Reported-by: Keerthy Tested-by: Keerthy Signed-off-by: Grygorii Strashko [ tony@atomide.com: updated based on bed570307ed7, added description ] Tested-by: Tony Lindgren Signed-off-by: Tony Lindgren Signed-off-by: Rafael J. Wysocki --- drivers/base/power/wakeirq.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c index 404d94c6c8bc..1b9fc7202933 100644 --- a/drivers/base/power/wakeirq.c +++ b/drivers/base/power/wakeirq.c @@ -312,8 +312,12 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq) if (!wirq) return; - if (device_may_wakeup(wirq->dev)) + if (device_may_wakeup(wirq->dev)) { + if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED) + enable_irq(wirq->irq); + enable_irq_wake(wirq->irq); + } } /** @@ -328,6 +332,10 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq) if (!wirq) return; - if (device_may_wakeup(wirq->dev)) + if (device_may_wakeup(wirq->dev)) { disable_irq_wake(wirq->irq); + + if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED) + disable_irq_nosync(wirq->irq); + } } From 0bf0ee8ef9a6c120f7f320b9fb5fe1270965d828 Mon Sep 17 00:00:00 2001 From: Grygorii Strashko Date: Fri, 10 Feb 2017 14:25:01 -0800 Subject: [PATCH 7/9] PM / wakeirq: Fix spurious wake-up events for dedicated wakeirqs Dedicated wakeirq is a one time event to wake-up the system from low-power state and then call pm_runtime_resume() on the device wired with the dedicated wakeirq. Sometimes dedicated wakeirqs can get deferred if they trigger after we call disable_irq_nosync() in dev_pm_disable_wake_irq(). This can happen if pm_runtime_get() is called around the same time a wakeirq fires. If an interrupt fires after disable_irq_nosync(), by default it will get tagged with IRQS_PENDING and will run later on when the interrupt is enabled again. Deferred wakeirqs usually just produce pointless wake-up events. But they can also cause suspend to fail if the deferred wakeirq fires during dpm_suspend_noirq() for example. So we really don't want to see the deferred wakeirqs triggering after the device has resumed. Let's fix the issue by setting IRQ_DISABLE_UNLAZY flag for the dedicated wakeirqs. The other option would be to implement irq_disable() in the dedicated wakeirq controller, but that's not a generic solution. For reference below is what happens with a IRQ_TYPE_EDGE_BOTH IRQ type wakeirq: - resume by dedicated IRQ (EDGE_FALLING) - suspend_enter() .... - arch_suspend_enable_irqs() |- dedicated IRQ armed and fired |- irq_pm_check_wakeup() |- disarm, disable IRQ and mark as IRQS_PENDING .... - dpm_resume_noirq() |- resume_device_irqs() |- __enable_irq() |- check_irq_resend() |- handle_threaded_wake_irq() |- dedicated IRQ processed |- device_wakeup_disarm_wake_irqs() |- disable_irq_wake() .... !-> dedicated IRQ (EDGE_RISING) -| handle_edge_irq() |- IRQ disabled: mask_ack_irq and mark as IRQS_PENDING .... - subsequent suspend .... |- dpm_suspend_noirq() |- device_wakeup_arm_wake_irqs() |- __enable_irq() |- check_irq_resend() (a) |- handle_threaded_wake_irq() |- pm_wakeup_event() --> abort suspend .... |- suspend_device_irqs() |- suspend_device_irq() |- dedicated IRQ armed .... (b) |- resend_irqs |- irq_pm_check_wakeup() |- IRQ armed -> abort suspend because of pending IRQ System suspend can be aborted at points (a)-not armed or (b)-armed. Fixes: 4990d4fe327b (PM / Wakeirq: Add automated device wake IRQ handling) Signed-off-by: Grygorii Strashko [ tony@atomide.com: added a comment, updated the description ] Tested-by: Tony Lindgren Signed-off-by: Tony Lindgren Signed-off-by: Rafael J. Wysocki --- drivers/base/power/wakeirq.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c index 1b9fc7202933..06f103ce9d82 100644 --- a/drivers/base/power/wakeirq.c +++ b/drivers/base/power/wakeirq.c @@ -183,6 +183,9 @@ int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq) wirq->irq = irq; irq_set_status_flags(irq, IRQ_NOAUTOEN); + /* Prevent deferred spurious wakeirqs with disable_irq_nosync() */ + irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY); + /* * Consumer device may need to power up and restore state * so we use a threaded irq. From 09bb6e93956ae5175b96905b723ec879c3ca0765 Mon Sep 17 00:00:00 2001 From: Grygorii Strashko Date: Mon, 13 Feb 2017 12:43:22 -0800 Subject: [PATCH 8/9] PM / wakeirq: report a wakeup_event on dedicated wekup irq There are two reasons for reporting wakeup event when dedicated wakeup IRQ is triggered: - wakeup events accounting, so proper statistical data will be displayed in sysfs and debugfs; - there are small window when System is entering suspend during which dedicated wakeup IRQ can be lost: dpm_suspend_noirq() |- device_wakeup_arm_wake_irqs() |- dev_pm_arm_wake_irq(X) |- IRQ is enabled and marked as wakeup source [1]... |- suspend_device_irqs() |- suspend_device_irq(X) |- irqd_set(X, IRQD_WAKEUP_ARMED); |- wakup IRQ armed The wakeup IRQ can be lost if it's triggered at point [1] and not armed yet. Hence, fix above cases by adding simple pm_wakeup_event() call in handle_threaded_wake_irq(). Fixes: 4990d4fe327b (PM / Wakeirq: Add automated device wake IRQ handling) Signed-off-by: Grygorii Strashko Tested-by: Keerthy [ tony@atomide.com: added missing return to avoid warnings ] Tested-by: Tony Lindgren Signed-off-by: Tony Lindgren Signed-off-by: Rafael J. Wysocki --- drivers/base/power/wakeirq.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c index 06f103ce9d82..ae0429827f31 100644 --- a/drivers/base/power/wakeirq.c +++ b/drivers/base/power/wakeirq.c @@ -141,6 +141,13 @@ static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq) struct wake_irq *wirq = _wirq; int res; + /* Maybe abort suspend? */ + if (irqd_is_wakeup_set(irq_get_irq_data(irq))) { + pm_wakeup_event(wirq->dev, 0); + + return IRQ_HANDLED; + } + /* We don't want RPM_ASYNC or RPM_NOWAIT here */ res = pm_runtime_resume(wirq->dev); if (res < 0) From e84b4a84e52d4cd8770b4242cd09df0133333f63 Mon Sep 17 00:00:00 2001 From: John Keeping Date: Thu, 16 Feb 2017 17:21:50 +0000 Subject: [PATCH 9/9] PM / QoS: Fix memory leak on resume_latency.notifiers Since commit 2d984ad132a8 (PM / QoS: Introcuce latency tolerance device PM QoS type) we reassign "c" to point at qos->latency_tolerance before freeing c->notifiers, but the notifiers field of latency_tolerance is never used. Restore the original behaviour of freeing the notifiers pointer on qos->resume_latency, which is used, and fix the following kmemleak warning. unreferenced object 0xed9dba00 (size 64): comm "kworker/0:1", pid 36, jiffies 4294670128 (age 15202.983s) hex dump (first 32 bytes): 00 00 00 00 04 ba 9d ed 04 ba 9d ed 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] kmemleak_alloc+0x74/0xb8 [] kmem_cache_alloc_trace+0x170/0x25c [] dev_pm_qos_constraints_allocate+0x3c/0xe4 [] __dev_pm_qos_add_request+0x84/0x1a0 [] dev_pm_qos_add_request+0x3c/0x54 [] usb_hub_create_port_device+0x110/0x2b8 [] hub_probe+0xadc/0xc80 [] usb_probe_interface+0x1b4/0x260 [] driver_probe_device+0x198/0x40c [] __device_attach_driver+0x8c/0x98 [] bus_for_each_drv+0x8c/0x9c [] __device_attach+0x98/0x138 [] device_initial_probe+0x14/0x18 [] bus_probe_device+0x30/0x88 [] device_add+0x430/0x554 [] usb_set_configuration+0x660/0x6fc Fixes: 2d984ad132a8 (PM / QoS: Introcuce latency tolerance device PM QoS type) Signed-off-by: John Keeping Signed-off-by: Rafael J. Wysocki --- drivers/base/power/qos.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c index 58fcc758334e..d888d9869b6a 100644 --- a/drivers/base/power/qos.c +++ b/drivers/base/power/qos.c @@ -281,7 +281,7 @@ void dev_pm_qos_constraints_destroy(struct device *dev) dev->power.qos = ERR_PTR(-ENODEV); spin_unlock_irq(&dev->power.lock); - kfree(c->notifiers); + kfree(qos->resume_latency.notifiers); kfree(qos); out: