Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1493065 > unrolled thread

[PATCH v4 0/5] Functional dependencies between devices

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 0/5] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-29 02:40 +0200
    [Resend][PATCH v4 3/5] PM / sleep: Make async suspend/resume of devices use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-29 02:40 +0200
    [PATCH v4 4/5] PM / runtime: Use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-29 02:40 +0200
    [Rebase][PATCH v4 5/5] PM / runtime: Optimize the use of device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-29 02:40 +0200
    [PATCH v4 2/5] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-29 02:40 +0200
      Re: [PATCH v4 2/5] driver core: Functional dependencies tracking  support Lukas Wunner <lukas@wunner.de> - 2016-10-01 09:50 +0200
        Re: [PATCH v4 2/5] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-10-02 01:40 +0200
    Re: [PATCH v4 0/5] Functional dependencies between devices Marek Szyprowski <m.szyprowski@samsung.com> - 2016-09-29 09:00 +0200
      Re: [PATCH v4 0/5] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-29 14:30 +0200
    Re: [PATCH v4 0/5] Functional dependencies between devices Lukas Wunner <lukas@wunner.de> - 2016-10-03 01:20 +0200

#1493065 — [PATCH v4 0/5] Functional dependencies between devices

Hi Everyone,

On Thursday, September 08, 2016 11:25:44 PM Rafael J. Wysocki wrote:
> > Hi Everyone,
> > 
> > This is a refresh of the functional dependencies series that I posted last
> > year and which has picked up by Marek quite recently.  For reference,
> > appended is my introductory message sent previously (which may be
> > slightly outdated now).
> > 
> > As last time, the first patch rearranges the code around
> > __device_release_driver() a bit to prepare it for the next one (it
> > actually hasn't changed AFAICS).
> > 
> > The second patch introduces the actual device links mechanics, but without
> > system suspend/resume and runtime PM support which are added by the
> > subsequent patches.
> > 
> > Some bugs found by Marek during his work on these patches should be fixed
> > here.  In particular, the endless recursion in device_reorder_to_tail()
> > which simply was broken before.
> > 
> > There are two additional patches to address the issue with runtime PM
> > support
> > that occured when runtime PM was disabled for some suppliers due to a PM
> > sleep transition in progress.  Those patches simply make runtime PM
> > helpers
> > return 0 in that case which may be controversial, so please let me know if
> > there are concerns about those.
> > 
> > The way device_link_add() works is a bit different, as it takes an
> > additional
> > status argument now.  That makes it possible to create a link in any
> > state,
> > with extra care of course, and should address the problem pointed to by
> > Lukas
> > during the previous discussion.
> > 
> > Also some comments from Tomeu have been addressed.
> 
> An update here.
> 
> The first patch hasn't changed, so I'm resending it.
> 
> The majority of changes in the other patches are in order to address Lukas'
> comments.
> 
> First off, I added a DEVICE_LINK_STATELESS flag that will prevent the driver
> core from trying to maintain device links having it set.
> 
> Also, the DEVICE_LINK_PERSISTENT flag was dropped (as link "persistence" is
> the default behavior now) and there's a new one, DEVICE_LINK_AUTOREMOVE,
> that will cause the driver core to remove the link on the consumer driver
> unbind.
> 
> Moreover, the code checks attempts to create a link between a parent and a
> child device now and actively prevents that from happening.
> 
> The changelog of the second patch has been updated as requested by Ulf.
> 
> The third patch was updated to fix a bug related to the (previously missing)
> clearing of power.direct_complete for supplier devices having consumers that
> don't use direct_complete.
> 
> The next two (runtime PM) patches turned out to be unnecessary, so I've
> dropped them.
> 
> The runtime PM patch [4/5] was reorganized somewhat to reduce the
> indentation
> level in there, but the code flow introduced by it is essentially the same
> and the last patch was simply rebased on top of the new series.

Time for another update. :-)

Fewer changes this time, mostly to address issues found by Lukas and Marek.

The most significant one is to make device_link_add() cope with the case when
the consumer device has not been registered yet when it is called.  The
supplier device still is required to be registered and the function will return
NULL if that is not the case.

Another significant change is in patch [4/5] that now makes the core apply
pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the probing
of a consumer one (in analogy with the parent).

Thanks,
Rafael

[toc] | [next] | [standalone]

#1493066 — [Resend][PATCH v4 3/5] PM / sleep: Make async suspend/resume of devices use device links

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

Make the device suspend/resume part of the core system
suspend/resume code use device links to ensure that supplier
and consumer devices will be suspended and resumed in the right
order in case of async suspend/resume.

The idea, roughly, is to use dpm_wait() to wait for all consumers
before a supplier device suspend and to wait for all suppliers
before a consumer device resume.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/base.h       |    2 +
 drivers/base/core.c       |    4 +-
 drivers/base/power/main.c |   85 ++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 83 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/base/base.h
