Re: [Kgdb-bugreport] [PATCH] Kill kgdb_serial

[George Anzinger <george@mvista.com>]

Tom Rini wrote:
> On Wed, Mar 03, 2004 at 04:27:57PM -0800, George Anzinger wrote:
> 
>>Tom Rini wrote:
>>
>>>But that's not what you get with kgdb_serial.  You get the possibility
>>>of serial from point A to B and you will have eth from point B onward,
>>>if compiled in.  With an arch serial driver you get the possibility of
>>>serial (or arch serial or whatever) from point A to B and eth from point
>>>B onward, if compiled in.
>>
>>I don't think we want to switch.  Rather we want to say something like: If 
>>no eth (or other input) options are on the command line then its is serial. 
>>If eth (or other input) is there, that is what we use.
>>
>>This does leave open what happens when "eth" is given and we hit a 
>>breakpoint prior to looking at the command line, but now this just fails so 
>>we would be hard put to do worse.
> 
> 
> This doesn't fail right now, or rather it shouldn't.  We would call
> kgdb_arch_init() which would set it to 8250 (or arch serial) and go.  If
> 8250||arch serial is compiled in.

But, if I understand this right, now you can have either eth or serial.  If you 
have eth and hit a breakpoint prior to its int, you are dead.
> 
> 
>>>I think you missed the point.  The problem isn't with providing weak
>>>functions, the problem is trying to set the function pointer.  PPC
>>>becomes quite clean since the next step is to kill off
>>>PPC_SIMPLE_SERIAL and just have kgdb_read/write_debug_char in the
>>>relevant serial drivers.
>>
>>No, you just set the default at configure time.  It is just done in such 
>>away as to allow it to be overridden.
> 
> 
> Which means you have to either c&p this into kgdb_arch_init for every
> arch that provides it's own, or (and I've been thinking that this isn't
> necessarily a bad idea) standardize on names for the arch serial driver,
> and in kernel/kgdb.c::kgdb_entry() do:
> #ifdef CONFIG_KGDB_8250
>   extern ... kgdb8250_serial;
>   kgdb_serial = &kgdb8250_serial;
> #elif CONFIG_KGDB_ARCH_SERIAL
>   extern ... kgdbarch_serial;
>   kgdb_serial = &kgdbarch_serial;
> #elif CONFIG_KGDB_ETH
>   extern ... kgdboe_serial;
>   kgdb_serial = &kgdboe_serial;
> #endif
> 

I would rather standardize on the name of the INIT block.  How about something like:

#include <linux/kdgb_io.h>
:
:
struc kgdb_io_table kgdb_io_table[]=KGDB_IO_FUNCTIONS;

then in linux/kgdb_io.h we have:

#include <asm/kgdb_io.h>

struc kgdb_io_table {
	char (*kgdb_read_char);
     	void  (*kgdb_write_char);
         :
	:
}

And in asm/kgdb_io.h we define:
extern char my_read_char(void);
:
#define KGDB_IO_FUNCTIONS {{my_read_char, my_write_char,...},\
                             {eth_read_char,...}}


So it is completely up to the arch asm/kgdb_io.h to define the names and even 
how many.  We then assume that the first one is the default.  An arch that wants 
to mess with different things can do it all in this file, including having 
several diffent serial drivers all with the same entry points, different default 
as set at configure time and so on.

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