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

Groups > linux.kernel > #1484564 > unrolled thread

[RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

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

  [RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-16 00:10 +0200
    Re: [RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies  tracking support Marek Szyprowski <m.szyprowski@samsung.com> - 2016-09-16 10:00 +0200
      Re: [RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies  tracking support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-16 14:10 +0200
    [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-16 14:30 +0200
      Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-20 00:50 +0200
        Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-23 15:10 +0200
        Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-23 15:40 +0200
          Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-26 19:00 +0200
            Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-27 14:20 +0200
      Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-27 11:00 +0200
        Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-27 14:00 +0200
          Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-28 12:50 +0200
            Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-28 13:40 +0200
              Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-29 12:40 +0200

#1484564 — [RFC/RFT][PATCH v3 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 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 a by mutex (for link object addition/removal).  In addition
to that, each link object has an internal status field whose
value reflects what's happening to the devices pointed to by
the link.  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 list
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 is 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>
---
 drivers/base/base.h    |   11 +
 drivers/base/core.c    |  471 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/base/dd.c      |   42 +++-
 include/linux/device.h |   38 +++
 4 files changed, 557 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 */
+extern int device_links_check_suppliers(struct device *dev);
+extern void device_links_driver_bound(struct device *dev);
+extern void device_links_driver_gone(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,450 @@ 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_kset_move_last(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.
+ */
+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 || supplier == consumer->parent ||
+	    ((flags & DEVICE_LINK_STATELESS) && (flags & DEVICE_LINK_AUTOREMOVE)))
+		return NULL;
+
+	mutex_lock(&device_links_lock);
+
+	/*
+	 * If there is a reverse dependency between the consumer and the
+	 * supplier already in the graph, return NULL.
+	 */
+	if (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_pm_lock();
+	device_reorder_to_tail(consumer, NULL);
+	device_pm_unlock();
+
+	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:
+	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 supplier devices for this one.
+ * @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_gone - Update links after driver removal.
+ * @dev: Device whose driver has 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_gone(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;
+		if (status == DEVICE_LINK_ACTIVE) {
+			struct device *consumer = link->consumer;
+
+			get_device(consumer);
+			spin_unlock(&link->lock);
+
+			device_links_read_unlock(idx);
+
+			device_release_driver_internal(consumer, NULL,
+						       consumer->parent);
+			put_device(consumer);
+			goto start;
+		}
+		spin_unlock(&link->lock);
+	}
+
+	device_links_read_unlock(idx);
+}
+
+/* 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 +1155,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);
 
@@ -1233,6 +1679,7 @@ void device_del(struct device *dev)
 {
 	struct device *parent = dev->parent;
 	struct class_interface *class_intf;
+	struct device_link *link, *ln;
 
 	/* Notify clients of device removal.  This call must come
 	 * before dpm_sysfs_remove().
@@ -1240,6 +1687,30 @@ void device_del(struct device *dev)
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DEL_DEVICE, dev);
+
+	/*
+	 * Delete all of the remaining links from this device to any other
+	 * devices (either consumers or suppliers).
+	 *
+	 * This requires that all links be dormant, so warn if that's no the
+	 * case.
+	 */
+	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_DORMANT &&
+			!(link->flags & DEVICE_LINK_STATELESS));
+		__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->flags & DEVICE_LINK_STATELESS));
+		__device_link_del(link);
+	}
+
+	mutex_unlock(&device_links_lock);
+
 	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);
 
@@ -399,6 +400,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;
@@ -489,6 +491,10 @@ int driver_probe_device(struct device_dr
 	if (!device_is_registered(dev))
 		return -ENODEV;
 
+	ret = device_links_check_suppliers(dev);
+	if (ret)
+		return ret;
+
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
@@ -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_gone(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 state machine is not applicable to this link.
+ * 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
 

[toc] | [next] | [standalone]

#1484762 — Re: [RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

Hi Rafael,

On 2016-09-16 00:06, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Currently, there is a problem with taking functional dependencies
> between 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 a by mutex (for link object addition/removal).  In addition
> to that, each link object has an internal status field whose
> value reflects what's happening to the devices pointed to by
> the link.  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 list
> 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 is 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>
> ---
>   drivers/base/base.h    |   11 +
>   drivers/base/core.c    |  471 +++++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/base/dd.c      |   42 +++-
>   include/linux/device.h |   38 +++
>   4 files changed, 557 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 */
> +extern int device_links_check_suppliers(struct device *dev);
> +extern void device_links_driver_bound(struct device *dev);
> +extern void device_links_driver_gone(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,450 @@ 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_kset_move_last(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.
> + */
> +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 || supplier == consumer->parent ||
> +	    ((flags & DEVICE_LINK_STATELESS) && (flags & DEVICE_LINK_AUTOREMOVE)))
> +		return NULL;
> +
> +	mutex_lock(&device_links_lock);
> +
> +	/*
> +	 * If there is a reverse dependency between the consumer and the
> +	 * supplier already in the graph, return NULL.
> +	 */
> +	if (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_pm_lock();
> +	device_reorder_to_tail(consumer, NULL);
> +	device_pm_unlock();
> +
> +	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:
> +	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 supplier devices for this one.
> + * @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_gone - Update links after driver removal.
> + * @dev: Device whose driver has 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_gone(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)

missing "{"

> +		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);
> +		}

missing "}"

> +
> +	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;
> +		if (status == DEVICE_LINK_ACTIVE) {
> +			struct device *consumer = link->consumer;
> +
> +			get_device(consumer);
> +			spin_unlock(&link->lock);
> +
> +			device_links_read_unlock(idx);
> +
> +			device_release_driver_internal(consumer, NULL,
> +						       consumer->parent);
> +			put_device(consumer);
> +			goto start;
> +		}
> +		spin_unlock(&link->lock);
> +	}
> +
> +	device_links_read_unlock(idx);
> +}
> +
> +/* 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 +1155,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);
>   
> @@ -1233,6 +1679,7 @@ void device_del(struct device *dev)
>   {
>   	struct device *parent = dev->parent;
>   	struct class_interface *class_intf;
> +	struct device_link *link, *ln;
>   
>   	/* Notify clients of device removal.  This call must come
>   	 * before dpm_sysfs_remove().
> @@ -1240,6 +1687,30 @@ void device_del(struct device *dev)
>   	if (dev->bus)
>   		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>   					     BUS_NOTIFY_DEL_DEVICE, dev);
> +
> +	/*
> +	 * Delete all of the remaining links from this device to any other
> +	 * devices (either consumers or suppliers).
> +	 *
> +	 * This requires that all links be dormant, so warn if that's no the
> +	 * case.
> +	 */
> +	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_DORMANT &&
> +			!(link->flags & DEVICE_LINK_STATELESS));
> +		__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->flags & DEVICE_LINK_STATELESS));
> +		__device_link_del(link);
> +	}
> +
> +	mutex_unlock(&device_links_lock);
> +
>   	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);
>   
> @@ -399,6 +400,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;
> @@ -489,6 +491,10 @@ int driver_probe_device(struct device_dr
>   	if (!device_is_registered(dev))
>   		return -ENODEV;
>   
> +	ret = device_links_check_suppliers(dev);
> +	if (ret)
> +		return ret;
> +
>   	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>   		 drv->bus->name, __func__, dev_name(dev), drv->name);
>   
> @@ -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_gone(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 state machine is not applicable to this link.
> + * 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
>   
>
>
>

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

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

#1484923 — Re: [RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Fri, Sep 16, 2016 at 9:53 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hi Rafael,
>
>
> On 2016-09-16 00:06, Rafael J. Wysocki wrote:
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>

[cut]

>> +/**
>> + * 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)
>
>
> missing "{"
>
>> +               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);
>> +               }
>
>
> missing "}"
>

Sorry for the breakage and thanks for the fix!

Rafael

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

#1484942 — [Update][RFC/RFT][PATCH v3 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 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 a by mutex (for link object addition/removal).  In addition
to that, each link object has an internal status field whose
value reflects what's happening to the devices pointed to by
the link.  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 list
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 is 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>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
---

The update adds missing braces in device_links_no_driver() found by Marek.

---
 drivers/base/base.h    |   11 +
 drivers/base/core.c    |  472 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/base/dd.c      |   42 +++-
 include/linux/device.h |   38 +++
 4 files changed, 558 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 */
+extern int device_links_check_suppliers(struct device *dev);
+extern void device_links_driver_bound(struct device *dev);
+extern void device_links_driver_gone(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,451 @@ 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_kset_move_last(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.
+ */
+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 || supplier == consumer->parent ||
+	    ((flags & DEVICE_LINK_STATELESS) && (flags & DEVICE_LINK_AUTOREMOVE)))
+		return NULL;
+
+	mutex_lock(&device_links_lock);
+
+	/*
+	 * If there is a reverse dependency between the consumer and the
+	 * supplier already in the graph, return NULL.
+	 */
+	if (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_pm_lock();
+	device_reorder_to_tail(consumer, NULL);
+	device_pm_unlock();
+
+	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:
+	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 supplier devices for this one.
+ * @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_gone - Update links after driver removal.
+ * @dev: Device whose driver has 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_gone(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;
+		if (status == DEVICE_LINK_ACTIVE) {
+			struct device *consumer = link->consumer;
+
+			get_device(consumer);
+			spin_unlock(&link->lock);
+
+			device_links_read_unlock(idx);
+
+			device_release_driver_internal(consumer, NULL,
+						       consumer->parent);
+			put_device(consumer);
+			goto start;
+		}
+		spin_unlock(&link->lock);
+	}
+
+	device_links_read_unlock(idx);
+}
+
+/* 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 +1156,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);
 
@@ -1233,6 +1680,7 @@ void device_del(struct device *dev)
 {
 	struct device *parent = dev->parent;
 	struct class_interface *class_intf;
+	struct device_link *link, *ln;
 
 	/* Notify clients of device removal.  This call must come
 	 * before dpm_sysfs_remove().
@@ -1240,6 +1688,30 @@ void device_del(struct device *dev)
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DEL_DEVICE, dev);
+
+	/*
+	 * Delete all of the remaining links from this device to any other
+	 * devices (either consumers or suppliers).
+	 *
+	 * This requires that all links be dormant, so warn if that's no the
+	 * case.
+	 */
+	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_DORMANT &&
+			!(link->flags & DEVICE_LINK_STATELESS));
+		__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->flags & DEVICE_LINK_STATELESS));
+		__device_link_del(link);
+	}
+
+	mutex_unlock(&device_links_lock);
+
 	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);
 
