[RFC v2 2/6] driver-core: add driver async_probe support

From: "Luis R. Rodriguez" <redacted>

We now have two documented use cases for probing asynchronously:

0) since we bundle together driver init() and probe() systemd's
   new 30 second timeout has put a limit on the amount of time
   a driver probe routine can take, we need to enable drivers
   to complete probe gracefully

1) when a built-in driver takes a few seconds to initialize its
   delays can stall the overall boot process

The built-in driver issues is pretty straight forward, and for
that we just need to let the probing happen behind the scenes.

The systemd issue is a bit more complex given the history of
how it was identified and work arounds proposed and evaluated.
The systemd issue was first identified first by Joseph when he
bisected and found that Tetsuo Handa's commit 786235ee
"kthread: make kthread_create() killable" modified kthread_create()
to bail as soon as SIGKILL is received [0] [1]. This was found
to cause some issues with some drivers and at times boot. There
are other patches which could also enable the SIGKILL trigger on
driver loading though:

70834d30 "usermodehelper: use UMH_WAIT_PROC consistently"
b3449922 "usermodehelper: introduce umh_complete(sub_info)"
d0bd587a "usermodehelper: implement UMH_KILLABLE"
9d944ef3 "usermodehelper: kill umh_wait, renumber UMH_* constants"
5b9bd473 "usermodehelper: ____call_usermodehelper() doesn't need do_exit()"
3e63a93b "kmod: introduce call_modprobe() helper"
1cc684ab "kmod: make __request_module() killable"

All of these went in on 3.4 upstream, and were part of the fixes
for CVE-2012-4398 [2] and documented more properly on Red Hat's
bugzilla [3]. Any of these patches may contribute to having a
module be properly killed now, but 786235ee is the latest in the
series. For instance on SLE12 cxgb4 has been fond to get the
SIGKILL even though SLE12 does not yet have 786235ee merged [4].

Joseph found that the systemd-udevd process sends SIGKILL to
systemd's usage of kmod for module loading if probe on a driver
takes over 30 seconds [5] [6]. When this happens probe will fail
on any driver, but *iff* its probe path ends up using kthreads.
Its why booting on some systems will fail if the driver happens
to be a storage related driver.  When helping debug the issue
Tetsuo suggested fixing this issue by kmodifying kthread_create()
to not leave upon SIGKILL immediately *unless* the source of the
SIGKILL was the OOM, and actually wait for 10 seconds more before
completing the kill [7] *unless* the source of the killer was OOM.
This is not the only source of a kill, as noted above, this same
issue is present on kernels without commit 786235ee. Additionally
upon review of this patch Oleg rejected this change [8] and the
discussion was punted out to systemd-devel to see if the default
timeout could be increased from 30 seconds to 120 [9]. The opinion
of the systemd maintainers was that the driver's behavior should
be fixed [10].  Linus seems to agree [11], however more recently
even networking drivers have been reported to fail on probe
since just writing the firmware to a device and kicking it can
take easy over 60 seconds [4]. Benjamim was able to trace the
issues reported on cxgb4 down to the same systemd-udevd 30
second timeout [6].  Even if asynch firmware loading was used
on the cxgb4 driver the driver would *still* hit other delays
due to the way the driver is currently designed, fixing that
will require a bit of time and until then some users are left
with a completely dysfunctional device.

Folks were a bit confused here though -- its not only module
initialization which is being penalized, because the driver core
will immediately trigger the driver's own bus probe routine if
autoprobe is enabled each driver's probe routine must also
complete within the same 30 second timeout.  This means not only
should driver's init complete within the set default systemd
timeout of 30 seconds but so should the probe routine, and
probe would obviously also have less time given that the
timeout is for both the module's init() and its own bus' probe().
A few drivers fail to complete the bus' probe within 30 seconds,
its not the init routine that takes long. The timeout seems
to currently hit *iff* kthreads are used somehow on the driver's
probe path. For example purposely breaking the e1000e driver
by adding a 30 second timeout on the probe path does not let
systemd kill it, however doing the same for iwlwifi triggers
the kill, this is because this driver uses request_threaded_irq()
and behind the scenes the kernel uses ktread_create() on
__setup_irq() to handle the thread *iff* its not nested, these
are drivers that set irq_set_nested_thread(irq, 1).

