[PATCH] drivercore: Add driver probe deferral mechanism

[PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

Allow drivers to report at probe time that they cannot get all the resources
required by the device, and should be retried at a later time.

This should completely solve the problem of getting devices
initialized in the right order.  Right now this is mostly handled by
mucking about with initcall ordering which is a complete hack, and
doesn't even remotely handle the case where device drivers are in
modules.  This approach completely sidesteps the issues by allowing
driver registration to occur in any order, and any driver can request
to be retried after a few more other drivers get probed.

This still is not tested, but I'd like to get early feedback on if
this is the correct approach.

v2: - added locking so it should no longer be utterly broken in that regard
    - remove device from deferred list at device_del time.
    - Still completely untested with any real use case, but has been boot tested.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

Mark, I'm particularly interested in your thoughts on this approach.
It is decidedly "low-tech" in its approach to handling device
dependencies, but it has the advantage of being simple and should
handle a wide range of use-cases reliably.  Would this work for ALSA
SoC probing?

g.


 drivers/base/base.h    |    1 +
 drivers/base/core.c    |    2 ++
 drivers/base/dd.c      |   64 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/device.h |    5 ++++
 4 files changed, 71 insertions(+), 1 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index a34dca0..9641309 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -105,6 +105,7 @@ extern void bus_remove_driver(struct device_driver *drv);
 
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
+extern void driver_deferred_probe_del(struct device *dev);
 static inline int driver_match_device(struct device_driver *drv,
 				      struct device *dev)
 {
diff --git a/drivers/base/core.c b/drivers/base/core.c
index bc8729d..0d37e18 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -588,6 +588,7 @@ void device_initialize(struct device *dev)
 {
 	dev->kobj.kset = devices_kset;
 	kobject_init(&dev->kobj, &device_ktype);
+	INIT_LIST_HEAD(&dev->deferred_probe);
 	INIT_LIST_HEAD(&dev->dma_pools);
 	mutex_init(&dev->mutex);
 	lockdep_set_novalidate_class(&dev->mutex);
@@ -1119,6 +1120,7 @@ void device_del(struct device *dev)
 	device_remove_file(dev, &uevent_attr);
 	device_remove_attrs(dev);
 	bus_remove_device(dev);
+	driver_deferred_probe_del(dev);
 
 	/*
 	 * Some platform devices are driven without driver attached
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 6658da7..ccbf3d3 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -28,6 +28,62 @@
 #include "base.h"
 #include "power/power.h"
 
+/**
+ * deferred_probe_work_func() - Retry probing devices in the deferred list.
+ */
+static DEFINE_MUTEX(deferred_probe_mutex);
+static LIST_HEAD(deferred_probe_list);
+static void deferred_probe_work_func(struct work_struct *work)
+{
+	struct device *dev;
+	/*
+	 * This bit is tricky.  We want to process every device in the
+	 * deferred list, but devices can be removed from the list at any
+	 * time while inside this for-each loop.  There are two things that
+	 * need to be protected against:
+	 * - if the device is removed from the deferred_probe_list, then we
+	 *   loose our place in the loop.  Since any device can be removed
+	 *   asynchronously, list_for_each_entry_safe() wouldn't make things
+	 *   much better.  Simplest solution is to restart walking the list
+	 *   whenever the current device gets removed.  Not the most efficient,
+	 *   but is simple to implement and easy to audit for correctness.
+	 * - if the device is unregistered, and freed, then there is a risk
+	 *   of a null pointer dereference.  This code uses get/put_device()
+	 *   to ensure the device cannot disappear from under our feet.
+	 */
+	mutex_lock(&deferred_probe_mutex);
+	list_for_each_entry(dev, &deferred_probe_list, deferred_probe) {
+		bool removed;
+		get_device(dev);
+		mutex_unlock(&deferred_probe_mutex);
+
+		bus_probe_device(dev);
+
+		mutex_lock(&deferred_probe_mutex);
+		removed = list_empty(&dev->deferred_probe);
+		put_device(dev);
+		if (removed)
+			break;
+	}
+	mutex_unlock(&deferred_probe_mutex);
+}
+static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);
+
+static void driver_deferred_probe_add(struct device *dev)
+{
+	mutex_lock(&deferred_probe_mutex);
+	if (list_empty(&dev->deferred_probe))
+		list_add(&dev->deferred_probe, &deferred_probe_list);
+	mutex_unlock(&deferred_probe_mutex);
+}
+
+void driver_deferred_probe_del(struct device *dev)
+{
+	mutex_lock(&deferred_probe_mutex);
+	if (!list_empty(&dev->deferred_probe))
+		list_del_init(&dev->deferred_probe);
+	mutex_unlock(&deferred_probe_mutex);
+}
 
 static void driver_bound(struct device *dev)
 {
@@ -41,6 +97,8 @@ static void driver_bound(struct device *dev)
 		 __func__, dev->driver->name);
 
 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
+	driver_deferred_probe_del(dev);
+	schedule_work(&deferred_probe_work);
 
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -142,7 +200,11 @@ probe_failed:
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;
 
-	if (ret != -ENODEV && ret != -ENXIO) {
+	if (ret == -EAGAIN) {
+		/* Driver requested deferred probing */
+		dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
+		driver_deferred_probe_add(dev);
+	} else if (ret != -ENODEV && ret != -ENXIO) {
 		/* driver matched but the probe failed */
 		printk(KERN_WARNING
 		       "%s: probe of %s failed with error %d\n",
diff --git a/include/linux/device.h b/include/linux/device.h
index e4f62d8..b44a3b1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -506,6 +506,10 @@ struct device_dma_parameters {
  * @mutex:	Mutex to synchronize calls to its driver.
  * @bus:	Type of bus device is on.
  * @driver:	Which driver has allocated this
+ * @deferred_probe: entry in deferred_probe_list which is used to retry the
+ * 		binding of drivers which were unable to get all the resources
+ * 		needed by the device; typically because it depends on another
+ * 		driver getting probed first.
  * @platform_data: Platform data specific to the device.
  * 		Example: For devices on custom boards, as typical of embedded
  * 		and SOC based hardware, Linux often uses platform_data to point
@@ -564,6 +568,7 @@ struct device {
 	struct bus_type	*bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	struct list_head	deferred_probe;
 	void		*platform_data;	/* Platform specific data, device
 					   core doesn't touch it */
 	struct dev_pm_info	power;

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg KH]

On Mon, Jul 04, 2011 at 11:11:59AM -0600, Grant Likely wrote:
> Allow drivers to report at probe time that they cannot get all the resources
> required by the device, and should be retried at a later time.

When is "later"?

And why would a driver not be able to get all of the proper resources?

Why can't a bus, at a later time, just try to reprobe everything when it
determines that it is a "later" time now, without having to do this
added change to the core?

> This should completely solve the problem of getting devices
> initialized in the right order.  Right now this is mostly handled by
> mucking about with initcall ordering which is a complete hack, and
> doesn't even remotely handle the case where device drivers are in
> modules.  This approach completely sidesteps the issues by allowing
> driver registration to occur in any order, and any driver can request
> to be retried after a few more other drivers get probed.

Why would drivers in modules be an issue?  If a driver depends on
another driver, making it a module dependancy would solve the problem,
right?

thanks,

greg k-h

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Mark Brown]

On Mon, Jul 04, 2011 at 10:41:26AM -0700, Greg KH wrote:

> And why would a driver not be able to get all of the proper resources?

If the resources are provided by another driver (for example, a clock,
GPIO or regulator) then the driver doing the providing needs to have
initialized before the resource can be used.  Currently we have no way
of ensuring that this happens.

> Why can't a bus, at a later time, just try to reprobe everything when it
> determines that it is a "later" time now, without having to do this
> added change to the core?

The buses can't do anything to help as they have no visibility of this
and the chances are that devices on several different buses are
involved.

> Why would drivers in modules be an issue?  If a driver depends on
> another driver, making it a module dependancy would solve the problem,
> right?

The general problem here is that the drivers depend on each other in
system-specific fashions that have nothing to do with the control
heirachy the device model uses - for example, a device may depend on
having a clock provided but the specific clock used could come from
essentially any device in a given system with the driver accessing that
clock via the clock API which matches devices up with their providers.
The API indirection means we can share code between systems but means
that the kernel infrastructure has no visibility of the relationships.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Mon, Jul 04, 2011 at 10:41:26AM -0700, Greg KH wrote:
> On Mon, Jul 04, 2011 at 11:11:59AM -0600, Grant Likely wrote:
> > Allow drivers to report at probe time that they cannot get all the resources
> > required by the device, and should be retried at a later time.
> 
> When is "later"?

In this case, after at least one other device has successfully probed.
The 'later' is handled in a workqueue that walks the list
asynchronously from normal initialization.

> And why would a driver not be able to get all of the proper resources?

Discussed below...

> Why can't a bus, at a later time, just try to reprobe everything when it
> determines that it is a "later" time now, without having to do this
> added change to the core?

It can't be done by a specific bus type because it has zero
relationship with the bus type.  For example, it is typical for an
SDHCI driver to require a GPIO line for the card detect switch, and
the device cannot be initialized until it has it.  However, the
bus_type that the SDHCI driver is attached to could be anything;
platform_bus_type, pci, amba, etc.  It isn't a bus_type deficiency,
but rather that the driver core has no way to gracefully handle
devices that get probed in an undetermined order.

It has to be done at the core level because any device in the system,
regardless of bus_type, may require another device to be probed first.
Originally I tried modifying the drivers to successfully probe
anyway and then 'go to sleep' to try again later, but it turned out to
push a lot of complexity into the device drivers when it can be solved
far more simply if the driver core has the ability to retry drivers
that request it.

Another example is sound on embedded systems.  A "sound card"
typically consists of multiple devices; one or more codecs (often i2c
or spi attached), a sound bus (often i2s), a dma controller, and a
lump of machine/platform specific code that ties them all together.
Right now the ASoC code is going through all kinds of gymnastics make
each component register with the ASoC layer and the 'tie together'
driver has to wait for each of them to show up.  As Mark will attest,
it is a lot of domain specific code that cannot be easily generalized
to be used by other subsystems.

I also could have implemented this without the deferred_probe list,
but doing so would have required every unbound device in the system to
be retried against every matching driver each time an -EAGAIN event
occurred.  I figured that would be rather excessive when the kernel
already has specific knowledge of devices that need to be retried.

> > This should completely solve the problem of getting devices
> > initialized in the right order.  Right now this is mostly handled by
> > mucking about with initcall ordering which is a complete hack, and
> > doesn't even remotely handle the case where device drivers are in
> > modules.  This approach completely sidesteps the issues by allowing
> > driver registration to occur in any order, and any driver can request
> > to be retried after a few more other drivers get probed.
> 
> Why would drivers in modules be an issue?  If a driver depends on
> another driver, making it a module dependancy would solve the problem,
> right?

Not nearly that simple.  Consider the case where driver A depends on a
gpio resource provided by driver B.  Well, driver A could depend on
driver B, but driver A has zero knowledge of what B actually is
because B is different for each system.

So, since A cannot depend on B because B is dynamic, the problem
cannot be solved with enforcing a module load order.

g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Randy Dunlap]

On Mon, 04 Jul 2011 11:11:59 -0600 Grant Likely wrote:

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 6658da7..ccbf3d3 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -28,6 +28,62 @@
>  #include "base.h"
>  #include "power/power.h"
>  
> +/**
> + * deferred_probe_work_func() - Retry probing devices in the deferred list.
> + */
> +static DEFINE_MUTEX(deferred_probe_mutex);
> +static LIST_HEAD(deferred_probe_list);
> +static void deferred_probe_work_func(struct work_struct *work)

The kernel-doc notation needs to be immediately before the function,
without the intervening data...

> +{
> +	struct device *dev;
> +	/*
> +	 * This bit is tricky.  We want to process every device in the
> +	 * deferred list, but devices can be removed from the list at any
> +	 * time while inside this for-each loop.  There are two things that
> +	 * need to be protected against:
> +	 * - if the device is removed from the deferred_probe_list, then we
> +	 *   loose our place in the loop.  Since any device can be removed
> +	 *   asynchronously, list_for_each_entry_safe() wouldn't make things
> +	 *   much better.  Simplest solution is to restart walking the list
> +	 *   whenever the current device gets removed.  Not the most efficient,
> +	 *   but is simple to implement and easy to audit for correctness.
> +	 * - if the device is unregistered, and freed, then there is a risk
> +	 *   of a null pointer dereference.  This code uses get/put_device()
> +	 *   to ensure the device cannot disappear from under our feet.
> +	 */


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Mark Brown]

On Mon, Jul 04, 2011 at 11:11:59AM -0600, Grant Likely wrote:

> Mark, I'm particularly interested in your thoughts on this approach.
> It is decidedly "low-tech" in its approach to handling device
> dependencies, but it has the advantage of being simple and should
> handle a wide range of use-cases reliably.  Would this work for ALSA
> SoC probing?

It's essentially what we're doing currently for the part of the system
where we decide that everything is registered and we should run the
actual probe functions so it'll help with that.  Having a clock API we
can actually use off-SoC will help with a lot of the remaining stuff.

I *think* we'll still going to need to have the infrastructure to deal
with running all the probes together, at least for a while, as the
current code really assumes that it's got some of the card wide stuff
around when all the devices get instantiated but I think if we were
starting from fresh this would be fairly good.  The only thing I can
think might be an issue is n way dependencies, but those mostly shake
out as being a dependency of the overall card on subdevices.  I'd need
to separate out the implementation issues from the assumptions to be
100% clear if that was the case, though.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Mon, Jul 04, 2011 at 01:47:18PM -0700, Mark Brown wrote:
> On Mon, Jul 04, 2011 at 11:11:59AM -0600, Grant Likely wrote:
> 
> > Mark, I'm particularly interested in your thoughts on this approach.
> > It is decidedly "low-tech" in its approach to handling device
> > dependencies, but it has the advantage of being simple and should
> > handle a wide range of use-cases reliably.  Would this work for ALSA
> > SoC probing?
> 
> It's essentially what we're doing currently for the part of the system
> where we decide that everything is registered and we should run the
> actual probe functions so it'll help with that.  Having a clock API we
> can actually use off-SoC will help with a lot of the remaining stuff.

Is that the bit that snd_soc_instantiate_cards() is doing?  If I made 
snd_soc_register_card() attempt to instantiate immediately and return
-EAGAIN if resources were not available, it looks like I should be
able to remove the card_list entirely.  Would that be the right way to
go about it?

> I *think* we'll still going to need to have the infrastructure to deal
> with running all the probes together, at least for a while, as the
> current code really assumes that it's got some of the card wide stuff
> around when all the devices get instantiated but I think if we were
> starting from fresh this would be fairly good.

I'm not sure what you're getting at here.  Are you talking about the
infrastructure to keep track of codecs, DAIs and the like?  Yes, that
infrastructure is still needed because the drivers need an api for
each of the kinds of resources it needs.

> The only thing I can
> think might be an issue is n way dependencies, but those mostly shake
> out as being a dependency of the overall card on subdevices.  I'd need
> to separate out the implementation issues from the assumptions to be
> 100% clear if that was the case, though.

Do you have an example?  I don't see any problems with complex
dependencies providing that none of them are circular.  If, say,
device A depends on B and C, and B also depends on C, then it may take
a couple of cycles before everything gets probed, but it all still
works correctly.

g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Mark Brown]

On Mon, Jul 04, 2011 at 05:25:09PM -0600, Grant Likely wrote:
> On Mon, Jul 04, 2011 at 01:47:18PM -0700, Mark Brown wrote:

> > where we decide that everything is registered and we should run the
> > actual probe functions so it'll help with that.  Having a clock API we
> > can actually use off-SoC will help with a lot of the remaining stuff.

> Is that the bit that snd_soc_instantiate_cards() is doing?  If I made 
> snd_soc_register_card() attempt to instantiate immediately and return
> -EAGAIN if resources were not available, it looks like I should be
> able to remove the card_list entirely.  Would that be the right way to
> go about it?

That's exactly what I'd expect to do, yes.

> > I *think* we'll still going to need to have the infrastructure to deal
> > with running all the probes together, at least for a while, as the
> > current code really assumes that it's got some of the card wide stuff
> > around when all the devices get instantiated but I think if we were
> > starting from fresh this would be fairly good.

> I'm not sure what you're getting at here.  Are you talking about the
> infrastructure to keep track of codecs, DAIs and the like?  Yes, that
> infrastructure is still needed because the drivers need an api for
> each of the kinds of resources it needs.

Sort of, but I'm more thinking of the fact that we currently defer all
the "real" probes until the full card is ready - ideally these could all
be done as part of the device model probe, but this is something we can
improve over time rather than a critical thing.  The regmap API will
help here.

> > The only thing I can
> > think might be an issue is n way dependencies, but those mostly shake
> > out as being a dependency of the overall card on subdevices.  I'd need
> > to separate out the implementation issues from the assumptions to be
> > 100% clear if that was the case, though.

> Do you have an example?  I don't see any problems with complex
> dependencies providing that none of them are circular.  If, say,
> device A depends on B and C, and B also depends on C, then it may take
> a couple of cycles before everything gets probed, but it all still
> works correctly.

It's circular dependencies that worry me.  I don't have any concrete
examples in mainline now but I'm aware of systems that are more
complicated than mainline.  Things like clock trees that go out of one
device, into another, and back into to the first.  Like I say I think
these all shake out as simple dependencies of component devices on the
card so it's probably not a problem.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg KH]

On Mon, Jul 04, 2011 at 12:01:59PM -0600, Grant Likely wrote:
> On Mon, Jul 04, 2011 at 10:41:26AM -0700, Greg KH wrote:
> > On Mon, Jul 04, 2011 at 11:11:59AM -0600, Grant Likely wrote:
> > > Allow drivers to report at probe time that they cannot get all the resources
> > > required by the device, and should be retried at a later time.
> > 
> > When is "later"?
> 
> In this case, after at least one other device has successfully probed.
> The 'later' is handled in a workqueue that walks the list
> asynchronously from normal initialization.
> 
> > And why would a driver not be able to get all of the proper resources?
> 
> Discussed below...
> 
> > Why can't a bus, at a later time, just try to reprobe everything when it
> > determines that it is a "later" time now, without having to do this
> > added change to the core?
> 
> It can't be done by a specific bus type because it has zero
> relationship with the bus type.  For example, it is typical for an
> SDHCI driver to require a GPIO line for the card detect switch, and
> the device cannot be initialized until it has it.  However, the
> bus_type that the SDHCI driver is attached to could be anything;
> platform_bus_type, pci, amba, etc.  It isn't a bus_type deficiency,
> but rather that the driver core has no way to gracefully handle
> devices that get probed in an undetermined order.
> 
> It has to be done at the core level because any device in the system,
> regardless of bus_type, may require another device to be probed first.
> Originally I tried modifying the drivers to successfully probe
> anyway and then 'go to sleep' to try again later, but it turned out to
> push a lot of complexity into the device drivers when it can be solved
> far more simply if the driver core has the ability to retry drivers
> that request it.

So the driver core is just going to sit and spin and continue to try to
probe drivers for as long as it gets that error value returned?  What is
going to ever cause that loop to terminate?  It seems a bit hacky to
just keep looping over and over and hoping that sometime everything will
all settle down so that we can go to sleep again.

It just doesn't feel right, there has to be some other way to handle
stuff like this in a way that is known to terminate properly other than
just guessing.

greg k-h

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Arnd Bergmann]

On Tuesday 05 July 2011, Greg KH wrote:
> So the driver core is just going to sit and spin and continue to try to
> probe drivers for as long as it gets that error value returned?  What is
> going to ever cause that loop to terminate?  It seems a bit hacky to
> just keep looping over and over and hoping that sometime everything will
> all settle down so that we can go to sleep again.

Well, it only needs to try as long as there are still new devices
succeeding to get probed. The order that I think this should happen
in is:

* go through all initcalls, record any devices that are not yet ready
* retry all devices on the list as long as at least one of them has
  succeeded.
* when a new device gets matched from a module load, do that loop again

If I read the patch correctly, the workqueue would be scheduled
every time a new device gets added, which retries the devices
more often than necessary and can have significant boot time
impact, and it also introduces more asynchronicity that may expose
new bugs.

Maybe we can have a late_initcall that enables the automatic retry
and probes everything once:

static bool deferred_probe;
static int __init deferred_probe_start(void)
{
	deferred_probe = true;
	mutex_lock(&deferred_probe_mutex);
	if (!list_empty(&deferred_probe_list))
		schedule_work(&deferred_probe_work);
	mutex_unlock(&deferred_probe_mutex);
	flush_work_sync(&deferred_probe_work);
}
late_initcall(retry_devices);

	Arnd

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg KH]

On Tue, Jul 05, 2011 at 05:21:01PM +0200, Arnd Bergmann wrote:
> On Tuesday 05 July 2011, Greg KH wrote:
> > So the driver core is just going to sit and spin and continue to try to
> > probe drivers for as long as it gets that error value returned?  What is
> > going to ever cause that loop to terminate?  It seems a bit hacky to
> > just keep looping over and over and hoping that sometime everything will
> > all settle down so that we can go to sleep again.
> 
> Well, it only needs to try as long as there are still new devices
> succeeding to get probed. The order that I think this should happen
> in is:
> 
> * go through all initcalls, record any devices that are not yet ready
> * retry all devices on the list as long as at least one of them has
>   succeeded.
> * when a new device gets matched from a module load, do that loop again

You don't know when init calls are finished, or if a module is loaded,
the driver core isn't that smart at all.

> If I read the patch correctly, the workqueue would be scheduled
> every time a new device gets added, which retries the devices
> more often than necessary and can have significant boot time
> impact, and it also introduces more asynchronicity that may expose
> new bugs.
> 
> Maybe we can have a late_initcall that enables the automatic retry
> and probes everything once:
> 
> static bool deferred_probe;
> static int __init deferred_probe_start(void)
> {
> 	deferred_probe = true;
> 	mutex_lock(&deferred_probe_mutex);
> 	if (!list_empty(&deferred_probe_list))
> 		schedule_work(&deferred_probe_work);
> 	mutex_unlock(&deferred_probe_mutex);
> 	flush_work_sync(&deferred_probe_work);
> }
> late_initcall(retry_devices);

I wonder if doing this all from a workqueue in the first place is going
to cause problems as probe isn't normally done this way at all.

greg k-h

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Arnd Bergmann]

