Hello. The following interdiff kills kgdb_serial in favor of function
names. This only adds a weak function for kgdb_flush_io, and documents
when it would need to be provided.
diff -u linux-2.6.3/drivers/net/kgdb_eth.c linux-2.6.3/drivers/net/kgdb_eth.c
--- linux-2.6.3/drivers/net/kgdb_eth.c 2004-02-27 14:16:02.339413768 -0700
+++ linux-2.6.3/drivers/net/kgdb_eth.c 2004-03-02 14:31:41.267262668 -0700
@@ -72,7 +72,7 @@
.remote_mac = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
};
-int eth_getDebugChar(void)
+int kgdb_read_debug_char(void)
{
int chr;
@@ -85,7 +85,7 @@
return chr;
}
-void eth_flushDebugChar(void)
+void kgdb_flush_io(void)
{
if (out_count && np.dev) {
netpoll_send_udp(&np, out_buf, out_count);
@@ -94,11 +94,11 @@
}
}
-void eth_putDebugChar(int chr)
+void kgdb_write_debug_char(int chr)
{
out_buf[out_count++] = chr;
if (out_count == OUT_BUF_SIZE)
- eth_flushDebugChar();
+ kgdb_flush_io();
}
static void rx_hook(struct netpoll *np, int port, char *msg, int len)
@@ -137,7 +137,7 @@
__setup("kgdboe=", option_setup);
-static int hook(void)
+static int kgdb_hook_io(void)
{
/* Un-initalized, don't go further. */
if (kgdboe != 1)
@@ -148,19 +148,11 @@
return 0;
}
-struct kgdb_serial kgdbeth_serial = {
- .read_char = eth_getDebugChar,
- .write_char = eth_putDebugChar,
- .hook = hook,
- .flush = eth_flushDebugChar,
-};
-
static int init_kgdboe(void)
{
if (!np.remote_ip || netpoll_setup(&np))
return 1;
- kgdb_serial = &kgdbeth_serial;
kgdboe = 1;
printk(KERN_INFO "kgdb: debugging over ethernet enabled\n");
diff -u linux-2.6.3/drivers/serial/kgdb_8250.c linux-2.6.3/drivers/serial/kgdb_8250.c
--- linux-2.6.3/drivers/serial/kgdb_8250.c 2004-03-01 08:12:18.293899848 -0700
+++ linux-2.6.3/drivers/serial/kgdb_8250.c 2004-03-02 14:31:59.210205936 -0700
@@ -128,7 +128,7 @@
* Wait until the interface can accept a char, then write it.
*/
void
-kgdb_put_debug_char(int chr)
+kgdb_write_debug_char(int chr)
{
while (!(serial_inb(kgdb8250_port + (UART_LSR << reg_shift)) &
UART_LSR_THRE))
@@ -154,7 +154,7 @@
*/
if (it & 0xc) {
kgdb8250_init();
- kgdb_put_debug_char('-');
+ kgdb_write_debug_char('-');
return '-';
}
@@ -167,7 +167,7 @@
*/
int
-kgdb_get_debug_char(void)
+kgdb_read_debug_char(void)
{
int retchr;
unsigned long flags;
@@ -379,12 +379,6 @@
return 0;
}
-struct kgdb_serial kgdb8250_serial_driver = {
- .read_char = kgdb_get_debug_char,
- .write_char = kgdb_put_debug_char,
- .hook = kgdb_hook_io,
-};
-
void
kgdb8250_add_port(int i, struct uart_port *serial_req)
{
@@ -422,8 +416,6 @@
/* Make the baud rate change happen. */
gdb_async_info.state->custom_divisor = BASE_BAUD / kgdb8250_baud;
- kgdb_serial = &kgdb8250_serial_driver;
-
return 1;
errout:
diff -u linux-2.6.3/arch/ppc/kernel/kgdb.c linux-2.6.3/arch/ppc/kernel/kgdb.c
--- linux-2.6.3/arch/ppc/kernel/kgdb.c 2004-03-01 10:01:48.164203161 -0700
+++ linux-2.6.3/arch/ppc/kernel/kgdb.c 2004-03-02 14:26:16.330767482 -0700
@@ -263,27 +263,21 @@
};
#ifdef CONFIG_PPC_SIMPLE_SERIAL
-static void kgdbppc_write_char(int chr)
+static void kgdb_write_debug_char(int chr)
{
putDebugChar(chr);
}
-static int kgdbppc_read_char(void)
+static int kgdb_read_debug_char(void)
{
return getDebugChar();
}
-int kgdbppc_hook(void)
+int kgdb_hook_io(void)
{
kgdb_map_scc();
return 0;
}
-
-struct kgdb_serial kgdbppc_serial = {
- .read_char = kgdbppc_read_char,
- .write_char = kgdbppc_write_char,
- .hook = kgdbppc_hook
-};
#endif
int kgdb_arch_init (void)
@@ -295,14 +289,4 @@
debugger_dabr_match = kgdb_dabr_match;
- /* If we have the bigger 8250 serial driver, set that to be
- * the output now. */
-#if defined(CONFIG_KGDB_8250)
- extern struct kgdb_serial kgdb8250_serial_driver;
- kgdb_serial = &kgdb8250_serial_driver;
-#elif defined(CONFIG_PPC_SIMPLE_SERIAL)
- /* Take our serial driver. */
- kgdb_serial = &kgdbppc_serial;
-#endif
-
return 0;
}
diff -u linux-2.6.3/include/linux/kgdb.h linux-2.6.3/include/linux/kgdb.h
--- linux-2.6.3/include/linux/kgdb.h 2004-03-01 08:19:06.668218454 -0700
+++ linux-2.6.3/include/linux/kgdb.h 2004-03-02 14:27:05.880556965 -0700
@@ -102,14 +102,12 @@
/* Thread reference */
typedef unsigned char threadref[8];
-struct kgdb_serial {
- int (*read_char) (void);
- void (*write_char) (int);
- void (*flush) (void);
- int (*hook) (void);
-};
+/* I/O */
+extern int kgdb_read_debug_char(void);
+extern void kgdb_write_debug_char(int ch);
+extern void kgdb_flush_io(void);
+extern int kgdb_hook_io(void);
-extern struct kgdb_serial *kgdb_serial;
extern struct kgdb_arch arch_kgdb_ops;
extern int kgdb_initialized;
diff -u linux-2.6.3/kernel/kgdb.c linux-2.6.3/kernel/kgdb.c
--- linux-2.6.3/kernel/kgdb.c 2004-03-01 08:19:06.672217551 -0700
+++ linux-2.6.3/kernel/kgdb.c 2004-03-02 14:25:42.590401068 -0700
@@ -73,8 +73,6 @@
* also at runtime. */
int kgdb_useraccess = 0;
-struct kgdb_serial *kgdb_serial;
-
/*
* Holds information about breakpoints in a kernel. These breakpoints are
* added and removed by gdb.
@@ -105,8 +103,7 @@
* The following are the stub functions for code which is arch specific
* and can be omitted on some arches
* This function will handle the initalization of any architecture specific
- * hooks. If there is a suitable early output driver, kgdb_serial
- * can be pointed at it now.
+ * hooks.
*/
int __attribute__ ((weak))
kgdb_arch_init(void)
@@ -190,6 +187,17 @@
return NULL;
}
+/*
+ * An I/O driver must provide routines for reading and writing a single
+ * character. However, if characters are buffered, a method to flush them
+ * must be provided. The following is used as the default when a flush
+ * is a nop.
+ */
+void __attribute__ ((weak))
+ kgdb_flush_io(void)
+{
+}
+
static int hex(char ch)
{
if ((ch >= 'a') && (ch <= 'f'))
@@ -213,7 +221,7 @@
do {
/* wait around for the start character, ignore all other
* characters */
- while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$')
+ while ((ch = (kgdb_read_debug_char() & 0x7f)) != '$')
; /* Spin. */
kgdb_connected = 1;
checksum = 0;
@@ -223,7 +231,7 @@
/* now, read until a # or end of buffer is found */
while (count < (BUFMAX - 1)) {
- ch = kgdb_serial->read_char() & 0x7f;
+ ch = kgdb_read_debug_char() & 0x7f;
if (ch == '#')
break;
checksum = checksum + ch;
@@ -233,15 +241,14 @@
buffer[count] = 0;
if (ch == '#') {
- xmitcsum = hex(kgdb_serial->read_char() & 0x7f) << 4;
- xmitcsum += hex(kgdb_serial->read_char() & 0x7f);
+ xmitcsum = hex(kgdb_read_debug_char() & 0x7f) << 4;
+ xmitcsum += hex(kgdb_read_debug_char() & 0x7f);
if (checksum != xmitcsum)
- kgdb_serial->write_char('-'); /* failed checksum */
+ kgdb_write_debug_char('-'); /* failed checksum */
else
- kgdb_serial->write_char('+'); /* successful transfer */
- if (kgdb_serial->flush)
- kgdb_serial->flush();
+ kgdb_write_debug_char('+'); /* successful transfer */
+ kgdb_flush_io();
}
} while (checksum != xmitcsum);
}
@@ -258,27 +265,26 @@
/* $<packet info>#<checksum>. */
while (1) {
- kgdb_serial->write_char('$');
+ kgdb_write_debug_char('$');
checksum = 0;
count = 0;
while ((ch = buffer[count])) {
- kgdb_serial->write_char(ch);
+ kgdb_write_debug_char(ch);
checksum += ch;
count++;
}
- kgdb_serial->write_char('#');
- kgdb_serial->write_char(hexchars[checksum >> 4]);
- kgdb_serial->write_char(hexchars[checksum % 16]);
- if (kgdb_serial->flush)
- kgdb_serial->flush();
+ kgdb_write_debug_char('#');
+ kgdb_write_debug_char(hexchars[checksum >> 4]);
+ kgdb_write_debug_char(hexchars[checksum % 16]);
+ kgdb_flush_io();
/* Now see what we get in reply. */
- ch = kgdb_serial->read_char();
+ ch = kgdb_read_debug_char();
if (ch == 3)
- ch = kgdb_serial->read_char();
+ ch = kgdb_read_debug_char();
/* If we get an ACK, we are done. */
if (ch == '+')
@@ -289,9 +295,8 @@
* the packet being sent, and stop trying to send this
* packet. */
if (ch == '$') {
- kgdb_serial->write_char('-');
- if (kgdb_serial->flush)
- kgdb_serial->flush();
+ kgdb_write_debug_char('-');
+ kgdb_flush_io();
return;
}
}
@@ -1126,10 +1131,10 @@
atomic_set(&kgdb_setting_breakpoint, 0);
- if (!kgdb_serial || kgdb_serial->hook() < 0) {
+ if (kgdb_hook_io())
/* KGDB interface isn't ready yet */
return;
- }
+
kgdb_initialized = 1;
if (!kgdb_enter) {
return;
--
Tom Rini
http://gate.crashing.org/~trini/
Tom Rini wrote:
> Hello. The following interdiff kills kgdb_serial in favor of function
> names. This only adds a weak function for kgdb_flush_io, and documents
> when it would need to be provided.
It looks like you are also dumping any notion of building a kernel that can
choose which method of communication to use for kgdb at run time. Is this so?
-g
>
> diff -u linux-2.6.3/drivers/net/kgdb_eth.c linux-2.6.3/drivers/net/kgdb_eth.c
> --- linux-2.6.3/drivers/net/kgdb_eth.c 2004-02-27 14:16:02.339413768 -0700
> +++ linux-2.6.3/drivers/net/kgdb_eth.c 2004-03-02 14:31:41.267262668 -0700
> @@ -72,7 +72,7 @@
> .remote_mac = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
> };
>
> -int eth_getDebugChar(void)
> +int kgdb_read_debug_char(void)
> {
> int chr;
>
> @@ -85,7 +85,7 @@
> return chr;
> }
>
> -void eth_flushDebugChar(void)
> +void kgdb_flush_io(void)
> {
> if (out_count && np.dev) {
> netpoll_send_udp(&np, out_buf, out_count);
> @@ -94,11 +94,11 @@
> }
> }
>
> -void eth_putDebugChar(int chr)
> +void kgdb_write_debug_char(int chr)
> {
> out_buf[out_count++] = chr;
> if (out_count == OUT_BUF_SIZE)
> - eth_flushDebugChar();
> + kgdb_flush_io();
> }
>
> static void rx_hook(struct netpoll *np, int port, char *msg, int len)
> @@ -137,7 +137,7 @@
>
> __setup("kgdboe=", option_setup);
>
> -static int hook(void)
> +static int kgdb_hook_io(void)
> {
> /* Un-initalized, don't go further. */
> if (kgdboe != 1)
> @@ -148,19 +148,11 @@
> return 0;
> }
>
> -struct kgdb_serial kgdbeth_serial = {
> - .read_char = eth_getDebugChar,
> - .write_char = eth_putDebugChar,
> - .hook = hook,
> - .flush = eth_flushDebugChar,
> -};
> -
> static int init_kgdboe(void)
> {
> if (!np.remote_ip || netpoll_setup(&np))
> return 1;
>
> - kgdb_serial = &kgdbeth_serial;
> kgdboe = 1;
> printk(KERN_INFO "kgdb: debugging over ethernet enabled\n");
>
> diff -u linux-2.6.3/drivers/serial/kgdb_8250.c linux-2.6.3/drivers/serial/kgdb_8250.c
> --- linux-2.6.3/drivers/serial/kgdb_8250.c 2004-03-01 08:12:18.293899848 -0700
> +++ linux-2.6.3/drivers/serial/kgdb_8250.c 2004-03-02 14:31:59.210205936 -0700
> @@ -128,7 +128,7 @@
> * Wait until the interface can accept a char, then write it.
> */
> void
> -kgdb_put_debug_char(int chr)
> +kgdb_write_debug_char(int chr)
> {
> while (!(serial_inb(kgdb8250_port + (UART_LSR << reg_shift)) &
> UART_LSR_THRE))
> @@ -154,7 +154,7 @@
> */
> if (it & 0xc) {
> kgdb8250_init();
> - kgdb_put_debug_char('-');
> + kgdb_write_debug_char('-');
> return '-';
> }
>
> @@ -167,7 +167,7 @@
> */
>
> int
> -kgdb_get_debug_char(void)
> +kgdb_read_debug_char(void)
> {
> int retchr;
> unsigned long flags;
> @@ -379,12 +379,6 @@
> return 0;
> }
>
> -struct kgdb_serial kgdb8250_serial_driver = {
> - .read_char = kgdb_get_debug_char,
> - .write_char = kgdb_put_debug_char,
> - .hook = kgdb_hook_io,
> -};
> -
> void
> kgdb8250_add_port(int i, struct uart_port *serial_req)
> {
> @@ -422,8 +416,6 @@
> /* Make the baud rate change happen. */
> gdb_async_info.state->custom_divisor = BASE_BAUD / kgdb8250_baud;
>
> - kgdb_serial = &kgdb8250_serial_driver;
> -
> return 1;
>
> errout:
> diff -u linux-2.6.3/arch/ppc/kernel/kgdb.c linux-2.6.3/arch/ppc/kernel/kgdb.c
> --- linux-2.6.3/arch/ppc/kernel/kgdb.c 2004-03-01 10:01:48.164203161 -0700
> +++ linux-2.6.3/arch/ppc/kernel/kgdb.c 2004-03-02 14:26:16.330767482 -0700
> @@ -263,27 +263,21 @@
> };
>
> #ifdef CONFIG_PPC_SIMPLE_SERIAL
> -static void kgdbppc_write_char(int chr)
> +static void kgdb_write_debug_char(int chr)
> {
> putDebugChar(chr);
> }
>
> -static int kgdbppc_read_char(void)
> +static int kgdb_read_debug_char(void)
> {
> return getDebugChar();
> }
>
> -int kgdbppc_hook(void)
> +int kgdb_hook_io(void)
> {
> kgdb_map_scc();
> return 0;
> }
> -
> -struct kgdb_serial kgdbppc_serial = {
> - .read_char = kgdbppc_read_char,
> - .write_char = kgdbppc_write_char,
> - .hook = kgdbppc_hook
> -};
> #endif
>
> int kgdb_arch_init (void)
> @@ -295,14 +289,4 @@
> debugger_dabr_match = kgdb_dabr_match;
>
> - /* If we have the bigger 8250 serial driver, set that to be
> - * the output now. */
> -#if defined(CONFIG_KGDB_8250)
> - extern struct kgdb_serial kgdb8250_serial_driver;
> - kgdb_serial = &kgdb8250_serial_driver;
> -#elif defined(CONFIG_PPC_SIMPLE_SERIAL)
> - /* Take our serial driver. */
> - kgdb_serial = &kgdbppc_serial;
> -#endif
> -
> return 0;
> }
> diff -u linux-2.6.3/include/linux/kgdb.h linux-2.6.3/include/linux/kgdb.h
> --- linux-2.6.3/include/linux/kgdb.h 2004-03-01 08:19:06.668218454 -0700
> +++ linux-2.6.3/include/linux/kgdb.h 2004-03-02 14:27:05.880556965 -0700
> @@ -102,14 +102,12 @@
> /* Thread reference */
> typedef unsigned char threadref[8];
>
> -struct kgdb_serial {
> - int (*read_char) (void);
> - void (*write_char) (int);
> - void (*flush) (void);
> - int (*hook) (void);
> -};
> +/* I/O */
> +extern int kgdb_read_debug_char(void);
> +extern void kgdb_write_debug_char(int ch);
> +extern void kgdb_flush_io(void);
> +extern int kgdb_hook_io(void);
>
> -extern struct kgdb_serial *kgdb_serial;
> extern struct kgdb_arch arch_kgdb_ops;
> extern int kgdb_initialized;
>
> diff -u linux-2.6.3/kernel/kgdb.c linux-2.6.3/kernel/kgdb.c
> --- linux-2.6.3/kernel/kgdb.c 2004-03-01 08:19:06.672217551 -0700
> +++ linux-2.6.3/kernel/kgdb.c 2004-03-02 14:25:42.590401068 -0700
> @@ -73,8 +73,6 @@
> * also at runtime. */
> int kgdb_useraccess = 0;
>
> -struct kgdb_serial *kgdb_serial;
> -
> /*
> * Holds information about breakpoints in a kernel. These breakpoints are
> * added and removed by gdb.
> @@ -105,8 +103,7 @@
> * The following are the stub functions for code which is arch specific
> * and can be omitted on some arches
> * This function will handle the initalization of any architecture specific
> - * hooks. If there is a suitable early output driver, kgdb_serial
> - * can be pointed at it now.
> + * hooks.
> */
> int __attribute__ ((weak))
> kgdb_arch_init(void)
> @@ -190,6 +187,17 @@
> return NULL;
> }
>
> +/*
> + * An I/O driver must provide routines for reading and writing a single
> + * character. However, if characters are buffered, a method to flush them
> + * must be provided. The following is used as the default when a flush
> + * is a nop.
> + */
> +void __attribute__ ((weak))
> + kgdb_flush_io(void)
> +{
> +}
> +
> static int hex(char ch)
> {
> if ((ch >= 'a') && (ch <= 'f'))
> @@ -213,7 +221,7 @@
> do {
> /* wait around for the start character, ignore all other
> * characters */
> - while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$')
> + while ((ch = (kgdb_read_debug_char() & 0x7f)) != '$')
> ; /* Spin. */
> kgdb_connected = 1;
> checksum = 0;
> @@ -223,7 +231,7 @@
>
> /* now, read until a # or end of buffer is found */
> while (count < (BUFMAX - 1)) {
> - ch = kgdb_serial->read_char() & 0x7f;
> + ch = kgdb_read_debug_char() & 0x7f;
> if (ch == '#')
> break;
> checksum = checksum + ch;
> @@ -233,15 +241,14 @@
> buffer[count] = 0;
>
> if (ch == '#') {
> - xmitcsum = hex(kgdb_serial->read_char() & 0x7f) << 4;
> - xmitcsum += hex(kgdb_serial->read_char() & 0x7f);
> + xmitcsum = hex(kgdb_read_debug_char() & 0x7f) << 4;
> + xmitcsum += hex(kgdb_read_debug_char() & 0x7f);
>
> if (checksum != xmitcsum)
> - kgdb_serial->write_char('-'); /* failed checksum */
> + kgdb_write_debug_char('-'); /* failed checksum */
> else
> - kgdb_serial->write_char('+'); /* successful transfer */
> - if (kgdb_serial->flush)
> - kgdb_serial->flush();
> + kgdb_write_debug_char('+'); /* successful transfer */
> + kgdb_flush_io();
> }
> } while (checksum != xmitcsum);
> }
> @@ -258,27 +265,26 @@
>
> /* $<packet info>#<checksum>. */
> while (1) {
> - kgdb_serial->write_char('$');
> + kgdb_write_debug_char('$');
> checksum = 0;
> count = 0;
>
> while ((ch = buffer[count])) {
> - kgdb_serial->write_char(ch);
> + kgdb_write_debug_char(ch);
> checksum += ch;
> count++;
> }
>
> - kgdb_serial->write_char('#');
> - kgdb_serial->write_char(hexchars[checksum >> 4]);
> - kgdb_serial->write_char(hexchars[checksum % 16]);
> - if (kgdb_serial->flush)
> - kgdb_serial->flush();
> + kgdb_write_debug_char('#');
> + kgdb_write_debug_char(hexchars[checksum >> 4]);
> + kgdb_write_debug_char(hexchars[checksum % 16]);
> + kgdb_flush_io();
>
> /* Now see what we get in reply. */
> - ch = kgdb_serial->read_char();
> + ch = kgdb_read_debug_char();
>
> if (ch == 3)
> - ch = kgdb_serial->read_char();
> + ch = kgdb_read_debug_char();
>
> /* If we get an ACK, we are done. */
> if (ch == '+')
> @@ -289,9 +295,8 @@
> * the packet being sent, and stop trying to send this
> * packet. */
> if (ch == '$') {
> - kgdb_serial->write_char('-');
> - if (kgdb_serial->flush)
> - kgdb_serial->flush();
> + kgdb_write_debug_char('-');
> + kgdb_flush_io();
> return;
> }
> }
> @@ -1126,10 +1131,10 @@
>
> atomic_set(&kgdb_setting_breakpoint, 0);
>
> - if (!kgdb_serial || kgdb_serial->hook() < 0) {
> + if (kgdb_hook_io())
> /* KGDB interface isn't ready yet */
> return;
> - }
> +
> kgdb_initialized = 1;
> if (!kgdb_enter) {
> return;
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
On Tue, Mar 02, 2004 at 02:02:16PM -0800, George Anzinger wrote:
> Tom Rini wrote:
> >Hello. The following interdiff kills kgdb_serial in favor of function
> >names. This only adds a weak function for kgdb_flush_io, and documents
> >when it would need to be provided.
>
> It looks like you are also dumping any notion of building a kernel that can
> choose which method of communication to use for kgdb at run time. Is this
> so?
Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
you try and allow for any 2 of 3 methods.
--
Tom Rini
http://gate.crashing.org/~trini/
Hi!
> > Tom Rini wrote:
> > >Hello. The following interdiff kills kgdb_serial in favor of function
> > >names. This only adds a weak function for kgdb_flush_io, and documents
> > >when it would need to be provided.
> >
> > It looks like you are also dumping any notion of building a kernel that can
> > choose which method of communication to use for kgdb at run time. Is this
> > so?
>
> Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
> you try and allow for any 2 of 3 methods.
I do not think that having kgdb_serial is so ugly. Are there any other
uglyness associated with that?
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
> Hi!
>
> > > Tom Rini wrote:
> > > >Hello. The following interdiff kills kgdb_serial in favor of function
> > > >names. This only adds a weak function for kgdb_flush_io, and documents
> > > >when it would need to be provided.
> > >
> > > It looks like you are also dumping any notion of building a kernel that can
> > > choose which method of communication to use for kgdb at run time. Is this
> > > so?
> >
> > Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
> > you try and allow for any 2 of 3 methods.
>
> I do not think that having kgdb_serial is so ugly. Are there any other
> uglyness associated with that?
Yes, switching from one of 3 choices to the other. It's not as simple
as 8250 or eth. It's 8250 or eth or arch for things which don't depend
on other initcalls.
This leads to things in the PPC code like:
#ifdef CONFIG_KGDB_8250
extern ...
kgdb_serial = &kgdb8250_serial;
#elif defined(CONFIG_PPC_SIMPLE_SERIAL)
kgdb_serial = &kgdbppc_serial;
#endif
And then all kgdb_arch_init's have to have kgdb_serial =
&kgdb8250_serial, for it to work prior to the initcall setting it.
--
Tom Rini
http://gate.crashing.org/~trini/
On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
> Hi!
>
> > > Tom Rini wrote:
> > > >Hello. The following interdiff kills kgdb_serial in favor of function
> > > >names. This only adds a weak function for kgdb_flush_io, and documents
> > > >when it would need to be provided.
> > >
> > > It looks like you are also dumping any notion of building a kernel that can
> > > choose which method of communication to use for kgdb at run time. Is this
> > > so?
> >
> > Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
> > you try and allow for any 2 of 3 methods.
>
> I do not think that having kgdb_serial is so ugly. Are there any other
> uglyness associated with that?
More precisely:
http://lkml.org/lkml/2004/2/11/224
--
Tom Rini
http://gate.crashing.org/~trini/
Tom Rini wrote:
> On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
>
>
>>Hi!
>>
>>
>>>>Tom Rini wrote:
>>>>
>>>>>Hello. The following interdiff kills kgdb_serial in favor of function
>>>>>names. This only adds a weak function for kgdb_flush_io, and documents
>>>>>when it would need to be provided.
>>>>
>>>>It looks like you are also dumping any notion of building a kernel that can
>>>>choose which method of communication to use for kgdb at run time. Is this
>>>>so?
>>>
>>>Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
>>>you try and allow for any 2 of 3 methods.
>>
>>I do not think that having kgdb_serial is so ugly. Are there any other
>>uglyness associated with that?
>
>
> More precisely:
> http://lkml.org/lkml/2004/2/11/224
Andrew seems to be comming from the point of view of a developer rather than a
developer/ maintainer.
So, the counter argument is the user who is sending the thing into the field and
wants to send just one binary kernel to all locations. But then he needs to
debug some problem that will work fine over the lan and later one that requires
an early connection which the lan can not, as yet, do. I agree that for you or
me, this is not an issue, but what of the IT folks...
As to KGDB_MORE and KGDB_OPTIONS, they were put in for those who want to change
the "O" option. I feel it should NOT be changed, but I recognize that some
folks want to reduce the optimizer distortions. I opted for a generalized
capability to cover all bases...
KGDB_TS is for code developers. I invented it to give a poor mans LTT and it
has served me well. I am not sure we need to be able to turn it off, however.
The stack overflow thing is, I think, now in the kernel AND I have never had a
machine behave so badly that it actually caught one. It can go.
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
On Tue, Mar 02, 2004 at 03:46:19PM -0800, George Anzinger wrote:
> Tom Rini wrote:
> >On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
> >
> >
> >>Hi!
> >>
> >>
> >>>>Tom Rini wrote:
> >>>>
> >>>>>Hello. The following interdiff kills kgdb_serial in favor of function
> >>>>>names. This only adds a weak function for kgdb_flush_io, and documents
> >>>>>when it would need to be provided.
> >>>>
> >>>>It looks like you are also dumping any notion of building a kernel that
> >>>>can choose which method of communication to use for kgdb at run time.
> >>>>Is this so?
> >>>
> >>>Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
> >>>you try and allow for any 2 of 3 methods.
> >>
> >>I do not think that having kgdb_serial is so ugly. Are there any other
> >>uglyness associated with that?
> >
> >
> >More precisely:
> >http://lkml.org/lkml/2004/2/11/224
>
> Andrew seems to be comming from the point of view of a developer rather
> than a developer/ maintainer.
>
> So, the counter argument is the user who is sending the thing into the
> field and wants to send just one binary kernel to all locations. But then
> he needs to debug some problem that will work fine over the lan and later
> one that requires an early connection which the lan can not, as yet, do. I
> agree that for you or me, this is not an issue, but what of the IT folks...
The IT person should be beaten for shipping KGDB on a production system?
:)
Regardless, it's not that we offer (nor does the -mm version, from what
I read of it) eth or serial at any point, it simply allows for serial to
be used and a switchover to eth. And if kgdb is attached at the time,
it's a 'fun' gdb session (or at least is was when I was trying it out in
-mm and then in my own version).
The real problem is that you start getting quite complex when you allow
for a system to be kgdb eth, or 8250, or some arch serial driver, or
some other I/O driver, and so on. PPC has 3, and I don't see it getting
smaller from there.
And with both of those points, I don't think it's worth the trouble that
point 2 is, given the limitations of point 1.
--
Tom Rini
http://gate.crashing.org/~trini/
Tom Rini wrote:
> On Tue, Mar 02, 2004 at 03:46:19PM -0800, George Anzinger wrote:
>
>>Tom Rini wrote:
>>
>>>On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
>>>
>>>
>>>
>>>>Hi!
>>>>
>>>>
>>>>
>>>>>>Tom Rini wrote:
>>>>>>
>>>>>>
>>>>>>>Hello. The following interdiff kills kgdb_serial in favor of function
>>>>>>>names. This only adds a weak function for kgdb_flush_io, and documents
>>>>>>>when it would need to be provided.
>>>>>>
>>>>>>It looks like you are also dumping any notion of building a kernel that
>>>>>>can choose which method of communication to use for kgdb at run time.
>>>>>>Is this so?
>>>>>
>>>>>Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
>>>>>you try and allow for any 2 of 3 methods.
>>>>
>>>>I do not think that having kgdb_serial is so ugly. Are there any other
>>>>uglyness associated with that?
>>>
>>>
>>>More precisely:
>>>http://lkml.org/lkml/2004/2/11/224
>>
>>Andrew seems to be comming from the point of view of a developer rather
>>than a developer/ maintainer.
>>
>>So, the counter argument is the user who is sending the thing into the
>>field and wants to send just one binary kernel to all locations. But then
>>he needs to debug some problem that will work fine over the lan and later
>>one that requires an early connection which the lan can not, as yet, do. I
>>agree that for you or me, this is not an issue, but what of the IT folks...
>
>
> The IT person should be beaten for shipping KGDB on a production system?
> :)
Well, they don't tell :)
>
> Regardless, it's not that we offer (nor does the -mm version, from what
> I read of it) eth or serial at any point, it simply allows for serial to
> be used and a switchover to eth. And if kgdb is attached at the time,
> it's a 'fun' gdb session (or at least is was when I was trying it out in
> -mm and then in my own version).
I am not really suggesting a live switch capability, more like something that is
set a boot time.
>
> The real problem is that you start getting quite complex when you allow
> for a system to be kgdb eth, or 8250, or some arch serial driver, or
> some other I/O driver, and so on. PPC has 3, and I don't see it getting
> smaller from there.
I had imagined that it would be rather like a file system. The stub would pass
(or it could be a global if you prefer) the index to use into an array of
interface structures. Something like:
struct kgdb_interface {
void (*kgdb_in)(*char)
:
:
}
struct kgdb_interface kgdb_io_array[N];
>
> And with both of those points, I don't think it's worth the trouble that
> point 2 is, given the limitations of point 1.
I imagine that I would like this. I would use the eth interface until required
to use the serial. I would rather not have to rebuild the kernel to do this....
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
On Wednesday 03 Mar 2004 5:16 am, George Anzinger wrote:
> Tom Rini wrote:
> > On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
> >>Hi!
> >>
> >>>>Tom Rini wrote:
> >>>>>Hello. The following interdiff kills kgdb_serial in favor of function
> >>>>>names. This only adds a weak function for kgdb_flush_io, and
> >>>>> documents when it would need to be provided.
> >>>>
> >>>>It looks like you are also dumping any notion of building a kernel that
> >>>> can choose which method of communication to use for kgdb at run time.
> >>>> Is this so?
> >>>
> >>>Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
> >>>you try and allow for any 2 of 3 methods.
> >>
> >>I do not think that having kgdb_serial is so ugly. Are there any other
> >>uglyness associated with that?
> >
> > More precisely:
> > http://lkml.org/lkml/2004/2/11/224
>
> Andrew seems to be comming from the point of view of a developer rather
> than a developer/ maintainer.
>
> So, the counter argument is the user who is sending the thing into the
> field and wants to send just one binary kernel to all locations. But then
> he needs to debug some problem that will work fine over the lan and later
> one that requires an early connection which the lan can not, as yet, do. I
> agree that for you or me, this is not an issue, but what of the IT folks...
This is the same reason specifying 8250 parameters on a kernel command line is
good. If one builds a different kernel for each test machine, fixing kgdb
interface parameters during a build doesn't cause any problems. I don't think
compiling different kernels is good when you have more than 2 machines (of
same architecture). It's much easier to build a single kernel with drivers
required by all of them and then boot them with kgdb as and when required
with interface parameters coming from grug.conf (or equivalent on other
archs).
kgdb_serial isn't ugly. It's just a function switch, similar to several of
them in the kernel. ppc is ugly, but that's anyway the case because of so
many varieties of ppc. If we are trying to make ppc code clean, it makes more
sense to move this weak function thing into ppc specific files IMHO.
-Amit
>
> As to KGDB_MORE and KGDB_OPTIONS, they were put in for those who want to
> change the "O" option. I feel it should NOT be changed, but I recognize
> that some folks want to reduce the optimizer distortions. I opted for a
> generalized capability to cover all bases...
>
> KGDB_TS is for code developers. I invented it to give a poor mans LTT and
> it has served me well. I am not sure we need to be able to turn it off,
> however.
>
> The stack overflow thing is, I think, now in the kernel AND I have never
> had a machine behave so badly that it actually caught one. It can go.
On ?t 02-03-04 16:00:18, Tom Rini wrote:
> On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
>
> > Hi!
> >
> > > > Tom Rini wrote:
> > > > >Hello. The following interdiff kills kgdb_serial in favor of function
> > > > >names. This only adds a weak function for kgdb_flush_io, and documents
> > > > >when it would need to be provided.
> > > >
> > > > It looks like you are also dumping any notion of building a kernel that can
> > > > choose which method of communication to use for kgdb at run time. Is this
> > > > so?
> > >
> > > Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
> > > you try and allow for any 2 of 3 methods.
> >
> > I do not think that having kgdb_serial is so ugly. Are there any other
> > uglyness associated with that?
>
> More precisely:
> http://lkml.org/lkml/2004/2/11/224
Well, that just says Andrew does not care too much. I think that
having both serial and ethernet support *is* good idea after all... I
have few machines here, some of them do not have serial, and some of
them do not have supported ethernet. It would be nice to use same
kernel on all of them. Also distribution wants to have "debugging
kernel", but does _not_ want to have 10 of them.
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
On Wed, Mar 03, 2004 at 11:13:02AM +0530, Amit S. Kale wrote:
> On Wednesday 03 Mar 2004 5:16 am, George Anzinger wrote:
> > Tom Rini wrote:
> > > On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
> > >>Hi!
> > >>
> > >>>>Tom Rini wrote:
> > >>>>>Hello. The following interdiff kills kgdb_serial in favor of function
> > >>>>>names. This only adds a weak function for kgdb_flush_io, and
> > >>>>> documents when it would need to be provided.
> > >>>>
> > >>>>It looks like you are also dumping any notion of building a kernel that
> > >>>> can choose which method of communication to use for kgdb at run time.
> > >>>> Is this so?
> > >>>
> > >>>Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
> > >>>you try and allow for any 2 of 3 methods.
> > >>
> > >>I do not think that having kgdb_serial is so ugly. Are there any other
> > >>uglyness associated with that?
> > >
> > > More precisely:
> > > http://lkml.org/lkml/2004/2/11/224
> >
> > Andrew seems to be comming from the point of view of a developer rather
> > than a developer/ maintainer.
> >
> > So, the counter argument is the user who is sending the thing into the
> > field and wants to send just one binary kernel to all locations. But then
> > he needs to debug some problem that will work fine over the lan and later
> > one that requires an early connection which the lan can not, as yet, do. I
> > agree that for you or me, this is not an issue, but what of the IT folks...
>
> This is the same reason specifying 8250 parameters on a kernel command line is
> good. If one builds a different kernel for each test machine, fixing kgdb
> interface parameters during a build doesn't cause any problems. I don't think
> compiling different kernels is good when you have more than 2 machines (of
> same architecture). It's much easier to build a single kernel with drivers
> required by all of them and then boot them with kgdb as and when required
> with interface parameters coming from grug.conf (or equivalent on other
> archs).
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.
> kgdb_serial isn't ugly. It's just a function switch, similar to several of
> them in the kernel. ppc is ugly, but that's anyway the case because of so
> many varieties of ppc. If we are trying to make ppc code clean, it makes more
> sense to move this weak function thing into ppc specific files IMHO.
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.
--
Tom Rini
http://gate.crashing.org/~trini/
On Wed, Mar 03, 2004 at 12:35:12AM +0100, Pavel Machek wrote:
> On ?t 02-03-04 16:00:18, Tom Rini wrote:
> > On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
> >
> > > Hi!
> > >
> > > > > Tom Rini wrote:
> > > > > >Hello. The following interdiff kills kgdb_serial in favor of function
> > > > > >names. This only adds a weak function for kgdb_flush_io, and documents
> > > > > >when it would need to be provided.
> > > > >
> > > > > It looks like you are also dumping any notion of building a kernel that can
> > > > > choose which method of communication to use for kgdb at run time. Is this
> > > > > so?
> > > >
> > > > Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
> > > > you try and allow for any 2 of 3 methods.
> > >
> > > I do not think that having kgdb_serial is so ugly. Are there any other
> > > uglyness associated with that?
> >
> > More precisely:
> > http://lkml.org/lkml/2004/2/11/224
>
> Well, that just says Andrew does not care too much. I think that
> having both serial and ethernet support *is* good idea after all... I
> have few machines here, some of them do not have serial, and some of
> them do not have supported ethernet. It would be nice to use same
> kernel on all of them. Also distribution wants to have "debugging
> kernel", but does _not_ want to have 10 of them.
But unless I'm missing something, supporting eth or 8250 at all times
doesn't work right now anyhow, as eth if available will always take over.
--
Tom Rini
http://gate.crashing.org/~trini/
On Tue, Mar 02, 2004 at 04:36:11PM -0800, George Anzinger wrote:
> Tom Rini wrote:
> >Regardless, it's not that we offer (nor does the -mm version, from what
> >I read of it) eth or serial at any point, it simply allows for serial to
> >be used and a switchover to eth. And if kgdb is attached at the time,
> >it's a 'fun' gdb session (or at least is was when I was trying it out in
> >-mm and then in my own version).
>
> I am not really suggesting a live switch capability, more like something
> that is set a boot time.
That's still not, AFAICT, what's offered in -mm or kgdb.sf.net.
> >The real problem is that you start getting quite complex when you allow
> >for a system to be kgdb eth, or 8250, or some arch serial driver, or
> >some other I/O driver, and so on. PPC has 3, and I don't see it getting
> >smaller from there.
>
> I had imagined that it would be rather like a file system. The stub would
> pass (or it could be a global if you prefer) the index to use into an array
> of interface structures. Something like:
>
> struct kgdb_interface {
> void (*kgdb_in)(*char)
> :
> :
> }
>
> struct kgdb_interface kgdb_io_array[N];
And how do you pick a default?
> >And with both of those points, I don't think it's worth the trouble that
> >point 2 is, given the limitations of point 1.
>
> I imagine that I would like this. I would use the eth interface until
> required to use the serial. I would rather not have to rebuild the kernel
> to do this....
Then you're talking about live switching again, or some sort of control
from userland.
--
Tom Rini
http://gate.crashing.org/~trini/
On Tue, Mar 02, 2004 at 04:00:18PM -0700, Tom Rini wrote:
> On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
>
> > Hi!
> >
> > > > Tom Rini wrote:
> > > > >Hello. The following interdiff kills kgdb_serial in favor of function
> > > > >names. This only adds a weak function for kgdb_flush_io, and documents
> > > > >when it would need to be provided.
> > > >
> > > > It looks like you are also dumping any notion of building a kernel that can
> > > > choose which method of communication to use for kgdb at run time. Is this
> > > > so?
> > >
> > > Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
> > > you try and allow for any 2 of 3 methods.
> >
> > I do not think that having kgdb_serial is so ugly. Are there any other
> > uglyness associated with that?
>
> More precisely:
> http://lkml.org/lkml/2004/2/11/224
I think it's a step in the wrong direction. I'd like to be able to
compile all my kernels with support for all the debugging
functionality I _might_ find myself needing. I'd generally prefer to
use kgdboe, but if I have to fall back to using serial for some early
boot issue or network issue and have to recompile on top of that, I'll
be annoyed.
I'd rather see everything be run-time. Ideally, serial kgdb would be
smart enough to read command line args to know whether to grab ports
and so on and need essentially no compile time config.
--
Matt Mackall : http://www.selenic.com : Linux development and consulting
Hi!
> > > More precisely:
> > > http://lkml.org/lkml/2004/2/11/224
> >
> > Well, that just says Andrew does not care too much. I think that
> > having both serial and ethernet support *is* good idea after all... I
> > have few machines here, some of them do not have serial, and some of
> > them do not have supported ethernet. It would be nice to use same
> > kernel on all of them. Also distribution wants to have "debugging
> > kernel", but does _not_ want to have 10 of them.
>
> But unless I'm missing something, supporting eth or 8250 at all times
> doesn't work right now anyhow, as eth if available will always take over.
Well, that can be fixed. [Probably if kgdbeth= is not passed, ethernet
interface should not take over. So user selects which one should be
used by either passing kgdbeth or kgdb8250. That means that 8250
should not be initialized until user passes kgdb8250=... not sure how
you'll like that].
Pavel
--
Horseback riding is like software...
...vgf orggre jura vgf serr.
On Wed, Mar 03, 2004 at 04:51:06PM +0100, Pavel Machek wrote:
> Hi!
>
> > > > More precisely:
> > > > http://lkml.org/lkml/2004/2/11/224
> > >
> > > Well, that just says Andrew does not care too much. I think that
> > > having both serial and ethernet support *is* good idea after all... I
> > > have few machines here, some of them do not have serial, and some of
> > > them do not have supported ethernet. It would be nice to use same
> > > kernel on all of them. Also distribution wants to have "debugging
> > > kernel", but does _not_ want to have 10 of them.
> >
> > But unless I'm missing something, supporting eth or 8250 at all times
> > doesn't work right now anyhow, as eth if available will always take over.
>
> Well, that can be fixed. [Probably if kgdbeth= is not passed, ethernet
> interface should not take over. So user selects which one should be
> used by either passing kgdbeth or kgdb8250. That means that 8250
> should not be initialized until user passes kgdb8250=... not sure how
> you'll like that].
At this point, I'm going to give up on killing kgdb_serial, and pass
along some comments from David Woodhouse on IRC as well (I was talking
about this issue, and the init/main.c change):
(Tartarus == me, dwmw2 == David Woodhouse)
<Tartarus> dwmw2, the problem is how do you deal with all of the
possibilities of i/o (8250, kgdboe, or other serial) and do you allow
for passing 'gdb' on the command line to result in kgdb not being dropped
into? You can always break in later on of course
<dwmw2> parse command line early for 'gdb=' argument specifying which
i/o device to use. init kgdb core early. init each i/o device as early
as possible for that i/o device. Start the selected i/o device as soon
as it becomes available.
<dwmw2> just like console could, if we looked for console= a little bit
earlier. (forget all the earlyconsole shite, it's not necessary)
<dwmw2> Tartarrus, do the __early_setup() thing to replace __setup() for
selected args. We can use that for console= too.
<dwmw2> since 'console=' on the command line _already_ remembers its
arguments, and starts to use the offending device as soon as it gets
registered with register_console().
<Tartarus> dwmw2, __early_setup() ?
<dwmw2> See __setup("gdb=", gdb_setup_func);
<dwmw2> Replace with __early_setup(...)
<Tartarus> where is __early_setup ?
<dwmw2> before we normally parse the command line
<dwmw2> in my head
So perhaps someone can take these ideas and fix both problems... :)
(I've got some other stuff I need to work on today).
--
Tom Rini
http://gate.crashing.org/~trini/
Tom Rini wrote:
> On Tue, Mar 02, 2004 at 04:36:11PM -0800, George Anzinger wrote:
>
>>Tom Rini wrote:
>>
>>>Regardless, it's not that we offer (nor does the -mm version, from what
>>>I read of it) eth or serial at any point, it simply allows for serial to
>>>be used and a switchover to eth. And if kgdb is attached at the time,
>>>it's a 'fun' gdb session (or at least is was when I was trying it out in
>>>-mm and then in my own version).
>>
>>I am not really suggesting a live switch capability, more like something
>>that is set a boot time.
>
>
> That's still not, AFAICT, what's offered in -mm or kgdb.sf.net.
>
>
>>>The real problem is that you start getting quite complex when you allow
>>>for a system to be kgdb eth, or 8250, or some arch serial driver, or
>>>some other I/O driver, and so on. PPC has 3, and I don't see it getting
>>>smaller from there.
>>
>>I had imagined that it would be rather like a file system. The stub would
>>pass (or it could be a global if you prefer) the index to use into an array
>>of interface structures. Something like:
>>
>>struct kgdb_interface {
>> void (*kgdb_in)(*char)
>> :
>> :
>>}
>>
>>struct kgdb_interface kgdb_io_array[N];
>
>
> And how do you pick a default?
A config option.
>
>
>>>And with both of those points, I don't think it's worth the trouble that
>>>point 2 is, given the limitations of point 1.
>>
>>I imagine that I would like this. I would use the eth interface until
>>required to use the serial. I would rather not have to rebuild the kernel
>>to do this....
>
>
> Then you're talking about live switching again, or some sort of control
> from userland.
>
No, I would still choose this at boot time, it is just that I have to reboot the
same kernel, not a new one. It would be frozen once kgdb sees the command line.
Prior to that it would be the default chosen at config time.
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
Tom Rini wrote:
> On Wed, Mar 03, 2004 at 11:13:02AM +0530, Amit S. Kale wrote:
>
>
>>On Wednesday 03 Mar 2004 5:16 am, George Anzinger wrote:
>>
>>>Tom Rini wrote:
>>>
>>>>On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
>>>>
>>>>>Hi!
>>>>>
>>>>>
>>>>>>>Tom Rini wrote:
>>>>>>>
>>>>>>>>Hello. The following interdiff kills kgdb_serial in favor of function
>>>>>>>>names. This only adds a weak function for kgdb_flush_io, and
>>>>>>>>documents when it would need to be provided.
>>>>>>>
>>>>>>>It looks like you are also dumping any notion of building a kernel that
>>>>>>>can choose which method of communication to use for kgdb at run time.
>>>>>>>Is this so?
>>>>>>
>>>>>>Yes, as this is how Andrew suggested we do it. It becomes quite ugly if
>>>>>>you try and allow for any 2 of 3 methods.
>>>>>
>>>>>I do not think that having kgdb_serial is so ugly. Are there any other
>>>>>uglyness associated with that?
>>>>
>>>>More precisely:
>>>>http://lkml.org/lkml/2004/2/11/224
>>>
>>>Andrew seems to be comming from the point of view of a developer rather
>>>than a developer/ maintainer.
>>>
>>>So, the counter argument is the user who is sending the thing into the
>>>field and wants to send just one binary kernel to all locations. But then
>>>he needs to debug some problem that will work fine over the lan and later
>>>one that requires an early connection which the lan can not, as yet, do. I
>>>agree that for you or me, this is not an issue, but what of the IT folks...
>>
>>This is the same reason specifying 8250 parameters on a kernel command line is
>>good. If one builds a different kernel for each test machine, fixing kgdb
>>interface parameters during a build doesn't cause any problems. I don't think
>>compiling different kernels is good when you have more than 2 machines (of
>>same architecture). It's much easier to build a single kernel with drivers
>>required by all of them and then boot them with kgdb as and when required
>>with interface parameters coming from grug.conf (or equivalent on other
>>archs).
>
>
> 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.
>
>
>>kgdb_serial isn't ugly. It's just a function switch, similar to several of
>>them in the kernel. ppc is ugly, but that's anyway the case because of so
>>many varieties of ppc. If we are trying to make ppc code clean, it makes more
>>sense to move this weak function thing into ppc specific files IMHO.
>
>
> 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.
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
Tom Rini wrote:
> On Wed, Mar 03, 2004 at 04:51:06PM +0100, Pavel Machek wrote:
>
>>Hi!
>>
>>
>>>>>More precisely:
>>>>>http://lkml.org/lkml/2004/2/11/224
>>>>
>>>>Well, that just says Andrew does not care too much. I think that
>>>>having both serial and ethernet support *is* good idea after all... I
>>>>have few machines here, some of them do not have serial, and some of
>>>>them do not have supported ethernet. It would be nice to use same
>>>>kernel on all of them. Also distribution wants to have "debugging
>>>>kernel", but does _not_ want to have 10 of them.
>>>
>>>But unless I'm missing something, supporting eth or 8250 at all times
>>>doesn't work right now anyhow, as eth if available will always take over.
>>
>>Well, that can be fixed. [Probably if kgdbeth= is not passed, ethernet
>>interface should not take over. So user selects which one should be
>>used by either passing kgdbeth or kgdb8250. That means that 8250
>>should not be initialized until user passes kgdb8250=... not sure how
>>you'll like that].
>
>
> At this point, I'm going to give up on killing kgdb_serial, and pass
> along some comments from David Woodhouse on IRC as well (I was talking
> about this issue, and the init/main.c change):
> (Tartarus == me, dwmw2 == David Woodhouse)
>
> <Tartarus> dwmw2, the problem is how do you deal with all of the
> possibilities of i/o (8250, kgdboe, or other serial) and do you allow
> for passing 'gdb' on the command line to result in kgdb not being dropped
> into? You can always break in later on of course
> <dwmw2> parse command line early for 'gdb=' argument specifying which
> i/o device to use. init kgdb core early. init each i/o device as early
> as possible for that i/o device. Start the selected i/o device as soon
> as it becomes available.
> <dwmw2> just like console could, if we looked for console= a little bit
> earlier. (forget all the earlyconsole shite, it's not necessary)
> <dwmw2> Tartarrus, do the __early_setup() thing to replace __setup() for
> selected args. We can use that for console= too.
> <dwmw2> since 'console=' on the command line _already_ remembers its
> arguments, and starts to use the offending device as soon as it gets
> registered with register_console().
> <Tartarus> dwmw2, __early_setup() ?
> <dwmw2> See __setup("gdb=", gdb_setup_func);
> <dwmw2> Replace with __early_setup(...)
> <Tartarus> where is __early_setup ?
> <dwmw2> before we normally parse the command line
> <dwmw2> in my head
>
> So perhaps someone can take these ideas and fix both problems... :)
> (I've got some other stuff I need to work on today).
Well, __early_setup could mean the fist setup call and if so that would be what
we do in -mm. It is done by putting the code in the first module ld sees, not
nice, but it works.
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
On Wednesday 03 Mar 2004 8:46 pm, Tom Rini wrote:
> On Wed, Mar 03, 2004 at 11:13:02AM +0530, Amit S. Kale wrote:
> > On Wednesday 03 Mar 2004 5:16 am, George Anzinger wrote:
> > > Tom Rini wrote:
> > > > On Tue, Mar 02, 2004 at 11:31:43PM +0100, Pavel Machek wrote:
> > > >>Hi!
> > > >>
> > > >>>>Tom Rini wrote:
> > > >>>>>Hello. The following interdiff kills kgdb_serial in favor of
> > > >>>>> function names. This only adds a weak function for
> > > >>>>> kgdb_flush_io, and documents when it would need to be provided.
> > > >>>>
> > > >>>>It looks like you are also dumping any notion of building a kernel
> > > >>>> that can choose which method of communication to use for kgdb at
> > > >>>> run time. Is this so?
> > > >>>
> > > >>>Yes, as this is how Andrew suggested we do it. It becomes quite
> > > >>> ugly if you try and allow for any 2 of 3 methods.
> > > >>
> > > >>I do not think that having kgdb_serial is so ugly. Are there any
> > > >> other uglyness associated with that?
> > > >
> > > > More precisely:
> > > > http://lkml.org/lkml/2004/2/11/224
> > >
> > > Andrew seems to be comming from the point of view of a developer rather
> > > than a developer/ maintainer.
> > >
> > > So, the counter argument is the user who is sending the thing into the
> > > field and wants to send just one binary kernel to all locations. But
> > > then he needs to debug some problem that will work fine over the lan
> > > and later one that requires an early connection which the lan can not,
> > > as yet, do. I agree that for you or me, this is not an issue, but what
> > > of the IT folks...
> >
> > This is the same reason specifying 8250 parameters on a kernel command
> > line is good. If one builds a different kernel for each test machine,
> > fixing kgdb interface parameters during a build doesn't cause any
> > problems. I don't think compiling different kernels is good when you have
> > more than 2 machines (of same architecture). It's much easier to build a
> > single kernel with drivers required by all of them and then boot them
> > with kgdb as and when required with interface parameters coming from
> > grug.conf (or equivalent on other archs).
>
> 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 was just providing an anology.
Yes. With kgdb_serial, one can switch an interface on the fly.
>
> > kgdb_serial isn't ugly. It's just a function switch, similar to several
> > of them in the kernel. ppc is ugly, but that's anyway the case because of
> > so many varieties of ppc. If we are trying to make ppc code clean, it
> > makes more sense to move this weak function thing into ppc specific files
> > IMHO.
>
> 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.
We can still have one single hardcoded function pointer for ppc and manage the
rest in ppc specific files.
-Amit
On Thursday 04 Mar 2004 6:04 am, George Anzinger wrote:
> Tom Rini wrote:
> > On Wed, Mar 03, 2004 at 04:51:06PM +0100, Pavel Machek wrote:
> >>Hi!
> >>
> >>>>>More precisely:
> >>>>>http://lkml.org/lkml/2004/2/11/224
> >>>>
> >>>>Well, that just says Andrew does not care too much. I think that
> >>>>having both serial and ethernet support *is* good idea after all... I
> >>>>have few machines here, some of them do not have serial, and some of
> >>>>them do not have supported ethernet. It would be nice to use same
> >>>>kernel on all of them. Also distribution wants to have "debugging
> >>>>kernel", but does _not_ want to have 10 of them.
> >>>
> >>>But unless I'm missing something, supporting eth or 8250 at all times
> >>>doesn't work right now anyhow, as eth if available will always take
> >>> over.
> >>
> >>Well, that can be fixed. [Probably if kgdbeth= is not passed, ethernet
> >>interface should not take over. So user selects which one should be
> >>used by either passing kgdbeth or kgdb8250. That means that 8250
> >>should not be initialized until user passes kgdb8250=... not sure how
> >>you'll like that].
> >
> > At this point, I'm going to give up on killing kgdb_serial, and pass
> > along some comments from David Woodhouse on IRC as well (I was talking
> > about this issue, and the init/main.c change):
> > (Tartarus == me, dwmw2 == David Woodhouse)
> >
> > <Tartarus> dwmw2, the problem is how do you deal with all of the
> > possibilities of i/o (8250, kgdboe, or other serial) and do you allow
> > for passing 'gdb' on the command line to result in kgdb not being dropped
> > into? You can always break in later on of course
> > <dwmw2> parse command line early for 'gdb=' argument specifying which
> > i/o device to use. init kgdb core early. init each i/o device as early
> > as possible for that i/o device. Start the selected i/o device as soon
> > as it becomes available.
> > <dwmw2> just like console could, if we looked for console= a little bit
> > earlier. (forget all the earlyconsole shite, it's not necessary)
> > <dwmw2> Tartarrus, do the __early_setup() thing to replace __setup() for
> > selected args. We can use that for console= too.
> > <dwmw2> since 'console=' on the command line _already_ remembers its
> > arguments, and starts to use the offending device as soon as it gets
> > registered with register_console().
> > <Tartarus> dwmw2, __early_setup() ?
> > <dwmw2> See __setup("gdb=", gdb_setup_func);
> > <dwmw2> Replace with __early_setup(...)
> > <Tartarus> where is __early_setup ?
> > <dwmw2> before we normally parse the command line
> > <dwmw2> in my head
> >
> > So perhaps someone can take these ideas and fix both problems... :)
> > (I've got some other stuff I need to work on today).
>
> Well, __early_setup could mean the fist setup call and if so that would be
> what we do in -mm. It is done by putting the code in the first module ld
> sees, not nice, but it works.
I would prefer something that modifies start_kernel itself, rather than
depending on ld. It will split start_kernel command line parsing into early
parse and late parse, but that's the price we have to pay to do special
parsing of kgdb arguments.
--
Amit Kale
EmSysSoft (http://www.emsyssoft.com)
KGDB: Linux Kernel Source Level Debugger (http://kgdb.sourceforge.net)
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.
> >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
--
Tom Rini
http://gate.crashing.org/~trini/
On Thu, Mar 04, 2004 at 10:11:39AM +0530, Amit S. Kale wrote:
> On Wednesday 03 Mar 2004 8:46 pm, Tom Rini wrote:
> > On Wed, Mar 03, 2004 at 11:13:02AM +0530, Amit S. Kale wrote:
[snip]
> > > kgdb_serial isn't ugly. It's just a function switch, similar to several
> > > of them in the kernel. ppc is ugly, but that's anyway the case because of
> > > so many varieties of ppc. If we are trying to make ppc code clean, it
> > > makes more sense to move this weak function thing into ppc specific files
> > > IMHO.
> >
> > 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.
>
> We can still have one single hardcoded function pointer for ppc and manage
> the rest in ppc specific files.
I think you're still missing the point.
Regardless, the solution to this is what dwmw2 suggested on IRC I
believe, as this should remove all of the #ifdef mess.
--
Tom Rini
http://gate.crashing.org/~trini/
Amit S. Kale wrote:
> On Thursday 04 Mar 2004 6:04 am, George Anzinger wrote:
>
>>Tom Rini wrote:
>>
>>>On Wed, Mar 03, 2004 at 04:51:06PM +0100, Pavel Machek wrote:
>>>
>>>>Hi!
>>>>
>>>>
>>>>>>>More precisely:
>>>>>>>http://lkml.org/lkml/2004/2/11/224
>>>>>>
>>>>>>Well, that just says Andrew does not care too much. I think that
>>>>>>having both serial and ethernet support *is* good idea after all... I
>>>>>>have few machines here, some of them do not have serial, and some of
>>>>>>them do not have supported ethernet. It would be nice to use same
>>>>>>kernel on all of them. Also distribution wants to have "debugging
>>>>>>kernel", but does _not_ want to have 10 of them.
>>>>>
>>>>>But unless I'm missing something, supporting eth or 8250 at all times
>>>>>doesn't work right now anyhow, as eth if available will always take
>>>>>over.
>>>>
>>>>Well, that can be fixed. [Probably if kgdbeth= is not passed, ethernet
>>>>interface should not take over. So user selects which one should be
>>>>used by either passing kgdbeth or kgdb8250. That means that 8250
>>>>should not be initialized until user passes kgdb8250=... not sure how
>>>>you'll like that].
>>>
>>>At this point, I'm going to give up on killing kgdb_serial, and pass
>>>along some comments from David Woodhouse on IRC as well (I was talking
>>>about this issue, and the init/main.c change):
>>>(Tartarus == me, dwmw2 == David Woodhouse)
>>>
>>><Tartarus> dwmw2, the problem is how do you deal with all of the
>>>possibilities of i/o (8250, kgdboe, or other serial) and do you allow
>>>for passing 'gdb' on the command line to result in kgdb not being dropped
>>>into? You can always break in later on of course
>>><dwmw2> parse command line early for 'gdb=' argument specifying which
>>>i/o device to use. init kgdb core early. init each i/o device as early
>>>as possible for that i/o device. Start the selected i/o device as soon
>>>as it becomes available.
>>><dwmw2> just like console could, if we looked for console= a little bit
>>>earlier. (forget all the earlyconsole shite, it's not necessary)
>>><dwmw2> Tartarrus, do the __early_setup() thing to replace __setup() for
>>>selected args. We can use that for console= too.
>>><dwmw2> since 'console=' on the command line _already_ remembers its
>>>arguments, and starts to use the offending device as soon as it gets
>>>registered with register_console().
>>><Tartarus> dwmw2, __early_setup() ?
>>><dwmw2> See __setup("gdb=", gdb_setup_func);
>>><dwmw2> Replace with __early_setup(...)
>>><Tartarus> where is __early_setup ?
>>><dwmw2> before we normally parse the command line
>>><dwmw2> in my head
>>>
>>>So perhaps someone can take these ideas and fix both problems... :)
>>>(I've got some other stuff I need to work on today).
>>
>>Well, __early_setup could mean the fist setup call and if so that would be
>>what we do in -mm. It is done by putting the code in the first module ld
>>sees, not nice, but it works.
>
>
> I would prefer something that modifies start_kernel itself, rather than
> depending on ld. It will split start_kernel command line parsing into early
> parse and late parse, but that's the price we have to pay to do special
> parsing of kgdb arguments.
There was some talk along these lines at one point, I don't think anything came
of it. We would like it to be the very first, not just one of the first.
I think modifying the kernel to support this for kgdb is more like the tail
wagging the dog :)
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
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 [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
On Thu, Mar 04, 2004 at 10:31:00AM +0530, Amit S. Kale wrote:
> On Thursday 04 Mar 2004 6:04 am, George Anzinger wrote:
> > Tom Rini wrote:
> > > On Wed, Mar 03, 2004 at 04:51:06PM +0100, Pavel Machek wrote:
> > >>Hi!
> > >>
> > >>>>>More precisely:
> > >>>>>http://lkml.org/lkml/2004/2/11/224
> > >>>>
> > >>>>Well, that just says Andrew does not care too much. I think that
> > >>>>having both serial and ethernet support *is* good idea after all... I
> > >>>>have few machines here, some of them do not have serial, and some of
> > >>>>them do not have supported ethernet. It would be nice to use same
> > >>>>kernel on all of them. Also distribution wants to have "debugging
> > >>>>kernel", but does _not_ want to have 10 of them.
> > >>>
> > >>>But unless I'm missing something, supporting eth or 8250 at all times
> > >>>doesn't work right now anyhow, as eth if available will always take
> > >>> over.
> > >>
> > >>Well, that can be fixed. [Probably if kgdbeth= is not passed, ethernet
> > >>interface should not take over. So user selects which one should be
> > >>used by either passing kgdbeth or kgdb8250. That means that 8250
> > >>should not be initialized until user passes kgdb8250=... not sure how
> > >>you'll like that].
> > >
> > > At this point, I'm going to give up on killing kgdb_serial, and pass
> > > along some comments from David Woodhouse on IRC as well (I was talking
> > > about this issue, and the init/main.c change):
> > > (Tartarus == me, dwmw2 == David Woodhouse)
> > >
> > > <Tartarus> dwmw2, the problem is how do you deal with all of the
> > > possibilities of i/o (8250, kgdboe, or other serial) and do you allow
> > > for passing 'gdb' on the command line to result in kgdb not being dropped
> > > into? You can always break in later on of course
> > > <dwmw2> parse command line early for 'gdb=' argument specifying which
> > > i/o device to use. init kgdb core early. init each i/o device as early
> > > as possible for that i/o device. Start the selected i/o device as soon
> > > as it becomes available.
> > > <dwmw2> just like console could, if we looked for console= a little bit
> > > earlier. (forget all the earlyconsole shite, it's not necessary)
> > > <dwmw2> Tartarrus, do the __early_setup() thing to replace __setup() for
> > > selected args. We can use that for console= too.
> > > <dwmw2> since 'console=' on the command line _already_ remembers its
> > > arguments, and starts to use the offending device as soon as it gets
> > > registered with register_console().
> > > <Tartarus> dwmw2, __early_setup() ?
> > > <dwmw2> See __setup("gdb=", gdb_setup_func);
> > > <dwmw2> Replace with __early_setup(...)
> > > <Tartarus> where is __early_setup ?
> > > <dwmw2> before we normally parse the command line
> > > <dwmw2> in my head
> > >
> > > So perhaps someone can take these ideas and fix both problems... :)
> > > (I've got some other stuff I need to work on today).
> >
> > Well, __early_setup could mean the fist setup call and if so that would be
> > what we do in -mm. It is done by putting the code in the first module ld
> > sees, not nice, but it works.
>
> I would prefer something that modifies start_kernel itself, rather than
> depending on ld. It will split start_kernel command line parsing into early
> parse and late parse, but that's the price we have to pay to do special
> parsing of kgdb arguments.
There's two things here, that it's important not to loose sight of.
First, it's not just kgdb that could stand to parse things earlier on
(console, as dwmw2 points out). Second, unless we're going to throw in
the initalizations of the i/o drivers manually, we're already depend on
ld to do things right.
Besides, this is probably going to be the cleanest way to get things
working like we want them to, not how they work now :)
--
Tom Rini
http://gate.crashing.org/~trini/
On Thu, Mar 04, 2004 at 02:18:08PM -0800, George Anzinger wrote:
> 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.
Ah yes, as a side effect of me being a bit too quick with my
kernel/Kconfig.kgdb changes, yes. Good catch :)
> >>>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:
I would rather not have to duplicate the above code (or, any code) for
every arch. :)
> #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,...}}
But eth_read_char, etc aren't, don't need to be, and shouldn't be global
functions.
> 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.
I think this is backwards. Stop thinking in terms of 8250, eth and arch
serial, but instead io1, io2 and io3 (and io4, and io5, ...). I don't
see what's wrong with what dwmw2 suggested that I passed along.
--
Tom Rini
http://gate.crashing.org/~trini/
Tom Rini wrote:
> On Thu, Mar 04, 2004 at 10:11:39AM +0530, Amit S. Kale wrote:
>
>>On Wednesday 03 Mar 2004 8:46 pm, Tom Rini wrote:
>>
>>>On Wed, Mar 03, 2004 at 11:13:02AM +0530, Amit S. Kale wrote:
>
> [snip]
>
>>>>kgdb_serial isn't ugly. It's just a function switch, similar to several
>>>>of them in the kernel. ppc is ugly, but that's anyway the case because of
>>>>so many varieties of ppc. If we are trying to make ppc code clean, it
>>>>makes more sense to move this weak function thing into ppc specific files
>>>>IMHO.
>>>
>>>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.
>>
>>We can still have one single hardcoded function pointer for ppc and manage
>>the rest in ppc specific files.
>
>
> I think you're still missing the point.
>
> Regardless, the solution to this is what dwmw2 suggested on IRC I
> believe, as this should remove all of the #ifdef mess.
I am afraid I don't quite understand what he was saying other than early init
stuff. On of the problems with trying early init stuff, by the way, is that a
lot of things depend on having alloc up and that happens rather late in the game.
But back to the method. Is he/are you suggesting that the init code plug the
array of functions into the kgdb table? This could be done by providing a
register function in kgdb to register an i/o method. Pass it a pointer to a
struc of entry points, keep the pointers in an array, etc. All that is left is
to define the default in some simple and clean way, a way that might be
overridden at command line parse time and so on.
I haven't looked at the latest version of the kgdb serial code, but it could,
rather easily, be set up to initiialize on first call (if it isn't now), so for
it, and others that can do likewise we could register the method at compile time.
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
On Thu, Mar 04, 2004 at 03:06:34PM -0800, George Anzinger wrote:
> Tom Rini wrote:
> >On Thu, Mar 04, 2004 at 10:11:39AM +0530, Amit S. Kale wrote:
> >
> >>On Wednesday 03 Mar 2004 8:46 pm, Tom Rini wrote:
> >>
> >>>On Wed, Mar 03, 2004 at 11:13:02AM +0530, Amit S. Kale wrote:
> >
> >[snip]
> >
> >>>>kgdb_serial isn't ugly. It's just a function switch, similar to several
> >>>>of them in the kernel. ppc is ugly, but that's anyway the case because
> >>>>of
> >>>>so many varieties of ppc. If we are trying to make ppc code clean, it
> >>>>makes more sense to move this weak function thing into ppc specific
> >>>>files
> >>>>IMHO.
> >>>
> >>>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.
> >>
> >>We can still have one single hardcoded function pointer for ppc and manage
> >>the rest in ppc specific files.
> >
> >
> >I think you're still missing the point.
> >
> >Regardless, the solution to this is what dwmw2 suggested on IRC I
> >believe, as this should remove all of the #ifdef mess.
>
> I am afraid I don't quite understand what he was saying other than early
> init stuff. On of the problems with trying early init stuff, by the way,
> is that a lot of things depend on having alloc up and that happens rather
> late in the game.
I assume you aren't talking about kgdb stuff here (or what would be the
point of going so early) but I believe he was talking about allowing for
stuff that could be done early, to be done early.
> But back to the method. Is he/are you suggesting that the init code plug
> the array of functions into the kgdb table? This could be done by
> providing a register function in kgdb to register an i/o method. Pass it a
> pointer to a struc of entry points, keep the pointers in an array, etc.
> All that is left is to define the default in some simple and clean way, a
> way that might be overridden at command line parse time and so on.
What I'm suggesting is that we don't need this table stuff at all.
Roughly:
- Call kgdb_entry(), minus the kgdb_serial->hook(), as early as
possible.
- We do early parsing of some cmd opts (this I believe is __early_setup)
- This means that to use kgdboe you _must_ pass kgdboe=whatever, same
for 8250, etc. This would also do kgdb_serial = kgdboe_serial, or
whatever.
- 8250, kgdboe, etc, must be able to init themselves.
- If ('gdb' || 'kgdbwait') && kgdb_serial != NULL -> breakpoint();
--
Tom Rini
http://gate.crashing.org/~trini/
Tom Rini wrote:
~
>>I am afraid I don't quite understand what he was saying other than early
>>init stuff. On of the problems with trying early init stuff, by the way,
>>is that a lot of things depend on having alloc up and that happens rather
>>late in the game.
>
>
> I assume you aren't talking about kgdb stuff here (or what would be the
> point of going so early) but I believe he was talking about allowing for
> stuff that could be done early, to be done early.
One of the issues with the UART set up is registering the interrupt handler with
the kernel. It will fail if alloc is not up. The -mm patch does two things
with this. a) It tries every getchar to register the interrupt handler, and b)
it has a module init entry to register it. This last will happen late in the
bring up and is safe. a) is there to get it ASAP if you are actually using kgdb
during the bring up.
>
>
~
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
On Thu, Mar 11, 2004 at 01:33:40PM -0800, George Anzinger wrote:
> Tom Rini wrote:
>
> >>I am afraid I don't quite understand what he was saying other than early
> >>init stuff. On of the problems with trying early init stuff, by the way,
> >>is that a lot of things depend on having alloc up and that happens rather
> >>late in the game.
> >
> >
> >I assume you aren't talking about kgdb stuff here (or what would be the
> >point of going so early) but I believe he was talking about allowing for
> >stuff that could be done early, to be done early.
>
> One of the issues with the UART set up is registering the interrupt handler
> with the kernel. It will fail if alloc is not up. The -mm patch does two
> things with this. a) It tries every getchar to register the interrupt
> handler, and b) it has a module init entry to register it. This last will
> happen late in the bring up and is safe. a) is there to get it ASAP if you
> are actually using kgdb during the bring up.
There's two ways to look at this.
- All the more reason to acknowledge that the earliest you can safely
get into KGDB is point X, where X is where alloc works, mappings done
if needed, etc, etc, and IFF we change things slightly in kgdboe so
that it can call kgdb_schedule_breakpoint() if it needs to as an
initial break, and handle setting kgdb_serial to the serial driver in
kgdb_arch_init, or something, and remove all of the extra kludges to
get us a few lines / function calls earlier on.
- More and more special cases.
Roughly. :)
--
Tom Rini
http://gate.crashing.org/~trini/
Tom Rini wrote:
> On Thu, Mar 11, 2004 at 01:33:40PM -0800, George Anzinger wrote:
>
>
>>Tom Rini wrote:
>>
>>
>>>>I am afraid I don't quite understand what he was saying other than early
>>>>init stuff. On of the problems with trying early init stuff, by the way,
>>>>is that a lot of things depend on having alloc up and that happens rather
>>>>late in the game.
>>>
>>>
>>>I assume you aren't talking about kgdb stuff here (or what would be the
>>>point of going so early) but I believe he was talking about allowing for
>>>stuff that could be done early, to be done early.
>>
>>One of the issues with the UART set up is registering the interrupt handler
>>with the kernel. It will fail if alloc is not up. The -mm patch does two
>>things with this. a) It tries every getchar to register the interrupt
>>handler, and b) it has a module init entry to register it. This last will
>>happen late in the bring up and is safe. a) is there to get it ASAP if you
>>are actually using kgdb during the bring up.
>
>
> There's two ways to look at this.
> - All the more reason to acknowledge that the earliest you can safely
> get into KGDB is point X, where X is where alloc works,
Just to get ^C to work? I would rather give it up entirely!
mappings done
> if needed, etc, etc, and IFF we change things slightly in kgdboe so
> that it can call kgdb_schedule_breakpoint() if it needs to as an
> initial break, and handle setting kgdb_serial to the serial driver in
> kgdb_arch_init, or something, and remove all of the extra kludges to
> get us a few lines / function calls earlier on.
> - More and more special cases.
How about a command line set up ASAP which calls a driver entry to do the break.
The driver being serial does it NOW, but being some thing that needs
additional resources, just sets a flag to break when it gets them and returns.
Seems rather simple.
>
> Roughly. :)
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
On Thu, Mar 11, 2004 at 02:53:34PM -0800, George Anzinger wrote:
> Tom Rini wrote:
> >On Thu, Mar 11, 2004 at 01:33:40PM -0800, George Anzinger wrote:
> >>Tom Rini wrote:
> >>>>I am afraid I don't quite understand what he was saying other than
> >>>>early init stuff. On of the problems with trying early init stuff, by
> >>>>the way, is that a lot of things depend on having alloc up and that
> >>>>happens rather late in the game.
> >>>
> >>>I assume you aren't talking about kgdb stuff here (or what would be the
> >>>point of going so early) but I believe he was talking about allowing for
> >>>stuff that could be done early, to be done early.
> >>
> >>One of the issues with the UART set up is registering the interrupt
> >>handler with the kernel. It will fail if alloc is not up. The -mm patch
> >>does two things with this. a) It tries every getchar to register the
> >>interrupt handler, and b) it has a module init entry to register it.
> >>This last will happen late in the bring up and is safe. a) is there to
> >>get it ASAP if you are actually using kgdb during the bring up.
> >
> >
> >There's two ways to look at this.
> >- All the more reason to acknowledge that the earliest you can safely
> > get into KGDB is point X, where X is where alloc works,
>
> Just to get ^C to work? I would rather give it up entirely!
> > mappings done
> > if needed, etc, etc, and IFF we change things slightly in kgdboe so
> > that it can call kgdb_schedule_breakpoint() if it needs to as an
> > initial break, and handle setting kgdb_serial to the serial driver in
> > kgdb_arch_init, or something, and remove all of the extra kludges to
> > get us a few lines / function calls earlier on.
> >- More and more special cases.
>
> How about a command line set up ASAP which calls a driver entry to do the
> break. The driver being serial does it NOW, but being some thing that
> needs additional resources, just sets a flag to break when it gets them and
> returns. Seems rather simple.
This sounds a lot like what I passed along from dwmw2 a week ago. :)
--
Tom Rini
http://gate.crashing.org/~trini/
Tom Rini wrote:
> On Thu, Mar 11, 2004 at 02:53:34PM -0800, George Anzinger wrote:
>
>
>>Tom Rini wrote:
>>
>>>On Thu, Mar 11, 2004 at 01:33:40PM -0800, George Anzinger wrote:
>>>
>>>>Tom Rini wrote:
>>>>
>>>>>>I am afraid I don't quite understand what he was saying other than
>>>>>>early init stuff. On of the problems with trying early init stuff, by
>>>>>>the way, is that a lot of things depend on having alloc up and that
>>>>>>happens rather late in the game.
>>>>>
>>>>>I assume you aren't talking about kgdb stuff here (or what would be the
>>>>>point of going so early) but I believe he was talking about allowing for
>>>>>stuff that could be done early, to be done early.
>>>>
>>>>One of the issues with the UART set up is registering the interrupt
>>>>handler with the kernel. It will fail if alloc is not up. The -mm patch
>>>>does two things with this. a) It tries every getchar to register the
>>>>interrupt handler, and b) it has a module init entry to register it.
>>>>This last will happen late in the bring up and is safe. a) is there to
>>>>get it ASAP if you are actually using kgdb during the bring up.
>>>
>>>
>>>There's two ways to look at this.
>>>- All the more reason to acknowledge that the earliest you can safely
>>> get into KGDB is point X, where X is where alloc works,
>>
>>Just to get ^C to work? I would rather give it up entirely!
>>
>>> mappings done
>>> if needed, etc, etc, and IFF we change things slightly in kgdboe so
>>> that it can call kgdb_schedule_breakpoint() if it needs to as an
>>> initial break, and handle setting kgdb_serial to the serial driver in
>>> kgdb_arch_init, or something, and remove all of the extra kludges to
>>> get us a few lines / function calls earlier on.
>>>- More and more special cases.
>>
>>How about a command line set up ASAP which calls a driver entry to do the
>>break. The driver being serial does it NOW, but being some thing that
>> needs additional resources, just sets a flag to break when it gets them and
>>returns. Seems rather simple.
>
>
> This sounds a lot like what I passed along from dwmw2 a week ago. :)
Must be something wrong here. We seem to be agreeing :)
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml
On Friday 12 Mar 2004 4:23 am, George Anzinger wrote:
> Tom Rini wrote:
> > On Thu, Mar 11, 2004 at 01:33:40PM -0800, George Anzinger wrote:
> >>Tom Rini wrote:
> >>>>I am afraid I don't quite understand what he was saying other than
> >>>> early init stuff. On of the problems with trying early init stuff, by
> >>>> the way, is that a lot of things depend on having alloc up and that
> >>>> happens rather late in the game.
> >>>
> >>>I assume you aren't talking about kgdb stuff here (or what would be the
> >>>point of going so early) but I believe he was talking about allowing for
> >>>stuff that could be done early, to be done early.
> >>
> >>One of the issues with the UART set up is registering the interrupt
> >> handler with the kernel. It will fail if alloc is not up. The -mm
> >> patch does two things with this. a) It tries every getchar to register
> >> the interrupt handler, and b) it has a module init entry to register it.
> >> This last will happen late in the bring up and is safe. a) is there to
> >> get it ASAP if you are actually using kgdb during the bring up.
> >
> > There's two ways to look at this.
> > - All the more reason to acknowledge that the earliest you can safely
> > get into KGDB is point X, where X is where alloc works,
>
> Just to get ^C to work? I would rather give it up entirely!
> mappings done
>
> > if needed, etc, etc, and IFF we change things slightly in kgdboe so
> > that it can call kgdb_schedule_breakpoint() if it needs to as an
> > initial break, and handle setting kgdb_serial to the serial driver in
> > kgdb_arch_init, or something, and remove all of the extra kludges to
> > get us a few lines / function calls earlier on.
> > - More and more special cases.
>
> How about a command line set up ASAP which calls a driver entry to do the
> break. The driver being serial does it NOW, but being some thing that needs
> additional resources, just sets a flag to break when it gets them and
> returns. Seems rather simple.
Sounds good. We have already agreed the need for early command line argument
processing in another thread.
Should do this as soon as lite version goes into the kernel.
-Amit
On Friday 12 Mar 2004 5:16 am, George Anzinger wrote:
> Tom Rini wrote:
> > On Thu, Mar 11, 2004 at 02:53:34PM -0800, George Anzinger wrote:
> >>Tom Rini wrote:
> >>>On Thu, Mar 11, 2004 at 01:33:40PM -0800, George Anzinger wrote:
> >>>>Tom Rini wrote:
> >>>>>>I am afraid I don't quite understand what he was saying other than
> >>>>>>early init stuff. On of the problems with trying early init stuff,
> >>>>>> by the way, is that a lot of things depend on having alloc up and
> >>>>>> that happens rather late in the game.
> >>>>>
> >>>>>I assume you aren't talking about kgdb stuff here (or what would be
> >>>>> the point of going so early) but I believe he was talking about
> >>>>> allowing for stuff that could be done early, to be done early.
> >>>>
> >>>>One of the issues with the UART set up is registering the interrupt
> >>>>handler with the kernel. It will fail if alloc is not up. The -mm
> >>>> patch does two things with this. a) It tries every getchar to
> >>>> register the interrupt handler, and b) it has a module init entry to
> >>>> register it. This last will happen late in the bring up and is safe.
> >>>> a) is there to get it ASAP if you are actually using kgdb during the
> >>>> bring up.
> >>>
> >>>There's two ways to look at this.
> >>>- All the more reason to acknowledge that the earliest you can safely
> >>> get into KGDB is point X, where X is where alloc works,
> >>
> >>Just to get ^C to work? I would rather give it up entirely!
> >>
> >>> mappings done
> >>> if needed, etc, etc, and IFF we change things slightly in kgdboe so
> >>> that it can call kgdb_schedule_breakpoint() if it needs to as an
> >>> initial break, and handle setting kgdb_serial to the serial driver in
> >>> kgdb_arch_init, or something, and remove all of the extra kludges to
> >>> get us a few lines / function calls earlier on.
> >>>- More and more special cases.
> >>
> >>How about a command line set up ASAP which calls a driver entry to do the
> >>break. The driver being serial does it NOW, but being some thing that
> >> needs additional resources, just sets a flag to break when it gets them
> >> and returns. Seems rather simple.
> >
> > This sounds a lot like what I passed along from dwmw2 a week ago. :)
>
> Must be something wrong here. We seem to be agreeing :)
You bet!
-Amit