linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies)
@ 2016-06-17  6:26 Marek Szyprowski
  2016-06-17  6:26 ` [PATCH v2 01/10] driver core: Add a wrapper around __device_release_driver() Marek Szyprowski
                   ` (10 more replies)
  0 siblings, 11 replies; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman

Hello,

This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using device links, which lets
SYSMMU controller's runtime PM to follow master's device runtime PM (the
device which actually performs DMA transaction). The main idea
behind this solution is an observation that any DMA activity from master
device can be done only when master device is active, thus when master
device is suspended SYSMMU controller device can also be suspended.

This patchset solves the situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).

Patches 1-5 are resend of the "[RFC][PATCH 0/5] Functional dependencies
between devices" patchset:
http://thread.gmane.org/gmane.linux.power-management.general/67424/focus=2126379
I've included them here, because it is hard to find them all on mailing
list archives.

Patches 6-8 are fixes to device dependencies/links code, which were
required to use this solution for Exynos IOMMU driver. I'm not PM/runtime
PM code expert, so please double check if my changes are really correct.

This patchset requires my previous changes to Exynos IOMMU driver
submitted in the "Exynos IOMMU: improve clock management" thread:
http://www.spinics.net/lists/arm-kernel/msg505695.html

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Changelog:
v2:
- replaced PM notifiers with generic device dependencies/links developped
  by Rafael J. Wysocki

v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version


Patch summary:

Marek Szyprowski (5):
  driver core: Avoid endless recursion if device has more than one link
  driver core: Add support for links to already probed drivers
  PM core: Fix restoring devices with links during system PM transition
  iommu/exynos: Remove excessive, useless debug
  iommu/exynos: Add proper runtime pm support

Rafael J. Wysocki (5):
  driver core: Add a wrapper around __device_release_driver()
  driver core: Functional dependencies tracking support
  PM core: Make async suspend/resume of devices use device links
  PM core: Make runtime PM of devices use device links
  PM core: Optimize the use of device links for runtime PM

 drivers/base/base.h          |  13 ++
 drivers/base/core.c          | 410 +++++++++++++++++++++++++++++++++++++++++++
 drivers/base/dd.c            |  65 +++++--
 drivers/base/power/main.c    |  68 ++++++-
 drivers/base/power/runtime.c | 130 +++++++++++++-
 drivers/iommu/exynos-iommu.c | 221 +++++++++++------------
 include/linux/device.h       |  41 +++++
 include/linux/pm.h           |   1 +
 include/linux/pm_runtime.h   |   6 +
 9 files changed, 809 insertions(+), 146 deletions(-)

-- 
1.9.1

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2 01/10] driver core: Add a wrapper around __device_release_driver()
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
@ 2016-06-17  6:26 ` Marek Szyprowski
  2016-06-17  6:26 ` [PATCH v2 02/10] driver core: Functional dependencies tracking support Marek Szyprowski
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman,
	Rafael J. Wysocki

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

Add an internal wrapper around __device_release_driver() that will
acquire device locks and do the necessary checks before calling it.

The next patch will make use of it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/base/dd.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 16688f50729c..d9e76e9205c7 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -796,6 +796,22 @@ static void __device_release_driver(struct device *dev)
 	}
 }
 
+static 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_unlock(dev);
+	if (parent)
+		device_unlock(parent);
+}
+
 /**
  * device_release_driver - manually detach device from driver.
  * @dev: device.
@@ -810,9 +826,7 @@ void device_release_driver(struct device *dev)
 	 * within their ->remove callback for the same device, they
 	 * will deadlock right here.
 	 */
-	device_lock(dev);
-	__device_release_driver(dev);
-	device_unlock(dev);
+	device_release_driver_internal(dev, NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(device_release_driver);
 
@@ -837,15 +851,7 @@ void driver_detach(struct device_driver *drv)
 		dev = dev_prv->device;
 		get_device(dev);
 		spin_unlock(&drv->p->klist_devices.k_lock);
-
-		if (dev->parent)	/* Needed for USB */
-			device_lock(dev->parent);
-		device_lock(dev);
-		if (dev->driver == drv)
-			__device_release_driver(dev);
-		device_unlock(dev);
-		if (dev->parent)
-			device_unlock(dev->parent);
+		device_release_driver_internal(dev, drv, dev->parent);
 		put_device(dev);
 	}
 }
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
  2016-06-17  6:26 ` [PATCH v2 01/10] driver core: Add a wrapper around __device_release_driver() Marek Szyprowski
@ 2016-06-17  6:26 ` Marek Szyprowski
  2016-06-17 10:36   ` Lukas Wunner
  2016-06-17  6:26 ` [PATCH v2 03/10] PM core: Make async suspend/resume of devices use device links Marek Szyprowski
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman,
	Rafael J. Wysocki

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

Currently, there is a problem with handling cases where functional
dependencies between devices are involved.

What I mean by a "functional dependency" is when the driver of device
B needs both device A and its driver to be present and functional to
be able to work.  This 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.  This also has certain consequences
for power management of these devices (suspend/resume and runtime PM
ordering).

Add support for representing those functional dependencies between
devices to allow the driver core to track them and act on them in
certain cases where they matter.

The argument for doing that in the driver core is that there are
quite a few distinct use cases related to that, 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).

To that end, represent links between devices (or more precisely
between device+driver combos) as a struct devlink object containing
pointers to the devices in question, a list node for each of them,
status information, flags, a lock and an RCU head for synchronization.

Also add two new list heads, supplier_links and consumer_links, 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 may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.

Link objects are deleted either explicitly, by calling
device_link_del() on the link object in question, or automatically,
when the consumer device is unbound from its driver or when one
of the target devices is deleted, depending on the link type.

There are two types of link objects, persistent and non-persistent.
The difference between them is that the persistent links stay around
until one of the target devices is deleted, which the non-persistent
ones are deleted when the consumer driver is unbound from its device
(ie. they are assumed to be valid only as long as the consumer device
has a driver bound to it).  The DEVICE_LINK_PERSISTENT flag has to
be passed to device_link_add() so as to create a persistent link.

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 the all of the supplier
and consumer devices.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/base/base.h    |  11 ++
 drivers/base/core.c    | 386 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/base/dd.c      |  42 +++++-
 include/linux/device.h |  36 +++++
 4 files changed, 470 insertions(+), 5 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index e05db388bd1c..cccb1d211541 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -107,6 +107,9 @@ extern void bus_remove_device(struct device *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);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0a8bdade53f2..416341df3268 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -44,6 +44,367 @@ static int __init sysfs_deprecated_setup(char *arg)
 early_param("sysfs.deprecated", sysfs_deprecated_setup);
 #endif
 