On Tuesday 05 July 2011, Greg KH wrote:
> > * go through all initcalls, record any devices that are not yet ready
> > * retry all devices on the list as long as at least one of them has
> >   succeeded.
> > * when a new device gets matched from a module load, do that loop again
> 
> You don't know when init calls are finished, or if a module is loaded,
> the driver core isn't that smart at all.

We know when most initcalls are finished (at late_initcall time), and
after that we don't need to know when a module gets loaded, only 
when a device gets matched, and that's something we do know and
that Grant's patch uses already.

> > late_initcall(retry_devices);
> 
> I wonder if doing this all from a workqueue in the first place is going
> to cause problems as probe isn't normally done this way at all.

True. It will definitely cause problems for any driver that calls
flush_work() in its probe function. We would need a private
singlethread_workqueue to avoid that.

	Arnd

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Tue, Jul 5, 2011 at 8:21 AM, Greg KH <gregkh@suse.de> wrote:
> On Mon, Jul 04, 2011 at 12:01:59PM -0600, Grant Likely wrote:
>> On Mon, Jul 04, 2011 at 10:41:26AM -0700, Greg KH wrote:
>> > On Mon, Jul 04, 2011 at 11:11:59AM -0600, Grant Likely wrote:
>> > > Allow drivers to report at probe time that they cannot get all the resources
>> > > required by the device, and should be retried at a later time.
>> >
>> > When is "later"?
>>
>> In this case, after at least one other device has successfully probed.
>> The 'later' is handled in a workqueue that walks the list
>> asynchronously from normal initialization.
>>
>> > And why would a driver not be able to get all of the proper resources?
>>
>> Discussed below...
>>
>> > Why can't a bus, at a later time, just try to reprobe everything when it
>> > determines that it is a "later" time now, without having to do this
>> > added change to the core?
>>
>> It can't be done by a specific bus type because it has zero
>> relationship with the bus type.  For example, it is typical for an
>> SDHCI driver to require a GPIO line for the card detect switch, and
>> the device cannot be initialized until it has it.  However, the
>> bus_type that the SDHCI driver is attached to could be anything;
>> platform_bus_type, pci, amba, etc.  It isn't a bus_type deficiency,
>> but rather that the driver core has no way to gracefully handle
>> devices that get probed in an undetermined order.
>>
>> It has to be done at the core level because any device in the system,
>> regardless of bus_type, may require another device to be probed first.
>> Originally I tried modifying the drivers to successfully probe
>> anyway and then 'go to sleep' to try again later, but it turned out to
>> push a lot of complexity into the device drivers when it can be solved
>> far more simply if the driver core has the ability to retry drivers
>> that request it.
>
> So the driver core is just going to sit and spin and continue to try to
> probe drivers for as long as it gets that error value returned?  What is
> going to ever cause that loop to terminate?  It seems a bit hacky to
> just keep looping over and over and hoping that sometime everything will
> all settle down so that we can go to sleep again.