===================================================================
--- linux-pm.orig/drivers/base/base.h
+++ linux-pm/drivers/base/base.h
@@ -163,3 +163,5 @@ extern void device_links_driver_cleanup(
 extern void device_links_no_driver(struct device *dev);
 extern bool device_links_busy(struct device *dev);
 extern void device_links_unbind_consumers(struct device *dev);
+extern int device_links_read_lock(void);
+extern void device_links_read_unlock(int idx);
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -227,12 +227,12 @@ void device_link_del(struct device_link
 }
 EXPORT_SYMBOL_GPL(device_link_del);
 
-static int device_links_read_lock(void)
+int device_links_read_lock(void)
 {
 	return srcu_read_lock(&device_links_srcu);
 }
 
-static void device_links_read_unlock(int idx)
+void device_links_read_unlock(int idx)
 {
 	return srcu_read_unlock(&device_links_srcu, idx);
 }
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -246,6 +246,62 @@ static void dpm_wait_for_children(struct
        device_for_each_child(dev, &async, dpm_wait_fn);
 }
 
+static void dpm_wait_for_suppliers(struct device *dev, bool async)
+{
+	struct device_link *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	/*
+	 * If the supplier goes away right after we've checked the link to it,
+	 * we'll wait for its completion to change the state, but that's fine,
+	 * because the only things that will block as a result are the SRCU
+	 * callbacks freeing the link objects for the links in the list we're
+	 * walking.
+	 */
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node)
+		if (link->status != DEVICE_LINK_DORMANT)
+			dpm_wait(link->supplier, async);
+
+	device_links_read_unlock(idx);
+}
+
+static void dpm_wait_for_superior(struct device *dev, bool async)
+{
+	dpm_wait(dev->parent, async);
+	dpm_wait_for_suppliers(dev, async);
+}
+
+static void dpm_wait_for_consumers(struct device *dev, bool async)
+{
+	struct device_link *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	/*
+	 * The status of a device link can only be changed from "dormant" by a
+	 * probe, but that cannot happen during system suspend/resume.  In
+	 * theory it can change to "dormant" at that time, but then it is
+	 * reasonable to wait for the target device anyway (eg. if it goes
+	 * away, it's better to wait for it to go away completely and then
+	 * continue instead of trying to continue in parallel with its
+	 * unregistration).
+	 */
+	list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node)
+		if (link->status != DEVICE_LINK_DORMANT)
+			dpm_wait(link->consumer, async);
+
+	device_links_read_unlock(idx);
+}
+
+static void dpm_wait_for_subordinate(struct device *dev, bool async)
+{
+	dpm_wait_for_children(dev, async);
+	dpm_wait_for_consumers(dev, async);
+}
+
 /**
  * pm_op - Return the PM operation appropriate for given PM event.
  * @ops: PM operations to choose from.
@@ -490,7 +546,7 @@ static int device_resume_noirq(struct de
 	if (!dev->power.is_noirq_suspended)
 		goto Out;
 
-	dpm_wait(dev->parent, async);
+	dpm_wait_for_superior(dev, async);
 
 	if (dev->pm_domain) {
 		info = "noirq power domain ";
@@ -620,7 +676,7 @@ static int device_resume_early(struct de
 	if (!dev->power.is_late_suspended)
 		goto Out;
 
-	dpm_wait(dev->parent, async);
+	dpm_wait_for_superior(dev, async);
 
 	if (dev->pm_domain) {
 		info = "early power domain ";
@@ -752,7 +808,7 @@ static int device_resume(struct device *
 		goto Complete;
 	}
 
-	dpm_wait(dev->parent, async);
+	dpm_wait_for_superior(dev, async);
 	dpm_watchdog_set(&wd, dev);
 	device_lock(dev);
 
@@ -1040,7 +1096,7 @@ static int __device_suspend_noirq(struct
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
-	dpm_wait_for_children(dev, async);
+	dpm_wait_for_subordinate(dev, async);
 
 	if (dev->pm_domain) {
 		info = "noirq power domain ";
@@ -1187,7 +1243,7 @@ static int __device_suspend_late(struct
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
-	dpm_wait_for_children(dev, async);
+	dpm_wait_for_subordinate(dev, async);
 
 	if (dev->pm_domain) {
 		info = "late power domain ";
@@ -1344,6 +1400,22 @@ static int legacy_suspend(struct device
 	return error;
 }
 
+static void dpm_clear_suppliers_direct_complete(struct device *dev)
+{
+	struct device_link *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node) {
+		spin_lock_irq(&link->supplier->power.lock);
+		link->supplier->power.direct_complete = false;
+		spin_unlock_irq(&link->supplier->power.lock);
+	}
+
+	device_links_read_unlock(idx);
+}
+
 /**
  * device_suspend - Execute "suspend" callbacks for given device.
  * @dev: Device to handle.
@@ -1360,7 +1432,7 @@ static int __device_suspend(struct devic
 	TRACE_DEVICE(dev);
 	TRACE_SUSPEND(0);
 
-	dpm_wait_for_children(dev, async);
+	dpm_wait_for_subordinate(dev, async);
 
 	if (async_error)
 		goto Complete;
@@ -1456,6 +1528,7 @@ static int __device_suspend(struct devic
 
 			spin_unlock_irq(&parent->power.lock);
 		}
+		dpm_clear_suppliers_direct_complete(dev);
 	}
 
 	device_unlock(dev);

[toc] | [prev] | [next] | [standalone]

#1493068 — [PATCH v4 4/5] PM / runtime: Use device links

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

Modify the runtime PM framework to use device links to ensure that
supplier devices will not be suspended if any of their consumer
devices are active.

The idea is to reference count suppliers on the consumer's resume
and drop references to them on its suspend.  The information on
whether or not the supplier has been reference counted by the
consumer's (runtime) resume is stored in a new field (rpm_active)
in the link object for each link.

It may be necessary to clean up those references when the
supplier is unbinding and that's why the links whose status is
DEVICE_LINK_SUPPLIER_UNBIND are skipped by the runtime suspend
and resume code.

The above means that if the consumer device is probed in the
runtime-active state, the supplier has to be resumed and reference
counted by device_link_add() so the code works as expected on its
(runtime) suspend.  There is a new flag, DEVICE_LINK_RPM_ACTIVE,
to tell device_link_add() about that (in which case the caller
is responsible for making sure that the consumer really will
be runtime-active when runtime PM is enabled for it).

The other new link flag, DEVICE_LINK_PM_RUNTIME, tells the core
whether or not the link should be used for runtime PM at all.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v3 -> v4:

- Add pm_runtime_get/put_suppliers() and make the core call them
  around the probing of a consumer device (in analogy with what it
  does to the device's parent).

---
 drivers/base/core.c          |   23 +++++-
 drivers/base/dd.c            |    3 
 drivers/base/power/runtime.c |  157 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/device.h       |    5 +
 include/linux/pm_runtime.h   |    6 +
 5 files changed, 186 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -109,8 +109,16 @@ static int device_reorder_to_tail(struct
  * @flags: Link flags.
  *
  * If the DEVICE_LINK_STATELESS flag is set, @status is ignored.  Otherwise,
- * the caller is responsible for ensuring that @status reflects the current
- * status of both @consumer and @supplier.
+ * the caller is responsible for ensuring that (a) @status reflects the current
+ * status of both @consumer and @supplier and (b) the creation of the link is
+ * properly synchronized with runtime PM.
+ *
+ * To that end, setting the DEVICE_LINK_PM_RUNTIME flag will cause the runtime
+ * PM framework to take the link into account.  Moreover, if the
+ * DEVICE_LINK_RPM_ACTIVE flag is set in addition to it, the supplier devices
+ * will be forced into the active metastate and reference-counted upon the
+ * creation of the link.  If DEVICE_LINK_PM_RUNTIME is not set,
+ * DEVICE_LINK_RPM_ACTIVE will be ignored.
  *
  * If the DEVICE_LINK_AUTOREMOVE is set, the link will be removed automatically
  * when the consumer device driver unbinds from it.  The combination of both
@@ -154,10 +162,19 @@ struct device_link *device_link_add(stru
 		if (link->consumer == consumer)
 			goto out;
 
-	link = kmalloc(sizeof(*link), GFP_KERNEL);
+	link = kzalloc(sizeof(*link), GFP_KERNEL);
 	if (!link)
 		goto out;
 
+	if ((flags & DEVICE_LINK_PM_RUNTIME) && (flags & DEVICE_LINK_RPM_ACTIVE)) {
+		if (pm_runtime_get_sync(supplier) < 0) {
+			pm_runtime_put_noidle(supplier);
+			kfree(link);
+			link = NULL;
+			goto out;
+		}
+		link->rpm_active = true;
+	}
 	get_device(supplier);
 	link->supplier = supplier;
 	INIT_LIST_HEAD(&link->s_node);
Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -498,6 +498,7 @@ int driver_probe_device(struct device_dr
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
+	pm_runtime_get_suppliers(dev);
 	if (dev->parent)
 		pm_runtime_get_sync(dev->parent);
 
@@ -508,6 +509,7 @@ int driver_probe_device(struct device_dr
 	if (dev->parent)
 		pm_runtime_put(dev->parent);
 
+	pm_runtime_put_suppliers(dev);
 	return ret;
 }
 
@@ -791,6 +793,7 @@ 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
@@ -12,6 +12,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/pm_wakeirq.h>
 #include <trace/events/rpm.h>
+
+#include "../base.h"
 #include "power.h"
 
 typedef int (*pm_callback_t)(struct device *);
@@ -258,6 +260,42 @@ static int rpm_check_suspend_allowed(str
 	return retval;
 }
 
+static int rpm_get_suppliers(struct device *dev)
+{
+	struct device_link *link;
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node) {
+		int retval;
+
+		if (!(link->flags & DEVICE_LINK_PM_RUNTIME))
+			continue;
+
+		if (link->status == DEVICE_LINK_SUPPLIER_UNBIND ||
+		    link->rpm_active)
+			continue;
+
+		retval = pm_runtime_get_sync(link->supplier);
+		if (retval < 0) {
+			pm_runtime_put_noidle(link->supplier);
+			return retval;
+		}
+		link->rpm_active = true;
+	}
+	return 0;
+}
+
+static void rpm_put_suppliers(struct device *dev)
+{
+	struct device_link *link;
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node)
+		if (link->rpm_active &&
+		    link->status != DEVICE_LINK_SUPPLIER_UNBIND) {
+			pm_runtime_put(link->supplier);
+			link->rpm_active = false;
+		}
+}
+
 /**
  * __rpm_callback - Run a given runtime PM callback for a given device.
  * @cb: Runtime PM callback to run.
@@ -266,19 +304,55 @@ static int rpm_check_suspend_allowed(str
 static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 	__releases(&dev->power.lock) __acquires(&dev->power.lock)
 {
-	int retval;
+	int retval, idx;
 
-	if (dev->power.irq_safe)
+	if (dev->power.irq_safe) {
 		spin_unlock(&dev->power.lock);
-	else
+	} else {
 		spin_unlock_irq(&dev->power.lock);
 
+		/*
+		 * 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 (dev->power.runtime_status == RPM_RESUMING) {
+			idx = device_links_read_lock();
+
+			retval = rpm_get_suppliers(dev);
+			if (retval)
+				goto fail;
+
+			device_links_read_unlock(idx);
+		}
+	}
+
 	retval = cb(dev);
 
-	if (dev->power.irq_safe)
+	if (dev->power.irq_safe) {
 		spin_lock(&dev->power.lock);
-	else
+	} 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 ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
+		    || (dev->power.runtime_status == RPM_RESUMING && retval)) {
+			idx = device_links_read_lock();
+
+ fail:
+			rpm_put_suppliers(dev);
+
+			device_links_read_unlock(idx);
+		}
+
 		spin_lock_irq(&dev->power.lock);
+	}
 
 	return retval;
 }
@@ -1447,6 +1521,79 @@ 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
+ * (once per link).
+ *
+ * Links with the DEVICE_LINK_STATELESS flag set 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 DEVICE_LINK_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_to_consumers, s_node) {
+		if (link->flags & DEVICE_LINK_STATELESS)
+			continue;
+
+		if (link->rpm_active) {
+			pm_runtime_put_noidle(dev);
+			link->rpm_active = false;
+		}
+	}
+
+	device_links_read_unlock(idx);
+}
+
+/**
+ * pm_runtime_get_suppliers - Resume and reference-count supplier devices.
+ * @dev: Consumer device.
+ */
+void pm_runtime_get_suppliers(struct device *dev)
+{
+	struct device_link *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node)
+		if (link->flags & DEVICE_LINK_PM_RUNTIME)
+			pm_runtime_get_sync(link->supplier);
+
+	device_links_read_unlock(idx);
+}
+
+/**
+ * pm_runtime_put_suppliers - Drop references to supplier devices.
+ * @dev: Consumer device.
+ */
+void pm_runtime_put_suppliers(struct device *dev)
+{
+	struct device_link *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node)
+		if (link->flags & DEVICE_LINK_PM_RUNTIME)
+			pm_runtime_put(link->supplier);
+
+	device_links_read_unlock(idx);
+}
+
+/**
  * pm_runtime_force_suspend - Force a device into suspend state if needed.
  * @dev: Device to suspend.
  *
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -720,9 +720,13 @@ enum device_link_status {
  *
  * STATELESS: The core won't track the presence of supplier/consumer drivers.
  * AUTOREMOVE: Remove this link automatically on cunsumer driver unbind.
+ * PM_RUNTIME: If set, the runtime PM framework will use this link.
+ * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
  */
 #define DEVICE_LINK_STATELESS	(1 << 0)
 #define DEVICE_LINK_AUTOREMOVE	(1 << 1)