+/* Device links support. */
+
+DEFINE_STATIC_SRCU(device_links_srcu);
+static DEFINE_MUTEX(device_links_lock);
+
+static int device_reorder_to_tail(struct device *dev, void *not_used)
+{
+	struct devlink *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->consumer_links, c_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.
+ * @flags: Link flags.
+ *
+ * At least one of the flags must be set.  If DEVICE_LINK_PROBE_TIME is set, the
+ * caller is expected to know that (a) the supplier device is present and active
+ * (ie. its driver is functional) and (b) the consumer device is probing at the
+ * moment and therefore the initial state of the link will be "consumer probe"
+ * in that case.  If DEVICE_LINK_PROBE_TIME is not set, DEVICE_LINK_PERSISTENT
+ * must be set (meaning that the link will not go away when the consumer driver
+ * goes away).
+ *
+ * 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 those lists.
+ */
+struct devlink *device_link_add(struct device *consumer,
+				struct device *supplier, u32 flags)
+{
+	struct devlink *link;
+
+	if (!consumer || !supplier || !flags)
+		return NULL;
+
+	mutex_lock(&device_links_lock);
+
+	list_for_each_entry(link, &supplier->supplier_links, 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);
+	link->flags = flags;
+	link->status = (flags & DEVICE_LINK_PROBE_TIME) ?
+			DEVICE_LINK_CONSUMER_PROBE : DEVICE_LINK_DORMANT;
+	spin_lock_init(&link->lock);
+
+	/*
+	 * Move the consumer and all of the devices depending on it to the end
+	 * of dpm_list and the devices_kset list.
+	 *
+	 * We have 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->supplier_links);
+	list_add_tail_rcu(&link->c_node, &consumer->consumer_links);
+
+	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 __devlink_free_srcu(struct rcu_head *rhead)
+{
+	struct devlink *link;
+
+	link = container_of(rhead, struct devlink, rcu_head);
+	put_device(link->consumer);
+	put_device(link->supplier);
+	kfree(link);
+}
+
+static void devlink_del(struct devlink *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, __devlink_free_srcu);
+}
+
+/**
+ * device_link_del - Delete a link between two devices.
+ * @link: Device link to delete.
+ */
+void device_link_del(struct devlink *link)
+{
+	mutex_lock(&device_links_lock);
+	devlink_del(link);
+	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 devlink *link;
+
+	list_for_each_entry_rcu(link, &dev->consumer_links, 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).
+ */
+int device_links_check_suppliers(struct device *dev)
+{
+	struct devlink *link;
+	int idx, ret = 0;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->consumer_links, c_node) {
+		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".
+ */
+void device_links_driver_bound(struct device *dev)
+{
+	struct devlink *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->supplier_links, s_node) {
+		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->consumer_links, c_node) {
+		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".
+ */
+void device_links_driver_gone(struct device *dev)
+{
+	struct devlink *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->supplier_links, s_node) {
+		WARN_ON(!(link->flags & DEVICE_LINK_PERSISTENT));
+		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.
+ */
+void device_links_no_driver(struct device *dev)
+{
+	struct devlink *link, *ln;
+
+	mutex_lock(&device_links_lock);
+
+	list_for_each_entry_safe_reverse(link, ln, &dev->consumer_links, c_node)
+		if (link->flags & DEVICE_LINK_PERSISTENT) {
+			spin_lock(&link->lock);
+
+			if (link->status != DEVICE_LINK_SUPPLIER_UNBIND)
+				link->status = DEVICE_LINK_AVAILABLE;
+
+			spin_unlock(&link->lock);
+		} else {
+			devlink_del(link);
+		}
+
+	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' it 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.
+ */
+bool device_links_busy(struct device *dev)
+{
+	struct devlink *link;
+	int idx;
+	bool ret = false;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->supplier_links, s_node) {
+		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).
+ */
+void device_links_unbind_consumers(struct device *dev)
+{
+	struct devlink *link;
+	int idx;
+
+ start:
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->supplier_links, s_node) {
+		enum devlink_status status;
+
+		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 +1072,8 @@ void device_initialize(struct device *dev)
 #ifdef CONFIG_GENERIC_MSI_IRQ
 	INIT_LIST_HEAD(&dev->msi_list);
 #endif
+	INIT_LIST_HEAD(&dev->supplier_links);
+	INIT_LIST_HEAD(&dev->consumer_links);
 }
 EXPORT_SYMBOL_GPL(device_initialize);
 
@@ -1233,6 +1596,7 @@ void device_del(struct device *dev)
 {
 	struct device *parent = dev->parent;
 	struct class_interface *class_intf;
+	struct devlink *link, *ln;
 
 	/* Notify clients of device removal.  This call must come
 	 * before dpm_sysfs_remove().
@@ -1240,6 +1604,28 @@ 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->consumer_links, c_node) {
+		WARN_ON(link->status != DEVICE_LINK_DORMANT);
+		devlink_del(link);
+	}
+
+	list_for_each_entry_safe_reverse(link, ln, &dev->supplier_links, s_node) {
+		WARN_ON(link->status != DEVICE_LINK_DORMANT);
+		devlink_del(link);
+	}
+
+	mutex_unlock(&device_links_lock);
+
 	dpm_sysfs_remove(dev);
 	if (parent)
 		klist_del(&dev->p->knode_parent);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index d9e76e9205c7..7c0abeba89e9 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -249,6 +249,7 @@ static void driver_bound(struct device *dev)
 		 __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_driver *drv, struct device *dev)
 	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(struct device *dev)
 		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(struct device *dev)
 			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(struct device *dev)
 	}
 }
 
-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_internal(struct device *dev,
  *
  * 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)
 {
diff --git a/include/linux/device.h b/include/linux/device.h
index 38f02814d53a..647204bd74a0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -706,6 +706,34 @@ struct device_dma_parameters {
 	unsigned long segment_boundary_mask;
 };
 
+enum devlink_status {
+	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.
+ *
+ * PERSISTENT: Do not delete the link on consumer device driver unbind.
+ * PROBE_TIME: Assume supplier device functional when creating the link.
+ */
+#define DEVICE_LINK_PERSISTENT	(1 << 0)
+#define DEVICE_LINK_PROBE_TIME	(1 << 1)
+
+struct devlink {
+	struct device *supplier;
+	struct list_head s_node;
+	struct device *consumer;
+	struct list_head c_node;
+	enum devlink_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 +759,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.
+ * @supplier_links: Links to consumer devices.
+ * @consumer_links: 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 +827,8 @@ struct device {
 					   core doesn't touch it */
 	void		*driver_data;	/* Driver data, set and get with
 					   dev_set/get_drvdata */
+	struct list_head	supplier_links;
+	struct list_head	consumer_links;
 	struct dev_pm_info	power;
 	struct dev_pm_domain	*pm_domain;
 
@@ -1113,6 +1145,10 @@ extern void device_shutdown(void);
 /* debugging and troubleshooting/diagnostic helpers. */
 extern const char *dev_driver_string(const struct device *dev);
 
+/* Device links interface. */
+struct devlink *device_link_add(struct device *consumer,
+				struct device *supplier, u32 flags);
+void device_link_del(struct devlink *link);
 
 #ifdef CONFIG_PRINTK
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 03/10] PM core: Make async suspend/resume of devices use device links
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
  2016-06-17  6:26 ` [PATCH v2 01/10] driver core: Add a wrapper around __device_release_driver() Marek Szyprowski
  2016-06-17  6:26 ` [PATCH v2 02/10] driver core: Functional dependencies tracking support Marek Szyprowski
@ 2016-06-17  6:26 ` Marek Szyprowski
  2016-06-17  6:26 ` [PATCH v2 04/10] PM core: Make runtime PM " Marek Szyprowski
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman,
	Rafael J. Wysocki

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

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

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

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

diff --git a/drivers/base/base.h b/drivers/base/base.h
index cccb1d211541..123b986eb061 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -163,3 +163,5 @@ 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);
+extern int device_links_read_lock(void);
+extern void device_links_read_unlock(int idx);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 416341df3268..51a479ed68b5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -163,12 +163,12 @@ void device_link_del(struct devlink *link)
 }
 EXPORT_SYMBOL_GPL(device_link_del);
 
-static int device_links_read_lock(void)
+int device_links_read_lock(void)
 {
 	return srcu_read_lock(&device_links_srcu);
 }
 
-static void device_links_read_unlock(int idx)
+void device_links_read_unlock(int idx)
 {
 	return srcu_read_unlock(&device_links_srcu, idx);
 }
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index e44944f4be77..9470ccd87f11 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -244,6 +244,62 @@ static void dpm_wait_for_children(struct device *dev, bool async)
        device_for_each_child(dev, &async, dpm_wait_fn);
 }
 
+static void dpm_wait_for_suppliers(struct device *dev, bool async)
+{
+	struct devlink *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	/*
+	 * If the supplier goes away right after we've checked the link to it,
+	 * we'll wait for its completion to change the state, but that's fine,
+	 * because the only things that will block as a result are the SRCU
+	 * callbacks freeing the link objects for the links in the list we're
+	 * walking.
+	 */
+	list_for_each_entry_rcu(link, &dev->consumer_links, c_node)
+		if (link->status != DEVICE_LINK_DORMANT)
+			dpm_wait(link->supplier, async);
+
+	device_links_read_unlock(idx);
+}
+
+static void dpm_wait_for_superior(struct device *dev, bool async)
+{
+	dpm_wait(dev->parent, async);
+	dpm_wait_for_suppliers(dev, async);
+}
+
+static void dpm_wait_for_consumers(struct device *dev, bool async)
+{
+	struct devlink *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	/*
+	 * The status of a device link can only be changed from "dormant" by a
+	 * probe, but that cannot happen during system suspend/resume.  In
+	 * theory it can change to "dormant" at that time, but then it is
+	 * reasonable to wait for the target device anyway (eg. if it goes
+	 * away, it's better to wait for it to go away completely and then
+	 * continue instead of trying to continue in parallel with its
+	 * unregistration).
+	 */
+	list_for_each_entry_rcu(link, &dev->supplier_links, s_node)
+		if (link->status != DEVICE_LINK_DORMANT)
+			dpm_wait(link->consumer, async);
+
+	device_links_read_unlock(idx);
+}
+
+static void dpm_wait_for_subordinate(struct device *dev, bool async)
+{
+	dpm_wait_for_children(dev, async);
+	dpm_wait_for_consumers(dev, async);
+}
+
 /**
  * pm_op - Return the PM operation appropriate for given PM event.
  * @ops: PM operations to choose from.
@@ -488,7 +544,7 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
 	if (!dev->power.is_noirq_suspended)
 		goto Out;
 
-	dpm_wait(dev->parent, async);
+	dpm_wait_for_superior(dev, async);
 
 	if (dev->pm_domain) {
 		info = "noirq power domain ";
@@ -618,7 +674,7 @@ static int device_resume_early(struct device *dev, pm_message_t state, bool asyn
 	if (!dev->power.is_late_suspended)
 		goto Out;
 
-	dpm_wait(dev->parent, async);
+	dpm_wait_for_superior(dev, async);
 
 	if (dev->pm_domain) {
 		info = "early power domain ";
@@ -750,7 +806,7 @@ static int device_resume(struct device *dev, pm_message_t state, bool async)
 		goto Complete;
 	}
 
-	dpm_wait(dev->parent, async);
+	dpm_wait_for_superior(dev, async);
 	dpm_watchdog_set(&wd, dev);
 	device_lock(dev);
 
@@ -1038,7 +1094,7 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
-	dpm_wait_for_children(dev, async);
+	dpm_wait_for_subordinate(dev, async);
 
 	if (dev->pm_domain) {
 		info = "noirq power domain ";
@@ -1185,7 +1241,7 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
-	dpm_wait_for_children(dev, async);
+	dpm_wait_for_subordinate(dev, async);
 
 	if (dev->pm_domain) {
 		info = "late power domain ";
@@ -1358,7 +1414,7 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
 	TRACE_DEVICE(dev);
 	TRACE_SUSPEND(0);
 
-	dpm_wait_for_children(dev, async);
+	dpm_wait_for_subordinate(dev, async);
 
 	if (async_error)
 		goto Complete;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 04/10] PM core: Make runtime PM of devices use device links
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
                   ` (2 preceding siblings ...)
  2016-06-17  6:26 ` [PATCH v2 03/10] PM core: Make async suspend/resume of devices use device links Marek Szyprowski
@ 2016-06-17  6:26 ` Marek Szyprowski
  2016-06-17  6:26 ` [PATCH v2 05/10] PM core: Optimize the use of device links for runtime PM Marek Szyprowski
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman,
	Rafael J. Wysocki

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

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

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

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

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

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

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/base/core.c          | 15 +++++++
 drivers/base/dd.c            |  1 +
 drivers/base/power/runtime.c | 93 +++++++++++++++++++++++++++++++++++++++++---
 include/linux/device.h       |  5 +++
 include/linux/pm_runtime.h   |  2 +
 5 files changed, 111 insertions(+), 5 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 51a479ed68b5..1fe7cf232e1e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -88,6 +88,11 @@ struct devlink *device_link_add(struct device *consumer,
 	if (!consumer || !supplier || !flags)
 		return NULL;
 
+#define RPM_ACTIVE_FLAGS (DEVICE_LINK_PM_RUNTIME | DEVICE_LINK_PROBE_TIME)
+	if ((flags & DEVICE_LINK_RPM_ACTIVE)
+	    && (flags & RPM_ACTIVE_FLAGS) != RPM_ACTIVE_FLAGS)
+		return NULL;
+
 	mutex_lock(&device_links_lock);
 
 	list_for_each_entry(link, &supplier->supplier_links, s_node)
@@ -98,6 +103,16 @@ struct devlink *device_link_add(struct device *consumer,
 	if (!link)
 		goto out;
 
+	if (flags & DEVICE_LINK_RPM_ACTIVE) {
+		if (pm_runtime_get_sync(supplier) < 0) {
+			pm_runtime_put_noidle(supplier);
+			kfree(link);
+			goto out;
+		}
+		link->rpm_active = true;
+	} else {
+		link->rpm_active = false;
+	}
 	get_device(supplier);
 	link->supplier = supplier;
 	INIT_LIST_HEAD(&link->s_node);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 7c0abeba89e9..2efec98ddb12 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -791,6 +791,7 @@ static void __device_release_driver(struct device *dev, struct device *parent)
 		}
 
 		pm_runtime_get_sync(dev);
+		pm_runtime_clean_up_links(dev);
 
 		driver_sysfs_remove(dev);
 
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b74690418504..ba21c123b16e 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -12,6 +12,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/pm_wakeirq.h>
 #include <trace/events/rpm.h>
+
+#include "../base.h"
 #include "power.h"
 
 typedef int (*pm_callback_t)(struct device *);
@@ -266,19 +268,69 @@ static int rpm_check_suspend_allowed(struct device *dev)
 static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 	__releases(&dev->power.lock) __acquires(&dev->power.lock)
 {
-	int retval;
+	struct devlink *link;
+	int retval, idx;
 
-	if (dev->power.irq_safe)
+	if (dev->power.irq_safe) {
 		spin_unlock(&dev->power.lock);
-	else
+	} else {
 		spin_unlock_irq(&dev->power.lock);
 
+		/*
+		 * Resume suppliers if necessary.
+		 *
+		 * The device's runtime PM status cannot change until this
+		 * routine returns, so it is safe to read the status outside of
+		 * the lock.
+		 */
+		if (dev->power.runtime_status == RPM_RESUMING) {
+			idx = device_links_read_lock();
+
+			list_for_each_entry_rcu(link, &dev->consumer_links, c_node)
+				if ((link->flags & DEVICE_LINK_PM_RUNTIME)
+				    && link->status != DEVICE_LINK_SUPPLIER_UNBIND
+				    && !link->rpm_active) {
+					retval = pm_runtime_get_sync(link->supplier);
+					if (retval < 0) {
+						pm_runtime_put_noidle(link->supplier);
+						goto fail;
+					}
+					link->rpm_active = true;
+				}
+
+			device_links_read_unlock(idx);
+		}
+	}
+
 	retval = cb(dev);
 
-	if (dev->power.irq_safe)
+	if (dev->power.irq_safe) {
 		spin_lock(&dev->power.lock);
-	else
+	} else {
+		/*
+		 * If the device is suspending and the callback has returned
+		 * success, drop the usage counters of the suppliers that have
+		 * been reference counted on its resume.
+		 *
+		 * Do that if resume fails too.
+		 */
+		if ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
+		    || (dev->power.runtime_status == RPM_RESUMING && retval)) {
+			idx = device_links_read_lock();
+
+ fail:
+			list_for_each_entry_rcu(link, &dev->consumer_links, c_node)
+				if (link->status != DEVICE_LINK_SUPPLIER_UNBIND
+				    && link->rpm_active) {
+					pm_runtime_put(link->supplier);
+					link->rpm_active = false;
+				}
+
+			device_links_read_unlock(idx);
+		}
+
 		spin_lock_irq(&dev->power.lock);
+	}
 
 	return retval;
 }
@@ -1443,6 +1495,37 @@ void pm_runtime_remove(struct device *dev)
 }
 
 /**
+ * pm_runtime_clean_up_links - Prepare links to consumers for driver removal.
+ * @dev: Device whose driver is going to be removed.
+ *
+ * Check links from this device to any consumers and if any of them have active
+ * runtime PM references to the device, drop the usage counter of the device
+ * (once per link).
+ *
+ * Since the device is guaranteed to be runtime-active at the point this is
+ * called, nothing else needs to be done here.
+ *
+ * Moreover, this is called after device_links_busy() has returned 'false', so
+ * the status of each link is guaranteed to be DEVICE_LINK_SUPPLIER_UNBIND and
+ * therefore rpm_active can't be manipulated concurrently.
+ */
+void pm_runtime_clean_up_links(struct device *dev)
+{
+	struct devlink *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->supplier_links, s_node)
+		if (link->rpm_active) {
+			pm_runtime_put_noidle(dev);
+			link->rpm_active = false;
+		}
+
+	device_links_read_unlock(idx);
+}
+
+/**
  * pm_runtime_force_suspend - Force a device into suspend state if needed.
  * @dev: Device to suspend.
  *
diff --git a/include/linux/device.h b/include/linux/device.h
index 647204bd74a0..536ce4058e50 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -719,9 +719,13 @@ enum devlink_status {
  *
  * PERSISTENT: Do not delete the link on consumer device driver unbind.
  * PROBE_TIME: Assume supplier device functional when creating the link.
+ * PM_RUNTIME: If set, the runtime PM framework will use this link.
+ * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
  */
 #define DEVICE_LINK_PERSISTENT	(1 << 0)
 #define DEVICE_LINK_PROBE_TIME	(1 << 1)
+#define DEVICE_LINK_PM_RUNTIME	(1 << 2)
+#define DEVICE_LINK_RPM_ACTIVE	(1 << 3)
 
 struct devlink {
 	struct device *supplier;
@@ -730,6 +734,7 @@ struct devlink {
 	struct list_head c_node;
 	enum devlink_status status;
 	u32 flags;
+	bool rpm_active;
 	spinlock_t lock;
 	struct rcu_head rcu_head;
 };
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 2e14d2667b6c..c21a21c064e3 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -55,6 +55,7 @@ extern unsigned long pm_runtime_autosuspend_expiration(struct device *dev);
 extern void pm_runtime_update_max_time_suspended(struct device *dev,
 						 s64 delta_ns);
 extern void pm_runtime_set_memalloc_noio(struct device *dev, bool enable);
+extern void pm_runtime_clean_up_links(struct device *dev);
 
 static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
 {
@@ -186,6 +187,7 @@ static inline unsigned long pm_runtime_autosuspend_expiration(
 				struct device *dev) { return 0; }
 static inline void pm_runtime_set_memalloc_noio(struct device *dev,
 						bool enable){}
+static inline void pm_runtime_clean_up_links(struct device *dev) {}
 
 #endif /* !CONFIG_PM */
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 05/10] PM core: Optimize the use of device links for runtime PM
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
                   ` (3 preceding siblings ...)
  2016-06-17  6:26 ` [PATCH v2 04/10] PM core: Make runtime PM " Marek Szyprowski
@ 2016-06-17  6:26 ` Marek Szyprowski
  2016-06-17  6:26 ` [PATCH v2 06/10] driver core: Avoid endless recursion if device has more than one link Marek Szyprowski
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman,
	Rafael J. Wysocki

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

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

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

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/base/core.c          |  6 ++++++
 drivers/base/power/runtime.c | 23 ++++++++++++++++++++---
 include/linux/pm.h           |  1 +
 include/linux/pm_runtime.h   |  4 ++++
 4 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 1fe7cf232e1e..215cd44de761 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -119,6 +119,9 @@ struct devlink *device_link_add(struct device *consumer,
 	get_device(consumer);
 	link->consumer = consumer;
 	INIT_LIST_HEAD(&link->c_node);
+	if (flags & DEVICE_LINK_PM_RUNTIME)
+		pm_runtime_new_link(consumer);
+
 	link->flags = flags;
 	link->status = (flags & DEVICE_LINK_PROBE_TIME) ?
 			DEVICE_LINK_CONSUMER_PROBE : DEVICE_LINK_DORMANT;
@@ -161,6 +164,9 @@ static void devlink_del(struct devlink *link)
 	dev_info(link->consumer, "Dropping the link to %s\n",
 		 dev_name(link->supplier));
 
+	if (link->flags & DEVICE_LINK_PM_RUNTIME)
+		pm_runtime_drop_link(link->consumer);
+
 	list_del_rcu(&link->s_node);
 	list_del_rcu(&link->c_node);
 	call_srcu(&device_links_srcu, &link->rcu_head, __devlink_free_srcu);
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index ba21c123b16e..0ea00d442e0f 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -270,6 +270,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 {
 	struct devlink *link;
 	int retval, idx;
+	bool use_links = dev->power.links_count > 0;
 
 	if (dev->power.irq_safe) {
 		spin_unlock(&dev->power.lock);
@@ -283,7 +284,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 		 * routine returns, so it is safe to read the status outside of
 		 * the lock.
 		 */
-		if (dev->power.runtime_status == RPM_RESUMING) {
+		if (use_links && dev->power.runtime_status == RPM_RESUMING) {
 			idx = device_links_read_lock();
 
 			list_for_each_entry_rcu(link, &dev->consumer_links, c_node)
@@ -314,8 +315,9 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 		 *
 		 * Do that if resume fails too.
 		 */
-		if ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
-		    || (dev->power.runtime_status == RPM_RESUMING && retval)) {
+		if (use_links
+		    && ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
+		    || (dev->power.runtime_status == RPM_RESUMING && retval))) {
 			idx = device_links_read_lock();
 
  fail:
@@ -1525,6 +1527,21 @@ void pm_runtime_clean_up_links(struct device *dev)
 	device_links_read_unlock(idx);
 }
 
+void pm_runtime_new_link(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.links_count++;
+	spin_unlock_irq(&dev->power.lock);
+}
+
+void pm_runtime_drop_link(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	WARN_ON(dev->power.links_count == 0);
+	dev->power.links_count--;
+	spin_unlock_irq(&dev->power.lock);
+}
+
 /**
  * pm_runtime_force_suspend - Force a device into suspend state if needed.
  * @dev: Device to suspend.
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 06eb353182ab..49c811378110 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -596,6 +596,7 @@ struct dev_pm_info {
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
 	unsigned int		memalloc_noio:1;
+	unsigned int		links_count;
 	enum rpm_request	request;
 	enum rpm_status		runtime_status;
 	int			runtime_error;
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index c21a21c064e3..bef6203aaf14 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -56,6 +56,8 @@ extern void pm_runtime_update_max_time_suspended(struct device *dev,
 						 s64 delta_ns);
 extern void pm_runtime_set_memalloc_noio(struct device *dev, bool enable);
 extern void pm_runtime_clean_up_links(struct device *dev);
+extern void pm_runtime_new_link(struct device *dev);
+extern void pm_runtime_drop_link(struct device *dev);
 
 static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
 {
@@ -188,6 +190,8 @@ static inline unsigned long pm_runtime_autosuspend_expiration(
 static inline void pm_runtime_set_memalloc_noio(struct device *dev,
 						bool enable){}
 static inline void pm_runtime_clean_up_links(struct device *dev) {}
+static inline void pm_runtime_new_link(struct device *dev) {}
+static inline void pm_runtime_drop_link(struct device *dev) {}
 
 #endif /* !CONFIG_PM */
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 06/10] driver core: Avoid endless recursion if device has more than one link
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
                   ` (4 preceding siblings ...)
  2016-06-17  6:26 ` [PATCH v2 05/10] PM core: Optimize the use of device links for runtime PM Marek Szyprowski
@ 2016-06-17  6:26 ` Marek Szyprowski
  2016-09-06 23:09   ` Rafael J. Wysocki
  2016-06-17  6:26 ` [PATCH v2 07/10] driver core: Add support for links to already probed drivers Marek Szyprowski
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman

This patch fixes endless recursion, which happends when device has
more than one link.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/base/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 215cd44de761..4e778539b750 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -57,7 +57,8 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
 	device_pm_move_last(dev);
 	device_for_each_child(dev, NULL, device_reorder_to_tail);
 	list_for_each_entry(link, &dev->consumer_links, c_node)
-		device_reorder_to_tail(link->consumer, NULL);
+		if (link->consumer != dev)
+			device_reorder_to_tail(link->consumer, NULL);
 
 	return 0;
 }
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 07/10] driver core: Add support for links to already probed drivers
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
                   ` (5 preceding siblings ...)
  2016-06-17  6:26 ` [PATCH v2 06/10] driver core: Avoid endless recursion if device has more than one link Marek Szyprowski
@ 2016-06-17  6:26 ` Marek Szyprowski
  2016-09-06 23:13   ` Rafael J. Wysocki
  2016-06-17  6:26 ` [PATCH v2 08/10] PM core: Fix restoring devices with links during system PM transition Marek Szyprowski
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman

Set proper link state if link is created between already probed supplier
device and to be probed consumer device.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/base/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4e778539b750..d9c5c5542a6b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -125,7 +125,9 @@ struct devlink *device_link_add(struct device *consumer,
 
 	link->flags = flags;
 	link->status = (flags & DEVICE_LINK_PROBE_TIME) ?
-			DEVICE_LINK_CONSUMER_PROBE : DEVICE_LINK_DORMANT;
+			DEVICE_LINK_CONSUMER_PROBE :
+			(supplier->driver ? DEVICE_LINK_AVAILABLE :
+			 DEVICE_LINK_DORMANT);
 	spin_lock_init(&link->lock);
 
 	/*
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 08/10] PM core: Fix restoring devices with links during system PM transition
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
                   ` (6 preceding siblings ...)
  2016-06-17  6:26 ` [PATCH v2 07/10] driver core: Add support for links to already probed drivers Marek Szyprowski
@ 2016-06-17  6:26 ` Marek Szyprowski
  2016-09-06 23:24   ` Rafael J. Wysocki
  2016-06-17  6:26 ` [PATCH v2 09/10] iommu/exynos: Remove excessive, useless debug Marek Szyprowski
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman

When devices are being runtime resumed during the system PM transition to
suspend state, the link suppliers might be already resumed and
have runtime pm disabled. This is normal case. This patch adds special
support for such case. Simple call to pm_runtime_get_syncreturns error
when device has runtime PM disabled, what results in incorrect runtime PM
state during system wide PM transition.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/base/power/runtime.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 0ea00d442e0f..d00b079fad88 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -261,6 +261,26 @@ static int rpm_check_suspend_allowed(struct device *dev)
 }
 
 /**
+ * __pm_runtime_force_resume - force resume of the device
+ * @dev: Device to resume
+ *
+ * This function works like __pm_runtime_resume(dev, RPM_GET_PUT), but
+ * also handles devices with runtime PM disabled. This allows to properly
+ * control all devices during preparation for system PM transition.
+ */
+static int __pm_runtime_force_resume(struct device *dev)
+{
+	if (!dev->power.disable_depth)
+		return pm_runtime_get_sync(dev);
+
+	if (dev->power.runtime_status != RPM_ACTIVE ||
+	    atomic_inc_not_zero(&dev->power.usage_count) == 0)
+		return -EINVAL;
+
+	return 0;
+}
+
+/**
  * __rpm_callback - Run a given runtime PM callback for a given device.
  * @cb: Runtime PM callback to run.
  * @dev: Device to run the callback for.
@@ -291,7 +311,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 				if ((link->flags & DEVICE_LINK_PM_RUNTIME)
 				    && link->status != DEVICE_LINK_SUPPLIER_UNBIND
 				    && !link->rpm_active) {
-					retval = pm_runtime_get_sync(link->supplier);
+					retval = __pm_runtime_force_resume(link->supplier);
 					if (retval < 0) {
 						pm_runtime_put_noidle(link->supplier);
 						goto fail;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 09/10] iommu/exynos: Remove excessive, useless debug
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
                   ` (7 preceding siblings ...)
  2016-06-17  6:26 ` [PATCH v2 08/10] PM core: Fix restoring devices with links during system PM transition Marek Szyprowski
@ 2016-06-17  6:26 ` Marek Szyprowski
  2016-06-17  6:27 ` [PATCH v2 10/10] iommu/exynos: Add proper runtime pm support Marek Szyprowski
  2016-07-14 15:41 ` [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Tobias Jakobi
  10 siblings, 0 replies; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:26 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman

Remove excessive, useless debug about skipping TLB invalidation, which
is a normal situation when more aggressive power management is enabled.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/exynos-iommu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 633e6d023c0d..9d1a14f88891 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -574,9 +574,6 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 			sysmmu_unblock(data);
 		}
 		clk_disable(data->clk_master);
-	} else {
-		dev_dbg(data->master,
-			"disabled. Skipping TLB invalidation @ %#x\n", iova);
 	}
 	spin_unlock_irqrestore(&data->lock, flags);
 }
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 10/10] iommu/exynos: Add proper runtime pm support
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
                   ` (8 preceding siblings ...)
  2016-06-17  6:26 ` [PATCH v2 09/10] iommu/exynos: Remove excessive, useless debug Marek Szyprowski
@ 2016-06-17  6:27 ` Marek Szyprowski
  2016-07-14 15:41 ` [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Tobias Jakobi
  10 siblings, 0 replies; 36+ messages in thread
From: Marek Szyprowski @ 2016-06-17  6:27 UTC (permalink / raw)
  To: linux-pm, linux-kernel, iommu, linux-samsung-soc, linux-arm-kernel
  Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Rafael J. Wysocki, Ulf Hansson, Mark Brown, Greg Kroah-Hartman

This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
active its master's device is active and can save/restore its state
instead of being enabled all the time. This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/exynos-iommu.c | 218 ++++++++++++++++++++-----------------------
 1 file changed, 99 insertions(+), 119 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 9d1a14f88891..80b7f1e7268c 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -233,8 +233,8 @@ struct sysmmu_drvdata {
 	struct clk *aclk;		/* SYSMMU's aclk clock */
 	struct clk *pclk;		/* SYSMMU's pclk clock */
 	struct clk *clk_master;		/* master's device clock */
-	int activations;		/* number of calls to sysmmu_enable */
 	spinlock_t lock;		/* lock for modyfying state */
+	int active;			/* current status */
 	struct exynos_iommu_domain *domain; /* domain we belong to */
 	struct list_head domain_node;	/* node for domain clients list */
 	struct list_head owner_node;	/* node for owner controllers list */
@@ -247,25 +247,6 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
 	return container_of(dom, struct exynos_iommu_domain, domain);
 }
 
-static bool set_sysmmu_active(struct sysmmu_drvdata *data)
-{
-	/* return true if the System MMU was not active previously
-	   and it needs to be initialized */
-	return ++data->activations == 1;
-}
-
-static bool set_sysmmu_inactive(struct sysmmu_drvdata *data)
-{
-	/* return true if the System MMU is needed to be disabled */
-	BUG_ON(data->activations < 1);
-	return --data->activations == 0;
-}
-
-static bool is_sysmmu_active(struct sysmmu_drvdata *data)
-{
-	return data->activations > 0;
-}
-
 static void sysmmu_unblock(struct sysmmu_drvdata *data)
 {
 	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
@@ -384,7 +365,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 	unsigned short reg_status, reg_clear;
 	int ret = -ENOSYS;
 
-	WARN_ON(!is_sysmmu_active(data));
+	WARN_ON(!data->active);
 
 	if (MMU_MAJ_VER(data->version) < 5) {
 		reg_status = REG_INT_STATUS;
@@ -430,7 +411,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data)
+static void __sysmmu_disable(struct sysmmu_drvdata *data)
 {
 	clk_enable(data->clk_master);
 
@@ -440,32 +421,6 @@ static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data)
 	__sysmmu_disable_clocks(data);
 }
 
-static bool __sysmmu_disable(struct sysmmu_drvdata *data)
-{
-	bool disabled;
-	unsigned long flags;
-
-	spin_lock_irqsave(&data->lock, flags);
-
-	disabled = set_sysmmu_inactive(data);
-
-	if (disabled) {
-		data->pgtable = 0;
-		data->domain = NULL;
-
-		__sysmmu_disable_nocount(data);
-
-		dev_dbg(data->sysmmu, "Disabled\n");
-	} else  {
-		dev_dbg(data->sysmmu, "%d times left to disable\n",
-					data->activations);
-	}
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return disabled;
-}
-
 static void __sysmmu_init_config(struct sysmmu_drvdata *data)
 {
 	unsigned int cfg;
@@ -480,7 +435,7 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data)
 	writel(cfg, data->sfrbase + REG_MMU_CFG);
 }
 
-static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
+static void __sysmmu_enable(struct sysmmu_drvdata *data)
 {
 	__sysmmu_enable_clocks(data);
 
@@ -501,34 +456,6 @@ static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
 	clk_disable(data->clk_master);
 }
 
-static int __sysmmu_enable(struct sysmmu_drvdata *data, phys_addr_t pgtable,
-			   struct exynos_iommu_domain *domain)
-{
-	int ret = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&data->lock, flags);
-	if (set_sysmmu_active(data)) {
-		data->pgtable = pgtable;
-		data->domain = domain;
-
-		__sysmmu_enable_nocount(data);
-
-		dev_dbg(data->sysmmu, "Enabled\n");
-	} else {
-		ret = (pgtable == data->pgtable) ? 1 : -EBUSY;
-
-		dev_dbg(data->sysmmu, "already enabled\n");
-	}
-
-	if (WARN_ON(ret < 0))
-		set_sysmmu_inactive(data); /* decrement count */
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return ret;
-}
-
 static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data,
 					    sysmmu_iova_t iova)
 {
@@ -536,7 +463,7 @@ static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data,
 
 
 	spin_lock_irqsave(&data->lock, flags);
-	if (is_sysmmu_active(data) && data->version >= MAKE_MMU_VER(3, 3)) {
+	if (data->active && data->version >= MAKE_MMU_VER(3, 3)) {
 		clk_enable(data->clk_master);
 		__sysmmu_tlb_invalidate_entry(data, iova, 1);
 		clk_disable(data->clk_master);
@@ -551,7 +478,7 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 	unsigned long flags;
 
 	spin_lock_irqsave(&data->lock, flags);
-	if (is_sysmmu_active(data)) {
+	if (data->active) {
 		unsigned int num_inv = 1;
 
 		clk_enable(data->clk_master);
@@ -652,40 +579,78 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 	}
 
 	pm_runtime_enable(dev);
-
 	of_iommu_set_ops(dev->of_node, &exynos_iommu_ops);
 
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
+static int exynos_sysmmu_runtime_suspend(struct device *dev)
+{
+	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&data->lock, flags);
+	if (data->master) {
+		dev_dbg(data->sysmmu, "runtime pm: saving state\n");
+		__sysmmu_disable(data);
+		data->active = false;
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
+static int exynos_sysmmu_runtime_resume(struct device *dev)
+{
+	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&data->lock, flags);
+	if (data->master) {
+		dev_dbg(data->sysmmu, "runtime pm: restoring state\n");
+		__sysmmu_enable(data);
+		data->active = true;
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
 static int exynos_sysmmu_suspend(struct device *dev)
 {
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	unsigned long flags;
 
-	dev_dbg(dev, "suspend\n");
-	if (is_sysmmu_active(data)) {
-		__sysmmu_disable_nocount(data);
-		pm_runtime_put(dev);
+	spin_lock_irqsave(&data->lock, flags);
+	if (data->active) {
+		dev_dbg(data->sysmmu, "saving state\n");
+		__sysmmu_disable(data);
 	}
+	spin_unlock_irqrestore(&data->lock, flags);
+
 	return 0;
 }
 
 static int exynos_sysmmu_resume(struct device *dev)
 {
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	unsigned long flags;
 
-	dev_dbg(dev, "resume\n");
-	if (is_sysmmu_active(data)) {
-		pm_runtime_get_sync(dev);
-		__sysmmu_enable_nocount(data);
+	spin_lock_irqsave(&data->lock, flags);
+	if (data->active) {
+		dev_dbg(data->sysmmu, "restoring state\n");
+		__sysmmu_enable(data);
 	}
+	spin_unlock_irqrestore(&data->lock, flags);
+
 	return 0;
 }
-#endif
 
 static const struct dev_pm_ops sysmmu_pm_ops = {
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume)
+	SET_RUNTIME_PM_OPS(exynos_sysmmu_runtime_suspend,
+			   exynos_sysmmu_runtime_resume, NULL)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend,
+				     exynos_sysmmu_resume)
 };
 
 static const struct of_device_id sysmmu_of_match[] __initconst = {
@@ -789,9 +754,16 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
 	spin_lock_irqsave(&domain->lock, flags);
 
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
-		if (__sysmmu_disable(data))
-			data->master = NULL;
+		spin_lock(&data->lock);
+		if (WARN_ON(data->active)) {
+			__sysmmu_disable(data);
+			data->active = false;
+		}
+		data->master = NULL;
+		data->pgtable = 0;
+		data->domain = NULL;
 		list_del_init(&data->domain_node);
+		spin_unlock(&data->lock);
 	}
 
 	spin_unlock_irqrestore(&domain->lock, flags);
@@ -832,18 +804,22 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 
 	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
+		spin_lock(&data->lock);
 		if (data->master == dev) {
-			if (__sysmmu_disable(data)) {
-				data->master = NULL;
-				list_del_init(&data->domain_node);
-			}
-			pm_runtime_put(data->sysmmu);
 			found = true;
+			if (data->active) {
+				__sysmmu_disable(data);
+				data->active = false;
+			}
+			data->master = NULL;
+			data->pgtable = 0;
+			data->domain = NULL;
+			list_del_init(&data->domain_node);
 		}
+		spin_unlock(&data->lock);
 	}
-	spin_unlock_irqrestore(&domain->lock, flags);
-
 	owner->domain = NULL;
+	spin_unlock_irqrestore(&domain->lock, flags);
 
 	if (found)
 		dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n",
@@ -860,7 +836,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	struct sysmmu_drvdata *data;
 	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
 	unsigned long flags;
-	int ret = -ENODEV;
 
 	if (!has_sysmmu(dev))
 		return -ENODEV;
@@ -868,29 +843,26 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	if (owner->domain)
 		exynos_iommu_detach_device(owner->domain, dev);
 
+	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry(data, &owner->controllers, owner_node) {
-		pm_runtime_get_sync(data->sysmmu);
-		ret = __sysmmu_enable(data, pagetable, domain);
-		if (ret >= 0) {
-			data->master = dev;
-
-			spin_lock_irqsave(&domain->lock, flags);
-			list_add_tail(&data->domain_node, &domain->clients);
-			spin_unlock_irqrestore(&domain->lock, flags);
+		spin_lock(&data->lock);
+		data->master = dev;
+		data->pgtable = pagetable;
+		data->domain = domain;
+		list_add_tail(&data->domain_node, &domain->clients);
+		if (pm_runtime_active(data->sysmmu)) {
+			__sysmmu_enable(data);
+			data->active = true;
 		}
+		spin_unlock(&data->lock);
 	}
-
-	if (ret < 0) {
-		dev_err(dev, "%s: Failed to attach IOMMU with pgtable %pa\n",
-					__func__, &pagetable);
-		return ret;
-	}
-
 	owner->domain = iommu_domain;
-	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa %s\n",
-		__func__, &pagetable, (ret == 0) ? "" : ", again");
+	spin_unlock_irqrestore(&domain->lock, flags);
 
-	return ret;
+	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n",
+		__func__, &pagetable);
+
+	return 0;
 }
 
 static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain,
@@ -1265,6 +1237,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
 	}
 
 	list_add_tail(&data->owner_node, &owner->controllers);
+
+	/*
+	 * SYSMMU will be runtime enabled via device link (dependency) to its
+	 * master device, so there are no direct calls to pm_runtime_get/put
+	 * in this driver
+	 */
+	device_link_add(dev, &sysmmu->dev, DEVICE_LINK_PERSISTENT |
+					   DEVICE_LINK_PM_RUNTIME);
 	return 0;
 }
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-06-17  6:26 ` [PATCH v2 02/10] driver core: Functional dependencies tracking support Marek Szyprowski
@ 2016-06-17 10:36   ` Lukas Wunner
  2016-06-17 12:54     ` Rafael J. Wysocki
  0 siblings, 1 reply; 36+ messages in thread
From: Lukas Wunner @ 2016-06-17 10:36 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-pm, linux-kernel, iommu, linux-samsung-soc,
	linux-arm-kernel, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andreas Noever

Hi Marek,

On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> Currently, there is a problem with handling cases where functional
> dependencies between devices are involved.
> 
> What I mean by a "functional dependency" is when the driver of device
> B needs both device A and its driver to be present and functional to
> be able to work.  This 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.  This also has certain consequences
> for power management of these devices (suspend/resume and runtime PM
> ordering).
> 
> Add support for representing those functional dependencies between
> devices to allow the driver core to track them and act on them in
> certain cases where they matter.

Rafael has indicated that he intends to respin this series:
https://lkml.org/lkml/2016/6/8/1061

We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:

> New links are added by calling device_link_add() which may happen
> either before the consumer device is probed or when probing it, in
> which case the caller needs to ensure that the driver of the
> supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> flag should be passed to device_link_add() to reflect that.

The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.

It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.

Thanks,

Lukas

> 
> Link objects are deleted either explicitly, by calling
> device_link_del() on the link object in question, or automatically,
> when the consumer device is unbound from its driver or when one
> of the target devices is deleted, depending on the link type.
> 
> There are two types of link objects, persistent and non-persistent.
> The difference between them is that the persistent links stay around
> until one of the target devices is deleted, which the non-persistent
> ones are deleted when the consumer driver is unbound from its device
> (ie. they are assumed to be valid only as long as the consumer device
> has a driver bound to it).  The DEVICE_LINK_PERSISTENT flag has to
> be passed to device_link_add() so as to create a persistent link.
> 
> 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 the all of the supplier
> and consumer devices.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/base/base.h    |  11 ++
>  drivers/base/core.c    | 386 +++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/base/dd.c      |  42 +++++-
>  include/linux/device.h |  36 +++++
>  4 files changed, 470 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index e05db388bd1c..cccb1d211541 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -107,6 +107,9 @@ extern void bus_remove_device(struct device *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);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 0a8bdade53f2..416341df3268 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -44,6 +44,367 @@ static int __init sysfs_deprecated_setup(char *arg)
>  early_param("sysfs.deprecated", sysfs_deprecated_setup);
>  #endif
>  
> +/* Device links support. */
> +
> +DEFINE_STATIC_SRCU(device_links_srcu);
> +static DEFINE_MUTEX(device_links_lock);
> +
> +static int device_reorder_to_tail(struct device *dev, void *not_used)
> +{
> +	struct devlink *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->consumer_links, c_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.
> + * @flags: Link flags.
> + *
> + * At least one of the flags must be set.  If DEVICE_LINK_PROBE_TIME is set, the
> + * caller is expected to know that (a) the supplier device is present and active
> + * (ie. its driver is functional) and (b) the consumer device is probing at the
> + * moment and therefore the initial state of the link will be "consumer probe"
> + * in that case.  If DEVICE_LINK_PROBE_TIME is not set, DEVICE_LINK_PERSISTENT
> + * must be set (meaning that the link will not go away when the consumer driver
> + * goes away).
> + *
> + * 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 those lists.
> + */
> +struct devlink *device_link_add(struct device *consumer,
> +				struct device *supplier, u32 flags)
> +{
> +	struct devlink *link;
> +
> +	if (!consumer || !supplier || !flags)
> +		return NULL;
> +
> +	mutex_lock(&device_links_lock);
> +
> +	list_for_each_entry(link, &supplier->supplier_links, 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);
> +	link->flags = flags;
> +	link->status = (flags & DEVICE_LINK_PROBE_TIME) ?
> +			DEVICE_LINK_CONSUMER_PROBE : DEVICE_LINK_DORMANT;
> +	spin_lock_init(&link->lock);
> +
> +	/*
> +	 * Move the consumer and all of the devices depending on it to the end
> +	 * of dpm_list and the devices_kset list.
> +	 *
> +	 * We have 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->supplier_links);
> +	list_add_tail_rcu(&link->c_node, &consumer->consumer_links);
> +
> +	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 __devlink_free_srcu(struct rcu_head *rhead)
> +{
> +	struct devlink *link;
> +
> +	link = container_of(rhead, struct devlink, rcu_head);
> +	put_device(link->consumer);
> +	put_device(link->supplier);
> +	kfree(link);
> +}
> +
> +static void devlink_del(struct devlink *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, __devlink_free_srcu);
> +}
> +
> +/**
> + * device_link_del - Delete a link between two devices.
> + * @link: Device link to delete.
> + */
> +void device_link_del(struct devlink *link)
> +{
> +	mutex_lock(&device_links_lock);
> +	devlink_del(link);
> +	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 devlink *link;
> +
> +	list_for_each_entry_rcu(link, &dev->consumer_links, 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).
> + */
> +int device_links_check_suppliers(struct device *dev)
> +{
> +	struct devlink *link;
> +	int idx, ret = 0;
> +
> +	idx = device_links_read_lock();
> +
> +	list_for_each_entry_rcu(link, &dev->consumer_links, c_node) {
> +		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".
> + */
> +void device_links_driver_bound(struct device *dev)
> +{
> +	struct devlink *link;
> +	int idx;
> +
> +	idx = device_links_read_lock();
> +
> +	list_for_each_entry_rcu(link, &dev->supplier_links, s_node) {
> +		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->consumer_links, c_node) {
> +		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".
> + */
> +void device_links_driver_gone(struct device *dev)
> +{
> +	struct devlink *link;
> +	int idx;
> +
> +	idx = device_links_read_lock();
> +
> +	list_for_each_entry_rcu(link, &dev->supplier_links, s_node) {
> +		WARN_ON(!(link->flags & DEVICE_LINK_PERSISTENT));
> +		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.
> + */
> +void device_links_no_driver(struct device *dev)
> +{
> +	struct devlink *link, *ln;
> +
> +	mutex_lock(&device_links_lock);
> +
> +	list_for_each_entry_safe_reverse(link, ln, &dev->consumer_links, c_node)
> +		if (link->flags & DEVICE_LINK_PERSISTENT) {
> +			spin_lock(&link->lock);
> +
> +			if (link->status != DEVICE_LINK_SUPPLIER_UNBIND)
> +				link->status = DEVICE_LINK_AVAILABLE;
> +
> +			spin_unlock(&link->lock);
> +		} else {
> +			devlink_del(link);
> +		}
> +
> +	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' it 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.
> + */
> +bool device_links_busy(struct device *dev)
> +{
> +	struct devlink *link;
> +	int idx;
> +	bool ret = false;
> +
> +	idx = device_links_read_lock();
> +
> +	list_for_each_entry_rcu(link, &dev->supplier_links, s_node) {
> +		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).
> + */
> +void device_links_unbind_consumers(struct device *dev)
> +{
> +	struct devlink *link;
> +	int idx;
> +
> + start:
> +	idx = device_links_read_lock();
> +
> +	list_for_each_entry_rcu(link, &dev->supplier_links, s_node) {
> +		enum devlink_status status;
> +
> +		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 +1072,8 @@ void device_initialize(struct device *dev)
>  #ifdef CONFIG_GENERIC_MSI_IRQ
>  	INIT_LIST_HEAD(&dev->msi_list);
>  #endif
> +	INIT_LIST_HEAD(&dev->supplier_links);
> +	INIT_LIST_HEAD(&dev->consumer_links);
>  }
>  EXPORT_SYMBOL_GPL(device_initialize);
>  
> @@ -1233,6 +1596,7 @@ void device_del(struct device *dev)
>  {
>  	struct device *parent = dev->parent;
>  	struct class_interface *class_intf;
> +	struct devlink *link, *ln;
>  
>  	/* Notify clients of device removal.  This call must come
>  	 * before dpm_sysfs_remove().
> @@ -1240,6 +1604,28 @@ 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->consumer_links, c_node) {
> +		WARN_ON(link->status != DEVICE_LINK_DORMANT);
> +		devlink_del(link);
> +	}
> +
> +	list_for_each_entry_safe_reverse(link, ln, &dev->supplier_links, s_node) {
> +		WARN_ON(link->status != DEVICE_LINK_DORMANT);
> +		devlink_del(link);
> +	}
> +
> +	mutex_unlock(&device_links_lock);
> +
>  	dpm_sysfs_remove(dev);
>  	if (parent)
>  		klist_del(&dev->p->knode_parent);
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index d9e76e9205c7..7c0abeba89e9 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -249,6 +249,7 @@ static void driver_bound(struct device *dev)
>  		 __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_driver *drv, struct device *dev)
>  	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(struct device *dev)
>  		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(struct device *dev)
>  			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(struct device *dev)
>  	}
>  }
>  
> -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_internal(struct device *dev,
>   *
>   * 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)
>  {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 38f02814d53a..647204bd74a0 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -706,6 +706,34 @@ struct device_dma_parameters {
>  	unsigned long segment_boundary_mask;
>  };
>  
> +enum devlink_status {
> +	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.
> + *
> + * PERSISTENT: Do not delete the link on consumer device driver unbind.
> + * PROBE_TIME: Assume supplier device functional when creating the link.
> + */
> +#define DEVICE_LINK_PERSISTENT	(1 << 0)
> +#define DEVICE_LINK_PROBE_TIME	(1 << 1)
> +
> +struct devlink {
> +	struct device *supplier;
> +	struct list_head s_node;
> +	struct device *consumer;
> +	struct list_head c_node;
> +	enum devlink_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 +759,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.
> + * @supplier_links: Links to consumer devices.
> + * @consumer_links: 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 +827,8 @@ struct device {
>  					   core doesn't touch it */
>  	void		*driver_data;	/* Driver data, set and get with
>  					   dev_set/get_drvdata */
> +	struct list_head	supplier_links;
> +	struct list_head	consumer_links;
>  	struct dev_pm_info	power;
>  	struct dev_pm_domain	*pm_domain;
>  
> @@ -1113,6 +1145,10 @@ extern void device_shutdown(void);
>  /* debugging and troubleshooting/diagnostic helpers. */
>  extern const char *dev_driver_string(const struct device *dev);
>  
> +/* Device links interface. */
> +struct devlink *device_link_add(struct device *consumer,
> +				struct device *supplier, u32 flags);
> +void device_link_del(struct devlink *link);
>  
>  #ifdef CONFIG_PRINTK
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-06-17 10:36   ` Lukas Wunner
@ 2016-06-17 12:54     ` Rafael J. Wysocki
  2016-06-17 14:07       ` Lukas Wunner
  0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-06-17 12:54 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andreas Noever

On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Marek,
>
> On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
>> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>>
>> Currently, there is a problem with handling cases where functional
>> dependencies between devices are involved.
>>
>> What I mean by a "functional dependency" is when the driver of device
>> B needs both device A and its driver to be present and functional to
>> be able to work.  This 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.  This also has certain consequences
>> for power management of these devices (suspend/resume and runtime PM
>> ordering).
>>
>> Add support for representing those functional dependencies between
>> devices to allow the driver core to track them and act on them in
>> certain cases where they matter.
>
> Rafael has indicated that he intends to respin this series:
> https://lkml.org/lkml/2016/6/8/1061

That's OK.

> We also have such a functional dependency for Thunderbolt on Macs:
> On resume from system sleep, the PCIe hotplug ports may not resume
> before the thunderbolt driver has reestablished the PCI tunnels.
> Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> in drivers/pci/quirks.c. It would be good if we could represent
> this dependency using something like Rafael's approach instead of
> open coding it, however one detail in Rafael's patches is problematic:
>
>> New links are added by calling device_link_add() which may happen
>> either before the consumer device is probed or when probing it, in
>> which case the caller needs to ensure that the driver of the
>> supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
>> flag should be passed to device_link_add() to reflect that.
>
> The thunderbolt driver cannot call device_link_add() before the
> PCIe hotplug ports are bound to a driver unless we amend portdrv
> to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> if the thunderbolt driver isn't loaded.
>
> It would therefore be beneficial if device_link_add() can be
> called even *after* the consumer is bound.

I don't quite follow.

Who's the provider and who's the consumer here?

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-06-17 12:54     ` Rafael J. Wysocki
@ 2016-06-17 14:07       ` Lukas Wunner
  2016-07-20  0:33         ` Rafael J. Wysocki
  0 siblings, 1 reply; 36+ messages in thread
From: Lukas Wunner @ 2016-06-17 14:07 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andreas Noever, linux-pci

On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > We also have such a functional dependency for Thunderbolt on Macs:
> > On resume from system sleep, the PCIe hotplug ports may not resume
> > before the thunderbolt driver has reestablished the PCI tunnels.
> > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > in drivers/pci/quirks.c. It would be good if we could represent
> > this dependency using something like Rafael's approach instead of
> > open coding it, however one detail in Rafael's patches is problematic:
> >
> > > New links are added by calling device_link_add() which may happen
> > > either before the consumer device is probed or when probing it, in
> > > which case the caller needs to ensure that the driver of the
> > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > flag should be passed to device_link_add() to reflect that.
> >
> > The thunderbolt driver cannot call device_link_add() before the
> > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > if the thunderbolt driver isn't loaded.
> >
> > It would therefore be beneficial if device_link_add() can be
> > called even *after* the consumer is bound.
> 
> I don't quite follow.
> 
> Who's the provider and who's the consumer here?

thunderbolt.ko is the supplier. The PCIe hotplug ports of Thunderbolt
host controllers are the consumers.

If thunderbolt.ko is not loaded, no special precautions are needed for
the hotplug ports.

However *if* it is loaded and the system is suspended and there are
Thunderbolt devices plugged in, then on resume the hotplug ports must
wait for thunderbolt.ko to reestablish the PCI tunnels. If they do not
wait, things will go south because when pci_pm_resume_noirq() is called
for the devices *below* the hotplug port, we will try to put them into D0
(via pci_pm_default_resume_early()) and call pci_restore_state() even
though those hotplugged devices are not yet reachable.

So what I'd like to do is call device_link_add() when thunderbolt.ko
is loaded to shove a dependency onto the hotplug ports. But the hotplug
ports will long since have been bound at that point (to portdrv).
So it should be allowed to call device_link_add() ex post facto, after
the consumer has been bound.

Let me know if I still failed to convey the problem in an intelligible
way.

I had a similar problem with the Runtime PM for Thunderbolt series
(which BTW is still awaiting reviews):
http://www.spinics.net/lists/linux-pci/msg51158.html

The PM core allows calls to dev_pm_domain_set() only during ->probe or
if the device is unbound. Same constraint as with device_link_add().
This constraint was a major obstacle for me and I had to jump through
numerous additional hoops to work around it:

Thunderbolt controllers on Macs can be put into D3cold, but not with
standard ACPI methods, so platform_pci_power_manageable() is false for
these devices. Now this nothing unusual, the same applies to dual GPU
laptops (custom ACPI DSMs for Nvidia Optimus and AMD PowerXpress).

For such discrete GPUs, a struct dev_pm_domain is set during ->probe
(drivers/gpu/vga/vga_switcheroo.c: vga_switcheroo_init_domain_pm_ops()).
But I can't do that for Thunderbolt because the device in question is
a PCIe upstream port. Ideally I would shove a dev_pm_domain on the
upstream port when thunderbolt.ko loads, but again at that point
the port has long since been bound (to portdrv) so I can't call
dev_pm_domain_set().

The workaround I settled on is to attach to the upstream port as a
service driver, call down to the service driver's ->runtime_suspend
hooks when the port runtime suspends and power the controller down.
In addition, I had to make sure that saved_state isn't clobbered on
resume (patch [09/13]).

It would be good if someone who has a bird's eye view of the PM core
(i.e., you) could make a decision
(1) if it's possible and worthwhile to allow dev_pm_domain_set() for
    already bound devices, and
(2) if the PCI core should be be able to deal with devices which are
    runtime suspended to D3cold but not by the platform (because that's
    what patch [09/13] does).

If the answer to (2) is yes then there's some more stuff to fix:
Discrete GPUs on dual GPU laptops can be manually suspended to D3cold
behind the PM core's back by loading nouveau / radeon / amdgpu with
runpm=0 and writing OFF to the vga_switcheroo debugfs interface.
This is currently broken in conjunction with system sleep (the GPU
is no longer accessible after resume) because pci_pm_resume_noirq()
calls pci_restore_state() even though the device is in D3cold and not
power-manageable by the platform.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies)
  2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
                   ` (9 preceding siblings ...)
  2016-06-17  6:27 ` [PATCH v2 10/10] iommu/exynos: Add proper runtime pm support Marek Szyprowski
@ 2016-07-14 15:41 ` Tobias Jakobi
  2016-07-15 13:21   ` Tobias Jakobi
  10 siblings, 1 reply; 36+ messages in thread
From: Tobias Jakobi @ 2016-07-14 15:41 UTC (permalink / raw)
  To: Marek Szyprowski, linux-pm, linux-kernel, iommu,
	linux-samsung-soc, linux-arm-kernel
  Cc: Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

Hello Marek,

I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
my ODROID-X2.

Going to check where exactly things break.

With best wishes,
Tobias


Marek Szyprowski wrote:
> Hello,
> 
> This patch series finally implements proper runtime PM support in Exynos
> IOMMU driver. This has been achieved by using device links, which lets
> SYSMMU controller's runtime PM to follow master's device runtime PM (the
> device which actually performs DMA transaction). The main idea
> behind this solution is an observation that any DMA activity from master
> device can be done only when master device is active, thus when master
> device is suspended SYSMMU controller device can also be suspended.
> 
> This patchset solves the situation that power domains are always enabled,
> because all SYSMMU controllers (which belongs to those domains) are
> permanently active (because existing driver was simplified and kept
> SYSMMU device active all the time after initialization).
> 
> Patches 1-5 are resend of the "[RFC][PATCH 0/5] Functional dependencies
> between devices" patchset:
> http://thread.gmane.org/gmane.linux.power-management.general/67424/focus=2126379
> I've included them here, because it is hard to find them all on mailing
> list archives.
> 
> Patches 6-8 are fixes to device dependencies/links code, which were
> required to use this solution for Exynos IOMMU driver. I'm not PM/runtime
> PM code expert, so please double check if my changes are really correct.
> 
> This patchset requires my previous changes to Exynos IOMMU driver
> submitted in the "Exynos IOMMU: improve clock management" thread:
> http://www.spinics.net/lists/arm-kernel/msg505695.html
> 
> Best regards
> Marek Szyprowski
> Samsung R&D Institute Poland
> 
> 
> Changelog:
> v2:
> - replaced PM notifiers with generic device dependencies/links developped
>   by Rafael J. Wysocki
> 
> v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
> - initial version
> 
> 
> Patch summary:
> 
> Marek Szyprowski (5):
>   driver core: Avoid endless recursion if device has more than one link
>   driver core: Add support for links to already probed drivers
>   PM core: Fix restoring devices with links during system PM transition
>   iommu/exynos: Remove excessive, useless debug
>   iommu/exynos: Add proper runtime pm support
> 
> Rafael J. Wysocki (5):
>   driver core: Add a wrapper around __device_release_driver()
>   driver core: Functional dependencies tracking support
>   PM core: Make async suspend/resume of devices use device links
>   PM core: Make runtime PM of devices use device links
>   PM core: Optimize the use of device links for runtime PM
> 
>  drivers/base/base.h          |  13 ++
>  drivers/base/core.c          | 410 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/base/dd.c            |  65 +++++--
>  drivers/base/power/main.c    |  68 ++++++-
>  drivers/base/power/runtime.c | 130 +++++++++++++-
>  drivers/iommu/exynos-iommu.c | 221 +++++++++++------------
>  include/linux/device.h       |  41 +++++
>  include/linux/pm.h           |   1 +
>  include/linux/pm_runtime.h   |   6 +
>  9 files changed, 809 insertions(+), 146 deletions(-)
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies)
  2016-07-14 15:41 ` [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Tobias Jakobi
@ 2016-07-15 13:21   ` Tobias Jakobi
  2016-07-18 10:32     ` Marek Szyprowski
  0 siblings, 1 reply; 36+ messages in thread
From: Tobias Jakobi @ 2016-07-15 13:21 UTC (permalink / raw)
  To: Tobias Jakobi, Marek Szyprowski, linux-pm, linux-kernel, iommu,
	linux-samsung-soc, linux-arm-kernel
  Cc: Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

Tobias Jakobi wrote:
> Hello Marek,
> 
> I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
> my ODROID-X2.
> 
> Going to check where exactly things break.
Sadly it's the last patch where everything comes together:
"iommu/exynos: Add proper runtime pm support"

I still have to check if forcing runpm status to 'on' makes a
difference. I suspect that the aggressive clock gating might be the reason?

With best wishes,
Tobias

> With best wishes,
> Tobias
> 
> 
> Marek Szyprowski wrote:
>> Hello,
>>
>> This patch series finally implements proper runtime PM support in Exynos
>> IOMMU driver. This has been achieved by using device links, which lets
>> SYSMMU controller's runtime PM to follow master's device runtime PM (the
>> device which actually performs DMA transaction). The main idea
>> behind this solution is an observation that any DMA activity from master
>> device can be done only when master device is active, thus when master
>> device is suspended SYSMMU controller device can also be suspended.
>>
>> This patchset solves the situation that power domains are always enabled,
>> because all SYSMMU controllers (which belongs to those domains) are
>> permanently active (because existing driver was simplified and kept
>> SYSMMU device active all the time after initialization).
>>
>> Patches 1-5 are resend of the "[RFC][PATCH 0/5] Functional dependencies
>> between devices" patchset:
>> http://thread.gmane.org/gmane.linux.power-management.general/67424/focus=2126379
>> I've included them here, because it is hard to find them all on mailing
>> list archives.
>>
>> Patches 6-8 are fixes to device dependencies/links code, which were
>> required to use this solution for Exynos IOMMU driver. I'm not PM/runtime
>> PM code expert, so please double check if my changes are really correct.
>>
>> This patchset requires my previous changes to Exynos IOMMU driver
>> submitted in the "Exynos IOMMU: improve clock management" thread:
>> http://www.spinics.net/lists/arm-kernel/msg505695.html
>>
>> Best regards
>> Marek Szyprowski
>> Samsung R&D Institute Poland
>>
>>
>> Changelog:
>> v2:
>> - replaced PM notifiers with generic device dependencies/links developped
>>   by Rafael J. Wysocki
>>
>> v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
>> - initial version
>>
>>
>> Patch summary:
>>
>> Marek Szyprowski (5):
>>   driver core: Avoid endless recursion if device has more than one link
>>   driver core: Add support for links to already probed drivers
>>   PM core: Fix restoring devices with links during system PM transition
>>   iommu/exynos: Remove excessive, useless debug
>>   iommu/exynos: Add proper runtime pm support
>>
>> Rafael J. Wysocki (5):
>>   driver core: Add a wrapper around __device_release_driver()
>>   driver core: Functional dependencies tracking support
>>   PM core: Make async suspend/resume of devices use device links
>>   PM core: Make runtime PM of devices use device links
>>   PM core: Optimize the use of device links for runtime PM
>>
>>  drivers/base/base.h          |  13 ++
>>  drivers/base/core.c          | 410 +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/base/dd.c            |  65 +++++--
>>  drivers/base/power/main.c    |  68 ++++++-
>>  drivers/base/power/runtime.c | 130 +++++++++++++-
>>  drivers/iommu/exynos-iommu.c | 221 +++++++++++------------
>>  include/linux/device.h       |  41 +++++
>>  include/linux/pm.h           |   1 +
>>  include/linux/pm_runtime.h   |   6 +
>>  9 files changed, 809 insertions(+), 146 deletions(-)
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies)
  2016-07-15 13:21   ` Tobias Jakobi
@ 2016-07-18 10:32     ` Marek Szyprowski
  2016-07-18 11:00       ` Tobias Jakobi
  0 siblings, 1 reply; 36+ messages in thread
From: Marek Szyprowski @ 2016-07-18 10:32 UTC (permalink / raw)
  To: Tobias Jakobi, linux-pm, linux-kernel, iommu, linux-samsung-soc,
	linux-arm-kernel
  Cc: Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

Hi Tobias,


On 2016-07-15 15:21, Tobias Jakobi wrote:
> Tobias Jakobi wrote:
>> Hello Marek,
>>
>> I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
>> my ODROID-X2.
>>
>> Going to check where exactly things break.
> Sadly it's the last patch where everything comes together:
> "iommu/exynos: Add proper runtime pm support"
>
> I still have to check if forcing runpm status to 'on' makes a
> difference. I suspect that the aggressive clock gating might be the reason?

Thanks for testing. I will check this issue. Could you send me your .config?

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies)
  2016-07-18 10:32     ` Marek Szyprowski
@ 2016-07-18 11:00       ` Tobias Jakobi
  2016-07-18 13:50         ` Marek Szyprowski
  0 siblings, 1 reply; 36+ messages in thread
From: Tobias Jakobi @ 2016-07-18 11:00 UTC (permalink / raw)
  To: Marek Szyprowski, linux-pm, linux-kernel, iommu,
	linux-samsung-soc, linux-arm-kernel
  Cc: Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

Hello Marek,


Marek Szyprowski wrote:
> Hi Tobias,
> 
> 
> On 2016-07-15 15:21, Tobias Jakobi wrote:
>> Tobias Jakobi wrote:
>>> Hello Marek,
>>>
>>> I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
>>> my ODROID-X2.
>>>
>>> Going to check where exactly things break.
>> Sadly it's the last patch where everything comes together:
>> "iommu/exynos: Add proper runtime pm support"
>>
>> I still have to check if forcing runpm status to 'on' makes a
>> difference. I suspect that the aggressive clock gating might be the
>> reason?
> 
> Thanks for testing. I will check this issue. Could you send me your
> .config?
This is the config I'm currently using:
https://github.com/tobiasjakobi/odroid-environment/blob/master/sourcecode/system/vanilla-4.7-debug.conf

Do you think checking this with no_console_suspend makes sense?

- Tobias


> 
> Best regards

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies)
  2016-07-18 11:00       ` Tobias Jakobi
@ 2016-07-18 13:50         ` Marek Szyprowski
  2016-07-18 16:43           ` Tobias Jakobi
  0 siblings, 1 reply; 36+ messages in thread
From: Marek Szyprowski @ 2016-07-18 13:50 UTC (permalink / raw)
  To: Tobias Jakobi, linux-pm, linux-kernel, iommu, linux-samsung-soc,
	linux-arm-kernel
  Cc: Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

Dear Tobias


On 2016-07-18 13:00, Tobias Jakobi wrote:
> Marek Szyprowski wrote:
>> On 2016-07-15 15:21, Tobias Jakobi wrote:
>>> Tobias Jakobi wrote:
>>>> Hello Marek,
>>>>
>>>> I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
>>>> my ODROID-X2.
>>>>
>>>> Going to check where exactly things break.
>>> Sadly it's the last patch where everything comes together:
>>> "iommu/exynos: Add proper runtime pm support"
>>>
>>> I still have to check if forcing runpm status to 'on' makes a
>>> difference. I suspect that the aggressive clock gating might be the
>>> reason?
>> Thanks for testing. I will check this issue. Could you send me your
>> .config?
> This is the config I'm currently using:
> https://github.com/tobiasjakobi/odroid-environment/blob/master/sourcecode/system/vanilla-4.7-debug.conf
>
> Do you think checking this with no_console_suspend makes sense?

no_console_suspend switch won't provide more information, but I managed 
to reproduce your issue. I'm really confused how enabling runtime pm can 
cause problems with usb/smsc95xx ethernet driver (that is the reason for 
failed reboot). Maybe it is somehow related to the global relations 
between devices and drivers and the fact that creating the runtime pm 
links change the order of operations. I will check this again when 
Rafael send updated patches. Here is the log I got (after waiting some 
time):

# reboot

Broadcast message from root@target (ttySAC1) (Mon Jul 18 13:33:38 2016):

The system is going down for reboot NOW!
INIT: Switching to runlevel: 6
INIT: Sending processes the TERM signal
[info] Using makefile-style concurrent boot in runlevel 6.
[ ok ] Stopping cgroup management proxy daemon: cgproxy[....] Stopping 
internet superserver: inetd.
[....] Stopping cgroup management daemon: cgmanagermax77686-rtc 
max77686-rtc: RTC alarm IRQ: 119
[ ok ] Shutting down ALSA...done.
random: nonblocking pool is initialized
[info] Saving the system clock.
[info] Hardware Clock updated to Mon Jul 18 13:33:40 UTC 2016.
[ ok ] Asking all remaining processes to terminate...done.
[ ok ] All processes ended within 1 seconds...done.
[ ok ] Stopping rpcbind daemon....
[ ok ] Deconfiguring network interfaces...done.
[ ok ] Unmounting temporary filesystems...done.
[ ok ] Deactivating swap...done.
EXT4-fs (mmcblk1p2): re-mounted. Opts: (null)
[info] Will now restart.
smsc95xx 1-2:1.0 eth0: Failed to read reg index 0x00000114: -110
smsc95xx 1-2:1.0 eth0: Error reading MII_ACCESS
smsc95xx 1-2:1.0 eth0: MII is busy in smsc95xx_mdio_read
smsc95xx 1-2:1.0 eth0: Failed to read MII_BMSR
INFO: task kworker/0:1:410 blocked for more than 120 seconds.
       Not tainted 4.7.0-rc7-debug+ #155
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/0:1     D c06850a8     0   410      2 0x00000000
Workqueue: events_freezable mmc_rescan
Backtrace:
[<c0684e90>] (__schedule) from [<c068560c>] (schedule+0x44/0xb0)
  r10:00000000 r9:eebe1d88 r8:c0686234 r7:00000000 r6:00000002 r5:7fffffff
  r4:7fffffff
[<c06855c8>] (schedule) from [<c068a398>] (schedule_timeout+0x154/0x1b0)
[<c068a244>] (schedule_timeout) from [<c0686250>] 
(wait_for_common+0xe0/0x174)
  r8:c0686234 r7:00000000 r6:00000002 r5:eebe1d8c r4:7fffffff
[<c0686170>] (wait_for_common) from [<c0686430>] 
(wait_for_completion+0x18/0x1c)
  r10:00000001 r9:00000000 r8:00000000 r7:eebe1d88 r6:eebe1d78 r5:ee260000
  r4:eebe1de4
[<c0686418>] (wait_for_completion) from [<c04e72c8>] 
(mmc_wait_for_req+0xc8/0x15c)
[<c04e7200>] (mmc_wait_for_req) from [<c04e73c8>] 
(mmc_wait_for_cmd+0x6c/0xa4)
  r8:eef73d00 r7:00000003 r6:ee260000 r5:c0a02448 r4:eebe1de4 r3:00000000
[<c04e735c>] (mmc_wait_for_cmd) from [<c04edbf4>] 
(mmc_send_status+0x8c/0xb0)
  r7:eebe1eb0 r6:ee260000 r5:ee260000 r4:00000000
[<c04edb68>] (mmc_send_status) from [<c04eaf80>] (mmc_alive+0x18/0x1c)
  r4:ee260000
[<c04eaf68>] (mmc_alive) from [<c04e93d0>] 
(_mmc_detect_card_removed+0x40/0x90)
[<c04e9390>] (_mmc_detect_card_removed) from [<c04eba80>] 
(mmc_detect+0x2c/0x78)
  r5:ee2602bc r4:ee260000
[<c04eba54>] (mmc_detect) from [<c04e96ac>] (mmc_rescan+0x1b0/0x324)
  r5:ee2602bc r4:ee260368
[<c04e94fc>] (mmc_rescan) from [<c013a818>] (process_one_work+0x194/0x414)
  r8:eef73d00 r7:eebe1eb0 r6:eef70280 r5:ee260368 r4:eea24080 r3:c04e94fc
[<c013a684>] (process_one_work) from [<c013ab0c>] (worker_thread+0x34/0x4d4)
  r10:eef70280 r9:c0a02100 r8:00000008 r7:eef70280 r6:eea24098 r5:eef702b4
  r4:eea24080
[<c013aad8>] (worker_thread) from [<c0141e18>] (kthread+0xf8/0x11c)
  r10:00000000 r9:00000000 r8:00000000 r7:c013aad8 r6:eea24080 r5:00000000
  r4:eea29d00
[<c0141d20>] (kthread) from [<c0107ff0>] (ret_from_fork+0x14/0x24)
  r7:00000000 r6:00000000 r5:c0141d20 r4:eea29d00
2 locks held by kworker/0:1/410:
  #0:  ("events_freezable"){.+.+.+}, at: [<c013a7ac>] 
process_one_work+0x128/0x414
  #1:  ((&(&host->detect)->work)){+.+.+.}, at: [<c013a7ac>] 
process_one_work+0x128/0x414


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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies)
  2016-07-18 13:50         ` Marek Szyprowski
@ 2016-07-18 16:43           ` Tobias Jakobi
  2016-07-19  6:26             ` Marek Szyprowski
  0 siblings, 1 reply; 36+ messages in thread
From: Tobias Jakobi @ 2016-07-18 16:43 UTC (permalink / raw)
  To: Marek Szyprowski, linux-pm, linux-kernel, iommu,
	linux-samsung-soc, linux-arm-kernel
  Cc: Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

Hey Marek,


Marek Szyprowski wrote:
> Dear Tobias
> 
> 
> On 2016-07-18 13:00, Tobias Jakobi wrote:
>> Marek Szyprowski wrote:
>>> On 2016-07-15 15:21, Tobias Jakobi wrote:
>>>> Tobias Jakobi wrote:
>>>>> Hello Marek,
>>>>>
>>>>> I've tested the patchset on 4.7-rc7 and noticed that it breaks
>>>>> reboot on
>>>>> my ODROID-X2.
>>>>>
>>>>> Going to check where exactly things break.
>>>> Sadly it's the last patch where everything comes together:
>>>> "iommu/exynos: Add proper runtime pm support"
>>>>
>>>> I still have to check if forcing runpm status to 'on' makes a
>>>> difference. I suspect that the aggressive clock gating might be the
>>>> reason?
>>> Thanks for testing. I will check this issue. Could you send me your
>>> .config?
>> This is the config I'm currently using:
>> https://github.com/tobiasjakobi/odroid-environment/blob/master/sourcecode/system/vanilla-4.7-debug.conf
>>
>>
>> Do you think checking this with no_console_suspend makes sense?
> 
> no_console_suspend switch won't provide more information, but I managed
> to reproduce your issue. I'm really confused how enabling runtime pm can
> cause problems with usb/smsc95xx ethernet driver (that is the reason for
> failed reboot). Maybe it is somehow related to the global relations
> between devices and drivers and the fact that creating the runtime pm
> links change the order of operations. I will check this again when
> Rafael send updated patches. Here is the log I got (after waiting some
> time):
thanks for looking into this! I'll try to reproduce this on my board. I
have to admit that I didn't wait too long for the hung task message to
appear.

I wonder if this has something to do with regulator code cutting some
supplies too early. Is this on a X2 or a U2/U3? I'm not sure if we
currently model the regulator setup correctly here (IIRC then buck8 is
supplying the LAN/USB block on U2/U3).


- Tobias


> 
> # reboot
> 
> Broadcast message from root@target (ttySAC1) (Mon Jul 18 13:33:38 2016):
> 
> The system is going down for reboot NOW!
> INIT: Switching to runlevel: 6
> INIT: Sending processes the TERM signal
> [info] Using makefile-style concurrent boot in runlevel 6.
> [ ok ] Stopping cgroup management proxy daemon: cgproxy[....] Stopping
> internet superserver: inetd.
> [....] Stopping cgroup management daemon: cgmanagermax77686-rtc
> max77686-rtc: RTC alarm IRQ: 119
> [ ok ] Shutting down ALSA...done.
> random: nonblocking pool is initialized
> [info] Saving the system clock.
> [info] Hardware Clock updated to Mon Jul 18 13:33:40 UTC 2016.
> [ ok ] Asking all remaining processes to terminate...done.
> [ ok ] All processes ended within 1 seconds...done.
> [ ok ] Stopping rpcbind daemon....
> [ ok ] Deconfiguring network interfaces...done.
> [ ok ] Unmounting temporary filesystems...done.
> [ ok ] Deactivating swap...done.
> EXT4-fs (mmcblk1p2): re-mounted. Opts: (null)
> [info] Will now restart.
> smsc95xx 1-2:1.0 eth0: Failed to read reg index 0x00000114: -110
> smsc95xx 1-2:1.0 eth0: Error reading MII_ACCESS
> smsc95xx 1-2:1.0 eth0: MII is busy in smsc95xx_mdio_read
> smsc95xx 1-2:1.0 eth0: Failed to read MII_BMSR
> INFO: task kworker/0:1:410 blocked for more than 120 seconds.
>       Not tainted 4.7.0-rc7-debug+ #155
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/0:1     D c06850a8     0   410      2 0x00000000
> Workqueue: events_freezable mmc_rescan
> Backtrace:
> [<c0684e90>] (__schedule) from [<c068560c>] (schedule+0x44/0xb0)
>  r10:00000000 r9:eebe1d88 r8:c0686234 r7:00000000 r6:00000002 r5:7fffffff
>  r4:7fffffff
> [<c06855c8>] (schedule) from [<c068a398>] (schedule_timeout+0x154/0x1b0)
> [<c068a244>] (schedule_timeout) from [<c0686250>]
> (wait_for_common+0xe0/0x174)
>  r8:c0686234 r7:00000000 r6:00000002 r5:eebe1d8c r4:7fffffff
> [<c0686170>] (wait_for_common) from [<c0686430>]
> (wait_for_completion+0x18/0x1c)
>  r10:00000001 r9:00000000 r8:00000000 r7:eebe1d88 r6:eebe1d78 r5:ee260000
>  r4:eebe1de4
> [<c0686418>] (wait_for_completion) from [<c04e72c8>]
> (mmc_wait_for_req+0xc8/0x15c)
> [<c04e7200>] (mmc_wait_for_req) from [<c04e73c8>]
> (mmc_wait_for_cmd+0x6c/0xa4)
>  r8:eef73d00 r7:00000003 r6:ee260000 r5:c0a02448 r4:eebe1de4 r3:00000000
> [<c04e735c>] (mmc_wait_for_cmd) from [<c04edbf4>]
> (mmc_send_status+0x8c/0xb0)
>  r7:eebe1eb0 r6:ee260000 r5:ee260000 r4:00000000
> [<c04edb68>] (mmc_send_status) from [<c04eaf80>] (mmc_alive+0x18/0x1c)
>  r4:ee260000
> [<c04eaf68>] (mmc_alive) from [<c04e93d0>]
> (_mmc_detect_card_removed+0x40/0x90)
> [<c04e9390>] (_mmc_detect_card_removed) from [<c04eba80>]
> (mmc_detect+0x2c/0x78)
>  r5:ee2602bc r4:ee260000
> [<c04eba54>] (mmc_detect) from [<c04e96ac>] (mmc_rescan+0x1b0/0x324)
>  r5:ee2602bc r4:ee260368
> [<c04e94fc>] (mmc_rescan) from [<c013a818>] (process_one_work+0x194/0x414)
>  r8:eef73d00 r7:eebe1eb0 r6:eef70280 r5:ee260368 r4:eea24080 r3:c04e94fc
> [<c013a684>] (process_one_work) from [<c013ab0c>]
> (worker_thread+0x34/0x4d4)
>  r10:eef70280 r9:c0a02100 r8:00000008 r7:eef70280 r6:eea24098 r5:eef702b4
>  r4:eea24080
> [<c013aad8>] (worker_thread) from [<c0141e18>] (kthread+0xf8/0x11c)
>  r10:00000000 r9:00000000 r8:00000000 r7:c013aad8 r6:eea24080 r5:00000000
>  r4:eea29d00
> [<c0141d20>] (kthread) from [<c0107ff0>] (ret_from_fork+0x14/0x24)
>  r7:00000000 r6:00000000 r5:c0141d20 r4:eea29d00
> 2 locks held by kworker/0:1/410:
>  #0:  ("events_freezable"){.+.+.+}, at: [<c013a7ac>]
> process_one_work+0x128/0x414
>  #1:  ((&(&host->detect)->work)){+.+.+.}, at: [<c013a7ac>]
> process_one_work+0x128/0x414
> 
> 
> Best regards

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies)
  2016-07-18 16:43           ` Tobias Jakobi
@ 2016-07-19  6:26             ` Marek Szyprowski
  2016-07-24 18:02               ` Tobias Jakobi
  0 siblings, 1 reply; 36+ messages in thread
From: Marek Szyprowski @ 2016-07-19  6:26 UTC (permalink / raw)
  To: Tobias Jakobi, linux-pm, linux-kernel, iommu, linux-samsung-soc,
	linux-arm-kernel
  Cc: Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

Hi Tobias


On 2016-07-18 18:43, Tobias Jakobi wrote:
> Marek Szyprowski wrote:
>> On 2016-07-18 13:00, Tobias Jakobi wrote:
>>> Marek Szyprowski wrote:
>>>> On 2016-07-15 15:21, Tobias Jakobi wrote:
>>>>> Tobias Jakobi wrote:
>>>>>> Hello Marek,
>>>>>>
>>>>>> I've tested the patchset on 4.7-rc7 and noticed that it breaks
>>>>>> reboot on
>>>>>> my ODROID-X2.
>>>>>>
>>>>>> Going to check where exactly things break.
>>>>> Sadly it's the last patch where everything comes together:
>>>>> "iommu/exynos: Add proper runtime pm support"
>>>>>
>>>>> I still have to check if forcing runpm status to 'on' makes a
>>>>> difference. I suspect that the aggressive clock gating might be the
>>>>> reason?
>>>> Thanks for testing. I will check this issue. Could you send me your
>>>> .config?
>>> This is the config I'm currently using:
>>> https://github.com/tobiasjakobi/odroid-environment/blob/master/sourcecode/system/vanilla-4.7-debug.conf
>>>
>>>
>>> Do you think checking this with no_console_suspend makes sense?
>> no_console_suspend switch won't provide more information, but I managed
>> to reproduce your issue. I'm really confused how enabling runtime pm can
>> cause problems with usb/smsc95xx ethernet driver (that is the reason for
>> failed reboot). Maybe it is somehow related to the global relations
>> between devices and drivers and the fact that creating the runtime pm
>> links change the order of operations. I will check this again when
>> Rafael send updated patches. Here is the log I got (after waiting some
>> time):
> thanks for looking into this! I'll try to reproduce this on my board. I
> have to admit that I didn't wait too long for the hung task message to
> appear.
>
> I wonder if this has something to do with regulator code cutting some
> supplies too early. Is this on a X2 or a U2/U3?

I've reproduced it on U3.

> I'm not sure if we
> currently model the regulator setup correctly here (IIRC then buck8 is
> supplying the LAN/USB block on U2/U3).

IMHO it is not really related to regulator operations, but the sequence
of shutting down logical devices in the system. For some reasons when pm 
links
are used, something changes the order of operations in system shutdown
procedure, what causes smsc95xx to hang. I have no idea why, but I don't 
have
time to investigate it further. I will wait for the next release of Rafael's
pm links patches and then check everything again.

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-06-17 14:07       ` Lukas Wunner
@ 2016-07-20  0:33         ` Rafael J. Wysocki
  2016-07-20  6:24           ` Lukas Wunner
  0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-07-20  0:33 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Rafael J. Wysocki, Marek Szyprowski, linux-pm,
	Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andreas Noever, linux-pci

On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
> On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > We also have such a functional dependency for Thunderbolt on Macs:
> > > On resume from system sleep, the PCIe hotplug ports may not resume
> > > before the thunderbolt driver has reestablished the PCI tunnels.
> > > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > > in drivers/pci/quirks.c. It would be good if we could represent
> > > this dependency using something like Rafael's approach instead of
> > > open coding it, however one detail in Rafael's patches is problematic:
> > >
> > > > New links are added by calling device_link_add() which may happen
> > > > either before the consumer device is probed or when probing it, in
> > > > which case the caller needs to ensure that the driver of the
> > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > > flag should be passed to device_link_add() to reflect that.
> > >
> > > The thunderbolt driver cannot call device_link_add() before the
> > > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > > if the thunderbolt driver isn't loaded.
> > >
> > > It would therefore be beneficial if device_link_add() can be
> > > called even *after* the consumer is bound.
> > 
> > I don't quite follow.
> > 
> > Who's the provider and who's the consumer here?
> 
> thunderbolt.ko is the supplier.

But it binds to the children of the ports that are supposed to be its
consumers?

Why is that even expected to work?

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-20  0:33         ` Rafael J. Wysocki
@ 2016-07-20  6:24           ` Lukas Wunner
  2016-07-20 12:52             ` Rafael J. Wysocki
  0 siblings, 1 reply; 36+ messages in thread
From: Lukas Wunner @ 2016-07-20  6:24 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
> On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
> > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > We also have such a functional dependency for Thunderbolt on Macs:
> > > > On resume from system sleep, the PCIe hotplug ports may not resume
> > > > before the thunderbolt driver has reestablished the PCI tunnels.
> > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > > > in drivers/pci/quirks.c. It would be good if we could represent
> > > > this dependency using something like Rafael's approach instead of
> > > > open coding it, however one detail in Rafael's patches is problematic:
> > > >
> > > > > New links are added by calling device_link_add() which may happen
> > > > > either before the consumer device is probed or when probing it, in
> > > > > which case the caller needs to ensure that the driver of the
> > > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > > > flag should be passed to device_link_add() to reflect that.
> > > >
> > > > The thunderbolt driver cannot call device_link_add() before the
> > > > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > > > if the thunderbolt driver isn't loaded.
> > > >
> > > > It would therefore be beneficial if device_link_add() can be
> > > > called even *after* the consumer is bound.
> > > 
> > > I don't quite follow.
> > > 
> > > Who's the provider and who's the consumer here?
> > 
> > thunderbolt.ko is the supplier.
> 
> But it binds to the children of the ports that are supposed to be its
> consumers?
> 
> Why is that even expected to work?

No, the consumers are aunts (or uncles) of the supplier, if you will. :-)

The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
the drawing below). The supplier is the NHI:

      (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
                                         +-- Downstream Bridge 1 --
                                         +-- Downstream Bridge 2 --
                                         ...

We're calling pci_power_up() and pci_restore_state() from
pci_pm_resume_noirq(). And that will fail for devices below
the hotplug ports if the PCI tunnels haven't been re-established
yet by the NHI.

Currently we achieve that via quirk_apple_wait_for_thunderbolt() in
drivers/pci/quirks.c. It would be more elegant if we could make this
relationship explicit with "device links" and let the core handle it.

Or am I mistaken and this particular use case is not what "device links"
are intended for?

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-20  6:24           ` Lukas Wunner
@ 2016-07-20 12:52             ` Rafael J. Wysocki
  2016-07-20 15:23               ` Lukas Wunner
  0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-07-20 12:52 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
> On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
> > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
> > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > > We also have such a functional dependency for Thunderbolt on Macs:
> > > > > On resume from system sleep, the PCIe hotplug ports may not resume
> > > > > before the thunderbolt driver has reestablished the PCI tunnels.
> > > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > > > > in drivers/pci/quirks.c. It would be good if we could represent
> > > > > this dependency using something like Rafael's approach instead of
> > > > > open coding it, however one detail in Rafael's patches is problematic:
> > > > >
> > > > > > New links are added by calling device_link_add() which may happen
> > > > > > either before the consumer device is probed or when probing it, in
> > > > > > which case the caller needs to ensure that the driver of the
> > > > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > > > > flag should be passed to device_link_add() to reflect that.
> > > > >
> > > > > The thunderbolt driver cannot call device_link_add() before the
> > > > > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > > > > if the thunderbolt driver isn't loaded.
> > > > >
> > > > > It would therefore be beneficial if device_link_add() can be
> > > > > called even *after* the consumer is bound.
> > > > 
> > > > I don't quite follow.
> > > > 
> > > > Who's the provider and who's the consumer here?
> > > 
> > > thunderbolt.ko is the supplier.
> > 
> > But it binds to the children of the ports that are supposed to be its
> > consumers?
> > 
> > Why is that even expected to work?
> 
> No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
> 
> The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
> the drawing below). The supplier is the NHI:
> 
>       (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
>                                          +-- Downstream Bridge 1 --
>                                          +-- Downstream Bridge 2 --
>                                          ...
> 
> We're calling pci_power_up() and pci_restore_state() from
> pci_pm_resume_noirq(). And that will fail for devices below
> the hotplug ports if the PCI tunnels haven't been re-established
> yet by the NHI.

So the NHI is a PCIe device, right?

Does the Thunderbolt driver bind to that device?

> Currently we achieve that via quirk_apple_wait_for_thunderbolt() in
> drivers/pci/quirks.c. It would be more elegant if we could make this
> relationship explicit with "device links" and let the core handle it.
> 
> Or am I mistaken and this particular use case is not what "device links"
> are intended for?

I'm not sure yet.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-20 12:52             ` Rafael J. Wysocki
@ 2016-07-20 15:23               ` Lukas Wunner
  2016-07-20 22:51                 ` Rafael J. Wysocki
  0 siblings, 1 reply; 36+ messages in thread
From: Lukas Wunner @ 2016-07-20 15:23 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
> > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
> > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
> > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > > > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > > > We also have such a functional dependency for Thunderbolt on Macs:
> > > > > > On resume from system sleep, the PCIe hotplug ports may not resume
> > > > > > before the thunderbolt driver has reestablished the PCI tunnels.
> > > > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > > > > > in drivers/pci/quirks.c. It would be good if we could represent
> > > > > > this dependency using something like Rafael's approach instead of
> > > > > > open coding it, however one detail in Rafael's patches is problematic:
> > > > > >
> > > > > > > New links are added by calling device_link_add() which may happen
> > > > > > > either before the consumer device is probed or when probing it, in
> > > > > > > which case the caller needs to ensure that the driver of the
> > > > > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > > > > > flag should be passed to device_link_add() to reflect that.
> > > > > >
> > > > > > The thunderbolt driver cannot call device_link_add() before the
> > > > > > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > > > > > if the thunderbolt driver isn't loaded.
> > > > > >
> > > > > > It would therefore be beneficial if device_link_add() can be
> > > > > > called even *after* the consumer is bound.
> > > > > 
> > > > > I don't quite follow.
> > > > > 
> > > > > Who's the provider and who's the consumer here?
> > > > 
> > > > thunderbolt.ko is the supplier.
> > > 
> > > But it binds to the children of the ports that are supposed to be its
> > > consumers?
> > > 
> > > Why is that even expected to work?
> > 
> > No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
> > 
> > The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
> > the drawing below). The supplier is the NHI:
> > 
> >       (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
> >                                          +-- Downstream Bridge 1 --
> >                                          +-- Downstream Bridge 2 --
> >                                          ...
> > 
> > We're calling pci_power_up() and pci_restore_state() from
> > pci_pm_resume_noirq(). And that will fail for devices below
> > the hotplug ports if the PCI tunnels haven't been re-established
> > yet by the NHI.
> 
> So the NHI is a PCIe device, right?
> 
> Does the Thunderbolt driver bind to that device?

The NHI is a PCI device but not a bridge. It has class 0x88000.
Yes, thunderbolt.ko binds to the NHI.

And portdrv binds to the upstream bridge and downstream bridges.
Those have class 0x60400.

Best regards,

Lukas

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-20 15:23               ` Lukas Wunner
@ 2016-07-20 22:51                 ` Rafael J. Wysocki
  2016-07-20 23:25                   ` Lukas Wunner
  0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-07-20 22:51 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
> On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
> > > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
> > > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
> > > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > > > > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > > > > We also have such a functional dependency for Thunderbolt on Macs:
> > > > > > > On resume from system sleep, the PCIe hotplug ports may not resume
> > > > > > > before the thunderbolt driver has reestablished the PCI tunnels.
> > > > > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > > > > > > in drivers/pci/quirks.c. It would be good if we could represent
> > > > > > > this dependency using something like Rafael's approach instead of
> > > > > > > open coding it, however one detail in Rafael's patches is problematic:
> > > > > > >
> > > > > > > > New links are added by calling device_link_add() which may happen
> > > > > > > > either before the consumer device is probed or when probing it, in
> > > > > > > > which case the caller needs to ensure that the driver of the
> > > > > > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > > > > > > flag should be passed to device_link_add() to reflect that.
> > > > > > >
> > > > > > > The thunderbolt driver cannot call device_link_add() before the
> > > > > > > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > > > > > > if the thunderbolt driver isn't loaded.
> > > > > > >
> > > > > > > It would therefore be beneficial if device_link_add() can be
> > > > > > > called even *after* the consumer is bound.
> > > > > > 
> > > > > > I don't quite follow.
> > > > > > 
> > > > > > Who's the provider and who's the consumer here?
> > > > > 
> > > > > thunderbolt.ko is the supplier.
> > > > 
> > > > But it binds to the children of the ports that are supposed to be its
> > > > consumers?
> > > > 
> > > > Why is that even expected to work?
> > > 
> > > No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
> > > 
> > > The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
> > > the drawing below). The supplier is the NHI:
> > > 
> > >       (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
> > >                                          +-- Downstream Bridge 1 --
> > >                                          +-- Downstream Bridge 2 --
> > >                                          ...
> > > 
> > > We're calling pci_power_up() and pci_restore_state() from
> > > pci_pm_resume_noirq(). And that will fail for devices below
> > > the hotplug ports if the PCI tunnels haven't been re-established
> > > yet by the NHI.
> > 
> > So the NHI is a PCIe device, right?
> > 
> > Does the Thunderbolt driver bind to that device?
> 
> The NHI is a PCI device but not a bridge. It has class 0x88000.
> Yes, thunderbolt.ko binds to the NHI.
> 
> And portdrv binds to the upstream bridge and downstream bridges.
> Those have class 0x60400.

OK, so why would there be a problem with creating links from the NHI (producer)
to the ports (consumers) before binding portdrv to them?

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-20 22:51                 ` Rafael J. Wysocki
@ 2016-07-20 23:25                   ` Lukas Wunner
  2016-07-21  0:25                     ` Rafael J. Wysocki
  0 siblings, 1 reply; 36+ messages in thread
From: Lukas Wunner @ 2016-07-20 23:25 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
> > On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
> > > > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
> > > > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
> > > > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> > > > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > > > > > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > > > > > We also have such a functional dependency for Thunderbolt on Macs:
> > > > > > > > On resume from system sleep, the PCIe hotplug ports may not resume
> > > > > > > > before the thunderbolt driver has reestablished the PCI tunnels.
> > > > > > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > > > > > > > in drivers/pci/quirks.c. It would be good if we could represent
> > > > > > > > this dependency using something like Rafael's approach instead of
> > > > > > > > open coding it, however one detail in Rafael's patches is problematic:
> > > > > > > >
> > > > > > > > > New links are added by calling device_link_add() which may happen
> > > > > > > > > either before the consumer device is probed or when probing it, in
> > > > > > > > > which case the caller needs to ensure that the driver of the
> > > > > > > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > > > > > > > flag should be passed to device_link_add() to reflect that.
> > > > > > > >
> > > > > > > > The thunderbolt driver cannot call device_link_add() before the
> > > > > > > > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > > > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > > > > > > > if the thunderbolt driver isn't loaded.
> > > > > > > >
> > > > > > > > It would therefore be beneficial if device_link_add() can be
> > > > > > > > called even *after* the consumer is bound.
> > > > > > > 
> > > > > > > I don't quite follow.
> > > > > > > 
> > > > > > > Who's the provider and who's the consumer here?
> > > > > > 
> > > > > > thunderbolt.ko is the supplier.
> > > > > 
> > > > > But it binds to the children of the ports that are supposed to be its
> > > > > consumers?
> > > > > 
> > > > > Why is that even expected to work?
> > > > 
> > > > No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
> > > > 
> > > > The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
> > > > the drawing below). The supplier is the NHI:
> > > > 
> > > >       (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
> > > >                                          +-- Downstream Bridge 1 --
> > > >                                          +-- Downstream Bridge 2 --
> > > >                                          ...
> > > > 
> > > > We're calling pci_power_up() and pci_restore_state() from
> > > > pci_pm_resume_noirq(). And that will fail for devices below
> > > > the hotplug ports if the PCI tunnels haven't been re-established
> > > > yet by the NHI.
> > > 
> > > So the NHI is a PCIe device, right?
> > > 
> > > Does the Thunderbolt driver bind to that device?
> > 
> > The NHI is a PCI device but not a bridge. It has class 0x88000.
> > Yes, thunderbolt.ko binds to the NHI.
> > 
> > And portdrv binds to the upstream bridge and downstream bridges.
> > Those have class 0x60400.
> 
> OK, so why would there be a problem with creating links from the NHI
> (producer) to the ports (consumers) before binding portdrv to them?

Because the ordering in which drivers bind isn't guaranteed. At least
on my machine (Debian), portdrv always binds before thunderbolt.

I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
no driver is bound to the NHI. Doesn't feel pretty to me though.

Ultimately this seems to be the same issue as with calling
dev_pm_domain_set() for a bound device. Perhaps device_link_add()
can likewise be allowed if a runtime PM ref is held for the devices
and the call happens under lock_system_sleep()?

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-20 23:25                   ` Lukas Wunner
@ 2016-07-21  0:25                     ` Rafael J. Wysocki
  2016-07-24 22:48                       ` Lukas Wunner
  0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-07-21  0:25 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
> On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
> > > On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
> > > > > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
> > > > > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
> > > > > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> > > > > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > > > > > > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > > > > > > We also have such a functional dependency for Thunderbolt on Macs:
> > > > > > > > > On resume from system sleep, the PCIe hotplug ports may not resume
> > > > > > > > > before the thunderbolt driver has reestablished the PCI tunnels.
> > > > > > > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > > > > > > > > in drivers/pci/quirks.c. It would be good if we could represent
> > > > > > > > > this dependency using something like Rafael's approach instead of
> > > > > > > > > open coding it, however one detail in Rafael's patches is problematic:
> > > > > > > > >
> > > > > > > > > > New links are added by calling device_link_add() which may happen
> > > > > > > > > > either before the consumer device is probed or when probing it, in
> > > > > > > > > > which case the caller needs to ensure that the driver of the
> > > > > > > > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > > > > > > > > flag should be passed to device_link_add() to reflect that.
> > > > > > > > >
> > > > > > > > > The thunderbolt driver cannot call device_link_add() before the
> > > > > > > > > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > > > > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > > > > > > > > if the thunderbolt driver isn't loaded.
> > > > > > > > >
> > > > > > > > > It would therefore be beneficial if device_link_add() can be
> > > > > > > > > called even *after* the consumer is bound.
> > > > > > > > 
> > > > > > > > I don't quite follow.
> > > > > > > > 
> > > > > > > > Who's the provider and who's the consumer here?
> > > > > > > 
> > > > > > > thunderbolt.ko is the supplier.
> > > > > > 
> > > > > > But it binds to the children of the ports that are supposed to be its
> > > > > > consumers?
> > > > > > 
> > > > > > Why is that even expected to work?
> > > > > 
> > > > > No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
> > > > > 
> > > > > The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
> > > > > the drawing below). The supplier is the NHI:
> > > > > 
> > > > >       (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
> > > > >                                          +-- Downstream Bridge 1 --
> > > > >                                          +-- Downstream Bridge 2 --
> > > > >                                          ...
> > > > > 
> > > > > We're calling pci_power_up() and pci_restore_state() from
> > > > > pci_pm_resume_noirq(). And that will fail for devices below
> > > > > the hotplug ports if the PCI tunnels haven't been re-established
> > > > > yet by the NHI.
> > > > 
> > > > So the NHI is a PCIe device, right?
> > > > 
> > > > Does the Thunderbolt driver bind to that device?
> > > 
> > > The NHI is a PCI device but not a bridge. It has class 0x88000.
> > > Yes, thunderbolt.ko binds to the NHI.
> > > 
> > > And portdrv binds to the upstream bridge and downstream bridges.
> > > Those have class 0x60400.
> > 
> > OK, so why would there be a problem with creating links from the NHI
> > (producer) to the ports (consumers) before binding portdrv to them?
> 
> Because the ordering in which drivers bind isn't guaranteed. At least
> on my machine (Debian), portdrv always binds before thunderbolt.

But what drivers have to do with that really?  Do you need drivers to
know that the dependency is there?

Just add likns *before* even probing for drivers (yes, you can do that)
and the core will handle that for you.

> I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
> no driver is bound to the NHI. Doesn't feel pretty to me though.
> 
> Ultimately this seems to be the same issue as with calling
> dev_pm_domain_set() for a bound device. Perhaps device_link_add()
> can likewise be allowed if a runtime PM ref is held for the devices
> and the call happens under lock_system_sleep()?

No, the whole synchronization scheme in the links code would have had to be
changed for that to really work.

And it really is about what is needed (at least in principle) to run your
device.  If you think you need device X with a driver to handle device Y
correctly, then either you need it all the time, from probe to remove, or
you just don't really need it at all.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies)
  2016-07-19  6:26             ` Marek Szyprowski
@ 2016-07-24 18:02               ` Tobias Jakobi
  0 siblings, 0 replies; 36+ messages in thread
From: Tobias Jakobi @ 2016-07-24 18:02 UTC (permalink / raw)
  To: Marek Szyprowski, Tobias Jakobi, linux-pm, linux-kernel, iommu,
	linux-samsung-soc, linux-arm-kernel
  Cc: Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

Hi Marek,


Marek Szyprowski wrote:
> Hi Tobias
> 
> 
> On 2016-07-18 18:43, Tobias Jakobi wrote:
>> Marek Szyprowski wrote:
>>> On 2016-07-18 13:00, Tobias Jakobi wrote:
>>>> Marek Szyprowski wrote:
>>>>> On 2016-07-15 15:21, Tobias Jakobi wrote:
>>>>>> Tobias Jakobi wrote:
>>>>>>> Hello Marek,
>>>>>>>
>>>>>>> I've tested the patchset on 4.7-rc7 and noticed that it breaks
>>>>>>> reboot on
>>>>>>> my ODROID-X2.
>>>>>>>
>>>>>>> Going to check where exactly things break.
>>>>>> Sadly it's the last patch where everything comes together:
>>>>>> "iommu/exynos: Add proper runtime pm support"
>>>>>>
>>>>>> I still have to check if forcing runpm status to 'on' makes a
>>>>>> difference. I suspect that the aggressive clock gating might be the
>>>>>> reason?
>>>>> Thanks for testing. I will check this issue. Could you send me your
>>>>> .config?
>>>> This is the config I'm currently using:
>>>> https://github.com/tobiasjakobi/odroid-environment/blob/master/sourcecode/system/vanilla-4.7-debug.conf
>>>>
>>>>
>>>>
>>>> Do you think checking this with no_console_suspend makes sense?
>>> no_console_suspend switch won't provide more information, but I managed
>>> to reproduce your issue. I'm really confused how enabling runtime pm can
>>> cause problems with usb/smsc95xx ethernet driver (that is the reason for
>>> failed reboot). Maybe it is somehow related to the global relations
>>> between devices and drivers and the fact that creating the runtime pm
>>> links change the order of operations. I will check this again when
>>> Rafael send updated patches. Here is the log I got (after waiting some
>>> time):
>> thanks for looking into this! I'll try to reproduce this on my board. I
>> have to admit that I didn't wait too long for the hung task message to
>> appear.
>>
>> I wonder if this has something to do with regulator code cutting some
>> supplies too early. Is this on a X2 or a U2/U3?
> 
> I've reproduced it on U3.
Here's what I get on my X2.

>  * Remounting remaining filesystems read-only ...
>  *   Remounting / read only ...
> [   59.695857] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null)
>  [ ok ]
>  [ ok ]
> [   59.858672] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [   66.537116] smsc95xx 1-2.1.1:1.0 eth0: Failed to read reg index 0x00000114: -110
> [   66.538887] smsc95xx 1-2.1.1:1.0 eth0: Error reading MII_ACCESS
> [   66.544803] smsc95xx 1-2.1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read
> [   66.551487] smsc95xx 1-2.1.1:1.0 eth0: Failed to read MII_BMSR
> [   93.597127] usb 1-2-port2: cannot reset (err = -110)
> [   94.596714] usb 1-2-port2: cannot reset (err = -110)
> [   95.596737] usb 1-2-port2: cannot reset (err = -110)
> [   96.596722] usb 1-2-port2: cannot reset (err = -110)
> [   97.596735] usb 1-2-port2: cannot reset (err = -110)
> [   97.596800] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
> [   98.602116] usb 1-2-port2: cannot disable (err = -110)
> [   99.601744] usb 1-2-port2: cannot reset (err = -110)
> [  100.601730] usb 1-2-port2: cannot reset (err = -110)
> [  101.601743] usb 1-2-port2: cannot reset (err = -110)
> [  102.601729] usb 1-2-port2: cannot reset (err = -110)
> [  103.601746] usb 1-2-port2: cannot reset (err = -110)
> [  103.601811] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
> [  104.606737] usb 1-2-port2: cannot disable (err = -110)
> [  105.606756] usb 1-2-port2: cannot reset (err = -110)
> [  106.606742] usb 1-2-port2: cannot reset (err = -110)
> [  107.606758] usb 1-2-port2: cannot reset (err = -110)
> [  108.606747] usb 1-2-port2: cannot reset (err = -110)
> [  109.606763] usb 1-2-port2: cannot reset (err = -110)
> [  109.606835] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
> [  110.611748] usb 1-2-port2: cannot disable (err = -110)
> [  111.611766] usb 1-2-port2: cannot reset (err = -110)
> [  112.611758] usb 1-2-port2: cannot reset (err = -110)
> [  113.611769] usb 1-2-port2: cannot reset (err = -110)
> [  114.611758] usb 1-2-port2: cannot reset (err = -110)
> [  115.611776] usb 1-2-port2: cannot reset (err = -110)
> [  115.611846] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
> [  116.616764] usb 1-2-port2: cannot disable (err = -110)
> [  117.617148] usb 1-2-port2: cannot disable (err = -110)
> [  122.616775] hub 1-2:1.0: hub_ext_port_status failed (err = -110)

Output stops there, I don't see any more output with respect to hung
tasks (even though hung task detection is on and I've waited for like 10
minutes).

Anyway, I looks like that smsc95xx_unbind() is never called, and hence
the check_carrier() delayed work queue isn't cancelled.

However I'm not convinced that this is the real problem. I have manually
triggered the unbind before shutdown, and while the error message from
the smsc95xx driver have disappeared, the other messages are still produced.

My guess is that we are seeing these messages because reboot is not
working, and not the other way round.


- Tobias


> 
>> I'm not sure if we
>> currently model the regulator setup correctly here (IIRC then buck8 is
>> supplying the LAN/USB block on U2/U3).
> 
> IMHO it is not really related to regulator operations, but the sequence
> of shutting down logical devices in the system. For some reasons when pm
> links
> are used, something changes the order of operations in system shutdown
> procedure, what causes smsc95xx to hang. I have no idea why, but I don't
> have
> time to investigate it further. I will wait for the next release of
> Rafael's
> pm links patches and then check everything again.
> 
> Best regards

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-21  0:25                     ` Rafael J. Wysocki
@ 2016-07-24 22:48                       ` Lukas Wunner
  2016-07-28  0:30                         ` Rafael J. Wysocki
  0 siblings, 1 reply; 36+ messages in thread
From: Lukas Wunner @ 2016-07-24 22:48 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote:
> On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
> > On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
> > > > On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
> > > > > > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
> > > > > > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
> > > > > > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> > > > > > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > > > > > > > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > > > > > > > We also have such a functional dependency for Thunderbolt on Macs:
> > > > > > > > > > On resume from system sleep, the PCIe hotplug ports may not resume
> > > > > > > > > > before the thunderbolt driver has reestablished the PCI tunnels.
> > > > > > > > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > > > > > > > > > in drivers/pci/quirks.c. It would be good if we could represent
> > > > > > > > > > this dependency using something like Rafael's approach instead of
> > > > > > > > > > open coding it, however one detail in Rafael's patches is problematic:
> > > > > > > > > >
> > > > > > > > > > > New links are added by calling device_link_add() which may happen
> > > > > > > > > > > either before the consumer device is probed or when probing it, in
> > > > > > > > > > > which case the caller needs to ensure that the driver of the
> > > > > > > > > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > > > > > > > > > flag should be passed to device_link_add() to reflect that.
> > > > > > > > > >
> > > > > > > > > > The thunderbolt driver cannot call device_link_add() before the
> > > > > > > > > > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > > > > > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > > > > > > > > > if the thunderbolt driver isn't loaded.
> > > > > > > > > >
> > > > > > > > > > It would therefore be beneficial if device_link_add() can be
> > > > > > > > > > called even *after* the consumer is bound.
> > > > > > > > > 
> > > > > > > > > I don't quite follow.
> > > > > > > > > 
> > > > > > > > > Who's the provider and who's the consumer here?
> > > > > > > > 
> > > > > > > > thunderbolt.ko is the supplier.
> > > > > > > 
> > > > > > > But it binds to the children of the ports that are supposed to be its
> > > > > > > consumers?
> > > > > > > 
> > > > > > > Why is that even expected to work?
> > > > > > 
> > > > > > No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
> > > > > > 
> > > > > > The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
> > > > > > the drawing below). The supplier is the NHI:
> > > > > > 
> > > > > >       (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
> > > > > >                                          +-- Downstream Bridge 1 --
> > > > > >                                          +-- Downstream Bridge 2 --
> > > > > >                                          ...
> > > > > > 
> > > > > > We're calling pci_power_up() and pci_restore_state() from
> > > > > > pci_pm_resume_noirq(). And that will fail for devices below
> > > > > > the hotplug ports if the PCI tunnels haven't been re-established
> > > > > > yet by the NHI.
> > > > > 
> > > > > So the NHI is a PCIe device, right?
> > > > > 
> > > > > Does the Thunderbolt driver bind to that device?
> > > > 
> > > > The NHI is a PCI device but not a bridge. It has class 0x88000.
> > > > Yes, thunderbolt.ko binds to the NHI.
> > > > 
> > > > And portdrv binds to the upstream bridge and downstream bridges.
> > > > Those have class 0x60400.
> > > 
> > > OK, so why would there be a problem with creating links from the NHI
> > > (producer) to the ports (consumers) before binding portdrv to them?
> > 
> > Because the ordering in which drivers bind isn't guaranteed. At least
> > on my machine (Debian), portdrv always binds before thunderbolt.
> 
> But what drivers have to do with that really?  Do you need drivers to
> know that the dependency is there?
> 
> Just add likns *before* even probing for drivers (yes, you can do that)
> and the core will handle that for you.

Forgive me for being dense: How do you suggest to add links before
probing drivers? Only way I could think of is with a PCI quirk.

Which is what we're already doing right now (see drivers/pci/quirk.c:
quirk_apple_wait_for_thunderbolt()). And it ain't pretty.


