[PATCH][1/3] Update CVS KGDB's serial driver

[PATCH][1/3] Update CVS KGDB's serial driver

[Tom Rini]

The following updates the serial driver with the fixes / cleanups that
are in George's version of the driver.  There's a few slightly 'odd'
things in this patch, which stem from the fact that in my next round of
patches there will (a) be kernel/Kconfig.kgdb and (b) 1 kgdb i/o driver
at a time.

# 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.1662  -> 1.1663 
#	drivers/serial/kgdb_8250.c	1.2     -> 1.3    
#	       kernel/kgdb.c	1.2     -> 1.3    
#	drivers/serial/Kconfig	1.21    -> 1.22   
#	include/linux/kgdb.h	1.2     -> 1.3    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25	trini@kernel.crashing.org	1.1663
# Serial update.
# --------------------------------------------
#
diff -Nru a/drivers/serial/Kconfig b/drivers/serial/Kconfig
--- a/drivers/serial/Kconfig	Wed Feb 25 14:21:26 2004
+++ b/drivers/serial/Kconfig	Wed Feb 25 14:21:26 2004
@@ -13,6 +13,27 @@
 	  Uses generic serial port (8250) for kgdb. This is independent of the
 	  option 9250/16550 and compatible serial port.
 
+config KGDB_PORT
+	hex "hex I/O port address of the debug serial port"
+	depends on KGDB_8250
+	default  3f8
+	help
+	  Some systems (x86 family at this writing) allow the port
+	  address to be configured.  The number entered is assumed to be
+	  hex, don't put 0x in front of it.  The standard address are:
+	  COM1 3f8 , irq 4 and COM2 2f8 irq 3.  Setserial /dev/ttySx
+	  will tell you what you have.  It is good to test the serial
+	  connection with a live system before trying to debug.
+
+config KGDB_IRQ
+	int "IRQ of the debug serial port"
+	depends on KGDB_8250
+	default 4
+	help
+	  This is the irq for the debug port.  If everything is working
+	  correctly and the kernel has interrupts on a control C to the
+	  port should cause a break into the kernel debug stub.
+
 #
 # The new 8250/16550 serial drivers
 config SERIAL_8250
diff -Nru a/drivers/serial/kgdb_8250.c b/drivers/serial/kgdb_8250.c
--- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:26 2004
+++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:26 2004
@@ -1,137 +1,218 @@
 /*
  * 8250 interface for kgdb.
  *
- * Restructured for making kgdb capable of handling different types of serial
- * interfaces, by Amit Kale (amitkale@emsyssoft.com)
- *
- * Written (hacked together) by David Grothe (dave@gcom.com)
- *
- * Modified by Scott Foehner (sfoehner@engr.sgi.com) to allow connect
- * on boot-up
+ * This is a merging of many different drivers, and all of the people have
+ * had an impact in some form or another:
  *
+ * Amit Kale <amitkale@emsyssoft.com>
+ * David Grothe <dave@gcom.com>
+ * Scott Foehner <sfoehner@engr.sgi.com>
+ * George Anzinger <george@mvista.com>
+ * Robert Walsh <rjwalsh@durables.org>
+ * wangdi <wangdi@clusterfs.com>
+ * San Mehat
+ * Tom Rini <trini@mvista.com>
  */
 
-#include <linux/module.h>
-#include <linux/errno.h>
-#include <linux/signal.h>
-#include <linux/sched.h>
-#include <linux/timer.h>
+#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>
-#include <linux/tty_flip.h>
 #include <linux/serial.h>
 #include <linux/serial_core.h>
 #include <linux/serial_reg.h>
 #include <linux/serialP.h>
-#include <linux/config.h>
-#include <linux/major.h>
-#include <linux/string.h>
-#include <linux/fcntl.h>
-#include <linux/termios.h>
-#include <linux/kgdb.h>
 
-#include <asm/system.h>
 #include <asm/io.h>
-#include <asm/segment.h>
-#include <asm/bitops.h>
-#include <asm/system.h>
-#include <asm/irq.h>
-#include <asm/atomic.h>
+#include <asm/serial.h>		/* For BASE_BAUD and SERIAL_PORT_DFNS */
 
 #define GDB_BUF_SIZE	512	/* power of 2, please */
 
+#if defined(CONFIG_KGDB_9600BAUD)
+#define SERIAL_BAUD 9600
+#elif defined(CONFIG_KGDB_19200BAUD)
+#define SERIAL_BAUD 19200
+#elif defined(CONFIG_KGDB_38400BAUD)
+#define SERIAL_BAUD 38400
+#elif defined(CONFIG_KGDB_57600BAUD)
+#define SERIAL_BAUD 57600
+#elif defined(CONFIG_KGDB_115200BAUD)
+#define SERIAL_BAUD 115200
+#else
+#define SERIAL_BAUD 115200	/* Start with this if not given */
+#endif
+
+#if defined(CONFIG_KGDB_TTYS0)
+#define KGDB_PORT 0
+#elif defined(CONFIG_KGDB_TTYS1)
+#define KGDB_PORT 1
+#elif defined(CONFIG_KGDB_TTYS2)
+#define KGDB_PORT 2
+#elif defined(CONFIG_KGDB_TTYS3)
+#define KGDB_PORT 3
+#else
+#define KGDB_PORT 0		/* Start with this if not given */
+#endif
+
+int kgdb8250_baud = SERIAL_BAUD;
+int kgdb8250_ttyS = KGDB_PORT;
+
 static char kgdb8250_buf[GDB_BUF_SIZE];
 static int kgdb8250_buf_in_inx;
 static atomic_t kgdb8250_buf_in_cnt;
 static int kgdb8250_buf_out_inx;
 
-static int kgdb8250_got_dollar = -3, kgdb8250_got_H = -3,
-    kgdb8250_interrupt_iteration = 0;
+/* Determine serial information. */
+static struct serial_state state = {
+	.magic = SSTATE_MAGIC,
+	.baud_base = BASE_BAUD,
+	.custom_divisor = BASE_BAUD / SERIAL_BAUD,
+};
 
-/* We only allow for 4 ports to be registered.  We default to standard
- * PC values. */
-static struct uart_port rs_table[4] = {
-	{.line = 0x3f8,.irq = 4},
-	{.line = 0x2f8,.irq = 3},
-	{.line = 0x3e8,.irq = 4},
-	{.line = 0x2e8,.irq = 3},
+static struct async_struct gdb_async_info = {
+	.magic = SERIAL_MAGIC,
+	.state = &state,
+	.tty = (struct tty_struct *) &state,
 };
-static void (*serial_outb) (unsigned char, unsigned long);
-static unsigned long (*serial_inb) (unsigned long);
+
+/* Space between registers. */
+static int reg_shift;
+
+/* Not all arches define this. */
+#ifndef SERIAL_PORT_DFNS
+#define SERIAL_PORT_DFNS
+#endif
+static struct serial_state rs_table[] = {
+	SERIAL_PORT_DFNS	/* defined in <asm/serial.h> */
+};
+
+/* Do we need to look in the rs_table? */
+static int serial_from_rs_table = 0;
+
+static void (*serial_outb) (unsigned char val, unsigned long addr);
+static unsigned long (*serial_inb) (unsigned long addr);
 
 int serial8250_release_irq(int irq);
 
-int kgdb8250_irq;
-unsigned long kgdb8250_port;
+static int kgdb8250_init(void);
+static unsigned long kgdb8250_port;
 
-int kgdb8250_baud = 115200;
-int kgdb8250_ttyS;
+static int initialized = -1;
 
-static unsigned long direct_inb(unsigned long addr)
+static unsigned long
+direct_inb(unsigned long addr)
 {
 	return readb(addr);
 }
 
-static void direct_outb(unsigned char val, unsigned long addr)
+static void
+direct_outb(unsigned char val, unsigned long addr)
 {
 	writeb(val, addr);
 }
 
-static unsigned long io_inb(unsigned long port)
+static unsigned long
+io_inb(unsigned long port)
 {
 	return inb(port);
 }
 
-static void io_outb(unsigned char val, unsigned long port)
+static void
+io_outb(unsigned char val, unsigned long port)
 {
 	outb(val, port);
 }
 
 /*
- * Get a byte from the hardware data buffer and return it
- * Get a char if available, return -EAGAIN if nothing available.
+ * Wait until the interface can accept a char, then write it.
  */
-static int read_data_bfr(void)
+void
+kgdb_put_debug_char(int chr)
 {
-	if (serial_inb(kgdb8250_port + UART_LSR) & UART_LSR_DR)
-		return (serial_inb(kgdb8250_port + UART_RX));
+	while (!(serial_inb(kgdb8250_port + (UART_LSR << reg_shift)) &
+		UART_LSR_THRE))
+		;
 
-	return -EAGAIN;
+	serial_outb(chr, kgdb8250_port + (UART_TX << reg_shift));
 }
 
 /*
- * Empty the receive buffer first, then look at the interface hardware.
- * It waits for a character from the serial interface and then returns it.
+ * Get a byte from the hardware data buffer and return it
  */
-static int kgdb8250_read_char(void)
+static int
+read_data_bfr(void)
 {
-	int retchar;
-	if (atomic_read(&kgdb8250_buf_in_cnt) != 0) {
-		/* intr routine has q'd chars read them from buffer */
-		int chr;
+	char it = serial_inb(kgdb8250_port + (UART_LSR << reg_shift));
 
-		chr = kgdb8250_buf[kgdb8250_buf_out_inx++];
-		kgdb8250_buf_out_inx &= (GDB_BUF_SIZE - 1);
-		atomic_dec(&kgdb8250_buf_in_cnt);
-		return chr;
+	if (it & UART_LSR_DR)
+		return serial_inb(kgdb8250_port + (UART_RX << reg_shift));
+
+	/*
+	 * If we have a framing error assume somebody messed with
+	 * our uart.  Reprogram it and send '-' both ways...
+	 */
+	if (it & 0xc) {
+		kgdb8250_init();
+		kgdb_put_debug_char('-');
+		return '-';
 	}
-	do {
-		/* read from hardware */
-		retchar = read_data_bfr();
-	} while (retchar < 0);
-	return retchar;
+
+	return -1;
 }
 
 /*
- * Wait until the interface can accept a char, then write it.
+ * 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 void kgdb8250_write_char(int chr)
+
+static spinlock_t uart_interrupt_lock = SPIN_LOCK_UNLOCKED;
+#ifdef CONFIG_SMP
+extern spinlock_t kgdb_spinlock;
+#endif
+
+int
+kgdb_get_debug_char(void)
 {
-	while (!(serial_inb(kgdb8250_port + UART_LSR) & UART_LSR_THRE))
-		/* Do nothing */ ;
+	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++];
+		kgdb8250_buf_out_inx &= (GDB_BUF_SIZE - 1);
+		atomic_dec(&kgdb8250_buf_in_cnt);
+		goto out;
+	}
+
+	do {
+		retchr = read_data_bfr();
+	} while (retchr < 0);
 
-	serial_outb(chr, kgdb8250_port + UART_TX);
+out:
+#ifdef CONFIG_SMP
+	if (!spin_is_locked(&kgdb_spinlock)) {
+		spin_unlock(&uart_interrupt_lock);
+	}
+#endif
+	local_irq_restore(flags);
 
+	return retchr;
 }
 
 /*
@@ -139,12 +220,12 @@
  * It will receive a limited number of characters of input
  * from the gdb  host machine and save them up in a buffer.
  *
- * When kgdb8250_read_char() is called it
+ * 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 kgdb8250_write_char() which
+ * since the stubs do all of that via kgdb_put_debug_char() which
  * writes one byte after waiting for the interface to become
  * ready.
  *
@@ -156,15 +237,27 @@
  * care to learn can make this work for any low level serial
  * driver.
  */
-static irqreturn_t kgdb8250_interrupt(int irq, void *dev_id,
-				      struct pt_regs *regs)
+static irqreturn_t
+kgdb8250_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 {
-	int chr;
-	int iir;
+	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) {
+		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);
+		iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
 		if (chr < 0)
 			continue;
 
@@ -174,28 +267,6 @@
 			continue;
 		}
 