No, that would be insane.  The list of deferred devices is only walked
when triggered, and it stops at the end of the list unless retriggered
again.  Also, for the drivers using this, reattempting probe is cheap
since the driver is expected to test resources first before
initializing hardware.  If it can't get what it needs, it returns
-EAGAIN immediately.  We /could/ implement dependency checking in the
core code instead of calling each driver's probe, but that would mean
teaching the driver core about every possible resource a driver might
want which would be absolutely horrible to code, would probably be
more expensive, and definitely more complex.

in this version of the patch there is no way for a driver to be
dropped from the deferred list and the list walk is quite inefficient.
 The next version will solve both problems by removing each device
from the list when the list is walked.  If a driver requests deferral
again, then the device gets re-added to the end.

Another problem with this version is that every successful probe will
trigger walking the list because that was the obvious way to implement
it.  So, it is theoretically possible that the list will get walked
once every single time a driver is bound (but probably not in practice
because the list walk is handled in a workqueue that is naturally
serialized).  I'm looking at changing how the workqueue is scheduled
so that it only gets kicked at the end of a block of driver probing
(like at the end of syscalls), but that is more of an optimization
than anything.

g.


>
> It just doesn't feel right, there has to be some other way to handle
> stuff like this in a way that is known to terminate properly other than
> just guessing.
>
> greg k-h
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Kay Sievers]

On Tue, Jul 5, 2011 at 17:50, Greg KH <gregkh@suse.de> wrote:

> I wonder if doing this all from a workqueue in the first place is going
> to cause problems as probe isn't normally done this way at all.

Yeah, I would expect unforeseen problems with the async thread too.
It's probably all solvable, but it sounds troublesome to find out if
things go wrong.

We have sync hooks (BUS_NOTIFY_*) where any kind of code can subscribe
to when devices get added or get bound to a driver. Can't the code
that relies on later hookups to already existing devices/bindings not
just plug into that?

Thanks,
Kay

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Tue, Jul 5, 2011 at 10:05 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 05 July 2011, Greg KH wrote:
>> > * go through all initcalls, record any devices that are not yet ready
>> > * retry all devices on the list as long as at least one of them has
>> >   succeeded.
>> > * when a new device gets matched from a module load, do that loop again
>>
>> You don't know when init calls are finished, or if a module is loaded,
>> the driver core isn't that smart at all.
>
> We know when most initcalls are finished (at late_initcall time), and
> after that we don't need to know when a module gets loaded, only
> when a device gets matched, and that's something we do know and
> that Grant's patch uses already.

Yes, I'm thinking about doing exactly that.

>> > late_initcall(retry_devices);
>>
>> I wonder if doing this all from a workqueue in the first place is going
>> to cause problems as probe isn't normally done this way at all.

Problems with drivers or problems with the driver core?  It shouldn't
be a problem for drivers since only drivers that have explicitly
requested deferral will end up with the device added to the deferral
list.

> True. It will definitely cause problems for any driver that calls
> flush_work() in its probe function. We would need a private
> singlethread_workqueue to avoid that.

Ah, good point.  That's easy enough to do.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Tue, Jul 5, 2011 at 10:11 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Tue, Jul 5, 2011 at 17:50, Greg KH <gregkh@suse.de> wrote:
>
>> I wonder if doing this all from a workqueue in the first place is going
>> to cause problems as probe isn't normally done this way at all.
>
> Yeah, I would expect unforeseen problems with the async thread too.
> It's probably all solvable, but it sounds troublesome to find out if
> things go wrong.
>
> We have sync hooks (BUS_NOTIFY_*) where any kind of code can subscribe
> to when devices get added or get bound to a driver. Can't the code
> that relies on later hookups to already existing devices/bindings not
> just plug into that?

I tried that.  It resulted in a lot of complexity that each driver
needs to implement correctly which is why I started looking for a
different way to go about it.

g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Tue, Jul 5, 2011 at 10:11 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Tue, Jul 5, 2011 at 17:50, Greg KH <gregkh@suse.de> wrote:
>
>> I wonder if doing this all from a workqueue in the first place is going
>> to cause problems as probe isn't normally done this way at all.
>
> Yeah, I would expect unforeseen problems with the async thread too.
> It's probably all solvable, but it sounds troublesome to find out if
> things go wrong.

I think it is worth solving though since the feature is important.  I
can certainly block out the deferral code with a Kconfig symbol so
that it only gets built when a driver selects CONFIG_PROBE_DEFERRAL.
That would keep the code out of x86 kernels while us embedded folks
iron out the bugs.