Hannes Reinecke has implemented now a timeout modifier for
systemd, however *systemd* still needs a way to gracefully
annotate drivers with long probes instead of failing these
drivers and at worst boot. On the kernel side of things we
can circumvent the timeout by probing asynchronously on only
drivers that need it. If a driver is changed to use this new
asynch probing, folks should be aware that any userspace
that assumed that completing driver loading would enable
device functionality will need to changed until the device
appears.

[0]  http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
[1]  https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
[2]  http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2012-4398
[3]  https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2012-4398
[4]  https://bugzilla.novell.com/show_bug.cgi?id=877622
[5]  http://article.gmane.org/gmane.linux.kernel/1669550
[6]  https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
[7]  https://launchpadlibrarian.net/169657493/kthread-defer-leaving.patch
[8]  http://article.gmane.org/gmane.linux.kernel/1669604
[9]  http://lists.freedesktop.org/archives/systemd-devel/2014-March/018006.html
[10] http://article.gmane.org/gmane.comp.sysutils.systemd.devel/17860
[11] http://article.gmane.org/gmane.linux.kernel/1671333

Cc: Tejun Heo <tj@kernel.org>
Cc: Arjan van de Ven <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <redacted>
Cc: Kay Sievers <redacted>
Cc: One Thousand Gnomes <redacted>
Cc: Tim Gardner <redacted>
Cc: Pierre Fersing <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <redacted>
Cc: Nagalakshmi Nandigama <redacted>
Cc: Praveen Krishnamoorthy <redacted>
Cc: Sreekanth Reddy <redacted>
Cc: Abhijit Mahajan <redacted>
Cc: Casey Leedom <redacted>
Cc: Hariprasad S <redacted>
Cc: Santosh Rastapur <redacted>
Cc: MPT-FusionLinux.pdl@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
 drivers/base/base.h    |  6 +++++
 drivers/base/bus.c     | 59 +++++++++++++++++++++++++++++++++++++++++++++++---
 drivers/base/dd.c      |  4 ++++
 include/linux/device.h |  5 +++++
 4 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 251c5d3..24836f1 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h

@@ -43,11 +43,17 @@ struct subsys_private {
 };
 #define to_subsys_private(obj) container_of(obj, struct subsys_private, subsys.kobj)
 
+struct driver_attach_work {
+	struct work_struct work;
+	struct device_driver *driver;
+};
+
 struct driver_private {
 	struct kobject kobj;
 	struct klist klist_devices;
 	struct klist_node knode_bus;
 	struct module_kobject *mkobj;
+	struct driver_attach_work *attach_work;
 	struct device_driver *driver;
 };
 #define to_driver(obj) container_of(obj, struct driver_private, kobj)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index a5f41e4..70d51b2 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c

@@ -85,6 +85,7 @@ static void driver_release(struct kobject *kobj)
 	struct driver_private *drv_priv = to_driver(kobj);
 
 	pr_debug("driver: '%s': %s\n", kobject_name(kobj), __func__);
+	kfree(drv_priv->attach_work);
 	kfree(drv_priv);
 }
 

@@ -662,10 +663,56 @@ static void remove_driver_private(struct device_driver *drv)
 	struct driver_private *priv = drv->p;
 
 	kobject_put(&priv->kobj);
+	kfree(priv->attach_work);
 	kfree(priv);
 	drv->p = NULL;
 }
 
