Re: [patch] kgdb light, v6
On Feb 10, 2008 2:40 PM, Ingo Molnar <[EMAIL PROTECTED]> wrote: > > * Jan Kiszka <[EMAIL PROTECTED]> wrote: > > > Ingo, please keep the original annotations, they where correct and > > should have been optimal (under the given constraints or runtime > > reconfiguration). > > agreed. I've regenerated the -v7 tree with this trivial revert. Tip is > commit 04b94b1dd5197bf737073ebbd4189ffdfdcea534, updated shortlog, > diffstat and patch can be found below. Tree is at: > > git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git > something related or not. my server doesn't have serial connector. the SP has serial port to host's internal serial port. I need to ssh to SP ( service processor) and start console there. wonder if there is program that is running on SP, and one program on develop workstation --- make it have one virtual serial port gdb or other legacy serial port program could use that virtual serial port. YH -- 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/
Re: [patch] kgdb light, v6
Bartlomiej Zolnierkiewicz wrote: > On Sunday 10 February 2008, Jan Kiszka wrote: >> Ingo Molnar wrote: >>> * Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> wrote: > +static int init_kgdboc(void) __init >>> done. >> Uuh, careful. We need this for runtime reconfiguration. > > it is used only for 'module_init(init_kgdboc);' in v6 Yeah, the usage chain is not obvious (given I'm looking at the right version ATM - git just spits at me), the issue moved to param_set_kgdboc_var which has to call into configure_kgdboc - now __init. Ingo, please keep the original annotations, they where correct and should have been optimal (under the given constraints or runtime reconfiguration). Jan -- 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/
Re: [patch] kgdb light, v6
* Jan Kiszka <[EMAIL PROTECTED]> wrote: > Ingo Molnar wrote: > > * Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> wrote: > >>> +static int init_kgdboc(void) > >> __init > > > > done. > > Uuh, careful. We need this for runtime reconfiguration. i think v7 is fine. worst-case we'll get a nice fat section warning :-) 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/
Re: [patch] kgdb light, v6
On Sunday 10 February 2008, Ingo Molnar wrote:
[...]
> > > +__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.
weird, should work with "kgdboc.kgdboc=" parameter
[...]
> > > +#ifndef KGDB_MAX_BREAKPOINTS
> > > +# define KGDB_MAX_BREAKPOINTS1000
> > > +#endif
> > > +
> > > +#define KGDB_HW_BREAKPOINT 1
> >
> > unused
>
> hm, both KGDB_MAX_BREAKPOINTS and KGDB_HW_BREAKPOINT are used.
my bad
[...]
> > 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
the whole difference w.r.t. arch/x86/kernel/kgdb.c should be:
- if (exception == 3 && kgdb_isremovedbreak(regs->ip - 1)) {
+ if (exception == 3 && kgdb_isremovedbreak(regs->ip - 1) >= 0) {
> about it, could you send a patch?
well, maybe in the future if nobody fixes it :)
[ added as low-prio to my TODO... ]
Thanks for fixing all the other stuff.
Bart
--
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/
Re: [patch] kgdb light, v6
On Sunday 10 February 2008, Jan Kiszka wrote: > Ingo Molnar wrote: > > * Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> wrote: > >>> +static int init_kgdboc(void) > >> __init > > > > done. > > Uuh, careful. We need this for runtime reconfiguration. it is used only for 'module_init(init_kgdboc);' in v6 -- 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/
Re: [patch] kgdb light, v6
Ingo Molnar wrote: > * Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> wrote: >>> +static int init_kgdboc(void) >> __init > > done. Uuh, careful. We need this for runtime reconfiguration. Jan -- 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/
Re: [patch] kgdb light, v6
* Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> wrote: > > no. These are not DocBook comments, if you look carefully at the > > format [it's not a leading '/**' comment block]. But obviously > > documenting this in the include file is very useful, because that's > > where people look first, so i kept it. (the APIs will not deviate > > across architectures) > > comments and variable names in include files have a tendency for going > out-of-sync in the long term so IMO having a DocBook to point people > at would be a better solution (+ it would shrink by 122 > lines) yes, i very much agree in general, but this is a _SPECIAL CASE_, and i already tried to point that out to Christoph but he's not the type of guy who listens to others all that easily when it comes to his pet peeves ;-) this is a special case because it's an _architecture facility_. read: right now we have 25 architectures, and this means that in a year we'll have 25 arch/*/kernel/kgdb.c files. What will be more likely to get out of sync, 25 full sets of DocBook entries of the same thing, spread across 25 architectures - or that lone single include/linux/kgdb.h file that is looked at by everyone? And what will be easier to update if we extend any of the APIs? so the DocBook rules are fine, but in this SPECIAL CASE they cause the possibly worst solution: total information anarchy! the correct approach is to put the _arch specific_ details into the arch/*/kernel/kgdb.c files, and to keep the generic bits in include/linux/kgdb.h. KGDB did exactly that and it's by far the cleanest and most maintainable approach. If DocBook does not pick that up then it's a _DocBook problem_. I dont mind adding some dummy weak aliases to kernel/kgdb.c for DocBook to pick up, to help solve this DocBook problem - but to blame it on KGDB is way off the mark. It used to be the crappiest piece of sh*t everyone would laugh about when looking at (right before suffering permanent brain damage), but now it's one of the cleanest and most CodingStyle conform kernel subsystems :-) case in point: errors lines of code errors/KLOC kernel/kgdb.c 01839 0 fs/xfs/ 2102 106019 19.8 right, XFS has more than 2 thousand bona fide CodingStyle violations! But yeah, it has the luxory of upstream integration ... ;-) [ not that i want to pick on XFS - it's a very clean codebase in my opinion, considering its fundamental complexity. It's just that anyone who wants to find a style error in KGDB now has to search _hard_ ;-) ] 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/
Re: [patch] kgdb light, v6
* 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/
Re: [patch] kgdb light, v6
few minor issues (some may have been addressed already)
On Sunday 10 February 2008, Ingo Molnar wrote:
[...]
> diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
> new file mode 100644
> index 000..7130273
> --- /dev/null
> +++ b/arch/x86/kernel/kgdb.c
[...]
> +static struct hw_breakpoint {
> + unsignedenabled;
> + unsignedtype;
> + unsignedlen;
> + unsigned long addr;
> +} breakinfo[4] = {
> + { .enabled = 0 },
> + { .enabled = 0 },
> + { .enabled = 0 },
> + { .enabled = 0 },
> +};
is this initialization really needed? the whole thing is static anyway
> +static void kgdb_correct_hw_break(void)
> +{
> + unsigned long dr7;
> + int correctit = 0;
> + int breakbit;
> + int breakno;
> +
> + get_debugreg(dr7, 7);
> + for (breakno = 0; breakno < 4; breakno++) {
> + breakbit = 2 << (breakno << 1);
> + if (!(dr7 & breakbit) && breakinfo[breakno].enabled) {
> + correctit = 1;
> + dr7 |= breakbit;
> + dr7 &= ~(0xf << (breakno << 2));
> + dr7 |= ((breakinfo[breakno].len << 2) |
> + breakinfo[breakno].type) <<
> +((breakno << 2) + 16);
> + switch (breakno) {
> + case 0:
> + set_debugreg(breakinfo[0].addr, 0);
> + break;
> +
> + case 1:
> + set_debugreg(breakinfo[1].addr, 1);
> + break;
> +
> + case 2:
> + set_debugreg(breakinfo[2].addr, 2);
> + break;
> +
> + case 3:
> + set_debugreg(breakinfo[3].addr, 3);
> + break;
if (breakno >= 0 && breakno <= 3)
set_debugreg(breakinfo[breakno].addr, breakno);
[...]
> +/**
> + * kgdb_arch_init - Perform any architecture specific initalization.
> + *
> + * This function will handle the initalization of any architecture
> + * specific callbacks.
> + */
> +int kgdb_arch_init(void)
> +{
> + register_die_notifier(&kgdb_notifier);
> + return 0;
return register_die_notifier();
[...]
> diff --git a/drivers/serial/kgdboc.c b/drivers/serial/kgdboc.c
> new file mode 100644
> index 000..a5d2d00
> --- /dev/null
> +++ b/drivers/serial/kgdboc.c
[...]
> +MODULE_DESCRIPTION("KGDB Console TTY Driver");
> +MODULE_LICENSE("GPL");
should be at the bottom of the file
> +static char config[MAX_KGDB_SERIAL_CONSOLE_CONFIG_STR];
> +static struct kparam_string kps = {
> + .string = config,
> + .maxlen = MAX_KGDB_SERIAL_CONSOLE_CONFIG_STR,
> +};
> +
> +static struct tty_driver *kgdb_tty_driver;
> +static int kgdb_tty_line;
> +
> +static int kgdboc_option_setup(char *opt)
__init
> +{
> + if (strlen(opt) > MAX_KGDB_SERIAL_CONSOLE_CONFIG_STR) {
> + printk(KERN_ERR "kgdboc: config string too long\n");
> + return -ENOSPC;
> + }
> + strcpy(config, opt);
> +
> + return 0;
> +}
> +__setup("kgdboc=", kgdboc_option_setup);
no need for obsolete __setup, we have module_param_call() below
> +static int configure_kgdboc(void)
__init
> +{
> + struct tty_driver *p;
> + int tty_line = 0;
> + int err;
> +
> + err = kgdboc_option_setup(config);
> + if (err || !strlen(config) || isspace(config[0]))
> + goto noconfig;
> +
> + err = -ENODEV;
> +
> + p = tty_find_polling_driver(config, &tty_line);
> + if (!p)
> + goto noconfig;
> +
> + kgdb_tty_driver = p;
> + kgdb_tty_line = tty_line;
> +
> + err = kgdb_register_io_module(&kgdboc_io_ops);
> + if (err)
> + goto noconfig;
> +
> + configured = 1;
> +
> + return 0;
> +
> +noconfig:
> + config[0] = 0;
> + configured = 0;
> +
> + return err;
> +}
> +
> +static int init_kgdboc(void)
__init
> +{
> + /* Already configured? */
> + if (configured == 1)
> + return 0;
> +
> + return configure_kgdboc();
> +}
> +
> +static void cleanup_kgdboc(void)
I would suggest __exit but it can be called from param_set_kgdboc_var()
[ I have a feeling that somethings is wrong with this but I'm too lazy
to read the code in depth... ]
> diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
> index 0f5a179..a72116a 100644
> --- a/drivers/serial/serial_core.c
> +++ b/drivers/serial/serial_core.c
[...]
> +#ifdef CONFIG_CONSOLE_POLL
> +
unnecessary new line
[...]
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> new file mode 100644
> index 000..7f4ee55
> --- /dev/null
> +++ b/include/linux/kgdb.h
> @@ -0,0 +1,329 @@
[...]
> +/* The maximu
Re: [patch] kgdb light, v6
On Sunday 10 February 2008, Ingo Molnar wrote: [...] > > > +/* > > > + * kgdb_get_shadow_thread - Get the shadowed &task_struct of > > > @threadid. > > > + * @regs: The &struct pt_regs of the current thread. > > > + * @threadid: The thread id of the shadowed process to get > > > information on. > > > + * > > > + * RETURN: > > > + * This returns a pointer to the &struct task_struct of the > > > shadowed > > > + * thread, @threadid. > > > + */ > > > +extern struct task_struct *kgdb_get_shadow_thread(struct pt_regs *regs, > > > + int threadid); > > > > So we have kerneldoc comments in both places now? Didn't you say > > you converted these to something else? > > no. These are not DocBook comments, if you look carefully at the format > [it's not a leading '/**' comment block]. But obviously documenting this > in the include file is very useful, because that's where people look > first, so i kept it. (the APIs will not deviate across architectures) comments and variable names in include files have a tendency for going out-of-sync in the long term so IMO having a DocBook to point people at would be a better solution (+ it would shrink by 122 lines) while at it: --- x86/kernel/kgdb.c 2008-02-10 20:30:39.0 +0100 +++ linux/kgdb.h2008-02-10 20:25:21.0 +0100 @@ -128,11 +131,13 @@ * process more packets, and a %0 or %1 if it wants to exit from the * kgdb callback. */ -int kgdb_arch_handle_exception(int e_vector, int signo, int err_code, - char *remcomInBuffer, char *remcomOutBuffer, - struct pt_regs *linux_regs) +extern int +kgdb_arch_handle_exception(int vector, int signo, int err_code, + char *remcom_in_buffer, + char *remcom_out_buffer, + struct pt_regs *regs); -- 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/
