Add / use kernel/Kconfig.kgdb

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

init/main.c +1 -0

@@ -581,6 +582,7 @@ static int init(void * unused)
 
 	smp_init();
 	do_basic_setup();
+	debugger_entry();
 
 	prepare_namespace();
 

Quoted 6 lines · Tom Rini

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

Please don't do this. How about configuring it along with the connection method. I really don't want to have to modify the kernel just to get in early.

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

init/main.c +1 -0

@@ -581,6 +582,7 @@ static int init(void * unused)
 
 	smp_init();
 	do_basic_setup();
+	debugger_entry();
 
 	prepare_namespace();

OK to checkin.

-Amit

Quoted 7 lines · Tom Rini

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

init/main.c static int init(void * unused) · line 586

Quoted 8 lines

init/main.c +1 -0

@@ -581,6 +582,7 @@ static int init(void * unused)
 
 	smp_init();
 	do_basic_setup();
+	debugger_entry();
 

init/main.c static int init(void * unused) · line 587

Quoted 1 line

init/main.c +0 -0

 	prepare_namespace();

-
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/

Quoted 16 lines · Amit S. Kale

OK to checkin.

-Amit

Quoted 11 lines · Tom Rini

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

init/main.c +0 -0

@@ -581,6 +582,7 @@ static int init(void * unused)
 

Quoted 3 lines

init/main.c static int init(void * unused) · line 585

Quoted 3 lines

init/main.c +1 -0

 	smp_init();
 	do_basic_setup();
+	debugger_entry();

It would be nice to not need this. Could it be a side effect of configuring the interface or some such so we don't have to patch init/main.c

-g

Quoted 7 lines

init/main.c static int init(void * unused) · line 587

Quoted 1 line

init/main.c +0 -0

 	prepare_namespace();

-
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/

Quoted 19 lines · George Anzinger

Quoted 17 lines · Amit S. Kale

OK to checkin.

-Amit

Quoted 11 lines · Tom Rini

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

init/main.c +0 -0

@@ -581,6 +582,7 @@ static int init(void * unused)
 

Quoted 7 lines

Quoted 3 lines

init/main.c static int init(void * unused) · line 585

Quoted 3 lines

init/main.c +1 -0

 	smp_init();
 	do_basic_setup();
+	debugger_entry();

It would be nice to not need this. Could it be a side effect of configuring the interface or some such so we don't have to patch init/main.c

I attempted doing this when I was trying to code a netpoll independent ethernet interface. I couldn't do without it. I needed one hook to kgdb in init to mark completion of smp_init. If an interface was ready, that hook called breakpoint. A similar hook was placed in interface initialization code, it called breakpoint, if kgdb core was ready on account of smp_init completion.

-Amit

Quoted 10 lines

-g

Quoted 7 lines

init/main.c static int init(void * unused) · line 587

Quoted 1 line

init/main.c +0 -0

 	prepare_namespace();

-
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/

-
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/

Quoted 31 lines · Amit S. Kale

Quoted 20 lines · George Anzinger

Quoted 18 lines · Amit S. Kale

OK to checkin.

-Amit

Quoted 12 lines · Tom Rini

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

init/main.c +0 -0

11:33:51.854388988 -0700

init/main.c +0 -0

@@ -581,6 +582,7 @@ static int init(void * unused)
 

Quoted 7 lines

Quoted 3 lines

init/main.c static int init(void * unused) · line 585

Quoted 3 lines

init/main.c +1 -0

 	smp_init();
 	do_basic_setup();
+	debugger_entry();

It would be nice to not need this. Could it be a side effect of configuring the interface or some such so we don't have to patch init/main.c

Quoted 7 lines

I attempted doing this when I was trying to code a netpoll independent ethernet interface. I couldn't do without it. I needed one hook to kgdb in init to mark completion of smp_init. If an interface was ready, that hook called breakpoint. A similar hook was placed in interface initialization code, it called breakpoint, if kgdb core was ready on account of smp_init completion.

I guess the real question is why do you need to wait so long? What is it that needs to be done prior to this call?

Quoted 36 lines · George Anzinger

Quoted 34 lines · Amit S. Kale

Quoted 29 lines · George Anzinger

Quoted 14 lines · Amit S. Kale

OK to checkin.

-Amit

Quoted 7 lines · Tom Rini

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

Quoted 8 lines

init/main.c static int init(void * unused) · line 585

Quoted 8 lines

init/main.c +0 -0

11:33:51.854388988 -0700

init/main.c +1 -0

@@ -581,6 +582,7 @@ static int init(void * unused)
 
 	smp_init();
 	do_basic_setup();
+	debugger_entry();

It would be nice to not need this. Could it be a side effect of configuring the interface or some such so we don't have to patch init/main.c

Quoted 4 lines

Quoted 2 lines

I attempted doing this when I was trying to code a netpoll independent ethernet interface. I couldn't do without it. I needed one hook to kgdb in init to mark completion of smp_init. If an interface was ready, that hook called breakpoint. A similar hook was placed in interface initialization code, it called breakpoint, if kgdb core was ready on account of smp_init completion.

I guess the real question is why do you need to wait so long? What is it that needs to be done prior to this call?

For kgdboe, you need to wait for the ethernet driver, networking, etc. Without going and trying to do some big special case, that's when it has to be, for kgdboe.

For serial, esp if you make kgdb_arch_init() handle kgdb_serial pointer, it can happen much earlier. In fact, in that case you could probably throw breakpoint() in as first line of C (PPC32) or close to it (some mappings needed i386, sh, others, generally speaking).

Tom Rini wrote:

Quoted 8 lines

For kgdboe, you need to wait for the ethernet driver, networking, etc. Without going and trying to do some big special case, that's when it has to be, for kgdboe.

For serial, esp if you make kgdb_arch_init() handle kgdb_serial pointer, it can happen much earlier. In fact, in that case you could probably throw breakpoint() in as first line of C (PPC32) or close to it (some mappings needed i386, sh, others, generally speaking).

So the question is do we cater to the worst or the best? If I put kgdb or what ever on the command line, I would like to see the connection ASAP what ever the connection is. So possibly, the command line parse part of kgdb sets a flag it it can not do the break NOW. The late bloomer code finds this flag when doing the kgdboe or what ever set up and enters at that time. Puts a little more smarts in the command line part, but gets things rolling ASAP regardless of the interface. This, of course, assumes that the kgdboe or what ever has an independent way of getting entered as soon as it can be set up, i.e. its prerequisites are satisfied.

Quoted 26 lines · Amit S. Kale

Quoted 17 lines · George Anzinger

Quoted 16 lines · Amit S. Kale

OK to checkin.

-Amit

Quoted 11 lines · Tom Rini

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

init/main.c +0 -1

-0700 @@ -581,6 +582,7 @@ static int init(void * unused)

Quoted 7 lines

Quoted 3 lines

init/main.c static int init(void * unused) · line 585

Quoted 3 lines

	smp_init();
	do_basic_setup();

init/main.c +1 -0

+	debugger_entry();

It would be nice to not need this. Could it be a side effect of configuring the interface or some such so we don't have to patch init/main.c

Quoted 6 lines

I attempted doing this when I was trying to code a netpoll independent ethernet interface. I couldn't do without it. I needed one hook to kgdb in init to mark completion of smp_init. If an interface was ready, that hook called breakpoint. A similar hook was placed in interface initialization code, it called breakpoint, if kgdb core was ready on account of smp_init completion.

I guess the real question is why do you need to wait so long? What is it that needs to be done prior to this call?

On the other hand, I have a late call (I use module init) to set up the interrupt handler for the UART, which needs to happen after malloc is working. This, however, does not cause, of it self, a break.

Quoted 17 lines

-Amit

Quoted 9 lines

-g

Quoted 7 lines

Quoted 1 line

	prepare_namespace();

-
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/

-
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/

Quoted 34 lines · George Anzinger

Quoted 32 lines · Amit S. Kale

Quoted 17 lines · George Anzinger

Quoted 16 lines · Amit S. Kale

OK to checkin.

-Amit

Quoted 11 lines · Tom Rini

Hello. When we use kgdboe, we can't use it until do_basic_setup() is done. So we have two options, not allow kgdboe to use the initial breakpoint or move debugger_entry() to be past the point where kgdboe will be usable. I've opted for the latter, as if an earlier breakpoint is needed you can still use serial and throw kgdb_schedule_breakpoint/breakpoint where desired.

init/main.c +0 -1

-0700 @@ -581,6 +582,7 @@ static int init(void * unused)

Quoted 7 lines

Quoted 3 lines

init/main.c static int init(void * unused) · line 585

Quoted 3 lines

	smp_init();
	do_basic_setup();

init/main.c +1 -0

+	debugger_entry();

It would be nice to not need this. Could it be a side effect of configuring the interface or some such so we don't have to patch init/main.c

I attempted doing this when I was trying to code a netpoll independent ethernet interface. I couldn't do without it. I needed one hook to kgdb in init to mark completion of smp_init. If an interface was ready, that hook called breakpoint. A similar hook was placed in interface initialization code, it called breakpoint, if kgdb core was ready on account of smp_init completion.

Quoted 2 lines

I guess the real question is why do you need to wait so long? What is it that needs to be done prior to this call?

2.4.x kgdb legacy code! That was needed in some early 2.4 kgdbs. Perhaps we can move it earlier now. If someone tests the patch on an smp machine and confirms that it works, I'll be happy change it.

Hello. The following is a couple of small but important cleanups to kgdboe. - memset out_buf in case we don't have it full when we flush.
- In rx_hook, if netpoll_trap() is set, clear it. If kgdb_isn't connected, schedule a breakpoint, and never call breakpoint() directly.
- In kgdb_serial->hook, set netpoll_trap(1). kgdb_serial->hook must be called prior to using a 'serial' port, so this is where we should stick this.
- Backout unneeded changes to other files.

--- linux-2.6.3/drivers/net/kgdb_eth.c.orig 2004-02-27 13:27:53.778658691
-0700 +++ linux-2.6.3/drivers/net/kgdb_eth.c 2004-02-27 13:32:07.920255062
-0700 @@ -18,6 +18,7 @@
* Refactored for netpoll API by Matt Mackall <[email protected]> *
* Some cleanups by Pavel Machek <[email protected]>
+ * Further cleanups by Tom Rini <[email protected]>
*/

#include <linux/module.h>

drivers/net/kgdb_eth.c renamed from drivers/net/kgdb_eth.c.orig +10 -4

@@ -60,7 +61,6 @@
 static atomic_t in_count;
 int kgdboe = 0;			/* Default to tty mode */
 
