[PATCH 1/5] drivercore: add new error value for deferred probe

[PATCH 1/5] drivercore: add new error value for deferred probe

[Greg KH]

On Fri, Oct 07, 2011 at 10:33:06AM +0500, G, Manjunath Kondaiah wrote:
> 
> Add new error value so that drivers can request deferred probe
> from drivercore.
> 
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> Reported-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> Cc: linux-omap at vger.kernel.org
> Cc: linux-mmc at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Greg Kroah-Hartman <greg@kroah.com>
> Cc: Dilan Lee <dilee@nvidia.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Manjunath GKondaiah <manjunath.gkondaiah@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> 
>  include/linux/errno.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/errno.h b/include/linux/errno.h
> index 4668583..83d8fcf 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	/* restart probe again after some time */

Can we really do this?  Isn't this some user/kernel api here?

What's wrong with just "overloading" on top of an existing error code?
Surely one of the other 516 types could be used here, right?

greg k-h

[PATCH 1/5] drivercore: add new error value for deferred probe

[Mark Brown]

On Thu, Oct 06, 2011 at 11:43:49PM -0700, Greg KH wrote:
> On Fri, Oct 07, 2011 at 10:33:06AM +0500, G, Manjunath Kondaiah wrote:

> > +#define EPROBE_DEFER	517	/* restart probe again after some time */

> Can we really do this?  Isn't this some user/kernel api here?

> What's wrong with just "overloading" on top of an existing error code?
> Surely one of the other 516 types could be used here, right?

There was some discussion of this in the previous patch round before the
code was changed - it does end up adding an externally visible error
code but it doesn't really make any difference, especially if we don't
propagate it externally.  We've already got some other codes in a
similar style, though I can't remember the examples that were quoted
offhand.  Adding the new code avoids confusion about exactly what the
intent of the driver is.

[PATCH 1/5] drivercore: add new error value for deferred probe

[Grant Likely]

On Fri, Oct 7, 2011 at 12:43 AM, Greg KH <greg@kroah.com> wrote:
> On Fri, Oct 07, 2011 at 10:33:06AM +0500, G, Manjunath Kondaiah wrote:
>>
>> Add new error value so that drivers can request deferred probe
>> from drivercore.
>>
>> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
>> Reported-by: Grant Likely <grant.likely@secretlab.ca>
>> ---
>> Cc: linux-omap at vger.kernel.org
>> Cc: linux-mmc at vger.kernel.org
>> Cc: linux-kernel at vger.kernel.org
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Cc: Greg Kroah-Hartman <greg@kroah.com>
>> Cc: Dilan Lee <dilee@nvidia.com>
>> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
>> Cc: Manjunath GKondaiah <manjunath.gkondaiah@linaro.org>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>>
>> ?include/linux/errno.h | ? ?1 +
>> ?1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/errno.h b/include/linux/errno.h
>> index 4668583..83d8fcf 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 ? ? /* restart probe again after some time */
>
> Can we really do this?

According to Arnd, yes this is okay.

> ?Isn't this some user/kernel api here?
>
> What's wrong with just "overloading" on top of an existing error code?
> Surely one of the other 516 types could be used here, right?

overloading makes it really hard to find the users at a later date.

g.

[PATCH 1/5] drivercore: add new error value for deferred probe

[Valdis.Kletnieks at vt.edu]

On Fri, 07 Oct 2011 16:12:45 MDT, Grant Likely said:
> On Fri, Oct 7, 2011 at 12:43 AM, Greg KH <greg@kroah.com> wrote:
> > On Fri, Oct 07, 2011 at 10:33:06AM +0500, G, Manjunath Kondaiah wrote:

> >> +#define EPROBE_DEFER 517 ? ? /* restart probe again after some time */
> >
> > Can we really do this?

> According to Arnd, yes this is okay.

> > ?Isn't this some user/kernel api here?

> > What's wrong with just "overloading" on top of an existing error code?
> > Surely one of the other 516 types could be used here, right?

> overloading makes it really hard to find the users at a later date.

Would proposing '#define EPROBE_DEFER EAGAIN' be acceptable to everybody? That
would allow overloading EAGAIN, but still make it easy to tell the usages apart
if we need to separate them later...

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111007/ed64fb47/attachment.sig>

[PATCH 1/5] drivercore: add new error value for deferred probe

[Greg KH]

On Fri, Oct 07, 2011 at 07:28:33PM -0400, Valdis.Kletnieks at vt.edu wrote:
> On Fri, 07 Oct 2011 16:12:45 MDT, Grant Likely said:
> > On Fri, Oct 7, 2011 at 12:43 AM, Greg KH <greg@kroah.com> wrote:
> > > On Fri, Oct 07, 2011 at 10:33:06AM +0500, G, Manjunath Kondaiah wrote:
> 
> > >> +#define EPROBE_DEFER 517 ? ? /* restart probe again after some time */
> > >
> > > Can we really do this?
> 
> > According to Arnd, yes this is okay.
> 
> > > ?Isn't this some user/kernel api here?
> 
> > > What's wrong with just "overloading" on top of an existing error code?
> > > Surely one of the other 516 types could be used here, right?
> 
> > overloading makes it really hard to find the users at a later date.
> 
> Would proposing '#define EPROBE_DEFER EAGAIN' be acceptable to everybody? That
> would allow overloading EAGAIN, but still make it easy to tell the usages apart
> if we need to separate them later...

Yes, please do that, it is what USB does for it's internal error code
handling.

greg k-h

[PATCH 1/5] drivercore: add new error value for deferred probe

[Grant Likely]

On Fri, Oct 7, 2011 at 6:12 PM, Greg KH <greg@kroah.com> wrote:
> On Fri, Oct 07, 2011 at 07:28:33PM -0400, Valdis.Kletnieks at vt.edu wrote:
>> On Fri, 07 Oct 2011 16:12:45 MDT, Grant Likely said:
>> > On Fri, Oct 7, 2011 at 12:43 AM, Greg KH <greg@kroah.com> wrote:
>> > > On Fri, Oct 07, 2011 at 10:33:06AM +0500, G, Manjunath Kondaiah wrote:
>>
>> > >> +#define EPROBE_DEFER 517 ? ? /* restart probe again after some time */
>> > >
>> > > Can we really do this?
>>
>> > According to Arnd, yes this is okay.
>>
>> > > ?Isn't this some user/kernel api here?
>>
>> > > What's wrong with just "overloading" on top of an existing error code?
>> > > Surely one of the other 516 types could be used here, right?
>>
>> > overloading makes it really hard to find the users at a later date.
>>
>> Would proposing '#define EPROBE_DEFER EAGAIN' be acceptable to everybody? That
>> would allow overloading EAGAIN, but still make it easy to tell the usages apart
>> if we need to separate them later...
>
> Yes, please do that, it is what USB does for it's internal error code
> handling.

Really?  When we've only currently used approximately 2^9 of a 2^31
numberspace?  I'm fine with making sure that the number doesn't show
up in the userspace headers, but it makes no sense to overload the
#defines.  Particularly so in this case where it isn't feasible to
audit every driver to figure out what probe might possibly return.  It
is well within the realm of possibility that existing drivers are
already returning -EAGAIN.

Besides; linux/errno.h *already* has linux-internal error codes that
do not get exported out to userspace.  There is an #ifdef __KERNEL__
block around ERESTARTSYS through EIOCBRETRY which is filtered out when
exporting headers.  I can't see any possible reason why we wouldn't
add Linux internal error codes here.

g.



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

[PATCH 1/5] drivercore: add new error value for deferred probe

[Greg KH]

On Sun, Oct 09, 2011 at 04:59:31PM -0600, Grant Likely wrote:
> On Fri, Oct 7, 2011 at 6:12 PM, Greg KH <greg@kroah.com> wrote:
> > On Fri, Oct 07, 2011 at 07:28:33PM -0400, Valdis.Kletnieks at vt.edu wrote:
> >> On Fri, 07 Oct 2011 16:12:45 MDT, Grant Likely said:
> >> > On Fri, Oct 7, 2011 at 12:43 AM, Greg KH <greg@kroah.com> wrote:
> >> > > On Fri, Oct 07, 2011 at 10:33:06AM +0500, G, Manjunath Kondaiah wrote:
> >>
> >> > >> +#define EPROBE_DEFER 517 ? ? /* restart probe again after some time */
> >> > >
> >> > > Can we really do this?
> >>
> >> > According to Arnd, yes this is okay.
> >>
> >> > > ?Isn't this some user/kernel api here?
> >>
> >> > > What's wrong with just "overloading" on top of an existing error code?
> >> > > Surely one of the other 516 types could be used here, right?
> >>
> >> > overloading makes it really hard to find the users at a later date.
> >>
> >> Would proposing '#define EPROBE_DEFER EAGAIN' be acceptable to everybody? That
> >> would allow overloading EAGAIN, but still make it easy to tell the usages apart
> >> if we need to separate them later...
> >
> > Yes, please do that, it is what USB does for it's internal error code
> > handling.
> 
> Really?  When we've only currently used approximately 2^9 of a 2^31
> numberspace?  I'm fine with making sure that the number doesn't show
> up in the userspace headers, but it makes no sense to overload the
> #defines.  Particularly so in this case where it isn't feasible to
> audit every driver to figure out what probe might possibly return.  It
> is well within the realm of possibility that existing drivers are
> already returning -EAGAIN.

I doubt they are, but you are right, it's really hard to tell.

> Besides; linux/errno.h *already* has linux-internal error codes that
> do not get exported out to userspace.  There is an #ifdef __KERNEL__
> block around ERESTARTSYS through EIOCBRETRY which is filtered out when
> exporting headers.  I can't see any possible reason why we wouldn't
> add Linux internal error codes here.

As long as it stays internal, that's fine, I was worried that this would
be exported to userspace.

Alan, still object to this?

greg k-h

[PATCH 1/5] drivercore: add new error value for deferred probe

[G, Manjunath Kondaiah]

On Sun, Oct 09, 2011 at 06:06:56PM -0700, Greg KH wrote:
> On Sun, Oct 09, 2011 at 04:59:31PM -0600, Grant Likely wrote:
> > On Fri, Oct 7, 2011 at 6:12 PM, Greg KH <greg@kroah.com> wrote:
> > > On Fri, Oct 07, 2011 at 07:28:33PM -0400, Valdis.Kletnieks at vt.edu wrote:
> > >> On Fri, 07 Oct 2011 16:12:45 MDT, Grant Likely said:
> > >> > On Fri, Oct 7, 2011 at 12:43 AM, Greg KH <greg@kroah.com> wrote:
> > >> > > On Fri, Oct 07, 2011 at 10:33:06AM +0500, G, Manjunath Kondaiah wrote:
> > >>
> > >> > >> +#define EPROBE_DEFER 517 ? ? /* restart probe again after some time */
> > >> > >
> > >> > > Can we really do this?
> > >>
> > >> > According to Arnd, yes this is okay.
> > >>
> > >> > > ?Isn't this some user/kernel api here?
> > >>
> > >> > > What's wrong with just "overloading" on top of an existing error code?
> > >> > > Surely one of the other 516 types could be used here, right?
> > >>
> > >> > overloading makes it really hard to find the users at a later date.
> > >>
> > >> Would proposing '#define EPROBE_DEFER EAGAIN' be acceptable to everybody? That
> > >> would allow overloading EAGAIN, but still make it easy to tell the usages apart
> > >> if we need to separate them later...
> > >
> > > Yes, please do that, it is what USB does for it's internal error code
> > > handling.
> > 
> > Really?  When we've only currently used approximately 2^9 of a 2^31
> > numberspace?  I'm fine with making sure that the number doesn't show
> > up in the userspace headers, but it makes no sense to overload the
> > #defines.  Particularly so in this case where it isn't feasible to
> > audit every driver to figure out what probe might possibly return.  It
> > is well within the realm of possibility that existing drivers are
> > already returning -EAGAIN.
> 
> I doubt they are, but you are right, it's really hard to tell.
> 
> > Besides; linux/errno.h *already* has linux-internal error codes that
> > do not get exported out to userspace.  There is an #ifdef __KERNEL__
> > block around ERESTARTSYS through EIOCBRETRY which is filtered out when
> > exporting headers.  I can't see any possible reason why we wouldn't
> > add Linux internal error codes here.
> 
> As long as it stays internal, that's fine, I was worried that this would
> be exported to userspace.
> 
> Alan, still object to this?

I hope no one has objections to use EPROBE_DEFER

-M

[PATCH 1/5] drivercore: add new error value for deferred probe

[Grant Likely]

On Wed, Oct 12, 2011 at 11:48:23AM +0530, G, Manjunath Kondaiah wrote:
> On Sun, Oct 09, 2011 at 06:06:56PM -0700, Greg KH wrote:
> > On Sun, Oct 09, 2011 at 04:59:31PM -0600, Grant Likely wrote:
> > > On Fri, Oct 7, 2011 at 6:12 PM, Greg KH <greg@kroah.com> wrote:
> > > > On Fri, Oct 07, 2011 at 07:28:33PM -0400, Valdis.Kletnieks at vt.edu wrote:
> > > >> On Fri, 07 Oct 2011 16:12:45 MDT, Grant Likely said:
> > > >> > On Fri, Oct 7, 2011 at 12:43 AM, Greg KH <greg@kroah.com> wrote:
> > > >> > > On Fri, Oct 07, 2011 at 10:33:06AM +0500, G, Manjunath Kondaiah wrote:
> > > >>
> > > >> > >> +#define EPROBE_DEFER 517 ? ? /* restart probe again after some time */
> > > >> > >
> > > >> > > Can we really do this?
> > > >>
> > > >> > According to Arnd, yes this is okay.
> > > >>
> > > >> > > ?Isn't this some user/kernel api here?
> > > >>
> > > >> > > What's wrong with just "overloading" on top of an existing error code?
> > > >> > > Surely one of the other 516 types could be used here, right?
> > > >>
> > > >> > overloading makes it really hard to find the users at a later date.
> > > >>
> > > >> Would proposing '#define EPROBE_DEFER EAGAIN' be acceptable to everybody? That
> > > >> would allow overloading EAGAIN, but still make it easy to tell the usages apart
> > > >> if we need to separate them later...
> > > >
> > > > Yes, please do that, it is what USB does for it's internal error code
> > > > handling.
> > > 
> > > Really?  When we've only currently used approximately 2^9 of a 2^31
> > > numberspace?  I'm fine with making sure that the number doesn't show
> > > up in the userspace headers, but it makes no sense to overload the
> > > #defines.  Particularly so in this case where it isn't feasible to
> > > audit every driver to figure out what probe might possibly return.  It
> > > is well within the realm of possibility that existing drivers are
> > > already returning -EAGAIN.
> > 
> > I doubt they are, but you are right, it's really hard to tell.
> > 
> > > Besides; linux/errno.h *already* has linux-internal error codes that
> > > do not get exported out to userspace.  There is an #ifdef __KERNEL__
> > > block around ERESTARTSYS through EIOCBRETRY which is filtered out when
> > > exporting headers.  I can't see any possible reason why we wouldn't
> > > add Linux internal error codes here.
> > 
> > As long as it stays internal, that's fine, I was worried that this would
> > be exported to userspace.
> > 
> > Alan, still object to this?
> 
> I hope no one has objections to use EPROBE_DEFER

I say go with that value.  If Alan still objects, then he will speak up.

g.

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Greg KH]

On Fri, Oct 07, 2011 at 10:33:07AM +0500, G, Manjunath Kondaiah wrote:
> 
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> 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.
> 
> Original patch posted by Grant Likely <grant.likely@secretlab.ca> at:
> http://lwn.net/Articles/460522/
> 
> Enhancements to original patch by G, Manjunath Kondaiah <manjugk@ti.com>
>  - checkpatch warning fixes
>  - added Kconfig symbol CONFIG_PROBE_DEFER
>  - replacing normal workqueue with singlethread_workqueue
>  - handling -EPROBE_DEFER error
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> ---
> Cc: linux-omap at vger.kernel.org
> Cc: linux-mmc at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Greg Kroah-Hartman <greg@kroah.com>
> Cc: Dilan Lee <dilee@nvidia.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Manjunath GKondaiah <manjunath.gkondaiah@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> 
>  drivers/base/Kconfig   |   11 ++++
>  drivers/base/base.h    |    3 +
>  drivers/base/core.c    |    6 ++
>  drivers/base/dd.c      |  145 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |    7 ++
>  5 files changed, 172 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 21cf46f..b412a71 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -172,6 +172,17 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +config PROBE_DEFER
> +	bool "Deferred Driver Probe"
> +	default y
> +	help
> +	  This option provides deferring driver probe if it has dependency on
> +	  other driver. Without this feature, initcall ordering should be done
> +	  manually to resolve driver dependencies. This feature completely side
> +	  steps 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 is this even an option?  Why would you ever want it disabled?  Why
does it need to be selected?

If you are going to default something to 'y' then just make it so it
can't be turned off any other way by just not making it an option at
all.

It also cleans up this diff a lot, as you really don't want #ifdef in .c
files.

> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -28,6 +28,136 @@
>  #include "base.h"
>  #include "power/power.h"
>  
> +#if defined CONFIG_PROBE_DEFER
> +/*
> + * Deferred Probe infrastructure.

Why not use kerneldoc?

> + *
> + * 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.
> + *
> + * 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;
> +	/*

Extra blank line please.

> +	 * 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:

That's what the klist structure is supposed to make easier, why not use
that here?

> +	 * - 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.

Ick, use a klist, it was created to handle this exact problem in the
driver core, so to not use it would be wrong, right?

> +	 */
> +	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 */

Use proper kernel multi-line comment format.  This needs to be done in a
number of places in this patch, please fix them all.

thanks,

greg k-h

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Josh Triplett]

On Thu, Oct 06, 2011 at 11:49:28PM -0700, Greg KH wrote:
> On Fri, Oct 07, 2011 at 10:33:07AM +0500, G, Manjunath Kondaiah wrote:
> > +config PROBE_DEFER
> > +	bool "Deferred Driver Probe"
> > +	default y
> > +	help
> > +	  This option provides deferring driver probe if it has dependency on
> > +	  other driver. Without this feature, initcall ordering should be done
> > +	  manually to resolve driver dependencies. This feature completely side
> > +	  steps 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 is this even an option?  Why would you ever want it disabled?  Why
> does it need to be selected?
> 
> If you are going to default something to 'y' then just make it so it
> can't be turned off any other way by just not making it an option at
> all.

Given that the drivers which use this mechanism will not necessarily get
built into the kernel, I'd suggest that it should remain optional and
default to n.  Those drivers can then add a dependency on PROBE_DEFER.
Let's try to avoid adding more infrastructure to the kernel that takes
up space even when unused; certainly embedded will appreciate not having
this feature unless a driver needs it.

(That said, it still feels to me like an explicit dependency mechanism
would make more sense than this "try again later" mechanism, but
nonetheless "try again later" seems like an improvement over what we
have now.)

> It also cleans up this diff a lot, as you really don't want #ifdef in .c
> files.

Ideally the entire .c file could become conditional on PROBE_DEFER via
kbuild, with the usual compatibility inlines in a .h file for the
!PROBE_DEFER case.

- Josh Triplett

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Greg KH]

On Fri, Oct 07, 2011 at 01:57:15PM -0700, Josh Triplett wrote:
> On Thu, Oct 06, 2011 at 11:49:28PM -0700, Greg KH wrote:
> > On Fri, Oct 07, 2011 at 10:33:07AM +0500, G, Manjunath Kondaiah wrote:
> > > +config PROBE_DEFER
> > > +	bool "Deferred Driver Probe"
> > > +	default y
> > > +	help
> > > +	  This option provides deferring driver probe if it has dependency on
> > > +	  other driver. Without this feature, initcall ordering should be done
> > > +	  manually to resolve driver dependencies. This feature completely side
> > > +	  steps 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 is this even an option?  Why would you ever want it disabled?  Why
> > does it need to be selected?
> > 
> > If you are going to default something to 'y' then just make it so it
> > can't be turned off any other way by just not making it an option at
> > all.
> 
> Given that the drivers which use this mechanism will not necessarily get
> built into the kernel, I'd suggest that it should remain optional and
> default to n.  Those drivers can then add a dependency on PROBE_DEFER.
> Let's try to avoid adding more infrastructure to the kernel that takes
> up space even when unused; certainly embedded will appreciate not having
> this feature unless a driver needs it.

How much extra space is this "feature" really?  I don't see it being
anything larger than the amount of memory increase that just happened as
I typed this email as part of the ongoing memory density changes.

greg k-h

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Josh Triplett]