+static void driver_attach_workfn(struct work_struct *work)
+{
+	int ret;
+	struct driver_attach_work *attach_work =
+		container_of(work, struct driver_attach_work, work);
+	struct device_driver *drv = attach_work->driver;
+	ktime_t calltime, delta, rettime;
+	unsigned long long duration;
+
+	calltime = ktime_get();
+
+	ret = driver_attach(drv);
+	if (ret != 0) {
+		remove_driver_private(drv);
+		bus_put(drv->bus);
+	}
+
+	rettime = ktime_get();
+	delta = ktime_sub(rettime, calltime);
+	duration = (unsigned long long) ktime_to_ns(delta) >> 10;
+
+	pr_debug("bus: '%s': add driver %s attach completed after %lld usecs\n",
+		 drv->bus->name, drv->name, duration);
+}
+
+int bus_driver_async_probe(struct device_driver *drv)
+{
+	struct driver_private *priv = drv->p;
+
+	priv->attach_work = kzalloc(sizeof(struct driver_attach_work),
+				    GFP_KERNEL);
+	if (!priv->attach_work)
+		return -ENOMEM;
+
+	priv->attach_work->driver = drv;
+	INIT_WORK(&priv->attach_work->work, driver_attach_workfn);
+
+	pr_debug("bus: '%s': probe for driver %s is run asynchronously\n",
+		 drv->bus->name, drv->name);
+
+	queue_work(system_unbound_wq, &priv->attach_work->work);
+
+	return 0;
+}
+
 /**
  * bus_add_driver - Add a driver to the bus.
  * @drv: driver.

@@ -698,9 +745,15 @@ int bus_add_driver(struct device_driver *drv)
 
 	klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
 	if (drv->bus->p->drivers_autoprobe) {
-		error = driver_attach(drv);
-		if (error)
-			goto out_unregister;
+		if (drv->owner && drv->async_probe) {
+			error = bus_driver_async_probe(drv);
+			if (error)
+				goto out_unregister;
+		} else {
+			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 e4ffbcf..f1565f3 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c

@@ -507,6 +507,10 @@ static void __device_release_driver(struct device *dev)
 
 	drv = dev->driver;
 	if (drv) {
+		if (drv->owner && drv->async_probe) {
+			struct driver_private *priv = drv->p;
+			flush_work(&priv->attach_work->work);
+		}
 		pm_runtime_get_sync(dev);
 
 		driver_sysfs_remove(dev);

diff --git a/include/linux/device.h b/include/linux/device.h
index 43d183a..7de1386b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h

@@ -200,6 +200,10 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus);
  * @owner:	The module owner.
  * @mod_name:	Used for built-in modules.
  * @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @async_probe: requests probe to be run asynchronously. Drivers that
+ * 	have this enabled must take care that userspace will return
+ * 	immediately upon driver loading as probing will happen behind the
+ * 	schenes asynchronously.
  * @of_match_table: The open firmware table.
  * @acpi_match_table: The ACPI match table.
  * @probe:	Called to query the existence of a specific device,

@@ -233,6 +237,7 @@ struct device_driver {
 	const char		*mod_name;	/* used for built-in modules */
 
 	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
+	bool async_probe;
 
 	const struct of_device_id	*of_match_table;
 	const struct acpi_device_id	*acpi_match_table;

-- 
2.0.3