-		if (atomic_read(&kgdb_killed_or_detached)) {
-			if (chr == '$')
-				kgdb8250_got_dollar =
-				    kgdb8250_interrupt_iteration;
-			if (kgdb8250_interrupt_iteration ==
-			    kgdb8250_got_dollar + 1 && chr == 'H')
-				kgdb8250_got_H = kgdb8250_interrupt_iteration;
-			else if (kgdb8250_interrupt_iteration ==
-				 kgdb8250_got_H + 1 && chr == 'c') {
-				kgdb8250_buf[kgdb8250_buf_in_inx++] = chr;
-				atomic_inc(&kgdb8250_buf_in_cnt);
-				atomic_set(&kgdb_might_be_resumed, 1);
-				wmb();
-				breakpoint();
-				atomic_set(&kgdb_might_be_resumed, 0);
-				kgdb8250_interrupt_iteration = 0;
-				kgdb8250_got_dollar = -3;
-				kgdb8250_got_H = -3;
-				continue;
-			}
-		}
-
 		if (atomic_read(&kgdb8250_buf_in_cnt) >= GDB_BUF_SIZE) {
 			/* buffer overflow, clear it */
 			kgdb8250_buf_in_inx = 0;
@@ -207,33 +278,30 @@
 		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);
+	} while (iir & UART_IIR_RDI);
 
-	if (atomic_read(&kgdb_killed_or_detached))
-		kgdb8250_interrupt_iteration++;
+	spin_unlock(&uart_interrupt_lock);
+	local_irq_restore(flags);
 
 	return IRQ_HANDLED;
-
 }
 
 /* 
- * Initializes serial port.
- *	ttyS - integer specifying which serial port to use for debugging
- *	baud - baud rate of specified serial port
+ *  Returns:
+ *	0 on success, 1 on failure.
  */
-static int kgdb8250_init(int ttyS, int baud)
+static int
+kgdb8250_init(void)
 {
 	unsigned cval;
 	int bits = 8;
 	int parity = 'n';
 	int cflag = CREAD | HUPCL | CLOCAL;
-	int quot = 0;
 
 	/*
 	 *      Now construct a cflag setting.
 	 */
-	switch (baud) {
+	switch (kgdb8250_baud) {
 	case 1200:
 		cflag |= B1200;
 		break;
@@ -256,7 +324,7 @@
 		cflag |= B115200;
 		break;
 	default:
-		baud = 9600;
+		kgdb8250_baud = 9600;
 		/* Fall through */
 	case 9600:
 		cflag |= B9600;
@@ -287,7 +355,6 @@
 	 *
 	 */
 
-	quot = (1843200 / 16) / baud;
 	cval = cflag & (CSIZE | CSTOPB);
 	cval >>= 4;
 	if (cflag & PARENB)
@@ -300,75 +367,118 @@
 	 *      and set speed.
 	 */
 	cval = 0x3;
-	serial_outb(cval | UART_LCR_DLAB, kgdb8250_port + UART_LCR);	/* set DLAB */
-	serial_outb(quot & 0xff, kgdb8250_port + UART_DLL);	/* LS of divisor */
-	serial_outb(quot >> 8, kgdb8250_port + UART_DLM);	/* MS of divisor */
-	serial_outb(cval, kgdb8250_port + UART_LCR);	/* reset DLAB */
-	serial_outb(UART_IER_RDI, kgdb8250_port + UART_IER);	/* turn on interrupts */
+	/* set DLAB */
+	serial_outb(cval | UART_LCR_DLAB, kgdb8250_port +
+			(UART_LCR << reg_shift));
+	/* LS */
+	serial_outb(gdb_async_info.state->custom_divisor & 0xff,
+			kgdb8250_port + (UART_DLL << reg_shift));
+	/* MS  */
+	serial_outb(gdb_async_info.state->custom_divisor >> 8,
+			kgdb8250_port + (UART_DLM << reg_shift));
+	/* reset DLAB */
+	serial_outb(cval, kgdb8250_port + (UART_LCR << reg_shift));
+	/* turn on interrupts */
+	serial_outb(UART_IER_RDI, kgdb8250_port + (UART_IER << reg_shift));
 	serial_outb(UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS,
-		    kgdb8250_port + UART_MCR);
+		    kgdb8250_port + (UART_MCR << reg_shift));
 
 	/*
 	 *      If we read 0xff from the LSR, there is no UART here.
 	 */
-	if (serial_inb(kgdb8250_port + UART_LSR) == 0xff)
-		return -ENODEV;
+	if (serial_inb(kgdb8250_port + (UART_LSR << reg_shift)) == 0xff)
+		return -1;
 	return 0;
 }
 
-int kgdb8250_hook(void)
+int
+kgdb_hook_io(void)
 {
 	int retval;
 
-	/*
-	 * Set port and irq number.
-	 */
-	kgdb8250_irq = rs_table[kgdb8250_ttyS].irq;
-	switch (rs_table[kgdb8250_ttyS].iotype) {
+	/* Setup any serial port information we may need to */
+#ifdef CONFIG_KGDB_SIMPLE_SERIAL
+	/* We must look in the rs_table[]. */
+	serial_from_rs_table = 1;
+#endif
+	/* If the user has overriden our definitions, or if we've only
+	 * been told the ttyS to use, look at rs_table. */
+	if (serial_from_rs_table) {
+		/* The user has selected one of ttyS[0-3], which we pull
+		 * from rs_table[].  If this doesn't exist, user error. */
+		gdb_async_info.port = gdb_async_info.state->port =
+		    rs_table[KGDB_PORT].port;
+		gdb_async_info.line = gdb_async_info.state->irq =
+		    rs_table[KGDB_PORT].irq;
+		gdb_async_info.state->io_type = rs_table[KGDB_PORT].io_type;
+		reg_shift = rs_table[KGDB_PORT].iomem_reg_shift;
+	}
+
+	switch (gdb_async_info.state->io_type) {
 	case SERIAL_IO_MEM:
-		kgdb8250_port = (unsigned long)rs_table[kgdb8250_ttyS].membase;
-		serial_inb = direct_inb;
+		kgdb8250_port = (unsigned long)
+		    rs_table[kgdb8250_ttyS].iomem_base;
 		serial_outb = direct_outb;
+		serial_inb = direct_inb;
 		break;
+	case SERIAL_IO_PORT:
 	default:
-		kgdb8250_port = rs_table[kgdb8250_ttyS].line;
-		serial_inb = io_inb;
+		kgdb8250_port = rs_table[kgdb8250_ttyS].port;
 		serial_outb = io_outb;
+		serial_inb = io_inb;
 	}
 
+#ifndef CONFIG_KGDB_SIMPLE_SERIAL
+	/* The user has provided the IRQ and I/O location. */
+	kgdb8250_port = gdb_async_info.port = gdb_async_info.state->port =
+		CONFIG_KGDB_PORT;
+	gdb_async_info.line = gdb_async_info.state->irq = CONFIG_KGDB_IRQ;
+#endif
+
 #ifdef CONFIG_SERIAL_8250
-	if ((retval = serial8250_release_irq(kgdb8250_irq)) < 0)
-		return retval;
+	if (serial8250_release_irq(gdb_async_info.line))
+		return -1;
 #endif
 
-	if ((retval = kgdb8250_init(kgdb8250_ttyS, kgdb8250_baud)) < 0)
-		return retval;
+	if (kgdb8250_init() == -1)
+		return -1;
 
-	retval = request_irq(kgdb8250_irq, kgdb8250_interrupt, SA_INTERRUPT,
-			     "GDB-stub", NULL);
-	return retval;
-}
+	retval = request_irq(gdb_async_info.line, kgdb8250_interrupt,
+			     SA_INTERRUPT, "GDB-stub", NULL);
+	if (retval == 0)
+		initialized = 1;
+	else
+		initialized = 0;
 
-void kgdb8250_add_port(int i, struct uart_port *serial_req)
-{
-	rs_table[i].iotype = serial_req->iotype;
-	rs_table[i].line = serial_req->line;
-	rs_table[i].membase = serial_req->membase;
-	rs_table[i].regshift = serial_req->regshift;
+	return 0;
 }
 
 struct kgdb_serial kgdb8250_serial_driver = {
-	.read_char = kgdb8250_read_char,
-	.write_char = kgdb8250_write_char,
-	.hook = kgdb8250_hook
+	.read_char = kgdb_get_debug_char,
+	.write_char = kgdb_put_debug_char,
+	.hook = kgdb_hook_io,
 };
 
+void
+kgdb8250_add_port(int i, struct uart_port *serial_req)
+{
+	rs_table[i].io_type = serial_req->iotype;
+	rs_table[i].port = serial_req->line;
+	rs_table[i].irq = serial_req->irq;
+	rs_table[i].iomem_base = serial_req->membase;
+	rs_table[i].iomem_reg_shift = serial_req->regshift;
+
+	/* We will want to look in the rs_table now. */
+	serial_from_rs_table = 1;
+}
+
 /*
  * Syntax for this cmdline option is
  * kgdb8250=ttyno,baudrate
  */
 
-static int __init kgdb8250_opt(char *str)
+static int __init
+kgdb8250_opt(char *str)
 {
 	if (*str < '0' || *str > '3')
 		goto errout;
@@ -382,7 +492,12 @@
 	    kgdb8250_baud != 38400 && kgdb8250_baud != 57600 &&
 	    kgdb8250_baud != 115200)
 		goto errout;
+
+	/* Make the baud rate change happen. */
+	gdb_async_info.state->custom_divisor = BASE_BAUD / kgdb8250_baud;
+
 	kgdb_serial = &kgdb8250_serial_driver;
+
 	return 1;
 
       errout:
diff -Nru a/include/linux/kgdb.h b/include/linux/kgdb.h
--- a/include/linux/kgdb.h	Wed Feb 25 14:21:26 2004
+++ b/include/linux/kgdb.h	Wed Feb 25 14:21:26 2004
@@ -25,6 +25,8 @@
 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;
 
 enum kgdb_bptype {
diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c
--- a/kernel/kgdb.c	Wed Feb 25 14:21:26 2004
+++ b/kernel/kgdb.c	Wed Feb 25 14:21:26 2004
@@ -63,7 +63,7 @@
  * has connected to kgdb.
  */
 int kgdb_initialized = 0;
-static volatile int kgdb_connected;
+volatile int kgdb_connected;
 
 /* If non-zero, wait for a gdb connection when kgdb_entry is called */
 int kgdb_enter = 0;
@@ -214,6 +214,7 @@
 	do {
 		/* wait around for the start character, ignore all other characters */
 		while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$') ;
+		kgdb_connected = 1;
 		checksum = 0;
 		xmitcsum = -1;
 
-- 
Tom Rini
http://gate.crashing.org/~trini/

[PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

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

# 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	trini@kernel.crashing.org	1.1664
# process_breakpoint/schedule_breakpoint.
# --------------------------------------------
#
diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c
--- a/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
+++ b/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
@@ -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
--- a/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
+++ b/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
@@ -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
--- a/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
+++ b/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
@@ -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
--- a/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
+++ b/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
@@ -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
--- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
+++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
@@ -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
--- a/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
+++ b/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
@@ -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
--- a/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
+++ b/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
@@ -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];
 
-- 
Tom Rini
http://gate.crashing.org/~trini/

[PATCH][3/3] Update CVS KGDB's wrt connect / detach

[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.
- 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	trini@kernel.crashing.org	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
--- a/arch/ppc/8260_io/uart.c	Wed Feb 25 14:21:38 2004
+++ b/arch/ppc/8260_io/uart.c	Wed Feb 25 14:21:38 2004
@@ -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
--- a/kernel/kgdb.c	Wed Feb 25 14:21:38 2004
+++ b/kernel/kgdb.c	Wed Feb 25 14:21:38 2004
@@ -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 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 */
 
@@ -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);
-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Amit S. Kale]

On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> 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:

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

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

> - Remove ok_packet(), excessive, IMHO.

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

Other lindent changes are good.

-Amit

> # 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	trini@kernel.crashing.org	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
> --- a/arch/ppc/8260_io/uart.c	Wed Feb 25 14:21:38 2004
> +++ b/arch/ppc/8260_io/uart.c	Wed Feb 25 14:21:38 2004
> @@ -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
> --- a/kernel/kgdb.c	Wed Feb 25 14:21:38 2004
> +++ b/kernel/kgdb.c	Wed Feb 25 14:21:38 2004
> @@ -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 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 */
>
> @@ -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);

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Tom Rini]

On Thu, Feb 26, 2004 at 01:44:49PM +0530, Amit S. Kale wrote:

> On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> > 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:
> 
> -		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.

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?

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

As in doesn't cleanly remove itself?

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

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

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Daniel Jacobowitz]

On Thu, Feb 26, 2004 at 07:41:55AM -0700, Tom Rini wrote:
> On Thu, Feb 26, 2004 at 01:44:49PM +0530, Amit S. Kale wrote:
> 
> > On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> > > 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:
> > 
> > -		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.
> 
> 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.

> 
> > > - 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
> 
> As in doesn't cleanly remove itself?

Seg faults, for instance.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Tom Rini]

