Update CVS KGDB's serial driver

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

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

- Remove ok_packet(), excessive, IMHO.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1664 -> 1.1665
# arch/ppc/8260_io/uart.c 1.31 -> 1.32
# kernel/kgdb.c 1.4 -> 1.5
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 [email protected] 1.1665
# - Kill kgdb_might_be_resumed and kgdb_killed_or_detached. # - Spacing and comments.
# - put_packet doesn't try and check for a connect now.
# - Kill ok_packet().
# - Only send an initial packet in kgdb_handle_exception if
# kgdb has connected already.
# --------------------------------------------
#
diff -Nru a/arch/ppc/8260_io/uart.c b/arch/ppc/8260_io/uart.c

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

@@ -396,14 +396,8 @@
 #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++;

diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c

kernel/kgdb.c +7 -40

@@ -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 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 @@
 	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 @@
  * 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 {
 		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('#');

@@ -277,30 +273,7 @@
 		kgdb_serial->write_char(hexchars[checksum % 16]);
 		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('+');
-			if (kgdb_serial->flush)
-				kgdb_serial->flush();
-			breakpoint();
-		}
-	} while ((ch & 0x7f) != '+');
+	} while ((kgdb_serial->read_char() & 0x7f) != '+');
 
 }
 

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

@@ -502,7 +470,8 @@
 	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 */

kernel/kgdb.c +12 -32

 	while (!procindebug[atomic_read(&debugger_active) - 1]) {
 		int i = 10;	/* an arbitrary number */

@@ -701,17 +670,7 @@
 	/* 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 +680,9 @@
 		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);

@@ -798,7 +755,7 @@
 			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 +795,10 @@
 			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 +904,10 @@
 				}
 				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 +922,7 @@
 					}
 					kgdb_contthread = thread;
 				}
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 				break;
 			}
 			break;

@@ -977,7 +933,7 @@
 			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 +974,7 @@
 			}
 
 			if (error == 0)
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 			else
 				error_packet(remcom_out_buffer, error);
 

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

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

@@ -1132,12 +1087,6 @@
 	/* 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 +1171,7 @@
 		*bufptr = '\0';
 		s += wcount;
 
-		put_packet(kgdbconbuf, 1);
+		put_packet(kgdbconbuf);
 
 	}
 	local_irq_restore(flags);

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

kernel/kgdb.c +0 -4

-		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);

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies

3. putpacket writes the packet and waits for a '+'
4. new gdb sends a protocol initialization packet
5. putpacket reads characters in that packet hoping for an incoming '+' sending out console message packet on each incoming character
6. gdb receives and rejects each console message packet

Quoted 1 line

- Remove ok_packet(), excessive, IMHO.

ok_packet is better than littering "OK" all over the place.

Quoted 15 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

kernel/kgdb.c +0 -4

-		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);

Quoted 2 lines

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

Er, what do you mean 'restarted' ? If gdb somehow disconnects 'detach' or ^D^D, remove_all_break() gets called. Is there another way for gdb to somehow disconnect that I don't know of?

Quoted 5 lines

Quoted 4 lines

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

Quoted 5 lines

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies

As in doesn't cleanly remove itself?

Quoted 17 lines · Tom Rini

Quoted 15 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

kernel/kgdb.c +0 -4

-		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);

Quoted 6 lines

Quoted 2 lines

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

Er, what do you mean 'restarted' ? If gdb somehow disconnects 'detach' or ^D^D, remove_all_break() gets called. Is there another way for gdb to somehow disconnect that I don't know of?

Yes. See the disconnect command in recent GDB versions.

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

Quoted 4 lines

Quoted 2 lines

1. kgdb console messages are to be put.
2. gdb dies

As in doesn't cleanly remove itself?

Seg faults, for instance.

Ack. Does gdb do anything to see if the rmote side will support being left hanging?

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies

Quoted 3 lines

Quoted 1 line

As in doesn't cleanly remove itself?

Seg faults, for instance.

That's what I was figuring. Does gdbserver handle things like this (gdb up and dying in the middle of a session, disconnecting without warning) well?

Quoted 16 lines · Tom Rini

Quoted 15 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

kernel/kgdb.c +0 -4

-		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);

Quoted 6 lines

Quoted 2 lines

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

Er, what do you mean 'restarted' ? If gdb somehow disconnects 'detach' or ^D^D, remove_all_break() gets called. Is there another way for gdb to somehow disconnect that I don't know of?

Yes. See the disconnect command in recent GDB versions.

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

Quoted 4 lines

Quoted 2 lines

1. kgdb console messages are to be put.
2. gdb dies

As in doesn't cleanly remove itself?

Seg faults, for instance.

Quoted 6 lines

3. putpacket writes the packet and waits for a '+'
4. new gdb sends a protocol initialization packet
5. putpacket reads characters in that packet hoping for an incoming '+' sending out console message packet on each incoming character
6. gdb receives and rejects each console message packet

Quoted 3 lines

Quoted 1 line

- Remove ok_packet(), excessive, IMHO.

ok_packet is better than littering "OK" all over the place.

I disagree. If ok_packet was anything more than
strcpy(remcom_out_buffer, "OK") you'd be right.

Hi!

Quoted 8 lines

Quoted 8 lines

3. putpacket writes the packet and waits for a '+'
4. new gdb sends a protocol initialization packet
5. putpacket reads characters in that packet hoping for an incoming '+' sending out console message packet on each incoming character
6. gdb receives and rejects each console message packet

Quoted 1 line

- Remove ok_packet(), excessive, IMHO.

Quoted 4 lines

Quoted 1 line

ok_packet is better than littering "OK" all over the place.

I disagree. If ok_packet was anything more than
strcpy(remcom_out_buffer, "OK") you'd be right.

Amit, he's right, having function just for one strcpy only makes code harder to read. And it does not even save much typing...

	ok_packet(foo);
	strcpy(foo,"OK");

...we are talking 2 or 3 characters here....
Pavel

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'
4. new gdb sends a protocol initialization packet
5. putpacket reads characters in that packet hoping for an incoming '+' sending out console message packet on each incoming character
6. gdb receives and rejects each console message packet

Quoted 7 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

Quoted 2 lines

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

OK. After talking with Daniel Jacobowitz abit about this, the '?' packet will only be sent once per session. So this is how we have to deal with disconnect / gdb dying. I'll re-work the patch a bit to reflect this.

Quoted 7 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

Quoted 7 lines

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'
4. new gdb sends a protocol initialization packet
5. putpacket reads characters in that packet hoping for an incoming '+' sending out console message packet on each incoming character
6. gdb receives and rejects each console message packet

How about something like the following:
putpacket(msg)
{
if (!kgdb_connected)
return; /* No one wants this */

 do {
   /* Write out $pack#checksum */
   ch = kgdb_get_debug_char();
   if (ch == '+' /* ACK */ || ch == '$' /* Packet before an ACK?
                                           must be a reattach. */ )
    return;

}
}

With the suggested change for the ? packet, we quickly get to the point where we're back in kgdb_handle_exception() and will know this is a restart shortly.

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

kernel/kgdb.c +0 -4

-		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);

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

Quoted 5 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

4. new gdb sends a protocol initialization packet
5. putpacket reads characters in that packet hoping for an incoming '+' sending out console message packet on each incoming character
6. gdb receives and rejects each console message packet

Quoted 1 line

- Remove ok_packet(), excessive, IMHO.

ok_packet is better than littering "OK" all over the place.

Other lindent changes are good.

-Amit

Quoted 280 lines

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1664 -> 1.1665
# arch/ppc/8260_io/uart.c 1.31 -> 1.32
# kernel/kgdb.c 1.4 -> 1.5
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 [email protected] 1.1665
# - Kill kgdb_might_be_resumed and kgdb_killed_or_detached. # - Spacing and comments.
# - put_packet doesn't try and check for a connect now.
# - Kill ok_packet().
# - Only send an initial packet in kgdb_handle_exception if
# kgdb has connected already.
# --------------------------------------------
#
diff -Nru a/arch/ppc/8260_io/uart.c b/arch/ppc/8260_io/uart.c

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

@@ -396,14 +396,8 @@
 #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++;

diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c

kernel/kgdb.c +7 -40

@@ -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 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 @@
 	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 @@
 * 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 {
 		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('#');

@@ -277,30 +273,7 @@
 		kgdb_serial->write_char(hexchars[checksum % 16]);
 		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('+');
-			if (kgdb_serial->flush)
-				kgdb_serial->flush();
-			breakpoint();
-		}
-	} while ((ch & 0x7f) != '+');
+	} while ((kgdb_serial->read_char() & 0x7f) != '+');
 
 }
 

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

@@ -502,7 +470,8 @@
 	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 */

kernel/kgdb.c +11 -31

@@ -701,17 +670,7 @@
 	/* 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 +680,9 @@
 		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);

@@ -798,7 +755,7 @@
 			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 +795,10 @@
 			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 +904,10 @@
 				}
 				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 +922,7 @@
 					}
 					kgdb_contthread = thread;
 				}
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 				break;
 			}
 			break;

@@ -977,7 +933,7 @@
 			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 +974,7 @@
 			}
 
 			if (error == 0)
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 			else
 				error_packet(remcom_out_buffer, error);
 

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

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

@@ -1132,12 +1087,6 @@
 	/* 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;
 

kernel/kgdb.c line 1177

Quoted 8 lines

kernel/kgdb.c +1 -1

@@ -1222,7 +1171,7 @@
 		*bufptr = '\0';
 		s += wcount;
 
-		put_packet(kgdbconbuf, 1);
+		put_packet(kgdbconbuf);
 
 	}
 	local_irq_restore(flags);

-
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 18 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

(quoted diff) +0 -1

-		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 

'c') {

kernel/kgdb.c +0 -3

-			remove_all_break();
-			atomic_set(&kgdb_killed_or_detached, 0);
-			ok_packet(remcom_out_buffer);

Quoted 2 lines

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

I wonder if this is worth the trouble. Does kgdb need to know about breakpoints at all? Is there some other reason it needs to track them?

Quoted 8 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

Quoted 3 lines

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

Quoted 20 lines · George Anzinger

Quoted 18 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

(quoted diff) +0 -1

-		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 

'c') {

kernel/kgdb.c +0 -3

-			remove_all_break();
-			atomic_set(&kgdb_killed_or_detached, 0);
-			ok_packet(remcom_out_buffer);

Quoted 3 lines

Quoted 1 line

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

I wonder if this is worth the trouble. Does kgdb need to know about breakpoints at all? Is there some other reason it needs to track them?

I don't know if it's strictly needed, but it's not the hard part of this particular issue (as I suggested in another thread, remove_all_break() on a ? packet works).

Quoted 11 lines

Quoted 10 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Quoted 1 line

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

I don't think this is that hard of a problem anymore. I haven't enabled console messages, but I've got the following being happy now:

- Connect to a waiting kernel, continue/^C/disconnect/reconnect.
- Connect to a running kernel, continue/^C/disconnect/reconnect.
- Once connected and running, ^C/hit breakpoint and disconnect/reconnect.
- Once connected, set a breakpoint, kill gdb and hit the breakpoint and reconnect.
- Once connected and running, kill gdb and reconnect.

The last two aren't as "fast" as I might like, but they're the "gdb went away in an ungraceful manner" situations, so I think it's OK. In the first (breakpoint hit, no gdb) I end up having to issue a few continues to get moving again, but it's a one-time event.

2 packet instead of ACKs, then a NAK, but I believe this is acceptable.

Quoted 53 lines · Tom Rini

Quoted 21 lines · George Anzinger

Quoted 19 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

(quoted diff) +0 -1

-		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 

'c') {

kernel/kgdb.c +0 -3

-			remove_all_break();
-			atomic_set(&kgdb_killed_or_detached, 0);
-			ok_packet(remcom_out_buffer);

Quoted 5 lines

Quoted 2 lines

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

I wonder if this is worth the trouble. Does kgdb need to know about breakpoints at all? Is there some other reason it needs to track them?

I don't know if it's strictly needed, but it's not the hard part of this particular issue (as I suggested in another thread, remove_all_break() on a ? packet works).

Quoted 16 lines

Quoted 11 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

Quoted 2 lines

I don't think this is that hard of a problem anymore. I haven't enabled console messages, but I've got the following being happy now:

console pass through is the hard one as it is done outside of kgdb under interrupt control. Thus the '+' will come to the interrupt handler.

There is a bit of a problem here WRT hiting a breakpoint while waiting for this '+'. Should only happen on SMP systems, but still....

Quoted 8 lines

- Connect to a waiting kernel, continue/^C/disconnect/reconnect.
- Connect to a running kernel, continue/^C/disconnect/reconnect.
- Once connected and running, ^C/hit breakpoint and disconnect/reconnect.
- Once connected, set a breakpoint, kill gdb and hit the breakpoint and reconnect.
- Once connected and running, kill gdb and reconnect.

Quoted 4 lines

The last two aren't as "fast" as I might like, but they're the "gdb went away in an ungraceful manner" situations, so I think it's OK. In the first (breakpoint hit, no gdb) I end up having to issue a few continues to get moving again, but it's a one-time event.

What are you referring to as "continues". How is this different from connect to a waiting kernel?

Usually this would be the end of the
session. If you are going to continue from here something needs to be done with the breakpoint that gdb does not know about. If kgdb can remove them, well fine, except your stopped on one. If you remove it, there could be some confusion as to why you are in the debugger.

Quoted 57 lines · George Anzinger

Quoted 47 lines · Tom Rini

Quoted 22 lines · George Anzinger

Quoted 19 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

(quoted diff) +0 -1

-		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 

'c') {

kernel/kgdb.c +0 -3

-			remove_all_break();
-			atomic_set(&kgdb_killed_or_detached, 0);
-			ok_packet(remcom_out_buffer);

Quoted 3 lines

Quoted 1 line

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

I wonder if this is worth the trouble. Does kgdb need to know about breakpoints at all? Is there some other reason it needs to track them?

I don't know if it's strictly needed, but it's not the hard part of this particular issue (as I suggested in another thread, remove_all_break() on a ? packet works).

Quoted 11 lines

Quoted 10 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Quoted 5 lines

Quoted 1 line

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

I don't think this is that hard of a problem anymore. I haven't enabled console messages, but I've got the following being happy now:

console pass through is the hard one as it is done outside of kgdb under interrupt control. Thus the '+' will come to the interrupt handler.

Quoted 1 line

There is a bit of a problem here WRT hiting a breakpoint while waiting for this '+'. Should only happen on SMP systems, but still....

Here's why I don't think it's a problem (I'll post the new patch shortly, getting from quilt to a patch against previous is still a pain). What happens is:
1. kgdb console tried to send a packet.
2. before ACK'ing the above, gdb dies.

3. kgdb loops on sending a packet and reading in a char.
4. gdb tries to reconnect and sends $somePacket#cs
5. put_packet sends out the console message again, and reads in a char.
6. put_packet sees a $ (or in the case of your .gdbinit, ^C$, which is still fine).
7. put_packet sees a packet coming in, which preempts sending this packet, and will call kgdb_schedule_breakpoint() and then return, giving up on the console message.
8. do_IRQ() calls kgdb_process_breakpoint(), which calls breakpoint() and gdb gets back in the game.

Quoted 8 lines

Quoted 8 lines

- Connect to a waiting kernel, continue/^C/disconnect/reconnect.
- Connect to a running kernel, continue/^C/disconnect/reconnect.
- Once connected and running, ^C/hit breakpoint and disconnect/reconnect.
- Once connected, set a breakpoint, kill gdb and hit the breakpoint and reconnect.
- Once connected and running, kill gdb and reconnect.

Quoted 6 lines

Quoted 4 lines

The last two aren't as "fast" as I might like, but they're the "gdb went away in an ungraceful manner" situations, so I think it's OK. In the first (breakpoint hit, no gdb) I end up having to issue a few continues to get moving again, but it's a one-time event.

What are you referring to as "continues". How is this different from connect to a waiting kernel?

The 'continue' command in gdb.

Quoted 1 line

Usually this would be the end of the session. If you are going to continue from here something needs to be done with the breakpoint that gdb does not know about. If kgdb can remove them, well fine, except your stopped on one. If you remove it, there could be some confusion as to why you are in the debugger.

Hmm. I think I need to test things a bit more, before I comment on this.

Quoted 73 lines · Tom Rini

Quoted 60 lines · George Anzinger

Quoted 58 lines · Tom Rini

Quoted 24 lines · George Anzinger

Quoted 21 lines · Amit S. Kale

Quoted 6 lines · Tom Rini

The following patch fixes a number of little issues here and there, and
ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

(quoted diff) +0 -1

-		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 

'c') {

(quoted diff) +0 -3

-			remove_all_break();
-			atomic_set(&kgdb_killed_or_detached, 0);
-			ok_packet(remcom_out_buffer);

Quoted 5 lines

Quoted 2 lines

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

I wonder if this is worth the trouble. Does kgdb need to know about breakpoints at all? Is there some other reason it needs to track them?

I don't know if it's strictly needed, but it's not the hard part of this particular issue (as I suggested in another thread, remove_all_break() on a ? packet works).

Quoted 16 lines

Quoted 11 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

Quoted 8 lines

Quoted 2 lines

I don't think this is that hard of a problem anymore. I haven't enabled console messages, but I've got the following being happy now:

console pass through is the hard one as it is done outside of kgdb under interrupt control. Thus the '+' will come to the interrupt handler.

There is a bit of a problem here WRT hiting a breakpoint while waiting for this '+'. Should only happen on SMP systems, but still....

Quoted 5 lines

Here's why I don't think it's a problem (I'll post the new patch shortly, getting from quilt to a patch against previous is still a pain). What happens is:
1. kgdb console tried to send a packet.
2. before ACK'ing the above, gdb dies.

What I am describing does not have anything to do with gdb going away. It is that in "normal" operation the console output is done with the interrupts on (i.e. we are not in kgdb as a result of a breakpoint, but only to do console output). This means that the interrupt that is generated by the '+' from gdb may well happen and the kgdb interrupt handler will see the '+' and, with the interrupt handler changes, generate a breakpoint. All we really want to do is to pass the '+' through to putpacket. In a UP machine, I think the wait for the '+' is done with the interrupt system off, however, in an SMP machine, other cpus may see it and interrupt... At the very least, the interrupt code needs to be able to determine that no character came in and ignore the interrupt.

Quoted 65 lines · George Anzinger

Quoted 55 lines · Tom Rini

Quoted 54 lines · George Anzinger

Quoted 41 lines · Tom Rini

Quoted 18 lines · George Anzinger

Quoted 17 lines · Amit S. Kale

Quoted 6 lines · Tom Rini

The following patch fixes a number of little issues here and there, and
ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

(quoted diff) +0 -1

-		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] ==

'c') {

(quoted diff) +0 -3

-			remove_all_break();
-			atomic_set(&kgdb_killed_or_detached, 0);
-			ok_packet(remcom_out_buffer);

Quoted 6 lines

Quoted 2 lines

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

I wonder if this is worth the trouble. Does kgdb need to know about breakpoints at all? Is there some other reason it needs to track them?

I don't know if it's strictly needed, but it's not the hard part of this particular issue (as I suggested in another thread, remove_all_break() on a ? packet works).

Quoted 11 lines

Quoted 10 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Quoted 7 lines

Quoted 4 lines

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

I don't think this is that hard of a problem anymore. I haven't enabled console messages, but I've got the following being happy now:

console pass through is the hard one as it is done outside of kgdb under interrupt control. Thus the '+' will come to the interrupt handler.

Quoted 8 lines

Quoted 2 lines

There is a bit of a problem here WRT hiting a breakpoint while waiting for this '+'. Should only happen on SMP systems, but still....

Here's why I don't think it's a problem (I'll post the new patch shortly, getting from quilt to a patch against previous is still a pain). What happens is:
1. kgdb console tried to send a packet.
2. before ACK'ing the above, gdb dies.

Quoted 12 lines

What I am describing does not have anything to do with gdb going away. It is that in "normal" operation the console output is done with the interrupts on (i.e. we are not in kgdb as a result of a breakpoint, but only to do console output). This means that the interrupt that is generated by the '+' from gdb may well happen and the kgdb interrupt handler will see the '+' and, with the interrupt handler changes, generate a breakpoint. All we really want to do is to pass the '+' through to putpacket. In a UP machine, I think the wait for the '+' is done with the interrupt system off, however, in an SMP machine, other cpus may see it and interrupt... At the very least, the interrupt code needs to be able to determine that no character came in and ignore the interrupt.

Today might not be a "smart day" for me, so perhaps I'm just not doing what's need to trigger this, or I'm misreading (but if you can trigger it, w/ Amit's patches in CVS and my 1/2 from yesterday and then my 7 from today, I'd be grateful) but UP and SMP on a UP box both have KGDB_CONSOLE behaving correctly.

Quoted 52 lines · Tom Rini

Quoted 51 lines · George Anzinger

Quoted 39 lines · Tom Rini

Quoted 17 lines · George Anzinger

Quoted 16 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

(quoted diff) +0 -1

-		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] ==

'c') {

(quoted diff) +0 -3

-			remove_all_break();
-			atomic_set(&kgdb_killed_or_detached, 0);
-			ok_packet(remcom_out_buffer);

Quoted 5 lines

Quoted 2 lines

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

I wonder if this is worth the trouble. Does kgdb need to know about breakpoints at all? Is there some other reason it needs to track them?

I don't know if it's strictly needed, but it's not the hard part of this particular issue (as I suggested in another thread, remove_all_break() on a ? packet works).

Quoted 11 lines

Quoted 10 lines

Quoted 3 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Quoted 7 lines

Quoted 4 lines

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

I don't think this is that hard of a problem anymore. I haven't enabled console messages, but I've got the following being happy now:

console pass through is the hard one as it is done outside of kgdb under interrupt control. Thus the '+' will come to the interrupt handler.

Quoted 8 lines

Quoted 2 lines

There is a bit of a problem here WRT hiting a breakpoint while waiting for this '+'. Should only happen on SMP systems, but still....

Here's why I don't think it's a problem (I'll post the new patch shortly, getting from quilt to a patch against previous is still a pain). What happens is:
1. kgdb console tried to send a packet.
2. before ACK'ing the above, gdb dies.

What I am describing does not have anything to do with gdb going away. It is that in "normal" operation the console output is done with the interrupts on (i.e. we are not in kgdb as a result of a breakpoint, but only to do console output). This means that the interrupt that is generated by the '+' from gdb may well happen and the kgdb interrupt handler will see the '+' and, with the interrupt handler changes, generate a breakpoint. All we really want to do is to pass the '+' through to putpacket. In a UP machine, I think the wait for the '+' is done with the interrupt system off, however, in an SMP machine, other cpus may see it and interrupt... At the very least, the interrupt code needs to be able to determine that no character came in and ignore the interrupt.

-g

Quoted 37 lines

3. kgdb loops on sending a packet and reading in a char.
4. gdb tries to reconnect and sends $somePacket#cs
5. put_packet sends out the console message again, and reads in a char.
6. put_packet sees a $ (or in the case of your .gdbinit, ^C$, which is still fine).
7. put_packet sees a packet coming in, which preempts sending this packet, and will call kgdb_schedule_breakpoint() and then return, giving up on the console message.
8. do_IRQ() calls kgdb_process_breakpoint(), which calls breakpoint() and gdb gets back in the game.

Quoted 8 lines

Quoted 8 lines

- Connect to a waiting kernel, continue/^C/disconnect/reconnect.
- Connect to a running kernel, continue/^C/disconnect/reconnect.
- Once connected and running, ^C/hit breakpoint and disconnect/reconnect.
- Once connected, set a breakpoint, kill gdb and hit the breakpoint and reconnect.
- Once connected and running, kill gdb and reconnect.

Quoted 7 lines

Quoted 4 lines

The last two aren't as "fast" as I might like, but they're the "gdb went away in an ungraceful manner" situations, so I think it's OK. In the first (breakpoint hit, no gdb) I end up having to issue a few continues to get moving again, but it's a one-time event.

What are you referring to as "continues". How is this different from connect to a waiting kernel?

The 'continue' command in gdb.

Quoted 5 lines

Usually this would be the end of the
session. If you are going to continue from here something needs to be done with the breakpoint that gdb does not know about. If kgdb can remove them, well fine, except your stopped on one. If you remove it, there could be some confusion as to why you are in the debugger.

Hmm. I think I need to test things a bit more, before I comment on this.

Quoted 8 lines · Tom Rini

- Connect to a waiting kernel, continue/^C/disconnect/reconnect.
- Connect to a running kernel, continue/^C/disconnect/reconnect.
- Once connected and running, ^C/hit breakpoint and disconnect/reconnect.
- Once connected, set a breakpoint, kill gdb and hit the breakpoint and reconnect.
- Once connected and running, kill gdb and reconnect.

Quoted 4 lines

The last two aren't as "fast" as I might like, but they're the "gdb went away in an ungraceful manner" situations, so I think it's OK. In the first (breakpoint hit, no gdb) I end up having to issue a few continues to get moving again, but it's a one-time event.

What are you referring to as "continues". How is this different from
connect to a waiting kernel? Usually this would be the end of the
session. If you are going to continue from here something needs to be done with the breakpoint that gdb does not know about. If kgdb can remove them, well fine, except your stopped on one. If you remove it, there could be some confusion as to why you are in the debugger. This would be a fine time for a note to the user from kgdb. It is too bad that the interface does not admit to such a thing.

Quoted 10 lines · George Anzinger

Quoted 8 lines · Tom Rini

- Connect to a waiting kernel, continue/^C/disconnect/reconnect.
- Connect to a running kernel, continue/^C/disconnect/reconnect.
- Once connected and running, ^C/hit breakpoint and disconnect/reconnect.
- Once connected, set a breakpoint, kill gdb and hit the breakpoint and reconnect.
- Once connected and running, kill gdb and reconnect.

Quoted 6 lines

Quoted 4 lines

The last two aren't as "fast" as I might like, but they're the "gdb went away in an ungraceful manner" situations, so I think it's OK. In the first (breakpoint hit, no gdb) I end up having to issue a few continues to get moving again, but it's a one-time event.

What are you referring to as "continues". How is this different from connect to a waiting kernel? Usually this would be the end of the session. If you are going to continue from here something needs to be done with the breakpoint that gdb does not know about. If kgdb can remove them, well fine, except your stopped on one. If you remove it, there could be some confusion as to why you are in the debugger. This would be a fine time for a note to the user from kgdb. It is too bad that the interface does not admit to such a thing.

OK, I've rechecked the senarios, and the only time gdb/kgdb seems a bit confused is when gdb sets a bpt / dies / bpt is triggered. It looks like this (on gdb reattaching to the now stuck host, gdb 6.0):
Remote debugging using /dev/ttyS0
Sending packet: $qPassSignals:0e;10;14;17;1a;1b;1c;21;24;25;4c;#df...Ack Packet received: Packet qPassSignals (pass-signals) is NOT supported Sending packet: $Hc-1#09...Ack
Packet received: OK
Sending packet: $qC#b4...Ack
Packet received: QC00000000000000d0
Sending packet: $qOffsets#4b...Ack
Packet received: Sending packet: $?#3f...Ack

<- At this point, kgdb calls remove_all_break() ->

Packet received: S05
Sending packet: $Hgd0#43...Ack
Packet received: OK
Sending packet: $g#67...Ack
Packet received: 27000000ff0100007b000000c0feffbf703f55d6bc3f55d6c0feffbfd4fdffbf8a4815c08202000060000000680000007b0069d77b000000ffff0000ffff0000 0xc015488a in sys_mkdir (Sending packet: $m27,8#3a...Ack
Packet received: E22
Sending packet: $m27,8#3a...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
pathname=0x27 <Address 0x27 out of bounds>,

    mode=-1073742144) at fs/namei.c:1533

1533 tmp = getname(pathname);
Sending packet: $qSymbol::#5b...Ack
Packet received: Packet qSymbol (symbol-lookup) is NOT supported

<- continue ->

Continuing.
Sending packet: $Hsd0#4f...Ack
Packet received: Sending packet: $Hc0#db...Ack
Packet received: OK
Sending packet: $c#63...Ack
Packet received: T0bthread:00000000000000d0;
[New Thread 208]
Sending packet: $g#67...Ack
Packet received: 27000000ff0100007b000000c0feffbf703f55d6bd3f55d6c0feffbfd4fdffbf8b4815c08602010060000000680000007b0069d77b000000ffff0000ffff0000

Program received signal SIGSEGV, Segmentation fault. 0xc015488b in sys_mkdir (Sending packet: $m27,8#3a...Ack Packet received: E22
Sending packet: $m27,8#3a...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
pathname=0x27 <Address 0x27 out of bounds>,

    mode=-1073742144) at fs/namei.c:1533

1533 tmp = getname(pathname);

<- continue ->

Continuing.
Sending packet: $C0b#d5...Ack
Packet received: Can't send signals to this remote system. SIGSEGV not sent.
Sending packet: $c#63...Ack
Packet received: T05thread:00000000000000d0;
Sending packet: $g#67...Ack
Packet received: 27000000ff0100007b000000c0feffbf703f55d6bd3f55d6c0feffbfd4fdffbf8b4815c08602010060000000680000007b0069d77b000000ffff0000ffff0000

Program received signal SIGTRAP, Trace/breakpoint trap. 0xc015488b in sys_mkdir (Sending packet: $m27,8#3a...Ack Packet received: E22
Sending packet: $m27,8#3a...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
Sending packet: $m27,1#33...Ack
Packet received: E22
pathname=0x27 <Address 0x27 out of bounds>,

    mode=-1073742144) at fs/namei.c:1533

1533 tmp = getname(pathname);

<- continue ->

Continuing.
Sending packet: $c#63...Ack

<- From here, the system is OK again ->

remote_interrupt called
remote_stop called
Packet received: T05thread:0000000000008000;
[New Thread 32768]
Sending packet: $g#67...Ack
Packet received: 0100000004000000000000000400000068df28c06cdf28c080000000809a28c05afd12c00200000060000000680000007b0010c07b000000ffff0000ffff0000

Program received signal SIGTRAP, Trace/breakpoint trap. [Switching to Thread 32768]
breakpoint () at kernel/kgdb.c:1088
1088 atomic_set(&kgdb_setting_breakpoint, 0);
Sending packet: $D#44...Ack
Packet received: OK
Ending remote debugging.

Quoted 15 lines · Amit S. Kale

Quoted 5 lines · Tom Rini

The following patch fixes a number of little issues here and there, and ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached. GDB attaching is GDB attaching, we haven't preserved any of the previous context anyhow.

If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

kernel/kgdb.c +0 -4

-		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);

Quoted 2 lines

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

I wonder if this is worth the trouble. Does kgdb need to know about breakpoints at all? Is there some other reason it needs to track them?

Quoted 8 lines

Quoted 4 lines

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

We have to check for gdb connection in putpacket or else following problem occurs.

Quoted 3 lines

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

-g

Quoted 307 lines

4. new gdb sends a protocol initialization packet
5. putpacket reads characters in that packet hoping for an incoming '+' sending out console message packet on each incoming character
6. gdb receives and rejects each console message packet

Quoted 1 line

- Remove ok_packet(), excessive, IMHO.

ok_packet is better than littering "OK" all over the place.

Other lindent changes are good.

-Amit

Quoted 280 lines

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1664 -> 1.1665
# arch/ppc/8260_io/uart.c 1.31 -> 1.32
# kernel/kgdb.c 1.4 -> 1.5
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 [email protected] 1.1665
# - Kill kgdb_might_be_resumed and kgdb_killed_or_detached. # - Spacing and comments.
# - put_packet doesn't try and check for a connect now.
# - Kill ok_packet().
# - Only send an initial packet in kgdb_handle_exception if
# kgdb has connected already.
# --------------------------------------------
#
diff -Nru a/arch/ppc/8260_io/uart.c b/arch/ppc/8260_io/uart.c

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

@@ -396,14 +396,8 @@
 #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++;

diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c

kernel/kgdb.c +1 -0

@@ -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 spinlock_t slavecpulocks[KGDB_MAX_NO_CPUS]; static volatile int procindebug[KGDB_MAX_NO_CPUS]; atomic_t kgdb_setting_breakpoint;

kernel/kgdb.c +0 -2

-atomic_t kgdb_killed_or_detached;
-atomic_t kgdb_might_be_resumed;

struct task_struct *kgdb_usethread, *kgdb_contthread;

int debugger_step;

kernel/kgdb.c +6 -38

@@ -212,8 +211,10 @@
 	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 @@
 * 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 {
 		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('#');

@@ -277,30 +273,7 @@
 		kgdb_serial->write_char(hexchars[checksum % 16]);
 		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('+');
-			if (kgdb_serial->flush)
-				kgdb_serial->flush();
-			breakpoint();
-		}
-	} while ((ch & 0x7f) != '+');
+	} while ((kgdb_serial->read_char() & 0x7f) != '+');
 
 }
 

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

@@ -502,7 +470,8 @@
 	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 */

