[RFC PATCH 0/5] driver core: driver probe asynchronously

[RFC PATCH 0/5] driver core: driver probe asynchronously

[Ming Lei]

Hi,

This patchset implements asynchronous driver probe for driver_register,
and try to address the below problems about driver init during
kernel boot:
	- help to solve some dependency problem during kernel boot
	(such as, request_firmware is called inside probe when driver
	is built in kernel[1])

The idea behind the patch is very simple:

	- seperate driver probe from driver_register and run this part
	in one standalone kernel thread context

	- so driver_register will become two parts: register the driver
	on the bus, and trigger to schedule a kernel thread to do the
	driver probe if autoprobe is set

Fortunately, my OMAP4 based Pandaboard boots fine with the patchset, and
looks it may work well.

More or less, some problems might be triggered by these patchset, but
it should be helpful and not a big deal:
	- the dependency problem may be found, and it either exposes
	the driver's probelm or help to improve the asynchronous probe
	approach

	- can use driver_register_sync to work around it

In summary, there are at least two advantages about asynchronous driver
probe:
	- speedup kernel boot when many drivers are built in kernel
	- make driver's probe() not need to consider running something
	asynchronously(such as, scsi scan, request_firmware_no_wait, ...),
	so easier to write drivers

It should a very simple way to help to solve the problem[1], without
any changes on current drivers which call request_firmware() in its
probe(). BTW, the patchset doesn't solve the problem completely, and
still some work is needed.


[1], http://marc.info/?t=134676416100002&r=1&w=2

[RFC PATCH 1/5] driver core: driver probe at the last minute in driver_register

[Ming Lei]

It should be correct to probe driver at the last minute in
driver_register since it will be so in !auto_probe case and
driver_register on no matched devices case.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/bus.c    |    5 -----
 drivers/base/driver.c |    4 ++++
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 181ed26..00d841d 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -714,11 +714,6 @@ int bus_add_driver(struct device_driver *drv)
 	if (error)
 		goto out_unregister;
 
-	if (drv->bus->p->drivers_autoprobe) {
-		error = driver_attach(drv);
-		if (error)
-			goto out_unregister;
-	}
 	klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
 	module_add_driver(drv->owner, drv);
 
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 974e301..27f4c85 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -191,6 +191,10 @@ int driver_register(struct device_driver *drv)
 	}
 	kobject_uevent(&drv->p->kobj, KOBJ_ADD);
 
+	/* driver probe at the last minute */
+	if (drv->bus->p->drivers_autoprobe)
+		ret = driver_attach(drv);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(driver_register);
-- 
1.7.9.5

[RFC PATCH 2/5] driver core: introduce driver_register_sync helper

[Ming Lei]

This patch introduces driver_register_sync helper to make
driver's probe called synchronously inside driver_register
path.