On Thu, Feb 26, 2004 at 11:05:23AM -0500, Daniel Jacobowitz wrote:

> On Thu, Feb 26, 2004 at 07:41:55AM -0700, Tom Rini wrote:
> > On Thu, Feb 26, 2004 at 01:44:49PM +0530, Amit S. Kale wrote:
> > 
> > > On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> > > > 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:
> > > 
> > > -		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.
> > 
> > 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.

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

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

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

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Daniel Jacobowitz]

On Thu, Feb 26, 2004 at 10:44:21AM -0700, Tom Rini wrote:
> Ack.  Does gdb do anything to see if the rmote side will support being
> left hanging?

No.

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

Generally yes, if the program was stopped, as an accident of the fact
that GDB removes breakpoints when you're stopped.

When running, no, not so much.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Amit S. Kale]

On Thursday 26 Feb 2004 9:35 pm, Daniel Jacobowitz wrote:
> On Thu, Feb 26, 2004 at 07:41:55AM -0700, Tom Rini wrote:
> > On Thu, Feb 26, 2004 at 01:44:49PM +0530, Amit S. Kale wrote:
> > > On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> > > > 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:
> > >
> > > -		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.
> >
> > 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.
>
> > > > - 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
> >
> > As in doesn't cleanly remove itself?
>
> Seg faults, for instance.

That's something easily recognizable. There are worse situations where gdb has 
to be killed.

GDB sometimes goes into an infinite loop of stub communications, doesn't 
respond to Ctrl+C and has to be killed. It's rare, though.

-Amit

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Pavel Machek]

Hi!

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

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


-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Amit S. Kale]

On Friday 27 Feb 2004 3:15 am, Pavel Machek wrote:
> Hi!
>
> > > 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
> > >
> > > > - 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.
>
> 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. I agree that strcpy is better.

-Amit

>
> 	ok_packet(foo);
> 	strcpy(foo,"OK");
>
> ...we are talking 2 or 3 characters here....
> 								Pavel

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Tom Rini]

On Thu, Feb 26, 2004 at 01:44:49PM +0530, Amit S. Kale wrote:

> On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> > 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:

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.

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

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.
     
-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Amit S. Kale]

On Thursday 26 Feb 2004 11:38 pm, Tom Rini wrote:
> On Thu, Feb 26, 2004 at 01:44:49PM +0530, Amit S. Kale wrote:
> > On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> > > 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:
>
> 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.
>

Sounds good.

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

We want a breakpoint (or call to kgdb_handle_exception) instead, if putpacket 
was called from kgdb console printing.

-Amit

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[George Anzinger]

Amit S. Kale wrote:
> On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> 
>>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:
> 
> -		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.

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

-g

> 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
> 
> 
>>- Remove ok_packet(), excessive, IMHO.
> 
> 
> ok_packet is better than littering "OK" all over the place.
> 
> Other lindent changes are good.
> 
> -Amit
> 
> 
>># 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	trini@kernel.crashing.org	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
>>--- a/arch/ppc/8260_io/uart.c	Wed Feb 25 14:21:38 2004
>>+++ b/arch/ppc/8260_io/uart.c	Wed Feb 25 14:21:38 2004
>>@@ -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
>>--- a/kernel/kgdb.c	Wed Feb 25 14:21:38 2004
>>+++ b/kernel/kgdb.c	Wed Feb 25 14:21:38 2004
>>@@ -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 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 */
>>
>>@@ -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);
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Tom Rini]

On Thu, Feb 26, 2004 at 03:30:08PM -0800, George Anzinger wrote:
> Amit S. Kale wrote:
> >On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> >
> >>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:
> >
> >-		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.
> 
> 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).

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

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.  For the last one, there's
2 packet instead of ACKs, then a NAK, but I believe this is acceptable.

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

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[George Anzinger]

Tom Rini wrote:
> On Thu, Feb 26, 2004 at 03:30:08PM -0800, George Anzinger wrote:
> 
>>Amit S. Kale wrote:
>>
>>>On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
>>>
>>>
>>>>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:
>>>
>>>-		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.
>>
>>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).
> 
> 
>>>>- 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.
> 
> 
> 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....


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

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.

To overcome, to some extent, this limitation I invented the struct kgdb_info 
which contains bits of information on kgdb's internal state, some of which can 
be modified to control kgdb.  It also contains information on the "other" cpus 
and where we found them.  Also flags on what to do with them when single 
stepping and continuing.

For the last one, there's
> 2 packet instead of ACKs, then a NAK, but I believe this is acceptable.
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Tom Rini]

On Thu, Feb 26, 2004 at 05:57:27PM -0800, George Anzinger wrote:

> Tom Rini wrote:
> >On Thu, Feb 26, 2004 at 03:30:08PM -0800, George Anzinger wrote:
> >
> >>Amit S. Kale wrote:
> >>
> >>>On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> >>>
> >>>
> >>>>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:
> >>>
> >>>-		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.
> >>
> >>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).
> >
> >
> >>>>- 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.
> >
> >
> >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....

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.

> >- 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.  
> 
> What are you referring to as "continues".  How is this different from 
> connect to a waiting kernel?

The 'continue' command in gdb.

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

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

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[George Anzinger]

Tom Rini wrote:
> On Thu, Feb 26, 2004 at 05:57:27PM -0800, George Anzinger wrote:
> 
> 
>>Tom Rini wrote:
>>
>>>On Thu, Feb 26, 2004 at 03:30:08PM -0800, George Anzinger wrote:
>>>
>>>
>>>>Amit S. Kale wrote:
>>>>
>>>>
>>>>>On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
>>>>>
>>>>>
>>>>>
>>>>>>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:
>>>>>
>>>>>-		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.
>>>>
>>>>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).
>>>
>>>
>>>
>>>>>>- 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.
>>>
>>>
>>>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....
> 
> 
> 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
> 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.
> 
> 
>>>- 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.  
>>
>>What are you referring to as "continues".  How is this different from 
>>connect to a waiting kernel?
> 
> 
> The 'continue' command in gdb.
> 
> 
>>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.
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Tom Rini]

On Fri, Feb 27, 2004 at 02:11:54PM -0800, George Anzinger wrote:

> Tom Rini wrote:
> >On Thu, Feb 26, 2004 at 05:57:27PM -0800, George Anzinger wrote:
> >
> >
> >>Tom Rini wrote:
> >>
> >>>On Thu, Feb 26, 2004 at 03:30:08PM -0800, George Anzinger wrote:
> >>>
> >>>
> >>>>Amit S. Kale wrote:
> >>>>
> >>>>
> >>>>>On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>>>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:
> >>>>>
> >>>>>-		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.
> >>>>
> >>>>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).
> >>>
> >>>
> >>>
> >>>>>>- 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.
> >>>
> >>>
> >>>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....
> >
> >
> >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.

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.

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

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Amit S. Kale]

On Saturday 28 Feb 2004 4:20 am, Tom Rini wrote:
> On Fri, Feb 27, 2004 at 02:11:54PM -0800, George Anzinger wrote:
> > Tom Rini wrote:
> > >On Thu, Feb 26, 2004 at 05:57:27PM -0800, George Anzinger wrote:
> > >>Tom Rini wrote:
> > >>>On Thu, Feb 26, 2004 at 03:30:08PM -0800, George Anzinger wrote:
> > >>>>Amit S. Kale wrote:
> > >>>>>On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> > >>>>>>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:
> > >>>>>
> > >>>>>-		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.
> > >>>>
> > >>>>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).
> > >>>
> > >>>>>>- 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.
> > >>>
> > >>>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....
> > >
> > >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.
>
> 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.

You may not have seen the race. I too believe that the race pointed out by 
George exists.

-Amit

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Amit S. Kale]

On Saturday 28 Feb 2004 3:41 am, George Anzinger wrote:
> Tom Rini wrote:
> > On Thu, Feb 26, 2004 at 05:57:27PM -0800, George Anzinger wrote:
> >>Tom Rini wrote:
> >>>On Thu, Feb 26, 2004 at 03:30:08PM -0800, George Anzinger wrote:
> >>>>Amit S. Kale wrote:
> >>>>>On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> >>>>>>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:
> >>>>>
> >>>>>-		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.
> >>>>
> >>>>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).
> >>>
> >>>>>>- 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.
> >>>
> >>>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....
> >
> > 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.

Yes.
We need some locking on smp for this purpose.

-Amit

>
> -g
>
> > 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.
> >
> >>>- 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.
> >>
> >>What are you referring to as "continues".  How is this different from
> >>connect to a waiting kernel?
> >
> > The 'continue' command in gdb.
> >
> >>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.

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Tom Rini]

On Thu, Feb 26, 2004 at 05:57:27PM -0800, George Anzinger wrote:
> Tom Rini wrote:
> >- 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.  
> 
> 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.

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

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[George Anzinger]

Tom Rini wrote:
> On Thu, Feb 26, 2004 at 05:57:27PM -0800, George Anzinger wrote:
> 
>>Tom Rini wrote:
>>
>>>- 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.  
>>
>>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() ->
I don't know this code, but if the current PC is at a given BP+1 you may need to 
back up the PC to point to the, now replaced, instruction.  You could verify 
this by looking, at this point, at what gdb has for the PC.  Odds are that it is 
either mid instruction or after the instruction that was replaced by the BP 
(depends on the relative length of the BP instruction and the replaced 
instruction).

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

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Amit S. Kale]

On Friday 27 Feb 2004 5:00 am, George Anzinger wrote:
> Amit S. Kale wrote:
> > On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> >>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:
> >
> > -		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.
>
> 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?

kgdb tracks breakpoints in the cvs version. 

gdb many a times has a shorter life compared to kgdb (unfortunate but true). 
If gdb dies when breakpoints are present in a kernel, the kernel too has to 
be killed as there is no way to correct the kernel code modified by gdb.

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

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

-Amit

>
> -g
>
> > 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
> >
> >>- Remove ok_packet(), excessive, IMHO.
> >
> > ok_packet is better than littering "OK" all over the place.
> >
> > Other lindent changes are good.
> >
> > -Amit
> >
> >># 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	trini@kernel.crashing.org	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
> >>--- a/arch/ppc/8260_io/uart.c	Wed Feb 25 14:21:38 2004
> >>+++ b/arch/ppc/8260_io/uart.c	Wed Feb 25 14:21:38 2004
> >>@@ -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
> >>--- a/kernel/kgdb.c	Wed Feb 25 14:21:38 2004
> >>+++ b/kernel/kgdb.c	Wed Feb 25 14:21:38 2004
> >>@@ -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 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 */
> >>
> >>@@ -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);
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> > in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach

[Tom Rini]

On Mon, Mar 01, 2004 at 02:06:17PM +0530, Amit S. Kale wrote:
> On Friday 27 Feb 2004 5:00 am, George Anzinger wrote:
> > Amit S. Kale wrote:
> > > On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:
> > >>- 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.
> 
> Yes. That's why I added a paramter to putpacket to skip causing a breakpoint 
> when not required.

The problem, and I think I've fixed it, is you always need to look for a
packet when you're sending one (think of gdb going away on you, you
don't really know what you'll be sending when it does), and you can only
look for a '$'.  That's what I've committed now does.

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

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[George Anzinger]

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

-g

Tom Rini wrote:
> 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	trini@kernel.crashing.org	1.1664
> # process_breakpoint/schedule_breakpoint.
> # --------------------------------------------
> #
> diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c
> --- a/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
> +++ b/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
> +++ b/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
> +++ b/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
> +++ b/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
> +++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
> +++ b/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
> @@ -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);
Might consider moving most of this to an internal to kgdb header.  This header 
should only define things that a user would want to access.  These would include 
the breakpoint, possibly asserts and the time stamp stuff.
>  
> +#else
> +#define kgdb_process_breakpoint()      do {} while(0)
> +#endif /* KGDB && !__ASSEMBLY__ */
>  #endif				/* _KGDB_H_ */
> diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c
> --- a/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
> +++ b/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
> @@ -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];
>  

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[Tom Rini]

On Wed, Feb 25, 2004 at 02:59:49PM -0800, George Anzinger wrote:

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

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

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[George Anzinger]

Tom Rini wrote:
> On Wed, Feb 25, 2004 at 02:59:49PM -0800, George Anzinger wrote:
> 
> 
>>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.
> 
Ok.

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[Amit S. Kale]

On Thursday 26 Feb 2004 3:13 am, Tom Rini wrote:
> 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

>
> # 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	trini@kernel.crashing.org	1.1664
> # process_breakpoint/schedule_breakpoint.
> # --------------------------------------------
> #
> diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c
> --- a/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
> +++ b/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
> +++ b/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
> +++ b/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
> +++ b/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
> +++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
> +++ b/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
> @@ -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
> --- a/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
> +++ b/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
> @@ -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];

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[George Anzinger]