On Fri, Oct 07, 2011 at 02:23:26PM -0700, Greg KH wrote:
> On Fri, Oct 07, 2011 at 01:57:15PM -0700, Josh Triplett wrote:
> > On Thu, Oct 06, 2011 at 11:49:28PM -0700, Greg KH wrote:
> > > On Fri, Oct 07, 2011 at 10:33:07AM +0500, G, Manjunath Kondaiah wrote:
> > > > +config PROBE_DEFER
> > > > +	bool "Deferred Driver Probe"
> > > > +	default y
> > > > +	help
> > > > +	  This option provides deferring driver probe if it has dependency on
> > > > +	  other driver. Without this feature, initcall ordering should be done
> > > > +	  manually to resolve driver dependencies. This feature completely side
> > > > +	  steps 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 is this even an option?  Why would you ever want it disabled?  Why
> > > does it need to be selected?
> > > 
> > > If you are going to default something to 'y' then just make it so it
> > > can't be turned off any other way by just not making it an option at
> > > all.
> > 
> > Given that the drivers which use this mechanism will not necessarily get
> > built into the kernel, I'd suggest that it should remain optional and
> > default to n.  Those drivers can then add a dependency on PROBE_DEFER.
> > Let's try to avoid adding more infrastructure to the kernel that takes
> > up space even when unused; certainly embedded will appreciate not having
> > this feature unless a driver needs it.
> 
> How much extra space is this "feature" really?

Just checked: 776 bytes, 640 of text and 136 of data.  We have kconfig
options for comparable amounts.

> I don't see it being
> anything larger than the amount of memory increase that just happened as
> I typed this email as part of the ongoing memory density changes.

I don't know about the changes you mean, but in any case I'd like to
prevent mandatory size increases wherever possible.  I'd love to see the
size of "allnoconfig" getting *smaller* over time, not larger.

- Josh Triplett

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Greg KH]

On Fri, Oct 07, 2011 at 09:03:51PM -0700, Josh Triplett wrote:
> On Fri, Oct 07, 2011 at 02:23:26PM -0700, Greg KH wrote:
> > On Fri, Oct 07, 2011 at 01:57:15PM -0700, Josh Triplett wrote:
> > > On Thu, Oct 06, 2011 at 11:49:28PM -0700, Greg KH wrote:
> > > > On Fri, Oct 07, 2011 at 10:33:07AM +0500, G, Manjunath Kondaiah wrote:
> > > > > +config PROBE_DEFER
> > > > > +	bool "Deferred Driver Probe"
> > > > > +	default y
> > > > > +	help
> > > > > +	  This option provides deferring driver probe if it has dependency on
> > > > > +	  other driver. Without this feature, initcall ordering should be done
> > > > > +	  manually to resolve driver dependencies. This feature completely side
> > > > > +	  steps 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 is this even an option?  Why would you ever want it disabled?  Why
> > > > does it need to be selected?
> > > > 
> > > > If you are going to default something to 'y' then just make it so it
> > > > can't be turned off any other way by just not making it an option at
> > > > all.
> > > 
> > > Given that the drivers which use this mechanism will not necessarily get
> > > built into the kernel, I'd suggest that it should remain optional and
> > > default to n.  Those drivers can then add a dependency on PROBE_DEFER.
> > > Let's try to avoid adding more infrastructure to the kernel that takes
> > > up space even when unused; certainly embedded will appreciate not having
> > > this feature unless a driver needs it.
> > 
> > How much extra space is this "feature" really?
> 
> Just checked: 776 bytes, 640 of text and 136 of data.  We have kconfig
> options for comparable amounts.
> 
> > I don't see it being
> > anything larger than the amount of memory increase that just happened as
> > I typed this email as part of the ongoing memory density changes.
> 
> I don't know about the changes you mean

Moore's law.

Really, 776 bytes, just always enable it, it's not worth it.

greg k-h

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Josh Triplett]

On Sat, Oct 08, 2011 at 08:55:02AM -0700, Greg KH wrote:
> On Fri, Oct 07, 2011 at 09:03:51PM -0700, Josh Triplett wrote:
> > On Fri, Oct 07, 2011 at 02:23:26PM -0700, Greg KH wrote:
> > > On Fri, Oct 07, 2011 at 01:57:15PM -0700, Josh Triplett wrote:
> > > > On Thu, Oct 06, 2011 at 11:49:28PM -0700, Greg KH wrote:
> > > > > On Fri, Oct 07, 2011 at 10:33:07AM +0500, G, Manjunath Kondaiah wrote:
> > > > > > +config PROBE_DEFER
> > > > > > +	bool "Deferred Driver Probe"
> > > > > > +	default y
> > > > > > +	help
> > > > > > +	  This option provides deferring driver probe if it has dependency on
> > > > > > +	  other driver. Without this feature, initcall ordering should be done
> > > > > > +	  manually to resolve driver dependencies. This feature completely side
> > > > > > +	  steps 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 is this even an option?  Why would you ever want it disabled?  Why
> > > > > does it need to be selected?
> > > > > 
> > > > > If you are going to default something to 'y' then just make it so it
> > > > > can't be turned off any other way by just not making it an option at
> > > > > all.
> > > > 
> > > > Given that the drivers which use this mechanism will not necessarily get
> > > > built into the kernel, I'd suggest that it should remain optional and
> > > > default to n.  Those drivers can then add a dependency on PROBE_DEFER.
> > > > Let's try to avoid adding more infrastructure to the kernel that takes
> > > > up space even when unused; certainly embedded will appreciate not having
> > > > this feature unless a driver needs it.
> > > 
> > > How much extra space is this "feature" really?
> > 
> > Just checked: 776 bytes, 640 of text and 136 of data.  We have kconfig
> > options for comparable amounts.
> > 
> > > I don't see it being
> > > anything larger than the amount of memory increase that just happened as
> > > I typed this email as part of the ongoing memory density changes.
> > 
> > I don't know about the changes you mean
> 
> Moore's law.

Ah, I see.  For new systems, sure; for systems or mechanisms with a
pre-existing size constraint, that doesn't help.

> Really, 776 bytes, just always enable it, it's not worth it.

776 bytes alone, no; 776 bytes times the next (or previous) thousand
features, yes.

- Josh Triplett

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Andrei Warkentin]

Hi,

----- Original Message -----
> From: "Greg KH" <greg@kroah.com>
> To: "Josh Triplett" <josh@joshtriplett.org>
> Cc: "G, Manjunath Kondaiah" <manjugk@ti.com>, linux-arm-kernel at lists.infradead.org, "Grant Likely"
> <grant.likely@secretlab.ca>, linux-omap at vger.kernel.org, linux-mmc at vger.kernel.org, linux-kernel at vger.kernel.org,
> "Dilan Lee" <dilee@nvidia.com>, "Mark Brown" <broonie@opensource.wolfsonmicro.com>, Manjunath at jasper.es
> Sent: Saturday, October 8, 2011 11:55:02 AM
> Subject: Re: [PATCH 2/5] drivercore: Add driver probe deferral mechanism
> 

I'm a bit of a fly on the wall here, but I'm curious how this impacts suspend/resume.
device_initialize->device_pm_init are called from device_register, so certainly this
patch doesn't also ensure that the PM ordering matches probe ordering, which is bound
to break suspend, right? Was this ever tested with the OMAP target? Shouldn't the
PM change be also part of this patch set? I don't see why you would want to have this in
without the PM changes.

Maybe I have it all wrong though :-).

A

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Ming Lei]

On Tue, Oct 11, 2011 at 1:37 AM, Andrei Warkentin <awarkentin@vmware.com> wrote:
> Hi,
>
> ----- Original Message -----
>> From: "Greg KH" <greg@kroah.com>
>> To: "Josh Triplett" <josh@joshtriplett.org>
>> Cc: "G, Manjunath Kondaiah" <manjugk@ti.com>, linux-arm-kernel at lists.infradead.org, "Grant Likely"
>> <grant.likely@secretlab.ca>, linux-omap at vger.kernel.org, linux-mmc at vger.kernel.org, linux-kernel at vger.kernel.org,
>> "Dilan Lee" <dilee@nvidia.com>, "Mark Brown" <broonie@opensource.wolfsonmicro.com>, Manjunath at jasper.es
>> Sent: Saturday, October 8, 2011 11:55:02 AM
>> Subject: Re: [PATCH 2/5] drivercore: Add driver probe deferral mechanism
>>
>
> I'm a bit of a fly on the wall here, but I'm curious how this impacts suspend/resume.
> device_initialize->device_pm_init are called from device_register, so certainly this
> patch doesn't also ensure that the PM ordering matches probe ordering, which is bound
> to break suspend, right? Was this ever tested with the OMAP target? Shouldn't the

Inside device_add(), device_pm_add is called before bus_probe_device,
so the patch can't change the device order in pm list, and just change
the driver probe order.

> PM change be also part of this patch set? I don't see why you would want to have this in
> without the PM changes.
>


thanks,
-- 
Ming Lei

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Tue, Oct 11, 2011 at 08:29:18PM +0800, Ming Lei wrote:
> On Tue, Oct 11, 2011 at 1:37 AM, Andrei Warkentin <awarkentin@vmware.com> wrote:
> > Hi,
> >
> > ----- Original Message -----
> >> From: "Greg KH" <greg@kroah.com>
> >> To: "Josh Triplett" <josh@joshtriplett.org>
> >> Cc: "G, Manjunath Kondaiah" <manjugk@ti.com>, linux-arm-kernel at lists.infradead.org, "Grant Likely"
> >> <grant.likely@secretlab.ca>, linux-omap at vger.kernel.org, linux-mmc at vger.kernel.org, linux-kernel at vger.kernel.org,
> >> "Dilan Lee" <dilee@nvidia.com>, "Mark Brown" <broonie@opensource.wolfsonmicro.com>, Manjunath at jasper.es
> >> Sent: Saturday, October 8, 2011 11:55:02 AM
> >> Subject: Re: [PATCH 2/5] drivercore: Add driver probe deferral mechanism
> >>
> >
> > I'm a bit of a fly on the wall here, but I'm curious how this impacts suspend/resume.
> > device_initialize->device_pm_init are called from device_register, so certainly this
> > patch doesn't also ensure that the PM ordering matches probe ordering, which is bound
> > to break suspend, right? Was this ever tested with the OMAP target? Shouldn't the
> 
> Inside device_add(), device_pm_add is called before bus_probe_device,
> so the patch can't change the device order in pm list, and just change
> the driver probe order.

That's the way it works now, but can it be reworked?  It would be
possible to adjust the list order after successful probe.  However,
I'm not clear on the ordering rules for the dpm_list.  Right now it is
explicitly ordered to have parents before children, but as already
expressed, that doesn't accurately represent ordering constraints for
multiple device dependancies.