> > I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
> > no driver is bound to the NHI. Doesn't feel pretty to me though.
> > 
> > Ultimately this seems to be the same issue as with calling
> > dev_pm_domain_set() for a bound device. Perhaps device_link_add()
> > can likewise be allowed if a runtime PM ref is held for the devices
> > and the call happens under lock_system_sleep()?
> 
> No, the whole synchronization scheme in the links code would have had to be
> changed for that to really work.
> 
> And it really is about what is needed (at least in principle) to run your
> device.  If you think you need device X with a driver to handle device Y
> correctly, then either you need it all the time, from probe to remove, or
> you just don't really need it at all.

Real life isn't as simple as that.

In this case, we have consumers (hotplug ports) which are doing fine
if the driver for the supplier (NHI) is not loaded. But once it loads,
the links must be in place. Seems only logical to put the links in
place when they're needed, i.e. at load time of the supplier's driver.
Which the patch set doesn't allow right now.

Best regards,

Lukas

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-24 22:48                       ` Lukas Wunner
@ 2016-07-28  0:30                         ` Rafael J. Wysocki
  2016-07-28 15:28                           ` Lukas Wunner
  0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-07-28  0:30 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Monday, July 25, 2016 12:48:32 AM Lukas Wunner wrote:
> On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
> > > On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
> > > > > On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
> > > > > > > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
> > > > > > > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
> > > > > > > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
> > > > > > > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > > > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
> > > > > > > > > > > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > > > > > > > > We also have such a functional dependency for Thunderbolt on Macs:
> > > > > > > > > > > On resume from system sleep, the PCIe hotplug ports may not resume
> > > > > > > > > > > before the thunderbolt driver has reestablished the PCI tunnels.
> > > > > > > > > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt()
> > > > > > > > > > > in drivers/pci/quirks.c. It would be good if we could represent
> > > > > > > > > > > this dependency using something like Rafael's approach instead of
> > > > > > > > > > > open coding it, however one detail in Rafael's patches is problematic:
> > > > > > > > > > >
> > > > > > > > > > > > New links are added by calling device_link_add() which may happen
> > > > > > > > > > > > either before the consumer device is probed or when probing it, in
> > > > > > > > > > > > which case the caller needs to ensure that the driver of the
> > > > > > > > > > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
> > > > > > > > > > > > flag should be passed to device_link_add() to reflect that.
> > > > > > > > > > >
> > > > > > > > > > > The thunderbolt driver cannot call device_link_add() before the
> > > > > > > > > > > PCIe hotplug ports are bound to a driver unless we amend portdrv
> > > > > > > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
> > > > > > > > > > > if the thunderbolt driver isn't loaded.
> > > > > > > > > > >
> > > > > > > > > > > It would therefore be beneficial if device_link_add() can be
> > > > > > > > > > > called even *after* the consumer is bound.
> > > > > > > > > > 
> > > > > > > > > > I don't quite follow.
> > > > > > > > > > 
> > > > > > > > > > Who's the provider and who's the consumer here?
> > > > > > > > > 
> > > > > > > > > thunderbolt.ko is the supplier.
> > > > > > > > 
> > > > > > > > But it binds to the children of the ports that are supposed to be its
> > > > > > > > consumers?
> > > > > > > > 
> > > > > > > > Why is that even expected to work?
> > > > > > > 
> > > > > > > No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
> > > > > > > 
> > > > > > > The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
> > > > > > > the drawing below). The supplier is the NHI:
> > > > > > > 
> > > > > > >       (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
> > > > > > >                                          +-- Downstream Bridge 1 --
> > > > > > >                                          +-- Downstream Bridge 2 --
> > > > > > >                                          ...
> > > > > > > 
> > > > > > > We're calling pci_power_up() and pci_restore_state() from
> > > > > > > pci_pm_resume_noirq(). And that will fail for devices below
> > > > > > > the hotplug ports if the PCI tunnels haven't been re-established
> > > > > > > yet by the NHI.
> > > > > > 
> > > > > > So the NHI is a PCIe device, right?
> > > > > > 
> > > > > > Does the Thunderbolt driver bind to that device?
> > > > > 
> > > > > The NHI is a PCI device but not a bridge. It has class 0x88000.
> > > > > Yes, thunderbolt.ko binds to the NHI.
> > > > > 
> > > > > And portdrv binds to the upstream bridge and downstream bridges.
> > > > > Those have class 0x60400.
> > > > 
> > > > OK, so why would there be a problem with creating links from the NHI
> > > > (producer) to the ports (consumers) before binding portdrv to them?
> > > 
> > > Because the ordering in which drivers bind isn't guaranteed. At least
> > > on my machine (Debian), portdrv always binds before thunderbolt.
> > 
> > But what drivers have to do with that really?  Do you need drivers to
> > know that the dependency is there?
> > 
> > Just add likns *before* even probing for drivers (yes, you can do that)
> > and the core will handle that for you.
> 
> Forgive me for being dense: How do you suggest to add links before
> probing drivers? Only way I could think of is with a PCI quirk.
> 
> Which is what we're already doing right now (see drivers/pci/quirk.c:
> quirk_apple_wait_for_thunderbolt()). And it ain't pretty.

Well, maybe not, but doing it once during enumeration would be better than
on every resume.

Plus there is runtime PM to cover.

> > > I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
> > > no driver is bound to the NHI. Doesn't feel pretty to me though.
> > > 
> > > Ultimately this seems to be the same issue as with calling
> > > dev_pm_domain_set() for a bound device. Perhaps device_link_add()
> > > can likewise be allowed if a runtime PM ref is held for the devices
> > > and the call happens under lock_system_sleep()?
> > 
> > No, the whole synchronization scheme in the links code would have had to be
> > changed for that to really work.
> > 
> > And it really is about what is needed (at least in principle) to run your
> > device.  If you think you need device X with a driver to handle device Y
> > correctly, then either you need it all the time, from probe to remove, or
> > you just don't really need it at all.
> 
> Real life isn't as simple as that.
> 
> In this case, we have consumers (hotplug ports) which are doing fine
> if the driver for the supplier (NHI) is not loaded. But once it loads,
> the links must be in place.

Hmm.

What if it is not loaded and the system suspends.  Will everything work
as expected after the subsequent resume?

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-28  0:30                         ` Rafael J. Wysocki
@ 2016-07-28 15:28                           ` Lukas Wunner
  2016-09-06 23:57                             ` Rafael J. Wysocki
  0 siblings, 1 reply; 36+ messages in thread