kernel/kgdb.c +11 -31

@@ -701,17 +670,7 @@
 	/* 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 +680,9 @@
 		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);

@@ -798,7 +755,7 @@
 			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 +795,10 @@
 			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 +904,10 @@
 				}
 				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 +922,7 @@
 					}
 					kgdb_contthread = thread;
 				}
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 				break;
 			}
 			break;

@@ -977,7 +933,7 @@
 			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 +974,7 @@
 			}
 
 			if (error == 0)
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 			else
 				error_packet(remcom_out_buffer, error);
 

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

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

@@ -1132,12 +1087,6 @@
 	/* 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;
 

kernel/kgdb.c line 1177

Quoted 8 lines

kernel/kgdb.c +1 -1

@@ -1222,7 +1171,7 @@
 		*bufptr = '\0';
 		s += wcount;
 
-		put_packet(kgdbconbuf, 1);
+		put_packet(kgdbconbuf);
 
 	}
 	local_irq_restore(flags);

-
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 13 lines · George Anzinger

Quoted 11 lines · Amit S. Kale

Quoted 3 lines · Tom Rini

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

We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Quoted 4 lines

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

Yes. That's why I added a paramter to putpacket to skip causing a breakpoint when not required.

The following adds, and then makes use of kgdb_process_breakpoint / kgdb_schedule_breakpoint. Using it i kgdb_8250.c isn't strictly needed, but it isn't wrong either.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1663 -> 1.1664 # arch/i386/kernel/irq.c 1.48 -> 1.49 # drivers/net/kgdb_eth.c 1.2 -> 1.3 # arch/x86_64/kernel/irq.c 1.21 -> 1.22 # drivers/serial/kgdb_8250.c 1.3 -> 1.4 # kernel/kgdb.c 1.3 -> 1.4 # arch/ppc/kernel/irq.c 1.36 -> 1.37 # include/linux/kgdb.h 1.3 -> 1.4 #
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 [email protected] 1.1664

# process_breakpoint/schedule_breakpoint.
# --------------------------------------------
#
diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c

arch/i386/kernel/irq.c +3 -0

@@ -34,6 +34,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/kallsyms.h>
+#include <linux/kgdb.h>
 
 #include <asm/atomic.h>
 #include <asm/io.h>

@@ -507,6 +508,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 
 	return 1;
 }

diff -Nru a/arch/ppc/kernel/irq.c b/arch/ppc/kernel/irq.c

arch/ppc/kernel/irq.c +4 -1

@@ -46,6 +46,7 @@
 #include <linux/random.h>
 #include <linux/seq_file.h>
 #include <linux/cpumask.h>
+#include <linux/kgdb.h>
 
 #include <asm/uaccess.h>
 #include <asm/bitops.h>

@@ -536,7 +537,9 @@
 	if (irq != -2 && first)
 		/* That's not SMP safe ... but who cares ? */
 		ppc_spurious_interrupts++;