Amit S. Kale wrote:
> On Thursday 26 Feb 2004 3:13 am, Tom Rini wrote:
> 
>>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 :-)

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
> 
> OK to checkin.
> -Amit
> 
> 
>># 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	trini@kernel.crashing.org	1.1664
>># process_breakpoint/schedule_breakpoint.
>># --------------------------------------------
>>#
>>diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c
>>--- a/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>+++ b/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>@@ -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
>>--- a/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>+++ b/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>@@ -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
>>--- a/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>+++ b/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>@@ -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
>>--- a/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
>>+++ b/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
>>@@ -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
>>--- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
>>+++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
>>@@ -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
>>--- a/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
>>+++ b/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
>>@@ -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
>>--- a/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
>>+++ b/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
>>@@ -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];
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[Amit S. Kale]

On Friday 27 Feb 2004 4:49 am, George Anzinger wrote:
> Amit S. Kale wrote:
> > On Thursday 26 Feb 2004 3:13 am, Tom Rini wrote:
> >>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 :-)
>
> 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

>
> comments...
>
> -g
>
> > OK to checkin.
> > -Amit
> >
> >># 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	trini@kernel.crashing.org	1.1664
> >># process_breakpoint/schedule_breakpoint.
> >># --------------------------------------------
> >>#
> >>diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c
> >>--- a/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>+++ b/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>@@ -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
> >>--- a/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>+++ b/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>@@ -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
> >>--- a/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>+++ b/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>@@ -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
> >>--- a/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
> >>+++ b/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
> >>@@ -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
> >>--- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
> >>+++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
> >>@@ -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
> >>--- a/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
> >>+++ b/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
> >>@@ -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
> >>--- a/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
> >>+++ b/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
> >>@@ -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];
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> > in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[George Anzinger]

Amit S. Kale wrote:
> On Friday 27 Feb 2004 4:49 am, George Anzinger wrote:
> 
>>Amit S. Kale wrote:
>>
>>>On Thursday 26 Feb 2004 3:13 am, Tom Rini wrote:
>>>
>>>>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 :-)
>>
>>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 :-)

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.

> 
> What are info files?
> 
> -Amit
> 
> 
>>comments...
>>
>>-g
>>
>>
>>>OK to checkin.
>>>-Amit
>>>
>>>
>>>># 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	trini@kernel.crashing.org	1.1664
>>>># process_breakpoint/schedule_breakpoint.
>>>># --------------------------------------------
>>>>#
>>>>diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c
>>>>--- a/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>>>+++ b/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>>>@@ -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
>>>>--- a/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>>>+++ b/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>>>@@ -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
>>>>--- a/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>>>+++ b/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
>>>>@@ -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
>>>>--- a/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
>>>>+++ b/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
>>>>@@ -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
>>>>--- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
>>>>+++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
>>>>@@ -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
>>>>--- a/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
>>>>+++ b/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
>>>>@@ -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
>>>>--- a/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
>>>>+++ b/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
>>>>@@ -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];
>>>
>>>-
>>>To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>>in the body of a message to majordomo@vger.kernel.org
>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[Amit S. Kale]

On Wednesday 03 Mar 2004 2:49 am, George Anzinger wrote:
> Amit S. Kale wrote:
> > On Friday 27 Feb 2004 4:49 am, George Anzinger wrote:
> >>Amit S. Kale wrote:
> >>>On Thursday 26 Feb 2004 3:13 am, Tom Rini wrote:
> >>>>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 :-)
> >>
> >>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 :-)
>
> 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;
>
> --------------------------------------

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.

-Amit


>
> 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.
>
> > What are info files?
> >
> > -Amit
> >
> >>comments...
> >>
> >>-g
> >>
> >>>OK to checkin.
> >>>-Amit
> >>>
> >>>># 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	trini@kernel.crashing.org	1.1664
> >>>># process_breakpoint/schedule_breakpoint.
> >>>># --------------------------------------------
> >>>>#
> >>>>diff -Nru a/arch/i386/kernel/irq.c b/arch/i386/kernel/irq.c
> >>>>--- a/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>>>+++ b/arch/i386/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>>>@@ -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
> >>>>--- a/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>>>+++ b/arch/ppc/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>>>@@ -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
> >>>>--- a/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>>>+++ b/arch/x86_64/kernel/irq.c	Wed Feb 25 14:21:32 2004
> >>>>@@ -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
> >>>>--- a/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
> >>>>+++ b/drivers/net/kgdb_eth.c	Wed Feb 25 14:21:32 2004
> >>>>@@ -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
> >>>>--- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
> >>>>+++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:32 2004
> >>>>@@ -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
> >>>>--- a/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
> >>>>+++ b/include/linux/kgdb.h	Wed Feb 25 14:21:32 2004
> >>>>@@ -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
> >>>>--- a/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
> >>>>+++ b/kernel/kgdb.c	Wed Feb 25 14:21:32 2004
> >>>>@@ -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];
> >>>
> >>>-
> >>>To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> >>>in the body of a message to majordomo@vger.kernel.org
> >>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>Please read the FAQ at  http://www.tux.org/lkml/

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[George Anzinger]

Amit S. Kale wrote:
> On Wednesday 03 Mar 2004 2:49 am, George Anzinger wrote:
> 
>>Amit S. Kale wrote:
>>
>>>On Friday 27 Feb 2004 4:49 am, George Anzinger wrote:
>>>
>>>>Amit S. Kale wrote:
>>>>
>>>>>On Thursday 26 Feb 2004 3:13 am, Tom Rini wrote:
>>>>>
>>>>>>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 :-)
>>>>
>>>>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 :-)
>>
>>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;
>>
>>--------------------------------------
> 
> 
> 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.

~

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[Amit S. Kale]

On Thursday 04 Mar 2004 5:50 am, George Anzinger wrote:
> Amit S. Kale wrote:
> > On Wednesday 03 Mar 2004 2:49 am, George Anzinger wrote:
> >>Amit S. Kale wrote:
> >>>On Friday 27 Feb 2004 4:49 am, George Anzinger wrote:
> >>>>Amit S. Kale wrote:
> >>>>>On Thursday 26 Feb 2004 3:13 am, Tom Rini wrote:
> >>>>>>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 :-)
> >>>>
> >>>>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 :-)
> >>
> >>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;
> >>
> >>--------------------------------------
> >
> > 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

User _will_ see the exact context where he is going to continue. If we type 
$pc=foo \n continue pc will be changed and will run in the same register 
values where the serial irq occured.


> 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

>
> will run in.
>
> ~

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[George Anzinger]

Amit S. Kale wrote:
~

> 
>>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.
~
-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[Amit S. Kale]

On Friday 12 Mar 2004 2:58 am, George Anzinger wrote:
> Amit S. Kale wrote:
> ~
>
> >>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.

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

-Amit

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[George Anzinger]

Amit S. Kale wrote:
> On Friday 12 Mar 2004 2:58 am, George Anzinger wrote:
> 
>>Amit S. Kale wrote:
>>~
>>
>>
>>>>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.
> 
> 
> 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.
> 
> -Amit
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[Amit S. Kale]

On Friday 12 Mar 2004 1:33 pm, George Anzinger wrote:
> Amit S. Kale wrote:
> > On Friday 12 Mar 2004 2:58 am, George Anzinger wrote:
> >>Amit S. Kale wrote:
> >>~
> >>
> >>>>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.
> >
> > 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.

I have seen that. It's too messy!

-Amit

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[George Anzinger]

Amit S. Kale wrote:
> On Friday 12 Mar 2004 1:33 pm, George Anzinger wrote:
> 
>>Amit S. Kale wrote:
>>
>>>On Friday 12 Mar 2004 2:58 am, George Anzinger wrote:
>>>
>>>>Amit S. Kale wrote:
>>>>~
>>>>
>>>>
>>>>>>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.
>>>
>>>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.
> 
> 
> I have seen that. It's too messy!

You have a better solution?
-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[Amit S. Kale]

On Tuesday 16 Mar 2004 1:22 am, George Anzinger wrote:
> Amit S. Kale wrote:
> > On Friday 12 Mar 2004 1:33 pm, George Anzinger wrote:
> >>Amit S. Kale wrote:
> >>>On Friday 12 Mar 2004 2:58 am, George Anzinger wrote:
> >>>>Amit S. Kale wrote:
> >>>>~
> >>>>
> >>>>>>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.
> >>>
> >>>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.
> >
> > I have seen that. It's too messy!
>
> You have a better solution?

Nope.

I think this feature is very likely to be abused by programmers. They do 
deserve suffering if they choose to shoot at their own feet.

Kernel programmers have to be aware of implementation of this feature if they 
choose to call arbitrary functions. This includes knowing whether interrupts 
are disabled, handling of exceptions in gdb called functions, whether other 
processors run, that breakpoints are  disabled. Given that kgdb is used for 
learning the kernel, such features are best kept aside.

-Amit

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[La Monte H.P. Yarroll]

Amit S. Kale wrote:

>On Tuesday 16 Mar 2004 1:22 am, George Anzinger wrote:
>  
>
>>Amit S. Kale wrote:
>>    
>>
>>>On Friday 12 Mar 2004 1:33 pm, George Anzinger wrote:
>>>      
>>>
>>>>Amit S. Kale wrote:
>>>>        
>>>>
>>>>>On Friday 12 Mar 2004 2:58 am, George Anzinger wrote:
>>>>>          
>>>>>
>>>>>>Amit S. Kale wrote:
>>>>>>~
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>>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.
>>>>>>            
>>>>>>
>>>>>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.
>>>>        
>>>>
>>>I have seen that. It's too messy!
>>>      
>>>
>>You have a better solution?
>>    
>>
>
>Nope.
>
>I think this feature is very likely to be abused by programmers. They do 
>deserve suffering if they choose to shoot at their own feet.
>
>Kernel programmers have to be aware of implementation of this feature if they 
>choose to call arbitrary functions. This includes knowing whether interrupts 
>are disabled, handling of exceptions in gdb called functions, whether other 
>processors run, that breakpoints are  disabled. Given that kgdb is used for 
>learning the kernel, such features are best kept aside.
>
>-Amit
>
Pardon my question.  I've lost track of the thread and the available context
doesn't really help me.

Is this the ability to step into an arbitrary function from the gdb command
line or something else?

-- 
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell's sig

Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint

[Amit S. Kale]

On Tuesday 16 Mar 2004 6:32 pm, La Monte H.P. Yarroll wrote:
> Amit S. Kale wrote:
> >On Tuesday 16 Mar 2004 1:22 am, George Anzinger wrote:
> >>Amit S. Kale wrote:
> >>>On Friday 12 Mar 2004 1:33 pm, George Anzinger wrote:
> >>>>Amit S. Kale wrote:
> >>>>>On Friday 12 Mar 2004 2:58 am, George Anzinger wrote:
> >>>>>>Amit S. Kale wrote:
> >>>>>>~
> >>>>>>
> >>>>>>>>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.
> >>>>>
> >>>>>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.
> >>>
> >>>I have seen that. It's too messy!
> >>
> >>You have a better solution?
> >
> >Nope.
> >
> >I think this feature is very likely to be abused by programmers. They do
> >deserve suffering if they choose to shoot at their own feet.
> >
> >Kernel programmers have to be aware of implementation of this feature if
> > they choose to call arbitrary functions. This includes knowing whether
> > interrupts are disabled, handling of exceptions in gdb called functions,
> > whether other processors run, that breakpoints are  disabled. Given that
> > kgdb is used for learning the kernel, such features are best kept aside.
> >
> >-Amit
>
> Pardon my question.  I've lost track of the thread and the available
> context doesn't really help me.
>
> Is this the ability to step into an arbitrary function from the gdb command
> line or something else?

This is ability to call arbitray function from gdb command line.
Example 
(gdb) p myfunction(1,2,3)
-Amit

Re: [Kgdb-bugreport] [PATCH][1/3] Update CVS KGDB's serial driver

[George Anzinger]

Convention has been that control C does the break.  If I read this correctly you 
are saying that any character does it.  If that is the intent and it works, then 
the whole buffer interrupt characters thing can be removed.  In fact, an 
interrupt implies we are not in kgdb (it holds all interrutps off) so that test, 
too, can go.  The interrupt thing then reduces to a breakpoint.

-g

