From 8b9ec6b732775849f506aa6c2649e626e82a297c Mon Sep 17 00:00:00 2001 From: Alexander Duyck Date: Tue, 22 Jan 2019 10:39:42 -0800 Subject: PM core: Use new async_schedule_dev command Use the device specific version of the async_schedule commands to defer various tasks related to power management. By doing this we should see a slight improvement in performance as any device that is sensitive to latency/locality in the setup will now be initializing on the node closest to the device. Reviewed-by: Dan Williams Reviewed-by: Bart Van Assche Reviewed-by: Rafael J. Wysocki Signed-off-by: Alexander Duyck Signed-off-by: Greg Kroah-Hartman --- drivers/base/power/main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers/base/power') diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 0992e67e862b..93ddbcebcece 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -727,7 +727,7 @@ void dpm_noirq_resume_devices(pm_message_t state) reinit_completion(&dev->power.completion); if (is_async(dev)) { get_device(dev); - async_schedule(async_resume_noirq, dev); + async_schedule_dev(async_resume_noirq, dev); } } @@ -884,7 +884,7 @@ void dpm_resume_early(pm_message_t state) reinit_completion(&dev->power.completion); if (is_async(dev)) { get_device(dev); - async_schedule(async_resume_early, dev); + async_schedule_dev(async_resume_early, dev); } } @@ -1048,7 +1048,7 @@ void dpm_resume(pm_message_t state) reinit_completion(&dev->power.completion); if (is_async(dev)) { get_device(dev); - async_schedule(async_resume, dev); + async_schedule_dev(async_resume, dev); } } @@ -1368,7 +1368,7 @@ static int device_suspend_noirq(struct device *dev) if (is_async(dev)) { get_device(dev); - async_schedule(async_suspend_noirq, dev); + async_schedule_dev(async_suspend_noirq, dev); return 0; } return __device_suspend_noirq(dev, pm_transition, false); @@ -1571,7 +1571,7 @@ static int device_suspend_late(struct device *dev) if (is_async(dev)) { get_device(dev); - async_schedule(async_suspend_late, dev); + async_schedule_dev(async_suspend_late, dev); return 0; } @@ -1835,7 +1835,7 @@ static int device_suspend(struct device *dev) if (is_async(dev)) { get_device(dev); - async_schedule(async_suspend, dev); + async_schedule_dev(async_suspend, dev); return 0; } -- cgit v1.2.3 From e2f3cd831a280fc226118d9369bf3f77aab58c56 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Fri, 1 Feb 2019 01:49:14 +0100 Subject: driver core: Fix handling of runtime PM flags in device_link_add() After commit ead18c23c263 ("driver core: Introduce device links reference counting"), if there is a link between the given supplier and the given consumer already, device_link_add() will refcount it and return it unconditionally without updating its flags. It is possible, however, that the second (or any subsequent) caller of device_link_add() for the same consumer-supplier pair will pass DL_FLAG_PM_RUNTIME, possibly along with DL_FLAG_RPM_ACTIVE, in flags to it and the existing link may not behave as expected then. First, if DL_FLAG_PM_RUNTIME is not set in the existing link's flags at all, it needs to be set like during the original initialization of the link. Second, if DL_FLAG_RPM_ACTIVE is passed to device_link_add() in flags (in addition to DL_FLAG_PM_RUNTIME), the existing link should to be updated to reflect the "active" runtime PM configuration of the consumer-supplier pair and extra care must be taken here to avoid possible destructive races with runtime PM of the consumer. To that end, redefine the rpm_active field in struct device_link as a refcount, initialize it to 1 and make rpm_resume() (for the consumer) and device_link_add() increment it whenever they acquire a runtime PM reference on the supplier device. Accordingly, make rpm_suspend() (for the consumer) and pm_runtime_clean_up_links() decrement it and drop runtime PM references to the supplier device in a loop until rpm_active becones 1 again. Fixes: ead18c23c263 ("driver core: Introduce device links reference counting") Signed-off-by: Rafael J. Wysocki Signed-off-by: Greg Kroah-Hartman --- drivers/base/core.c | 45 +++++++++++++++++++++++++++++--------------- drivers/base/power/runtime.c | 26 +++++++++++-------------- include/linux/device.h | 2 +- 3 files changed, 42 insertions(+), 31 deletions(-) (limited to 'drivers/base/power') diff --git a/drivers/base/core.c b/drivers/base/core.c index 50610cd87e71..8611385e44b5 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -165,6 +165,19 @@ void device_pm_move_to_tail(struct device *dev) device_links_read_unlock(idx); } +static void device_link_rpm_prepare(struct device *consumer, + struct device *supplier) +{ + pm_runtime_new_link(consumer); + /* + * If the link is being added by the consumer driver at probe time, + * balance the decrementation of the supplier's runtime PM usage counter + * after consumer probe in driver_probe_device(). + */ + if (consumer->links.status == DL_DEV_PROBING) + pm_runtime_get_noresume(supplier); +} + /** * device_link_add - Create a link between two devices. * @consumer: Consumer end of the link. @@ -201,7 +214,6 @@ struct device_link *device_link_add(struct device *consumer, struct device *supplier, u32 flags) { struct device_link *link; - bool rpm_put_supplier = false; if (!consumer || !supplier || (flags & DL_FLAG_STATELESS && @@ -213,7 +225,6 @@ struct device_link *device_link_add(struct device *consumer, pm_runtime_put_noidle(supplier); return NULL; } - rpm_put_supplier = true; } device_links_write_lock(); @@ -249,6 +260,15 @@ struct device_link *device_link_add(struct device *consumer, if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER) link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER; + if (flags & DL_FLAG_PM_RUNTIME) { + if (!(link->flags & DL_FLAG_PM_RUNTIME)) { + device_link_rpm_prepare(consumer, supplier); + link->flags |= DL_FLAG_PM_RUNTIME; + } + if (flags & DL_FLAG_RPM_ACTIVE) + refcount_inc(&link->rpm_active); + } + kref_get(&link->kref); goto out; } @@ -257,20 +277,15 @@ struct device_link *device_link_add(struct device *consumer, if (!link) goto out; + refcount_set(&link->rpm_active, 1); + if (flags & DL_FLAG_PM_RUNTIME) { - if (flags & DL_FLAG_RPM_ACTIVE) { - link->rpm_active = true; - rpm_put_supplier = false; - } - pm_runtime_new_link(consumer); - /* - * If the link is being added by the consumer driver at probe - * time, balance the decrementation of the supplier's runtime PM - * usage counter after consumer probe in driver_probe_device(). - */ - if (consumer->links.status == DL_DEV_PROBING) - pm_runtime_get_noresume(supplier); + if (flags & DL_FLAG_RPM_ACTIVE) + refcount_inc(&link->rpm_active); + + device_link_rpm_prepare(consumer, supplier); } + get_device(supplier); link->supplier = supplier; INIT_LIST_HEAD(&link->s_node); @@ -333,7 +348,7 @@ struct device_link *device_link_add(struct device *consumer, device_pm_unlock(); device_links_write_unlock(); - if (rpm_put_supplier) + if ((flags & DL_FLAG_PM_RUNTIME && flags & DL_FLAG_RPM_ACTIVE) && !link) pm_runtime_put(supplier); return link; diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 457be03b744d..8bc9a432de70 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -259,11 +259,8 @@ static int rpm_get_suppliers(struct device *dev) list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) { int retval; - if (!(link->flags & DL_FLAG_PM_RUNTIME)) - continue; - - if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND || - link->rpm_active) + if (!(link->flags & DL_FLAG_PM_RUNTIME) || + READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND) continue; retval = pm_runtime_get_sync(link->supplier); @@ -272,7 +269,7 @@ static int rpm_get_suppliers(struct device *dev) pm_runtime_put_noidle(link->supplier); return retval; } - link->rpm_active = true; + refcount_inc(&link->rpm_active); } return 0; } @@ -281,12 +278,13 @@ static void rpm_put_suppliers(struct device *dev) { struct device_link *link; - list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) - if (link->rpm_active && - READ_ONCE(link->status) != DL_STATE_SUPPLIER_UNBIND) { + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) { + if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND) + continue; + + while (refcount_dec_not_one(&link->rpm_active)) pm_runtime_put(link->supplier); - link->rpm_active = false; - } + } } /** @@ -1539,7 +1537,7 @@ void pm_runtime_remove(struct device *dev) * * Check links from this device to any consumers and if any of them have active * runtime PM references to the device, drop the usage counter of the device - * (once per link). + * (as many times as needed). * * Links with the DL_FLAG_STATELESS flag set are ignored. * @@ -1561,10 +1559,8 @@ void pm_runtime_clean_up_links(struct device *dev) if (link->flags & DL_FLAG_STATELESS) continue; - if (link->rpm_active) { + while (refcount_dec_not_one(&link->rpm_active)) pm_runtime_put_noidle(dev); - link->rpm_active = false; - } } device_links_read_unlock(idx); diff --git a/include/linux/device.h b/include/linux/device.h index d0e452fd0bff..5f49d2eff6ed 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -853,7 +853,7 @@ struct device_link { struct list_head c_node; enum device_link_state status; u32 flags; - bool rpm_active; + refcount_t rpm_active; struct kref kref; #ifdef CONFIG_SRCU struct rcu_head rcu_head; -- cgit v1.2.3 From a1fdbfbb1da2063ba98a12eb6f1bdd07451c7145 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Fri, 1 Feb 2019 01:52:45 +0100 Subject: driver core: Do not call rpm_put_suppliers() in pm_runtime_drop_link() Calling rpm_put_suppliers() from pm_runtime_drop_link() is excessive as it affects all suppliers of the consumer device and not just the one pointed to by the device link being dropped. Worst case it may cause the consumer device to stop working unexpectedly. Moreover, in principle it is racy with respect to runtime PM of the consumer device. To avoid these problems drop runtime PM references on the particular supplier pointed to by the link in question only and do that after the link has been dropped from the consumer device's list of links to suppliers, which is in device_link_free(). Fixes: a0504aecba76 ("PM / runtime: Drop usage count for suppliers at device link removal") Signed-off-by: Rafael J. Wysocki Signed-off-by: Greg Kroah-Hartman --- drivers/base/core.c | 3 +++ drivers/base/power/runtime.c | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) (limited to 'drivers/base/power') diff --git a/drivers/base/core.c b/drivers/base/core.c index d58ea075f807..8c7327d45406 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -374,6 +374,9 @@ EXPORT_SYMBOL_GPL(device_link_add); static void device_link_free(struct device_link *link) { + while (refcount_dec_not_one(&link->rpm_active)) + pm_runtime_put(link->supplier); + put_device(link->consumer); put_device(link->supplier); kfree(link); diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 8bc9a432de70..fd5c4a7b96f0 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1611,8 +1611,6 @@ void pm_runtime_new_link(struct device *dev) void pm_runtime_drop_link(struct device *dev) { - rpm_put_suppliers(dev); - spin_lock_irq(&dev->power.lock); WARN_ON(dev->power.links_count == 0); dev->power.links_count--; -- cgit v1.2.3 From 4080ab083000a1e9656b0d1607e238e7001e0c84 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Thu, 7 Feb 2019 19:38:56 +0100 Subject: PM-runtime: Take suppliers into account in __pm_runtime_set_status() If the target device has any suppliers, as reflected by device links to them, __pm_runtime_set_status() does not take them into account, which is not consistent with the other parts of the PM-runtime framework and may lead to programming mistakes. Modify __pm_runtime_set_status() to take suppliers into account by activating them upfront if the new status is RPM_ACTIVE and deactivating them on exit if the new status is RPM_SUSPENDED. If the activation of one of the suppliers fails, the new status will be RPM_SUSPENDED and the (remaining) suppliers will be deactivated on exit (the child count of the device's parent will be dropped too then). Of course, adding device links locking to __pm_runtime_set_status() means that it cannot be run fron interrupt context, so make it use spin_lock_irq() and spin_unlock_irq() instead of spin_lock_irqsave() and spin_unlock_irqrestore(), respectively. Signed-off-by: Rafael J. Wysocki Signed-off-by: Greg Kroah-Hartman --- drivers/base/power/runtime.c | 45 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 5 deletions(-) (limited to 'drivers/base/power') diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index fd5c4a7b96f0..a0e55065817b 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1089,20 +1089,43 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use); * and the device parent's counter of unsuspended children is modified to * reflect the new status. If the new status is RPM_SUSPENDED, an idle * notification request for the parent is submitted. + * + * If @dev has any suppliers (as reflected by device links to them), and @status + * is RPM_ACTIVE, they will be activated upfront and if the activation of one + * of them fails, the status of @dev will be changed to RPM_SUSPENDED (instead + * of the @status value) and the suppliers will be deacticated on exit. The + * error returned by the failing supplier activation will be returned in that + * case. */ int __pm_runtime_set_status(struct device *dev, unsigned int status) { struct device *parent = dev->parent; - unsigned long flags; bool notify_parent = false; int error = 0; if (status != RPM_ACTIVE && status != RPM_SUSPENDED) return -EINVAL; - spin_lock_irqsave(&dev->power.lock, flags); + /* + * If the new status is RPM_ACTIVE, the suppliers can be activated + * upfront regardless of the current status, because next time + * rpm_put_suppliers() runs, the rpm_active refcounts of the links + * involved will be dropped down to one anyway. + */ + if (status == RPM_ACTIVE) { + int idx = device_links_read_lock(); + + error = rpm_get_suppliers(dev); + if (error) + status = RPM_SUSPENDED; + + device_links_read_unlock(idx); + } + + spin_lock_irq(&dev->power.lock); if (!dev->power.runtime_error && !dev->power.disable_depth) { + status = dev->power.runtime_status; error = -EAGAIN; goto out; } @@ -1134,19 +1157,31 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) spin_unlock(&parent->power.lock); - if (error) + if (error) { + status = RPM_SUSPENDED; goto out; + } } out_set: __update_runtime_status(dev, status); - dev->power.runtime_error = 0; + if (!error) + dev->power.runtime_error = 0; + out: - spin_unlock_irqrestore(&dev->power.lock, flags); + spin_unlock_irq(&dev->power.lock); if (notify_parent) pm_request_idle(parent); + if (status == RPM_SUSPENDED) { + int idx = device_links_read_lock(); + + rpm_put_suppliers(dev); + + device_links_read_unlock(idx); + } + return error; } EXPORT_SYMBOL_GPL(__pm_runtime_set_status); -- cgit v1.2.3 From c1567f813a9992a64f8d0f6cfb912c3922812c35 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Tue, 12 Feb 2019 13:04:12 +0100 Subject: PM-runtime: Fix __pm_runtime_set_status() race with runtime resume Commit 4080ab083000 ("PM-runtime: Take suppliers into account in __pm_runtime_set_status()") introduced a race condition that may trigger if __pm_runtime_set_status() is used incorrectly (that is, if it is called when PM-runtime is enabled for the target device and working). In that case, if the original PM-runtime status of the device is RPM_SUSPENDED, a runtime resume of the device may occur after __pm_runtime_set_status() has dropped its power.lock spinlock and before deactivating its suppliers, so the suppliers may be deactivated while the device is PM-runtime-active which may lead to functional issues. To avoid that, modify __pm_runtime_set_status() to check whether or not PM-runtime is enabled for the device before activating its suppliers (if the new status is RPM_ACTIVE) and either return an error if that's the case or increment the device's disable_depth counter to prevent PM-runtime from being enabled for it while the remaining part of the function is running (disable_depth is then decremented on the way out). Fixes: 4080ab083000 ("PM-runtime: Take suppliers into account in __pm_runtime_set_status()") Signed-off-by: Rafael J. Wysocki Reviewed-by: Ulf Hansson Signed-off-by: Greg Kroah-Hartman --- drivers/base/power/runtime.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) (limited to 'drivers/base/power') diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index ac54f65b6d63..af23eb327f57 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1106,6 +1106,22 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) if (status != RPM_ACTIVE && status != RPM_SUSPENDED) return -EINVAL; + spin_lock_irq(&dev->power.lock); + + /* + * Prevent PM-runtime from being enabled for the device or return an + * error if it is enabled already and working. + */ + if (dev->power.runtime_error || dev->power.disable_depth) + dev->power.disable_depth++; + else + error = -EAGAIN; + + spin_unlock_irq(&dev->power.lock); + + if (error) + return error; + /* * If the new status is RPM_ACTIVE, the suppliers can be activated * upfront regardless of the current status, because next time @@ -1124,12 +1140,6 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) spin_lock_irq(&dev->power.lock); - if (!dev->power.runtime_error && !dev->power.disable_depth) { - status = dev->power.runtime_status; - error = -EAGAIN; - goto out; - } - if (dev->power.runtime_status == status || !parent) goto out_set; @@ -1182,6 +1192,8 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) device_links_read_unlock(idx); } + pm_runtime_enable(dev); + return error; } EXPORT_SYMBOL_GPL(__pm_runtime_set_status); -- cgit v1.2.3 From 4c06c4e6cf63d7f3d5dfe62593a073253d750a59 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Tue, 12 Feb 2019 13:08:10 +0100 Subject: driver core: Fix possible supplier PM-usage counter imbalance If a stateless device link to a certain supplier with DL_FLAG_PM_RUNTIME set in the flags is added and then removed by the consumer driver's probe callback, the supplier's PM-runtime usage counter will be nonzero after that which effectively causes the supplier to remain "always on" going forward. Namely, device_link_add() called to add the link invokes device_link_rpm_prepare() which notices that the consumer driver is probing, so it increments the supplier's PM-runtime usage counter with the assumption that the link will stay around until pm_runtime_put_suppliers() is called by driver_probe_device(), but if the link goes away before that point, the supplier's PM-runtime usage counter will remain nonzero. To prevent that from happening, first rework pm_runtime_get_suppliers() and pm_runtime_put_suppliers() to use the rpm_active refounts of device links and make the latter only drop rpm_active and the supplier's PM-runtime usage counter for each link by one, unless rpm_active is one already for it. Next, modify device_link_add() to bump up the new link's rpm_active refcount and the suppliers PM-runtime usage counter by two, to prevent pm_runtime_put_suppliers(), if it is called subsequently, from suspending the supplier prematurely (in case its PM-runtime usage counter goes down to 0 in there). Due to the way rpm_put_suppliers() works, this change does not affect runtime suspend of the consumer ends of new device links (or, generally, device links for which DL_FLAG_PM_RUNTIME has just been set). Fixes: e2f3cd831a28 ("driver core: Fix handling of runtime PM flags in device_link_add()") Reported-by: Ulf Hansson Signed-off-by: Rafael J. Wysocki Reviewed-by: Ulf Hansson Tested-by: Ulf Hansson Signed-off-by: Greg Kroah-Hartman --- drivers/base/core.c | 21 ++++----------------- drivers/base/power/runtime.c | 27 +++++++++++++++++++++++++-- include/linux/pm_runtime.h | 4 ++++ 3 files changed, 33 insertions(+), 19 deletions(-) (limited to 'drivers/base/power') diff --git a/drivers/base/core.c b/drivers/base/core.c index abfce4f613f8..787190238753 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -165,19 +165,6 @@ void device_pm_move_to_tail(struct device *dev) device_links_read_unlock(idx); } -static void device_link_rpm_prepare(struct device *consumer, - struct device *supplier) -{ - pm_runtime_new_link(consumer); - /* - * If the link is being added by the consumer driver at probe time, - * balance the decrementation of the supplier's runtime PM usage counter - * after consumer probe in driver_probe_device(). - */ - if (consumer->links.status == DL_DEV_PROBING) - pm_runtime_get_noresume(supplier); -} - /** * device_link_add - Create a link between two devices. * @consumer: Consumer end of the link. @@ -286,11 +273,11 @@ struct device_link *device_link_add(struct device *consumer, if (flags & DL_FLAG_PM_RUNTIME) { if (!(link->flags & DL_FLAG_PM_RUNTIME)) { - device_link_rpm_prepare(consumer, supplier); + pm_runtime_new_link(consumer); link->flags |= DL_FLAG_PM_RUNTIME; } if (flags & DL_FLAG_RPM_ACTIVE) - refcount_inc(&link->rpm_active); + pm_runtime_active_link(link, supplier); } if (flags & DL_FLAG_STATELESS) { @@ -323,9 +310,9 @@ struct device_link *device_link_add(struct device *consumer, if (flags & DL_FLAG_PM_RUNTIME) { if (flags & DL_FLAG_RPM_ACTIVE) - refcount_inc(&link->rpm_active); + pm_runtime_active_link(link, supplier); - device_link_rpm_prepare(consumer, supplier); + pm_runtime_new_link(consumer); } get_device(supplier); diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index af23eb327f57..6b8aa6bed064 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1625,8 +1625,10 @@ void pm_runtime_get_suppliers(struct device *dev) idx = device_links_read_lock(); list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) - if (link->flags & DL_FLAG_PM_RUNTIME) + if (link->flags & DL_FLAG_PM_RUNTIME) { + refcount_inc(&link->rpm_active); pm_runtime_get_sync(link->supplier); + } device_links_read_unlock(idx); } @@ -1643,7 +1645,8 @@ void pm_runtime_put_suppliers(struct device *dev) idx = device_links_read_lock(); list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) - if (link->flags & DL_FLAG_PM_RUNTIME) + if (link->flags & DL_FLAG_PM_RUNTIME && + refcount_dec_not_one(&link->rpm_active)) pm_runtime_put(link->supplier); device_links_read_unlock(idx); @@ -1656,6 +1659,26 @@ void pm_runtime_new_link(struct device *dev) spin_unlock_irq(&dev->power.lock); } +/** + * pm_runtime_active_link - Set up new device link as active for PM-runtime. + * @link: Device link to be set up as active. + * @supplier: Supplier end of the link. + * + * Add 2 to the rpm_active refcount of @link and increment the PM-runtime + * usage counter of @supplier once more in case the link is being added while + * the consumer driver is probing and pm_runtime_put_suppliers() will be called + * subsequently. + * + * Note that this doesn't prevent rpm_put_suppliers() from decreasing the link's + * rpm_active refcount down to one, so runtime suspend of the consumer end of + * @link is not affected. + */ +void pm_runtime_active_link(struct device_link *link, struct device *supplier) +{ + refcount_add(2, &link->rpm_active); + pm_runtime_get_noresume(supplier); +} + void pm_runtime_drop_link(struct device *dev) { spin_lock_irq(&dev->power.lock); diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index fed5be706bc9..a27bbb5937b8 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -59,6 +59,8 @@ extern void pm_runtime_clean_up_links(struct device *dev); extern void pm_runtime_get_suppliers(struct device *dev); extern void pm_runtime_put_suppliers(struct device *dev); extern void pm_runtime_new_link(struct device *dev); +extern void pm_runtime_active_link(struct device_link *link, + struct device *supplier); extern void pm_runtime_drop_link(struct device *dev); static inline void pm_suspend_ignore_children(struct device *dev, bool enable) @@ -176,6 +178,8 @@ static inline void pm_runtime_clean_up_links(struct device *dev) {} static inline void pm_runtime_get_suppliers(struct device *dev) {} static inline void pm_runtime_put_suppliers(struct device *dev) {} static inline void pm_runtime_new_link(struct device *dev) {} +static inline void pm_runtime_active_link(struct device_link *link, + struct device *supplier) {} static inline void pm_runtime_drop_link(struct device *dev) {} #endif /* !CONFIG_PM */ -- cgit v1.2.3 From 36003d4cf57ca431fb3f94d317bcca426a2394d6 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Tue, 19 Feb 2019 17:53:26 +0100 Subject: driver core: Fix PM-runtime for links added during consumer probe Commit 4c06c4e6cf63 ("driver core: Fix possible supplier PM-usage counter imbalance") introduced a regression that causes suppliers to be suspended prematurely for device links added during consumer driver probe if the initial PM-runtime status of the consumer is "suspended" and the consumer is resumed after adding the link and before pm_runtime_put_suppliers() is called. In that case, pm_runtime_put_suppliers() will drop the rpm_active refcount for the link by one and (since rpm_active is equal to two after the preceding consumer resume) the supplier's PM-runtime usage counter will be decremented, which may cause the supplier to suspend even though the consumer's PM-runtime status is "active". For this reason, partially revert commit 4c06c4e6cf63 as the problem it tried to fix needs to be addressed somewhat differently, and change pm_runtime_get_suppliers() and pm_runtime_put_suppliers() so that the latter only drops rpm_active references acquired by the former. [This requires adding a new field to struct device_link, but I coulnd't find a cleaner way to address the issue that would work in all cases.] This causes pm_runtime_put_suppliers() to effectively ignore device links added during consumer probe, so device_link_add() doesn't need to worry about ensuring that suppliers will remain active after pm_runtime_put_suppliers() for links created with DL_FLAG_RPM_ACTIVE set and it only needs to bump up rpm_active by one for those links, so pm_runtime_active_link() is not necessary any more. Fixes: 4c06c4e6cf63 ("driver core: Fix possible supplier PM-usage counter imbalance") Reported-by: Jon Hunter Tested-by: Jon Hunter Tested-by: Ulf Hansson Reviewed-by: Ulf Hansson Signed-off-by: Rafael J. Wysocki Tested-by: Thierry Reding Signed-off-by: Greg Kroah-Hartman --- drivers/base/core.c | 4 ++-- drivers/base/power/runtime.c | 29 ++++++----------------------- include/linux/device.h | 1 + include/linux/pm_runtime.h | 4 ---- 4 files changed, 9 insertions(+), 29 deletions(-) (limited to 'drivers/base/power') diff --git a/drivers/base/core.c b/drivers/base/core.c index 787190238753..4aeaa0c92bda 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -277,7 +277,7 @@ struct device_link *device_link_add(struct device *consumer, link->flags |= DL_FLAG_PM_RUNTIME; } if (flags & DL_FLAG_RPM_ACTIVE) - pm_runtime_active_link(link, supplier); + refcount_inc(&link->rpm_active); } if (flags & DL_FLAG_STATELESS) { @@ -310,7 +310,7 @@ struct device_link *device_link_add(struct device *consumer, if (flags & DL_FLAG_PM_RUNTIME) { if (flags & DL_FLAG_RPM_ACTIVE) - pm_runtime_active_link(link, supplier); + refcount_inc(&link->rpm_active); pm_runtime_new_link(consumer); } diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 6b8aa6bed064..70d2cb188601 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1626,6 +1626,7 @@ void pm_runtime_get_suppliers(struct device *dev) list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) if (link->flags & DL_FLAG_PM_RUNTIME) { + link->supplier_preactivated = true; refcount_inc(&link->rpm_active); pm_runtime_get_sync(link->supplier); } @@ -1645,9 +1646,11 @@ void pm_runtime_put_suppliers(struct device *dev) idx = device_links_read_lock(); list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) - if (link->flags & DL_FLAG_PM_RUNTIME && - refcount_dec_not_one(&link->rpm_active)) - pm_runtime_put(link->supplier); + if (link->supplier_preactivated) { + link->supplier_preactivated = false; + if (refcount_dec_not_one(&link->rpm_active)) + pm_runtime_put(link->supplier); + } device_links_read_unlock(idx); } @@ -1659,26 +1662,6 @@ void pm_runtime_new_link(struct device *dev) spin_unlock_irq(&dev->power.lock); } -/** - * pm_runtime_active_link - Set up new device link as active for PM-runtime. - * @link: Device link to be set up as active. - * @supplier: Supplier end of the link. - * - * Add 2 to the rpm_active refcount of @link and increment the PM-runtime - * usage counter of @supplier once more in case the link is being added while - * the consumer driver is probing and pm_runtime_put_suppliers() will be called - * subsequently. - * - * Note that this doesn't prevent rpm_put_suppliers() from decreasing the link's - * rpm_active refcount down to one, so runtime suspend of the consumer end of - * @link is not affected. - */ -void pm_runtime_active_link(struct device_link *link, struct device *supplier) -{ - refcount_add(2, &link->rpm_active); - pm_runtime_get_noresume(supplier); -} - void pm_runtime_drop_link(struct device *dev) { spin_lock_irq(&dev->power.lock); diff --git a/include/linux/device.h b/include/linux/device.h index 292b720c4bc2..a7967a48cdc9 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -861,6 +861,7 @@ struct device_link { #ifdef CONFIG_SRCU struct rcu_head rcu_head; #endif + bool supplier_preactivated; /* Owned by consumer probe. */ }; /** diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index a27bbb5937b8..fed5be706bc9 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -59,8 +59,6 @@ extern void pm_runtime_clean_up_links(struct device *dev); extern void pm_runtime_get_suppliers(struct device *dev); extern void pm_runtime_put_suppliers(struct device *dev); extern void pm_runtime_new_link(struct device *dev); -extern void pm_runtime_active_link(struct device_link *link, - struct device *supplier); extern void pm_runtime_drop_link(struct device *dev); static inline void pm_suspend_ignore_children(struct device *dev, bool enable) @@ -178,8 +176,6 @@ static inline void pm_runtime_clean_up_links(struct device *dev) {} static inline void pm_runtime_get_suppliers(struct device *dev) {} static inline void pm_runtime_put_suppliers(struct device *dev) {} static inline void pm_runtime_new_link(struct device *dev) {} -static inline void pm_runtime_active_link(struct device_link *link, - struct device *supplier) {} static inline void pm_runtime_drop_link(struct device *dev) {} #endif /* !CONFIG_PM */ -- cgit v1.2.3