[PATCH 0/3] PM: runtime: Fixes related to device links management

[PATCH 0/3] PM: runtime: Fixes related to device links management

[Rafael J. Wysocki]

Hi Greg & all,

Commit d12544fb2aa9 ("PM: runtime: Remove link state checks in
rpm_get/put_supplier()") merged recently introduced a weakness
in the handling of device links in the runtime PM framework that
may be confusing and even harmful.

Namely, the checks removed by that commit prevented PM-runtime from
getting or dropping references to the supplier device whose driver
was going away via its links to consumers, which specifically allowed
the pm_runtime_clean_up_links() called from __device_release_driver()
to run without interfering with runtime suspend/resume of consumer
devices (which still might happen even though the drivers had been
unbound from them by that time).

After the above commit, calling pm_runtime_clean_up_links() from
__device_release_driver() makes a little sense and it may be interfering
destructively with regular PM-runtime suspend/resume control flows, so
it needs to be either fixed or dropped altogether.  I prefer the latter,
because among other things this removes an arbitrary difference in the
handling of managed device links with respect to the stateless ones,
so patch [2/3] is doing just that.

However, in some rare cases pm_runtime_clean_up_links() may help to clean
up leftover PM-runtime references, so if that function goes away, they
need to be cleaned up elsewhere.  That's why patch [1/3] modifies
__device_link_del() to drop them upon device link removal (which also
needs to be done for stateless device links and that's why I'm regarding
this patch as a fix).

Finally, to avoid pointless overhead related to suspending and resuming
the target device for multiple times in a row in __device_release_driver(),
it is better to resume it upfront before checking its links to consumers,
which is done by patch [3/3].

While this series touches the driver core, it really is mostly related to
runtime PM, so I can apply it if that's OK.

Thanks!

[PATCH 1/3] PM: runtime: Drop runtime PM references to supplier on link removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

While removing a device link, drop the supplier device's runtime PM
usage counter as many times as needed to drop all of the runtime PM
references to it from the consumer in addition to dropping the
consumer's link count.

Fixes: baa8809f6097 ("PM / runtime: Optimize the use of device links")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: 5.1+ <stable@vger.kernel.org> # 5.1+
---
 drivers/base/core.c          |    6 ++----
 drivers/base/power/runtime.c |   21 ++++++++++++++++++++-
 include/linux/pm_runtime.h   |    4 ++--
 3 files changed, 24 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -1729,7 +1729,7 @@ void pm_runtime_new_link(struct device *
 	spin_unlock_irq(&dev->power.lock);
 }
 
-void pm_runtime_drop_link(struct device *dev)
+static void pm_runtime_drop_link_count(struct device *dev)
 {
 	spin_lock_irq(&dev->power.lock);
 	WARN_ON(dev->power.links_count == 0);
@@ -1737,6 +1737,25 @@ void pm_runtime_drop_link(struct device
 	spin_unlock_irq(&dev->power.lock);
 }
 
+/**
+ * pm_runtime_drop_link - Prepare for device link removal.
+ * @link: Device link going away.
+ *
+ * Drop the link count of the consumer end of @link and decrement the supplier
+ * device's runtime PM usage counter as many times as needed to drop all of the
+ * PM runtime reference to it from the consumer.
+ */
+void pm_runtime_drop_link(struct device_link *link)
+{
+	if (!(link->flags & DL_FLAG_PM_RUNTIME))
+		return;
+
+	pm_runtime_drop_link_count(link->consumer);
+
+	while (refcount_dec_not_one(&link->rpm_active))
+		pm_runtime_put(link->supplier);
+}
+
 static bool pm_runtime_need_not_resume(struct device *dev)
 {
 	return atomic_read(&dev->power.usage_count) <= 1 &&
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -58,7 +58,7 @@ extern void pm_runtime_clean_up_links(st
 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_drop_link(struct device *dev);
+extern void pm_runtime_drop_link(struct device_link *link);
 
 /**
  * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
@@ -280,7 +280,7 @@ static inline void pm_runtime_clean_up_l
 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_drop_link(struct device *dev) {}
+static inline void pm_runtime_drop_link(struct device_link *link) {}
 
 #endif /* !CONFIG_PM */
 
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -763,8 +763,7 @@ static void __device_link_del(struct kre
 	dev_dbg(link->consumer, "Dropping the link to %s\n",
 		dev_name(link->supplier));
 
-	if (link->flags & DL_FLAG_PM_RUNTIME)
-		pm_runtime_drop_link(link->consumer);
+	pm_runtime_drop_link(link);
 
 	list_del_rcu(&link->s_node);
 	list_del_rcu(&link->c_node);
@@ -778,8 +777,7 @@ static void __device_link_del(struct kre
 	dev_info(link->consumer, "Dropping the link to %s\n",
 		 dev_name(link->supplier));
 
-	if (link->flags & DL_FLAG_PM_RUNTIME)
-		pm_runtime_drop_link(link->consumer);
+	pm_runtime_drop_link(link);
 
 	list_del(&link->s_node);
 	list_del(&link->c_node);

[PATCH 2/3] PM: runtime: Drop pm_runtime_clean_up_links()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit d12544fb2aa9 ("PM: runtime: Remove link state checks in
rpm_get/put_supplier()") nothing prevents the consumer device's
runtime PM from acquiring additional references to the supplier
device after pm_runtime_clean_up_links() has run (or even while it
is running), so calling this function from __device_release_driver()
may be pointless (or even harmful).

Moreover, it ignores stateless device links, so the runtime PM
handling of managed and stateless device links is inconsistent
because of it, so better get rid of it entirely.

Fixes: d12544fb2aa9 ("PM: runtime: Remove link state checks in rpm_get/put_supplier()")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: 5.1+ <stable@vger.kernel.org> # 5.1+
---
 drivers/base/dd.c            |    1 -
 drivers/base/power/runtime.c |   36 ------------------------------------
 include/linux/pm_runtime.h   |    2 --
 3 files changed, 39 deletions(-)

Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -1133,7 +1133,6 @@ static void __device_release_driver(stru
 		}
 
 		pm_runtime_get_sync(dev);
-		pm_runtime_clean_up_links(dev);
 
 		driver_sysfs_remove(dev);
 
Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -1643,42 +1643,6 @@ void pm_runtime_remove(struct device *de
 }
 
 /**
- * pm_runtime_clean_up_links - Prepare links to consumers for driver removal.
- * @dev: Device whose driver is going to be removed.
- *
- * 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
- * (as many times as needed).
- *
- * Links with the DL_FLAG_MANAGED flag unset are ignored.
- *
- * Since the device is guaranteed to be runtime-active at the point this is
- * called, nothing else needs to be done here.
- *
- * Moreover, this is called after device_links_busy() has returned 'false', so
- * the status of each link is guaranteed to be DL_STATE_SUPPLIER_UNBIND and
- * therefore rpm_active can't be manipulated concurrently.
- */
-void pm_runtime_clean_up_links(struct device *dev)
-{
-	struct device_link *link;
-	int idx;
-
-	idx = device_links_read_lock();
-
-	list_for_each_entry_rcu(link, &dev->links.consumers, s_node,
-				device_links_read_lock_held()) {
-		if (!(link->flags & DL_FLAG_MANAGED))
-			continue;
-
-		while (refcount_dec_not_one(&link->rpm_active))
-			pm_runtime_put_noidle(dev);
-	}
-
-	device_links_read_unlock(idx);
-}
-
-/**
  * pm_runtime_get_suppliers - Resume and reference-count supplier devices.
  * @dev: Consumer device.
  */
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -54,7 +54,6 @@ extern u64 pm_runtime_autosuspend_expira
 extern void pm_runtime_update_max_time_suspended(struct device *dev,
 						 s64 delta_ns);
 extern void pm_runtime_set_memalloc_noio(struct device *dev, bool enable);
-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);
@@ -276,7 +275,6 @@ static inline u64 pm_runtime_autosuspend
 				struct device *dev) { return 0; }
 static inline void pm_runtime_set_memalloc_noio(struct device *dev,
 						bool enable){}
-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) {}

[PATCH 3/3] PM: runtime: Resume the device earlier in __device_release_driver()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since the device is resumed from runtime-suspend in
__device_release_driver() anyway, it is better to do that before
looking for busy managed device links from it to consumers, because
if there are any, device_links_unbind_consumers() will be called
and it will cause the consumer devices' drivers to unbind, so the
consumer devices will be runtime-resumed.  In turn, resuming each
consumer device will cause the supplier to be resumed and when the
runtime PM references from the given consumer to it are dropped, it
may be suspended.  Then, the runtime-resume of the next consumer
will cause the supplier to resume again and so on.

Update the code accordingly.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Fixes: 9ed9895370ae ("driver core: Functional dependencies tracking support")
Cc: All applicable <stable@vger.kernel.org> # All applicable
---
 drivers/base/dd.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -1117,6 +1117,8 @@ static void __device_release_driver(stru
 
 	drv = dev->driver;
 	if (drv) {
+		pm_runtime_get_sync(dev);
+
 		while (device_links_busy(dev)) {
 			__device_driver_unlock(dev, parent);
 
@@ -1132,8 +1134,6 @@ static void __device_release_driver(stru
 				return;
 		}
 
-		pm_runtime_get_sync(dev);
-
 		driver_sysfs_remove(dev);
 
 		if (dev->bus)

Re: [PATCH 3/3] PM: runtime: Resume the device earlier in __device_release_driver()

[chenxiang (M)]

Hi Rafael,

在 2020/10/22 3:14, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Since the device is resumed from runtime-suspend in
> __device_release_driver() anyway, it is better to do that before
> looking for busy managed device links from it to consumers, because
> if there are any, device_links_unbind_consumers() will be called
> and it will cause the consumer devices' drivers to unbind, so the
> consumer devices will be runtime-resumed.  In turn, resuming each
> consumer device will cause the supplier to be resumed and when the
> runtime PM references from the given consumer to it are dropped, it
> may be suspended.  Then, the runtime-resume of the next consumer
> will cause the supplier to resume again and so on.
>
> Update the code accordingly.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Fixes: 9ed9895370ae ("driver core: Functional dependencies tracking support")
> Cc: All applicable <stable@vger.kernel.org> # All applicable
> ---
>   drivers/base/dd.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/base/dd.c
> ===================================================================
> --- linux-pm.orig/drivers/base/dd.c
> +++ linux-pm/drivers/base/dd.c
> @@ -1117,6 +1117,8 @@ static void __device_release_driver(stru
>   
>   	drv = dev->driver;
>   	if (drv) {
> +		pm_runtime_get_sync(dev);
> +
>   		while (device_links_busy(dev)) {
>   			__device_driver_unlock(dev, parent);
>   
> @@ -1132,8 +1134,6 @@ static void __device_release_driver(stru

pm_runtime_put_sync() is required to be called if existed from here.

>   				return;
>   		}
>   
> -		pm_runtime_get_sync(dev);
> -
>   		driver_sysfs_remove(dev);
>   
>   		if (dev->bus)
>
>
>
>
> .
>

Re: [PATCH 3/3] PM: runtime: Resume the device earlier in __device_release_driver()

[Rafael J. Wysocki]

On Thu, Oct 22, 2020 at 3:40 PM chenxiang (M) <chenxiang66@hisilicon.com> wrote:
>
> Hi Rafael,
>
> 在 2020/10/22 3:14, Rafael J. Wysocki 写道:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Since the device is resumed from runtime-suspend in
> > __device_release_driver() anyway, it is better to do that before
> > looking for busy managed device links from it to consumers, because
> > if there are any, device_links_unbind_consumers() will be called
> > and it will cause the consumer devices' drivers to unbind, so the
> > consumer devices will be runtime-resumed.  In turn, resuming each
> > consumer device will cause the supplier to be resumed and when the
> > runtime PM references from the given consumer to it are dropped, it
> > may be suspended.  Then, the runtime-resume of the next consumer
> > will cause the supplier to resume again and so on.
> >
> > Update the code accordingly.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Fixes: 9ed9895370ae ("driver core: Functional dependencies tracking support")
> > Cc: All applicable <stable@vger.kernel.org> # All applicable
> > ---
> >   drivers/base/dd.c |    4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > Index: linux-pm/drivers/base/dd.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/dd.c
> > +++ linux-pm/drivers/base/dd.c
> > @@ -1117,6 +1117,8 @@ static void __device_release_driver(stru
> >
> >       drv = dev->driver;
> >       if (drv) {
> > +             pm_runtime_get_sync(dev);
> > +
> >               while (device_links_busy(dev)) {
> >                       __device_driver_unlock(dev, parent);
> >
> > @@ -1132,8 +1134,6 @@ static void __device_release_driver(stru
>
> pm_runtime_put_sync() is required to be called if existed from here.
>
> >                               return;

Right, I overlooked this, thanks!

> >               }
> >
> > -             pm_runtime_get_sync(dev);
> > -
> >               driver_sysfs_remove(dev);
> >
> >               if (dev->bus)

[PATCH update 3/3] PM: runtime: Resume the device earlier in __device_release_driver()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since the device is resumed from runtime-suspend in
__device_release_driver() anyway, it is better to do that before
looking for busy managed device links from it to consumers, because
if there are any, device_links_unbind_consumers() will be called
and it will cause the consumer devices' drivers to unbind, so the
consumer devices will be runtime-resumed.  In turn, resuming each
consumer device will cause the supplier to be resumed and when the
runtime PM references from the given consumer to it are dropped, it
may be suspended.  Then, the runtime-resume of the next consumer
will cause the supplier to resume again and so on.

Update the code accordingly.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Fixes: 9ed9895370ae ("driver core: Functional dependencies tracking support")
Cc: All applicable <stable@vger.kernel.org> # All applicable
---

This update adds a missing pm_runtime_put() before a return statement.

Thanks!

---
 drivers/base/dd.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -1117,6 +1117,8 @@ static void __device_release_driver(stru
 
 	drv = dev->driver;
 	if (drv) {
+		pm_runtime_get_sync(dev);
+
 		while (device_links_busy(dev)) {
 			__device_driver_unlock(dev, parent);
 
@@ -1128,12 +1130,12 @@ static void __device_release_driver(stru
 			 * have released the driver successfully while this one
 			 * was waiting, so check for that.
 			 */
-			if (dev->driver != drv)
+			if (dev->driver != drv) {
+				pm_runtime_put(dev);
 				return;
+			}
 		}
 
-		pm_runtime_get_sync(dev);
-
 		driver_sysfs_remove(dev);
 
 		if (dev->bus)

Re: [PATCH 0/3] PM: runtime: Fixes related to device links management

[Rafael J. Wysocki]

On Friday, October 23, 2020 5:50:04 AM CEST chenxiang (M) wrote:
> Hi Rafael,
> 
> 在 2020/10/22 3:10, Rafael J. Wysocki 写道:
> > Hi Greg & all,
> >
> > Commit d12544fb2aa9 ("PM: runtime: Remove link state checks in
> > rpm_get/put_supplier()") merged recently introduced a weakness
> > in the handling of device links in the runtime PM framework that
> > may be confusing and even harmful.
> >
> > Namely, the checks removed by that commit prevented PM-runtime from
> > getting or dropping references to the supplier device whose driver
> > was going away via its links to consumers, which specifically allowed
> > the pm_runtime_clean_up_links() called from __device_release_driver()
> > to run without interfering with runtime suspend/resume of consumer
> > devices (which still might happen even though the drivers had been
> > unbound from them by that time).
> >
> > After the above commit, calling pm_runtime_clean_up_links() from
> > __device_release_driver() makes a little sense and it may be interfering
> > destructively with regular PM-runtime suspend/resume control flows, so
> > it needs to be either fixed or dropped altogether.  I prefer the latter,
> > because among other things this removes an arbitrary difference in the
> > handling of managed device links with respect to the stateless ones,
> > so patch [2/3] is doing just that.
> >
> > However, in some rare cases pm_runtime_clean_up_links() may help to clean
> > up leftover PM-runtime references, so if that function goes away, they
> > need to be cleaned up elsewhere.  That's why patch [1/3] modifies
> > __device_link_del() to drop them upon device link removal (which also
> > needs to be done for stateless device links and that's why I'm regarding
> > this patch as a fix).
> >
> > Finally, to avoid pointless overhead related to suspending and resuming
> > the target device for multiple times in a row in __device_release_driver(),
> > it is better to resume it upfront before checking its links to consumers,
> > which is done by patch [3/3].
> 
> 
> I have tested the patchset, and it solves my reported issue, so please 
> feel free to add :
> Tested-by: Xiang Chen <chenxiang66@hisilicon.com>

Thank you!

Re: [PATCH 0/3] PM: runtime: Fixes related to device links management

[Rafael J. Wysocki]

Hi Greg,

On Wed, Oct 21, 2020 at 9:14 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Greg & all,
>
> Commit d12544fb2aa9 ("PM: runtime: Remove link state checks in
> rpm_get/put_supplier()") merged recently introduced a weakness
> in the handling of device links in the runtime PM framework that
> may be confusing and even harmful.
>
> Namely, the checks removed by that commit prevented PM-runtime from
> getting or dropping references to the supplier device whose driver
> was going away via its links to consumers, which specifically allowed
> the pm_runtime_clean_up_links() called from __device_release_driver()
> to run without interfering with runtime suspend/resume of consumer
> devices (which still might happen even though the drivers had been
> unbound from them by that time).
>
> After the above commit, calling pm_runtime_clean_up_links() from
> __device_release_driver() makes a little sense and it may be interfering
> destructively with regular PM-runtime suspend/resume control flows, so
> it needs to be either fixed or dropped altogether.  I prefer the latter,
> because among other things this removes an arbitrary difference in the
> handling of managed device links with respect to the stateless ones,
> so patch [2/3] is doing just that.
>
> However, in some rare cases pm_runtime_clean_up_links() may help to clean
> up leftover PM-runtime references, so if that function goes away, they
> need to be cleaned up elsewhere.  That's why patch [1/3] modifies
> __device_link_del() to drop them upon device link removal (which also
> needs to be done for stateless device links and that's why I'm regarding
> this patch as a fix).
>
> Finally, to avoid pointless overhead related to suspending and resuming
> the target device for multiple times in a row in __device_release_driver(),
> it is better to resume it upfront before checking its links to consumers,
> which is done by patch [3/3].
>
> While this series touches the driver core, it really is mostly related to
> runtime PM, so I can apply it if that's OK.

Any concerns regarding this series?

If not, I'd like to queue it up for -rc3, because the current behavior
in there is quite confusing (or worse).

Cheers!

Re: [PATCH 0/3] PM: runtime: Fixes related to device links management

[Greg Kroah-Hartman]

On Wed, Oct 21, 2020 at 09:10:08PM +0200, Rafael J. Wysocki wrote:
> Hi Greg & all,
> 
> Commit d12544fb2aa9 ("PM: runtime: Remove link state checks in
> rpm_get/put_supplier()") merged recently introduced a weakness
> in the handling of device links in the runtime PM framework that
> may be confusing and even harmful.
> 
> Namely, the checks removed by that commit prevented PM-runtime from
> getting or dropping references to the supplier device whose driver
> was going away via its links to consumers, which specifically allowed
> the pm_runtime_clean_up_links() called from __device_release_driver()
> to run without interfering with runtime suspend/resume of consumer
> devices (which still might happen even though the drivers had been
> unbound from them by that time).
> 
> After the above commit, calling pm_runtime_clean_up_links() from
> __device_release_driver() makes a little sense and it may be interfering
> destructively with regular PM-runtime suspend/resume control flows, so
> it needs to be either fixed or dropped altogether.  I prefer the latter,
> because among other things this removes an arbitrary difference in the
> handling of managed device links with respect to the stateless ones,
> so patch [2/3] is doing just that.
> 
> However, in some rare cases pm_runtime_clean_up_links() may help to clean
> up leftover PM-runtime references, so if that function goes away, they
> need to be cleaned up elsewhere.  That's why patch [1/3] modifies
> __device_link_del() to drop them upon device link removal (which also
> needs to be done for stateless device links and that's why I'm regarding
> this patch as a fix).
> 
> Finally, to avoid pointless overhead related to suspending and resuming
> the target device for multiple times in a row in __device_release_driver(),
> it is better to resume it upfront before checking its links to consumers,
> which is done by patch [3/3].
> 
> While this series touches the driver core, it really is mostly related to
> runtime PM, so I can apply it if that's OK.

Please do, sorry for the delay in reviewing them:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>