From: Lukas Wunner @ 2016-07-28 15:28 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Thu, Jul 28, 2016 at 02:30:31AM +0200, Rafael J. Wysocki wrote:
> On Monday, July 25, 2016 12:48:32 AM Lukas Wunner wrote:
> > On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote:
> > > On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
> > > > I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
> > > > no driver is bound to the NHI. Doesn't feel pretty to me though.
> > > > 
> > > > Ultimately this seems to be the same issue as with calling
> > > > dev_pm_domain_set() for a bound device. Perhaps device_link_add()
> > > > can likewise be allowed if a runtime PM ref is held for the devices
> > > > and the call happens under lock_system_sleep()?
> > > 
> > > No, the whole synchronization scheme in the links code would have had to be
> > > changed for that to really work.
> > > 
> > > And it really is about what is needed (at least in principle) to run your
> > > device.  If you think you need device X with a driver to handle device Y
> > > correctly, then either you need it all the time, from probe to remove, or
> > > you just don't really need it at all.
> > 
> > Real life isn't as simple as that.
> > 
> > In this case, we have consumers (hotplug ports) which are doing fine
> > if the driver for the supplier (NHI) is not loaded. But once it loads,
> > the links must be in place.
> 
> Hmm.
> 
> What if it is not loaded and the system suspends.  Will everything work
> as expected after the subsequent resume?

