Re: [patch] kgdb light, v6

[Ingo Molnar <mingo@elte.hu>]

* Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> > +} breakinfo[4] = {
> > +	{ .enabled = 0 },
> > +	{ .enabled = 0 },
> > +	{ .enabled = 0 },
> > +	{ .enabled = 0 },
> > +};
> 
> is this initialization really needed?  the whole thing is static 
> anyway

good point! It's not needed at all: fixed.

> > +			case 3:
> > +				set_debugreg(breakinfo[3].addr, 3);
> > +				break;
> 
> 			if (breakno >= 0 && breakno <= 3)
> 				set_debugreg(breakinfo[breakno].addr, breakno);

nice! I've added your simplification.

> > + */
> > +int kgdb_arch_init(void)
> > +{
> > +	register_die_notifier(&kgdb_notifier);
> > +	return 0;
> 
> return register_die_notifier();

agreed - done. (btw., for kicks i checked kernel/notifier.c - 
register_die_notifier() never fails and always returns 0!)

> [...]
> 
> > +MODULE_DESCRIPTION("KGDB Console TTY Driver");
> > +MODULE_LICENSE("GPL");
> 
> should be at the bottom of the file

agreed - i moved it.

> > +static int kgdboc_option_setup(char *opt)
> 
> __init

done.

> > +__setup("kgdboc=", kgdboc_option_setup);
> 
> no need for obsolete __setup, we have module_param_call() below

it's needed for bzImage kernels. I just tested it and without __setup() 
no init sequence is run and KGDB is not activated.

> > +static int configure_kgdboc(void)
> 
> __init

ok, done.

> > +static int init_kgdboc(void)
> 
> __init

done.

> > +#ifdef CONFIG_CONSOLE_POLL
> > +
> 
> unnecessary new line

(that is a personal taste/style thing - to me it simply looks more 
readable if there's an empty line before function declarations.)

> > +/* The maximum number of KGDB I/O modules that can be loaded */
> > +#define KGDB_MAX_IO_HANDLERS	3
> 
> unused

good - zapped it.

> > +#ifndef KGDB_MAX_BREAKPOINTS
> > +# define KGDB_MAX_BREAKPOINTS	1000
> > +#endif
> > +
> > +#define KGDB_HW_BREAKPOINT	1
> 
> unused

hm, both KGDB_MAX_BREAKPOINTS and KGDB_HW_BREAKPOINT are used.

> > + * @late_init: Pointer to a function that will do any setup that has
> 
> there is no late_init in the structure

zapped it.

> identical cache flushing code is present in
> kgdb_deactivate_sw_breakpoints() below
> 
> maybe it would make sense to have some common helper

agreed. Incidentally, while looking at uaccess patterns i noticed this 
and i've already written one: kgdb_flush_swbreak_addr().

> if kgdb_isremovedbreak() helper is moved before kgdb_set_sw_break() 
> and converted to return 'i' on success and '-1' on failure then it can 
> be used instead the above for () loop

dunno - that would complicate arch/x86/kernel/kgdb.c's use of 
kgdb_isremovedbreak() and looks a bit complex. If you feel strongly 
about it, could you send a patch?

in any case, thanks Bartlomiej for the many very useful comments, i 
fixed all of the the things you noticed in my current tree.

	Ingo