On 09/04, Luis R. Rodriguez wrote:

 struct driver_private {
 	struct kobject kobj;
 	struct klist klist_devices;
 	struct klist_node knode_bus;
 	struct module_kobject *mkobj;
+	struct driver_attach_work *attach_work;
 	struct device_driver *driver;

I am not arguing, just curious...

Are you trying to shrink sizeof(driver_private) ? The code can be simpler
if you just embedd "struct work_struct attach_work" into driver_private,
and you do not need "struct driver_attach_work" or another ->driver pointer
this way.

Oleg.

On Fri, Sep 05, 2014 at 01:24:17PM +0200, Oleg Nesterov wrote:

On 09/04, Luis R. Rodriguez wrote:

quoted

 struct driver_private {
 	struct kobject kobj;
 	struct klist klist_devices;
 	struct klist_node knode_bus;
 	struct module_kobject *mkobj;
+	struct driver_attach_work *attach_work;
 	struct device_driver *driver;

I am not arguing, just curious...

Are you trying to shrink sizeof(driver_private) ? 

Yeap.

The code can be simpler
if you just embedd "struct work_struct attach_work" into driver_private,
and you do not need "struct driver_attach_work" or another ->driver pointer
this way.

Agreed, I considered it and figured it wouldn't make much sense
to push onto folks more bytes if this feature was optional and
likely only used by a few drivers, so a pointer / kzalloc seemed
better to deal with. This saves us 24 bytes. I even tried to
implement a container_of_p() for pointers but that obviosly
didn't work well fast as a pointer can have any address and is
not relative to the parent, and if its on stack the address
can vary depending on implementation. For example the first member
should always have the same address as the struct but if the
first member is a pointer it would be off for me by 12 bytes.
I am not sure if this is standarized or not.

  Luis

Hi Luis,

On Thu, Sep 04, 2014 at 11:37:23PM -0700, Luis R. Rodriguez wrote:

1) when a built-in driver takes a few seconds to initialize its
   delays can stall the overall boot process

This patch does not solve the 2nd issue fully as it only calls probe
asynchronously during driver registration (and also only for modules???
- it checks drv->owner in a few places). The device may get created
  after driver is initialized, in this case we still want probe to be
called asynchronously.

I think something like the patch below should work. Note that it uses
async_checdule(), so that will satisy for the moment Tejun's objections
to the behavior with regard to module loading and initialization, but it
does not solve your issue with modules being killed after 30 seconds.

To tell the truth I think systemd should not be doing it; it is not its
place to dictate how long module should take to load. It may print
warnings and we'll work on fixing the drivers, but aborting boot just
because they feel like it took too long is not a good idea.

Thanks.

-- 
Dmitry


driver-core: add driver async_probe support

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Some devices take a long time when initializing, and not all drivers are
suited to initialize their devices when they are open. For example, input
drivers need to interrogate device in order to publish its capabilities
before userspace will open them. When such drivers are compiled into kernel
they may stall entire kernel initialization.

This change allows drivers request for their probe functions to be called
asynchronously during driver and device registration (manual binding is
still synchronous). Because async_schedule is used to perform asynchronous
calls module loading will still wait for the probing to complete.

This is based on earlier patch by "Luis R. Rodriguez" [off-list ref]

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/bus.c     |   31 ++++++++++----
 drivers/base/dd.c      |  106 +++++++++++++++++++++++++++++++++++++++---------
 include/linux/device.h |    2 +
 3 files changed, 112 insertions(+), 27 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 83e910a..49fe573 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c

@@ -10,6 +10,7 @@
  *
  */
 
+#include <linux/async.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/errno.h>

@@ -547,15 +548,12 @@ void bus_probe_device(struct device *dev)
 {
 	struct bus_type *bus = dev->bus;
 	struct subsys_interface *sif;
-	int ret;
 
 	if (!bus)
 		return;
 
-	if (bus->p->drivers_autoprobe) {
-		ret = device_attach(dev);
-		WARN_ON(ret < 0);
-	}
+	if (bus->p->drivers_autoprobe)
+		device_initial_probe(dev);
 
 	mutex_lock(&bus->p->mutex);
 	list_for_each_entry(sif, &bus->p->interfaces, node)

@@ -657,6 +655,17 @@ 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.

@@ -689,9 +698,15 @@ int bus_add_driver(struct device_driver *drv)
 
 	klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
 	if (drv->bus->p->drivers_autoprobe) {
-		error = driver_attach(drv);
-		if (error)
-			goto out_unregister;
+		if (drv->async_probe) {
+			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;
+		}
 	}
 	module_add_driver(drv->owner, drv);
 

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..67a2f85 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c

@@ -402,31 +402,52 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
 	return ret;
 }
 
-static int __device_attach(struct device_driver *drv, void *data)
+struct device_attach_data {
+	struct device *dev;
+	bool check_async;
+	bool want_async;
+	bool have_async;
+};
+
+static int __device_attach_driver(struct device_driver *drv, void *_data)
 {
-	struct device *dev = data;
+	struct device_attach_data *data = _data;
+	struct device *dev = data->dev;
 
 	if (!driver_match_device(drv, dev))
 		return 0;
 
+	if (drv->async_probe)
+		data->have_async = true;
+
+	if (data->check_async && drv->async_probe != data->want_async)
+		return 0;
+
 	return driver_probe_device(drv, dev);
 }
 
-/**
- * device_attach - try to attach device to a driver.
- * @dev: device.
- *
- * Walk the list of drivers that the bus has and call
- * driver_probe_device() for each pair. If a compatible
- * pair is found, break out and return.
- *
- * Returns 1 if the device was bound to a driver;
- * 0 if no matching driver was found;
- * -ENODEV if the device is not registered.
- *
- * When called for a USB interface, @dev->parent lock must be held.
- */
-int device_attach(struct device *dev)
+static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
+{
+	struct device *dev = _dev;
+	struct device_attach_data data = {
+		.dev		= dev,
+		.check_async	= true,
+		.want_async	= true,
+	};
+
+	device_lock(dev);
+
+	bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver);
+	dev_dbg(dev, "async probe completed\n");
+
+	pm_request_idle(dev);
+
+	device_unlock(dev);
+
+	put_device(dev);
+}
+
+int __device_attach(struct device *dev, bool allow_async)
 {
 	int ret = 0;
 

@@ -444,15 +465,59 @@ int device_attach(struct device *dev)
 			ret = 0;
 		}
 	} else {
-		ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
-		pm_request_idle(dev);
+		struct device_attach_data data = {
+			.dev = dev,
+			.check_async = allow_async,
+			.want_async = false,
+		};
+
+		ret = bus_for_each_drv(dev->bus, NULL, &data,
+					__device_attach_driver);
+		if (!ret && allow_async && data.have_async) {
+			/*
+			 * If we could not find appropriate driver
+			 * synchronously and we are allowed to do
+			 * async probes and there are drivers that
+			 * want to probe asynchronously, we'll
+			 * try them.
+			 */
+			dev_dbg(dev, "scheduling asynchronous probe\n");
+			get_device(dev);
+			async_schedule(__device_attach_async_helper, dev);
+		} else {
+			pm_request_idle(dev);
+		}
 	}
 out_unlock:
 	device_unlock(dev);
 	return ret;
 }