-        irq_exit();
+	irq_exit();
+
+	kgdb_process_breakpoint();
 }
 
 unsigned long probe_irq_on (void)

diff -Nru a/arch/x86_64/kernel/irq.c b/arch/x86_64/kernel/irq.c

arch/x86_64/kernel/irq.c +2 -0

@@ -405,6 +405,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 	return 1;
 }
 

diff -Nru a/drivers/net/kgdb_eth.c b/drivers/net/kgdb_eth.c

drivers/net/kgdb_eth.c +4 -7

@@ -60,7 +60,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 = {

@@ -106,14 +105,12 @@
 
 	np->remote_port = port;
 
-	/* Is this gdb trying to attach? */
-	if (!netpoll_trap() && len == 8 && !strncmp(msg, "$Hc-1#09", 8))
-		breakpoint();
+	/* Is this gdb trying to attach (!kgdb_connected) or break in
+	 * (msg[0] == 3) ? */
+	if (!netpoll_trap() && (!kgdb_connected || msg[0] == 3))
+		 kgdb_schedule_breakpoint();
 
 	for (i = 0; i < len; i++) {
-		if (msg[i] == 3)
-			breakpoint();
-
 		if (atomic_read(&in_count) >= IN_BUF_SIZE) {
 			/* buffer overflow, clear it */
 			in_head = in_tail = 0;

diff -Nru a/drivers/serial/kgdb_8250.c b/drivers/serial/kgdb_8250.c

drivers/serial/kgdb_8250.c +1 -1

@@ -248,7 +248,7 @@
 
 	/* If we get an interrupt, then KGDB is trying to connect. */
 	if (!kgdb_connected) {
-		breakpoint();
+		kgdb_schedule_breakpoint();
 		return IRQ_HANDLED;
 	}
 

diff -Nru a/include/linux/kgdb.h b/include/linux/kgdb.h

include/linux/kgdb.h +15 -1

@@ -11,8 +11,22 @@
 #include <asm/atomic.h>
 #include <linux/debugger.h>
 
+/*
+ * This file should not include ANY others.  This makes it usable
+ * most anywhere without the fear of include order or inclusion.
+ * TODO: Make it so!
+ *
+ * This file may be included all the time.  It is only active if
+ * CONFIG_KGDB is defined, otherwise it stubs out all the macros
+ * and entry points.
+ */
+
+#if defined(CONFIG_KGDB) && !defined(__ASSEMBLY__)
 /* To enter the debugger explicitly. */
-void breakpoint(void);
+extern void breakpoint(void);
+extern void kgdb_schedule_breakpoint(void);
+extern void kgdb_process_breakpoint(void);
+extern volatile int kgdb_connected;
 
 #ifndef KGDB_MAX_NO_CPUS
 #if CONFIG_NR_CPUS > 8

include/linux/kgdb.h +3 -0

+#else
+#define kgdb_process_breakpoint()      do {} while(0)
+#endif /* KGDB && !__ASSEMBLY__ */
 #endif				/* _KGDB_H_ */

diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c

kernel/kgdb.c +23 -0

@@ -1169,6 +1169,29 @@
 	printk("Connected.\n");
 }
 
+/*
+ * Sometimes we need to schedule a breakpoint because we can't break
+ * right where we are.
+ */
+static int kgdb_need_breakpoint[NR_CPUS];
+
+void kgdb_schedule_breakpoint(void)
+{
+	kgdb_need_breakpoint[smp_processor_id()] = 1;
+}
+
+void kgdb_process_breakpoint(void)
+{
+	/*
+	 * Handle a breakpoint queued from inside network driver code
+	  * to avoid reentrancy issues
+	 */
+	if (kgdb_need_breakpoint[smp_processor_id()]) {
+		 kgdb_need_breakpoint[smp_processor_id()] = 0;
+		 breakpoint();
+	}
+}
+
 #ifdef CONFIG_KGDB_CONSOLE
 char kgdbconbuf[BUFMAX];
 

If you are always inserting after irq_exit(), why not modify irq_exit()? Makes a cleaner patch.

Quoted 1 line · George Anzinger

If you are always inserting after irq_exit(), why not modify irq_exit()? Makes a cleaner patch.

irq_exit() is in <asm/hardirq.h>, so it doesn't buy us anything in terms of files modified.

The following adds, and then makes use of kgdb_process_breakpoint / kgdb_schedule_breakpoint. Using it i kgdb_8250.c isn't strictly needed, but it isn't wrong either.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1663 -> 1.1664
# arch/i386/kernel/irq.c 1.48 -> 1.49
# drivers/net/kgdb_eth.c 1.2 -> 1.3
# arch/x86_64/kernel/irq.c 1.21 -> 1.22
# drivers/serial/kgdb_8250.c 1.3 -> 1.4
# kernel/kgdb.c 1.3 -> 1.4
# arch/ppc/kernel/irq.c 1.36 -> 1.37
# include/linux/kgdb.h 1.3 -> 1.4
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 [email protected] 1.1664

# process_breakpoint/schedule_breakpoint.
# --------------------------------------------
#
diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c

arch/i386/kernel/irq.c +3 -0

@@ -34,6 +34,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/kallsyms.h>
+#include <linux/kgdb.h>
 
 #include <asm/atomic.h>
 #include <asm/io.h>

@@ -507,6 +508,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 
 	return 1;
 }

diff -Nru a/arch/ppc/kernel/irq.c b/arch/ppc/kernel/irq.c

arch/ppc/kernel/irq.c +4 -1

@@ -46,6 +46,7 @@
 #include <linux/random.h>
 #include <linux/seq_file.h>
 #include <linux/cpumask.h>
+#include <linux/kgdb.h>
 
 #include <asm/uaccess.h>
 #include <asm/bitops.h>

@@ -536,7 +537,9 @@
 	if (irq != -2 && first)
 		/* That's not SMP safe ... but who cares ? */
 		ppc_spurious_interrupts++;
-        irq_exit();
+	irq_exit();
+
+	kgdb_process_breakpoint();
 }
 
 unsigned long probe_irq_on (void)

