diff mbox

[v2] driver core: add wait event for deferred probe

Message ID 1360637915-11212-1-git-send-email-haojian.zhuang@linaro.org
State Changes Requested
Headers show

Commit Message

Haojian Zhuang Feb. 12, 2013, 2:58 a.m. UTC
do_initcalls() could call all driver initialization code in kernel_init
thread. It means that probe() function will be also called from that
time. After this, kernel could access console & release __init section
in the same thread.

But if device probe fails and moves into deferred probe list, a new
thread is created to reinvoke probe. If the device is serial console,
kernel has to open console failure & release __init section before
reinvoking failure. Because there's no synchronization mechanism.
Now add wait event to synchronize after do_initcalls().

Changelog:
v2: add comments on wait_for_device_probe().

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 drivers/base/dd.c |    8 +++++---
 init/main.c       |    2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

anish singh Feb. 13, 2013, 4:41 p.m. UTC | #1
On Tue, 2013-02-12 at 10:58 +0800, Haojian Zhuang wrote:
> do_initcalls() could call all driver initialization code in kernel_init
> thread. It means that probe() function will be also called from that
> time. After this, kernel could access console & release __init section
> in the same thread.
> 
> But if device probe fails and moves into deferred probe list, a new
> thread is created to reinvoke probe. If the device is serial console,
> kernel has to open console failure & release __init section before
> reinvoking failure. Because there's no synchronization mechanism.
> Now add wait event to synchronize after do_initcalls().
Can you describe the problem you are facing in detail i.e. without this
patch what is happening weird?
> 
> Changelog:
> v2: add comments on wait_for_device_probe().
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  drivers/base/dd.c |    8 +++++---
>  init/main.c       |    2 ++
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 65631015..23db672 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -53,6 +53,9 @@ static LIST_HEAD(deferred_probe_pending_list);
>  static LIST_HEAD(deferred_probe_active_list);
>  static struct workqueue_struct *deferred_wq;
>  
> +static atomic_t probe_count = ATOMIC_INIT(0);
> +static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
> +
>  /**
>   * deferred_probe_work_func() - Retry probing devices in the active list.
>   */
> @@ -77,6 +80,7 @@ static void deferred_probe_work_func(struct work_struct *work)
>  		private = list_first_entry(&deferred_probe_active_list,
>  					typeof(*dev->p), deferred_probe);
>  		dev = private->device;
> +		atomic_dec(&probe_count);
>  		list_del_init(&private->deferred_probe);
>  
>  		get_device(dev);
> @@ -257,9 +261,6 @@ int device_bind_driver(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(device_bind_driver);
>  
> -static atomic_t probe_count = ATOMIC_INIT(0);
> -static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
> -
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
>  	int ret = 0;
> @@ -308,6 +309,7 @@ probe_failed:
>  		/* Driver requested deferred probing */
>  		dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
>  		driver_deferred_probe_add(dev);
> +		atomic_inc(&probe_count);
>  	} else if (ret != -ENODEV && ret != -ENXIO) {
>  		/* driver matched but the probe failed */
>  		printk(KERN_WARNING
> diff --git a/init/main.c b/init/main.c
> index 63534a1..141a6a4 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -786,6 +786,8 @@ static void __init do_basic_setup(void)
>  	do_ctors();
>  	usermodehelper_enable();
>  	do_initcalls();
> +	/* wait all deferred probe finished & prepare to drop __init section */
> +	wait_for_device_probe();

static int __ref kernel_init(void *unused)
{
        kernel_init_freeable();
        /* need to finish all async __init code before freeing the
memory */
        async_synchronize_full();

Is not your wait_for_device_probe() also calling
async_synchronize_full() function?AFAIK this function will send all the
instructions before moving ahead no?My understanding originates from the
below text.

From kernel/async.c file
Subsystem/driver initialization code that scheduled asynchronous probe
functions, but which shares global resources with other
drivers/subsystems that do not use the asynchronous call feature, need
to do a full synchronization with the async_synchronize_full() function,
before returning from their init function. This is to maintain strict
ordering between the asynchronous and synchronous parts of the kernel.

>  }
>  
>  static void __init do_pre_smp_initcalls(void)
Grant Likely Feb. 13, 2013, 9:39 p.m. UTC | #2
On Tue, 12 Feb 2013 10:58:35 +0800, Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
> do_initcalls() could call all driver initialization code in kernel_init
> thread. It means that probe() function will be also called from that
> time. After this, kernel could access console & release __init section
> in the same thread.
> 
> But if device probe fails and moves into deferred probe list, a new
> thread is created to reinvoke probe. If the device is serial console,
> kernel has to open console failure & release __init section before
> reinvoking failure. Because there's no synchronization mechanism.
> Now add wait event to synchronize after do_initcalls().
> 
> Changelog:
> v2: add comments on wait_for_device_probe().
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

As replied to on v1 of your patch, this change will cause a deadlock.
The following change would do the same without the deadlock issue:

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

 	driver_deferred_probe_enable = true;
+	deferred_probe_work_func(NULL);
-	driver_deferred_probe_trigger();
 	return 0;
 }
 late_initcall(deferred_probe_initcall);

g.
diff mbox

Patch

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 65631015..23db672 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -53,6 +53,9 @@  static LIST_HEAD(deferred_probe_pending_list);
 static LIST_HEAD(deferred_probe_active_list);
 static struct workqueue_struct *deferred_wq;
 
+static atomic_t probe_count = ATOMIC_INIT(0);
+static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
+
 /**
  * deferred_probe_work_func() - Retry probing devices in the active list.
  */
@@ -77,6 +80,7 @@  static void deferred_probe_work_func(struct work_struct *work)
 		private = list_first_entry(&deferred_probe_active_list,
 					typeof(*dev->p), deferred_probe);
 		dev = private->device;
+		atomic_dec(&probe_count);
 		list_del_init(&private->deferred_probe);
 
 		get_device(dev);
@@ -257,9 +261,6 @@  int device_bind_driver(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_bind_driver);
 
-static atomic_t probe_count = ATOMIC_INIT(0);
-static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
-
 static int really_probe(struct device *dev, struct device_driver *drv)
 {
 	int ret = 0;
@@ -308,6 +309,7 @@  probe_failed:
 		/* Driver requested deferred probing */
 		dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
 		driver_deferred_probe_add(dev);
+		atomic_inc(&probe_count);
 	} else if (ret != -ENODEV && ret != -ENXIO) {
 		/* driver matched but the probe failed */
 		printk(KERN_WARNING
diff --git a/init/main.c b/init/main.c
index 63534a1..141a6a4 100644
--- a/init/main.c
+++ b/init/main.c
@@ -786,6 +786,8 @@  static void __init do_basic_setup(void)
 	do_ctors();
 	usermodehelper_enable();
 	do_initcalls();
+	/* wait all deferred probe finished & prepare to drop __init section */
+	wait_for_device_probe();
 }
 
 static void __init do_pre_smp_initcalls(void)