From 0cab893f409c53634d0d818fa414641cbcdb0dab Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Fri, 19 Mar 2021 15:47:25 +0100 Subject: Revert "PM: runtime: Update device status before letting suppliers suspend" Revert commit 44cc89f76464 ("PM: runtime: Update device status before letting suppliers suspend") that introduced a race condition into __rpm_callback() which allowed a concurrent rpm_resume() to run and resume the device prematurely after its status had been changed to RPM_SUSPENDED by __rpm_callback(). Fixes: 44cc89f76464 ("PM: runtime: Update device status before letting suppliers suspend") Link: https://lore.kernel.org/linux-pm/24dfb6fc-5d54-6ee2-9195-26428b7ecf8a@intel.com/ Reported-by: Adrian Hunter Cc: 4.10+ # 4.10+ Signed-off-by: Rafael J. Wysocki Reviewed-by: Ulf Hansson --- drivers/base/power/runtime.c | 62 ++++++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 37 deletions(-) (limited to 'drivers/base/power/runtime.c') diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 18b82427d0cb..a46a7e30881b 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -325,22 +325,22 @@ static void rpm_put_suppliers(struct device *dev) static int __rpm_callback(int (*cb)(struct device *), struct device *dev) __releases(&dev->power.lock) __acquires(&dev->power.lock) { - bool use_links = dev->power.links_count > 0; - bool get = false; int retval, idx; - bool put; + bool use_links = dev->power.links_count > 0; if (dev->power.irq_safe) { spin_unlock(&dev->power.lock); - } else if (!use_links) { - spin_unlock_irq(&dev->power.lock); } else { - get = dev->power.runtime_status == RPM_RESUMING; - spin_unlock_irq(&dev->power.lock); - /* Resume suppliers if necessary. */ - if (get) { + /* + * Resume suppliers if necessary. + * + * The device's runtime PM status cannot change until this + * routine returns, so it is safe to read the status outside of + * the lock. + */ + if (use_links && dev->power.runtime_status == RPM_RESUMING) { idx = device_links_read_lock(); retval = rpm_get_suppliers(dev); @@ -355,36 +355,24 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev) if (dev->power.irq_safe) { spin_lock(&dev->power.lock); - return retval; - } - - spin_lock_irq(&dev->power.lock); - - if (!use_links) - return retval; - - /* - * If the device is suspending and the callback has returned success, - * drop the usage counters of the suppliers that have been reference - * counted on its resume. - * - * Do that if the resume fails too. - */ - put = dev->power.runtime_status == RPM_SUSPENDING && !retval; - if (put) - __update_runtime_status(dev, RPM_SUSPENDED); - else - put = get && retval; - - if (put) { - spin_unlock_irq(&dev->power.lock); - - idx = device_links_read_lock(); + } else { + /* + * If the device is suspending and the callback has returned + * success, drop the usage counters of the suppliers that have + * been reference counted on its resume. + * + * Do that if resume fails too. + */ + if (use_links + && ((dev->power.runtime_status == RPM_SUSPENDING && !retval) + || (dev->power.runtime_status == RPM_RESUMING && retval))) { + idx = device_links_read_lock(); -fail: - rpm_put_suppliers(dev); + fail: + rpm_put_suppliers(dev); - device_links_read_unlock(idx); + device_links_read_unlock(idx); + } spin_lock_irq(&dev->power.lock); } -- cgit v1.2.3 From 5244f5e2d801259af877ee759e8c22364c607072 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Fri, 19 Mar 2021 15:47:31 +0100 Subject: PM: runtime: Defer suspending suppliers Because the PM-runtime status of the device is not updated in __rpm_callback(), attempts to suspend the suppliers of the given device triggered by the rpm_put_suppliers() call in there may cause a supplier to be suspended completely before the status of the consumer is updated to RPM_SUSPENDED, which is confusing. To avoid that (1) modify __rpm_callback() to only decrease the PM-runtime usage counter of each supplier and (2) make rpm_suspend() try to suspend the suppliers after changing the consumer's status to RPM_SUSPENDED, in analogy with the device's parent. Link: https://lore.kernel.org/linux-pm/CAPDyKFqm06KDw_p8WXsM4dijDbho4bb6T4k50UqqvR1_COsp8g@mail.gmail.com/ Fixes: 21d5c57b3726 ("PM / runtime: Use device links") Reported-by: elaine.zhang Diagnosed-by: Ulf Hansson Reviewed-by: Ulf Hansson Signed-off-by: Rafael J. Wysocki --- drivers/base/power/runtime.c | 45 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-) (limited to 'drivers/base/power/runtime.c') diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index a46a7e30881b..d54e540067bf 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -305,7 +305,7 @@ static int rpm_get_suppliers(struct device *dev) return 0; } -static void rpm_put_suppliers(struct device *dev) +static void __rpm_put_suppliers(struct device *dev, bool try_to_suspend) { struct device_link *link; @@ -313,10 +313,30 @@ static void rpm_put_suppliers(struct device *dev) device_links_read_lock_held()) { while (refcount_dec_not_one(&link->rpm_active)) - pm_runtime_put(link->supplier); + pm_runtime_put_noidle(link->supplier); + + if (try_to_suspend) + pm_request_idle(link->supplier); } } +static void rpm_put_suppliers(struct device *dev) +{ + __rpm_put_suppliers(dev, true); +} + +static void rpm_suspend_suppliers(struct device *dev) +{ + struct device_link *link; + int idx = device_links_read_lock(); + + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node, + device_links_read_lock_held()) + pm_request_idle(link->supplier); + + device_links_read_unlock(idx); +} + /** * __rpm_callback - Run a given runtime PM callback for a given device. * @cb: Runtime PM callback to run. @@ -344,8 +364,10 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev) idx = device_links_read_lock(); retval = rpm_get_suppliers(dev); - if (retval) + if (retval) { + rpm_put_suppliers(dev); goto fail; + } device_links_read_unlock(idx); } @@ -368,9 +390,9 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev) || (dev->power.runtime_status == RPM_RESUMING && retval))) { idx = device_links_read_lock(); - fail: - rpm_put_suppliers(dev); + __rpm_put_suppliers(dev, false); +fail: device_links_read_unlock(idx); } @@ -642,8 +664,11 @@ static int rpm_suspend(struct device *dev, int rpmflags) goto out; } + if (dev->power.irq_safe) + goto out; + /* Maybe the parent is now able to suspend. */ - if (parent && !parent->power.ignore_children && !dev->power.irq_safe) { + if (parent && !parent->power.ignore_children) { spin_unlock(&dev->power.lock); spin_lock(&parent->power.lock); @@ -652,6 +677,14 @@ static int rpm_suspend(struct device *dev, int rpmflags) spin_lock(&dev->power.lock); } + /* Maybe the suppliers are now able to suspend. */ + if (dev->power.links_count > 0) { + spin_unlock_irq(&dev->power.lock); + + rpm_suspend_suppliers(dev); + + spin_lock_irq(&dev->power.lock); + } out: trace_rpm_return_int_rcuidle(dev, _THIS_IP_, retval); -- cgit v1.2.3