Re: [PATCH 1/8] kgdb: core API and gdb protocol handler

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

jason.wessel@windriver.com wrote:
> --- /dev/null
> +++ b/include/asm-generic/kgdb.h
> @@ -0,0 +1,105 @@
> +/*
> + * include/asm-generic/kgdb.h
> + *
> + * This provides the assembly level information so that KGDB can provide
> + * a GDB that has been patched with enough information to know to stop
> + * trying to unwind the function.
> + *
> + * Author: Tom Rini <trini@kernel.crashing.org>
> + *
> + * 2005 (c) MontaVista Software, Inc.
> + * 2006 (c) Embedded Alley Solutions, Inc.
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + */
> +
> +#ifndef __ASM_GENERIC_KGDB_H__
> +#define __ASM_GENERIC_KGDB_H__
> +
> +struct pt_regs;
> +
> +#ifdef CONFIG_X86

Hmm, I just realized that I didn't really looked at the arch interface
in details yet. Now it appears a bit inconsistent to me in its way to
tell "this arch supports that". Something in include/asm-generic that
starts with #ifdef CONFIG_X86 looks ugly, to begin with.

...
> +#ifdef CONFIG_KGDB_ARCH_HAS_SHADOW_INFO

Then we have this KGDB_ARCH_HAS thing here, the second way to control
arch-dependent stuff.

...
> --- /dev/null
> +++ b/include/linux/kgdb.h
...
> +struct kgdb_arch {
> +	unsigned char		gdb_bpt_instr[BREAK_INSTR_SIZE];
> +	unsigned long		flags;
> +	unsigned		shadowth;
> +
> +	int	(*set_breakpoint)(unsigned long, char *);
> +	int	(*remove_breakpoint)(unsigned long, char *);
> +	int	(*set_hw_breakpoint)(unsigned long, int, enum kgdb_bptype);
> +	int	(*remove_hw_breakpoint)(unsigned long, int, enum kgdb_bptype);
> +	void	(*remove_all_hw_break)(void);
> +	void	(*correct_hw_break)(void);
> +};

And this is the third way. Can we consolidate this?

...
> --- /dev/null
> +++ b/kernel/kgdb.c
...
> +/*
> + * Convert the memory pointed to by mem into hex, placing result in buf.
> + * Return a pointer to the last char put in buf (null). May return an error.
> + */
> +char *kgdb_mem2hex(char *mem, char *buf, int count)

As Linus remarked, still pointer-based return values while only the
error code is used by the callers (there are more of such cases in the
core).

...
> --- a/kernel/softlockup.c
> +++ b/kernel/softlockup.c
...
> @@ -52,7 +55,6 @@ static unsigned long get_timestamp(int this_cpu)
>  void touch_softlockup_watchdog(void)
>  {
>  	int this_cpu = raw_smp_processor_id();
> -

Probably some relic from older changes which should be killed.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]