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).
--
Tom Rini
http://gate.crashing.org/~trini/
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).
On Tue, Mar 02, 2004 at 05:06:03PM +0530, Amit S. Kale wrote:
> 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) {
Hmm. I take it you passed in a different ttyS than you configured for?
Been a while since I tried that, and the above look correct. Go ahead.
--
Tom Rini
http://gate.crashing.org/~trini/