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

Groups > linux.kernel > #1479523 > unrolled thread

[RFC/RFT][PATCH v2 0/7] Functional dependencies between devices

Back to article view | Back to linux.kernel

Contents

  [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-08 23:30 +0200
    [RFC/RFT][PATCH v2 7/7] PM / runtime: Optimize the use of device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-08 23:30 +0200
    [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep transitions in progress "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-08 23:30 +0200
      Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep  transitions in progress Lukas Wunner <lukas@wunner.de> - 2016-09-12 16:10 +0200
        Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep transitions in progress "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-12 23:20 +0200
          Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep  transitions in progress Lukas Wunner <lukas@wunner.de> - 2016-09-13 01:00 +0200
          Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep  transitions in progress Marek Szyprowski <m.szyprowski@samsung.com> - 2016-09-13 09:30 +0200
            Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep transitions in progress "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-14 02:00 +0200
    [RFC/RFT][PATCH v2 4/7] PM / runtime: Pass flags argument to __pm_runtime_disable() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-08 23:30 +0200
    Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-08 23:30 +0200
    [RFC/RFT][PATCH v2 1/7] driver core: Add a wrapper around __device_release_driver() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-08 23:30 +0200
    [RFC/RFT][PATCH v2 3/7] PM / sleep: Make async suspend/resume of devices use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-08 23:30 +0200
      Re: [RFC/RFT][PATCH v2 3/7] PM / sleep: Make async suspend/resume of  devices use device links Lukas Wunner <lukas@wunner.de> - 2016-09-10 15:40 +0200
        Re: [RFC/RFT][PATCH v2 3/7] PM / sleep: Make async suspend/resume of  devices use device links "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-11 00:20 +0200
    [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-08 23:30 +0200
      Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies  tracking support Ulf Hansson <ulf.hansson@linaro.org> - 2016-09-09 10:30 +0200
        Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies  tracking support Mark Brown <broonie@kernel.org> - 2016-09-09 14:10 +0200
          Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies  tracking support Ulf Hansson <ulf.hansson@linaro.org> - 2016-09-09 16:20 +0200
        Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-15 03:10 +0200
      Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies  tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-11 15:50 +0200
        Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies  tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-11 22:50 +0200
          Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-14 03:20 +0200
            Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies  tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-14 10:30 +0200
              Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-14 15:20 +0200
    [RFC/RFT][PATCH v2 6/7] PM / runtime: Use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-08 23:30 +0200
      Re: [RFC/RFT][PATCH v2 6/7] PM / runtime: Use device links Lukas Wunner <lukas@wunner.de> - 2016-09-12 11:50 +0200
        Re: [RFC/RFT][PATCH v2 6/7] PM / runtime: Use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-12 16:00 +0200
          Re: [RFC/RFT][PATCH v2 6/7] PM / runtime: Use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-14 03:20 +0200
    Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices Lukas Wunner <lukas@wunner.de> - 2016-09-10 13:40 +0200
      Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-11 00:10 +0200
        Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices Lukas Wunner <lukas@wunner.de> - 2016-09-13 20:00 +0200
          Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-14 01:20 +0200
            Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices Lukas Wunner <lukas@wunner.de> - 2016-09-18 14:40 +0200
    Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices Marek Szyprowski <m.szyprowski@samsung.com> - 2016-09-13 12:00 +0200
      Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-14 00:40 +0200
        Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices Lukas Wunner <lukas@wunner.de> - 2016-09-18 13:30 +0200
    [RFC/RFT][PATCH v3 5/5] PM / runtime: Optimize the use of device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-16 00:10 +0200
    [Resend][RFC/RFT][PATCH v3 1/5] driver core: Add a wrapper around __device_release_driver() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-16 00:10 +0200
    [RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-16 00:10 +0200
      Re: [RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies  tracking support Marek Szyprowski <m.szyprowski@samsung.com> - 2016-09-16 10:00 +0200
        Re: [RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies  tracking support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-16 14:10 +0200
      [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-16 14:30 +0200
        Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-20 00:50 +0200
    [RFC/RFT][PATCH v3 3/5] PM / sleep: Make async suspend/resume of devices use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-16 00:10 +0200
    [RFC/RFT][PATCH v3 0/5] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-16 00:10 +0200
      [RFC/RFT][PATCH v3 4/5] PM / runtime: Use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-16 00:10 +0200
      Re: [RFC/RFT][PATCH v3 0/5] Functional dependencies between devices Marek Szyprowski <m.szyprowski@samsung.com> - 2016-09-16 09:30 +0200
        Re: [RFC/RFT][PATCH v3 0/5] Functional dependencies between devices Marek Szyprowski <m.szyprowski@samsung.com> - 2016-09-16 10:00 +0200
          Re: [RFC/RFT][PATCH v3 0/5] Functional dependencies between devices "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-16 14:10 +0200

Page 1 of 3  [1] 2 3  Next page →

#1479523 — [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices

Hi Everyone,

This is a refresh of the functional dependencies series that I posted last
year and which has picked up by Marek quite recently.  For reference, appended
is my introductory message sent previously (which may be slightly outdated now).

As last time, the first patch rearranges the code around __device_release_driver()
a bit to prepare it for the next one (it actually hasn't changed AFAICS).

The second patch introduces the actual device links mechanics, but without
system suspend/resume and runtime PM support which are added by the subsequent
patches.

Some bugs found by Marek during his work on these patches should be fixed
here.  In particular, the endless recursion in device_reorder_to_tail()
which simply was broken before.

There are two additional patches to address the issue with runtime PM support
that occured when runtime PM was disabled for some suppliers due to a PM
sleep transition in progress.  Those patches simply make runtime PM helpers
return 0 in that case which may be controversial, so please let me know if
there are concerns about those.

The way device_link_add() works is a bit different, as it takes an additional
status argument now.  That makes it possible to create a link in any state,
with extra care of course, and should address the problem pointed to by Lukas
during the previous discussion.

Also some comments from Tomeu have been addressed.

This hasn't been really tested yet and I'm sort of relying on Marek to test
it, because he has a use case ready.  Hence, the RFT tag on the series.

Overall, please let me know what you think.

Thanks,
Rafael


Introduction:

As discussed in the recent "On-demand device probing" thread and in a Kernel
Summit session earlier today, 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).

So I want to be able to represent those functional dependencies between devices
and I'd like 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).

My idea is to represent a supplier-consumer dependency between devices (or
more precisely between device+driver combos) as a "link" object containing
pointers to the devices in question, a list node for each of them and some
additional information related to the management of those objects, ie.
something like:

struct device_link {
        struct device *supplier;
        struct list_head supplier_node;
        struct device *consumer;
        struct list_head consumer_node;
        <flags, status etc>
};

In general, there will be two lists of those things per device, one list
of links to consumers and one list of links to suppliers.

In that picture, links will be created by calling, say:

int device_add_link(struct device *me, struct device *my_supplier, unsigned int flags);

and they will be deleted by the driver core when not needed any more.  The
creation of a link should also cause dpm_list and the list used during shutdown
to be reordered if needed.

In principle, it seems usefult to consider two types of links, one created
at device registration time (when registering the second device from the linked
pair, whichever it is) and one created at probe time (of the consumer device).
I'll refer to them as "permanent" and "probe-time" links, respectively.

The permanent links (created at device registration time) will stay around
until one of the linked devices is unregistered (at which time the driver
core will drop the link along with the device going away).  The probe-time
ones will be dropped (automatically) at the consumer device driver unbind time.

There's a question about what if the supplier device is being unbound before
the consumer one (for example, as a result of a hotplug event).  My current
view on that is that the consumer needs to be force-unbound in that case too,
but I guess I may be persuaded otherwise given sufficiently convincing
arguments.  Anyway, there are reasons to do that, like for example it may
help with the synchronization.  Namely, if there's a rule that suppliers
cannot be unbound before any consumers linked to them, than the list of links
to suppliers for a consumer can only change at its registration/probe or
unbind/remove times (which simplifies things quite a bit).

With that, the permanent links existing at the probe time for a consumer
device can be used to check whether or not to defer the probing of it
even before executing its probe callback.  In turn, system suspend
synchronization should be a matter of calling device_pm_wait_for_dev()
for all consumers of a supplier device, in analogy with dpm_wait_for_children(),
and so on.

Of course, the new lists have to be stable during those operations and ensuring
that is going to be somewhat tricky (AFAICS right now at least), but apart from
that the whole concept looks reasonably straightforward to me.

[toc] | [next] | [standalone]

#1479524 — [RFC/RFT][PATCH v2 7/7] PM / runtime: Optimize the use of device links

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

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

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

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/core.c          |    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(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -151,6 +151,9 @@ struct device_link *device_link_add(stru
 	link->consumer = consumer;
 	INIT_LIST_HEAD(&link->c_node);
 	spin_lock_init(&link->lock);
+	if (flags & DEVICE_LINK_PM_RUNTIME)
+		pm_runtime_new_link(consumer);
+
 	link->flags = flags;
 	link->status = status;
 
@@ -191,6 +194,9 @@ static void __device_link_del(struct dev
 	dev_info(link->consumer, "Dropping the link to %s\n",
 		 dev_name(link->supplier));
 
+	if (link->flags & DEVICE_LINK_PM_RUNTIME)
+		pm_runtime_drop_link(link->consumer);
+
 	list_del_rcu(&link->s_node);
 	list_del_rcu(&link->c_node);
 	call_srcu(&device_links_srcu, &link->rcu_head, __device_link_free_srcu);
Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -270,6 +270,7 @@ static int __rpm_callback(int (*cb)(stru
 {
 	struct device_link *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)(stru
 		 * routine returns, so it is safe to read the status outside of
 		 * the lock.
 		 */
-		if (dev->power.runtime_status == RPM_RESUMING) {
+		if (use_links && dev->power.runtime_status == RPM_RESUMING) {
 			idx = device_links_read_lock();
 
 			list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node)
@@ -314,8 +315,9 @@ static int __rpm_callback(int (*cb)(stru
 		 *
 		 * Do that if resume fails too.
 		 */
-		if ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
-		    || (dev->power.runtime_status == RPM_RESUMING && retval)) {
+		if (use_links
+		    && ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
+		    || (dev->power.runtime_status == RPM_RESUMING && retval))) {
 			idx = device_links_read_lock();
 
  fail:
@@ -1546,6 +1548,21 @@ void pm_runtime_clean_up_links(struct de
 	device_links_read_unlock(idx);
 }
 
+void pm_runtime_new_link(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.links_count++;
+	spin_unlock_irq(&dev->power.lock);
+}
+
+void pm_runtime_drop_link(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	WARN_ON(dev->power.links_count == 0);
+	dev->power.links_count--;
+	spin_unlock_irq(&dev->power.lock);
+}
+
 /**
  * pm_runtime_force_suspend - Force a device into suspend state if needed.
  * @dev: Device to suspend.
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -597,6 +597,7 @@ struct dev_pm_info {
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
 	unsigned int		memalloc_noio:1;
+	unsigned int		links_count;
 	enum rpm_request	request;
 	enum rpm_status		runtime_status;
 	int			runtime_error;
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -62,6 +62,8 @@ extern void pm_runtime_update_max_time_s
 						 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)
 {
@@ -194,6 +196,8 @@ static inline unsigned long pm_runtime_a
 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 */
 

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

#1479525 — [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep transitions in progress

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

Introduce a new flag in struct dev_pm_info, pm_sleep_in_progress, to
indicate that runtime PM has been disabled because of a PM sleep
transition in progress.

Make __pm_runtime_disable() set that flag when invoked with
RPM_DISABLE_ENABLE_PM_SLEEP set in its second argument and make
the PM core call it that way during system sleep transitions.

Introduce __pm_runtime_enable() so that it can take a second flags
argument and make it clear power.pm_sleep_in_progress for the device
if invoked with RPM_DISABLE_ENABLE_PM_SLEEP set.  Also make the PM
core pass RPM_DISABLE_ENABLE_PM_SLEEP to it during transitions from
system sleep to the working state.

Modify rpm_idle(), rpm_resume, rpm_suspend() and pm_schedule_suspend()
to neglect error codes and return 0 (without doing anything else)
when power.pm_sleep_in_progress is set for the device.

That will allow helpers like pm_runtime_get_sync() to be called
during system sleep transitions without worrying about possible
error codes they may return because runtime PM is disabled at
that point.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/power/runtime_pm.txt |   15 ++++++++-------
 drivers/base/power/main.c          |   12 ++++++------
 drivers/base/power/runtime.c       |   29 +++++++++++++++++++++++------
 include/linux/pm.h                 |    1 +
 include/linux/pm_runtime.h         |   12 ++++++++++--
 5 files changed, 48 insertions(+), 21 deletions(-)

Index: linux-pm/Documentation/power/runtime_pm.txt
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.txt
+++ linux-pm/Documentation/power/runtime_pm.txt
@@ -685,14 +685,15 @@ out the following operations:
     right before executing the subsystem-level .prepare() callback for it and
     pm_runtime_barrier() is called for every device right before executing the
     subsystem-level .suspend() callback for it.  In addition to that the PM core
-    calls  __pm_runtime_disable() with 0 as the second argument for every
-    device right before executing the subsystem-level .suspend_late() callback
-    for it.
+    calls  __pm_runtime_disable() with RPM_DISABLE_ENABLE_PM_SLEEP as the second
+    argument for every device right before executing the subsystem-level
+    .suspend_late() callback for it.
 
-  * During system resume pm_runtime_enable() and pm_runtime_put() are called for
-    every device right after executing the subsystem-level .resume_early()
-    callback and right after executing the subsystem-level .complete() callback
-    for it, respectively.
+  * During system resume __pm_runtime_enable() (with RPM_DISABLE_ENABLE_PM_SLEEP
+    as the second argument) and pm_runtime_put() are called for every device
+    right after executing the subsystem-level .resume_early() callback and right
+    after executing the subsystem-level .complete() callback for it,
+    respectively.
 
 7. Generic subsystem callbacks
 
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -701,7 +701,7 @@ static int device_resume_early(struct de
  Out:
 	TRACE_RESUME(error);
 
-	pm_runtime_enable(dev);
+	__pm_runtime_enable(dev, RPM_DISABLE_ENABLE_PM_SLEEP);
 	complete_all(&dev->power.completion);
 	return error;
 }
@@ -801,8 +801,8 @@ static int device_resume(struct device *
 		goto Complete;
 
 	if (dev->power.direct_complete) {
-		/* Match the pm_runtime_disable() in __device_suspend(). */
-		pm_runtime_enable(dev);
+		/* Match the __pm_runtime_disable() in __device_suspend(). */
+		__pm_runtime_enable(dev, RPM_DISABLE_ENABLE_PM_SLEEP);
 		goto Complete;
 	}
 
@@ -1228,7 +1228,7 @@ static int __device_suspend_late(struct
 	TRACE_DEVICE(dev);
 	TRACE_SUSPEND(0);
 
-	__pm_runtime_disable(dev, 0);
+	__pm_runtime_disable(dev, RPM_DISABLE_ENABLE_PM_SLEEP);
 
 	if (async_error)
 		goto Complete;
@@ -1438,11 +1438,11 @@ static int __device_suspend(struct devic
 
 	if (dev->power.direct_complete) {
 		if (pm_runtime_status_suspended(dev)) {
-			pm_runtime_disable(dev);
+			__pm_runtime_disable(dev, RPM_DISABLE_ALL);
 			if (pm_runtime_status_suspended(dev))
 				goto Complete;
 
-			pm_runtime_enable(dev);
+			__pm_runtime_enable(dev, RPM_DISABLE_ENABLE_PM_SLEEP);
 		}
 		dev->power.direct_complete = false;
 	}
Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -353,6 +353,9 @@ static int rpm_idle(struct device *dev,
 
  out:
 	trace_rpm_return_int_rcuidle(dev, _THIS_IP_, retval);
+	if (retval && dev->power.pm_sleep_in_progress)
+		return 0;
+
 	return retval ? retval : rpm_suspend(dev, rpmflags | RPM_AUTO);
 }
 
@@ -550,6 +553,8 @@ static int rpm_suspend(struct device *de
 
  out:
 	trace_rpm_return_int(dev, _THIS_IP_, retval);
+	if (retval && dev->power.pm_sleep_in_progress)
+		return 0;
 
 	return retval;
 
@@ -765,6 +770,8 @@ static int rpm_resume(struct device *dev
 	}
 
 	trace_rpm_return_int_rcuidle(dev, _THIS_IP_, retval);
+	if (retval && dev->power.pm_sleep_in_progress)
+		return 0;
 
 	return retval;
 }
@@ -867,6 +874,8 @@ int pm_schedule_suspend(struct device *d
 
  out:
 	spin_unlock_irqrestore(&dev->power.lock, flags);
+	if (retval && dev->power.pm_sleep_in_progress)
+		return 0;
 
 	return retval;
 }
@@ -1200,28 +1209,35 @@ void __pm_runtime_disable(struct device
 		__pm_runtime_barrier(dev);
 
  out:
+	if (flags & RPM_DISABLE_ENABLE_PM_SLEEP)
+		dev->power.pm_sleep_in_progress = true;
+
 	spin_unlock_irq(&dev->power.lock);
 }
 EXPORT_SYMBOL_GPL(__pm_runtime_disable);
 
 /**
- * pm_runtime_enable - Enable runtime PM of a device.
+ * __pm_runtime_enable - Enable runtime PM of a device.
  * @dev: Device to handle.
+ * @flags: Behavior modifiers.
  */
-void pm_runtime_enable(struct device *dev)
+void __pm_runtime_enable(struct device *dev, unsigned int flags)
 {
-	unsigned long flags;
+	unsigned long irqflags;
 
-	spin_lock_irqsave(&dev->power.lock, flags);
+	spin_lock_irqsave(&dev->power.lock, irqflags);
+
+	if (flags & RPM_DISABLE_ENABLE_PM_SLEEP)
+		dev->power.pm_sleep_in_progress = false;
 
 	if (dev->power.disable_depth > 0)
 		dev->power.disable_depth--;
 	else
 		dev_warn(dev, "Unbalanced %s!\n", __func__);
 
-	spin_unlock_irqrestore(&dev->power.lock, flags);
+	spin_unlock_irqrestore(&dev->power.lock, irqflags);
 }
-EXPORT_SYMBOL_GPL(pm_runtime_enable);
+EXPORT_SYMBOL_GPL(__pm_runtime_enable);
 
 /**
  * pm_runtime_forbid - Block runtime PM of a device.
@@ -1396,6 +1412,7 @@ void pm_runtime_init(struct device *dev)
 	dev->power.idle_notification = false;
 
 	dev->power.disable_depth = 1;
+	dev->power.pm_sleep_in_progress = false;
 	atomic_set(&dev->power.usage_count, 0);
 
 	dev->power.runtime_error = 0;
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -585,6 +585,7 @@ struct dev_pm_info {
 	atomic_t		usage_count;
 	atomic_t		child_count;
 	unsigned int		disable_depth:3;
+	bool			pm_sleep_in_progress:1;
 	unsigned int		idle_notification:1;
 	unsigned int		request_pending:1;
 	unsigned int		deferred_resume:1;
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -25,6 +25,9 @@
 
 /* Runtime PM disable/enable flags */
 #define RPM_DISABLE_CHECK_RESUME	(1 << 0)
+#define RPM_DISABLE_ENABLE_PM_SLEEP	(1 << 1)
+
+#define RPM_DISABLE_ALL	(RPM_DISABLE_CHECK_RESUME | RPM_DISABLE_ENABLE_PM_SLEEP)
 
 #ifdef CONFIG_PM
 extern struct workqueue_struct *pm_wq;
@@ -46,7 +49,7 @@ extern int pm_runtime_get_if_in_use(stru
 extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
-extern void pm_runtime_enable(struct device *dev);
+extern void __pm_runtime_enable(struct device *dev, unsigned int flags);
 extern void __pm_runtime_disable(struct device *dev, unsigned int flags);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
@@ -159,7 +162,7 @@ static inline int pm_runtime_get_if_in_u
 static inline int __pm_runtime_set_status(struct device *dev,
 					    unsigned int status) { return 0; }
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }
-static inline void pm_runtime_enable(struct device *dev) {}
+static inline void __pm_runtime_enable(struct device *dev, unsigned int flags) {}
 static inline void __pm_runtime_disable(struct device *dev, unsigned int flags) {}
 static inline void pm_runtime_allow(struct device *dev) {}
 static inline void pm_runtime_forbid(struct device *dev) {}
@@ -278,6 +281,11 @@ static inline void pm_runtime_disable(st
 	__pm_runtime_disable(dev, RPM_DISABLE_CHECK_RESUME);
 }
 
+static inline void pm_runtime_enable(struct device *dev)
+{
+	__pm_runtime_enable(dev, 0);
+}
+
 static inline void pm_runtime_use_autosuspend(struct device *dev)
 {
 	__pm_runtime_use_autosuspend(dev, true);

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

#1481312 — Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep transitions in progress

On Thu, Sep 08, 2016 at 11:29:48PM +0200, Rafael J. Wysocki wrote:
> Introduce a new flag in struct dev_pm_info, pm_sleep_in_progress, to
> indicate that runtime PM has been disabled because of a PM sleep
> transition in progress.
[...]
> That will allow helpers like pm_runtime_get_sync() to be called
> during system sleep transitions without worrying about possible
> error codes they may return because runtime PM is disabled at
> that point.

I have a suspicion that this patch papers over the direct_complete bug
I reported Sep 10 and that the patch is unnecessary once that bug is
fixed.

AFAICS, runtime PM is only disabled in two places during the system
sleep process: In __device_suspend() for devices using direct_complete,
and __device_suspend_late() for all devices.

In both of these phases (dpm_suspend() and dpm_suspend_late()), the
device tree is walked bottom-up. Since we've reordered consumers to
the back of dpm_list, they will be treated *before* their suppliers.
Thus, runtime PM is disabled on the consumers first, and only later
on the suppliers.

Then how can it be that runtime PM is already disabled on the supplier?
The only scenario I can imagine is that the supplier chose to exercise
direct_complete, thus was pm_runtime_disabled() in the __device_suspend()
phase, and the consumer did *not* choose to exercise direct_complete and
later tried to runtime resume its suppliers and itself.

I assume this patch is a replacement for Marek's [v2 08/10].
@Marek, does this scenario match with what you witnessed?

Best regards,

Lukas

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

#1481980 — Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep transitions in progress

On Monday, September 12, 2016 04:07:27 PM Lukas Wunner wrote:
> On Thu, Sep 08, 2016 at 11:29:48PM +0200, Rafael J. Wysocki wrote:
> > Introduce a new flag in struct dev_pm_info, pm_sleep_in_progress, to
> > indicate that runtime PM has been disabled because of a PM sleep
> > transition in progress.
> [...]
> > That will allow helpers like pm_runtime_get_sync() to be called
> > during system sleep transitions without worrying about possible
> > error codes they may return because runtime PM is disabled at
> > that point.
> 
> I have a suspicion that this patch papers over the direct_complete bug
> I reported Sep 10 and that the patch is unnecessary once that bug is
> fixed.

It doesn't paper over anything, but it may not be necessary anyway.

> AFAICS, runtime PM is only disabled in two places during the system
> sleep process: In __device_suspend() for devices using direct_complete,
> and __device_suspend_late() for all devices.
> 
> In both of these phases (dpm_suspend() and dpm_suspend_late()), the
> device tree is walked bottom-up. Since we've reordered consumers to
> the back of dpm_list, they will be treated *before* their suppliers.
> Thus, runtime PM is disabled on the consumers first, and only later
> on the suppliers.
> 
> Then how can it be that runtime PM is already disabled on the supplier?

Actually, I think that this was a consequence of a bug in device_reorder_to_tail()
that was present in the previous iteration of the patchset (it walked suppliers
instead of consumers).

> The only scenario I can imagine is that the supplier chose to exercise
> direct_complete, thus was pm_runtime_disabled() in the __device_suspend()
> phase, and the consumer did *not* choose to exercise direct_complete and
> later tried to runtime resume its suppliers and itself.
> 
> I assume this patch is a replacement for Marek's [v2 08/10].
> @Marek, does this scenario match with what you witnessed?

It is not strictly a replacement for it.  The Marek's patch was the
reason to post it, but I started to think about this earlier.

Some people have complained to me about having to deal with error codes
returned by the runtime PM framework during system suspend, so I thought
it might be useful to deal with that too.

That said it probably is not necessary right now.

Thanks,
Rafael

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

#1482052 — Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep transitions in progress

On Mon, Sep 12, 2016 at 11:25:36PM +0200, Rafael J. Wysocki wrote:
> On Monday, September 12, 2016 04:07:27 PM Lukas Wunner wrote:
> > On Thu, Sep 08, 2016 at 11:29:48PM +0200, Rafael J. Wysocki wrote:
> > > Introduce a new flag in struct dev_pm_info, pm_sleep_in_progress, to
> > > indicate that runtime PM has been disabled because of a PM sleep
> > > transition in progress.
> > [...]
> > > That will allow helpers like pm_runtime_get_sync() to be called
> > > during system sleep transitions without worrying about possible
> > > error codes they may return because runtime PM is disabled at
> > > that point.
> > 
> > I have a suspicion that this patch papers over the direct_complete bug
> > I reported Sep 10 and that the patch is unnecessary once that bug is
> > fixed.
> 
> It doesn't paper over anything, but it may not be necessary anyway.
> 
> > AFAICS, runtime PM is only disabled in two places during the system
> > sleep process: In __device_suspend() for devices using direct_complete,
> > and __device_suspend_late() for all devices.
> > 
> > In both of these phases (dpm_suspend() and dpm_suspend_late()), the
> > device tree is walked bottom-up. Since we've reordered consumers to
> > the back of dpm_list, they will be treated *before* their suppliers.
> > Thus, runtime PM is disabled on the consumers first, and only later
> > on the suppliers.
> > 
> > Then how can it be that runtime PM is already disabled on the supplier?
> 
> Actually, I think that this was a consequence of a bug in
> device_reorder_to_tail() that was present in the previous iteration
> of the patchset (it walked suppliers instead of consumers).
> 
> > The only scenario I can imagine is that the supplier chose to exercise
> > direct_complete, thus was pm_runtime_disabled() in the __device_suspend()
> > phase, and the consumer did *not* choose to exercise direct_complete and
> > later tried to runtime resume its suppliers and itself.
> > 
> > I assume this patch is a replacement for Marek's [v2 08/10].
> > @Marek, does this scenario match with what you witnessed?
> 
> It is not strictly a replacement for it.  The Marek's patch was the
> reason to post it, but I started to think about this earlier.
> 
> Some people have complained to me about having to deal with error codes
> returned by the runtime PM framework during system suspend, so I thought
> it might be useful to deal with that too.
> 
> That said it probably is not necessary right now.

Understood, thanks for providing this context which was unknown to me.

I'm wondering if it's necessary to introduce a new "pm_sleep_in_progress"
flag. We've already got "is_prepared", "is_suspended", "is_late_suspended",
"is_noirq_suspended", so we should have a pretty good idea of the fact
that the device is going to sleep and which stage it's in.

E.g. (dev->power.direct_complete || dev->power.is_suspended) covers a bit
more than the time frame when runtime PM is disabled for system sleep,
but might perhaps still suffice as a proxy.

Should a new flag be unavoidable, setting it directly in
__device_suspend_late(), device_resume_early(), __device_suspend()
and device_resume() would result in a smaller patch. (E.g. you
wouldn't have to modify the prototype of pm_runtime_enable().)

Thanks,

Lukas

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

#1482203 — Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep transitions in progress

Hi Rafael,


On 2016-09-12 23:25, Rafael J. Wysocki wrote:
> On Monday, September 12, 2016 04:07:27 PM Lukas Wunner wrote:
>> On Thu, Sep 08, 2016 at 11:29:48PM +0200, Rafael J. Wysocki wrote:
>>> Introduce a new flag in struct dev_pm_info, pm_sleep_in_progress, to
>>> indicate that runtime PM has been disabled because of a PM sleep
>>> transition in progress.
>> [...]
>>> That will allow helpers like pm_runtime_get_sync() to be called
>>> during system sleep transitions without worrying about possible
>>> error codes they may return because runtime PM is disabled at
>>> that point.
>> I have a suspicion that this patch papers over the direct_complete bug
>> I reported Sep 10 and that the patch is unnecessary once that bug is
>> fixed.
> It doesn't paper over anything, but it may not be necessary anyway.
>
>> AFAICS, runtime PM is only disabled in two places during the system
>> sleep process: In __device_suspend() for devices using direct_complete,
>> and __device_suspend_late() for all devices.
>>
>> In both of these phases (dpm_suspend() and dpm_suspend_late()), the
>> device tree is walked bottom-up. Since we've reordered consumers to
>> the back of dpm_list, they will be treated *before* their suppliers.
>> Thus, runtime PM is disabled on the consumers first, and only later
>> on the suppliers.
>>
>> Then how can it be that runtime PM is already disabled on the supplier?
> Actually, I think that this was a consequence of a bug in device_reorder_to_tail()
> that was present in the previous iteration of the patchset (it walked suppliers
> instead of consumers).
>
>> The only scenario I can imagine is that the supplier chose to exercise
>> direct_complete, thus was pm_runtime_disabled() in the __device_suspend()
>> phase, and the consumer did *not* choose to exercise direct_complete and
>> later tried to runtime resume its suppliers and itself.
>>
>> I assume this patch is a replacement for Marek's [v2 08/10].
>> @Marek, does this scenario match with what you witnessed?
> It is not strictly a replacement for it.  The Marek's patch was the
> reason to post it, but I started to think about this earlier.
>
> Some people have complained to me about having to deal with error codes
> returned by the runtime PM framework during system suspend, so I thought
> it might be useful to deal with that too.
>
> That said it probably is not necessary right now.

I've tested this patchset without this patch and system sleep with 
device link
enabled worked fine. However this might be also a consequence of 
enabling runtime
pm during system sleep since v4.8-rc1.

It looks that for now this patch can be skipped until a real use case for it
appears.

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

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

#1482848 — Re: [RFC/RFT][PATCH v2 5/7] PM / runtime: Flag to indicate PM sleep transitions in progress

On Tuesday, September 13, 2016 09:21:23 AM Marek Szyprowski wrote:
> Hi Rafael,
> 
> 
> On 2016-09-12 23:25, Rafael J. Wysocki wrote:
> > On Monday, September 12, 2016 04:07:27 PM Lukas Wunner wrote:
> >> On Thu, Sep 08, 2016 at 11:29:48PM +0200, Rafael J. Wysocki wrote:
> >>> Introduce a new flag in struct dev_pm_info, pm_sleep_in_progress, to
> >>> indicate that runtime PM has been disabled because of a PM sleep
> >>> transition in progress.
> >> [...]
> >>> That will allow helpers like pm_runtime_get_sync() to be called
> >>> during system sleep transitions without worrying about possible
> >>> error codes they may return because runtime PM is disabled at
> >>> that point.
> >> I have a suspicion that this patch papers over the direct_complete bug
> >> I reported Sep 10 and that the patch is unnecessary once that bug is
> >> fixed.
> > It doesn't paper over anything, but it may not be necessary anyway.
> >
> >> AFAICS, runtime PM is only disabled in two places during the system
> >> sleep process: In __device_suspend() for devices using direct_complete,
> >> and __device_suspend_late() for all devices.
> >>
> >> In both of these phases (dpm_suspend() and dpm_suspend_late()), the
> >> device tree is walked bottom-up. Since we've reordered consumers to
> >> the back of dpm_list, they will be treated *before* their suppliers.
> >> Thus, runtime PM is disabled on the consumers first, and only later
> >> on the suppliers.
> >>
> >> Then how can it be that runtime PM is already disabled on the supplier?
> > Actually, I think that this was a consequence of a bug in device_reorder_to_tail()
> > that was present in the previous iteration of the patchset (it walked suppliers
> > instead of consumers).
> >
> >> The only scenario I can imagine is that the supplier chose to exercise
> >> direct_complete, thus was pm_runtime_disabled() in the __device_suspend()
> >> phase, and the consumer did *not* choose to exercise direct_complete and
> >> later tried to runtime resume its suppliers and itself.
> >>
> >> I assume this patch is a replacement for Marek's [v2 08/10].
> >> @Marek, does this scenario match with what you witnessed?
> > It is not strictly a replacement for it.  The Marek's patch was the
> > reason to post it, but I started to think about this earlier.
> >
> > Some people have complained to me about having to deal with error codes
> > returned by the runtime PM framework during system suspend, so I thought
> > it might be useful to deal with that too.
> >
> > That said it probably is not necessary right now.
> 
> I've tested this patchset without this patch and system sleep with 
> device link
> enabled worked fine. However this might be also a consequence of 
> enabling runtime
> pm during system sleep since v4.8-rc1.
> 
> It looks that for now this patch can be skipped until a real use case for it
> appears.

OK, thanks!

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

#1479527 — [RFC/RFT][PATCH v2 4/7] PM / runtime: Pass flags argument to __pm_runtime_disable()

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

Modify __pm_runtime_disable() to take a flags argument instead of
the bool one it takes currently which will allow its behavior to
be modified in more than one way.  Introduce a flag
RPM_DISABLE_CHECK_RESUME to address the case currenty addressed by
passing 'true' to __pm_runtime_disable() as the second argument.

A subsequent change will add one more flag to use with
__pm_runtime_disable().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/power/runtime_pm.txt |    2 +-
 drivers/base/power/main.c          |    2 +-
 drivers/base/power/runtime.c       |   10 +++++-----
 include/linux/pm_runtime.h         |    9 ++++++---
 4 files changed, 13 insertions(+), 10 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -1158,18 +1158,18 @@ EXPORT_SYMBOL_GPL(pm_runtime_barrier);
 /**
  * __pm_runtime_disable - Disable runtime PM of a device.
  * @dev: Device to handle.
- * @check_resume: If set, check if there's a resume request for the device.
+ * @flags: Behavior modifiers.
  *
  * Increment power.disable_depth for the device and if it was zero previously,
  * cancel all pending runtime PM requests for the device and wait for all
  * operations in progress to complete.  The device can be either active or
  * suspended after its runtime PM has been disabled.
  *
- * If @check_resume is set and there's a resume request pending when
+ * If the CHECK_RESUME flag is set and there's a resume request pending when
  * __pm_runtime_disable() is called and power.disable_depth is zero, the
  * function will wake up the device before disabling its runtime PM.
  */
-void __pm_runtime_disable(struct device *dev, bool check_resume)
+void __pm_runtime_disable(struct device *dev, unsigned int flags)
 {
 	spin_lock_irq(&dev->power.lock);
 
@@ -1183,7 +1183,7 @@ void __pm_runtime_disable(struct device
 	 * means there probably is some I/O to process and disabling runtime PM
 	 * shouldn't prevent the device from processing the I/O.
 	 */
-	if (check_resume && dev->power.request_pending
+	if ((flags & RPM_DISABLE_CHECK_RESUME) && dev->power.request_pending
 	    && dev->power.request == RPM_REQ_RESUME) {
 		/*
 		 * Prevent suspends and idle notifications from being carried
@@ -1442,7 +1442,7 @@ void pm_runtime_reinit(struct device *de
  */
 void pm_runtime_remove(struct device *dev)
 {
-	__pm_runtime_disable(dev, false);
+	__pm_runtime_disable(dev, 0);
 	pm_runtime_reinit(dev);
 }
 
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -23,6 +23,9 @@
 					    usage_count */
 #define RPM_AUTO		0x08	/* Use autosuspend_delay */
 
+/* Runtime PM disable/enable flags */
+#define RPM_DISABLE_CHECK_RESUME	(1 << 0)
+
 #ifdef CONFIG_PM
 extern struct workqueue_struct *pm_wq;
 
@@ -44,7 +47,7 @@ extern int pm_schedule_suspend(struct de
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
 extern void pm_runtime_enable(struct device *dev);
-extern void __pm_runtime_disable(struct device *dev, bool check_resume);
+extern void __pm_runtime_disable(struct device *dev, unsigned int flags);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
 extern void pm_runtime_no_callbacks(struct device *dev);
@@ -157,7 +160,7 @@ static inline int __pm_runtime_set_statu
 					    unsigned int status) { return 0; }
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }
 static inline void pm_runtime_enable(struct device *dev) {}
-static inline void __pm_runtime_disable(struct device *dev, bool c) {}
+static inline void __pm_runtime_disable(struct device *dev, unsigned int flags) {}
 static inline void pm_runtime_allow(struct device *dev) {}
 static inline void pm_runtime_forbid(struct device *dev) {}
 
@@ -272,7 +275,7 @@ static inline void pm_runtime_set_suspen
 
 static inline void pm_runtime_disable(struct device *dev)
 {
-	__pm_runtime_disable(dev, true);
+	__pm_runtime_disable(dev, RPM_DISABLE_CHECK_RESUME);
 }
 
 static inline void pm_runtime_use_autosuspend(struct device *dev)
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1228,7 +1228,7 @@ static int __device_suspend_late(struct
 	TRACE_DEVICE(dev);
 	TRACE_SUSPEND(0);
 
-	__pm_runtime_disable(dev, false);
+	__pm_runtime_disable(dev, 0);
 
 	if (async_error)
 		goto Complete;
Index: linux-pm/Documentation/power/runtime_pm.txt
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.txt
+++ linux-pm/Documentation/power/runtime_pm.txt
@@ -685,7 +685,7 @@ out the following operations:
     right before executing the subsystem-level .prepare() callback for it and
     pm_runtime_barrier() is called for every device right before executing the
     subsystem-level .suspend() callback for it.  In addition to that the PM core
-    calls  __pm_runtime_disable() with 'false' as the second argument for every
+    calls  __pm_runtime_disable() with 0 as the second argument for every
     device right before executing the subsystem-level .suspend_late() callback
     for it.
 

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

#1479528

On Thursday, September 08, 2016 11:25:44 PM Rafael J. Wysocki wrote:
> Hi Everyone,
> 
> This is a refresh of the functional dependencies series that I posted last
> year and which has picked up by Marek quite recently.  For reference, appended
> is my introductory message sent previously (which may be slightly outdated now).
> 
> As last time, the first patch rearranges the code around __device_release_driver()
> a bit to prepare it for the next one (it actually hasn't changed AFAICS).
> 
> The second patch introduces the actual device links mechanics, but without
> system suspend/resume and runtime PM support which are added by the subsequent
> patches.
> 
> Some bugs found by Marek during his work on these patches should be fixed
> here.  In particular, the endless recursion in device_reorder_to_tail()
> which simply was broken before.
> 
> There are two additional patches to address the issue with runtime PM support
> that occured when runtime PM was disabled for some suppliers due to a PM
> sleep transition in progress.  Those patches simply make runtime PM helpers
> return 0 in that case which may be controversial, so please let me know if
> there are concerns about those.
> 
> The way device_link_add() works is a bit different, as it takes an additional
> status argument now.  That makes it possible to create a link in any state,
> with extra care of course, and should address the problem pointed to by Lukas
> during the previous discussion.
> 
> Also some comments from Tomeu have been addressed.
> 
> This hasn't been really tested yet and I'm sort of relying on Marek to test
> it, because he has a use case ready.  Hence, the RFT tag on the series.
> 
> Overall, please let me know what you think.
> 
> Thanks,
> Rafael
> 
> 
> Introduction:
> 
> As discussed in the recent "On-demand device probing" thread and in a Kernel
> Summit session earlier today, 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).
> 
> So I want to be able to represent those functional dependencies between devices
> and I'd like 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).
> 
> My idea is to represent a supplier-consumer dependency between devices (or
> more precisely between device+driver combos) as a "link" object containing
> pointers to the devices in question, a list node for each of them and some
> additional information related to the management of those objects, ie.
> something like:
> 
> struct device_link {
>         struct device *supplier;
>         struct list_head supplier_node;
>         struct device *consumer;
>         struct list_head consumer_node;
>         <flags, status etc>
> };
> 
> In general, there will be two lists of those things per device, one list
> of links to consumers and one list of links to suppliers.
> 
> In that picture, links will be created by calling, say:
> 
> int device_add_link(struct device *me, struct device *my_supplier, unsigned int flags);
> 
> and they will be deleted by the driver core when not needed any more.  The
> creation of a link should also cause dpm_list and the list used during shutdown
> to be reordered if needed.
> 
> In principle, it seems usefult to consider two types of links, one created
> at device registration time (when registering the second device from the linked
> pair, whichever it is) and one created at probe time (of the consumer device).
> I'll refer to them as "permanent" and "probe-time" links, respectively.
> 
> The permanent links (created at device registration time) will stay around
> until one of the linked devices is unregistered (at which time the driver
> core will drop the link along with the device going away).  The probe-time
> ones will be dropped (automatically) at the consumer device driver unbind time.
> 
> There's a question about what if the supplier device is being unbound before
> the consumer one (for example, as a result of a hotplug event).  My current
> view on that is that the consumer needs to be force-unbound in that case too,
> but I guess I may be persuaded otherwise given sufficiently convincing
> arguments.  Anyway, there are reasons to do that, like for example it may
> help with the synchronization.  Namely, if there's a rule that suppliers
> cannot be unbound before any consumers linked to them, than the list of links
> to suppliers for a consumer can only change at its registration/probe or
> unbind/remove times (which simplifies things quite a bit).
> 
> With that, the permanent links existing at the probe time for a consumer
> device can be used to check whether or not to defer the probing of it
> even before executing its probe callback.  In turn, system suspend
> synchronization should be a matter of calling device_pm_wait_for_dev()
> for all consumers of a supplier device, in analogy with dpm_wait_for_children(),
> and so on.
> 
> Of course, the new lists have to be stable during those operations and ensuring
> that is going to be somewhat tricky (AFAICS right now at least), but apart from
> that the whole concept looks reasonably straightforward to me.
> --

The Mark's address is broken in this series.  Again, sadly.

Really sorry about that and please fix it up when you reply.

Thanks,
Rafael

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

#1479529 — [RFC/RFT][PATCH v2 1/7] driver core: Add a wrapper around __device_release_driver()

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.

A subsequent change will make use of it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.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);
 	}
 }

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

#1479531 — [RFC/RFT][PATCH v2 3/7] PM / sleep: Make async suspend/resume of devices use device links

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

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

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

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

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

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

#1480629 — Re: [RFC/RFT][PATCH v2 3/7] PM / sleep: Make async suspend/resume of devices use device links

On Thu, Sep 08, 2016 at 11:28:33PM +0200, Rafael J. Wysocki wrote:
> 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.

For devices with a parent/child relationship, if the child does not
utilize direct_complete, the parent is not allowed to utilize it
either and is runtime resumed upon system sleep.

Don't we need the same for supplier/consumer relationships?

The code enforcing this is in __device_suspend() and looks like this:

	if (parent) {
		spin_lock_irq(&parent->power.lock);

		dev->parent->power.direct_complete = false;
		if (dev->power.wakeup_path
		    && !dev->parent->power.ignore_children)
			dev->parent->power.wakeup_path = true;

		spin_unlock_irq(&parent->power.lock);
	}

I guess we need to iterate over the suppliers here and execute
the block for each of them.

Thanks,

Lukas

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

#1480718 — Re: [RFC/RFT][PATCH v2 3/7] PM / sleep: Make async suspend/resume of devices use device links

On Sat, Sep 10, 2016 at 3:31 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Thu, Sep 08, 2016 at 11:28:33PM +0200, Rafael J. Wysocki wrote:
>> 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.
>
> For devices with a parent/child relationship, if the child does not
> utilize direct_complete, the parent is not allowed to utilize it
> either and is runtime resumed upon system sleep.
>
> Don't we need the same for supplier/consumer relationships?
>
> The code enforcing this is in __device_suspend() and looks like this:
>
>         if (parent) {
>                 spin_lock_irq(&parent->power.lock);
>
>                 dev->parent->power.direct_complete = false;
>                 if (dev->power.wakeup_path
>                     && !dev->parent->power.ignore_children)
>                         dev->parent->power.wakeup_path = true;
>
>                 spin_unlock_irq(&parent->power.lock);
>         }
>
> I guess we need to iterate over the suppliers here and execute
> the block for each of them.

You are right about the direct_complete thing, but the wakeup_path
thing is another matter.  It is about forwarding wakeup signals up the
hierarchy and I'd confine it to parents at least for the time being.

Thanks,
Rafael

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

#1479532 — [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support

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 device_link 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, links_to_consumers and links_to_suppliers,
to struct device to represent the lists of links to the devices that
depend on the given one (consumers) and to the devices depended on
by it (suppliers), respectively.

The entire data structure consisting of all of the lists of link
objects for all devices is protected by SRCU (for list walking)
and a by mutex (for link object addition/removal).  In addition
to that, each link object has an internal status field whose
value reflects what's happening to the devices pointed to by
the link.  That status field is protected by an internal spinlock.

New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller must ensure that the driver of the supplier
device is present and functional and the DEVICE_LINK_SUPPLIER_READY
flag must 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 persistent ones stay around until one of the target devices is
deleted, while 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 is passed to device_link_add() to create
a persistent link and it cannot be used for links created at the
consumer probe time (that is, persistent links must be created before
probing the consumer devices).

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>
---
 drivers/base/base.h    |   11 +
 drivers/base/core.c    |  421 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/base/dd.c      |   42 ++++
 include/linux/device.h |   35 ++++
 4 files changed, 504 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/base/base.h
===================================================================
--- linux-pm.orig/drivers/base/base.h
+++ linux-pm/drivers/base/base.h
@@ -107,6 +107,9 @@ extern void bus_remove_device(struct dev
 
 extern int bus_add_driver(struct device_driver *drv);
 extern void bus_remove_driver(struct device_driver *drv);
+extern void device_release_driver_internal(struct device *dev,
+					   struct device_driver *drv,
+					   struct device *parent);
 
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
@@ -152,3 +155,11 @@ extern int devtmpfs_init(void);
 #else
 static inline int devtmpfs_init(void) { return 0; }
 #endif
+
+/* Device links */
+extern int device_links_check_suppliers(struct device *dev);
+extern void device_links_driver_bound(struct device *dev);
+extern void device_links_driver_gone(struct device *dev);
+extern void device_links_no_driver(struct device *dev);
+extern bool device_links_busy(struct device *dev);
+extern void device_links_unbind_consumers(struct device *dev);
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -44,6 +44,402 @@ static int __init sysfs_deprecated_setup
 early_param("sysfs.deprecated", sysfs_deprecated_setup);
 #endif
 
+/* Device links support. */
+
+DEFINE_STATIC_SRCU(device_links_srcu);
+static DEFINE_MUTEX(device_links_lock);
+
+/**
+ * device_is_dependent - Check if one device depends on another one
+ * @dev: Device to check dependencies for.
+ * @target: Device to check against.
+ *
+ * Check if @dev or any device dependent on it (its child or its consumer etc)
+ * depends on @target.  Return 1 if that is the case or 0 otherwise.
+ */
+static int device_is_dependent(struct device *dev, void *target)
+{
+	struct device_link *link;
+	int ret;
+
+	ret = device_for_each_child(dev, target, device_is_dependent);
+	list_for_each_entry(link, &dev->links_to_consumers, s_node) {
+		if (WARN_ON(link->consumer == target))
+			return 1;
+
+		ret = ret || device_is_dependent(link->consumer, target);
+	}
+	return ret;
+}
+
+static int device_reorder_to_tail(struct device *dev, void *not_used)
+{
+	struct device_link *link;
+
+	devices_kset_move_last(dev);
+	device_pm_move_last(dev);
+	device_for_each_child(dev, NULL, device_reorder_to_tail);
+	list_for_each_entry(link, &dev->links_to_consumers, s_node)
+		device_reorder_to_tail(link->consumer, NULL);
+
+	return 0;
+}
+
+/**
+ * device_link_add - Create a link between two devices.
+ * @consumer: Consumer end of the link.
+ * @supplier: Supplier end of the link.
+ * @status: Initial status of the link.
+ * @flags: Link flags.
+ *
+ * The caller is responsible for ensuring that (a) @status reflects the current
+ * status of both @consumer and @supplier and (b) the creation of the new link
+ * is properly synchronized with runtime PM (especially if @status is
+ * DEVICE_LINK_ACTIVE).
+ *
+ * A side effect of the link creation is re-ordering of dpm_list and the
+ * devices_kset list by moving the consumer device and all devices depending
+ * on it to the ends of these lists.
+ */
+struct device_link *device_link_add(struct device *consumer,
+				    struct device *supplier,
+				    enum device_link_status status, u32 flags)
+{
+	struct device_link *link;
+
+	if (!consumer || !supplier || status == DEVICE_LINK_SUPPLIER_UNBIND)
+		return NULL;
+
+	mutex_lock(&device_links_lock);
+
+	/*
+	 * If there is a reverse dependency between the consumer and the
+	 * supplier already in the graph, return NULL.
+	 */
+	if (device_is_dependent(consumer, supplier)) {
+		link = NULL;
+		goto out;
+	}
+
+	list_for_each_entry(link, &supplier->links_to_consumers, s_node)
+		if (link->consumer == consumer)
+			goto out;
+
+	link = kmalloc(sizeof(*link), GFP_KERNEL);
+	if (!link)
+		goto out;
+
+	get_device(supplier);
+	link->supplier = supplier;
+	INIT_LIST_HEAD(&link->s_node);
+	get_device(consumer);
+	link->consumer = consumer;
+	INIT_LIST_HEAD(&link->c_node);
+	spin_lock_init(&link->lock);
+	link->flags = flags;
+	link->status = status;
+
+	/*
+	 * Move the consumer and all of the devices depending on it to the end
+	 * of dpm_list and the devices_kset list.
+	 *
+	 * It is necessary to hold dpm_list locked throughout all that or else
+	 * we may end up suspending with a wrong ordering of it.
+	 */
+	device_pm_lock();
+	device_reorder_to_tail(consumer, NULL);
+	device_pm_unlock();
+
+	list_add_tail_rcu(&link->s_node, &supplier->links_to_consumers);
+	list_add_tail_rcu(&link->c_node, &consumer->links_to_suppliers);
+
+	dev_info(consumer, "Linked as a consumer to %s\n", dev_name(supplier));
+
+ out:
+	mutex_unlock(&device_links_lock);
+	return link;
+}
+EXPORT_SYMBOL_GPL(device_link_add);
+
+static void __device_link_free_srcu(struct rcu_head *rhead)
+{
+	struct device_link *link;
+
+	link = container_of(rhead, struct device_link, rcu_head);
+	put_device(link->consumer);
+	put_device(link->supplier);
+	kfree(link);
+}
+
+static void __device_link_del(struct device_link *link)
+{
+	dev_info(link->consumer, "Dropping the link to %s\n",
+		 dev_name(link->supplier));
+
+	list_del_rcu(&link->s_node);
+	list_del_rcu(&link->c_node);
+	call_srcu(&device_links_srcu, &link->rcu_head, __device_link_free_srcu);
+}
+
+/**
+ * device_link_del - Delete a link between two devices.
+ * @link: Device link to delete.
+ *
+ * The caller must ensure proper synchronization of this function with runtime
+ * PM.
+ */
+void device_link_del(struct device_link *link)
+{
+	mutex_lock(&device_links_lock);
+	device_pm_lock();
+	__device_link_del(link);
+	device_pm_unlock();
+	mutex_unlock(&device_links_lock);
+}
+EXPORT_SYMBOL_GPL(device_link_del);
+
+static int device_links_read_lock(void)
+{
+	return srcu_read_lock(&device_links_srcu);
+}
+
+static void device_links_read_unlock(int idx)
+{
+	return srcu_read_unlock(&device_links_srcu, idx);
+}
+
+static void device_links_missing_supplier(struct device *dev)
+{
+	struct device_link *link;
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node) {
+		spin_lock(&link->lock);
+
+		if (link->status == DEVICE_LINK_CONSUMER_PROBE)
+			link->status = DEVICE_LINK_AVAILABLE;
+
+		spin_unlock(&link->lock);
+	}
+}
+
+/**
+ * device_links_check_suppliers - Check supplier devices for this one.
+ * @dev: Consumer device.
+ *
+ * Check links from this device to any suppliers.  Walk the list of the device's
+ * consumer links and see if all of the suppliers are available.  If not, simply
+ * return -EPROBE_DEFER.
+ *
+ * Walk the list under SRCU and check each link's status field under its lock.
+ *
+ * We need to guarantee that the supplier will not go away after the check has
+ * been positive here.  It only can go away in __device_release_driver() and
+ * that function  checks the device's links to consumers.  This means we need to
+ * mark the link as "consumer probe in progress" to make the supplier removal
+ * wait for us to complete (or bad things may happen).
+ */
+int device_links_check_suppliers(struct device *dev)
+{
+	struct device_link *link;
+	int idx, ret = 0;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_suppliers, c_node) {
+		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 device_link *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_consumers, 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->links_to_suppliers, 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 device_link *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_consumers, 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 device_link *link, *ln;
+
+	mutex_lock(&device_links_lock);
+
+	list_for_each_entry_safe_reverse(link, ln, &dev->links_to_suppliers, c_node)
+		if (link->flags & DEVICE_LINK_PERSISTENT) {
+			spin_lock(&link->lock);
+
+			if (link->status != DEVICE_LINK_SUPPLIER_UNBIND)
+				link->status = DEVICE_LINK_AVAILABLE;
+
+			spin_unlock(&link->lock);
+		} else {
+			__device_link_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' 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 device_link *link;
+	int idx;
+	bool ret = false;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
+		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 device_link *link;
+	int idx;
+
+ start:
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
+		enum device_link_status status;
+
+		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 +1107,8 @@ void device_initialize(struct device *de
 #ifdef CONFIG_GENERIC_MSI_IRQ
 	INIT_LIST_HEAD(&dev->msi_list);
 #endif
+	INIT_LIST_HEAD(&dev->links_to_consumers);
+	INIT_LIST_HEAD(&dev->links_to_suppliers);
 }
 EXPORT_SYMBOL_GPL(device_initialize);
 
@@ -1233,6 +1631,7 @@ void device_del(struct device *dev)
 {
 	struct device *parent = dev->parent;
 	struct class_interface *class_intf;
+	struct device_link *link, *ln;
 
 	/* Notify clients of device removal.  This call must come
 	 * before dpm_sysfs_remove().
@@ -1240,6 +1639,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->links_to_suppliers, c_node) {
+		WARN_ON(link->status != DEVICE_LINK_DORMANT);
+		__device_link_del(link);
+	}
+
+	list_for_each_entry_safe_reverse(link, ln, &dev->links_to_consumers, s_node) {
+		WARN_ON(link->status != DEVICE_LINK_DORMANT);
+		__device_link_del(link);
+	}
+
+	mutex_unlock(&device_links_lock);
+
 	dpm_sysfs_remove(dev);
 	if (parent)
 		klist_del(&dev->p->knode_parent);
Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -249,6 +249,7 @@ static void driver_bound(struct device *
 		 __func__, dev_name(dev));
 
 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
+	device_links_driver_bound(dev);
 
 	device_pm_check_callbacks(dev);
 
@@ -399,6 +400,7 @@ probe_failed:
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
 pinctrl_bind_failed:
+	device_links_no_driver(dev);
 	devres_release_all(dev);
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;
@@ -489,6 +491,10 @@ int driver_probe_device(struct device_dr
 	if (!device_is_registered(dev))
 		return -ENODEV;
 
+	ret = device_links_check_suppliers(dev);
+	if (ret)
+		return ret;
+
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
@@ -756,7 +762,7 @@ EXPORT_SYMBOL_GPL(driver_attach);
  * __device_release_driver() must be called with @dev lock held.
  * When called for a USB interface, @dev->parent lock must be held as well.
  */
-static void __device_release_driver(struct device *dev)
+static void __device_release_driver(struct device *dev, struct device *parent)
 {
 	struct device_driver *drv;
 
@@ -765,6 +771,25 @@ static void __device_release_driver(stru
 		if (driver_allows_async_probing(drv))
 			async_synchronize_full();
 
+		while (device_links_busy(dev)) {
+			device_unlock(dev);
+			if (parent)
+				device_unlock(parent);
+
+			device_links_unbind_consumers(dev);
+			if (parent)
+				device_lock(parent);
+
+			device_lock(dev);
+			/*
+			 * A concurrent invocation of the same function might
+			 * have released the driver successfully while this one
+			 * was waiting, so check for that.
+			 */
+			if (dev->driver != drv)
+				return;
+		}
+
 		pm_runtime_get_sync(dev);
 
 		driver_sysfs_remove(dev);
@@ -780,6 +805,9 @@ static void __device_release_driver(stru
 			dev->bus->remove(dev);
 		else if (drv->remove)
 			drv->remove(dev);
+
+		device_links_driver_gone(dev);
+		device_links_no_driver(dev);
 		devres_release_all(dev);
 		dev->driver = NULL;
 		dev_set_drvdata(dev, NULL);
@@ -796,16 +824,16 @@ static void __device_release_driver(stru
 	}
 }
 
-static void device_release_driver_internal(struct device *dev,
-					   struct device_driver *drv,
-					   struct device *parent)
+void device_release_driver_internal(struct device *dev,
+				    struct device_driver *drv,
+				    struct device *parent)
 {
 	if (parent)
 		device_lock(parent);
 
 	device_lock(dev);
 	if (!drv || drv == dev->driver)
-		__device_release_driver(dev);
+		__device_release_driver(dev, parent);
 
 	device_unlock(dev);
 	if (parent)
@@ -818,6 +846,10 @@ static void device_release_driver_intern
  *
  * Manually detach device from driver.
  * When called for a USB interface, @dev->parent lock must be held.
+ *
+ * If this function is to be called with @dev->parent lock held, ensure that
+ * the device's consumers are unbound in advance or that their locks can be
+ * acquired under the @dev->parent lock.
  */
 void device_release_driver(struct device *dev)
 {
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -706,6 +706,32 @@ struct device_dma_parameters {
 	unsigned long segment_boundary_mask;
 };
 
+enum device_link_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.
+ */
+#define DEVICE_LINK_PERSISTENT	(1 << 0)
+
+struct device_link {
+	struct device *supplier;
+	struct list_head s_node;
+	struct device *consumer;
+	struct list_head c_node;
+	enum device_link_status status;
+	u32 flags;
+	spinlock_t lock;
+	struct rcu_head rcu_head;
+};
+
 /**
  * struct device - The basic device structure
  * @parent:	The device's "parent" device, the device to which it is attached.
@@ -731,6 +757,8 @@ struct device_dma_parameters {
  * 		on.  This shrinks the "Board Support Packages" (BSPs) and
  * 		minimizes board-specific #ifdefs in drivers.
  * @driver_data: Private pointer for driver specific info.
+ * @links_to_consumers: Links to consumer devices.
+ * @links_to_suppliers: Links to supplier devices.
  * @power:	For device power management.
  * 		See Documentation/power/devices.txt for details.
  * @pm_domain:	Provide callbacks that are executed during system suspend,
@@ -797,6 +825,8 @@ struct device {
 					   core doesn't touch it */
 	void		*driver_data;	/* Driver data, set and get with
 					   dev_set/get_drvdata */
+	struct list_head	links_to_consumers;
+	struct list_head	links_to_suppliers;
 	struct dev_pm_info	power;
 	struct dev_pm_domain	*pm_domain;
 
@@ -1113,6 +1143,11 @@ extern void device_shutdown(void);
 /* debugging and troubleshooting/diagnostic helpers. */
 extern const char *dev_driver_string(const struct device *dev);
 
+/* Device links interface. */
+struct device_link *device_link_add(struct device *consumer,
+				    struct device *supplier,
+				    enum device_link_status status, u32 flags);
+void device_link_del(struct device_link *link);
 
 #ifdef CONFIG_PRINTK
 

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

#1479739 — Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support

+ Mark

On 8 September 2016 at 23:27, Rafael J. Wysocki <rjw@rjwysocki.net> 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.
>
> 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 device_link 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, links_to_consumers and links_to_suppliers,
> to struct device to represent the lists of links to the devices that
> depend on the given one (consumers) and to the devices depended on
> by it (suppliers), respectively.
>
> The entire data structure consisting of all of the lists of link
> objects for all devices is protected by SRCU (for list walking)
> and a by mutex (for link object addition/removal).  In addition
> to that, each link object has an internal status field whose
> value reflects what's happening to the devices pointed to by
> the link.  That status field is protected by an internal spinlock.
>
> New links are added by calling device_link_add() which may happen
> either before the consumer device is probed or when probing it, in
> which case the caller must ensure that the driver of the supplier
> device is present and functional and the DEVICE_LINK_SUPPLIER_READY
> flag must 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 persistent ones stay around until one of the target devices is
> deleted, while 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 is passed to device_link_add() to create
> a persistent link and it cannot be used for links created at the
> consumer probe time (that is, persistent links must be created before
> probing the consumer devices).
>
> 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.

Rafael, thanks for working on this and re-spinning this series. It's
indeed very interesting!

I am hoping "device links" should be able to solve some of those
device ordering issues I have observed for several SoCs, particularly
during system PM and in combination with runtime PM.

I intend to test the series as soon as I can and try to deploy it to
see if it solves some of the issues I have seen. I will also try to
review in more detail. No promises short term though. :-)

BTW, as I am mostly working on DT based platforms, I guess we would
later on need to discuss with the DT maintainers how to describe
device links.

A minor comment to the change-log. I would appreciate some information
about "error" handling. Especially, what happens in the driver core
when it's about to probe a device with a configured device link, but
the link hasn’t been established yet (the other device isn't
successfully probed). In the ideal scenario this shouldn't happen, but
of course it will. So I assume the driver core relies on the existing
deferred probe mechanism for this.

[...]

Kind regards
Uffe

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

#1479926 — Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support

[Multipart message — attachments visible in raw view] — view raw

On Fri, Sep 09, 2016 at 10:25:30AM +0200, Ulf Hansson wrote:

> BTW, as I am mostly working on DT based platforms, I guess we would
> later on need to discuss with the DT maintainers how to describe
> device links.

I think the expectation had been that since DT tends to get right down
to the hardware details we already have the information in there in
order to use things so the problem becomes figuring out how/when we
parse the existing bindings to tell the driver core about links.

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

#1480041 — Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support

On 9 September 2016 at 14:06, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Sep 09, 2016 at 10:25:30AM +0200, Ulf Hansson wrote:
>
>> BTW, as I am mostly working on DT based platforms, I guess we would
>> later on need to discuss with the DT maintainers how to describe
>> device links.
>
> I think the expectation had been that since DT tends to get right down
> to the hardware details we already have the information in there in
> order to use things so the problem becomes figuring out how/when we
> parse the existing bindings to tell the driver core about links.

I guess what we need from a functional point of view, is to be able to
first initialize all devices (device_initialize()) then add the
relevant device links before we add the devices (device_add()). That
seems like a piece of cake to fix. :-)

Kind regards
Uffe

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

#1483803 — Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support

On Friday, September 09, 2016 10:25:30 AM Ulf Hansson wrote:
> + Mark
> 
> On 8 September 2016 at 23:27, Rafael J. Wysocki <rjw@rjwysocki.net> 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.
> >
> > 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 device_link 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, links_to_consumers and links_to_suppliers,
> > to struct device to represent the lists of links to the devices that
> > depend on the given one (consumers) and to the devices depended on
> > by it (suppliers), respectively.
> >
> > The entire data structure consisting of all of the lists of link
> > objects for all devices is protected by SRCU (for list walking)
> > and a by mutex (for link object addition/removal).  In addition
> > to that, each link object has an internal status field whose
> > value reflects what's happening to the devices pointed to by
> > the link.  That status field is protected by an internal spinlock.
> >
> > New links are added by calling device_link_add() which may happen
> > either before the consumer device is probed or when probing it, in
> > which case the caller must ensure that the driver of the supplier
> > device is present and functional and the DEVICE_LINK_SUPPLIER_READY
> > flag must 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 persistent ones stay around until one of the target devices is
> > deleted, while 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 is passed to device_link_add() to create
> > a persistent link and it cannot be used for links created at the
> > consumer probe time (that is, persistent links must be created before
> > probing the consumer devices).
> >
> > 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.
> 
> Rafael, thanks for working on this and re-spinning this series. It's
> indeed very interesting!

Well, you're welcome. :-)

> I am hoping "device links" should be able to solve some of those
> device ordering issues I have observed for several SoCs, particularly
> during system PM and in combination with runtime PM.
> 
> I intend to test the series as soon as I can and try to deploy it to
> see if it solves some of the issues I have seen. I will also try to
> review in more detail. No promises short term though. :-)

All feedback will be appreciated.

I think I'll send an update of it tomorrow, though.

> BTW, as I am mostly working on DT based platforms, I guess we would
> later on need to discuss with the DT maintainers how to describe
> device links.
> 
> A minor comment to the change-log. I would appreciate some information
> about "error" handling. Especially, what happens in the driver core
> when it's about to probe a device with a configured device link, but
> the link hasn’t been established yet (the other device isn't
> successfully probed). In the ideal scenario this shouldn't happen, but
> of course it will. So I assume the driver core relies on the existing
> deferred probe mechanism for this.

Yes, it does.  I'll update the changelog with this information.

Thanks,
Rafael

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

#1480819 — Re: [RFC/RFT][PATCH v2 2/7] driver core: Functional dependencies tracking support

On Thu, Sep 08, 2016 at 11:27:45PM +0200, Rafael J. Wysocki wrote:
> +/**
> + * device_is_dependent - Check if one device depends on another one
> + * @dev: Device to check dependencies for.
> + * @target: Device to check against.
> + *
> + * Check if @dev or any device dependent on it (its child or its consumer etc)
> + * depends on @target.  Return 1 if that is the case or 0 otherwise.
> + */
> +static int device_is_dependent(struct device *dev, void *target)
> +{
> +	struct device_link *link;
> +	int ret;
> +
> +	ret = device_for_each_child(dev, target, device_is_dependent);
> +	list_for_each_entry(link, &dev->links_to_consumers, s_node) {
> +		if (WARN_ON(link->consumer == target))
> +			return 1;
> +
> +		ret = ret || device_is_dependent(link->consumer, target);
> +	}
> +	return ret;
> +}

What happens if someone tries to add a device link from a parent
(as the consumer) to a child (as a supplier)?  You're only checking
if target is a consumer of dev, for full correctness you'd also have
to check if target is a parent of dev.  (Or grandparent, or great-
grandparent, ... you need to walk the tree up to the root.)


The function can be sped up by returning immediately if a match
is found instead of continuing searching and accumulating the
result in ret, i.e.:

	if (device_for_each_child(dev, target, device_is_dependent))
		return 1;

and in the list_for_each_entry block:

	if (device_is_dependent(link->consumer, target))
		return 1;

Then at the end of the function "return 0".


I'd move the WARN_ON() to the single invocation of this function in
device_link_add(), that way it's possible to use the function as a
helper elsewhere should the need arise.

Thanks,

Lukas

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web