-extern void breakpoint(void);
 static void rx_hook(struct netpoll *np, int port, char *msg, int len);
 
 static struct netpoll np = {

@@ -89,6 +89,7 @@
 {
 	if (out_count && np.dev) {
 		netpoll_send_udp(&np, out_buf, out_count);
+		memset(out_buf, 0, sizeof(out_buf));
 		out_count = 0;
 	}
 }

@@ -107,12 +108,15 @@
 	np->remote_port = port;
 
+	/* Do we need to clear the trap? */
+	if (netpoll_trap())
+		netpoll_set_trap(0);
 	/* Is this gdb trying to attach? */
-	if (!netpoll_trap() && len == 8 && !strncmp(msg, "$Hc-1#09", 8))
-		breakpoint();
+	if (kgdb_connected) {
+		kgdb_schedule_breakpoint();
 
 	for (i = 0; i < len; i++) {
 		if (msg[i] == 3)
-			breakpoint();
+			kgdb_schedule_breakpoint();
 
 		if (atomic_read(&in_count) >= IN_BUF_SIZE) {
 			/* buffer overflow, clear it */

@@ -138,6 +141,9 @@
 	/* Un-initalized, don't go further. */
 	if (kgdboe != 1)
 		return 1;
+
+	netpoll_set_trap(1);
+
 	return 0;
 }
 

net/core/skbuff.c renamed from net/core/skbuff.c.orig +0 -1

@@ -55,7 +55,6 @@
 #include <linux/rtnetlink.h>
 #include <linux/init.h>
 #include <linux/highmem.h>
-#include <linux/debugger.h>
 
 #include <net/protocol.h>
 #include <net/dst.h>

net/core/dev.c renamed from net/core/dev.c.orig +1 -0

@@ -1547,6 +1547,7 @@
 }
 #endif
 
+
 /**
  *	netif_rx	-	post buffer to the network code
  *	@skb: buffer to post

Hello. The following removes the only PPC32 CHK_DEBUGGER statement, as we've always been making kgdb take over the various debugger pointers.

diff -zrupN linux-2.6.3+config+serial/arch/ppc/mm/fault.c linux-2.6.3+config+serial+sysrq+arch_hooks/arch/ppc/mm/fault.c ---
linux-2.6.3+config+serial/arch/ppc/mm/fault.c 2004-02-27 12:16:13.000000000
-0700 +++
linux-2.6.3+config+serial+sysrq+arch_hooks/arch/ppc/mm/fault.c 2004-02-27
12:16:14.000000000 -0700 @@ -351,13 +351,11 @@ bad_page_fault(struct pt_regs *regs, uns

arch/ppc/mm/fault.c +1 -3

 	}
 
 	/* kernel has accessed a bad area */
-#if defined(CONFIG_XMON)
+#if defined(CONFIG_XMON) || defined(CONFIG_KGDB)
 	if (debugger_kernel_faults)
 		debugger(regs);
 #endif
 
-	CHK_DEBUGGER(14, sig,0, regs,)
-
 	die("kernel access of bad area", regs, sig);
 }

Hello. The following is more complicated than it looks. This makes the x86_64 stub add kgdb to the die_chain, and then remove all of the now unneeded CHK_DEBUGGER statements, and other whitespace changes. In the end, the x86_64 patch looks like:

 arch/x86_64/Kconfig            |    4
 arch/x86_64/kernel/Makefile    |    1
 arch/x86_64/kernel/entry.S     |   41 +++
 arch/x86_64/kernel/irq.c       |    2
 arch/x86_64/kernel/kgdb.c      |  478
+++++++++++++++++++++++++++++++++++++++++ arch/x86_64/kernel/traps.c     | 
  3
 include/asm-x86_64/kgdb.h      |   54 ++++
 include/asm-x86_64/processor.h |    1
 8 files changed, 577 insertions(+), 7 deletions(-)

As an aside, I now know how to kill off user_schedule bits on i386/x86_64, so with more thought on how to do it cleanly for everyone, the entry.S changes can die.

diff -zrupN linux-2.6.3+config+serial/arch/x86_64/kernel/kgdb.c linux-2.6.3+config+serial+sysrq+arch_hooks/arch/x86_64/kernel/kgdb.c ---
linux-2.6.3+config+serial/arch/x86_64/kernel/kgdb.c 2004-02-27
12:16:13.000000000 -0700 +++
linux-2.6.3+config+serial+sysrq+arch_hooks/arch/x86_64/kernel/kgdb.c 2004-0
2-27 12:16:14.000000000 -0700 @@ -41,6 +41,7 @@
#endif
#include <linux/init.h>
#include <linux/debugger.h>

arch/x86_64/kernel/kgdb.c +14 -0

+#include <asm/kdebug.h>
 
 /* Put the error code here just in case the user cares.  */
 int gdb_x86_64errcode;

@@ -443,6 +444,33 @@ struct pt_regs *kgdb_shadow_regs(struct
 	return NULL;
 }
 
+/* Register KGDB with the die_chain so that we hook into all of the right
+ * spots. */
+static int kgdb_notify(struct notifier_block *self, unsigned long cmd,
+		void *ptr)
+{
+        struct die_args *d = ptr;
+
+        if (!kgdb_connected || (cmd == DIE_DEBUG && user_mode(d->regs)))
+                return NOTIFY_DONE;
+        if (cmd == DIE_NMI_IPI) {
+                if (atomic_read(debugger_active))
+                        return NOTIFY_BAD;
+        } else if ((*linux_debug_hook)(d->trapnr, d->signr, d->err,

d->regs)) + return NOTIFY_BAD; /* skip */

arch/x86_64/kernel/kgdb.c +13 -0

+
+        return NOTIFY_DONE;
+}
+
+static struct notifier_block kgdb_notifier = {
+        .notifier_call = kgdb_notify,
+};
+
+int kgdb_arch_init(void)
+{
+	notifier_chain_register(&die_chain, &kgdb_notifier);
+}
+
 struct kgdb_arch arch_kgdb_ops = {
 	.gdb_bpt_instr = {0xcc},
 	.flags = KGDB_HW_BREAKPOINT,

diff -zrupN linux-2.6.3+config+serial/arch/x86_64/kernel/nmi.c linux-2.6.3+config+serial+sysrq+arch_hooks/arch/x86_64/kernel/nmi.c ---
linux-2.6.3+config+serial/arch/x86_64/kernel/nmi.c 2004-02-27
12:16:13.000000000 -0700 +++
linux-2.6.3+config+serial+sysrq+arch_hooks/arch/x86_64/kernel/nmi.c 2004-02
-27 12:11:20.000000000 -0700 @@ -24,7 +24,6 @@
#include <linux/module.h>
#include <linux/sysdev.h>
#include <linux/nmi.h>

arch/x86_64/kernel/nmi.c +1 -15

-#include <linux/debugger.h>
 
 #include <asm/smp.h>
 #include <asm/mtrr.h>

@@ -318,26 +317,13 @@ void nmi_watchdog_tick (struct pt_regs *
 		return;
 
 	sum = read_pda(apic_timer_irqs);
-	if (atomic_read(&debugger_active)) {
-
-		/*
-		 * The machine is in debugger, hold this cpu if already
-		 * not held.
-		 */
-		debugger_nmihook(cpu, regs);
-		alert_counter[cpu] = 0;
-
-	} else if (last_irq_sums[cpu] == sum) {
-
+	if (last_irq_sums[cpu] == sum) {
 		/*
 		 * Ayiee, looks like this CPU is stuck ...
 		 * wait a few IRQs (5 seconds) before doing the oops ...
 		 */
 		alert_counter[cpu]++;
 		if (alert_counter[cpu] == 5*nmi_hz) {
-
-			CHK_DEBUGGER(2,SIGSEGV,0,regs,)
-
 			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT) == NOTIFY_BAD)
 { alert_counter[cpu] = 0;
 				return;

diff -zrupN linux-2.6.3+config+serial/arch/x86_64/kernel/traps.c linux-2.6.3+config+serial+sysrq+arch_hooks/arch/x86_64/kernel/traps.c ---
linux-2.6.3+config+serial/arch/x86_64/kernel/traps.c 2004-02-27
12:16:13.000000000 -0700 +++
linux-2.6.3+config+serial+sysrq+arch_hooks/arch/x86_64/kernel/traps.c 2004-
02-27 12:16:14.000000000 -0700 @@ -364,7 +364,6 @@ void __die(const char * str, struct pt_r

void die(const char * str, struct pt_regs * regs, long err) {

arch/x86_64/kernel/traps.c +0 -3

-	CHK_DEBUGGER(1,SIGTRAP,err,regs,)
 	oops_begin();
 	handle_BUG(regs);
 	__die(str, regs, err);

@@ -438,7 +437,6 @@ static void do_trap(int trapnr, int sign
 #define DO_ERROR(trapnr, signr, str, name) \
 asmlinkage void do_##name(struct pt_regs * regs, long error_code) \
 { \
-	CHK_DEBUGGER(1,SIGTRAP,error_code,regs,return) \
 	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) ==
 NOTIFY_BAD) \ return; \
 	do_trap(trapnr, signr, str, regs, error_code, NULL); \

@@ -646,7 +644,6 @@ asmlinkage void do_debug(struct pt_regs
 	force_sig_info(SIGTRAP, &info, tsk);
 clear_dr7:
 	asm volatile("movq %0,%%db7"::"r"(0UL));
-	CHK_DEBUGGER(1,SIGTRAP,error_code,regs,return)
 	notify_die(DIE_DEBUG, "debug", regs, error_code, 1, SIGTRAP);
 	return;
 

diff -zrupN linux-2.6.3+config+serial/arch/x86_64/mm/fault.c linux-2.6.3+config+serial+sysrq+arch_hooks/arch/x86_64/mm/fault.c ---
linux-2.6.3+config+serial/arch/x86_64/mm/fault.c 2004-02-27
12:16:13.000000000 -0700 +++
linux-2.6.3+config+serial+sysrq+arch_hooks/arch/x86_64/mm/fault.c 2004-02-2
7 12:10:49.000000000 -0700 @@ -23,7 +23,6 @@
#include <linux/vt_kern.h> /* For unblank_screen() */
#include <linux/compiler.h>
#include <linux/module.h>

arch/x86_64/mm/fault.c +0 -2

-#include <linux/debugger.h>
 
 #include <asm/system.h>
 #include <asm/uaccess.h>

@@ -408,7 +407,6 @@ no_context:
 
  	if (is_prefetch(regs, address))
  		return;
-	CHK_DEBUGGER(14, SIGSEGV,error_code, regs,)
 
 	if (is_errata93(regs, address))
 		return;

diff -zrupN linux-2.6.3+config+serial/include/asm-x86_64/calling.h linux-2.6.3+config+serial+sysrq+arch_hooks/include/asm-x86_64/calling.h ---
linux-2.6.3+config+serial/include/asm-x86_64/calling.h 2004-02-27
12:16:13.000000000 -0700 +++
linux-2.6.3+config+serial+sysrq+arch_hooks/include/asm-x86_64/calling.h 200
4-02-27 12:12:03.000000000 -0700 @@ -12,7 +12,7 @@
#define RBX 40
/* arguments: interrupts/non tracing syscalls only save upto here*/ #define R11 48

include/asm-x86_64/calling.h +1 -1

-#define R10 56
+#define R10 56
 #define R9 64
 #define R8 72
 #define RAX 80

Hello. The following adds SysRq-G. This is always configured in on CONFIG_KGDB && CONFIG_SYSRQ.

diff -zrupN linux-2.6.3+config+serial/drivers/char/sysrq.c linux-2.6.3+config+serial+sysrq+arch_hooks/drivers/char/sysrq.c

drivers/char/sysrq.c +21 -1

@@ -31,6 +31,7 @@
 #include <linux/suspend.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>		/* for fsync_bdev() */
+#include <linux/kgdb.h>			/* for breakpoint() */
 
 #include <linux/spinlock.h>
 

@@ -44,6 +45,25 @@ int sysrq_enabled = 1;
 /* Machine specific power off function */
 void (*sysrq_power_off)(void);
 
+/* Make a breakpoint() right now. */
+#ifdef CONFIG_KGDB
+#define  GDB_OP &kgdb_op
+static void kgdb_sysrq(int key, struct pt_regs *pt_regs, struct tty_struct *tty)
+{
+	printk("kgdb sysrq\n");
+	breakpoint();
+}
+
+static struct sysrq_key_op kgdb_op = {
+	.handler	= kgdb_sysrq,
+	.help_msg	= "kGdb|Fgdb",
+	.action_msg	= "Debug breakpoint\n",
+};
+
+#else
+#define  GDB_OP NULL
+#endif
+
 /* Loglevel sysrq handler */
 static void sysrq_handle_loglevel(int key, struct pt_regs *pt_regs,
 				  struct tty_struct *tty) 

@@ -239,7 +259,7 @@ static struct sysrq_key_op *sysrq_key_ta
 /* d */	NULL,
 /* e */	&sysrq_term_op,
 /* f */	NULL,
-/* g */	NULL,
+/* g */	GDB_OP,
 /* h */	NULL,
 /* i */	&sysrq_kill_op,
 /* j */	NULL,

There may be a need to change the keyboard driver for this. I have had trouble with it in the past. Seems it doesn't like to not get control back for a very long time. I am not sure what the problem is but, be warned.

-g

Quoted 51 lines · Tom Rini

Hello. The following adds SysRq-G. This is always configured in on CONFIG_KGDB && CONFIG_SYSRQ.

diff -zrupN linux-2.6.3+config+serial/drivers/char/sysrq.c linux-2.6.3+config+serial+sysrq+arch_hooks/drivers/char/sysrq.c ---
linux-2.6.3+config+serial/drivers/char/sysrq.c 2004-02-27
12:06:22.000000000 -0700 +++
linux-2.6.3+config+serial+sysrq+arch_hooks/drivers/char/sysrq.c 2004-02-2
7 12:16:14.000000000 -0700 @@ -31,6 +31,7 @@
#include <linux/suspend.h>
#include <linux/writeback.h>
#include <linux/buffer_head.h> /* for fsync_bdev() */

drivers/char/sysrq.c +20 -1

+#include <linux/kgdb.h>			/* for breakpoint() */
 
 #include <linux/spinlock.h>

@@ -44,6 +45,25 @@ int sysrq_enabled = 1;
 /* Machine specific power off function */
 void (*sysrq_power_off)(void);
 
+/* Make a breakpoint() right now. */
+#ifdef CONFIG_KGDB
+#define  GDB_OP &kgdb_op
+static void kgdb_sysrq(int key, struct pt_regs *pt_regs, struct
 tty_struct *tty) +{
+	printk("kgdb sysrq\n");
+	breakpoint();
+}
+
+static struct sysrq_key_op kgdb_op = {
+	.handler	= kgdb_sysrq,
+	.help_msg	= "kGdb|Fgdb",
+	.action_msg	= "Debug breakpoint\n",
+};
+
+#else
+#define  GDB_OP NULL
+#endif
+
 /* Loglevel sysrq handler */
 static void sysrq_handle_loglevel(int key, struct pt_regs *pt_regs,
 				  struct tty_struct *tty)

@@ -239,7 +259,7 @@ static struct sysrq_key_op *sysrq_key_ta
 /* d */	NULL,
 /* e */	&sysrq_term_op,
 /* f */	NULL,
-/* g */	NULL,
+/* g */	GDB_OP,
 /* h */	NULL,
 /* i */	&sysrq_kill_op,
 /* j */	NULL,

The following is my second attempt at making the serial driver more robust. This has the following changes:
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached.
- Don't try and look for a connection in put_packet, after we've tried to put a packet. Instead, when we receive a packet, GDB has connected. Also make put_packet look at the char it reads, and handle a new packet in the middle (or ^C+packet).
- Remove ok_packet(), excessive, IMHO.
- In kgdb8250_interrupt, only check if this is the right line, and if so, call kgdb_schedule_breakpoint(). This ends up killing off a lot of (incomplete due to my not testing SMP) locking.
- Remove a duplicate extern of kgdb_connected.

diff -zrupN linux-2.6.3+nothing/arch/ppc/8260_io/uart.c linux-2.6.3+config+serial/arch/ppc/8260_io/uart.c

arch/ppc/8260_io/uart.c +1 -7

@@ -396,16 +396,10 @@ static _INLINE_ void receive_chars(ser_i
 #ifdef CONFIG_KGDB
 		if (info->state->smc_scc_num == KGDB_SER_IDX) {
 			while (i-- > 0) {
-				if (*cp == 0x03) {
+				if (*cp == 0x03 || *cp == '$') {
 					breakpoint();
 					return;
 				}
-				if (*cp == '$') {
-					atomic_set(&kgdb_might_be_resumed, 1);
-					breakpoint();
-					atomic_set(&kgdb_might_be_resumed, 0);
-					return;
-				}
 				cp++;
 			}
 			bdp->cbd_sc |= BD_SC_EMPTY;

diff -zrupN linux-2.6.3+nothing/drivers/serial/kgdb_8250.c linux-2.6.3+config+serial/drivers/serial/kgdb_8250.c

drivers/serial/kgdb_8250.c +4 -73

@@ -17,7 +17,6 @@
 #include <linux/config.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
-#include <linux/spinlock.h>
 #include <linux/kgdb.h>
 #include <linux/interrupt.h>
 #include <linux/tty.h>

@@ -166,32 +165,15 @@ read_data_bfr(void)
 /*
  * Get a char if available, return -1 if nothing available.
  * Empty the receive buffer first, then look at the interface hardware.
- *
- * Locking here is a bit of a problem.	We MUST not lock out communication
- * if we are trying to talk to gdb about a kgdb entry.	ON the other hand
- * we can loose chars in the console pass thru if we don't lock.  It is also
- * possible that we could hold the lock or be waiting for it when kgdb
- * NEEDS to talk.  Since kgdb locks down the world, it does not need locks.
- * We do, of course have possible issues with interrupting a uart operation,
- * but we will just depend on the uart status to help keep that straight.
  */
 
-static spinlock_t uart_interrupt_lock = SPIN_LOCK_UNLOCKED;
-#ifdef CONFIG_SMP
-extern spinlock_t kgdb_spinlock;
-#endif
-
 int
 kgdb_get_debug_char(void)
 {
 	int retchr;
 	unsigned long flags;
 	local_irq_save(flags);
-#ifdef CONFIG_SMP
-	if (!spin_is_locked(&kgdb_spinlock)) {
-		spin_lock(&uart_interrupt_lock);
-	}
-#endif
+
 	/* intr routine has q'd chars */
 	if (atomic_read(&kgdb8250_buf_in_cnt) != 0) {
 		retchr = kgdb8250_buf[kgdb8250_buf_out_inx++];

@@ -205,11 +187,6 @@ kgdb_get_debug_char(void)
 	} while (retchr < 0);
 
 out:
-#ifdef CONFIG_SMP
-	if (!spin_is_locked(&kgdb_spinlock)) {
-		spin_unlock(&uart_interrupt_lock);
-	}
-#endif
 	local_irq_restore(flags);
 
 	return retchr;

@@ -217,72 +194,17 @@ out:
 
 /*
  * This is the receiver interrupt routine for the GDB stub.
- * It will receive a limited number of characters of input
- * from the gdb  host machine and save them up in a buffer.
- *
- * When kgdb_get_debug_char() is called it
- * draws characters out of the buffer until it is empty and
- * then reads directly from the serial port.
- *
- * We do not attempt to write chars from the interrupt routine
- * since the stubs do all of that via kgdb_put_debug_char() which
- * writes one byte after waiting for the interface to become
- * ready.
- *
- * The debug stubs like to run with interrupts disabled since,
- * after all, they run as a consequence of a breakpoint in
- * the kernel.
- *
- * Perhaps someone who knows more about the tty driver than I
- * care to learn can make this work for any low level serial
- * driver.
+ * All that we need to do is verify that the interrupt happened on the
+ * line we're in charge of.  If this is true, schedule a breakpoint and
+ * return.
  */
 static irqreturn_t
 kgdb8250_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 {
-	int chr, iir;
-	unsigned long flags;
-
 	if (irq != gdb_async_info.line)
 		return IRQ_NONE;
 
-	/* If we get an interrupt, then KGDB is trying to connect. */
-	if (!kgdb_connected) {
-		kgdb_schedule_breakpoint();
-		return IRQ_HANDLED;
-	}
-
-	local_irq_save(flags);
-	spin_lock(&uart_interrupt_lock);
-
-	do {
-		chr = read_data_bfr();
-		iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
-		if (chr < 0)
-			continue;
-
-		/* Check whether gdb sent interrupt */
-		if (chr == 3) {
-			breakpoint();
-			continue;
-		}
-
-		if (atomic_read(&kgdb8250_buf_in_cnt) >= GDB_BUF_SIZE) {
-			/* buffer overflow, clear it */
-			kgdb8250_buf_in_inx = 0;
-			atomic_set(&kgdb8250_buf_in_cnt, 0);
-			kgdb8250_buf_out_inx = 0;
-			break;

drivers/serial/kgdb_8250.c +0 -0

 	return IRQ_HANDLED;
 }
 

diff -zrupN linux-2.6.3+nothing/include/linux/kgdb.h linux-2.6.3+config+serial/include/linux/kgdb.h

include/linux/kgdb.h +0 -4

@@ -36,10 +36,6 @@ extern volatile int kgdb_connected;
 #endif
 
 extern atomic_t kgdb_setting_breakpoint;
-extern atomic_t kgdb_killed_or_detached;
-extern atomic_t kgdb_might_be_resumed;
-
-extern volatile int kgdb_connected;
 
 extern struct task_struct *kgdb_usethread, *kgdb_contthread;
 

diff -zrupN linux-2.6.3+nothing/kernel/kgdb.c linux-2.6.3+config+serial/kernel/kgdb.c

kernel/kgdb.c +16 -29

@@ -15,6 +15,7 @@
  * Copyright (C) 2002-2004 Timesys Corporation
  * Copyright (C) 2003-2004 Amit S. Kale
  * Copyright (C) 2004 Pavel Machek <[email protected]>
+ * Copyright (C) 2004 Tom Rini <[email protected]>
  *
  * Restructured KGDB for 2.6 kernels.
  * thread support, support for multiple processors,support for ia-32(x86) 

@@ -87,8 +88,6 @@ static const char hexchars[] = "01234567
 static spinlock_t slavecpulocks[KGDB_MAX_NO_CPUS];
 static volatile int procindebug[KGDB_MAX_NO_CPUS];
 atomic_t kgdb_setting_breakpoint;
-atomic_t kgdb_killed_or_detached;
-atomic_t kgdb_might_be_resumed;
 struct task_struct *kgdb_usethread, *kgdb_contthread;
 
 int debugger_step;

@@ -212,8 +211,10 @@ static void get_packet(char *buffer)
 	char ch;
 
 	do {
-		/* wait around for the start character, ignore all other characters */
-		while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$') ;
+		/* wait around for the start character, ignore all other
+		 * characters */
+		while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$')
+			;	/* Spin. */
 		kgdb_connected = 1;
 		checksum = 0;
 		xmitcsum = -1;

@@ -249,27 +250,22 @@ static void get_packet(char *buffer)
  * Send the packet in buffer.
  * Check for gdb connection if asked for.
  */
-static void put_packet(char *buffer, int checkconnect)
+static void put_packet(char *buffer)
 {
 	unsigned char checksum;
 	int count;
 	char ch;
-	static char gdbseq[] = "$Hc-1#09";
-	int i;
-	int send_count;
 
 	/*  $<packet info>#<checksum>. */
-	do {
+	while (1) {
 		kgdb_serial->write_char('$');
 		checksum = 0;
 		count = 0;
-		send_count = 0;
 
 		while ((ch = buffer[count])) {
 			kgdb_serial->write_char(ch);
 			checksum += ch;
 			count++;
-			send_count++;
 		}
 
 		kgdb_serial->write_char('#');

@@ -278,30 +274,27 @@ static void put_packet(char *buffer, int
 		if (kgdb_serial->flush)
 			kgdb_serial->flush();
 
-		i = 0;
-		while ((ch = kgdb_serial->read_char()) == gdbseq[i++] &&
-		       checkconnect) {
-			if (!gdbseq[i]) {
-				kgdb_serial->write_char('+');
-				if (kgdb_serial->flush)
-					kgdb_serial->flush();
-				breakpoint();
-
-				/*
-				 * GDB is available now.
-				 * Retransmit this packet.
-				 */
-				break;
-			}
-		}
-		if (checkconnect && ch == 3) {
-			kgdb_serial->write_char('+');
+		/* Now see what we get in reply. */
+		ch = kgdb_serial->read_char();
+
+		if (ch == 3)
+			ch = kgdb_serial->read_char();
+
+		/* If we get an ACK, we are done. */
+		if (ch == '+')
+			return;

kernel/kgdb.c +21 -40

+			return;
 		}
-	} while ((ch & 0x7f) != '+');
-
+	}
 }
 
 static int get_char(char *addr, unsigned char *data)

@@ -427,11 +420,6 @@ static inline void error_packet(char *pk
 	pkt[3] = '\0';
 }
 
-static void ok_packet(char *pkt)
-{
-	strcpy(pkt, "OK");
-}
-
 static char *pack_threadid(char *pkt, threadref * id)
 {
 	char *limit;

@@ -502,7 +490,8 @@ void kgdb_wait(struct pt_regs *regs)
 	procindebug[processor] = 1;
 	current->thread.debuggerinfo = regs;
 
-	/* Wait till master processor goes completely into the debugger. FIXME: this looks racy */
+	/* Wait till master processor goes completely into the debugger.
+	 * FIXME: this looks racy */
 	while (!procindebug[atomic_read(&debugger_active) - 1]) {
 		int i = 10;	/* an arbitrary number */
 

@@ -701,17 +690,7 @@ int kgdb_handle_exception(int exVector, 
 	/* Master processor is completely in the debugger */
 	kgdb_post_master_code(linux_regs, exVector, err_code);
 
-	if (atomic_read(&kgdb_killed_or_detached) &&
-	    atomic_read(&kgdb_might_be_resumed)) {
-		get_packet(remcom_in_buffer);
-		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 'c') {
-			remove_all_break();
-			atomic_set(&kgdb_killed_or_detached, 0);
-			ok_packet(remcom_out_buffer);
-		} else
-			return 1;
-	} else {
-
+	if (kgdb_connected) {
 		/* reply to host that an exception has occurred */
 		ptr = remcom_out_buffer;
 		*ptr++ = 'T';

@@ -721,11 +700,9 @@ int kgdb_handle_exception(int exVector, 
 		int_to_threadref(&thref, shadow_pid(current->pid));
 		ptr = pack_threadid(ptr, &thref);
 		*ptr++ = ';';
-		*ptr = '\0';
-	}
 
-	put_packet(remcom_out_buffer, 0);
-	kgdb_connected = 1;
+		put_packet(remcom_out_buffer);
+	}
 
 	kgdb_usethread = current;
 	kgdb_usethreadid = shadow_pid(current->pid);

@@ -741,6 +718,11 @@ int kgdb_handle_exception(int exVector, 
 
 		switch (remcom_in_buffer[0]) {
 		case '?':
+			/* We know that this packet is only sent
+			 * during initial connect.  So to be safe,
+			 * we clear out our breakpoints now incase
+			 * GDB is reconnecting. */
+			remove_all_break();
 			remcom_out_buffer[0] = 'S';
 			remcom_out_buffer[1] = hexchars[signo >> 4];
 			remcom_out_buffer[2] = hexchars[signo % 16];

@@ -798,7 +780,7 @@ int kgdb_handle_exception(int exVector, 
 			else {
 				gdb_regs_to_regs(gdb_regs, (struct pt_regs *)
 						 current->thread.debuggerinfo);
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 			}
 
 			break;

@@ -838,10 +820,10 @@ int kgdb_handle_exception(int exVector, 
 			if ((error = remove_all_break()) < 0) {
 				error_packet(remcom_out_buffer, error);
 			} else {
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 				kgdb_connected = 0;
 			}
-			put_packet(remcom_out_buffer, 0);
+			put_packet(remcom_out_buffer);
 			goto default_handle;
 
 		case 'k':

@@ -947,11 +929,10 @@ int kgdb_handle_exception(int exVector, 
 				}
 				kgdb_usethread = thread;
 				kgdb_usethreadid = threadid;
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 				break;
 
 			case 'c':
-				atomic_set(&kgdb_killed_or_detached, 0);
 				ptr = &remcom_in_buffer[2];
 				kgdb_hex2long(&ptr, &threadid);
 				if (!threadid) {

@@ -966,7 +947,7 @@ int kgdb_handle_exception(int exVector, 
 					}
 					kgdb_contthread = thread;
 				}
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 				break;
 			}
 			break;

@@ -977,7 +958,7 @@ int kgdb_handle_exception(int exVector, 
 			kgdb_hex2long(&ptr, &threadid);
 			thread = getthread(linux_regs, threadid);
 			if (thread)
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 			else
 				error_packet(remcom_out_buffer, -EINVAL);
 			break;

@@ -1018,7 +999,7 @@ int kgdb_handle_exception(int exVector, 
 			}
 
 			if (error == 0)
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 			else
 				error_packet(remcom_out_buffer, error);
 

@@ -1039,7 +1020,7 @@ int kgdb_handle_exception(int exVector, 
 		}		/* switch */
 
 		/* reply to the request */
-		put_packet(remcom_out_buffer, 0);
+		put_packet(remcom_out_buffer);
 	}
 
       kgdb_exit:

@@ -1063,7 +1044,6 @@ int kgdb_handle_exception(int exVector, 
 	}
 
 	/* Free debugger_active */
-	atomic_set(&kgdb_killed_or_detached, 1);
 	atomic_set(&debugger_active, 0);
 	local_irq_restore(flags);
 

@@ -1132,12 +1112,6 @@ void kgdb_entry(void)
 	/* Free debugger_active */
 	atomic_set(&debugger_active, 0);
 
-	/* This flag is used, if gdb has detached and wants to start
-	 * another session
-	 */
-	atomic_set(&kgdb_killed_or_detached, 1);
-	atomic_set(&kgdb_might_be_resumed, 0);
-
 	for (i = 0; i < MAX_BREAKPOINTS; i++)
 		kgdb_break[i].state = bp_disabled;
 

@@ -1222,7 +1196,7 @@ void kgdb_console_write(struct console *
 		*bufptr = '\0';
 		s += wcount;
 
-		put_packet(kgdbconbuf, 1);
+		put_packet(kgdbconbuf);
 
 	}
 	local_irq_restore(flags);

Couple of comments below...

Tom Rini wrote:

drivers/serial/kgdb_8250.c out: · line 285

Quoted 3 lines

drivers/serial/kgdb_8250.c +0 -3

-	spin_unlock(&uart_interrupt_lock);
-	local_irq_restore(flags);
-

I think you need at least this (especially in SMP, but works in all):

	char iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
        if(iir & UART_IIR_RDI){

drivers/serial/kgdb_8250.c out: · line 207

Quoted 1 line

drivers/serial/kgdb_8250.c +1 -0

+		kgdb_schedule_breakpoint();

	}

Quoted 1 line

	return IRQ_HANDLED;

Quoted 5 lines

kernel/kgdb.c +5 -0

+		/* If we get the start of another packet, this means
+		 * that GDB is attempting to reconnect.  We will NAK
+		 * the packet being sent, and stop trying to send this
+		 * packet. */
+		if (ch == '$') {

Review

Quoted 4 lines

kernel/kgdb.c +1 -0

+			kgdb_serial->write_char('-');

			if (kgdb_serial->flush)
				kgdb_serial->flush();

(quoted diff) +0 -1

-			breakpoint();

Flags go up in my mind here about recursion... What if we are already handling a breakpoint??? This may all be cool, but, as I said, alarms are ringing.

Quoted 3 lines · George Anzinger

Couple of comments below...

Tom Rini wrote:

[snip]

Quoted 8 lines

drivers/serial/kgdb_8250.c out: · line 285

Quoted 3 lines

drivers/serial/kgdb_8250.c +0 -3

-	spin_unlock(&uart_interrupt_lock);
-	local_irq_restore(flags);
-

I think you need at least this (especially in SMP, but works in all):

	char iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
       if(iir & UART_IIR_RDI){

Quoted 5 lines

drivers/serial/kgdb_8250.c out: · line 207

Quoted 1 line

drivers/serial/kgdb_8250.c +1 -0

+		kgdb_schedule_breakpoint();

	}

Quoted 1 line

	return IRQ_HANDLED;

Would this be to ensure that we only schedule one breakpoint not 2?

[snip]

Quoted 11 lines

Review

Quoted 9 lines

kernel/kgdb.c +6 -0

+		/* If we get the start of another packet, this means
+		 * that GDB is attempting to reconnect.  We will NAK
+		 * the packet being sent, and stop trying to send this
+		 * packet. */
+		if (ch == '$') {
+			kgdb_serial->write_char('-');

			if (kgdb_serial->flush)
				kgdb_serial->flush();

(quoted diff) +0 -1

-			breakpoint();

Flags go up in my mind here about recursion... What if we are already handling a breakpoint??? This may all be cool, but, as I said, alarms are ringing.

There's two cases here, IMHO. If GDB is connected, the only time we'll get a '$' when we're sending a packet is if we're out-of-sync (in regular gdb/kgdb communication) or we're prempting a console message (i.e. we're trying to send something while gdb wants to break in).

the latter case, it's OK to give up on the console packet (it's not critical) and trying to send a console message during a breakpoint is asking for trouble.

communication, what will happen w/o this change is that we get stuck in a "Packet instead of ACK" loop on gdb, and we get stuck trying to
transmit that same packet on the kgdb side. I think that if we call
breakpoint() here we can try and start over...

The other case is what the comment describes, and in that case we quite quickly get to the ponit of acting like gdb is connected again.

Quoted 21 lines · Tom Rini

Quoted 3 lines · George Anzinger

Couple of comments below...

Tom Rini wrote:

[snip]

Quoted 12 lines

drivers/serial/kgdb_8250.c out: · line 285

Quoted 3 lines

drivers/serial/kgdb_8250.c +0 -3

-	spin_unlock(&uart_interrupt_lock);
-	local_irq_restore(flags);
-

I think you need at least this (especially in SMP, but works in all):

	char iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
      if(iir & UART_IIR_RDI){

drivers/serial/kgdb_8250.c out: · line 207

Quoted 1 line

drivers/serial/kgdb_8250.c +1 -0

+		kgdb_schedule_breakpoint();

	}

Quoted 4 lines

Quoted 1 line

Quoted 1 line

	return IRQ_HANDLED;

Would this be to ensure that we only schedule one breakpoint not 2?

Well, that too, but the notion is to take care of an interrupt on cpu 1 while doing console output on cpu 0. If cpu 0 doesn't grab the '+' fast enough to prevent the interrupt cpu 1 may get an interrupt with no characters in the fifo. This code just ignores that.

Quoted 17 lines

[snip]

Quoted 5 lines

Quoted 5 lines

kernel/kgdb.c +5 -0

+		/* If we get the start of another packet, this means
+		 * that GDB is attempting to reconnect.  We will NAK
+		 * the packet being sent, and stop trying to send this
+		 * packet. */
+		if (ch == '$') {

Quoted 8 lines

Review

Quoted 4 lines

kernel/kgdb.c +1 -0

+			kgdb_serial->write_char('-');

			if (kgdb_serial->flush)
				kgdb_serial->flush();

(quoted diff) +0 -1

-			breakpoint();

Flags go up in my mind here about recursion... What if we are already handling a breakpoint??? This may all be cool, but, as I said, alarms are ringing.

Quoted 4 lines

There's two cases here, IMHO. If GDB is connected, the only time we'll get a '$' when we're sending a packet is if we're out-of-sync (in regular gdb/kgdb communication) or we're prempting a console message (i.e. we're trying to send something while gdb wants to break in).

I am a little unclear about this. Assuming that the user has not been so dumb as to put a breakpoint in kgdb's console handling code, I suspect that the entry code should allow the console message to complete prior to sending the first message to gdb.

I made a rather lame attempt to do this
in the -mm kgdb. What I think should happen is that, on entry kgdb should send an nmi to all but self. The kgdb nmi code (at in_kgdb() in the -mm
patch) should check to see if the CURRENT cpu is in the kgdb console code
and, if so, set a flag for the console code that a kgdb entry is pending and then return. The console code should check this flag on exit, after clearing the "i am in console" flag and if set, do a send nmi self. This would allow the console code to complete prior to the kgdb entry while still rounding up all the other cpus with the nmi.

Quoted 5 lines

If things get out-of-sync in normal gdb/kgdb
communication, what will happen w/o this change is that we get stuck in a "Packet instead of ACK" loop on gdb, and we get stuck trying to
transmit that same packet on the kgdb side. I think that if we call
breakpoint() here we can try and start over...

The problem is that you are now doing a breakpoint from inside kgdb while handling a prior breakpoint.

At the very least you would need to consider
very carefully what happens after the breakpoint. It is not clear that you can recover from this condition... but you may be able to look around.

Quoted 20 lines · George Anzinger

Quoted 19 lines · Tom Rini

Quoted 3 lines · George Anzinger

Couple of comments below...

Tom Rini wrote:

[snip]

Quoted 12 lines

drivers/serial/kgdb_8250.c out: · line 285

Quoted 3 lines

drivers/serial/kgdb_8250.c +0 -3

-	spin_unlock(&uart_interrupt_lock);
-	local_irq_restore(flags);
-

I think you need at least this (especially in SMP, but works in all):

	char iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
      if(iir & UART_IIR_RDI){

drivers/serial/kgdb_8250.c out: · line 207

Quoted 1 line

drivers/serial/kgdb_8250.c +1 -0

+		kgdb_schedule_breakpoint();

	}

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

	return IRQ_HANDLED;

Would this be to ensure that we only schedule one breakpoint not 2?

Well, that too, but the notion is to take care of an interrupt on cpu 1 while doing console output on cpu 0. If cpu 0 doesn't grab the '+' fast enough to prevent the interrupt cpu 1 may get an interrupt with no characters in the fifo. This code just ignores that.

OK, done.

Quoted 21 lines

Quoted 16 lines

[snip]

Quoted 5 lines

Quoted 5 lines

kernel/kgdb.c +5 -0

+		/* If we get the start of another packet, this means
+		 * that GDB is attempting to reconnect.  We will NAK
+		 * the packet being sent, and stop trying to send this
+		 * packet. */
+		if (ch == '$') {

Quoted 8 lines

Review

Quoted 4 lines

kernel/kgdb.c +1 -0

+			kgdb_serial->write_char('-');

			if (kgdb_serial->flush)
				kgdb_serial->flush();

(quoted diff) +0 -1

-			breakpoint();

Flags go up in my mind here about recursion... What if we are already handling a breakpoint??? This may all be cool, but, as I said, alarms are ringing.

Quoted 4 lines

There's two cases here, IMHO. If GDB is connected, the only time we'll get a '$' when we're sending a packet is if we're out-of-sync (in regular gdb/kgdb communication) or we're prempting a console message (i.e. we're trying to send something while gdb wants to break in).

Quoted 4 lines

I am a little unclear about this. Assuming that the user has not been so dumb as to put a breakpoint in kgdb's console handling code, I suspect that the entry code should allow the console message to complete prior to sending the first message to gdb.

I don't think so. If you don't break out of put_packet(), kgdb will try to send the packet (console or for a previous, now dead, session) forever. And gdb will keep trying to send it's first packet, timing out and moving on.

Quoted 9 lines

I made a rather lame attempt to do this
in the -mm kgdb. What I think should happen is that, on entry kgdb should send an nmi to all but self. The kgdb nmi code (at in_kgdb() in the -mm patch) should check to see if the CURRENT cpu is in the kgdb console code and, if so, set a flag for the console code that a kgdb entry is pending and then return. The console code should check this flag on exit, after clearing the "i am in console" flag and if set, do a send nmi self. This would allow the console code to complete prior to the kgdb entry while still rounding up all the other cpus with the nmi.

IMHO, that's awfully complex for something we can just not skip out on.

Quoted 8 lines

Quoted 5 lines

If things get out-of-sync in normal gdb/kgdb
communication, what will happen w/o this change is that we get stuck in a "Packet instead of ACK" loop on gdb, and we get stuck trying to
transmit that same packet on the kgdb side. I think that if we call
breakpoint() here we can try and start over...

The problem is that you are now doing a breakpoint from inside kgdb while handling a prior breakpoint.

Only in the case where we aren't out-of-sync, but gdb died / disconnected and we then hit a breakpoint.

Quoted 4 lines

At the very least you would need to consider
very carefully what happens after the breakpoint. It is not clear that you can recover from this condition... but you may be able to look around.

I don't follow you here. I agree that in the case of hitting a breakpoint while gdb isn't connected isn't necessarily clean, but since gdb didn't go away cleanly, and there isn't a way for it to tell us it's trying to recover, I'm not sure what we can do aside from clear out the existing breakpoints (just like we could have done, if gdb was still connected) and keep going. We should document that this particular behavior might not be a good thing, but it's just that one corner case of things (disconnect is fine, detach/reattach is fine, heck even gdb dying and not hitting a breakpoint is fine).

This patch broke Ctrl+C response on my system. Fix as follows With this fix I can kill or detach gdb with pending console messages and then connect a new gdb.

Is following fix ok to checkin? (interdiff)

(quoted diff) +4 -4

@@ -412,11 +418,11 @@
 +              /* The user has selected one of ttyS[0-3], which we pull
 +               * from rs_table[].  If this doesn't exist, user error. */
 +              gdb_async_info.port = gdb_async_info.state->port =
-+                  rs_table[KGDB_PORT].port;
++                  rs_table[kgdb8250_ttyS].port;
 +              gdb_async_info.line = gdb_async_info.state->irq =
-+                  rs_table[KGDB_PORT].irq;
-+              gdb_async_info.state->io_type = rs_table[KGDB_PORT].io_type;
-+              reg_shift = rs_table[KGDB_PORT].iomem_reg_shift;
++                  rs_table[kgdb8250_ttyS].irq;
++              gdb_async_info.state->io_type = 
 rs_table[kgdb8250_ttyS].io_type;
++              reg_shift = rs_table[kgdb8250_ttyS].iomem_reg_shift;
 +      }
 +

+ switch (gdb_async_info.state->io_type) {

Hello. The following patch moves all of the config options into one file, kernel/Kconfig.kgdb.

diff -zrupN linux-2.6.3+nothing/arch/i386/Kconfig linux-2.6.3+config+serial/arch/i386/Kconfig

arch/i386/Kconfig +1 -30

@@ -1253,36 +1253,7 @@ config DEBUG_SPINLOCK_SLEEP
 	  If you say Y here, various routines which may sleep will become very
 	  noisy if they are called with a spinlock held.	
 
-config KGDB
-	bool "KGDB: kernel debugging with remote gdb"
-	depends on DEBUG_KERNEL
-	select DEBUG_INFO
-	select FRAME_POINTER
-	help
-	  If you say Y here, it will be possible to remotely debug the
-	  kernel using gdb. This enlarges your kernel image disk size by
-	  several megabytes and requires a machine with more than 128 MB
-	  RAM to avoid excessive linking time. 
-	  Documentation of kernel debugger available at
-	  http://kgdb.sourceforge.net
-	  This is only useful for kernel hackers. If unsure, say N.
-
-config KGDB_THREAD
-	bool "KGDB: Thread analysis"
-	depends on KGDB
-	help
-	  With thread analysis enabled, gdb can talk to kgdb stub to list
-	  threads and to get stack trace for a thread. This option also enables
-	  some code which helps gdb get exact status of thread. Thread analysis
-	  adds some overhead to schedule and down functions. You can disable
-	  this option if you do not want to compromise on speed.
-
-config KGDB_CONSOLE
-	bool "KGDB: Console messages through gdb"
-	depends on KGDB
-	help
-	  If you say Y here, console messages will appear through gdb.
-	  Other consoles such as tty or ttyS will continue to work as usual.
+source "kernel/Kconfig.kgdb"
 
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"

diff -zrupN linux-2.6.3+nothing/arch/ppc/Kconfig linux-2.6.3+config+serial/arch/ppc/Kconfig

arch/ppc/Kconfig +1 -46

@@ -1170,52 +1170,7 @@ config DEBUG_SPINLOCK_SLEEP
 	  If you say Y here, various routines which may sleep will become very
 	  noisy if they are called with a spinlock held.
 
-config KGDB
-	bool "Include kgdb kernel debugger"
-	depends on DEBUG_KERNEL
-	select DEBUG_INFO
-	select FRAME_POINTER
-	help
-	  Include in-kernel hooks for kgdb, the Linux kernel source level
-	  debugger.  See <http://kgdb.sourceforge.net/> for more information.
-	  Unless you are intending to debug the kernel, say N here.
-
-choice
-	prompt "Serial Port"
-	depends on KGDB
-	default KGDB_TTYS1
-
-config KGDB_TTYS0
-	bool "ttyS0"
-
-config KGDB_TTYS1
-	bool "ttyS1"
-
-config KGDB_TTYS2
-	bool "ttyS2"
-
-config KGDB_TTYS3
-	bool "ttyS3"
-
-endchoice
-
-config KGDB_THREAD
-	bool "KGDB: Thread analysis"
-	depends on KGDB
-	help
-	  With thread analysis enabled, gdb can talk to kgdb stub to list
-	  threads and to get stack trace for a thread. This option also enables
-	  some code which helps gdb get exact status of thread. Thread analysis
-	  adds some overhead to schedule and down functions. You can disable
-	  this option if you do not want to compromise on speed.
-
-config KGDB_CONSOLE
-	bool "Enable serial console thru kgdb port"
-	depends on KGDB && 8xx || 8260
-	help
-	  If you enable this, all serial console messages will be sent
-	  over the gdb stub.
-	  If unsure, say N.
+source "kernel/Kconfig.kgdb"
 
 config XMON
 	bool "Include xmon kernel debugger"

diff -zrupN linux-2.6.3+nothing/arch/x86_64/Kconfig linux-2.6.3+config+serial/arch/x86_64/Kconfig

arch/x86_64/Kconfig +1 -31

@@ -465,37 +465,7 @@ config IOMMU_LEAK
          Add a simple leak tracer to the IOMMU code. This is useful when you
 	 are debugging a buggy device driver that leaks IOMMU mappings.
 
-config KGDB
-	bool "KGDB: kernel debugging with remote gdb"
-	depends on DEBUG_KERNEL
-	select DEBUG_INFO
-	select FRAME_POINTER
-	help
-	  If you say Y here, it will be possible to remotely debug the
-	  kernel using gdb. This enlarges your kernel image disk size by
-	  several megabytes and requires a machine with more than 128 MB
-	  RAM to avoid excessive linking time. 
-	  Documentation of kernel debugger available at
-	  http://kgdb.sourceforge.net
-	  This is only useful for kernel hackers. If unsure, say N.
-
-config KGDB_THREAD
-	bool "KGDB: Thread analysis"
-	depends on KGDB
-	help
-	  With thread analysis enabled, gdb can talk to kgdb stub to list
-	  threads and to get stack trace for a thread. This option also enables
-	  some code which helps gdb get exact status of thread. Thread analysis
-	  adds some overhead to schedule and down functions. You can disable
-	  this option if you do not want to compromise on speed.
-
-config KGDB_CONSOLE
-	bool "KGDB: Console messages through gdb"
-	depends on KGDB
-	help
-	  If you say Y here, console messages will appear through gdb.
-	  Other consoles such as tty or ttyS will continue to work as usual.
-
+source "kernel/Kconfig.kgdb"
 endmenu
 
 source "security/Kconfig"

diff -zrupN linux-2.6.3+nothing/drivers/net/Kconfig linux-2.6.3+config+serial/drivers/net/Kconfig

drivers/net/Kconfig +0 -6

@@ -187,12 +187,6 @@ config NET_ETHERNET
 	  Note that the answer to this question won't directly affect the
 	  kernel: saying N will just cause the configurator to skip all
 	  the questions about Ethernet network cards. If unsure, say N.
-	
-config KGDB_ETH
-	bool "KGDB: On ethernet"
-	depends on KGDB
-	help
-	  Uses ethernet interface for kgdb.
 
 config MII
 	tristate "Generic Media Independent Interface device support"

diff -zrupN linux-2.6.3+nothing/drivers/serial/Kconfig linux-2.6.3+config+serial/drivers/serial/Kconfig

drivers/serial/Kconfig +0 -28

@@ -6,34 +6,6 @@
 
 menu "Serial drivers"
 
-config KGDB_8250
-	bool "KGDB: On generic serial port (8250)"
-	depends on KGDB
-	help
-	  Uses generic serial port (8250) for kgdb. This is independent of the
-	  option 9250/16550 and compatible serial port.
-
-config KGDB_PORT
-	hex "hex I/O port address of the debug serial port"
-	depends on KGDB_8250
-	default  3f8
-	help
-	  Some systems (x86 family at this writing) allow the port
-	  address to be configured.  The number entered is assumed to be
-	  hex, don't put 0x in front of it.  The standard address are:
-	  COM1 3f8 , irq 4 and COM2 2f8 irq 3.  Setserial /dev/ttySx
-	  will tell you what you have.  It is good to test the serial
-	  connection with a live system before trying to debug.
-
-config KGDB_IRQ
-	int "IRQ of the debug serial port"
-	depends on KGDB_8250
-	default 4
-	help
-	  This is the irq for the debug port.  If everything is working
-	  correctly and the kernel has interrupts on a control C to the
-	  port should cause a break into the kernel debug stub.
-
 #
 # The new 8250/16550 serial drivers
 config SERIAL_8250

diff -zrupN linux-2.6.3+nothing/kernel/Kconfig.kgdb linux-2.6.3+config+serial/kernel/Kconfig.kgdb

kernel/Kconfig.kgdb +43 -0

+
+choice
+	prompt "Method for KGDB communication"
+	depends on KGDB
+	default PPC_SIMPLE_SERIAL if PPC32 && (8xx || 8260)
+	default KGDB_8250
+	help
+	  There are a number of different ways in which you can communicate
+	  with KGDB.  The oldest is using a serial driver.  A newer method
+	  is to use UDP packets and a special network driver.
+
+config KGDB_8250
+	bool "KGDB: On generic serial port (8250)"
+	help
+	  Uses generic serial port (8250) for kgdb. This is independent of the
+	  option 9250/16550 and compatible serial port.
+
+config KGDB_ETH
+	bool "KGDB: On ethernet"
+	select NETPOLL
+	select NETPOLL_TRAP
+	select NETPOLL_RX
+	help
+	  Uses ethernet interface for kgdb.
+
+config PPC_SIMPLE_SERIAL 
+	bool "KGDB: On any serial port"
+	depends on PPC32
+	help
+	  Use a very simple, and not necessarily feature complete serial
+	  driver.  This is the only serial option currently for MPC8xx or
+	  MPC82xx based ports that do not offer an 8250-style UART.
+
+endchoice
+
+config KGDB_SIMPLE_SERIAL
+	bool "Simple selection of KGDB serial port"
+	depends on KGDB_8250 || PPC_SIMPLE_SERIAL
+	help
+	  If you say Y here, you will only have to pick the baud rate
+	  and serial port (ttyS) that you wish to use for KGDB.  If you
+	  say N, you will have provide the I/O port and IRQ number.  Note
+	  that if your serial ports are iomapped, then you must say Y here.

kernel/Kconfig.kgdb +46 -0

+
+config KGDB_9600BAUD
+	bool "9600"
+
+config KGDB_19200BAUD
+	bool "19200"
+
+config KGDB_38400BAUD
+	bool "38400"
+
+config KGDB_57600BAUD
+	bool "57600"
+
+config KGDB_115200BAUD
+	bool "115200"
+endchoice
+
+choice
+	prompt "Serial port for KGDB"
+	depends on KGDB_SIMPLE_SERIAL
+	default KGDB_TTYS0
+
+config KGDB_TTYS0
+	bool "ttyS0"
+
+config KGDB_TTYS1
+	bool "ttyS1"
+
+config KGDB_TTYS2
+	bool "ttyS2"
+
+config KGDB_TTYS3
+	bool "ttyS3"
+
+endchoice
+
+config KGDB_PORT
+	hex "hex I/O port address of the debug serial port"
+	depends on !KGDB_SIMPLE_SERIAL && (KGDB_8250 || PPC_SIMPLE_SERIAL)
+	default  3f8
+	help
+	  Some systems (x86 family at this writing) allow the port
+	  address to be configured.  The number entered is assumed to be
+	  hex, don't put 0x in front of it.  The standard address are:
+	  COM1 3f8 , irq 4 and COM2 2f8 irq 3.  Setserial /dev/ttySx
+	  will tell you what you have.  It is good to test the serial

Comments below...

kernel/Kconfig.kgdb line 68

Quoted 8 lines

kernel/Kconfig.kgdb +8 -0

+choice
+	depends on KGDB_8250 || PPC_SIMPLE_SERIAL
+    	prompt "Debug serial port BAUD"
+	default KGDB_115200BAUD
+	help
+	  Gdb and the kernel stub need to agree on the baud rate to be
+	  used.  Some systems (x86 family at this writing) allow this to
+	  be configured.

Should this depend on the archs that can use it?

kernel/Kconfig.kgdb line 124

Quoted 8 lines

kernel/Kconfig.kgdb +8 -0

+config KGDB_IRQ
+	int "IRQ of the debug serial port"
+	depends on !KGDB_SIMPLE_SERIAL && (KGDB_8250 || PPC_SIMPLE_SERIAL)
+	default 4
+	help
+	  This is the irq for the debug port.  If everything is working
+	  correctly and the kernel has interrupts on a control C to the
+	  port should cause a break into the kernel debug stub.

I think setserial will also spit this out.

Quoted 6 lines

kernel/Kconfig.kgdb +6 -0

+config KGDB_THREAD
+	bool "KGDB: Thread analysis"
+	depends on KGDB
+	help
+	  With thread analysis enabled, gdb can talk to kgdb stub to list
+	  threads and to get stack trace for a thread. This option also 

kernel/Kconfig.kgdb line 134

Quoted 5 lines

enables

(quoted diff) +1 -0

+	  some code which helps gdb get exact status of thread. Thread 

analysis

kernel/Kconfig.kgdb +2 -0

+	  adds some overhead to schedule and down functions. You can disable
+	  this option if you do not want to compromise on speed.

Lets remove the overhead and eliminate the need for this option in favor of always having threads. Works in the mm kgdb...

kernel/Kconfig.kgdb line 141

Quoted 6 lines

kernel/Kconfig.kgdb +6 -0

+config KGDB_CONSOLE
+	bool "KGDB: Console messages through gdb"
+	depends on KGDB
+	help
+	  If you say Y here, console messages will appear through gdb.
+	  Other consoles such as tty or ttyS will continue to work as usual.

Might want to warn them of the need to connect prior to the first message from the kernel....

Quoted 6 lines

kernel/Kconfig.kgdb +6 -0

+config KGDB_THREAD
+	bool "KGDB: Thread analysis"
+	depends on KGDB
+	help
+	  With thread analysis enabled, gdb can talk to kgdb stub to list
+	  threads and to get stack trace for a thread. This option also 

kernel/Kconfig.kgdb line 134

Quoted 5 lines

enables

(quoted diff) +1 -0

+	  some code which helps gdb get exact status of thread. Thread 

analysis

kernel/Kconfig.kgdb +2 -0

+	  adds some overhead to schedule and down functions. You can disable
+	  this option if you do not want to compromise on speed.

Lets remove the overhead and eliminate the need for this option in favor of always having threads. Works in the mm kgdb...

Hi!

Quoted 5 lines

Quoted 5 lines

kernel/Kconfig.kgdb +5 -0

+config KGDB_THREAD
+	bool "KGDB: Thread analysis"
+	depends on KGDB
+	help
+	  With thread analysis enabled, gdb can talk to kgdb stub to list

Quoted 8 lines

kernel/Kconfig.kgdb line 134

Quoted 6 lines

(quoted diff) +1 -0

+	  threads and to get stack trace for a thread. This option also 

enables

(quoted diff) +1 -0

+	  some code which helps gdb get exact status of thread. Thread 

analysis

kernel/Kconfig.kgdb +2 -0

+	  adds some overhead to schedule and down functions. You can disable
+	  this option if you do not want to compromise on speed.

Lets remove the overhead and eliminate the need for this option in favor of always having threads. Works in the mm kgdb...

No. Thread analysis is unsuitable for the mainline (manipulates sched.c in ugly way). It may be okay for -mm, but in such case it should better be separated.

Quoted 17 lines · Pavel Machek

Hi!

Quoted 14 lines

kernel/Kconfig.kgdb line 134

Quoted 11 lines

kernel/Kconfig.kgdb +6 -0

+config KGDB_THREAD
+	bool "KGDB: Thread analysis"
+	depends on KGDB
+	help
+	  With thread analysis enabled, gdb can talk to kgdb stub to list
+	  threads and to get stack trace for a thread. This option also

enables

(quoted diff) +1 -0

+	  some code which helps gdb get exact status of thread. Thread

analysis

kernel/Kconfig.kgdb +2 -0

+	  adds some overhead to schedule and down functions. You can disable
+	  this option if you do not want to compromise on speed.

Lets remove the overhead and eliminate the need for this option in favor of always having threads. Works in the mm kgdb...

Quoted 3 lines

No. Thread analysis is unsuitable for the mainline (manipulates sched.c in ugly way). It may be okay for -mm, but in such case it should better be separated.

Not in the -mm version. I agree that sched.c should NEVER be treated this way and it is not in the -mm version. I also think that, most of the time, it is useful to have the thread stuff, but that may be just my usage...

Quoted 24 lines · George Anzinger

Quoted 21 lines · Pavel Machek

Hi!

Quoted 6 lines

Quoted 6 lines

kernel/Kconfig.kgdb +6 -0

+config KGDB_THREAD
+	bool "KGDB: Thread analysis"
+	depends on KGDB
+	help
+	  With thread analysis enabled, gdb can talk to kgdb stub to list
+	  threads and to get stack trace for a thread. This option also

Quoted 8 lines

kernel/Kconfig.kgdb line 134

Quoted 5 lines

enables

(quoted diff) +1 -0

+	  some code which helps gdb get exact status of thread. Thread

analysis

kernel/Kconfig.kgdb +2 -0

+	  adds some overhead to schedule and down functions. You can disable
+	  this option if you do not want to compromise on speed.

Lets remove the overhead and eliminate the need for this option in favor of always having threads. Works in the mm kgdb...

No. Thread analysis is unsuitable for the mainline (manipulates sched.c in ugly way). It may be okay for -mm, but in such case it should better be separated.

Quoted 3 lines

Not in the -mm version. I agree that sched.c should NEVER be treated this way and it is not in the -mm version. I also think that, most of the time, it is useful to have the thread stuff, but that may be just my usage...

If threads stuff didn't introduce any unclean code changes, I too would prefer to have it on all the time. As things stands, threads stuff is rather intrusive.

Quoted 23 lines · Amit S. Kale

Quoted 22 lines · George Anzinger

Quoted 20 lines · Pavel Machek

Hi!

Quoted 6 lines

Quoted 6 lines

kernel/Kconfig.kgdb +6 -0

+config KGDB_THREAD
+	bool "KGDB: Thread analysis"
+	depends on KGDB
+	help
+	  With thread analysis enabled, gdb can talk to kgdb stub to list
+	  threads and to get stack trace for a thread. This option also

Quoted 8 lines

kernel/Kconfig.kgdb line 134

Quoted 5 lines

enables

(quoted diff) +1 -0

+	  some code which helps gdb get exact status of thread. Thread

analysis

kernel/Kconfig.kgdb +2 -0

+	  adds some overhead to schedule and down functions. You can disable
+	  this option if you do not want to compromise on speed.

Lets remove the overhead and eliminate the need for this option in favor of always having threads. Works in the mm kgdb...

No. Thread analysis is unsuitable for the mainline (manipulates sched.c in ugly way). It may be okay for -mm, but in such case it should better be separated.

Quoted 8 lines

Quoted 4 lines

Not in the -mm version. I agree that sched.c should NEVER be treated this way and it is not in the -mm version. I also think that, most of the time, it is useful to have the thread stuff, but that may be just my usage...

If threads stuff didn't introduce any unclean code changes, I too would prefer to have it on all the time. As things stands, threads stuff is rather intrusive.

Lets put the threads stuff in the stub. The only stuff we need in the kernel is the flag that indicateds that the pid hash table has been initialized.

Meanwhile, I would like to make a change to the gdb "info thread" command to do a better job of displaying the threads. Here is what I am proposing:

Gdb would work as it does now if the following set is not done.

A new "set thread_level" command that would take the "bt" level to use on the thread display.
A new "set thread_limits command that would take two expressions that would reduce to two memory addresses.

Which ever of these is entered last will be active and used by "info thread" as follows:

if thread_level is active gdb will do the indicated number of "up" operations and display the result on the info thread line for that thread (note there is other info on this line that will not be changed).

if thread_limits is active gdb will do 0 or more "up" commands until the resultant PC is NOT between the given limits.

The kernel, at this time, has defined symbols for the thread_limits command (it is used in the kernel for its internal display of threads). I would expect that the thread_level version would be the answer for theaded application programs.

Daniel, how does this sound?

Quoted 51 lines · George Anzinger

Quoted 26 lines · Amit S. Kale

Quoted 24 lines · George Anzinger

Quoted 21 lines · Pavel Machek

Hi!

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+config KGDB_THREAD
+	bool "KGDB: Thread analysis"
+	depends on KGDB
+	help
+	  With thread analysis enabled, gdb can talk to kgdb stub to list
+	  threads and to get stack trace for a thread. This option also

Quoted 8 lines

Review

Quoted 5 lines

enables

(quoted diff) +1 -0

+	  some code which helps gdb get exact status of thread. Thread

analysis

(quoted diff) +2 -0

+	  adds some overhead to schedule and down functions. You can disable
+	  this option if you do not want to compromise on speed.

Lets remove the overhead and eliminate the need for this option in favor of always having threads. Works in the mm kgdb...

No. Thread analysis is unsuitable for the mainline (manipulates sched.c in ugly way). It may be okay for -mm, but in such case it should better be separated.

Quoted 8 lines

Quoted 4 lines

Not in the -mm version. I agree that sched.c should NEVER be treated this way and it is not in the -mm version. I also think that, most of the time, it is useful to have the thread stuff, but that may be just my usage...

If threads stuff didn't introduce any unclean code changes, I too would prefer to have it on all the time. As things stands, threads stuff is rather intrusive.

Lets put the threads stuff in the stub. The only stuff we need in the kernel is the flag that indicateds that the pid hash table has been initialized.

Meanwhile, I would like to make a change to the gdb "info thread" command to do a better job of displaying the threads. Here is what I am proposing:

Gdb would work as it does now if the following set is not done.

A new "set thread_level" command that would take the "bt" level to use on the thread display.
A new "set thread_limits command that would take two expressions that would reduce to two memory addresses.

Quoted 6 lines

Which ever of these is entered last will be active and used by "info thread" as follows:

if thread_level is active gdb will do the indicated number of "up" operations and display the result on the info thread line for that thread (note there is other info on this line that will not be changed).

You can already do a backtrace on all threads using gdb command "thread apply all backtrace".

Quoted 2 lines

if thread_limits is active gdb will do 0 or more "up" commands until the resultant PC is NOT between the given limits.

How does a user specify PC? There are umpteen number of kernel entry points (irqs, exceptions, system calls).

Quoted 7 lines

The kernel, at this time, has defined symbols for the thread_limits command (it is used in the kernel for its internal display of threads). I would expect that the thread_level version would be the answer for theaded application programs.

Quoted 1 line

Daniel, how does this sound?

The problem with kernel backtraces not stopping at kernel entry points is a tough one. gdbmod at kgdb.sourceforge.net attempts to do that. This gdb detects if we are debugging a kernel. If we are, a few things kick in like scanning of modules instead of .so libraries and stopping backtraces earlier.

GDB uses main as the function where backtraces stop unless overridden. This is broken by definition for multithreaded programs becauses non initial threads don't start from main. Kernel too doesn't have main and has several entry points.

If there is a way for gdb to know entry points from the kernel, it would be very easy to maintain. Say a .entrypoint section that lists pc ranges of entry points. GDB then stop a backtrace as soon as it enters one of these ranges.

Hello. The following patch moves all of the config options into one file, kernel/Kconfig.kgdb.
<snip>

Hello. The following patch moves all of the config options into one file, kernel/Kconfig.kgdb.

diff -zrupN linux-2.6.3+nothing/arch/i386/Kconfig linux-2.6.3+config+serial/arch/i386/Kconfig ---
linux-2.6.3+nothing/arch/i386/Kconfig 2004-02-27 12:16:14.296187607 -0700
+++ linux-2.6.3+config+serial/arch/i386/Kconfig 2004-02-27
12:16:13.707320867 -0700 @@ -1253,36 +1253,7 @@ config DEBUG_SPINLOCK_SLEEP

arch/i386/Kconfig +1 -30

 	  If you say Y here, various routines which may sleep will become very
 	  noisy if they are called with a spinlock held.
 
-config KGDB
-	bool "KGDB: kernel debugging with remote gdb"
-	depends on DEBUG_KERNEL
-	select DEBUG_INFO
-	select FRAME_POINTER
-	help
-	  If you say Y here, it will be possible to remotely debug the
-	  kernel using gdb. This enlarges your kernel image disk size by
-	  several megabytes and requires a machine with more than 128 MB
-	  RAM to avoid excessive linking time.
-	  Documentation of kernel debugger available at
-	  http://kgdb.sourceforge.net
-	  This is only useful for kernel hackers. If unsure, say N.
-
-config KGDB_THREAD
-	bool "KGDB: Thread analysis"
-	depends on KGDB
-	help
-	  With thread analysis enabled, gdb can talk to kgdb stub to list
-	  threads and to get stack trace for a thread. This option also enables
-	  some code which helps gdb get exact status of thread. Thread analysis
-	  adds some overhead to schedule and down functions. You can disable
-	  this option if you do not want to compromise on speed.
-
-config KGDB_CONSOLE
-	bool "KGDB: Console messages through gdb"
-	depends on KGDB
-	help
-	  If you say Y here, console messages will appear through gdb.
-	  Other consoles such as tty or ttyS will continue to work as usual.
+source "kernel/Kconfig.kgdb"
 
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"

diff -zrupN linux-2.6.3+nothing/arch/ppc/Kconfig linux-2.6.3+config+serial/arch/ppc/Kconfig ---
linux-2.6.3+nothing/arch/ppc/Kconfig 2004-02-27 12:16:14.403163398 -0700
+++ linux-2.6.3+config+serial/arch/ppc/Kconfig 2004-02-27
12:16:13.771306387 -0700 @@ -1170,52 +1170,7 @@ config DEBUG_SPINLOCK_SLEEP

arch/ppc/Kconfig +1 -46

 	  If you say Y here, various routines which may sleep will become very
 	  noisy if they are called with a spinlock held.
 
-config KGDB
-	bool "Include kgdb kernel debugger"
-	depends on DEBUG_KERNEL
-	select DEBUG_INFO
-	select FRAME_POINTER
-	help
-	  Include in-kernel hooks for kgdb, the Linux kernel source level
-	  debugger.  See <http://kgdb.sourceforge.net/> for more information.
-	  Unless you are intending to debug the kernel, say N here.
-
-choice
-	prompt "Serial Port"
-	depends on KGDB
-	default KGDB_TTYS1
-
-config KGDB_TTYS0
-	bool "ttyS0"
-
-config KGDB_TTYS1
-	bool "ttyS1"
-
-config KGDB_TTYS2
-	bool "ttyS2"
-
-config KGDB_TTYS3
-	bool "ttyS3"
-
-endchoice
-
-config KGDB_THREAD
-	bool "KGDB: Thread analysis"
-	depends on KGDB
-	help
-	  With thread analysis enabled, gdb can talk to kgdb stub to list
-	  threads and to get stack trace for a thread. This option also enables
-	  some code which helps gdb get exact status of thread. Thread analysis
-	  adds some overhead to schedule and down functions. You can disable
-	  this option if you do not want to compromise on speed.
-
-config KGDB_CONSOLE
-	bool "Enable serial console thru kgdb port"
-	depends on KGDB && 8xx || 8260
-	help
-	  If you enable this, all serial console messages will be sent
-	  over the gdb stub.
-	  If unsure, say N.
+source "kernel/Kconfig.kgdb"
 
 config XMON
 	bool "Include xmon kernel debugger"

diff -zrupN linux-2.6.3+nothing/arch/x86_64/Kconfig linux-2.6.3+config+serial/arch/x86_64/Kconfig ---
linux-2.6.3+nothing/arch/x86_64/Kconfig 2004-02-27 12:16:14.350175389 -0700
+++ linux-2.6.3+config+serial/arch/x86_64/Kconfig 2004-02-27
12:16:13.718318378 -0700 @@ -465,37 +465,7 @@ config IOMMU_LEAK
Add a simple leak tracer to the IOMMU code. This is useful when
you are debugging a buggy device driver that leaks IOMMU mappings.

arch/x86_64/Kconfig +1 -31

-config KGDB
-	bool "KGDB: kernel debugging with remote gdb"
-	depends on DEBUG_KERNEL
-	select DEBUG_INFO
-	select FRAME_POINTER
-	help
-	  If you say Y here, it will be possible to remotely debug the
-	  kernel using gdb. This enlarges your kernel image disk size by
-	  several megabytes and requires a machine with more than 128 MB
-	  RAM to avoid excessive linking time.
-	  Documentation of kernel debugger available at
-	  http://kgdb.sourceforge.net
-	  This is only useful for kernel hackers. If unsure, say N.
-
-config KGDB_THREAD
-	bool "KGDB: Thread analysis"
-	depends on KGDB
-	help
-	  With thread analysis enabled, gdb can talk to kgdb stub to list
-	  threads and to get stack trace for a thread. This option also enables
-	  some code which helps gdb get exact status of thread. Thread analysis
-	  adds some overhead to schedule and down functions. You can disable
-	  this option if you do not want to compromise on speed.
-
-config KGDB_CONSOLE
-	bool "KGDB: Console messages through gdb"
-	depends on KGDB
-	help
-	  If you say Y here, console messages will appear through gdb.
-	  Other consoles such as tty or ttyS will continue to work as usual.
-
+source "kernel/Kconfig.kgdb"
 endmenu
 
 source "security/Kconfig"

diff -zrupN linux-2.6.3+nothing/drivers/net/Kconfig linux-2.6.3+config+serial/drivers/net/Kconfig ---
linux-2.6.3+nothing/drivers/net/Kconfig 2004-02-27 12:16:14.521136701 -0700
+++ linux-2.6.3+config+serial/drivers/net/Kconfig 2004-02-27
12:06:22.000000000 -0700 @@ -187,12 +187,6 @@ config NET_ETHERNET

drivers/net/Kconfig +0 -6

 	  Note that the answer to this question won't directly affect the
 	  kernel: saying N will just cause the configurator to skip all
 	  the questions about Ethernet network cards. If unsure, say N.
-
-config KGDB_ETH
-	bool "KGDB: On ethernet"
-	depends on KGDB
-	help
-	  Uses ethernet interface for kgdb.
 
 config MII
 	tristate "Generic Media Independent Interface device support"

diff -zrupN linux-2.6.3+nothing/drivers/serial/Kconfig linux-2.6.3+config+serial/drivers/serial/Kconfig ---
linux-2.6.3+nothing/drivers/serial/Kconfig 2004-02-27 12:16:14.545131271
-0700 +++ linux-2.6.3+config+serial/drivers/serial/Kconfig 2004-02-27
12:06:30.000000000 -0700 @@ -6,34 +6,6 @@

menu "Serial drivers"

drivers/serial/Kconfig +0 -28

-config KGDB_8250
-	bool "KGDB: On generic serial port (8250)"
-	depends on KGDB
-	help
-	  Uses generic serial port (8250) for kgdb. This is independent of the
-	  option 9250/16550 and compatible serial port.
-
-config KGDB_PORT
-	hex "hex I/O port address of the debug serial port"
-	depends on KGDB_8250
-	default  3f8
-	help
-	  Some systems (x86 family at this writing) allow the port
-	  address to be configured.  The number entered is assumed to be
-	  hex, don't put 0x in front of it.  The standard address are:
-	  COM1 3f8 , irq 4 and COM2 2f8 irq 3.  Setserial /dev/ttySx
-	  will tell you what you have.  It is good to test the serial
-	  connection with a live system before trying to debug.
-
-config KGDB_IRQ
-	int "IRQ of the debug serial port"
-	depends on KGDB_8250
-	default 4
-	help
-	  This is the irq for the debug port.  If everything is working
-	  correctly and the kernel has interrupts on a control C to the
-	  port should cause a break into the kernel debug stub.
-
 #
 # The new 8250/16550 serial drivers
 config SERIAL_8250

diff -zrupN linux-2.6.3+nothing/kernel/Kconfig.kgdb linux-2.6.3+config+serial/kernel/Kconfig.kgdb ---
linux-2.6.3+nothing/kernel/Kconfig.kgdb 1969-12-31 17:00:00.000000000 -0700
+++ linux-2.6.3+config+serial/kernel/Kconfig.kgdb 2004-02-27
12:16:13.000000000 -0700 @@ -0,0 +1,141 @@

kernel/Kconfig.kgdb +141 -0

+config KGDB
+	bool "KGDB: kernel debugging with remote gdb"
+	depends on DEBUG_KERNEL
+	select DEBUG_INFO
+	select FRAME_POINTER
+	# XXX: Doesn't work w/o this right now
+	select KGDB_THREAD if PPC32
+	help
+	  If you say Y here, it will be possible to remotely debug the
+	  kernel using gdb. This enlarges your kernel image disk size by
+	  several megabytes and requires a machine with more than 128 MB
+	  RAM to avoid excessive linking time.
+	  Documentation of kernel debugger available at
+	  http://kgdb.sourceforge.net
+	  This is only useful for kernel hackers. If unsure, say N.
+
+choice
+	prompt "Method for KGDB communication"
+	depends on KGDB
+	default PPC_SIMPLE_SERIAL if PPC32 && (8xx || 8260)
+	default KGDB_8250
+	help
+	  There are a number of different ways in which you can communicate
+	  with KGDB.  The oldest is using a serial driver.  A newer method
+	  is to use UDP packets and a special network driver.
+
+config KGDB_8250
+	bool "KGDB: On generic serial port (8250)"
+	help
+	  Uses generic serial port (8250) for kgdb. This is independent of the
+	  option 9250/16550 and compatible serial port.
+
+config KGDB_ETH
+	bool "KGDB: On ethernet"
+	select NETPOLL
+	select NETPOLL_TRAP
+	select NETPOLL_RX
+	help
+	  Uses ethernet interface for kgdb.
+
+config PPC_SIMPLE_SERIAL
+	bool "KGDB: On any serial port"
+	depends on PPC32
+	help
+	  Use a very simple, and not necessarily feature complete serial
+	  driver.  This is the only serial option currently for MPC8xx or
+	  MPC82xx based ports that do not offer an 8250-style UART.
+
+endchoice
+
+config KGDB_SIMPLE_SERIAL
+	bool "Simple selection of KGDB serial port"
+	depends on KGDB_8250 || PPC_SIMPLE_SERIAL
+	help
+	  If you say Y here, you will only have to pick the baud rate
+	  and serial port (ttyS) that you wish to use for KGDB.  If you
+	  say N, you will have provide the I/O port and IRQ number.  Note
+	  that if your serial ports are iomapped, then you must say Y here.
+	  If in doubt, say Y.
+
+choice
+	depends on KGDB_8250 || PPC_SIMPLE_SERIAL
+    	prompt "Debug serial port BAUD"
+	default KGDB_115200BAUD
+	help
+	  Gdb and the kernel stub need to agree on the baud rate to be
+	  used.  Some systems (x86 family at this writing) allow this to
+	  be configured.
+
+config KGDB_9600BAUD
+	bool "9600"
+
+config KGDB_19200BAUD
+	bool "19200"
+
+config KGDB_38400BAUD
+	bool "38400"
+
+config KGDB_57600BAUD
+	bool "57600"
+
+config KGDB_115200BAUD
+	bool "115200"
+endchoice
+
+choice
+	prompt "Serial port for KGDB"
+	depends on KGDB_SIMPLE_SERIAL
+	default KGDB_TTYS0
+
+config KGDB_TTYS0
+	bool "ttyS0"
+
+config KGDB_TTYS1
+	bool "ttyS1"
+
+config KGDB_TTYS2
+	bool "ttyS2"
+
+config KGDB_TTYS3
+	bool "ttyS3"
+
+endchoice
+
+config KGDB_PORT
+	hex "hex I/O port address of the debug serial port"
+	depends on !KGDB_SIMPLE_SERIAL && (KGDB_8250 || PPC_SIMPLE_SERIAL)
+	default  3f8
+	help
+	  Some systems (x86 family at this writing) allow the port
+	  address to be configured.  The number entered is assumed to be
+	  hex, don't put 0x in front of it.  The standard address are:
+	  COM1 3f8 , irq 4 and COM2 2f8 irq 3.  Setserial /dev/ttySx
+	  will tell you what you have.  It is good to test the serial
+	  connection with a live system before trying to debug.
+
+config KGDB_IRQ
+	int "IRQ of the debug serial port"
+	depends on !KGDB_SIMPLE_SERIAL && (KGDB_8250 || PPC_SIMPLE_SERIAL)
+	default 4
+	help
+	  This is the irq for the debug port.  If everything is working
+	  correctly and the kernel has interrupts on a control C to the
+	  port should cause a break into the kernel debug stub.
+
+config KGDB_THREAD
+	bool "KGDB: Thread analysis"
+	depends on KGDB
+	help
+	  With thread analysis enabled, gdb can talk to kgdb stub to list
+	  threads and to get stack trace for a thread. This option also enables
+	  some code which helps gdb get exact status of thread. Thread analysis
+	  adds some overhead to schedule and down functions. You can disable
+	  this option if you do not want to compromise on speed.
+
+config KGDB_CONSOLE
+	bool "KGDB: Console messages through gdb"
+	depends on KGDB
+	help
+	  If you say Y here, console messages will appear through gdb.
+	  Other consoles such as tty or ttyS will continue to work as usual.

It also makes core.patch dependent on 8250.patch
Any ideas on fixing that?

Quoted 2 lines

It also makes core.patch dependent on 8250.patch
Any ideas on fixing that?

No, it's intentional. eth.patch also depends on 8250.patch.

Hi!

Quoted 3 lines

Quoted 2 lines

It also makes core.patch dependent on 8250.patch
Any ideas on fixing that?

Quoted 1 line

No, it's intentional. eth.patch also depends on 8250.patch.

Perhaps that parts should be moved to core-lite? It should not be neccessary to have serial support...

Or perhaps we want to decrease number of modules, and merge 8250 into core-lite, having one less patch to care about?

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

It also makes core.patch dependent on 8250.patch
Any ideas on fixing that?

No, it's intentional. eth.patch also depends on 8250.patch.

Perhaps that parts should be moved to core-lite? It should not be neccessary to have serial support...

Or perhaps we want to decrease number of modules, and merge 8250 into core-lite, having one less patch to care about?

If it's really important to not have patches depend on eachother until we get to the non-lite core, i386 (and soon ppc, x86_64) patches, then we should put all of the Kconfig bits in the main patch, so long as we think all of the related drivers will be able to make it in as well (or will move those bits out again when it's time). Or we could just document dependancies and move on.

Hi!

Quoted 10 lines

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

It also makes core.patch dependent on 8250.patch
Any ideas on fixing that?

No, it's intentional. eth.patch also depends on 8250.patch.

Perhaps that parts should be moved to core-lite? It should not be neccessary to have serial support...

Or perhaps we want to decrease number of modules, and merge 8250 into core-lite, having one less patch to care about?

Quoted 6 lines

If it's really important to not have patches depend on eachother until we get to the non-lite core, i386 (and soon ppc, x86_64) patches, then we should put all of the Kconfig bits in the main patch, so long as we think all of the related drivers will be able to make it in as well (or will move those bits out again when it's time). Or we could just document dependancies and move on.

I believe "put kconfig into core" is the right thing to do.