* [PATCH v2 0/7] driver-core: async probe support @ 2014-10-03 21:44 Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 1/7] taint: add TAINT_DEBUG for invasive debugging features Luis R. Rodriguez ` (6 more replies) 0 siblings, 7 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-03 21:44 UTC (permalink / raw) To: gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel From: "Luis R. Rodriguez" <mcgrof@suse.com> This second series addresses all comments from the v1 series, it removed the white list, tidies up the commit logs, adds documentation for the kernel parameters, adds a new debug taint flag and uses it for the testing kernel parameters we have added. This series also now goes with built-in async probe support by defining a kernel parameter bus.enable_kern_async=1 userspace can set to indicate to the kernel userspace has been vetted for async probe support. This essentially means you will verify your userspace will not depend on sync probe on scripts / daemons, and if such issues exist you're willing to fix this in userspace. All of these features require userspace intervention so by default synchronous probe is used just as before. Both mechanisms allow us to create a new async probe universe safely without affecting old userspace. Folks wanting to do immediate testing can compile and just load modules manually with async_probe=1 as a module parameter for any module they wish to probe asynchrounously. Folks wishing to test a debug feature to enable *all* modules to be probed asynchronously can enable bus.__DEBUG__module_force_mod_async_probe=1 Likewise to test all built-in drivers with async probe folks can use both bus.enable_kern_async=1 and bus.__DEBUG__kernel_force_mod_async_probe=1. Any of these new __DEBUG__ kernel parameters will taint your kernel, in practice for now folks should not be surprised if they run into issues with any of these parameters until async probe gets widely tested and drivers which require sync probe are found. This series goes with one example driver where sync probe was required, a fix to the driver is pending to enable async probe but the issue has already been identified and should be easy to fix. Folks wishing to test built-in drivers can enable bus.enable_kern_async=1 and then modify their driver with something like this: diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index 247335d..9f0b636 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -7241,6 +7241,7 @@ static struct pci_driver e1000_driver = { .remove = e1000_remove, .driver = { .pm = &e1000_pm_ops, + .prefer_async_probe = true, }, .shutdown = e1000_shutdown, .err_handler = &e1000_err_handler This will *not* taint your kernel. Folks wishing to go all out and use async probe for *all* device drivers, whether built-in or not can enable the following and start debugging drivers as their heart pleases: bus.enable_kern_async=1 bus.__DEBUG__module_force_mod_async_probe=1 bus.__DEBUG__kernel_force_mod_async_probe=1 I've given this a whirl on 3 different machines now, bus.__DEBUG__module_force_mod_async_probe=1 is actually fine for all 3 systems however bus.__DEBUG__kernel_force_mod_async_probe=1 requires quite a bit of drivers identified / annotated for sync probe. Luis R. Rodriguez (7): taint: add TAINT_DEBUG for invasive debugging features 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 module asynchronous probe support driver-core: add preferred async probe option for built-in and modules Documentation/kernel-parameters.txt | 16 ++++ Documentation/sysctl/kernel.txt | 1 + arch/powerpc/mm/hugetlbpage.c | 4 +- drivers/base/base.h | 6 ++ drivers/base/bus.c | 149 ++++++++++++++++++++++++++++++++++-- drivers/base/dd.c | 7 ++ drivers/edac/amd64_edac.c | 1 + include/linux/device.h | 12 +++ include/linux/kernel.h | 1 + include/linux/module.h | 2 + include/linux/moduleparam.h | 3 +- init/main.c | 25 +++--- kernel/module.c | 20 +++-- kernel/panic.c | 2 + kernel/params.c | 11 ++- lib/dynamic_debug.c | 4 +- 16 files changed, 234 insertions(+), 30 deletions(-) -- 2.1.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 1/7] taint: add TAINT_DEBUG for invasive debugging features 2014-10-03 21:44 [PATCH v2 0/7] driver-core: async probe support Luis R. Rodriguez @ 2014-10-03 21:44 ` Luis R. Rodriguez 2014-10-15 5:35 ` Rusty Russell 2014-10-03 21:44 ` [PATCH v2 2/7] module: add extra argument for parse_params() callback Luis R. Rodriguez ` (5 subsequent siblings) 6 siblings, 1 reply; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-03 21:44 UTC (permalink / raw) To: gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel, Rusty Russell From: "Luis R. Rodriguez" <mcgrof@suse.com> At times we may add module parameters or debugging / testing kernel features, when enabled though we don't really want to be spending time debugging them. Add a taint flag for this to avoid spurious bug reports. Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: Tejun Heo <tj@kernel.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com> --- Documentation/sysctl/kernel.txt | 1 + include/linux/kernel.h | 1 + kernel/module.c | 4 ++-- kernel/panic.c | 2 ++ 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt index f79eb96..1780a68 100644 --- a/Documentation/sysctl/kernel.txt +++ b/Documentation/sysctl/kernel.txt @@ -827,6 +827,7 @@ can be ORed together: 8192 - An unsigned module has been loaded in a kernel supporting module signature. 16384 - A soft lockup has previously occurred on the system. +32768 - A possibly disruptive testing / debug kernel feature has been enabled ============================================================== diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 95624be..8901809 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -471,6 +471,7 @@ extern enum system_states { #define TAINT_OOT_MODULE 12 #define TAINT_UNSIGNED_MODULE 13 #define TAINT_SOFTLOCKUP 14 +#define TAINT_DEBUG 15 extern const char hex_asc[]; #define hex_asc_lo(x) hex_asc[((x) & 0x0f)] diff --git a/kernel/module.c b/kernel/module.c index 03214bd2..3f7d02a 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1013,8 +1013,8 @@ static size_t module_flags_taint(struct module *mod, char *buf) buf[l++] = 'E'; /* * TAINT_FORCED_RMMOD: could be added. - * TAINT_CPU_OUT_OF_SPEC, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't - * apply to modules. + * TAINT_CPU_OUT_OF_SPEC, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE + * and TAINT_DEBUG don't apply to modules. */ return l; } diff --git a/kernel/panic.c b/kernel/panic.c index d09dc5c..8e5a77b 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -225,6 +225,7 @@ static const struct tnt tnts[] = { { TAINT_OOT_MODULE, 'O', ' ' }, { TAINT_UNSIGNED_MODULE, 'E', ' ' }, { TAINT_SOFTLOCKUP, 'L', ' ' }, + { TAINT_DEBUG, 'T', ' ' }, }; /** @@ -244,6 +245,7 @@ static const struct tnt tnts[] = { * 'I' - Working around severe firmware bug. * 'O' - Out-of-tree module has been loaded. * 'E' - Unsigned module has been loaded. + * 'T' - System has enabled an intrusive testing / debug kernel feature. * * The string is overwritten by the next call to print_tainted(). */ -- 2.1.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/7] taint: add TAINT_DEBUG for invasive debugging features 2014-10-03 21:44 ` [PATCH v2 1/7] taint: add TAINT_DEBUG for invasive debugging features Luis R. Rodriguez @ 2014-10-15 5:35 ` Rusty Russell 2014-10-20 20:37 ` Luis R. Rodriguez 0 siblings, 1 reply; 25+ messages in thread From: Rusty Russell @ 2014-10-15 5:35 UTC (permalink / raw) To: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel "Luis R. Rodriguez" <mcgrof@do-not-panic.com> writes: > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > At times we may add module parameters or debugging / testing > kernel features, when enabled though we don't really want > to be spending time debugging them. Add a taint flag for > this to avoid spurious bug reports. The recently introduced KERNEL_PARAM_FL_UNSAFE flag for module parameters does this explicitly. It uses TAINT_USER, should it use this instead? Thanks, Rusty. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/7] taint: add TAINT_DEBUG for invasive debugging features 2014-10-15 5:35 ` Rusty Russell @ 2014-10-20 20:37 ` Luis R. Rodriguez 0 siblings, 0 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-20 20:37 UTC (permalink / raw) To: Rusty Russell Cc: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, tj, arjan, teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, linux-kernel, Jani Nikula On Wed, Oct 15, 2014 at 04:05:06PM +1030, Rusty Russell wrote: > "Luis R. Rodriguez" <mcgrof@do-not-panic.com> writes: > > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > > > At times we may add module parameters or debugging / testing > > kernel features, when enabled though we don't really want > > to be spending time debugging them. Add a taint flag for > > this to avoid spurious bug reports. > > The recently introduced KERNEL_PARAM_FL_UNSAFE flag for module > parameters does this explicitly. It uses TAINT_USER, should it use > this instead? Absolutely, dropped my taint thing in favor of this, thanks. Luis ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 2/7] module: add extra argument for parse_params() callback 2014-10-03 21:44 [PATCH v2 0/7] driver-core: async probe support Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 1/7] taint: add TAINT_DEBUG for invasive debugging features Luis R. Rodriguez @ 2014-10-03 21:44 ` Luis R. Rodriguez 2014-10-04 12:55 ` [Cocci] " SF Markus Elfring 2014-10-15 5:35 ` Rusty Russell 2014-10-03 21:44 ` [PATCH v2 3/7] driver-core: enable drivers to opt-out of async probe Luis R. Rodriguez ` (4 subsequent siblings) 6 siblings, 2 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-03 21:44 UTC (permalink / raw) To: gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel, cocci, Rusty Russell, Christoph Hellwig, Felipe Contreras, Ewan Milne, Jean Delvare, Hannes Reinecke 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 3f7d02a..6a07671 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.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Cocci] [PATCH v2 2/7] module: add extra argument for parse_params() callback 2014-10-03 21:44 ` [PATCH v2 2/7] module: add extra argument for parse_params() callback Luis R. Rodriguez @ 2014-10-04 12:55 ` SF Markus Elfring 2014-10-06 20:38 ` Luis R. Rodriguez 2014-10-15 5:35 ` Rusty Russell 1 sibling, 1 reply; 25+ messages in thread From: SF Markus Elfring @ 2014-10-04 12:55 UTC (permalink / raw) To: Luis R. Rodriguez Cc: linux-kernel, Coccinelle, Arjan van de Ven, Benjamin Poirier, Christoph Hellwig, Davidlohr Bueso, Dmitry Torokhov, Ewan Milne, Felipe Contreras, Hannes Reinecke, Jean Delvare, Oleg Nesterov, Petr Mladek, Robert Milasan, Rusty Russell, Santosh Rastapur, Takashi Iwai, Tom Gundersen, Werner Fink > 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. How do you think about to work with more data type definitions for such callback functions? Regards, Markus ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Cocci] [PATCH v2 2/7] module: add extra argument for parse_params() callback 2014-10-04 12:55 ` [Cocci] " SF Markus Elfring @ 2014-10-06 20:38 ` Luis R. Rodriguez 2014-10-06 21:06 ` SF Markus Elfring 0 siblings, 1 reply; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-06 20:38 UTC (permalink / raw) To: SF Markus Elfring Cc: Luis R. Rodriguez, Tom Gundersen, Rusty Russell, Felipe Contreras, Davidlohr Bueso, Takashi Iwai, Dmitry Torokhov, Werner Fink, linux-kernel, Ewan Milne, Oleg Nesterov, Christoph Hellwig, Santosh Rastapur, Petr Mladek, Robert Milasan, Hannes Reinecke, Arjan van de Ven, Coccinelle, Jean Delvare, Benjamin Poirier On Sat, Oct 04, 2014 at 02:55:38PM +0200, SF Markus Elfring wrote: > > 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. > > How do you think about to work with more data type definitions for such callback > functions? Sorry I don't understand what you mean. Luis ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Cocci] [PATCH v2 2/7] module: add extra argument for parse_params() callback 2014-10-06 20:38 ` Luis R. Rodriguez @ 2014-10-06 21:06 ` SF Markus Elfring 2014-10-06 22:15 ` Luis R. Rodriguez 0 siblings, 1 reply; 25+ messages in thread From: SF Markus Elfring @ 2014-10-06 21:06 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Luis R. Rodriguez, Tom Gundersen, Rusty Russell, Felipe Contreras, Davidlohr Bueso, Takashi Iwai, Dmitry Torokhov, Werner Fink, linux-kernel, Ewan Milne, Oleg Nesterov, Christoph Hellwig, Santosh Rastapur, Petr Mladek, Robert Milasan, Hannes Reinecke, Arjan van de Ven, Coccinelle, Jean Delvare, Benjamin Poirier >> How do you think about to work with more data type definitions for such >> callback functions? > > Sorry I don't understand what you mean. Can a specific "typedef" help in corresponding software maintenance? Do you find descriptions from an other software application useful for such an use case here? http://www.fltk.org/doc-1.3/group__callback__functions.html Regards, Markus ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Cocci] [PATCH v2 2/7] module: add extra argument for parse_params() callback 2014-10-06 21:06 ` SF Markus Elfring @ 2014-10-06 22:15 ` Luis R. Rodriguez 0 siblings, 0 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-06 22:15 UTC (permalink / raw) To: SF Markus Elfring Cc: Luis R. Rodriguez, Tom Gundersen, Rusty Russell, Felipe Contreras, Davidlohr Bueso, Takashi Iwai, Dmitry Torokhov, Werner Fink, linux-kernel, Ewan Milne, Oleg Nesterov, Christoph Hellwig, Santosh Rastapur, Petr Mladek, Robert Milasan, Hannes Reinecke, Arjan van de Ven, Coccinelle, Jean Delvare, Benjamin Poirier On Mon, Oct 06, 2014 at 11:06:34PM +0200, SF Markus Elfring wrote: > >> How do you think about to work with more data type definitions for such > >> callback functions? > > > > Sorry I don't understand what you mean. > > Can a specific "typedef" help in corresponding software maintenance? > > Do you find descriptions from an other software application useful for such an > use case here? > http://www.fltk.org/doc-1.3/group__callback__functions.html Thanks I've reviewed this but I prefer to avoid typedefs. Luis ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/7] module: add extra argument for parse_params() callback 2014-10-03 21:44 ` [PATCH v2 2/7] module: add extra argument for parse_params() callback Luis R. Rodriguez 2014-10-04 12:55 ` [Cocci] " SF Markus Elfring @ 2014-10-15 5:35 ` Rusty Russell 1 sibling, 0 replies; 25+ messages in thread From: Rusty Russell @ 2014-10-15 5:35 UTC (permalink / raw) To: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel, cocci, Christoph Hellwig, Felipe Contreras, Ewan Milne, Jean Delvare, Hannes Reinecke "Luis R. Rodriguez" <mcgrof@do-not-panic.com> writes: > 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. Acked-by: Rusty Russell <rusty@rustcorp.com.au> Cheers, Rusty. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 3/7] driver-core: enable drivers to opt-out of async probe 2014-10-03 21:44 [PATCH v2 0/7] driver-core: async probe support Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 1/7] taint: add TAINT_DEBUG for invasive debugging features Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 2/7] module: add extra argument for parse_params() callback Luis R. Rodriguez @ 2014-10-03 21:44 ` Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 4/7] amd64_edac: enforce synchronous probe Luis R. Rodriguez ` (3 subsequent siblings) 6 siblings, 0 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-03 21:44 UTC (permalink / raw) To: gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel, Doug Thompson, Borislav Petkov, Mauro Carvalho Chehab, linux-edac 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 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/device.h b/include/linux/device.h index 43d183a..aa14b95 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -200,6 +200,8 @@ 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: Use this to annotate drivers which don't work well with + * async 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 +235,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.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 4/7] amd64_edac: enforce synchronous probe 2014-10-03 21:44 [PATCH v2 0/7] driver-core: async probe support Luis R. Rodriguez ` (2 preceding siblings ...) 2014-10-03 21:44 ` [PATCH v2 3/7] driver-core: enable drivers to opt-out of async probe Luis R. Rodriguez @ 2014-10-03 21:44 ` Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 5/7] driver-core: generalize freeing driver private member Luis R. Rodriguez ` (2 subsequent siblings) 6 siblings, 0 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-03 21:44 UTC (permalink / raw) To: gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel, Doug Thompson, Borislav Petkov, Mauro Carvalho Chehab, linux-edac 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. The reason async probe fails is that the init call for this driver relies on probe to have finished for at least one device. This needs to be addressed before enabling async probe. 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.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 5/7] driver-core: generalize freeing driver private member 2014-10-03 21:44 [PATCH v2 0/7] driver-core: async probe support Luis R. Rodriguez ` (3 preceding siblings ...) 2014-10-03 21:44 ` [PATCH v2 4/7] amd64_edac: enforce synchronous probe Luis R. Rodriguez @ 2014-10-03 21:44 ` Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 6/7] driver-core: add driver module asynchronous probe support Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules Luis R. Rodriguez 6 siblings, 0 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-03 21:44 UTC (permalink / raw) To: gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel 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.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 6/7] driver-core: add driver module asynchronous probe support 2014-10-03 21:44 [PATCH v2 0/7] driver-core: async probe support Luis R. Rodriguez ` (4 preceding siblings ...) 2014-10-03 21:44 ` [PATCH v2 5/7] driver-core: generalize freeing driver private member Luis R. Rodriguez @ 2014-10-03 21:44 ` Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules Luis R. Rodriguez 6 siblings, 0 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-03 21:44 UTC (permalink / raw) To: gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev 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 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 enabling 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 kernel parameter bus.__DEBUG__module_safe_mod_async_probe=1 which will work as if userspace would have requested all modules to load with async probe. Daring folks can also use bus.__DEBUG__module_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. Both of these flag taint the kernel as being in a debug intrusive mode to avoid spurious bug reports while this mechanism gets tested. 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 Acked-by: Tom Gundersen <teg@jklm.no> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com> --- Documentation/kernel-parameters.txt | 7 +++ drivers/base/base.h | 6 +++ drivers/base/bus.c | 104 ++++++++++++++++++++++++++++++++++-- drivers/base/dd.c | 7 +++ include/linux/module.h | 2 + kernel/module.c | 12 ++++- 6 files changed, 133 insertions(+), 5 deletions(-) diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 10d51c2..e4be3b8 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -914,6 +914,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted. Enable debug messages at boot time. See Documentation/dynamic-debug-howto.txt for details. + module.async_probe [KNL] + Enable asynchronous probe on this module. + bus.__DEBUG__module_force_mod_async_probe=1 [KNL] + Enable asynchronous probe on all modules. This is + testing parameter and using it will taint your + kernel. + early_ioremap_debug [KNL] Enable debug messages in early_ioremap support. This is useful for tracking down temporary early mappings 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..ec203d6 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,93 @@ 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) +{ + struct driver_attach_work *attach_work = + container_of(work, struct driver_attach_work, work); + struct device_driver *drv = attach_work->driver; + int ret; + + ret = driver_attach(drv); + if (ret != 0) { + remove_driver_private(drv); + bus_put(drv->bus); + } +} + +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 force_mod_async = false; +module_param_named(__DEBUG__module_force_mod_async_probe, force_mod_async, bool, 0400); +MODULE_PARM_DESC(__DEBUG__module_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 + * through a few mechanisms. We're careful to ensure that async probe can + * only be used by userspace that is prepared and has been vetted to support + * async probe support given that the driver core has historically only + * supported synchronous probe. If any driver is known to not work well with + * async probe the @drv can enable sync_probe and asynchronous probe will never + * be used on it. + * + * Drivers can be built-in or modules. Userspace can inform the kernel that + * it is prepared for async probe by passing the module parameter + * async_probe on each module it wishes to load. The async_probe parameter is + * module specific and gives userspace the flexibility to opt out of using + * async probe for certain types of modules. Built-in drivers are currently + * not supported for async probe. + * + * If you'd like to test enabling async probe for all modules you can enable + * the bus.__DEBUG__module_force_mod_async_probe=1 kernel parameter. This + * parameter should only be used to help test and should 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 (!mod->async_probe_requested) + return false; + + return true; +} + /** * bus_add_driver - Add a driver to the bus. * @drv: driver. @@ -675,6 +759,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 +781,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 +1360,11 @@ EXPORT_SYMBOL_GPL(subsys_virtual_register); int __init buses_init(void) { + if (unlikely(force_mod_async)) { + pr_info("Enabling force_mod_async -- you're on your own!\n"); + add_taint(TAINT_DEBUG, LOCKDEP_STILL_OK); + } + 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 6a07671..f5f709d 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) { + struct module *mod = arg; + int ret; + + 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.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-03 21:44 [PATCH v2 0/7] driver-core: async probe support Luis R. Rodriguez ` (5 preceding siblings ...) 2014-10-03 21:44 ` [PATCH v2 6/7] driver-core: add driver module asynchronous probe support Luis R. Rodriguez @ 2014-10-03 21:44 ` Luis R. Rodriguez 2014-10-06 20:19 ` Tejun Heo 2014-10-06 20:41 ` Dmitry Torokhov 6 siblings, 2 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-03 21:44 UTC (permalink / raw) To: gregkh, dmitry.torokhov, tiwai, tj, arjan Cc: teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev From: "Luis R. Rodriguez" <mcgrof@suse.com> At times we may wish to express the desire to prefer to have a device driver probe asynchronously by default. We cannot simply enable all device drivers to do this without vetting that userspace is prepared for this though given that some old userspace is expected to exist which is not equipped to deal with broad async probe support. This defines a new kernel parameter, bus.enable_kern_async=1, to help address this both to help enable async probe support for built-in drivers and to enable drivers to specify a preference to opt in for async probe support. If you have a device driver that should use async probe support when possible enable the prefer_async_probe bool. Folks wishing to test enabling async probe for all built-in drivers can enable bus.__DEBUG__kernel_force_mod_async_probe=1, if you use that though you are 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> --- Documentation/kernel-parameters.txt | 9 +++++++ drivers/base/bus.c | 52 ++++++++++++++++++++++++++++++------- include/linux/device.h | 9 +++++++ 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index e4be3b8..56f4d4e 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -914,12 +914,21 @@ bytes respectively. Such letter suffixes can also be entirely omitted. Enable debug messages at boot time. See Documentation/dynamic-debug-howto.txt for details. + bus.enable_kern_async=1 [KNL] + This tells the kernel userspace has been vetted for + asynchronous probe support and can listen to the device + driver prefer_async_probe flag for both built-in device + drivers and modules. module.async_probe [KNL] Enable asynchronous probe on this module. bus.__DEBUG__module_force_mod_async_probe=1 [KNL] Enable asynchronous probe on all modules. This is testing parameter and using it will taint your kernel. + bus.__DEBUG__kernel_force_mod_async_probe=1 [KNL] + Enable asynchronous probe on all built-in drivers. This + is testing parameter and using it will taint your + kernel. early_ioremap_debug [KNL] Enable debug messages in early_ioremap support. This diff --git a/drivers/base/bus.c b/drivers/base/bus.c index ec203d6..25d0412 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -695,8 +695,10 @@ int bus_driver_async_probe(struct device_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); + pr_info("bus: '%s': probe for %s driver %s is run asynchronously\n", + drv->bus->name, + drv->owner ? "module" : "built-in", + drv->name); queue_work(system_unbound_wq, &priv->attach_work->work); @@ -708,6 +710,16 @@ module_param_named(__DEBUG__module_force_mod_async_probe, force_mod_async, bool, MODULE_PARM_DESC(__DEBUG__module_force_mod_async_probe, "Force async probe on all modules"); +static bool force_kern_async = false; +module_param_named(__DEBUG__kernel_force_mod_async_probe, force_kern_async, bool, 0400); +MODULE_PARM_DESC(__DEBUG__kernel_force_mod_async_probe, + "Force async probe on all modules"); + +static bool enable_kern_async = false; +module_param_named(enable_kern_async, enable_kern_async, bool, 0400); +MODULE_PARM_DESC(enable_kern_async, + "Userspace is vetted to handle driver async probe"); + /** * drv_enable_async_probe - evaluates if async probe should be used * @drv: device driver to evaluate @@ -722,25 +734,41 @@ MODULE_PARM_DESC(__DEBUG__module_force_mod_async_probe, * be used on it. * * Drivers can be built-in or modules. Userspace can inform the kernel that - * it is prepared for async probe by passing the module parameter - * async_probe on each module it wishes to load. The async_probe parameter is + * it is prepared for async probe by either passing the module parameter + * async_probe on each module it wishes to load or by enabling the + * bus.enable_kern_async=1 kernel parameter. The async_probe parameter is * module specific and gives userspace the flexibility to opt out of using - * async probe for certain types of modules. Built-in drivers are currently - * not supported for async probe. + * async probe for certain types of modules. Built-in drivers and modules which + * are known to work well with async probe can enable @drv->prefer_async_probe, + * async probe will be used for it if the kernel parameter but only if the + * kernel parameter bus.enable_kern_async=1 has been set. * * If you'd like to test enabling async probe for all modules you can enable - * the bus.__DEBUG__module_force_mod_async_probe=1 kernel parameter. This - * parameter should only be used to help test and should be used with caution. + * the bus.__DEBUG__module_force_mod_async_probe=1 kernel parameter. If you'd + * like to test enabling async probe for all built-in drivers you can enable + * the bus.__DEBUG__kernel_force_mod_async_probe=1 kernel parameter. These + * parameters should only be used to help test and should 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) + if (drv->sync_probe) + return false; + + /* built-in drivers */ + if (!drv->owner) { + if (!enable_kern_async) + return false; + if (drv->prefer_async_probe || force_kern_async) + return true; return false; + } - if (force_mod_async) + /* modules */ + if ((enable_kern_async && drv->prefer_async_probe) || + force_mod_async) return true; mod = drv->owner; @@ -1364,6 +1392,10 @@ int __init buses_init(void) pr_info("Enabling force_mod_async -- you're on your own!\n"); add_taint(TAINT_DEBUG, LOCKDEP_STILL_OK); } + if (unlikely(force_kern_async)) { + pr_info("Enabling force_kern_async -- you're on your own!\n"); + add_taint(TAINT_DEBUG, LOCKDEP_STILL_OK); + } bus_kset = kset_create_and_add("bus", &bus_uevent_ops, NULL); if (!bus_kset) diff --git a/include/linux/device.h b/include/linux/device.h index aa14b95..058a8e0 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -202,6 +202,14 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus); * @suppress_bind_attrs: Disables bind/unbind via sysfs. * @sync_probe: Use this to annotate drivers which don't work well with * async probe. + * @prefer_async_probe: if userspace is vetted for async probe support enable + * async probe on this device driver whether module or built-in. + * Userspace expresses it is vetted for async probe support by + * enabling the bus.enable_kern_async=1 kernel parameter. Without + * this option enabled userspace can still request modules to be + * loaded asynchronously by using the shared async_probe module + * parameter. Built-in drivers must however enable + * prefer_async_probe and cope with bus.enable_kern_async=1 * @of_match_table: The open firmware table. * @acpi_match_table: The ACPI match table. * @probe: Called to query the existence of a specific device, @@ -236,6 +244,7 @@ struct device_driver { bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ bool sync_probe; + bool prefer_async_probe; const struct of_device_id *of_match_table; const struct acpi_device_id *acpi_match_table; -- 2.1.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-03 21:44 ` [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules Luis R. Rodriguez @ 2014-10-06 20:19 ` Tejun Heo 2014-10-06 20:36 ` Luis R. Rodriguez 2014-10-06 20:41 ` Dmitry Torokhov 1 sibling, 1 reply; 25+ messages in thread From: Tejun Heo @ 2014-10-06 20:19 UTC (permalink / raw) To: Luis R. Rodriguez Cc: gregkh, dmitry.torokhov, tiwai, arjan, teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev Hello, Luis. The patchset generally looks good to me. Please feel free to add Reviewed-by: Tejun Heo <tj@kernel.org> A question below. On Fri, Oct 03, 2014 at 02:44:43PM -0700, Luis R. Rodriguez wrote: > + bus.enable_kern_async=1 [KNL] > + This tells the kernel userspace has been vetted for > + asynchronous probe support and can listen to the device > + driver prefer_async_probe flag for both built-in device > + drivers and modules. Do we intend to keep this param permanently? Isn't this more of a temp tool to be used during development? If so, maybe we should make that clear with __DEVEL__ too? Thanks. -- tejun ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-06 20:19 ` Tejun Heo @ 2014-10-06 20:36 ` Luis R. Rodriguez 2014-10-06 21:01 ` Tejun Heo 0 siblings, 1 reply; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-06 20:36 UTC (permalink / raw) To: Tejun Heo Cc: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, arjan, teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev On Mon, Oct 06, 2014 at 04:19:26PM -0400, Tejun Heo wrote: > Hello, Luis. > > The patchset generally looks good to me. Please feel free to add > > Reviewed-by: Tejun Heo <tj@kernel.org> Will do. > A question below. > > On Fri, Oct 03, 2014 at 02:44:43PM -0700, Luis R. Rodriguez wrote: > > + bus.enable_kern_async=1 [KNL] > > + This tells the kernel userspace has been vetted for > > + asynchronous probe support and can listen to the device > > + driver prefer_async_probe flag for both built-in device > > + drivers and modules. > > Do we intend to keep this param permanently? Isn't this more of a > temp tool to be used during development? If so, maybe we should make > that clear with __DEVEL__ too? As its designed right now no, its not a temp tool, its there to require compatibility with old userspace. For modules we can require the module parameter but for built-in we need something else and this is what came to mind. It is also what would allow the prefer_async_probe flag too as otherwise we won't know if userspace is prepared. If we want to get rid of it, it would mean that we're letting go of the idea that some userspace might exist which depends on *not* doing async probe. As such I would not consider it a __DEVEL__ param and it'd be a judgement call to eventually *not require* it. I can see that happening but perhaps a large series of kernels down the road? Luis ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-06 20:36 ` Luis R. Rodriguez @ 2014-10-06 21:01 ` Tejun Heo 2014-10-06 23:10 ` Luis R. Rodriguez 0 siblings, 1 reply; 25+ messages in thread From: Tejun Heo @ 2014-10-06 21:01 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, arjan, teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev Hello, On Mon, Oct 06, 2014 at 10:36:27PM +0200, Luis R. Rodriguez wrote: > > Do we intend to keep this param permanently? Isn't this more of a > > temp tool to be used during development? If so, maybe we should make > > that clear with __DEVEL__ too? > > As its designed right now no, its not a temp tool, its there to > require compatibility with old userspace. For modules we can require > the module parameter but for built-in we need something else and this > is what came to mind. It is also what would allow the prefer_async_probe > flag too as otherwise we won't know if userspace is prepared. I don't get it. For in-kernel stuff, we already have a clear synchronization point where we already synchronize all async calls. Shouldn't we be flushing these async probes there too? insmod'ing is userland visible but there's no reason this has to be for the built-in drivers. Thanks. -- tejun ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-06 21:01 ` Tejun Heo @ 2014-10-06 23:10 ` Luis R. Rodriguez 2014-10-07 17:34 ` Tejun Heo 0 siblings, 1 reply; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-06 23:10 UTC (permalink / raw) To: Tejun Heo Cc: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, arjan, teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev On Mon, Oct 06, 2014 at 05:01:18PM -0400, Tejun Heo wrote: > Hello, > > On Mon, Oct 06, 2014 at 10:36:27PM +0200, Luis R. Rodriguez wrote: > > > Do we intend to keep this param permanently? Isn't this more of a > > > temp tool to be used during development? If so, maybe we should make > > > that clear with __DEVEL__ too? > > > > As its designed right now no, its not a temp tool, its there to > > require compatibility with old userspace. For modules we can require > > the module parameter but for built-in we need something else and this > > is what came to mind. It is also what would allow the prefer_async_probe > > flag too as otherwise we won't know if userspace is prepared. > > I don't get it. By prepared I meant that userspace can handle async probe, but you're right that we don't need to know that. I don't see how we'd be breaking old userspace by doing async probe of a driver is built-in right now... unless of course built-in always assumes all possible devices would be present after right before userspace init. > For in-kernel stuff, we already have a clear > synchronization point where we already synchronize all async calls. > Shouldn't we be flushing these async probes there too? This seems to be addressing if what I meant by prepared, "ready", so let me address this as I do think its important. By async calls do you mean users of async_schedule()? I see it also uses system_unbound_wq as well but I do not see anyone calling flush_workqueue(system_unbound_wq) on the kernel. We do use async_synchronize_full() on kernel_init() but that just waits. As it is we don't wait on init then, should we? Must we? Could / should we use bus.enable_kern_async=1 to enable avoiding having to wait ? At this point I'd prefer to address what we must do only. > insmod'ing is > userland visible but there's no reason this has to be for the built-in > drivers. Good point. bus.enable_kern_async=1 would still also serve as a helper for the driver core to figure out if it should use async probe then on modules if prefer_async_probe was enabled. Let me know if you figure out a way to avoid it. Luis ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-06 23:10 ` Luis R. Rodriguez @ 2014-10-07 17:34 ` Tejun Heo 2014-10-07 17:50 ` Luis R. Rodriguez 0 siblings, 1 reply; 25+ messages in thread From: Tejun Heo @ 2014-10-07 17:34 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, arjan, teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev Hello, On Tue, Oct 07, 2014 at 01:10:46AM +0200, Luis R. Rodriguez wrote: > On Mon, Oct 06, 2014 at 05:01:18PM -0400, Tejun Heo wrote: > > For in-kernel stuff, we already have a clear > > synchronization point where we already synchronize all async calls. > > Shouldn't we be flushing these async probes there too? > > This seems to be addressing if what I meant by prepared, "ready", so let > me address this as I do think its important. > > By async calls do you mean users of async_schedule()? I see it Yes. > also uses system_unbound_wq as well but I do not see anyone calling > flush_workqueue(system_unbound_wq) on the kernel. We do use > async_synchronize_full() on kernel_init() but that just waits. But you can create a new workqueue and queue all the async probing work items there and flush the workqueue right after async_synchronize_full(). ... > bus.enable_kern_async=1 would still also serve as a helper for the driver core > to figure out if it should use async probe then on modules if prefer_async_probe > was enabled. Let me know if you figure out a way to avoid it. Why do we need the choice at all? It always should, no? Thanks. -- tejun ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-07 17:34 ` Tejun Heo @ 2014-10-07 17:50 ` Luis R. Rodriguez 2014-10-07 17:55 ` Tejun Heo 0 siblings, 1 reply; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-07 17:50 UTC (permalink / raw) To: Tejun Heo Cc: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, arjan, teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev On Tue, Oct 07, 2014 at 01:34:04PM -0400, Tejun Heo wrote: > Hello, > > On Tue, Oct 07, 2014 at 01:10:46AM +0200, Luis R. Rodriguez wrote: > > On Mon, Oct 06, 2014 at 05:01:18PM -0400, Tejun Heo wrote: > > > For in-kernel stuff, we already have a clear > > > synchronization point where we already synchronize all async calls. > > > Shouldn't we be flushing these async probes there too? > > > > This seems to be addressing if what I meant by prepared, "ready", so let > > me address this as I do think its important. > > > > By async calls do you mean users of async_schedule()? I see it > > Yes. > > > also uses system_unbound_wq as well but I do not see anyone calling > > flush_workqueue(system_unbound_wq) on the kernel. We do use > > async_synchronize_full() on kernel_init() but that just waits. > > But you can create a new workqueue and queue all the async probing > work items there and flush the workqueue right after > async_synchronize_full(). On second thought I would prefer to avoid this, I see this being good to help with old userspace but other than that I don't see a requirement for new userspace. Do you? > ... > > bus.enable_kern_async=1 would still also serve as a helper for the driver core > > to figure out if it should use async probe then on modules if prefer_async_probe > > was enabled. Let me know if you figure out a way to avoid it. > > Why do we need the choice at all? It always should, no? I'm OK to live with that, in that case I see no point to bus.enable_kern_async=1 at all. Luis ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-07 17:50 ` Luis R. Rodriguez @ 2014-10-07 17:55 ` Tejun Heo 2014-10-07 18:55 ` Luis R. Rodriguez 0 siblings, 1 reply; 25+ messages in thread From: Tejun Heo @ 2014-10-07 17:55 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, arjan, teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev Hello, On Tue, Oct 07, 2014 at 07:50:10PM +0200, Luis R. Rodriguez wrote: > On Tue, Oct 07, 2014 at 01:34:04PM -0400, Tejun Heo wrote: > > But you can create a new workqueue and queue all the async probing > > work items there and flush the workqueue right after > > async_synchronize_full(). > > On second thought I would prefer to avoid this, I see this being good > to help with old userspace but other than that I don't see a requirement > for new userspace. Do you? Hmmm... we batch up and do everything parallel, so I'm not sure how much gain we'd be looking at by not waiting for at the end before jumping into the userland. Also, it's a bit of an orthogonal issue. If we wanna skip such synchornization point before passing control to userland, why are we applying that to this but not async_synchronize_full() which has a far larger impact? It's weird to synchronize one while not the other, so yeah, if there are actual benefits we can consider it but let's do it separately. Thanks. -- tejun ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-07 17:55 ` Tejun Heo @ 2014-10-07 18:55 ` Luis R. Rodriguez 2014-10-07 19:07 ` Luis R. Rodriguez 0 siblings, 1 reply; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-07 18:55 UTC (permalink / raw) To: Tejun Heo Cc: Greg Kroah-Hartman, Dmitry Torokhov, Takashi Iwai, Arjan van de Ven, Tom Gundersen, Robert Milasan, werner, Oleg Nesterov, hare, Benjamin Poirier, Santosh Rastapur, Petr Mladek, dbueso, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, mpt-fusionlinux.pdl, Linux SCSI List, netdev On Tue, Oct 7, 2014 at 10:55 AM, Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Tue, Oct 07, 2014 at 07:50:10PM +0200, Luis R. Rodriguez wrote: >> On Tue, Oct 07, 2014 at 01:34:04PM -0400, Tejun Heo wrote: >> > But you can create a new workqueue and queue all the async probing >> > work items there and flush the workqueue right after >> > async_synchronize_full(). >> >> On second thought I would prefer to avoid this, I see this being good >> to help with old userspace but other than that I don't see a requirement >> for new userspace. Do you? > > Hmmm... we batch up and do everything parallel, so I'm not sure how > much gain we'd be looking at by not waiting for at the end before > jumping into the userland. Also, it's a bit of an orthogonal issue. > If we wanna skip such synchornization point before passing control to > userland, why are we applying that to this but not > async_synchronize_full() which has a far larger impact? It's weird to > synchronize one while not the other, so yeah, if there are actual > benefits we can consider it but let's do it separately. OK I'll just kill bus.enable_kern_async=1 to enable built-in async probe support *and* also have prefer_async_probe *always* be respected, whether modular or not. Luis ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-07 18:55 ` Luis R. Rodriguez @ 2014-10-07 19:07 ` Luis R. Rodriguez 0 siblings, 0 replies; 25+ messages in thread From: Luis R. Rodriguez @ 2014-10-07 19:07 UTC (permalink / raw) To: Tejun Heo Cc: Greg Kroah-Hartman, Dmitry Torokhov, Takashi Iwai, Arjan van de Ven, Tom Gundersen, Robert Milasan, werner, Oleg Nesterov, hare, Benjamin Poirier, Santosh Rastapur, Petr Mladek, dbueso, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, mpt-fusionlinux.pdl, Linux SCSI List, netdev On Tue, Oct 7, 2014 at 11:55 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote: > OK I'll just kill bus.enable_kern_async=1 to enable built-in async > probe support *and* also have prefer_async_probe *always* be > respected, whether modular or not. Well and I just realized you *do* want to flush, so will throw that in too without an option to skip it. Luis ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules 2014-10-03 21:44 ` [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules Luis R. Rodriguez 2014-10-06 20:19 ` Tejun Heo @ 2014-10-06 20:41 ` Dmitry Torokhov 1 sibling, 0 replies; 25+ messages in thread From: Dmitry Torokhov @ 2014-10-06 20:41 UTC (permalink / raw) To: Luis R. Rodriguez Cc: gregkh, tiwai, tj, arjan, teg, rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso, mcgrof, linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers, One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton, Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy, Abhijit Mahajan, Casey Leedom, Hariprasad S, MPT-FusionLinux.pdl, linux-scsi, netdev Hi Luis, On Fri, Oct 03, 2014 at 02:44:43PM -0700, Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > At times we may wish to express the desire to prefer to have > a device driver probe asynchronously by default. We cannot > simply enable all device drivers to do this without vetting > that userspace is prepared for this though given that some > old userspace is expected to exist which is not equipped to > deal with broad async probe support. This defines a new kernel > parameter, bus.enable_kern_async=1, to help address this both to > help enable async probe support for built-in drivers and to > enable drivers to specify a preference to opt in for async > probe support. > > If you have a device driver that should use async probe > support when possible enable the prefer_async_probe bool. > > Folks wishing to test enabling async probe for all built-in > drivers can enable bus.__DEBUG__kernel_force_mod_async_probe=1, > if you use that though you are on your own. Thank you for working on this. However there are still couple of issues with the async probe. 1. As far as I can see you only handle the case when device is already present and you load a driver. In this case we will do either async or sync probing, depending on the driver/module settings. However if driver has already been loaded/registered and we are adding a new device (another module load, for example you load i2c controller module and it enumerates its children, or driver signalled deferral during binding) the probe will be synchronous regardless of the settings. 2. I thin kin the current implementation deferred binding process is still single-threaded and basically synchronous. Both of these issues stem form the fact that you only plugging into bus_add_driver(), but you also need to plug into bus_probe_device(). I believe I handled these 2 cases properly in the version of patch I sent a couple of weeks ago so if you could incorporate that in your work that would be great. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-10-20 20:37 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-10-03 21:44 [PATCH v2 0/7] driver-core: async probe support Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 1/7] taint: add TAINT_DEBUG for invasive debugging features Luis R. Rodriguez 2014-10-15 5:35 ` Rusty Russell 2014-10-20 20:37 ` Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 2/7] module: add extra argument for parse_params() callback Luis R. Rodriguez 2014-10-04 12:55 ` [Cocci] " SF Markus Elfring 2014-10-06 20:38 ` Luis R. Rodriguez 2014-10-06 21:06 ` SF Markus Elfring 2014-10-06 22:15 ` Luis R. Rodriguez 2014-10-15 5:35 ` Rusty Russell 2014-10-03 21:44 ` [PATCH v2 3/7] driver-core: enable drivers to opt-out of async probe Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 4/7] amd64_edac: enforce synchronous probe Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 5/7] driver-core: generalize freeing driver private member Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 6/7] driver-core: add driver module asynchronous probe support Luis R. Rodriguez 2014-10-03 21:44 ` [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules Luis R. Rodriguez 2014-10-06 20:19 ` Tejun Heo 2014-10-06 20:36 ` Luis R. Rodriguez 2014-10-06 21:01 ` Tejun Heo 2014-10-06 23:10 ` Luis R. Rodriguez 2014-10-07 17:34 ` Tejun Heo 2014-10-07 17:50 ` Luis R. Rodriguez 2014-10-07 17:55 ` Tejun Heo 2014-10-07 18:55 ` Luis R. Rodriguez 2014-10-07 19:07 ` Luis R. Rodriguez 2014-10-06 20:41 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).