So, reordering the list would probably require maintaining the
existing parent-child ordering constraint, but to also shift
devices (and any possible children?) to be after drivers that are
already probed.  That alone will be difficult to implement and get
right, but maybe the constraints can be simplified.  It needs some
further thought.

g.

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Ming Lei]

CC Rafael and linux-pm

On Thu, Oct 13, 2011 at 12:09 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Tue, Oct 11, 2011 at 08:29:18PM +0800, Ming Lei wrote:
>> On Tue, Oct 11, 2011 at 1:37 AM, Andrei Warkentin <awarkentin@vmware.com> wrote:
>> > Hi,
>> >
>> > ----- Original Message -----
>> >> From: "Greg KH" <greg@kroah.com>
>> >> To: "Josh Triplett" <josh@joshtriplett.org>
>> >> Cc: "G, Manjunath Kondaiah" <manjugk@ti.com>, linux-arm-kernel at lists.infradead.org, "Grant Likely"
>> >> <grant.likely@secretlab.ca>, linux-omap at vger.kernel.org, linux-mmc at vger.kernel.org, linux-kernel at vger.kernel.org,
>> >> "Dilan Lee" <dilee@nvidia.com>, "Mark Brown" <broonie@opensource.wolfsonmicro.com>, Manjunath at jasper.es
>> >> Sent: Saturday, October 8, 2011 11:55:02 AM
>> >> Subject: Re: [PATCH 2/5] drivercore: Add driver probe deferral mechanism
>> >>
>> >
>> > I'm a bit of a fly on the wall here, but I'm curious how this impacts suspend/resume.
>> > device_initialize->device_pm_init are called from device_register, so certainly this
>> > patch doesn't also ensure that the PM ordering matches probe ordering, which is bound
>> > to break suspend, right? Was this ever tested with the OMAP target? Shouldn't the
>>
>> Inside device_add(), device_pm_add is called before bus_probe_device,
>> so the patch can't change the device order in pm list, and just change
>> the driver probe order.
>
> That's the way it works now, but can it be reworked? ?It would be

IMO, it depends on what shape you plan to rework.  Currently, the
deferred probe may found a resource dependency, but I am not sure
that pm dependency is same with the resource dependency found
during probe.

> possible to adjust the list order after successful probe. ?However,
> I'm not clear on the ordering rules for the dpm_list. ?Right now it is
> explicitly ordered to have parents before children, but as already
> expressed, that doesn't accurately represent ordering constraints for
> multiple device dependancies.

Maybe we should understand the correct model of the ordering constraints
for the multiple device dependancies first, could you give a description or
some examples about it?

>
> So, reordering the list would probably require maintaining the
> existing parent-child ordering constraint, but to also shift
> devices (and any possible children?) to be after drivers that are
> already probed. ?That alone will be difficult to implement and get
> right, but maybe the constraints can be simplified. ?It needs some
> further thought.
>
> g.
>
>


thanks,
-- 
Ming Lei

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Alan Stern]

On Thu, 13 Oct 2011, Ming Lei wrote:

> >> Inside device_add(), device_pm_add is called before bus_probe_device,
> >> so the patch can't change the device order in pm list, and just change
> >> the driver probe order.
> >
> > That's the way it works now, but can it be reworked? ?It would be
> 
> IMO, it depends on what shape you plan to rework.  Currently, the
> deferred probe may found a resource dependency, but I am not sure
> that pm dependency is same with the resource dependency found
> during probe.
> 
> > possible to adjust the list order after successful probe. ?However,
> > I'm not clear on the ordering rules for the dpm_list. ?Right now it is
> > explicitly ordered to have parents before children, but as already
> > expressed, that doesn't accurately represent ordering constraints for
> > multiple device dependancies.
> 
> Maybe we should understand the correct model of the ordering constraints
> for the multiple device dependancies first, could you give a description or
> some examples about it?

The requirement is that devices must be capable of resuming in the 
order given by dpm_list, and they must be capable of suspending in 
the reverse order.

Therefore if device A can't work unless device B is functional, then B 
must come before A in dpm_list.

Alan Stern

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Ming Lei]

Hi,

On Thu, Oct 13, 2011 at 10:31 PM, Alan Stern <stern@rowland.harvard.edu>
>> Maybe we should understand the correct model of the ordering constraints
>> for the multiple device dependancies first, could you give a description or
>> some examples about it?
>
> The requirement is that devices must be capable of resuming in the
> order given by dpm_list, and they must be capable of suspending in
> the reverse order.
>
> Therefore if device A can't work unless device B is functional, then B
> must come before A in dpm_list.

If all devices can support async suspend and resume correctly, looks like
the device order given by dpm_list is not needed any longer, doesn't it?


thanks,
-- 
Ming Lei

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Alan Stern]

On Thu, 13 Oct 2011, Ming Lei wrote:

> Hi,
> 
> On Thu, Oct 13, 2011 at 10:31 PM, Alan Stern <stern@rowland.harvard.edu>
> >> Maybe we should understand the correct model of the ordering constraints
> >> for the multiple device dependancies first, could you give a description or
> >> some examples about it?
> >
> > The requirement is that devices must be capable of resuming in the
> > order given by dpm_list, and they must be capable of suspending in
> > the reverse order.
> >
> > Therefore if device A can't work unless device B is functional, then B
> > must come before A in dpm_list.
> 
> If all devices can support async suspend and resume correctly, looks like
> the device order given by dpm_list is not needed any longer, doesn't it?

It _is_ needed, because the user can disable async suspend/resume via 
/sys/power/pm_async.

Also, not all devices do support async suspend/resume.

Alan Stern

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Ming Lei]

On Fri, Oct 14, 2011 at 12:04 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 13 Oct 2011, Ming Lei wrote:
>
>> Hi,
>>
>> On Thu, Oct 13, 2011 at 10:31 PM, Alan Stern <stern@rowland.harvard.edu>
>> >> Maybe we should understand the correct model of the ordering constraints
>> >> for the multiple device dependancies first, could you give a description or
>> >> some examples about it?
>> >
>> > The requirement is that devices must be capable of resuming in the
>> > order given by dpm_list, and they must be capable of suspending in
>> > the reverse order.
>> >
>> > Therefore if device A can't work unless device B is functional, then B
>> > must come before A in dpm_list.
>>
>> If all devices can support async suspend and resume correctly, looks like
>> the device order given by dpm_list is not needed any longer, doesn't it?
>
> It _is_ needed, because the user can disable async suspend/resume via
> /sys/power/pm_async.
>
> Also, not all devices do support async suspend/resume.

Basically, the devices which don't support async suspend/resume
should have out of tree PM dependency. If out of tree PM dependency
issue is solved, all devices can support async suspend/resume.

thanks,
-- 
Ming Lei

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Thu, Oct 13, 2011 at 10:31:45AM -0400, Alan Stern wrote:
> On Thu, 13 Oct 2011, Ming Lei wrote:
> 
> > >> Inside device_add(), device_pm_add is called before bus_probe_device,
> > >> so the patch can't change the device order in pm list, and just change
> > >> the driver probe order.
> > >
> > > That's the way it works now, but can it be reworked? ?It would be
> > 
> > IMO, it depends on what shape you plan to rework.  Currently, the
> > deferred probe may found a resource dependency, but I am not sure
> > that pm dependency is same with the resource dependency found
> > during probe.
> > 
> > > possible to adjust the list order after successful probe. ?However,
> > > I'm not clear on the ordering rules for the dpm_list. ?Right now it is
> > > explicitly ordered to have parents before children, but as already
> > > expressed, that doesn't accurately represent ordering constraints for
> > > multiple device dependancies.
> > 
> > Maybe we should understand the correct model of the ordering constraints
> > for the multiple device dependancies first, could you give a description or
> > some examples about it?
> 
> The requirement is that devices must be capable of resuming in the 
> order given by dpm_list, and they must be capable of suspending in 
> the reverse order.
> 
> Therefore if device A can't work unless device B is functional, then B 
> must come before A in dpm_list.

For the deferred case; here is an example of the additional
constraint.  Consider the following hierarchy:

-A
 +-B
 | +-C
 | +-D
 |
 +-E
   +-F
   +-G

dpm_list could be ordered in at least the following ways (depending on
exactly when devices get registered).  There are many permutation, but
children are always be listed after its direct parent.

1) ABECDFG (breadth first)
2) AEBFGCD (breadth first)
3) ABCDEFG (depth first)
4) AEFGBCD (depth first)

Now, assume that device B depends on device F, and also assume that
there is no way either to express that in the hierarchy or even for
the constraint to be known at device registration time (the is exactly
the situation we're dealing with on embedded platforms).  Only the
driver for B knows that it needs a resource provided by F's driver.
So, the situation becomes that the ordering of dpm_list must now also
be sorted so that non-tree dependencies are also accounted for.  Of
the list above, only sort order 4 meets the new constraint.

The question then becomes, how can the dpm_list get resorted
dynamically at runtime to ensure that the new constraints are always
met without breaking old ones.  Here are some options I can think of:

1) When a deferred probe succeeds, move the deferred device to the
end of the dpm_list.  Then the new sort order might be one of:

	1) AECDFGB
	2) AEFGCDB
	3) ACDEFGB
	4) AEFGCDB

However, that approach breaks the guarantee that children appear after
their parents (C & D appear before B in all cases above)

2a) When a deferred probe succeeds, move the deferred device and it's
entire child hierarchy to the end of the list to become one of:

	1) AEFGBCD
	2) AEFGBCD
	3) AEFGBCD
	4) AEFGBCD (hey! they're all the same now, but there are other
			orderings possible that aren't)  :-)

Problem: Complexity increases.  This requires iterating through all
the children and performing a reorder for each.  Fortunately, this
should be too expensive since I believe each individual move is an
O(1) operation.  I don't think the code will need to walk the list for
each device.

Potential problem: This may result in unnecessary sorting in a lot of
cases.  It may be that the newly probed device is already after the
device it depends on, but the driver just hadn't been probed early
enough to avoid deferral.

Potential problem: It may end up exposing implicit dependencies
between drivers that weren't observed before.

Potential problem: This completely breaks if a parent gets probed
*after* its child which might happen if something other than the
parent driver creates the child devices.  I don't think this is a real
problem, but I think the kernel would first need to ensure that none
of the children are bound to drivers, and complain loudly if they are.
Otherwise the dpm_list won't reflect probe order.

2b) alternately, when *any* probe succeeds, move the deferred device
and its children to the end of the list.  This continues to maintain
the parent-child relationship, and it ensures that the dpm_list is
always also sorted in probe-order (devices bound to drivers will
collect at the end of the list).