The short answer is yes.

Long answer:

With Thunderbolt, the switch fabric is told to set up PCI tunnels
through the NHI (Native Host Interface). Once set up, the tunnels
stay as they are and attached devices are reachable. However after
a power cycle of the controller (suspend/resume), the tunnels are
gone and need to be re-established.

On Macs, there are two software components communicating with the
NHI: The first one is an EFI driver which sets up tunnels to all
devices present on boot and lights up all attached DP-over-Thunderbolt
displays. Once ExitBootServices is called, the EFI driver is shut
down but the configured tunnels stay as they are. The kernel is thus
able to enumerate attached PCI devices.

The second component is the OS driver, thunderbolt.ko. It is needed
to set up tunnels to hot-plugged devices (i.e., not present at boot).
It is also needed to re-establish tunnels after suspend/resume.

The necessity of quirk_apple_wait_for_thunderbolt() arises because
we walk the entire PCI hierarchy during ->resume_noirq and call
pci_power_up() and pci_restore_state() for each device. Now remember,
the PCI tunnels are gone after a power cycle, so the attached devices
aren't reachable. Waking them and restoring their state will fail
unless the thunderbolt driver reconfigures the switch fabric first.

=> So if there are no devices attached and thunderbolt.ko isn't loaded,
   everything is fine. No device link needed.