@@ -399,6 +400,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;
@@ -489,6 +491,10 @@ int driver_probe_device(struct device_dr
 	if (!device_is_registered(dev))
 		return -ENODEV;
 
+	ret = device_links_check_suppliers(dev);
+	if (ret)
+		return ret;
+
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
@@ -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_gone(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 state machine is not applicable to this link.
+ * 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
 

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

#1486934 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Currently, there is a problem with taking functional dependencies
> between into account.
         ^
	 devices

> 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.  
                                                                 ^^
                                                Trailing whitespace.

> 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 a by mutex (for link object addition/removal).  In addition
      ^^^^
      by a

> to that, each link object has an internal status field whose
> value reflects what's happening to the devices pointed to by
> the link.  That status field is protected by an internal spinlock.

More precisely, the status field tracks the driver boundness of
the two devices comprising the link.  ("what's happening to the
devices" is a bit broad, this is really about the *drivers*.)

> 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 list
                                                                  ^
                                                                  s

> 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 is the
                                                       ^^
                                                       if

> 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>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
[snip]
> +/**
> + * 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.
> + */
> +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 || supplier == consumer->parent ||

So a link from the child to the parent is forbidden, but e.g. to the
grandparent is not.  Either this should be forbidden for all ancestors
of the child or none.  I'd vote for the latter.  I don't see a reason
why this shouldn't be allowed.  It doesn't introduce a dependency loop
and it might be useful if someone needs a driver presence dependency
between parent and child.

[snip]
> +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);
> +}

How about declaring the above two functions inline?

[snip]
> +/**
> + * device_links_check_suppliers - Check supplier devices for this one.

The short description is mostly a repetition of the function name
and thus not very informative.

Imagine someone coming here from driver_probe_device(), where this
function is invoked.  They should immediately get an idea what
the function does.

How about: "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."

[snip]
> +/**
> + * 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.

Kerneldoc refers to persistent links, a term which no longer exists in v3.

[snip]
> +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);

The link will be autoremoved not only when the consumer unbinds,
but also when probing the consumer fails.

Looks like a bug.

Without the autoremove functionality, this function shrinks to just a
few LoC and it's probably okay to just duplicate that code and copy it
into device_links_driver_gone().  Then you'd have two separate
functions, one for the error path in really_probe() and the other for
__device_release_driver().

And you could then also give the functions names that match where
they're called from, e.g. device_links_probe_failed() and
device_links_driver_unbound().  Because the existing names
device_links_driver_gone() and device_links_no_driver() are very
similar and thus a bit confusing.

[snip]
> +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;
> +		if (status == DEVICE_LINK_ACTIVE) {
> +			struct device *consumer = link->consumer;
> +
> +			get_device(consumer);

As long as the struct device_link exists, a ref is held on the
supplier and consumer.  Why acquire another ref here?

> +			spin_unlock(&link->lock);

The lock is released both at the beginning of this if-block and
immediately after the if-block (in case the if-condition is false).
Why not simply release the lock *before* the if-block?

[snip]
> @@ -1233,6 +1680,7 @@ void device_del(struct device *dev)
>  {
>  	struct device *parent = dev->parent;
>  	struct class_interface *class_intf;
> +	struct device_link *link, *ln;
>  
>  	/* Notify clients of device removal.  This call must come
>  	 * before dpm_sysfs_remove().
> @@ -1240,6 +1688,30 @@ void device_del(struct device *dev)
>  	if (dev->bus)
>  		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>  					     BUS_NOTIFY_DEL_DEVICE, dev);
> +
> +	/*
> +	 * Delete all of the remaining links from this device to any other
> +	 * devices (either consumers or suppliers).
> +	 *
> +	 * This requires that all links be dormant, so warn if that's no the
> +	 * case.
> +	 */

How about moving this to a separate function, e.g. device_links_purge(dev)?

In all other cases you've created a new function even though it's only
called from a single place, which makes sense because it avoids cluttering
up these central functions like device_del().

> +	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_DORMANT &&
> +			!(link->flags & DEVICE_LINK_STATELESS));
> +		__device_link_del(link);
> +	}