diff -Nru a/arch/x86_64/kernel/irq.c b/arch/x86_64/kernel/irq.c

arch/x86_64/kernel/irq.c +2 -0

@@ -405,6 +405,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 	return 1;
 }
 

diff -Nru a/drivers/net/kgdb_eth.c b/drivers/net/kgdb_eth.c

drivers/net/kgdb_eth.c +4 -7

@@ -60,7 +60,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 = {

@@ -106,14 +105,12 @@
 
 	np->remote_port = port;
 
-	/* Is this gdb trying to attach? */
-	if (!netpoll_trap() && len == 8 && !strncmp(msg, "$Hc-1#09", 8))
-		breakpoint();
+	/* Is this gdb trying to attach (!kgdb_connected) or break in
+	 * (msg[0] == 3) ? */
+	if (!netpoll_trap() && (!kgdb_connected || msg[0] == 3))
+		 kgdb_schedule_breakpoint();
 
 	for (i = 0; i < len; i++) {
-		if (msg[i] == 3)
-			breakpoint();
-
 		if (atomic_read(&in_count) >= IN_BUF_SIZE) {
 			/* buffer overflow, clear it */
 			in_head = in_tail = 0;

diff -Nru a/drivers/serial/kgdb_8250.c b/drivers/serial/kgdb_8250.c

drivers/serial/kgdb_8250.c +1 -1

@@ -248,7 +248,7 @@
 
 	/* If we get an interrupt, then KGDB is trying to connect. */
 	if (!kgdb_connected) {
-		breakpoint();
+		kgdb_schedule_breakpoint();
 		return IRQ_HANDLED;
 	}
 

diff -Nru a/include/linux/kgdb.h b/include/linux/kgdb.h

include/linux/kgdb.h +18 -1

@@ -11,8 +11,22 @@
 #include <asm/atomic.h>
 #include <linux/debugger.h>
 
+/*
+ * This file should not include ANY others.  This makes it usable
+ * most anywhere without the fear of include order or inclusion.
+ * TODO: Make it so!
+ *
+ * This file may be included all the time.  It is only active if
+ * CONFIG_KGDB is defined, otherwise it stubs out all the macros
+ * and entry points.
+ */
+
+#if defined(CONFIG_KGDB) && !defined(__ASSEMBLY__)
 /* To enter the debugger explicitly. */
-void breakpoint(void);
+extern void breakpoint(void);
+extern void kgdb_schedule_breakpoint(void);
+extern void kgdb_process_breakpoint(void);
+extern volatile int kgdb_connected;
 
 #ifndef KGDB_MAX_NO_CPUS
 #if CONFIG_NR_CPUS > 8

@@ -112,4 +126,7 @@
 char *kgdb_hex2mem(char *buf, char *mem, int count);
 int kgdb_get_mem(char *addr, unsigned char *buf, int count);
 
+#else
+#define kgdb_process_breakpoint()      do {} while(0)
+#endif /* KGDB && !__ASSEMBLY__ */
 #endif				/* _KGDB_H_ */

diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c

kernel/kgdb.c +23 -0

@@ -1169,6 +1169,29 @@
 	printk("Connected.\n");
 }
 
+/*
+ * Sometimes we need to schedule a breakpoint because we can't break
+ * right where we are.
+ */
+static int kgdb_need_breakpoint[NR_CPUS];
+
+void kgdb_schedule_breakpoint(void)
+{
+	kgdb_need_breakpoint[smp_processor_id()] = 1;
+}
+
+void kgdb_process_breakpoint(void)
+{
+	/*
+	 * Handle a breakpoint queued from inside network driver code
+	  * to avoid reentrancy issues
+	 */
+	if (kgdb_need_breakpoint[smp_processor_id()]) {
+		 kgdb_need_breakpoint[smp_processor_id()] = 0;
+		 breakpoint();
+	}
+}
+
 #ifdef CONFIG_KGDB_CONSOLE
 char kgdbconbuf[BUFMAX];

Quoted 3 lines · Tom Rini

The following adds, and then makes use of kgdb_process_breakpoint / kgdb_schedule_breakpoint. Using it i kgdb_8250.c isn't strictly needed, but it isn't wrong either.

That makes 8250 response it a _bit_ slower. A user will notice when kgdb doesn't respond within a millisecond of pressing Ctrl+C :-)

OK to checkin.
-Amit

Quoted 172 lines

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1663 -> 1.1664
# arch/i386/kernel/irq.c 1.48 -> 1.49
# drivers/net/kgdb_eth.c 1.2 -> 1.3
# arch/x86_64/kernel/irq.c 1.21 -> 1.22
# drivers/serial/kgdb_8250.c 1.3 -> 1.4
# kernel/kgdb.c 1.3 -> 1.4
# arch/ppc/kernel/irq.c 1.36 -> 1.37
# include/linux/kgdb.h 1.3 -> 1.4
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 [email protected] 1.1664

# process_breakpoint/schedule_breakpoint.
# --------------------------------------------
#
diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c

arch/i386/kernel/irq.c +3 -0

@@ -34,6 +34,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/kallsyms.h>
+#include <linux/kgdb.h>
 
 #include <asm/atomic.h>
 #include <asm/io.h>

@@ -507,6 +508,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 
 	return 1;
 }

diff -Nru a/arch/ppc/kernel/irq.c b/arch/ppc/kernel/irq.c

arch/ppc/kernel/irq.c +4 -1

@@ -46,6 +46,7 @@
 #include <linux/random.h>
 #include <linux/seq_file.h>
 #include <linux/cpumask.h>
+#include <linux/kgdb.h>
 
 #include <asm/uaccess.h>
 #include <asm/bitops.h>

@@ -536,7 +537,9 @@
 	if (irq != -2 && first)
 		/* That's not SMP safe ... but who cares ? */
 		ppc_spurious_interrupts++;
-        irq_exit();
+	irq_exit();
+
+	kgdb_process_breakpoint();
 }
 
 unsigned long probe_irq_on (void)

diff -Nru a/arch/x86_64/kernel/irq.c b/arch/x86_64/kernel/irq.c

arch/x86_64/kernel/irq.c +2 -0

@@ -405,6 +405,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 	return 1;
 }
 

diff -Nru a/drivers/net/kgdb_eth.c b/drivers/net/kgdb_eth.c

drivers/net/kgdb_eth.c +0 -0

