* Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> 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
--
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/