Shouldn't it also be legal for the supplier links to be in
DEVICE_LINK_AVAILABLE state upon removal of a consumer device?

(And perhaps also DEVICE_LINK_SUPPLIER_UNBIND?)

Looks like a bug.

[snip]
> --- 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);
>  
> @@ -399,6 +400,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;
> @@ -489,6 +491,10 @@ int driver_probe_device(struct device_dr
>  	if (!device_is_registered(dev))
>  		return -ENODEV;
>  
> +	ret = device_links_check_suppliers(dev);
> +	if (ret)
> +		return ret;
> +

I think clarity would improve if you would move the call to
device_links_check_suppliers() down the call stack into really_probe().
Then it would be in the same place as the call to device_links_no_driver()
(if probing fails).

Furthermore, driver_probe_device() runtime resumes the parent before
probing a device, but you're not doing the same for the suppliers.
Looks like a bug.

[snip]
> --- 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,

How about a comment like /* Stateless. */ so that the relationship to
the DEVICE_LINK_STATELESS flag is clear.

> +	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 state machine is not applicable to this link.

IMO the consequence of setting this flag is not immediately clear from the
comment. How about: "Ignore driver presence."

> + * AUTOREMOVE: Remove this link automatically on cunsumer driver unbind.
                                                     ^
                                                     o

Thanks,

Lukas

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

#1490051 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Tue, Sep 20, 2016 at 12:46:30AM +0200, Lukas Wunner wrote:
> On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> > +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;
> > +		if (status == DEVICE_LINK_ACTIVE) {
> > +			struct device *consumer = link->consumer;
> > +
> > +			get_device(consumer);
> 
> As long as the struct device_link exists, a ref is held on the
> supplier and consumer.  Why acquire another ref here?

I'm withdrawing this particular comment as I failed to see that
device_links_read_unlock() is called next, so nothing prevents
the device link from being deleted, same for the consumer, thus
the ref needs to be acquired for device_release_driver_internal()
and this portion of Rafael's code seems perfectly correct.

Thanks & sorry for the noise,

Lukas

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

#1490087 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Tuesday, September 20, 2016 12:46:30 AM Lukas Wunner wrote:
> On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Currently, there is a problem with taking functional dependencies
> > between into account.
>          ^
> 	 devices
> 
> > 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.  
>                                                                  ^^
>                                                 Trailing whitespace.
> 
> > 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 a by mutex (for link object addition/removal).  In addition
>       ^^^^
>       by a
> 
> > to that, each link object has an internal status field whose
> > value reflects what's happening to the devices pointed to by
> > the link.  That status field is protected by an internal spinlock.
> 
> More precisely, the status field tracks the driver boundness of
> the two devices comprising the link.  ("what's happening to the
> devices" is a bit broad, this is really about the *drivers*.)

Basically OK, but it will be more than that most likely due to the bug
reported by Marek.

And thanks for the typo fixes. :-)

> > 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 list
>                                                                   ^
>                                                                   s
> 
> > 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 is the
>                                                        ^^
>                                                        if
> 
> > 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>
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> [snip]
> > +/**
> > + * 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.
> > + */
> > +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 || supplier == consumer->parent ||
> 
> So a link from the child to the parent is forbidden, but e.g. to the
> grandparent is not.  Either this should be forbidden for all ancestors
> of the child or none.  I'd vote for the latter.  I don't see a reason
> why this shouldn't be allowed.  It doesn't introduce a dependency loop
> and it might be useful if someone needs a driver presence dependency
> between parent and child.

While I can remove this check, I don't see why anyone would use links for
that instead of adding a device flag to cause the core to wait for the
parent to be probed if the child requires that.

> [snip]
> > +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);
> > +}
> 
> How about declaring the above two functions inline?