@@ -60,7 +60,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 = {

drivers/net/kgdb_eth.c +4 -6

@@ -106,14 +105,12 @@
 
 	np->remote_port = port;
 
-	/* Is this gdb trying to attach? */
-	if (!netpoll_trap() && len == 8 && !strncmp(msg, "$Hc-1#09", 8))
-		breakpoint();
+	/* Is this gdb trying to attach (!kgdb_connected) or break in
+	 * (msg[0] == 3) ? */
+	if (!netpoll_trap() && (!kgdb_connected || msg[0] == 3))
+		 kgdb_schedule_breakpoint();
 
 	for (i = 0; i < len; i++) {
-		if (msg[i] == 3)
-			breakpoint();
-
 		if (atomic_read(&in_count) >= IN_BUF_SIZE) {
 			/* buffer overflow, clear it */
 			in_head = in_tail = 0;

diff -Nru a/drivers/serial/kgdb_8250.c b/drivers/serial/kgdb_8250.c

drivers/serial/kgdb_8250.c +1 -1

@@ -248,7 +248,7 @@
 
 	/* If we get an interrupt, then KGDB is trying to connect. */
 	if (!kgdb_connected) {
-		breakpoint();
+		kgdb_schedule_breakpoint();
 		return IRQ_HANDLED;
 	}
 

diff -Nru a/include/linux/kgdb.h b/include/linux/kgdb.h

include/linux/kgdb.h +18 -1

@@ -11,8 +11,22 @@
 #include <asm/atomic.h>
 #include <linux/debugger.h>
 
+/*
+ * This file should not include ANY others.  This makes it usable
+ * most anywhere without the fear of include order or inclusion.
+ * TODO: Make it so!
+ *
+ * This file may be included all the time.  It is only active if
+ * CONFIG_KGDB is defined, otherwise it stubs out all the macros
+ * and entry points.
+ */
+
+#if defined(CONFIG_KGDB) && !defined(__ASSEMBLY__)
 /* To enter the debugger explicitly. */
-void breakpoint(void);
+extern void breakpoint(void);
+extern void kgdb_schedule_breakpoint(void);
+extern void kgdb_process_breakpoint(void);
+extern volatile int kgdb_connected;
 
 #ifndef KGDB_MAX_NO_CPUS
 #if CONFIG_NR_CPUS > 8

@@ -112,4 +126,7 @@
 char *kgdb_hex2mem(char *buf, char *mem, int count);
 int kgdb_get_mem(char *addr, unsigned char *buf, int count);
 
+#else
+#define kgdb_process_breakpoint()      do {} while(0)
+#endif /* KGDB && !__ASSEMBLY__ */
 #endif				/* _KGDB_H_ */

diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c

kernel/kgdb.c +15 -0

@@ -1169,6 +1169,29 @@
 	printk("Connected.\n");
 }
 
+/*
+ * Sometimes we need to schedule a breakpoint because we can't break
+ * right where we are.
+ */
+static int kgdb_need_breakpoint[NR_CPUS];
+
+void kgdb_schedule_breakpoint(void)
+{
+	kgdb_need_breakpoint[smp_processor_id()] = 1;
+}
+
+void kgdb_process_breakpoint(void)
+{
+	/*
+	 * Handle a breakpoint queued from inside network driver code

kernel/kgdb.c line 1196

Quoted 10 lines

kernel/kgdb.c +8 -0

+	  * to avoid reentrancy issues
+	 */
+	if (kgdb_need_breakpoint[smp_processor_id()]) {
+		 kgdb_need_breakpoint[smp_processor_id()] = 0;
+		 breakpoint();
+	}
+}
+
 #ifdef CONFIG_KGDB_CONSOLE
 char kgdbconbuf[BUFMAX];

-
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 5 lines · Amit S. Kale

Quoted 3 lines · Tom Rini

The following adds, and then makes use of kgdb_process_breakpoint / kgdb_schedule_breakpoint. Using it i kgdb_8250.c isn't strictly needed, but it isn't wrong either.

Quoted 2 lines

That makes 8250 response it a _bit_ slower. A user will notice when kgdb doesn't respond within a millisecond of pressing Ctrl+C :-)

Hm... I have been wondering if it might not be a good idea to put some comments to the user at or around the breakpoint. Possibly we might want to tell the user about where the info files are or some such. This would then come up on his screen when the source code at the breakpoint is displayed.

comments...

-g

Quoted 191 lines

OK to checkin.
-Amit

Quoted 172 lines

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1663 -> 1.1664
# arch/i386/kernel/irq.c 1.48 -> 1.49
# drivers/net/kgdb_eth.c 1.2 -> 1.3
# arch/x86_64/kernel/irq.c 1.21 -> 1.22
# drivers/serial/kgdb_8250.c 1.3 -> 1.4
# kernel/kgdb.c 1.3 -> 1.4
# arch/ppc/kernel/irq.c 1.36 -> 1.37
# include/linux/kgdb.h 1.3 -> 1.4
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 [email protected] 1.1664

# process_breakpoint/schedule_breakpoint.
# --------------------------------------------
#
diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c

arch/i386/kernel/irq.c +3 -0

@@ -34,6 +34,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/kallsyms.h>
+#include <linux/kgdb.h>
 
 #include <asm/atomic.h>
 #include <asm/io.h>

@@ -507,6 +508,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 
 	return 1;
 }

diff -Nru a/arch/ppc/kernel/irq.c b/arch/ppc/kernel/irq.c

arch/ppc/kernel/irq.c +4 -1

@@ -46,6 +46,7 @@
 #include <linux/random.h>
 #include <linux/seq_file.h>
 #include <linux/cpumask.h>
+#include <linux/kgdb.h>
 
 #include <asm/uaccess.h>
 #include <asm/bitops.h>

@@ -536,7 +537,9 @@
 	if (irq != -2 && first)
 		/* That's not SMP safe ... but who cares ? */
 		ppc_spurious_interrupts++;
-        irq_exit();
+	irq_exit();
+
+	kgdb_process_breakpoint();
 }
 
 unsigned long probe_irq_on (void)

diff -Nru a/arch/x86_64/kernel/irq.c b/arch/x86_64/kernel/irq.c

arch/x86_64/kernel/irq.c +2 -0

@@ -405,6 +405,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 	return 1;
 }
 

diff -Nru a/drivers/net/kgdb_eth.c b/drivers/net/kgdb_eth.c

drivers/net/kgdb_eth.c +0 -0

@@ -60,7 +60,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 = {

drivers/net/kgdb_eth.c +4 -6

@@ -106,14 +105,12 @@
 
 	np->remote_port = port;
 
-	/* Is this gdb trying to attach? */
-	if (!netpoll_trap() && len == 8 && !strncmp(msg, "$Hc-1#09", 8))
-		breakpoint();
+	/* Is this gdb trying to attach (!kgdb_connected) or break in
+	 * (msg[0] == 3) ? */
+	if (!netpoll_trap() && (!kgdb_connected || msg[0] == 3))
+		 kgdb_schedule_breakpoint();
 
 	for (i = 0; i < len; i++) {
-		if (msg[i] == 3)
-			breakpoint();
-
 		if (atomic_read(&in_count) >= IN_BUF_SIZE) {
 			/* buffer overflow, clear it */
 			in_head = in_tail = 0;

diff -Nru a/drivers/serial/kgdb_8250.c b/drivers/serial/kgdb_8250.c

drivers/serial/kgdb_8250.c +1 -1

@@ -248,7 +248,7 @@
 
 	/* If we get an interrupt, then KGDB is trying to connect. */
 	if (!kgdb_connected) {
-		breakpoint();
+		kgdb_schedule_breakpoint();
 		return IRQ_HANDLED;
 	}
 

diff -Nru a/include/linux/kgdb.h b/include/linux/kgdb.h

include/linux/kgdb.h +18 -1

@@ -11,8 +11,22 @@
 #include <asm/atomic.h>
 #include <linux/debugger.h>
 
+/*
+ * This file should not include ANY others.  This makes it usable
+ * most anywhere without the fear of include order or inclusion.
+ * TODO: Make it so!
+ *
+ * This file may be included all the time.  It is only active if
+ * CONFIG_KGDB is defined, otherwise it stubs out all the macros
+ * and entry points.
+ */
+
+#if defined(CONFIG_KGDB) && !defined(__ASSEMBLY__)
 /* To enter the debugger explicitly. */
-void breakpoint(void);
+extern void breakpoint(void);
+extern void kgdb_schedule_breakpoint(void);
+extern void kgdb_process_breakpoint(void);
+extern volatile int kgdb_connected;
 
 #ifndef KGDB_MAX_NO_CPUS
 #if CONFIG_NR_CPUS > 8

@@ -112,4 +126,7 @@
 char *kgdb_hex2mem(char *buf, char *mem, int count);
 int kgdb_get_mem(char *addr, unsigned char *buf, int count);
 
+#else
+#define kgdb_process_breakpoint()      do {} while(0)
+#endif /* KGDB && !__ASSEMBLY__ */
 #endif				/* _KGDB_H_ */

diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c

kernel/kgdb.c +15 -0

@@ -1169,6 +1169,29 @@
 	printk("Connected.\n");
 }
 
+/*
+ * Sometimes we need to schedule a breakpoint because we can't break
+ * right where we are.
+ */
+static int kgdb_need_breakpoint[NR_CPUS];
+
+void kgdb_schedule_breakpoint(void)
+{
+	kgdb_need_breakpoint[smp_processor_id()] = 1;
+}
+
+void kgdb_process_breakpoint(void)
+{
+	/*
+	 * Handle a breakpoint queued from inside network driver code

kernel/kgdb.c line 1196

Quoted 10 lines

kernel/kgdb.c +8 -0

+	  * to avoid reentrancy issues
+	 */
+	if (kgdb_need_breakpoint[smp_processor_id()]) {
+		 kgdb_need_breakpoint[smp_processor_id()] = 0;
+		 breakpoint();
+	}
+}
+
 #ifdef CONFIG_KGDB_CONSOLE
 char kgdbconbuf[BUFMAX];

-
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 8 lines · George Anzinger

Quoted 6 lines · Amit S. Kale

Quoted 3 lines · Tom Rini

The following adds, and then makes use of kgdb_process_breakpoint / kgdb_schedule_breakpoint. Using it i kgdb_8250.c isn't strictly needed, but it isn't wrong either.

Quoted 8 lines

Quoted 2 lines

That makes 8250 response it a _bit_ slower. A user will notice when kgdb doesn't respond within a millisecond of pressing Ctrl+C :-)

Hm... I have been wondering if it might not be a good idea to put some comments to the user at or around the breakpoint. Possibly we might want to tell the user about where the info files are or some such. This would then come up on his screen when the source code at the breakpoint is displayed.

Err, well, I don't seem to understand this.

Do you mean we print a (gdb) console message that indicates something about a breakpoint? If we do that, there has to be a way to turn it off. I have a (rather bad) habit of stepping through kernel code :-)

What are info files?

-Amit

Quoted 197 lines

comments...

-g

Quoted 192 lines

OK to checkin.
-Amit

Quoted 172 lines

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1663 -> 1.1664
# arch/i386/kernel/irq.c 1.48 -> 1.49
# drivers/net/kgdb_eth.c 1.2 -> 1.3
# arch/x86_64/kernel/irq.c 1.21 -> 1.22
# drivers/serial/kgdb_8250.c 1.3 -> 1.4
# kernel/kgdb.c 1.3 -> 1.4
# arch/ppc/kernel/irq.c 1.36 -> 1.37
# include/linux/kgdb.h 1.3 -> 1.4
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 [email protected] 1.1664

# process_breakpoint/schedule_breakpoint.
# --------------------------------------------
#
diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c

arch/i386/kernel/irq.c +3 -0

@@ -34,6 +34,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/kallsyms.h>
+#include <linux/kgdb.h>
 
 #include <asm/atomic.h>
 #include <asm/io.h>

@@ -507,6 +508,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 
 	return 1;
 }