+
+/**
+ * device_attach - try to attach device to a driver.
+ * @dev: device.
+ *
+ * Walk the list of drivers that the bus has and call
+ * driver_probe_device() for each pair. If a compatible
+ * pair is found, break out and return.
+ *
+ * Returns 1 if the device was bound to a driver;
+ * 0 if no matching driver was found;
+ * -ENODEV if the device is not registered.
+ *
+ * When called for a USB interface, @dev->parent lock must be held.
+ */
+int device_attach(struct device *dev)
+{
+	return __device_attach(dev, false);
+}
 EXPORT_SYMBOL_GPL(device_attach);
 
+void device_initial_probe(struct device *dev)
+{
+	__device_attach(dev, true);
+}
+
 static int __driver_attach(struct device *dev, void *data)
 {
 	struct device_driver *drv = data;

@@ -507,6 +572,9 @@ static void __device_release_driver(struct device *dev)
 
 	drv = dev->driver;
 	if (drv) {
+		if (drv->async_probe)
+			async_synchronize_full();
+
 		pm_runtime_get_sync(dev);
 
 		driver_sysfs_remove(dev);

diff --git a/include/linux/device.h b/include/linux/device.h
index 43d183a..c6fa2e7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h

@@ -233,6 +233,7 @@ struct device_driver {
 	const char		*mod_name;	/* used for built-in modules */
 
 	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
+	bool async_probe;
 
 	const struct of_device_id	*of_match_table;
 	const struct acpi_device_id	*acpi_match_table;

@@ -966,6 +967,7 @@ extern int __must_check device_bind_driver(struct device *dev);
 extern void device_release_driver(struct device *dev);
 extern int  __must_check device_attach(struct device *dev);
 extern int __must_check driver_attach(struct device_driver *drv);
+extern void device_initial_probe(struct device *dev);
 extern int __must_check device_reprobe(struct device *dev);
 
 /*

Based on my review with my latest changes this is what I was missing,
I'll be sure to address this.

 Luis