Well, does it really matter?  The complier should be able to make them inline
anyway if it sees the point.

> [snip]
> > +/**
> > + * device_links_check_suppliers - Check supplier devices for this one.
> 
> The short description is mostly a repetition of the function name
> and thus not very informative.
> 
> Imagine someone coming here from driver_probe_device(), where this
> function is invoked.  They should immediately get an idea what
> the function does.
> 
> How about: "Check presence of supplier drivers"

Sounds good.

> > + * @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."
> 
> [snip]
> > +/**
> > + * 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.
> 
> Kerneldoc refers to persistent links, a term which no longer exists in v3.

It does exist.

The flag is not present any more, but the links may still be persistent
(and the changelog talks about "persistent" and "non-persistent" links too).

> [snip]
> > +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);
> 
> The link will be autoremoved not only when the consumer unbinds,
> but also when probing the consumer fails.
> 
> Looks like a bug.

It really was intentional, because the use case I see for AUTOREMOVE (and
the only one to be honest) is when the link is created by the consumer
probe in which case it wants to avoid worrying about the cleanup part.

Which also is applicable to the cleanup when the probe fails IMO.

> Without the autoremove functionality, this function shrinks to just a
> few LoC and it's probably okay to just duplicate that code and copy it
> into device_links_driver_gone().  Then you'd have two separate
> functions, one for the error path in really_probe() and the other for
> __device_release_driver().
> 
> And you could then also give the functions names that match where
> they're called from, e.g. device_links_probe_failed() and
> device_links_driver_unbound().  Because the existing names
> device_links_driver_gone() and device_links_no_driver() are very
> similar and thus a bit confusing.

I can drop AUTOREMOVE entirely if it is problematic.

> [snip]
> > +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;
> > +		if (status == DEVICE_LINK_ACTIVE) {
> > +			struct device *consumer = link->consumer;
> > +
> > +			get_device(consumer);
> 
> As long as the struct device_link exists, a ref is held on the
> supplier and consumer.  Why acquire another ref here?

This comment has just been withdrawn. :-)

> > +			spin_unlock(&link->lock);
> 
> The lock is released both at the beginning of this if-block and
> immediately after the if-block (in case the if-condition is false).
> Why not simply release the lock *before* the if-block?

Because the get_device() needs to be done under the lock.

> 
> [snip]
> > @@ -1233,6 +1680,7 @@ void device_del(struct device *dev)
> >  {
> >  	struct device *parent = dev->parent;
> >  	struct class_interface *class_intf;
> > +	struct device_link *link, *ln;
> >  
> >  	/* Notify clients of device removal.  This call must come
> >  	 * before dpm_sysfs_remove().
> > @@ -1240,6 +1688,30 @@ void device_del(struct device *dev)
> >  	if (dev->bus)
> >  		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> >  					     BUS_NOTIFY_DEL_DEVICE, dev);
> > +
> > +	/*
> > +	 * Delete all of the remaining links from this device to any other
> > +	 * devices (either consumers or suppliers).
> > +	 *
> > +	 * This requires that all links be dormant, so warn if that's no the
> > +	 * case.
> > +	 */
> 
> How about moving this to a separate function, e.g. device_links_purge(dev)?
> 
> In all other cases you've created a new function even though it's only
> called from a single place, which makes sense because it avoids cluttering
> up these central functions like device_del().

