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

Amit S. Kale
2004-03-02T11:36:31Z

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