=> If devices are attached and thunderbolt.ko is loaded, then the hotplug
   ports need to wait for re-establishment of the PCI tunnels.
   Device link is needed.

=> If devices were attached on boot and thunderbolt.ko isn't loaded, they
   will be unreachable after resume. Nothing we can do about that.
   No device link needed.

So this is a case of a "weak" device link, "weak" referring to the fact
that it's only needed if the supplier is bound.

All that said, I don't know if this case exists often enough that it's
worth making allowances for it in the driver core.

Sorry for the wall of text, just want to make sure we're on the same page
and all possible use cases of device links are discussed and considered.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 06/10] driver core: Avoid endless recursion if device has more than one link
  2016-06-17  6:26 ` [PATCH v2 06/10] driver core: Avoid endless recursion if device has more than one link Marek Szyprowski
@ 2016-09-06 23:09   ` Rafael J. Wysocki
  0 siblings, 0 replies; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-09-06 23:09 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-pm, linux-kernel, iommu, linux-samsung-soc,
	linux-arm-kernel, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

On Friday, June 17, 2016 08:26:56 AM Marek Szyprowski wrote:
> This patch fixes endless recursion, which happends when device has
> more than one link.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/base/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 215cd44de761..4e778539b750 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -57,7 +57,8 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
>  	device_pm_move_last(dev);
>  	device_for_each_child(dev, NULL, device_reorder_to_tail);
>  	list_for_each_entry(link, &dev->consumer_links, c_node)
> -		device_reorder_to_tail(link->consumer, NULL);
> +		if (link->consumer != dev)
> +			device_reorder_to_tail(link->consumer, NULL);
>  
>  	return 0;
>  }
> 