OK

> > +	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_DORMANT &&
> > +			!(link->flags & DEVICE_LINK_STATELESS));
> > +		__device_link_del(link);
> > +	}
> 
> Shouldn't it also be legal for the supplier links to be in
> DEVICE_LINK_AVAILABLE state upon removal of a consumer device?
> 
> (And perhaps also DEVICE_LINK_SUPPLIER_UNBIND?)
> 
> Looks like a bug.

But this is done after removing the supplier driver, so the state should be
DORMANT (unless the link is stateless), shouldn't it?

> [snip]
> > --- 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);
> >  
> > @@ -399,6 +400,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;
> > @@ -489,6 +491,10 @@ int driver_probe_device(struct device_dr
> >  	if (!device_is_registered(dev))
> >  		return -ENODEV;
> >  
> > +	ret = device_links_check_suppliers(dev);
> > +	if (ret)
> > +		return ret;
> > +
> 
> I think clarity would improve if you would move the call to
> device_links_check_suppliers() down the call stack into really_probe().
> Then it would be in the same place as the call to device_links_no_driver()
> (if probing fails).

I'll have a look.

> Furthermore, driver_probe_device() runtime resumes the parent before
> probing a device, but you're not doing the same for the suppliers.
> Looks like a bug.

I need to check, but for this patch it doesn't matter anyway as links don't
apply to runtime PM at this point yet.

> [snip]
> > --- 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,
> 
> How about a comment like /* Stateless. */ so that the relationship to
> the DEVICE_LINK_STATELESS flag is clear.
> 
> > +	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 state machine is not applicable to this link.
> 
> IMO the consequence of setting this flag is not immediately clear from the
> comment. How about: "Ignore driver presence."

Well, I'm not sure if that's any better to be honest.  The consequences would
still be unclear with it, but for a different reason. :-)

