[KGDB PATCH][2/7] Serial updates, take 2

[Tom Rini <trini@kernel.crashing.org>]

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

diff -zrupN linux-2.6.3+nothing/arch/ppc/8260_io/uart.c linux-2.6.3+config+serial/arch/ppc/8260_io/uart.c
--- linux-2.6.3+nothing/arch/ppc/8260_io/uart.c	2004-02-27 12:16:14.397164756 -0700
+++ linux-2.6.3+config+serial/arch/ppc/8260_io/uart.c	2004-02-27 12:16:13.765307745 -0700
@@ -396,16 +396,10 @@ static _INLINE_ void receive_chars(ser_i
 #ifdef CONFIG_KGDB
 		if (info->state->smc_scc_num == KGDB_SER_IDX) {
 			while (i-- > 0) {
-				if (*cp == 0x03) {
+				if (*cp == 0x03 || *cp == '$') {
 					breakpoint();
 					return;
 				}
-				if (*cp == '$') {
-					atomic_set(&kgdb_might_be_resumed, 1);
-					breakpoint();
-					atomic_set(&kgdb_might_be_resumed, 0);
-					return;
-				}
 				cp++;
 			}
 			bdp->cbd_sc |= BD_SC_EMPTY;
diff -zrupN linux-2.6.3+nothing/drivers/serial/kgdb_8250.c linux-2.6.3+config+serial/drivers/serial/kgdb_8250.c
--- linux-2.6.3+nothing/drivers/serial/kgdb_8250.c	2004-02-27 12:16:14.557128556 -0700
+++ linux-2.6.3+config+serial/drivers/serial/kgdb_8250.c	2004-02-27 12:16:13.000000000 -0700
@@ -17,7 +17,6 @@
 #include <linux/config.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
-#include <linux/spinlock.h>
 #include <linux/kgdb.h>
 #include <linux/interrupt.h>
 #include <linux/tty.h>
@@ -166,32 +165,15 @@ read_data_bfr(void)
 /*
  * Get a char if available, return -1 if nothing available.
  * Empty the receive buffer first, then look at the interface hardware.
- *
- * Locking here is a bit of a problem.	We MUST not lock out communication
- * if we are trying to talk to gdb about a kgdb entry.	ON the other hand
- * we can loose chars in the console pass thru if we don't lock.  It is also
- * possible that we could hold the lock or be waiting for it when kgdb
- * NEEDS to talk.  Since kgdb locks down the world, it does not need locks.
- * We do, of course have possible issues with interrupting a uart operation,
- * but we will just depend on the uart status to help keep that straight.
  */
 
-static spinlock_t uart_interrupt_lock = SPIN_LOCK_UNLOCKED;
-#ifdef CONFIG_SMP
-extern spinlock_t kgdb_spinlock;
-#endif
-
 int
 kgdb_get_debug_char(void)
 {
 	int retchr;
 	unsigned long flags;
 	local_irq_save(flags);
-#ifdef CONFIG_SMP
-	if (!spin_is_locked(&kgdb_spinlock)) {
-		spin_lock(&uart_interrupt_lock);
-	}
-#endif
+
 	/* intr routine has q'd chars */
 	if (atomic_read(&kgdb8250_buf_in_cnt) != 0) {
 		retchr = kgdb8250_buf[kgdb8250_buf_out_inx++];
@@ -205,11 +187,6 @@ kgdb_get_debug_char(void)
 	} while (retchr < 0);
 
 out:
-#ifdef CONFIG_SMP
-	if (!spin_is_locked(&kgdb_spinlock)) {
-		spin_unlock(&uart_interrupt_lock);
-	}
-#endif
 	local_irq_restore(flags);
 
 	return retchr;
@@ -217,72 +194,17 @@ out:
 
 /*
  * This is the receiver interrupt routine for the GDB stub.
- * It will receive a limited number of characters of input
- * from the gdb  host machine and save them up in a buffer.
- *
- * When kgdb_get_debug_char() is called it
- * draws characters out of the buffer until it is empty and
- * then reads directly from the serial port.
- *
- * We do not attempt to write chars from the interrupt routine
- * since the stubs do all of that via kgdb_put_debug_char() which
- * writes one byte after waiting for the interface to become
- * ready.
- *
- * The debug stubs like to run with interrupts disabled since,
- * after all, they run as a consequence of a breakpoint in
- * the kernel.
- *
- * Perhaps someone who knows more about the tty driver than I
- * care to learn can make this work for any low level serial
- * driver.
+ * All that we need to do is verify that the interrupt happened on the
+ * line we're in charge of.  If this is true, schedule a breakpoint and
+ * return.
  */
 static irqreturn_t
 kgdb8250_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 {
-	int chr, iir;
-	unsigned long flags;
-
 	if (irq != gdb_async_info.line)
 		return IRQ_NONE;
 
-	/* If we get an interrupt, then KGDB is trying to connect. */
-	if (!kgdb_connected) {
-		kgdb_schedule_breakpoint();
-		return IRQ_HANDLED;
-	}
-
-	local_irq_save(flags);
-	spin_lock(&uart_interrupt_lock);
-
-	do {
-		chr = read_data_bfr();
-		iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
-		if (chr < 0)
-			continue;
-
-		/* Check whether gdb sent interrupt */
-		if (chr == 3) {
-			breakpoint();
-			continue;
-		}
-
-		if (atomic_read(&kgdb8250_buf_in_cnt) >= GDB_BUF_SIZE) {
-			/* buffer overflow, clear it */
-			kgdb8250_buf_in_inx = 0;
-			atomic_set(&kgdb8250_buf_in_cnt, 0);
-			kgdb8250_buf_out_inx = 0;
-			break;
-		}
-
-		kgdb8250_buf[kgdb8250_buf_in_inx++] = chr;
-		kgdb8250_buf_in_inx &= (GDB_BUF_SIZE - 1);
-		atomic_inc(&kgdb8250_buf_in_cnt);
-	} while (iir & UART_IIR_RDI);
-
-	spin_unlock(&uart_interrupt_lock);
-	local_irq_restore(flags);
-
+	kgdb_schedule_breakpoint();
 	return IRQ_HANDLED;
 }
 
diff -zrupN linux-2.6.3+nothing/include/linux/kgdb.h linux-2.6.3+config+serial/include/linux/kgdb.h
--- linux-2.6.3+nothing/include/linux/kgdb.h	2004-02-27 12:16:14.000000000 -0700
+++ linux-2.6.3+config+serial/include/linux/kgdb.h	2004-02-27 12:16:13.000000000 -0700
@@ -36,10 +36,6 @@ extern volatile int kgdb_connected;
 #endif
 
 extern atomic_t kgdb_setting_breakpoint;
-extern atomic_t kgdb_killed_or_detached;
-extern atomic_t kgdb_might_be_resumed;
-
-extern volatile int kgdb_connected;
 
 extern struct task_struct *kgdb_usethread, *kgdb_contthread;
 
diff -zrupN linux-2.6.3+nothing/kernel/kgdb.c linux-2.6.3+config+serial/kernel/kgdb.c
--- linux-2.6.3+nothing/kernel/kgdb.c	2004-02-27 12:16:14.000000000 -0700
+++ linux-2.6.3+config+serial/kernel/kgdb.c	2004-02-27 12:16:13.000000000 -0700
@@ -15,6 +15,7 @@
  * Copyright (C) 2002-2004 Timesys Corporation
  * Copyright (C) 2003-2004 Amit S. Kale
  * Copyright (C) 2004 Pavel Machek <pavel@suse.cz>
+ * Copyright (C) 2004 Tom Rini <trini@kernel.crashing.org>
  *
  * Restructured KGDB for 2.6 kernels.
  * thread support, support for multiple processors,support for ia-32(x86) 
@@ -87,8 +88,6 @@ static const char hexchars[] = "01234567
 static spinlock_t slavecpulocks[KGDB_MAX_NO_CPUS];
 static volatile int procindebug[KGDB_MAX_NO_CPUS];
 atomic_t kgdb_setting_breakpoint;
-atomic_t kgdb_killed_or_detached;
-atomic_t kgdb_might_be_resumed;
 struct task_struct *kgdb_usethread, *kgdb_contthread;
 
 int debugger_step;
@@ -212,8 +211,10 @@ static void get_packet(char *buffer)
 	char ch;
 
 	do {
-		/* wait around for the start character, ignore all other characters */
-		while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$') ;
+		/* wait around for the start character, ignore all other
+		 * characters */
+		while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$')
+			;	/* Spin. */
 		kgdb_connected = 1;
 		checksum = 0;
 		xmitcsum = -1;
@@ -249,27 +250,22 @@ static void get_packet(char *buffer)
  * Send the packet in buffer.
  * Check for gdb connection if asked for.
  */
-static void put_packet(char *buffer, int checkconnect)
+static void put_packet(char *buffer)
 {
 	unsigned char checksum;
 	int count;
 	char ch;
-	static char gdbseq[] = "$Hc-1#09";
-	int i;
-	int send_count;
 
 	/*  $<packet info>#<checksum>. */
-	do {
+	while (1) {
 		kgdb_serial->write_char('$');
 		checksum = 0;
 		count = 0;
-		send_count = 0;
 
 		while ((ch = buffer[count])) {
 			kgdb_serial->write_char(ch);
 			checksum += ch;
 			count++;
-			send_count++;
 		}
 
 		kgdb_serial->write_char('#');
@@ -278,30 +274,27 @@ static void put_packet(char *buffer, int
 		if (kgdb_serial->flush)
 			kgdb_serial->flush();
 
-		i = 0;
-		while ((ch = kgdb_serial->read_char()) == gdbseq[i++] &&
-		       checkconnect) {
-			if (!gdbseq[i]) {
-				kgdb_serial->write_char('+');
-				if (kgdb_serial->flush)
-					kgdb_serial->flush();
-				breakpoint();
-
-				/*
-				 * GDB is available now.
-				 * Retransmit this packet.
-				 */
-				break;
-			}
-		}
-		if (checkconnect && ch == 3) {
-			kgdb_serial->write_char('+');
+		/* Now see what we get in reply. */
+		ch = kgdb_serial->read_char();
+
+		if (ch == 3)
+			ch = kgdb_serial->read_char();
+
+		/* If we get an ACK, we are done. */
+		if (ch == '+')
+			return;
+
+		/* If we get the start of another packet, this means
+		 * that GDB is attempting to reconnect.  We will NAK
+		 * the packet being sent, and stop trying to send this
+		 * packet. */
+		if (ch == '$') {
+			kgdb_serial->write_char('-');
 			if (kgdb_serial->flush)
 				kgdb_serial->flush();
-			breakpoint();
+			return;
 		}
-	} while ((ch & 0x7f) != '+');
-
+	}
 }
 
 static int get_char(char *addr, unsigned char *data)
@@ -427,11 +420,6 @@ static inline void error_packet(char *pk
 	pkt[3] = '\0';
 }
 
-static void ok_packet(char *pkt)
-{
-	strcpy(pkt, "OK");
-}
-
 static char *pack_threadid(char *pkt, threadref * id)
 {
 	char *limit;
@@ -502,7 +490,8 @@ void kgdb_wait(struct pt_regs *regs)
 	procindebug[processor] = 1;
 	current->thread.debuggerinfo = regs;
 
-	/* Wait till master processor goes completely into the debugger. FIXME: this looks racy */
+	/* Wait till master processor goes completely into the debugger.
+	 * FIXME: this looks racy */
 	while (!procindebug[atomic_read(&debugger_active) - 1]) {
 		int i = 10;	/* an arbitrary number */
 
@@ -701,17 +690,7 @@ int kgdb_handle_exception(int exVector, 
 	/* Master processor is completely in the debugger */
 	kgdb_post_master_code(linux_regs, exVector, err_code);
 
-	if (atomic_read(&kgdb_killed_or_detached) &&
-	    atomic_read(&kgdb_might_be_resumed)) {
-		get_packet(remcom_in_buffer);
-		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 'c') {
-			remove_all_break();
-			atomic_set(&kgdb_killed_or_detached, 0);
-			ok_packet(remcom_out_buffer);
-		} else
-			return 1;
-	} else {
-
+	if (kgdb_connected) {
 		/* reply to host that an exception has occurred */
 		ptr = remcom_out_buffer;
 		*ptr++ = 'T';
@@ -721,11 +700,9 @@ int kgdb_handle_exception(int exVector, 
 		int_to_threadref(&thref, shadow_pid(current->pid));
 		ptr = pack_threadid(ptr, &thref);
 		*ptr++ = ';';
-		*ptr = '\0';
-	}
 
-	put_packet(remcom_out_buffer, 0);
-	kgdb_connected = 1;
+		put_packet(remcom_out_buffer);
+	}
 
 	kgdb_usethread = current;
 	kgdb_usethreadid = shadow_pid(current->pid);
@@ -741,6 +718,11 @@ int kgdb_handle_exception(int exVector, 
 
 		switch (remcom_in_buffer[0]) {
 		case '?':
+			/* We know that this packet is only sent
+			 * during initial connect.  So to be safe,
+			 * we clear out our breakpoints now incase
+			 * GDB is reconnecting. */
+			remove_all_break();
 			remcom_out_buffer[0] = 'S';
 			remcom_out_buffer[1] = hexchars[signo >> 4];
 			remcom_out_buffer[2] = hexchars[signo % 16];
@@ -798,7 +780,7 @@ int kgdb_handle_exception(int exVector, 
 			else {
 				gdb_regs_to_regs(gdb_regs, (struct pt_regs *)
 						 current->thread.debuggerinfo);
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 			}
 
 			break;
@@ -838,10 +820,10 @@ int kgdb_handle_exception(int exVector, 
 			if ((error = remove_all_break()) < 0) {
 				error_packet(remcom_out_buffer, error);
 			} else {
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 				kgdb_connected = 0;
 			}
-			put_packet(remcom_out_buffer, 0);
+			put_packet(remcom_out_buffer);
 			goto default_handle;
 
 		case 'k':
@@ -947,11 +929,10 @@ int kgdb_handle_exception(int exVector, 
 				}
 				kgdb_usethread = thread;
 				kgdb_usethreadid = threadid;
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 				break;
 
 			case 'c':
-				atomic_set(&kgdb_killed_or_detached, 0);
 				ptr = &remcom_in_buffer[2];
 				kgdb_hex2long(&ptr, &threadid);
 				if (!threadid) {
@@ -966,7 +947,7 @@ int kgdb_handle_exception(int exVector, 
 					}
 					kgdb_contthread = thread;
 				}
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 				break;
 			}
 			break;
@@ -977,7 +958,7 @@ int kgdb_handle_exception(int exVector, 
 			kgdb_hex2long(&ptr, &threadid);
 			thread = getthread(linux_regs, threadid);
 			if (thread)
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 			else
 				error_packet(remcom_out_buffer, -EINVAL);
 			break;
@@ -1018,7 +999,7 @@ int kgdb_handle_exception(int exVector, 
 			}
 
 			if (error == 0)
-				ok_packet(remcom_out_buffer);
+				strcpy(remcom_out_buffer, "OK");
 			else
 				error_packet(remcom_out_buffer, error);
 
@@ -1039,7 +1020,7 @@ int kgdb_handle_exception(int exVector, 
 		}		/* switch */
 
 		/* reply to the request */
-		put_packet(remcom_out_buffer, 0);
+		put_packet(remcom_out_buffer);
 	}
 
       kgdb_exit:
@@ -1063,7 +1044,6 @@ int kgdb_handle_exception(int exVector, 
 	}
 
 	/* Free debugger_active */
-	atomic_set(&kgdb_killed_or_detached, 1);
 	atomic_set(&debugger_active, 0);
 	local_irq_restore(flags);
 
@@ -1132,12 +1112,6 @@ void kgdb_entry(void)
 	/* Free debugger_active */
 	atomic_set(&debugger_active, 0);
 
-	/* This flag is used, if gdb has detached and wants to start
-	 * another session
-	 */
-	atomic_set(&kgdb_killed_or_detached, 1);
-	atomic_set(&kgdb_might_be_resumed, 0);
-
 	for (i = 0; i < MAX_BREAKPOINTS; i++)
 		kgdb_break[i].state = bp_disabled;
 
@@ -1222,7 +1196,7 @@ void kgdb_console_write(struct console *
 		*bufptr = '\0';
 		s += wcount;
 
-		put_packet(kgdbconbuf, 1);
+		put_packet(kgdbconbuf);
 
 	}
 	local_irq_restore(flags);

-- 
Tom Rini
http://gate.crashing.org/~trini/