[driver-core PATCH v5 0/9] Add NUMA aware async_schedule calls

[driver-core PATCH v5 0/9] Add NUMA aware async_schedule calls

[Alexander Duyck]

This patch set provides functionality that will help to improve the
locality of the async_schedule calls used to provide deferred
initialization.

This patch set originally started out with me focused on just the one call
to async_schedule_domain in the nvdimm tree that was being used to
defer the device_add call however after doing some digging I realized the
scope of this was much broader than I had originally planned. As such I
went through and reworked the underlying infrastructure down to replacing
the queue_work call itself with a function of my own and opted to try and
provide a NUMA aware solution that would work for a broader audience.

RFC->v1:
    Dropped nvdimm patch to submit later.
        It relies on code in libnvdimm development tree.
    Simplified queue_work_near to just convert node into a CPU.
    Split up drivers core and PM core patches.
v1->v2:
    Renamed queue_work_near to queue_work_node
    Added WARN_ON_ONCE if we use queue_work_node with per-cpu workqueue
v2->v3:
    Added Acked-by for queue_work_node patch
    Continued rename from _near to _node to be consistent with queue_work_node
        Renamed async_schedule_near_domain to async_schedule_node_domain
        Renamed async_schedule_near to async_schedule_node
    Added kerneldoc for new async_schedule_XXX functions
    Updated patch description for patch 4 to include data on potential gains
v3->v4
    Added patch to consolidate use of need_parent_lock
    Make asynchronous driver probing explicit about use of drvdata
v4->v5
    Added patch to move async_synchronize_full to address deadlock
    Added bit async_probe to act as mutex for probe/remove calls
    Added back nvdimm patch as code it relies on is now in Linus's tree
    Incorporated review comments on parent & device locking consolidation
    Rebased on latest linux-next

---

Alexander Duyck (9):
      workqueue: Provide queue_work_node to queue work near a given NUMA node
      async: Add support for queueing on specific NUMA node
      device core: Consolidate locking and unlocking of parent and device
      driver core: Move async_synchronize_full call
      driver core: Establish clear order of operations for deferred probe and remove
      driver core: Probe devices asynchronously instead of the driver
      driver core: Attach devices on CPU local to device node
      PM core: Use new async_schedule_dev command
      libnvdimm: Schedule device registration on node local to the device


 drivers/base/base.h       |    2 
 drivers/base/bus.c        |   46 +-------
 drivers/base/dd.c         |  249 +++++++++++++++++++++++++++++++++------------
 drivers/base/power/main.c |   12 +-
 drivers/nvdimm/bus.c      |   11 +-
 include/linux/async.h     |   84 +++++++++++++++
 include/linux/device.h    |   35 +++++-
 include/linux/workqueue.h |    2 
 kernel/async.c            |   53 +++++-----
 kernel/workqueue.c        |   84 +++++++++++++++
 10 files changed, 432 insertions(+), 146 deletions(-)

--

[driver-core PATCH v5 1/9] workqueue: Provide queue_work_node to queue work near a given NUMA node

[Alexander Duyck]

This patch provides a new function queue_work_node which is meant to
schedule work on a "random" CPU of the requested NUMA node. The main
motivation for this is to help assist asynchronous init to better improve
boot times for devices that are local to a specific node.

For now we just default to the first CPU that is in the intersection of the
cpumask of the node and the online cpumask. The only exception is if the
CPU is local to the node we will just use the current CPU. This should work
for our purposes as we are currently only using this for unbound work so
the CPU will be translated to a node anyway instead of being directly used.

As we are only using the first CPU to represent the NUMA node for now I am
limiting the scope of the function so that it can only be used with unbound
workqueues.

Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 include/linux/workqueue.h |    2 +
 kernel/workqueue.c        |   84 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 60d673e15632..1f50c1e586e7 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -463,6 +463,8 @@ int workqueue_set_unbound_cpumask(cpumask_var_t cpumask);
 
 extern bool queue_work_on(int cpu, struct workqueue_struct *wq,
 			struct work_struct *work);