Potential problem: On a big device hierarchy, this will mean a lot of
moves.  It should still only be O(1) for each move, but O(N^2) over
probing the entire hierarchy.  Devices will end up being moved once
for itself, and once for each parent and grandparent bound to a
driver.  It could become slow.

This option also has the potential problem when a parent gets probed
after its child as discussed in 2a.

3) Add devices to dpm_list after successful probe instead of at
device_add time.

Potential problem: this means that only devices with drivers actually
get suspend/resume calls.  I don't know nearly enough about the PM
subsystem, but I suspect that this is a problem.

4) ignore parent-child relationships entirely and just move devices to
the end of the list on successful probe.  If it probed successfully,
then it can be successfully suspended regardless of whether it has any
children.  That breaks the parent-child ordering, but guarantees the
probe order ordering.  Any devices without drivers will end up
collecting at the beginning of the list, and will be suspended after
all the devices with drivers.

Problem: Same as with 3, I suspect that even devices without drivers
need to process PM suspend, which makes this option unworkable.

...

For my money, I think that option 2b shows the most promise despite
the potential O(N^2) complexity.  There may be a better algorithm for
doing the runtime reordering that isn't O(N^2) that I haven't thought
of.  Having a list that is strongly ordered both on hierarchy *and*
probe order I think is the right thing to do, even without deferred
probe support.

g.

> Alan Stern
> 

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Alan Stern]

On Thu, 13 Oct 2011, Grant Likely wrote:

> For the deferred case; here is an example of the additional
> constraint.  Consider the following hierarchy:
> 
> -A
>  +-B
>  | +-C
>  | +-D
>  |
>  +-E
>    +-F
>    +-G
> 
> dpm_list could be ordered in at least the following ways (depending on
> exactly when devices get registered).  There are many permutation, but
> children are always be listed after its direct parent.
> 
> 1) ABECDFG (breadth first)
> 2) AEBFGCD (breadth first)
> 3) ABCDEFG (depth first)
> 4) AEFGBCD (depth first)
> 
> Now, assume that device B depends on device F, and also assume that
> there is no way either to express that in the hierarchy or even for
> the constraint to be known at device registration time (the is exactly
> the situation we're dealing with on embedded platforms).  Only the
> driver for B knows that it needs a resource provided by F's driver.
> So, the situation becomes that the ordering of dpm_list must now also
> be sorted so that non-tree dependencies are also accounted for.  Of
> the list above, only sort order 4 meets the new constraint.
> 
> The question then becomes, how can the dpm_list get resorted
> dynamically at runtime to ensure that the new constraints are always
> met without breaking old ones.  Here are some options I can think of:

This was a long message and I haven't absorbed the whole thing.  
However it's worth pointing out right at the start that we already have
device_pm_move_before(), device_pm_move_after(), and
device_pm_move_last().  They are intended specifically to provide
drivers with a way of making sure that dpm_list is in the right order 
-- people have been aware of these issues for some time.

Alan Stern

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Thu, Oct 13, 2011 at 02:16:42PM -0400, Alan Stern wrote:
> On Thu, 13 Oct 2011, Grant Likely wrote:
> 
> > For the deferred case; here is an example of the additional
> > constraint.  Consider the following hierarchy:
> > 
> > -A
> >  +-B
> >  | +-C
> >  | +-D
> >  |
> >  +-E
> >    +-F
> >    +-G
> > 
> > dpm_list could be ordered in at least the following ways (depending on
> > exactly when devices get registered).  There are many permutation, but
> > children are always be listed after its direct parent.
> > 
> > 1) ABECDFG (breadth first)
> > 2) AEBFGCD (breadth first)
> > 3) ABCDEFG (depth first)
> > 4) AEFGBCD (depth first)
> > 
> > Now, assume that device B depends on device F, and also assume that
> > there is no way either to express that in the hierarchy or even for
> > the constraint to be known at device registration time (the is exactly
> > the situation we're dealing with on embedded platforms).  Only the
> > driver for B knows that it needs a resource provided by F's driver.
> > So, the situation becomes that the ordering of dpm_list must now also
> > be sorted so that non-tree dependencies are also accounted for.  Of
> > the list above, only sort order 4 meets the new constraint.
> > 
> > The question then becomes, how can the dpm_list get resorted
> > dynamically at runtime to ensure that the new constraints are always
> > met without breaking old ones.  Here are some options I can think of:
> 
> This was a long message and I haven't absorbed the whole thing.  

heh; I did get rather verbose there.

> However it's worth pointing out right at the start that we already have
> device_pm_move_before(), device_pm_move_after(), and
> device_pm_move_last().  They are intended specifically to provide
> drivers with a way of making sure that dpm_list is in the right order 
> -- people have been aware of these issues for some time.

I saw those.  I also notice that they are only used by device_move()
when reparenting a device (which is another wrinkle I hadn't though
about).  Reparenting a device becomes problematic if the probe order
is also represented in the list.  If device_move() gets called, then
any implicit probe-order sorting for that device would be lost.

I also notice that device_move disregards any children when moving a
device, which could also be a problem.

Although it looks like the only users of device_move are:

drivers/media/video/pvrusb2/pvrusb2-v4l2.c
drivers/s390/cio/device.c
net/bluetooth/hci_sysfs.c
net/bluetooth/rfcomm/tty.c

So it may not be significant to adapt.

g.

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Alan Stern]

On Thu, 13 Oct 2011, Grant Likely wrote:

> > However it's worth pointing out right at the start that we already have
> > device_pm_move_before(), device_pm_move_after(), and
> > device_pm_move_last().  They are intended specifically to provide
> > drivers with a way of making sure that dpm_list is in the right order 
> > -- people have been aware of these issues for some time.
> 
> I saw those.  I also notice that they are only used by device_move()
> when reparenting a device (which is another wrinkle I hadn't though
> about).  Reparenting a device becomes problematic if the probe order
> is also represented in the list.  If device_move() gets called, then
> any implicit probe-order sorting for that device would be lost.
> 
> I also notice that device_move disregards any children when moving a
> device, which could also be a problem.
> 
> Although it looks like the only users of device_move are:
> 
> drivers/media/video/pvrusb2/pvrusb2-v4l2.c
> drivers/s390/cio/device.c
> net/bluetooth/hci_sysfs.c
> net/bluetooth/rfcomm/tty.c
> 
> So it may not be significant to adapt.

I trust that very little will be needed.  I haven't checked the 
existing callers, but it's reasonable to require that a device being 
moved not have any children.

Alan Stern

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Fri, Oct 14, 2011 at 9:37 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 13 Oct 2011, Grant Likely wrote:
>
>> For the deferred case; here is an example of the additional
>> constraint.  Consider the following hierarchy:
>>
>> -A
>>  +-B
>>  | +-C
>>  | +-D
>>  |
>>  +-E
>>    +-F
>>    +-G
>>
>> dpm_list could be ordered in at least the following ways (depending on
>> exactly when devices get registered).  There are many permutation, but
>> children are always be listed after its direct parent.
>>
>> 1) ABECDFG (breadth first)
>> 2) AEBFGCD (breadth first)
>> 3) ABCDEFG (depth first)
>> 4) AEFGBCD (depth first)
>>
>> Now, assume that device B depends on device F, and also assume that
>> there is no way either to express that in the hierarchy or even for
>> the constraint to be known at device registration time (the is exactly
>> the situation we're dealing with on embedded platforms).  Only the
>> driver for B knows that it needs a resource provided by F's driver.
>> So, the situation becomes that the ordering of dpm_list must now also
>> be sorted so that non-tree dependencies are also accounted for.  Of
>> the list above, only sort order 4 meets the new constraint.
>>
>> The question then becomes, how can the dpm_list get resorted
>> dynamically at runtime to ensure that the new constraints are always
>> met without breaking old ones.  Here are some options I can think of:
>>
>> 1) When a deferred probe succeeds, move the deferred device to the
>> end of the dpm_list.  Then the new sort order might be one of:
>>
>>       1) AECDFGB
>>       2) AEFGCDB
>>       3) ACDEFGB
>>       4) AEFGCDB
>>
>> However, that approach breaks the guarantee that children appear after
>> their parents (C & D appear before B in all cases above)
>
> How can a device acquire children before it has a driver?

There are a few potential situations in embedded systems (or at least
nothing currently prevents it) where platform setup code constructs a
device hierarchy without the aid of device drivers, and it is still
possible for a parent device to be attached to a driver.  IIUC, SPARC
creates an entire hierarchy of platform_devices from all the nodes in
the OpenFirmware device tree, and any of those devices can be bound to
a driver.  I don't like that approach, but at the very least it needs
to be guarded against.

On Fri, Oct 14, 2011 at 9:39 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 13 Oct 2011, Grant Likely wrote:
>> I saw those. ?I also notice that they are only used by device_move()
>> when reparenting a device (which is another wrinkle I hadn't though
>> about). ?Reparenting a device becomes problematic if the probe order
>> is also represented in the list. ?If device_move() gets called, then
>> any implicit probe-order sorting for that device would be lost.
>>
>> I also notice that device_move disregards any children when moving a
>> device, which could also be a problem.
>>
>> Although it looks like the only users of device_move are:
>>
>> drivers/media/video/pvrusb2/pvrusb2-v4l2.c
>> drivers/s390/cio/device.c
>> net/bluetooth/hci_sysfs.c
>> net/bluetooth/rfcomm/tty.c
>>
>> So it may not be significant to adapt.
>
> I trust that very little will be needed. ?I haven't checked the
> existing callers, but it's reasonable to require that a device being
> moved not have any children.

Yes, that does indeed simplify the issue considerably.  Let's do that.
 So, for this patch, there will be two bits added.  First, don't allow
deferral on devices with children, and second a successful probe
(deferred or otherwise) should always move a device to the end of the
dap_list if it doesn't have children to guarantee that the list order
satisfies both the hierarchical and probe order.  Manjunath, do you
think you can prototype this?

g.

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

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Alan Stern]

On Fri, 14 Oct 2011, Grant Likely wrote:

> > How can a device acquire children before it has a driver?
> 
> There are a few potential situations in embedded systems (or at least
> nothing currently prevents it) where platform setup code constructs a
> device hierarchy without the aid of device drivers, and it is still
> possible for a parent device to be attached to a driver.  IIUC, SPARC
> creates an entire hierarchy of platform_devices from all the nodes in
> the OpenFirmware device tree, and any of those devices can be bound to
> a driver.  I don't like that approach, but at the very least it needs
> to be guarded against.

Do these devices ever require deferred probes?