> > + * AUTOREMOVE: Remove this link automatically on cunsumer driver unbind.

Thanks,
Rafael

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

#1491438 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Fri, Sep 23, 2016 at 03:42:31PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 20, 2016 12:46:30 AM Lukas Wunner wrote:
> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> > > +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);
> > 
> > The link will be autoremoved not only when the consumer unbinds,
> > but also when probing the consumer fails.
> > 
> > Looks like a bug.
> 
> It really was intentional, because the use case I see for AUTOREMOVE (and
> the only one to be honest) is when the link is created by the consumer
> probe in which case it wants to avoid worrying about the cleanup part.
> 
> Which also is applicable to the cleanup when the probe fails IMO.

You're right, makes sense.


> > > +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;
> > > +		if (status == DEVICE_LINK_ACTIVE) {
> > > +			struct device *consumer = link->consumer;
> > > +
> > > +			get_device(consumer);
> > > +			spin_unlock(&link->lock);
> > 
> > The lock is released both at the beginning of this if-block and
> > immediately after the if-block (in case the if-condition is false).
> > Why not simply release the lock *before* the if-block?
> 
> Because the get_device() needs to be done under the lock.

According to the commit message, the spinlock only protects the status
field and the consumer device is prevented from disappearing with the RCU.
So the spin lock could be released before the if-block AFAICS.
(But perhaps there are style/readability reasons to have the unlock both
in the if-block and afterwards.)


> > > @@ -1233,6 +1680,7 @@ void device_del(struct device *dev)
> > >  {
> > >  	struct device *parent = dev->parent;
> > >  	struct class_interface *class_intf;
> > > +	struct device_link *link, *ln;
> > >  
> > >  	/* Notify clients of device removal.  This call must come
> > >  	 * before dpm_sysfs_remove().
> > > @@ -1240,6 +1688,30 @@ void device_del(struct device *dev)
> > >  	if (dev->bus)
> > >  		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> > >  					     BUS_NOTIFY_DEL_DEVICE, dev);
> > > +
> > > +	/*
> > > +	 * Delete all of the remaining links from this device to any other
> > > +	 * devices (either consumers or suppliers).
> > > +	 *
> > > +	 * This requires that all links be dormant, so warn if that's no the
> > > +	 * case.
> > > +	 */
> > > +	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_DORMANT &&
> > > +			!(link->flags & DEVICE_LINK_STATELESS));
> > > +		__device_link_del(link);
> > > +	}
> > 
> > Shouldn't it also be legal for the supplier links to be in
> > DEVICE_LINK_AVAILABLE state upon removal of a consumer device?
> > 
> > (And perhaps also DEVICE_LINK_SUPPLIER_UNBIND?)
> > 
> > Looks like a bug.
> 
> But this is done after removing the supplier driver, so the state should be
> DORMANT (unless the link is stateless), shouldn't it?

The scenario I have in mind is that the supplier device is bound to a
driver and the consumer device has no driver and is being removed.
In that case the status will be DEVICE_LINK_AVAILABLE and the user
will get a WARN splat, which seems gratuitous because it should be legal.

And the other scenario is when the supplier is unbinding. It iterates
over the links to consumers and puts them in DEVICE_LINK_SUPPLIER_UNBIND.
Let's say the link to consumer A was put into that state, but there's
a consumer B remaining which is bound.  The RCU and spinlock are unlocked
before device_release_driver_internal() is called for that consumer.
If at that point consumer device A is removed for whatever reason,
the link will also be removed and the user will again get a gratuitous
WARN splat.


Thanks,

Lukas

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

