Re: [Kgdb-bugreport] [KGDB PATCH][2/7] Serial updates, take 2

[George Anzinger <george@mvista.com>]

Couple of comments below...

Tom Rini wrote:
> The following is my second attempt at making the serial driver more robust.
> This has the following changes:
> - We don't need kgdb_might_be_resumed or kgdb_killed_or_detached.
> - Don't try and look for a connection in put_packet, after we've tried
>   to put a packet.  Instead, when we receive a packet, GDB has
>   connected.  Also make put_packet look at the char it reads, and handle
>   a new packet in the middle (or ^C+packet).
> - Remove ok_packet(), excessive, IMHO.
> - In kgdb8250_interrupt, only check if this is the right line, and if so,
>   call kgdb_schedule_breakpoint().  This ends up killing off a lot of
>   (incomplete due to my not testing SMP) locking.
> - Remove a duplicate extern of kgdb_connected.
> 
> diff -zrupN linux-2.6.3+nothing/arch/ppc/8260_io/uart.c linux-2.6.3+config+serial/arch/ppc/8260_io/uart.c
> --- linux-2.6.3+nothing/arch/ppc/8260_io/uart.c	2004-02-27 12:16:14.397164756 -0700
> +++ linux-2.6.3+config+serial/arch/ppc/8260_io/uart.c	2004-02-27 12:16:13.765307745 -0700
> @@ -396,16 +396,10 @@ static _INLINE_ void receive_chars(ser_i
>  #ifdef CONFIG_KGDB
>  		if (info->state->smc_scc_num == KGDB_SER_IDX) {
>  			while (i-- > 0) {
> -				if (*cp == 0x03) {
> +				if (*cp == 0x03 || *cp == '$') {
>  					breakpoint();
>  					return;
>  				}
> -				if (*cp == '$') {
> -					atomic_set(&kgdb_might_be_resumed, 1);
> -					breakpoint();
> -					atomic_set(&kgdb_might_be_resumed, 0);
> -					return;
> -				}
>  				cp++;
>  			}
>  			bdp->cbd_sc |= BD_SC_EMPTY;
> diff -zrupN linux-2.6.3+nothing/drivers/serial/kgdb_8250.c linux-2.6.3+config+serial/drivers/serial/kgdb_8250.c
> --- linux-2.6.3+nothing/drivers/serial/kgdb_8250.c	2004-02-27 12:16:14.557128556 -0700
> +++ linux-2.6.3+config+serial/drivers/serial/kgdb_8250.c	2004-02-27 12:16:13.000000000 -0700
> @@ -17,7 +17,6 @@
>  #include <linux/config.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> -#include <linux/spinlock.h>
>  #include <linux/kgdb.h>
>  #include <linux/interrupt.h>
>  #include <linux/tty.h>
> @@ -166,32 +165,15 @@ read_data_bfr(void)
>  /*
>   * Get a char if available, return -1 if nothing available.
>   * Empty the receive buffer first, then look at the interface hardware.
> - *
> - * Locking here is a bit of a problem.	We MUST not lock out communication
> - * if we are trying to talk to gdb about a kgdb entry.	ON the other hand
> - * we can loose chars in the console pass thru if we don't lock.  It is also
> - * possible that we could hold the lock or be waiting for it when kgdb
> - * NEEDS to talk.  Since kgdb locks down the world, it does not need locks.
> - * We do, of course have possible issues with interrupting a uart operation,
> - * but we will just depend on the uart status to help keep that straight.
>   */
>  
> -static spinlock_t uart_interrupt_lock = SPIN_LOCK_UNLOCKED;
> -#ifdef CONFIG_SMP
> -extern spinlock_t kgdb_spinlock;
> -#endif
> -
>  int
>  kgdb_get_debug_char(void)
>  {
>  	int retchr;
>  	unsigned long flags;
>  	local_irq_save(flags);
> -#ifdef CONFIG_SMP
> -	if (!spin_is_locked(&kgdb_spinlock)) {
> -		spin_lock(&uart_interrupt_lock);
> -	}
> -#endif
> +
>  	/* intr routine has q'd chars */
>  	if (atomic_read(&kgdb8250_buf_in_cnt) != 0) {
>  		retchr = kgdb8250_buf[kgdb8250_buf_out_inx++];
> @@ -205,11 +187,6 @@ kgdb_get_debug_char(void)
>  	} while (retchr < 0);
>  
>  out:
> -#ifdef CONFIG_SMP
> -	if (!spin_is_locked(&kgdb_spinlock)) {
> -		spin_unlock(&uart_interrupt_lock);
> -	}
> -#endif
>  	local_irq_restore(flags);
>  
>  	return retchr;
> @@ -217,72 +194,17 @@ out:
>  
>  /*
>   * This is the receiver interrupt routine for the GDB stub.
> - * It will receive a limited number of characters of input
> - * from the gdb  host machine and save them up in a buffer.
> - *
> - * When kgdb_get_debug_char() is called it
> - * draws characters out of the buffer until it is empty and
> - * then reads directly from the serial port.
> - *
> - * We do not attempt to write chars from the interrupt routine
> - * since the stubs do all of that via kgdb_put_debug_char() which
> - * writes one byte after waiting for the interface to become
> - * ready.
> - *
> - * The debug stubs like to run with interrupts disabled since,
> - * after all, they run as a consequence of a breakpoint in
> - * the kernel.
> - *
> - * Perhaps someone who knows more about the tty driver than I
> - * care to learn can make this work for any low level serial
> - * driver.
> + * All that we need to do is verify that the interrupt happened on the
> + * line we're in charge of.  If this is true, schedule a breakpoint and
> + * return.
>   */
>  static irqreturn_t
>  kgdb8250_interrupt(int irq, void *dev_id, struct pt_regs *regs)
>  {
> -	int chr, iir;
> -	unsigned long flags;
> -
>  	if (irq != gdb_async_info.line)
>  		return IRQ_NONE;
>  
> -	/* If we get an interrupt, then KGDB is trying to connect. */
> -	if (!kgdb_connected) {
> -		kgdb_schedule_breakpoint();
> -		return IRQ_HANDLED;
> -	}
> -
> -	local_irq_save(flags);
> -	spin_lock(&uart_interrupt_lock);
> -
> -	do {
> -		chr = read_data_bfr();
> -		iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
> -		if (chr < 0)
> -			continue;
> -
> -		/* Check whether gdb sent interrupt */
> -		if (chr == 3) {
> -			breakpoint();
> -			continue;
> -		}
> -
> -		if (atomic_read(&kgdb8250_buf_in_cnt) >= GDB_BUF_SIZE) {
> -			/* buffer overflow, clear it */
> -			kgdb8250_buf_in_inx = 0;
> -			atomic_set(&kgdb8250_buf_in_cnt, 0);
> -			kgdb8250_buf_out_inx = 0;
> -			break;
> -		}
> -
> -		kgdb8250_buf[kgdb8250_buf_in_inx++] = chr;
> -		kgdb8250_buf_in_inx &= (GDB_BUF_SIZE - 1);
> -		atomic_inc(&kgdb8250_buf_in_cnt);
> -	} while (iir & UART_IIR_RDI);
> -
> -	spin_unlock(&uart_interrupt_lock);
> -	local_irq_restore(flags);
> -
I think you need at least this (especially in SMP, but works in all):
	char iir = serial_inb(kgdb8250_port + (UART_IIR << reg_shift));
         if(iir & UART_IIR_RDI){
> +		kgdb_schedule_breakpoint();
	}
>  	return IRQ_HANDLED;
>  }
>  
> diff -zrupN linux-2.6.3+nothing/include/linux/kgdb.h linux-2.6.3+config+serial/include/linux/kgdb.h
> --- linux-2.6.3+nothing/include/linux/kgdb.h	2004-02-27 12:16:14.000000000 -0700
> +++ linux-2.6.3+config+serial/include/linux/kgdb.h	2004-02-27 12:16:13.000000000 -0700
> @@ -36,10 +36,6 @@ extern volatile int kgdb_connected;
>  #endif
>  
>  extern atomic_t kgdb_setting_breakpoint;
> -extern atomic_t kgdb_killed_or_detached;
> -extern atomic_t kgdb_might_be_resumed;
> -
> -extern volatile int kgdb_connected;
>  
>  extern struct task_struct *kgdb_usethread, *kgdb_contthread;
>  
> diff -zrupN linux-2.6.3+nothing/kernel/kgdb.c linux-2.6.3+config+serial/kernel/kgdb.c
> --- linux-2.6.3+nothing/kernel/kgdb.c	2004-02-27 12:16:14.000000000 -0700
> +++ linux-2.6.3+config+serial/kernel/kgdb.c	2004-02-27 12:16:13.000000000 -0700
> @@ -15,6 +15,7 @@
>   * Copyright (C) 2002-2004 Timesys Corporation
>   * Copyright (C) 2003-2004 Amit S. Kale
>   * Copyright (C) 2004 Pavel Machek <pavel@suse.cz>
> + * Copyright (C) 2004 Tom Rini <trini@kernel.crashing.org>
>   *
>   * Restructured KGDB for 2.6 kernels.
>   * thread support, support for multiple processors,support for ia-32(x86) 
> @@ -87,8 +88,6 @@ static const char hexchars[] = "01234567
>  static spinlock_t slavecpulocks[KGDB_MAX_NO_CPUS];
>  static volatile int procindebug[KGDB_MAX_NO_CPUS];
>  atomic_t kgdb_setting_breakpoint;
> -atomic_t kgdb_killed_or_detached;
> -atomic_t kgdb_might_be_resumed;
>  struct task_struct *kgdb_usethread, *kgdb_contthread;
>  
>  int debugger_step;
> @@ -212,8 +211,10 @@ static void get_packet(char *buffer)
>  	char ch;
>  
>  	do {
> -		/* wait around for the start character, ignore all other characters */
> -		while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$') ;
> +		/* wait around for the start character, ignore all other
> +		 * characters */
> +		while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$')
> +			;	/* Spin. */
>  		kgdb_connected = 1;
>  		checksum = 0;
>  		xmitcsum = -1;
> @@ -249,27 +250,22 @@ static void get_packet(char *buffer)
>   * Send the packet in buffer.
>   * Check for gdb connection if asked for.
>   */
> -static void put_packet(char *buffer, int checkconnect)
> +static void put_packet(char *buffer)
>  {
>  	unsigned char checksum;
>  	int count;
>  	char ch;
> -	static char gdbseq[] = "$Hc-1#09";
> -	int i;
> -	int send_count;
>  
>  	/*  $<packet info>#<checksum>. */
> -	do {
> +	while (1) {
>  		kgdb_serial->write_char('$');
>  		checksum = 0;
>  		count = 0;
> -		send_count = 0;
>  
>  		while ((ch = buffer[count])) {
>  			kgdb_serial->write_char(ch);
>  			checksum += ch;
>  			count++;
> -			send_count++;
>  		}
>  
>  		kgdb_serial->write_char('#');
> @@ -278,30 +274,27 @@ static void put_packet(char *buffer, int
>  		if (kgdb_serial->flush)
>  			kgdb_serial->flush();
>  
> -		i = 0;
> -		while ((ch = kgdb_serial->read_char()) == gdbseq[i++] &&
> -		       checkconnect) {
> -			if (!gdbseq[i]) {
> -				kgdb_serial->write_char('+');
> -				if (kgdb_serial->flush)
> -					kgdb_serial->flush();
> -				breakpoint();
> -
> -				/*
> -				 * GDB is available now.
> -				 * Retransmit this packet.
> -				 */
> -				break;
> -			}
> -		}
> -		if (checkconnect && ch == 3) {
> -			kgdb_serial->write_char('+');
> +		/* Now see what we get in reply. */
> +		ch = kgdb_serial->read_char();
> +
> +		if (ch == 3)
> +			ch = kgdb_serial->read_char();
> +
> +		/* If we get an ACK, we are done. */
> +		if (ch == '+')
> +			return;
> +
> +		/* If we get the start of another packet, this means
> +		 * that GDB is attempting to reconnect.  We will NAK
> +		 * the packet being sent, and stop trying to send this
> +		 * packet. */
> +		if (ch == '$') {
> +			kgdb_serial->write_char('-');
>  			if (kgdb_serial->flush)
>  				kgdb_serial->flush();
> -			breakpoint();

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

> +			return;
>  		}
> -	} while ((ch & 0x7f) != '+');
> -
> +	}
>  }
>  
>  static int get_char(char *addr, unsigned char *data)
> @@ -427,11 +420,6 @@ static inline void error_packet(char *pk
>  	pkt[3] = '\0';
>  }
>  
> -static void ok_packet(char *pkt)
> -{
> -	strcpy(pkt, "OK");
> -}
> -
>  static char *pack_threadid(char *pkt, threadref * id)
>  {
>  	char *limit;
> @@ -502,7 +490,8 @@ void kgdb_wait(struct pt_regs *regs)
>  	procindebug[processor] = 1;
>  	current->thread.debuggerinfo = regs;
>  
> -	/* Wait till master processor goes completely into the debugger. FIXME: this looks racy */
> +	/* Wait till master processor goes completely into the debugger.
> +	 * FIXME: this looks racy */
>  	while (!procindebug[atomic_read(&debugger_active) - 1]) {
>  		int i = 10;	/* an arbitrary number */
>  
> @@ -701,17 +690,7 @@ int kgdb_handle_exception(int exVector, 
>  	/* Master processor is completely in the debugger */
>  	kgdb_post_master_code(linux_regs, exVector, err_code);
>  
> -	if (atomic_read(&kgdb_killed_or_detached) &&
> -	    atomic_read(&kgdb_might_be_resumed)) {
> -		get_packet(remcom_in_buffer);
> -		if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 'c') {
> -			remove_all_break();
> -			atomic_set(&kgdb_killed_or_detached, 0);
> -			ok_packet(remcom_out_buffer);
> -		} else
> -			return 1;
> -	} else {
> -
> +	if (kgdb_connected) {
>  		/* reply to host that an exception has occurred */
>  		ptr = remcom_out_buffer;
>  		*ptr++ = 'T';
> @@ -721,11 +700,9 @@ int kgdb_handle_exception(int exVector, 
>  		int_to_threadref(&thref, shadow_pid(current->pid));
>  		ptr = pack_threadid(ptr, &thref);
>  		*ptr++ = ';';
> -		*ptr = '\0';
> -	}
>  
> -	put_packet(remcom_out_buffer, 0);
> -	kgdb_connected = 1;
> +		put_packet(remcom_out_buffer);
> +	}
>  
>  	kgdb_usethread = current;
>  	kgdb_usethreadid = shadow_pid(current->pid);
> @@ -741,6 +718,11 @@ int kgdb_handle_exception(int exVector, 
>  
>  		switch (remcom_in_buffer[0]) {
>  		case '?':
> +			/* We know that this packet is only sent
> +			 * during initial connect.  So to be safe,
> +			 * we clear out our breakpoints now incase
> +			 * GDB is reconnecting. */
> +			remove_all_break();
>  			remcom_out_buffer[0] = 'S';
>  			remcom_out_buffer[1] = hexchars[signo >> 4];
>  			remcom_out_buffer[2] = hexchars[signo % 16];
> @@ -798,7 +780,7 @@ int kgdb_handle_exception(int exVector, 
>  			else {
>  				gdb_regs_to_regs(gdb_regs, (struct pt_regs *)
>  						 current->thread.debuggerinfo);
> -				ok_packet(remcom_out_buffer);
> +				strcpy(remcom_out_buffer, "OK");
>  			}
>  
>  			break;
> @@ -838,10 +820,10 @@ int kgdb_handle_exception(int exVector, 
>  			if ((error = remove_all_break()) < 0) {
>  				error_packet(remcom_out_buffer, error);
>  			} else {
> -				ok_packet(remcom_out_buffer);
> +				strcpy(remcom_out_buffer, "OK");
>  				kgdb_connected = 0;
>  			}
> -			put_packet(remcom_out_buffer, 0);
> +			put_packet(remcom_out_buffer);
>  			goto default_handle;
>  
>  		case 'k':
> @@ -947,11 +929,10 @@ int kgdb_handle_exception(int exVector, 
>  				}
>  				kgdb_usethread = thread;
>  				kgdb_usethreadid = threadid;
> -				ok_packet(remcom_out_buffer);
> +				strcpy(remcom_out_buffer, "OK");
>  				break;
>  
>  			case 'c':
> -				atomic_set(&kgdb_killed_or_detached, 0);
>  				ptr = &remcom_in_buffer[2];
>  				kgdb_hex2long(&ptr, &threadid);
>  				if (!threadid) {
> @@ -966,7 +947,7 @@ int kgdb_handle_exception(int exVector, 
>  					}
>  					kgdb_contthread = thread;
>  				}
> -				ok_packet(remcom_out_buffer);
> +				strcpy(remcom_out_buffer, "OK");
>  				break;
>  			}
>  			break;
> @@ -977,7 +958,7 @@ int kgdb_handle_exception(int exVector, 
>  			kgdb_hex2long(&ptr, &threadid);
>  			thread = getthread(linux_regs, threadid);
>  			if (thread)
> -				ok_packet(remcom_out_buffer);
> +				strcpy(remcom_out_buffer, "OK");
>  			else
>  				error_packet(remcom_out_buffer, -EINVAL);
>  			break;
> @@ -1018,7 +999,7 @@ int kgdb_handle_exception(int exVector, 
>  			}
>  
>  			if (error == 0)
> -				ok_packet(remcom_out_buffer);
> +				strcpy(remcom_out_buffer, "OK");
>  			else
>  				error_packet(remcom_out_buffer, error);
>  
> @@ -1039,7 +1020,7 @@ int kgdb_handle_exception(int exVector, 
>  		}		/* switch */
>  
>  		/* reply to the request */
> -		put_packet(remcom_out_buffer, 0);
> +		put_packet(remcom_out_buffer);
>  	}
>  
>        kgdb_exit:
> @@ -1063,7 +1044,6 @@ int kgdb_handle_exception(int exVector, 
>  	}
>  
>  	/* Free debugger_active */
> -	atomic_set(&kgdb_killed_or_detached, 1);
>  	atomic_set(&debugger_active, 0);
>  	local_irq_restore(flags);
>  
> @@ -1132,12 +1112,6 @@ void kgdb_entry(void)
>  	/* Free debugger_active */
>  	atomic_set(&debugger_active, 0);
>  
> -	/* This flag is used, if gdb has detached and wants to start
> -	 * another session
> -	 */
> -	atomic_set(&kgdb_killed_or_detached, 1);
> -	atomic_set(&kgdb_might_be_resumed, 0);
> -
>  	for (i = 0; i < MAX_BREAKPOINTS; i++)
>  		kgdb_break[i].state = bp_disabled;
>  
> @@ -1222,7 +1196,7 @@ void kgdb_console_write(struct console *
>  		*bufptr = '\0';
>  		s += wcount;
>  
> -		put_packet(kgdbconbuf, 1);
> +		put_packet(kgdbconbuf);
>  
>  	}
>  	local_irq_restore(flags);
> 

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