> On Fri, Oct 14, 2011 at 9:39 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 13 Oct 2011, Grant Likely wrote:
> >> I saw those. ?I also notice that they are only used by device_move()
> >> when reparenting a device (which is another wrinkle I hadn't though
> >> about). ?Reparenting a device becomes problematic if the probe order
> >> is also represented in the list. ?If device_move() gets called, then
> >> any implicit probe-order sorting for that device would be lost.
> >>
> >> I also notice that device_move disregards any children when moving a
> >> device, which could also be a problem.
> >>
> >> Although it looks like the only users of device_move are:
> >>
> >> drivers/media/video/pvrusb2/pvrusb2-v4l2.c
> >> drivers/s390/cio/device.c
> >> net/bluetooth/hci_sysfs.c
> >> net/bluetooth/rfcomm/tty.c
> >>
> >> So it may not be significant to adapt.
> >
> > I trust that very little will be needed. ?I haven't checked the
> > existing callers, but it's reasonable to require that a device being
> > moved not have any children.
> 
> Yes, that does indeed simplify the issue considerably.  Let's do that.
>  So, for this patch, there will be two bits added.  First, don't allow
> deferral on devices with children, and second a successful probe
> (deferred or otherwise) should always move a device to the end of the
> dap_list if it doesn't have children to guarantee that the list order
> satisfies both the hierarchical and probe order.  Manjunath, do you
> think you can prototype this?

I don't think the second part needs to be quite so invasive.  
Certainly drivers that never defer probes shouldn't require anything to
be moved.

A deferred probe _should_ move the device to the end of the list.  But
this needs to happen in the probe routine itself (after it has verified
that all the other required devices are present and before it has
registered any children) to prevent races.  It can't be done in a
central location.

Alan Stern

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Fri, Oct 14, 2011 at 10:33 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 14 Oct 2011, Grant Likely wrote:
>
>> > How can a device acquire children before it has a driver?
>>
>> There are a few potential situations in embedded systems (or at least
>> nothing currently prevents it) where platform setup code constructs a
>> device hierarchy without the aid of device drivers, and it is still
>> possible for a parent device to be attached to a driver. ?IIUC, SPARC
>> creates an entire hierarchy of platform_devices from all the nodes in
>> the OpenFirmware device tree, and any of those devices can be bound to
>> a driver. ?I don't like that approach, but at the very least it needs
>> to be guarded against.
>
> Do these devices ever require deferred probes?

Yes, they very well might.  However, I'm happy with the limitation
that only leaf devices can take advantage of probe deferral.

>> On Fri, Oct 14, 2011 at 9:39 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Thu, 13 Oct 2011, Grant Likely wrote:
>> Yes, that does indeed simplify the issue considerably. ?Let's do that.
>> ?So, for this patch, there will be two bits added. ?First, don't allow
>> deferral on devices with children, and second a successful probe
>> (deferred or otherwise) should always move a device to the end of the
>> dap_list if it doesn't have children to guarantee that the list order
>> satisfies both the hierarchical and probe order. ?Manjunath, do you
>> think you can prototype this?
>
> I don't think the second part needs to be quite so invasive.
> Certainly drivers that never defer probes shouldn't require anything to
> be moved.

Think about that a minute.  Consider a dpm_list of devices:
        abcDefGh

Now assume that D has an implicit dependency on G.  If D gets probed
first, then it will be deferred until G gets probed and then D will
get retried and moved to the end of the list resulting in:
        abcefGhD
Everything is good now for the order that things need to be suspended in.

Now lets assume that the drivers get linked into the kernel in a
different order (or the modules get loaded in a different order) and G
gets probed first, followed by D.  No deferral occurred and so no
reordering occurs, resulting in:
        abcDefGh (unchanged)
But now suspend is broken because D depends on G, but G will be
suspended before D.  This looks and smells like a bug to me.  In fact,
even without using probe deferral it looks like a bug because the
dap_list isn't taking into account the fact that there are very likely
to be implicit dependencies between devices that are not represented
in the device hierarchy (maybe not common in PCs, but certainly is the
case for embedded).  But, it is also easy to solve by ensuring the
dap_list is also probe-order sorted.

> A deferred probe _should_ move the device to the end of the list. ?But
> this needs to happen in the probe routine itself (after it has verified
> that all the other required devices are present and before it has
> registered any children) to prevent races. ?It can't be done in a
> central location.

I'm really concerned about drivers having to implement this and not
getting it correct; particularly when moving a device to the end of
the list is cheap, and it shouldn't be a problem to do the move
unconditionally after a driver has been matches, but before probe() is
called.  We may be able to keep watch to make sure that drivers using
probe deferral are also reordering themselves, but that does nothing
for the cases described above where the link order of the drivers
determines probe order, not the dap_list order.

g.


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

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Alan Stern]

On Fri, 14 Oct 2011, Grant Likely wrote:

> > I don't think the second part needs to be quite so invasive.
> > Certainly drivers that never defer probes shouldn't require anything to
> > be moved.
> 
> Think about that a minute.  Consider a dpm_list of devices:
>         abcDefGh
> 
> Now assume that D has an implicit dependency on G.  If D gets probed
> first, then it will be deferred until G gets probed and then D will
> get retried and moved to the end of the list resulting in:
>         abcefGhD
> Everything is good now for the order that things need to be suspended in.
> 
> Now lets assume that the drivers get linked into the kernel in a
> different order (or the modules get loaded in a different order) and G
> gets probed first, followed by D.  No deferral occurred and so no
> reordering occurs, resulting in:
>         abcDefGh (unchanged)
> But now suspend is broken because D depends on G, but G will be
> suspended before D.

However D sometimes does defer probes.  Therefore the device always
needs to be moved, even though this particular probe wasn't deferred.

>  This looks and smells like a bug to me.  In fact,
> even without using probe deferral it looks like a bug because the
> dap_list isn't taking into account the fact that there are very likely
> to be implicit dependencies between devices that are not represented
> in the device hierarchy (maybe not common in PCs, but certainly is the
> case for embedded).  But, it is also easy to solve by ensuring the
> dap_list is also probe-order sorted.
> 
> > A deferred probe _should_ move the device to the end of the list. ?But
> > this needs to happen in the probe routine itself (after it has verified
> > that all the other required devices are present and before it has
> > registered any children) to prevent races. ?It can't be done in a
> > central location.
> 
> I'm really concerned about drivers having to implement this and not
> getting it correct; particularly when moving a device to the end of
> the list is cheap, and it shouldn't be a problem to do the move
> unconditionally after a driver has been matches, but before probe() is
> called.

But that's too early.  What if D gets moved to the end of the list, 
then G gets added to the list and probed, and then D's probe succeeds?

And after the probe returns is too late, because by then there may well 
be child devices.

>  We may be able to keep watch to make sure that drivers using
> probe deferral are also reordering themselves, but that does nothing
> for the cases described above where the link order of the drivers
> determines probe order, not the dap_list order.

Devices need to be moved whenever they have any external dependencies,
regardless of the particular order they get probed in.

Alan Stern

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Fri, Oct 14, 2011 at 11:33 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 14 Oct 2011, Grant Likely wrote:
>
>> > I don't think the second part needs to be quite so invasive.
>> > Certainly drivers that never defer probes shouldn't require anything to
>> > be moved.
>>
>> Think about that a minute. ?Consider a dpm_list of devices:
>> ? ? ? ? abcDefGh
>>
>> Now assume that D has an implicit dependency on G. ?If D gets probed
>> first, then it will be deferred until G gets probed and then D will
>> get retried and moved to the end of the list resulting in:
>> ? ? ? ? abcefGhD
>> Everything is good now for the order that things need to be suspended in.
>>
>> Now lets assume that the drivers get linked into the kernel in a
>> different order (or the modules get loaded in a different order) and G
>> gets probed first, followed by D. ?No deferral occurred and so no
>> reordering occurs, resulting in:
>> ? ? ? ? abcDefGh (unchanged)
>> But now suspend is broken because D depends on G, but G will be
>> suspended before D.
>
> However D sometimes does defer probes. ?Therefore the device always
> needs to be moved, even though this particular probe wasn't deferred.

Yes, that's part of my point.

>> ?This looks and smells like a bug to me. ?In fact,
>> even without using probe deferral it looks like a bug because the
>> dap_list isn't taking into account the fact that there are very likely
>> to be implicit dependencies between devices that are not represented
>> in the device hierarchy (maybe not common in PCs, but certainly is the
>> case for embedded). ?But, it is also easy to solve by ensuring the
>> dap_list is also probe-order sorted.
>>
>> > A deferred probe _should_ move the device to the end of the list. ?But
>> > this needs to happen in the probe routine itself (after it has verified
>> > that all the other required devices are present and before it has
>> > registered any children) to prevent races. ?It can't be done in a
>> > central location.
>>
>> I'm really concerned about drivers having to implement this and not
>> getting it correct; particularly when moving a device to the end of
>> the list is cheap, and it shouldn't be a problem to do the move
>> unconditionally after a driver has been matches, but before probe() is
>> called.
>
> But that's too early. ?What if D gets moved to the end of the list,
> then G gets added to the list and probed, and then D's probe succeeds?

So the argument is that if really_probe() called dpm_move_last()
immediately before the dev->bus->probe()/drv->probe() call then there
is a race condition that G could get both registered and probed before
D's probe() starts using G's resources.  And so, the list would still
have G after D which is in the wrong order.  Do I understand
correctly?

> And after the probe returns is too late, because by then there may well
> be child devices.

Agreed, after probe returns is definitely too late.

>> ?We may be able to keep watch to make sure that drivers using
>> probe deferral are also reordering themselves, but that does nothing
>> for the cases described above where the link order of the drivers
>> determines probe order, not the dap_list order.
>
> Devices need to be moved whenever they have any external dependencies,
> regardless of the particular order they get probed in.

I suspect this gets messy in a hurry, but perhaps it is worth trying.
So, any device that makes use of a PHY, GPIO line, codec, etc will
need to call dpm_move_last() before adding child devices, correct?
I'd be much happier to find a way to do this in core code though.  And
there is still a potential race condition here.  For example, if G is
in the middle of it's probe routine, and D gets probed between G
registering GPIOs and calling dpm_move_last(), then we're in the same
boat again.  I think the window for this race can be considered to be
of the same magnitude as the moved to early race described above.  I
need to think about this more...

g.

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

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Alan Stern]

On Fri, 14 Oct 2011, Grant Likely wrote:

> > However D sometimes does defer probes. ?Therefore the device always
> > needs to be moved, even though this particular probe wasn't deferred.
> 
> Yes, that's part of my point.

Okay, then we agree. :-)