#1491878 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Mon, Sep 26, 2016 at 6:51 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Fri, Sep 23, 2016 at 03:42:31PM +0200, Rafael J. Wysocki wrote:
>> On Tuesday, September 20, 2016 12:46:30 AM Lukas Wunner wrote:
>> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
>> > > +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);
>> >
>> > The link will be autoremoved not only when the consumer unbinds,
>> > but also when probing the consumer fails.
>> >
>> > Looks like a bug.
>>
>> It really was intentional, because the use case I see for AUTOREMOVE (and
>> the only one to be honest) is when the link is created by the consumer
>> probe in which case it wants to avoid worrying about the cleanup part.
>>
>> Which also is applicable to the cleanup when the probe fails IMO.
>
> You're right, makes sense.
>
>
>> > > +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;
>> > > +         if (status == DEVICE_LINK_ACTIVE) {
>> > > +                 struct device *consumer = link->consumer;
>> > > +
>> > > +                 get_device(consumer);
>> > > +                 spin_unlock(&link->lock);
>> >
>> > The lock is released both at the beginning of this if-block and
>> > immediately after the if-block (in case the if-condition is false).
>> > Why not simply release the lock *before* the if-block?
>>
>> Because the get_device() needs to be done under the lock.
>
> According to the commit message, the spinlock only protects the status
> field and the consumer device is prevented from disappearing with the RCU.
> So the spin lock could be released before the if-block AFAICS.
> (But perhaps there are style/readability reasons to have the unlock both
> in the if-block and afterwards.)

OK

Apparently, I was worrying about that the link might go away after the
device_links_read_unlock() in the if () block, so the object pointed
to by "consumer" had to be prevented from going away as well at that
point, but you are right that it's sufficient to call the get_device()
before the device_links_read_unlock() for that and it doesn't have to
go under the spinlock.

>> > > @@ -1233,6 +1680,7 @@ void device_del(struct device *dev)
>> > >  {
>> > >   struct device *parent = dev->parent;
>> > >   struct class_interface *class_intf;
>> > > + struct device_link *link, *ln;
>> > >
>> > >   /* Notify clients of device removal.  This call must come
>> > >    * before dpm_sysfs_remove().
>> > > @@ -1240,6 +1688,30 @@ void device_del(struct device *dev)
>> > >   if (dev->bus)
>> > >           blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>> > >                                        BUS_NOTIFY_DEL_DEVICE, dev);
>> > > +
>> > > + /*
>> > > +  * Delete all of the remaining links from this device to any other
>> > > +  * devices (either consumers or suppliers).
>> > > +  *
>> > > +  * This requires that all links be dormant, so warn if that's no the
>> > > +  * case.
>> > > +  */
>> > > + 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_DORMANT &&
>> > > +                 !(link->flags & DEVICE_LINK_STATELESS));
>> > > +         __device_link_del(link);
>> > > + }
>> >
>> > Shouldn't it also be legal for the supplier links to be in
>> > DEVICE_LINK_AVAILABLE state upon removal of a consumer device?
>> >
>> > (And perhaps also DEVICE_LINK_SUPPLIER_UNBIND?)
>> >
>> > Looks like a bug.
>>
>> But this is done after removing the supplier driver, so the state should be
>> DORMANT (unless the link is stateless), shouldn't it?
>
> The scenario I have in mind is that the supplier device is bound to a
> driver and the consumer device has no driver and is being removed.
> In that case the status will be DEVICE_LINK_AVAILABLE and the user
> will get a WARN splat, which seems gratuitous because it should be legal.
>
> And the other scenario is when the supplier is unbinding. It iterates
> over the links to consumers and puts them in DEVICE_LINK_SUPPLIER_UNBIND.
> Let's say the link to consumer A was put into that state, but there's
> a consumer B remaining which is bound.  The RCU and spinlock are unlocked
> before device_release_driver_internal() is called for that consumer.
> If at that point consumer device A is removed for whatever reason,
> the link will also be removed and the user will again get a gratuitous
> WARN splat.

Well, it all boils down to the observation that the consumer device
may be deleted when the supplier still has a driver or is unbinding,
which is a good point.

Thanks,
Rafael

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

#1491769 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> +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;
> +		}

While revisiting this function it just occurred to me that there's
a theoretical infinite loop here if the consumer probes, is unbound
by the supplier, then reprobes again before the supplier had a chance
to update the link to DEVICE_LINK_SUPPLIER_UNBIND.  Perhaps this isn't
a problem in practice, but noting anyway.

The problem is that the link state is written to both by the supplier
and consumer.  If there was a separate bit in struct device_link to
indicate the supplier's desire to unbind, the problem would go away.
However a mix of such a bit plus the state machine would become
somewhat confusing...

Best regards,

Lukas

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

#1491856 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Tue, Sep 27, 2016 at 10:54 AM, Lukas Wunner <lukas@wunner.de> wrote:
> On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
>> +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;
>> +             }
>
> While revisiting this function it just occurred to me that there's
> a theoretical infinite loop here if the consumer probes, is unbound
> by the supplier, then reprobes again before the supplier had a chance
> to update the link to DEVICE_LINK_SUPPLIER_UNBIND.  Perhaps this isn't
> a problem in practice, but noting anyway.

But the consumer is unbound only after setting the link status to
DEVICE_LINK_SUPPLIER_UNBIND and then it won't probe again.

Or am I overlooking something?

Thanks,
Rafael

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

