[PATCH v1 0/5] driver-core: async probe support

[PATCH v1 0/5] driver-core: async probe support

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

What started off as a witch hunt for a solution to the systemd
kernel-module-loading-timeout-and-it-killing-drivers-issue (TM)
ended up in our last discussions [0] agreement for us to add to the
kernel what systemd expected us to be doing: asynchronous probing.

This implements support for that, but please note that upon further
inspection of what the timeout is doing and implicating (documented on
patch number 5 in this series) its also recommended that systemd
revisit its timeout and perhaps consider simply not killing kmod
workers. Even if the killing goes away this framework can still be
desirable, and its expected this will be expanded to support
built-in drivers as well later.

This goes build tested with allmodconfig, and run time tested
with and without bus.safe_mod_async_probe=1 on 4 different systems.
As noted on patch #5 systemd should not use bus.safe_mod_async_probe=1
but instead issue a request per module so that in other cases where
its not advisable to use async probe (also documented on patch #5)
it can avoid it. We also enable a flag to let drivers specify if
they never want to enable async probe for whatever reason and an
example is provided on how to do this.

This should have 0 impact unless userspace is issuing the module
parameter for modules, or if any of the new module parameters are
used to help test things.

[0] https://lkml.org/lkml/2014/9/12/625

Luis R. Rodriguez (5):
  module: add extra argument for parse_params() callback
  driver-core: enable drivers to opt-out of async probe
  amd64_edac: enforce synchronous probe
  driver-core: generalize freeing driver private member
  driver-core: add driver asynchronous probe support

 arch/powerpc/mm/hugetlbpage.c |   4 +-
 drivers/base/base.h           |   6 ++
 drivers/base/bus.c            | 150 ++++++++++++++++++++++++++++++++++++++++--
 drivers/base/dd.c             |   7 ++
 drivers/edac/amd64_edac.c     |   1 +
 include/linux/device.h        |   5 ++
 include/linux/module.h        |   2 +
 include/linux/moduleparam.h   |   3 +-
 init/main.c                   |  25 ++++---
 kernel/module.c               |  16 ++++-
 kernel/params.c               |  11 ++--
 lib/dynamic_debug.c           |   4 +-
 12 files changed, 206 insertions(+), 28 deletions(-)

-- 
2.1.0

[PATCH v1 1/5] module: add extra argument for parse_params() callback

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This adds an extra argument onto parse_params() to be used
as a way to make the unused callback a bit more useful and
generic by allowing the caller to pass on a data structure
of its choice. An example use case is to allow us to easily
make module parameters for every module which we will do
next.

@ parse @
identifier name, args, params, num, level_min, level_max;
identifier unknown, param, val, doing;
type s16;
@@
 extern char *parse_args(const char *name,
 			 char *args,
 			 const struct kernel_param *params,
 			 unsigned num,
 			 s16 level_min,
 			 s16 level_max,
+			 void *arg,
 			 int (*unknown)(char *param, char *val,
					const char *doing
+					, void *arg,
					));

@ parse_mod @
identifier name, args, params, num, level_min, level_max;
identifier unknown, param, val, doing;
type s16;
@@
 char *parse_args(const char *name,
 			 char *args,
 			 const struct kernel_param *params,
 			 unsigned num,
 			 s16 level_min,
 			 s16 level_max,
+			 void *arg,
 			 int (*unknown)(char *param, char *val,
					const char *doing
+					, void *arg,
					))
{
	...
}

@ parse_args_found @
expression R, E1, E2, E3, E4, E5, E6;
identifier func;
@@

(
	R =
	parse_args(E1, E2, E3, E4, E5, E6,
+		   NULL,
		   func);
|
	R =
	parse_args(E1, E2, E3, E4, E5, E6,
+		   NULL,
		   &func);
|
	R =
	parse_args(E1, E2, E3, E4, E5, E6,
+		   NULL,
		   NULL);
|
	parse_args(E1, E2, E3, E4, E5, E6,
+		   NULL,
		   func);
|
	parse_args(E1, E2, E3, E4, E5, E6,
+		   NULL,
		   &func);
|
	parse_args(E1, E2, E3, E4, E5, E6,
+		   NULL,
		   NULL);
)

@ parse_args_unused depends on parse_args_found @
identifier parse_args_found.func;
@@

int func(char *param, char *val, const char *unused
+		 , void *arg
		 )
{
	...
}

@ mod_unused depends on parse_args_found @
identifier parse_args_found.func;
expression A1, A2, A3;
@@

-	func(A1, A2, A3);
+	func(A1, A2, A3, NULL);

Generated-by: Coccinelle SmPL
Cc: cocci@systeme.lip6.fr
Cc: Tejun Heo <tj@kernel.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Felipe Contreras <felipe.contreras@gmail.com>
Cc: Ewan Milne <emilne@redhat.com>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 arch/powerpc/mm/hugetlbpage.c |  4 ++--
 include/linux/moduleparam.h   |  3 ++-
 init/main.c                   | 25 +++++++++++++++----------
 kernel/module.c               |  6 ++++--
 kernel/params.c               | 11 +++++++----
 lib/dynamic_debug.c           |  4 ++--
 6 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index 7e70ae9..f3f836e 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -333,7 +333,7 @@ int alloc_bootmem_huge_page(struct hstate *hstate)
 unsigned long gpage_npages[MMU_PAGE_COUNT];
 
 static int __init do_gpage_early_setup(char *param, char *val,
-				       const char *unused)
+				       const char *unused, void *arg)
 {
 	static phys_addr_t size;
 	unsigned long npages;
@@ -375,7 +375,7 @@ void __init reserve_hugetlb_gpages(void)
 
 	strlcpy(cmdline, boot_command_line, COMMAND_LINE_SIZE);
 	parse_args("hugetlb gpages", cmdline, NULL, 0, 0, 0,
-			&do_gpage_early_setup);
+			NULL, &do_gpage_early_setup);
 
 	/*
 	 * Walk gpage list in reverse, allocating larger page sizes first.
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 494f99e..e975628 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -327,8 +327,9 @@ extern char *parse_args(const char *name,
 		      unsigned num,
 		      s16 level_min,
 		      s16 level_max,
+		      void *arg,
 		      int (*unknown)(char *param, char *val,
-			      const char *doing));
+				     const char *doing, void *arg));
 
 /* Called by module remove. */
 #ifdef CONFIG_SYSFS
diff --git a/init/main.c b/init/main.c
index bb1aed9..32ae416 100644
--- a/init/main.c
+++ b/init/main.c
@@ -236,7 +236,8 @@ static int __init loglevel(char *str)
 early_param("loglevel", loglevel);
 
 /* Change NUL term back to "=", to make "param" the whole string. */
-static int __init repair_env_string(char *param, char *val, const char *unused)
+static int __init repair_env_string(char *param, char *val,
+				    const char *unused, void *arg)
 {
 	if (val) {
 		/* param=val or param="val"? */
@@ -253,14 +254,15 @@ static int __init repair_env_string(char *param, char *val, const char *unused)
 }
 
 /* Anything after -- gets handed straight to init. */
-static int __init set_init_arg(char *param, char *val, const char *unused)
+static int __init set_init_arg(char *param, char *val,
+			       const char *unused, void *arg)
 {
 	unsigned int i;
 
 	if (panic_later)
 		return 0;
 
-	repair_env_string(param, val, unused);
+	repair_env_string(param, val, unused, NULL);
 
 	for (i = 0; argv_init[i]; i++) {
 		if (i == MAX_INIT_ARGS) {
@@ -277,9 +279,10 @@ static int __init set_init_arg(char *param, char *val, const char *unused)
  * Unknown boot options get handed to init, unless they look like
  * unused parameters (modprobe will find them in /proc/cmdline).
  */
-static int __init unknown_bootoption(char *param, char *val, const char *unused)
+static int __init unknown_bootoption(char *param, char *val,
+				     const char *unused, void *arg)
 {
-	repair_env_string(param, val, unused);
+	repair_env_string(param, val, unused, NULL);
 
 	/* Handle obsolete-style parameters */
 	if (obsolete_checksetup(param))
@@ -419,7 +422,8 @@ static noinline void __init_refok rest_init(void)
 }
 
 /* Check for early params. */
-static int __init do_early_param(char *param, char *val, const char *unused)
+static int __init do_early_param(char *param, char *val,
+				 const char *unused, void *arg)
 {
 	const struct obs_kernel_param *p;
 
@@ -438,7 +442,8 @@ static int __init do_early_param(char *param, char *val, const char *unused)
 
 void __init parse_early_options(char *cmdline)
 {
-	parse_args("early options", cmdline, NULL, 0, 0, 0, do_early_param);
+	parse_args("early options", cmdline, NULL, 0, 0, 0, NULL,
+		   do_early_param);
 }
 
 /* Arch code calls this early on, or if not, just before other parsing. */
@@ -543,10 +548,10 @@ asmlinkage __visible void __init start_kernel(void)
 	after_dashes = parse_args("Booting kernel",
 				  static_command_line, __start___param,
 				  __stop___param - __start___param,
-				  -1, -1, &unknown_bootoption);
+				  -1, -1, NULL, &unknown_bootoption);
 	if (after_dashes)
 		parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
-			   set_init_arg);
+			   NULL, set_init_arg);
 
 	jump_label_init();
 
@@ -851,7 +856,7 @@ static void __init do_initcall_level(int level)
 		   initcall_command_line, __start___param,
 		   __stop___param - __start___param,
 		   level, level,
-		   &repair_env_string);
+		   NULL, &repair_env_string);
 
 	for (fn = initcall_levels[level]; fn < initcall_levels[level+1]; fn++)
 		do_one_initcall(*fn);
diff --git a/kernel/module.c b/kernel/module.c
index 03214bd2..88f3d6c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3172,7 +3172,8 @@ out:
 	return err;
 }
 
-static int unknown_module_param_cb(char *param, char *val, const char *modname)
+static int unknown_module_param_cb(char *param, char *val, const char *modname,
+				   void *arg)
 {
 	/* Check for magic 'dyndbg' arg */ 
 	int ret = ddebug_dyndbg_module_param_cb(param, val, modname);
@@ -3277,7 +3278,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	/* Module is ready to execute: parsing args may do that. */
 	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
-				  -32768, 32767, unknown_module_param_cb);
+				  -32768, 32767, NULL,
+				  unknown_module_param_cb);
 	if (IS_ERR(after_dashes)) {
 		err = PTR_ERR(after_dashes);
 		goto bug_cleanup;
diff --git a/kernel/params.c b/kernel/params.c
index 34f5270..4182607 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -90,8 +90,9 @@ static int parse_one(char *param,
 		     unsigned num_params,
 		     s16 min_level,
 		     s16 max_level,
+		     void *arg,
 		     int (*handle_unknown)(char *param, char *val,
-				     const char *doing))
+				     const char *doing, void *arg))
 {
 	unsigned int i;
 	int err;
@@ -117,7 +118,7 @@ static int parse_one(char *param,
 
 	if (handle_unknown) {
 		pr_debug("doing %s: %s='%s'\n", doing, param, val);
-		return handle_unknown(param, val, doing);
+		return handle_unknown(param, val, doing, arg);
 	}
 
 	pr_debug("Unknown argument '%s'\n", param);
@@ -183,7 +184,9 @@ char *parse_args(const char *doing,
 		 unsigned num,
 		 s16 min_level,
 		 s16 max_level,
-		 int (*unknown)(char *param, char *val, const char *doing))
+		 void *arg,
+		 int (*unknown)(char *param, char *val,
+				const char *doing, void *arg))
 {
 	char *param, *val;
 
@@ -203,7 +206,7 @@ char *parse_args(const char *doing,
 			return args;
 		irq_was_disabled = irqs_disabled();
 		ret = parse_one(param, val, doing, params, num,
-				min_level, max_level, unknown);
+				min_level, max_level, arg, unknown);
 		if (irq_was_disabled && !irqs_disabled())
 			pr_warn("%s: option '%s' enabled irq's!\n",
 				doing, param);
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c9afbe2..d0a8898 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -910,7 +910,7 @@ static int ddebug_dyndbg_param_cb(char *param, char *val,
 
 /* handle both dyndbg and $module.dyndbg params at boot */
 static int ddebug_dyndbg_boot_param_cb(char *param, char *val,
-				const char *unused)
+				const char *unused, void *arg)
 {
 	vpr_info("%s=\"%s\"\n", param, val);
 	return ddebug_dyndbg_param_cb(param, val, NULL, 0);
@@ -1051,7 +1051,7 @@ static int __init dynamic_debug_init(void)
 	 */
 	cmdline = kstrdup(saved_command_line, GFP_KERNEL);
 	parse_args("dyndbg params", cmdline, NULL,
-		   0, 0, 0, &ddebug_dyndbg_boot_param_cb);
+		   0, 0, 0, NULL, &ddebug_dyndbg_boot_param_cb);
 	kfree(cmdline);
 	return 0;
 
-- 
2.1.0

[PATCH v1 2/5] driver-core: enable drivers to opt-out of async probe

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

We'll soon add generic support for asynchronous probe, before
that gets merged lets let drivers annotate if they should never
probe asynchronously.

Cc: Tejun Heo <tj@kernel.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Doug Thompson <dougthompson@xmission.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 include/linux/device.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 43d183a..4de6328 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -200,6 +200,10 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus);
  * @owner:	The module owner.
  * @mod_name:	Used for built-in modules.
  * @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @sync_probe: requests probe to be run always be run synchronously even
+ * 	if userspace asked us to run asynchronously. Some devices drivers
+ * 	may be known to not work well with async probe, use this to annotate
+ * 	your driver if you know it needs synchronous probe.
  * @of_match_table: The open firmware table.
  * @acpi_match_table: The ACPI match table.
  * @probe:	Called to query the existence of a specific device,
@@ -233,6 +237,7 @@ struct device_driver {
 	const char		*mod_name;	/* used for built-in modules */
 
 	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
+	bool sync_probe;
 
 	const struct of_device_id	*of_match_table;
 	const struct acpi_device_id	*acpi_match_table;
-- 
2.1.0

Re: [PATCH v1 2/5] driver-core: enable drivers to opt-out of async probe

[Tejun Heo]

On Fri, Sep 26, 2014 at 02:57:14PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> We'll soon add generic support for asynchronous probe, before
> that gets merged lets let drivers annotate if they should never
> probe asynchronously.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Doug Thompson <dougthompson@xmission.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: linux-edac@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  include/linux/device.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 43d183a..4de6328 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -200,6 +200,10 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus);
>   * @owner:	The module owner.
>   * @mod_name:	Used for built-in modules.
>   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> + * @sync_probe: requests probe to be run always be run synchronously even

"be run" repeated in the sentence.

> + * 	if userspace asked us to run asynchronously. Some devices drivers
> + * 	may be known to not work well with async probe, use this to annotate
> + * 	your driver if you know it needs synchronous probe.

Maybe something like "Use this to annotate drivers which don't work
well with async probe." is better?

The formatting seems inconsistent with other comments.

Thanks.

-- 
tejun

Re: [PATCH v1 2/5] driver-core: enable drivers to opt-out of async probe

[Luis R. Rodriguez]

On Sun, Sep 28, 2014 at 10:33:24AM -0400, Tejun Heo wrote:
> On Fri, Sep 26, 2014 at 02:57:14PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > We'll soon add generic support for asynchronous probe, before
> > that gets merged lets let drivers annotate if they should never
> > probe asynchronously.
> > 
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Arjan van de Ven <arjan@linux.intel.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Doug Thompson <dougthompson@xmission.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > Cc: linux-edac@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >  include/linux/device.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 43d183a..4de6328 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -200,6 +200,10 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus);
> >   * @owner:	The module owner.
> >   * @mod_name:	Used for built-in modules.
> >   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> > + * @sync_probe: requests probe to be run always be run synchronously even
> 
> "be run" repeated in the sentence.
> 
> > + * 	if userspace asked us to run asynchronously. Some devices drivers
> > + * 	may be known to not work well with async probe, use this to annotate
> > + * 	your driver if you know it needs synchronous probe.
> 
> Maybe something like "Use this to annotate drivers which don't work
> well with async probe." is better?

Sure.

> The formatting seems inconsistent with other comments.

Fixed.

  LUis

[PATCH v1 3/5] amd64_edac: enforce synchronous probe

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

While testing asynchronous PCI probe on this driver I noticed
it failed so enforce just synchronouse probe for now. Asynchronous
probe is not used by default and requires userepace intervention.
Patches for its support will be merged later.

[   14.411083] AMD64 EDAC driver v3.4.0
[   14.411156] bus: 'pci': probe for driver amd64_edac is run asynchronously
[   14.411308] really_probe: driver_sysfs_add(0000:00:18.2) failed
[   14.411354] ------------[ cut here ]------------
[   14.411398] WARNING: CPU: 4 PID: 224 at fs/kernfs/dir.c:1220 kernfs_remove_by_name_ns+0x83/0x90()
[   14.411446] kernfs: can not remove '0000:00:18.2', no directory
[   14.411484] Modules linked in: glue_helper(+) amd64_edac_mod(-) <etc>
[   14.413654] CPU: 4 PID: 224 Comm: kworker/u16:3 Not tainted 3.17.0-rc6+ #1
[   14.413695] Hardware name: To be filled by O.E.M. To be filled by O.E.M./M5A97, BIOS 1605 10/25/2012
[   14.413809] Workqueue: events_unbound driver_attach_workfn
[   14.413909]  0000000000000009 ffffffff814d2cf9 ffff8803f0513d20 ffffffff81061972
[   14.414145]  0000000000000000
[   14.414254]  ffff8803f0513d70
[   14.414383]  ffffffffa032c068 0000000000000038
[   14.414507]  0000000000000000 ffffffff810619d7 ffffffff81723238 ffff880300000020
[   14.414701] Call Trace:
[   14.414746]  [<ffffffff814d2cf9>] ? dump_stack+0x41/0x51
[   14.414790]  [<ffffffff81061972>] ? warn_slowpath_common+0x72/0x90
[   14.414834]  [<ffffffff810619d7>] ? warn_slowpath_fmt+0x47/0x50
[   14.414880]  [<ffffffff814d0ac3>] ? printk+0x4f/0x51
[   14.414921]  [<ffffffff811f8593>] ? kernfs_remove_by_name_ns+0x83/0x90
[   14.415000]  [<ffffffff8137433d>] ? driver_sysfs_remove+0x1d/0x40
[   14.415046]  [<ffffffff81374a15>] ? driver_probe_device+0x1d5/0x250
[   14.415099]  [<ffffffff81374b4b>] ? __driver_attach+0x7b/0x80
[   14.415149]  [<ffffffff81374ad0>] ? __device_attach+0x40/0x40
[   14.415204]  [<ffffffff81372a13>] ? bus_for_each_dev+0x53/0x90
[   14.415254]  [<ffffffff81373913>] ? driver_attach_workfn+0x13/0x80
[   14.415298]  [<ffffffff81077403>] ? process_one_work+0x143/0x3c0
[   14.415342]  [<ffffffff81077a44>] ? worker_thread+0x114/0x480
[   14.415384]  [<ffffffff81077930>] ? rescuer_thread+0x2b0/0x2b0
[   14.415427]  [<ffffffff8107c261>] ? kthread+0xc1/0xe0
[   14.415468]  [<ffffffff8107c1a0>] ? kthread_create_on_node+0x170/0x170
[   14.415511]  [<ffffffff814d883c>] ? ret_from_fork+0x7c/0xb0
[   14.415554]  [<ffffffff8107c1a0>] ? kthread_create_on_node+0x170/0x170
[   14.415595] ---[ end trace dd11ab53a6e1ec0d ]---
[   14.415643] amd64_edac: probe of 0000:00:18.2 failed with error 0

Cc: Tejun Heo <tj@kernel.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Doug Thompson <dougthompson@xmission.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/edac/amd64_edac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index f8bf000..dc997ae 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -2872,6 +2872,7 @@ static struct pci_driver amd64_pci_driver = {
 	.probe		= probe_one_instance,
 	.remove		= remove_one_instance,
 	.id_table	= amd64_pci_table,
+	.driver.sync_probe = true,
 };
 
 static void setup_pci_device(void)
-- 
2.1.0

Re: [PATCH v1 3/5] amd64_edac: enforce synchronous probe

[Tejun Heo]

On Fri, Sep 26, 2014 at 02:57:15PM -0700, Luis R. Rodriguez wrote:
...
> [   14.414746]  [<ffffffff814d2cf9>] ? dump_stack+0x41/0x51
> [   14.414790]  [<ffffffff81061972>] ? warn_slowpath_common+0x72/0x90
> [   14.414834]  [<ffffffff810619d7>] ? warn_slowpath_fmt+0x47/0x50
> [   14.414880]  [<ffffffff814d0ac3>] ? printk+0x4f/0x51
> [   14.414921]  [<ffffffff811f8593>] ? kernfs_remove_by_name_ns+0x83/0x90
> [   14.415000]  [<ffffffff8137433d>] ? driver_sysfs_remove+0x1d/0x40
> [   14.415046]  [<ffffffff81374a15>] ? driver_probe_device+0x1d5/0x250
> [   14.415099]  [<ffffffff81374b4b>] ? __driver_attach+0x7b/0x80
> [   14.415149]  [<ffffffff81374ad0>] ? __device_attach+0x40/0x40
> [   14.415204]  [<ffffffff81372a13>] ? bus_for_each_dev+0x53/0x90
> [   14.415254]  [<ffffffff81373913>] ? driver_attach_workfn+0x13/0x80
> [   14.415298]  [<ffffffff81077403>] ? process_one_work+0x143/0x3c0
> [   14.415342]  [<ffffffff81077a44>] ? worker_thread+0x114/0x480
> [   14.415384]  [<ffffffff81077930>] ? rescuer_thread+0x2b0/0x2b0
> [   14.415427]  [<ffffffff8107c261>] ? kthread+0xc1/0xe0
> [   14.415468]  [<ffffffff8107c1a0>] ? kthread_create_on_node+0x170/0x170
> [   14.415511]  [<ffffffff814d883c>] ? ret_from_fork+0x7c/0xb0
> [   14.415554]  [<ffffffff8107c1a0>] ? kthread_create_on_node+0x170/0x170

Do you have CONFIG_FRAME_POINTER turned off?  Probe failure itself
aside, the above looks like a bug in really_probe()'s error path.  It
looks like it can try to remove sysfs directory that it hasn't added
yet.

Thanks.

-- 
tejun

Re: [PATCH v1 3/5] amd64_edac: enforce synchronous probe

[Luis R. Rodriguez]

On Sun, Sep 28, 2014 at 10:41:23AM -0400, Tejun Heo wrote:
> On Fri, Sep 26, 2014 at 02:57:15PM -0700, Luis R. Rodriguez wrote:
> ...
> > [   14.414746]  [<ffffffff814d2cf9>] ? dump_stack+0x41/0x51
> > [   14.414790]  [<ffffffff81061972>] ? warn_slowpath_common+0x72/0x90
> > [   14.414834]  [<ffffffff810619d7>] ? warn_slowpath_fmt+0x47/0x50
> > [   14.414880]  [<ffffffff814d0ac3>] ? printk+0x4f/0x51
> > [   14.414921]  [<ffffffff811f8593>] ? kernfs_remove_by_name_ns+0x83/0x90
> > [   14.415000]  [<ffffffff8137433d>] ? driver_sysfs_remove+0x1d/0x40
> > [   14.415046]  [<ffffffff81374a15>] ? driver_probe_device+0x1d5/0x250
> > [   14.415099]  [<ffffffff81374b4b>] ? __driver_attach+0x7b/0x80
> > [   14.415149]  [<ffffffff81374ad0>] ? __device_attach+0x40/0x40
> > [   14.415204]  [<ffffffff81372a13>] ? bus_for_each_dev+0x53/0x90
> > [   14.415254]  [<ffffffff81373913>] ? driver_attach_workfn+0x13/0x80
> > [   14.415298]  [<ffffffff81077403>] ? process_one_work+0x143/0x3c0
> > [   14.415342]  [<ffffffff81077a44>] ? worker_thread+0x114/0x480
> > [   14.415384]  [<ffffffff81077930>] ? rescuer_thread+0x2b0/0x2b0
> > [   14.415427]  [<ffffffff8107c261>] ? kthread+0xc1/0xe0
> > [   14.415468]  [<ffffffff8107c1a0>] ? kthread_create_on_node+0x170/0x170
> > [   14.415511]  [<ffffffff814d883c>] ? ret_from_fork+0x7c/0xb0
> > [   14.415554]  [<ffffffff8107c1a0>] ? kthread_create_on_node+0x170/0x170
> 
> Do you have CONFIG_FRAME_POINTER turned off?

Yeah..

> Probe failure itself
> aside, the above looks like a bug in really_probe()'s error path.  It
> looks like it can try to remove sysfs directory that it hasn't added
> yet.

I see one area where that can happen, will debug a bit more to see if we
are running into that here with async probe.

  Luis

Re: [PATCH v1 3/5] amd64_edac: enforce synchronous probe

[Luis R. Rodriguez]

On Tue, Sep 30, 2014 at 09:23:28AM +0200, Luis R. Rodriguez wrote:
> On Sun, Sep 28, 2014 at 10:41:23AM -0400, Tejun Heo wrote:
> > On Fri, Sep 26, 2014 at 02:57:15PM -0700, Luis R. Rodriguez wrote:
> > ...
> > > [   14.414746]  [<ffffffff814d2cf9>] ? dump_stack+0x41/0x51
> > > [   14.414790]  [<ffffffff81061972>] ? warn_slowpath_common+0x72/0x90
> > > [   14.414834]  [<ffffffff810619d7>] ? warn_slowpath_fmt+0x47/0x50
> > > [   14.414880]  [<ffffffff814d0ac3>] ? printk+0x4f/0x51
> > > [   14.414921]  [<ffffffff811f8593>] ? kernfs_remove_by_name_ns+0x83/0x90
> > > [   14.415000]  [<ffffffff8137433d>] ? driver_sysfs_remove+0x1d/0x40
> > > [   14.415046]  [<ffffffff81374a15>] ? driver_probe_device+0x1d5/0x250
> > > [   14.415099]  [<ffffffff81374b4b>] ? __driver_attach+0x7b/0x80
> > > [   14.415149]  [<ffffffff81374ad0>] ? __device_attach+0x40/0x40
> > > [   14.415204]  [<ffffffff81372a13>] ? bus_for_each_dev+0x53/0x90
> > > [   14.415254]  [<ffffffff81373913>] ? driver_attach_workfn+0x13/0x80
> > > [   14.415298]  [<ffffffff81077403>] ? process_one_work+0x143/0x3c0
> > > [   14.415342]  [<ffffffff81077a44>] ? worker_thread+0x114/0x480
> > > [   14.415384]  [<ffffffff81077930>] ? rescuer_thread+0x2b0/0x2b0
> > > [   14.415427]  [<ffffffff8107c261>] ? kthread+0xc1/0xe0
> > > [   14.415468]  [<ffffffff8107c1a0>] ? kthread_create_on_node+0x170/0x170
> > > [   14.415511]  [<ffffffff814d883c>] ? ret_from_fork+0x7c/0xb0
> > > [   14.415554]  [<ffffffff8107c1a0>] ? kthread_create_on_node+0x170/0x170
> > 
> > Do you have CONFIG_FRAME_POINTER turned off?
> 
> Yeah..

So the above warn came from having DWARF2 EH-frame based stack unwinding
but no CONFIG_FRAME_POINTER. By enabling CONFIG_FRAME_POINTER *and*
removing the DWARF2 EH-frame based stack unwinding patches the warning
I get is slightly different:

[   13.208930] EDAC MC: Ver: 3.0.0
[   13.213807] MCE: In-kernel MCE decoding enabled.
[   13.235121] AMD64 EDAC driver v3.4.0
[   13.235170] bus: 'pci': probe for driver amd64_edac is run asynchronously
[   13.235236] ------------[ cut here ]------------
[   13.235283] WARNING: CPU: 2 PID: 127 at fs/kernfs/dir.c:377 kernfs_get+0x31/0x40()
[   13.235323] Modules linked in: amd64_edac_mod(-) lrw serio_raw gf128mul edac_mce_amd glue_helper edac_core sp5100_tco pcspkr snd_timer i2c_piix4 k10temp fam15h_power snd soundcore i2c_core wmi button xen_acpi_processor processor thermal_sys xen_pciback xen_netback xen_blkback xen_gntalloc xen_gntdev xen_evtchn loop fuse autofs4 ext4 crc16 mbcache jbd2 sg sd_mod crc_t10dif crct10dif_generic crct10dif_common hid_logitech_dj usbhid hid dm_mod ahci xhci_hcd ohci_pci libahci ohci_hcd ehci_pci ehci_hcd libata usbcore scsi_mod r8169 usb_common mii
[   13.237129] CPU: 2 PID: 127 Comm: kworker/u16:5 Not tainted 3.17.0-rc7+ #2
[   13.237165] Hardware name: To be filled by O.E.M. To be filled by O.E.M./M5A97, BIOS 1605 10/25/2012
[   13.237207] Workqueue: events_unbound driver_attach_workfn
[   13.237271]  0000000000000009 ffff88040a7e7c48 ffffffff814f7f1f 0000000000000000
[   13.237426]  ffff88040a7e7c80 ffffffff81066378 ffff880409a63be0 ffff88040a259a78
[   13.237582]  ffff880409a63be0 ffff880409a63be0 ffff88040f15cf00 ffff88040a7e7c90
[   13.237740] Call Trace:
[   13.237777]  [<ffffffff814f7f1f>] dump_stack+0x45/0x56
[   13.237814]  [<ffffffff81066378>] warn_slowpath_common+0x78/0xa0
[   13.237851]  [<ffffffff81066455>] warn_slowpath_null+0x15/0x20
[   13.237887]  [<ffffffff8120f6c1>] kernfs_get+0x31/0x40
[   13.237950]  [<ffffffff812107e1>] kernfs_new_node+0x31/0x40
[   13.238003]  [<ffffffff812122ce>] kernfs_create_link+0x1e/0x80
[   13.238052]  [<ffffffff81212e7a>] sysfs_do_create_link_sd.isra.2+0x5a/0xb0
[   13.238097]  [<ffffffff81212ef0>] sysfs_create_link+0x20/0x40
[   13.238143]  [<ffffffff8139ab70>] driver_sysfs_add+0x50/0xb0
[   13.238216]  [<ffffffff8139b159>] driver_probe_device+0x59/0x250
[   13.238253]  [<ffffffff8139b41b>] __driver_attach+0x8b/0x90
[   13.238290]  [<ffffffff8139b390>] ? __device_attach+0x40/0x40
[   13.238327]  [<ffffffff81399033>] bus_for_each_dev+0x63/0xa0
[   13.238367]  [<ffffffff8139ac99>] driver_attach+0x19/0x20
[   13.238409]  [<ffffffff813999a8>] driver_attach_workfn+0x18/0x80
[   13.238446]  [<ffffffff8107d3df>] process_one_work+0x14f/0x400
[   13.238482]  [<ffffffff8107dc9b>] worker_thread+0x6b/0x4b0
[   13.238519]  [<ffffffff8107dc30>] ? rescuer_thread+0x270/0x270
[   13.238556]  [<ffffffff810826d6>] kthread+0xd6/0xf0
[   13.238592]  [<ffffffff81082600>] ? kthread_create_on_node+0x180/0x180
[   13.238630]  [<ffffffff814fddfc>] ret_from_fork+0x7c/0xb0
[   13.238666]  [<ffffffff81082600>] ? kthread_create_on_node+0x180/0x180
[   13.238702] ---[ end trace bfbfc1541fcb030e ]---
[   13.238739] really_probe: driver_sysfs_add(0000:00:18.2) failed
[   13.238776] amd64_edac: probe of 0000:00:18.2 failed with error 0
[   13.299111] AVX version of gcm_enc/dec engaged.
[   13.312828] alg: No test for __gcm-aes-aesni (__driver-gcm-aes-aesni)

I tried looking into this but a later in a later kernel I had enabled
a few other things I had forgotten (like acpi thermal stuff) and then
the kernel just spewed out similar error and unfortunately I was not
able to capture the top but it all seemed related to the above.

I decided to try this:

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index dc997ae..f8bf000 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -2872,7 +2872,6 @@ static struct pci_driver amd64_pci_driver = {
 	.probe		= probe_one_instance,
 	.remove		= remove_one_instance,
 	.id_table	= amd64_pci_table,
-	.driver.sync_probe = true,
 };
 
 static void setup_pci_device(void)
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index aecb15f..8401c0a 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -41,6 +41,9 @@ static int sysfs_do_create_link_sd(struct kernfs_node *parent,
 	if (!target)
 		return -ENOENT;
 
+	if (WARN_ON(!atomic_read(&target->count)))
+		return -ENOENT;
+
 	kn = kernfs_create_link(parent, name, target);
 	kernfs_put(target);
 

and my system was still useless and even end up in some fun page faults,
but again I think this is all related. I reviewed sysfs / kernfs code
and didn't see issues there with how symlinks are handled so I started
reviewing the driver itself a bit and saw it had strong use of sysfs
on itself and also on helpers such as edac_create_sysfs_mci_device().
I would not be surprised if the issue lies more in there than elsewhere.

I could keep on debugging but I think at this point this is enough
work to at least show the driver does need sync probe. I do not think this
is a core driver issue at this point.

  Luis

Re: [PATCH v1 3/5] amd64_edac: enforce synchronous probe

[Borislav Petkov]

On Thu, Oct 02, 2014 at 12:39:59AM +0200, Luis R. Rodriguez wrote:
> ...
> and my system was still useless and even end up in some fun page faults,
> but again I think this is all related. I reviewed sysfs / kernfs code
> and didn't see issues there with how symlinks are handled so I started
> reviewing the driver itself a bit and saw it had strong use of sysfs
> on itself and also on helpers such as edac_create_sysfs_mci_device().
> I would not be surprised if the issue lies more in there than elsewhere.

Right, but that would point at sysfs being not asyns-ready, right? I
mean, the driver is just a user of sysfs and doesn't do anything out of
the ordinary in that respect. And the sysfs usage comes from the EDAC
core so I think you'll basically end up marking *all* EDAC drivers as
sync because they all call edac_create_sysfs_mci_device() - you just
happened to run an AMD box and see it there.

This might turn fast into a nasty game if other drivers do similar things
so I'd say this needs to get properly debugged to see what is causing it
and fixed there. And my money goes on that

	WARNING: CPU: 2 PID: 127 at fs/kernfs/dir.c:377 kernfs_get+0x31/0x40()

which is there for some reason and apparently shouldn't be happening...

Just an IMHO of course.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v1 3/5] amd64_edac: enforce synchronous probe

[Luis R. Rodriguez]

On Thu, Oct 02, 2014 at 11:18:00AM +0200, Borislav Petkov wrote:
> On Thu, Oct 02, 2014 at 12:39:59AM +0200, Luis R. Rodriguez wrote:
> > ...
> > and my system was still useless and even end up in some fun page faults,
> > but again I think this is all related. I reviewed sysfs / kernfs code
> > and didn't see issues there with how symlinks are handled so I started
> > reviewing the driver itself a bit and saw it had strong use of sysfs
> > on itself and also on helpers such as edac_create_sysfs_mci_device().
> > I would not be surprised if the issue lies more in there than elsewhere.
> 
> Right, but that would point at sysfs being not asyns-ready, right? I
> mean, the driver is just a user of sysfs and doesn't do anything out of
> the ordinary in that respect. And the sysfs usage comes from the EDAC
> core so I think you'll basically end up marking *all* EDAC drivers as
> sync because they all call edac_create_sysfs_mci_device() - you just
> happened to run an AMD box and see it there.
> 
> This might turn fast into a nasty game if other drivers do similar things
> so I'd say this needs to get properly debugged to see what is causing it
> and fixed there. And my money goes on that
> 
> 	WARNING: CPU: 2 PID: 127 at fs/kernfs/dir.c:377 kernfs_get+0x31/0x40()
> 
> which is there for some reason and apparently shouldn't be happening...

I looked into this further and I've determined the issue was a driver
side issue, it actually *does* rely on synchronous probe, it calls on
a routine which expects at least one device to be registered already
otherwise it fails early on its init routine. The patch below fixes
the issue, for example. This patch is only correct if my interpretation
that the driver only wants to call the setup routine once is valid,
which it seems it is the case, and it does this only for the first
registered device.

With these changes the driver can probe asynchronously. The race
that was happing was that since the probe async and since the init
routine did assume sync probe towards the end if a device was not
yet registered it would bail and pci_unregister_driver() the driver
and obviously that would delete the sysfs directory for the PCI device.
The symlink attempt that was ongoing during probe for place a symlink
name driver on the PCI sysfs directory was taken underneath probe.

This begs a few questions:

The original code seems to have run setup_pci_device() only once
and it was for the first ID device matching 0 that could be returned by
amd_get_node_id(). If its ensured that the first probed device
should always match the ID 0 for amd_get_node_id() (since setup_pci_device()
uses mci = mcis[0]) and if calling edac_pci_create_generic_ctl() is safe
only right after this ID 0 device is registered then we could simplify
the code below even further by just calling amd64_edac_register() without
a workqueue for the first probed device.

Please revise and let me know what is preferred. In the meantime I'll
continue on with the series and this can be handled orthogonally.

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index dc997ae..1d4dbdf 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -16,9 +16,9 @@ module_param(ecc_enable_override, int, 0644);
 static struct msr __percpu *msrs;
 
 /*
- * count successfully initialized driver instances for setup_pci_device()
+ * Prevent two runs on setup_pci_device()
  */
-static atomic_t drv_instances = ATOMIC_INIT(0);
+static bool amd64_first_dev_registered;
 
 /* Per-node driver instances */
 static struct mem_ctl_info **mcis;
@@ -2702,8 +2702,6 @@ static int init_one_instance(struct pci_dev *F2)
 
 	mcis[nid] = mci;
 
-	atomic_inc(&drv_instances);
-
 	return 0;
 
 err_add_sysfs:
@@ -2721,6 +2719,35 @@ err_ret:
 	return ret;
 }
 
+static void amd64_edac_register(struct work_struct *work)
+{
+	struct mem_ctl_info *mci;
+	struct amd64_pvt *pvt;
+
+	if (amd64_first_dev_registered) {
+		pr_warn("amd64_edca: First device already registered\n");
+		return;
+	}
+
+	if (pci_ctl)
+		return;
+
+	mci = mcis[0];
+	if (!mci)
+		return;
+
+	pvt = mci->pvt_info;
+	pci_ctl = edac_pci_create_generic_ctl(&pvt->F2->dev, EDAC_MOD_STR);
+	if (!pci_ctl) {
+		pr_warn("%s(): Unable to create PCI control\n", __func__);
+		pr_warn("%s(): PCI error report via EDAC not set\n", __func__);
+	}
+	amd64_first_dev_registered = true;
+}
+
+static DECLARE_WORK(amd64_edac_work, amd64_edac_register);
+
+
 static int probe_one_instance(struct pci_dev *pdev,
 			      const struct pci_device_id *mc_type)
 {
@@ -2760,6 +2787,9 @@ static int probe_one_instance(struct pci_dev *pdev,
 		restore_ecc_error_reporting(s, nid, F3);
 	}
 
+	if (!schedule_work(&amd64_edac_work))
+		return -EBUSY;
+
 	return ret;
 
 err_enable:
@@ -2872,33 +2902,14 @@ static struct pci_driver amd64_pci_driver = {
 	.probe		= probe_one_instance,
 	.remove		= remove_one_instance,
 	.id_table	= amd64_pci_table,
-	.driver.sync_probe = true,
 };
 
-static void setup_pci_device(void)
-{
-	struct mem_ctl_info *mci;
-	struct amd64_pvt *pvt;
-
-	if (pci_ctl)
-		return;
-
-	mci = mcis[0];
-	if (!mci)
-		return;
-
-	pvt = mci->pvt_info;
-	pci_ctl = edac_pci_create_generic_ctl(&pvt->F2->dev, EDAC_MOD_STR);
-	if (!pci_ctl) {
-		pr_warn("%s(): Unable to create PCI control\n", __func__);
-		pr_warn("%s(): PCI error report via EDAC not set\n", __func__);
-	}
-}
-
 static int __init amd64_edac_init(void)
 {
 	int err = -ENODEV;
 
+	amd64_first_dev_registered = false;
+
 	printk(KERN_INFO "AMD64 EDAC driver v%s\n", EDAC_AMD64_VERSION);
 
 	opstate_init();
@@ -2920,16 +2931,8 @@ static int __init amd64_edac_init(void)
 	if (err)
 		goto err_pci;
 
-	err = -ENODEV;
-	if (!atomic_read(&drv_instances))
-		goto err_no_instances;
-
-	setup_pci_device();
 	return 0;
 
-err_no_instances:
-	pci_unregister_driver(&amd64_pci_driver);
-
 err_pci:
 	msrs_free(msrs);
 	msrs = NULL;
@@ -2947,6 +2950,8 @@ err_ret:
 
 static void __exit amd64_edac_exit(void)
 {
+	cancel_work_sync(&amd64_edac_work);
+
 	if (pci_ctl)
 		edac_pci_release_generic_ctl(pci_ctl);
 

[PATCH v1 4/5] driver-core: generalize freeing driver private member

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This will be used later.

Cc: Tejun Heo <tj@kernel.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/base/bus.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 83e910a..a5f41e4 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -657,6 +657,15 @@ static ssize_t uevent_store(struct device_driver *drv, const char *buf,
 }
 static DRIVER_ATTR_WO(uevent);
 
+static void remove_driver_private(struct device_driver *drv)
+{
+	struct driver_private *priv = drv->p;
+
+	kobject_put(&priv->kobj);
+	kfree(priv);
+	drv->p = NULL;
+}
+
 /**
  * bus_add_driver - Add a driver to the bus.
  * @drv: driver.
@@ -719,9 +728,7 @@ int bus_add_driver(struct device_driver *drv)
 	return 0;
 
 out_unregister:
-	kobject_put(&priv->kobj);
-	kfree(drv->p);
-	drv->p = NULL;
+	remove_driver_private(drv);
 out_put_bus:
 	bus_put(bus);
 	return error;
-- 
2.1.0

[PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Some init systems may wish to express the desire to have
device drivers run their device driver's bus probe() run
asynchronously. This implements support for this and
allows userspace to request async probe as a preference
through a generic shared device driver module parameter,
async_probe. Implemention for async probe is supported
through a module parameter given that since synchronous
probe has been prevalent for years some userspace might
exist which relies on the fact that the device driver will
probe synchronously and the assumption that devices it
provides will be immediately available after this.

Some device driver might not be able to run async probe
so we enable device drivers to annotate this to prevent
this module parameter from having any effect on them.

This implementation uses queue_work(system_unbound_wq)
to queue async probes, this should enable probe to run
slightly *faster* if the driver's probe path did not
have much interaction with other workqueues otherwise
it may run _slightly_ slower. Tests were done with cxgb4,
which is known to take long on probe, both without
having to run request_firmware() [0] and then by
requiring it to use request_firmware() [1]. The
difference in run time are only measurable in microseconds:

=====================================================================|
strategy                                fw (usec)       no-fw (usec) |
---------------------------------------------------------------------|
synchronous                             24472569        1307563      |
kthread                                 25066415.5      1309868.5    |
queue_work(system_unbound_wq)           24913661.5      1307631      |
---------------------------------------------------------------------|

In practice, in seconds, the difference is barely noticeable:

=====================================================================|
strategy                                fw (s)          no-fw (s)    |
---------------------------------------------------------------------|
synchronous                             24.47           1.31         |
kthread                                 25.07           1.31         |
queue_work(system_unbound_wq)           24.91           1.31         |
---------------------------------------------------------------------|

[0] http://ftp.suse.com/pub/people/mcgrof/async-probe/probe-cgxb4-no-firmware.png
[1] http://ftp.suse.com/pub/people/mcgrof/async-probe/probe-cgxb4-firmware.png

The rest of the commit log documents why this feature was implemented
primarily first for systemd and things it should consider next.

Systemd has a general timeout for all workers currently set to 180
seconds after which it will send a sigkill signal. Systemd now has a
warning which is issued once it reaches 1/3 of the timeout. The original
motivation for the systemd timeout was to help track device drivers
which do not use asynch firmware loading on init() and the timeout was
originally set to 30 seconds.

Since systemd + kernel are heavily tied in for the purposes of this
patch it is assumed you have merged on systemd the following
commits:

671174136525ddf208cdbe75d6d6bd159afa961f        udev: timeout - warn after a third of the timeout before killing
b5338a19864ac3f5632aee48069a669479621dca        udev: timeout - increase timeout
2e92633dbae52f5ac9b7b2e068935990d475d2cd        udev: bump event timeout to 60 seconds
be2ea723b1d023b3d385d3b791ee4607cbfb20ca        udev: remove userspace firmware loading support
9f20a8a376f924c8eb5423cfc1f98644fc1e2d1a        udev: fixup commit
dd5eddd28a74a49607a8fffcaf960040dba98479        udev: unify event timeout handling
9719859c07aa13539ed2cd4b31972cd30f678543        udevd: add --event-timeout commandline option

Since we bundle together serially driver init() and probe()
on module initialiation systemd's imposed timeout  put a limit on the
amount of time a driver init() and probe routines can take. There's a
few overlooked issues with this and the timeout in general:

0) Not all drivers are killed, the signal is just sent and
   the kill will only be acted upoon if the driver you loaded
   happens to have some code path that either uses kthreads (which
   as of 786235ee are now killable), or uses some code which checks for
   fatal_signal_pending() on the kernel somewhere -- i.e: pci_read_vpd().

1) Since systemd is the only one logging the sigkill debugging that
   drivers are not loaded or in the worst case *failed to boot* because
   of a sigkill has proven hard to debug.

2) When and if the signal is received by the driver somehow
   the driver may fail at different points in its initialization
   and unless all error paths on the driver are implemented
   perfectly this could mean leaving a device in a half
   initialized state.

3) The timeout is penalizing device drivers that take long on
   probe(), this wasn't the original motivation. Systemd seems
   to have been under assumption that probe was asynchronous,
   this perhaps is true as an *objective* and goal for *some
   subsystems* but by no means is it true that we've been on a wide
   crusade to ensure this for all device drivers. It may be a good
   idea for *many* device drivers but penalizing them with a kill
   for taking long on probe is simply unacceptable specially
   when the timeout is completely arbitrary.

4) The driver core calls probe for *all* devices that a driver can
   claim and it does so serially, so if a device driver will need
   to probe 3 devices and if probe on the device driver is synchronous
   the amount of time that module loading will take will be:

   driver load time = init() + probe for 3 devices serially

   The timeout ultimatley ends up limiting the number of devices that
   *any* device driver can support based on the following formula:

   number_devices =          systemd_timeout
                      -------------------------------------
                         max known probe time for driver

   Lastly since the error value passed down is the value of
   the probe for the last device probed the module will fail
   to load and all devices will fail to be available.

In the Linux kernel we don't want to work around the timeout,
instead systemd must be changed to take all the above into
consideration when issuing any kills on device drivers, ideally
the sigkill should be considered to be ignored at least for
kmod. In addition to this we help systemd by giving it what it
originally considered was there and enable it to ask device
drivers to use asynchronous probe. This patch addresses that
feature.

Systemd should consider enabling async probe on device drivers
it loads through systemd-udev but probably does not want to
enable it for modules loaded through systemd-modules-load
(modules-load.d). At least on my booting enablign async probe
for all modules fails to boot as such in order to make this
a bit more useful we whitelist a few buses where it should be
at least in theory safe to try to enable async probe. This
way even if systemd tried to ask to enable async probe for all
its device drivers the kernel won't blindly do this. We also
have the sync_probe flag which device drivers can themselves
enable *iff* its known the device driver should never async
probe.

In order to help *test* things folks can use the bus.safe_mod_async_probe=1
kernel parameter which will work as if userspace would have
requested all modules to load with async probe. Daring folks can
also use bus.force_mod_async_probe=1 which will enable asynch probe
even on buses not tested in any way yet, if you use that though
you're on your own.

Cc: Tejun Heo <tj@kernel.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <joseph.salisbury@canonical.com>
Cc: Kay Sievers <kay@vrfy.org>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <tim.gardner@canonical.com>
Cc: Pierre Fersing <pierre-fersing@pierref.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <bpoirier@suse.de>
Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
Cc: Casey Leedom <leedom@chelsio.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
Cc: MPT-FusionLinux.pdl@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/base/base.h    |   6 +++
 drivers/base/bus.c     | 137 +++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/base/dd.c      |   7 +++
 include/linux/module.h |   2 +
 kernel/module.c        |  12 ++++-
 5 files changed, 159 insertions(+), 5 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 251c5d3..24836f1 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -43,11 +43,17 @@ struct subsys_private {
 };
 #define to_subsys_private(obj) container_of(obj, struct subsys_private, subsys.kobj)
 
+struct driver_attach_work {
+	struct work_struct work;
+	struct device_driver *driver;
+};
+
 struct driver_private {
 	struct kobject kobj;
 	struct klist klist_devices;
 	struct klist_node knode_bus;
 	struct module_kobject *mkobj;
+	struct driver_attach_work *attach_work;
 	struct device_driver *driver;
 };
 #define to_driver(obj) container_of(obj, struct driver_private, kobj)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index a5f41e4..41e321e6 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -85,6 +85,7 @@ static void driver_release(struct kobject *kobj)
 	struct driver_private *drv_priv = to_driver(kobj);
 
 	pr_debug("driver: '%s': %s\n", kobject_name(kobj), __func__);
+	kfree(drv_priv->attach_work);
 	kfree(drv_priv);
 }
 
@@ -662,10 +663,125 @@ static void remove_driver_private(struct device_driver *drv)
 	struct driver_private *priv = drv->p;
 
 	kobject_put(&priv->kobj);
+	kfree(priv->attach_work);
 	kfree(priv);
 	drv->p = NULL;
 }
 
+static void driver_attach_workfn(struct work_struct *work)
+{
+	int ret;
+	struct driver_attach_work *attach_work =
+		container_of(work, struct driver_attach_work, work);
+	struct device_driver *drv = attach_work->driver;
+	ktime_t calltime, delta, rettime;
+	unsigned long long duration;
+
+	calltime = ktime_get();
+
+	ret = driver_attach(drv);
+	if (ret != 0) {
+		remove_driver_private(drv);
+		bus_put(drv->bus);
+	}
+
+	rettime = ktime_get();
+	delta = ktime_sub(rettime, calltime);
+	duration = (unsigned long long) ktime_to_ns(delta) >> 10;
+
+	pr_debug("bus: '%s': add driver %s attach completed after %lld usecs\n",
+		 drv->bus->name, drv->name, duration);
+}
+
+int bus_driver_async_probe(struct device_driver *drv)
+{
+	struct driver_private *priv = drv->p;
+
+	priv->attach_work = kzalloc(sizeof(struct driver_attach_work),
+				    GFP_KERNEL);
+	if (!priv->attach_work)
+		return -ENOMEM;
+
+	priv->attach_work->driver = drv;
+	INIT_WORK(&priv->attach_work->work, driver_attach_workfn);
+
+	/* Keep this as pr_info() until this is prevalent */
+	pr_info("bus: '%s': probe for driver %s is run asynchronously\n",
+		 drv->bus->name, drv->name);
+
+	queue_work(system_unbound_wq, &priv->attach_work->work);
+
+	return 0;
+}
+
+/*
+ */
+static bool safe_mod_async = false;
+module_param_named(safe_mod_async_probe, safe_mod_async, bool, 0400);
+MODULE_PARM_DESC(safe_mod_async_probe,
+		 "Enable async probe on all modules safely");
+
+static bool force_mod_async = false;
+module_param_named(force_mod_async_probe, force_mod_async, bool, 0400);
+MODULE_PARM_DESC(force_mod_async_probe,
+		 "Force async probe on all modules");
+
+/**
+ * drv_enable_async_probe - evaluates if async probe should be used
+ * @drv: device driver to evaluate
+ * @bus: the bus for the device driver
+ *
+ * The driver core supports enabling asynchronous probe on device drivers
+ * by requiring userspace to pass the module parameter "async_probe".
+ * Currently only modules are enabled to use this feature. If a device
+ * driver is known to not work properly with asynchronous probe they
+ * can force disable asynchronous probe from being enabled through
+ * userspace by adding setting sync_probe to true on the @drv. We require
+ * async probe to be requested from userspace given that we have historically
+ * supported synchronous probe and some userspaces may exist which depend
+ * on this functionality. Userspace may wish to use asynchronous probe for
+ * most device drivers but since this can fail boot in practice we only
+ * enable it currently for a set of buses.
+ *
+ * If you'd like to test enabling async probe for all buses whitelisted
+ * you can enable the safe_mod_async_probe module parameter. Note that its
+ * not a good idea to always enable this, in particular you probably don't
+ * want drivers under modules-load.d to use this. This module parameter should
+ * only be used to help test. If you'd like to test even futher you can
+ * use force_mod_async_probe, that will force enable async probe on all
+ * drivers, regardless if its bus type, it should however be used with
+ * caution.
+ */
+static bool drv_enable_async_probe(struct device_driver *drv,
+				   struct bus_type *bus)
+{
+	struct module *mod;
+
+	if (!drv->owner || drv->sync_probe)
+		return false;
+
+	if (force_mod_async)
+		return true;
+
+	mod = drv->owner;
+	if (!safe_mod_async && !mod->async_probe_requested)
+		return false;
+
+	/* For now lets avoid stupid bug reports */
+	if (!strcmp(bus->name, "pci") ||
+	    !strcmp(bus->name, "pci_express") ||
+	    !strcmp(bus->name, "hid") ||
+	    !strcmp(bus->name, "sdio") ||
+	    !strcmp(bus->name, "gameport") ||
+	    !strcmp(bus->name, "mmc") ||
+	    !strcmp(bus->name, "i2c") ||
+	    !strcmp(bus->name, "platform") ||
+	    !strcmp(bus->name, "usb"))
+		return true;
+
+	return false;
+}
+
 /**
  * bus_add_driver - Add a driver to the bus.
  * @drv: driver.
@@ -675,6 +791,7 @@ int bus_add_driver(struct device_driver *drv)
 	struct bus_type *bus;
 	struct driver_private *priv;
 	int error = 0;
+	bool async_probe = false;
 
 	bus = bus_get(drv->bus);
 	if (!bus)
@@ -696,11 +813,19 @@ int bus_add_driver(struct device_driver *drv)
 	if (error)
 		goto out_unregister;
 
+	async_probe = drv_enable_async_probe(drv, bus);
+
 	klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
 	if (drv->bus->p->drivers_autoprobe) {
-		error = driver_attach(drv);
-		if (error)
-			goto out_unregister;
+		if (async_probe) {
+			error = bus_driver_async_probe(drv);
+			if (error)
+				goto out_unregister;
+		} else {
+			error = driver_attach(drv);
+			if (error)
+				goto out_unregister;
+		}
 	}
 	module_add_driver(drv->owner, drv);
 
@@ -1267,6 +1392,12 @@ EXPORT_SYMBOL_GPL(subsys_virtual_register);
 
 int __init buses_init(void)
 {
+	if (unlikely(safe_mod_async))
+		pr_info("Enabled safe_mod_async -- you may run into issues\n");
+
+	if (unlikely(force_mod_async))
+		pr_info("Enabling force_mod_async -- you're on your own!\n");
+
 	bus_kset = kset_create_and_add("bus", &bus_uevent_ops, NULL);
 	if (!bus_kset)
 		return -ENOMEM;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..7999aba 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -507,6 +507,13 @@ static void __device_release_driver(struct device *dev)
 
 	drv = dev->driver;
 	if (drv) {
+		if (drv->owner && !drv->sync_probe) {
+			struct module *mod = drv->owner;
+			struct driver_private *priv = drv->p;
+
+			if (mod->async_probe_requested)
+				flush_work(&priv->attach_work->work);
+		}
 		pm_runtime_get_sync(dev);
 
 		driver_sysfs_remove(dev);
diff --git a/include/linux/module.h b/include/linux/module.h
index 71f282a..1e9e017 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -271,6 +271,8 @@ struct module {
 	bool sig_ok;
 #endif
 
+	bool async_probe_requested;
+
 	/* symbols that will be GPL-only in the near future. */
 	const struct kernel_symbol *gpl_future_syms;
 	const unsigned long *gpl_future_crcs;
diff --git a/kernel/module.c b/kernel/module.c
index 88f3d6c..31d71ff 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3175,8 +3175,16 @@ out:
 static int unknown_module_param_cb(char *param, char *val, const char *modname,
 				   void *arg)
 {
+	int ret;
+	struct module *mod = arg;
+
+	if (strcmp(param, "async_probe") == 0) {
+		mod->async_probe_requested = true;
+		return 0;
+	}
+
 	/* Check for magic 'dyndbg' arg */ 
-	int ret = ddebug_dyndbg_module_param_cb(param, val, modname);
+	ret = ddebug_dyndbg_module_param_cb(param, val, modname);
 	if (ret != 0)
 		pr_warn("%s: unknown parameter '%s' ignored\n", modname, param);
 	return 0;
@@ -3278,7 +3286,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	/* Module is ready to execute: parsing args may do that. */
 	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
-				  -32768, 32767, NULL,
+				  -32768, 32767, mod,
 				  unknown_module_param_cb);
 	if (IS_ERR(after_dashes)) {
 		err = PTR_ERR(after_dashes);
-- 
2.1.0

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Tejun Heo]

Hello,

On Fri, Sep 26, 2014 at 02:57:17PM -0700, Luis R. Rodriguez wrote:
...
> Systemd should consider enabling async probe on device drivers
> it loads through systemd-udev but probably does not want to
> enable it for modules loaded through systemd-modules-load
> (modules-load.d). At least on my booting enablign async probe
> for all modules fails to boot as such in order to make this

Did you find out why boot failed with those modules?

> a bit more useful we whitelist a few buses where it should be
> at least in theory safe to try to enable async probe. This
> way even if systemd tried to ask to enable async probe for all
> its device drivers the kernel won't blindly do this. We also
> have the sync_probe flag which device drivers can themselves
> enable *iff* its known the device driver should never async
> probe.
> 
> In order to help *test* things folks can use the bus.safe_mod_async_probe=1
> kernel parameter which will work as if userspace would have
> requested all modules to load with async probe. Daring folks can
> also use bus.force_mod_async_probe=1 which will enable asynch probe
> even on buses not tested in any way yet, if you use that though
> you're on your own.

If those two knobs are meant for debugging, let's please make that
fact immediately evident.  e.g. Make them ugly boot params like
"__DEVEL__driver_force_mod_async_probe".  Devel/debug options ending
up becoming stable interface are really nasty.

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

How many bytes are we saving by allocating it separately?  Can't we
just embed it in driver_private?

> +static void driver_attach_workfn(struct work_struct *work)
> +{
> +	int ret;
> +	struct driver_attach_work *attach_work =
> +		container_of(work, struct driver_attach_work, work);
> +	struct device_driver *drv = attach_work->driver;
> +	ktime_t calltime, delta, rettime;
> +	unsigned long long duration;

This could just be a personal preference but I think it's easier to
read if local vars w/ initializers come before the ones w/o.

> +
> +	calltime = ktime_get();
> +
> +	ret = driver_attach(drv);
> +	if (ret != 0) {
> +		remove_driver_private(drv);
> +		bus_put(drv->bus);
> +	}
> +
> +	rettime = ktime_get();
> +	delta = ktime_sub(rettime, calltime);
> +	duration = (unsigned long long) ktime_to_ns(delta) >> 10;
> +
> +	pr_debug("bus: '%s': add driver %s attach completed after %lld usecs\n",
> +		 drv->bus->name, drv->name, duration);

Why do we have the above printout for async path but not sync path?
It's kinda weird for the code path to diverge like this.  Shouldn't
the only difference be the context probes are running from?

...
> +static bool drv_enable_async_probe(struct device_driver *drv,
> +				   struct bus_type *bus)
> +{
> +	struct module *mod;
> +
> +	if (!drv->owner || drv->sync_probe)
> +		return false;
> +
> +	if (force_mod_async)
> +		return true;
> +
> +	mod = drv->owner;
> +	if (!safe_mod_async && !mod->async_probe_requested)
> +		return false;
> +
> +	/* For now lets avoid stupid bug reports */
> +	if (!strcmp(bus->name, "pci") ||
> +	    !strcmp(bus->name, "pci_express") ||
> +	    !strcmp(bus->name, "hid") ||
> +	    !strcmp(bus->name, "sdio") ||
> +	    !strcmp(bus->name, "gameport") ||
> +	    !strcmp(bus->name, "mmc") ||
> +	    !strcmp(bus->name, "i2c") ||
> +	    !strcmp(bus->name, "platform") ||
> +	    !strcmp(bus->name, "usb"))
> +		return true;

Ugh... things like this tend to become permanent.  Do we really need
this?  And how are we gonna find out what's broken why w/o bug
reports?

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index e4ffbcf..7999aba 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -507,6 +507,13 @@ static void __device_release_driver(struct device *dev)
>  
>  	drv = dev->driver;
>  	if (drv) {
> +		if (drv->owner && !drv->sync_probe) {
> +			struct module *mod = drv->owner;
> +			struct driver_private *priv = drv->p;
> +
> +			if (mod->async_probe_requested)
> +				flush_work(&priv->attach_work->work);

This can be unconditional flus_work(&priv->attach_work) if attach_work
isn't separately allocated.

>  static int unknown_module_param_cb(char *param, char *val, const char *modname,
>  				   void *arg)
>  {
> +	int ret;
> +	struct module *mod = arg;

Ditto with the order of definitions.

> +	if (strcmp(param, "async_probe") == 0) {
> +		mod->async_probe_requested = true;
> +		return 0;
> +	}

Generally looks good to me.

Thanks a lot for doing this! :)

-- 
tejun

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Sun, Sep 28, 2014 at 11:03:29AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Fri, Sep 26, 2014 at 02:57:17PM -0700, Luis R. Rodriguez wrote:
> ...
> > Systemd should consider enabling async probe on device drivers
> > it loads through systemd-udev but probably does not want to
> > enable it for modules loaded through systemd-modules-load
> > (modules-load.d). At least on my booting enablign async probe
> > for all modules fails to boot as such in order to make this
> 
> Did you find out why boot failed with those modules?

No, it seems this was early in boot and I haven't been able to capture the logs
yet of the faults. More on this below.

> > a bit more useful we whitelist a few buses where it should be
> > at least in theory safe to try to enable async probe. This
> > way even if systemd tried to ask to enable async probe for all
> > its device drivers the kernel won't blindly do this. We also
> > have the sync_probe flag which device drivers can themselves
> > enable *iff* its known the device driver should never async
> > probe.
> > 
> > In order to help *test* things folks can use the bus.safe_mod_async_probe=1
> > kernel parameter which will work as if userspace would have
> > requested all modules to load with async probe. Daring folks can
> > also use bus.force_mod_async_probe=1 which will enable asynch probe
> > even on buses not tested in any way yet, if you use that though
> > you're on your own.
> 
> If those two knobs are meant for debugging, let's please make that
> fact immediately evident.  e.g. Make them ugly boot params like
> "__DEVEL__driver_force_mod_async_probe".  Devel/debug options ending
> up becoming stable interface are really nasty.

Sure make sense, I wasn't quite sure how to make this quite clear,
a naming convention seems good to me but I also had added at least
a print about this on the log. Ideally I think a TAIN_DEBUG would
be best and it seems it could be useful for many other cases in
the kernel, we could also just re-use TAINT_CRAP as well. Thoughts?
Greg?

> > +struct driver_attach_work {
> > +	struct work_struct work;
> > +	struct device_driver *driver;
> > +};
> > +
> >  struct driver_private {
> >  	struct kobject kobj;
> >  	struct klist klist_devices;
> >  	struct klist_node knode_bus;
> >  	struct module_kobject *mkobj;
> > +	struct driver_attach_work *attach_work;
> >  	struct device_driver *driver;
> >  };
> 
> How many bytes are we saving by allocating it separately?

This saves us 24 bytes per device driver.

>  Can't we just embed it in driver_private?

We sure can and it is my preference to do that as well but just
in case I wanted to take the alternative space saving approach
as well and let folks decide. There's also the technical aspect
of hiding that data structure from drivers, and that may be worth
to do but I personally also prefer the simplicity of stuffing
it on the public data structure, as you noted below we could then also
unconditionally flush_work() on __device_release_driver().

Greg, any preference?

> > +static void driver_attach_workfn(struct work_struct *work)
> > +{
> > +	int ret;
> > +	struct driver_attach_work *attach_work =
> > +		container_of(work, struct driver_attach_work, work);
> > +	struct device_driver *drv = attach_work->driver;
> > +	ktime_t calltime, delta, rettime;
> > +	unsigned long long duration;
> 
> This could just be a personal preference but I think it's easier to
> read if local vars w/ initializers come before the ones w/o.

We gotta standardize on *something*, I tend to declare them
in the order in which they are used, in this case I failed to
list calltime first, but yeah I'll put initialized first, I
don't care much.

> > +
> > +	calltime = ktime_get();
> > +
> > +	ret = driver_attach(drv);
> > +	if (ret != 0) {
> > +		remove_driver_private(drv);
> > +		bus_put(drv->bus);
> > +	}
> > +
> > +	rettime = ktime_get();
> > +	delta = ktime_sub(rettime, calltime);
> > +	duration = (unsigned long long) ktime_to_ns(delta) >> 10;
> > +
> > +	pr_debug("bus: '%s': add driver %s attach completed after %lld usecs\n",
> > +		 drv->bus->name, drv->name, duration);
> 
> Why do we have the above printout for async path but not sync path?
> It's kinda weird for the code path to diverge like this.  Shouldn't
> the only difference be the context probes are running from?

Yeah sure, I'll remove this, it was useful for me for testing purposes
in evaluation against kthreads / sync runs, but that certainly was mostly
for debugging.

> ...
> > +static bool drv_enable_async_probe(struct device_driver *drv,
> > +				   struct bus_type *bus)
> > +{
> > +	struct module *mod;
> > +
> > +	if (!drv->owner || drv->sync_probe)
> > +		return false;
> > +
> > +	if (force_mod_async)
> > +		return true;
> > +
> > +	mod = drv->owner;
> > +	if (!safe_mod_async && !mod->async_probe_requested)
> > +		return false;
> > +
> > +	/* For now lets avoid stupid bug reports */
> > +	if (!strcmp(bus->name, "pci") ||
> > +	    !strcmp(bus->name, "pci_express") ||
> > +	    !strcmp(bus->name, "hid") ||
> > +	    !strcmp(bus->name, "sdio") ||
> > +	    !strcmp(bus->name, "gameport") ||
> > +	    !strcmp(bus->name, "mmc") ||
> > +	    !strcmp(bus->name, "i2c") ||
> > +	    !strcmp(bus->name, "platform") ||
> > +	    !strcmp(bus->name, "usb"))
> > +		return true;
> 
> Ugh... things like this tend to become permanent.  Do we really need
> this?  And how are we gonna find out what's broken why w/o bug
> reports?

Yeah... well we have two options, one is have something like this to
at least make it generally useful or remove this and let folks who
care start fixing async for all modules. The downside to removing
this is it makes async probe pretty much useless on most systems
right now, it would mean systemd would have to probably consider
the list above if they wanted to start using this without expecting
systems to not work.

Let me know what is preferred.

> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index e4ffbcf..7999aba 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -507,6 +507,13 @@ static void __device_release_driver(struct device *dev)
> >  
> >  	drv = dev->driver;
> >  	if (drv) {
> > +		if (drv->owner && !drv->sync_probe) {
> > +			struct module *mod = drv->owner;
> > +			struct driver_private *priv = drv->p;
> > +
> > +			if (mod->async_probe_requested)
> > +				flush_work(&priv->attach_work->work);
> 
> This can be unconditional flush_work(&priv->attach_work) if attach_work
> isn't separately allocated.

Indeed.

> >  static int unknown_module_param_cb(char *param, char *val, const char *modname,
> >  				   void *arg)
> >  {
> > +	int ret;
> > +	struct module *mod = arg;
> 
> Ditto with the order of definitions.

Amended.

> Generally looks good to me.
> 
> Thanks a lot for doing this! :)

Thanks for the review and pointers so far.

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Tejun Heo]

Hello, Luis.

On Mon, Sep 29, 2014 at 11:22:08PM +0200, Luis R. Rodriguez wrote:
> > > +	/* For now lets avoid stupid bug reports */
> > > +	if (!strcmp(bus->name, "pci") ||
> > > +	    !strcmp(bus->name, "pci_express") ||
> > > +	    !strcmp(bus->name, "hid") ||
> > > +	    !strcmp(bus->name, "sdio") ||
> > > +	    !strcmp(bus->name, "gameport") ||
> > > +	    !strcmp(bus->name, "mmc") ||
> > > +	    !strcmp(bus->name, "i2c") ||
> > > +	    !strcmp(bus->name, "platform") ||
> > > +	    !strcmp(bus->name, "usb"))
> > > +		return true;
> > 
> > Ugh... things like this tend to become permanent.  Do we really need
> > this?  And how are we gonna find out what's broken why w/o bug
> > reports?
> 
> Yeah... well we have two options, one is have something like this to
> at least make it generally useful or remove this and let folks who
> care start fixing async for all modules. The downside to removing
> this is it makes async probe pretty much useless on most systems
> right now, it would mean systemd would have to probably consider
> the list above if they wanted to start using this without expecting
> systems to not work.

So, I'd much prefer blacklist approach if something like this is a
necessity.  That way, we'd at least know what doesn't work.

Thanks.

-- 
tejun

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Mon, Sep 29, 2014 at 05:26:01PM -0400, Tejun Heo wrote:
> Hello, Luis.
> 
> On Mon, Sep 29, 2014 at 11:22:08PM +0200, Luis R. Rodriguez wrote:
> > > > +	/* For now lets avoid stupid bug reports */
> > > > +	if (!strcmp(bus->name, "pci") ||
> > > > +	    !strcmp(bus->name, "pci_express") ||
> > > > +	    !strcmp(bus->name, "hid") ||
> > > > +	    !strcmp(bus->name, "sdio") ||
> > > > +	    !strcmp(bus->name, "gameport") ||
> > > > +	    !strcmp(bus->name, "mmc") ||
> > > > +	    !strcmp(bus->name, "i2c") ||
> > > > +	    !strcmp(bus->name, "platform") ||
> > > > +	    !strcmp(bus->name, "usb"))
> > > > +		return true;
> > > 
> > > Ugh... things like this tend to become permanent.  Do we really need
> > > this?  And how are we gonna find out what's broken why w/o bug
> > > reports?
> > 
> > Yeah... well we have two options, one is have something like this to
> > at least make it generally useful or remove this and let folks who
> > care start fixing async for all modules. The downside to removing
> > this is it makes async probe pretty much useless on most systems
> > right now, it would mean systemd would have to probably consider
> > the list above if they wanted to start using this without expecting
> > systems to not work.
> 
> So, I'd much prefer blacklist approach if something like this is a
> necessity.  That way, we'd at least know what doesn't work.

For buses? Or do you mean you'd want to wait until we have a decent
list of drivers with the sync probe flag set? If the later it may take
a while to get that list for this to be somewhat useful.

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Tue, Sep 30, 2014 at 09:21:59AM +0200, Luis R. Rodriguez wrote:
> On Mon, Sep 29, 2014 at 05:26:01PM -0400, Tejun Heo wrote:
> > Hello, Luis.
> > 
> > On Mon, Sep 29, 2014 at 11:22:08PM +0200, Luis R. Rodriguez wrote:
> > > > > +	/* For now lets avoid stupid bug reports */
> > > > > +	if (!strcmp(bus->name, "pci") ||
> > > > > +	    !strcmp(bus->name, "pci_express") ||
> > > > > +	    !strcmp(bus->name, "hid") ||
> > > > > +	    !strcmp(bus->name, "sdio") ||
> > > > > +	    !strcmp(bus->name, "gameport") ||
> > > > > +	    !strcmp(bus->name, "mmc") ||
> > > > > +	    !strcmp(bus->name, "i2c") ||
> > > > > +	    !strcmp(bus->name, "platform") ||
> > > > > +	    !strcmp(bus->name, "usb"))
> > > > > +		return true;
> > > > 
> > > > Ugh... things like this tend to become permanent.  Do we really need
> > > > this?  And how are we gonna find out what's broken why w/o bug
> > > > reports?
> > > 
> > > Yeah... well we have two options, one is have something like this to
> > > at least make it generally useful or remove this and let folks who
> > > care start fixing async for all modules. The downside to removing
> > > this is it makes async probe pretty much useless on most systems
> > > right now, it would mean systemd would have to probably consider
> > > the list above if they wanted to start using this without expecting
> > > systems to not work.
> > 
> > So, I'd much prefer blacklist approach if something like this is a
> > necessity.  That way, we'd at least know what doesn't work.
> 
> For buses? Or do you mean you'd want to wait until we have a decent
> list of drivers with the sync probe flag set? If the later it may take
> a while to get that list for this to be somewhat useful.

OK I'm removing this part and it works well for me now on my laptop
and an AMD server without a white list, so all the junk above will
be removed in the next series.

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Greg KH]

On Mon, Sep 29, 2014 at 11:22:08PM +0200, Luis R. Rodriguez wrote:
> On Sun, Sep 28, 2014 at 11:03:29AM -0400, Tejun Heo wrote:
> > Hello,
> > 
> > On Fri, Sep 26, 2014 at 02:57:17PM -0700, Luis R. Rodriguez wrote:
> > ...
> > > Systemd should consider enabling async probe on device drivers
> > > it loads through systemd-udev but probably does not want to
> > > enable it for modules loaded through systemd-modules-load
> > > (modules-load.d). At least on my booting enablign async probe
> > > for all modules fails to boot as such in order to make this
> > 
> > Did you find out why boot failed with those modules?
> 
> No, it seems this was early in boot and I haven't been able to capture the logs
> yet of the faults. More on this below.
> 
> > > a bit more useful we whitelist a few buses where it should be
> > > at least in theory safe to try to enable async probe. This
> > > way even if systemd tried to ask to enable async probe for all
> > > its device drivers the kernel won't blindly do this. We also
> > > have the sync_probe flag which device drivers can themselves
> > > enable *iff* its known the device driver should never async
> > > probe.
> > > 
> > > In order to help *test* things folks can use the bus.safe_mod_async_probe=1
> > > kernel parameter which will work as if userspace would have
> > > requested all modules to load with async probe. Daring folks can
> > > also use bus.force_mod_async_probe=1 which will enable asynch probe
> > > even on buses not tested in any way yet, if you use that though
> > > you're on your own.
> > 
> > If those two knobs are meant for debugging, let's please make that
> > fact immediately evident.  e.g. Make them ugly boot params like
> > "__DEVEL__driver_force_mod_async_probe".  Devel/debug options ending
> > up becoming stable interface are really nasty.
> 
> Sure make sense, I wasn't quite sure how to make this quite clear,
> a naming convention seems good to me but I also had added at least
> a print about this on the log. Ideally I think a TAIN_DEBUG would
> be best and it seems it could be useful for many other cases in
> the kernel, we could also just re-use TAINT_CRAP as well. Thoughts?
> Greg?

TAINT_CRAP is for drivers/staging/ code, don't try to repurpose it for
some other horrid option.  There's no reason we can't add more taint
flags for this.

greg k-h

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Mon, Sep 29, 2014 at 2:59 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> Sure make sense, I wasn't quite sure how to make this quite clear,
>> a naming convention seems good to me but I also had added at least
>> a print about this on the log. Ideally I think a TAIN_DEBUG would
>> be best and it seems it could be useful for many other cases in
>> the kernel, we could also just re-use TAINT_CRAP as well. Thoughts?
>> Greg?
>
> TAINT_CRAP is for drivers/staging/ code, don't try to repurpose it for
> some other horrid option.  There's no reason we can't add more taint
> flags for this.

OK thanks, I'll add TAINT_DEBUG. Any preference where to stuff struct
driver_attach_work *attach_work ? On the private data structure as
this patch currently implements, saving us 24 bytes and hiding it from
drivers, or stuffing it on the device driver and simplifying the core
code?

 Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Greg KH]

On Mon, Sep 29, 2014 at 03:10:22PM -0700, Luis R. Rodriguez wrote:
> On Mon, Sep 29, 2014 at 2:59 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> Sure make sense, I wasn't quite sure how to make this quite clear,
> >> a naming convention seems good to me but I also had added at least
> >> a print about this on the log. Ideally I think a TAIN_DEBUG would
> >> be best and it seems it could be useful for many other cases in
> >> the kernel, we could also just re-use TAINT_CRAP as well. Thoughts?
> >> Greg?
> >
> > TAINT_CRAP is for drivers/staging/ code, don't try to repurpose it for
> > some other horrid option.  There's no reason we can't add more taint
> > flags for this.
> 
> OK thanks, I'll add TAINT_DEBUG. Any preference where to stuff struct
> driver_attach_work *attach_work ? On the private data structure as
> this patch currently implements, saving us 24 bytes and hiding it from
> drivers, or stuffing it on the device driver and simplifying the core
> code?

I honestly haven't even looked at this series, sorry.  It's too late
near the close of the merge window for 3.18 and have been on the road
for the past week in France.

greg k-h

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Tom Gundersen]

Hi Luis,

Thanks for the patches and the detailed analysis.

Feel free to add

Acked-by: Tom Gundersen <teg@jklm.no>

Minor comments on the commit message below.

On Fri, Sep 26, 2014 at 11:57 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> Some init systems may wish to express the desire to have
> device drivers run their device driver's bus probe() run
> asynchronously. This implements support for this and
> allows userspace to request async probe as a preference
> through a generic shared device driver module parameter,
> async_probe. Implemention for async probe is supported
> through a module parameter given that since synchronous
> probe has been prevalent for years some userspace might
> exist which relies on the fact that the device driver will
> probe synchronously and the assumption that devices it
> provides will be immediately available after this.
>
> Some device driver might not be able to run async probe
> so we enable device drivers to annotate this to prevent
> this module parameter from having any effect on them.
>
> This implementation uses queue_work(system_unbound_wq)
> to queue async probes, this should enable probe to run
> slightly *faster* if the driver's probe path did not
> have much interaction with other workqueues otherwise
> it may run _slightly_ slower. Tests were done with cxgb4,
> which is known to take long on probe, both without
> having to run request_firmware() [0] and then by
> requiring it to use request_firmware() [1]. The
> difference in run time are only measurable in microseconds:
>
> =====================================================================|
> strategy                                fw (usec)       no-fw (usec) |
> ---------------------------------------------------------------------|
> synchronous                             24472569        1307563      |
> kthread                                 25066415.5      1309868.5    |
> queue_work(system_unbound_wq)           24913661.5      1307631      |
> ---------------------------------------------------------------------|
>
> In practice, in seconds, the difference is barely noticeable:
>
> =====================================================================|
> strategy                                fw (s)          no-fw (s)    |
> ---------------------------------------------------------------------|
> synchronous                             24.47           1.31         |
> kthread                                 25.07           1.31         |
> queue_work(system_unbound_wq)           24.91           1.31         |
> ---------------------------------------------------------------------|
>
> [0] http://ftp.suse.com/pub/people/mcgrof/async-probe/probe-cgxb4-no-firmware.png
> [1] http://ftp.suse.com/pub/people/mcgrof/async-probe/probe-cgxb4-firmware.png
>
> The rest of the commit log documents why this feature was implemented
> primarily first for systemd and things it should consider next.
>
> Systemd has a general timeout for all workers currently set to 180
> seconds after which it will send a sigkill signal. Systemd now has a
> warning which is issued once it reaches 1/3 of the timeout. The original
> motivation for the systemd timeout was to help track device drivers
> which do not use asynch firmware loading on init() and the timeout was
> originally set to 30 seconds.

Please note that the motivation for the timeout in systemd had nothing
to do with async firmware loading (that was just the case where
problems cropped up). The motivation was to not allow udev-workers to
stay around indefinitely, and hence put an upper-bound on
their duration (initially 180 s). At some point the bound was reduced
to 30 seconds to make sure module-loading would bail out before the
kernel's firmware loading timeout would bail out (60s I believe). That
is no longer relevant, which is why it was safe to reset the timeout
to 180 s.

> Since systemd + kernel are heavily tied in for the purposes of this
> patch it is assumed you have merged on systemd the following
> commits:
>
> 671174136525ddf208cdbe75d6d6bd159afa961f        udev: timeout - warn after a third of the timeout before killing
> b5338a19864ac3f5632aee48069a669479621dca        udev: timeout - increase timeout
> 2e92633dbae52f5ac9b7b2e068935990d475d2cd        udev: bump event timeout to 60 seconds
> be2ea723b1d023b3d385d3b791ee4607cbfb20ca        udev: remove userspace firmware loading support
> 9f20a8a376f924c8eb5423cfc1f98644fc1e2d1a        udev: fixup commit
> dd5eddd28a74a49607a8fffcaf960040dba98479        udev: unify event timeout handling
> 9719859c07aa13539ed2cd4b31972cd30f678543        udevd: add --event-timeout commandline option
>
> Since we bundle together serially driver init() and probe()
> on module initialiation systemd's imposed timeout  put a limit on the
> amount of time a driver init() and probe routines can take. There's a
> few overlooked issues with this and the timeout in general:
>
> 0) Not all drivers are killed, the signal is just sent and
>    the kill will only be acted upoon if the driver you loaded
>    happens to have some code path that either uses kthreads (which
>    as of 786235ee are now killable), or uses some code which checks for
>    fatal_signal_pending() on the kernel somewhere -- i.e: pci_read_vpd().

Shouldn't this be seen as something to be fixed in the kernel? I mean,
do we not want userspace to have the possibility to kill udev/modprobe
even disregarding the worker timeouts (say at shutdown, or before
switching from the initrd)?

> 1) Since systemd is the only one logging the sigkill debugging that
>    drivers are not loaded or in the worst case *failed to boot* because
>    of a sigkill has proven hard to debug.

Care to clarify this a bit? Are the udev logs somehow unclear? If you
think we can improve the logging from udev, please ping me about that
and I'll sort it out.

> 2) When and if the signal is received by the driver somehow
>    the driver may fail at different points in its initialization
>    and unless all error paths on the driver are implemented
>    perfectly this could mean leaving a device in a half
>    initialized state.
>
> 3) The timeout is penalizing device drivers that take long on
>    probe(), this wasn't the original motivation. Systemd seems
>    to have been under assumption that probe was asynchronous,
>    this perhaps is true as an *objective* and goal for *some
>    subsystems* but by no means is it true that we've been on a wide
>    crusade to ensure this for all device drivers. It may be a good
>    idea for *many* device drivers but penalizing them with a kill
>    for taking long on probe is simply unacceptable specially
>    when the timeout is completely arbitrary.

The point is really not to "penalize" anything, we just need to make
sure we put some sort of restrictions on our workers so they don't
hang around forever.

> 4) The driver core calls probe for *all* devices that a driver can
>    claim and it does so serially, so if a device driver will need
>    to probe 3 devices and if probe on the device driver is synchronous
>    the amount of time that module loading will take will be:
>
>    driver load time = init() + probe for 3 devices serially
>
>    The timeout ultimatley ends up limiting the number of devices that
>    *any* device driver can support based on the following formula:
>
>    number_devices =          systemd_timeout
>                       -------------------------------------
>                          max known probe time for driver
>
>    Lastly since the error value passed down is the value of
>    the probe for the last device probed the module will fail
>    to load and all devices will fail to be available.
>
> In the Linux kernel we don't want to work around the timeout,
> instead systemd must be changed to take all the above into
> consideration when issuing any kills on device drivers, ideally
> the sigkill should be considered to be ignored at least for
> kmod. In addition to this we help systemd by giving it what it
> originally considered was there and enable it to ask device
> drivers to use asynchronous probe. This patch addresses that
> feature.
>
> Systemd should consider enabling async probe on device drivers
> it loads through systemd-udev but probably does not want to
> enable it for modules loaded through systemd-modules-load
> (modules-load.d). At least on my booting enablign async probe
> for all modules fails to boot as such in order to make this
> a bit more useful we whitelist a few buses where it should be
> at least in theory safe to try to enable async probe. This
> way even if systemd tried to ask to enable async probe for all
> its device drivers the kernel won't blindly do this. We also
> have the sync_probe flag which device drivers can themselves
> enable *iff* its known the device driver should never async
> probe.
>
> In order to help *test* things folks can use the bus.safe_mod_async_probe=1
> kernel parameter which will work as if userspace would have
> requested all modules to load with async probe. Daring folks can
> also use bus.force_mod_async_probe=1 which will enable asynch probe
> even on buses not tested in any way yet, if you use that though
> you're on your own.
>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Joseph Salisbury <joseph.salisbury@canonical.com>
> Cc: Kay Sievers <kay@vrfy.org>
> Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
> Cc: Tim Gardner <tim.gardner@canonical.com>
> Cc: Pierre Fersing <pierre-fersing@pierref.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Benjamin Poirier <bpoirier@suse.de>
> Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
> Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
> Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
> Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
> Cc: Casey Leedom <leedom@chelsio.com>
> Cc: Hariprasad S <hariprasad@chelsio.com>
> Cc: Santosh Rastapur <santosh@chelsio.com>
> Cc: MPT-FusionLinux.pdl@avagotech.com
> Cc: linux-scsi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  drivers/base/base.h    |   6 +++
>  drivers/base/bus.c     | 137 +++++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/base/dd.c      |   7 +++
>  include/linux/module.h |   2 +
>  kernel/module.c        |  12 ++++-
>  5 files changed, 159 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 251c5d3..24836f1 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -43,11 +43,17 @@ struct subsys_private {
>  };
>  #define to_subsys_private(obj) container_of(obj, struct subsys_private, subsys.kobj)
>
> +struct driver_attach_work {
> +       struct work_struct work;
> +       struct device_driver *driver;
> +};
> +
>  struct driver_private {
>         struct kobject kobj;
>         struct klist klist_devices;
>         struct klist_node knode_bus;
>         struct module_kobject *mkobj;
> +       struct driver_attach_work *attach_work;
>         struct device_driver *driver;
>  };
>  #define to_driver(obj) container_of(obj, struct driver_private, kobj)
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index a5f41e4..41e321e6 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -85,6 +85,7 @@ static void driver_release(struct kobject *kobj)
>         struct driver_private *drv_priv = to_driver(kobj);
>
>         pr_debug("driver: '%s': %s\n", kobject_name(kobj), __func__);
> +       kfree(drv_priv->attach_work);
>         kfree(drv_priv);
>  }
>
> @@ -662,10 +663,125 @@ static void remove_driver_private(struct device_driver *drv)
>         struct driver_private *priv = drv->p;
>
>         kobject_put(&priv->kobj);
> +       kfree(priv->attach_work);
>         kfree(priv);
>         drv->p = NULL;
>  }
>
> +static void driver_attach_workfn(struct work_struct *work)
> +{
> +       int ret;
> +       struct driver_attach_work *attach_work =
> +               container_of(work, struct driver_attach_work, work);
> +       struct device_driver *drv = attach_work->driver;
> +       ktime_t calltime, delta, rettime;
> +       unsigned long long duration;
> +
> +       calltime = ktime_get();
> +
> +       ret = driver_attach(drv);
> +       if (ret != 0) {
> +               remove_driver_private(drv);
> +               bus_put(drv->bus);
> +       }
> +
> +       rettime = ktime_get();
> +       delta = ktime_sub(rettime, calltime);
> +       duration = (unsigned long long) ktime_to_ns(delta) >> 10;
> +
> +       pr_debug("bus: '%s': add driver %s attach completed after %lld usecs\n",
> +                drv->bus->name, drv->name, duration);
> +}
> +
> +int bus_driver_async_probe(struct device_driver *drv)
> +{
> +       struct driver_private *priv = drv->p;
> +
> +       priv->attach_work = kzalloc(sizeof(struct driver_attach_work),
> +                                   GFP_KERNEL);
> +       if (!priv->attach_work)
> +               return -ENOMEM;
> +
> +       priv->attach_work->driver = drv;
> +       INIT_WORK(&priv->attach_work->work, driver_attach_workfn);
> +
> +       /* Keep this as pr_info() until this is prevalent */
> +       pr_info("bus: '%s': probe for driver %s is run asynchronously\n",
> +                drv->bus->name, drv->name);
> +
> +       queue_work(system_unbound_wq, &priv->attach_work->work);
> +
> +       return 0;
> +}
> +
> +/*
> + */
> +static bool safe_mod_async = false;
> +module_param_named(safe_mod_async_probe, safe_mod_async, bool, 0400);
> +MODULE_PARM_DESC(safe_mod_async_probe,
> +                "Enable async probe on all modules safely");
> +
> +static bool force_mod_async = false;
> +module_param_named(force_mod_async_probe, force_mod_async, bool, 0400);
> +MODULE_PARM_DESC(force_mod_async_probe,
> +                "Force async probe on all modules");
> +
> +/**
> + * drv_enable_async_probe - evaluates if async probe should be used
> + * @drv: device driver to evaluate
> + * @bus: the bus for the device driver
> + *
> + * The driver core supports enabling asynchronous probe on device drivers
> + * by requiring userspace to pass the module parameter "async_probe".
> + * Currently only modules are enabled to use this feature. If a device
> + * driver is known to not work properly with asynchronous probe they
> + * can force disable asynchronous probe from being enabled through
> + * userspace by adding setting sync_probe to true on the @drv. We require
> + * async probe to be requested from userspace given that we have historically
> + * supported synchronous probe and some userspaces may exist which depend
> + * on this functionality. Userspace may wish to use asynchronous probe for
> + * most device drivers but since this can fail boot in practice we only
> + * enable it currently for a set of buses.
> + *
> + * If you'd like to test enabling async probe for all buses whitelisted
> + * you can enable the safe_mod_async_probe module parameter. Note that its
> + * not a good idea to always enable this, in particular you probably don't
> + * want drivers under modules-load.d to use this. This module parameter should
> + * only be used to help test. If you'd like to test even futher you can
> + * use force_mod_async_probe, that will force enable async probe on all
> + * drivers, regardless if its bus type, it should however be used with
> + * caution.
> + */
> +static bool drv_enable_async_probe(struct device_driver *drv,
> +                                  struct bus_type *bus)
> +{
> +       struct module *mod;
> +
> +       if (!drv->owner || drv->sync_probe)
> +               return false;
> +
> +       if (force_mod_async)
> +               return true;
> +
> +       mod = drv->owner;
> +       if (!safe_mod_async && !mod->async_probe_requested)
> +               return false;
> +
> +       /* For now lets avoid stupid bug reports */
> +       if (!strcmp(bus->name, "pci") ||
> +           !strcmp(bus->name, "pci_express") ||
> +           !strcmp(bus->name, "hid") ||
> +           !strcmp(bus->name, "sdio") ||
> +           !strcmp(bus->name, "gameport") ||
> +           !strcmp(bus->name, "mmc") ||
> +           !strcmp(bus->name, "i2c") ||
> +           !strcmp(bus->name, "platform") ||
> +           !strcmp(bus->name, "usb"))
> +               return true;
> +
> +       return false;
> +}
> +
>  /**
>   * bus_add_driver - Add a driver to the bus.
>   * @drv: driver.
> @@ -675,6 +791,7 @@ int bus_add_driver(struct device_driver *drv)
>         struct bus_type *bus;
>         struct driver_private *priv;
>         int error = 0;
> +       bool async_probe = false;
>
>         bus = bus_get(drv->bus);
>         if (!bus)
> @@ -696,11 +813,19 @@ int bus_add_driver(struct device_driver *drv)
>         if (error)
>                 goto out_unregister;
>
> +       async_probe = drv_enable_async_probe(drv, bus);
> +
>         klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
>         if (drv->bus->p->drivers_autoprobe) {
> -               error = driver_attach(drv);
> -               if (error)
> -                       goto out_unregister;
> +               if (async_probe) {
> +                       error = bus_driver_async_probe(drv);
> +                       if (error)
> +                               goto out_unregister;
> +               } else {
> +                       error = driver_attach(drv);
> +                       if (error)
> +                               goto out_unregister;
> +               }
>         }
>         module_add_driver(drv->owner, drv);
>
> @@ -1267,6 +1392,12 @@ EXPORT_SYMBOL_GPL(subsys_virtual_register);
>
>  int __init buses_init(void)
>  {
> +       if (unlikely(safe_mod_async))
> +               pr_info("Enabled safe_mod_async -- you may run into issues\n");
> +
> +       if (unlikely(force_mod_async))
> +               pr_info("Enabling force_mod_async -- you're on your own!\n");
> +
>         bus_kset = kset_create_and_add("bus", &bus_uevent_ops, NULL);
>         if (!bus_kset)
>                 return -ENOMEM;
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index e4ffbcf..7999aba 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -507,6 +507,13 @@ static void __device_release_driver(struct device *dev)
>
>         drv = dev->driver;
>         if (drv) {
> +               if (drv->owner && !drv->sync_probe) {
> +                       struct module *mod = drv->owner;
> +                       struct driver_private *priv = drv->p;
> +
> +                       if (mod->async_probe_requested)
> +                               flush_work(&priv->attach_work->work);
> +               }
>                 pm_runtime_get_sync(dev);
>
>                 driver_sysfs_remove(dev);
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 71f282a..1e9e017 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -271,6 +271,8 @@ struct module {
>         bool sig_ok;
>  #endif
>
> +       bool async_probe_requested;
> +
>         /* symbols that will be GPL-only in the near future. */
>         const struct kernel_symbol *gpl_future_syms;
>         const unsigned long *gpl_future_crcs;
> diff --git a/kernel/module.c b/kernel/module.c
> index 88f3d6c..31d71ff 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3175,8 +3175,16 @@ out:
>  static int unknown_module_param_cb(char *param, char *val, const char *modname,
>                                    void *arg)
>  {
> +       int ret;
> +       struct module *mod = arg;
> +
> +       if (strcmp(param, "async_probe") == 0) {
> +               mod->async_probe_requested = true;
> +               return 0;
> +       }
> +
>         /* Check for magic 'dyndbg' arg */
> -       int ret = ddebug_dyndbg_module_param_cb(param, val, modname);
> +       ret = ddebug_dyndbg_module_param_cb(param, val, modname);
>         if (ret != 0)
>                 pr_warn("%s: unknown parameter '%s' ignored\n", modname, param);
>         return 0;
> @@ -3278,7 +3286,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>
>         /* Module is ready to execute: parsing args may do that. */
>         after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
> -                                 -32768, 32767, NULL,
> +                                 -32768, 32767, mod,
>                                   unknown_module_param_cb);
>         if (IS_ERR(after_dashes)) {
>                 err = PTR_ERR(after_dashes);
> --
> 2.1.0
>

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Sun, Sep 28, 2014 at 07:07:24PM +0200, Tom Gundersen wrote:
> On Fri, Sep 26, 2014 at 11:57 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > Systemd has a general timeout for all workers currently set to 180
> > seconds after which it will send a sigkill signal. Systemd now has a
> > warning which is issued once it reaches 1/3 of the timeout. The original
> > motivation for the systemd timeout was to help track device drivers
> > which do not use asynch firmware loading on init() and the timeout was
> > originally set to 30 seconds.
> 
> Please note that the motivation for the timeout in systemd had nothing
> to do with async firmware loading (that was just the case where
> problems cropped up).

*Part *of the original kill logic, according to the commit log, was actually
due to the assumption that the issues observed *were* synchronous firmware
loading on module init():

commit e64fae5573e566ce4fd9b23c68ac8f3096603314
Author: Kay Sievers <kay.sievers@vrfy.org>
Date:   Wed Jan 18 05:06:18 2012 +0100

    udevd: kill hanging event processes after 30 seconds
    
    Some broken kernel drivers load firmware synchronously in the module init
    path and block modprobe until the firmware request is fulfilled.
    <...>

My point here is not to point fingers but to explain why we went on with
this and how we failed to realize only until later that the driver core
ran probe together with init. When a few folks pointed out the issues
with the kill the issue was punted back to kernel developers and the
assumption even among some kernel maintainers was that it was init paths
with sync behaviour that was causing some delays and they were broken
drivers. It is important to highlight these assumptions ended up setting
us off on the wrong path for a while in a hunt to try to fix this issue
either in driver or elsewhere.

> The motivation was to not allow udev-workers to
> stay around indefinitely, and hence put an upper-bound on
> their duration (initially 180 s). At some point the bound was reduced
> to 30 seconds to make sure module-loading would bail out before the
> kernel's firmware loading timeout would bail out (60s I believe).

Sure, part of it was that, but folks beat on driver developer about
the kill insisting it was drivers that were broken. It was only until
Chelsie folks called bloody murder becuase their delays were on probe
that we realized there was a bit more to this than what was being pushed
back on to driver developers.

> That
> is no longer relevant, which is why it was safe to reset the timeout
> to 180 s.

Indeed :D

> > Since systemd + kernel are heavily tied in for the purposes of this
> > patch it is assumed you have merged on systemd the following
> > commits:
> >
> > 671174136525ddf208cdbe75d6d6bd159afa961f        udev: timeout - warn after a third of the timeout before killing
> > b5338a19864ac3f5632aee48069a669479621dca        udev: timeout - increase timeout
> > 2e92633dbae52f5ac9b7b2e068935990d475d2cd        udev: bump event timeout to 60 seconds
> > be2ea723b1d023b3d385d3b791ee4607cbfb20ca        udev: remove userspace firmware loading support
> > 9f20a8a376f924c8eb5423cfc1f98644fc1e2d1a        udev: fixup commit
> > dd5eddd28a74a49607a8fffcaf960040dba98479        udev: unify event timeout handling
> > 9719859c07aa13539ed2cd4b31972cd30f678543        udevd: add --event-timeout commandline option
> >
> > Since we bundle together serially driver init() and probe()
> > on module initialiation systemd's imposed timeout  put a limit on the
> > amount of time a driver init() and probe routines can take. There's a
> > few overlooked issues with this and the timeout in general:
> >
> > 0) Not all drivers are killed, the signal is just sent and
> >    the kill will only be acted upoon if the driver you loaded
> >    happens to have some code path that either uses kthreads (which
> >    as of 786235ee are now killable), or uses some code which checks for
> >    fatal_signal_pending() on the kernel somewhere -- i.e: pci_read_vpd().
> 
> Shouldn't this be seen as something to be fixed in the kernel?

That's a great question. In practice now after CVE-2012-4398 and its series of
patches added which enabled OOM to kill things followed by 786235ee to also
handle OOM on kthreads it seems imperative we strive towards this, in practive
however if you're getting OOMs on boot you have far more serious issue to be
concerned over than handling CVE-2012-4398. Another issue is that even if we
wanted to address this a critical right now on module loading driver error
paths tend to be pretty buggy and we'd probably end up causing more issues than
fixing anything if the sigkill that triggered this was an arbitrary timeout,
specially if the timeout is not properly justified. Addressing sigkill due
to OOM is important, but as noted if you're running out of memory at load
time you have a bit other problems to be concerned over.

So extending the kill onto more drivers *because* of the timeout is probably
not a good reason as it would probably create more issue than fix anything
right now.

> I mean,
> do we not want userspace to have the possibility to kill udev/modprobe
> even disregarding the worker timeouts (say at shutdown, or before
> switching from the initrd)?

That's a good point and I think the merit to handle a kill due to the
other reasons (shutdown, switching from the initrd) should be addressed
separately. I mean that validating addressing the kill for the other
reasons does not validate the existing kill on timeout for synchronous
probing.

If its important to handle the kill on shutdown / switching initrd
that should be dealt with orthogonally.

> > 1) Since systemd is the only one logging the sigkill debugging that
> >    drivers are not loaded or in the worst case *failed to boot* because
> >    of a sigkill has proven hard to debug.
> 
> Care to clarify this a bit? Are the udev logs somehow unclear? 

Sure, so the problem is that folks debugging were not aware of what systemd was
doing.  Let me be clear that the original 30 second sigkill timeout thing was
passed down onto driver maintainers as a non-documented new kernel policy
slap-in-the-face-you-must-obviously-be-doing-something-wrong (TM) approach.
This was a policy decision passed down as a *reactive* measure, not many folks
were aware of it and of what systemd was doing. What made the situation even worse
was that as noted on 1) even though the sigkill was being sent since commit
e64fae55 (January 2012) on systemd the sigkill was not being picked up on many
drivers. To be clear the sigkill was being picked up if you had a driver that
by chance had some code on init / probe that by chance checked for
fatal_signal_pending(), and even when that triggered folks debugging were in no
way shape or form expecting a sigkill from userspace on modprobe as it was not well
known that this was part of the policy they should follow. Shit started to hit
the fan a bit more widely when kernel commit 786235ee (Nov 2013) was merged
upstream which allowed kthreads to be killed, and more drivers started failing.

An example of an ancient bug that no one handled until recently:

https://bugzilla.kernel.org/show_bug.cgi?id=59581

There is a proper fix to this now but the kill was what was causing this
in the first place. The kill was justified as theese drivers *should*
be using async probe but by no means does that mean the kill was
justified for all subsystems / drivers. The bug also really also sent
people on the wrong track and it was only until  Alexander poked me
about the issue we were seeing on cxbg4 likely being related that we
started to really zeroe in on the real issue.

The first driver reported / studied due to the kill from system was
mptsas:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705

A full bisect was done to even try to understand what the issue was..
Then there was the tug of war between either reverting the patch that
allowed the kthread to be killed or if this was systemd issue which
required increasing the timeout. This was still a storage driver,
and increasing the timeout arbitrarily really would not have helped
address the root cause of the issue.

The next non-storage driver bug that was reported and heavily
debugged was cxgb4 and it wasn't easy to debug:

https://bugzilla.suse.com/show_bug.cgi?id=877622

Conclusion then is that folks were simply not aware of this new de-facto
policy, it was obviously incorrect but well intentioned, and no one
really was paying attention to systemd-udevd logs. If we want chatty
behaviour that people will pick up we probably instead want a WARN()
on the kernel specially before we kill a driver and even then I'm sure
this can irritate some folks.

> If you think we can improve the logging from udev, please ping me about that
> and I'll sort it out.

I think the logging done on systemd is fine, there are a few issues with the
way things trickled down and what we now need to do. First and foremost there
was general communication issue about this new timing policy and obviously it
would have helped if this also had more design / review from others. Its no
one's fault, but we should learn from it. Design policies on systemd that can
affect the kernel / drivers could likely use some bit more review from a wider
audience and probably include folks who are probably going to be more critical
than those who likely would typically be favorable. Without wider review we
could fail to end up with something like a filter bubble [0] but applied to
engineering, a design filter bubble, if you will. So apart from addressing
logging its important to reflect on this issue and try to aim for having
something like a Red Team [1] on design involving systemd and kernel. This is
specially true if we are to really marry these two together more and more. 
The more critical people can be the better, but of course those need to
provide constructive criticism, not just rants.

In terms of logging:

Do we know if distributions / users are reviewing systemd-udevd logs for
certain types of issues with as much dilligence as they put to kernel logs when
systemd makes decision affecting the kernel? If not we should consider a way so
that that happens. In this case the fact that drivers were being killed while
being loaded was missed since it was unexpected that would happen so folks
didn't know to look for that option, but apart from that the *reason* for the
kill probably could have helped too. To help both of these we have to consider if
we are going to keep the sigkill on systemd on module loading due to a timeout.
As you clarified the goal of the timeout is to avoid having udev workers stay
around indefinitely, but I think we need to give kmod workers a bit more
consideration.  The point of this patch set was partly to give systemd what it
assumed was there, but clearly we can't assume all drivers can be loaded
asynchronously without issues right now. That means that even with this
functionality merged systemd will have to cope with the fact that some drivers
will be loaded with synchronous probe.  A general timeout and specially with a
sigkill is probably not a good idea then, unless of course:

0) those device drivers / subsystem maintainer want a timeout
1) the above decision can distinguish between sync probe / async probe
   being done

To address 0) perhaps one solution is that if subsystem maintainers
feel this is needed they can express this on data structure somewhere,
perhaps on the bus and/or have a driver value override, for example.

For 1) we could expose what we end up doing through sysfs.

Of course userspace could also simply want to put in place some
requirements but in terms of a timeout / kill it would have to also
accept that it cannot get what it might want. For instance we now know
it may be that an async probe is not possible on some drivers.

Perhaps its best to think about this differently and address now a
way to do that efficiently instead of reactively. Apart form having
the ability to let systemd ask for async probe, what else do we want
to accomplish?

[0] http://en.wikipedia.org/wiki/Filter_bubble
[1] http://en.wikipedia.org/wiki/Red_team

> > 2) When and if the signal is received by the driver somehow
> >    the driver may fail at different points in its initialization
> >    and unless all error paths on the driver are implemented
> >    perfectly this could mean leaving a device in a half
> >    initialized state.
> >
> > 3) The timeout is penalizing device drivers that take long on
> >    probe(), this wasn't the original motivation. Systemd seems
> >    to have been under assumption that probe was asynchronous,
> >    this perhaps is true as an *objective* and goal for *some
> >    subsystems* but by no means is it true that we've been on a wide
> >    crusade to ensure this for all device drivers. It may be a good
> >    idea for *many* device drivers but penalizing them with a kill
> >    for taking long on probe is simply unacceptable specially
> >    when the timeout is completely arbitrary.
> 
> The point is really not to "penalize" anything, we just need to make
> sure we put some sort of restrictions on our workers so they don't
> hang around forever.

Thanks for clarifying this, can you explain what issues could arise
from making an exception to allowing kmod workers to hang around
completing init + probe over a certain defined amount of time without
being killed?

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Tue, Sep 30, 2014 at 04:27:51AM +0200, Luis R. Rodriguez wrote:
> On Sun, Sep 28, 2014 at 07:07:24PM +0200, Tom Gundersen wrote:
> > On Fri, Sep 26, 2014 at 11:57 PM, Luis R. Rodriguez
> > <mcgrof@do-not-panic.com> wrote:
> > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > > 0) Not all drivers are killed, the signal is just sent and
> > >    the kill will only be acted upoon if the driver you loaded
> > >    happens to have some code path that either uses kthreads (which
> > >    as of 786235ee are now killable), or uses some code which checks for
> > >    fatal_signal_pending() on the kernel somewhere -- i.e: pci_read_vpd().
> > 
> > Shouldn't this be seen as something to be fixed in the kernel?
> 
> That's a great question. In practice now after CVE-2012-4398 and its series of
> patches added which enabled OOM to kill things followed by 786235ee to also
> handle OOM on kthreads it seems imperative we strive towards this, in practive
> however if you're getting OOMs on boot you have far more serious issue to be
> concerned over than handling CVE-2012-4398. Another issue is that even if we
> wanted to address this a critical right now on module loading driver error
> paths tend to be pretty buggy and we'd probably end up causing more issues than
> fixing anything if the sigkill that triggered this was an arbitrary timeout,
> specially if the timeout is not properly justified.

<-- snip -->

> So extending the kill onto more drivers *because* of the timeout is probably
> not a good reason as it would probably create more issue than fix anything
> right now.

A bit more on this. Tejun had added devres while trying to convert libata to
use iomap but in that process also help address buggy failure paths on drivers [0].
Even with devres in place and devm functions being available they actually
haven't been popularized until recent kernels [1]. There is even further
research on precicely these sorts of errors, such as "Hector: Detecting
Resource-Release Omission Faults in error-handling code for systems software" [2]
but unfortunately there is no data over time. Another paper is "An approach to
improving the structure of error-handling code in the Linux kernel" [3] which
tries to address moving error handling code in the middle of the function to gotos
to shared code at the end of the function...

So we have buggy error paths on drivers and trusting them unfortunately isn't
a good idea at this point. They should be fixed but saying we should equally
kill all drivers right now would likley introduce more issues than anything.

[0] http://lwn.net/Articles/215861/
[1] http://www.slideshare.net/ennael/kernel-recipes-2013?qid=f0888b85-377b-4b29-95c3-f4e59822f5b3&v=default&b=&from_search=6
    See slide 6 on graph usage of devm functions over time
[2] http://coccinelle.lip6.fr/papers/dsn2013.pdf
[3] http://coccinelle.lip6.fr/papers/lctes11.pdf

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Tom Gundersen]

On Tue, Sep 30, 2014 at 4:27 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Sun, Sep 28, 2014 at 07:07:24PM +0200, Tom Gundersen wrote:
>> On Fri, Sep 26, 2014 at 11:57 PM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> > Systemd has a general timeout for all workers currently set to 180
>> > seconds after which it will send a sigkill signal. Systemd now has a
>> > warning which is issued once it reaches 1/3 of the timeout. The original
>> > motivation for the systemd timeout was to help track device drivers
>> > which do not use asynch firmware loading on init() and the timeout was
>> > originally set to 30 seconds.
>>
>> Please note that the motivation for the timeout in systemd had nothing
>> to do with async firmware loading (that was just the case where
>> problems cropped up).
>
> *Part *of the original kill logic, according to the commit log, was actually
> due to the assumption that the issues observed *were* synchronous firmware
> loading on module init():
>
> commit e64fae5573e566ce4fd9b23c68ac8f3096603314
> Author: Kay Sievers <kay.sievers@vrfy.org>
> Date:   Wed Jan 18 05:06:18 2012 +0100
>
>     udevd: kill hanging event processes after 30 seconds
>
>     Some broken kernel drivers load firmware synchronously in the module init
>     path and block modprobe until the firmware request is fulfilled.
>     <...>

This was a workaround to avoid a deadlock between udev and the kernel.
The 180 s timeout was already in place before this change, and was not
motivated by firmware loading. Also note that this patch was not about
"tracking device drivers", just about avoiding dead-lock.

> My point here is not to point fingers but to explain why we went on with
> this and how we failed to realize only until later that the driver core
> ran probe together with init. When a few folks pointed out the issues
> with the kill the issue was punted back to kernel developers and the
> assumption even among some kernel maintainers was that it was init paths
> with sync behaviour that was causing some delays and they were broken
> drivers. It is important to highlight these assumptions ended up setting
> us off on the wrong path for a while in a hunt to try to fix this issue
> either in driver or elsewhere.

Ok. I'm not sure the motivations for user-space changes is important
to include in the commit message, but if you do I'll try to clarify
things to avoid misunderstandings.

> Thanks for clarifying this, can you explain what issues could arise
> from making an exception to allowing kmod workers to hang around
> completing init + probe over a certain defined amount of time without
> being killed?

We could run out of udev workers and the whole boot would hang.

The way I see it, the current status from systemd's side is: our
short-term work-around is to increase the timeout, and at the moment
it appears no long-term solution is needed (i.e., it seems like the
right thing to do is to make sure insmod can be near instantaneous, it
appears people are working towards this goal, and so far no examples
have cropped up showing that it is fundamentally impossible (once/if
they do, we should of course revisit the problem)).

Cheers,

Tom

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Tue, Sep 30, 2014 at 11:22:14AM +0200, Tom Gundersen wrote:
> On Tue, Sep 30, 2014 at 4:27 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Sun, Sep 28, 2014 at 07:07:24PM +0200, Tom Gundersen wrote:
> >> On Fri, Sep 26, 2014 at 11:57 PM, Luis R. Rodriguez
> >> <mcgrof@do-not-panic.com> wrote:
> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> > Systemd has a general timeout for all workers currently set to 180
> >> > seconds after which it will send a sigkill signal. Systemd now has a
> >> > warning which is issued once it reaches 1/3 of the timeout. The original
> >> > motivation for the systemd timeout was to help track device drivers
> >> > which do not use asynch firmware loading on init() and the timeout was
> >> > originally set to 30 seconds.
> >>
> >> Please note that the motivation for the timeout in systemd had nothing
> >> to do with async firmware loading (that was just the case where
> >> problems cropped up).
> >
> > *Part *of the original kill logic, according to the commit log, was actually
> > due to the assumption that the issues observed *were* synchronous firmware
> > loading on module init():
> >
> > commit e64fae5573e566ce4fd9b23c68ac8f3096603314
> > Author: Kay Sievers <kay.sievers@vrfy.org>
> > Date:   Wed Jan 18 05:06:18 2012 +0100
> >
> >     udevd: kill hanging event processes after 30 seconds
> >
> >     Some broken kernel drivers load firmware synchronously in the module init
> >     path and block modprobe until the firmware request is fulfilled.
> >     <...>
> 
> This was a workaround to avoid a deadlock between udev and the kernel.
> The 180 s timeout was already in place before this change, and was not
> motivated by firmware loading. Also note that this patch was not about
> "tracking device drivers", just about avoiding dead-lock.

Thanks, can you elaborate on how a deadlock can occur if the kmod
worker is not at some point sigkilled?

> > My point here is not to point fingers but to explain why we went on with
> > this and how we failed to realize only until later that the driver core
> > ran probe together with init. When a few folks pointed out the issues
> > with the kill the issue was punted back to kernel developers and the
> > assumption even among some kernel maintainers was that it was init paths
> > with sync behaviour that was causing some delays and they were broken
> > drivers. It is important to highlight these assumptions ended up setting
> > us off on the wrong path for a while in a hunt to try to fix this issue
> > either in driver or elsewhere.
> 
> Ok. I'm not sure the motivations for user-space changes is important
> to include in the commit message, but if you do I'll try to clarify
> things to avoid misunderstandings.

I can try to omit it on the next series.

> > Thanks for clarifying this, can you explain what issues could arise
> > from making an exception to allowing kmod workers to hang around
> > completing init + probe over a certain defined amount of time without
> > being killed?
> 
> We could run out of udev workers and the whole boot would hang.

Is the issue that if there is no extra worker available and all are
idling on sleep / synchronous long work boot will potentially hang
unless a new worker becomes available to do more work? If so I can
see the sigkill helping for hanging tasks but it doesn't necessarily
mean its a good idea to kill modules loading taking a while. Also
what if the sigkill is just avoided for *just* kmod workers?

> The way I see it, the current status from systemd's side is: our
> short-term work-around is to increase the timeout, and at the moment
> it appears no long-term solution is needed (i.e., it seems like the
> right thing to do is to make sure insmod can be near instantaneous, it
> appears people are working towards this goal, and so far no examples
> have cropped up showing that it is fundamentally impossible (once/if
> they do, we should of course revisit the problem)).

That again would be reactive behaviour, what would prevent avoiding the
sigkill only for kmod workers? Is it known the deadlock is immiment?
If the amount of workers for kmod that would hit the timeout is
considered low I don't see how that's possible and why not just lift
the sigkill.

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Tom Gundersen]

On Tue, Sep 30, 2014 at 5:24 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> > commit e64fae5573e566ce4fd9b23c68ac8f3096603314
>> > Author: Kay Sievers <kay.sievers@vrfy.org>
>> > Date:   Wed Jan 18 05:06:18 2012 +0100
>> >
>> >     udevd: kill hanging event processes after 30 seconds
>> >
>> >     Some broken kernel drivers load firmware synchronously in the module init
>> >     path and block modprobe until the firmware request is fulfilled.
>> >     <...>
>>
>> This was a workaround to avoid a deadlock between udev and the kernel.
>> The 180 s timeout was already in place before this change, and was not
>> motivated by firmware loading. Also note that this patch was not about
>> "tracking device drivers", just about avoiding dead-lock.
>
> Thanks, can you elaborate on how a deadlock can occur if the kmod
> worker is not at some point sigkilled?

This was only relevant whet udev did the firmware loading. modprobe
would wait for the kernel, which would wait for the firmware loading,
which would wait for modprobe. This is no longer a problem as udev
does not do firmware loading any more.

> Is the issue that if there is no extra worker available and all are
> idling on sleep / synchronous long work boot will potentially hang
> unless a new worker becomes available to do more work?

Correct.

> If so I can
> see the sigkill helping for hanging tasks but it doesn't necessarily
> mean its a good idea to kill modules loading taking a while. Also
> what if the sigkill is just avoided for *just* kmod workers?

Depending on the number of devices you have, I suppose we could still
exhaust the workers.

>> The way I see it, the current status from systemd's side is: our
>> short-term work-around is to increase the timeout, and at the moment
>> it appears no long-term solution is needed (i.e., it seems like the
>> right thing to do is to make sure insmod can be near instantaneous, it
>> appears people are working towards this goal, and so far no examples
>> have cropped up showing that it is fundamentally impossible (once/if
>> they do, we should of course revisit the problem)).
>
> That again would be reactive behaviour, what would prevent avoiding the
> sigkill only for kmod workers? Is it known the deadlock is immiment?
> If the amount of workers for kmod that would hit the timeout is
> considered low I don't see how that's possible and why not just lift
> the sigkill.

Making kmod a special case is of course possible. However, as long as
there is no fundamental reason why kmod should get this special
treatment, this just looks like a work-around to me. We already have a
work-around, which is to increase the global timeout. If you still
think we should do something different in systemd, it is probably best
to take the discussion to systemd-devel to make sure all the relevant
people are involved.

Cheers,

Tom

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

As per Tom, adding systemd-devel for advice / review / of the request to avoid
the sigkill for kmod workers. Keeping others on Cc as its a discussion that
I think can help if both camps are involved. Specially since we've been
ping ponging back and forth on this particular topic for a long time now.

On Thu, Oct 02, 2014 at 08:12:37AM +0200, Tom Gundersen wrote:
> On Tue, Sep 30, 2014 at 5:24 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> >> > commit e64fae5573e566ce4fd9b23c68ac8f3096603314
> >> > Author: Kay Sievers <kay.sievers@vrfy.org>
> >> > Date:   Wed Jan 18 05:06:18 2012 +0100
> >> >
> >> >     udevd: kill hanging event processes after 30 seconds
> >> >
> >> >     Some broken kernel drivers load firmware synchronously in the module init
> >> >     path and block modprobe until the firmware request is fulfilled.
> >> >     <...>
> >>
> >> This was a workaround to avoid a deadlock between udev and the kernel.
> >> The 180 s timeout was already in place before this change, and was not
> >> motivated by firmware loading. Also note that this patch was not about
> >> "tracking device drivers", just about avoiding dead-lock.
> >
> > Thanks, can you elaborate on how a deadlock can occur if the kmod
> > worker is not at some point sigkilled?
> 
> This was only relevant whet udev did the firmware loading. modprobe
> would wait for the kernel, which would wait for the firmware loading,
> which would wait for modprobe. This is no longer a problem as udev
> does not do firmware loading any more.

Thanks for clarifying. So the deadlock concern is no longer there, therefore
it is not a reason to keep the sigkill for kmod.

> > Is the issue that if there is no extra worker available and all are
> > idling on sleep / synchronous long work boot will potentially hang
> > unless a new worker becomes available to do more work?
> 
> Correct.

Ok.

> > If so I can
> > see the sigkill helping for hanging tasks but it doesn't necessarily
> > mean its a good idea to kill modules loading taking a while. Also
> > what if the sigkill is just avoided for *just* kmod workers?
> 
> Depending on the number of devices you have, I suppose we could still
> exhaust the workers.

Ok can systemd dynamically create a worker or set of workers per device
that creeps up? Async probe for most drivers will help with this but
having it dynamic should help as well, specially since there are drivers
that will require probe synchronously -- and the fact that async probe
mechanism is not yet merged.

> >> The way I see it, the current status from systemd's side is: our
> >> short-term work-around is to increase the timeout, and at the moment
> >> it appears no long-term solution is needed (i.e., it seems like the
> >> right thing to do is to make sure insmod can be near instantaneous, it
> >> appears people are working towards this goal, and so far no examples
> >> have cropped up showing that it is fundamentally impossible (once/if
> >> they do, we should of course revisit the problem)).
> >
> > That again would be reactive behaviour, what would prevent avoiding the
> > sigkill only for kmod workers? Is it known the deadlock is immiment?
> > If the amount of workers for kmod that would hit the timeout is
> > considered low I don't see how that's possible and why not just lift
> > the sigkill.
> 
> Making kmod a special case is of course possible. However, as long as
> there is no fundamental reason why kmod should get this special
> treatment, this just looks like a work-around to me. 

I've mentioned a series of five reasons why its a bad idea right now to
sigkill modules [0], we're reviewed them each and still at least
items 2-4 remain particularly valid fundamental reasons to avoid it
specially if the deadlock is no longer possible. Running out of
workers because they are loading modules and that is taking a while
is not really a good standing reason to be killing them, specially
if the timeout already is set to a high value. All we're doing there is
limiting Linux / number of devices arbitrarily just to help free
workers, and it seems that should be dealt with differently. Killing
module loading arbitrarily in the middle is not advisable and can cause
more issue than help in any way.

Async probe mechanism will help free workers faster but this patch series is
still being evolved, we should still address the sigkill for kmod workers
separately and see what remaining reasons we have for it in light of the
possible issues highlighted that it can introduce if kept. If we want to
capture drivers taking long on probe each subsystem should handle that and WARN
/ pick up on it, we cannot however assume that this a generally bad things as
discussed before. We will also not be able to async probe *every* driver,
which is why the series allows a flag to specify for this.

[0] https://lkml.org/lkml/2014/9/26/879

> We already have a
> work-around, which is to increase the global timeout. If you still
> think we should do something different in systemd, it is probably best
> to take the discussion to systemd-devel to make sure all the relevant
> people are involved.

Sure, I've included systemd-devel. Hope is we can have a constructive
discussion on the sigkill for kmod.

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Tom Gundersen]

On Thu, Oct 2, 2014 at 10:06 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Thu, Oct 02, 2014 at 08:12:37AM +0200, Tom Gundersen wrote:
>> Making kmod a special case is of course possible. However, as long as
>> there is no fundamental reason why kmod should get this special
>> treatment, this just looks like a work-around to me.
>
> I've mentioned a series of five reasons why its a bad idea right now to
> sigkill modules [0], we're reviewed them each and still at least
> items 2-4 remain particularly valid fundamental reasons to avoid it

So items 2-4 basically say "there currently are drivers that cannot
deal with sigkill after a three minute timeout".

In the short-term we already have the solution: increase the timeout.
In the long-term, we have two choices, either permanently add some
heuristic to udev to deal with drivers taking a very long time to be
inserted, or fix the drivers not to take such a long time. A priori,
it makes no sense to me that drivers spend unbounded amounts of time
to get inserted, so fixing the drivers seems like the most reasonable
approach to me. That said, I'm of course open to be proven wrong if
there are some drivers that fundamentally _must_ take a long time to
insert (but we should then discuss why that is and how we can best
deal with the situation, rather than adding some hack up-front when we
don't even know if it is needed).

Your patch series should go a long way towards fixing the drivers (and
I imagine there being a lot of low-hanging fruit that can easily be
fixed once your series has landed), and the fact that we have now
increased the udev timeout from 30 to 180 seconds should also greatly
reduce the problem.

Cheers,

Tom

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Fri, Oct 3, 2014 at 1:23 AM, Tom Gundersen <teg@jklm.no> wrote:
> On Thu, Oct 2, 2014 at 10:06 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> On Thu, Oct 02, 2014 at 08:12:37AM +0200, Tom Gundersen wrote:
>>> Making kmod a special case is of course possible. However, as long as
>>> there is no fundamental reason why kmod should get this special
>>> treatment, this just looks like a work-around to me.
>>
>> I've mentioned a series of five reasons why its a bad idea right now to
>> sigkill modules [0], we're reviewed them each and still at least
>> items 2-4 remain particularly valid fundamental reasons to avoid it
>
> So items 2-4 basically say "there currently are drivers that cannot
> deal with sigkill after a three minute timeout".

No, dealing with the sigkill gracefully is all related to 2) as it
says its probably a terrible idea to be triggering exit paths at
random points on device drivers on init / probe. And while one could
argue that perhaps that can be cleaned up I provided tons of
references and even *research effort* on this particular area so the
issues over this point should by no means easily be brushed off. And
it may be true that we can fix some things on Linux but a) that
requires a kernel upgrade on users and b) Some users may end up buying
hardware that only is supported through a proprietary driver and
getting those fixes is not trivial and almost impossible on some
cases.

3) says it is fundamentally incorrect to limit with any arbitrary
timeout the bus probe routine

4) talks about how the timeout is creating a limit on the number of
devices a device driver can support on Linux as follows give the
driver core batches *all* probes for one device driver serially:

   number_devices =          systemd_timeout
                      -------------------------------------
                         max known probe time for driver

We have device drivers which we *know* just on *probe* will take over
1 minute, this means that by default for these device drivers folks
can only install 3 devices of that type on a system. One can surely
address things on the kernel but again assuming folks use defaults and
don't upgrade their kernel the sigkill is simply limiting Linux right
now, even if it is for the short term.

> In the short-term we already have the solution: increase the timeout.

Short term implicates what will be supported for a while for tons of
deployments of systemd. The kernel command line work around for
increasing the timeout is a reactive measure, its not addressing the
problem architecturally. If the sigkill is going to be maintained for
kmod its implications should be well documented as well in terms of
the impact and limitations on both device drivers and number of
devices a driver can support.

> In the long-term, we have two choices, either permanently add some
> heuristic to udev to deal with drivers taking a very long time to be
> inserted, or fix the drivers not to take such a long time.

Drivers taking long on init should probably be addressed, drivers
taking long on probe are not broken specially since the driver core
probe's all supported devices on one device driver serially, so the
probe time is actually cumulative.

> A priori,
> it makes no sense to me that drivers spend unbounded amounts of time
> to get inserted, so fixing the drivers seems like the most reasonable
> approach to me. That said, I'm of course open to be proven wrong if
> there are some drivers that fundamentally _must_ take a long time to
> insert (but we should then discuss why that is and how we can best
> deal with the situation, rather than adding some hack up-front when we
> don't even know if it is needed).

Ok hold on. Async probe on the driver core will be a new feature and
there are even caveats that Tejun pointed out which are important for
distributions to consider before embracing it. Of course folks can
ignore these but by no means should it be considered that tons of
device device drivers were broken, what we are providing is a new
mechanism. And then there are device drivers which will need work in
order to use async probe, some will require fixes on init / probe
assumptions as I provided for the amd64_edac driver but for others
only time will tell what is required.

> Your patch series should go a long way towards fixing the drivers (and
> I imagine there being a lot of low-hanging fruit that can easily be
> fixed once your series has landed), and the fact that we have now
> increased the udev timeout from 30 to 180 seconds should also greatly
> reduce the problem.

Sure, I do ask for folks to revisit the short term solution though, I
did my best to communicate / document the issues.

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Dmitry Torokhov]

Hi Luis,

On Fri, Sep 26, 2014 at 02:57:17PM -0700, Luis R. Rodriguez wrote:
> +static bool drv_enable_async_probe(struct device_driver *drv,
> +				   struct bus_type *bus)
> +{
> +	struct module *mod;
> +
> +	if (!drv->owner || drv->sync_probe)
> +		return false;

This bit is one of the biggest issues I have with the patch set. Why async
probing is limited to modules only? I mentioned several times that we need
async probing for built-in drivers and the way you are structuring the flags
(async by default for modules, possibly opt-out of async for modules, forcibly
sync for built-in) it is hard to extend the infrastructure for built-in case.

Also, as far as I can see, you are only considering the case where driver is
being bound to already registered devices. If you have a module that creates a
device for a driver that is already loaded and takes long time to probe you
would still be probing synchronously even if driver/module requested async
behavior.

So for me it is NAK in the current form.

Thanks.

-- 
Dmitry

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Sun, Sep 28, 2014 at 12:22:47PM -0700, Dmitry Torokhov wrote:
> Hi Luis,
> 
> On Fri, Sep 26, 2014 at 02:57:17PM -0700, Luis R. Rodriguez wrote:
> > +static bool drv_enable_async_probe(struct device_driver *drv,
> > +				   struct bus_type *bus)
> > +{
> > +	struct module *mod;
> > +
> > +	if (!drv->owner || drv->sync_probe)
> > +		return false;
> 
> This bit is one of the biggest issues I have with the patch set. Why async
> probing is limited to modules only?

Because Tejun wanted to address this separately, so its not that we will restrict
this but we should have non-module solution added as an evolution on top of this,
as a secondary step.

> I mentioned several times that we need
> async probing for built-in drivers and the way you are structuring the flags
> (async by default for modules, possibly opt-out of async for modules, forcibly
> sync for built-in) it is hard to extend the infrastructure for built-in case.

I confess I haven't tried enabling built-in as a secondary step but its just
due to lack of time right now but I don't think impossible and think actually
think it should be fairly trivial. Are there real blockers to do this that
you see as an evolutionary step?

> Also, as far as I can see, you are only considering the case where driver is
> being bound to already registered devices. If you have a module that creates a
> device for a driver that is already loaded and takes long time to probe you
> would still be probing synchronously even if driver/module requested async
> behavior.

Can you provide an example code path hit here? I'll certainly like to address
that as well.

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Tue, Sep 30, 2014 at 09:15:55AM +0200, Luis R. Rodriguez wrote:
> Can you provide an example code path hit here? I'll certainly like to address
> that as well.

I managed to enable built-in driver support on top of this series,
I'll send them as part of the next series but I suspect we'll want
to discuss blacklist/whitelist a bit more there.

 Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Fri, Sep 26, 2014 at 02:57:17PM -0700, Luis R. Rodriguez wrote:
> +	queue_work(system_unbound_wq, &priv->attach_work->work);

Tejun,

based on my testing so far using system_highpri_wq instead of
system_unbound_wq yields close to par / better boot times
than synchronous probe support for all modules. How set are
you on using system_unbound_wq? About to punt out a new
series which also addresses built-in.

  Luis

Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support

[Luis R. Rodriguez]

On Fri, Oct 03, 2014 at 10:11:26PM +0200, Luis R. Rodriguez wrote:
> On Fri, Sep 26, 2014 at 02:57:17PM -0700, Luis R. Rodriguez wrote:
> > +	queue_work(system_unbound_wq, &priv->attach_work->work);
> 
> Tejun,
> 
> based on my testing so far using system_highpri_wq instead of
> system_unbound_wq yields close to par / better boot times
> than synchronous probe support for all modules. How set are
> you on using system_unbound_wq? About to punt out a new
> series which also addresses built-in.

Nevermind, folks can change this later with better empirical
testing than I can provide and right now the differences I
see are not too conclusive and I suspect we'll see more of
a difference once the right built-in drivers are selected
to probe asynchrounously.

  Luis