Hi,
Let me quote from the previous intro messages for this series first:
> > Time for another update. :-)
> >
> > Fewer changes this time, mostly to address issues found by Lukas and
> > Marek.
> >
> > The most significant one is to make device_link_add() cope with the case
> > when
> > the consumer device has not been registered yet when it is called. The
> > supplier device still is required to be registered and the function will
> > return NULL if that is not the case.
> >
> > Another significant change is in patch [4/5] that now makes the core apply
> > pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the
> > probing of a consumer one (in analogy with the parent).
>
> One more update after some conversations during LinuxCon Europe.
>
> The main point was to make it possible for device_link_add() to figure out
> the initial state of the link instead of expecting the caller to provide it
> which might not be reliable enough in general.
>
> In this version device_link_add() takes three arguments, the supplier and
> consumer pointers and flags and it sets the correct initial state of the
> link automatically (unless invoked with the "stateless" flag, of course).
> The cost is one additional field in struct device (I moved all of the
> links-related fields in struct device to a separate sub-structure while at
> it) to track the "driver presence status" of the device (to be used by
> device_link_add()).
>
> In addition to that, the links list walks in the core.c and dd.c code are
> under the device links mutex now, so the iternal link spinlock is not needed
> any more and I have renamed symbols to distinguish between flags, link
> states and device "driver presence statuses".
The most significant change in this revision with respect to the previous one is
related to the fact that SRCU is not available on some architectures, so the
code falls back to using an RW semaphore for synchronization if SRCU is not
there. Fortunately, the code changes needed for that turned out to be quite
straightforward and confined to the second patch.
Apart from this, the flags are defined using BIT(x) now (instead of open coding
the latter in the flag definitions).
Updated is mostly patch [2/5]. Patches [1,3,5/5] have not changed (except for
trivial rebasing) and patch [4/5] needed to be refreshed on top of the modified
[2/5].
FWIW, I've run the series through 0-day which has not reported any problems
with it.
Thanks,
Rafael
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
Tested-by: Marek Szyprowski <[email protected]>
---
drivers/base/power/main.c | 85 ++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 79 insertions(+), 6 deletions(-)
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -246,6 +246,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.suppliers, c_node)
+ if (READ_ONCE(link->status) != DL_STATE_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.consumers, s_node)
+ if (READ_ONCE(link->status) != DL_STATE_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.
@@ -490,7 +546,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 ";
@@ -620,7 +676,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 ";
@@ -752,7 +808,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);
@@ -1040,7 +1096,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 ";
@@ -1187,7 +1243,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 ";
@@ -1344,6 +1400,22 @@ static int legacy_suspend(struct device
return error;
}
+static void dpm_clear_suppliers_direct_complete(struct device *dev)
+{
+ struct device_link *link;
+ int idx;
+
+ idx = device_links_read_lock();
+
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+ spin_lock_irq(&link->supplier->power.lock);
+ link->supplier->power.direct_complete = false;
+ spin_unlock_irq(&link->supplier->power.lock);
+ }
+
+ device_links_read_unlock(idx);
+}
+
/**
* device_suspend - Execute "suspend" callbacks for given device.
* @dev: Device to handle.
@@ -1360,7 +1432,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;
@@ -1456,6 +1528,7 @@ static int __device_suspend(struct devic
spin_unlock_irq(&parent->power.lock);
}
+ dpm_clear_suppliers_direct_complete(dev);
}
device_unlock(dev);
From: Rafael J. Wysocki <[email protected]>
Subject:
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 <[email protected]>
---
drivers/base/core.c | 20 +++++++++++++-------
drivers/base/power/runtime.c | 23 ++++++++++++++++++++---
include/linux/pm.h | 1 +
include/linux/pm_runtime.h | 4 ++++
4 files changed, 38 insertions(+), 10 deletions(-)
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -205,14 +205,17 @@ struct device_link *device_link_add(stru
if (!link)
goto out;
- if ((flags & DL_FLAG_PM_RUNTIME) && (flags & DL_FLAG_RPM_ACTIVE)) {
- if (pm_runtime_get_sync(supplier) < 0) {
- pm_runtime_put_noidle(supplier);
- kfree(link);
- link = NULL;
- goto out;
+ if (flags & DL_FLAG_PM_RUNTIME) {
+ if (flags & DL_FLAG_RPM_ACTIVE) {
+ if (pm_runtime_get_sync(supplier) < 0) {
+ pm_runtime_put_noidle(supplier);
+ kfree(link);
+ link = NULL;
+ goto out;
+ }
+ link->rpm_active = true;
}
- link->rpm_active = true;
+ pm_runtime_new_link(consumer);
}
get_device(supplier);
link->supplier = supplier;
@@ -296,6 +299,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 & DL_FLAG_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
@@ -305,6 +305,7 @@ static int __rpm_callback(int (*cb)(stru
__releases(&dev->power.lock) __acquires(&dev->power.lock)
{
int retval, idx;
+ bool use_links = dev->power.links_count > 0;
if (dev->power.irq_safe) {
spin_unlock(&dev->power.lock);
@@ -318,7 +319,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();
retval = rpm_get_suppliers(dev);
@@ -341,8 +342,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:
@@ -1593,6 +1595,21 @@ void pm_runtime_put_suppliers(struct dev
device_links_read_unlock(idx);
}
+void pm_runtime_new_link(struct device *dev)
+{
+ spin_lock_irq(&dev->power.lock);
+ dev->power.links_count++;
+ spin_unlock_irq(&dev->power.lock);
+}
+
+void pm_runtime_drop_link(struct device *dev)
+{
+ spin_lock_irq(&dev->power.lock);
+ WARN_ON(dev->power.links_count == 0);
+ dev->power.links_count--;
+ spin_unlock_irq(&dev->power.lock);
+}
+
/**
* pm_runtime_force_suspend - Force a device into suspend state if needed.
* @dev: Device to suspend.
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
@@ -58,6 +58,8 @@ extern void pm_runtime_set_memalloc_noio
extern void pm_runtime_clean_up_links(struct device *dev);
extern void pm_runtime_get_suppliers(struct device *dev);
extern void pm_runtime_put_suppliers(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)
{
@@ -192,6 +194,8 @@ static inline void pm_runtime_set_memall
static inline void pm_runtime_clean_up_links(struct device *dev) {}
static inline void pm_runtime_get_suppliers(struct device *dev) {}
static inline void pm_runtime_put_suppliers(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 */
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
v5 -> v6:
- Rebase.
- Use BIT(x) in flag definitions.
v4 -> v5:
- Rebase, change the symbol names in accordance with patch [2/5] etc.
- Use READ_ONCE() to annotate "unprotected" accesses to link status data.
v3 -> v4:
- Add pm_runtime_get/put_suppliers() and make the core call them
around the probing of a consumer device (in analogy with what it
does to the device's parent).
---
drivers/base/core.c | 27 +++++++
drivers/base/dd.c | 3
drivers/base/power/runtime.c | 157 +++++++++++++++++++++++++++++++++++++++++--
include/linux/device.h | 6 +
include/linux/pm_runtime.h | 6 +
5 files changed, 193 insertions(+), 6 deletions(-)
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -152,6 +152,14 @@ static int device_reorder_to_tail(struct
* @supplier: Supplier end of the link.
* @flags: Link flags.
*
+ * The caller is responsible for the proper synchronization of the link creation
+ * with runtime PM. First, setting the DL_FLAG_PM_RUNTIME flag will cause the
+ * runtime PM framework to take the link into account. Second, if the
+ * DL_FLAG_RPM_ACTIVE flag is set in addition to it, the supplier devices will
+ * be forced into the active metastate and reference-counted upon the creation
+ * of the link. If DL_FLAG_PM_RUNTIME is not set, DL_FLAG_RPM_ACTIVE will be
+ * ignored.
+ *
* If the DL_FLAG_AUTOREMOVE is set, the link will be removed automatically
* when the consumer device driver unbinds from it. The combination of both
* DL_FLAG_AUTOREMOVE and DL_FLAG_STATELESS set is invalid and will cause NULL
@@ -193,10 +201,19 @@ struct device_link *device_link_add(stru
if (link->consumer == consumer)
goto out;
- link = kmalloc(sizeof(*link), GFP_KERNEL);
+ link = kzalloc(sizeof(*link), GFP_KERNEL);
if (!link)
goto out;
+ if ((flags & DL_FLAG_PM_RUNTIME) && (flags & DL_FLAG_RPM_ACTIVE)) {
+ if (pm_runtime_get_sync(supplier) < 0) {
+ pm_runtime_put_noidle(supplier);
+ kfree(link);
+ link = NULL;
+ goto out;
+ }
+ link->rpm_active = true;
+ }
get_device(supplier);
link->supplier = supplier;
INIT_LIST_HEAD(&link->s_node);
@@ -213,6 +230,14 @@ struct device_link *device_link_add(stru
case DL_DEV_DRIVER_BOUND:
switch (consumer->links.status) {
case DL_DEV_PROBING:
+ /*
+ * Balance the decrementation of the supplier's
+ * runtime PM usage counter after consumer probe
+ * in driver_probe_device().
+ */
+ if (flags & DL_FLAG_PM_RUNTIME)
+ pm_runtime_get_sync(supplier);
+
link->status = DL_STATE_CONSUMER_PROBE;
break;
case DL_DEV_DRIVER_BOUND:
Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -513,6 +513,7 @@ int driver_probe_device(struct device_dr
pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
drv->bus->name, __func__, dev_name(dev), drv->name);
+ pm_runtime_get_suppliers(dev);
if (dev->parent)
pm_runtime_get_sync(dev->parent);
@@ -523,6 +524,7 @@ int driver_probe_device(struct device_dr
if (dev->parent)
pm_runtime_put(dev->parent);
+ pm_runtime_put_suppliers(dev);
return ret;
}
@@ -806,6 +808,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/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 *);
@@ -258,6 +260,42 @@ static int rpm_check_suspend_allowed(str
return retval;
}
+static int rpm_get_suppliers(struct device *dev)
+{
+ struct device_link *link;
+
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+ int retval;
+
+ if (!(link->flags & DL_FLAG_PM_RUNTIME))
+ continue;
+
+ if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND ||
+ link->rpm_active)
+ continue;
+
+ retval = pm_runtime_get_sync(link->supplier);
+ if (retval < 0) {
+ pm_runtime_put_noidle(link->supplier);
+ return retval;
+ }
+ link->rpm_active = true;
+ }
+ return 0;
+}
+
+static void rpm_put_suppliers(struct device *dev)
+{
+ struct device_link *link;
+
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+ if (link->rpm_active &&
+ READ_ONCE(link->status) != DL_STATE_SUPPLIER_UNBIND) {
+ pm_runtime_put(link->supplier);
+ link->rpm_active = false;
+ }
+}
+
/**
* __rpm_callback - Run a given runtime PM callback for a given device.
* @cb: Runtime PM callback to run.
@@ -266,19 +304,55 @@ 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;
+ 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();
+
+ retval = rpm_get_suppliers(dev);
+ if (retval)
+ goto fail;
+
+ 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:
+ rpm_put_suppliers(dev);
+
+ device_links_read_unlock(idx);
+ }
+
spin_lock_irq(&dev->power.lock);
+ }
return retval;
}
@@ -1447,6 +1521,79 @@ 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).
+ *
+ * Links with the DL_FLAG_STATELESS flag set are ignored.
+ *
+ * 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 DL_STATE_SUPPLIER_UNBIND and
+ * therefore rpm_active can't be manipulated concurrently.
+ */
+void pm_runtime_clean_up_links(struct device *dev)
+{
+ struct device_link *link;
+ int idx;
+
+ idx = device_links_read_lock();
+
+ list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
+ if (link->flags & DL_FLAG_STATELESS)
+ continue;
+
+ if (link->rpm_active) {
+ pm_runtime_put_noidle(dev);
+ link->rpm_active = false;
+ }
+ }
+
+ device_links_read_unlock(idx);
+}
+
+/**
+ * pm_runtime_get_suppliers - Resume and reference-count supplier devices.
+ * @dev: Consumer device.
+ */
+void pm_runtime_get_suppliers(struct device *dev)
+{
+ struct device_link *link;
+ int idx;
+
+ idx = device_links_read_lock();
+
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+ if (link->flags & DL_FLAG_PM_RUNTIME)
+ pm_runtime_get_sync(link->supplier);
+
+ device_links_read_unlock(idx);
+}
+
+/**
+ * pm_runtime_put_suppliers - Drop references to supplier devices.
+ * @dev: Consumer device.
+ */
+void pm_runtime_put_suppliers(struct device *dev)
+{
+ struct device_link *link;
+ int idx;
+
+ idx = device_links_read_lock();
+
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+ if (link->flags & DL_FLAG_PM_RUNTIME)
+ pm_runtime_put(link->supplier);
+
+ 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
@@ -730,9 +730,13 @@ enum device_link_state {
*
* STATELESS: The core won't track the presence of supplier/consumer drivers.
* AUTOREMOVE: Remove this link automatically on consumer driver unbind.
+ * 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 DL_FLAG_STATELESS BIT(0)
#define DL_FLAG_AUTOREMOVE BIT(1)
+#define DL_FLAG_PM_RUNTIME BIT(2)
+#define DL_FLAG_RPM_ACTIVE BIT(3)
/**
* struct device_link - Device link representation.
@@ -742,6 +746,7 @@ enum device_link_state {
* @c_node: Hook to the consumer device's list of links to suppliers.
* @status: The state of the link (with respect to the presence of drivers).
* @flags: Link flags.
+ * @rpm_active: Whether or not the consumer device is runtime-PM-active.
* @rcu_head: An RCU head to use for deferred execution of SRCU callbacks.
*/
struct device_link {
@@ -751,6 +756,7 @@ struct device_link {
struct list_head c_node;
enum device_link_state status;
u32 flags;
+ bool rpm_active;
#ifdef CONFIG_SRCU
struct rcu_head rcu_head;
#endif
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,9 @@ 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);
+extern void pm_runtime_get_suppliers(struct device *dev);
+extern void pm_runtime_put_suppliers(struct device *dev);
static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
{
@@ -186,6 +189,9 @@ 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) {}
+static inline void pm_runtime_get_suppliers(struct device *dev) {}
+static inline void pm_runtime_put_suppliers(struct device *dev) {}
#endif /* !CONFIG_PM */
From: Rafael J. Wysocki <[email protected]>
Add an internal wrapper around __device_release_driver() that will
acquire device locks and do the necessary checks before calling it.
The next patch will make use of it.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Tested-by: Marek Szyprowski <[email protected]>
---
drivers/base/dd.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -811,6 +811,22 @@ static void __device_release_driver(stru
}
}
+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.
@@ -825,9 +841,7 @@ void device_release_driver(struct device
* 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);
@@ -852,15 +866,7 @@ void driver_detach(struct device_driver
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);
}
}
From: Rafael J. Wysocki <[email protected]>
Currently, there is a problem with taking functional dependencies
between devices into account.
What I mean by a "functional dependency" is when the driver of device
B needs device A to be functional and (generally) its driver to be
present in order to work properly. This has certain consequences
for power management (suspend/resume and runtime PM ordering) and
shutdown ordering of these devices. In general, it also implies that
the driver of A needs to be working for B to be probed successfully
and it cannot be unbound from the device before the B's driver.
Support for representing those functional dependencies between
devices is added here to allow the driver core to track them and act
on them in certain cases where applicable.
The argument for doing that in the driver core is that there are
quite a few distinct use cases involving device dependencies, they
are relatively hard to get right in a driver (if one wants to
address all of them properly) and it only gets worse if multiplied
by the number of drivers potentially needing to do it. Morever, at
least one case (asynchronous system suspend/resume) cannot be handled
in a single driver at all, because it requires the driver of A to
wait for B to suspend (during system suspend) and the driver of B to
wait for A to resume (during system resume).
For this reason, represent dependencies between devices as "links",
with the help of struct device_link objects each containing pointers
to the "linked" devices, a list node for each of them, status
information, flags, and an RCU head for synchronization.
Also add two new list heads, representing the lists of links to the
devices that depend on the given one (consumers) and to the devices
depended on by it (suppliers), and a "driver presence status" field
(needed for figuring out initial states of device links) to struct
device.
The entire data structure consisting of all of the lists of link
objects for all devices is protected by a mutex (for link object
addition/removal and for list walks during device driver probing
and removal) and by SRCU (for list walking in other case that will
be introduced by subsequent change sets). If CONFIG_SRCU is not
selected, however, an rwsem is used for protecting the entire data
structure.
In addition, each link object has an internal status field whose
value reflects whether or not drivers are bound to the devices
pointed to by the link or probing/removal of their drivers is in
progress etc. That field is only modified under the device links
mutex, but it may be read outside of it in some cases (introduced by
subsequent change sets), so modifications of it are annotated with
WRITE_ONCE().
New links are added by calling device_link_add() which takes three
arguments: pointers to the devices in question and flags. In
particular, if DL_FLAG_STATELESS is set in the flags, the link status
is not to be taken into account for this link and the driver core
will not manage it. In turn, if DL_FLAG_AUTOREMOVE is set in the
flags, the driver core will remove the link automatically when the
consumer device driver unbinds from it.
One of the actions carried out by device_link_add() is to reorder
the lists used for device shutdown and system suspend/resume to
put the consumer device along with all of its children and all of
its consumers (and so on, recursively) to the ends of those lists
in order to ensure the right ordering between all of the supplier
and consumer devices.
For this reason, it is not possible to create a link between two
devices if the would-be supplier device already depends on the
would-be consumer device as either a direct descendant of it or a
consumer of one of its direct descendants or one of its consumers
and so on.
There are two types of link objects, persistent and non-persistent.
The persistent ones stay around until one of the target devices is
deleted, while the non-persistent ones are removed automatically when
the consumer driver unbinds from its device (ie. they are assumed to
be valid only as long as the consumer device has a driver bound to
it). Persistent links are created by default and non-persistent
links are created when the DL_FLAG_AUTOREMOVE flag is passed
to device_link_add().
Both persistent and non-persistent device links can be deleted
with an explicit call to device_link_del().
Links created without the DL_FLAG_STATELESS flag set are managed
by the driver core using a simple state machine. There are 5 states
each link can be in: DORMANT (unused), AVAILABLE (the supplier driver
is present and functional), CONSUMER_PROBE (the consumer driver is
probing), ACTIVE (both supplier and consumer drivers are present and
functional), and SUPPLIER_UNBIND (the supplier driver is unbinding).
The driver core updates the link state automatically depending on
what happens to the linked devices and for each link state specific
actions are taken in addition to that.
For example, if the supplier driver unbinds from its device, the
driver core will also unbind the drivers of all of its consumers
automatically under the assumption that they cannot function
properly without the supplier. Analogously, the driver core will
only allow the consumer driver to bind to its device if the
supplier driver is present and functional (ie. the link is in
the AVAILABLE state). If that's not the case, it will rely on
the existing deferred probing mechanism to wait for the supplier
driver to become available.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v5 -> v6:
- Fall back to using an rwsem for protecting the device links data structure
if SRCU is not available (mutex+SRCU is used as before if CONFIG_SRCU is set,
so the code should have not changed on systems with SRCU selected).
- Use BIT(x) in flag definitions.
v4 -> v5:
- Redefine device_link_add() to take three arguments, the supplier and consumer
pointers and flags, and to figure out the initial link state automatically.
- Move the links-related fields in struct device to a separate sub-structure
and add a "driver presence tracking" field to it (to help device_link_add()
to do its job).
- Modify device_links_check_suppliers(), device_links_driver_bound(),
device_links_no_driver(), device_links_driver_cleanup(), device_links_busy(),
and device_links_unbind_consumers() to walk link lists under device_links_lock
(to make the new "driver presence tracking" mechanism work reliably).
- Drop the (not necessary any more) spinlock from struct device_link.
- Rename symbols to better reflect their purpose (flags vs link states etc).
v3 -> v4:
- Add the in_dpm_list field to struct dev_pm_info and use it for checking
if the device has been added to dpm_list already, which is needed for
handling the "not registered consumer" case in device_link_add().
- Rework device_link_add() to handle consumer devices that have not been
registered before calling it.
- Drop the parent check from device_link_add().
- Rename device_links_driver_gone() to device_links_driver_cleanup()
- Rearrange device_links_unbind_consumers() to avoid one spin_unlock()
instance (which wasn't necessary).
- Introduce device_links_purge() to delete links from a device going away
and make device_del() call it (instead of running links-related code
directly).
- Move the invocation of device_links_ckeck_suppliers() to really_probe().
- Change the description of the DEVICE_LINK_STATELESS flag.
---
drivers/base/base.h | 13 +
drivers/base/core.c | 540 +++++++++++++++++++++++++++++++++++++++++++++
drivers/base/dd.c | 41 +++
drivers/base/power/main.c | 2
drivers/base/power/power.h | 10
include/linux/device.h | 80 ++++++
include/linux/pm.h | 1
7 files changed, 682 insertions(+), 5 deletions(-)
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -559,6 +559,7 @@ struct dev_pm_info {
pm_message_t power_state;
unsigned int can_wakeup:1;
unsigned int async_suspend:1;
+ bool in_dpm_list:1; /* Owned by the PM core */
bool is_prepared:1; /* Owned by the PM core */
bool is_suspended:1; /* Ditto */
bool is_noirq_suspended:1;
Index: linux-pm/drivers/base/power/power.h
===================================================================
--- linux-pm.orig/drivers/base/power/power.h
+++ linux-pm/drivers/base/power/power.h
@@ -127,6 +127,11 @@ extern void device_pm_move_after(struct
extern void device_pm_move_last(struct device *);
extern void device_pm_check_callbacks(struct device *dev);
+static inline bool device_pm_initialized(struct device *dev)
+{
+ return dev->power.in_dpm_list;
+}
+
#else /* !CONFIG_PM_SLEEP */
static inline void device_pm_sleep_init(struct device *dev) {}
@@ -146,6 +151,11 @@ static inline void device_pm_move_last(s
static inline void device_pm_check_callbacks(struct device *dev) {}
+static inline bool device_pm_initialized(struct device *dev)
+{
+ return device_is_registered(dev);
+}
+
#endif /* !CONFIG_PM_SLEEP */
static inline void device_pm_init(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
@@ -131,6 +131,7 @@ void device_pm_add(struct device *dev)
dev_warn(dev, "parent %s should not be sleeping\n",
dev_name(dev->parent));
list_add_tail(&dev->power.entry, &dpm_list);
+ dev->power.in_dpm_list = true;
mutex_unlock(&dpm_list_mtx);
}
@@ -145,6 +146,7 @@ void device_pm_remove(struct device *dev
complete_all(&dev->power.completion);
mutex_lock(&dpm_list_mtx);
list_del_init(&dev->power.entry);
+ dev->power.in_dpm_list = false;
mutex_unlock(&dpm_list_mtx);
device_wakeup_disable(dev);
pm_runtime_remove(dev);
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -708,6 +708,81 @@ struct device_dma_parameters {
};
/**
+ * enum device_link_state - Device link states.
+ * @DL_STATE_NONE: The presence of the drivers is not being tracked.
+ * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
+ * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer is not.
+ * @DL_STATE_CONSUMER_PROBE: The consumer is probing (supplier driver present).
+ * @DL_STATE_ACTIVE: Both the supplier and consumer drivers are present.
+ * @DL_STATE_SUPPLIER_UNBIND: The supplier driver is unbinding.
+ */
+enum device_link_state {
+ DL_STATE_NONE = -1,
+ DL_STATE_DORMANT = 0,
+ DL_STATE_AVAILABLE,
+ DL_STATE_CONSUMER_PROBE,
+ DL_STATE_ACTIVE,
+ DL_STATE_SUPPLIER_UNBIND,
+};
+
+/*
+ * Device link flags.
+ *
+ * STATELESS: The core won't track the presence of supplier/consumer drivers.
+ * AUTOREMOVE: Remove this link automatically on consumer driver unbind.
+ */
+#define DL_FLAG_STATELESS BIT(0)
+#define DL_FLAG_AUTOREMOVE BIT(1)
+
+/**
+ * struct device_link - Device link representation.
+ * @supplier: The device on the supplier end of the link.
+ * @s_node: Hook to the supplier device's list of links to consumers.
+ * @consumer: The device on the consumer end of the link.
+ * @c_node: Hook to the consumer device's list of links to suppliers.
+ * @status: The state of the link (with respect to the presence of drivers).
+ * @flags: Link flags.
+ * @rcu_head: An RCU head to use for deferred execution of SRCU callbacks.
+ */
+struct device_link {
+ struct device *supplier;
+ struct list_head s_node;
+ struct device *consumer;
+ struct list_head c_node;
+ enum device_link_state status;
+ u32 flags;
+#ifdef CONFIG_SRCU
+ struct rcu_head rcu_head;
+#endif
+};
+
+/**
+ * enum dl_dev_state - Device driver presence tracking information.
+ * @DL_DEV_NO_DRIVER: There is no driver attached to the device.
+ * @DL_DEV_PROBING: A driver is probing.
+ * @DL_DEV_DRIVER_BOUND: The driver has been bound to the device.
+ * @DL_DEV_UNBINDING: The driver is unbinding from the device.
+ */
+enum dl_dev_state {
+ DL_DEV_NO_DRIVER = 0,
+ DL_DEV_PROBING,
+ DL_DEV_DRIVER_BOUND,
+ DL_DEV_UNBINDING,
+};
+
+/**
+ * struct dev_links_info - Device data related to device links.
+ * @suppliers: List of links to supplier devices.
+ * @consumers: List of links to consumer devices.
+ * @status: Driver status information.
+ */
+struct dev_links_info {
+ struct list_head suppliers;
+ struct list_head consumers;
+ enum dl_dev_state status;
+};
+
+/**
* struct device - The basic device structure
* @parent: The device's "parent" device, the device to which it is attached.
* In most cases, a parent device is some sort of bus or host
@@ -799,6 +874,7 @@ struct device {
core doesn't touch it */
void *driver_data; /* Driver data, set and get with
dev_set/get_drvdata */
+ struct dev_links_info links;
struct dev_pm_info power;
struct dev_pm_domain *pm_domain;
@@ -1116,6 +1192,10 @@ extern void device_shutdown(void);
/* debugging and troubleshooting/diagnostic helpers. */
extern const char *dev_driver_string(const struct device *dev);
+/* Device links interface. */
+struct device_link *device_link_add(struct device *consumer,
+ struct device *supplier, u32 flags);
+void device_link_del(struct device_link *link);
#ifdef CONFIG_PRINTK
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,13 @@ extern int devtmpfs_init(void);
#else
static inline int devtmpfs_init(void) { return 0; }
#endif
+
+/* Device links support */
+extern int device_links_read_lock(void);
+extern void device_links_read_unlock(int idx);
+extern int device_links_check_suppliers(struct device *dev);
+extern void device_links_driver_bound(struct device *dev);
+extern void device_links_driver_cleanup(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,541 @@ static int __init sysfs_deprecated_setup
early_param("sysfs.deprecated", sysfs_deprecated_setup);
#endif
+/* Device links support. */
+
+#ifdef CONFIG_SRCU
+static DEFINE_MUTEX(device_links_lock);
+DEFINE_STATIC_SRCU(device_links_srcu);
+
+static inline void device_links_write_lock(void)
+{
+ mutex_lock(&device_links_lock);
+}
+
+static inline void device_links_write_unlock(void)
+{
+ mutex_unlock(&device_links_lock);
+}
+
+int device_links_read_lock(void)
+{
+ return srcu_read_lock(&device_links_srcu);
+}
+
+void device_links_read_unlock(int idx)
+{
+ srcu_read_unlock(&device_links_srcu, idx);
+}
+#else /* !CONFIG_SRCU */
+static DECLARE_RWSEM(device_links_lock);
+
+static inline void device_links_write_lock(void)
+{
+ down_write(&device_links_lock);
+}
+
+static inline void device_links_write_unlock(void)
+{
+ up_write(&device_links_lock);
+}
+
+int device_links_read_lock(void)
+{
+ down_read(&device_links_lock);
+ return 0;
+}
+
+void device_links_read_unlock(int not_used)
+{
+ up_read(&device_links_lock);
+}
+#endif /* !CONFIG_SRCU */
+
+/**
+ * device_is_dependent - Check if one device depends on another one
+ * @dev: Device to check dependencies for.
+ * @target: Device to check against.
+ *
+ * Check if @target depends on @dev or any device dependent on it (its child or
+ * its consumer etc). Return 1 if that is the case or 0 otherwise.
+ */
+static int device_is_dependent(struct device *dev, void *target)
+{
+ struct device_link *link;
+ int ret;
+
+ if (WARN_ON(dev == target))
+ return 1;
+
+ ret = device_for_each_child(dev, target, device_is_dependent);
+ if (ret)
+ return ret;
+
+ list_for_each_entry(link, &dev->links.consumers, s_node) {
+ if (WARN_ON(link->consumer == target))
+ return 1;
+
+ ret = device_is_dependent(link->consumer, target);
+ if (ret)
+ break;
+ }
+ return ret;
+}
+
+static int device_reorder_to_tail(struct device *dev, void *not_used)
+{
+ struct device_link *link;
+
+ /*
+ * Devices that have not been registered yet will be put to the ends
+ * of the lists during the registration, so skip them here.
+ */
+ if (device_is_registered(dev))
+ devices_kset_move_last(dev);
+
+ if (device_pm_initialized(dev))
+ device_pm_move_last(dev);
+
+ device_for_each_child(dev, NULL, device_reorder_to_tail);
+ list_for_each_entry(link, &dev->links.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.
+ * @flags: Link flags.
+ *
+ * If the DL_FLAG_AUTOREMOVE is set, the link will be removed automatically
+ * when the consumer device driver unbinds from it. The combination of both
+ * DL_FLAG_AUTOREMOVE and DL_FLAG_STATELESS set is invalid and will cause NULL
+ * to be returned.
+ *
+ * A side effect of the link creation is re-ordering of dpm_list and the
+ * devices_kset list by moving the consumer device and all devices depending
+ * on it to the ends of these lists (that does not happen to devices that have
+ * not been registered when this function is called).
+ *
+ * The supplier device is required to be registered when this function is called
+ * and NULL will be returned if that is not the case. The consumer device need
+ * not be registerd, however.
+ */
+struct device_link *device_link_add(struct device *consumer,
+ struct device *supplier, u32 flags)
+{
+ struct device_link *link;
+
+ if (!consumer || !supplier ||
+ ((flags & DL_FLAG_STATELESS) && (flags & DL_FLAG_AUTOREMOVE)))
+ return NULL;
+
+ device_links_write_lock();
+ device_pm_lock();
+
+ /*
+ * If the supplier has not been fully registered yet or there is a
+ * reverse dependency between the consumer and the supplier already in
+ * the graph, return NULL.
+ */
+ if (!device_pm_initialized(supplier)
+ || device_is_dependent(consumer, supplier)) {
+ link = NULL;
+ goto out;
+ }
+
+ list_for_each_entry(link, &supplier->links.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);
+ link->flags = flags;
+
+ /* Deterine the initial link state. */
+ if (flags & DL_FLAG_STATELESS) {
+ link->status = DL_STATE_NONE;
+ } else {
+ switch (supplier->links.status) {
+ case DL_DEV_DRIVER_BOUND:
+ switch (consumer->links.status) {
+ case DL_DEV_PROBING:
+ link->status = DL_STATE_CONSUMER_PROBE;
+ break;
+ case DL_DEV_DRIVER_BOUND:
+ link->status = DL_STATE_ACTIVE;
+ break;
+ default:
+ link->status = DL_STATE_AVAILABLE;
+ break;
+ }
+ break;
+ case DL_DEV_UNBINDING:
+ link->status = DL_STATE_SUPPLIER_UNBIND;
+ break;
+ default:
+ link->status = DL_STATE_DORMANT;
+ break;
+ }
+ }
+
+ /*
+ * 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_reorder_to_tail(consumer, NULL);
+
+ list_add_tail_rcu(&link->s_node, &supplier->links.consumers);
+ list_add_tail_rcu(&link->c_node, &consumer->links.suppliers);
+
+ dev_info(consumer, "Linked as a consumer to %s\n", dev_name(supplier));
+
+ out:
+ device_pm_unlock();
+ device_links_write_unlock();
+ return link;
+}
+EXPORT_SYMBOL_GPL(device_link_add);
+
+static void device_link_free(struct device_link *link)
+{
+ put_device(link->consumer);
+ put_device(link->supplier);
+ kfree(link);
+}
+
+#ifdef CONFIG_SRCU
+static void __device_link_free_srcu(struct rcu_head *rhead)
+{
+ device_link_free(container_of(rhead, struct device_link, rcu_head));
+}
+
+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);
+}
+#else /* !CONFIG_SRCU */
+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(&link->s_node);
+ list_del(&link->c_node);
+ device_link_free(link);
+}
+#endif /* !CONFIG_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)
+{
+ device_links_write_lock();
+ device_pm_lock();
+ __device_link_del(link);
+ device_pm_unlock();
+ device_links_write_unlock();
+}
+EXPORT_SYMBOL_GPL(device_link_del);
+
+static void device_links_missing_supplier(struct device *dev)
+{
+ struct device_link *link;
+
+ list_for_each_entry(link, &dev->links.suppliers, c_node)
+ if (link->status == DL_STATE_CONSUMER_PROBE)
+ WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
+}
+
+/**
+ * device_links_check_suppliers - Check presence of supplier drivers.
+ * @dev: Consumer device.
+ *
+ * Check links from this device to any suppliers. Walk the list of the device's
+ * links to suppliers and see if all of them are available. If not, simply
+ * return -EPROBE_DEFER.
+ *
+ * We need to guarantee that the supplier will not go away after the check has
+ * been positive here. It only can go away in __device_release_driver() and
+ * that function checks the device's links to consumers. This means we need to
+ * mark the link as "consumer probe in progress" to make the supplier removal
+ * wait for us to complete (or bad things may happen).
+ *
+ * Links with the DL_FLAG_STATELESS flag set are ignored.
+ */
+int device_links_check_suppliers(struct device *dev)
+{
+ struct device_link *link;
+ int ret = 0;
+
+ device_links_write_lock();
+
+ list_for_each_entry(link, &dev->links.suppliers, c_node) {
+ if (link->flags & DL_FLAG_STATELESS)
+ continue;
+
+ if (link->status != DL_STATE_AVAILABLE) {
+ device_links_missing_supplier(dev);
+ ret = -EPROBE_DEFER;
+ break;
+ }
+ WRITE_ONCE(link->status, DL_STATE_CONSUMER_PROBE);
+ }
+ dev->links.status = DL_DEV_PROBING;
+
+ device_links_write_unlock();
+ return ret;
+}
+
+/**
+ * device_links_driver_bound - Update device links after probing its driver.
+ * @dev: Device to update the links for.
+ *
+ * The probe has been successful, so update links from this device to any
+ * consumers by changing their status to "available".
+ *
+ * Also change the status of @dev's links to suppliers to "active".
+ *
+ * Links with the DL_FLAG_STATELESS flag set are ignored.
+ */
+void device_links_driver_bound(struct device *dev)
+{
+ struct device_link *link;
+
+ device_links_write_lock();
+
+ list_for_each_entry(link, &dev->links.consumers, s_node) {
+ if (link->flags & DL_FLAG_STATELESS)
+ continue;
+
+ WARN_ON(link->status != DL_STATE_DORMANT);
+ WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
+ }
+
+ list_for_each_entry(link, &dev->links.suppliers, c_node) {
+ if (link->flags & DL_FLAG_STATELESS)
+ continue;
+
+ WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
+ WRITE_ONCE(link->status, DL_STATE_ACTIVE);
+ }
+
+ dev->links.status = DL_DEV_DRIVER_BOUND;
+
+ device_links_write_unlock();
+}
+
+/**
+ * __device_links_no_driver - Update links of a device without a driver.
+ * @dev: Device without a drvier.
+ *
+ * Delete all non-persistent links from this device to any suppliers.
+ *
+ * Persistent links stay around, but their status is changed to "available",
+ * unless they already are in the "supplier unbind in progress" state in which
+ * case they need not be updated.
+ *
+ * Links with the DL_FLAG_STATELESS flag set are ignored.
+ */
+static void __device_links_no_driver(struct device *dev)
+{
+ struct device_link *link, *ln;
+
+ list_for_each_entry_safe_reverse(link, ln, &dev->links.suppliers, c_node) {
+ if (link->flags & DL_FLAG_STATELESS)
+ continue;
+
+ if (link->flags & DL_FLAG_AUTOREMOVE)
+ __device_link_del(link);
+ else if (link->status != DL_STATE_SUPPLIER_UNBIND)
+ WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
+ }
+
+ dev->links.status = DL_DEV_NO_DRIVER;
+}
+
+void device_links_no_driver(struct device *dev)
+{
+ device_links_write_lock();
+ __device_links_no_driver(dev);
+ device_links_write_unlock();
+}
+
+/**
+ * device_links_driver_cleanup - Update links after driver removal.
+ * @dev: Device whose driver has just gone away.
+ *
+ * Update links to consumers for @dev by changing their status to "dormant" and
+ * invoke %__device_links_no_driver() to update links to suppliers for it as
+ * appropriate.
+ *
+ * Links with the DL_FLAG_STATELESS flag set are ignored.
+ */
+void device_links_driver_cleanup(struct device *dev)
+{
+ struct device_link *link;
+
+ device_links_write_lock();
+
+ list_for_each_entry(link, &dev->links.consumers, s_node) {
+ if (link->flags & DL_FLAG_STATELESS)
+ continue;
+
+ WARN_ON(link->flags & DL_FLAG_AUTOREMOVE);
+ WARN_ON(link->status != DL_STATE_SUPPLIER_UNBIND);
+ WRITE_ONCE(link->status, DL_STATE_DORMANT);
+ }
+
+ __device_links_no_driver(dev);
+
+ device_links_write_unlock();
+}
+
+/**
+ * device_links_busy - Check if there are any busy links to consumers.
+ * @dev: Device to check.
+ *
+ * Check each consumer of the device and return 'true' if its link's status
+ * is one of "consumer probe" or "active" (meaning that the given consumer is
+ * probing right now or its driver is present). Otherwise, change the link
+ * state to "supplier unbind" to prevent the consumer from being probed
+ * successfully going forward.
+ *
+ * Return 'false' if there are no probing or active consumers.
+ *
+ * Links with the DL_FLAG_STATELESS flag set are ignored.
+ */
+bool device_links_busy(struct device *dev)
+{
+ struct device_link *link;
+ bool ret = false;
+
+ device_links_write_lock();
+
+ list_for_each_entry(link, &dev->links.consumers, s_node) {
+ if (link->flags & DL_FLAG_STATELESS)
+ continue;
+
+ if (link->status == DL_STATE_CONSUMER_PROBE
+ || link->status == DL_STATE_ACTIVE) {
+ ret = true;
+ break;
+ }
+ WRITE_ONCE(link->status, DL_STATE_SUPPLIER_UNBIND);
+ }
+
+ dev->links.status = DL_DEV_UNBINDING;
+
+ device_links_write_unlock();
+ return ret;
+}
+
+/**
+ * device_links_unbind_consumers - Force unbind consumers of the given device.
+ * @dev: Device to unbind the consumers of.
+ *
+ * Walk the list of links to consumers for @dev and if any of them is in the
+ * "consumer probe" state, wait for all device probes in progress to complete
+ * and start over.
+ *
+ * If that's not the case, change the status of the link to "supplier unbind"
+ * and check if the link was in the "active" state. If so, force the consumer
+ * driver to unbind and start over (the consumer will not re-probe as we have
+ * changed the state of the link already).
+ *
+ * Links with the DL_FLAG_STATELESS flag set are ignored.
+ */
+void device_links_unbind_consumers(struct device *dev)
+{
+ struct device_link *link;
+
+ start:
+ device_links_write_lock();
+
+ list_for_each_entry(link, &dev->links.consumers, s_node) {
+ enum device_link_state status;
+
+ if (link->flags & DL_FLAG_STATELESS)
+ continue;
+
+ status = link->status;
+ if (status == DL_STATE_CONSUMER_PROBE) {
+ device_links_write_unlock();
+
+ wait_for_device_probe();
+ goto start;
+ }
+ WRITE_ONCE(link->status, DL_STATE_SUPPLIER_UNBIND);
+ if (status == DL_STATE_ACTIVE) {
+ struct device *consumer = link->consumer;
+
+ get_device(consumer);
+
+ device_links_write_unlock();
+
+ device_release_driver_internal(consumer, NULL,
+ consumer->parent);
+ put_device(consumer);
+ goto start;
+ }
+ }
+
+ device_links_write_unlock();
+}
+
+/**
+ * device_links_purge - Delete existing links to other devices.
+ * @dev: Target device.
+ */
+static void device_links_purge(struct device *dev)
+{
+ struct device_link *link, *ln;
+
+ /*
+ * Delete all of the remaining links from this device to any other
+ * devices (either consumers or suppliers).
+ */
+ device_links_write_lock();
+
+ list_for_each_entry_safe_reverse(link, ln, &dev->links.suppliers, c_node) {
+ WARN_ON(link->status == DL_STATE_ACTIVE);
+ __device_link_del(link);
+ }
+
+ list_for_each_entry_safe_reverse(link, ln, &dev->links.consumers, s_node) {
+ WARN_ON(link->status != DL_STATE_DORMANT &&
+ link->status != DL_STATE_NONE);
+ __device_link_del(link);
+ }
+
+ device_links_write_unlock();
+}
+
+/* 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 +1246,9 @@ void device_initialize(struct device *de
#ifdef CONFIG_GENERIC_MSI_IRQ
INIT_LIST_HEAD(&dev->msi_list);
#endif
+ INIT_LIST_HEAD(&dev->links.consumers);
+ INIT_LIST_HEAD(&dev->links.suppliers);
+ dev->links.status = DL_DEV_NO_DRIVER;
}
EXPORT_SYMBOL_GPL(device_initialize);
@@ -1258,6 +1796,8 @@ void device_del(struct device *dev)
if (dev->bus)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
BUS_NOTIFY_DEL_DEVICE, dev);
+
+ device_links_purge(dev);
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
@@ -244,6 +244,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);
@@ -337,6 +338,10 @@ static int really_probe(struct device *d
return ret;
}
+ ret = device_links_check_suppliers(dev);
+ if (ret)
+ return ret;
+
atomic_inc(&probe_count);
pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
drv->bus->name, __func__, drv->name, dev_name(dev));
@@ -415,6 +420,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;
@@ -771,7 +777,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;
@@ -780,6 +786,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);
@@ -795,6 +820,8 @@ static void __device_release_driver(stru
dev->bus->remove(dev);
else if (drv->remove)
drv->remove(dev);
+
+ device_links_driver_cleanup(dev);
devres_release_all(dev);
dev->driver = NULL;
dev_set_drvdata(dev, NULL);
@@ -811,16 +838,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)
@@ -833,6 +860,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)
{
On Sunday, October 30, 2016 05:22:13 PM Rafael J. Wysocki wrote:
> Hi,
>
> Let me quote from the previous intro messages for this series first:
>
> > > Time for another update. :-)
> > >
> > > Fewer changes this time, mostly to address issues found by Lukas and
> > > Marek.
> > >
> > > The most significant one is to make device_link_add() cope with the case
> > > when
> > > the consumer device has not been registered yet when it is called. The
> > > supplier device still is required to be registered and the function will
> > > return NULL if that is not the case.
> > >
> > > Another significant change is in patch [4/5] that now makes the core apply
> > > pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the
> > > probing of a consumer one (in analogy with the parent).
> >
> > One more update after some conversations during LinuxCon Europe.
> >
> > The main point was to make it possible for device_link_add() to figure out
> > the initial state of the link instead of expecting the caller to provide it
> > which might not be reliable enough in general.
> >
> > In this version device_link_add() takes three arguments, the supplier and
> > consumer pointers and flags and it sets the correct initial state of the
> > link automatically (unless invoked with the "stateless" flag, of course).
> > The cost is one additional field in struct device (I moved all of the
> > links-related fields in struct device to a separate sub-structure while at
> > it) to track the "driver presence status" of the device (to be used by
> > device_link_add()).
> >
> > In addition to that, the links list walks in the core.c and dd.c code are
> > under the device links mutex now, so the iternal link spinlock is not needed
> > any more and I have renamed symbols to distinguish between flags, link
> > states and device "driver presence statuses".
>
> The most significant change in this revision with respect to the previous one is
> related to the fact that SRCU is not available on some architectures, so the
> code falls back to using an RW semaphore for synchronization if SRCU is not
> there. Fortunately, the code changes needed for that turned out to be quite
> straightforward and confined to the second patch.
>
> Apart from this, the flags are defined using BIT(x) now (instead of open coding
> the latter in the flag definitions).
>
> Updated is mostly patch [2/5]. Patches [1,3,5/5] have not changed (except for
> trivial rebasing) and patch [4/5] needed to be refreshed on top of the modified
> [2/5].
>
> FWIW, I've run the series through 0-day which has not reported any problems
> with it.
BTW, the series is available from the device-links-test branch in my tree:
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git device-links-test
in case someone wants to try it out.
Thanks,
Rafael
On Sun, Oct 30, 2016 at 05:22:13PM +0100, Rafael J. Wysocki wrote:
> Hi,
>
> Let me quote from the previous intro messages for this series first:
>
> > > Time for another update. :-)
> > >
> > > Fewer changes this time, mostly to address issues found by Lukas and
> > > Marek.
> > >
> > > The most significant one is to make device_link_add() cope with the case
> > > when
> > > the consumer device has not been registered yet when it is called. The
> > > supplier device still is required to be registered and the function will
> > > return NULL if that is not the case.
> > >
> > > Another significant change is in patch [4/5] that now makes the core apply
> > > pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the
> > > probing of a consumer one (in analogy with the parent).
> >
> > One more update after some conversations during LinuxCon Europe.
> >
> > The main point was to make it possible for device_link_add() to figure out
> > the initial state of the link instead of expecting the caller to provide it
> > which might not be reliable enough in general.
> >
> > In this version device_link_add() takes three arguments, the supplier and
> > consumer pointers and flags and it sets the correct initial state of the
> > link automatically (unless invoked with the "stateless" flag, of course).
> > The cost is one additional field in struct device (I moved all of the
> > links-related fields in struct device to a separate sub-structure while at
> > it) to track the "driver presence status" of the device (to be used by
> > device_link_add()).
> >
> > In addition to that, the links list walks in the core.c and dd.c code are
> > under the device links mutex now, so the iternal link spinlock is not needed
> > any more and I have renamed symbols to distinguish between flags, link
> > states and device "driver presence statuses".
>
> The most significant change in this revision with respect to the previous one is
> related to the fact that SRCU is not available on some architectures, so the
> code falls back to using an RW semaphore for synchronization if SRCU is not
> there. Fortunately, the code changes needed for that turned out to be quite
> straightforward and confined to the second patch.
>
> Apart from this, the flags are defined using BIT(x) now (instead of open coding
> the latter in the flag definitions).
>
> Updated is mostly patch [2/5]. Patches [1,3,5/5] have not changed (except for
> trivial rebasing) and patch [4/5] needed to be refreshed on top of the modified
> [2/5].
>
> FWIW, I've run the series through 0-day which has not reported any problems
> with it.
Great, they are now applied to my tree, thanks again for doing this
work.
greg k-h
On Monday, October 31, 2016 11:47:03 AM Greg Kroah-Hartman wrote:
> On Sun, Oct 30, 2016 at 05:22:13PM +0100, Rafael J. Wysocki wrote:
> > Hi,
> >
> > Let me quote from the previous intro messages for this series first:
> >
> > > > Time for another update. :-)
> > > >
> > > > Fewer changes this time, mostly to address issues found by Lukas and
> > > > Marek.
> > > >
> > > > The most significant one is to make device_link_add() cope with the case
> > > > when
> > > > the consumer device has not been registered yet when it is called. The
> > > > supplier device still is required to be registered and the function will
> > > > return NULL if that is not the case.
> > > >
> > > > Another significant change is in patch [4/5] that now makes the core apply
> > > > pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the
> > > > probing of a consumer one (in analogy with the parent).
> > >
> > > One more update after some conversations during LinuxCon Europe.
> > >
> > > The main point was to make it possible for device_link_add() to figure out
> > > the initial state of the link instead of expecting the caller to provide it
> > > which might not be reliable enough in general.
> > >
> > > In this version device_link_add() takes three arguments, the supplier and
> > > consumer pointers and flags and it sets the correct initial state of the
> > > link automatically (unless invoked with the "stateless" flag, of course).
> > > The cost is one additional field in struct device (I moved all of the
> > > links-related fields in struct device to a separate sub-structure while at
> > > it) to track the "driver presence status" of the device (to be used by
> > > device_link_add()).
> > >
> > > In addition to that, the links list walks in the core.c and dd.c code are
> > > under the device links mutex now, so the iternal link spinlock is not needed
> > > any more and I have renamed symbols to distinguish between flags, link
> > > states and device "driver presence statuses".
> >
> > The most significant change in this revision with respect to the previous one is
> > related to the fact that SRCU is not available on some architectures, so the
> > code falls back to using an RW semaphore for synchronization if SRCU is not
> > there. Fortunately, the code changes needed for that turned out to be quite
> > straightforward and confined to the second patch.
> >
> > Apart from this, the flags are defined using BIT(x) now (instead of open coding
> > the latter in the flag definitions).
> >
> > Updated is mostly patch [2/5]. Patches [1,3,5/5] have not changed (except for
> > trivial rebasing) and patch [4/5] needed to be refreshed on top of the modified
> > [2/5].
> >
> > FWIW, I've run the series through 0-day which has not reported any problems
> > with it.
>
> Great, they are now applied to my tree, thanks again for doing this
> work.
Thanks!
Hi Greg,
On 2016-10-31 18:47, Greg Kroah-Hartman wrote:
> On Sun, Oct 30, 2016 at 05:22:13PM +0100, Rafael J. Wysocki wrote:
>> Let me quote from the previous intro messages for this series first:
>>
>>>> Time for another update. :-)
>>>>
>>>> Fewer changes this time, mostly to address issues found by Lukas and
>>>> Marek.
>>>>
>>>> The most significant one is to make device_link_add() cope with the case
>>>> when
>>>> the consumer device has not been registered yet when it is called. The
>>>> supplier device still is required to be registered and the function will
>>>> return NULL if that is not the case.
>>>>
>>>> Another significant change is in patch [4/5] that now makes the core apply
>>>> pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the
>>>> probing of a consumer one (in analogy with the parent).
>>> One more update after some conversations during LinuxCon Europe.
>>>
>>> The main point was to make it possible for device_link_add() to figure out
>>> the initial state of the link instead of expecting the caller to provide it
>>> which might not be reliable enough in general.
>>>
>>> In this version device_link_add() takes three arguments, the supplier and
>>> consumer pointers and flags and it sets the correct initial state of the
>>> link automatically (unless invoked with the "stateless" flag, of course).
>>> The cost is one additional field in struct device (I moved all of the
>>> links-related fields in struct device to a separate sub-structure while at
>>> it) to track the "driver presence status" of the device (to be used by
>>> device_link_add()).
>>>
>>> In addition to that, the links list walks in the core.c and dd.c code are
>>> under the device links mutex now, so the iternal link spinlock is not needed
>>> any more and I have renamed symbols to distinguish between flags, link
>>> states and device "driver presence statuses".
>> The most significant change in this revision with respect to the previous one is
>> related to the fact that SRCU is not available on some architectures, so the
>> code falls back to using an RW semaphore for synchronization if SRCU is not
>> there. Fortunately, the code changes needed for that turned out to be quite
>> straightforward and confined to the second patch.
>>
>> Apart from this, the flags are defined using BIT(x) now (instead of open coding
>> the latter in the flag definitions).
>>
>> Updated is mostly patch [2/5]. Patches [1,3,5/5] have not changed (except for
>> trivial rebasing) and patch [4/5] needed to be refreshed on top of the modified
>> [2/5].
>>
>> FWIW, I've run the series through 0-day which has not reported any problems
>> with it.
> Great, they are now applied to my tree, thanks again for doing this
> work.
Thanks for merging those patches! Could you provide a stable tag with
them, so I can
ask Joerg to merge my Exynos IOMMU PM patches on top of it via IOMMU tree?
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Wed, Nov 02, 2016 at 08:58:38AM +0100, Marek Szyprowski wrote:
> Hi Greg,
>
>
> On 2016-10-31 18:47, Greg Kroah-Hartman wrote:
> > On Sun, Oct 30, 2016 at 05:22:13PM +0100, Rafael J. Wysocki wrote:
> > > Let me quote from the previous intro messages for this series first:
> > >
> > > > > Time for another update. :-)
> > > > >
> > > > > Fewer changes this time, mostly to address issues found by Lukas and
> > > > > Marek.
> > > > >
> > > > > The most significant one is to make device_link_add() cope with the case
> > > > > when
> > > > > the consumer device has not been registered yet when it is called. The
> > > > > supplier device still is required to be registered and the function will
> > > > > return NULL if that is not the case.
> > > > >
> > > > > Another significant change is in patch [4/5] that now makes the core apply
> > > > > pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the
> > > > > probing of a consumer one (in analogy with the parent).
> > > > One more update after some conversations during LinuxCon Europe.
> > > >
> > > > The main point was to make it possible for device_link_add() to figure out
> > > > the initial state of the link instead of expecting the caller to provide it
> > > > which might not be reliable enough in general.
> > > >
> > > > In this version device_link_add() takes three arguments, the supplier and
> > > > consumer pointers and flags and it sets the correct initial state of the
> > > > link automatically (unless invoked with the "stateless" flag, of course).
> > > > The cost is one additional field in struct device (I moved all of the
> > > > links-related fields in struct device to a separate sub-structure while at
> > > > it) to track the "driver presence status" of the device (to be used by
> > > > device_link_add()).
> > > >
> > > > In addition to that, the links list walks in the core.c and dd.c code are
> > > > under the device links mutex now, so the iternal link spinlock is not needed
> > > > any more and I have renamed symbols to distinguish between flags, link
> > > > states and device "driver presence statuses".
> > > The most significant change in this revision with respect to the previous one is
> > > related to the fact that SRCU is not available on some architectures, so the
> > > code falls back to using an RW semaphore for synchronization if SRCU is not
> > > there. Fortunately, the code changes needed for that turned out to be quite
> > > straightforward and confined to the second patch.
> > >
> > > Apart from this, the flags are defined using BIT(x) now (instead of open coding
> > > the latter in the flag definitions).
> > >
> > > Updated is mostly patch [2/5]. Patches [1,3,5/5] have not changed (except for
> > > trivial rebasing) and patch [4/5] needed to be refreshed on top of the modified
> > > [2/5].
> > >
> > > FWIW, I've run the series through 0-day which has not reported any problems
> > > with it.
> > Great, they are now applied to my tree, thanks again for doing this
> > work.
>
> Thanks for merging those patches! Could you provide a stable tag with them,
> so I can
> ask Joerg to merge my Exynos IOMMU PM patches on top of it via IOMMU tree?
My trees do not get rebased so you can pull from it directly right now,
or if you really need a signed tag, I can make one up, but it will not
be until Monday that I can do that.
thanks,
greg k-h
On Wed, Nov 02, 2016 at 08:58:38AM +0100, Marek Szyprowski wrote:
> Hi Greg,
>
>
> On 2016-10-31 18:47, Greg Kroah-Hartman wrote:
> > On Sun, Oct 30, 2016 at 05:22:13PM +0100, Rafael J. Wysocki wrote:
> > > Let me quote from the previous intro messages for this series first:
> > >
> > > > > Time for another update. :-)
> > > > >
> > > > > Fewer changes this time, mostly to address issues found by Lukas and
> > > > > Marek.
> > > > >
> > > > > The most significant one is to make device_link_add() cope with the case
> > > > > when
> > > > > the consumer device has not been registered yet when it is called. The
> > > > > supplier device still is required to be registered and the function will
> > > > > return NULL if that is not the case.
> > > > >
> > > > > Another significant change is in patch [4/5] that now makes the core apply
> > > > > pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the
> > > > > probing of a consumer one (in analogy with the parent).
> > > > One more update after some conversations during LinuxCon Europe.
> > > >
> > > > The main point was to make it possible for device_link_add() to figure out
> > > > the initial state of the link instead of expecting the caller to provide it
> > > > which might not be reliable enough in general.
> > > >
> > > > In this version device_link_add() takes three arguments, the supplier and
> > > > consumer pointers and flags and it sets the correct initial state of the
> > > > link automatically (unless invoked with the "stateless" flag, of course).
> > > > The cost is one additional field in struct device (I moved all of the
> > > > links-related fields in struct device to a separate sub-structure while at
> > > > it) to track the "driver presence status" of the device (to be used by
> > > > device_link_add()).
> > > >
> > > > In addition to that, the links list walks in the core.c and dd.c code are
> > > > under the device links mutex now, so the iternal link spinlock is not needed
> > > > any more and I have renamed symbols to distinguish between flags, link
> > > > states and device "driver presence statuses".
> > > The most significant change in this revision with respect to the previous one is
> > > related to the fact that SRCU is not available on some architectures, so the
> > > code falls back to using an RW semaphore for synchronization if SRCU is not
> > > there. Fortunately, the code changes needed for that turned out to be quite
> > > straightforward and confined to the second patch.
> > >
> > > Apart from this, the flags are defined using BIT(x) now (instead of open coding
> > > the latter in the flag definitions).
> > >
> > > Updated is mostly patch [2/5]. Patches [1,3,5/5] have not changed (except for
> > > trivial rebasing) and patch [4/5] needed to be refreshed on top of the modified
> > > [2/5].
> > >
> > > FWIW, I've run the series through 0-day which has not reported any problems
> > > with it.
> > Great, they are now applied to my tree, thanks again for doing this
> > work.
>
> Thanks for merging those patches! Could you provide a stable tag with them,
> so I can
> ask Joerg to merge my Exynos IOMMU PM patches on top of it via IOMMU tree?
You want these patches to be merged into stable?! This is a whole new set of
functionality, the patches in no way describe any *fixes* or critical issues,
why are you saying this is needed? What makes you believe this is a stable
candidate?
Luis
Hi Luis,
On 2016-11-07 22:15, Luis R. Rodriguez wrote:
> On Wed, Nov 02, 2016 at 08:58:38AM +0100, Marek Szyprowski wrote:
>> On 2016-10-31 18:47, Greg Kroah-Hartman wrote:
>>> On Sun, Oct 30, 2016 at 05:22:13PM +0100, Rafael J. Wysocki wrote:
>>>> Let me quote from the previous intro messages for this series first:
>>>>
>>>>>> Time for another update. :-)
>>>>>>
>>>>>> Fewer changes this time, mostly to address issues found by Lukas and
>>>>>> Marek.
>>>>>>
>>>>>> The most significant one is to make device_link_add() cope with the case
>>>>>> when
>>>>>> the consumer device has not been registered yet when it is called. The
>>>>>> supplier device still is required to be registered and the function will
>>>>>> return NULL if that is not the case.
>>>>>>
>>>>>> Another significant change is in patch [4/5] that now makes the core apply
>>>>>> pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the
>>>>>> probing of a consumer one (in analogy with the parent).
>>>>> One more update after some conversations during LinuxCon Europe.
>>>>>
>>>>> The main point was to make it possible for device_link_add() to figure out
>>>>> the initial state of the link instead of expecting the caller to provide it
>>>>> which might not be reliable enough in general.
>>>>>
>>>>> In this version device_link_add() takes three arguments, the supplier and
>>>>> consumer pointers and flags and it sets the correct initial state of the
>>>>> link automatically (unless invoked with the "stateless" flag, of course).
>>>>> The cost is one additional field in struct device (I moved all of the
>>>>> links-related fields in struct device to a separate sub-structure while at
>>>>> it) to track the "driver presence status" of the device (to be used by
>>>>> device_link_add()).
>>>>>
>>>>> In addition to that, the links list walks in the core.c and dd.c code are
>>>>> under the device links mutex now, so the iternal link spinlock is not needed
>>>>> any more and I have renamed symbols to distinguish between flags, link
>>>>> states and device "driver presence statuses".
>>>> The most significant change in this revision with respect to the previous one is
>>>> related to the fact that SRCU is not available on some architectures, so the
>>>> code falls back to using an RW semaphore for synchronization if SRCU is not
>>>> there. Fortunately, the code changes needed for that turned out to be quite
>>>> straightforward and confined to the second patch.
>>>>
>>>> Apart from this, the flags are defined using BIT(x) now (instead of open coding
>>>> the latter in the flag definitions).
>>>>
>>>> Updated is mostly patch [2/5]. Patches [1,3,5/5] have not changed (except for
>>>> trivial rebasing) and patch [4/5] needed to be refreshed on top of the modified
>>>> [2/5].
>>>>
>>>> FWIW, I've run the series through 0-day which has not reported any problems
>>>> with it.
>>> Great, they are now applied to my tree, thanks again for doing this
>>> work.
>> Thanks for merging those patches! Could you provide a stable tag with them,
>> so I can
>> ask Joerg to merge my Exynos IOMMU PM patches on top of it via IOMMU tree?
> You want these patches to be merged into stable?! This is a whole new set of
> functionality, the patches in no way describe any *fixes* or critical issues,
> why are you saying this is needed? What makes you believe this is a stable
> candidate?
I don't want to merge those patches to stale kernel release. By 'stable
tag' I just
meant something that can be pulled by Joerg to have a base for my Exynos
IOMMU
patches.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Tue, Nov 08, 2016 at 07:36:45AM +0100, Marek Szyprowski wrote:
> Hi Luis,
>
>
> On 2016-11-07 22:15, Luis R. Rodriguez wrote:
> > On Wed, Nov 02, 2016 at 08:58:38AM +0100, Marek Szyprowski wrote:
> > > On 2016-10-31 18:47, Greg Kroah-Hartman wrote:
> > > > On Sun, Oct 30, 2016 at 05:22:13PM +0100, Rafael J. Wysocki wrote:
> > > > > Let me quote from the previous intro messages for this series first:
> > > > >
> > > > > > > Time for another update. :-)
> > > > > > >
> > > > > > > Fewer changes this time, mostly to address issues found by Lukas and
> > > > > > > Marek.
> > > > > > >
> > > > > > > The most significant one is to make device_link_add() cope with the case
> > > > > > > when
> > > > > > > the consumer device has not been registered yet when it is called. The
> > > > > > > supplier device still is required to be registered and the function will
> > > > > > > return NULL if that is not the case.
> > > > > > >
> > > > > > > Another significant change is in patch [4/5] that now makes the core apply
> > > > > > > pm_runtime_get_sync()/pm_runtime_put() to supplier devices around the
> > > > > > > probing of a consumer one (in analogy with the parent).
> > > > > > One more update after some conversations during LinuxCon Europe.
> > > > > >
> > > > > > The main point was to make it possible for device_link_add() to figure out
> > > > > > the initial state of the link instead of expecting the caller to provide it
> > > > > > which might not be reliable enough in general.
> > > > > >
> > > > > > In this version device_link_add() takes three arguments, the supplier and
> > > > > > consumer pointers and flags and it sets the correct initial state of the
> > > > > > link automatically (unless invoked with the "stateless" flag, of course).
> > > > > > The cost is one additional field in struct device (I moved all of the
> > > > > > links-related fields in struct device to a separate sub-structure while at
> > > > > > it) to track the "driver presence status" of the device (to be used by
> > > > > > device_link_add()).
> > > > > >
> > > > > > In addition to that, the links list walks in the core.c and dd.c code are
> > > > > > under the device links mutex now, so the iternal link spinlock is not needed
> > > > > > any more and I have renamed symbols to distinguish between flags, link
> > > > > > states and device "driver presence statuses".
> > > > > The most significant change in this revision with respect to the previous one is
> > > > > related to the fact that SRCU is not available on some architectures, so the
> > > > > code falls back to using an RW semaphore for synchronization if SRCU is not
> > > > > there. Fortunately, the code changes needed for that turned out to be quite
> > > > > straightforward and confined to the second patch.
> > > > >
> > > > > Apart from this, the flags are defined using BIT(x) now (instead of open coding
> > > > > the latter in the flag definitions).
> > > > >
> > > > > Updated is mostly patch [2/5]. Patches [1,3,5/5] have not changed (except for
> > > > > trivial rebasing) and patch [4/5] needed to be refreshed on top of the modified
> > > > > [2/5].
> > > > >
> > > > > FWIW, I've run the series through 0-day which has not reported any problems
> > > > > with it.
> > > > Great, they are now applied to my tree, thanks again for doing this
> > > > work.
> > > Thanks for merging those patches! Could you provide a stable tag with them,
> > > so I can
> > > ask Joerg to merge my Exynos IOMMU PM patches on top of it via IOMMU tree?
> > You want these patches to be merged into stable?! This is a whole new set of
> > functionality, the patches in no way describe any *fixes* or critical issues,
> > why are you saying this is needed? What makes you believe this is a stable
> > candidate?
>
> I don't want to merge those patches to stale kernel release. By 'stable tag'
> I just meant something that can be pulled by Joerg to have a base for my
> Exynos IOMMU patches.
Phew! Thanks for the clarification!
Luis
Hi Rafael,
spotted what looks like a bug in the device links runtime PM code:
When resuming a device, __rpm_callback() calls rpm_get_suppliers(dev):
> + retval = rpm_get_suppliers(dev);
> + if (retval)
> + goto fail;
This will walk the list of suppliers and call pm_runtime_get_sync()
for each of them:
> +static int rpm_get_suppliers(struct device *dev)
> +{
> + struct device_link *link;
> +
> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
> + int retval;
[...]
> + retval = pm_runtime_get_sync(link->supplier);
> + if (retval < 0) {
> + pm_runtime_put_noidle(link->supplier);
> + return retval;
If pm_runtime_get_sync() failed, e.g. because runtime PM is disabled
on a supplier, the function will put the reference of the failed
supplier and return.
Back in __rpm_callback() we jump to the fail mark, where we call
rpm_put_suppliers().
> + fail:
> + rpm_put_suppliers(dev);
> +
> + device_links_read_unlock(idx);
This walks the list of suppliers and releases a ref for each of them:
> +static void rpm_put_suppliers(struct device *dev)
> +{
> + struct device_link *link;
> +
> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
> + if (link->rpm_active &&
> + READ_ONCE(link->status) != DL_STATE_SUPPLIER_UNBIND) {
> + pm_runtime_put(link->supplier);
> + link->rpm_active = false;
> + }
> +}
This looks wrong: We've already put a ref on the failed supplier, so here
we're putting another one. And if there are further suppliers in the list
following the failed one, we'll decrement their refcount even though we've
never incremented it.
Thanks,
Lukas
On Sun, Dec 18, 2016 at 3:01 PM, Lukas Wunner <[email protected]> wrote:
> Hi Rafael,
>
> spotted what looks like a bug in the device links runtime PM code:
>
> When resuming a device, __rpm_callback() calls rpm_get_suppliers(dev):
>
>> + retval = rpm_get_suppliers(dev);
>> + if (retval)
>> + goto fail;
>
>
> This will walk the list of suppliers and call pm_runtime_get_sync()
> for each of them:
>
>> +static int rpm_get_suppliers(struct device *dev)
>> +{
>> + struct device_link *link;
>> +
>> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
>> + int retval;
> [...]
>> + retval = pm_runtime_get_sync(link->supplier);
>> + if (retval < 0) {
>> + pm_runtime_put_noidle(link->supplier);
>> + return retval;
>
>
> If pm_runtime_get_sync() failed, e.g. because runtime PM is disabled
> on a supplier, the function will put the reference of the failed
> supplier and return.
>
> Back in __rpm_callback() we jump to the fail mark, where we call
> rpm_put_suppliers().
>
>> + fail:
>> + rpm_put_suppliers(dev);
>> +
>> + device_links_read_unlock(idx);
>
>
> This walks the list of suppliers and releases a ref for each of them:
>
>> +static void rpm_put_suppliers(struct device *dev)
>> +{
>> + struct device_link *link;
>> +
>> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
>> + if (link->rpm_active &&
>> + READ_ONCE(link->status) != DL_STATE_SUPPLIER_UNBIND) {
>> + pm_runtime_put(link->supplier);
>> + link->rpm_active = false;
>> + }
>> +}
>
>
> This looks wrong: We've already put a ref on the failed supplier, so here
> we're putting another one.
Are we? I would think link->rpm_active would be false for the failed
one, wouldn't it?
> And if there are further suppliers in the list
> following the failed one, we'll decrement their refcount even though we've
> never incremented it.
I'm not following you here, sorry.
Thanks,
Rafael
On Sun, Dec 18, 2016 at 04:53:26PM +0100, Rafael J. Wysocki wrote:
> On Sun, Dec 18, 2016 at 3:01 PM, Lukas Wunner <[email protected]> wrote:
> > Hi Rafael,
> >
> > spotted what looks like a bug in the device links runtime PM code:
> >
> > When resuming a device, __rpm_callback() calls rpm_get_suppliers(dev):
> >
> >> + retval = rpm_get_suppliers(dev);
> >> + if (retval)
> >> + goto fail;
> >
> >
> > This will walk the list of suppliers and call pm_runtime_get_sync()
> > for each of them:
> >
> >> +static int rpm_get_suppliers(struct device *dev)
> >> +{
> >> + struct device_link *link;
> >> +
> >> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
> >> + int retval;
> > [...]
> >> + retval = pm_runtime_get_sync(link->supplier);
> >> + if (retval < 0) {
> >> + pm_runtime_put_noidle(link->supplier);
> >> + return retval;
> >
> >
> > If pm_runtime_get_sync() failed, e.g. because runtime PM is disabled
> > on a supplier, the function will put the reference of the failed
> > supplier and return.
> >
> > Back in __rpm_callback() we jump to the fail mark, where we call
> > rpm_put_suppliers().
> >
> >> + fail:
> >> + rpm_put_suppliers(dev);
> >> +
> >> + device_links_read_unlock(idx);
> >
> >
> > This walks the list of suppliers and releases a ref for each of them:
> >
> >> +static void rpm_put_suppliers(struct device *dev)
> >> +{
> >> + struct device_link *link;
> >> +
> >> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
> >> + if (link->rpm_active &&
> >> + READ_ONCE(link->status) != DL_STATE_SUPPLIER_UNBIND) {
> >> + pm_runtime_put(link->supplier);
> >> + link->rpm_active = false;
> >> + }
> >> +}
> >
> >
> > This looks wrong: We've already put a ref on the failed supplier, so here
> > we're putting another one.
>
> Are we? I would think link->rpm_active would be false for the failed
> one, wouldn't it?
Ah, so link->rpm_active means the consumer is holding a ref on the supplier.
Missed that, sorry for the false alarm and thanks for the clarification.
Lukas
On Sun, Dec 18, 2016 at 5:37 PM, Lukas Wunner <[email protected]> wrote:
> On Sun, Dec 18, 2016 at 04:53:26PM +0100, Rafael J. Wysocki wrote:
>> On Sun, Dec 18, 2016 at 3:01 PM, Lukas Wunner <[email protected]> wrote:
>> > Hi Rafael,
>> >
>> > spotted what looks like a bug in the device links runtime PM code:
>> >
>> > When resuming a device, __rpm_callback() calls rpm_get_suppliers(dev):
>> >
>> >> + retval = rpm_get_suppliers(dev);
>> >> + if (retval)
>> >> + goto fail;
>> >
>> >
>> > This will walk the list of suppliers and call pm_runtime_get_sync()
>> > for each of them:
>> >
>> >> +static int rpm_get_suppliers(struct device *dev)
>> >> +{
>> >> + struct device_link *link;
>> >> +
>> >> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
>> >> + int retval;
>> > [...]
>> >> + retval = pm_runtime_get_sync(link->supplier);
>> >> + if (retval < 0) {
>> >> + pm_runtime_put_noidle(link->supplier);
>> >> + return retval;
>> >
>> >
>> > If pm_runtime_get_sync() failed, e.g. because runtime PM is disabled
>> > on a supplier, the function will put the reference of the failed
>> > supplier and return.
>> >
>> > Back in __rpm_callback() we jump to the fail mark, where we call
>> > rpm_put_suppliers().
>> >
>> >> + fail:
>> >> + rpm_put_suppliers(dev);
>> >> +
>> >> + device_links_read_unlock(idx);
>> >
>> >
>> > This walks the list of suppliers and releases a ref for each of them:
>> >
>> >> +static void rpm_put_suppliers(struct device *dev)
>> >> +{
>> >> + struct device_link *link;
>> >> +
>> >> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
>> >> + if (link->rpm_active &&
>> >> + READ_ONCE(link->status) != DL_STATE_SUPPLIER_UNBIND) {
>> >> + pm_runtime_put(link->supplier);
>> >> + link->rpm_active = false;
>> >> + }
>> >> +}
>> >
>> >
>> > This looks wrong: We've already put a ref on the failed supplier, so here
>> > we're putting another one.
>>
>> Are we? I would think link->rpm_active would be false for the failed
>> one, wouldn't it?
>
> Ah, so link->rpm_active means the consumer is holding a ref on the supplier.
Yes, that's the idea. :-)
> Missed that, sorry for the false alarm and thanks for the clarification.
No problem.
Thanks,
Rafael