diff -Nru a/arch/ppc/kernel/irq.c b/arch/ppc/kernel/irq.c

arch/ppc/kernel/irq.c +4 -1

@@ -46,6 +46,7 @@
 #include <linux/random.h>
 #include <linux/seq_file.h>
 #include <linux/cpumask.h>
+#include <linux/kgdb.h>
 
 #include <asm/uaccess.h>
 #include <asm/bitops.h>

@@ -536,7 +537,9 @@
 	if (irq != -2 && first)
 		/* That's not SMP safe ... but who cares ? */
 		ppc_spurious_interrupts++;
-        irq_exit();
+	irq_exit();
+
+	kgdb_process_breakpoint();
 }
 
 unsigned long probe_irq_on (void)

diff -Nru a/arch/x86_64/kernel/irq.c b/arch/x86_64/kernel/irq.c

arch/x86_64/kernel/irq.c +2 -0

@@ -405,6 +405,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 	return 1;
 }
 

diff -Nru a/drivers/net/kgdb_eth.c b/drivers/net/kgdb_eth.c

drivers/net/kgdb_eth.c +0 -0

@@ -60,7 +60,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 = {

drivers/net/kgdb_eth.c +4 -6

@@ -106,14 +105,12 @@
 
 	np->remote_port = port;
 
-	/* Is this gdb trying to attach? */
-	if (!netpoll_trap() && len == 8 && !strncmp(msg, "$Hc-1#09", 8))
-		breakpoint();
+	/* Is this gdb trying to attach (!kgdb_connected) or break in
+	 * (msg[0] == 3) ? */
+	if (!netpoll_trap() && (!kgdb_connected || msg[0] == 3))
+		 kgdb_schedule_breakpoint();
 
 	for (i = 0; i < len; i++) {
-		if (msg[i] == 3)
-			breakpoint();
-
 		if (atomic_read(&in_count) >= IN_BUF_SIZE) {
 			/* buffer overflow, clear it */
 			in_head = in_tail = 0;

diff -Nru a/drivers/serial/kgdb_8250.c b/drivers/serial/kgdb_8250.c

drivers/serial/kgdb_8250.c +1 -1

@@ -248,7 +248,7 @@
 
 	/* If we get an interrupt, then KGDB is trying to connect. */
 	if (!kgdb_connected) {
-		breakpoint();
+		kgdb_schedule_breakpoint();
 		return IRQ_HANDLED;
 	}
 

diff -Nru a/include/linux/kgdb.h b/include/linux/kgdb.h

include/linux/kgdb.h +18 -1

@@ -11,8 +11,22 @@
 #include <asm/atomic.h>
 #include <linux/debugger.h>
 
+/*
+ * This file should not include ANY others.  This makes it usable
+ * most anywhere without the fear of include order or inclusion.
+ * TODO: Make it so!
+ *
+ * This file may be included all the time.  It is only active if
+ * CONFIG_KGDB is defined, otherwise it stubs out all the macros
+ * and entry points.
+ */
+
+#if defined(CONFIG_KGDB) && !defined(__ASSEMBLY__)
 /* To enter the debugger explicitly. */
-void breakpoint(void);
+extern void breakpoint(void);
+extern void kgdb_schedule_breakpoint(void);
+extern void kgdb_process_breakpoint(void);
+extern volatile int kgdb_connected;
 
 #ifndef KGDB_MAX_NO_CPUS
 #if CONFIG_NR_CPUS > 8

@@ -112,4 +126,7 @@
 char *kgdb_hex2mem(char *buf, char *mem, int count);
 int kgdb_get_mem(char *addr, unsigned char *buf, int count);
 
+#else
+#define kgdb_process_breakpoint()      do {} while(0)
+#endif /* KGDB && !__ASSEMBLY__ */
 #endif				/* _KGDB_H_ */

diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c

kernel/kgdb.c +15 -0

@@ -1169,6 +1169,29 @@
 	printk("Connected.\n");
 }
 
+/*
+ * Sometimes we need to schedule a breakpoint because we can't break
+ * right where we are.
+ */
+static int kgdb_need_breakpoint[NR_CPUS];
+
+void kgdb_schedule_breakpoint(void)
+{
+	kgdb_need_breakpoint[smp_processor_id()] = 1;
+}
+
+void kgdb_process_breakpoint(void)
+{
+	/*
+	 * Handle a breakpoint queued from inside network driver code

kernel/kgdb.c line 1196

Quoted 10 lines

kernel/kgdb.c +8 -0

+	  * to avoid reentrancy issues
+	 */
+	if (kgdb_need_breakpoint[smp_processor_id()]) {
+		 kgdb_need_breakpoint[smp_processor_id()] = 0;
+		 breakpoint();
+	}
+}
+
 #ifdef CONFIG_KGDB_CONSOLE
 char kgdbconbuf[BUFMAX];

-
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

Quoted 6 lines · George Anzinger

Quoted 5 lines · Amit S. Kale

Quoted 3 lines · Tom Rini

The following adds, and then makes use of kgdb_process_breakpoint / kgdb_schedule_breakpoint. Using it i kgdb_8250.c isn't strictly needed, but it isn't wrong either.

Quoted 8 lines

Quoted 2 lines

That makes 8250 response it a _bit_ slower. A user will notice when kgdb doesn't respond within a millisecond of pressing Ctrl+C :-)

Hm... I have been wondering if it might not be a good idea to put some comments to the user at or around the breakpoint. Possibly we might want to tell the user about where the info files are or some such. This would then come up on his screen when the source code at the breakpoint is displayed.

Quoted 5 lines

Err, well, I don't seem to understand this.

Do you mean we print a (gdb) console message that indicates something about a breakpoint? If we do that, there has to be a way to turn it off. I have a (rather bad) habit of stepping through kernel code :-)

No that is not what I mean. I don't want to try to send messages to the gdb console (it is not supported by gdb at this time). Rather, the hard coded breakpoint instruction that we use to get to the the stub when the user enters a ^C is in a particular bit of source code. Most gdb front ends display this bit of source centered on the breakpoint instruction. What I am asking about is puting something useful here from the user point of view. For example we might have this:

/*
* This is the KGDB control C break point. For additional info on KGDB
options * and suggested macros see .../Documentation/kgdb/* in your kernel tree. * KGDB version XX.YY
*/

	BREAKPOINT;

--------------------------------------

Lets face it, this bit of source is most likly of very little interest to most users. I.e. it is a free page on which we can post what ever we want.

Quoted 199 lines

What are info files?

-Amit

Quoted 195 lines

comments...

-g

Quoted 191 lines

OK to checkin.
-Amit

Quoted 172 lines

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1663 -> 1.1664
# arch/i386/kernel/irq.c 1.48 -> 1.49
# drivers/net/kgdb_eth.c 1.2 -> 1.3
# arch/x86_64/kernel/irq.c 1.21 -> 1.22
# drivers/serial/kgdb_8250.c 1.3 -> 1.4
# kernel/kgdb.c 1.3 -> 1.4
# arch/ppc/kernel/irq.c 1.36 -> 1.37
# include/linux/kgdb.h 1.3 -> 1.4
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 [email protected] 1.1664

# process_breakpoint/schedule_breakpoint.
# --------------------------------------------
#
diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c

arch/i386/kernel/irq.c +3 -0

@@ -34,6 +34,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/kallsyms.h>
+#include <linux/kgdb.h>
 
 #include <asm/atomic.h>
 #include <asm/io.h>

@@ -507,6 +508,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 
 	return 1;
 }

diff -Nru a/arch/ppc/kernel/irq.c b/arch/ppc/kernel/irq.c

arch/ppc/kernel/irq.c +4 -1

@@ -46,6 +46,7 @@
 #include <linux/random.h>
 #include <linux/seq_file.h>
 #include <linux/cpumask.h>
+#include <linux/kgdb.h>
 
 #include <asm/uaccess.h>
 #include <asm/bitops.h>

@@ -536,7 +537,9 @@
 	if (irq != -2 && first)
 		/* That's not SMP safe ... but who cares ? */
 		ppc_spurious_interrupts++;
-        irq_exit();
+	irq_exit();
+
+	kgdb_process_breakpoint();
 }
 
 unsigned long probe_irq_on (void)

diff -Nru a/arch/x86_64/kernel/irq.c b/arch/x86_64/kernel/irq.c

arch/x86_64/kernel/irq.c +2 -0

@@ -405,6 +405,8 @@
 	spin_unlock(&desc->lock);
 
 	irq_exit();