+extern bool queue_work_node(int node, struct workqueue_struct *wq,
+			    struct work_struct *work);
 extern bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
 			struct delayed_work *work, unsigned long delay);
 extern bool mod_delayed_work_on(int cpu, struct workqueue_struct *wq,
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 0280deac392e..6ed7c2eb84b0 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1492,6 +1492,90 @@ bool queue_work_on(int cpu, struct workqueue_struct *wq,
 }
 EXPORT_SYMBOL(queue_work_on);
 
+/**
+ * workqueue_select_cpu_near - Select a CPU based on NUMA node
+ * @node: NUMA node ID that we want to bind a CPU from
+ *
+ * This function will attempt to find a "random" cpu available on a given
+ * node. If there are no CPUs available on the given node it will return
+ * WORK_CPU_UNBOUND indicating that we should just schedule to any
+ * available CPU if we need to schedule this work.
+ */
+static int workqueue_select_cpu_near(int node)
+{
+	int cpu;
+
+	/* No point in doing this if NUMA isn't enabled for workqueues */
+	if (!wq_numa_enabled)
+		return WORK_CPU_UNBOUND;
+
+	/* Delay binding to CPU if node is not valid or online */
+	if (node < 0 || node >= MAX_NUMNODES || !node_online(node))
+		return WORK_CPU_UNBOUND;
+
+	/* Use local node/cpu if we are already there */
+	cpu = raw_smp_processor_id();
+	if (node == cpu_to_node(cpu))
+		return cpu;
+
+	/* Use "random" otherwise know as "first" online CPU of node */
+	cpu = cpumask_any_and(cpumask_of_node(node), cpu_online_mask);
+
+	/* If CPU is valid return that, otherwise just defer */
+	return (cpu < nr_cpu_ids) ? cpu : WORK_CPU_UNBOUND;
+}
+
+/**
+ * queue_work_node - queue work on a "random" cpu for a given NUMA node
+ * @node: NUMA node that we are targeting the work for
+ * @wq: workqueue to use
+ * @work: work to queue
+ *
+ * We queue the work to a "random" CPU within a given NUMA node. The basic
+ * idea here is to provide a way to somehow associate work with a given
+ * NUMA node.
+ *
+ * This function will only make a best effort attempt at getting this onto
+ * the right NUMA node. If no node is requested or the requested node is
+ * offline then we just fall back to standard queue_work behavior.
+ *
+ * Currently the "random" CPU ends up being the first available CPU in the
+ * intersection of cpu_online_mask and the cpumask of the node, unless we
+ * are running on the node. In that case we just use the current CPU.
+ *
+ * Return: %false if @work was already on a queue, %true otherwise.
+ */
+bool queue_work_node(int node, struct workqueue_struct *wq,
+		     struct work_struct *work)
+{
+	unsigned long flags;
+	bool ret = false;
+
+	/*
+	 * This current implementation is specific to unbound workqueues.
+	 * Specifically we only return the first available CPU for a given
+	 * node instead of cycling through individual CPUs within the node.
+	 *
+	 * If this is used with a per-cpu workqueue then the logic in
+	 * workqueue_select_cpu_near would need to be updated to allow for
+	 * some round robin type logic.
+	 */
+	WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND));
+
+	local_irq_save(flags);
+
+	if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
+		int cpu = workqueue_select_cpu_near(node);
+
+		__queue_work(cpu, wq, work);
+		ret = true;
+	}
+
+	local_irq_restore(flags);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(queue_work_node);
+
 void delayed_work_timer_fn(struct timer_list *t)
 {
 	struct delayed_work *dwork = from_timer(dwork, t, timer);

[driver-core PATCH v5 2/9] async: Add support for queueing on specific NUMA node

[Alexander Duyck]

This patch introduces four new variants of the async_schedule_ functions
that allow scheduling on a specific NUMA node.

The first two functions are async_schedule_near and
async_schedule_near_domain which end up mapping to async_schedule and
async_schedule_domain but provide NUMA node specific functionality. They
replace the original functions which were moved to inline function
definitions that call the new functions while passing NUMA_NO_NODE.

The second two functions are async_schedule_dev and
async_schedule_dev_domain which provide NUMA specific functionality when
passing a device as the data member and that device has a NUMA node other
than NUMA_NO_NODE.

The main motivation behind this is to address the need to be able to
schedule device specific init work on specific NUMA nodes in order to
improve performance of memory initialization.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 include/linux/async.h |   84 +++++++++++++++++++++++++++++++++++++++++++++++--
 kernel/async.c        |   53 +++++++++++++++++--------------
 2 files changed, 110 insertions(+), 27 deletions(-)

diff --git a/include/linux/async.h b/include/linux/async.h
index 6b0226bdaadc..98a94e6e367d 100644
--- a/include/linux/async.h
+++ b/include/linux/async.h
@@ -14,6 +14,8 @@
 
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/numa.h>
+#include <linux/device.h>
 
 typedef u64 async_cookie_t;
 typedef void (*async_func_t) (void *data, async_cookie_t cookie);
@@ -37,9 +39,85 @@ struct async_domain {
 	struct async_domain _name = { .pending = LIST_HEAD_INIT(_name.pending), \
 				      .registered = 0 }
 
-extern async_cookie_t async_schedule(async_func_t func, void *data);
-extern async_cookie_t async_schedule_domain(async_func_t func, void *data,
-					    struct async_domain *domain);
+async_cookie_t async_schedule_node(async_func_t func, void *data,
+				   int node);
+async_cookie_t async_schedule_node_domain(async_func_t func, void *data,
+					  int node,
+					  struct async_domain *domain);
+
+/**
+ * async_schedule - schedule a function for asynchronous execution
+ * @func: function to execute asynchronously
+ * @data: data pointer to pass to the function
+ *
+ * Returns an async_cookie_t that may be used for checkpointing later.
+ * Note: This function may be called from atomic or non-atomic contexts.
+ */
+static inline async_cookie_t async_schedule(async_func_t func, void *data)
+{
+	return async_schedule_node(func, data, NUMA_NO_NODE);
+}
+
+/**
+ * async_schedule_domain - schedule a function for asynchronous execution within a certain domain
+ * @func: function to execute asynchronously
+ * @data: data pointer to pass to the function
+ * @domain: the domain
+ *
+ * Returns an async_cookie_t that may be used for checkpointing later.
+ * @domain may be used in the async_synchronize_*_domain() functions to
+ * wait within a certain synchronization domain rather than globally.  A
+ * synchronization domain is specified via @domain.
+ * Note: This function may be called from atomic or non-atomic contexts.
+ */
+static inline async_cookie_t
+async_schedule_domain(async_func_t func, void *data,
+		      struct async_domain *domain)
+{
+	return async_schedule_node_domain(func, data, NUMA_NO_NODE, domain);
+}
+
+/**
+ * async_schedule_dev - A device specific version of async_schedule
+ * @func: function to execute asynchronously
+ * @dev: device argument to be passed to function
+ *
+ * Returns an async_cookie_t that may be used for checkpointing later.
+ * @dev is used as both the argument for the function and to provide NUMA
+ * context for where to run the function. By doing this we can try to
+ * provide for the best possible outcome by operating on the device on the
+ * CPUs closest to the device.
+ * Note: This function may be called from atomic or non-atomic contexts.
+ */
+static inline async_cookie_t
+async_schedule_dev(async_func_t func, struct device *dev)
+{
+	return async_schedule_node(func, dev, dev_to_node(dev));
+}
+
+/**
+ * async_schedule_dev_domain - A device specific version of async_schedule_domain
+ * @func: function to execute asynchronously
+ * @dev: device argument to be passed to function
+ * @domain: the domain
+ *
+ * Returns an async_cookie_t that may be used for checkpointing later.
+ * @dev is used as both the argument for the function and to provide NUMA
+ * context for where to run the function. By doing this we can try to
+ * provide for the best possible outcome by operating on the device on the
+ * CPUs closest to the device.
+ * @domain may be used in the async_synchronize_*_domain() functions to
+ * wait within a certain synchronization domain rather than globally.  A
+ * synchronization domain is specified via @domain.
+ * Note: This function may be called from atomic or non-atomic contexts.
+ */
+static inline async_cookie_t
+async_schedule_dev_domain(async_func_t func, struct device *dev,
+			  struct async_domain *domain)
+{
+	return async_schedule_node_domain(func, dev, dev_to_node(dev), domain);
+}
+
 void async_unregister_domain(struct async_domain *domain);
 extern void async_synchronize_full(void);
 extern void async_synchronize_full_domain(struct async_domain *domain);
diff --git a/kernel/async.c b/kernel/async.c
index a893d6170944..23cf67b4b4f8 100644
--- a/kernel/async.c
+++ b/kernel/async.c
@@ -149,7 +149,25 @@ static void async_run_entry_fn(struct work_struct *work)
 	wake_up(&async_done);
 }
 
-static async_cookie_t __async_schedule(async_func_t func, void *data, struct async_domain *domain)
+/**
+ * async_schedule_node_domain - NUMA specific version of async_schedule_domain
+ * @func: function to execute asynchronously
+ * @data: data pointer to pass to the function
+ * @node: NUMA node that we want to schedule this on or close to
+ * @domain: the domain
+ *
+ * Returns an async_cookie_t that may be used for checkpointing later.
+ * @domain may be used in the async_synchronize_*_domain() functions to
+ * wait within a certain synchronization domain rather than globally.  A
+ * synchronization domain is specified via @domain.  Note: This function
+ * may be called from atomic or non-atomic contexts.
+ *
+ * The node requested will be honored on a best effort basis. If the node
+ * has no CPUs associated with it then the work is distributed among all
+ * available CPUs.
+ */
+async_cookie_t async_schedule_node_domain(async_func_t func, void *data,
+					  int node, struct async_domain *domain)
 {
 	struct async_entry *entry;
 	unsigned long flags;
@@ -195,43 +213,30 @@ static async_cookie_t __async_schedule(async_func_t func, void *data, struct asy
 	current->flags |= PF_USED_ASYNC;
 
 	/* schedule for execution */
-	queue_work(system_unbound_wq, &entry->work);
+	queue_work_node(node, system_unbound_wq, &entry->work);
 
 	return newcookie;
 }
+EXPORT_SYMBOL_GPL(async_schedule_node_domain);
 
 /**
- * async_schedule - schedule a function for asynchronous execution
+ * async_schedule_node - NUMA specific version of async_schedule
  * @func: function to execute asynchronously
  * @data: data pointer to pass to the function
+ * @node: NUMA node that we want to schedule this on or close to
  *
  * Returns an async_cookie_t that may be used for checkpointing later.
  * Note: This function may be called from atomic or non-atomic contexts.
- */
-async_cookie_t async_schedule(async_func_t func, void *data)
-{
-	return __async_schedule(func, data, &async_dfl_domain);
-}
-EXPORT_SYMBOL_GPL(async_schedule);
-
-/**
- * async_schedule_domain - schedule a function for asynchronous execution within a certain domain
- * @func: function to execute asynchronously
- * @data: data pointer to pass to the function
- * @domain: the domain
  *
- * Returns an async_cookie_t that may be used for checkpointing later.
- * @domain may be used in the async_synchronize_*_domain() functions to
- * wait within a certain synchronization domain rather than globally.  A
- * synchronization domain is specified via @domain.  Note: This function
- * may be called from atomic or non-atomic contexts.
+ * The node requested will be honored on a best effort basis. If the node
+ * has no CPUs associated with it then the work is distributed among all
+ * available CPUs.
  */
-async_cookie_t async_schedule_domain(async_func_t func, void *data,
-				     struct async_domain *domain)
+async_cookie_t async_schedule_node(async_func_t func, void *data, int node)
 {
-	return __async_schedule(func, data, domain);
+	return async_schedule_node_domain(func, data, node, &async_dfl_domain);
 }
-EXPORT_SYMBOL_GPL(async_schedule_domain);
+EXPORT_SYMBOL_GPL(async_schedule_node);
 
 /**
  * async_synchronize_full - synchronize all asynchronous function calls

[driver-core PATCH v5 3/9] device core: Consolidate locking and unlocking of parent and device

[Alexander Duyck]

This patch is meant to try and consolidate all of the locking and unlocking
of both the parent and device when attaching or removing a driver from a
given device.

To do that I first consolidated the lock pattern into two functions
__device_driver_lock and __device_driver_unlock. After doing that I then
created functions specific to attaching and detaching the driver while
acquiring these locks. By doing this I was able to reduce the number of
spots where we touch need_parent_lock from 12 down to 4.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/base/base.h |    2 +
 drivers/base/bus.c  |   23 ++-----------
 drivers/base/dd.c   |   91 ++++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 77 insertions(+), 39 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 7a419a7a6235..3f22ebd6117a 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -124,6 +124,8 @@ extern int driver_add_groups(struct device_driver *drv,
 			     const struct attribute_group **groups);
 extern void driver_remove_groups(struct device_driver *drv,
 				 const struct attribute_group **groups);
+int device_driver_attach(struct device_driver *drv, struct device *dev);
+void device_driver_detach(struct device *dev);
 
 extern char *make_class_name(const char *name, struct kobject *kobj);
 
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 8bfd27ec73d6..8a630f9bd880 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -184,11 +184,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == drv) {
-		if (dev->parent && dev->bus->need_parent_lock)
-			device_lock(dev->parent);
-		device_release_driver(dev);
-		if (dev->parent && dev->bus->need_parent_lock)
-			device_unlock(dev->parent);
+		device_driver_detach(dev);
 		err = count;
 	}
 	put_device(dev);
@@ -211,13 +207,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
-		if (dev->parent && bus->need_parent_lock)
-			device_lock(dev->parent);
-		device_lock(dev);
-		err = driver_probe_device(drv, dev);
-		device_unlock(dev);
-		if (dev->parent && bus->need_parent_lock)
-			device_unlock(dev->parent);
+		err = device_driver_attach(drv, dev);
 
 		if (err > 0) {
 			/* success */
@@ -769,13 +759,8 @@ EXPORT_SYMBOL_GPL(bus_rescan_devices);
  */
 int device_reprobe(struct device *dev)
 {
-	if (dev->driver) {
-		if (dev->parent && dev->bus->need_parent_lock)
-			device_lock(dev->parent);
-		device_release_driver(dev);
-		if (dev->parent && dev->bus->need_parent_lock)
-			device_unlock(dev->parent);
-	}
+	if (dev->driver)
+		device_driver_detach(dev);
 	return bus_rescan_devices_helper(dev, NULL);
 }
 EXPORT_SYMBOL_GPL(device_reprobe);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 169412ee4ae8..76c40fe69463 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -864,6 +864,60 @@ void device_initial_probe(struct device *dev)
 	__device_attach(dev, true);
 }
 
+/*
+ * __device_driver_lock - acquire locks needed to manipulate dev->drv
+ * @dev: Device we will update driver info for
+ * @parent: Parent device. Needed if the bus requires parent lock
+ *
+ * This function will take the required locks for manipulating dev->drv.
+ * Normally this will just be the @dev lock, but when called for a USB
+ * interface, @parent lock will be held as well.
+ */
+static void __device_driver_lock(struct device *dev, struct device *parent)
+{
+	if (parent && dev->bus->need_parent_lock)
+		device_lock(parent);
+	device_lock(dev);
+}
+
+/*
+ * __device_driver_lock - release locks needed to manipulate dev->drv
+ * @dev: Device we will update driver info for
+ * @parent: Parent device. Needed if the bus requires parent lock
+ *
+ * This function will release the required locks for manipulating dev->drv.
+ * Normally this will just be the the @dev lock, but when called for a
+ * USB interface, @parent lock will be released as well.
+ */
+static void __device_driver_unlock(struct device *dev, struct device *parent)
+{
+	device_unlock(dev);
+	if (parent && dev->bus->need_parent_lock)
+		device_unlock(parent);
+}
+
+/**
+ * device_driver_attach - attach a specific driver to a specific device
+ * @drv: Driver to attach
+ * @dev: Device to attach it to
+ *
+ * Manually attach driver to a device. Will acquire both @dev lock and
+ * @dev->parent lock if needed.
+ */
+int device_driver_attach(struct device_driver *drv, struct device *dev)
+{
+	int ret = 0;
+
+	__device_driver_lock(dev, dev->parent);
+
+	if (!dev->driver)
+		ret = driver_probe_device(drv, dev);
+
+	__device_driver_unlock(dev, dev->parent);
+
+	return ret;
+}
+
 static int __driver_attach(struct device *dev, void *data)
 {
 	struct device_driver *drv = data;
@@ -891,14 +945,7 @@ static int __driver_attach(struct device *dev, void *data)
 		return ret;
 	} /* ret > 0 means positive match */
 
-	if (dev->parent && dev->bus->need_parent_lock)
-		device_lock(dev->parent);
-	device_lock(dev);
-	if (!dev->driver)
-		driver_probe_device(drv, dev);
-	device_unlock(dev);
-	if (dev->parent && dev->bus->need_parent_lock)
-		device_unlock(dev->parent);
+	device_driver_attach(drv, dev);
 
 	return 0;
 }
