From 013c074f8642d8e815ad670601f8e27155a74b57 Mon Sep 17 00:00:00 2001 From: "Strashko, Grygorii" Date: Tue, 10 Nov 2015 11:42:34 +0200 Subject: [PATCH 1/2] PM / sleep: prohibit devices probing during suspend/hibernation It is unsafe [1] if probing of devices will happen during suspend or hibernation and system behavior will be unpredictable in this case. So, let's prohibit device's probing in dpm_prepare() and defer their probing instead. The normal behavior will be restored in dpm_complete(). This patch introduces new DD core APIs: device_block_probing() It will disable probing of devices and defer their probes instead. device_unblock_probing() It will restore normal behavior and trigger re-probing of deferred devices. [1] https://lkml.org/lkml/2015/9/11/554 Signed-off-by: Grygorii Strashko Acked-by: Pavel Machek Signed-off-by: Rafael J. Wysocki --- drivers/base/base.h | 2 ++ drivers/base/dd.c | 48 ++++++++++++++++++++++++++++++++++++++- drivers/base/power/main.c | 17 ++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/drivers/base/base.h b/drivers/base/base.h index 1782f3aa386e..e05db388bd1c 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -131,6 +131,8 @@ extern void device_remove_groups(struct device *dev, extern char *make_class_name(const char *name, struct kobject *kobj); extern int devres_release_all(struct device *dev); +extern void device_block_probing(void); +extern void device_unblock_probing(void); /* /sys/devices directory */ extern struct kset *devices_kset; diff --git a/drivers/base/dd.c b/drivers/base/dd.c index a641cf3ccad6..b605f734b882 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -54,6 +54,13 @@ static LIST_HEAD(deferred_probe_active_list); static struct workqueue_struct *deferred_wq; static atomic_t deferred_trigger_count = ATOMIC_INIT(0); +/* + * In some cases, like suspend to RAM or hibernation, It might be reasonable + * to prohibit probing of devices as it could be unsafe. + * Once defer_all_probes is true all drivers probes will be forcibly deferred. + */ +static bool defer_all_probes; + /* * deferred_probe_work_func() - Retry probing devices in the active list. */ @@ -171,6 +178,30 @@ static void driver_deferred_probe_trigger(void) queue_work(deferred_wq, &deferred_probe_work); } +/** + * device_block_probing() - Block/defere device's probes + * + * It will disable probing of devices and defer their probes instead. + */ +void device_block_probing(void) +{ + defer_all_probes = true; + /* sync with probes to avoid races. */ + wait_for_device_probe(); +} + +/** + * device_unblock_probing() - Unblock/enable device's probes + * + * It will restore normal behavior and trigger re-probing of deferred + * devices. + */ +void device_unblock_probing(void) +{ + defer_all_probes = false; + driver_deferred_probe_trigger(); +} + /** * deferred_probe_initcall() - Enable probing of deferred devices * @@ -277,9 +308,20 @@ static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue); static int really_probe(struct device *dev, struct device_driver *drv) { - int ret = 0; + int ret = -EPROBE_DEFER; int local_trigger_count = atomic_read(&deferred_trigger_count); + if (defer_all_probes) { + /* + * Value of defer_all_probes can be set only by + * device_defer_all_probes_enable() which, in turn, will call + * wait_for_device_probe() right after that to avoid any races. + */ + dev_dbg(dev, "Driver %s force probe deferral\n", drv->name); + driver_deferred_probe_add(dev); + return ret; + } + atomic_inc(&probe_count); pr_debug("bus: '%s': %s: probing driver %s with device %s\n", drv->bus->name, __func__, drv->name, dev_name(dev)); @@ -393,6 +435,10 @@ int driver_probe_done(void) */ void wait_for_device_probe(void) { + /* wait for the deferred probe workqueue to finish */ + if (driver_deferred_probe_enable) + flush_workqueue(deferred_wq); + /* wait for the known devices to complete their probing */ wait_event(probe_waitqueue, atomic_read(&probe_count) == 0); async_synchronize_full(); diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 1710c26ba097..9d626ac08d9c 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -963,6 +963,9 @@ void dpm_complete(pm_message_t state) } list_splice(&list, &dpm_list); mutex_unlock(&dpm_list_mtx); + + /* Allow device probing and trigger re-probing of deferred devices */ + device_unblock_probing(); trace_suspend_resume(TPS("dpm_complete"), state.event, false); } @@ -1624,6 +1627,20 @@ int dpm_prepare(pm_message_t state) trace_suspend_resume(TPS("dpm_prepare"), state.event, true); might_sleep(); + /* + * Give a chance for the known devices to complete their probes, before + * disable probing of devices. This sync point is important at least + * at boot time + hibernation restore. + */ + wait_for_device_probe(); + /* + * It is unsafe if probing of devices will happen during suspend or + * hibernation and system behavior will be unpredictable in this case. + * So, let's prohibit device's probing here and defer their probes + * instead. The normal behavior will be restored in dpm_complete(). + */ + device_block_probing(); + mutex_lock(&dpm_list_mtx); while (!list_empty(&dpm_list)) { struct device *dev = to_device(dpm_list.next); From 5de85b9d57aba3ed2e04759e6db3b9e826dd0b06 Mon Sep 17 00:00:00 2001 From: Ulf Hansson Date: Wed, 18 Nov 2015 11:48:39 +0100 Subject: [PATCH 2/2] PM / runtime: Re-init runtime PM states at probe error and driver unbind There are two common expectations among several subsystems/drivers that deploys runtime PM support, but which isn't met by the driver core. Expectation 1) At ->probe() the subsystem/driver expects the runtime PM status of the device to be RPM_SUSPENDED, which is the initial status being assigned at device registration. This expectation is especially common among some of those subsystems/ drivers that manages devices with an attached PM domain, as those requires the ->runtime_resume() callback at the PM domain level to be invoked during ->probe(). Moreover these subsystems/drivers entirely relies on runtime PM resources being managed at the PM domain level, thus don't implement their own set of runtime PM callbacks. These are two scenarios that suffers from this unmet expectation. i) A failed ->probe() sequence requests probe deferral: ->probe() ... pm_runtime_enable() pm_runtime_get_sync() ... err: pm_runtime_put() pm_runtime_disable() ... As there are no guarantees that such sequence turns the runtime PM status of the device into RPM_SUSPENDED, the re-trying ->probe() may start with the status in RPM_ACTIVE. In such case the runtime PM core won't invoke the ->runtime_resume() callback because of a pm_runtime_get_sync(), as it considers the device to be already runtime resumed. ii) A driver re-bind sequence: At driver unbind, the subsystem/driver's >remove() callback invokes a sequence of runtime PM APIs, to undo actions during ->probe() and to put the device into low power state. ->remove() ... pm_runtime_put() pm_runtime_disable() ... Similar as in the failing ->probe() case, this sequence don't guarantee the runtime PM status of the device to turn into RPM_SUSPENDED. Trying to re-bind the driver thus causes the same issue as when re-trying ->probe(), in the probe deferral scenario. Expectation 2) Drivers that invokes the pm_runtime_irq_safe() API during ->probe(), triggers the runtime PM core to increase the usage count for the device's parent and permanently make it runtime resumed. The usage count is only dropped at device removal, which also allows it to be runtime suspended again. A re-trying ->probe() repeats the call to pm_runtime_irq_safe() and thus once more triggers the usage count of the device's parent to be increased. This leads to not only an imbalance issue of the usage count of the device's parent, but also to keep it runtime resumed permanently even if ->probe() fails. To address these issues, let's change the policy of the driver core to meet these expectations. More precisely, at ->probe() failures and driver unbind, restore the initial states of runtime PM. Although to still allow subsystem's to control PM for devices that doesn't ->probe() successfully, don't restore the initial states unless runtime PM is disabled. Signed-off-by: Ulf Hansson Reviewed-by: Kevin Hilman Signed-off-by: Rafael J. Wysocki --- drivers/base/dd.c | 2 ++ drivers/base/power/power.h | 2 ++ drivers/base/power/runtime.c | 26 ++++++++++++++++++++------ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/drivers/base/dd.c b/drivers/base/dd.c index a641cf3ccad6..cd2d79b1bf01 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -340,6 +340,7 @@ static int really_probe(struct device *dev, struct device_driver *drv) dev_set_drvdata(dev, NULL); if (dev->pm_domain && dev->pm_domain->dismiss) dev->pm_domain->dismiss(dev); + pm_runtime_reinit(dev); switch (ret) { case -EPROBE_DEFER: @@ -695,6 +696,7 @@ static void __device_release_driver(struct device *dev) dev_set_drvdata(dev, NULL); if (dev->pm_domain && dev->pm_domain->dismiss) dev->pm_domain->dismiss(dev); + pm_runtime_reinit(dev); klist_remove(&dev->p->knode_driver); if (dev->bus) diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h index 998fa6b23084..8b06193d4a5e 100644 --- a/drivers/base/power/power.h +++ b/drivers/base/power/power.h @@ -18,6 +18,7 @@ static inline void pm_runtime_early_init(struct device *dev) } extern void pm_runtime_init(struct device *dev); +extern void pm_runtime_reinit(struct device *dev); extern void pm_runtime_remove(struct device *dev); struct wake_irq { @@ -84,6 +85,7 @@ static inline void pm_runtime_early_init(struct device *dev) } static inline void pm_runtime_init(struct device *dev) {} +static inline void pm_runtime_reinit(struct device *dev) {} static inline void pm_runtime_remove(struct device *dev) {} static inline int dpm_sysfs_add(struct device *dev) { return 0; } diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index e1a10a03df8e..ab3fcd9f6c98 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1389,6 +1389,25 @@ void pm_runtime_init(struct device *dev) init_waitqueue_head(&dev->power.wait_queue); } +/** + * pm_runtime_reinit - Re-initialize runtime PM fields in given device object. + * @dev: Device object to re-initialize. + */ +void pm_runtime_reinit(struct device *dev) +{ + if (!pm_runtime_enabled(dev)) { + if (dev->power.runtime_status == RPM_ACTIVE) + pm_runtime_set_suspended(dev); + if (dev->power.irq_safe) { + spin_lock_irq(&dev->power.lock); + dev->power.irq_safe = 0; + spin_unlock_irq(&dev->power.lock); + if (dev->parent) + pm_runtime_put(dev->parent); + } + } +} + /** * pm_runtime_remove - Prepare for removing a device from device hierarchy. * @dev: Device object being removed from device hierarchy. @@ -1396,12 +1415,7 @@ void pm_runtime_init(struct device *dev) void pm_runtime_remove(struct device *dev) { __pm_runtime_disable(dev, false); - - /* Change the status back to 'suspended' to match the initial status. */ - if (dev->power.runtime_status == RPM_ACTIVE) - pm_runtime_set_suspended(dev); - if (dev->power.irq_safe && dev->parent) - pm_runtime_put(dev->parent); + pm_runtime_reinit(dev); } /**