+
+	kgdb_process_breakpoint();
 	return 1;
 }
 

diff -Nru a/drivers/net/kgdb_eth.c b/drivers/net/kgdb_eth.c

drivers/net/kgdb_eth.c +0 -0

@@ -60,7 +60,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 = {

drivers/net/kgdb_eth.c +4 -6

@@ -106,14 +105,12 @@
 
 	np->remote_port = port;
 
-	/* Is this gdb trying to attach? */
-	if (!netpoll_trap() && len == 8 && !strncmp(msg, "$Hc-1#09", 8))
-		breakpoint();
+	/* Is this gdb trying to attach (!kgdb_connected) or break in
+	 * (msg[0] == 3) ? */
+	if (!netpoll_trap() && (!kgdb_connected || msg[0] == 3))
+		 kgdb_schedule_breakpoint();
 
 	for (i = 0; i < len; i++) {
-		if (msg[i] == 3)
-			breakpoint();
-
 		if (atomic_read(&in_count) >= IN_BUF_SIZE) {
 			/* buffer overflow, clear it */
 			in_head = in_tail = 0;

diff -Nru a/drivers/serial/kgdb_8250.c b/drivers/serial/kgdb_8250.c

drivers/serial/kgdb_8250.c +1 -1

@@ -248,7 +248,7 @@
 
 	/* If we get an interrupt, then KGDB is trying to connect. */
 	if (!kgdb_connected) {
-		breakpoint();
+		kgdb_schedule_breakpoint();
 		return IRQ_HANDLED;
 	}
 

diff -Nru a/include/linux/kgdb.h b/include/linux/kgdb.h

include/linux/kgdb.h +18 -1

@@ -11,8 +11,22 @@
 #include <asm/atomic.h>
 #include <linux/debugger.h>
 
+/*
+ * This file should not include ANY others.  This makes it usable
+ * most anywhere without the fear of include order or inclusion.
+ * TODO: Make it so!
+ *
+ * This file may be included all the time.  It is only active if
+ * CONFIG_KGDB is defined, otherwise it stubs out all the macros
+ * and entry points.
+ */
+
+#if defined(CONFIG_KGDB) && !defined(__ASSEMBLY__)
 /* To enter the debugger explicitly. */
-void breakpoint(void);
+extern void breakpoint(void);
+extern void kgdb_schedule_breakpoint(void);
+extern void kgdb_process_breakpoint(void);
+extern volatile int kgdb_connected;
 
 #ifndef KGDB_MAX_NO_CPUS
 #if CONFIG_NR_CPUS > 8

@@ -112,4 +126,7 @@
 char *kgdb_hex2mem(char *buf, char *mem, int count);
 int kgdb_get_mem(char *addr, unsigned char *buf, int count);
 
+#else
+#define kgdb_process_breakpoint()      do {} while(0)
+#endif /* KGDB && !__ASSEMBLY__ */
 #endif				/* _KGDB_H_ */

diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c

kernel/kgdb.c +15 -0

@@ -1169,6 +1169,29 @@
 	printk("Connected.\n");
 }
 
+/*
+ * Sometimes we need to schedule a breakpoint because we can't break
+ * right where we are.
+ */
+static int kgdb_need_breakpoint[NR_CPUS];
+
+void kgdb_schedule_breakpoint(void)
+{
+	kgdb_need_breakpoint[smp_processor_id()] = 1;
+}
+
+void kgdb_process_breakpoint(void)
+{
+	/*
+	 * Handle a breakpoint queued from inside network driver code

kernel/kgdb.c line 1196

Quoted 10 lines

kernel/kgdb.c +8 -0

+	  * to avoid reentrancy issues
+	 */
+	if (kgdb_need_breakpoint[smp_processor_id()]) {
+		 kgdb_need_breakpoint[smp_processor_id()] = 0;
+		 breakpoint();
+	}
+}
+
 #ifdef CONFIG_KGDB_CONSOLE
 char kgdbconbuf[BUFMAX];

-
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 41 lines · George Anzinger

Quoted 19 lines · Amit S. Kale

Quoted 8 lines · George Anzinger

Quoted 6 lines · Amit S. Kale

Quoted 3 lines · Tom Rini

The following adds, and then makes use of kgdb_process_breakpoint / kgdb_schedule_breakpoint. Using it i kgdb_8250.c isn't strictly needed, but it isn't wrong either.

Quoted 8 lines

Quoted 2 lines

That makes 8250 response it a _bit_ slower. A user will notice when kgdb doesn't respond within a millisecond of pressing Ctrl+C :-)

Hm... I have been wondering if it might not be a good idea to put some comments to the user at or around the breakpoint. Possibly we might want to tell the user about where the info files are or some such. This would then come up on his screen when the source code at the breakpoint is displayed.

Quoted 5 lines

Err, well, I don't seem to understand this.

Do you mean we print a (gdb) console message that indicates something about a breakpoint? If we do that, there has to be a way to turn it off. I have a (rather bad) habit of stepping through kernel code :-)

No that is not what I mean. I don't want to try to send messages to the
gdb console (it is not supported by gdb at this time). Rather, the hard
coded breakpoint instruction that we use to get to the the stub when the
user enters a ^C is in a particular bit of source code. Most gdb front
ends display this bit of source centered on the breakpoint instruction. What I am asking about is puting something useful here from the user point of view. For example we might have this:

/*
* This is the KGDB control C break point. For additional info on KGDB
options * and suggested macros see .../Documentation/kgdb/* in your kernel tree. * KGDB version XX.YY
*/

	BREAKPOINT;

Quoted 1 line

--------------------------------------

This is definitely a good point.

We may also report the exact location where kernel was running when Ctrl+C came in. We have pt_regs available in do_IRQ. We can pass that to process breakpoint function. The process breakpoint function can then straight call handle_exception passing it pt_regs instead of going through breakpoint. That will save some stack also.

Quoted 41 lines · Amit S. Kale

Quoted 31 lines · George Anzinger

Quoted 16 lines · Amit S. Kale

Quoted 6 lines · George Anzinger

Quoted 5 lines · Amit S. Kale

Quoted 3 lines · Tom Rini

The following adds, and then makes use of kgdb_process_breakpoint / kgdb_schedule_breakpoint. Using it i kgdb_8250.c isn't strictly needed, but it isn't wrong either.

Quoted 8 lines

Quoted 2 lines

That makes 8250 response it a _bit_ slower. A user will notice when kgdb doesn't respond within a millisecond of pressing Ctrl+C :-)

Hm... I have been wondering if it might not be a good idea to put some comments to the user at or around the breakpoint. Possibly we might want to tell the user about where the info files are or some such. This would then come up on his screen when the source code at the breakpoint is displayed.

Quoted 5 lines

Err, well, I don't seem to understand this.

Do you mean we print a (gdb) console message that indicates something about a breakpoint? If we do that, there has to be a way to turn it off. I have a (rather bad) habit of stepping through kernel code :-)

No that is not what I mean. I don't want to try to send messages to the gdb console (it is not supported by gdb at this time). Rather, the hard coded breakpoint instruction that we use to get to the the stub when the user enters a ^C is in a particular bit of source code. Most gdb front ends display this bit of source centered on the breakpoint instruction. What I am asking about is puting something useful here from the user point of view. For example we might have this:

Quoted 8 lines

/*
* This is the KGDB control C break point. For additional info on KGDB
options * and suggested macros see .../Documentation/kgdb/* in your kernel tree. * KGDB version XX.YY
*/

	BREAKPOINT;

--------------------------------------

Quoted 7 lines

This is definitely a good point.

We may also report the exact location where kernel was running when Ctrl+C came in. We have pt_regs available in do_IRQ. We can pass that to process breakpoint function. The process breakpoint function can then straight call handle_exception passing it pt_regs instead of going through breakpoint. That will save some stack also.

Uh, I would think that "bt" should cover this. Why muddy the waters with other stuff. I think the user should see exactly where he is going to go on the continue. One really good reason for this is that this is the

context any

p fun()

will run in.

~

Quoted 4 lines

context any

p fun()

p fun() will push arguments on stack over the place where irq occured, which is exactly how it'll run.

Amit S. Kale wrote:
~

Quoted 2 lines

Quoted 2 lines

context any

Quoted 4 lines

Quoted 1 line

p fun()

p fun() will push arguments on stack over the place where irq occured, which is exactly how it'll run.

Is this capability in kgdb lite? It was one of the last things I added to -mm version.

Quoted 8 lines · George Anzinger

Amit S. Kale wrote:
~

Quoted 4 lines

Quoted 3 lines

context any

p fun()

Quoted 5 lines

Quoted 2 lines

p fun() will push arguments on stack over the place where irq occured, which is exactly how it'll run.

Is this capability in kgdb lite? It was one of the last things I added to -mm version.

No! It's not present in kgdb heavy also. All you can do is set $pc, continue.

-Amit

Quoted 14 lines · Amit S. Kale

Quoted 12 lines · George Anzinger

Amit S. Kale wrote:
~

Quoted 6 lines

Quoted 3 lines

context any

p fun()

p fun() will push arguments on stack over the place where irq occured, which is exactly how it'll run.

Is this capability in kgdb lite? It was one of the last things I added to -mm version.

Quoted 2 lines

No! It's not present in kgdb heavy also. All you can do is set $pc, continue.

Possibly I can help here. I did it for the mm version. It does require a couple of asm bits and it sort of messes up the set/fetch memory, but it does do the job.

Quoted 21 lines · George Anzinger

Quoted 18 lines · Amit S. Kale

Quoted 6 lines · George Anzinger

Amit S. Kale wrote:
~

Quoted 2 lines

Quoted 2 lines

context any

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

p fun()

p fun() will push arguments on stack over the place where irq occured, which is exactly how it'll run.

Is this capability in kgdb lite? It was one of the last things I added to -mm version.

No! It's not present in kgdb heavy also. All you can do is set $pc, continue.