> So the argument is that if really_probe() called dpm_move_last()
> immediately before the dev->bus->probe()/drv->probe() call then there
> is a race condition that G could get both registered and probed before
> D's probe() starts using G's resources.  And so, the list would still
> have G after D which is in the wrong order.  Do I understand
> correctly?

Exactly so.

> > Devices need to be moved whenever they have any external dependencies,
> > regardless of the particular order they get probed in.
> 
> I suspect this gets messy in a hurry, but perhaps it is worth trying.
> So, any device that makes use of a PHY, GPIO line, codec, etc will
> need to call dpm_move_last() before adding child devices, correct?

Pretty much, yes.  Unless the driver somehow knows that it will become
sufficiently idle at an early suspend stage (like the prepare callback) 
that it doesn't need to use the PHY, GPIO, codec, etc.

> I'd be much happier to find a way to do this in core code though.  And
> there is still a potential race condition here.  For example, if G is
> in the middle of it's probe routine, and D gets probed between G
> registering GPIOs and calling dpm_move_last(), then we're in the same
> boat again.

Of course, this means that G must call dpm_move_last() _before_ 
registering its GPIOs.  So the overall flow of a probe routine is 
simple enough:

	1. Check that all the resources you need are available.

	2. If not, defer your probe.  If yes, call dpm_move_last().

	3. Finish the probe, including registration of resources
	   that will be available to other drivers (such as child
	   devices).

>  I think the window for this race can be considered to be
> of the same magnitude as the moved to early race described above.  I
> need to think about this more...

Alan Stern

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Fri, Oct 14, 2011 at 12:39 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 14 Oct 2011, Grant Likely wrote:
>> I'd be much happier to find a way to do this in core code though. ?And
>> there is still a potential race condition here. ?For example, if G is
>> in the middle of it's probe routine, and D gets probed between G
>> registering GPIOs and calling dpm_move_last(), then we're in the same
>> boat again.
>
> Of course, this means that G must call dpm_move_last() _before_
> registering its GPIOs. ?So the overall flow of a probe routine is
> simple enough:
>
> ? ? ? ?1. Check that all the resources you need are available.
>
> ? ? ? ?2. If not, defer your probe. ?If yes, call dpm_move_last().
>
> ? ? ? ?3. Finish the probe, including registration of resources
> ? ? ? ? ? that will be available to other drivers (such as child
> ? ? ? ? ? devices).

Alright, let's start with this.  That also means that the current
probe deferral patch doesn't need to have any knowledge of dpm_list
added to it.  It will be required only of the users.

I'd still like to look closely at the ordering of dpm_list for the
non-deferred use case, but that can be an entirely separate patch set.
 It doesn't need to block the probe deferral work.

g.

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[David Daney]

On 10/14/2011 10:20 AM, Grant Likely wrote:
> On Fri, Oct 14, 2011 at 10:33 AM, Alan Stern<stern@rowland.harvard.edu>  wrote:
>> On Fri, 14 Oct 2011, Grant Likely wrote:
>>
>>>> How can a device acquire children before it has a driver?
>>>
>>> There are a few potential situations in embedded systems (or at least
>>> nothing currently prevents it) where platform setup code constructs a
>>> device hierarchy without the aid of device drivers, and it is still
>>> possible for a parent device to be attached to a driver.  IIUC, SPARC
>>> creates an entire hierarchy of platform_devices from all the nodes in
>>> the OpenFirmware device tree, and any of those devices can be bound to
>>> a driver.  I don't like that approach, but at the very least it needs
>>> to be guarded against.
>>
>> Do these devices ever require deferred probes?
>
> Yes, they very well might.  However, I'm happy with the limitation
> that only leaf devices can take advantage of probe deferral.
>


I have:

I2C-Bus-A
   +--Mux-I2C (controlled by parent I2C-Bus-A)
       +---I2C-Bus-1
       |      +--GPIO-Expander-A
       |
       +---I2C-Bus-2
              +--GPIO-Expander-B

These all have a parent/child relationship so no deferral is needed, so 
far so good.


Then this:

MDIO-Bus-A
    +---Mux-MDIO (controlled by GPIO-Expander-A)
          +---MDIO-Bus-1
          |
          +---MDIO-Bus-2
                +---PHY-1
                |
                +---PHY-2

In this case the driver for Mux-MDIO needs to be deferred until *both* 
MDIO-Bus-A's driver *and* GPIO-Expander-B's driver are loaded.  A 
perfect use case for the patch.

Would you consider Mux-MDIO to be a 'leaf device'?  If not, then I have 
real problems with 'the limitation that only leaf devices can take 
advantage of probe deferral'

David Daney

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Fri, Oct 14, 2011 at 12:56 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 10/14/2011 10:20 AM, Grant Likely wrote:
>>
>> On Fri, Oct 14, 2011 at 10:33 AM, Alan Stern<stern@rowland.harvard.edu>
>> ?wrote:
>>>
>>> On Fri, 14 Oct 2011, Grant Likely wrote:
>>>
>>>>> How can a device acquire children before it has a driver?
>>>>
>>>> There are a few potential situations in embedded systems (or at least
>>>> nothing currently prevents it) where platform setup code constructs a
>>>> device hierarchy without the aid of device drivers, and it is still
>>>> possible for a parent device to be attached to a driver. ?IIUC, SPARC
>>>> creates an entire hierarchy of platform_devices from all the nodes in
>>>> the OpenFirmware device tree, and any of those devices can be bound to
>>>> a driver. ?I don't like that approach, but at the very least it needs
>>>> to be guarded against.
>>>
>>> Do these devices ever require deferred probes?
>>
>> Yes, they very well might. ?However, I'm happy with the limitation
>> that only leaf devices can take advantage of probe deferral.
>>
>
>
> I have:
>
> I2C-Bus-A
> ?+--Mux-I2C (controlled by parent I2C-Bus-A)
> ? ? ?+---I2C-Bus-1
> ? ? ?| ? ? ?+--GPIO-Expander-A
> ? ? ?|
> ? ? ?+---I2C-Bus-2
> ? ? ? ? ? ? +--GPIO-Expander-B
>
> These all have a parent/child relationship so no deferral is needed, so far
> so good.
>
>
> Then this:
>
> MDIO-Bus-A
> ? +---Mux-MDIO (controlled by GPIO-Expander-A)
> ? ? ? ? +---MDIO-Bus-1
> ? ? ? ? |
> ? ? ? ? +---MDIO-Bus-2
> ? ? ? ? ? ? ? +---PHY-1
> ? ? ? ? ? ? ? |
> ? ? ? ? ? ? ? +---PHY-2
>
> In this case the driver for Mux-MDIO needs to be deferred until *both*
> MDIO-Bus-A's driver *and* GPIO-Expander-B's driver are loaded. ?A perfect
> use case for the patch.
>
> Would you consider Mux-MDIO to be a 'leaf device'? ?If not, then I have real
> problems with 'the limitation that only leaf devices can take advantage of
> probe deferral'

leaf device **at the time of its driver probe**.  :-)  After the
device has all of its dependencies met, it can freely add child
devices.  In your case, the child devices will get added by the
Mux-MDIO device driver, so all is good.

g.

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[David Daney]

On 10/14/2011 12:03 PM, Grant Likely wrote:
> On Fri, Oct 14, 2011 at 12:56 PM, David Daney<ddaney.cavm@gmail.com>  wrote:
>> On 10/14/2011 10:20 AM, Grant Likely wrote:
>>>
>>> On Fri, Oct 14, 2011 at 10:33 AM, Alan Stern<stern@rowland.harvard.edu>
>>>   wrote:
>>>>
>>>> On Fri, 14 Oct 2011, Grant Likely wrote:
>>>>
>>>>>> How can a device acquire children before it has a driver?
>>>>>
>>>>> There are a few potential situations in embedded systems (or at least
>>>>> nothing currently prevents it) where platform setup code constructs a
>>>>> device hierarchy without the aid of device drivers, and it is still
>>>>> possible for a parent device to be attached to a driver.  IIUC, SPARC
>>>>> creates an entire hierarchy of platform_devices from all the nodes in
>>>>> the OpenFirmware device tree, and any of those devices can be bound to
>>>>> a driver.  I don't like that approach, but at the very least it needs
>>>>> to be guarded against.
>>>>
>>>> Do these devices ever require deferred probes?
>>>
>>> Yes, they very well might.  However, I'm happy with the limitation
>>> that only leaf devices can take advantage of probe deferral.
>>>
>>
>>
>> I have:
>>
>> I2C-Bus-A
>>   +--Mux-I2C (controlled by parent I2C-Bus-A)
>>       +---I2C-Bus-1
>>       |      +--GPIO-Expander-A
>>       |
>>       +---I2C-Bus-2
>>              +--GPIO-Expander-B
>>
>> These all have a parent/child relationship so no deferral is needed, so far
>> so good.
>>
>>
>> Then this:
>>
>> MDIO-Bus-A
>>    +---Mux-MDIO (controlled by GPIO-Expander-A)
>>          +---MDIO-Bus-1
>>          |
>>          +---MDIO-Bus-2
>>                +---PHY-1
>>                |
>>                +---PHY-2
>>
>> In this case the driver for Mux-MDIO needs to be deferred until *both*
>> MDIO-Bus-A's driver *and* GPIO-Expander-B's driver are loaded.  A perfect
>> use case for the patch.
>>
>> Would you consider Mux-MDIO to be a 'leaf device'?  If not, then I have real
>> problems with 'the limitation that only leaf devices can take advantage of
>> probe deferral'
>
> leaf device **at the time of its driver probe**.  :-)  After the
> device has all of its dependencies met, it can freely add child
> devices.  In your case, the child devices will get added by the
> Mux-MDIO device driver, so all is good.
>

Indeed.  Thanks for the confirmation.

David Daney

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Alan Stern]

On Thu, 13 Oct 2011, Grant Likely wrote:

> For the deferred case; here is an example of the additional
> constraint.  Consider the following hierarchy:
> 
> -A
>  +-B
>  | +-C
>  | +-D
>  |
>  +-E
>    +-F
>    +-G
> 
> dpm_list could be ordered in at least the following ways (depending on
> exactly when devices get registered).  There are many permutation, but
> children are always be listed after its direct parent.
> 
> 1) ABECDFG (breadth first)
> 2) AEBFGCD (breadth first)
> 3) ABCDEFG (depth first)
> 4) AEFGBCD (depth first)
> 
> Now, assume that device B depends on device F, and also assume that
> there is no way either to express that in the hierarchy or even for
> the constraint to be known at device registration time (the is exactly
> the situation we're dealing with on embedded platforms).  Only the
> driver for B knows that it needs a resource provided by F's driver.
> So, the situation becomes that the ordering of dpm_list must now also
> be sorted so that non-tree dependencies are also accounted for.  Of
> the list above, only sort order 4 meets the new constraint.
> 
> The question then becomes, how can the dpm_list get resorted
> dynamically at runtime to ensure that the new constraints are always
> met without breaking old ones.  Here are some options I can think of:
> 
> 1) When a deferred probe succeeds, move the deferred device to the
> end of the dpm_list.  Then the new sort order might be one of:
> 
> 	1) AECDFGB
> 	2) AEFGCDB
> 	3) ACDEFGB
> 	4) AEFGCDB
> 
> However, that approach breaks the guarantee that children appear after
> their parents (C & D appear before B in all cases above)

How can a device acquire children before it has a driver?

> 2a) When a deferred probe succeeds, move the deferred device and it's
> entire child hierarchy to the end of the list to become one of:

This is the same as the above, under the reasonable assumption that
devices without drivers can't have any children.  Or to be more
carefully precise, that a device with children won't need to defer a
probe.

We can check that easily enough: Fail a deferral if the device already 
has children.

> 2b) alternately, when *any* probe succeeds, move the deferred device
> and its children to the end of the list.  This continues to maintain
> the parent-child relationship, and it ensures that the dpm_list is
> always also sorted in probe-order (devices bound to drivers will
> collect at the end of the list).

We do not want to move entire hierarchies.  And in fact, even in 1 or
2a, we would have to do the move from _within_ the deferred probe
routine, not afterward, to make sure that it occurs before any children
are added (and after all the prerequisite devices have been 
registered).

> 3) Add devices to dpm_list after successful probe instead of at
> device_add time.
> 
> Potential problem: this means that only devices with drivers actually
> get suspend/resume calls.  I don't know nearly enough about the PM
> subsystem, but I suspect that this is a problem.

Yes, this is no good.

> 4) ignore parent-child relationships entirely and just move devices to
> the end of the list on successful probe.  If it probed successfully,
> then it can be successfully suspended regardless of whether it has any
> children.  That breaks the parent-child ordering, but guarantees the
> probe order ordering.  Any devices without drivers will end up
> collecting at the beginning of the list, and will be suspended after
> all the devices with drivers.
> 
> Problem: Same as with 3, I suspect that even devices without drivers
> need to process PM suspend, which makes this option unworkable.
> 
> ...
> 
> For my money, I think that option 2b shows the most promise despite
> the potential O(N^2) complexity.  There may be a better algorithm for
> doing the runtime reordering that isn't O(N^2) that I haven't thought
> of.  Having a list that is strongly ordered both on hierarchy *and*
> probe order I think is the right thing to do, even without deferred
> probe support.

The answer is 1, but do the move at the appropriate time within the 
probe routine.

Alan Stern

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[G, Manjunath Kondaiah]

On Mon, Oct 10, 2011 at 10:37:22AM -0700, Andrei Warkentin wrote:
> Hi,
> 
> ----- Original Message -----
> > From: "Greg KH" <greg@kroah.com>
> > To: "Josh Triplett" <josh@joshtriplett.org>
> > Cc: "G, Manjunath Kondaiah" <manjugk@ti.com>, linux-arm-kernel at lists.infradead.org, "Grant Likely"
> > <grant.likely@secretlab.ca>, linux-omap at vger.kernel.org, linux-mmc at vger.kernel.org, linux-kernel at vger.kernel.org,
> > "Dilan Lee" <dilee@nvidia.com>, "Mark Brown" <broonie@opensource.wolfsonmicro.com>, Manjunath at jasper.es
> > Sent: Saturday, October 8, 2011 11:55:02 AM
> > Subject: Re: [PATCH 2/5] drivercore: Add driver probe deferral mechanism
> > 
> 
> I'm a bit of a fly on the wall here, but I'm curious how this impacts suspend/resume.
> device_initialize->device_pm_init are called from device_register, so certainly this
> patch doesn't also ensure that the PM ordering matches probe ordering, which is bound
> to break suspend, right? Was this ever tested with the OMAP target? Shouldn't the
> PM change be also part of this patch set? I don't see why you would want to have this in
> without the PM changes.

suspend/resume handling is already in TODO list:
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/135461

-M

> 
> Maybe I have it all wrong though :-).
> 
> A

[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Fri, Oct 7, 2011 at 12:49 AM, Greg KH <greg@kroah.com> wrote:
> On Fri, Oct 07, 2011 at 10:33:07AM +0500, G, Manjunath Kondaiah wrote:
>> +config PROBE_DEFER
>> + ? ? bool "Deferred Driver Probe"
>> + ? ? default y
>> + ? ? help
>> + ? ? ? This option provides deferring driver probe if it has dependency on
>> + ? ? ? other driver. Without this feature, initcall ordering should be done
>> + ? ? ? manually to resolve driver dependencies. This feature completely side
>> + ? ? ? steps 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 is this even an option? ?Why would you ever want it disabled? ?Why
> does it need to be selected?
>
> If you are going to default something to 'y' then just make it so it
> can't be turned off any other way by just not making it an option at
> all.
>
> It also cleans up this diff a lot, as you really don't want #ifdef in .c
> files.

Okay, we'll drop the kconfig

>> + ? ? ?* 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:
>
> That's what the klist structure is supposed to make easier, why not use
> that here?
>
>> + ? ? ?* - 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.
>
> Ick, use a klist, it was created to handle this exact problem in the
> driver core, so to not use it would be wrong, right?

This comment block is stale.  I reworked the code before I handed it
off to Manjunath, but I never rewrote the comment.  The current code
uses a pair of list to keep deferred devices separate from devices
currently being probed, and the problem described above no longer
exists.  However, Manjunath and I will look into switching from the
two list design to using klist.

Thanks for the feedback.
g.

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

[PATCH 0/5] Driver Probe Deferral Mechanism

[Greg KH]

On Fri, Oct 07, 2011 at 10:33:05AM +0500, G, Manjunath Kondaiah wrote:

Why did you send this series of patches out twice?  Were they different?

confused,

greg k-h

[PATCH 0/5] Driver Probe Deferral Mechanism

[G, Manjunath Kondaiah]

Hi Greg,

On Thu, Oct 06, 2011 at 11:50:42PM -0700, Greg KH wrote:
> On Fri, Oct 07, 2011 at 10:33:05AM +0500, G, Manjunath Kondaiah wrote:
> 
> Why did you send this series of patches out twice?  Were they different?
> 
> confused,

Looks like this patch series has reached only individual recepients and 
not the mailing lists hence it was posted once again.

Sorry for the spam.

-Manjunath

[PATCH 4/5] gpiolib: handle deferral probe error

[Alan Cox]

On Fri, 07 Oct 2011 10:33:09 +0500
"G, Manjunath Kondaiah" <manjugk@ti.com> wrote:

> 
> The gpio library should return -EPROBE_DEFER in gpio_request
> if gpio driver is not ready.

Why not use the perfectly good existing error codes we have for this ?

We have EAGAIN and EUNATCH both of which look sensible.

[PATCH 4/5] gpiolib: handle deferral probe error

[Grant Likely]

On Fri, Oct 7, 2011 at 4:06 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Fri, 07 Oct 2011 10:33:09 +0500
> "G, Manjunath Kondaiah" <manjugk@ti.com> wrote:
>
>>
>> The gpio library should return -EPROBE_DEFER in gpio_request
>> if gpio driver is not ready.
>
> Why not use the perfectly good existing error codes we have for this ?
>
> We have EAGAIN and EUNATCH both of which look sensible.

I want a distinct error code for probe deferral so that a) it doesn't
overlap with something a driver is already doing, and b) so that all
the users can be found again at a later date.

That said, I'm not in agreement with this patch.  It is fine for gpio
lib to have a code that means the pin doesn't exist (yet), but the
device driver needs to be the one to decide whether or not it is
appropriate to use probe deferral.

g.

[PATCH 4/5] gpiolib: handle deferral probe error

[G, Manjunath Kondaiah]

On Fri, Oct 07, 2011 at 04:09:38PM -0600, Grant Likely wrote:
> On Fri, Oct 7, 2011 at 4:06 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > On Fri, 07 Oct 2011 10:33:09 +0500
> > "G, Manjunath Kondaiah" <manjugk@ti.com> wrote:
> >
> >>
> >> The gpio library should return -EPROBE_DEFER in gpio_request
> >> if gpio driver is not ready.
> >
> > Why not use the perfectly good existing error codes we have for this ?
> >
> > We have EAGAIN and EUNATCH both of which look sensible.
> 
> I want a distinct error code for probe deferral so that a) it doesn't
> overlap with something a driver is already doing, and b) so that all
> the users can be found again at a later date.
> 
> That said, I'm not in agreement with this patch.  It is fine for gpio
> lib to have a code that means the pin doesn't exist (yet), but the
> device driver needs to be the one to decide whether or not it is
> appropriate to use probe deferral.

During gpio_request, driver gpio_request is not available. How can we expect
driver to request deferred probe in this case?

-M

[PATCH 4/5] gpiolib: handle deferral probe error

[Grant Likely]

On Wed, Oct 12, 2011 at 11:44:32AM +0530, G, Manjunath Kondaiah wrote:
> On Fri, Oct 07, 2011 at 04:09:38PM -0600, Grant Likely wrote:
> > On Fri, Oct 7, 2011 at 4:06 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > > On Fri, 07 Oct 2011 10:33:09 +0500
> > > "G, Manjunath Kondaiah" <manjugk@ti.com> wrote:
> > >
> > >>
> > >> The gpio library should return -EPROBE_DEFER in gpio_request
> > >> if gpio driver is not ready.
> > >
> > > Why not use the perfectly good existing error codes we have for this ?
> > >
> > > We have EAGAIN and EUNATCH both of which look sensible.
> > 
> > I want a distinct error code for probe deferral so that a) it doesn't
> > overlap with something a driver is already doing, and b) so that all
> > the users can be found again at a later date.
> > 
> > That said, I'm not in agreement with this patch.  It is fine for gpio
> > lib to have a code that means the pin doesn't exist (yet), but the
> > device driver needs to be the one to decide whether or not it is
> > appropriate to use probe deferral.
> 
> During gpio_request, driver gpio_request is not available. How can we expect
> driver to request deferred probe in this case?

If gpio_request fails, the driver can then explicitly make the
decision to return -EPROBE_DEFER.  It isn't forced to pass on the
error code from gpio_request().

g.