g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg KH]

On Tue, Jul 05, 2011 at 10:28:37AM -0600, Grant Likely wrote:
> On Tue, Jul 5, 2011 at 10:11 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> > On Tue, Jul 5, 2011 at 17:50, Greg KH <gregkh@suse.de> wrote:
> >
> >> I wonder if doing this all from a workqueue in the first place is going
> >> to cause problems as probe isn't normally done this way at all.
> >
> > Yeah, I would expect unforeseen problems with the async thread too.
> > It's probably all solvable, but it sounds troublesome to find out if
> > things go wrong.
> >
> > We have sync hooks (BUS_NOTIFY_*) where any kind of code can subscribe
> > to when devices get added or get bound to a driver. Can't the code
> > that relies on later hookups to already existing devices/bindings not
> > just plug into that?
> 
> I tried that.  It resulted in a lot of complexity that each driver
> needs to implement correctly which is why I started looking for a
> different way to go about it.

No, the bus that wants this just has to do it, not the drivers
themselves, right?

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Tue, Jul 5, 2011 at 10:36 AM, Greg KH <gregkh@suse.de> wrote:
> On Tue, Jul 05, 2011 at 10:28:37AM -0600, Grant Likely wrote:
>> On Tue, Jul 5, 2011 at 10:11 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> > On Tue, Jul 5, 2011 at 17:50, Greg KH <gregkh@suse.de> wrote:
>> >
>> >> I wonder if doing this all from a workqueue in the first place is going
>> >> to cause problems as probe isn't normally done this way at all.
>> >
>> > Yeah, I would expect unforeseen problems with the async thread too.
>> > It's probably all solvable, but it sounds troublesome to find out if
>> > things go wrong.
>> >
>> > We have sync hooks (BUS_NOTIFY_*) where any kind of code can subscribe
>> > to when devices get added or get bound to a driver. Can't the code
>> > that relies on later hookups to already existing devices/bindings not
>> > just plug into that?
>>
>> I tried that.  It resulted in a lot of complexity that each driver
>> needs to implement correctly which is why I started looking for a
>> different way to go about it.
>
> No, the bus that wants this just has to do it, not the drivers
> themselves, right?

It's not about the bus_type, and there is nothing that the bus can do
to solve this problem because it the dependencies are completely
orthogonal to the bus.  ie. what does an i2c bus know about the audio
path to a codec?  The problem must be solved at the driver scope.

g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg KH]

On Tue, Jul 05, 2011 at 11:17:46AM -0600, Grant Likely wrote:
> On Tue, Jul 5, 2011 at 10:36 AM, Greg KH <gregkh@suse.de> wrote:
> > On Tue, Jul 05, 2011 at 10:28:37AM -0600, Grant Likely wrote:
> >> On Tue, Jul 5, 2011 at 10:11 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> >> > On Tue, Jul 5, 2011 at 17:50, Greg KH <gregkh@suse.de> wrote:
> >> >
> >> >> I wonder if doing this all from a workqueue in the first place is going
> >> >> to cause problems as probe isn't normally done this way at all.
> >> >
> >> > Yeah, I would expect unforeseen problems with the async thread too.
> >> > It's probably all solvable, but it sounds troublesome to find out if
> >> > things go wrong.
> >> >
> >> > We have sync hooks (BUS_NOTIFY_*) where any kind of code can subscribe
> >> > to when devices get added or get bound to a driver. Can't the code
> >> > that relies on later hookups to already existing devices/bindings not
> >> > just plug into that?
> >>
> >> I tried that.  It resulted in a lot of complexity that each driver
> >> needs to implement correctly which is why I started looking for a
> >> different way to go about it.
> >
> > No, the bus that wants this just has to do it, not the drivers
> > themselves, right?
> 
> It's not about the bus_type, and there is nothing that the bus can do
> to solve this problem because it the dependencies are completely
> orthogonal to the bus.  ie. what does an i2c bus know about the audio
> path to a codec?  The problem must be solved at the driver scope.

Ok, let's look at your next implementation and see how it goes.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Tue, Jul 5, 2011 at 11:29 AM, Greg KH <gregkh@suse.de> wrote:
> On Tue, Jul 05, 2011 at 11:17:46AM -0600, Grant Likely wrote:
>> On Tue, Jul 5, 2011 at 10:36 AM, Greg KH <gregkh@suse.de> wrote:
>> > On Tue, Jul 05, 2011 at 10:28:37AM -0600, Grant Likely wrote:
>> >> On Tue, Jul 5, 2011 at 10:11 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> >> > On Tue, Jul 5, 2011 at 17:50, Greg KH <gregkh@suse.de> wrote:
>> >> >
>> >> >> I wonder if doing this all from a workqueue in the first place is going
>> >> >> to cause problems as probe isn't normally done this way at all.
>> >> >
>> >> > Yeah, I would expect unforeseen problems with the async thread too.
>> >> > It's probably all solvable, but it sounds troublesome to find out if
>> >> > things go wrong.
>> >> >
>> >> > We have sync hooks (BUS_NOTIFY_*) where any kind of code can subscribe
>> >> > to when devices get added or get bound to a driver. Can't the code
>> >> > that relies on later hookups to already existing devices/bindings not
>> >> > just plug into that?
>> >>
>> >> I tried that.  It resulted in a lot of complexity that each driver
>> >> needs to implement correctly which is why I started looking for a
>> >> different way to go about it.
>> >
>> > No, the bus that wants this just has to do it, not the drivers
>> > themselves, right?
>>
>> It's not about the bus_type, and there is nothing that the bus can do
>> to solve this problem because it the dependencies are completely
>> orthogonal to the bus.  ie. what does an i2c bus know about the audio
>> path to a codec?  The problem must be solved at the driver scope.
>
> Ok, let's look at your next implementation and see how it goes.

Okay, thanks.  I'll get it posted soon.

g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Kay Sievers]

On Tue, Jul 5, 2011 at 18:28, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue, Jul 5, 2011 at 10:11 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> On Tue, Jul 5, 2011 at 17:50, Greg KH <gregkh@suse.de> wrote:
>>
>>> I wonder if doing this all from a workqueue in the first place is going
>>> to cause problems as probe isn't normally done this way at all.
>>
>> Yeah, I would expect unforeseen problems with the async thread too.
>> It's probably all solvable, but it sounds troublesome to find out if
>> things go wrong.
>>
>> We have sync hooks (BUS_NOTIFY_*) where any kind of code can subscribe
>> to when devices get added or get bound to a driver. Can't the code
>> that relies on later hookups to already existing devices/bindings not
>> just plug into that?
>
> I tried that.  It resulted in a lot of complexity that each driver
> needs to implement correctly which is why I started looking for a
> different way to go about it.

What I mean is that we might not necessarily need to have that in
'struct device' and the driver-binding code. Can't we just allocate
the reprobe-in-the-back logic in its own object, let this object hook
into the existing notifier chain (if needed add more notifiers) and
have your reprobe object itself check for the device, carry its own
list of devices and trigger the needed work?

(As a general note, not specifically related to this patch: The
current 'struct device' is a total mess already, and really needs to
be cleaned up. The list.h way of doing lists might perform nice in
some always-be-a-list-member used cases, but it is really not the
appropriate way of doing non-common functionality on common objects:
performance does not matter at all here, and the object-to-add itself
has to carry the list-data needed for every non-common list.)

Kay

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg KH]

On Mon, Mar 05, 2012 at 08:47:41AM -0700, Grant Likely wrote:
> Allow drivers to report at probe time that they cannot get all the resources
> required by the device, and should be retried at a later time.
> 
> This should completely solve the problem of getting devices
> initialized in the right order.  Right now this is mostly handled by
> mucking about with initcall ordering which is a complete hack, and
> doesn't even remotely handle the case where device drivers are in
> modules.  This approach completely sidesteps the issues by allowing
> driver registration to occur in any order, and any driver can request
> to be retried after a few more other drivers get probed.
> 
> v4: - Integrate Manjunath's addition of a separate workqueue
>     - Change -EAGAIN to -EPROBE_DEFER for drivers to trigger deferral
>     - Update comment blocks to reflect how the code really works
> v3: - Hold off workqueue scheduling until late_initcall so that the bulk
>       of driver probes are complete before we start retrying deferred devices.
>     - Tested with simple use cases.  Still needs more testing though.
>       Using it to get rid of the gpio early_initcall madness, or to replace
>       the ASoC internal probe deferral code would be ideal.
> v2: - added locking so it should no longer be utterly broken in that regard
>     - remove device from deferred list at device_del time.
>     - Still completely untested with any real use case, but has been
>       boot tested.

Now applied, thanks for pushing this and seeing it through.

greg k-h

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Arnd Bergmann]

On Monday 05 March 2012, Grant Likely wrote:
> >
> > Also, What protects the device from going away between being put on the list
> > and taken off of it? Don't you have to do the device_get during
> > driver_deferred_probe_add()?
> 
> The deferred_probe_mutex.  Nothing can be removed from the deferred
> list without holding the deferred_probe_mutex, and no device can be
> freed before it is taken off the deferred list.  If the device is in
> the process of being removed, then get_device() occurs before dropping
> the mutex to protect against freeing, and bus_probe_device() won't
> attach a driver to a device getting removed.