Tom Rini wrote:
> The following updates the serial driver with the fixes / cleanups that
> are in George's version of the driver.  There's a few slightly 'odd'
> things in this patch, which stem from the fact that in my next round of
> patches there will (a) be kernel/Kconfig.kgdb and (b) 1 kgdb i/o driver
> at a time.
> 
> # 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.1662  -> 1.1663 
> #	drivers/serial/kgdb_8250.c	1.2     -> 1.3    
> #	       kernel/kgdb.c	1.2     -> 1.3    
> #	drivers/serial/Kconfig	1.21    -> 1.22   
> #	include/linux/kgdb.h	1.2     -> 1.3    
> #
> # The following is the BitKeeper ChangeSet Log
> # --------------------------------------------
> # 04/02/25	trini@kernel.crashing.org	1.1663
> # Serial update.
> # --------------------------------------------
> #
> diff -Nru a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> --- a/drivers/serial/Kconfig	Wed Feb 25 14:21:26 2004
> +++ b/drivers/serial/Kconfig	Wed Feb 25 14:21:26 2004
> @@ -13,6 +13,27 @@
>  	  Uses generic serial port (8250) for kgdb. This is independent of the
>  	  option 9250/16550 and compatible serial port.
>  
> +config KGDB_PORT
> +	hex "hex I/O port address of the debug serial port"
> +	depends on KGDB_8250
> +	default  3f8
> +	help
> +	  Some systems (x86 family at this writing) allow the port
> +	  address to be configured.  The number entered is assumed to be
> +	  hex, don't put 0x in front of it.  The standard address are:
> +	  COM1 3f8 , irq 4 and COM2 2f8 irq 3.  Setserial /dev/ttySx
> +	  will tell you what you have.  It is good to test the serial
> +	  connection with a live system before trying to debug.
> +
> +config KGDB_IRQ
> +	int "IRQ of the debug serial port"
> +	depends on KGDB_8250
> +	default 4
> +	help
> +	  This is the irq for the debug port.  If everything is working
> +	  correctly and the kernel has interrupts on a control C to the
> +	  port should cause a break into the kernel debug stub.
> +
>  #
>  # The new 8250/16550 serial drivers
>  config SERIAL_8250
> diff -Nru a/drivers/serial/kgdb_8250.c b/drivers/serial/kgdb_8250.c
> --- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:26 2004
> +++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:26 2004
> @@ -1,137 +1,218 @@
>  /*
>   * 8250 interface for kgdb.
>   *
> - * Restructured for making kgdb capable of handling different types of serial
> - * interfaces, by Amit Kale (amitkale@emsyssoft.com)
> - *
> - * Written (hacked together) by David Grothe (dave@gcom.com)
> - *
> - * Modified by Scott Foehner (sfoehner@engr.sgi.com) to allow connect
> - * on boot-up
> + * This is a merging of many different drivers, and all of the people have
> + * had an impact in some form or another:
>   *
> + * Amit Kale <amitkale@emsyssoft.com>
> + * David Grothe <dave@gcom.com>
> + * Scott Foehner <sfoehner@engr.sgi.com>
> + * George Anzinger <george@mvista.com>
> + * Robert Walsh <rjwalsh@durables.org>
> + * wangdi <wangdi@clusterfs.com>
> + * San Mehat
> + * Tom Rini <trini@mvista.com>
>   */
>  
> -#include <linux/module.h>
> -#include <linux/errno.h>
> -#include <linux/signal.h>
> -#include <linux/sched.h>
> -#include <linux/timer.h>
> +#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>
> -#include <linux/tty_flip.h>
>  #include <linux/serial.h>
>  #include <linux/serial_core.h>
>  #include <linux/serial_reg.h>
>  #include <linux/serialP.h>
> -#include <linux/config.h>
> -#include <linux/major.h>
> -#include <linux/string.h>
> -#include <linux/fcntl.h>
> -#include <linux/termios.h>
> -#include <linux/kgdb.h>
>  
> -#include <asm/system.h>
>  #include <asm/io.h>
> -#include <asm/segment.h>
> -#include <asm/bitops.h>
> -#include <asm/system.h>
> -#include <asm/irq.h>
> -#include <asm/atomic.h>
> +#include <asm/serial.h>		/* For BASE_BAUD and SERIAL_PORT_DFNS */
>  
>  #define GDB_BUF_SIZE	512	/* power of 2, please */
>  
> +#if defined(CONFIG_KGDB_9600BAUD)
> +#define SERIAL_BAUD 9600
> +#elif defined(CONFIG_KGDB_19200BAUD)
> +#define SERIAL_BAUD 19200
> +#elif defined(CONFIG_KGDB_38400BAUD)
> +#define SERIAL_BAUD 38400
> +#elif defined(CONFIG_KGDB_57600BAUD)
> +#define SERIAL_BAUD 57600
> +#elif defined(CONFIG_KGDB_115200BAUD)
> +#define SERIAL_BAUD 115200
> +#else
> +#define SERIAL_BAUD 115200	/* Start with this if not given */
> +#endif
> +
> +#if defined(CONFIG_KGDB_TTYS0)
> +#define KGDB_PORT 0
> +#elif defined(CONFIG_KGDB_TTYS1)
> +#define KGDB_PORT 1
> +#elif defined(CONFIG_KGDB_TTYS2)
> +#define KGDB_PORT 2
> +#elif defined(CONFIG_KGDB_TTYS3)
> +#define KGDB_PORT 3
> +#else
> +#define KGDB_PORT 0		/* Start with this if not given */
> +#endif
> +
> +int kgdb8250_baud = SERIAL_BAUD;
> +int kgdb8250_ttyS = KGDB_PORT;
> +
>  static char kgdb8250_buf[GDB_BUF_SIZE];
>  static int kgdb8250_buf_in_inx;
>  static atomic_t kgdb8250_buf_in_cnt;
>  static int kgdb8250_buf_out_inx;
>  
> -static int kgdb8250_got_dollar = -3, kgdb8250_got_H = -3,
> -    kgdb8250_interrupt_iteration = 0;
> +/* Determine serial information. */
> +static struct serial_state state = {
> +	.magic = SSTATE_MAGIC,
> +	.baud_base = BASE_BAUD,
> +	.custom_divisor = BASE_BAUD / SERIAL_BAUD,
> +};
>  
> -/* We only allow for 4 ports to be registered.  We default to standard
> - * PC values. */
> -static struct uart_port rs_table[4] = {
> -	{.line = 0x3f8,.irq = 4},
> -	{.line = 0x2f8,.irq = 3},
> -	{.line = 0x3e8,.irq = 4},
> -	{.line = 0x2e8,.irq = 3},
> +static struct async_struct gdb_async_info = {
> +	.magic = SERIAL_MAGIC,
> +	.state = &state,
> +	.tty = (struct tty_struct *) &state,
>  };
> -static void (*serial_outb) (unsigned char, unsigned long);
> -static unsigned long (*serial_inb) (unsigned long);
> +
> +/* Space between registers. */
> +static int reg_shift;
> +
> +/* Not all arches define this. */
> +#ifndef SERIAL_PORT_DFNS
> +#define SERIAL_PORT_DFNS
> +#endif
> +static struct serial_state rs_table[] = {
> +	SERIAL_PORT_DFNS	/* defined in <asm/serial.h> */
> +};
> +
> +/* Do we need to look in the rs_table? */
> +static int serial_from_rs_table = 0;
> +
> +static void (*serial_outb) (unsigned char val, unsigned long addr);
> +static unsigned long (*serial_inb) (unsigned long addr);
>  
>  int serial8250_release_irq(int irq);
>  
> -int kgdb8250_irq;
> -unsigned long kgdb8250_port;
> +static int kgdb8250_init(void);
> +static unsigned long kgdb8250_port;
>  
> -int kgdb8250_baud = 115200;
> -int kgdb8250_ttyS;
> +static int initialized = -1;
>  
> -static unsigned long direct_inb(unsigned long addr)
> +static unsigned long
> +direct_inb(unsigned long addr)
>  {
>  	return readb(addr);
>  }
>  
> -static void direct_outb(unsigned char val, unsigned long addr)
> +static void
> +direct_outb(unsigned char val, unsigned long addr)
>  {
>  	writeb(val, addr);
>  }
>  
> -static unsigned long io_inb(unsigned long port)
> +static unsigned long
> +io_inb(unsigned long port)
>  {
>  	return inb(port);
>  }
>  
> -static void io_outb(unsigned char val, unsigned long port)
> +static void
> +io_outb(unsigned char val, unsigned long port)
>  {
>  	outb(val, port);
>  }
>  
>  /*
> - * Get a byte from the hardware data buffer and return it
> - * Get a char if available, return -EAGAIN if nothing available.
> + * Wait until the interface can accept a char, then write it.
>   */
> -static int read_data_bfr(void)
> +void
> +kgdb_put_debug_char(int chr)
>  {
> -	if (serial_inb(kgdb8250_port + UART_LSR) & UART_LSR_DR)
> -		return (serial_inb(kgdb8250_port + UART_RX));
> +	while (!(serial_inb(kgdb8250_port + (UART_LSR << reg_shift)) &
> +		UART_LSR_THRE))
> +		;
>  
> -	return -EAGAIN;
> +	serial_outb(chr, kgdb8250_port + (UART_TX << reg_shift));
>  }
>  
>  /*
> - * Empty the receive buffer first, then look at the interface hardware.
> - * It waits for a character from the serial interface and then returns it.
> + * Get a byte from the hardware data buffer and return it
>   */
> -static int kgdb8250_read_char(void)
> +static int
> +read_data_bfr(void)
>  {
> -	int retchar;
> -	if (atomic_read(&kgdb8250_buf_in_cnt) != 0) {
> -		/* intr routine has q'd chars read them from buffer */
> -		int chr;
> +	char it = serial_inb(kgdb8250_port + (UART_LSR << reg_shift));
>  
> -		chr = kgdb8250_buf[kgdb8250_buf_out_inx++];
> -		kgdb8250_buf_out_inx &= (GDB_BUF_SIZE - 1);
> -		atomic_dec(&kgdb8250_buf_in_cnt);
> -		return chr;
> +	if (it & UART_LSR_DR)
> +		return serial_inb(kgdb8250_port + (UART_RX << reg_shift));
> +
> +	/*
> +	 * If we have a framing error assume somebody messed with
> +	 * our uart.  Reprogram it and send '-' both ways...
> +	 */
> +	if (it & 0xc) {
> +		kgdb8250_init();
> +		kgdb_put_debug_char('-');
> +		return '-';
>  	}
> -	do {
> -		/* read from hardware */
> -		retchar = read_data_bfr();
> -	} while (retchar < 0);
> -	return retchar;
> +
> +	return -1;
>  }
>  
>  /*
> - * Wait until the interface can accept a char, then write it.
> + * 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 void kgdb8250_write_char(int chr)
> +
> +static spinlock_t uart_interrupt_lock = SPIN_LOCK_UNLOCKED;
> +#ifdef CONFIG_SMP
> +extern spinlock_t kgdb_spinlock;
> +#endif
> +
> +int
> +kgdb_get_debug_char(void)
>  {
> -	while (!(serial_inb(kgdb8250_port + UART_LSR) & UART_LSR_THRE))
> -		/* Do nothing */ ;
> +	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++];
> +		kgdb8250_buf_out_inx &= (GDB_BUF_SIZE - 1);
> +		atomic_dec(&kgdb8250_buf_in_cnt);
> +		goto out;
> +	}
> +
> +	do {
> +		retchr = read_data_bfr();
> +	} while (retchr < 0);
>  
> -	serial_outb(chr, kgdb8250_port + UART_TX);
> +out:
> +#ifdef CONFIG_SMP
> +	if (!spin_is_locked(&kgdb_spinlock)) {
> +		spin_unlock(&uart_interrupt_lock);
> +	}
> +#endif
> +	local_irq_restore(flags);
>  
> +	return retchr;
>  }
>  
>  /*
> @@ -139,12 +220,12 @@
>   * It will receive a limited number of characters of input
>   * from the gdb  host machine and save them up in a buffer.
>   *
> - * When kgdb8250_read_char() is called it
> + * 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 kgdb8250_write_char() which
> + * since the stubs do all of that via kgdb_put_debug_char() which
>   * writes one byte after waiting for the interface to become
>   * ready.
>   *
> @@ -156,15 +237,27 @@
>   * care to learn can make this work for any low level serial
>   * driver.
>   */
> -static irqreturn_t kgdb8250_interrupt(int irq, void *dev_id,
> -				      struct pt_regs *regs)
> +static irqreturn_t
> +kgdb8250_interrupt(int irq, void *dev_id, struct pt_regs *regs)
>  {
> -	int chr;
> -	int iir;
> +	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) {
> +		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);
> +		iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
>  		if (chr < 0)
>  			continue;
>  
> @@ -174,28 +267,6 @@
>  			continue;
>  		}
>  
> -		if (atomic_read(&kgdb_killed_or_detached)) {
> -			if (chr == '$')
> -				kgdb8250_got_dollar =
> -				    kgdb8250_interrupt_iteration;
> -			if (kgdb8250_interrupt_iteration ==
> -			    kgdb8250_got_dollar + 1 && chr == 'H')
> -				kgdb8250_got_H = kgdb8250_interrupt_iteration;
> -			else if (kgdb8250_interrupt_iteration ==
> -				 kgdb8250_got_H + 1 && chr == 'c') {
> -				kgdb8250_buf[kgdb8250_buf_in_inx++] = chr;
> -				atomic_inc(&kgdb8250_buf_in_cnt);
> -				atomic_set(&kgdb_might_be_resumed, 1);
> -				wmb();
> -				breakpoint();
> -				atomic_set(&kgdb_might_be_resumed, 0);
> -				kgdb8250_interrupt_iteration = 0;
> -				kgdb8250_got_dollar = -3;
> -				kgdb8250_got_H = -3;
> -				continue;
> -			}
> -		}
> -
>  		if (atomic_read(&kgdb8250_buf_in_cnt) >= GDB_BUF_SIZE) {
>  			/* buffer overflow, clear it */
>  			kgdb8250_buf_in_inx = 0;
> @@ -207,33 +278,30 @@
>  		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);
> +	} while (iir & UART_IIR_RDI);
>  
> -	if (atomic_read(&kgdb_killed_or_detached))
> -		kgdb8250_interrupt_iteration++;
> +	spin_unlock(&uart_interrupt_lock);
> +	local_irq_restore(flags);
>  
>  	return IRQ_HANDLED;
> -
>  }
>  
>  /* 
> - * Initializes serial port.
> - *	ttyS - integer specifying which serial port to use for debugging
> - *	baud - baud rate of specified serial port
> + *  Returns:
> + *	0 on success, 1 on failure.
>   */
> -static int kgdb8250_init(int ttyS, int baud)
> +static int
> +kgdb8250_init(void)
>  {
>  	unsigned cval;
>  	int bits = 8;
>  	int parity = 'n';
>  	int cflag = CREAD | HUPCL | CLOCAL;
> -	int quot = 0;
>  
>  	/*
>  	 *      Now construct a cflag setting.
>  	 */
> -	switch (baud) {
> +	switch (kgdb8250_baud) {
>  	case 1200:
>  		cflag |= B1200;
>  		break;
> @@ -256,7 +324,7 @@
>  		cflag |= B115200;
>  		break;
>  	default:
> -		baud = 9600;
> +		kgdb8250_baud = 9600;
>  		/* Fall through */
>  	case 9600:
>  		cflag |= B9600;
> @@ -287,7 +355,6 @@
>  	 *
>  	 */
>  
> -	quot = (1843200 / 16) / baud;
>  	cval = cflag & (CSIZE | CSTOPB);
>  	cval >>= 4;
>  	if (cflag & PARENB)
> @@ -300,75 +367,118 @@
>  	 *      and set speed.
>  	 */
>  	cval = 0x3;
> -	serial_outb(cval | UART_LCR_DLAB, kgdb8250_port + UART_LCR);	/* set DLAB */
> -	serial_outb(quot & 0xff, kgdb8250_port + UART_DLL);	/* LS of divisor */
> -	serial_outb(quot >> 8, kgdb8250_port + UART_DLM);	/* MS of divisor */
> -	serial_outb(cval, kgdb8250_port + UART_LCR);	/* reset DLAB */
> -	serial_outb(UART_IER_RDI, kgdb8250_port + UART_IER);	/* turn on interrupts */
> +	/* set DLAB */
> +	serial_outb(cval | UART_LCR_DLAB, kgdb8250_port +
> +			(UART_LCR << reg_shift));
> +	/* LS */
> +	serial_outb(gdb_async_info.state->custom_divisor & 0xff,
> +			kgdb8250_port + (UART_DLL << reg_shift));
> +	/* MS  */
> +	serial_outb(gdb_async_info.state->custom_divisor >> 8,
> +			kgdb8250_port + (UART_DLM << reg_shift));
> +	/* reset DLAB */
> +	serial_outb(cval, kgdb8250_port + (UART_LCR << reg_shift));
> +	/* turn on interrupts */
> +	serial_outb(UART_IER_RDI, kgdb8250_port + (UART_IER << reg_shift));
>  	serial_outb(UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS,
> -		    kgdb8250_port + UART_MCR);
> +		    kgdb8250_port + (UART_MCR << reg_shift));
>  
>  	/*
>  	 *      If we read 0xff from the LSR, there is no UART here.
>  	 */
> -	if (serial_inb(kgdb8250_port + UART_LSR) == 0xff)
> -		return -ENODEV;
> +	if (serial_inb(kgdb8250_port + (UART_LSR << reg_shift)) == 0xff)
> +		return -1;
>  	return 0;
>  }
>  
> -int kgdb8250_hook(void)
> +int
> +kgdb_hook_io(void)
>  {
>  	int retval;
>  
> -	/*
> -	 * Set port and irq number.
> -	 */
> -	kgdb8250_irq = rs_table[kgdb8250_ttyS].irq;
> -	switch (rs_table[kgdb8250_ttyS].iotype) {
> +	/* Setup any serial port information we may need to */
> +#ifdef CONFIG_KGDB_SIMPLE_SERIAL
> +	/* We must look in the rs_table[]. */
> +	serial_from_rs_table = 1;
> +#endif
> +	/* If the user has overriden our definitions, or if we've only
> +	 * been told the ttyS to use, look at rs_table. */
> +	if (serial_from_rs_table) {
> +		/* The user has selected one of ttyS[0-3], which we pull
> +		 * from rs_table[].  If this doesn't exist, user error. */
> +		gdb_async_info.port = gdb_async_info.state->port =
> +		    rs_table[KGDB_PORT].port;
> +		gdb_async_info.line = gdb_async_info.state->irq =
> +		    rs_table[KGDB_PORT].irq;
> +		gdb_async_info.state->io_type = rs_table[KGDB_PORT].io_type;
> +		reg_shift = rs_table[KGDB_PORT].iomem_reg_shift;
> +	}
> +
> +	switch (gdb_async_info.state->io_type) {
>  	case SERIAL_IO_MEM:
> -		kgdb8250_port = (unsigned long)rs_table[kgdb8250_ttyS].membase;
> -		serial_inb = direct_inb;
> +		kgdb8250_port = (unsigned long)
> +		    rs_table[kgdb8250_ttyS].iomem_base;
>  		serial_outb = direct_outb;
> +		serial_inb = direct_inb;
>  		break;
> +	case SERIAL_IO_PORT:
>  	default:
> -		kgdb8250_port = rs_table[kgdb8250_ttyS].line;
> -		serial_inb = io_inb;
> +		kgdb8250_port = rs_table[kgdb8250_ttyS].port;
>  		serial_outb = io_outb;
> +		serial_inb = io_inb;
>  	}
>  
> +#ifndef CONFIG_KGDB_SIMPLE_SERIAL
> +	/* The user has provided the IRQ and I/O location. */
> +	kgdb8250_port = gdb_async_info.port = gdb_async_info.state->port =
> +		CONFIG_KGDB_PORT;
> +	gdb_async_info.line = gdb_async_info.state->irq = CONFIG_KGDB_IRQ;
> +#endif
> +
>  #ifdef CONFIG_SERIAL_8250
> -	if ((retval = serial8250_release_irq(kgdb8250_irq)) < 0)
> -		return retval;
> +	if (serial8250_release_irq(gdb_async_info.line))
> +		return -1;
>  #endif
>  
> -	if ((retval = kgdb8250_init(kgdb8250_ttyS, kgdb8250_baud)) < 0)
> -		return retval;
> +	if (kgdb8250_init() == -1)
> +		return -1;
>  
> -	retval = request_irq(kgdb8250_irq, kgdb8250_interrupt, SA_INTERRUPT,
> -			     "GDB-stub", NULL);
> -	return retval;
> -}
> +	retval = request_irq(gdb_async_info.line, kgdb8250_interrupt,
> +			     SA_INTERRUPT, "GDB-stub", NULL);
> +	if (retval == 0)
> +		initialized = 1;
> +	else
> +		initialized = 0;
>  
> -void kgdb8250_add_port(int i, struct uart_port *serial_req)
> -{
> -	rs_table[i].iotype = serial_req->iotype;
> -	rs_table[i].line = serial_req->line;
> -	rs_table[i].membase = serial_req->membase;
> -	rs_table[i].regshift = serial_req->regshift;
> +	return 0;
>  }
>  
>  struct kgdb_serial kgdb8250_serial_driver = {
> -	.read_char = kgdb8250_read_char,
> -	.write_char = kgdb8250_write_char,
> -	.hook = kgdb8250_hook
> +	.read_char = kgdb_get_debug_char,
> +	.write_char = kgdb_put_debug_char,
> +	.hook = kgdb_hook_io,
>  };
>  
> +void
> +kgdb8250_add_port(int i, struct uart_port *serial_req)
> +{
> +	rs_table[i].io_type = serial_req->iotype;
> +	rs_table[i].port = serial_req->line;
> +	rs_table[i].irq = serial_req->irq;
> +	rs_table[i].iomem_base = serial_req->membase;
> +	rs_table[i].iomem_reg_shift = serial_req->regshift;
> +
> +	/* We will want to look in the rs_table now. */
> +	serial_from_rs_table = 1;
> +}
> +
>  /*
>   * Syntax for this cmdline option is
>   * kgdb8250=ttyno,baudrate
>   */
>  
> -static int __init kgdb8250_opt(char *str)
> +static int __init
> +kgdb8250_opt(char *str)
>  {
>  	if (*str < '0' || *str > '3')
>  		goto errout;
> @@ -382,7 +492,12 @@
>  	    kgdb8250_baud != 38400 && kgdb8250_baud != 57600 &&
>  	    kgdb8250_baud != 115200)
>  		goto errout;
> +
> +	/* Make the baud rate change happen. */
> +	gdb_async_info.state->custom_divisor = BASE_BAUD / kgdb8250_baud;
> +
>  	kgdb_serial = &kgdb8250_serial_driver;
> +
>  	return 1;
>  
>        errout:
> diff -Nru a/include/linux/kgdb.h b/include/linux/kgdb.h
> --- a/include/linux/kgdb.h	Wed Feb 25 14:21:26 2004
> +++ b/include/linux/kgdb.h	Wed Feb 25 14:21:26 2004
> @@ -25,6 +25,8 @@
>  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;
>  
>  enum kgdb_bptype {
> diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c
> --- a/kernel/kgdb.c	Wed Feb 25 14:21:26 2004
> +++ b/kernel/kgdb.c	Wed Feb 25 14:21:26 2004
> @@ -63,7 +63,7 @@
>   * has connected to kgdb.
>   */
>  int kgdb_initialized = 0;
> -static volatile int kgdb_connected;
> +volatile int kgdb_connected;
>  
>  /* If non-zero, wait for a gdb connection when kgdb_entry is called */
>  int kgdb_enter = 0;
> @@ -214,6 +214,7 @@
>  	do {
>  		/* wait around for the start character, ignore all other characters */
>  		while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$') ;
> +		kgdb_connected = 1;
>  		checksum = 0;
>  		xmitcsum = -1;
>  

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [PATCH][1/3] Update CVS KGDB's serial driver

[Tom Rini]

On Wed, Feb 25, 2004 at 03:10:03PM -0800, George Anzinger wrote:

> Convention has been that control C does the break.  If I read this 
> correctly you are saying that any character does it.

There are two cases.  One is that kgdb is not connected (so it's not
active) and since we're the int handler, someone wants to connect.  This
lets us do very nice things like break in any old time (gdb doesn't need
to be modified to send a ^C before it's first packet, etc).  The second
case is that we have connected, we've issued a few commands, continued,
and we want to break in now, so we ^C in gdb (which sends a ^C to the
target) and we do what we've got to do.

So perhaps, and I haven't read all of the code again to verify this,
kgdb_connected could be eliminated in favor of debugger_active.

> If that is the intent 
> and it works, then the whole buffer interrupt characters thing can be 
> removed.  In fact, an interrupt implies we are not in kgdb (it holds all 
> interrutps off) so that test, too, can go.  The interrupt thing then 
> reduces to a breakpoint.

I'll give that a shot tomorrow and see what happens.

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

Re: [Kgdb-bugreport] [PATCH][1/3] Update CVS KGDB's serial driver

[Amit S. Kale]

On Thursday 26 Feb 2004 3:06 am, Tom Rini wrote:
> The following updates the serial driver with the fixes / cleanups that
> are in George's version of the driver.  There's a few slightly 'odd'
> things in this patch, which stem from the fact that in my next round of
> patches there will (a) be kernel/Kconfig.kgdb and (b) 1 kgdb i/o driver
> at a time.

Please send them in consecutive emails. Having separate patches is fine.
I can't figure out from this patch how a user is going to configure kgdb 8250 
options from menuconfig.

-Amit

>
> # 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.1662  -> 1.1663
> #	drivers/serial/kgdb_8250.c	1.2     -> 1.3
> #	       kernel/kgdb.c	1.2     -> 1.3
> #	drivers/serial/Kconfig	1.21    -> 1.22
> #	include/linux/kgdb.h	1.2     -> 1.3
> #
> # The following is the BitKeeper ChangeSet Log
> # --------------------------------------------
> # 04/02/25	trini@kernel.crashing.org	1.1663
> # Serial update.
> # --------------------------------------------
> #
> diff -Nru a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> --- a/drivers/serial/Kconfig	Wed Feb 25 14:21:26 2004
> +++ b/drivers/serial/Kconfig	Wed Feb 25 14:21:26 2004
> @@ -13,6 +13,27 @@
>  	  Uses generic serial port (8250) for kgdb. This is independent of the
>  	  option 9250/16550 and compatible serial port.
>
> +config KGDB_PORT
> +	hex "hex I/O port address of the debug serial port"
> +	depends on KGDB_8250
> +	default  3f8
> +	help
> +	  Some systems (x86 family at this writing) allow the port
> +	  address to be configured.  The number entered is assumed to be
> +	  hex, don't put 0x in front of it.  The standard address are:
> +	  COM1 3f8 , irq 4 and COM2 2f8 irq 3.  Setserial /dev/ttySx
> +	  will tell you what you have.  It is good to test the serial
> +	  connection with a live system before trying to debug.
> +
> +config KGDB_IRQ
> +	int "IRQ of the debug serial port"
> +	depends on KGDB_8250
> +	default 4
> +	help
> +	  This is the irq for the debug port.  If everything is working
> +	  correctly and the kernel has interrupts on a control C to the
> +	  port should cause a break into the kernel debug stub.
> +
>  #
>  # The new 8250/16550 serial drivers
>  config SERIAL_8250
> diff -Nru a/drivers/serial/kgdb_8250.c b/drivers/serial/kgdb_8250.c
> --- a/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:26 2004
> +++ b/drivers/serial/kgdb_8250.c	Wed Feb 25 14:21:26 2004
> @@ -1,137 +1,218 @@
>  /*
>   * 8250 interface for kgdb.
>   *
> - * Restructured for making kgdb capable of handling different types of
> serial - * interfaces, by Amit Kale (amitkale@emsyssoft.com)
> - *
> - * Written (hacked together) by David Grothe (dave@gcom.com)
> - *
> - * Modified by Scott Foehner (sfoehner@engr.sgi.com) to allow connect
> - * on boot-up
> + * This is a merging of many different drivers, and all of the people have
> + * had an impact in some form or another:
>   *
> + * Amit Kale <amitkale@emsyssoft.com>
> + * David Grothe <dave@gcom.com>
> + * Scott Foehner <sfoehner@engr.sgi.com>
> + * George Anzinger <george@mvista.com>
> + * Robert Walsh <rjwalsh@durables.org>
> + * wangdi <wangdi@clusterfs.com>
> + * San Mehat
> + * Tom Rini <trini@mvista.com>
>   */
>
> -#include <linux/module.h>
> -#include <linux/errno.h>
> -#include <linux/signal.h>
> -#include <linux/sched.h>
> -#include <linux/timer.h>
> +#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>
> -#include <linux/tty_flip.h>
>  #include <linux/serial.h>
>  #include <linux/serial_core.h>
>  #include <linux/serial_reg.h>
>  #include <linux/serialP.h>
> -#include <linux/config.h>
> -#include <linux/major.h>
> -#include <linux/string.h>
> -#include <linux/fcntl.h>
> -#include <linux/termios.h>
> -#include <linux/kgdb.h>
>
> -#include <asm/system.h>
>  #include <asm/io.h>
> -#include <asm/segment.h>
> -#include <asm/bitops.h>
> -#include <asm/system.h>
> -#include <asm/irq.h>
> -#include <asm/atomic.h>
> +#include <asm/serial.h>		/* For BASE_BAUD and SERIAL_PORT_DFNS */
>
>  #define GDB_BUF_SIZE	512	/* power of 2, please */
>
> +#if defined(CONFIG_KGDB_9600BAUD)
> +#define SERIAL_BAUD 9600
> +#elif defined(CONFIG_KGDB_19200BAUD)
> +#define SERIAL_BAUD 19200
> +#elif defined(CONFIG_KGDB_38400BAUD)
> +#define SERIAL_BAUD 38400
> +#elif defined(CONFIG_KGDB_57600BAUD)
> +#define SERIAL_BAUD 57600
> +#elif defined(CONFIG_KGDB_115200BAUD)
> +#define SERIAL_BAUD 115200
> +#else
> +#define SERIAL_BAUD 115200	/* Start with this if not given */
> +#endif
> +
> +#if defined(CONFIG_KGDB_TTYS0)
> +#define KGDB_PORT 0
> +#elif defined(CONFIG_KGDB_TTYS1)
> +#define KGDB_PORT 1
> +#elif defined(CONFIG_KGDB_TTYS2)
> +#define KGDB_PORT 2
> +#elif defined(CONFIG_KGDB_TTYS3)
> +#define KGDB_PORT 3
> +#else
> +#define KGDB_PORT 0		/* Start with this if not given */
> +#endif
> +
> +int kgdb8250_baud = SERIAL_BAUD;
> +int kgdb8250_ttyS = KGDB_PORT;
> +
>  static char kgdb8250_buf[GDB_BUF_SIZE];
>  static int kgdb8250_buf_in_inx;
>  static atomic_t kgdb8250_buf_in_cnt;
>  static int kgdb8250_buf_out_inx;
>
> -static int kgdb8250_got_dollar = -3, kgdb8250_got_H = -3,
> -    kgdb8250_interrupt_iteration = 0;
> +/* Determine serial information. */
> +static struct serial_state state = {
> +	.magic = SSTATE_MAGIC,
> +	.baud_base = BASE_BAUD,
> +	.custom_divisor = BASE_BAUD / SERIAL_BAUD,
> +};
>
> -/* We only allow for 4 ports to be registered.  We default to standard
> - * PC values. */
> -static struct uart_port rs_table[4] = {
> -	{.line = 0x3f8,.irq = 4},
> -	{.line = 0x2f8,.irq = 3},
> -	{.line = 0x3e8,.irq = 4},
> -	{.line = 0x2e8,.irq = 3},
> +static struct async_struct gdb_async_info = {
> +	.magic = SERIAL_MAGIC,
> +	.state = &state,
> +	.tty = (struct tty_struct *) &state,
>  };
> -static void (*serial_outb) (unsigned char, unsigned long);
> -static unsigned long (*serial_inb) (unsigned long);
> +
> +/* Space between registers. */
> +static int reg_shift;
> +
> +/* Not all arches define this. */
> +#ifndef SERIAL_PORT_DFNS
> +#define SERIAL_PORT_DFNS
> +#endif
> +static struct serial_state rs_table[] = {
> +	SERIAL_PORT_DFNS	/* defined in <asm/serial.h> */
> +};
> +
> +/* Do we need to look in the rs_table? */
> +static int serial_from_rs_table = 0;
> +
> +static void (*serial_outb) (unsigned char val, unsigned long addr);
> +static unsigned long (*serial_inb) (unsigned long addr);
>
>  int serial8250_release_irq(int irq);
>
> -int kgdb8250_irq;
> -unsigned long kgdb8250_port;
> +static int kgdb8250_init(void);
> +static unsigned long kgdb8250_port;
>
> -int kgdb8250_baud = 115200;
> -int kgdb8250_ttyS;
> +static int initialized = -1;
>
> -static unsigned long direct_inb(unsigned long addr)
> +static unsigned long
> +direct_inb(unsigned long addr)
>  {
>  	return readb(addr);
>  }
>
> -static void direct_outb(unsigned char val, unsigned long addr)
> +static void
> +direct_outb(unsigned char val, unsigned long addr)
>  {
>  	writeb(val, addr);
>  }
>
> -static unsigned long io_inb(unsigned long port)
> +static unsigned long
> +io_inb(unsigned long port)
>  {
>  	return inb(port);
>  }
>
> -static void io_outb(unsigned char val, unsigned long port)
> +static void
> +io_outb(unsigned char val, unsigned long port)
>  {
>  	outb(val, port);
>  }
>
>  /*
> - * Get a byte from the hardware data buffer and return it
> - * Get a char if available, return -EAGAIN if nothing available.
> + * Wait until the interface can accept a char, then write it.
>   */
> -static int read_data_bfr(void)
> +void
> +kgdb_put_debug_char(int chr)
>  {
> -	if (serial_inb(kgdb8250_port + UART_LSR) & UART_LSR_DR)
> -		return (serial_inb(kgdb8250_port + UART_RX));
> +	while (!(serial_inb(kgdb8250_port + (UART_LSR << reg_shift)) &
> +		UART_LSR_THRE))
> +		;
>
> -	return -EAGAIN;
> +	serial_outb(chr, kgdb8250_port + (UART_TX << reg_shift));
>  }
>
>  /*
> - * Empty the receive buffer first, then look at the interface hardware.
> - * It waits for a character from the serial interface and then returns it.
> + * Get a byte from the hardware data buffer and return it
>   */
> -static int kgdb8250_read_char(void)
> +static int
> +read_data_bfr(void)
>  {
> -	int retchar;
> -	if (atomic_read(&kgdb8250_buf_in_cnt) != 0) {
> -		/* intr routine has q'd chars read them from buffer */
> -		int chr;
> +	char it = serial_inb(kgdb8250_port + (UART_LSR << reg_shift));
>
> -		chr = kgdb8250_buf[kgdb8250_buf_out_inx++];
> -		kgdb8250_buf_out_inx &= (GDB_BUF_SIZE - 1);
> -		atomic_dec(&kgdb8250_buf_in_cnt);
> -		return chr;
> +	if (it & UART_LSR_DR)
> +		return serial_inb(kgdb8250_port + (UART_RX << reg_shift));
> +
> +	/*
> +	 * If we have a framing error assume somebody messed with
> +	 * our uart.  Reprogram it and send '-' both ways...
> +	 */
> +	if (it & 0xc) {
> +		kgdb8250_init();
> +		kgdb_put_debug_char('-');
> +		return '-';
>  	}
> -	do {
> -		/* read from hardware */
> -		retchar = read_data_bfr();
> -	} while (retchar < 0);
> -	return retchar;
> +
> +	return -1;
>  }
>
>  /*
> - * Wait until the interface can accept a char, then write it.
> + * 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 void kgdb8250_write_char(int chr)
> +
> +static spinlock_t uart_interrupt_lock = SPIN_LOCK_UNLOCKED;
> +#ifdef CONFIG_SMP
> +extern spinlock_t kgdb_spinlock;
> +#endif
> +
> +int
> +kgdb_get_debug_char(void)
>  {
> -	while (!(serial_inb(kgdb8250_port + UART_LSR) & UART_LSR_THRE))
> -		/* Do nothing */ ;
> +	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++];
> +		kgdb8250_buf_out_inx &= (GDB_BUF_SIZE - 1);
> +		atomic_dec(&kgdb8250_buf_in_cnt);
> +		goto out;
> +	}
> +
> +	do {
> +		retchr = read_data_bfr();
> +	} while (retchr < 0);
>
> -	serial_outb(chr, kgdb8250_port + UART_TX);
> +out:
> +#ifdef CONFIG_SMP
> +	if (!spin_is_locked(&kgdb_spinlock)) {
> +		spin_unlock(&uart_interrupt_lock);
> +	}
> +#endif
> +	local_irq_restore(flags);
>
> +	return retchr;
>  }
>
>  /*
> @@ -139,12 +220,12 @@
>   * It will receive a limited number of characters of input
>   * from the gdb  host machine and save them up in a buffer.
>   *
> - * When kgdb8250_read_char() is called it
> + * 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 kgdb8250_write_char() which
> + * since the stubs do all of that via kgdb_put_debug_char() which
>   * writes one byte after waiting for the interface to become
>   * ready.
>   *
> @@ -156,15 +237,27 @@
>   * care to learn can make this work for any low level serial
>   * driver.
>   */
> -static irqreturn_t kgdb8250_interrupt(int irq, void *dev_id,
> -				      struct pt_regs *regs)
> +static irqreturn_t
> +kgdb8250_interrupt(int irq, void *dev_id, struct pt_regs *regs)
>  {
> -	int chr;
> -	int iir;
> +	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) {
> +		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);
> +		iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
>  		if (chr < 0)
>  			continue;
>
> @@ -174,28 +267,6 @@
>  			continue;
>  		}
>
> -		if (atomic_read(&kgdb_killed_or_detached)) {
> -			if (chr == '$')
> -				kgdb8250_got_dollar =
> -				    kgdb8250_interrupt_iteration;
> -			if (kgdb8250_interrupt_iteration ==
> -			    kgdb8250_got_dollar + 1 && chr == 'H')
> -				kgdb8250_got_H = kgdb8250_interrupt_iteration;
> -			else if (kgdb8250_interrupt_iteration ==
> -				 kgdb8250_got_H + 1 && chr == 'c') {
> -				kgdb8250_buf[kgdb8250_buf_in_inx++] = chr;
> -				atomic_inc(&kgdb8250_buf_in_cnt);
> -				atomic_set(&kgdb_might_be_resumed, 1);
> -				wmb();
> -				breakpoint();
> -				atomic_set(&kgdb_might_be_resumed, 0);
> -				kgdb8250_interrupt_iteration = 0;
> -				kgdb8250_got_dollar = -3;
> -				kgdb8250_got_H = -3;
> -				continue;
> -			}
> -		}
> -
>  		if (atomic_read(&kgdb8250_buf_in_cnt) >= GDB_BUF_SIZE) {
>  			/* buffer overflow, clear it */
>  			kgdb8250_buf_in_inx = 0;
> @@ -207,33 +278,30 @@
>  		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);
> +	} while (iir & UART_IIR_RDI);
>
> -	if (atomic_read(&kgdb_killed_or_detached))
> -		kgdb8250_interrupt_iteration++;
> +	spin_unlock(&uart_interrupt_lock);
> +	local_irq_restore(flags);
>
>  	return IRQ_HANDLED;
> -
>  }
>
>  /*
> - * Initializes serial port.
> - *	ttyS - integer specifying which serial port to use for debugging
> - *	baud - baud rate of specified serial port
> + *  Returns:
> + *	0 on success, 1 on failure.
>   */
> -static int kgdb8250_init(int ttyS, int baud)
> +static int
> +kgdb8250_init(void)
>  {
>  	unsigned cval;
>  	int bits = 8;
>  	int parity = 'n';
>  	int cflag = CREAD | HUPCL | CLOCAL;
> -	int quot = 0;
>
>  	/*
>  	 *      Now construct a cflag setting.
>  	 */
> -	switch (baud) {
> +	switch (kgdb8250_baud) {
>  	case 1200:
>  		cflag |= B1200;
>  		break;
> @@ -256,7 +324,7 @@
>  		cflag |= B115200;
>  		break;
>  	default:
> -		baud = 9600;
> +		kgdb8250_baud = 9600;
>  		/* Fall through */
>  	case 9600:
>  		cflag |= B9600;
> @@ -287,7 +355,6 @@
>  	 *
>  	 */
>
> -	quot = (1843200 / 16) / baud;
>  	cval = cflag & (CSIZE | CSTOPB);
>  	cval >>= 4;
>  	if (cflag & PARENB)
> @@ -300,75 +367,118 @@
>  	 *      and set speed.
>  	 */
>  	cval = 0x3;
> -	serial_outb(cval | UART_LCR_DLAB, kgdb8250_port + UART_LCR);	/* set DLAB
> */ -	serial_outb(quot & 0xff, kgdb8250_port + UART_DLL);	/* LS of divisor
> */ -	serial_outb(quot >> 8, kgdb8250_port + UART_DLM);	/* MS of divisor */
> -	serial_outb(cval, kgdb8250_port + UART_LCR);	/* reset DLAB */
> -	serial_outb(UART_IER_RDI, kgdb8250_port + UART_IER);	/* turn on
> interrupts */ +	/* set DLAB */
> +	serial_outb(cval | UART_LCR_DLAB, kgdb8250_port +
> +			(UART_LCR << reg_shift));
> +	/* LS */
> +	serial_outb(gdb_async_info.state->custom_divisor & 0xff,
> +			kgdb8250_port + (UART_DLL << reg_shift));
> +	/* MS  */
> +	serial_outb(gdb_async_info.state->custom_divisor >> 8,
> +			kgdb8250_port + (UART_DLM << reg_shift));
> +	/* reset DLAB */
> +	serial_outb(cval, kgdb8250_port + (UART_LCR << reg_shift));
> +	/* turn on interrupts */
> +	serial_outb(UART_IER_RDI, kgdb8250_port + (UART_IER << reg_shift));
>  	serial_outb(UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS,
> -		    kgdb8250_port + UART_MCR);
> +		    kgdb8250_port + (UART_MCR << reg_shift));
>
>  	/*
>  	 *      If we read 0xff from the LSR, there is no UART here.
>  	 */
> -	if (serial_inb(kgdb8250_port + UART_LSR) == 0xff)
> -		return -ENODEV;
> +	if (serial_inb(kgdb8250_port + (UART_LSR << reg_shift)) == 0xff)
> +		return -1;
>  	return 0;
>  }
>
> -int kgdb8250_hook(void)
> +int
> +kgdb_hook_io(void)
>  {
>  	int retval;
>
> -	/*
> -	 * Set port and irq number.
> -	 */
> -	kgdb8250_irq = rs_table[kgdb8250_ttyS].irq;
> -	switch (rs_table[kgdb8250_ttyS].iotype) {
> +	/* Setup any serial port information we may need to */
> +#ifdef CONFIG_KGDB_SIMPLE_SERIAL
> +	/* We must look in the rs_table[]. */
> +	serial_from_rs_table = 1;
> +#endif
> +	/* If the user has overriden our definitions, or if we've only
> +	 * been told the ttyS to use, look at rs_table. */
> +	if (serial_from_rs_table) {
> +		/* The user has selected one of ttyS[0-3], which we pull
> +		 * from rs_table[].  If this doesn't exist, user error. */
> +		gdb_async_info.port = gdb_async_info.state->port =
> +		    rs_table[KGDB_PORT].port;
> +		gdb_async_info.line = gdb_async_info.state->irq =
> +		    rs_table[KGDB_PORT].irq;
> +		gdb_async_info.state->io_type = rs_table[KGDB_PORT].io_type;
> +		reg_shift = rs_table[KGDB_PORT].iomem_reg_shift;
> +	}
> +
> +	switch (gdb_async_info.state->io_type) {
>  	case SERIAL_IO_MEM:
> -		kgdb8250_port = (unsigned long)rs_table[kgdb8250_ttyS].membase;
> -		serial_inb = direct_inb;
> +		kgdb8250_port = (unsigned long)
> +		    rs_table[kgdb8250_ttyS].iomem_base;
>  		serial_outb = direct_outb;
> +		serial_inb = direct_inb;
>  		break;
> +	case SERIAL_IO_PORT:
>  	default:
> -		kgdb8250_port = rs_table[kgdb8250_ttyS].line;
> -		serial_inb = io_inb;
> +		kgdb8250_port = rs_table[kgdb8250_ttyS].port;
>  		serial_outb = io_outb;
> +		serial_inb = io_inb;
>  	}
>
> +#ifndef CONFIG_KGDB_SIMPLE_SERIAL
> +	/* The user has provided the IRQ and I/O location. */
> +	kgdb8250_port = gdb_async_info.port = gdb_async_info.state->port =
> +		CONFIG_KGDB_PORT;
> +	gdb_async_info.line = gdb_async_info.state->irq = CONFIG_KGDB_IRQ;
> +#endif
> +
>  #ifdef CONFIG_SERIAL_8250
> -	if ((retval = serial8250_release_irq(kgdb8250_irq)) < 0)
> -		return retval;
> +	if (serial8250_release_irq(gdb_async_info.line))
> +		return -1;
>  #endif
>
> -	if ((retval = kgdb8250_init(kgdb8250_ttyS, kgdb8250_baud)) < 0)
> -		return retval;
> +	if (kgdb8250_init() == -1)
> +		return -1;
>
> -	retval = request_irq(kgdb8250_irq, kgdb8250_interrupt, SA_INTERRUPT,
> -			     "GDB-stub", NULL);
> -	return retval;
> -}
> +	retval = request_irq(gdb_async_info.line, kgdb8250_interrupt,
> +			     SA_INTERRUPT, "GDB-stub", NULL);
> +	if (retval == 0)
> +		initialized = 1;
> +	else
> +		initialized = 0;
>
> -void kgdb8250_add_port(int i, struct uart_port *serial_req)
> -{
> -	rs_table[i].iotype = serial_req->iotype;
> -	rs_table[i].line = serial_req->line;
> -	rs_table[i].membase = serial_req->membase;
> -	rs_table[i].regshift = serial_req->regshift;
> +	return 0;
>  }
>
>  struct kgdb_serial kgdb8250_serial_driver = {
> -	.read_char = kgdb8250_read_char,
> -	.write_char = kgdb8250_write_char,
> -	.hook = kgdb8250_hook
> +	.read_char = kgdb_get_debug_char,
> +	.write_char = kgdb_put_debug_char,
> +	.hook = kgdb_hook_io,
>  };
>
> +void
> +kgdb8250_add_port(int i, struct uart_port *serial_req)
> +{
> +	rs_table[i].io_type = serial_req->iotype;
> +	rs_table[i].port = serial_req->line;
> +	rs_table[i].irq = serial_req->irq;
> +	rs_table[i].iomem_base = serial_req->membase;
> +	rs_table[i].iomem_reg_shift = serial_req->regshift;
> +
> +	/* We will want to look in the rs_table now. */
> +	serial_from_rs_table = 1;
> +}
> +
>  /*
>   * Syntax for this cmdline option is
>   * kgdb8250=ttyno,baudrate
>   */
>
> -static int __init kgdb8250_opt(char *str)
> +static int __init
> +kgdb8250_opt(char *str)
>  {
>  	if (*str < '0' || *str > '3')
>  		goto errout;
> @@ -382,7 +492,12 @@
>  	    kgdb8250_baud != 38400 && kgdb8250_baud != 57600 &&
>  	    kgdb8250_baud != 115200)
>  		goto errout;
> +
> +	/* Make the baud rate change happen. */
> +	gdb_async_info.state->custom_divisor = BASE_BAUD / kgdb8250_baud;
> +
>  	kgdb_serial = &kgdb8250_serial_driver;
> +
>  	return 1;
>
>        errout:
> diff -Nru a/include/linux/kgdb.h b/include/linux/kgdb.h
> --- a/include/linux/kgdb.h	Wed Feb 25 14:21:26 2004
> +++ b/include/linux/kgdb.h	Wed Feb 25 14:21:26 2004
> @@ -25,6 +25,8 @@
>  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;
>
>  enum kgdb_bptype {
> diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c
> --- a/kernel/kgdb.c	Wed Feb 25 14:21:26 2004
> +++ b/kernel/kgdb.c	Wed Feb 25 14:21:26 2004
> @@ -63,7 +63,7 @@
>   * has connected to kgdb.
>   */
>  int kgdb_initialized = 0;
> -static volatile int kgdb_connected;
> +volatile int kgdb_connected;
>
>  /* If non-zero, wait for a gdb connection when kgdb_entry is called */
>  int kgdb_enter = 0;
> @@ -214,6 +214,7 @@
>  	do {
>  		/* wait around for the start character, ignore all other characters */
>  		while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$') ;
> +		kgdb_connected = 1;
>  		checksum = 0;
>  		xmitcsum = -1;

Re: [Kgdb-bugreport] [PATCH][1/3] Update CVS KGDB's serial driver

[Tom Rini]

On Thu, Feb 26, 2004 at 01:55:18PM +0530, Amit S. Kale wrote:
> On Thursday 26 Feb 2004 3:06 am, Tom Rini wrote:
> > The following updates the serial driver with the fixes / cleanups that
> > are in George's version of the driver.  There's a few slightly 'odd'
> > things in this patch, which stem from the fact that in my next round of
> > patches there will (a) be kernel/Kconfig.kgdb and (b) 1 kgdb i/o driver
> > at a time.
> 
> Please send them in consecutive emails. Having separate patches is fine.
> I can't figure out from this patch how a user is going to configure kgdb 8250 
> options from menuconfig.

I'll do the config bits one today.  But 'hex' is a valid word in
menuconfig :)

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