[PATCH 0/6] driver core: Fix some issues related to device links

[PATCH 0/6] driver core: Fix some issues related to device links

[Rafael J. Wysocki]

Hi Greg at al,

Recently I have been looking at the device links code because of the
recent discussion on possibly using them in the DRM subsystem (see for
example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
found a few issues in that code which should be addressed by this patch
series.  Please refer to the patch changelogs for details.

None of the problems addressed here should be manifesting themselves in
mainline kernel today, but if there are more device links users in the
future, they most likely will be encountered sooner or later.  Also they
need to be fixed for the DRM use case to be supported IMO.

This series does not fix all issues in device links that have become
apparent (generally speaking, the idea of returning an existing link
in case there is one already for the given consumer-supplier pair
doesn't play well with stateful links and their flags), so there will
be a follow-up series of patches to clean that up.  Still, I don't see
a reason to sit on these fixes while working on the other patches, so
here they go.

Cheers,
Rafael

[PATCH 1/6] driver core: Fix DL_FLAG_AUTOREMOVE_SUPPLIER device link flag handling

[Rafael J. Wysocki]

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

First, if the kref_put() in device_links_driver_cleanup() added by
commit 1689cac5b32a ("driver core: Add flag to autoremove device
link on supplier unbind") actually causes the link object to be
freed, the subsequent attempt to change the state of the link
can crash the kernel, so avoid it by changing the state of the link
before attempting to drop it.  Use the observation that the original
state of the link can be changed to "dormant" whatever it is, because
the link becomes, in fact, dormant at that point.

Second, change the list walk in device_links_driver_cleanup() to
a safe one to avoid use-after-free when dropping a link from the list
during the walk.

Finally, while at it, fix device_link_add() to refuse to create
stateless device links with DL_FLAG_AUTOREMOVE_SUPPLIER set, which is
an invalid combination (setting that flag means that the driver core
should manage the link, so it cannot be stateless), and extend the
kerneldoc comment of device_link_add() to cover the
DL_FLAG_AUTOREMOVE_SUPPLIER flag properly too.

Fixes: 1689cac5b32a ("driver core: Add flag to autoremove device link on supplier unbind")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/core.c |   33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -179,10 +179,15 @@ void device_pm_move_to_tail(struct devic
  * of the link.  If DL_FLAG_PM_RUNTIME is not set, DL_FLAG_RPM_ACTIVE will be
  * ignored.
  *
- * If the DL_FLAG_AUTOREMOVE_CONSUMER is set, the link will be removed
- * automatically when the consumer device driver unbinds from it.
- * The combination of both DL_FLAG_AUTOREMOVE_CONSUMER and DL_FLAG_STATELESS
- * set is invalid and will cause NULL to be returned.
+ * If the DL_FLAG_AUTOREMOVE_CONSUMER flag is set, the reference to the link
+ * acquired by this function will be dropped automatically when the consumer
+ * device driver unbinds from it.  Analogously, if DL_FLAG_AUTOREMOVE_SUPPLIER
+ * is set in @flags, the reference to the link acquired by this function will
+ * be dropped automatically when the supplier device driver unbinds from it.
+ *
+ * The combination of DL_FLAG_STATELESS and either DL_FLAG_AUTOREMOVE_CONSUMER
+ * or DL_FLAG_AUTOREMOVE_SUPPLIER set in @flags at the same time is invalid and
+ * will cause NULL to be returned upfront.
  *
  * 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
@@ -199,8 +204,8 @@ struct device_link *device_link_add(stru
 	struct device_link *link;
 
 	if (!consumer || !supplier ||
-	    ((flags & DL_FLAG_STATELESS) &&
-	     (flags & DL_FLAG_AUTOREMOVE_CONSUMER)))
+	    (flags & DL_FLAG_STATELESS &&
+	     flags & (DL_FLAG_AUTOREMOVE_CONSUMER | DL_FLAG_AUTOREMOVE_SUPPLIER)))
 		return NULL;
 
 	device_links_write_lock();
@@ -539,27 +544,21 @@ void device_links_no_driver(struct devic
  */
 void device_links_driver_cleanup(struct device *dev)
 {
-	struct device_link *link;
+	struct device_link *link, *ln;
 
 	device_links_write_lock();
 
-	list_for_each_entry(link, &dev->links.consumers, s_node) {
+	list_for_each_entry_safe(link, ln, &dev->links.consumers, s_node) {
 		if (link->flags & DL_FLAG_STATELESS)
 			continue;
 
 		WARN_ON(link->flags & DL_FLAG_AUTOREMOVE_CONSUMER);
 		WARN_ON(link->status != DL_STATE_SUPPLIER_UNBIND);
 
-		/*
-		 * autoremove the links between this @dev and its consumer
-		 * devices that are not active, i.e. where the link state
-		 * has moved to DL_STATE_SUPPLIER_UNBIND.
-		 */
-		if (link->status == DL_STATE_SUPPLIER_UNBIND &&
-		    link->flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
-			kref_put(&link->kref, __device_link_del);
-
 		WRITE_ONCE(link->status, DL_STATE_DORMANT);
+
+		if (link->flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
+			kref_put(&link->kref, __device_link_del);
 	}
 
 	__device_links_no_driver(dev);

[PATCH 2/6] driver core: Reorder actions in __device_links_no_driver()

[Rafael J. Wysocki]

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

All links to suppliers have to be either "available" or in the
"supplier unbind in progress" state after __device_links_no_driver()
and the kref_put() may not actually delete them, so change the state
of each link in __device_links_no_driver() before attempting to drop
it.

While at it, update the kernedoc comment of __device_links_no_driver()
to reflect what happens more accurately.

Fixes: ead18c23c263 ("driver core: Introduce device links reference counting")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/core.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -500,11 +500,11 @@ void device_links_driver_bound(struct de
  * __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.
+ * Drop references to non-persistent links from this device to any suppliers.
  *
- * Persistent links stay around, but their status is changed to "available",
- * unless they already are in the "supplier unbind in progress" state in which
- * case they need not be updated.
+ * Links with DL_FLAG_AUTOREMOVE_CONSUMER unset stay around, but their status is
+ * changed to "available", unless they already are in the "supplier unbind in
+ * progress" state in which case they need not be updated.
  *
  * Links with the DL_FLAG_STATELESS flag set are ignored.
  */
@@ -516,10 +516,11 @@ static void __device_links_no_driver(str
 		if (link->flags & DL_FLAG_STATELESS)
 			continue;
 
+		if (link->status != DL_STATE_SUPPLIER_UNBIND)
+			WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
+
 		if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER)
 			kref_put(&link->kref, __device_link_del);
-		else if (link->status != DL_STATE_SUPPLIER_UNBIND)
-			WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
 	}
 
 	dev->links.status = DL_DEV_NO_DRIVER;

[PATCH 3/6] driver core: Avoid careless re-use of existing device links

[Rafael J. Wysocki]

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

After commit ead18c23c263 ("driver core: Introduce device links
reference counting"), if there is a link between the given supplier
and the given consumer already, device_link_add() will refcount it
and return it unconditionally.  However, if the flags passed to
it on the second (or any subsequent) attempt to create a device
link between the same consumer-supplier pair are not compatible with
the existing link's flags, that is incorrect.

First off, if the existing link is stateless and the next caller of
device_link_add() for the same consumer-supplier pair wants a
stateful one, or the other way around, the existing link cannot be
returned, because it will not match the expected behavior, so make
device_link_add() dump the stack and return NULL in that case.

Moreover, if the DL_FLAG_AUTOREMOVE_CONSUMER flag is passed to
device_link_add(), its caller will expect its reference to the link
to be dropped automatically on consumer driver removal, which will
not happen if that flag is not set in the link's flags (and
analogously for DL_FLAG_AUTOREMOVE_SUPPLIER).  For this reason, make
device_link_add() update the existing link's flags accordingly
before returning it to the caller.

Fixes: ead18c23c263 ("driver core: Introduce device links reference counting")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/core.c |   23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -222,12 +222,29 @@ struct device_link *device_link_add(stru
 		goto out;
 	}
 
-	list_for_each_entry(link, &supplier->links.consumers, s_node)
-		if (link->consumer == consumer) {
-			kref_get(&link->kref);
+	list_for_each_entry(link, &supplier->links.consumers, s_node) {
+		if (link->consumer != consumer)
+			continue;
+
+		/*
+		 * Don't return a stateless link if the caller wants a stateful
+		 * one and vice versa.
+		 */
+		if (WARN_ON((flags & DL_FLAG_STATELESS) != (link->flags & DL_FLAG_STATELESS))) {
+			link = NULL;
 			goto out;
 		}
 
+		if (flags & DL_FLAG_AUTOREMOVE_CONSUMER)
+			link->flags |= DL_FLAG_AUTOREMOVE_CONSUMER;
+
+		if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
+			link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
+
+		kref_get(&link->kref);
+		goto out;
+	}
+
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
 	if (!link)
 		goto out;

[PATCH 4/6] driver core: Do not resume suppliers under device_links_write_lock()

[Rafael J. Wysocki]

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

It is incorrect to call pm_runtime_get_sync() under
device_links_write_lock(), because it may end up trying to take
device_links_read_lock() while resuming the target device and that
will deadlock in the non-SRCU case, so avoid that by resuming the
supplier device in device_link_add() before calling
device_links_write_lock().

Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
Fixes: baa8809f6097 ("PM / runtime: Optimize the use of device links")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/core.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -202,12 +202,21 @@ struct device_link *device_link_add(stru
 				    struct device *supplier, u32 flags)
 {
 	struct device_link *link;
+	bool rpm_put_supplier = false;
 
 	if (!consumer || !supplier ||
 	    (flags & DL_FLAG_STATELESS &&
 	     flags & (DL_FLAG_AUTOREMOVE_CONSUMER | DL_FLAG_AUTOREMOVE_SUPPLIER)))
 		return NULL;
 
+	if (flags & DL_FLAG_PM_RUNTIME && flags & DL_FLAG_RPM_ACTIVE) {
+		if (pm_runtime_get_sync(supplier) < 0) {
+			pm_runtime_put_noidle(supplier);
+			return NULL;
+		}
+		rpm_put_supplier = true;
+	}
+
 	device_links_write_lock();
 	device_pm_lock();
 
@@ -251,13 +260,8 @@ struct device_link *device_link_add(stru
 
 	if (flags & DL_FLAG_PM_RUNTIME) {
 		if (flags & DL_FLAG_RPM_ACTIVE) {
-			if (pm_runtime_get_sync(supplier) < 0) {
-				pm_runtime_put_noidle(supplier);
-				kfree(link);
-				link = NULL;
-				goto out;
-			}
 			link->rpm_active = true;
+			rpm_put_supplier = false;
 		}
 		pm_runtime_new_link(consumer);
 		/*
@@ -329,6 +333,10 @@ struct device_link *device_link_add(stru
  out:
 	device_pm_unlock();
 	device_links_write_unlock();
+
+	if (rpm_put_supplier)
+		pm_runtime_put(supplier);
+
 	return link;
 }
 EXPORT_SYMBOL_GPL(device_link_add);

[PATCH 5/6] driver core: Fix handling of runtime PM flags in device_link_add()

[Rafael J. Wysocki]

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

After commit ead18c23c263 ("driver core: Introduce device links
reference counting"), if if there is a link between the given supplier
and the given consumer already, device_link_add() will refcount it
and return it unconditionally without updating its flags.  It is
possible, however, that the second (or any subsequent) caller of
device_link_add() for the same consumer-supplier pair will pass
DL_FLAG_PM_RUNTIME, possibly along with DL_FLAG_RPM_ACTIVE, in flags
to it and the existing link may not behave as expected then.

First, if DL_FLAG_PM_RUNTIME is not set in the existing link's flags
at all, it needs to be set like during the original initialization of
the link.  In that case DL_FLAG_RPM_ACTIVE should take effect like
during the original initialization of the link too.

Second, however, if DL_FLAG_PM_RUNTIME is set in the existing link's
flags, attempting to cause DL_FLAG_RPM_ACTIVE to take its normal
effect is generally unsafe and it is better to return NULL from
device_link_add() in that case (to indicate to the caller that the
expected behavior of the new device link could not be guaranteed).

Modify device_link_add() to behave as per the above.

[Note that the change in behavior regarding DL_FLAG_RPM_ACTIVE should
 not affect the existing users of that flag.]

Fixes: ead18c23c263 ("driver core: Introduce device links reference counting")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/core.c |   56 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -165,6 +165,23 @@ void device_pm_move_to_tail(struct devic
 	device_links_read_unlock(idx);
 }
 
+static void device_link_rpm_prepare(struct device *consumer,
+				    struct device *supplier,
+				    struct device_link *link, u32 flags)
+{
+	if (flags & DL_FLAG_RPM_ACTIVE)
+		link->rpm_active = true;
+
+	pm_runtime_new_link(consumer);
+	/*
+	 * If the link is being added by the consumer driver at probe time,
+	 * balance the decrementation of the supplier's runtime PM usage counter
+	 * after consumer probe in driver_probe_device().
+	 */
+	if (consumer->links.status == DL_DEV_PROBING)
+		pm_runtime_get_noresume(supplier);
+}
+
 /**
  * device_link_add - Create a link between two devices.
  * @consumer: Consumer end of the link.
@@ -177,7 +194,9 @@ void device_pm_move_to_tail(struct devic
  * DL_FLAG_RPM_ACTIVE flag is set in addition to it, the supplier devices will
  * be forced into the active metastate and reference-counted upon the creation
  * of the link.  If DL_FLAG_PM_RUNTIME is not set, DL_FLAG_RPM_ACTIVE will be
- * ignored.
+ * ignored.  However, passing both DL_FLAG_PM_RUNTIME and DL_FLAG_RPM_ACTIVE in
+ * @flags will cause NULL to be returned if an existing link between @consumer
+ * and @supplier with the DL_FLAG_PM_RUNTIME flag set is found.
  *
  * If the DL_FLAG_AUTOREMOVE_CONSUMER flag is set, the reference to the link
  * acquired by this function will be dropped automatically when the consumer
@@ -202,7 +221,6 @@ struct device_link *device_link_add(stru
 				    struct device *supplier, u32 flags)
 {
 	struct device_link *link;
-	bool rpm_put_supplier = false;
 
 	if (!consumer || !supplier ||
 	    (flags & DL_FLAG_STATELESS &&
@@ -214,7 +232,6 @@ struct device_link *device_link_add(stru
 			pm_runtime_put_noidle(supplier);
 			return NULL;
 		}
-		rpm_put_supplier = true;
 	}
 
 	device_links_write_lock();
@@ -250,6 +267,20 @@ struct device_link *device_link_add(stru
 		if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
 			link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
 
+		if (flags & DL_FLAG_PM_RUNTIME) {
+			if (link->flags & DL_FLAG_PM_RUNTIME) {
+				if (WARN_ON(flags & DL_FLAG_RPM_ACTIVE)) {
+					link = NULL;
+					goto out;
+				}
+			} else {
+				link->flags |= DL_FLAG_PM_RUNTIME;
+
+				device_link_rpm_prepare(consumer, supplier,
+							link, flags);
+			}
+		}
+
 		kref_get(&link->kref);
 		goto out;
 	}
@@ -258,20 +289,9 @@ struct device_link *device_link_add(stru
 	if (!link)
 		goto out;
 
-	if (flags & DL_FLAG_PM_RUNTIME) {
-		if (flags & DL_FLAG_RPM_ACTIVE) {
-			link->rpm_active = true;
-			rpm_put_supplier = false;
-		}
-		pm_runtime_new_link(consumer);
-		/*
-		 * If the link is being added by the consumer driver at probe
-		 * time, balance the decrementation of the supplier's runtime PM
-		 * usage counter after consumer probe in driver_probe_device().
-		 */
-		if (consumer->links.status == DL_DEV_PROBING)
-			pm_runtime_get_noresume(supplier);
-	}
+	if (flags & DL_FLAG_PM_RUNTIME)
+		device_link_rpm_prepare(consumer, supplier, link, flags);
+
 	get_device(supplier);
 	link->supplier = supplier;
 	INIT_LIST_HEAD(&link->s_node);
@@ -334,7 +354,7 @@ struct device_link *device_link_add(stru
 	device_pm_unlock();
 	device_links_write_unlock();
 
-	if (rpm_put_supplier)
+	if ((flags & DL_FLAG_PM_RUNTIME && flags & DL_FLAG_RPM_ACTIVE) && !link)
 		pm_runtime_put(supplier);
 
 	return link;

[PATCH v2 5/6] driver core: Fix handling of runtime PM flags in device_link_add()

[Rafael J. Wysocki]

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

After commit ead18c23c263 ("driver core: Introduce device links
reference counting"), if if there is a link between the given supplier
and the given consumer already, device_link_add() will refcount it
and return it unconditionally without updating its flags.  It is
possible, however, that the second (or any subsequent) caller of
device_link_add() for the same consumer-supplier pair will pass
DL_FLAG_PM_RUNTIME, possibly along with DL_FLAG_RPM_ACTIVE, in flags
to it and the existing link may not behave as expected then.

First, if DL_FLAG_PM_RUNTIME is not set in the existing link's flags
at all, it needs to be set like during the original initialization of
the link.

Second, if DL_FLAG_RPM_ACTIVE is passed to device_link_add() in flags
(in addition to DL_FLAG_PM_RUNTIME), the existing link should to be
updated to reflect the "active" runtime PM configuration of the
consumer-supplier pair and extra care must be taken here to avoid
possible destructive races with runtime PM of the consumer.

To that end, redefine the rpm_active field in struct device_link
as a refcount, initialize it to 1 and make rpm_resume() (for the
consumer) and device_link_add() increment it whenever they acquire
a runtime PM reference on the supplier device.  Accordingly, make
rpm_suspend() (for the consumer) and pm_runtime_clean_up_links()
decrement it and drop runtime PM references to the supplier
device in a loop until rpm_active becones 1 again.

Fixes: ead18c23c263 ("driver core: Introduce device links reference counting")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

I have overlooked the fact that rpm_put_suppliers() doesn't even look
at the DL_FLAG_PM_RUNTIME link flag and only checks rpm_active for each
link in the list, so updating rpm_active for existing links from
device_link_add() is unsafe, regardless of whether or not DL_FLAG_PM_RUNTIME
is set in their flags.  Of course, this means that the previous version of
this patch was incorrect.

I also was kind of unhappy with returning NULL to the callers of
device_link_add() who passed DL_FLAG_RPM_ACTIVE in flags, but the link
(with DL_FLAG_PM_RUNTIME set) had been there already.

So, here's a v2 of just this patch (the other patches in the series don't
change).

-> v2:
  * Redefine rpm_active in struct device_link as a refcount.
  * Rework the code to use the refcount instead of the bool properly.
  * Make device_link_add() increment rpm_active for existing links.

Admittedly, turning rpm_active into a refcount theoretically increases the
overhead of using device links in runtime PM, so it is a concession of sorts,
but I didn't see any other way to make this work.

---
 drivers/base/core.c          |   45 ++++++++++++++++++++++++++++---------------
 drivers/base/power/runtime.c |   26 ++++++++++--------------
 include/linux/device.h       |    2 -
 3 files changed, 42 insertions(+), 31 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -165,6 +165,19 @@ void device_pm_move_to_tail(struct devic
 	device_links_read_unlock(idx);
 }
 
+static void device_link_rpm_prepare(struct device *consumer,
+				    struct device *supplier)
+{
+	pm_runtime_new_link(consumer);
+	/*
+	 * If the link is being added by the consumer driver at probe time,
+	 * balance the decrementation of the supplier's runtime PM usage counter
+	 * after consumer probe in driver_probe_device().
+	 */
+	if (consumer->links.status == DL_DEV_PROBING)
+		pm_runtime_get_noresume(supplier);
+}
+
 /**
  * device_link_add - Create a link between two devices.
  * @consumer: Consumer end of the link.
@@ -202,7 +215,6 @@ struct device_link *device_link_add(stru
 				    struct device *supplier, u32 flags)
 {
 	struct device_link *link;
-	bool rpm_put_supplier = false;
 
 	if (!consumer || !supplier ||
 	    (flags & DL_FLAG_STATELESS &&
@@ -214,7 +226,6 @@ struct device_link *device_link_add(stru
 			pm_runtime_put_noidle(supplier);
 			return NULL;
 		}
-		rpm_put_supplier = true;
 	}
 
 	device_links_write_lock();
@@ -250,6 +261,15 @@ struct device_link *device_link_add(stru
 		if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
 			link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
 
+		if (flags & DL_FLAG_PM_RUNTIME) {
+			if (!(link->flags & DL_FLAG_PM_RUNTIME)) {
+				device_link_rpm_prepare(consumer, supplier);
+				link->flags |= DL_FLAG_PM_RUNTIME;
+			}
+			if (flags & DL_FLAG_RPM_ACTIVE)
+				refcount_inc(&link->rpm_active);
+		}
+
 		kref_get(&link->kref);
 		goto out;
 	}
@@ -258,20 +278,15 @@ struct device_link *device_link_add(stru
 	if (!link)
 		goto out;
 
+	refcount_set(&link->rpm_active, 1);
+
 	if (flags & DL_FLAG_PM_RUNTIME) {
-		if (flags & DL_FLAG_RPM_ACTIVE) {
-			link->rpm_active = true;
-			rpm_put_supplier = false;
-		}
-		pm_runtime_new_link(consumer);
-		/*
-		 * If the link is being added by the consumer driver at probe
-		 * time, balance the decrementation of the supplier's runtime PM
-		 * usage counter after consumer probe in driver_probe_device().
-		 */
-		if (consumer->links.status == DL_DEV_PROBING)
-			pm_runtime_get_noresume(supplier);
+		if (flags & DL_FLAG_RPM_ACTIVE)
+			refcount_inc(&link->rpm_active);
+
+		device_link_rpm_prepare(consumer, supplier);
 	}
+
 	get_device(supplier);
 	link->supplier = supplier;
 	INIT_LIST_HEAD(&link->s_node);
@@ -334,7 +349,7 @@ struct device_link *device_link_add(stru
 	device_pm_unlock();
 	device_links_write_unlock();
 
-	if (rpm_put_supplier)
+	if ((flags & DL_FLAG_PM_RUNTIME && flags & DL_FLAG_RPM_ACTIVE) && !link)
 		pm_runtime_put(supplier);
 
 	return link;
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -853,7 +853,7 @@ struct device_link {
 	struct list_head c_node;
 	enum device_link_state status;
 	u32 flags;
-	bool rpm_active;
+	refcount_t rpm_active;
 	struct kref kref;
 #ifdef CONFIG_SRCU
 	struct rcu_head rcu_head;
Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -271,11 +271,8 @@ static int rpm_get_suppliers(struct devi
 	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
 		int retval;
 
-		if (!(link->flags & DL_FLAG_PM_RUNTIME))
-			continue;
-
-		if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND ||
-		    link->rpm_active)
+		if (!(link->flags & DL_FLAG_PM_RUNTIME) ||
+		    READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND)
 			continue;
 
 		retval = pm_runtime_get_sync(link->supplier);
@@ -284,7 +281,7 @@ static int rpm_get_suppliers(struct devi
 			pm_runtime_put_noidle(link->supplier);
 			return retval;
 		}
-		link->rpm_active = true;
+		refcount_inc(&link->rpm_active);
 	}
 	return 0;
 }
@@ -293,12 +290,13 @@ static void rpm_put_suppliers(struct dev
 {
 	struct device_link *link;
 
-	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
-		if (link->rpm_active &&
-		    READ_ONCE(link->status) != DL_STATE_SUPPLIER_UNBIND) {
+	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+		if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND)
+			continue;
+
+		while (refcount_dec_not_one(&link->rpm_active))
 			pm_runtime_put(link->supplier);
-			link->rpm_active = false;
-		}
+	}
 }
 
 /**
@@ -1551,7 +1549,7 @@ void pm_runtime_remove(struct device *de
  *
  * 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).
+ * (as many times as needed).
  *
  * Links with the DL_FLAG_STATELESS flag set are ignored.
  *
@@ -1573,10 +1571,8 @@ void pm_runtime_clean_up_links(struct de
 		if (link->flags & DL_FLAG_STATELESS)
 			continue;
 
-		if (link->rpm_active) {
+		while (refcount_dec_not_one(&link->rpm_active))
 			pm_runtime_put_noidle(dev);
-			link->rpm_active = false;
-		}
 	}
 
 	device_links_read_unlock(idx);

[PATCH 6/6] driver core: Fix adding device links to probing suppliers

[Rafael J. Wysocki]

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

Currently, it is not valid to add a device link from a consumer
driver ->probe callback to a supplier that is still probing too, but
generally this is a valid use case.  For example, if the consumer has
just acquired a resource that can only be available if the supplier
is functional, adding a device link to that supplier right away
should be safe (and even desirable arguably), but device_link_add()
doesn't handle that case correctly and the initial state of the link
created by it is wrong then.

To address this problem, change the initial state of device links
added between a probing supplier and a probing consumer to
DL_STATE_CONSUMER_PROBE and update device_links_driver_bound() to
skip such links on the supplier side.

With this change, if the supplier probe completes first,
device_links_driver_bound() called for it will skip the link state
update and when it is called for the consumer, the link state will
be updated to "active".  In turn, if the consumer probe completes
first, device_links_driver_bound() called for it will change the
state of the link to "active" and when it is called for the
supplier, the link status update will be skipped.

However, in principle the supplier or consumer probe may still fail
after the link has been added, so modify device_links_no_driver() to
change device links in the "active" or "consumer probe" state to
"dormant" on the supplier side and update __device_links_no_driver()
to change the link state to "available" only if it is "consumer
probe" or "active".

Then, if the supplier probe fails first, the leftover link to the
probing consumer will become "dormant" and device_links_no_driver()
called for the consumer (when its probe fails) will clean it up.
In turn, if the consumer probe fails first, it will either drop the
link, or change its state to "available" and, in the latter case,
when device_links_no_driver() is called for the supplier, it will
update the link state to "dormant".  [If the supplier probe fails,
but the consumer probe succeeds, which should not happen as long as
the consumer driver is correct, the link still will be around, but
it will be "dormant" until the supplier is probed again.]

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

This patch has been tested somewhat already and it has been only
minimally rebased after that.

---
 Documentation/driver-api/device_link.rst |   10 ++--
 drivers/base/core.c                      |   74 +++++++++++++++++++++++++++----
 2 files changed, 73 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -306,17 +306,26 @@ struct device_link *device_link_add(stru
 		link->status = DL_STATE_NONE;
 	} else {
 		switch (supplier->links.status) {
-		case DL_DEV_DRIVER_BOUND:
+		case DL_DEV_PROBING:
 			switch (consumer->links.status) {
 			case DL_DEV_PROBING:
 				/*
-				 * Some callers expect the link creation during
-				 * consumer driver probe to resume the supplier
-				 * even without DL_FLAG_RPM_ACTIVE.
+				 * A consumer driver can create a link to a
+				 * supplier that has not completed its probing
+				 * yet as long as it knows that the supplier is
+				 * already functional (for example, it has just
+				 * acquired some resources from the supplier).
 				 */
-				if (flags & DL_FLAG_PM_RUNTIME)
-					pm_runtime_resume(supplier);
-
+				link->status = DL_STATE_CONSUMER_PROBE;
+				break;
+			default:
+				link->status = DL_STATE_DORMANT;
+				break;
+			}
+			break;
+		case DL_DEV_DRIVER_BOUND:
+			switch (consumer->links.status) {
+			case DL_DEV_PROBING:
 				link->status = DL_STATE_CONSUMER_PROBE;
 				break;
 			case DL_DEV_DRIVER_BOUND:
@@ -337,6 +346,14 @@ struct device_link *device_link_add(stru
 	}
 
 	/*
+	 * Some callers expect the link creation during consumer driver probe to
+	 * resume the supplier even without DL_FLAG_RPM_ACTIVE.
+	 */
+	if (link->status == DL_STATE_CONSUMER_PROBE &&
+	    flags & DL_FLAG_PM_RUNTIME)
+		pm_runtime_resume(supplier);
+
+	/*
 	 * Move the consumer and all of the devices depending on it to the end
 	 * of dpm_list and the devices_kset list.
 	 *
@@ -524,6 +541,16 @@ void device_links_driver_bound(struct de
 		if (link->flags & DL_FLAG_STATELESS)
 			continue;
 
+		/*
+		 * Links created during consumer probe may be in the "consumer
+		 * probe" state to start with if the supplier is still probing
+		 * when they are created and they may become "active" if the
+		 * consumer probe returns first.  Skip them here.
+		 */
+		if (link->status == DL_STATE_CONSUMER_PROBE ||
+		    link->status == DL_STATE_ACTIVE)
+			continue;
+
 		WARN_ON(link->status != DL_STATE_DORMANT);
 		WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
 	}
@@ -561,7 +588,8 @@ static void __device_links_no_driver(str
 		if (link->flags & DL_FLAG_STATELESS)
 			continue;
 
-		if (link->status != DL_STATE_SUPPLIER_UNBIND)
+		if (link->status == DL_STATE_CONSUMER_PROBE ||
+		    link->status == DL_STATE_ACTIVE)
 			WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
 
 		if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER)
@@ -571,10 +599,40 @@ static void __device_links_no_driver(str
 	dev->links.status = DL_DEV_NO_DRIVER;
 }
 
+/**
+ * device_links_no_driver - Update links after failing driver probe.
+ * @dev: Device whose driver has just failed to probe.
+ *
+ * Clean up leftover links to consumers for @dev and invoke
+ * %__device_links_no_driver() to update links to suppliers for it as
+ * appropriate.
+ *
+ * Links with the DL_FLAG_STATELESS flag set are ignored.
+ */
 void device_links_no_driver(struct device *dev)
 {
+	struct device_link *link;
+
 	device_links_write_lock();
+
+	list_for_each_entry(link, &dev->links.consumers, s_node) {
+		if (link->flags & DL_FLAG_STATELESS)
+			continue;
+
+		/*
+		 * The probe has failed, so if the status of the link is
+		 * "consumer probe" or "active", it must have been added by
+		 * a probing consumer while this device was still probing.
+		 * Change its state to "dormant", as it represents a valid
+		 * relationship, but it is not functionally meaningful.
+		 */
+		if (link->status == DL_STATE_CONSUMER_PROBE ||
+		    link->status == DL_STATE_ACTIVE)
+			WRITE_ONCE(link->status, DL_STATE_DORMANT);
+	}
+
 	__device_links_no_driver(dev);
+
 	device_links_write_unlock();
 }
 
Index: linux-pm/Documentation/driver-api/device_link.rst
===================================================================
--- linux-pm.orig/Documentation/driver-api/device_link.rst
+++ linux-pm/Documentation/driver-api/device_link.rst
@@ -59,11 +59,15 @@ device ``->probe`` callback or a boot-ti
 
 Another example for an inconsistent state would be a device link that
 represents a driver presence dependency, yet is added from the consumer's
-``->probe`` callback while the supplier hasn't probed yet:  Had the driver
-core known about the device link earlier, it wouldn't have probed the
+``->probe`` callback while the supplier hasn't started to probe yet:  Had the
+driver core known about the device link earlier, it wouldn't have probed the
 consumer in the first place.  The onus is thus on the consumer to check
 presence of the supplier after adding the link, and defer probing on
-non-presence.
+non-presence.  [Note that it is valid to create a link from the consumer's
+``->probe`` callback while the supplier is still probing, but the consumer must
+know that the supplier is functional already at the link creation time (that is
+the case, for instance, if the consumer has just acquired some resources that
+would not have been available had the supplier not been functional then).]
 
 If a device link is added in the ``->probe`` callback of the supplier or
 consumer driver, it is typically deleted in its ``->remove`` callback for

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Russell King - ARM Linux admin]

On Thu, Jan 24, 2019 at 12:13:06PM +0100, Rafael J. Wysocki wrote:
> Hi Greg at al,
> 
> Recently I have been looking at the device links code because of the
> recent discussion on possibly using them in the DRM subsystem (see for
> example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> found a few issues in that code which should be addressed by this patch
> series.  Please refer to the patch changelogs for details.
> 
> None of the problems addressed here should be manifesting themselves in
> mainline kernel today, but if there are more device links users in the
> future, they most likely will be encountered sooner or later.  Also they
> need to be fixed for the DRM use case to be supported IMO.
> 
> This series does not fix all issues in device links that have become
> apparent (generally speaking, the idea of returning an existing link
> in case there is one already for the given consumer-supplier pair
> doesn't play well with stateful links and their flags), so there will
> be a follow-up series of patches to clean that up.  Still, I don't see
> a reason to sit on these fixes while working on the other patches, so
> here they go.

Hi Rafael,

I've just given these a quick boot, unbind and rebind test with my
DRM case, and nothing seems to have regressed on the face of it.

Tested-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Rafael J. Wysocki]

On Thu, Jan 24, 2019 at 3:58 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jan 24, 2019 at 12:13:06PM +0100, Rafael J. Wysocki wrote:
> > Hi Greg at al,
> >
> > Recently I have been looking at the device links code because of the
> > recent discussion on possibly using them in the DRM subsystem (see for
> > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > found a few issues in that code which should be addressed by this patch
> > series.  Please refer to the patch changelogs for details.
> >
> > None of the problems addressed here should be manifesting themselves in
> > mainline kernel today, but if there are more device links users in the
> > future, they most likely will be encountered sooner or later.  Also they
> > need to be fixed for the DRM use case to be supported IMO.
> >
> > This series does not fix all issues in device links that have become
> > apparent (generally speaking, the idea of returning an existing link
> > in case there is one already for the given consumer-supplier pair
> > doesn't play well with stateful links and their flags), so there will
> > be a follow-up series of patches to clean that up.  Still, I don't see
> > a reason to sit on these fixes while working on the other patches, so
> > here they go.
>
> Hi Rafael,
>
> I've just given these a quick boot, unbind and rebind test with my
> DRM case, and nothing seems to have regressed on the face of it.
>
> Tested-by: Russell King <rmk+kernel@armlinux.org.uk>

Thank you!

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Rafael J. Wysocki]

On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Greg at al,
>
> Recently I have been looking at the device links code because of the
> recent discussion on possibly using them in the DRM subsystem (see for
> example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> found a few issues in that code which should be addressed by this patch
> series.  Please refer to the patch changelogs for details.
>
> None of the problems addressed here should be manifesting themselves in
> mainline kernel today, but if there are more device links users in the
> future, they most likely will be encountered sooner or later.  Also they
> need to be fixed for the DRM use case to be supported IMO.
>
> This series does not fix all issues in device links that have become
> apparent (generally speaking, the idea of returning an existing link
> in case there is one already for the given consumer-supplier pair
> doesn't play well with stateful links and their flags), so there will
> be a follow-up series of patches to clean that up.  Still, I don't see
> a reason to sit on these fixes while working on the other patches, so
> here they go.

Any concerns regarding this lot?

[Please note that patch 5 in the series was replaced with the v2 at
https://patchwork.kernel.org/patch/10781205/]

If not, and if you don't mind, I would like to queue it up next week,
possibly along with the follow-up material posted on Monday
(https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
that is not problematic, so it gets some linux-next coverage before
the next merge window.

Cheers,
Rafael

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Greg Kroah-Hartman]

On Thu, Jan 31, 2019 at 11:09:51AM +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi Greg at al,
> >
> > Recently I have been looking at the device links code because of the
> > recent discussion on possibly using them in the DRM subsystem (see for
> > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > found a few issues in that code which should be addressed by this patch
> > series.  Please refer to the patch changelogs for details.
> >
> > None of the problems addressed here should be manifesting themselves in
> > mainline kernel today, but if there are more device links users in the
> > future, they most likely will be encountered sooner or later.  Also they
> > need to be fixed for the DRM use case to be supported IMO.
> >
> > This series does not fix all issues in device links that have become
> > apparent (generally speaking, the idea of returning an existing link
> > in case there is one already for the given consumer-supplier pair
> > doesn't play well with stateful links and their flags), so there will
> > be a follow-up series of patches to clean that up.  Still, I don't see
> > a reason to sit on these fixes while working on the other patches, so
> > here they go.
> 
> Any concerns regarding this lot?
> 
> [Please note that patch 5 in the series was replaced with the v2 at
> https://patchwork.kernel.org/patch/10781205/]
> 
> If not, and if you don't mind, I would like to queue it up next week,
> possibly along with the follow-up material posted on Monday
> (https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
> that is not problematic, so it gets some linux-next coverage before
> the next merge window.

Can I queue it up in my tree, given that I have a number of other driver
core patches in there, and I don't know how the merge issues will be if
we start to diverge.

Or do you need this for some other work?

thanks,

greg k-h

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Greg Kroah-Hartman]

On Thu, Jan 31, 2019 at 02:22:47PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 31, 2019 at 11:09:51AM +0100, Rafael J. Wysocki wrote:
> > On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > Hi Greg at al,
> > >
> > > Recently I have been looking at the device links code because of the
> > > recent discussion on possibly using them in the DRM subsystem (see for
> > > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > > found a few issues in that code which should be addressed by this patch
> > > series.  Please refer to the patch changelogs for details.
> > >
> > > None of the problems addressed here should be manifesting themselves in
> > > mainline kernel today, but if there are more device links users in the
> > > future, they most likely will be encountered sooner or later.  Also they
> > > need to be fixed for the DRM use case to be supported IMO.
> > >
> > > This series does not fix all issues in device links that have become
> > > apparent (generally speaking, the idea of returning an existing link
> > > in case there is one already for the given consumer-supplier pair
> > > doesn't play well with stateful links and their flags), so there will
> > > be a follow-up series of patches to clean that up.  Still, I don't see
> > > a reason to sit on these fixes while working on the other patches, so
> > > here they go.
> > 
> > Any concerns regarding this lot?
> > 
> > [Please note that patch 5 in the series was replaced with the v2 at
> > https://patchwork.kernel.org/patch/10781205/]
> > 
> > If not, and if you don't mind, I would like to queue it up next week,
> > possibly along with the follow-up material posted on Monday
> > (https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
> > that is not problematic, so it gets some linux-next coverage before
> > the next merge window.
> 
> Can I queue it up in my tree, given that I have a number of other driver
> core patches in there, and I don't know how the merge issues will be if
> we start to diverge.
> 
> Or do you need this for some other work?

To make this clearer, I have no objection to take this through my tree
now, along with your second set of patches.  Is that ok?

thanks,

greg k-h

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Rafael J. Wysocki]

On Thu, Jan 31, 2019 at 2:24 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 31, 2019 at 02:22:47PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 31, 2019 at 11:09:51AM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > Hi Greg at al,
> > > >
> > > > Recently I have been looking at the device links code because of the
> > > > recent discussion on possibly using them in the DRM subsystem (see for
> > > > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > > > found a few issues in that code which should be addressed by this patch
> > > > series.  Please refer to the patch changelogs for details.
> > > >
> > > > None of the problems addressed here should be manifesting themselves in
> > > > mainline kernel today, but if there are more device links users in the
> > > > future, they most likely will be encountered sooner or later.  Also they
> > > > need to be fixed for the DRM use case to be supported IMO.
> > > >
> > > > This series does not fix all issues in device links that have become
> > > > apparent (generally speaking, the idea of returning an existing link
> > > > in case there is one already for the given consumer-supplier pair
> > > > doesn't play well with stateful links and their flags), so there will
> > > > be a follow-up series of patches to clean that up.  Still, I don't see
> > > > a reason to sit on these fixes while working on the other patches, so
> > > > here they go.
> > >
> > > Any concerns regarding this lot?
> > >
> > > [Please note that patch 5 in the series was replaced with the v2 at
> > > https://patchwork.kernel.org/patch/10781205/]
> > >
> > > If not, and if you don't mind, I would like to queue it up next week,
> > > possibly along with the follow-up material posted on Monday
> > > (https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
> > > that is not problematic, so it gets some linux-next coverage before
> > > the next merge window.
> >
> > Can I queue it up in my tree, given that I have a number of other driver
> > core patches in there, and I don't know how the merge issues will be if
> > we start to diverge.
> >
> > Or do you need this for some other work?
>
> To make this clearer, I have no objection to take this through my tree
> now, along with your second set of patches.  Is that ok?

Yes, it is, AFAICS.  Thank you!

Do you need me to resend all of this as one series?

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Greg Kroah-Hartman]

On Thu, Jan 31, 2019 at 05:02:05PM +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 31, 2019 at 2:24 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jan 31, 2019 at 02:22:47PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Jan 31, 2019 at 11:09:51AM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > >
> > > > > Hi Greg at al,
> > > > >
> > > > > Recently I have been looking at the device links code because of the
> > > > > recent discussion on possibly using them in the DRM subsystem (see for
> > > > > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > > > > found a few issues in that code which should be addressed by this patch
> > > > > series.  Please refer to the patch changelogs for details.
> > > > >
> > > > > None of the problems addressed here should be manifesting themselves in
> > > > > mainline kernel today, but if there are more device links users in the
> > > > > future, they most likely will be encountered sooner or later.  Also they
> > > > > need to be fixed for the DRM use case to be supported IMO.
> > > > >
> > > > > This series does not fix all issues in device links that have become
> > > > > apparent (generally speaking, the idea of returning an existing link
> > > > > in case there is one already for the given consumer-supplier pair
> > > > > doesn't play well with stateful links and their flags), so there will
> > > > > be a follow-up series of patches to clean that up.  Still, I don't see
> > > > > a reason to sit on these fixes while working on the other patches, so
> > > > > here they go.
> > > >
> > > > Any concerns regarding this lot?
> > > >
> > > > [Please note that patch 5 in the series was replaced with the v2 at
> > > > https://patchwork.kernel.org/patch/10781205/]
> > > >
> > > > If not, and if you don't mind, I would like to queue it up next week,
> > > > possibly along with the follow-up material posted on Monday
> > > > (https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
> > > > that is not problematic, so it gets some linux-next coverage before
> > > > the next merge window.
> > >
> > > Can I queue it up in my tree, given that I have a number of other driver
> > > core patches in there, and I don't know how the merge issues will be if
> > > we start to diverge.
> > >
> > > Or do you need this for some other work?
> >
> > To make this clearer, I have no objection to take this through my tree
> > now, along with your second set of patches.  Is that ok?
> 
> Yes, it is, AFAICS.  Thank you!
> 
> Do you need me to resend all of this as one series?

Yes, can you please do that.  Also, can you rebase it against my
driver-core-next branch as right now, patch 1/6 has conflicts due to
other patches that are in my tree :(

thanks,

greg k-h

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Rafael J. Wysocki]

On Thu, Jan 31, 2019 at 7:24 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 31, 2019 at 05:02:05PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Jan 31, 2019 at 2:24 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Jan 31, 2019 at 02:22:47PM +0100, Greg Kroah-Hartman wrote:
> > > > On Thu, Jan 31, 2019 at 11:09:51AM +0100, Rafael J. Wysocki wrote:
> > > > > On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > >
> > > > > > Hi Greg at al,
> > > > > >
> > > > > > Recently I have been looking at the device links code because of the
> > > > > > recent discussion on possibly using them in the DRM subsystem (see for
> > > > > > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > > > > > found a few issues in that code which should be addressed by this patch
> > > > > > series.  Please refer to the patch changelogs for details.
> > > > > >
> > > > > > None of the problems addressed here should be manifesting themselves in
> > > > > > mainline kernel today, but if there are more device links users in the
> > > > > > future, they most likely will be encountered sooner or later.  Also they
> > > > > > need to be fixed for the DRM use case to be supported IMO.
> > > > > >
> > > > > > This series does not fix all issues in device links that have become
> > > > > > apparent (generally speaking, the idea of returning an existing link
> > > > > > in case there is one already for the given consumer-supplier pair
> > > > > > doesn't play well with stateful links and their flags), so there will
> > > > > > be a follow-up series of patches to clean that up.  Still, I don't see
> > > > > > a reason to sit on these fixes while working on the other patches, so
> > > > > > here they go.
> > > > >
> > > > > Any concerns regarding this lot?
> > > > >
> > > > > [Please note that patch 5 in the series was replaced with the v2 at
> > > > > https://patchwork.kernel.org/patch/10781205/]
> > > > >
> > > > > If not, and if you don't mind, I would like to queue it up next week,
> > > > > possibly along with the follow-up material posted on Monday
> > > > > (https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
> > > > > that is not problematic, so it gets some linux-next coverage before
> > > > > the next merge window.
> > > >
> > > > Can I queue it up in my tree, given that I have a number of other driver
> > > > core patches in there, and I don't know how the merge issues will be if
> > > > we start to diverge.
> > > >
> > > > Or do you need this for some other work?
> > >
> > > To make this clearer, I have no objection to take this through my tree
> > > now, along with your second set of patches.  Is that ok?
> >
> > Yes, it is, AFAICS.  Thank you!
> >
> > Do you need me to resend all of this as one series?
>
> Yes, can you please do that.  Also, can you rebase it against my
> driver-core-next branch as right now, patch 1/6 has conflicts due to
> other patches that are in my tree :(

I'll do that, thanks!

Cheers,
Rafael

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Rafael J. Wysocki]

On Thursday, January 31, 2019 7:24:07 PM CET Greg Kroah-Hartman wrote:
> On Thu, Jan 31, 2019 at 05:02:05PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Jan 31, 2019 at 2:24 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Jan 31, 2019 at 02:22:47PM +0100, Greg Kroah-Hartman wrote:
> > > > On Thu, Jan 31, 2019 at 11:09:51AM +0100, Rafael J. Wysocki wrote:
> > > > > On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > >
> > > > > > Hi Greg at al,
> > > > > >
> > > > > > Recently I have been looking at the device links code because of the
> > > > > > recent discussion on possibly using them in the DRM subsystem (see for
> > > > > > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > > > > > found a few issues in that code which should be addressed by this patch
> > > > > > series.  Please refer to the patch changelogs for details.
> > > > > >
> > > > > > None of the problems addressed here should be manifesting themselves in
> > > > > > mainline kernel today, but if there are more device links users in the
> > > > > > future, they most likely will be encountered sooner or later.  Also they
> > > > > > need to be fixed for the DRM use case to be supported IMO.
> > > > > >
> > > > > > This series does not fix all issues in device links that have become
> > > > > > apparent (generally speaking, the idea of returning an existing link
> > > > > > in case there is one already for the given consumer-supplier pair
> > > > > > doesn't play well with stateful links and their flags), so there will
> > > > > > be a follow-up series of patches to clean that up.  Still, I don't see
> > > > > > a reason to sit on these fixes while working on the other patches, so
> > > > > > here they go.
> > > > >
> > > > > Any concerns regarding this lot?
> > > > >
> > > > > [Please note that patch 5 in the series was replaced with the v2 at
> > > > > https://patchwork.kernel.org/patch/10781205/]
> > > > >
> > > > > If not, and if you don't mind, I would like to queue it up next week,
> > > > > possibly along with the follow-up material posted on Monday
> > > > > (https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
> > > > > that is not problematic, so it gets some linux-next coverage before
> > > > > the next merge window.
> > > >
> > > > Can I queue it up in my tree, given that I have a number of other driver
> > > > core patches in there, and I don't know how the merge issues will be if
> > > > we start to diverge.
> > > >
> > > > Or do you need this for some other work?
> > >
> > > To make this clearer, I have no objection to take this through my tree
> > > now, along with your second set of patches.  Is that ok?
> > 
> > Yes, it is, AFAICS.  Thank you!
> > 
> > Do you need me to resend all of this as one series?
> 
> Yes, can you please do that.  Also, can you rebase it against my
> driver-core-next branch as right now, patch 1/6 has conflicts due to
> other patches that are in my tree :(

So commit 0fe6f7874d467 in your driver-core-next branch, which is the source
of this conflict and I can't recall seeing that patch, if missing a Fixes: tag.

Cheers,
Rafael

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Greg Kroah-Hartman]

On Fri, Feb 01, 2019 at 12:50:58AM +0100, Rafael J. Wysocki wrote:
> On Thursday, January 31, 2019 7:24:07 PM CET Greg Kroah-Hartman wrote:
> > On Thu, Jan 31, 2019 at 05:02:05PM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Jan 31, 2019 at 2:24 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Jan 31, 2019 at 02:22:47PM +0100, Greg Kroah-Hartman wrote:
> > > > > On Thu, Jan 31, 2019 at 11:09:51AM +0100, Rafael J. Wysocki wrote:
> > > > > > On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > > >
> > > > > > > Hi Greg at al,
> > > > > > >
> > > > > > > Recently I have been looking at the device links code because of the
> > > > > > > recent discussion on possibly using them in the DRM subsystem (see for
> > > > > > > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > > > > > > found a few issues in that code which should be addressed by this patch
> > > > > > > series.  Please refer to the patch changelogs for details.
> > > > > > >
> > > > > > > None of the problems addressed here should be manifesting themselves in
> > > > > > > mainline kernel today, but if there are more device links users in the
> > > > > > > future, they most likely will be encountered sooner or later.  Also they
> > > > > > > need to be fixed for the DRM use case to be supported IMO.
> > > > > > >
> > > > > > > This series does not fix all issues in device links that have become
> > > > > > > apparent (generally speaking, the idea of returning an existing link
> > > > > > > in case there is one already for the given consumer-supplier pair
> > > > > > > doesn't play well with stateful links and their flags), so there will
> > > > > > > be a follow-up series of patches to clean that up.  Still, I don't see
> > > > > > > a reason to sit on these fixes while working on the other patches, so
> > > > > > > here they go.
> > > > > >
> > > > > > Any concerns regarding this lot?
> > > > > >
> > > > > > [Please note that patch 5 in the series was replaced with the v2 at
> > > > > > https://patchwork.kernel.org/patch/10781205/]
> > > > > >
> > > > > > If not, and if you don't mind, I would like to queue it up next week,
> > > > > > possibly along with the follow-up material posted on Monday
> > > > > > (https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
> > > > > > that is not problematic, so it gets some linux-next coverage before
> > > > > > the next merge window.
> > > > >
> > > > > Can I queue it up in my tree, given that I have a number of other driver
> > > > > core patches in there, and I don't know how the merge issues will be if
> > > > > we start to diverge.
> > > > >
> > > > > Or do you need this for some other work?
> > > >
> > > > To make this clearer, I have no objection to take this through my tree
> > > > now, along with your second set of patches.  Is that ok?
> > > 
> > > Yes, it is, AFAICS.  Thank you!
> > > 
> > > Do you need me to resend all of this as one series?
> > 
> > Yes, can you please do that.  Also, can you rebase it against my
> > driver-core-next branch as right now, patch 1/6 has conflicts due to
> > other patches that are in my tree :(
> 
> So commit 0fe6f7874d467 in your driver-core-next branch, which is the source
> of this conflict and I can't recall seeing that patch, if missing a Fixes: tag.

It was sent on the 1st, and seems to have come from this thread:
	https://lore.kernel.org/lkml/1546318276-18993-3-git-send-email-yong.wu@mediatek.com/

I'll gladly revert it if you do not think it is correct.

Hm, in looking at that closer, it does feel odd that the reference count
is being ignored, that doesn't seem right, and might be a driver bug...

Let me look at it again in the morning...

greg k-h

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Rafael J. Wysocki]

On Friday, February 1, 2019 1:12:31 AM CET Greg Kroah-Hartman wrote:
> On Fri, Feb 01, 2019 at 12:50:58AM +0100, Rafael J. Wysocki wrote:
> > On Thursday, January 31, 2019 7:24:07 PM CET Greg Kroah-Hartman wrote:
> > > On Thu, Jan 31, 2019 at 05:02:05PM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Jan 31, 2019 at 2:24 PM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, Jan 31, 2019 at 02:22:47PM +0100, Greg Kroah-Hartman wrote:
> > > > > > On Thu, Jan 31, 2019 at 11:09:51AM +0100, Rafael J. Wysocki wrote:
> > > > > > > On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > > > >
> > > > > > > > Hi Greg at al,
> > > > > > > >
> > > > > > > > Recently I have been looking at the device links code because of the
> > > > > > > > recent discussion on possibly using them in the DRM subsystem (see for
> > > > > > > > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > > > > > > > found a few issues in that code which should be addressed by this patch
> > > > > > > > series.  Please refer to the patch changelogs for details.
> > > > > > > >
> > > > > > > > None of the problems addressed here should be manifesting themselves in
> > > > > > > > mainline kernel today, but if there are more device links users in the
> > > > > > > > future, they most likely will be encountered sooner or later.  Also they
> > > > > > > > need to be fixed for the DRM use case to be supported IMO.
> > > > > > > >
> > > > > > > > This series does not fix all issues in device links that have become
> > > > > > > > apparent (generally speaking, the idea of returning an existing link
> > > > > > > > in case there is one already for the given consumer-supplier pair
> > > > > > > > doesn't play well with stateful links and their flags), so there will
> > > > > > > > be a follow-up series of patches to clean that up.  Still, I don't see
> > > > > > > > a reason to sit on these fixes while working on the other patches, so
> > > > > > > > here they go.
> > > > > > >
> > > > > > > Any concerns regarding this lot?
> > > > > > >
> > > > > > > [Please note that patch 5 in the series was replaced with the v2 at
> > > > > > > https://patchwork.kernel.org/patch/10781205/]
> > > > > > >
> > > > > > > If not, and if you don't mind, I would like to queue it up next week,
> > > > > > > possibly along with the follow-up material posted on Monday
> > > > > > > (https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
> > > > > > > that is not problematic, so it gets some linux-next coverage before
> > > > > > > the next merge window.
> > > > > >
> > > > > > Can I queue it up in my tree, given that I have a number of other driver
> > > > > > core patches in there, and I don't know how the merge issues will be if
> > > > > > we start to diverge.
> > > > > >
> > > > > > Or do you need this for some other work?
> > > > >
> > > > > To make this clearer, I have no objection to take this through my tree
> > > > > now, along with your second set of patches.  Is that ok?
> > > > 
> > > > Yes, it is, AFAICS.  Thank you!
> > > > 
> > > > Do you need me to resend all of this as one series?
> > > 
> > > Yes, can you please do that.  Also, can you rebase it against my
> > > driver-core-next branch as right now, patch 1/6 has conflicts due to
> > > other patches that are in my tree :(
> > 
> > So commit 0fe6f7874d467 in your driver-core-next branch, which is the source
> > of this conflict and I can't recall seeing that patch, if missing a Fixes: tag.
> 
> It was sent on the 1st, and seems to have come from this thread:
> 	https://lore.kernel.org/lkml/1546318276-18993-3-git-send-email-yong.wu@mediatek.com/
> 
> I'll gladly revert it if you do not think it is correct.

It is not entirely correct, but that's not going to matter after my series
(and I've already rebased it on top of that commit, so please hold on to it :-)).

> Hm, in looking at that closer, it does feel odd that the reference count
> is being ignored, that doesn't seem right, and might be a driver bug...

This is a device links code issue which was introduced along with the reference
counting that was kind of an afterthought and not really well thought through
IMO.

> Let me look at it again in the morning...

So one of the goals of my series is to clean up that mess (and sorry about
letting it in in the first place).

Cheers,
Rafael

Re: [PATCH 0/6] driver core: Fix some issues related to device links

[Ulf Hansson]

On Thu, 31 Jan 2019 at 14:24, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 31, 2019 at 02:22:47PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 31, 2019 at 11:09:51AM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Jan 24, 2019 at 12:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > Hi Greg at al,
> > > >
> > > > Recently I have been looking at the device links code because of the
> > > > recent discussion on possibly using them in the DRM subsystem (see for
> > > > example https://marc.info/?l=linux-pm&m=154832771905309&w=2) and I have
> > > > found a few issues in that code which should be addressed by this patch
> > > > series.  Please refer to the patch changelogs for details.
> > > >
> > > > None of the problems addressed here should be manifesting themselves in
> > > > mainline kernel today, but if there are more device links users in the
> > > > future, they most likely will be encountered sooner or later.  Also they
> > > > need to be fixed for the DRM use case to be supported IMO.
> > > >
> > > > This series does not fix all issues in device links that have become
> > > > apparent (generally speaking, the idea of returning an existing link
> > > > in case there is one already for the given consumer-supplier pair
> > > > doesn't play well with stateful links and their flags), so there will
> > > > be a follow-up series of patches to clean that up.  Still, I don't see
> > > > a reason to sit on these fixes while working on the other patches, so
> > > > here they go.
> > >
> > > Any concerns regarding this lot?
> > >
> > > [Please note that patch 5 in the series was replaced with the v2 at
> > > https://patchwork.kernel.org/patch/10781205/]
> > >
> > > If not, and if you don't mind, I would like to queue it up next week,
> > > possibly along with the follow-up material posted on Monday
> > > (https://lore.kernel.org/lkml/2405639.4es7pRLqn0@aspire.rjw.lan/) if
> > > that is not problematic, so it gets some linux-next coverage before
> > > the next merge window.
> >
> > Can I queue it up in my tree, given that I have a number of other driver
> > core patches in there, and I don't know how the merge issues will be if
> > we start to diverge.
> >
> > Or do you need this for some other work?
>
> To make this clearer, I have no objection to take this through my tree
> now, along with your second set of patches.  Is that ok?
>

Greg, Rafael,

FWIW, I intend to review these patches during tomorrow, or at latest
on Monday as I have been a bit busy lately.

Kind regards
Uffe