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

["Amit S. Kale" <amitkale@emsyssoft.com>]

This patch broke Ctrl+C response on my system. Fix as follows
With this fix I can kill or detach gdb with pending console messages and then 
connect a new gdb.

Is following fix ok to checkin? (interdiff)

@@ -412,11 +418,11 @@
 +              /* 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;
++                  rs_table[kgdb8250_ttyS].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;
++                  rs_table[kgdb8250_ttyS].irq;
++              gdb_async_info.state->io_type = 
rs_table[kgdb8250_ttyS].io_type;
++              reg_shift = rs_table[kgdb8250_ttyS].iomem_reg_shift;
 +      }
 +
 +      switch (gdb_async_info.state->io_type) {



On Monday 01 Mar 2004 8:58 pm, Tom Rini wrote:
> On Fri, Feb 27, 2004 at 03:53:12PM -0800, George Anzinger wrote:
> > Tom Rini wrote:
> > >On Fri, Feb 27, 2004 at 02:44:33PM -0800, George Anzinger wrote:
> > >>Couple of comments below...
> > >>
> > >>Tom Rini wrote:
> > >
> > >[snip]
> > >
> > >>>-	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;
> > >
> > >Would this be to ensure that we only schedule one breakpoint not 2?
> >
> > Well, that too, but the notion is to take care of an interrupt on cpu 1
> > while doing console output on cpu 0.  If cpu 0 doesn't grab the '+' fast
> > enough to prevent the interrupt cpu 1 may get an interrupt with no
> > characters in the fifo. This code just ignores that.
>
> OK, done.
>
> > >[snip]
> > >
> > >>>+		/* 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.
> > >
> > >There's two cases here, IMHO.  If GDB is connected, the only time we'll
> > >get a '$' when we're sending a packet is if we're out-of-sync (in
> > >regular gdb/kgdb communication) or we're prempting a console message
> > >(i.e. we're trying to send something while gdb wants to break in).
> >
> > I am a little unclear about this.  Assuming that the user has not been so
> > dumb as to put a breakpoint in kgdb's console handling code, I suspect
> > that the entry code should allow the console message to complete prior to
> > sending the first message to gdb.
>
> I don't think so.  If you don't break out of put_packet(), kgdb will
> try to send the packet (console or for a previous, now dead, session)
> forever.  And gdb will keep trying to send it's first packet, timing out
> and moving on.
>
> > I made a rather lame attempt to do this
> > in the -mm kgdb.  What I think should happen is that, on entry kgdb
> > should send an nmi to all but self. The kgdb nmi code (at in_kgdb() in
> > the -mm patch) should check to see if the CURRENT cpu is in the kgdb
> > console code and, if so, set a flag for the console code that a kgdb
> > entry is pending and then return.  The console code should check this
> > flag on exit, after clearing the "i am in console" flag and if set, do a
> > send nmi self.  This would allow the console code to complete prior to
> > the kgdb entry while still rounding up all the other cpus with the nmi.
>
> IMHO, that's awfully complex for something we can just not skip out on.
>
> > >If things get out-of-sync in normal gdb/kgdb
> > >communication, what will happen w/o this change is that we get stuck in
> > >a "Packet instead of ACK" loop on gdb, and we get stuck trying to
> > >transmit that same packet on the kgdb side.   I think that if we call
> > >breakpoint() here we can try and start over...
> >
> > The problem is that you are now doing a breakpoint from inside kgdb while
> > handling a prior breakpoint.
>
> Only in the case where we aren't out-of-sync, but gdb died /
> disconnected and we then hit a breakpoint.
>
> > At the very least you would need to consider
> > very carefully what happens after the breakpoint.  It is not clear that
> > you can recover from this condition... but you may be able to look
> > around.
>
> I don't follow you here.  I agree that in the case of hitting a
> breakpoint while gdb isn't connected isn't necessarily clean, but since
> gdb didn't go away cleanly, and there isn't a way for it to tell us it's
> trying to recover, I'm not sure what we can do aside from clear out the
> existing breakpoints (just like we could have done, if gdb was still
> connected) and keep going.  We should document that this particular
> behavior might not be a good thing, but it's just that one corner case
> of things (disconnect is fine, detach/reattach is fine, heck even gdb
> dying and not hitting a breakpoint is fine).