@@ -932,15 +979,11 @@ static void __device_release_driver(struct device *dev, struct device *parent)
 			async_synchronize_full();
 
 		while (device_links_busy(dev)) {
-			device_unlock(dev);
-			if (parent)
-				device_unlock(parent);
+			__device_driver_unlock(dev, parent);
 
 			device_links_unbind_consumers(dev);
-			if (parent)
-				device_lock(parent);
 
-			device_lock(dev);
+			__device_driver_lock(dev, parent);
 			/*
 			 * A concurrent invocation of the same function might
 			 * have released the driver successfully while this one
@@ -993,16 +1036,12 @@ void device_release_driver_internal(struct device *dev,
 				    struct device_driver *drv,
 				    struct device *parent)
 {
-	if (parent && dev->bus->need_parent_lock)
-		device_lock(parent);
+	__device_driver_lock(dev, parent);
 
-	device_lock(dev);
 	if (!drv || drv == dev->driver)
 		__device_release_driver(dev, parent);
 
-	device_unlock(dev);
-	if (parent && dev->bus->need_parent_lock)
-		device_unlock(parent);
+	__device_driver_unlock(dev, parent);
 }
 
 /**
@@ -1027,6 +1066,18 @@ void device_release_driver(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_release_driver);
 
+/**
+ * device_driver_detach - detach driver from a specific device
+ * @dev: device to detach driver from
+ *
+ * Detach driver from device. Will acquire both @dev lock and @dev->parent
+ * lock if needed.
+ */
+void device_driver_detach(struct device *dev)
+{
+	device_release_driver_internal(dev, NULL, dev->parent);
+}
+
 /**
  * driver_detach - detach driver from all devices it controls.
  * @drv: driver.

[driver-core PATCH v5 4/9] driver core: Move async_synchronize_full call

[Alexander Duyck]

This patch moves the async_synchronize_full call out of
__device_release_driver and into driver_detach.

The idea behind this is that the async_synchronize_full call will only
guarantee that any existing async operations are flushed. This doesn't do
anything to guarantee that a hotplug event that may occur while we are
doing the release of the driver will not be asynchronously scheduled.

By moving this into the driver_detach path we can avoid potential deadlocks
as we aren't holding the device lock at this point and we should not have
the driver we want to flush loaded so the flush will take care of any
asynchronous events the driver we are detaching might have scheduled.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/base/dd.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 76c40fe69463..e74cefeb5b69 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -975,9 +975,6 @@ static void __device_release_driver(struct device *dev, struct device *parent)
 
 	drv = dev->driver;
 	if (drv) {
-		if (driver_allows_async_probing(drv))
-			async_synchronize_full();
-
 		while (device_links_busy(dev)) {
 			__device_driver_unlock(dev, parent);
 
@@ -1087,6 +1084,9 @@ void driver_detach(struct device_driver *drv)
 	struct device_private *dev_prv;
 	struct device *dev;
 
+	if (driver_allows_async_probing(drv))
+		async_synchronize_full();
+
 	for (;;) {
 		spin_lock(&drv->p->klist_devices.k_lock);
 		if (list_empty(&drv->p->klist_devices.k_list)) {

[driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Alexander Duyck]

This patch adds an additional bit to the device struct named async_probe.
This additional bit allows us to guarantee ordering between probe and
remove operations.

This allows us to guarantee that if we execute a remove operation or a
driver load attempt on a given interface it will not attempt to update the
driver member asynchronously following the earlier operation. Previously
this guarantee was not present and could result in us attempting to remove
a driver from an interface only to have it show up later when it is
asynchronously loaded.

One change I made in addition is I replaced the use of "bool X:1" to define
the bitfield to a "u8 X:1" setup in order to resolve some checkpatch
warnings.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/base/dd.c      |  104 +++++++++++++++++++++++++++---------------------
 include/linux/device.h |    9 ++--
 2 files changed, 64 insertions(+), 49 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e74cefeb5b69..ed19cf0d6f9a 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -472,6 +472,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 		 drv->bus->name, __func__, drv->name, dev_name(dev));
 	WARN_ON(!list_empty(&dev->devres_head));
 
+	/* clear async_probe flag as we are no longer deferring driver load */
+	dev->async_probe = false;
 re_probe:
 	dev->driver = drv;
 
@@ -771,6 +773,10 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
 
 	device_lock(dev);
 
+	/* nothing to do if async_probe has been cleared */
+	if (!dev->async_probe)
+		goto out_unlock;
+
 	if (dev->parent)
 		pm_runtime_get_sync(dev->parent);
 
@@ -781,7 +787,7 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
 
 	if (dev->parent)
 		pm_runtime_put(dev->parent);
-
+out_unlock:
 	device_unlock(dev);
 
 	put_device(dev);
@@ -826,6 +832,7 @@ static int __device_attach(struct device *dev, bool allow_async)
 			 */
 			dev_dbg(dev, "scheduling asynchronous probe\n");
 			get_device(dev);
+			dev->async_probe = true;
 			async_schedule(__device_attach_async_helper, dev);
 		} else {
 			pm_request_idle(dev);
@@ -971,62 +978,69 @@ EXPORT_SYMBOL_GPL(driver_attach);
  */
 static void __device_release_driver(struct device *dev, struct device *parent)
 {
-	struct device_driver *drv;
+	struct device_driver *drv = dev->driver;
 
-	drv = dev->driver;
-	if (drv) {
-		while (device_links_busy(dev)) {
-			__device_driver_unlock(dev, parent);
+	/*
+	 * In the event that we are asked to release the driver on an
+	 * interface that is still waiting on a probe we can just terminate
+	 * the probe by setting async_probe to false. When the async call
+	 * is finally completed it will see this state and just exit.
+	 */
+	dev->async_probe = false;
+	if (!drv)
+		return;
 
-			device_links_unbind_consumers(dev);
+	while (device_links_busy(dev)) {
+		__device_driver_unlock(dev, parent);
 
-			__device_driver_lock(dev, parent);
-			/*
-			 * 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;
-		}
+		device_links_unbind_consumers(dev);
 
-		pm_runtime_get_sync(dev);
-		pm_runtime_clean_up_links(dev);
+		__device_driver_lock(dev, parent);
+		/*
+		 * 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;
+	}
 
-		driver_sysfs_remove(dev);
+	pm_runtime_get_sync(dev);
+	pm_runtime_clean_up_links(dev);
 
-		if (dev->bus)
-			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
-						     BUS_NOTIFY_UNBIND_DRIVER,
-						     dev);
+	driver_sysfs_remove(dev);
 
-		pm_runtime_put_sync(dev);
+	if (dev->bus)
+		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
+					     BUS_NOTIFY_UNBIND_DRIVER,
+					     dev);
 
-		if (dev->bus && dev->bus->remove)
-			dev->bus->remove(dev);
-		else if (drv->remove)
-			drv->remove(dev);
+	pm_runtime_put_sync(dev);
 
-		device_links_driver_cleanup(dev);
-		arch_teardown_dma_ops(dev);
+	if (dev->bus && dev->bus->remove)
+		dev->bus->remove(dev);
+	else if (drv->remove)
+		drv->remove(dev);
 
-		devres_release_all(dev);
-		dev->driver = NULL;
-		dev_set_drvdata(dev, NULL);
-		if (dev->pm_domain && dev->pm_domain->dismiss)
-			dev->pm_domain->dismiss(dev);
-		pm_runtime_reinit(dev);
-		dev_pm_set_driver_flags(dev, 0);
+	device_links_driver_cleanup(dev);
+	arch_teardown_dma_ops(dev);
+
+	devres_release_all(dev);
+	dev->driver = NULL;
+	dev_set_drvdata(dev, NULL);
+	if (dev->pm_domain && dev->pm_domain->dismiss)
+		dev->pm_domain->dismiss(dev);
+	pm_runtime_reinit(dev);
+	dev_pm_set_driver_flags(dev, 0);
 
-		klist_remove(&dev->p->knode_driver);
-		device_pm_check_callbacks(dev);
-		if (dev->bus)
-			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
-						     BUS_NOTIFY_UNBOUND_DRIVER,
-						     dev);
+	klist_remove(&dev->p->knode_driver);
+	device_pm_check_callbacks(dev);
+	if (dev->bus)
+		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
+					     BUS_NOTIFY_UNBOUND_DRIVER,
+					     dev);
 
-		kobject_uevent(&dev->kobj, KOBJ_UNBIND);
-	}
+	kobject_uevent(&dev->kobj, KOBJ_UNBIND);
 }
 
 void device_release_driver_internal(struct device *dev,
diff --git a/include/linux/device.h b/include/linux/device.h
index 1b25c7a43f4c..fc7091d436c2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1043,14 +1043,15 @@ struct device {
 	struct iommu_group	*iommu_group;
 	struct iommu_fwspec	*iommu_fwspec;
 
-	bool			offline_disabled:1;
-	bool			offline:1;
-	bool			of_node_reused:1;
+	u8			offline_disabled:1;
+	u8			offline:1;
+	u8			of_node_reused:1;
 #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
     defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
     defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
-	bool			dma_coherent:1;
+	u8			dma_coherent:1;
 #endif
+	u8			async_probe:1;
 };
 
 static inline struct device *kobj_to_dev(struct kobject *kobj)

[driver-core PATCH v5 6/9] driver core: Probe devices asynchronously instead of the driver

[Alexander Duyck]

This change makes it so that we probe devices asynchronously instead of the
driver. This results in us seeing the same behavior if the device is
registered before the driver or after. This way we can avoid serializing
the initialization should the driver not be loaded until after the devices
have already been added.

The motivation behind this is that if we have a set of devices that
take a significant amount of time to load we can greatly reduce the time to
load by processing them in parallel instead of one at a time. In addition,
each device can exist on a different node so placing a single thread on one
CPU to initialize all of the devices for a given driver can result in poor
performance on a system with multiple nodes.

I am using the driver_data member of the device struct to store the driver
pointer while we wait on the deferred probe call. This should be safe to do
as the value will either be set to NULL on a failed probe or driver load
followed by unload, or the driver value itself will be set on a successful
driver load. In addition I have used the async_probe flag to add additional
protection as it will be cleared if someone overwrites the driver_data
member as a part of loading the driver.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/base/bus.c     |   23 +++------------------
 drivers/base/dd.c      |   52 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |   26 +++++++++++++++++++++++-
 3 files changed, 80 insertions(+), 21 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 8a630f9bd880..0cd2eadd0816 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -606,17 +606,6 @@ static ssize_t uevent_store(struct device_driver *drv, const char *buf,
 }
 static DRIVER_ATTR_WO(uevent);
 
-static void driver_attach_async(void *_drv, async_cookie_t cookie)
-{
-	struct device_driver *drv = _drv;
-	int ret;
-
-	ret = driver_attach(drv);
-
-	pr_debug("bus: '%s': driver %s async attach completed: %d\n",
-		 drv->bus->name, drv->name, ret);
-}
-
 /**
  * bus_add_driver - Add a driver to the bus.
  * @drv: driver.
@@ -649,15 +638,9 @@ int bus_add_driver(struct device_driver *drv)
 
 	klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
 	if (drv->bus->p->drivers_autoprobe) {
-		if (driver_allows_async_probing(drv)) {
-			pr_debug("bus: '%s': probing driver %s asynchronously\n",
-				drv->bus->name, drv->name);
-			async_schedule(driver_attach_async, drv);
-		} else {
-			error = driver_attach(drv);
-			if (error)
-				goto out_unregister;
-		}
+		error = driver_attach(drv);
+		if (error)
+			goto out_unregister;
 	}
 	module_add_driver(drv->owner, drv);
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index ed19cf0d6f9a..2fdfe45bb6ea 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -808,6 +808,7 @@ static int __device_attach(struct device *dev, bool allow_async)
 			ret = 1;
 		else {
 			dev->driver = NULL;
+			dev_set_drvdata(dev, NULL);
 			ret = 0;
 		}
 	} else {
@@ -925,6 +926,32 @@ int device_driver_attach(struct device_driver *drv, struct device *dev)
 	return ret;
 }
 
+static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie)
+{
+	struct device *dev = _dev;
+	struct device_driver *drv;
+
+	__device_driver_lock(dev, dev->parent);
+
+	/*
+	 * If someone attempted to bind a driver either successfully or
+	 * unsuccessfully before we got here we should just skip the driver
+	 * probe call.
+	 */
+	drv = dev_get_drv_async(dev);
+	if (drv && !dev->driver)
+		driver_probe_device(drv, dev);
+
+	/* We made our attempt at an async_probe, clear the flag */
+	dev->async_probe = false;
+
+	__device_driver_unlock(dev, dev->parent);
+
+	put_device(dev);
+
+	dev_dbg(dev, "async probe completed\n");
+}
+
 static int __driver_attach(struct device *dev, void *data)
 {
 	struct device_driver *drv = data;
@@ -952,6 +979,25 @@ static int __driver_attach(struct device *dev, void *data)
 		return ret;
 	} /* ret > 0 means positive match */
 
+	if (driver_allows_async_probing(drv)) {
+		/*
+		 * Instead of probing the device synchronously we will
+		 * probe it asynchronously to allow for more parallelism.
+		 *
+		 * We only take the device lock here in order to guarantee
+		 * that the dev->driver and driver_data fields are protected
+		 */
+		dev_dbg(dev, "scheduling asynchronous probe\n");
+		device_lock(dev);
+		if (!dev->driver) {
+			get_device(dev);
+			dev_set_drv_async(dev, drv);
+			async_schedule(__driver_attach_async_helper, dev);
+		}
+		device_unlock(dev);
+		return 0;
+	}
+
 	device_driver_attach(drv, dev);
 
 	return 0;
@@ -1049,6 +1095,12 @@ void device_release_driver_internal(struct device *dev,
 {
 	__device_driver_lock(dev, parent);
 
+	/*
+	 * We shouldn't need to add a check for any pending async_probe here
+	 * because the only caller that will pass us a driver, driver_detach,
+	 * should have been called after the driver was removed from the bus
+	 * and will call async_synchronize_full before we get to this point.
+	 */
 	if (!drv || drv == dev->driver)
 		__device_release_driver(dev, parent);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index fc7091d436c2..4d9a39769081 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -910,7 +910,9 @@ struct dev_links_info {
  * 		variants, which GPIO pins act in what additional roles, and so
  * 		on.  This shrinks the "Board Support Packages" (BSPs) and
  * 		minimizes board-specific #ifdefs in drivers.
- * @driver_data: Private pointer for driver specific info.
+ * @driver_data: Private pointer for driver specific info if driver is
+ *		non-NULL. Pointer to deferred driver to be attached if driver
+ *		is NULL.
  * @links:	Links to suppliers and consumers of this device.
  * @power:	For device power management.
  *		See Documentation/driver-api/pm/devices.rst for details.
@@ -1116,9 +1118,31 @@ static inline void *dev_get_drvdata(const struct device *dev)
 
 static inline void dev_set_drvdata(struct device *dev, void *data)
 {
+	/*
+	 * clear async_probe to prevent us from attempting to read driver_data
+	 * as a driver. We can reset this to true for the one case where we are
+	 * using this to record an actual driver.
+	 */
+	dev->async_probe = false;
 	dev->driver_data = data;
 }
 
+static inline struct device_driver *dev_get_drv_async(const struct device *dev)
+{
+	return dev->async_probe ? dev->driver_data : NULL;
+}
+
+static inline void dev_set_drv_async(struct device *dev,
+				     struct device_driver *drv)
+{
+	/*
+	 * Set async_probe to true indicating we are waiting for this data to be
+	 * loaded as a potential driver.
+	 */
+	dev->driver_data = drv;
+	dev->async_probe = true;
+}
+
 static inline struct pm_subsys_data *dev_to_psd(struct device *dev)
 {
 	return dev ? dev->power.subsys_data : NULL;

[driver-core PATCH v5 7/9] driver core: Attach devices on CPU local to device node

[Alexander Duyck]

This change makes it so that we call the asynchronous probe routines on a
CPU local to the device node. By doing this we should be able to improve
our initialization time significantly as we can avoid having to access the
device from a remote node which may introduce higher latency.

For example, in the case of initializing memory for NVDIMM this can have a
significant impact as initialing 3TB on remote node can take up to 39
seconds while initialing it on a local node only takes 23 seconds. It is
situations like this where we will see the biggest improvement.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/base/dd.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 2fdfe45bb6ea..cf7681309ee3 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -834,7 +834,7 @@ static int __device_attach(struct device *dev, bool allow_async)
 			dev_dbg(dev, "scheduling asynchronous probe\n");
 			get_device(dev);
 			dev->async_probe = true;
-			async_schedule(__device_attach_async_helper, dev);
+			async_schedule_dev(__device_attach_async_helper, dev);
 		} else {
 			pm_request_idle(dev);
 		}
@@ -992,7 +992,7 @@ static int __driver_attach(struct device *dev, void *data)
 		if (!dev->driver) {
 			get_device(dev);
 			dev_set_drv_async(dev, drv);
-			async_schedule(__driver_attach_async_helper, dev);
+			async_schedule_dev(__driver_attach_async_helper, dev);
 		}
 		device_unlock(dev);
 		return 0;

[driver-core PATCH v5 8/9] PM core: Use new async_schedule_dev command

[Alexander Duyck]

This change makes it so that we use the device specific version of the
async_schedule commands to defer various tasks related to power management.
By doing this we should see a slight improvement in performance as any
device that is sensitive to latency/locality in the setup will now be
initializing on the node closest to the device.

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/base/power/main.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index a690fd400260..ebb8b61b52e9 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -726,7 +726,7 @@ void dpm_noirq_resume_devices(pm_message_t state)
 		reinit_completion(&dev->power.completion);
 		if (is_async(dev)) {
 			get_device(dev);
-			async_schedule(async_resume_noirq, dev);
+			async_schedule_dev(async_resume_noirq, dev);
 		}
 	}
 
@@ -883,7 +883,7 @@ void dpm_resume_early(pm_message_t state)
 		reinit_completion(&dev->power.completion);
 		if (is_async(dev)) {
 			get_device(dev);
-			async_schedule(async_resume_early, dev);
+			async_schedule_dev(async_resume_early, dev);
 		}
 	}
 
@@ -1047,7 +1047,7 @@ void dpm_resume(pm_message_t state)
 		reinit_completion(&dev->power.completion);
 		if (is_async(dev)) {
 			get_device(dev);
-			async_schedule(async_resume, dev);
+			async_schedule_dev(async_resume, dev);
 		}
 	}
 
@@ -1366,7 +1366,7 @@ static int device_suspend_noirq(struct device *dev)
 
 	if (is_async(dev)) {
 		get_device(dev);
-		async_schedule(async_suspend_noirq, dev);
+		async_schedule_dev(async_suspend_noirq, dev);
 		return 0;
 	}
 	return __device_suspend_noirq(dev, pm_transition, false);
@@ -1569,7 +1569,7 @@ static int device_suspend_late(struct device *dev)
 
 	if (is_async(dev)) {
 		get_device(dev);
-		async_schedule(async_suspend_late, dev);
+		async_schedule_dev(async_suspend_late, dev);
 		return 0;
 	}
 
@@ -1833,7 +1833,7 @@ static int device_suspend(struct device *dev)
 
 	if (is_async(dev)) {
 		get_device(dev);
-		async_schedule(async_suspend, dev);
+		async_schedule_dev(async_suspend, dev);
 		return 0;
 	}
 

[driver-core PATCH v5 9/9] libnvdimm: Schedule device registration on node local to the device

[Alexander Duyck]

This patch is meant to force the device registration for nvdimm devices to
be closer to the actual device. This is achieved by using either the NUMA
node ID of the region, or of the parent. By doing this we can have
everything above the region based on the region, and everything below the
region based on the nvdimm bus.

By guaranteeing NUMA locality I see an improvement of as high as 25% for
per-node init of a system with 12TB of persistent memory.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/nvdimm/bus.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index f1fb39921236..b1e193541874 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -23,6 +23,7 @@
 #include <linux/ndctl.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/cpu.h>
 #include <linux/fs.h>
 #include <linux/io.h>
 #include <linux/mm.h>
@@ -513,11 +514,15 @@ void __nd_device_register(struct device *dev)
 		set_dev_node(dev, to_nd_region(dev)->numa_node);
 
 	dev->bus = &nvdimm_bus_type;
-	if (dev->parent)
+	if (dev->parent) {
 		get_device(dev->parent);
+		if (dev_to_node(dev) == NUMA_NO_NODE)
+			set_dev_node(dev, dev_to_node(dev->parent));
+	}
 	get_device(dev);
-	async_schedule_domain(nd_async_device_register, dev,
-			&nd_async_domain);
+
+	async_schedule_dev_domain(nd_async_device_register, dev,
+				  &nd_async_domain);
 }
 
 void nd_device_register(struct device *dev)

Re: [driver-core PATCH v5 1/9] workqueue: Provide queue_work_node to queue work near a given NUMA node

[Bart Van Assche]

On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> +/**
> + * workqueue_select_cpu_near - Select a CPU based on NUMA node
> + * @node: NUMA node ID that we want to bind a CPU from
                                          ^^^^
                                          select?
> +	/* If CPU is valid return that, otherwise just defer */
> +	return (cpu < nr_cpu_ids) ? cpu : WORK_CPU_UNBOUND;

Please leave out the superfluous parentheses if this patch series would have to
be reposted.

Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [driver-core PATCH v5 0/9] Add NUMA aware async_schedule calls

[Bart Van Assche]

On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> This patch set provides functionality that will help to improve the
> locality of the async_schedule calls used to provide deferred
> initialization.

Hi Alexander,

Is this patch series perhaps available in a public git tree? That would make
it easier for me to test my sd changes on top of your patches than applying
this patch series myself on kernel v4.20-rc1.

Thanks,

Bart.

Re: [driver-core PATCH v5 4/9] driver core: Move async_synchronize_full call

[Bart Van Assche]

On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> This patch moves the async_synchronize_full call out of
> __device_release_driver and into driver_detach.
> 
> The idea behind this is that the async_synchronize_full call will only
> guarantee that any existing async operations are flushed. This doesn't do
> anything to guarantee that a hotplug event that may occur while we are
> doing the release of the driver will not be asynchronously scheduled.
> 
> By moving this into the driver_detach path we can avoid potential deadlocks
> as we aren't holding the device lock at this point and we should not have
> the driver we want to flush loaded so the flush will take care of any
> asynchronous events the driver we are detaching might have scheduled.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  drivers/base/dd.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 76c40fe69463..e74cefeb5b69 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -975,9 +975,6 @@ static void __device_release_driver(struct device *dev, struct device *parent)
>  
>  	drv = dev->driver;
>  	if (drv) {
> -		if (driver_allows_async_probing(drv))
> -			async_synchronize_full();
> -
>  		while (device_links_busy(dev)) {
>  			__device_driver_unlock(dev, parent);
>  
> @@ -1087,6 +1084,9 @@ void driver_detach(struct device_driver *drv)
>  	struct device_private *dev_prv;
>  	struct device *dev;
>  
> +	if (driver_allows_async_probing(drv))
> +		async_synchronize_full();
> +
>  	for (;;) {
>  		spin_lock(&drv->p->klist_devices.k_lock);
>  		if (list_empty(&drv->p->klist_devices.k_list)) {

Have you considered to move that async_synchronize_full() call into
bus_remove_driver()? Verifying the correctness of this patch requires to
check whether the async_synchronize_full() comes after the
klist_remove(&drv->p->knode_bus) call. That verification is easier when
the async_synchronize_full() call occurs in bus_remove_driver() instead
of in driver_detach().

Thanks,

Bart.

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 24544 bytes --]

Hi Alexander,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on driver-core/master]

url:    https://github.com/0day-ci/linux/commits/Alexander-Duyck/Add-NUMA-aware-async_schedule-calls/20181106-093800
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/net/mac80211.h:1001: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:1001: warning: Function parameter or member 'status.status_driver_data' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:1001: warning: Function parameter or member 'driver_rates' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:1001: warning: Function parameter or member 'pad' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:1001: warning: Function parameter or member 'rate_driver_data' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info'
   kernel/rcu/tree.c:685: warning: Excess function parameter 'irq' description in 'rcu_nmi_exit'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf'
   include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array'
   include/linux/gpio/driver.h:375: warning: Function parameter or member 'init_valid_mask' not described in 'gpio_chip'
   include/linux/iio/hw-consumer.h:1: warning: no structured comments found
>> include/linux/device.h:1056: warning: Function parameter or member 'async_probe' not described in 'device'
   include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry'
   drivers/mtd/nand/raw/nand_base.c:603: warning: Excess function parameter 'mtd' description in 'panic_nand_wait'
   drivers/mtd/nand/raw/nand_base.c:603: warning: Excess function parameter 'mtd' description in 'panic_nand_wait'
   include/linux/regulator/driver.h:227: warning: Function parameter or member 'resume' not described in 'regulator_ops'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb'
   drivers/slimbus/stream.c:1: warning: no structured comments found
   include/linux/spi/spi.h:177: warning: Function parameter or member 'driver_override' not described in 'spi_device'
   drivers/target/target_core_device.c:1: warning: no structured comments found
   drivers/usb/typec/bus.c:1: warning: no structured comments found
   drivers/usb/typec/class.c:1: warning: no structured comments found
   include/linux/w1.h:281: warning: Function parameter or member 'of_match_table' not described in 'w1_family'
   fs/direct-io.c:257: warning: Excess function parameter 'offset' description in 'dio_complete'
   fs/file_table.c:1: warning: no structured comments found
   fs/libfs.c:477: warning: Excess function parameter 'available' description in 'simple_write_end'
   fs/posix_acl.c:646: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:646: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:646: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:183: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:254: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_invalidate_range_start_gfx'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:302: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_invalidate_range_start_hsa'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:382: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:383: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'end' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:603: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:603: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:603: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:603: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:848: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:3096: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
   include/drm/drm_mode_config.h:869: warning: Function parameter or member 'quirk_addfb_prefer_xbgr_30bpp' not described in 'drm_mode_config'
   drivers/gpu/drm/drm_fourcc.c:112: warning: Function parameter or member 'dev' not described in 'drm_driver_legacy_fb_format'
   drivers/gpu/drm/drm_fourcc.c:112: warning: Excess function parameter 'native' description in 'drm_driver_legacy_fb_format'
   drivers/gpu/drm/i915/i915_vma.h:49: warning: cannot understand function prototype: 'struct i915_vma '
   drivers/gpu/drm/i915/i915_vma.h:1: warning: no structured comments found
   drivers/gpu/drm/i915/intel_guc_fwif.h:554: warning: cannot understand function prototype: 'struct guc_log_buffer_state '
   drivers/gpu/drm/i915/i915_trace.h:1: warning: no structured comments found
   include/linux/skbuff.h:862: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'list' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'offload_fwd_mark' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'offload_mr_fwd_mark' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
   include/net/sock.h:238: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.rmem_alloc' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.len' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.head' not described in 'sock'

vim +1056 include/linux/device.h

^1da177e Linus Torvalds 2005-04-16 @1056  

:::::: The code at line 1056 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6597 bytes --]

Re: [driver-core PATCH v5 4/9] driver core: Move async_synchronize_full call

[Alexander Duyck]

On Mon, 2018-11-05 at 17:04 -0800, Bart Van Assche wrote:
> On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> > This patch moves the async_synchronize_full call out of
> > __device_release_driver and into driver_detach.
> > 
> > The idea behind this is that the async_synchronize_full call will only
> > guarantee that any existing async operations are flushed. This doesn't do
> > anything to guarantee that a hotplug event that may occur while we are
> > doing the release of the driver will not be asynchronously scheduled.
> > 
> > By moving this into the driver_detach path we can avoid potential deadlocks
> > as we aren't holding the device lock at this point and we should not have
> > the driver we want to flush loaded so the flush will take care of any
> > asynchronous events the driver we are detaching might have scheduled.
> > 
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  drivers/base/dd.c |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 76c40fe69463..e74cefeb5b69 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -975,9 +975,6 @@ static void __device_release_driver(struct device *dev, struct device *parent)
> >  
> >  	drv = dev->driver;
> >  	if (drv) {
> > -		if (driver_allows_async_probing(drv))
> > -			async_synchronize_full();
> > -
> >  		while (device_links_busy(dev)) {
> >  			__device_driver_unlock(dev, parent);
> >  
> > @@ -1087,6 +1084,9 @@ void driver_detach(struct device_driver *drv)
> >  	struct device_private *dev_prv;
> >  	struct device *dev;
> >  
> > +	if (driver_allows_async_probing(drv))
> > +		async_synchronize_full();
> > +
> >  	for (;;) {
> >  		spin_lock(&drv->p->klist_devices.k_lock);
> >  		if (list_empty(&drv->p->klist_devices.k_list)) {
> 
> Have you considered to move that async_synchronize_full() call into
> bus_remove_driver()? Verifying the correctness of this patch requires to
> check whether the async_synchronize_full() comes after the
> klist_remove(&drv->p->knode_bus) call. That verification is easier when
> the async_synchronize_full() call occurs in bus_remove_driver() instead
> of in driver_detach().
> 
> Thanks,
> 
> Bart.

I considered it, however it ends up with things being more symmetric to
have use take care of synchronizing things in driver_detach since after
this patch set we are scheduling thing asynchronously in driver_attach.

Also I don't think it would be any great risk simply because calling
driver_detach with the driver still associated with the bus would be a
blatent error as it could easily lead to issues where you unbind a
driver but have it get hotplugged to a device while that is going on.

- Alex

Re: [driver-core PATCH v5 0/9] Add NUMA aware async_schedule calls

[Alexander Duyck]

On Mon, 2018-11-05 at 16:50 -0800, Bart Van Assche wrote:
> On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> > This patch set provides functionality that will help to improve the
> > locality of the async_schedule calls used to provide deferred
> > initialization.
> 
> Hi Alexander,
> 
> Is this patch series perhaps available in a public git tree? That would make
> it easier for me to test my sd changes on top of your patches than applying
> this patch series myself on kernel v4.20-rc1.
> 
> Thanks,
> 
> Bart.

Hi Bart,

Unfortunately I don't have a public git tree to host this on.

You may want to see if you can just put the patches together as a
bundle in patchwork and download them off of lore.kernel.org at:
https://lore.kernel.org/patchwork/project/lkml/list/?series=371783

Thanks.

-Alex

Re: [driver-core PATCH v5 1/9] workqueue: Provide queue_work_node to queue work near a given NUMA node

[Alexander Duyck]

On Mon, 2018-11-05 at 16:42 -0800, Bart Van Assche wrote:
> On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> > +/**
> > + * workqueue_select_cpu_near - Select a CPU based on NUMA node
> > + * @node: NUMA node ID that we want to bind a CPU from
> 
>                                           ^^^^
>                                           select?
> > +	/* If CPU is valid return that, otherwise just defer */
> > +	return (cpu < nr_cpu_ids) ? cpu : WORK_CPU_UNBOUND;
> 
> Please leave out the superfluous parentheses if this patch series would have to
> be reposted.
> 
> Anyway:
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

I will incorporate you suggestions and add your Reviewed-by for the
next submission.

Thanks.

- Alex

Re: [driver-core PATCH v5 4/9] driver core: Move async_synchronize_full call

[Bart Van Assche]

On Tue, 2018-11-06 at 08:18 -0800, Alexander Duyck wrote:
> On Mon, 2018-11-05 at 17:04 -0800, Bart Van Assche wrote:
> > On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> > > This patch moves the async_synchronize_full call out of
> > > __device_release_driver and into driver_detach.
> > > 
> > > The idea behind this is that the async_synchronize_full call will only
> > > guarantee that any existing async operations are flushed. This doesn't do
> > > anything to guarantee that a hotplug event that may occur while we are
> > > doing the release of the driver will not be asynchronously scheduled.
> > > 
> > > By moving this into the driver_detach path we can avoid potential deadlocks
> > > as we aren't holding the device lock at this point and we should not have
> > > the driver we want to flush loaded so the flush will take care of any
> > > asynchronous events the driver we are detaching might have scheduled.
> > > 
> > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > ---
> > >  drivers/base/dd.c |    6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > index 76c40fe69463..e74cefeb5b69 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -975,9 +975,6 @@ static void __device_release_driver(struct device *dev, struct device *parent)
> > >  
> > >  	drv = dev->driver;
> > >  	if (drv) {
> > > -		if (driver_allows_async_probing(drv))
> > > -			async_synchronize_full();
> > > -
> > >  		while (device_links_busy(dev)) {
> > >  			__device_driver_unlock(dev, parent);
> > >  
> > > @@ -1087,6 +1084,9 @@ void driver_detach(struct device_driver *drv)
> > >  	struct device_private *dev_prv;
> > >  	struct device *dev;
> > >  
> > > +	if (driver_allows_async_probing(drv))
> > > +		async_synchronize_full();
> > > +
> > >  	for (;;) {
> > >  		spin_lock(&drv->p->klist_devices.k_lock);
> > >  		if (list_empty(&drv->p->klist_devices.k_list)) {
> > 
> > Have you considered to move that async_synchronize_full() call into
> > bus_remove_driver()? Verifying the correctness of this patch requires to
> > check whether the async_synchronize_full() comes after the
> > klist_remove(&drv->p->knode_bus) call. That verification is easier when
> > the async_synchronize_full() call occurs in bus_remove_driver() instead
> > of in driver_detach().
> 
> I considered it, however it ends up with things being more symmetric to
> have use take care of synchronizing things in driver_detach since after
> this patch set we are scheduling thing asynchronously in driver_attach.
> 
> Also I don't think it would be any great risk simply because calling
> driver_detach with the driver still associated with the bus would be a
> blatent error as it could easily lead to issues where you unbind a
> driver but have it get hotplugged to a device while that is going on.

Thanks for the additional clarification. Since I'm fine with this patch:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Bart Van Assche]

On Mon, 2018-11-05 at 13:12 -0800, Alexander Duyck wrote:
> One change I made in addition is I replaced the use of "bool X:1" to define
> the bitfield to a "u8 X:1" setup in order to resolve some checkpatch
> warnings.

Please use "bool X:1" instead of "u8 X:1". I think it was a bad idea to make
checkpatch complain about "bool X:1" since "bool X:1" should only be avoided
in structures for which alignment must be architecture-independent. For struct
device it is fine if member alignment differs per architecture. Additionally,
changing "bool X:1" into "u8 X:1" will reduce performance on architectures that
cannot do byte addressing.

>  static void __device_release_driver(struct device *dev, struct device *parent)
>  {
> -	struct device_driver *drv;
> +	struct device_driver *drv = dev->driver;
>  
> -	drv = dev->driver;
> -	if (drv) {
> -		while (device_links_busy(dev)) {
> -			__device_driver_unlock(dev, parent);
> +	/*
> +	 * In the event that we are asked to release the driver on an
> +	 * interface that is still waiting on a probe we can just terminate
> +	 * the probe by setting async_probe to false. When the async call
> +	 * is finally completed it will see this state and just exit.
> +	 */
> +	dev->async_probe = false;
> +	if (!drv)
> +		return;
>  
> -			device_links_unbind_consumers(dev);
> +	while (device_links_busy(dev)) {
> +		__device_driver_unlock(dev, parent);
>  
> -			__device_driver_lock(dev, parent);
> -			/*
> -			 * 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;
> -		}
> +		device_links_unbind_consumers(dev);
>  
> -		pm_runtime_get_sync(dev);
> -		pm_runtime_clean_up_links(dev);
> +		__device_driver_lock(dev, parent);
> +		/*
> +		 * 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;
> +	}
>  
> -		driver_sysfs_remove(dev);
> +	pm_runtime_get_sync(dev);
> +	pm_runtime_clean_up_links(dev);
>  
> -		if (dev->bus)
> -			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> -						     BUS_NOTIFY_UNBIND_DRIVER,
> -						     dev);
> +	driver_sysfs_remove(dev);
>  
> -		pm_runtime_put_sync(dev);
> +	if (dev->bus)
> +		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> +					     BUS_NOTIFY_UNBIND_DRIVER,
> +					     dev);
>  
> -		if (dev->bus && dev->bus->remove)
> -			dev->bus->remove(dev);
> -		else if (drv->remove)
> -			drv->remove(dev);
> +	pm_runtime_put_sync(dev);
>  
> -		device_links_driver_cleanup(dev);
> -		arch_teardown_dma_ops(dev);
> +	if (dev->bus && dev->bus->remove)
> +		dev->bus->remove(dev);
> +	else if (drv->remove)
> +		drv->remove(dev);
>  
> -		devres_release_all(dev);
> -		dev->driver = NULL;
> -		dev_set_drvdata(dev, NULL);
> -		if (dev->pm_domain && dev->pm_domain->dismiss)
> -			dev->pm_domain->dismiss(dev);
> -		pm_runtime_reinit(dev);
> -		dev_pm_set_driver_flags(dev, 0);
> +	device_links_driver_cleanup(dev);
> +	arch_teardown_dma_ops(dev);
> +
> +	devres_release_all(dev);
> +	dev->driver = NULL;
> +	dev_set_drvdata(dev, NULL);
> +	if (dev->pm_domain && dev->pm_domain->dismiss)
> +		dev->pm_domain->dismiss(dev);
> +	pm_runtime_reinit(dev);
> +	dev_pm_set_driver_flags(dev, 0);
>  
> -		klist_remove(&dev->p->knode_driver);
> -		device_pm_check_callbacks(dev);
> -		if (dev->bus)
> -			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> -						     BUS_NOTIFY_UNBOUND_DRIVER,
> -						     dev);
> +	klist_remove(&dev->p->knode_driver);
> +	device_pm_check_callbacks(dev);
> +	if (dev->bus)
> +		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> +					     BUS_NOTIFY_UNBOUND_DRIVER,
> +					     dev);
>  
> -		kobject_uevent(&dev->kobj, KOBJ_UNBIND);
> -	}
> +	kobject_uevent(&dev->kobj, KOBJ_UNBIND);
>  }

This patch mixes functional changes with whitespace changes. Please move the
whitespace changes into a separate patch such that this patch becomes easier
to read.
 
>  void device_release_driver_internal(struct device *dev,
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1b25c7a43f4c..fc7091d436c2 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1043,14 +1043,15 @@ struct device {
>  	struct iommu_group	*iommu_group;
>  	struct iommu_fwspec	*iommu_fwspec;
>  
> -	bool			offline_disabled:1;
> -	bool			offline:1;
> -	bool			of_node_reused:1;
> +	u8			offline_disabled:1;
> +	u8			offline:1;
> +	u8			of_node_reused:1;
>  #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>      defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>      defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> -	bool			dma_coherent:1;
> +	u8			dma_coherent:1;
>  #endif
> +	u8			async_probe:1;

The new async_probe field can be changed from multiple threads. Concurrent
changes of a bitfield are only safe if these are serialized in some way.
Please document the locking requirements for the async_probe bitfield in
device.h.

Thanks,

Bart.

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Bart Van Assche]

On Tue, 2018-11-06 at 12:10 +0800, kbuild test robot wrote:
> Hi Alexander,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on driver-core/master]
> 
> url:    https://github.com/0day-ci/linux/commits/Alexander-Duyck/Add-NUMA-aware-async_schedule-calls/20181106-093800
> reproduce: make htmldocs
> 
> All warnings (new ones prefixed by >>):
> 
>    include/net/mac80211.h:1001: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info'
> [ ... ]

There are plenty of references in this report to header files not touched
by patch 5/9 in this series. I assume that this report indicates a bug in
the 0-day testing infrastructure?

Bart.

Re: [driver-core PATCH v5 6/9] driver core: Probe devices asynchronously instead of the driver

[Bart Van Assche]

On Mon, 2018-11-05 at 13:12 -0800, Alexander Duyck wrote:
> diff --git a/include/linux/device.h b/include/linux/device.h
> index fc7091d436c2..4d9a39769081 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> [ ... ]
> +static inline struct device_driver *dev_get_drv_async(const struct device *dev)
> +{
> +	return dev->async_probe ? dev->driver_data : NULL;
> +}
> +
> +static inline void dev_set_drv_async(struct device *dev,
> +				     struct device_driver *drv)
> +{
> +	/*
> +	 * Set async_probe to true indicating we are waiting for this data to be
> +	 * loaded as a potential driver.
> +	 */
> +	dev->driver_data = drv;
> +	dev->async_probe = true;
> +}

Since these two new functions are only used in the driver core please move
their definition into the driver core.

Thanks,

Bart.

Re: [driver-core PATCH v5 7/9] driver core: Attach devices on CPU local to device node

[Bart Van Assche]

On Mon, 2018-11-05 at 13:12 -0800, Alexander Duyck wrote:
> This change makes it so that we call the asynchronous probe routines on a
> CPU local to the device node. By doing this we should be able to improve
> our initialization time significantly as we can avoid having to access the
> device from a remote node which may introduce higher latency.
> 
> For example, in the case of initializing memory for NVDIMM this can have a
> significant impact as initialing 3TB on remote node can take up to 39
> seconds while initialing it on a local node only takes 23 seconds. It is
> situations like this where we will see the biggest improvement.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  drivers/base/dd.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 2fdfe45bb6ea..cf7681309ee3 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -834,7 +834,7 @@ static int __device_attach(struct device *dev, bool allow_async)
>  			dev_dbg(dev, "scheduling asynchronous probe\n");
>  			get_device(dev);
>  			dev->async_probe = true;
> -			async_schedule(__device_attach_async_helper, dev);
> +			async_schedule_dev(__device_attach_async_helper, dev);
>  		} else {
>  			pm_request_idle(dev);
>  		}
> @@ -992,7 +992,7 @@ static int __driver_attach(struct device *dev, void *data)
>  		if (!dev->driver) {
>  			get_device(dev);
>  			dev_set_drv_async(dev, drv);
> -			async_schedule(__driver_attach_async_helper, dev);
> +			async_schedule_dev(__driver_attach_async_helper, dev);
>  		}
>  		device_unlock(dev);
>  		return 0;

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [driver-core PATCH v5 8/9] PM core: Use new async_schedule_dev command

[Bart Van Assche]

On Mon, 2018-11-05 at 13:12 -0800, Alexander Duyck wrote:
> This change makes it so that we use the device specific version of the
> async_schedule commands to defer various tasks related to power management.
> By doing this we should see a slight improvement in performance as any
> device that is sensitive to latency/locality in the setup will now be
> initializing on the node closest to the device.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [driver-core PATCH v5 9/9] libnvdimm: Schedule device registration on node local to the device

[Bart Van Assche]

On Mon, 2018-11-05 at 13:12 -0800, Alexander Duyck wrote:
> This patch is meant to force the device registration for nvdimm devices to
> be closer to the actual device. This is achieved by using either the NUMA
> node ID of the region, or of the parent. By doing this we can have
> everything above the region based on the region, and everything below the
> region based on the nvdimm bus.
> 
> By guaranteeing NUMA locality I see an improvement of as high as 25% for
> per-node init of a system with 12TB of persistent memory.

Thank you for having included the performance numbers.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [driver-core PATCH v5 2/9] async: Add support for queueing on specific NUMA node

[Bart Van Assche]

On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> +/**
> + * async_schedule_domain - schedule a function for asynchronous execution within a certain domain
> + * @func: function to execute asynchronously
> + * @data: data pointer to pass to the function
> + * @domain: the domain
> + *
> + * Returns an async_cookie_t that may be used for checkpointing later.
> + * @domain may be used in the async_synchronize_*_domain() functions to
> + * wait within a certain synchronization domain rather than globally.  A
> + * synchronization domain is specified via @domain.
> + * Note: This function may be called from atomic or non-atomic contexts.
> + */

Please leave out "A synchronization domain is specified via @domain." since
that text is redundant due to "@domain: the domain".

> +/**
> + * async_schedule_dev_domain - A device specific version of async_schedule_domain
> + * @func: function to execute asynchronously
> + * @dev: device argument to be passed to function
> + * @domain: the domain
> + *
> + * Returns an async_cookie_t that may be used for checkpointing later.
> + * @dev is used as both the argument for the function and to provide NUMA
> + * context for where to run the function. By doing this we can try to
> + * provide for the best possible outcome by operating on the device on the
> + * CPUs closest to the device.
> + * @domain may be used in the async_synchronize_*_domain() functions to
> + * wait within a certain synchronization domain rather than globally.  A
> + * synchronization domain is specified via @domain.
> + * Note: This function may be called from atomic or non-atomic contexts.
> + */

Same comment here: I think "A synchronization domain is specified via @domain."
is redundant.

> +/**
> + * async_schedule_node_domain - NUMA specific version of async_schedule_domain
> + * @func: function to execute asynchronously
> + * @data: data pointer to pass to the function
> + * @node: NUMA node that we want to schedule this on or close to
> + * @domain: the domain
> + *
> + * Returns an async_cookie_t that may be used for checkpointing later.
> + * @domain may be used in the async_synchronize_*_domain() functions to
> + * wait within a certain synchronization domain rather than globally.  A
> + * synchronization domain is specified via @domain.  Note: This function
> + * may be called from atomic or non-atomic contexts.
> + *
> + * The node requested will be honored on a best effort basis. If the node
> + * has no CPUs associated with it then the work is distributed among all
> + * available CPUs.
> + */

Same comment here: I think that also in the above "A synchronization domain is
specified via @domain." is redundant.

Otherwise this patch looks fine to me.

Bart.

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Alexander Duyck]

On Tue, Nov 6, 2018 at 3:51 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On Tue, 2018-11-06 at 12:10 +0800, kbuild test robot wrote:
> > Hi Alexander,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on driver-core/master]
> >
> > url:    https://github.com/0day-ci/linux/commits/Alexander-Duyck/Add-NUMA-aware-async_schedule-calls/20181106-093800
> > reproduce: make htmldocs
> >
> > All warnings (new ones prefixed by >>):
> >
> >    include/net/mac80211.h:1001: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info'
> > [ ... ]
>
> There are plenty of references in this report to header files not touched
> by patch 5/9 in this series. I assume that this report indicates a bug in
> the 0-day testing infrastructure?
>
> Bart.

It isn't a bug. It is pointing out the same thing you did. I didn't
document the async_probe value so it added a warning to the list.

>> include/linux/device.h:1056: warning: Function parameter or member 'async_probe' not described in 'device'

I'll fix it for the next version.

- Alex

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Joe Perches]

On Tue, 2018-11-06 at 15:48 -0800, Bart Van Assche wrote:
> On Mon, 2018-11-05 at 13:12 -0800, Alexander Duyck wrote:
> > One change I made in addition is I replaced the use of "bool X:1" to define
> > the bitfield to a "u8 X:1" setup in order to resolve some checkpatch
> > warnings.
> 
> Please use "bool X:1" instead of "u8 X:1". I think it was a bad idea to make
> checkpatch complain about "bool X:1" since "bool X:1" should only be avoided
> in structures for which alignment must be architecture-independent. For struct
> device it is fine if member alignment differs per architecture. Additionally,
> changing "bool X:1" into "u8 X:1" will reduce performance on architectures that
> cannot do byte addressing.

I generally agree.  But the checkpatch warning _could_
be useful in those cases where alignment should be
architecture-independent.

Any suggestion on how to improve the message?
s

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Bart Van Assche]

On Tue, 2018-11-06 at 17:34 -0800, Joe Perches wrote:
> On Tue, 2018-11-06 at 15:48 -0800, Bart Van Assche wrote:
> > On Mon, 2018-11-05 at 13:12 -0800, Alexander Duyck wrote:
> > > One change I made in addition is I replaced the use of "bool X:1" to define
> > > the bitfield to a "u8 X:1" setup in order to resolve some checkpatch
> > > warnings.
> > 
> > Please use "bool X:1" instead of "u8 X:1". I think it was a bad idea to make
> > checkpatch complain about "bool X:1" since "bool X:1" should only be avoided
> > in structures for which alignment must be architecture-independent. For struct
> > device it is fine if member alignment differs per architecture. Additionally,
> > changing "bool X:1" into "u8 X:1" will reduce performance on architectures that
> > cannot do byte addressing.
> 
> I generally agree.  But the checkpatch warning _could_
> be useful in those cases where alignment should be
> architecture-independent.
> 
> Any suggestion on how to improve the message?

It would be great if a heuristic could be developed that recognizes structs
for which the data layout must be architecture independent. If such a
heuristic could be developed it could be used to only display warn about
"bool X:n" for such structures.

Bart.

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

Hi!

> > One change I made in addition is I replaced the use of "bool X:1" to define
> > the bitfield to a "u8 X:1" setup in order to resolve some checkpatch
> > warnings.
> 
> Please use "bool X:1" instead of "u8 X:1". I think it was a bad idea to make
> checkpatch complain about "bool X:1" since "bool X:1" should only be avoided
> in structures for which alignment must be architecture-independent. For struct
> device it is fine if member alignment differs per architecture. Additionally,
> changing "bool X:1" into "u8 X:1" will reduce performance on architectures that
> cannot do byte addressing.

Should we introduce typedef bit boolean "bbool" and set it
appropriately, so that architecture sets it "right" and people just
use it?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Rong Chen]

On 11/07/2018 07:51 AM, Bart Van Assche wrote:
> On Tue, 2018-11-06 at 12:10 +0800, kbuild test robot wrote:
>> Hi Alexander,
>>
>> Thank you for the patch! Perhaps something to improve:
>>
>> [auto build test WARNING on driver-core/master]
>>
>> url:    https://github.com/0day-ci/linux/commits/Alexander-Duyck/Add-NUMA-aware-async_schedule-calls/20181106-093800
>> reproduce: make htmldocs
>>
>> All warnings (new ones prefixed by >>):
>>
>>     include/net/mac80211.h:1001: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info'
>> [ ... ]
> There are plenty of references in this report to header files not touched
> by patch 5/9 in this series. I assume that this report indicates a bug in
> the 0-day testing infrastructure?
>
> Bart.
>
Hi Bart,

Kbuild robot printed all warnings and the below warning was found in 
patch 5/9.

"include/linux/device.h:1056: warning: Function parameter or member 
'async_probe' not described in 'device'"

Best Regards,
Rong Chen

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Bart Van Assche]

On 11/22/18 5:23 PM, Rong Chen wrote:
> Kbuild robot printed all warnings and the below warning was found in 
> patch 5/9.
> 
> "include/linux/device.h:1056: warning: Function parameter or member 
> 'async_probe' not described in 'device'"

Hi Rong,

Thanks for having taken a look. That's indeed something that must be 
fixed in patch 5/9.

Bart.

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Dan Williams]

On Mon, Nov 5, 2018 at 1:12 PM Alexander Duyck
<alexander.h.duyck@linux.intel.com> wrote:
>
> This patch adds an additional bit to the device struct named async_probe.
> This additional bit allows us to guarantee ordering between probe and
> remove operations.
>
> This allows us to guarantee that if we execute a remove operation or a
> driver load attempt on a given interface it will not attempt to update the
> driver member asynchronously following the earlier operation. Previously
> this guarantee was not present and could result in us attempting to remove
> a driver from an interface only to have it show up later when it is
> asynchronously loaded.
>
> One change I made in addition is I replaced the use of "bool X:1" to define
> the bitfield to a "u8 X:1" setup in order to resolve some checkpatch
> warnings.

The usage of "us" in the changelog through me off, please reword this
to explicitly state the subject like: "The additional bit allows the
driver core to guarantee ordering between probe and remove
operations."

> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  drivers/base/dd.c      |  104 +++++++++++++++++++++++++++---------------------
>  include/linux/device.h |    9 ++--
>  2 files changed, 64 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index e74cefeb5b69..ed19cf0d6f9a 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -472,6 +472,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>                  drv->bus->name, __func__, drv->name, dev_name(dev));
>         WARN_ON(!list_empty(&dev->devres_head));
>
> +       /* clear async_probe flag as we are no longer deferring driver load */
> +       dev->async_probe = false;
>  re_probe:
>         dev->driver = drv;
>
> @@ -771,6 +773,10 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
>
>         device_lock(dev);
>
> +       /* nothing to do if async_probe has been cleared */
> +       if (!dev->async_probe)
> +               goto out_unlock;
> +
>         if (dev->parent)
>                 pm_runtime_get_sync(dev->parent);
>
> @@ -781,7 +787,7 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
>
>         if (dev->parent)
>                 pm_runtime_put(dev->parent);
> -
> +out_unlock:
>         device_unlock(dev);
>
>         put_device(dev);
> @@ -826,6 +832,7 @@ static int __device_attach(struct device *dev, bool allow_async)
>                          */
>                         dev_dbg(dev, "scheduling asynchronous probe\n");
>                         get_device(dev);
> +                       dev->async_probe = true;
>                         async_schedule(__device_attach_async_helper, dev);
>                 } else {
>                         pm_request_idle(dev);
> @@ -971,62 +978,69 @@ EXPORT_SYMBOL_GPL(driver_attach);
>   */
>  static void __device_release_driver(struct device *dev, struct device *parent)
>  {
> -       struct device_driver *drv;
> +       struct device_driver *drv = dev->driver;
>
> -       drv = dev->driver;
> -       if (drv) {
> -               while (device_links_busy(dev)) {
> -                       __device_driver_unlock(dev, parent);
> +       /*
> +        * In the event that we are asked to release the driver on an
> +        * interface that is still waiting on a probe we can just terminate
> +        * the probe by setting async_probe to false. When the async call
> +        * is finally completed it will see this state and just exit.
> +        */
> +       dev->async_probe = false;
> +       if (!drv)
> +               return;

Patch 4 deleted the async_synchronize_full() that would have flushed
in-flight ->probe() relative to the current ->remove(). If remove runs
before probe then would seem to be deadlock condition, but if
->remove() runs before probe then dev->driver is NULL and we abort. So
I'm struggling to see what value dev->async_probe provides over
dev->driver?

Re: [driver-core PATCH v5 5/9] driver core: Establish clear order of operations for deferred probe and remove

[Alexander Duyck]

On Mon, 2018-11-26 at 18:35 -0800, Dan Williams wrote:
> On Mon, Nov 5, 2018 at 1:12 PM Alexander Duyck
> <alexander.h.duyck@linux.intel.com> wrote:
> > 
> > This patch adds an additional bit to the device struct named async_probe.
> > This additional bit allows us to guarantee ordering between probe and
> > remove operations.
> > 
> > This allows us to guarantee that if we execute a remove operation or a
> > driver load attempt on a given interface it will not attempt to update the
> > driver member asynchronously following the earlier operation. Previously
> > this guarantee was not present and could result in us attempting to remove
> > a driver from an interface only to have it show up later when it is
> > asynchronously loaded.
> > 
> > One change I made in addition is I replaced the use of "bool X:1" to define
> > the bitfield to a "u8 X:1" setup in order to resolve some checkpatch
> > warnings.
> 
> The usage of "us" in the changelog through me off, please reword this
> to explicitly state the subject like: "The additional bit allows the
> driver core to guarantee ordering between probe and remove
> operations."

Okay, I will work on the wording here.

> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  drivers/base/dd.c      |  104 +++++++++++++++++++++++++++---------------------
> >  include/linux/device.h |    9 ++--
> >  2 files changed, 64 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index e74cefeb5b69..ed19cf0d6f9a 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -472,6 +472,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> >                  drv->bus->name, __func__, drv->name, dev_name(dev));
> >         WARN_ON(!list_empty(&dev->devres_head));
> > 
> > +       /* clear async_probe flag as we are no longer deferring driver load */
> > +       dev->async_probe = false;
> >  re_probe:
> >         dev->driver = drv;
> > 
> > @@ -771,6 +773,10 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
> > 
> >         device_lock(dev);
> > 
> > +       /* nothing to do if async_probe has been cleared */
> > +       if (!dev->async_probe)
> > +               goto out_unlock;
> > +
> >         if (dev->parent)
> >                 pm_runtime_get_sync(dev->parent);
> > 
> > @@ -781,7 +787,7 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
> > 
> >         if (dev->parent)
> >                 pm_runtime_put(dev->parent);
> > -
> > +out_unlock:
> >         device_unlock(dev);
> > 
> >         put_device(dev);
> > @@ -826,6 +832,7 @@ static int __device_attach(struct device *dev, bool allow_async)
> >                          */
> >                         dev_dbg(dev, "scheduling asynchronous probe\n");
> >                         get_device(dev);
> > +                       dev->async_probe = true;
> >                         async_schedule(__device_attach_async_helper, dev);
> >                 } else {
> >                         pm_request_idle(dev);
> > @@ -971,62 +978,69 @@ EXPORT_SYMBOL_GPL(driver_attach);
> >   */
> >  static void __device_release_driver(struct device *dev, struct device *parent)
> >  {
> > -       struct device_driver *drv;
> > +       struct device_driver *drv = dev->driver;
> > 
> > -       drv = dev->driver;
> > -       if (drv) {
> > -               while (device_links_busy(dev)) {
> > -                       __device_driver_unlock(dev, parent);
> > +       /*
> > +        * In the event that we are asked to release the driver on an
> > +        * interface that is still waiting on a probe we can just terminate
> > +        * the probe by setting async_probe to false. When the async call
> > +        * is finally completed it will see this state and just exit.
> > +        */
> > +       dev->async_probe = false;
> > +       if (!drv)
> > +               return;
> 
> Patch 4 deleted the async_synchronize_full() that would have flushed
> in-flight ->probe() relative to the current ->remove(). If remove runs
> before probe then would seem to be deadlock condition, but if
> ->remove() runs before probe then dev->driver is NULL and we abort. So
> I'm struggling to see what value dev->async_probe provides over
> dev->driver?

So the issue addressed by patch 4 is a deadlock on the device driver
lock. Also it didn't make much sense to flush per device when we really
only needed to flush things once and then clean up the devices.

What I am doing with the async_probe value is providing a way to
gracefully shutdown any deferred probe calls that may be outstanding
without having to resort to an explicit flush. The problem with the
flush is that you have to release the device lock and as soon as you
have done that the potential for something else to occur gets opended
up. The general issue I have with just trying to use dev->driver is
that it assumes the probe has already completed. That may not always be
the case. My main concern would be with a device that is popping in and
out of existence quickly so that something like a remove call is being
made before the driver has been loaded. I would prefer all possible
cases where probe and then remove is called in quick succession to
result in no driver being loaded instead of a driver loading on a
device that maybe shouldn't be there.

As far as the remove before probe case that should have no effect with
this code since it would just transition async_probe from false to
false so the extra write would have no effect.