platform_driver_probe should use driver_register_sync.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/driver.c  |    5 +++--
 include/linux/device.h |   13 ++++++++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 27f4c85..9365076 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -156,12 +156,13 @@ static void driver_remove_groups(struct device_driver *drv,
 /**
  * driver_register - register driver with bus
  * @drv: driver to register
+ * @sync: if the .probe() is called synchronously
  *
  * We pass off most of the work to the bus_add_driver() call,
  * since most of the things we have to do deal with the bus
  * structures.
  */
-int driver_register(struct device_driver *drv)
+int __driver_register(struct device_driver *drv, int sync)
 {
 	int ret;
 	struct device_driver *other;
@@ -197,7 +198,7 @@ int driver_register(struct device_driver *drv)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(driver_register);
+EXPORT_SYMBOL_GPL(__driver_register);
 
 /**
  * driver_unregister - remove driver from system.
diff --git a/include/linux/device.h b/include/linux/device.h
index dd0d684..7222c0b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -237,9 +237,20 @@ struct device_driver {
 };
 
 
-extern int __must_check driver_register(struct device_driver *drv);
+extern int __must_check __driver_register(struct device_driver *drv,
+					  int sync);
 extern void driver_unregister(struct device_driver *drv);
 
+static inline int __must_check driver_register(struct device_driver *drv)
+{
+	return __driver_register(drv, 0);
+}
+
+static inline int __must_check driver_register_sync(struct device_driver *drv)
+{
+	return __driver_register(drv, 1);
+}
+
 extern struct device_driver *driver_find(const char *name,
 					 struct bus_type *bus);
 extern int driver_probe_done(void);
-- 
1.7.9.5

[RFC PATCH 3/5] driver core: platform: apply driver_register_sync for platform_driver_probe

[Ming Lei]

platform_driver_probe supposes that the driver's probe is always called
inside platform_driver_probe, so apply driver_register_sync for it.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/platform.c         |   11 +++++++----
 include/linux/platform_device.h |   12 +++++++++++-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 42ca90c..f1d3507 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -500,7 +500,7 @@ static void platform_drv_shutdown(struct device *_dev)
  * platform_driver_register - register a driver for platform-level devices
  * @drv: platform driver structure
  */
-int platform_driver_register(struct platform_driver *drv)
+int __platform_driver_register(struct platform_driver *drv, int sync)
 {
 	drv->driver.bus = &platform_bus_type;
 	if (drv->probe)
@@ -510,9 +510,12 @@ int platform_driver_register(struct platform_driver *drv)
 	if (drv->shutdown)
 		drv->driver.shutdown = platform_drv_shutdown;
 
-	return driver_register(&drv->driver);
+	if (sync)
+		return driver_register_sync(&drv->driver);
+	else
+		return driver_register(&drv->driver);
 }
-EXPORT_SYMBOL_GPL(platform_driver_register);
+EXPORT_SYMBOL_GPL(__platform_driver_register);
 
 /**
  * platform_driver_unregister - unregister a driver for platform-level devices
@@ -551,7 +554,7 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
 
 	/* temporary section violation during probe() */
 	drv->probe = probe;
-	retval = code = platform_driver_register(drv);
+	retval = code = platform_driver_register_sync(drv);
 
 	/*
 	 * Fixup that section violation, being paranoid about code scanning
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 5711e95..f791a59 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -175,9 +175,19 @@ struct platform_driver {
 	const struct platform_device_id *id_table;
 };
 
-extern int platform_driver_register(struct platform_driver *);
+extern int __platform_driver_register(struct platform_driver *, int sync);
 extern void platform_driver_unregister(struct platform_driver *);
 
+static inline int platform_driver_register(struct platform_driver *drv)
+{
+	return __platform_driver_register(drv, 0);
+}
+
+static inline int platform_driver_register_sync(struct platform_driver *drv)
+{
+	return __platform_driver_register(drv, 1);
+}
+
 /* non-hotpluggable platform devices may use this so that probe() and
  * its support may live in __init sections, conserving runtime memory.
  */
-- 
1.7.9.5

[RFC PATCH 4/5] driver core: remove __must_check on declaration of driver_attach

[Ming Lei]

The check is not necessary since driver_attach should return 0
always.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 include/linux/device.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index 7222c0b..dbb5efd 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -858,7 +858,7 @@ static inline void *dev_get_platdata(const struct device *dev)
 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 int driver_attach(struct device_driver *drv);
 extern int __must_check device_reprobe(struct device *dev);
 
 /*
-- 
1.7.9.5

[RFC PATCH 5/5] driver core: implement asynchorous probe() inside driver_register

[Ming Lei]

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/driver.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 9365076..67fb63c 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -15,6 +15,7 @@
 #include <linux/errno.h>
 #include <linux/slab.h>
 #include <linux/string.h>
+#include <linux/async.h>
 #include "base.h"
 
 static struct device *next_device(struct klist_iter *i)
@@ -153,6 +154,13 @@ static void driver_remove_groups(struct device_driver *drv,
 			sysfs_remove_group(&drv->p->kobj, groups[i]);
 }
 
+static void async_driver_attach(void *data, async_cookie_t cookie)
+{
+	struct device_driver *drv = data;
+
+	driver_attach(drv);
+}
+
 /**
  * driver_register - register driver with bus
  * @drv: driver to register
@@ -193,8 +201,12 @@ int __driver_register(struct device_driver *drv, int sync)
 	kobject_uevent(&drv->p->kobj, KOBJ_ADD);
 
 	/* driver probe at the last minute */
-	if (drv->bus->p->drivers_autoprobe)
-		ret = driver_attach(drv);
+	if (drv->bus->p->drivers_autoprobe) {
+		if (sync)
+			ret = driver_attach(drv);
+		else
+			async_schedule(async_driver_attach, drv);
+	}
 
 	return ret;
 }
-- 
1.7.9.5

Re: [RFC PATCH 0/5] driver core: driver probe asynchronously

[Greg Kroah-Hartman]

On Sun, Sep 09, 2012 at 07:25:15AM +0800, Ming Lei wrote:
> Hi,
> 
> This patchset implements asynchronous driver probe for driver_register,
> and try to address the below problems about driver init during
> kernel boot:
> 	- help to solve some dependency problem during kernel boot
> 	(such as, request_firmware is called inside probe when driver
> 	is built in kernel[1])
> 
> The idea behind the patch is very simple:
> 
> 	- seperate driver probe from driver_register and run this part
> 	in one standalone kernel thread context
> 
> 	- so driver_register will become two parts: register the driver
> 	on the bus, and trigger to schedule a kernel thread to do the
> 	driver probe if autoprobe is set

You do realize we tried this out about 5+ years ago, it caused major
problems, no real benefit, and so we removed it (or maybe never did the
final commit with it, it might never have hit Linus's tree due to all of
the problems we found.)

> Fortunately, my OMAP4 based Pandaboard boots fine with the patchset, and
> looks it may work well.
> 
> More or less, some problems might be triggered by these patchset, but
> it should be helpful and not a big deal:
> 	- the dependency problem may be found, and it either exposes
> 	the driver's probelm or help to improve the asynchronous probe
> 	approach
> 
> 	- can use driver_register_sync to work around it
> 
> In summary, there are at least two advantages about asynchronous driver
> probe:
> 	- speedup kernel boot when many drivers are built in kernel

I would be really amazed if this is true in any measurable fashion.
Again, we tried this, and it did seem to be a tiny bit faster, but in
the end, wasn't worth it.

> 	- make driver's probe() not need to consider running something
> 	asynchronously(such as, scsi scan, request_firmware_no_wait, ...),
> 	so easier to write drivers

No, that's a bus issue, not a driver issue.  Busses can do this today,
if they want to, no problem at all.  So, some busses have (maybe, maybe
not, I can't remember, but I think ATA did), and that is the level you
need to do this at, not at the driver core level, sorry.

> It should a very simple way to help to solve the problem[1], without
> any changes on current drivers which call request_firmware() in its
> probe(). BTW, the patchset doesn't solve the problem completely, and
> still some work is needed.

As you state, this doesn't really solve the problem, and will cause a
lot more (think about all the busses that aren't expecting to have their
probe functions called at the same time as other probe functions are
being called.)

So while I applaud the effort, I can't accept this due to the past
history of when it was tried before.

sorry,

greg k-h

Re: [RFC PATCH 0/5] driver core: driver probe asynchronously

[Benjamin Herrenschmidt]

On Sat, 2012-09-08 at 19:57 -0700, Greg Kroah-Hartman wrote:
> As you state, this doesn't really solve the problem, and will cause a
> lot more (think about all the busses that aren't expecting to have
> their
> probe functions called at the same time as other probe functions are
> being called.)
> 
> So while I applaud the effort, I can't accept this due to the past
> history of when it was tried before. 

Maybe a compromise would be to have ->probe() be called synchronously as
done currently, but to add support for a specific -EAGAIN return from
it, requiring a later asynchronous re-probe ? (Possibly requiring an
explicit trigger)

That would allow drivers who fail some kind of "oddball" dependency (the
firmware load is such as case, there are a few others) to basically
install some kind of notifier and try again later when the dependency
has been fulfilled.

That way existing drivers still behave synchronously, there is no
breakage unless drivers are explicitly modified for async probing.

Cheers,
Ben.

Re: [RFC PATCH 0/5] driver core: driver probe asynchronously

[Takashi Iwai]

At Sun, 09 Sep 2012 17:47:46 +1000,
Benjamin Herrenschmidt wrote:
> 
> On Sat, 2012-09-08 at 19:57 -0700, Greg Kroah-Hartman wrote:
> > As you state, this doesn't really solve the problem, and will cause a
> > lot more (think about all the busses that aren't expecting to have
> > their
> > probe functions called at the same time as other probe functions are
> > being called.)
> > 
> > So while I applaud the effort, I can't accept this due to the past
> > history of when it was tried before. 
> 
> Maybe a compromise would be to have ->probe() be called synchronously as
> done currently, but to add support for a specific -EAGAIN return from
> it, requiring a later asynchronous re-probe ? (Possibly requiring an
> explicit trigger)

So it's something pretty similar like -EPROBE_DEFER, but the deferred
probing is rather based on the standard async probe?

I find it'd make sense, as the current deferred probing is somewhat
hackish.  Forcing all asynchronous right now looks risky, but
selective way of easy async probing would be welcome.


thanks,

Takashi

> That would allow drivers who fail some kind of "oddball" dependency (the
> firmware load is such as case, there are a few others) to basically
> install some kind of notifier and try again later when the dependency
> has been fulfilled.
> 
> That way existing drivers still behave synchronously, there is no
> breakage unless drivers are explicitly modified for async probing.

Re: [RFC PATCH 0/5] driver core: driver probe asynchronously

[Ming Lei]

On Sun, Sep 9, 2012 at 10:57 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Sep 09, 2012 at 07:25:15AM +0800, Ming Lei wrote:
>> Hi,
>>
>> This patchset implements asynchronous driver probe for driver_register,
>> and try to address the below problems about driver init during
>> kernel boot:
>>       - help to solve some dependency problem during kernel boot
>>       (such as, request_firmware is called inside probe when driver
>>       is built in kernel[1])
>>
>> The idea behind the patch is very simple:
>>
>>       - seperate driver probe from driver_register and run this part
>>       in one standalone kernel thread context
>>
>>       - so driver_register will become two parts: register the driver
>>       on the bus, and trigger to schedule a kernel thread to do the
>>       driver probe if autoprobe is set
>
> You do realize we tried this out about 5+ years ago, it caused major
> problems, no real benefit, and so we removed it (or maybe never did the
> final commit with it, it might never have hit Linus's tree due to all of
> the problems we found.)

Thanks for the information you provided.

I guess you mean the commit 21c7f30b1d3f8a3de3128478daca3ce203fc8733

        driver core: per-subsystem multithreaded probing

which was reverted surely.

IMO, this patch is different with the above commit:

      -  the probe order is guaranteed to be that parent first,
children second since
         both parent's lock and child's lock are held inside
driver_attach. In the
         commit of 21c7f30b, only device's lock is held in the
asynchronous kthread.

      - it should be supported already by hotplug buses: suppose all drivers
        in the bus have been loaded already, so the probe order depends on
        the order of device_add, which is an uncertain event and is random,
        so there should be no probe order dependency on hotplug buses.

      - the async probe is just triggered in driver_init(module_init)
path, which
        is very similar with loading driver from module(looks no obvious probe
        order dependency for non-dependent  moduels)

I will study the original commit further and other non-hotplug buses to see
if there are more dependency about probe order. But IMO, this patch does
respect the basic probe order: parent first, children second.

>
>> Fortunately, my OMAP4 based Pandaboard boots fine with the patchset, and
>> looks it may work well.
>>
>> More or less, some problems might be triggered by these patchset, but
>> it should be helpful and not a big deal:
>>       - the dependency problem may be found, and it either exposes
>>       the driver's probelm or help to improve the asynchronous probe
>>       approach
>>
>>       - can use driver_register_sync to work around it
>>
>> In summary, there are at least two advantages about asynchronous driver
>> probe:
>>       - speedup kernel boot when many drivers are built in kernel
>
> I would be really amazed if this is true in any measurable fashion.
> Again, we tried this, and it did seem to be a tiny bit faster, but in
> the end, wasn't worth it.

If we can make sure that the approach is doable, I will provide some
test data.

>
>>       - make driver's probe() not need to consider running something
>>       asynchronously(such as, scsi scan, request_firmware_no_wait, ...),
>>       so easier to write drivers
>
> No, that's a bus issue, not a driver issue.  Busses can do this today,
> if they want to, no problem at all.  So, some busses have (maybe, maybe
> not, I can't remember, but I think ATA did), and that is the level you
> need to do this at, not at the driver core level, sorry.

IMO, most of drivers have no probe order dependency, and that is my
motivation to try it. From current async_schedule users, most
of them are used inside driver probe, see below:

arch/sh/drivers/pci/pcie-sh7786.c:572:		async_schedule(sh7786_pcie_hwops->port_init_hw,
port);
drivers/acpi/battery.c:1109:	async_schedule(acpi_battery_init_async, NULL);
drivers/ata/libata-core.c:6118:		async_schedule(async_port_probe, ap);
drivers/base/power/main.c:694:			async_schedule(async_resume, dev);
drivers/base/power/main.c:1166:		async_schedule(async_suspend, dev);
drivers/block/floppy.c:4353:	async_schedule(floppy_async_init, NULL);
drivers/md/raid5.c:1478:	async_schedule(async_run_ops, sh);
drivers/regulator/core.c:2964:		async_schedule_domain(regulator_bulk_enable_async,
drivers/s390/block/dasd.c:3011:		async_schedule(dasd_generic_auto_online, cdev);
drivers/scsi/libsas/sas_ata.c:857:			async_schedule_domain(async_sas_ata_eh,
dev, &async);
drivers/scsi/scsi_scan.c:1875:	async_schedule(do_scan_async, data);
drivers/scsi/sd.c:2734:	async_schedule_domain(sd_probe_async, sdkp,
&scsi_sd_probe_domain);
sound/soc/soc-dapm.c:1656:		async_schedule_domain(dapm_pre_sequence_async, d,
sound/soc/soc-dapm.c:1670:		async_schedule_domain(dapm_post_sequence_async, d,


>
>> It should a very simple way to help to solve the problem[1], without
>> any changes on current drivers which call request_firmware() in its
>> probe(). BTW, the patchset doesn't solve the problem completely, and
>> still some work is needed.
>
> As you state, this doesn't really solve the problem, and will cause a
> lot more (think about all the busses that aren't expecting to have their
> probe functions called at the same time as other probe functions are
> being called.)

If the solution can be proved doable, we may use request_firmware
safely in driver probe in either built-in and module situations, so we
can easily to apply the asynchronous probe to fix current problem:

           - replace async_synchronize_full in prepare_namespace
             with async_synchronize_full_domain

           - introduce one completion which will be pended on inside
             request_firmware and will be woken up after prepare_namespace
             is completed in built-in situations.

Thanks,
--
Ming Lei