From: George Anzinger <george@mvista.com>
To: "Amit S. Kale" <amitkale@emsyssoft.com>
Cc: Tom Rini <trini@kernel.crashing.org>,
kernel list <linux-kernel@vger.kernel.org>,
Pavel Machek <pavel@suse.cz>,
kgdb-bugreport@lists.sourceforge.net
Subject: Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach
Date: Thu, 26 Feb 2004 15:30:08 -0800 [thread overview]
Message-ID: <403E8180.1060008@mvista.com> (raw)
In-Reply-To: <200402261344.49261.amitkale@emsyssoft.com>
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
next prev parent reply other threads:[~2004-02-26 23:33 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2004-02-25 21:36 [PATCH][1/3] Update CVS KGDB's serial driver Tom Rini
2004-02-25 21:43 ` [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint Tom Rini
2004-02-25 21:53 ` [PATCH][3/3] Update CVS KGDB's wrt connect / detach Tom Rini
2004-02-26 8:14 ` [Kgdb-bugreport] " Amit S. Kale
2004-02-26 14:41 ` Tom Rini
2004-02-26 16:05 ` Daniel Jacobowitz
2004-02-26 17:44 ` Tom Rini
2004-02-26 23:32 ` Daniel Jacobowitz
2004-03-01 8:12 ` Amit S. Kale
2004-02-26 21:45 ` Pavel Machek
2004-03-01 8:29 ` Amit S. Kale
2004-02-26 18:08 ` Tom Rini
2004-03-01 8:15 ` Amit S. Kale
2004-02-26 23:30 ` George Anzinger [this message]
2004-02-26 23:59 ` Tom Rini
2004-02-27 1:57 ` George Anzinger
2004-02-27 15:49 ` Tom Rini
2004-02-27 22:11 ` George Anzinger
2004-02-27 22:50 ` Tom Rini
2004-03-01 10:18 ` Amit S. Kale
2004-03-01 10:17 ` Amit S. Kale
2004-02-27 17:13 ` Tom Rini
2004-02-27 21:55 ` George Anzinger
2004-03-01 8:36 ` Amit S. Kale
2004-03-01 16:31 ` Tom Rini
2004-02-25 22:59 ` [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint George Anzinger
2004-02-25 23:04 ` Tom Rini
2004-02-25 23:23 ` George Anzinger
2004-02-26 7:30 ` Amit S. Kale
2004-02-26 23:19 ` George Anzinger
2004-03-01 8:32 ` Amit S. Kale
2004-03-02 21:19 ` George Anzinger
2004-03-03 5:13 ` Amit S. Kale
2004-03-04 0:20 ` George Anzinger
2004-03-04 4:58 ` Amit S. Kale
2004-03-11 21:28 ` George Anzinger
2004-03-12 4:44 ` Amit S. Kale
2004-03-12 8:03 ` George Anzinger
2004-03-15 11:20 ` Amit S. Kale
2004-03-15 19:52 ` George Anzinger
2004-03-16 4:30 ` Amit S. Kale
2004-03-16 13:02 ` La Monte H.P. Yarroll
2004-03-16 15:04 ` Amit S. Kale
2004-02-25 23:10 ` [Kgdb-bugreport] [PATCH][1/3] Update CVS KGDB's serial driver George Anzinger
2004-02-25 23:18 ` Tom Rini
2004-02-26 8:25 ` Amit S. Kale
2004-02-26 14:43 ` Tom Rini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=403E8180.1060008@mvista.com \
--to=george@mvista.com \
--cc=amitkale@emsyssoft.com \
--cc=kgdb-bugreport@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
--cc=pavel@suse.cz \
--cc=trini@kernel.crashing.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox