Re: [PATCH] block2mtd lockdep_init_map warning
In message <[EMAIL PROTECTED]>, =?utf-8?B?SsO2cm4=?= Engel writes:
[...]
> Patch didn't compile due to function ordering. Here is an updated version.
Joern/Peter, I've tested this updated patch with v2.6.24-rc8-74-ga7da60f.
It worked fine for me.
Thanks,
Erez.
> Acked-and-tested-by: Joern Engel <[EMAIL PROTECTED]>
>
> diff --git a/kernel/module.c b/kernel/module.c
> index c2e3e2e..0397b1e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1645,6 +1645,17 @@ static inline void add_kallsyms(struct module *mod,
> }
> #endif /* CONFIG_KALLSYMS */
>
> +/*
> + * link the module with the whole machine is stopped with interrupts off
> + * - this defends against kallsyms not taking locks
> + */
> +static int __link_module(void *_mod)
> +{
> + struct module *mod = _mod;
> + list_add(>list, );
> + return 0;
> +}
> +
> /* Allocate and load the module: note that size of section 0 is always
> zero, and we rely on this for optional sections. */
> static struct module *load_module(void __user *umod,
> @@ -2023,6 +2034,11 @@ static struct module *load_module(void __user *umod,
> printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
> mod->name);
>
> + /* Now sew it into the lists so we can get lockdep and oops
> + * info during argument parsing. Noone should access us, since
> + * strong_try_module_get() will fail. */
> + stop_machine_run(__link_module, mod, NR_CPUS);
> +
> /* Size of section 0 is 0, so this works well if no params */
> err = parse_args(mod->name, mod->args,
>(struct kernel_param *)
> @@ -2031,7 +2047,7 @@ static struct module *load_module(void __user *umod,
>/ sizeof(struct kernel_param),
>NULL);
> if (err < 0)
> - goto arch_cleanup;
> + goto unlink;
>
> err = mod_sysfs_setup(mod,
> (struct kernel_param *)
> @@ -2039,7 +2055,7 @@ static struct module *load_module(void __user *umod,
> sechdrs[setupindex].sh_size
> / sizeof(struct kernel_param));
> if (err < 0)
> - goto arch_cleanup;
> + goto unlink;
> add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
> add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
>
> @@ -2054,7 +2070,8 @@ static struct module *load_module(void __user *umod,
> /* Done! */
> return mod;
>
> - arch_cleanup:
> + unlink:
> + stop_machine_run(__unlink_module, mod, NR_CPUS);
> module_arch_cleanup(mod);
> cleanup:
> module_unload_free(mod);
> @@ -2076,17 +2093,6 @@ static struct module *load_module(void __user *umod,
> goto free_hdr;
> }
>
> -/*
> - * link the module with the whole machine is stopped with interrupts off
> - * - this defends against kallsyms not taking locks
> - */
> -static int __link_module(void *_mod)
> -{
> - struct module *mod = _mod;
> - list_add(>list, );
> - return 0;
> -}
> -
> /* This is where the real work happens */
> asmlinkage long
> sys_init_module(void __user *umod,
> @@ -2111,10 +2117,6 @@ sys_init_module(void __user *umod,
> return PTR_ERR(mod);
> }
>
> - /* Now sew it into the lists. They won't access us, since
> - strong_try_module_get() will fail. */
> - stop_machine_run(__link_module, mod, NR_CPUS);
> -
> /* Drop lock so they can recurse */
> mutex_unlock(_mutex);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Tue, 8 January 2008 11:47:00 +1100, Rusty Russell wrote:
>
> There's nothing wrong with this patch, but I think it papers over a more
> general problem: we enter the module (to parse args) while it's not in the
> module list. This also means we won't get a nice oops if it crashes.
>
> This is untested, but does it solve it for you?
Not sure about lockdep, but it makes all the difference for an oops,
forced by explicit BUG() in block2mtd.
Before:
kernel BUG at drivers/mtd/devices/block2mtd.c:382!
invalid opcode: [#1]
Modules linked in:
CPU:0
EIP:0060:[]Not tainted VLI
EFLAGS: 0246 (2.6.22 #38)
EIP is at 0xe0818003
eax: dfe8c7da ebx: dfe8c7d0 ecx: e08185a8 edx: e08185a8
esi: e0818b1c edi: dfe8c7da ebp: df95de78 esp: df95de78
ds: 007b es: 007b fs: gs: 0033 ss: 0068
Process insmod (pid: 1076, ti=df95c000 task=df903080 task.ti=df95c000)
Stack: df95dec4 c0125576 df95de90 fff8 e0815932 0006
e08185a8 e0818b4c 0283 000b e0818b1c dfe8c7dc
0014 e0815b48 0014 df95dfb0 c01314e4 0001 df95df24
Call Trace:
[] show_trace_log_lvl+0x1a/0x2f
[] show_stack_log_lvl+0x9b/0xa3
[] show_registers+0x1be/0x290
[] die+0xdf/0x1a1
[] do_trap+0x89/0xa2
[] do_invalid_op+0x88/0x92
[] error_code+0x6a/0x70
[] parse_args+0x120/0x1f6
[] sys_init_module+0xf76/0x1297
[] syscall_call+0x7/0xb
===
Code: <0f> 0b eb fe 55 85 c0 89 e5 53 89 c3 74 31 8b 40 28 e8 ee 3e 93 df
EIP: [] 0xe0818003 SS:ESP 0068:df95de78
After:
kernel BUG at drivers/mtd/devices/block2mtd.c:382!
invalid opcode: [#1]
Modules linked in: block2mtd
CPU:0
EIP:0060:[]Not tainted VLI
EFLAGS: 0246 (2.6.22 #39)
EIP is at block2mtd_setup+0x3/0x7 [block2mtd]
eax: f7da387a ebx: f7da3870 ecx: f88205a8 edx: f88205a8
esi: f8820b1c edi: f7da387a ebp: f788be78 esp: f788be78
ds: 007b es: 007b fs: gs: 0033 ss: 0068
Process insmod (pid: 1072, ti=f788a000 task=c1b22b00 task.ti=f788a000)
Stack: f788bec4 c0125576 f788be90 fff8 f881d932 0006
f88205a8 f8820b4c 0283 000b f8820b1c f7da387c
0014 f881db48 0014 f788bfb0 c01314f1 0001 f788bf24
Call Trace:
[] show_trace_log_lvl+0x1a/0x2f
[] show_stack_log_lvl+0x9b/0xa3
[] show_registers+0x1be/0x290
[] die+0xdf/0x1a1
[] do_trap+0x89/0xa2
[] do_invalid_op+0x88/0x92
[] error_code+0x6a/0x70
[] parse_args+0x120/0x1f6
[] sys_init_module+0xf83/0x12a4
[] syscall_call+0x7/0xb
===
Code: <0f> 0b eb fe 55 85 c0 89 e5 53 89 c3 74 31 8b 40 28 e8 fa be 92 c7
EIP: [] block2mtd_setup+0x3/0x7 [block2mtd] SS:ESP 0068:f788be78
Patch didn't compile due to function ordering. Here is an updated version.
Acked-and-tested-by: Joern Engel <[EMAIL PROTECTED]>
diff --git a/kernel/module.c b/kernel/module.c
index c2e3e2e..0397b1e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1645,6 +1645,17 @@ static inline void add_kallsyms(struct module *mod,
}
#endif /* CONFIG_KALLSYMS */
+/*
+ * link the module with the whole machine is stopped with interrupts off
+ * - this defends against kallsyms not taking locks
+ */
+static int __link_module(void *_mod)
+{
+ struct module *mod = _mod;
+ list_add(>list, );
+ return 0;
+}
+
/* Allocate and load the module: note that size of section 0 is always
zero, and we rely on this for optional sections. */
static struct module *load_module(void __user *umod,
@@ -2023,6 +2034,11 @@ static struct module *load_module(void __user *umod,
printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
mod->name);
+ /* Now sew it into the lists so we can get lockdep and oops
+ * info during argument parsing. Noone should access us, since
+ * strong_try_module_get() will fail. */
+ stop_machine_run(__link_module, mod, NR_CPUS);
+
/* Size of section 0 is 0, so this works well if no params */
err = parse_args(mod->name, mod->args,
(struct kernel_param *)
@@ -2031,7 +2047,7 @@ static struct module *load_module(void __user *umod,
/ sizeof(struct kernel_param),
NULL);
if (err < 0)
- goto arch_cleanup;
+ goto unlink;
err = mod_sysfs_setup(mod,
(struct kernel_param *)
@@ -2039,7 +2055,7 @@ static struct module *load_module(void __user *umod,
sechdrs[setupindex].sh_size
/ sizeof(struct kernel_param));
if (err < 0)
- goto arch_cleanup;
+ goto unlink;
add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
@@ -2054,7 +2070,8 @@ static struct module *load_module(void __user *umod,
/*
Re: [PATCH] block2mtd lockdep_init_map warning
On Tue, 2008-01-08 at 11:47 +1100, Rusty Russell wrote: > On Monday 07 January 2008 21:05:26 Peter Zijlstra wrote: > > On Sun, 2008-01-06 at 14:11 -0500, Erez Zadok wrote: > > > > Ingo, Peter, does either of you actually care about this problem? In > > > > the last round when I debugged this problem there was a notable lack of > > > > reaction from either of you. > > > > > > The problem appears to be an interaction of two components--module > > > loading and lockdep--that's perhaps why it wasn't given enough attention. > > > > Would something like this work for people? > > Hi Peter, > > There's nothing wrong with this patch, but I think it papers over a more > general problem: we enter the module (to parse args) while it's not in the > module list. This also means we won't get a nice oops if it crashes. > > This is untested, but does it solve it for you? I think it should, Erez care to give it a spin? > diff -r 68fd1b22db89 kernel/module.c > --- a/kernel/module.c Mon Jan 07 18:59:50 2008 +1100 > +++ b/kernel/module.c Tue Jan 08 11:46:11 2008 +1100 > @@ -2043,6 +2043,11 @@ static struct module *load_module(void _ > printk(KERN_WARNING "%s: Ignoring obsolete parameters\n", > mod->name); > > + /* Now sew it into the lists so we can get lockdep and oops > + * info during argument parsing. Noone should access us, since > + * strong_try_module_get() will fail. */ > + stop_machine_run(__link_module, mod, NR_CPUS); > + > /* Size of section 0 is 0, so this works well if no params */ > err = parse_args(mod->name, mod->args, >(struct kernel_param *) > @@ -2051,7 +2056,7 @@ static struct module *load_module(void _ >/ sizeof(struct kernel_param), >NULL); > if (err < 0) > - goto arch_cleanup; > + goto unlink; > > err = mod_sysfs_setup(mod, > (struct kernel_param *) > @@ -2059,7 +2064,7 @@ static struct module *load_module(void _ > sechdrs[setupindex].sh_size > / sizeof(struct kernel_param)); > if (err < 0) > - goto arch_cleanup; > + goto unlink; > add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs); > add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs); > > @@ -2074,7 +2079,8 @@ static struct module *load_module(void _ > /* Done! */ > return mod; > > - arch_cleanup: > + unlink: > + stop_machine_run(__unlink_module, mod, NR_CPUS); > module_arch_cleanup(mod); > cleanup: > module_unload_free(mod); > @@ -2130,10 +2136,6 @@ sys_init_module(void __user *umod, > mutex_unlock(_mutex); > return PTR_ERR(mod); > } > - > - /* Now sew it into the lists. They won't access us, since > - strong_try_module_get() will fail. */ > - stop_machine_run(__link_module, mod, NR_CPUS); > > /* Drop lock so they can recurse */ > mutex_unlock(_mutex); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Tue, 2008-01-08 at 11:47 +1100, Rusty Russell wrote: On Monday 07 January 2008 21:05:26 Peter Zijlstra wrote: On Sun, 2008-01-06 at 14:11 -0500, Erez Zadok wrote: Ingo, Peter, does either of you actually care about this problem? In the last round when I debugged this problem there was a notable lack of reaction from either of you. The problem appears to be an interaction of two components--module loading and lockdep--that's perhaps why it wasn't given enough attention. Would something like this work for people? Hi Peter, There's nothing wrong with this patch, but I think it papers over a more general problem: we enter the module (to parse args) while it's not in the module list. This also means we won't get a nice oops if it crashes. This is untested, but does it solve it for you? I think it should, Erez care to give it a spin? diff -r 68fd1b22db89 kernel/module.c --- a/kernel/module.c Mon Jan 07 18:59:50 2008 +1100 +++ b/kernel/module.c Tue Jan 08 11:46:11 2008 +1100 @@ -2043,6 +2043,11 @@ static struct module *load_module(void _ printk(KERN_WARNING %s: Ignoring obsolete parameters\n, mod-name); + /* Now sew it into the lists so we can get lockdep and oops + * info during argument parsing. Noone should access us, since + * strong_try_module_get() will fail. */ + stop_machine_run(__link_module, mod, NR_CPUS); + /* Size of section 0 is 0, so this works well if no params */ err = parse_args(mod-name, mod-args, (struct kernel_param *) @@ -2051,7 +2056,7 @@ static struct module *load_module(void _ / sizeof(struct kernel_param), NULL); if (err 0) - goto arch_cleanup; + goto unlink; err = mod_sysfs_setup(mod, (struct kernel_param *) @@ -2059,7 +2064,7 @@ static struct module *load_module(void _ sechdrs[setupindex].sh_size / sizeof(struct kernel_param)); if (err 0) - goto arch_cleanup; + goto unlink; add_sect_attrs(mod, hdr-e_shnum, secstrings, sechdrs); add_notes_attrs(mod, hdr-e_shnum, secstrings, sechdrs); @@ -2074,7 +2079,8 @@ static struct module *load_module(void _ /* Done! */ return mod; - arch_cleanup: + unlink: + stop_machine_run(__unlink_module, mod, NR_CPUS); module_arch_cleanup(mod); cleanup: module_unload_free(mod); @@ -2130,10 +2136,6 @@ sys_init_module(void __user *umod, mutex_unlock(module_mutex); return PTR_ERR(mod); } - - /* Now sew it into the lists. They won't access us, since - strong_try_module_get() will fail. */ - stop_machine_run(__link_module, mod, NR_CPUS); /* Drop lock so they can recurse */ mutex_unlock(module_mutex); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Tue, 8 January 2008 11:47:00 +1100, Rusty Russell wrote:
There's nothing wrong with this patch, but I think it papers over a more
general problem: we enter the module (to parse args) while it's not in the
module list. This also means we won't get a nice oops if it crashes.
This is untested, but does it solve it for you?
Not sure about lockdep, but it makes all the difference for an oops,
forced by explicit BUG() in block2mtd.
Before:
kernel BUG at drivers/mtd/devices/block2mtd.c:382!
invalid opcode: [#1]
Modules linked in:
CPU:0
EIP:0060:[e0818003]Not tainted VLI
EFLAGS: 0246 (2.6.22 #38)
EIP is at 0xe0818003
eax: dfe8c7da ebx: dfe8c7d0 ecx: e08185a8 edx: e08185a8
esi: e0818b1c edi: dfe8c7da ebp: df95de78 esp: df95de78
ds: 007b es: 007b fs: gs: 0033 ss: 0068
Process insmod (pid: 1076, ti=df95c000 task=df903080 task.ti=df95c000)
Stack: df95dec4 c0125576 df95de90 fff8 e0815932 0006
e08185a8 e0818b4c 0283 000b e0818b1c dfe8c7dc
0014 e0815b48 0014 df95dfb0 c01314e4 0001 df95df24
Call Trace:
[c0103248] show_trace_log_lvl+0x1a/0x2f
[c01032f8] show_stack_log_lvl+0x9b/0xa3
[c01034be] show_registers+0x1be/0x290
[c010366f] die+0xdf/0x1a1
[c01037ba] do_trap+0x89/0xa2
[c0103b25] do_invalid_op+0x88/0x92
[c0384e82] error_code+0x6a/0x70
[c0125576] parse_args+0x120/0x1f6
[c01314e4] sys_init_module+0xf76/0x1297
[c01023ee] syscall_call+0x7/0xb
===
Code: 0f 0b eb fe 55 85 c0 89 e5 53 89 c3 74 31 8b 40 28 e8 ee 3e 93 df
EIP: [e0818003] 0xe0818003 SS:ESP 0068:df95de78
After:
kernel BUG at drivers/mtd/devices/block2mtd.c:382!
invalid opcode: [#1]
Modules linked in: block2mtd
CPU:0
EIP:0060:[f8820003]Not tainted VLI
EFLAGS: 0246 (2.6.22 #39)
EIP is at block2mtd_setup+0x3/0x7 [block2mtd]
eax: f7da387a ebx: f7da3870 ecx: f88205a8 edx: f88205a8
esi: f8820b1c edi: f7da387a ebp: f788be78 esp: f788be78
ds: 007b es: 007b fs: gs: 0033 ss: 0068
Process insmod (pid: 1072, ti=f788a000 task=c1b22b00 task.ti=f788a000)
Stack: f788bec4 c0125576 f788be90 fff8 f881d932 0006
f88205a8 f8820b4c 0283 000b f8820b1c f7da387c
0014 f881db48 0014 f788bfb0 c01314f1 0001 f788bf24
Call Trace:
[c0103248] show_trace_log_lvl+0x1a/0x2f
[c01032f8] show_stack_log_lvl+0x9b/0xa3
[c01034be] show_registers+0x1be/0x290
[c010366f] die+0xdf/0x1a1
[c01037ba] do_trap+0x89/0xa2
[c0103b25] do_invalid_op+0x88/0x92
[c0384e92] error_code+0x6a/0x70
[c0125576] parse_args+0x120/0x1f6
[c01314f1] sys_init_module+0xf83/0x12a4
[c01023ee] syscall_call+0x7/0xb
===
Code: 0f 0b eb fe 55 85 c0 89 e5 53 89 c3 74 31 8b 40 28 e8 fa be 92 c7
EIP: [f8820003] block2mtd_setup+0x3/0x7 [block2mtd] SS:ESP 0068:f788be78
Patch didn't compile due to function ordering. Here is an updated version.
Acked-and-tested-by: Joern Engel [EMAIL PROTECTED]
diff --git a/kernel/module.c b/kernel/module.c
index c2e3e2e..0397b1e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1645,6 +1645,17 @@ static inline void add_kallsyms(struct module *mod,
}
#endif /* CONFIG_KALLSYMS */
+/*
+ * link the module with the whole machine is stopped with interrupts off
+ * - this defends against kallsyms not taking locks
+ */
+static int __link_module(void *_mod)
+{
+ struct module *mod = _mod;
+ list_add(mod-list, modules);
+ return 0;
+}
+
/* Allocate and load the module: note that size of section 0 is always
zero, and we rely on this for optional sections. */
static struct module *load_module(void __user *umod,
@@ -2023,6 +2034,11 @@ static struct module *load_module(void __user *umod,
printk(KERN_WARNING %s: Ignoring obsolete parameters\n,
mod-name);
+ /* Now sew it into the lists so we can get lockdep and oops
+ * info during argument parsing. Noone should access us, since
+ * strong_try_module_get() will fail. */
+ stop_machine_run(__link_module, mod, NR_CPUS);
+
/* Size of section 0 is 0, so this works well if no params */
err = parse_args(mod-name, mod-args,
(struct kernel_param *)
@@ -2031,7 +2047,7 @@ static struct module *load_module(void __user *umod,
/ sizeof(struct kernel_param),
NULL);
if (err 0)
- goto arch_cleanup;
+ goto unlink;
err = mod_sysfs_setup(mod,
(struct kernel_param *)
@@ -2039,7 +2055,7 @@ static struct module *load_module(void __user *umod,
sechdrs[setupindex].sh_size
/ sizeof(struct kernel_param));
if (err 0)
- goto arch_cleanup;
+ goto unlink;
add_sect_attrs(mod,
Re: [PATCH] block2mtd lockdep_init_map warning
On Monday 07 January 2008 21:05:26 Peter Zijlstra wrote: > On Sun, 2008-01-06 at 14:11 -0500, Erez Zadok wrote: > > > Ingo, Peter, does either of you actually care about this problem? In > > > the last round when I debugged this problem there was a notable lack of > > > reaction from either of you. > > > > The problem appears to be an interaction of two components--module > > loading and lockdep--that's perhaps why it wasn't given enough attention. > > Would something like this work for people? Hi Peter, There's nothing wrong with this patch, but I think it papers over a more general problem: we enter the module (to parse args) while it's not in the module list. This also means we won't get a nice oops if it crashes. This is untested, but does it solve it for you? Thanks, Rusty. diff -r 68fd1b22db89 kernel/module.c --- a/kernel/module.c Mon Jan 07 18:59:50 2008 +1100 +++ b/kernel/module.c Tue Jan 08 11:46:11 2008 +1100 @@ -2043,6 +2043,11 @@ static struct module *load_module(void _ printk(KERN_WARNING "%s: Ignoring obsolete parameters\n", mod->name); + /* Now sew it into the lists so we can get lockdep and oops + * info during argument parsing. Noone should access us, since + * strong_try_module_get() will fail. */ + stop_machine_run(__link_module, mod, NR_CPUS); + /* Size of section 0 is 0, so this works well if no params */ err = parse_args(mod->name, mod->args, (struct kernel_param *) @@ -2051,7 +2056,7 @@ static struct module *load_module(void _ / sizeof(struct kernel_param), NULL); if (err < 0) - goto arch_cleanup; + goto unlink; err = mod_sysfs_setup(mod, (struct kernel_param *) @@ -2059,7 +2064,7 @@ static struct module *load_module(void _ sechdrs[setupindex].sh_size / sizeof(struct kernel_param)); if (err < 0) - goto arch_cleanup; + goto unlink; add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs); add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs); @@ -2074,7 +2079,8 @@ static struct module *load_module(void _ /* Done! */ return mod; - arch_cleanup: + unlink: + stop_machine_run(__unlink_module, mod, NR_CPUS); module_arch_cleanup(mod); cleanup: module_unload_free(mod); @@ -2130,10 +2136,6 @@ sys_init_module(void __user *umod, mutex_unlock(_mutex); return PTR_ERR(mod); } - - /* Now sew it into the lists. They won't access us, since - strong_try_module_get() will fail. */ - stop_machine_run(__link_module, mod, NR_CPUS); /* Drop lock so they can recurse */ mutex_unlock(_mutex); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Mon, 2008-01-07 at 11:20 +0100, Jörn Engel wrote: > On Mon, 7 January 2008 11:05:26 +0100, Peter Zijlstra wrote: > > > > Would something like this work for people? > > Looks a lot better than what I thought of. However, does the #ifdef > within is_module_address() make sense when afaict lockdep is the only > caller of that function? Looks as if the whole function should be made > conditional or none of it. Ah, I hadn't bothered to check if there were more users. /me does a (not so quick) git grep and finds lockdep is indeed the only caller. Sure, we can move the whole function into the ifdef. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Mon, 7 January 2008 11:05:26 +0100, Peter Zijlstra wrote:
>
> Would something like this work for people?
Looks a lot better than what I thought of. However, does the #ifdef
within is_module_address() make sense when afaict lockdep is the only
caller of that function? Looks as if the whole function should be made
conditional or none of it.
> Not-Yet-Signed-off-by: Peter Zijlstra <[EMAIL PROTECTED]>
> ---
> Index: linux-2.6/include/linux/sched.h
> ===
> --- linux-2.6.orig/include/linux/sched.h
> +++ linux-2.6/include/linux/sched.h
> @@ -1160,6 +1160,7 @@ struct task_struct {
> int lockdep_depth;
> struct held_lock held_locks[MAX_LOCK_DEPTH];
> unsigned int lockdep_recursion;
> + struct module *loading_module;
> #endif
>
> /* journalling filesystem info */
> Index: linux-2.6/kernel/module.c
> ===
> --- linux-2.6.orig/kernel/module.c
> +++ linux-2.6/kernel/module.c
> @@ -2023,6 +2023,9 @@ static struct module *load_module(void _
> printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
> mod->name);
>
> +#ifdef CONFIG_LOCKDEP
> + current->loading_module = mod;
> +#endif
> /* Size of section 0 is 0, so this works well if no params */
> err = parse_args(mod->name, mod->args,
>(struct kernel_param *)
> @@ -2030,6 +2033,9 @@ static struct module *load_module(void _
>sechdrs[setupindex].sh_size
>/ sizeof(struct kernel_param),
>NULL);
> +#ifdef CONFIG_LOCKDEP
> + current->loading_module = NULL
> +#endif
> if (err < 0)
> goto arch_cleanup;
>
> @@ -2454,6 +2460,17 @@ int is_module_address(unsigned long addr
> }
> }
>
> +#ifdef CONFIG_LOCKDEP
> + if (current->loading_module) {
> + mod = current->loading_module;
> + if (within(addr, mod->module_init, mod->init_text_size)
> + || within(addr, mod->module_core, mod->core_text_size)) {
> + preempt_enable();
> + return 1;
> + }
> + }
> +#endif
> +
> preempt_enable();
>
> return 0;
>
>
>
Jörn
--
I don't understand it. Nobody does.
-- Richard P. Feynman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Sun, 2008-01-06 at 14:11 -0500, Erez Zadok wrote:
> > Ingo, Peter, does either of you actually care about this problem? In
> > the last round when I debugged this problem there was a notable lack of
> > reaction from either of you.
>
> The problem appears to be an interaction of two components--module loading
> and lockdep--that's perhaps why it wasn't given enough attention.
Would something like this work for people?
Not-Yet-Signed-off-by: Peter Zijlstra <[EMAIL PROTECTED]>
---
Index: linux-2.6/include/linux/sched.h
===
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -1160,6 +1160,7 @@ struct task_struct {
int lockdep_depth;
struct held_lock held_locks[MAX_LOCK_DEPTH];
unsigned int lockdep_recursion;
+ struct module *loading_module;
#endif
/* journalling filesystem info */
Index: linux-2.6/kernel/module.c
===
--- linux-2.6.orig/kernel/module.c
+++ linux-2.6/kernel/module.c
@@ -2023,6 +2023,9 @@ static struct module *load_module(void _
printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
mod->name);
+#ifdef CONFIG_LOCKDEP
+ current->loading_module = mod;
+#endif
/* Size of section 0 is 0, so this works well if no params */
err = parse_args(mod->name, mod->args,
(struct kernel_param *)
@@ -2030,6 +2033,9 @@ static struct module *load_module(void _
sechdrs[setupindex].sh_size
/ sizeof(struct kernel_param),
NULL);
+#ifdef CONFIG_LOCKDEP
+ current->loading_module = NULL
+#endif
if (err < 0)
goto arch_cleanup;
@@ -2454,6 +2460,17 @@ int is_module_address(unsigned long addr
}
}
+#ifdef CONFIG_LOCKDEP
+ if (current->loading_module) {
+ mod = current->loading_module;
+ if (within(addr, mod->module_init, mod->init_text_size)
+ || within(addr, mod->module_core, mod->core_text_size)) {
+ preempt_enable();
+ return 1;
+ }
+ }
+#endif
+
preempt_enable();
return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Mon, 7 January 2008 11:05:26 +0100, Peter Zijlstra wrote:
Would something like this work for people?
Looks a lot better than what I thought of. However, does the #ifdef
within is_module_address() make sense when afaict lockdep is the only
caller of that function? Looks as if the whole function should be made
conditional or none of it.
Not-Yet-Signed-off-by: Peter Zijlstra [EMAIL PROTECTED]
---
Index: linux-2.6/include/linux/sched.h
===
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -1160,6 +1160,7 @@ struct task_struct {
int lockdep_depth;
struct held_lock held_locks[MAX_LOCK_DEPTH];
unsigned int lockdep_recursion;
+ struct module *loading_module;
#endif
/* journalling filesystem info */
Index: linux-2.6/kernel/module.c
===
--- linux-2.6.orig/kernel/module.c
+++ linux-2.6/kernel/module.c
@@ -2023,6 +2023,9 @@ static struct module *load_module(void _
printk(KERN_WARNING %s: Ignoring obsolete parameters\n,
mod-name);
+#ifdef CONFIG_LOCKDEP
+ current-loading_module = mod;
+#endif
/* Size of section 0 is 0, so this works well if no params */
err = parse_args(mod-name, mod-args,
(struct kernel_param *)
@@ -2030,6 +2033,9 @@ static struct module *load_module(void _
sechdrs[setupindex].sh_size
/ sizeof(struct kernel_param),
NULL);
+#ifdef CONFIG_LOCKDEP
+ current-loading_module = NULL
+#endif
if (err 0)
goto arch_cleanup;
@@ -2454,6 +2460,17 @@ int is_module_address(unsigned long addr
}
}
+#ifdef CONFIG_LOCKDEP
+ if (current-loading_module) {
+ mod = current-loading_module;
+ if (within(addr, mod-module_init, mod-init_text_size)
+ || within(addr, mod-module_core, mod-core_text_size)) {
+ preempt_enable();
+ return 1;
+ }
+ }
+#endif
+
preempt_enable();
return 0;
Jörn
--
I don't understand it. Nobody does.
-- Richard P. Feynman
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Mon, 2008-01-07 at 11:20 +0100, Jörn Engel wrote: On Mon, 7 January 2008 11:05:26 +0100, Peter Zijlstra wrote: Would something like this work for people? Looks a lot better than what I thought of. However, does the #ifdef within is_module_address() make sense when afaict lockdep is the only caller of that function? Looks as if the whole function should be made conditional or none of it. Ah, I hadn't bothered to check if there were more users. /me does a (not so quick) git grep and finds lockdep is indeed the only caller. Sure, we can move the whole function into the ifdef. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Monday 07 January 2008 21:05:26 Peter Zijlstra wrote: On Sun, 2008-01-06 at 14:11 -0500, Erez Zadok wrote: Ingo, Peter, does either of you actually care about this problem? In the last round when I debugged this problem there was a notable lack of reaction from either of you. The problem appears to be an interaction of two components--module loading and lockdep--that's perhaps why it wasn't given enough attention. Would something like this work for people? Hi Peter, There's nothing wrong with this patch, but I think it papers over a more general problem: we enter the module (to parse args) while it's not in the module list. This also means we won't get a nice oops if it crashes. This is untested, but does it solve it for you? Thanks, Rusty. diff -r 68fd1b22db89 kernel/module.c --- a/kernel/module.c Mon Jan 07 18:59:50 2008 +1100 +++ b/kernel/module.c Tue Jan 08 11:46:11 2008 +1100 @@ -2043,6 +2043,11 @@ static struct module *load_module(void _ printk(KERN_WARNING %s: Ignoring obsolete parameters\n, mod-name); + /* Now sew it into the lists so we can get lockdep and oops + * info during argument parsing. Noone should access us, since + * strong_try_module_get() will fail. */ + stop_machine_run(__link_module, mod, NR_CPUS); + /* Size of section 0 is 0, so this works well if no params */ err = parse_args(mod-name, mod-args, (struct kernel_param *) @@ -2051,7 +2056,7 @@ static struct module *load_module(void _ / sizeof(struct kernel_param), NULL); if (err 0) - goto arch_cleanup; + goto unlink; err = mod_sysfs_setup(mod, (struct kernel_param *) @@ -2059,7 +2064,7 @@ static struct module *load_module(void _ sechdrs[setupindex].sh_size / sizeof(struct kernel_param)); if (err 0) - goto arch_cleanup; + goto unlink; add_sect_attrs(mod, hdr-e_shnum, secstrings, sechdrs); add_notes_attrs(mod, hdr-e_shnum, secstrings, sechdrs); @@ -2074,7 +2079,8 @@ static struct module *load_module(void _ /* Done! */ return mod; - arch_cleanup: + unlink: + stop_machine_run(__unlink_module, mod, NR_CPUS); module_arch_cleanup(mod); cleanup: module_unload_free(mod); @@ -2130,10 +2136,6 @@ sys_init_module(void __user *umod, mutex_unlock(module_mutex); return PTR_ERR(mod); } - - /* Now sew it into the lists. They won't access us, since - strong_try_module_get() will fail. */ - stop_machine_run(__link_module, mod, NR_CPUS); /* Drop lock so they can recurse */ mutex_unlock(module_mutex); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Sun, 6 January 2008 14:11:47 -0500, Erez Zadok wrote: > > The problem appears to be an interaction of two components--module loading > and lockdep--that's perhaps why it wasn't given enough attention. Correct. For modules lockdep depends on initializations done after module_init has finished. However block2mtd is an odd sod that can call into lockdep code during module_init, causing the bug you noticed. Several solutions are possible. Modules could get two initcalls, one to decide whether module load should get aborted, the other run later, after the remaining module initializations are done. Or the module loader could always do the initializations and revoke them later, if module_init failed. But I personally am too unfamiliar with the module code to trust my judgement and have yet to receive feedback. Even you seem to ignore my mails and not even Cc: me later on. I must have done something really horrible in my last life, it seems. Jörn -- A quarrel is quickly settled when deserted by one party; there is no battle unless there be two. -- Seneca -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
In message <[EMAIL PROTECTED]>, =?utf-8?B?SsO2cm4=?= Engel writes: > Maybe it is not obvious that I maintain this driver and would like to be > kept on Cc:. Will send a patch to fix that shortly. I was looking in MAINTAINERS, and you weren't listed on the main MTD section as a maintainer. Perhaps an update? If you get a patch, I'd love to test it. > On Sun, 6 January 2008 02:17:32 -0500, Erez Zadok wrote: [...] > > In lieu of a better fix, is this patch acceptable? > > Not for me. I don't mind if you keep such a hack until a proper > solution if found in your private tree. But it is a horrible solution > to a problem introduced elsewhere. Yup, that's what I figured. I'll keep this ugly hack but I won't push it to my unionfs tree on kernel.org. > Ingo, Peter, does either of you actually care about this problem? In > the last round when I debugged this problem there was a notable lack of > reaction from either of you. The problem appears to be an interaction of two components--module loading and lockdep--that's perhaps why it wasn't given enough attention. I run a lot of regression tests w/ unionfs on top of many different file systems, and monitoring for all sorts of problems, including lockdep warnings. So it's frustrating that each time I get to test w/ jffs2, I get this lockdep warning, and lockdep shuts down until a reboot. That kills my automated testing and masks out possible other lockdep problems (that's why I wanted to at least have some workaround). Other lockdep problems I reported with other subsystems had been fixed within days; this one, unfortunately, has lingered for a while. So if a fix can be made in a reasonable time-frame, I'd really appreciate it. Thanks, Erez. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Sun, 2008-01-06 at 14:13 +0100, Jörn Engel wrote: > Ingo, Peter, does either of you actually care about this problem? In > the last round when I debugged this problem there was a notable lack of > reaction from either of you. Yeah I do, I just know very little about the module stuff and haven't come around to looking into it. I agree that Erez's patch is quite horrible. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
Maybe it is not obvious that I maintain this driver and would like to be
kept on Cc:. Will send a patch to fix that shortly.
On Sun, 6 January 2008 02:17:32 -0500, Erez Zadok wrote:
>
> Hi David,
>
> I've reported before a lockdep warning when block2mtd is modloaded, and a
> device is initialized, as in
>
> modprobe block2mtd block2mtd=/dev/loop0
>
> A typical warning looks like this:
>
> BUG: key f88565c0 not in .data!
> WARNING: at kernel/lockdep.c:2331 lockdep_init_map()
> Pid: 1823, comm: modprobe Not tainted 2.6.24-rc6-unionfs2 #135
> [] show_trace_log_lvl+0x1a/0x2f
> [] show_trace+0x12/0x14
> [] dump_stack+0x6c/0x72
> [] lockdep_init_map+0x94/0x374
> [] debug_mutex_init+0x2c/0x3c
> [] __mutex_init+0x38/0x40
> [] 0xf885520d
> [] parse_args+0x121/0x1fb
> [] sys_init_module+0x10e8/0x1576
> [] sysenter_past_esp+0x5f/0xa5
> ===
>
> This is a long-standing problem I've seen in several of the latest stable
> kernels. Once lockdep turns itself off, there's no easy way to turn it back
> on short of a reboot.
>
> I looked more closely at the mtd code. I believe the problem is that
> lockdep doesn't like a mutex_init to be called from a module_init code path,
> possibly because the module's symbols aren't all initialized yet. (This
> could arguably be considered a limitation of lockdep.)
>
> So I tried to defer the call to module_init until it's absolutely needed. I
> couldn't find a clean way to do that via the struct mtd_info ops (there's no
> suitable ->init op), so instead I used an int to mark whether the mutex is
> initialized or not. Below is a patch. It works, but it's not as clean as
> it should be: a better way would be to probably add an mtd_info ->init op or
> so.
>
> At least with this patch, lockdep doesn't complain any longer, so I can run
> a clean set of regression tests w/ unionfs on top of jffs2 and other file
> systems.
>
> In lieu of a better fix, is this patch acceptable?
Not for me. I don't mind if you keep such a hack until a proper
solution if found in your private tree. But it is a horrible solution
to a problem introduced elsewhere.
Ingo, Peter, does either of you actually care about this problem? In
the last round when I debugged this problem there was a notable lack of
reaction from either of you.
> block2mtd: defer mutex initialization to avoid a lockdep warning
>
> Signed-off-by: Erez Zadok <[EMAIL PROTECTED]>
>
> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> index be4b994..2c6d3e7 100644
> --- a/drivers/mtd/devices/block2mtd.c
> +++ b/drivers/mtd/devices/block2mtd.c
> @@ -32,6 +32,7 @@ struct block2mtd_dev {
> struct list_head list;
> struct block_device *blkdev;
> struct mtd_info mtd;
> + int mutex_initialized;
> struct mutex write_mutex;
> };
>
> @@ -85,6 +86,11 @@ static int block2mtd_erase(struct mtd_info *mtd, struct
> erase_info *instr)
> size_t len = instr->len;
> int err;
>
> + if (!dev->mutex_initialized) {
> + mutex_init(>write_mutex);
> + dev->mutex_initialized = 1;
> + }
> +
> instr->state = MTD_ERASING;
> mutex_lock(>write_mutex);
> err = _block2mtd_erase(dev, from, len);
> @@ -194,6 +200,11 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t
> to, size_t len,
> struct block2mtd_dev *dev = mtd->priv;
> int err;
>
> + if (!dev->mutex_initialized) {
> + mutex_init(>write_mutex);
> + dev->mutex_initialized = 1;
> + }
> +
> if (!len)
> return 0;
> if (to >= mtd->size)
> @@ -275,8 +286,6 @@ static struct block2mtd_dev *add_device(char *devname,
> int erase_size)
> goto devinit_err;
> }
>
> - mutex_init(>write_mutex);
> -
> /* Setup the MTD structure */
> /* make the name contain the block device in */
> dev->mtd.name = kmalloc(sizeof("block2mtd: ") + strlen(devname),
>
> __
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Jörn
--
Joern's library part 13:
http://www.chip-architect.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Sun, 2008-01-06 at 14:13 +0100, Jörn Engel wrote: Ingo, Peter, does either of you actually care about this problem? In the last round when I debugged this problem there was a notable lack of reaction from either of you. Yeah I do, I just know very little about the module stuff and haven't come around to looking into it. I agree that Erez's patch is quite horrible. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
In message [EMAIL PROTECTED], =?utf-8?B?SsO2cm4=?= Engel writes: Maybe it is not obvious that I maintain this driver and would like to be kept on Cc:. Will send a patch to fix that shortly. I was looking in MAINTAINERS, and you weren't listed on the main MTD section as a maintainer. Perhaps an update? If you get a patch, I'd love to test it. On Sun, 6 January 2008 02:17:32 -0500, Erez Zadok wrote: [...] In lieu of a better fix, is this patch acceptable? Not for me. I don't mind if you keep such a hack until a proper solution if found in your private tree. But it is a horrible solution to a problem introduced elsewhere. Yup, that's what I figured. I'll keep this ugly hack but I won't push it to my unionfs tree on kernel.org. Ingo, Peter, does either of you actually care about this problem? In the last round when I debugged this problem there was a notable lack of reaction from either of you. The problem appears to be an interaction of two components--module loading and lockdep--that's perhaps why it wasn't given enough attention. I run a lot of regression tests w/ unionfs on top of many different file systems, and monitoring for all sorts of problems, including lockdep warnings. So it's frustrating that each time I get to test w/ jffs2, I get this lockdep warning, and lockdep shuts down until a reboot. That kills my automated testing and masks out possible other lockdep problems (that's why I wanted to at least have some workaround). Other lockdep problems I reported with other subsystems had been fixed within days; this one, unfortunately, has lingered for a while. So if a fix can be made in a reasonable time-frame, I'd really appreciate it. Thanks, Erez. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block2mtd lockdep_init_map warning
On Sun, 6 January 2008 14:11:47 -0500, Erez Zadok wrote: The problem appears to be an interaction of two components--module loading and lockdep--that's perhaps why it wasn't given enough attention. Correct. For modules lockdep depends on initializations done after module_init has finished. However block2mtd is an odd sod that can call into lockdep code during module_init, causing the bug you noticed. Several solutions are possible. Modules could get two initcalls, one to decide whether module load should get aborted, the other run later, after the remaining module initializations are done. Or the module loader could always do the initializations and revoke them later, if module_init failed. But I personally am too unfamiliar with the module code to trust my judgement and have yet to receive feedback. Even you seem to ignore my mails and not even Cc: me later on. I must have done something really horrible in my last life, it seems. Jörn -- A quarrel is quickly settled when deserted by one party; there is no battle unless there be two. -- Seneca -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] block2mtd lockdep_init_map warning
Hi David,
I've reported before a lockdep warning when block2mtd is modloaded, and a
device is initialized, as in
modprobe block2mtd block2mtd=/dev/loop0
A typical warning looks like this:
BUG: key f88565c0 not in .data!
WARNING: at kernel/lockdep.c:2331 lockdep_init_map()
Pid: 1823, comm: modprobe Not tainted 2.6.24-rc6-unionfs2 #135
[] show_trace_log_lvl+0x1a/0x2f
[] show_trace+0x12/0x14
[] dump_stack+0x6c/0x72
[] lockdep_init_map+0x94/0x374
[] debug_mutex_init+0x2c/0x3c
[] __mutex_init+0x38/0x40
[] 0xf885520d
[] parse_args+0x121/0x1fb
[] sys_init_module+0x10e8/0x1576
[] sysenter_past_esp+0x5f/0xa5
===
This is a long-standing problem I've seen in several of the latest stable
kernels. Once lockdep turns itself off, there's no easy way to turn it back
on short of a reboot.
I looked more closely at the mtd code. I believe the problem is that
lockdep doesn't like a mutex_init to be called from a module_init code path,
possibly because the module's symbols aren't all initialized yet. (This
could arguably be considered a limitation of lockdep.)
So I tried to defer the call to module_init until it's absolutely needed. I
couldn't find a clean way to do that via the struct mtd_info ops (there's no
suitable ->init op), so instead I used an int to mark whether the mutex is
initialized or not. Below is a patch. It works, but it's not as clean as
it should be: a better way would be to probably add an mtd_info ->init op or
so.
At least with this patch, lockdep doesn't complain any longer, so I can run
a clean set of regression tests w/ unionfs on top of jffs2 and other file
systems.
In lieu of a better fix, is this patch acceptable?
Thanks,
Erez.
--
block2mtd: defer mutex initialization to avoid a lockdep warning
Signed-off-by: Erez Zadok <[EMAIL PROTECTED]>
diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index be4b994..2c6d3e7 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -32,6 +32,7 @@ struct block2mtd_dev {
struct list_head list;
struct block_device *blkdev;
struct mtd_info mtd;
+ int mutex_initialized;
struct mutex write_mutex;
};
@@ -85,6 +86,11 @@ static int block2mtd_erase(struct mtd_info *mtd, struct
erase_info *instr)
size_t len = instr->len;
int err;
+ if (!dev->mutex_initialized) {
+ mutex_init(>write_mutex);
+ dev->mutex_initialized = 1;
+ }
+
instr->state = MTD_ERASING;
mutex_lock(>write_mutex);
err = _block2mtd_erase(dev, from, len);
@@ -194,6 +200,11 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t
to, size_t len,
struct block2mtd_dev *dev = mtd->priv;
int err;
+ if (!dev->mutex_initialized) {
+ mutex_init(>write_mutex);
+ dev->mutex_initialized = 1;
+ }
+
if (!len)
return 0;
if (to >= mtd->size)
@@ -275,8 +286,6 @@ static struct block2mtd_dev *add_device(char *devname, int
erase_size)
goto devinit_err;
}
- mutex_init(>write_mutex);
-
/* Setup the MTD structure */
/* make the name contain the block device in */
dev->mtd.name = kmalloc(sizeof("block2mtd: ") + strlen(devname),
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] block2mtd lockdep_init_map warning
Hi David,
I've reported before a lockdep warning when block2mtd is modloaded, and a
device is initialized, as in
modprobe block2mtd block2mtd=/dev/loop0
A typical warning looks like this:
BUG: key f88565c0 not in .data!
WARNING: at kernel/lockdep.c:2331 lockdep_init_map()
Pid: 1823, comm: modprobe Not tainted 2.6.24-rc6-unionfs2 #135
[c02038c0] show_trace_log_lvl+0x1a/0x2f
[c02042bb] show_trace+0x12/0x14
[c0204a01] dump_stack+0x6c/0x72
[c022edf0] lockdep_init_map+0x94/0x374
[c022e79d] debug_mutex_init+0x2c/0x3c
[c0229bb0] __mutex_init+0x38/0x40
[f885520d] 0xf885520d
[c0226816] parse_args+0x121/0x1fb
[c0237aaf] sys_init_module+0x10e8/0x1576
[c0202836] sysenter_past_esp+0x5f/0xa5
===
This is a long-standing problem I've seen in several of the latest stable
kernels. Once lockdep turns itself off, there's no easy way to turn it back
on short of a reboot.
I looked more closely at the mtd code. I believe the problem is that
lockdep doesn't like a mutex_init to be called from a module_init code path,
possibly because the module's symbols aren't all initialized yet. (This
could arguably be considered a limitation of lockdep.)
So I tried to defer the call to module_init until it's absolutely needed. I
couldn't find a clean way to do that via the struct mtd_info ops (there's no
suitable -init op), so instead I used an int to mark whether the mutex is
initialized or not. Below is a patch. It works, but it's not as clean as
it should be: a better way would be to probably add an mtd_info -init op or
so.
At least with this patch, lockdep doesn't complain any longer, so I can run
a clean set of regression tests w/ unionfs on top of jffs2 and other file
systems.
In lieu of a better fix, is this patch acceptable?
Thanks,
Erez.
--
block2mtd: defer mutex initialization to avoid a lockdep warning
Signed-off-by: Erez Zadok [EMAIL PROTECTED]
diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index be4b994..2c6d3e7 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -32,6 +32,7 @@ struct block2mtd_dev {
struct list_head list;
struct block_device *blkdev;
struct mtd_info mtd;
+ int mutex_initialized;
struct mutex write_mutex;
};
@@ -85,6 +86,11 @@ static int block2mtd_erase(struct mtd_info *mtd, struct
erase_info *instr)
size_t len = instr-len;
int err;
+ if (!dev-mutex_initialized) {
+ mutex_init(dev-write_mutex);
+ dev-mutex_initialized = 1;
+ }
+
instr-state = MTD_ERASING;
mutex_lock(dev-write_mutex);
err = _block2mtd_erase(dev, from, len);
@@ -194,6 +200,11 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t
to, size_t len,
struct block2mtd_dev *dev = mtd-priv;
int err;
+ if (!dev-mutex_initialized) {
+ mutex_init(dev-write_mutex);
+ dev-mutex_initialized = 1;
+ }
+
if (!len)
return 0;
if (to = mtd-size)
@@ -275,8 +286,6 @@ static struct block2mtd_dev *add_device(char *devname, int
erase_size)
goto devinit_err;
}
- mutex_init(dev-write_mutex);
-
/* Setup the MTD structure */
/* make the name contain the block device in */
dev-mtd.name = kmalloc(sizeof(block2mtd: ) + strlen(devname),
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