#1492528 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Tue, Sep 27, 2016 at 01:52:48PM +0200, Rafael J. Wysocki wrote:
> On Tue, Sep 27, 2016 at 10:54 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> >> +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;
> >
> > While revisiting this function it just occurred to me that there's
> > a theoretical infinite loop here if the consumer probes, is unbound
> > by the supplier, then reprobes again before the supplier had a chance
> > to update the link to DEVICE_LINK_SUPPLIER_UNBIND.  Perhaps this isn't
> > a problem in practice, but noting anyway.
> 
> But the consumer is unbound only after setting the link status to
> DEVICE_LINK_SUPPLIER_UNBIND and then it won't probe again.

Sorry, looking at the code with a fresh pair of eyeballs I realize the
scenario for the infinite loop is different from what I've written above:
The infinite loop can occur if the consumer probes continuously but never
succeeds, e.g. due to some unfulfilled condition in its ->probe hook.

That could be fixed by moving the assignment

		link->status = DEVICE_LINK_SUPPLIER_UNBIND;

above the preceding if-block (but below "status = link->status;").

The next time the consumer probes, it will return with -EPROBE_DEFER
(return value of device_links_check_suppliers()).

However the semantics of DEVICE_LINK_SUPPLIER_UNBIND are "consumer not
bound and blocked from probing", with the above change it would become
"consumer may or may not be bound and blocked from probing".

Thus it would also be necessary to change device_links_driver_bound()
so that it doesn't update the status to DEVICE_LINK_ACTIVE.  Also,
device_links_busy() and device_links_unbind_consumers() would have
to check boundness with device_is_bound() if the status is
DEVICE_LINK_SUPPLIER_UNBIND.  Perhaps it would be easier to add
separate link states for this, or perhaps this problem is too
theoretical to bother dealing with it.

Thanks,

Lukas

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

#1492539 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Wed, Sep 28, 2016 at 12:43 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Tue, Sep 27, 2016 at 01:52:48PM +0200, Rafael J. Wysocki wrote:
>> On Tue, Sep 27, 2016 at 10:54 AM, Lukas Wunner <lukas@wunner.de> wrote:
>> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
>> >> +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;
>> >
>> > While revisiting this function it just occurred to me that there's
>> > a theoretical infinite loop here if the consumer probes, is unbound
>> > by the supplier, then reprobes again before the supplier had a chance
>> > to update the link to DEVICE_LINK_SUPPLIER_UNBIND.  Perhaps this isn't
>> > a problem in practice, but noting anyway.
>>
>> But the consumer is unbound only after setting the link status to
>> DEVICE_LINK_SUPPLIER_UNBIND and then it won't probe again.
>
> Sorry, looking at the code with a fresh pair of eyeballs I realize the
> scenario for the infinite loop is different from what I've written above:
> The infinite loop can occur if the consumer probes continuously but never
> succeeds, e.g. due to some unfulfilled condition in its ->probe hook.

I'm not sure how that can happen.

If it doesn't succeed, the driver's ->probe() will return an error, so
that driver is not going to be tried again, unless the error is
-EPROBE_DEFER, but that will cause it to wait for another driver to
probe successfully in the meantime.

Or do you have any particular example in which things work differently in mind?

Thanks,
Rafael

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

#1493323 — Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support

On Wed, Sep 28, 2016 at 01:31:36PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 28, 2016 at 12:43 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Tue, Sep 27, 2016 at 01:52:48PM +0200, Rafael J. Wysocki wrote:
> >> On Tue, Sep 27, 2016 at 10:54 AM, Lukas Wunner <lukas@wunner.de> wrote:
> >> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> >> >> +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;
> >> >
> >> > While revisiting this function it just occurred to me that there's
> >> > a theoretical infinite loop here if the consumer probes, is unbound
> >> > by the supplier, then reprobes again before the supplier had a chance
> >> > to update the link to DEVICE_LINK_SUPPLIER_UNBIND.  Perhaps this isn't
> >> > a problem in practice, but noting anyway.
> >>
> >> But the consumer is unbound only after setting the link status to
> >> DEVICE_LINK_SUPPLIER_UNBIND and then it won't probe again.
> >
> > Sorry, looking at the code with a fresh pair of eyeballs I realize the
> > scenario for the infinite loop is different from what I've written above:
> > The infinite loop can occur if the consumer probes continuously but never
> > succeeds, e.g. due to some unfulfilled condition in its ->probe hook.
> 
> I'm not sure how that can happen.
> 
> If it doesn't succeed, the driver's ->probe() will return an error, so
> that driver is not going to be tried again, unless the error is
> -EPROBE_DEFER, but that will cause it to wait for another driver to
> probe successfully in the meantime.

You're right, it seems that the code is safe.  Sorry for the noise. :)

Best regards,

Lukas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web