Hi Tom,
Thank you for your time at looking at my proposed enhancement.
I've attached a kgbd.c.udiff file containing the output of diff -u.
Well, you are right about the protocol specification, unfortunately the
existing get_packet() fails to implement the correct protocol error
handling !
There are many scenarios where the existing get_packet() fails and will
not produce a NAK or ACK at the correct time. This causes gdb to
retransmit multiple times trying to get the get_packet() code to
respond. This can be improved by using proper protocol decoding :)
The protocol is designed to have unique $ and # markers. Therefore, $
always indicates the start of a new packet and should be trapped. The
existing code can fail to spot a new packet if the previous packet was
bad. eg. # character corrupt or missing. This causes the next "good"
packet to be incorrectly decoded as the code is stuck in a loop waiting
for a # of the previous packet.
For me, state machines are ideal for decoding this layer 2 protocol and
because it uses a single call to the read function. In other words the
flow of the code is dependent on the "meaning" of the received data and
the current state rather than a fixed sequential flow.
In my opinion, the existing get_packet() only works when the comms are
100% good. In which case why bother with the checksum ?
I have many years of experience of layer 2 protocols on embedded systems
and I was horrified to see the existing get_packet() implementation and
I therefore posted a better solution for you to consider.
I understand that my enhancement is crucial to the performance of the
protocol link and I welcome feedback. However, I think the existing
get_packet() code is broken and needs fixing :)
It is quite easy to see that existing get_packet() is broken, for
example using minicom try to send the following "connect" sequence to
kgdb
$?#3$?#3f
The last checksum digit of the first packet is missing but results in no
ACK (+) being produced for the good packet $?#3f. Whereas my solution
detects the $ of the second packet and aborts the decode of the first
packet resulting in ACK being produced. This scenario could happen if
the cable is disconnected whilst gdb is trying to connect and then
sometime later ( could be 6 months later, there is no timeout ) the
cable is reconnected.
Thanks for your interest in my solution.
Regards,
Dean.
On Tue, 2006-10-17 at 09:52 -0700, Tom Rini wrote:
> On Tue, Oct 10, 2006 at 01:35:42PM +0100, Dean Jenkins wrote:
>
> > Hi,
> >
> > I am a newbie to this process so please tell me if I need to make
> > changes to this message.
> >
> > I wish to submit an enhancement to replace the original get_packet()
> > function in kgdb.c. Please give me some feedback.
>
> Can you please resend this as a Unified Diff (diff -u) ? Thanks!
>
> But generally, I'm not sure about this change. The protocol itself is
> defined in a way that if we get a bad or garbage packet, we just NAK and
> the other side (or us, if we see the NAK) resends.
>
--- kgdb.c.orig 2006-10-09 16:33:39.000000000 +0100
+++ kgdb.c 2006-10-10 13:26:22.000000000 +0100
@@ -24,6 +24,11 @@
* This file is licensed under the terms of the GNU General Public License
* version 2. This program is licensed "as is" without any warranty of any
* kind, whether express or implied.
+ *
+ * Modified by Dean Jenkins
+ * Copyright (C) 2006 Liberte Software Ltd. <[EMAIL PROTECTED]>
+ * get_packet() made more robust to avoid loss of the next "good" received
packet.
+ *
*/
#include <linux/string.h>
@@ -338,50 +343,136 @@
return (-1);
}
+typedef enum {
+ WAIT_DOLLAR,
+ GOT_DOLLAR,
+ GOT_HASH,
+ GOT_CHK1,
+ GOOD_PACKET
+} rx_state;
+
/* scan for the sequence $<data>#<checksum> */
static void get_packet(char *buffer)
{
- unsigned char checksum;
- unsigned char xmitcsum;
- int count;
+ unsigned char checksum = 0;
+ unsigned char xmitcsum = 0;
+ int count = 0, hexvalue;
char ch;
- if (!kgdb_io_ops.read_char)
- return;
- do {
- /* Spin and wait around for the start character, ignore all
- * other characters */
- while ((ch = (kgdb_io_ops.read_char())) != '$') ;
- kgdb_connected = 1;
- checksum = 0;
- xmitcsum = -1;
+ rx_state state = WAIT_DOLLAR;
- count = 0;
+ while( state != GOOD_PACKET ) {
- /* now, read until a # or end of buffer is found */
- while (count < (BUFMAX - 1)) {
- ch = kgdb_io_ops.read_char();
- if (ch == '#')
- break;
- checksum = checksum + ch;
- buffer[count] = ch;
- count = count + 1;
+ /* read a character ( uses blocking ) */
+ ch = kgdb_io_ops.read_char();
+
+ if( ch == '$' ) {
+ /* start of new packet */
+ /* abandon any old packet processing */
+ /* as incomplete packet received */
+
+ checksum = 0;
+ count = 0;
+
+ state = GOT_DOLLAR;
}
- buffer[count] = 0;
+ else {
+ switch (state) {
- if (ch == '#') {
- xmitcsum = hex(kgdb_io_ops.read_char()) << 4;
- xmitcsum += hex(kgdb_io_ops.read_char());
-
- if (checksum != xmitcsum)
- /* failed checksum */
- kgdb_io_ops.write_char('-');
- else
- /* successful transfer */
- kgdb_io_ops.write_char('+');
- if (kgdb_io_ops.flush)
- kgdb_io_ops.flush();
+ case WAIT_DOLLAR:
+ /* continue to wait for the next $ */
+ break;
+
+ case GOT_DOLLAR:
+ /* receive the command */
+
+ /* terminator found ? */
+ if (ch == '#') {
+ buffer[count] = 0;
+ state = GOT_HASH;
+ }
+ else if (count < (BUFMAX - 1)) {
+ checksum = checksum + ch;
+ buffer[count] = ch;
+ count = count + 1;
+ }
+ else {
+ /* command is TOO long so abort
*/
+ kgdb_io_ops.write_char('-');
+
+ if (kgdb_io_ops.flush) {
+ kgdb_io_ops.flush();
+ }
+
+ state = WAIT_DOLLAR;
+ }
+ break;
+
+ case GOT_HASH:
+ /* look at first checksum char */
+ hexvalue = hex(ch);
+
+ if( hexvalue == -1 ) {
+ /* invalid hex char received so
abort */
+ kgdb_io_ops.write_char('-');
+
+ if (kgdb_io_ops.flush) {
+ kgdb_io_ops.flush();
+ }
+
+ state = WAIT_DOLLAR;
+ }
+ else {
+ xmitcsum = hexvalue << 4;
+ state = GOT_CHK1;
+ }
+ break;
+
+ case GOT_CHK1:
+ /* look at second checksum char */
+ hexvalue = hex(ch);
+
+ if( hexvalue == -1 ) {
+ /* invalid hex char received so
abort */
+ kgdb_io_ops.write_char('-');
+
+ if (kgdb_io_ops.flush) {
+ kgdb_io_ops.flush();
+ }
+
+ state = WAIT_DOLLAR;
+ }
+ else {
+ xmitcsum += hexvalue ;
+
+ /* end of packet so now
validate the checksum */
+ if (checksum != xmitcsum) {
+ /* failed checksum */
+
kgdb_io_ops.write_char('-');
+ /* bad packet so look
for new packet */
+ state = WAIT_DOLLAR;
+ }
+ else {
+ /* successful transfer
*/
+
kgdb_io_ops.write_char('+');
+ /* exit as got a good
packet */
+ state = GOOD_PACKET;
+ }
+
+ if (kgdb_io_ops.flush) {
+ kgdb_io_ops.flush();
+ }
+ }
+ break;
+
+ default:
+ /* should never get here */
+ state = WAIT_DOLLAR;
+ break;
+ }
}
- } while (checksum != xmitcsum);
+ }
+
+ kgdb_connected = 1;
}
/*
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
Kgdb-bugreport mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