Ok, got it now. So there were actually three bugs that I found that
turned out to be correct in the end. This also means that if you
want to change to making deferred_probe_active_list local
to the work function and not locked, you actually will need to
get the reference count on the device as soon as you put it on
the pending list. Since that would be a major design change with
little benefit, I agree that you should not change it from the tested
version for now.

	Arnd

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Mon, Mar 5, 2012 at 10:28 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Mar 05, 2012 at 05:08:30PM -0700, Grant Likely wrote:
>> On Mon, Mar 5, 2012 at 3:15 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Mar 05, 2012 at 03:09:27PM -0700, Grant Likely wrote:
>> >> On Mon, Mar 5, 2012 at 2:47 PM, Greg Kroah-Hartman
>> >> <gregkh@linuxfoundation.org> wrote:
>> >> > On Mon, Mar 05, 2012 at 08:47:41AM -0700, Grant Likely wrote:
>> >> >> --- a/include/linux/device.h
>> >> >> +++ b/include/linux/device.h
>> >> >> @@ -587,6 +587,10 @@ struct device_dma_parameters {
>> >> >>   * @mutex:   Mutex to synchronize calls to its driver.
>> >> >>   * @bus:     Type of bus device is on.
>> >> >>   * @driver:  Which driver has allocated this
>> >> >> + * @deferred_probe: entry in deferred_probe_list which is used to retry the
>> >> >> + *           binding of drivers which were unable to get all the resources
>> >> >> + *           needed by the device; typically because it depends on another
>> >> >> + *           driver getting probed first.
>> >> >>   * @platform_data: Platform data specific to the device.
>> >> >>   *           Example: For devices on custom boards, as typical of embedded
>> >> >>   *           and SOC based hardware, Linux often uses platform_data to point
>> >> >> @@ -646,6 +650,7 @@ struct device {
>> >> >>       struct bus_type *bus;           /* type of bus device is on */
>> >> >>       struct device_driver *driver;   /* which driver has allocated this
>> >> >>                                          device */
>> >> >> +     struct list_head        deferred_probe;
>> >> >>       void            *platform_data; /* Platform specific data, device
>> >> >>                                          core doesn't touch it */
>> >> >>       struct dev_pm_info      power;
>> >> >
>> >> > This can go into the "struct device_private" structure instead, right?
>> >> > That would be better to ensure that no non-driver-core code ever touches
>> >> > this thing.
>> >>
>> >> I don't see any reason why not.  I'll make the change and repost.
>> >
>> > If you are going to repost, care to fix up your multi-line comment
>> > blocks to follow the "standard" way of doing it?
>> >
>> > That saves me doing the follow-on patch to do it myself :)
>>
>> Yeah, I can do that.
>>
>> Actually, if you're okay with this version other than the private data
>> change, could you pick up this version (it's actually received
>> testing) and I'll provide follow up patches to fix up the issues
>> pointed out by you and Arnd.
>
> Yes, I can, and I might as well fix up the private thing myself and the
> comments when I do that, I'll cc: you on that.  I have a flight tomorrow
> so I might get the chance to do it then.

Alright; that works for me.  :-)

g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg Kroah-Hartman]