+#define DEVICE_LINK_PM_RUNTIME	(1 << 2)
+#define DEVICE_LINK_RPM_ACTIVE	(1 << 3)
 
 struct device_link {
 	struct device *supplier;
@@ -731,6 +735,7 @@ struct device_link {
 	struct list_head c_node;
 	enum device_link_status status;
 	u32 flags;
+	bool rpm_active;
 	spinlock_t lock;
 	struct rcu_head rcu_head;
 };
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -55,6 +55,9 @@ extern unsigned long pm_runtime_autosusp
 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);
 
 static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
 {
@@ -186,6 +189,9 @@ static inline unsigned long pm_runtime_a
 				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) {}
 
 #endif /* !CONFIG_PM */
 

[toc] | [prev] | [next] | [standalone]

#1493069 — [Rebase][PATCH v4 5/5] PM / runtime: Optimize the use of device links

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

If the device has no links to suppliers that should be used for
runtime PM (links with DEVICE_LINK_PM_RUNTIME set), there is no
reason to walk the list of suppliers for that device during
runtime suspend and resume.

Add a simple mechanism to detect that case and possibly avoid the
extra unnecessary overhead.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/core.c          |   20 +++++++++++++-------
 drivers/base/power/runtime.c |   23 ++++++++++++++++++++---
 include/linux/pm.h           |    1 +
 include/linux/pm_runtime.h   |    4 ++++
 4 files changed, 38 insertions(+), 10 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -166,14 +166,17 @@ struct device_link *device_link_add(stru
 	if (!link)
 		goto out;
 
-	if ((flags & DEVICE_LINK_PM_RUNTIME) && (flags & DEVICE_LINK_RPM_ACTIVE)) {
-		if (pm_runtime_get_sync(supplier) < 0) {
-			pm_runtime_put_noidle(supplier);
-			kfree(link);
-			link = NULL;
-			goto out;
+	if (flags & DEVICE_LINK_PM_RUNTIME) {
+		if (flags & DEVICE_LINK_RPM_ACTIVE) {
+			if (pm_runtime_get_sync(supplier) < 0) {
+				pm_runtime_put_noidle(supplier);
+				kfree(link);
+				link = NULL;
+				goto out;
+			}
+			link->rpm_active = true;
 		}
-		link->rpm_active = true;
+		pm_runtime_new_link(consumer);
 	}
 	get_device(supplier);
 	link->supplier = supplier;
@@ -222,6 +225,9 @@ static void __device_link_del(struct dev
 	dev_info(link->consumer, "Dropping the link to %s\n",
 		 dev_name(link->supplier));
 
+	if (link->flags & DEVICE_LINK_PM_RUNTIME)
+		pm_runtime_drop_link(link->consumer);
+
 	list_del_rcu(&link->s_node);
 	list_del_rcu(&link->c_node);
 	call_srcu(&device_links_srcu, &link->rcu_head, __device_link_free_srcu);
Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -305,6 +305,7 @@ static int __rpm_callback(int (*cb)(stru
 	__releases(&dev->power.lock) __acquires(&dev->power.lock)
 {
 	int retval, idx;
+	bool use_links = dev->power.links_count > 0;
 
 	if (dev->power.irq_safe) {
 		spin_unlock(&dev->power.lock);
@@ -318,7 +319,7 @@ static int __rpm_callback(int (*cb)(stru
 		 * routine returns, so it is safe to read the status outside of
 		 * the lock.
 		 */
-		if (dev->power.runtime_status == RPM_RESUMING) {
+		if (use_links && dev->power.runtime_status == RPM_RESUMING) {
 			idx = device_links_read_lock();
 
 			retval = rpm_get_suppliers(dev);
@@ -341,8 +342,9 @@ static int __rpm_callback(int (*cb)(stru
 		 *
 		 * Do that if resume fails too.
 		 */
-		if ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
-		    || (dev->power.runtime_status == RPM_RESUMING && retval)) {
+		if (use_links
+		    && ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
+		    || (dev->power.runtime_status == RPM_RESUMING && retval))) {
 			idx = device_links_read_lock();
 
  fail:
@@ -1593,6 +1595,21 @@ void pm_runtime_put_suppliers(struct dev
 	device_links_read_unlock(idx);
 }
 
+void pm_runtime_new_link(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.links_count++;
+	spin_unlock_irq(&dev->power.lock);
+}
+
+void pm_runtime_drop_link(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	WARN_ON(dev->power.links_count == 0);
+	dev->power.links_count--;
+	spin_unlock_irq(&dev->power.lock);
+}
+
 /**
  * pm_runtime_force_suspend - Force a device into suspend state if needed.
  * @dev: Device to suspend.
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -597,6 +597,7 @@ struct dev_pm_info {
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
 	unsigned int		memalloc_noio:1;
+	unsigned int		links_count;
 	enum rpm_request	request;
 	enum rpm_status		runtime_status;
 	int			runtime_error;
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -58,6 +58,8 @@ extern void pm_runtime_set_memalloc_noio
 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_drop_link(struct device *dev);
 
 static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
 {
@@ -192,6 +194,8 @@ static inline void pm_runtime_set_memall
 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_drop_link(struct device *dev) {}
 
 #endif /* !CONFIG_PM */
 

[toc] | [prev] | [next] | [standalone]

#1493070 — [PATCH v4 2/5] driver core: Functional dependencies tracking support

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

Currently, there is a problem with taking functional dependencies
between devices into account.

What I mean by a "functional dependency" is when the driver of device
B needs device A to be functional and (generally) its driver to be
present in order to work properly.  This has certain consequences
for power management (suspend/resume and runtime PM ordering) and
shutdown ordering of these devices.  In general, it also implies that
the driver of A needs to be working for B to be probed successfully
and it cannot be unbound from the device before the B's driver.

Support for representing those functional dependencies between
devices is added here to allow the driver core to track them and act
on them in certain cases where applicable.

The argument for doing that in the driver core is that there are
quite a few distinct use cases involving device dependencies, they
are relatively hard to get right in a driver (if one wants to
address all of them properly) and it only gets worse if multiplied
by the number of drivers potentially needing to do it.  Morever, at
least one case (asynchronous system suspend/resume) cannot be handled
in a single driver at all, because it requires the driver of A to
wait for B to suspend (during system suspend) and the driver of B to
wait for A to resume (during system resume).

For this reason, represent dependencies between devices as "links",
with the help of struct device_link objects each containing pointers
to the "linked" devices, a list node for each of them, status
information, flags, a lock and an RCU head for synchronization.

Also add two new list heads, links_to_consumers and links_to_suppliers,
to struct device to represent the lists of links to the devices that
depend on the given one (consumers) and to the devices depended on
by it (suppliers), respectively.

The entire data structure consisting of all of the lists of link
objects for all devices is protected by SRCU (for list walking)
and by a mutex (for link object addition/removal).  In addition
to that, each link object has an internal status field whose
value reflects whether or not drivers are bound to the devices
pointed to by the link or probing/removal of their drivers is in
progress etc.  That status field is protected by an internal
spinlock.

New links are added by calling device_link_add() which takes four
arguments: pointers to the devices in question, the initial status
of the link and flags.  In particular, if DEVICE_LINK_STATELESS is
set in the flags, the link status is not to be taken into account
for this link and the driver core will not manage it.  In turn, if
DEVICE_LINK_AUTOREMOVE is set in the flags, the driver core will
remove the link automatically when the consumer device driver
unbinds from it.

One of the actions carried out by device_link_add() is to reorder
the lists used for device shutdown and system suspend/resume to
put the consumer device along with all of its children and all of
its consumers (and so on, recursively) to the ends of those lists
in order to ensure the right ordering between all of the supplier
and consumer devices.

For this reason, it is not possible to create a link between two
devices if the would-be supplier device already depends on the
would-be consumer device as either a direct descendant of it or a
consumer of one of its direct descendants or one of its consumers
and so on.

It also is impossible to create a link between a parent and a child
device (in any direction).

There are two types of link objects, persistent and non-persistent.
The persistent ones stay around until one of the target devices is
deleted, while the non-persistent ones are removed automatically when
the consumer driver unbinds from its device (ie. they are assumed to
be valid only as long as the consumer device has a driver bound to
it).  Persistent links are created by default and non-persistent
links are created when the DEVICE_LINK_AUTOREMOVE flag is passed
to device_link_add().

Both persistent and non-persistent device links can be deleted
explicitly with the help of device_link_del().

Links created without the DEVICE_LINK_STATELESS flag set are managed
by the driver core using a simple state machine.  There are 5 states
each link can be in: DORMANT (unused), AVAILABLE (the supplier driver
is present and functional), CONSUMER_PROBE (the consumer driver is
probing), ACTIVE (both supplier and consumer drivers are present and
functional), and SUPPLIER_UNBIND (the supplier driver is unbinding).
The driver core updates the link state automatically depending on
what happens to the linked devices and for each link state specific
actions are taken in addition to that.

For example, if the supplier driver unbinds from its device, the
driver core will also unbind the drivers of all of its consumers
automatically under the assumption that they cannot function
properly without the supplier.  Analogously, the driver core will
only allow the consumer driver to bind to its device if the
supplier driver is present and functional (ie. the link is in
the AVAILABLE state).  If that's not the case, it will rely on
the existing deferred probing mechanism to wait for the supplier
driver to become available.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v3 -> v4:

- Add the in_dpm_list field to struct dev_pm_info and use it for checking
  if the device has been added to dpm_list already, which is needed for
  handling the "not registered consumer" case in device_link_add().

- Rework device_link_add() to handle consumer devices that have not been
  registered before calling it.

- Drop the parent check from device_link_add().

- Rename device_links_driver_gone() to device_links_driver_cleanup()

- Rearrange device_links_unbind_consumers() to avoid one spin_unlock()
  instance (which wasn't necessary).

- Introduce device_links_purge() to delete links from a device going away
  and make device_del() call it (instead of running links-related code
  directly).

- Move the invocation of device_links_ckeck_suppliers() to really_probe().

- Change the description of the DEVICE_LINK_STATELESS flag.

---
 drivers/base/base.h        |   11 +
 drivers/base/core.c        |  491 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/base/dd.c          |   42 +++
 drivers/base/power/main.c  |    2 
 drivers/base/power/power.h |   10 
 include/linux/device.h     |   38 +++
 include/linux/pm.h         |    1 
 7 files changed, 590 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/base/base.h
===================================================================
--- linux-pm.orig/drivers/base/base.h
+++ linux-pm/drivers/base/base.h
@@ -107,6 +107,9 @@ extern void bus_remove_device(struct dev
 
 extern int bus_add_driver(struct device_driver *drv);
 extern void bus_remove_driver(struct device_driver *drv);
+extern void device_release_driver_internal(struct device *dev,
+					   struct device_driver *drv,
+					   struct device *parent);
 
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
@@ -152,3 +155,11 @@ extern int devtmpfs_init(void);
 #else
 static inline int devtmpfs_init(void) { return 0; }
 #endif
+
+/* Device links support */
+extern int device_links_check_suppliers(struct device *dev);
+extern void device_links_driver_bound(struct device *dev);
+extern void device_links_driver_cleanup(struct device *dev);
+extern void device_links_no_driver(struct device *dev);
+extern bool device_links_busy(struct device *dev);
+extern void device_links_unbind_consumers(struct device *dev);
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -44,6 +44,493 @@ static int __init sysfs_deprecated_setup
 early_param("sysfs.deprecated", sysfs_deprecated_setup);
 #endif
 
+/* Device links support. */
+
+DEFINE_STATIC_SRCU(device_links_srcu);
+static DEFINE_MUTEX(device_links_lock);
+
+/**
+ * device_is_dependent - Check if one device depends on another one
+ * @dev: Device to check dependencies for.
+ * @target: Device to check against.
+ *
+ * Check if @target depends on @dev or any device dependent on it (its child or
+ * its consumer etc).  Return 1 if that is the case or 0 otherwise.
+ */
+static int device_is_dependent(struct device *dev, void *target)
+{
+	struct device_link *link;
+	int ret;
+
+	if (WARN_ON(dev == target))
+		return 1;
+
+	ret = device_for_each_child(dev, target, device_is_dependent);
+	if (ret)
+		return ret;
+
+	list_for_each_entry(link, &dev->links_to_consumers, s_node) {
+		if (WARN_ON(link->consumer == target))
+			return 1;
+
+		ret = device_is_dependent(link->consumer, target);
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
+static int device_reorder_to_tail(struct device *dev, void *not_used)
+{
+	struct device_link *link;
+
+	/*
+	 * Devices that have not been registered yet will be put to the ends
+	 * of the lists during the registratio, so skip them here.
+	 */
+	if (device_is_registered(dev))
+		devices_kset_move_last(dev);
+
+	if (device_pm_initialized(dev))
+		device_pm_move_last(dev);
+
+	device_for_each_child(dev, NULL, device_reorder_to_tail);
+	list_for_each_entry(link, &dev->links_to_consumers, s_node)
+		device_reorder_to_tail(link->consumer, NULL);
+
+	return 0;
+}
+
+/**
+ * device_link_add - Create a link between two devices.
+ * @consumer: Consumer end of the link.
+ * @supplier: Supplier end of the link.
+ * @status: The initial status of the link.
+ * @flags: Link flags.
+ *
+ * If the DEVICE_LINK_STATELESS flag is set, @status is ignored.  Otherwise,
+ * the caller is responsible for ensuring that @status reflects the current
+ * status of both @consumer and @supplier.
+ *
+ * If the DEVICE_LINK_AUTOREMOVE is set, the link will be removed automatically
+ * when the consumer device driver unbinds from it.  The combination of both
+ * DEVICE_LINK_AUTOREMOVE and DEVICE_LINK_STATELESS set is invalid and will
+ * cause NULL to be returned.
+ *
+ * A side effect of the link creation is re-ordering of dpm_list and the
+ * devices_kset list by moving the consumer device and all devices depending
+ * on it to the ends of these lists (that does not happen to devices that have
+ * not been registered when this function is called).
+ *
+ * The supplier device is required to be registered when this function is called
+ * and NULL will be returned if that is not the case.  The consumer device need
+ * not be registerd, however.
+ */
+struct device_link *device_link_add(struct device *consumer,
+				    struct device *supplier,
+				    enum device_link_status status, u32 flags)
+{
+	struct device_link *link;
+
+	if (!consumer || !supplier ||
+	    ((flags & DEVICE_LINK_STATELESS) && (flags & DEVICE_LINK_AUTOREMOVE)))
+		return NULL;
+
+	mutex_lock(&device_links_lock);
+	device_pm_lock();
+
+	/*
+	 * If the supplier has not been fully registered yet or there is a
+	 * reverse dependency between the consumer and the supplier already in
+	 * the graph, return NULL.
+	 */
+	if (!device_pm_initialized(supplier)
+	    || device_is_dependent(consumer, supplier)) {
+		link = NULL;
+		goto out;
+	}
+
+	list_for_each_entry(link, &supplier->links_to_consumers, s_node)
+		if (link->consumer == consumer)
+			goto out;
+
+	link = kmalloc(sizeof(*link), GFP_KERNEL);
+	if (!link)
+		goto out;
+
+	get_device(supplier);
+	link->supplier = supplier;
+	INIT_LIST_HEAD(&link->s_node);
+	get_device(consumer);
+	link->consumer = consumer;
+	INIT_LIST_HEAD(&link->c_node);
+	spin_lock_init(&link->lock);
+	link->flags = flags;
+	link->status = (flags & DEVICE_LINK_STATELESS) ?
+					DEVICE_LINK_NO_STATE : status;
+
+	/*
+	 * Move the consumer and all of the devices depending on it to the end
+	 * of dpm_list and the devices_kset list.
+	 *
+	 * It is necessary to hold dpm_list locked throughout all that or else
+	 * we may end up suspending with a wrong ordering of it.
+	 */
+	device_reorder_to_tail(consumer, NULL);
+
+	list_add_tail_rcu(&link->s_node, &supplier->links_to_consumers);
+	list_add_tail_rcu(&link->c_node, &consumer->links_to_suppliers);
+
+	dev_info(consumer, "Linked as a consumer to %s\n", dev_name(supplier));
+
+ out:
+	device_pm_unlock();
+	mutex_unlock(&device_links_lock);
+	return link;
+}
+EXPORT_SYMBOL_GPL(device_link_add);
+
+static void __device_link_free_srcu(struct rcu_head *rhead)
+{
+	struct device_link *link;
+
+	link = container_of(rhead, struct device_link, rcu_head);
+	put_device(link->consumer);
+	put_device(link->supplier);
+	kfree(link);
+}
+
+static void __device_link_del(struct device_link *link)
+{
+	dev_info(link->consumer, "Dropping the link to %s\n",
+		 dev_name(link->supplier));
+
+	list_del_rcu(&link->s_node);
+	list_del_rcu(&link->c_node);
+	call_srcu(&device_links_srcu, &link->rcu_head, __device_link_free_srcu);
+}
+
+/**
+ * device_link_del - Delete a link between two devices.
+ * @link: Device link to delete.
+ *
+ * The caller must ensure proper synchronization of this function with runtime
+ * PM.
+ */
+void device_link_del(struct device_link *link)
+{
+	mutex_lock(&device_links_lock);
+	device_pm_lock();
+	__device_link_del(link);
+	device_pm_unlock();
+	mutex_unlock(&device_links_lock);
+}
+EXPORT_SYMBOL_GPL(device_link_del);
+
+static int device_links_read_lock(void)
+{
+	return srcu_read_lock(&device_links_srcu);
+}
+
+static void device_links_read_unlock(int idx)
+{
+	return srcu_read_unlock(&device_links_srcu, idx);
+}
+
+static void device_links_missing_supplier(struct device *dev)
+{
+	struct device_link *link;
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node) {
+		spin_lock(&link->lock);
+
+		if (link->status == DEVICE_LINK_CONSUMER_PROBE)
+			link->status = DEVICE_LINK_AVAILABLE;
+
+		spin_unlock(&link->lock);
+	}
+}
+
+/**
+ * device_links_check_suppliers - Check presence of supplier drivers.
+ * @dev: Consumer device.
+ *
+ * Check links from this device to any suppliers.  Walk the list of the device's
+ * consumer links and see if all of the suppliers are available.  If not, simply
+ * return -EPROBE_DEFER.
+ *
+ * Walk the list under SRCU and check each link's status field under its lock.
+ *
+ * We need to guarantee that the supplier will not go away after the check has
+ * been positive here.  It only can go away in __device_release_driver() and
+ * that function  checks the device's links to consumers.  This means we need to
+ * mark the link as "consumer probe in progress" to make the supplier removal
+ * wait for us to complete (or bad things may happen).
+ *
+ * Links with the DEVICE_LINK_STATELESS flag set are ignored.
+ */
+int device_links_check_suppliers(struct device *dev)
+{
+	struct device_link *link;
+	int idx, ret = 0;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node) {
+		if (link->flags & DEVICE_LINK_STATELESS)
+			continue;
+
+		spin_lock(&link->lock);
+		if (link->status != DEVICE_LINK_AVAILABLE) {
+			spin_unlock(&link->lock);
+			device_links_missing_supplier(dev);
+			ret = -EPROBE_DEFER;
+			break;
+		}
+		link->status = DEVICE_LINK_CONSUMER_PROBE;
+		spin_unlock(&link->lock);
+	}
+
+	device_links_read_unlock(idx);
+	return ret;
+}
+
+/**
+ * device_links_driver_bound - Update device links after probing its driver.
+ * @dev: Device to update the links for.
+ *
+ * The probe has been successful, so update links from this device to any
+ * consumers by changing their status to "available".
+ *
+ * Also change the status of @dev's links to suppliers to "active".
+ *
+ * Links with the DEVICE_LINK_STATELESS flag set are ignored.
+ */
+void device_links_driver_bound(struct device *dev)
+{
+	struct device_link *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
+		if (link->flags & DEVICE_LINK_STATELESS)
+			continue;
+
+		spin_lock(&link->lock);
+		WARN_ON(link->status != DEVICE_LINK_DORMANT);
+		link->status = DEVICE_LINK_AVAILABLE;
+		spin_unlock(&link->lock);
+	}
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node) {
+		if (link->flags & DEVICE_LINK_STATELESS)
+			continue;
+
+		spin_lock(&link->lock);
+		WARN_ON(link->status != DEVICE_LINK_CONSUMER_PROBE);
+		link->status = DEVICE_LINK_ACTIVE;
+		spin_unlock(&link->lock);
+	}
+
+	device_links_read_unlock(idx);
+}
+
+/**
+ * device_links_driver_cleanup - Update links after driver removal.
+ * @dev: Device whose driver has just gone away.
+ *
+ * Update links to consumers for @dev by changing their status to "dormant".
+ *
+ * Links with the DEVICE_LINK_STATELESS flag set are ignored.
+ */
+void device_links_driver_cleanup(struct device *dev)
+{
+	struct device_link *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
+		if (link->flags & DEVICE_LINK_STATELESS)
+			continue;
+
+		WARN_ON(link->flags & DEVICE_LINK_AUTOREMOVE);
+		spin_lock(&link->lock);
+		WARN_ON(link->status != DEVICE_LINK_SUPPLIER_UNBIND);
+		link->status = DEVICE_LINK_DORMANT;
+		spin_unlock(&link->lock);
+	}
+
+	device_links_read_unlock(idx);
+}
+
+/**
+ * device_links_no_driver - Update links of a device without a driver.
+ * @dev: Device without a drvier.
+ *
+ * Delete all non-persistent links from this device to any suppliers.
+ *
+ * Persistent links stay around, but their status is changed to "available",
+ * unless they already are in the "supplier unbind in progress" state in which
+ * case they need not be updated.
+ *
+ * Links with the DEVICE_LINK_STATELESS flag set are ignored.
+ */
+void device_links_no_driver(struct device *dev)
+{
+	struct device_link *link, *ln;
+
+	mutex_lock(&device_links_lock);
+
+	list_for_each_entry_safe_reverse(link, ln, &dev->links_to_suppliers, c_node) {
+		if (link->flags & DEVICE_LINK_STATELESS)
+			continue;
+
+		if (link->flags & DEVICE_LINK_AUTOREMOVE) {
+			__device_link_del(link);
+		} else {
+			spin_lock(&link->lock);
+
+			if (link->status != DEVICE_LINK_SUPPLIER_UNBIND)
+				link->status = DEVICE_LINK_AVAILABLE;
+
+			spin_unlock(&link->lock);
+		}
+	}
+
+	mutex_unlock(&device_links_lock);
+}
+
+/**
+ * device_links_busy - Check if there are any busy links to consumers.
+ * @dev: Device to check.
+ *
+ * Check each consumer of the device and return 'true' if its link's status
+ * is one of "consumer probe" or "active" (meaning that the given consumer is
+ * probing right now or its driver is present).  Otherwise, change the link
+ * state to "supplier unbind" to prevent the consumer from being probed
+ * successfully going forward.
+ *
+ * Return 'false' if there are no probing or active consumers.
+ *
+ * Links with the DEVICE_LINK_STATELESS flag set are ignored.
+ */
+bool device_links_busy(struct device *dev)
+{
+	struct device_link *link;
+	int idx;
+	bool ret = false;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
+		if (link->flags & DEVICE_LINK_STATELESS)
+			continue;
+
+		spin_lock(&link->lock);
+		if (link->status == DEVICE_LINK_CONSUMER_PROBE
+		    || link->status == DEVICE_LINK_ACTIVE) {
+			spin_unlock(&link->lock);
+			ret = true;
+			break;
+		}
+		link->status = DEVICE_LINK_SUPPLIER_UNBIND;
+		spin_unlock(&link->lock);
+	}
+
+	device_links_read_unlock(idx);
+	return ret;
+}
+
+/**
+ * device_links_unbind_consumers - Force unbind consumers of the given device.
+ * @dev: Device to unbind the consumers of.
+ *
+ * Walk the list of links to consumers for @dev and if any of them is in the
+ * "consumer probe" state, wait for all device probes in progress to complete
+ * and start over.
+ *
+ * If that's not the case, change the status of the link to "supplier unbind"
+ * and check if the link was in the "active" state.  If so, force the consumer
+ * driver to unbind and start over (the consumer will not re-probe as we have
+ * changed the state of the link already).
+ *
+ * Links with the DEVICE_LINK_STATELESS flag set are ignored.
+ */
+void device_links_unbind_consumers(struct device *dev)
+{
+	struct device_link *link;
+	int idx;
+
+ start:
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
+		enum device_link_status status;
+
+		if (link->flags & DEVICE_LINK_STATELESS)
+			continue;
+
+		spin_lock(&link->lock);
+		status = link->status;
+		if (status == DEVICE_LINK_CONSUMER_PROBE) {
+			spin_unlock(&link->lock);
+
+			device_links_read_unlock(idx);
+
+			wait_for_device_probe();
+			goto start;
+		}
+		link->status = DEVICE_LINK_SUPPLIER_UNBIND;
+		spin_unlock(&link->lock);
+		if (status == DEVICE_LINK_ACTIVE) {
+			struct device *consumer = link->consumer;
+
+			get_device(consumer);
+
+			device_links_read_unlock(idx);
+
+			device_release_driver_internal(consumer, NULL,
+						       consumer->parent);
+			put_device(consumer);
+			goto start;
+		}
+	}
+
+	device_links_read_unlock(idx);
+}
+
+/**
+ * device_links_purge - Delete existing links to other devices.
+ * @dev: Target device.
+ */
+static void device_links_purge(struct device *dev)
+{
+	struct device_link *link, *ln;
+
+	/*
+	 * Delete all of the remaining links from this device to any other
+	 * devices (either consumers or suppliers).
+	 */
+	mutex_lock(&device_links_lock);
+
+	list_for_each_entry_safe_reverse(link, ln, &dev->links_to_suppliers, c_node) {
+		WARN_ON(link->status == DEVICE_LINK_ACTIVE);
+		__device_link_del(link);
+	}
+
+	list_for_each_entry_safe_reverse(link, ln, &dev->links_to_consumers, s_node) {
+		WARN_ON(link->status != DEVICE_LINK_DORMANT &&
+			link->status != DEVICE_LINK_NO_STATE);
+		__device_link_del(link);
+	}
+
+	mutex_unlock(&device_links_lock);
+}
+
+/* Device links support end. */
+
 int (*platform_notify)(struct device *dev) = NULL;
 int (*platform_notify_remove)(struct device *dev) = NULL;
 static struct kobject *dev_kobj;
@@ -711,6 +1198,8 @@ void device_initialize(struct device *de
 #ifdef CONFIG_GENERIC_MSI_IRQ
 	INIT_LIST_HEAD(&dev->msi_list);
 #endif
+	INIT_LIST_HEAD(&dev->links_to_consumers);
+	INIT_LIST_HEAD(&dev->links_to_suppliers);
 }
 EXPORT_SYMBOL_GPL(device_initialize);
 
@@ -1240,6 +1729,8 @@ void device_del(struct device *dev)
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DEL_DEVICE, dev);
+
+	device_links_purge(dev);
 	dpm_sysfs_remove(dev);
 	if (parent)
 		klist_del(&dev->p->knode_parent);
Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -249,6 +249,7 @@ static void driver_bound(struct device *
 		 __func__, dev_name(dev));
 
 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
+	device_links_driver_bound(dev);
 
 	device_pm_check_callbacks(dev);
 
@@ -341,6 +342,10 @@ static int really_probe(struct device *d
 		return ret;
 	}
 
+	ret = device_links_check_suppliers(dev);
+	if (ret)
+		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));
@@ -399,6 +404,7 @@ probe_failed:
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
 pinctrl_bind_failed:
+	device_links_no_driver(dev);
 	devres_release_all(dev);
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;
@@ -756,7 +762,7 @@ EXPORT_SYMBOL_GPL(driver_attach);
  * __device_release_driver() must be called with @dev lock held.
  * When called for a USB interface, @dev->parent lock must be held as well.
  */
-static void __device_release_driver(struct device *dev)
+static void __device_release_driver(struct device *dev, struct device *parent)
 {
 	struct device_driver *drv;
 
@@ -765,6 +771,25 @@ static void __device_release_driver(stru
 		if (driver_allows_async_probing(drv))
 			async_synchronize_full();
 
+		while (device_links_busy(dev)) {
+			device_unlock(dev);
+			if (parent)
+				device_unlock(parent);
+
+			device_links_unbind_consumers(dev);
+			if (parent)
+				device_lock(parent);
+
+			device_lock(dev);
+			/*
+			 * A concurrent invocation of the same function might
+			 * have released the driver successfully while this one
+			 * was waiting, so check for that.
+			 */
+			if (dev->driver != drv)
+				return;
+		}
+
 		pm_runtime_get_sync(dev);
 
 		driver_sysfs_remove(dev);
@@ -780,6 +805,9 @@ static void __device_release_driver(stru
 			dev->bus->remove(dev);
 		else if (drv->remove)
 			drv->remove(dev);
+
+		device_links_driver_cleanup(dev);
+		device_links_no_driver(dev);
 		devres_release_all(dev);
 		dev->driver = NULL;
 		dev_set_drvdata(dev, NULL);
@@ -796,16 +824,16 @@ static void __device_release_driver(stru
 	}
 }
 
-static void device_release_driver_internal(struct device *dev,
-					   struct device_driver *drv,
-					   struct device *parent)
+void device_release_driver_internal(struct device *dev,
+				    struct device_driver *drv,
+				    struct device *parent)
 {
 	if (parent)
 		device_lock(parent);
 
 	device_lock(dev);
 	if (!drv || drv == dev->driver)
-		__device_release_driver(dev);
+		__device_release_driver(dev, parent);
 
 	device_unlock(dev);
 	if (parent)
@@ -818,6 +846,10 @@ static void device_release_driver_intern
  *
  * Manually detach device from driver.
  * When called for a USB interface, @dev->parent lock must be held.
+ *
+ * If this function is to be called with @dev->parent lock held, ensure that
+ * the device's consumers are unbound in advance or that their locks can be
+ * acquired under the @dev->parent lock.
  */
 void device_release_driver(struct device *dev)
 {
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -706,6 +706,35 @@ struct device_dma_parameters {
 	unsigned long segment_boundary_mask;
 };
 
+enum device_link_status {
+	DEVICE_LINK_NO_STATE = -1,
+	DEVICE_LINK_DORMANT = 0,	/* Link not in use. */
+	DEVICE_LINK_AVAILABLE,		/* Supplier driver is present. */
+	DEVICE_LINK_ACTIVE,		/* Consumer driver is present too. */
+	DEVICE_LINK_CONSUMER_PROBE,	/* Consumer is probing. */
+	DEVICE_LINK_SUPPLIER_UNBIND,	/* Supplier is unbinding. */
+};
+
+/*
+ * Device link flags.
+ *
+ * STATELESS: The core won't track the presence of supplier/consumer drivers.
+ * AUTOREMOVE: Remove this link automatically on cunsumer driver unbind.
+ */
+#define DEVICE_LINK_STATELESS	(1 << 0)
+#define DEVICE_LINK_AUTOREMOVE	(1 << 1)
+
+struct device_link {
+	struct device *supplier;
+	struct list_head s_node;
+	struct device *consumer;
+	struct list_head c_node;
+	enum device_link_status status;
+	u32 flags;
+	spinlock_t lock;
+	struct rcu_head rcu_head;
+};
+
 /**
  * struct device - The basic device structure
  * @parent:	The device's "parent" device, the device to which it is attached.
@@ -731,6 +760,8 @@ struct device_dma_parameters {
  * 		on.  This shrinks the "Board Support Packages" (BSPs) and
  * 		minimizes board-specific #ifdefs in drivers.
  * @driver_data: Private pointer for driver specific info.
+ * @links_to_consumers: Links to consumer devices.
+ * @links_to_suppliers: Links to supplier devices.
  * @power:	For device power management.
  * 		See Documentation/power/devices.txt for details.
  * @pm_domain:	Provide callbacks that are executed during system suspend,
@@ -797,6 +828,8 @@ struct device {
 					   core doesn't touch it */
 	void		*driver_data;	/* Driver data, set and get with
 					   dev_set/get_drvdata */
+	struct list_head	links_to_consumers;
+	struct list_head	links_to_suppliers;
 	struct dev_pm_info	power;
 	struct dev_pm_domain	*pm_domain;
 
@@ -1113,6 +1146,11 @@ extern void device_shutdown(void);
 /* debugging and troubleshooting/diagnostic helpers. */
 extern const char *dev_driver_string(const struct device *dev);
 
+/* Device links interface. */
+struct device_link *device_link_add(struct device *consumer,
+				    struct device *supplier,
+				    enum device_link_status status, u32 flags);
+void device_link_del(struct device_link *link);
 
 #ifdef CONFIG_PRINTK
 
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -131,6 +131,7 @@ void device_pm_add(struct device *dev)
 		dev_warn(dev, "parent %s should not be sleeping\n",
 			dev_name(dev->parent));
 	list_add_tail(&dev->power.entry, &dpm_list);
+	dev->power.in_dpm_list = true;
 	mutex_unlock(&dpm_list_mtx);
 }
 
@@ -145,6 +146,7 @@ void device_pm_remove(struct device *dev
 	complete_all(&dev->power.completion);
 	mutex_lock(&dpm_list_mtx);
 	list_del_init(&dev->power.entry);
+	dev->power.in_dpm_list = false;
 	mutex_unlock(&dpm_list_mtx);
 	device_wakeup_disable(dev);
 	pm_runtime_remove(dev);
Index: linux-pm/drivers/base/power/power.h
===================================================================
--- linux-pm.orig/drivers/base/power/power.h
+++ linux-pm/drivers/base/power/power.h
@@ -127,6 +127,11 @@ extern void device_pm_move_after(struct
 extern void device_pm_move_last(struct device *);
 extern void device_pm_check_callbacks(struct device *dev);
 
+static inline bool device_pm_initialized(struct device *dev)
+{
+	return dev->power.in_dpm_list;
+}
+
 #else /* !CONFIG_PM_SLEEP */
 
 static inline void device_pm_sleep_init(struct device *dev) {}
@@ -146,6 +151,11 @@ static inline void device_pm_move_last(s
 
 static inline void device_pm_check_callbacks(struct device *dev) {}
 
+static inline bool device_pm_initialized(struct device *dev)
+{
+	return device_is_registered(dev);
+}
+
 #endif /* !CONFIG_PM_SLEEP */
 
 static inline void device_pm_init(struct device *dev)
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -559,6 +559,7 @@ struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned int		can_wakeup:1;
 	unsigned int		async_suspend:1;
+	bool			in_dpm_list:1;	/* Owned by the PM core */
 	bool			is_prepared:1;	/* Owned by the PM core */
 	bool			is_suspended:1;	/* Ditto */
 	bool			is_noirq_suspended:1;

[toc] | [prev] | [next] | [standalone]

#1494374 — Re: [PATCH v4 2/5] driver core: Functional dependencies tracking support

On Thu, Sep 29, 2016 at 02:38:04AM +0200, Rafael J. Wysocki wrote:
> +static int device_reorder_to_tail(struct device *dev, void *not_used)
> +{
> +	struct device_link *link;
> +
> +	/*
> +	 * Devices that have not been registered yet will be put to the ends
> +	 * of the lists during the registratio, so skip them here.
                                              ^
                                              n

> +     if (device_is_registered(dev))
> +             devices_kset_move_last(dev);
> +
> +     if (device_pm_initialized(dev))
> +             device_pm_move_last(dev);

Clever solution to this problem.  So little code!


> +/**
> + * device_links_check_suppliers - Check presence of supplier drivers.
> + * @dev: Consumer device.
> + *
> + * Check links from this device to any suppliers.  Walk the list of the device's
> + * consumer links and see if all of the suppliers are available.  If not, simply
      ^^^^^^^^
     "supplier links and see if all if them are available."


> +/*
> + * Device link flags.
> + *
> + * STATELESS: The core won't track the presence of supplier/consumer drivers.
> + * AUTOREMOVE: Remove this link automatically on cunsumer driver unbind.
                                                     ^
                                                     o


Apart from these nits patch [2/5] LGTM, so FWIW:

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thanks,

Lukas

[toc] | [prev] | [next] | [standalone]

#1494508 — Re: [PATCH v4 2/5] driver core: Functional dependencies tracking support

On Sat, Oct 1, 2016 at 9:43 AM, Lukas Wunner <lukas@wunner.de> wrote:
> On Thu, Sep 29, 2016 at 02:38:04AM +0200, Rafael J. Wysocki wrote:
>> +static int device_reorder_to_tail(struct device *dev, void *not_used)
>> +{
>> +     struct device_link *link;
>> +
>> +     /*
>> +      * Devices that have not been registered yet will be put to the ends
>> +      * of the lists during the registratio, so skip them here.
>                                               ^
>                                               n
>
>> +     if (device_is_registered(dev))
>> +             devices_kset_move_last(dev);
>> +
>> +     if (device_pm_initialized(dev))
>> +             device_pm_move_last(dev);
>
> Clever solution to this problem.  So little code!
>
>
>> +/**
>> + * device_links_check_suppliers - Check presence of supplier drivers.
>> + * @dev: Consumer device.
>> + *
>> + * Check links from this device to any suppliers.  Walk the list of the device's
>> + * consumer links and see if all of the suppliers are available.  If not, simply
>       ^^^^^^^^
>      "supplier links and see if all if them are available."
>
>
>> +/*
>> + * Device link flags.
>> + *
>> + * STATELESS: The core won't track the presence of supplier/consumer drivers.
>> + * AUTOREMOVE: Remove this link automatically on cunsumer driver unbind.
>                                                      ^
>                                                      o
>
>
> Apart from these nits patch [2/5] LGTM, so FWIW:
>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thanks for the review, much appreciated!

Cheers,
Rafael

[toc] | [prev] | [next] | [standalone]

#1493169

Hi Rafael,

On 2016-09-29 02:24, Rafael J. Wysocki wrote:
> Hi Everyone,
>
> On Thursday, September 08, 2016 11:25:44 PM Rafael J. Wysocki wrote:
>>> Hi Everyone,
>>>
>>> This is a refresh of the functional dependencies series that I posted last
>>> year and which has picked up by Marek quite recently.  For reference,
>>> appended is my introductory message sent previously (which may be
>>> slightly outdated now).
>>>
>>> As last time, the first patch rearranges the code around
>>> __device_release_driver() a bit to prepare it for the next one (it
>>> actually hasn't changed AFAICS).
>>>
>>> The second patch introduces the actual device links mechanics, but without
>>> system suspend/resume and runtime PM support which are added by the
>>> subsequent patches.
>>>
>>> Some bugs found by Marek during his work on these patches should be fixed
>>> here.  In particular, the endless recursion in device_reorder_to_tail()
>>> which simply was broken before.
>>>
>>> There are two additional patches to address the issue with runtime PM
>>> support
>>> that occured when runtime PM was disabled for some suppliers due to a PM
>>> sleep transition in progress.  Those patches simply make runtime PM
>>> helpers
>>> return 0 in that case which may be controversial, so please let me know if
>>> there are concerns about those.
>>>
>>> The way device_link_add() works is a bit different, as it takes an
>>> additional
>>> status argument now.  That makes it possible to create a link in any
>>> state,
>>> with extra care of course, and should address the problem pointed to by
>>> Lukas
>>> during the previous discussion.
>>>
>>> Also some comments from Tomeu have been addressed.
>> An update here.
>>
>> The first patch hasn't changed, so I'm resending it.
>>
>> The majority of changes in the other patches are in order to address Lukas'
>> comments.
>>
>> First off, I added a DEVICE_LINK_STATELESS flag that will prevent the driver
>> core from trying to maintain device links having it set.
>>
>> Also, the DEVICE_LINK_PERSISTENT flag was dropped (as link "persistence" is
>> the default behavior now) and there's a new one, DEVICE_LINK_AUTOREMOVE,
>> that will cause the driver core to remove the link on the consumer driver
>> unbind.
>>
>> Moreover, the code checks attempts to create a link between a parent and a
>> child device now and actively prevents that from happening.
>>
>> The changelog of the second patch has been updated as requested by Ulf.
>>
>> The third patch was updated to fix a bug related to the (previously missing)
>> clearing of power.direct_complete for supplier devices having consumers that
>> don't use direct_complete.
>>
>> The next two (runtime PM) patches turned out to be unnecessary, so I've
>> dropped them.
>>
>> The runtime PM patch [4/5] was reorganized somewhat to reduce the
>> indentation
>> level in there, but the code flow introduced by it is essentially the same
>> and the last patch was simply rebased on top of the new series.
> Time for another update. :-)
>
> Fewer changes this time, mostly to address issues found by Lukas and Marek.
>
> The most significant one is to make device_link_add() cope with the case when
> the consumer device has not been registered yet when it is called.  The
> supplier device still is required to be registered and the function will return
> NULL if that is not the case.
>
> Another significant change is in patch [4/5] that now makes the core apply
> pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the probing
> of a consumer one (in analogy with the parent).

Thanks for the update! Updated version fixes all the remaining issues.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[toc] | [prev] | [next] | [standalone]

#1493438

On Thursday, September 29, 2016 08:58:43 AM Marek Szyprowski wrote:
> Hi Rafael,
> 
> On 2016-09-29 02:24, Rafael J. Wysocki wrote:
> > Hi Everyone,
> >
> > On Thursday, September 08, 2016 11:25:44 PM Rafael J. Wysocki wrote:
> >>> Hi Everyone,
> >>>
> >>> This is a refresh of the functional dependencies series that I posted last
> >>> year and which has picked up by Marek quite recently.  For reference,
> >>> appended is my introductory message sent previously (which may be
> >>> slightly outdated now).
> >>>
> >>> As last time, the first patch rearranges the code around
> >>> __device_release_driver() a bit to prepare it for the next one (it
> >>> actually hasn't changed AFAICS).
> >>>
> >>> The second patch introduces the actual device links mechanics, but without
> >>> system suspend/resume and runtime PM support which are added by the
> >>> subsequent patches.
> >>>
> >>> Some bugs found by Marek during his work on these patches should be fixed
> >>> here.  In particular, the endless recursion in device_reorder_to_tail()
> >>> which simply was broken before.
> >>>
> >>> There are two additional patches to address the issue with runtime PM
> >>> support
> >>> that occured when runtime PM was disabled for some suppliers due to a PM
> >>> sleep transition in progress.  Those patches simply make runtime PM
> >>> helpers
> >>> return 0 in that case which may be controversial, so please let me know if
> >>> there are concerns about those.
> >>>
> >>> The way device_link_add() works is a bit different, as it takes an
> >>> additional
> >>> status argument now.  That makes it possible to create a link in any
> >>> state,
> >>> with extra care of course, and should address the problem pointed to by
> >>> Lukas
> >>> during the previous discussion.
> >>>
> >>> Also some comments from Tomeu have been addressed.
> >> An update here.
> >>
> >> The first patch hasn't changed, so I'm resending it.
> >>
> >> The majority of changes in the other patches are in order to address Lukas'
> >> comments.
> >>
> >> First off, I added a DEVICE_LINK_STATELESS flag that will prevent the driver
> >> core from trying to maintain device links having it set.
> >>
> >> Also, the DEVICE_LINK_PERSISTENT flag was dropped (as link "persistence" is
> >> the default behavior now) and there's a new one, DEVICE_LINK_AUTOREMOVE,
> >> that will cause the driver core to remove the link on the consumer driver
> >> unbind.
> >>
> >> Moreover, the code checks attempts to create a link between a parent and a
> >> child device now and actively prevents that from happening.
> >>
> >> The changelog of the second patch has been updated as requested by Ulf.
> >>
> >> The third patch was updated to fix a bug related to the (previously missing)
> >> clearing of power.direct_complete for supplier devices having consumers that
> >> don't use direct_complete.
> >>
> >> The next two (runtime PM) patches turned out to be unnecessary, so I've
> >> dropped them.
> >>
> >> The runtime PM patch [4/5] was reorganized somewhat to reduce the
> >> indentation
> >> level in there, but the code flow introduced by it is essentially the same
> >> and the last patch was simply rebased on top of the new series.
> > Time for another update. :-)
> >
> > Fewer changes this time, mostly to address issues found by Lukas and Marek.
> >
> > The most significant one is to make device_link_add() cope with the case when
> > the consumer device has not been registered yet when it is called.  The
> > supplier device still is required to be registered and the function will return
> > NULL if that is not the case.
> >
> > Another significant change is in patch [4/5] that now makes the core apply
> > pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the probing
> > of a consumer one (in analogy with the parent).
> 
> Thanks for the update! Updated version fixes all the remaining issues.
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks Marek!

[toc] | [prev] | [next] | [standalone]

#1494621

[+cc Andreas Noever]

On Thu, Sep 29, 2016 at 02:24:58AM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 08, 2016 11:25:44 PM Rafael J. Wysocki wrote:
> > > This is a refresh of the functional dependencies series that I posted
> > > last year and which has been picked up by Marek quite recently.

I've cooked up a patch to replace quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c with the "device links" functionality added by
Rafael's series quoted above.

The patch is included below.  I've also pushed a branch to GitHub which
comprises Rafael's series plus the patch on top:
https://github.com/l1k/linux/commits/device_links

One issue that cropped up is that the API does not provide public
functions to lock a device's link lists for traversal.  Thus when
iterating over the links and deleting them in the thunderbolt driver's
->remove hook, the list is completely unprotected.  It's not an issue
for this particular use case as noone else but this driver adds or
deletes links to the NHI, it just *looks* a bit fishy and there may
be other use cases where locking matters.  Maybe patch [2/5] should
export device_links_read_lock() / device_links_read_unlock()?

Apart from that, everything seems to work as it should:

On driver load:
[   13.829752] thunderbolt 0000:07:00.0: NHI initialized, starting thunderbolt
[...]
[   13.853747] pcieport 0000:06:03.0: Linked as a consumer to 0000:07:00.0
[   13.853749] pcieport 0000:06:04.0: Linked as a consumer to 0000:07:00.0
[   13.853751] pcieport 0000:06:05.0: Linked as a consumer to 0000:07:00.0
[   13.853753] pcieport 0000:06:06.0: Linked as a consumer to 0000:07:00.0

Those are the four hotplug ports on the controller.

On driver unload:
[   89.378691] pcieport 0000:06:03.0: Dropping the link to 0000:07:00.0
[   89.383346] pcieport 0000:06:04.0: Dropping the link to 0000:07:00.0
[   89.387977] pcieport 0000:06:05.0: Dropping the link to 0000:07:00.0
[   89.392589] pcieport 0000:06:06.0: Dropping the link to 0000:07:00.0
[...]
[   89.424511] thunderbolt 0000:07:00.0: shutdown

On resume from system sleep:
[  282.537470] ACPI: Waking up from system sleep state S3
[...]
[  282.625378] pcieport 0000:06:04.0: start waiting for 0000:07:00.0
[  282.625380] pcieport 0000:06:03.0: start waiting for 0000:07:00.0
[  282.625382] pcieport 0000:06:05.0: start waiting for 0000:07:00.0
[  282.625383] pcieport 0000:06:06.0: start waiting for 0000:07:00.0
[  282.656789] thunderbolt 0000:07:00.0: resuming...
[...]
[  283.500660] thunderbolt 0000:07:00.0: resume finished
[  283.500672] pcieport 0000:06:04.0:  done waiting for 0000:07:00.0
[  283.500673] pcieport 0000:06:03.0:  done waiting for 0000:07:00.0
[  283.500675] pcieport 0000:06:05.0:  done waiting for 0000:07:00.0
[  283.500677] pcieport 0000:06:06.0:  done waiting for 0000:07:00.0
[  283.564849] PM: noirq resume of devices complete after 971.845 msecs
[  283.564868] pciehp 0000:06:04.0:pcie204: Slot(4-1): Card present
[  283.564873] pciehp 0000:06:04.0:pcie204: Slot(4-1): Link Up

The "start waiting for" and "done waiting for" messages are debug
printk's that I had put in there.  I've also rebased and tested this
on my Thunderbolt runpm series and the only difference is that the
dpm_wait() is only executed for port 0000:06:04.0 (the one which
actually has a device connected), the three other hotplug ports use
direct_complete.

So Rafael's series is now also

Tested-by: Lukas Wunner <lukas@wunner.de>

though it should be noted that my patch does not make use of the optional
DEVICE_LINK_PM_RUNTIME flag introduced with patch [4/5].  (But I believe
Marek has tested that feature.)

Thanks,

Lukas

-- >8 --
Subject: [PATCH] thunderbolt: Use device links instead of PCI quirk

When resuming from system sleep, attached Thunderbolt devices are
inaccessible until the NHI has re-established PCI tunnels to them.  As a
consequence, the Thunderbolt controller's hotplug bridges (below which
the attached devices appear) must delay resuming until the NHI has
finished.  That requirement is not enforced by the PM core automatically
as it only guarantees correct resume ordering between parent and child,
and the NHI is not a parent of the hotplug bridges, but rather a niece.

So far we've open coded this requirement in a PCI quirk, which has the
following disadvantages:

- The code for the NHI resides in drivers/thunderbolt/, whereas the code
  for the hotplug bridges resides in drivers/pci/quirks.c, which may be
  surprising and non-obvious in particular for new contributors.

- Whenever support for an additional Thunderbolt controller is added,
  its PCI device ID needs to be amended in both places, which invites
  mistakes.  E.g. commit a42fb351ca1f ("thunderbolt: Allow loading of
  module on recent Apple MacBooks with thunderbolt 2 controller")
  relaxed the subvendor and subdevice ID in the NHI code but forgot to
  also change the hotplug bridge code.

- Since the PCI quirk cannot keep any state, it has to search for the
  NHI over and over again on every resume.

The PM core has just gained the ability to declare "device links", i.e.
dependencies between devices beyond the mere parent/child relationship.

Replace the PCI quirk with device links from the hotplug bridges to the
NHI.  The device links are set up when loading the thunderbolt driver
and torn down when it is removed.  The PM core takes care of everything
else.  As a bonus, the amount of code is reduced considerably.

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Andreas Noever <andreas.noever@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/quirks.c      | 56 -----------------------------------------------
 drivers/thunderbolt/nhi.c | 28 ++++++++++++++++++++++--
 drivers/thunderbolt/tb.h  |  1 +
 3 files changed, 27 insertions(+), 58 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 44e0ff3..9b78a03 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3293,62 +3293,6 @@ static void quirk_apple_poweroff_thunderbolt(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
 			       PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
 			       quirk_apple_poweroff_thunderbolt);
-
-/*
- * Apple: Wait for the thunderbolt controller to reestablish pci tunnels.
- *
- * During suspend the thunderbolt controller is reset and all pci
- * tunnels are lost. The NHI driver will try to reestablish all tunnels
- * during resume. We have to manually wait for the NHI since there is
- * no parent child relationship between the NHI and the tunneled
- * bridges.
- */
-static void quirk_apple_wait_for_thunderbolt(struct pci_dev *dev)
-{
-	struct pci_dev *sibling = NULL;
-	struct pci_dev *nhi = NULL;
-
-	if (!dmi_match(DMI_BOARD_VENDOR, "Apple Inc."))
-		return;
-	if (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)
-		return;
-	/*
-	 * Find the NHI and confirm that we are a bridge on the tb host
-	 * controller and not on a tb endpoint.
-	 */
-	sibling = pci_get_slot(dev->bus, 0x0);
-	if (sibling == dev)
-		goto out; /* we are the downstream bridge to the NHI */
-	if (!sibling || !sibling->subordinate)
-		goto out;
-	nhi = pci_get_slot(sibling->subordinate, 0x0);
-	if (!nhi)
-		goto out;
-	if (nhi->vendor != PCI_VENDOR_ID_INTEL
-		    || (nhi->device != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE &&
-			nhi->device != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C &&
-			nhi->device != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI &&
-			nhi->device != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI)
-		    || nhi->class != PCI_CLASS_SYSTEM_OTHER << 8)
-		goto out;
-	dev_info(&dev->dev, "quirk: waiting for thunderbolt to reestablish PCI tunnels...\n");
-	device_pm_wait_for_dev(&dev->dev, &nhi->dev);
-out:
-	pci_dev_put(nhi);
-	pci_dev_put(sibling);
-}
-DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL,
-			       PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
-			       quirk_apple_wait_for_thunderbolt);
-DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL,
-			       PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
-			       quirk_apple_wait_for_thunderbolt);
-DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL,
-			       PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE,
-			       quirk_apple_wait_for_thunderbolt);
-DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL,
-			       PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE,
-			       quirk_apple_wait_for_thunderbolt);
 #endif
 
 static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index a8c2041..e57f2e4 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -533,6 +533,21 @@ static void nhi_shutdown(struct tb_nhi *nhi)
 	mutex_destroy(&nhi->lock);
 }
 
+static int nhi_device_link_add_cb(struct pci_dev *pdev, void *ptr)
+{
+	struct tb *tb = ptr;
+
+	/*
+	 * Let all downstream bridges of the switch depend on the NHI, except
+	 * for the NHI's parent bridge.  This forces them to dpm_wait() in the
+	 * ->resume_noirq phase until the NHI has re-established the tunnels.
+	 */
+	if (pdev->bus == tb->downstream0->bus && pdev != tb->downstream0)
+		device_link_add(&pdev->dev, &tb->nhi->pdev->dev,
+				DEVICE_LINK_NO_STATE, DEVICE_LINK_STATELESS);
+	return 0;
+}
+
 static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct tb_nhi *nhi;
@@ -605,6 +620,10 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	}
 	pci_set_drvdata(pdev, tb);
 
+	tb->downstream0 = pci_upstream_bridge(pdev);
+	if (tb->downstream0)
+		pci_walk_bus(tb->downstream0->bus, nhi_device_link_add_cb, tb);
+
 	return 0;
 }
 
@@ -612,14 +631,19 @@ static void nhi_remove(struct pci_dev *pdev)
 {
 	struct tb *tb = pci_get_drvdata(pdev);
 	struct tb_nhi *nhi = tb->nhi;
+	struct device_link *link, *ln;
+
+	list_for_each_entry_safe(link, ln, &pdev->dev.links_to_consumers, s_node)
+		device_link_del(link);
+
 	thunderbolt_shutdown_and_free(tb);
 	nhi_shutdown(nhi);
 }
 
 /*
  * The tunneled pci bridges are siblings of us. Use resume_noirq to reenable
- * the tunnels asap. A corresponding pci quirk blocks the downstream bridges
- * resume_noirq until we are done.
+ * the tunnels asap. Device links block the downstream bridges' resume_noirq
+ * until we are done.
  */
 static const struct dev_pm_ops nhi_pm_ops = {
 	.suspend_noirq = nhi_suspend_noirq,
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 61d57ba..f9c45d5 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -106,6 +106,7 @@ struct tb {
 	struct workqueue_struct *wq; /* ordered workqueue for plug events */
 	struct tb_switch *root_switch;
 	struct list_head tunnel_list; /* list of active PCIe tunnels */
+	struct pci_dev *downstream0; /* downstream bridge to the NHI */
 	bool hotplug_active; /*
 			      * tb_handle_hotplug will stop progressing plug
 			      * events and exit if this is not set (it needs to
-- 
2.9.3

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web