Date:	Wed, 07 Mar 2012 11:00:56 +1030
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Jason Baron <jbaron@...hat.com>, Thomas Renninger <trenn@...e.de>
Cc:	Jim Cromie <jim.cromie@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 18/25] dynamic_debug: Introduce global fake module param module.ddebug

On Tue, 6 Mar 2012 09:47:54 -0500, Jason Baron <jbaron@...hat.com> wrote:
> On Tue, Mar 06, 2012 at 10:02:56AM +0100, Thomas Renninger wrote:
> > On Monday, March 05, 2012 06:22:06 PM Jim Cromie wrote:
> > > 2012/3/5 Thomas Renninger <trenn@...e.de>:
> > ...
> > > I have reworked 18-25 on top of linux-next, and have been
> > > casually boot testing it on my i586 based soekris, also for
> > > several weeks.
> > > 
> > > I have some misgivings about trivial things I added to params.c
> > > (primarily pr-debugs in init-level), and about a few other details,
> > > but I guess I should just write up an explanation and send it out.
> > Seeing the stuff in linux-next soon would be great.

Agreed.  OK, here's a partial review.

Patch 1: init: add a pr_info, pr_debug to initcall-levels
> add a pr_info to print current level of initcall,
> add a pr_debug and a counter to initcall_level() to indicate

Meh.  We have an initcall_debug flag.  Let's use it please.

Patch 2: params: add a pr-debug to parse_one's min/max level

No, this is a great deal of spew for little purpose.

Patch 3: params: parse_one's pr_debug("They are equal! ...") isnt informative

Actually, callback address is informative, since it's kernel devs who
are debugging this.  But as they can add it, we should just drop this
patch, or remove the line altogether.

[PATCH 04/15] params: add 3rd arg to option handler callback

This is fine, I'll take this as is.

[PATCH 05/15] dynamic_debug: make dynamic-debug work during module initialization:

> +static inline int ddebug_dyndbg_param_cb(char *param, char *val,
> +					const char *modname)
> +{
> +	if (strstr(param, "dyndbg"))
> +		pr_warn("dyndbg supported only in "
> +			"CONFIG_DYNAMIC_DEBUG builds\n");
> +	return 0; /* allow unknown options */
> +}

No, this breaks unknown module params!  Please split into two callbacks:

        check_for_dyndebug()

        check_for_module_dyndebug()

The latter may be in module.c and call a common helper, depends how neat it
is.

> +	char *param, *modname;
> +
> +	param = strstr(fqparam, "dyndbg");
> +	if (!param) {
> +		pr_debug("%s: skip %s\n", doing, fqparam);
> +		return 0; /* param is unknown, ignore it (for boot) */
> +	}
> +	if (param > fqparam) {
> +		/* fqparam has module prefix, split it in 2 */
> +		*(param-1) = '\0';
> +		modname = fqparam;
> +	}
> +	else
> +		modname = doing;
> +
> +	if (verbose)
> +		pr_info("module: %s %s=\"%s\"\n", modname, param, val);
> +
> +	ddebug_exec_queries(val ? val : "+p");
> +	return 0; /* query failure shouldnt stop module load */

Please don't hack-parse, this accepts all kinds of invalid crap.

Your wildcard patch is even worse.  A sane option is:

        "dyndbg[=...]" on boot line turns dyndbg for everything.
        "<modname>.dyndbg[=...]" on boot line turns on dyndbg for
        that module.
        "dyndbg[=...]" on module line turns dyndbg for that module.

Something like:
        const char *modname = NULL;
        char *sep;
       
        sep = strchr(fqparam, '.');
        if (sep) {
                *sep = '\0';
                modname = fqparam;
                fqparam = sep + 1;
        }

        ...

[PATCH 07/15] dynamic_debug: add modname arg to exec_query callchain

This looks sane.

[PATCH 08/15] dynamic_debug: allow *.dyndbg=+p in boot args

No, just make it "dyndbg=+p", as above.

[PATCH 09/15] dynamic_debug: protect "dyndbg" fake module param name at compile-time

BUILD_BUG_DECL is redundant, you should use BUILD_BUG_ZERO.  But that
insists on a constant expression, so you'd need a new variant.  Probably
easiest to drop this one.  If someone calls their parameter dyndbg they
probably mean exactly what they say.

[PATCH 11/15] dyndbg: init at core level, not arch

Please say why.

[PATCH 14/15] dyndbg: replace if (verbose) pr_info with macro vpr_info

>  #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
> +#define vpr_info(fmt, ...) if (verbose) { pr_info(fmt, ##__VA_ARGS__); }

This is a bear trap waiting to happen.  Please do { } while(0) wrap!

Thanks,
Rusty.
-- 
  How could I marry someone with more hair than me?  http://baldalex.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists