From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S263186AbUB0WsJ (ORCPT ); Fri, 27 Feb 2004 17:48:09 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S263184AbUB0WrG (ORCPT ); Fri, 27 Feb 2004 17:47:06 -0500 Received: from gateway-1237.mvista.com ([12.44.186.158]:3069 "EHLO av.mvista.com") by vger.kernel.org with ESMTP id S263187AbUB0Woq (ORCPT ); Fri, 27 Feb 2004 17:44:46 -0500 Message-ID: <403FC851.70103@mvista.com> Date: Fri, 27 Feb 2004 14:44:33 -0800 From: George Anzinger Organization: MontaVista Software User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20030225 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Tom Rini CC: Kernel Mailing List , Pavel Machek , amit@gate.crashing.org, kgdb-bugreport@lists.sourceforge.net Subject: Re: [Kgdb-bugreport] [KGDB PATCH][2/7] Serial updates, take 2 References: <20040227212301.GC1052@smtp.west.cox.net> <20040227212548.GD1052@smtp.west.cox.net> In-Reply-To: <20040227212548.GD1052@smtp.west.cox.net> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org 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 > #include > #include > -#include > #include > #include > #include > @@ -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 > + * Copyright (C) 2004 Tom Rini > * > * 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; > > /* $#. */ > - 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