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

Groups > linux.kernel > #1308958 > unrolled thread

[RFC][PATCH 0/5] Functional dependencies between devices

Back to article view | Back to linux.kernel

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

Contents

  [RFC][PATCH 0/5] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-14 03:00 +0100
    [RFC][PATCH 3/5] PM core: Make async suspend/resume of devices use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-14 03:00 +0100
    [RFC][PATCH 4/5] PM core: Make runtime PM of devices use device links "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-14 03:00 +0100
    Re: [RFC][PATCH 0/5] Functional dependencies between devices Tomeu Vizoso <tomeu.vizoso@collabora.com> - 2016-01-14 15:20 +0100
      Re: [RFC][PATCH 0/5] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-15 01:50 +0100

#1308958 — [RFC][PATCH 0/5] Functional dependencies between devices

On Tuesday, October 27, 2015 04:24:14 PM Rafael J. Wysocki wrote:
> Hi All,
> 
> 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.
> 

What follows is my prototype implementation of this.  It took some time
to develop (much more than I was hoping for), but here it goes at last.

The first patch rearranges the code around __device_release_driver() a bit
to prepare it for the next one.

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.

This hasn't been really tested yet (apart from checking that it doesn't break
things when device links are not in used, which would be rather embarrassing),
but at this time I'd really like you to have a look and tell me what you think
(especially if you see a reason why this is not going to work).

Thanks,
Rafael

[toc] | [next] | [standalone]

#1308960 — [RFC][PATCH 3/5] PM core: 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
@@ -163,12 +163,12 @@ void device_link_del(struct devlink *lin
 }
 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 devlink *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	/*
+	 * If the supplier goes away right after we've checked the link to it,
+	 * we'll wait for its completion to change the state, but that's fine,
+	 * because the only things that will block as a result are the SRCU
+	 * callbacks freeing the link objects for the links in the list we're
+	 * walking.
+	 */
+	list_for_each_entry_rcu(link, &dev->consumer_links, c_node)
+		if (link->status != DEVICE_LINK_DORMANT)
+			dpm_wait(link->supplier, async);
+
+	device_links_read_unlock(idx);
+}
+
+static void dpm_wait_for_superior(struct device *dev, bool async)
+{
+	dpm_wait(dev->parent, async);
+	dpm_wait_for_suppliers(dev, async);
+}
+
+static void dpm_wait_for_consumers(struct device *dev, bool async)
+{
+	struct devlink *link;
+	int idx;
+
+	idx = device_links_read_lock();
+
+	/*
+	 * The status of a device link can only be changed from "dormant" by a
+	 * probe, but that cannot happen during system suspend/resume.  In
+	 * theory it can change to "dormant" at that time, but then it is
+	 * reasonable to wait for the target device anyway (eg. if it goes
+	 * away, it's better to wait for it to go away completely and then
+	 * continue instead of trying to continue in parallel with its
+	 * unregistration).
+	 */
+	list_for_each_entry_rcu(link, &dev->supplier_links, s_node)
+		if (link->status != DEVICE_LINK_DORMANT)
+			dpm_wait(link->consumer, async);
+
+	device_links_read_unlock(idx);
+}
+
+static void dpm_wait_for_subordinate(struct device *dev, bool async)
+{
+	dpm_wait_for_children(dev, async);
+	dpm_wait_for_consumers(dev, async);
+}
+
 /**
  * pm_op - Return the PM operation appropriate for given PM event.
  * @ops: PM operations to choose from.
@@ -488,7 +544,7 @@ static int device_resume_noirq(struct 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 ";
@@ -1357,7 +1413,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]

#1308962 — [RFC][PATCH 4/5] PM core: Make runtime PM of devices use device links

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

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

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

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

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

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

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

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

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

#1309316

On 14 January 2016 at 02:52, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, October 27, 2015 04:24:14 PM Rafael J. Wysocki wrote:
>> Hi All,
>>
>> 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.
>>
>
> What follows is my prototype implementation of this.  It took some time
> to develop (much more than I was hoping for), but here it goes at last.
>
> The first patch rearranges the code around __device_release_driver() a bit
> to prepare it for the next one.
>
> 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.
>
> This hasn't been really tested yet (apart from checking that it doesn't break
> things when device links are not in used, which would be rather embarrassing),
> but at this time I'd really like you to have a look and tell me what you think
> (especially if you see a reason why this is not going to work).

Hi Rafael,

have given a quick look and I have 2 questions for now:

- Why deferring the probe if a supplier isn't ready? Seems like quite
a bit of a waste to keep iterating that list until all suppliers have
probed. If we know that a supplier is needed at a given time, why not
probe it right away?

- When were you thinking of calling device_link_add for permanent links?

I also wonder if we could find clearer names for supplier_links and
consumer_links, as it wasn't immediately clear to me what those lists
contained. Maybe just "consumers" and "suppliers"?

Thanks,

Tomeu

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

#1309787

On Thursday, January 14, 2016 03:19:03 PM Tomeu Vizoso wrote:
> On 14 January 2016 at 02:52, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, October 27, 2015 04:24:14 PM Rafael J. Wysocki wrote:
> >> Hi All,
> >>
> >> 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.
> >>
> >
> > What follows is my prototype implementation of this.  It took some time
> > to develop (much more than I was hoping for), but here it goes at last.
> >
> > The first patch rearranges the code around __device_release_driver() a bit
> > to prepare it for the next one.
> >
> > 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.
> >
> > This hasn't been really tested yet (apart from checking that it doesn't break
> > things when device links are not in used, which would be rather embarrassing),
> > but at this time I'd really like you to have a look and tell me what you think
> > (especially if you see a reason why this is not going to work).
> 
> Hi Rafael,
> 
> have given a quick look and I have 2 questions for now:
> 
> - Why deferring the probe if a supplier isn't ready? Seems like quite
> a bit of a waste to keep iterating that list until all suppliers have
> probed. If we know that a supplier is needed at a given time, why not
> probe it right away?

Because it is not guaranteed to succeed.

The fact that the supplier is not ready may very well mean that its driver
has not been loaded yet and we need to wait for that to happen before
actually trying to bind the supplier.

So what is done in the current patches is needed anyway and there might
be some optimizations on top of that, but I'm not really sure how much
of a difference they would make in practice.

> 
> - When were you thinking of calling device_link_add for permanent links?

For example, I'm envisioning calling it from the ACPI layer for devices
with _DEP.

> I also wonder if we could find clearer names for supplier_links and
> consumer_links, as it wasn't immediately clear to me what those lists
> contained. Maybe just "consumers" and "suppliers"?

OK, I see why these names may be confusing.  These are lists of links,
not lists of devices, though, so I'd like the names to reflect that.

What about links_to_suppliers and links_to_consumers?  Somewhat longer,
but should be less confusing IMO.

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web