If I'm not mistaken, this should not be necessary unless dev has a link
pointing to itself as a consumer.  That would be a bug, though.

I can add a WARN_ON() to catch this case, but then if there's a link from
a consumer of dev pointing back to dev as a consumer, that still will loop
forever.

I guess we need to detect circular dependencies and fail link creation in
such cases.  Oh well.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 07/10] driver core: Add support for links to already probed drivers
  2016-06-17  6:26 ` [PATCH v2 07/10] driver core: Add support for links to already probed drivers Marek Szyprowski
@ 2016-09-06 23:13   ` Rafael J. Wysocki
  0 siblings, 0 replies; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-09-06 23:13 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-pm, linux-kernel, iommu, linux-samsung-soc,
	linux-arm-kernel, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

On Friday, June 17, 2016 08:26:57 AM Marek Szyprowski wrote:
> Set proper link state if link is created between already probed supplier
> device and to be probed consumer device.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/base/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4e778539b750..d9c5c5542a6b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -125,7 +125,9 @@ struct devlink *device_link_add(struct device *consumer,
>  
>  	link->flags = flags;
>  	link->status = (flags & DEVICE_LINK_PROBE_TIME) ?
> -			DEVICE_LINK_CONSUMER_PROBE : DEVICE_LINK_DORMANT;
> +			DEVICE_LINK_CONSUMER_PROBE :
> +			(supplier->driver ? DEVICE_LINK_AVAILABLE :
> +			 DEVICE_LINK_DORMANT);
>  	spin_lock_init(&link->lock);
>  
>  	/*
> 

The supplier->driver check is insufficient and racy.

It is insufficient, because supplier->driver is also set during supplier probe
and the probe may still not be successful.

It is racy, because supplier->driver may be modified right after this check.

The only way to address the issue at hand I can see is to add a flag to
indicate to device_link_add() that the supplier has already been probed
successfully.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 08/10] PM core: Fix restoring devices with links during system PM transition
  2016-06-17  6:26 ` [PATCH v2 08/10] PM core: Fix restoring devices with links during system PM transition Marek Szyprowski
@ 2016-09-06 23:24   ` Rafael J. Wysocki
  0 siblings, 0 replies; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-09-06 23:24 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-pm, linux-kernel, iommu, linux-samsung-soc,
	linux-arm-kernel, Joerg Roedel, Inki Dae, Kukjin Kim,
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Ulf Hansson,
	Mark Brown, Greg Kroah-Hartman

On Friday, June 17, 2016 08:26:58 AM Marek Szyprowski wrote:
> When devices are being runtime resumed during the system PM transition to
> suspend state, the link suppliers might be already resumed and
> have runtime pm disabled. This is normal case. This patch adds special
> support for such case. Simple call to pm_runtime_get_sync returns error
> when device has runtime PM disabled, what results in incorrect runtime PM
> state during system wide PM transition.

The problem is real, but the proposed solution is questionable.

> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/base/power/runtime.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 0ea00d442e0f..d00b079fad88 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -261,6 +261,26 @@ static int rpm_check_suspend_allowed(struct device *dev)
>  }
>  
>  /**
> + * __pm_runtime_force_resume - force resume of the device
> + * @dev: Device to resume
> + *
> + * This function works like __pm_runtime_resume(dev, RPM_GET_PUT), but
> + * also handles devices with runtime PM disabled. This allows to properly
> + * control all devices during preparation for system PM transition.
> + */
> +static int __pm_runtime_force_resume(struct device *dev)
> +{
> +	if (!dev->power.disable_depth)
> +		return pm_runtime_get_sync(dev);
> +
> +	if (dev->power.runtime_status != RPM_ACTIVE ||
> +	    atomic_inc_not_zero(&dev->power.usage_count) == 0)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/**
>   * __rpm_callback - Run a given runtime PM callback for a given device.
>   * @cb: Runtime PM callback to run.
>   * @dev: Device to run the callback for.
> @@ -291,7 +311,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
>  				if ((link->flags & DEVICE_LINK_PM_RUNTIME)
>  				    && link->status != DEVICE_LINK_SUPPLIER_UNBIND
>  				    && !link->rpm_active) {
> -					retval = pm_runtime_get_sync(link->supplier);
> +					retval = __pm_runtime_force_resume(link->supplier);
>  					if (retval < 0) {
>  						pm_runtime_put_noidle(link->supplier);
>  						goto fail;

The reason why power.disable_depth is different from 0 may not be system suspend,
in which case it should fail even if the status happens to be RPM_ACTIVE.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
  2016-07-28 15:28                           ` Lukas Wunner
@ 2016-09-06 23:57                             ` Rafael J. Wysocki
  0 siblings, 0 replies; 36+ messages in thread
From: Rafael J. Wysocki @ 2016-09-06 23:57 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Marek Szyprowski, linux-pm, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	linux-samsung-soc, linux-arm-kernel, Joerg Roedel, Inki Dae,
	Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
	Ulf Hansson, Mark Brown, Greg Kroah-Hartman, Andreas Noever,
	linux-pci

On Thursday, July 28, 2016 05:28:31 PM Lukas Wunner wrote:
> On Thu, Jul 28, 2016 at 02:30:31AM +0200, Rafael J. Wysocki wrote:
> > On Monday, July 25, 2016 12:48:32 AM Lukas Wunner wrote:
> > > On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote:
> > > > On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
> > > > > I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
> > > > > no driver is bound to the NHI. Doesn't feel pretty to me though.
> > > > > 
> > > > > Ultimately this seems to be the same issue as with calling
> > > > > dev_pm_domain_set() for a bound device. Perhaps device_link_add()
> > > > > can likewise be allowed if a runtime PM ref is held for the devices
> > > > > and the call happens under lock_system_sleep()?
> > > > 
> > > > No, the whole synchronization scheme in the links code would have had to be
> > > > changed for that to really work.
> > > > 
> > > > And it really is about what is needed (at least in principle) to run your
> > > > device.  If you think you need device X with a driver to handle device Y
> > > > correctly, then either you need it all the time, from probe to remove, or
> > > > you just don't really need it at all.
> > > 
> > > Real life isn't as simple as that.
> > > 
> > > In this case, we have consumers (hotplug ports) which are doing fine
> > > if the driver for the supplier (NHI) is not loaded. But once it loads,
> > > the links must be in place.
> > 
> > Hmm.
> > 
> > What if it is not loaded and the system suspends.  Will everything work
> > as expected after the subsequent resume?
> 
> The short answer is yes.

OK

I think it's possible to add a link flag to address this case.

Namely, if that flag is passed to device_link_add(), the link will be
added in the DEVICE_LINK_ACTIVE state right away, but that will need to
be synchronized against all possible transitions of the consumer device
(at least).

It's better to do that in a separate patch for this reason IMO.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2016-09-06 23:51 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 01/10] driver core: Add a wrapper around __device_release_driver() Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 02/10] driver core: Functional dependencies tracking support Marek Szyprowski
2016-06-17 10:36   ` Lukas Wunner
2016-06-17 12:54     ` Rafael J. Wysocki
2016-06-17 14:07       ` Lukas Wunner
2016-07-20  0:33         ` Rafael J. Wysocki
2016-07-20  6:24           ` Lukas Wunner
2016-07-20 12:52             ` Rafael J. Wysocki
2016-07-20 15:23               ` Lukas Wunner
2016-07-20 22:51                 ` Rafael J. Wysocki
2016-07-20 23:25                   ` Lukas Wunner
2016-07-21  0:25                     ` Rafael J. Wysocki
2016-07-24 22:48                       ` Lukas Wunner
2016-07-28  0:30                         ` Rafael J. Wysocki
2016-07-28 15:28                           ` Lukas Wunner
2016-09-06 23:57                             ` Rafael J. Wysocki
2016-06-17  6:26 ` [PATCH v2 03/10] PM core: Make async suspend/resume of devices use device links Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 04/10] PM core: Make runtime PM " Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 05/10] PM core: Optimize the use of device links for runtime PM Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 06/10] driver core: Avoid endless recursion if device has more than one link Marek Szyprowski
2016-09-06 23:09   ` Rafael J. Wysocki
2016-06-17  6:26 ` [PATCH v2 07/10] driver core: Add support for links to already probed drivers Marek Szyprowski
2016-09-06 23:13   ` Rafael J. Wysocki
2016-06-17  6:26 ` [PATCH v2 08/10] PM core: Fix restoring devices with links during system PM transition Marek Szyprowski
2016-09-06 23:24   ` Rafael J. Wysocki
2016-06-17  6:26 ` [PATCH v2 09/10] iommu/exynos: Remove excessive, useless debug Marek Szyprowski
2016-06-17  6:27 ` [PATCH v2 10/10] iommu/exynos: Add proper runtime pm support Marek Szyprowski
2016-07-14 15:41 ` [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Tobias Jakobi
2016-07-15 13:21   ` Tobias Jakobi
2016-07-18 10:32     ` Marek Szyprowski
2016-07-18 11:00       ` Tobias Jakobi
2016-07-18 13:50         ` Marek Szyprowski
2016-07-18 16:43           ` Tobias Jakobi
2016-07-19  6:26             ` Marek Szyprowski
2016-07-24 18:02               ` Tobias Jakobi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).