On Mon, Mar 05, 2012 at 05:08:30PM -0700, Grant Likely wrote:
> On Mon, Mar 5, 2012 at 3:15 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Mar 05, 2012 at 03:09:27PM -0700, Grant Likely wrote:
> >> On Mon, Mar 5, 2012 at 2:47 PM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > On Mon, Mar 05, 2012 at 08:47:41AM -0700, Grant Likely wrote:
> >> >> --- a/include/linux/device.h
> >> >> +++ b/include/linux/device.h
> >> >> @@ -587,6 +587,10 @@ struct device_dma_parameters {
> >> >>   * @mutex:   Mutex to synchronize calls to its driver.
> >> >>   * @bus:     Type of bus device is on.
> >> >>   * @driver:  Which driver has allocated this
> >> >> + * @deferred_probe: entry in deferred_probe_list which is used to retry the
> >> >> + *           binding of drivers which were unable to get all the resources
> >> >> + *           needed by the device; typically because it depends on another
> >> >> + *           driver getting probed first.
> >> >>   * @platform_data: Platform data specific to the device.
> >> >>   *           Example: For devices on custom boards, as typical of embedded
> >> >>   *           and SOC based hardware, Linux often uses platform_data to point
> >> >> @@ -646,6 +650,7 @@ struct device {
> >> >>       struct bus_type *bus;           /* type of bus device is on */
> >> >>       struct device_driver *driver;   /* which driver has allocated this
> >> >>                                          device */
> >> >> +     struct list_head        deferred_probe;
> >> >>       void            *platform_data; /* Platform specific data, device
> >> >>                                          core doesn't touch it */
> >> >>       struct dev_pm_info      power;
> >> >
> >> > This can go into the "struct device_private" structure instead, right?
> >> > That would be better to ensure that no non-driver-core code ever touches
> >> > this thing.
> >>
> >> I don't see any reason why not.  I'll make the change and repost.
> >
> > If you are going to repost, care to fix up your multi-line comment
> > blocks to follow the "standard" way of doing it?
> >
> > That saves me doing the follow-on patch to do it myself :)
> 
> Yeah, I can do that.
> 
> Actually, if you're okay with this version other than the private data
> change, could you pick up this version (it's actually received
> testing) and I'll provide follow up patches to fix up the issues
> pointed out by you and Arnd.

Yes, I can, and I might as well fix up the private thing myself and the
comments when I do that, I'll cc: you on that.  I have a flight tomorrow
so I might get the chance to do it then.

thanks,

greg k-h

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Mon, Mar 5, 2012 at 3:15 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Mar 05, 2012 at 03:09:27PM -0700, Grant Likely wrote:
>> On Mon, Mar 5, 2012 at 2:47 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Mar 05, 2012 at 08:47:41AM -0700, Grant Likely wrote:
>> >> --- a/include/linux/device.h
>> >> +++ b/include/linux/device.h
>> >> @@ -587,6 +587,10 @@ struct device_dma_parameters {
>> >>   * @mutex:   Mutex to synchronize calls to its driver.
>> >>   * @bus:     Type of bus device is on.
>> >>   * @driver:  Which driver has allocated this
>> >> + * @deferred_probe: entry in deferred_probe_list which is used to retry the
>> >> + *           binding of drivers which were unable to get all the resources
>> >> + *           needed by the device; typically because it depends on another
>> >> + *           driver getting probed first.
>> >>   * @platform_data: Platform data specific to the device.
>> >>   *           Example: For devices on custom boards, as typical of embedded
>> >>   *           and SOC based hardware, Linux often uses platform_data to point
>> >> @@ -646,6 +650,7 @@ struct device {
>> >>       struct bus_type *bus;           /* type of bus device is on */
>> >>       struct device_driver *driver;   /* which driver has allocated this
>> >>                                          device */
>> >> +     struct list_head        deferred_probe;
>> >>       void            *platform_data; /* Platform specific data, device
>> >>                                          core doesn't touch it */
>> >>       struct dev_pm_info      power;
>> >
>> > This can go into the "struct device_private" structure instead, right?
>> > That would be better to ensure that no non-driver-core code ever touches
>> > this thing.
>>
>> I don't see any reason why not.  I'll make the change and repost.
>
> If you are going to repost, care to fix up your multi-line comment
> blocks to follow the "standard" way of doing it?
>
> That saves me doing the follow-on patch to do it myself :)

Yeah, I can do that.

Actually, if you're okay with this version other than the private data
change, could you pick up this version (it's actually received
testing) and I'll provide follow up patches to fix up the issues
pointed out by you and Arnd.

Thanks,
g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg Kroah-Hartman]

On Mon, Mar 05, 2012 at 03:09:27PM -0700, Grant Likely wrote:
> On Mon, Mar 5, 2012 at 2:47 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Mar 05, 2012 at 08:47:41AM -0700, Grant Likely wrote:
> >> --- a/include/linux/device.h
> >> +++ b/include/linux/device.h
> >> @@ -587,6 +587,10 @@ struct device_dma_parameters {
> >>   * @mutex:   Mutex to synchronize calls to its driver.
> >>   * @bus:     Type of bus device is on.
> >>   * @driver:  Which driver has allocated this
> >> + * @deferred_probe: entry in deferred_probe_list which is used to retry the
> >> + *           binding of drivers which were unable to get all the resources
> >> + *           needed by the device; typically because it depends on another
> >> + *           driver getting probed first.
> >>   * @platform_data: Platform data specific to the device.
> >>   *           Example: For devices on custom boards, as typical of embedded
> >>   *           and SOC based hardware, Linux often uses platform_data to point
> >> @@ -646,6 +650,7 @@ struct device {
> >>       struct bus_type *bus;           /* type of bus device is on */
> >>       struct device_driver *driver;   /* which driver has allocated this
> >>                                          device */
> >> +     struct list_head        deferred_probe;
> >>       void            *platform_data; /* Platform specific data, device
> >>                                          core doesn't touch it */
> >>       struct dev_pm_info      power;
> >
> > This can go into the "struct device_private" structure instead, right?
> > That would be better to ensure that no non-driver-core code ever touches
> > this thing.
> 
> I don't see any reason why not.  I'll make the change and repost.

If you are going to repost, care to fix up your multi-line comment
blocks to follow the "standard" way of doing it?

That saves me doing the follow-on patch to do it myself :)

thanks,

greg k-h

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Mon, Mar 5, 2012 at 2:47 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Mar 05, 2012 at 08:47:41AM -0700, Grant Likely wrote:
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -587,6 +587,10 @@ struct device_dma_parameters {
>>   * @mutex:   Mutex to synchronize calls to its driver.
>>   * @bus:     Type of bus device is on.
>>   * @driver:  Which driver has allocated this
>> + * @deferred_probe: entry in deferred_probe_list which is used to retry the
>> + *           binding of drivers which were unable to get all the resources
>> + *           needed by the device; typically because it depends on another
>> + *           driver getting probed first.
>>   * @platform_data: Platform data specific to the device.
>>   *           Example: For devices on custom boards, as typical of embedded
>>   *           and SOC based hardware, Linux often uses platform_data to point
>> @@ -646,6 +650,7 @@ struct device {
>>       struct bus_type *bus;           /* type of bus device is on */
>>       struct device_driver *driver;   /* which driver has allocated this
>>                                          device */
>> +     struct list_head        deferred_probe;
>>       void            *platform_data; /* Platform specific data, device
>>                                          core doesn't touch it */
>>       struct dev_pm_info      power;
>
> This can go into the "struct device_private" structure instead, right?
> That would be better to ensure that no non-driver-core code ever touches
> this thing.

I don't see any reason why not.  I'll make the change and repost.

g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg Kroah-Hartman]

On Mon, Mar 05, 2012 at 08:47:41AM -0700, Grant Likely wrote:
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -587,6 +587,10 @@ struct device_dma_parameters {
>   * @mutex:	Mutex to synchronize calls to its driver.
>   * @bus:	Type of bus device is on.
>   * @driver:	Which driver has allocated this
> + * @deferred_probe: entry in deferred_probe_list which is used to retry the
> + * 		binding of drivers which were unable to get all the resources
> + * 		needed by the device; typically because it depends on another
> + * 		driver getting probed first.
>   * @platform_data: Platform data specific to the device.
>   * 		Example: For devices on custom boards, as typical of embedded
>   * 		and SOC based hardware, Linux often uses platform_data to point
> @@ -646,6 +650,7 @@ struct device {
>  	struct bus_type	*bus;		/* type of bus device is on */
>  	struct device_driver *driver;	/* which driver has allocated this
>  					   device */
> +	struct list_head	deferred_probe;
>  	void		*platform_data;	/* Platform specific data, device
>  					   core doesn't touch it */
>  	struct dev_pm_info	power;

This can go into the "struct device_private" structure instead, right?
That would be better to ensure that no non-driver-core code ever touches
this thing.

thanks,

greg k-h

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Mark Brown]

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

On Mon, Mar 05, 2012 at 01:24:42PM -0800, Greg Kroah-Hartman wrote:

> Or, as this really is a bus-specific thing, have you converted any
> busses to use it?  If so, I'd like to take that patch at the same time,
> I'm leary of infrastructure changes without any users.

It's not bus specific, it's more subsystem and/or driver specific.
There was a patch to the regulator core which would add at least one
user.

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

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Greg Kroah-Hartman]

On Mon, Mar 05, 2012 at 02:10:52PM -0700, Grant Likely wrote:
> > I think "deferwq" is not a good name for a global thread: all work queues are
> > there for deferring somehting. How about "deferredprobe"?
> 
> Sure; again for a follow-on patch if Greg is okay with this version.

It looks good to me.

But, we need some documentation somewhere that we can point people to in
order to show how to use this.

Or, as this really is a bus-specific thing, have you converted any
busses to use it?  If so, I'd like to take that patch at the same time,
I'm leary of infrastructure changes without any users.

thanks,

greg k-h

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Mon, Mar 5, 2012 at 12:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 05 March 2012, Grant Likely wrote:
>> Allow drivers to report at probe time that they cannot get all the resources
>> required by the device, and should be retried at a later time.
>>
>> This should completely solve the problem of getting devices
>> initialized in the right order.  Right now this is mostly handled by
>> mucking about with initcall ordering which is a complete hack, and
>> doesn't even remotely handle the case where device drivers are in
>> modules.  This approach completely sidesteps the issues by allowing
>> driver registration to occur in any order, and any driver can request
>> to be retried after a few more other drivers get probed.
>
> Hi Grant,
>
> Looks great! I thought I had found two bugs but it turned out to
> all be correct on second look.  What remains in my review is basically
> bike-shedding, but I'll send it anyway since I took the time to
> write it before I noticed I was wrong on the other points ;-)
>
> Anyway, I'm happy for this to go in in the current way,

Cool, thanks.  I've responded below even though some of the items are
the non-existent bugs you mentioned.  :-)

>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>
>> @@ -28,6 +28,133 @@
>>  #include "base.h"
>>  #include "power/power.h"
>>
>> +/*
>> + * Deferred Probe infrastructure.
>> + *
>> + * Sometimes driver probe order matters, but the kernel doesn't always have
>> + * dependency information which means some drivers will get probed before a
>> + * resource it depends on is available.  For example, an SDHCI driver may
>> + * first need a GPIO line from an i2c GPIO controller before it can be
>> + * initialized.  If a required resource is not available yet, a driver can
>> + * request probing to be deferred by returning -EPROBE_DEFER from its probe hook
>> + *
>> + * Deferred probe maintains two lists of devices, a pending list and an active
>> + * list.  A driver returning -EPROBE_DEFER causes the device to be added to the
>> + * pending list.  A successful driver probe will trigger moving all devices
>> + * from the pending to the active list so that the workqueue will eventually
>> + * retry them.
>> + *
>> + * The deferred_probe_mutex must be held any time the deferred_probe_*_list
>> + * of the (struct device*)->deferred_probe pointers are manipulated
>> + */
>> +static DEFINE_MUTEX(deferred_probe_mutex);
>> +static LIST_HEAD(deferred_probe_pending_list);
>> +static LIST_HEAD(deferred_probe_active_list);
>> +static struct workqueue_struct *deferred_wq;
>
> I don't understand why you want both lists to be global, it seems to
> complicate things.
>
>> +/**
>> + * deferred_probe_work_func() - Retry probing devices in the active list.
>> + */
>> +static void deferred_probe_work_func(struct work_struct *work)
>> +{
>> +     struct device *dev;
>> +     /*
>> +      * This block processes every device in the deferred 'active' list.
>> +      * Each device is removed from the active list and passed to
>> +      * bus_probe_device() to re-attempt the probe.  The loop continues
>> +      * until every device in the active list is removed and retried.
>> +      *
>> +      * Note: Once the device is removed from the list and the mutex is
>> +      * released, it is possible for the device get freed by another thread
>> +      * and cause a illegal pointer dereference.  This code uses
>> +      * get/put_device() to ensure the device structure cannot disappear
>> +      * from under our feet.
>> +      */
>> +     mutex_lock(&deferred_probe_mutex);
>> +     while (!list_empty(&deferred_probe_active_list)) {
>> +             dev = list_first_entry(&deferred_probe_active_list,
>> +                                     typeof(*dev), deferred_probe);
>> +             list_del_init(&dev->deferred_probe);
>> +
>> +             get_device(dev);
>> +
>> +             /* Drop the mutex while probing each device; the probe path
>> +              * may manipulate the deferred list */
>> +             mutex_unlock(&deferred_probe_mutex);
>> +             dev_dbg(dev, "Retrying from deferred list\n");
>> +             bus_probe_device(dev);
>> +             mutex_lock(&deferred_probe_mutex);
>> +
>> +             put_device(dev);
>> +     }
>> +     mutex_unlock(&deferred_probe_mutex);
>
>
> If you make the deferred_probe_active_list local to this function, and do
> the splice inside of it, you only need to hold the mutex for the
> splice, and the loop can become a simpler

Hmmm; that is true.  My original though was that new devices can be
moved to the pending list at any time; even while the work function is
running.  That means that the active list is always up to date with
items that should be retried immediately.  However, if I moved the
active list to be local into this function then it means the work
function needs to exit and reenter if a new trigger occurs part way
through the loop.  However, the kick of the workqueue guarantees that
it will happen anyway so all is still good.  Regardless, this is the
tested version; I'd like to get this merged and use a follow-on patch
to try it out the other way.

>
>        LIST_HEAD(list);
>
>        mutex_lock(&deferred_probe_mutex);
>        list_splice_tail_init(&deferred_probe_pending_list, &list);
>        mutex_unlock(&deferred_probe_mutex);
>
>        list_for_each_entry_safe(...) {
>                list_del_init(&dev->deferred_probe);
>                bus_probe_device(dev);
>                put_device(dev);
>        }
>
> Also, What protects the device from going away between being put on the list
> and taken off of it? Don't you have to do the device_get during
> driver_deferred_probe_add()?

The deferred_probe_mutex.  Nothing can be removed from the deferred
list without holding the deferred_probe_mutex, and no device can be
freed before it is taken off the deferred list.  If the device is in
the process of being removed, then get_device() occurs before dropping
the mutex to protect against freeing, and bus_probe_device() won't
attach a driver to a device getting removed.

>
>> +static bool driver_deferred_probe_enable = false;
>> +/**
>> + * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
>> + *
>> + * This functions moves all devices from the pending list to the active
>> + * list and schedules the deferred probe workqueue to process them.  It
>> + * should be called anytime a driver is successfully bound to a device.
>> + */
>> +static void driver_deferred_probe_trigger(void)
>> +{
>> +     if (!driver_deferred_probe_enable)
>> +             return;
>
> I tried to understand whether you need to have locking around
> driver_deferred_probe_enable, but I think you don't even need this
> variable at all:

The only purpose to this variable is to hold off deferred probing
until after all the initcalls have completed.

>
>> +
>> +     /* A successful probe means that all the devices in the pending list
>> +      * should be triggered to be reprobed.  Move all the deferred devices
>> +      * into the active list so they can be retried by the workqueue */
>> +     mutex_lock(&deferred_probe_mutex);
>> +     list_splice_tail_init(&deferred_probe_pending_list,
>> +                           &deferred_probe_active_list);
>> +     mutex_unlock(&deferred_probe_mutex);
>> +
>> +     /* Kick the re-probe thread.  It may already be scheduled, but
>> +      * it is safe to kick it again. */
>> +     queue_work(deferred_wq, &deferred_probe_work);
>> +}
>
> You can simply check whether deferred_wq is non-NULL here before you call it,
> because it never goes away after it has been created.

Ah, true.  Alright, that can be a follow-on patch too.

>
>> +static int deferred_probe_initcall(void)
>> +{
>> +     deferred_wq = create_singlethread_workqueue("deferwq");
>> +     if (WARN_ON(!deferred_wq))
>> +             return -ENOMEM;
>
> I think "deferwq" is not a good name for a global thread: all work queues are
> there for deferring somehting. How about "deferredprobe"?

Sure; again for a follow-on patch if Greg is okay with this version.

g.

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Arnd Bergmann]

On Monday 05 March 2012, Grant Likely wrote:
> Allow drivers to report at probe time that they cannot get all the resources
> required by the device, and should be retried at a later time.
> 
> This should completely solve the problem of getting devices
> initialized in the right order.  Right now this is mostly handled by
> mucking about with initcall ordering which is a complete hack, and
> doesn't even remotely handle the case where device drivers are in
> modules.  This approach completely sidesteps the issues by allowing
> driver registration to occur in any order, and any driver can request
> to be retried after a few more other drivers get probed.

Hi Grant,

Looks great! I thought I had found two bugs but it turned out to
all be correct on second look.  What remains in my review is basically
bike-shedding, but I'll send it anyway since I took the time to
write it before I noticed I was wrong on the other points ;-)

Anyway, I'm happy for this to go in in the current way,

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> @@ -28,6 +28,133 @@
>  #include "base.h"
>  #include "power/power.h"
>  
> +/*
> + * Deferred Probe infrastructure.
> + *
> + * Sometimes driver probe order matters, but the kernel doesn't always have
> + * dependency information which means some drivers will get probed before a
> + * resource it depends on is available.  For example, an SDHCI driver may
> + * first need a GPIO line from an i2c GPIO controller before it can be
> + * initialized.  If a required resource is not available yet, a driver can
> + * request probing to be deferred by returning -EPROBE_DEFER from its probe hook
> + *
> + * Deferred probe maintains two lists of devices, a pending list and an active
> + * list.  A driver returning -EPROBE_DEFER causes the device to be added to the
> + * pending list.  A successful driver probe will trigger moving all devices
> + * from the pending to the active list so that the workqueue will eventually
> + * retry them.
> + *
> + * The deferred_probe_mutex must be held any time the deferred_probe_*_list
> + * of the (struct device*)->deferred_probe pointers are manipulated
> + */
> +static DEFINE_MUTEX(deferred_probe_mutex);
> +static LIST_HEAD(deferred_probe_pending_list);
> +static LIST_HEAD(deferred_probe_active_list);
> +static struct workqueue_struct *deferred_wq;

I don't understand why you want both lists to be global, it seems to
complicate things.

> +/**
> + * deferred_probe_work_func() - Retry probing devices in the active list.
> + */
> +static void deferred_probe_work_func(struct work_struct *work)
> +{
> +	struct device *dev;
> +	/*
> +	 * This block processes every device in the deferred 'active' list.
> +	 * Each device is removed from the active list and passed to
> +	 * bus_probe_device() to re-attempt the probe.  The loop continues
> +	 * until every device in the active list is removed and retried.
> +	 *
> +	 * Note: Once the device is removed from the list and the mutex is
> +	 * released, it is possible for the device get freed by another thread
> +	 * and cause a illegal pointer dereference.  This code uses
> +	 * get/put_device() to ensure the device structure cannot disappear
> +	 * from under our feet.
> +	 */
> +	mutex_lock(&deferred_probe_mutex);
> +	while (!list_empty(&deferred_probe_active_list)) {
> +		dev = list_first_entry(&deferred_probe_active_list,
> +					typeof(*dev), deferred_probe);
> +		list_del_init(&dev->deferred_probe);
> +
> +		get_device(dev);
> +
> +		/* Drop the mutex while probing each device; the probe path
> +		 * may manipulate the deferred list */
> +		mutex_unlock(&deferred_probe_mutex);
> +		dev_dbg(dev, "Retrying from deferred list\n");
> +		bus_probe_device(dev);
> +		mutex_lock(&deferred_probe_mutex);
> +
> +		put_device(dev);
> +	}
> +	mutex_unlock(&deferred_probe_mutex);


If you make the deferred_probe_active_list local to this function, and do
the splice inside of it, you only need to hold the mutex for the
splice, and the loop can become a simpler

	LIST_HEAD(list);

	mutex_lock(&deferred_probe_mutex);
	list_splice_tail_init(&deferred_probe_pending_list, &list);
	mutex_unlock(&deferred_probe_mutex);

	list_for_each_entry_safe(...) {
		list_del_init(&dev->deferred_probe);
		bus_probe_device(dev);
		put_device(dev);
	}

Also, What protects the device from going away between being put on the list
and taken off of it? Don't you have to do the device_get during
driver_deferred_probe_add()?

> +static bool driver_deferred_probe_enable = false;
> +/**
> + * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
> + *
> + * This functions moves all devices from the pending list to the active
> + * list and schedules the deferred probe workqueue to process them.  It
> + * should be called anytime a driver is successfully bound to a device.
> + */
> +static void driver_deferred_probe_trigger(void)
> +{
> +	if (!driver_deferred_probe_enable)
> +		return;

I tried to understand whether you need to have locking around
driver_deferred_probe_enable, but I think you don't even need this
variable at all:

> +
> +	/* A successful probe means that all the devices in the pending list
> +	 * should be triggered to be reprobed.  Move all the deferred devices
> +	 * into the active list so they can be retried by the workqueue */
> +	mutex_lock(&deferred_probe_mutex);
> +	list_splice_tail_init(&deferred_probe_pending_list,
> +			      &deferred_probe_active_list);
> +	mutex_unlock(&deferred_probe_mutex);
> +
> +	/* Kick the re-probe thread.  It may already be scheduled, but
> +	 * it is safe to kick it again. */
> +	queue_work(deferred_wq, &deferred_probe_work);
> +}

You can simply check whether deferred_wq is non-NULL here before you call it,
because it never goes away after it has been created.

> +static int deferred_probe_initcall(void)
> +{
> +	deferred_wq = create_singlethread_workqueue("deferwq");
> +	if (WARN_ON(!deferred_wq))
> +		return -ENOMEM;

I think "deferwq" is not a good name for a global thread: all work queues are
there for deferring somehting. How about "deferredprobe"?

	Arnd

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Mark Brown]

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

On Mon, Mar 05, 2012 at 08:47:41AM -0700, Grant Likely wrote:
> Allow drivers to report at probe time that they cannot get all the resources
> required by the device, and should be retried at a later time.

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

In my copious, copious free time I'll try to replace the ASoC code with
this for test.  I did a regulator parch against one of the previous
versions which has acks and so on, if you don't get that merged along
with this (the external interface didn't change I think) then I'll do so
myself as soon as this hits mainline or somewhere mergable.

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

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[David Daney]

On 03/05/2012 07:47 AM, Grant Likely wrote:
> Allow drivers to report at probe time that they cannot get all the resources
> required by the device, and should be retried at a later time.
>
> This should completely solve the problem of getting devices
> initialized in the right order.  Right now this is mostly handled by
> mucking about with initcall ordering which is a complete hack, and
> doesn't even remotely handle the case where device drivers are in
> modules.  This approach completely sidesteps the issues by allowing
> driver registration to occur in any order, and any driver can request
> to be retried after a few more other drivers get probed.
>
> v4: - Integrate Manjunath's addition of a separate workqueue
>      - Change -EAGAIN to -EPROBE_DEFER for drivers to trigger deferral
>      - Update comment blocks to reflect how the code really works
> v3: - Hold off workqueue scheduling until late_initcall so that the bulk
>        of driver probes are complete before we start retrying deferred devices.
>      - Tested with simple use cases.  Still needs more testing though.
>        Using it to get rid of the gpio early_initcall madness, or to replace
>        the ASoC internal probe deferral code would be ideal.
> v2: - added locking so it should no longer be utterly broken in that regard
>      - remove device from deferred list at device_del time.
>      - Still completely untested with any real use case, but has been
>        boot tested.
>
> Signed-off-by: Grant Likely<grant.likely@secretlab.ca>

Hi Grant, thanks for working on this:

Acked-by: David Daney <david.daney@cavium.com>

> Cc: Greg Kroah-Hartman<greg@kroah.com>
> Cc: Mark Brown<broonie@opensource.wolfsonmicro.com>
> Cc: Arnd Bergmann<arnd@arndb.de>
> Cc: Dilan Lee<dilee@nvidia.com>
> Cc: Manjunath GKondaiah<manjunath.gkondaiah@linaro.org>
> Cc: Alan Stern<stern@rowland.harvard.edu>
> Cc: Tony Lindgren<tony@atomide.com>
> ---
>
> Hi Greg,
>
> This has been through several revisions now and I think it's ready to go
> in.  The summary from the last discussion is that users need to have the
> dpm_list order adjusted if they defer themselves, but that is something
> which just cannot be performed by the core code (It needs to be manipulated
> mid-probe() call).
>
> I know that not everybody is happy with this approach, but I've yet to
> see a better alternative.  However, it is *really easy* to find all the
> users of deferred probe since any user must return -EPROBE_DEFER explicitly.
> If/when a better approach is found, all the users will be easy to find
> and modify.
>
> If this patch is not merged, then I'm going to have to merge another round
> of patches that futz with initcall ordering to get some drivers to probe
> correctly.  :-(
>
> g.
>
>   drivers/base/base.h    |    1 +
>   drivers/base/core.c    |    2 +
>   drivers/base/dd.c      |  138 +++++++++++++++++++++++++++++++++++++++++++++++-
>   include/linux/device.h |    5 ++
>   include/linux/errno.h  |    1 +
>   5 files changed, 146 insertions(+), 1 deletions(-)
>

Re: [PATCH] drivercore: Add driver probe deferral mechanism

[Alan Cox]

On Mon,  5 Mar 2012 08:47:41 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:

> Allow drivers to report at probe time that they cannot get all the resources
> required by the device, and should be retried at a later time.

Stick my oar in for that as well. We've got several bits of Intel SCU
using code that right now have ugly hacks in them (and generally aren't
upstream) because they must cope with random ordering problems.

Alan

[PATCH] drivercore: Add driver probe deferral mechanism

[Grant Likely]

Allow drivers to report at probe time that they cannot get all the resources
required by the device, and should be retried at a later time.

This should completely solve the problem of getting devices
initialized in the right order.  Right now this is mostly handled by
mucking about with initcall ordering which is a complete hack, and
doesn't even remotely handle the case where device drivers are in
modules.  This approach completely sidesteps the issues by allowing
driver registration to occur in any order, and any driver can request
to be retried after a few more other drivers get probed.

v4: - Integrate Manjunath's addition of a separate workqueue
    - Change -EAGAIN to -EPROBE_DEFER for drivers to trigger deferral
    - Update comment blocks to reflect how the code really works
v3: - Hold off workqueue scheduling until late_initcall so that the bulk
      of driver probes are complete before we start retrying deferred devices.
    - Tested with simple use cases.  Still needs more testing though.
      Using it to get rid of the gpio early_initcall madness, or to replace
      the ASoC internal probe deferral code would be ideal.
v2: - added locking so it should no longer be utterly broken in that regard
    - remove device from deferred list at device_del time.
    - Still completely untested with any real use case, but has been
      boot tested.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Greg Kroah-Hartman <greg@kroah.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dilan Lee <dilee@nvidia.com>
Cc: Manjunath GKondaiah <manjunath.gkondaiah@linaro.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Tony Lindgren <tony@atomide.com>
---

Hi Greg,

This has been through several revisions now and I think it's ready to go
in.  The summary from the last discussion is that users need to have the
dpm_list order adjusted if they defer themselves, but that is something
which just cannot be performed by the core code (It needs to be manipulated
mid-probe() call).

I know that not everybody is happy with this approach, but I've yet to
see a better alternative.  However, it is *really easy* to find all the
users of deferred probe since any user must return -EPROBE_DEFER explicitly.
If/when a better approach is found, all the users will be easy to find
and modify.

If this patch is not merged, then I'm going to have to merge another round
of patches that futz with initcall ordering to get some drivers to probe
correctly.  :-(

g.

 drivers/base/base.h    |    1 +
 drivers/base/core.c    |    2 +
 drivers/base/dd.c      |  138 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/device.h |    5 ++
 include/linux/errno.h  |    1 +
 5 files changed, 146 insertions(+), 1 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index b858dfd..2c13dea 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -105,6 +105,7 @@ extern void bus_remove_driver(struct device_driver *drv);
 
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
+extern void driver_deferred_probe_del(struct device *dev);
 static inline int driver_match_device(struct device_driver *drv,
 				      struct device *dev)
 {
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 74dda4f..d4ff7ad 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -644,6 +644,7 @@ void device_initialize(struct device *dev)
 {
 	dev->kobj.kset = devices_kset;
 	kobject_init(&dev->kobj, &device_ktype);
+	INIT_LIST_HEAD(&dev->deferred_probe);
 	INIT_LIST_HEAD(&dev->dma_pools);
 	mutex_init(&dev->mutex);
 	lockdep_set_novalidate_class(&dev->mutex);
@@ -1188,6 +1189,7 @@ void device_del(struct device *dev)
 	device_remove_file(dev, &uevent_attr);
 	device_remove_attrs(dev);
 	bus_remove_device(dev);
+	driver_deferred_probe_del(dev);
 
 	/*
 	 * Some platform devices are driven without driver attached
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 142e3d600..442b764 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -28,6 +28,133 @@
 #include "base.h"
 #include "power/power.h"
 
+/*
+ * Deferred Probe infrastructure.
+ *
+ * Sometimes driver probe order matters, but the kernel doesn't always have
+ * dependency information which means some drivers will get probed before a
+ * resource it depends on is available.  For example, an SDHCI driver may
+ * first need a GPIO line from an i2c GPIO controller before it can be
+ * initialized.  If a required resource is not available yet, a driver can
+ * request probing to be deferred by returning -EPROBE_DEFER from its probe hook
+ *
+ * Deferred probe maintains two lists of devices, a pending list and an active
+ * list.  A driver returning -EPROBE_DEFER causes the device to be added to the
+ * pending list.  A successful driver probe will trigger moving all devices
+ * from the pending to the active list so that the workqueue will eventually
+ * retry them.
+ *
+ * The deferred_probe_mutex must be held any time the deferred_probe_*_list
+ * of the (struct device*)->deferred_probe pointers are manipulated
+ */
+static DEFINE_MUTEX(deferred_probe_mutex);
+static LIST_HEAD(deferred_probe_pending_list);
+static LIST_HEAD(deferred_probe_active_list);
+static struct workqueue_struct *deferred_wq;
+
+/**
+ * deferred_probe_work_func() - Retry probing devices in the active list.
+ */
+static void deferred_probe_work_func(struct work_struct *work)
+{
+	struct device *dev;
+	/*
+	 * This block processes every device in the deferred 'active' list.
+	 * Each device is removed from the active list and passed to
+	 * bus_probe_device() to re-attempt the probe.  The loop continues
+	 * until every device in the active list is removed and retried.
+	 *
+	 * Note: Once the device is removed from the list and the mutex is
+	 * released, it is possible for the device get freed by another thread
+	 * and cause a illegal pointer dereference.  This code uses
+	 * get/put_device() to ensure the device structure cannot disappear
+	 * from under our feet.
+	 */
+	mutex_lock(&deferred_probe_mutex);
+	while (!list_empty(&deferred_probe_active_list)) {
+		dev = list_first_entry(&deferred_probe_active_list,
+					typeof(*dev), deferred_probe);
+		list_del_init(&dev->deferred_probe);
+
+		get_device(dev);
+
+		/* Drop the mutex while probing each device; the probe path
+		 * may manipulate the deferred list */
+		mutex_unlock(&deferred_probe_mutex);
+		dev_dbg(dev, "Retrying from deferred list\n");
+		bus_probe_device(dev);
+		mutex_lock(&deferred_probe_mutex);
+
+		put_device(dev);
+	}
+	mutex_unlock(&deferred_probe_mutex);
+}
+static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);
+
+static void driver_deferred_probe_add(struct device *dev)
+{
+	mutex_lock(&deferred_probe_mutex);
+	if (list_empty(&dev->deferred_probe)) {
+		dev_dbg(dev, "Added to deferred list\n");
+		list_add(&dev->deferred_probe, &deferred_probe_pending_list);
+	}
+	mutex_unlock(&deferred_probe_mutex);
+}
+
+void driver_deferred_probe_del(struct device *dev)
+{
+	mutex_lock(&deferred_probe_mutex);
+	if (!list_empty(&dev->deferred_probe)) {
+		dev_dbg(dev, "Removed from deferred list\n");
+		list_del_init(&dev->deferred_probe);
+	}
+	mutex_unlock(&deferred_probe_mutex);
+}
+
+static bool driver_deferred_probe_enable = false;
+/**
+ * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
+ *
+ * This functions moves all devices from the pending list to the active
+ * list and schedules the deferred probe workqueue to process them.  It
+ * should be called anytime a driver is successfully bound to a device.
+ */
+static void driver_deferred_probe_trigger(void)
+{
+	if (!driver_deferred_probe_enable)
+		return;
+
+	/* A successful probe means that all the devices in the pending list
+	 * should be triggered to be reprobed.  Move all the deferred devices
+	 * into the active list so they can be retried by the workqueue */
+	mutex_lock(&deferred_probe_mutex);
+	list_splice_tail_init(&deferred_probe_pending_list,
+			      &deferred_probe_active_list);
+	mutex_unlock(&deferred_probe_mutex);
+
+	/* Kick the re-probe thread.  It may already be scheduled, but
+	 * it is safe to kick it again. */
+	queue_work(deferred_wq, &deferred_probe_work);
+}
+
+/**
+ * deferred_probe_initcall() - Enable probing of deferred devices
+ *
+ * We don't want to get in the way when the bulk of drivers are getting probed.
+ * Instead, this initcall makes sure that deferred probing is delayed until
+ * late_initcall time.
+ */
+static int deferred_probe_initcall(void)
+{
+	deferred_wq = create_singlethread_workqueue("deferwq");
+	if (WARN_ON(!deferred_wq))
+		return -ENOMEM;
+
+	driver_deferred_probe_enable = true;
+	driver_deferred_probe_trigger();
+	return 0;
+}
+late_initcall(deferred_probe_initcall);
 
 static void driver_bound(struct device *dev)
 {
@@ -42,6 +169,11 @@ static void driver_bound(struct device *dev)
 
 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
 
+	/* Make sure the device is no longer in one of the deferred lists
+	 * and kick off retrying all pending devices */
+	driver_deferred_probe_del(dev);
+	driver_deferred_probe_trigger();
+
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_BOUND_DRIVER, dev);
@@ -142,7 +274,11 @@ probe_failed:
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;
 
-	if (ret != -ENODEV && ret != -ENXIO) {
+	if (ret == -EPROBE_DEFER) {
+		/* Driver requested deferred probing */
+		dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
+		driver_deferred_probe_add(dev);
+	} else if (ret != -ENODEV && ret != -ENXIO) {
 		/* driver matched but the probe failed */
 		printk(KERN_WARNING
 		       "%s: probe of %s failed with error %d\n",
diff --git a/include/linux/device.h b/include/linux/device.h
index b63fb39..adf090d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -587,6 +587,10 @@ struct device_dma_parameters {
  * @mutex:	Mutex to synchronize calls to its driver.
  * @bus:	Type of bus device is on.
  * @driver:	Which driver has allocated this
+ * @deferred_probe: entry in deferred_probe_list which is used to retry the
+ * 		binding of drivers which were unable to get all the resources
+ * 		needed by the device; typically because it depends on another
+ * 		driver getting probed first.
  * @platform_data: Platform data specific to the device.
  * 		Example: For devices on custom boards, as typical of embedded
  * 		and SOC based hardware, Linux often uses platform_data to point
@@ -646,6 +650,7 @@ struct device {
 	struct bus_type	*bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	struct list_head	deferred_probe;
 	void		*platform_data;	/* Platform specific data, device
 					   core doesn't touch it */
 	struct dev_pm_info	power;
diff --git a/include/linux/errno.h b/include/linux/errno.h
index 4668583..2d09bfa 100644
--- a/include/linux/errno.h
+++ b/include/linux/errno.h
@@ -16,6 +16,7 @@
 #define ERESTARTNOHAND	514	/* restart if no handler.. */
 #define ENOIOCTLCMD	515	/* No ioctl command */
 #define ERESTART_RESTARTBLOCK 516 /* restart by calling sys_restart_syscall */
+#define EPROBE_DEFER	517	/* Driver requests probe retry */
 
 /* Defined for the NFSv3 protocol */
 #define EBADHANDLE	521	/* Illegal NFS file handle */
-- 
1.7.9