Re: [PATCH 1/7] dynamic debug v2 - infrastructure

[Rusty Russell <rusty@rustcorp.com.au>]

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.