Messages in this thread | | | From | Rusty Russell <> | Subject | Re: [PATCH 1/7] dynamic debug v2 - infrastructure | Date | Tue, 16 Sep 2008 10:03:34 +1000 |
| |
On Wednesday 16 July 2008 07:31:08 Jason Baron wrote: > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -359,6 +359,10 @@ struct module > struct marker *markers; > unsigned int num_markers; > #endif > +#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG > + struct mod_debug *start_verbose; > + unsigned int num_verbose; > +#endif
Hi Jason,
Couple of nit-picks about the module part of this patch. First, this could just be called "verbose" rather than "start_verbose"...
> @@ -1717,6 +1718,9 @@ static struct module *load_module(void __user *umod, > unsigned int unusedgplcrcindex; > unsigned int markersindex; > unsigned int markersstringsindex; > + unsigned int verboseindex; > + struct mod_debug *iter; > + unsigned long value; > struct module *mod; > long err = 0; > void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */ > @@ -1993,6 +1997,7 @@ static struct module *load_module(void __user *umod, > markersindex = find_sec(hdr, sechdrs, secstrings, "__markers"); > markersstringsindex = find_sec(hdr, sechdrs, secstrings, > "__markers_strings"); > + verboseindex = find_sec(hdr, sechdrs, secstrings, "__verbose"); > > /* Now do relocations. */ > for (i = 1; i < hdr->e_shnum; i++) { > @@ -2020,6 +2025,11 @@ static struct module *load_module(void __user *umod, > mod->num_markers = > sechdrs[markersindex].sh_size / sizeof(*mod->markers); > #endif > +#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG > + mod->start_verbose = (void *)sechdrs[verboseindex].sh_addr; > + mod->num_verbose = sechdrs[verboseindex].sh_size / > + sizeof(*mod->start_verbose); > +#endif > > /* Find duplicate symbols */ > err = verify_export_symbols(mod); > @@ -2043,6 +2053,19 @@ static struct module *load_module(void __user *umod, > marker_update_probe_range(mod->markers, > mod->markers + mod->num_markers); > #endif > +#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG > + for (value = (unsigned long)mod->start_verbose; > + value < (unsigned long)mod->start_verbose + > + (unsigned long)(mod->num_verbose * sizeof(struct mod_debug)); > + value += sizeof(struct mod_debug)) { > + iter = (struct mod_debug *)value; > + register_debug_module(iter->modname, > + simple_strtoul(iter->type, NULL, 10), > + iter->logical_modname, > + simple_strtoul(iter->num_flags, NULL, 10), > + iter->flag_names); > + } > +#endif
This loop seems way more complex than it needs to be. Perhaps pull these two out into a setup_verbose_debug() func which is a noop for !CONFIG_DYNAMIC_PRINTK_DEBUG, and drop all the casts?
Thanks, Rusty.
| |