[PATCH 4/5] KGDB-8250: refactor configuration

[PATCH 4/5] KGDB-8250: refactor configuration

[Jan Kiszka]

This major refactoring of the quite complex kgdb8250 configuration does
the following:

 - ensures that static configurations according to SERIAL_PORT_DFNS are
   always loaded first
 - tries to pull more accurate configuration via serial8250_get_port_def
   if simple-config is used
 - detects empty/invalid simple-configs
 - enforces KGDB_PORT_NUM <= SERIAL_8250_NR_UARTS at kconfig level
 - removes kgdb8250_add_port and its hook in serial_core (calling
   serial8250_get_port_def in demand should provide us the same
   information)

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>

---
 drivers/serial/8250.c        |   47 ++++++++++++----------------
 drivers/serial/8250_kgdb.c   |   72 +++++++++++++------------------------------
 drivers/serial/serial_core.c |    7 ----
 include/linux/kgdb.h         |    1 
 lib/Kconfig.kgdb             |   16 ++++-----
 5 files changed, 52 insertions(+), 91 deletions(-)

Index: b/drivers/serial/8250_kgdb.c
===================================================================
--- a/drivers/serial/8250_kgdb.c
+++ b/drivers/serial/8250_kgdb.c
@@ -53,16 +53,6 @@ static char kgdb8250_buf[GDB_BUF_SIZE];
 static atomic_t kgdb8250_buf_in_cnt;
 static int kgdb8250_buf_out_inx;
 
-/* Old-style serial definitions, if existant, and a counter. */
-#ifdef CONFIG_KGDB_SIMPLE_SERIAL
-static int should_copy_rs_table = 1;
-static struct serial_state old_rs_table[] __initdata = {
-#ifdef SERIAL_PORT_DFNS
-	SERIAL_PORT_DFNS
-#endif
-};
-#endif
-
 /* Our internal table of UARTS. */
 #define UART_NR	CONFIG_SERIAL_8250_NR_UARTS
 static struct uart_port kgdb8250_ports[UART_NR];
@@ -255,12 +245,18 @@ static int kgdb8250_uart_init(void)
  * had values specifically configured in.  We need to make sure this only
  * happens once.
  */
-static void __init kgdb8250_copy_rs_table(void)
+static void kgdb8250_copy_rs_table(void)
 {
 #ifdef CONFIG_KGDB_SIMPLE_SERIAL
+	static struct serial_state old_rs_table[] = {
+#ifdef SERIAL_PORT_DFNS
+		SERIAL_PORT_DFNS
+#endif
+	};
+	static int rs_table_copied;
 	int i;
 
-	if (!should_copy_rs_table)
+	if (rs_table_copied)
 		return;
 
 	for (i = 0; i < ARRAY_SIZE(old_rs_table); i++) {
@@ -273,8 +269,8 @@ static void __init kgdb8250_copy_rs_tabl
 		kgdb8250_ports[i].line = i;
 	}
 
-	should_copy_rs_table = 0;
-#endif
+	rs_table_copied = 1;
+#endif /* CONFIG_KGDB_SIMPLE_SERIAL */
 }
 
 /*
@@ -284,9 +280,6 @@ static void __init kgdb8250_copy_rs_tabl
  */
 static void __init kgdb8250_late_init(void)
 {
-	/* Try and copy the old_rs_table. */
-	kgdb8250_copy_rs_table();
-
 #if defined(CONFIG_SERIAL_8250) || defined(CONFIG_SERIAL_8250_MODULE)
 	/* Take the port away from the main driver. */
 	serial8250_unregister_by_port(current_port);
@@ -306,9 +299,6 @@ static void __init kgdb8250_late_init(vo
 
 static __init int kgdb_init_io(void)
 {
-	/* Give us the basic table of uarts. */
-	kgdb8250_copy_rs_table();
-
 	/* We're either a module and parse a config string, or we have a
 	 * semi-static config. */
 #ifdef CONFIG_KGDB_8250_MODULE
@@ -325,8 +315,15 @@ static __init int kgdb_init_io(void)
 #else  /* !CONFIG_KGDB_8250_MODULE */
 	if (!params_evaluated) {
 #ifdef CONFIG_KGDB_SIMPLE_SERIAL
+		kgdb8250_copy_rs_table();
+
 		kgdb8250_baud = CONFIG_KGDB_BAUDRATE;
 		current_port = &kgdb8250_ports[CONFIG_KGDB_PORT_NUM];
+
+		if (serial8250_get_port_def(current_port,
+					    CONFIG_KGDB_PORT_NUM) < 0 &&
+		    !current_port->iobase && !current_port->membase)
+			return -EINVAL;
 #else /* !CONFIG_KGDB_SIMPLE_SERIAL */
 		if (kgdb8250_opt(CONFIG_KGDB_8250_CONF_STRING))
 			return -EINVAL;
@@ -395,29 +392,6 @@ struct kgdb_io kgdb_io_ops = {
 };
 
 /**
- * 	kgdb8250_add_port - Define a serial port for use with KGDB
- * 	@i: The index of the port being added
- * 	@serial_req: The &struct uart_port describing the port
- *
- * 	On platforms where we must register the serial device
- * 	dynamically, this is the best option if a platform also normally
- * 	calls early_serial_setup().
- */
-void kgdb8250_add_port(int i, struct uart_port *serial_req)
-{
-#ifdef CONFIG_KGDB_SIMPLE_SERIAL
-	if (should_copy_rs_table)
-		printk(KERN_ERR "8250_kgdb: warning will over write serial"
-			   " port definitions at kgdb init time\n");
-#endif
-
-	/* Copy the whole thing over. */
-	if (current_port != &kgdb8250_ports[i])
-		memcpy(&kgdb8250_ports[i], serial_req,
-		sizeof(struct uart_port));
-}
-
-/**
  * 	kgdb8250_add_platform_port - Define a serial port for use with KGDB
  * 	@i: The index of the port being added
  * 	@p: The &struct plat_serial8250_port describing the port
@@ -447,7 +421,10 @@ void __init kgdb8250_add_platform_port(i
  */
 static int __init kgdb8250_opt(char *str)
 {
-	/* We'll fill out and use the first slot. */
+	/* Give us the basic table of uarts. */
+	kgdb8250_copy_rs_table();
+
+	/* If we overwrite, we'll fill out and use the first slot. */
 	current_port = &kgdb8250_ports[0];
 
 	if (!strncmp(str, "io", 2)) {
@@ -467,7 +444,8 @@ static int __init kgdb8250_opt(char *str
 		if (line >= UART_NR)
 			goto errout;
 		current_port = &kgdb8250_ports[line];
-		if (serial8250_get_port_def(current_port, line))
+		if (serial8250_get_port_def(current_port, line) < 0 &&
+		    !current_port->iobase && !current_port->membase)
 			goto errout;
 		str++;
 		if (*str != ',')
@@ -512,16 +490,12 @@ static int __init kgdb8250_opt(char *str
 	current_port->irq = simple_strtoul(str, &str, 10);
 
 finish:
-#ifdef CONFIG_KGDB_SIMPLE_SERIAL
-	should_copy_rs_table = 0;
-#endif
 #ifdef CONFIG_KGDB_8250
 	params_evaluated = 1;
 #endif
 	return 0;
 
 errout:
-	printk(KERN_ERR "Invalid syntax for option kgdb8250=\n");
 	return 1;
 }
 
Index: b/drivers/serial/8250.c
===================================================================
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2860,37 +2860,32 @@ void serial8250_unregister_port(int line
 EXPORT_SYMBOL(serial8250_unregister_port);
 
 /**
- *  serial8250_get_port_def - Get port definition for a specific line
- *  @port: generic uart_port output for a specific serial line
- *  @line: specific serial line index
+ *	serial8250_get_port_def - Get port definition for a specific line
+ *	@port: generic uart_port output for a specific serial line
+ *	@line: specific serial line index
  *
- *  Return 0 if the port existed
- *  Return 1 on failure
+ *	Returns 0 on success, -ENODEV if the port does not exist.
  */
-
 int serial8250_get_port_def(struct uart_port *port, int line)
 {
-	struct uart_8250_port *uart;
+	struct uart_port *port8250 = &serial8250_ports[line].port;
 
-	if (line < 0 || line >= UART_NR)
-		return 1;
-	uart = &serial8250_ports[line];
-	if (uart) {
-		port->iobase = uart->port.iobase;
-		port->membase = uart->port.membase;
-		port->irq = uart->port.irq;
-		port->uartclk = uart->port.uartclk;
-		port->fifosize = uart->port.fifosize;
-		port->regshift = uart->port.regshift;
-		port->iotype = uart->port.iotype;
-		port->flags = uart->port.flags;
-		port->mapbase = uart->port.mapbase;
-		port->dev =	uart->port.dev;
-		return 0;
-	}
-	return 1;
+	if (!port8250->iobase && !port8250->membase)
+		return -ENODEV;
+
+	port->iobase   = port8250->iobase;
+	port->membase  = port8250->membase;
+	port->irq      = port8250->irq;
+	port->uartclk  = port8250->uartclk;
+	port->fifosize = port8250->fifosize;
+	port->regshift = port8250->regshift;
+	port->iotype   = port8250->iotype;
+	port->flags    = port8250->flags;
+	port->mapbase  = port8250->mapbase;
+	port->dev      = port8250->dev;
+	return 0;
 }
-EXPORT_SYMBOL(serial8250_get_port_def);
+EXPORT_SYMBOL_GPL(serial8250_get_port_def);
 
 /**
  *	serial8250_unregister_by_port - remove a 16x50 serial port
@@ -2909,7 +2904,7 @@ void serial8250_unregister_by_port(struc
 	if (uart)
 		serial8250_unregister_port(uart->port.line);
 }
-EXPORT_SYMBOL(serial8250_unregister_by_port);
+EXPORT_SYMBOL_GPL(serial8250_unregister_by_port);
 
 static int __init serial8250_init(void)
 {
Index: b/drivers/serial/serial_core.c
===================================================================
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -33,7 +33,6 @@
 #include <linux/serial.h> /* for serial_state and serial_icounter_struct */
 #include <linux/delay.h>
 #include <linux/mutex.h>
-#include <linux/kgdb.h>
 
 #include <asm/irq.h>
 #include <asm/uaccess.h>
@@ -2370,12 +2369,6 @@ int uart_add_one_port(struct uart_driver
 	 */
 	port->flags &= ~UPF_DEAD;
 
-#if defined(CONFIG_KGDB_8250)
-	/* Add any 8250-like ports we find later. */
-	if (port->type <= PORT_MAX_8250)
-		kgdb8250_add_port(port->line, port);
-#endif
-
  out:
 	mutex_unlock(&state->mutex);
 	mutex_unlock(&port_mutex);
Index: b/include/linux/kgdb.h
===================================================================
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -271,7 +271,6 @@ extern struct kgdb_arch arch_kgdb_ops;
 int kgdb_register_io_module(struct kgdb_io *local_kgdb_io_ops);
 void kgdb_unregister_io_module(struct kgdb_io *local_kgdb_io_ops);
 
-void kgdb8250_add_port(int i, struct uart_port *serial_req);
 void __init kgdb8250_add_platform_port(int i,
 	struct plat_serial8250_port *serial_req);
 
Index: b/lib/Kconfig.kgdb
===================================================================
--- a/lib/Kconfig.kgdb
+++ b/lib/Kconfig.kgdb
@@ -36,6 +36,14 @@ config KGDB_SIMPLE_SERIAL
 	  or on the command line, the type (I/O or MMIO), IRQ and
 	  address to use.  If in doubt, say Y.
 
+config KGDB_PORT_NUM
+	int "Serial port number for KGDB"
+	range 0 SERIAL_8250_NR_UARTS
+	depends on KGDB_SIMPLE_SERIAL
+	default "1"
+	help
+	  Pick the port number (0 based) for KGDB to use.
+
 config KGDB_BAUDRATE
 	int "Debug serial port baud rate"
 	depends on KGDB_SIMPLE_SERIAL
@@ -45,14 +53,6 @@ config KGDB_BAUDRATE
 	  used.  Standard rates from 9600 to 115200 are allowed, and this
 	  may be overridden via the commandline.
 
-config KGDB_PORT_NUM
-	int "Serial port number for KGDB"
-	range 0 3
-	depends on KGDB_SIMPLE_SERIAL
-	default "1"
-	help
-	  Pick the port number (0 based) for KGDB to use.
-
 config KGDB_8250_CONF_STRING
 	string "Configuration string for KGDB"
 	depends on (KGDB_8250 = y) && !KGDB_SIMPLE_SERIAL

Re: [PATCH 4/5] KGDB-8250: refactor configuration

[Jan Kiszka]

Sorry, previous version was missing some __init[data] attributes which
were dropped in an intermediate stage. Here comes an updated patch:

<---snip--->

This major refactoring of the quite complex kgdb8250 configuration does
the following:

 - ensures that static configurations according to SERIAL_PORT_DFNS are
   always loaded first
 - tries to pull more accurate configuration via serial8250_get_port_def
   if simple-config is used
 - detects empty/invalid simple-configs
 - enforces KGDB_PORT_NUM <= SERIAL_8250_NR_UARTS at kconfig level
 - removes kgdb8250_add_port and its hook in serial_core (calling
   serial8250_get_port_def in demand should provide us the same
   information)

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>

---
 drivers/serial/8250.c        |   47 ++++++++++++----------------
 drivers/serial/8250_kgdb.c   |   70 +++++++++++++------------------------------
 drivers/serial/serial_core.c |    7 ----
 include/linux/kgdb.h         |    1 
 lib/Kconfig.kgdb             |   16 ++++-----
 5 files changed, 51 insertions(+), 90 deletions(-)

Index: b/drivers/serial/8250_kgdb.c
===================================================================
--- a/drivers/serial/8250_kgdb.c
+++ b/drivers/serial/8250_kgdb.c
@@ -53,16 +53,6 @@ static char kgdb8250_buf[GDB_BUF_SIZE];
 static atomic_t kgdb8250_buf_in_cnt;
 static int kgdb8250_buf_out_inx;
 
-/* Old-style serial definitions, if existant, and a counter. */
-#ifdef CONFIG_KGDB_SIMPLE_SERIAL
-static int should_copy_rs_table = 1;
-static struct serial_state old_rs_table[] __initdata = {
-#ifdef SERIAL_PORT_DFNS
-	SERIAL_PORT_DFNS
-#endif
-};
-#endif
-
 /* Our internal table of UARTS. */
 #define UART_NR	CONFIG_SERIAL_8250_NR_UARTS
 static struct uart_port kgdb8250_ports[UART_NR];
@@ -258,9 +248,15 @@ static int kgdb8250_uart_init(void)
 static void __init kgdb8250_copy_rs_table(void)
 {
 #ifdef CONFIG_KGDB_SIMPLE_SERIAL
+	static struct serial_state old_rs_table[] __initdata = {
+#ifdef SERIAL_PORT_DFNS
+		SERIAL_PORT_DFNS
+#endif
+	};
+	static int rs_table_copied __initdata;
 	int i;
 
-	if (!should_copy_rs_table)
+	if (rs_table_copied)
 		return;
 
 	for (i = 0; i < ARRAY_SIZE(old_rs_table); i++) {
@@ -273,8 +269,8 @@ static void __init kgdb8250_copy_rs_tabl
 		kgdb8250_ports[i].line = i;
 	}
 
-	should_copy_rs_table = 0;
-#endif
+	rs_table_copied = 1;
+#endif /* CONFIG_KGDB_SIMPLE_SERIAL */
 }
 
 /*
@@ -284,9 +280,6 @@ static void __init kgdb8250_copy_rs_tabl
  */
 static void __init kgdb8250_late_init(void)
 {
-	/* Try and copy the old_rs_table. */
-	kgdb8250_copy_rs_table();
-
 #if defined(CONFIG_SERIAL_8250) || defined(CONFIG_SERIAL_8250_MODULE)
 	/* Take the port away from the main driver. */
 	serial8250_unregister_by_port(current_port);
@@ -306,9 +299,6 @@ static void __init kgdb8250_late_init(vo
 
 static __init int kgdb_init_io(void)
 {
-	/* Give us the basic table of uarts. */
-	kgdb8250_copy_rs_table();
-
 	/* We're either a module and parse a config string, or we have a
 	 * semi-static config. */
 #ifdef CONFIG_KGDB_8250_MODULE
@@ -325,8 +315,15 @@ static __init int kgdb_init_io(void)
 #else  /* !CONFIG_KGDB_8250_MODULE */
 	if (!params_evaluated) {
 #ifdef CONFIG_KGDB_SIMPLE_SERIAL
+		kgdb8250_copy_rs_table();
+
 		kgdb8250_baud = CONFIG_KGDB_BAUDRATE;
 		current_port = &kgdb8250_ports[CONFIG_KGDB_PORT_NUM];
+
+		if (serial8250_get_port_def(current_port,
+					    CONFIG_KGDB_PORT_NUM) < 0 &&
+		    !current_port->iobase && !current_port->membase)
+			return -EINVAL;
 #else /* !CONFIG_KGDB_SIMPLE_SERIAL */
 		if (kgdb8250_opt(CONFIG_KGDB_8250_CONF_STRING))
 			return -EINVAL;
@@ -395,29 +392,6 @@ struct kgdb_io kgdb_io_ops = {
 };
 
 /**
- * 	kgdb8250_add_port - Define a serial port for use with KGDB
- * 	@i: The index of the port being added
- * 	@serial_req: The &struct uart_port describing the port
- *
- * 	On platforms where we must register the serial device
- * 	dynamically, this is the best option if a platform also normally
- * 	calls early_serial_setup().
- */
-void kgdb8250_add_port(int i, struct uart_port *serial_req)
-{
-#ifdef CONFIG_KGDB_SIMPLE_SERIAL
-	if (should_copy_rs_table)
-		printk(KERN_ERR "8250_kgdb: warning will over write serial"
-			   " port definitions at kgdb init time\n");
-#endif
-
-	/* Copy the whole thing over. */
-	if (current_port != &kgdb8250_ports[i])
-		memcpy(&kgdb8250_ports[i], serial_req,
-		sizeof(struct uart_port));
-}
-
-/**
  * 	kgdb8250_add_platform_port - Define a serial port for use with KGDB
  * 	@i: The index of the port being added
  * 	@p: The &struct plat_serial8250_port describing the port
@@ -447,7 +421,10 @@ void __init kgdb8250_add_platform_port(i
  */
 static int __init kgdb8250_opt(char *str)
 {
-	/* We'll fill out and use the first slot. */
+	/* Give us the basic table of uarts. */
+	kgdb8250_copy_rs_table();
+
+	/* If we overwrite, we'll fill out and use the first slot. */
 	current_port = &kgdb8250_ports[0];
 
 	if (!strncmp(str, "io", 2)) {
@@ -467,7 +444,8 @@ static int __init kgdb8250_opt(char *str
 		if (line >= UART_NR)
 			goto errout;
 		current_port = &kgdb8250_ports[line];
-		if (serial8250_get_port_def(current_port, line))
+		if (serial8250_get_port_def(current_port, line) < 0 &&
+		    !current_port->iobase && !current_port->membase)
 			goto errout;
 		str++;
 		if (*str != ',')
@@ -512,16 +490,12 @@ static int __init kgdb8250_opt(char *str
 	current_port->irq = simple_strtoul(str, &str, 10);
 
 finish:
-#ifdef CONFIG_KGDB_SIMPLE_SERIAL
-	should_copy_rs_table = 0;
-#endif
 #ifdef CONFIG_KGDB_8250
 	params_evaluated = 1;
 #endif
 	return 0;
 
 errout:
-	printk(KERN_ERR "Invalid syntax for option kgdb8250=\n");
 	return 1;
 }
 
Index: b/drivers/serial/8250.c
===================================================================
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2860,37 +2860,32 @@ void serial8250_unregister_port(int line
 EXPORT_SYMBOL(serial8250_unregister_port);
 
 /**
- *  serial8250_get_port_def - Get port definition for a specific line
- *  @port: generic uart_port output for a specific serial line
- *  @line: specific serial line index
+ *	serial8250_get_port_def - Get port definition for a specific line
+ *	@port: generic uart_port output for a specific serial line
+ *	@line: specific serial line index
  *
- *  Return 0 if the port existed
- *  Return 1 on failure
+ *	Returns 0 on success, -ENODEV if the port does not exist.
  */
-
 int serial8250_get_port_def(struct uart_port *port, int line)
 {
-	struct uart_8250_port *uart;
+	struct uart_port *port8250 = &serial8250_ports[line].port;
 
-	if (line < 0 || line >= UART_NR)
-		return 1;
-	uart = &serial8250_ports[line];
-	if (uart) {
-		port->iobase = uart->port.iobase;
-		port->membase = uart->port.membase;
-		port->irq = uart->port.irq;
-		port->uartclk = uart->port.uartclk;
-		port->fifosize = uart->port.fifosize;
-		port->regshift = uart->port.regshift;
-		port->iotype = uart->port.iotype;
-		port->flags = uart->port.flags;
-		port->mapbase = uart->port.mapbase;
-		port->dev =	uart->port.dev;
-		return 0;
-	}
-	return 1;
+	if (!port8250->iobase && !port8250->membase)
+		return -ENODEV;
+
+	port->iobase   = port8250->iobase;
+	port->membase  = port8250->membase;
+	port->irq      = port8250->irq;
+	port->uartclk  = port8250->uartclk;
+	port->fifosize = port8250->fifosize;
+	port->regshift = port8250->regshift;
+	port->iotype   = port8250->iotype;
+	port->flags    = port8250->flags;
+	port->mapbase  = port8250->mapbase;
+	port->dev      = port8250->dev;
+	return 0;
 }
-EXPORT_SYMBOL(serial8250_get_port_def);
+EXPORT_SYMBOL_GPL(serial8250_get_port_def);
 
 /**
  *	serial8250_unregister_by_port - remove a 16x50 serial port
@@ -2909,7 +2904,7 @@ void serial8250_unregister_by_port(struc
 	if (uart)
 		serial8250_unregister_port(uart->port.line);
 }
-EXPORT_SYMBOL(serial8250_unregister_by_port);
+EXPORT_SYMBOL_GPL(serial8250_unregister_by_port);
 
 static int __init serial8250_init(void)
 {
Index: b/drivers/serial/serial_core.c
===================================================================
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -33,7 +33,6 @@
 #include <linux/serial.h> /* for serial_state and serial_icounter_struct */
 #include <linux/delay.h>
 #include <linux/mutex.h>
-#include <linux/kgdb.h>
 
 #include <asm/irq.h>
 #include <asm/uaccess.h>
@@ -2370,12 +2369,6 @@ int uart_add_one_port(struct uart_driver
 	 */
 	port->flags &= ~UPF_DEAD;
 
-#if defined(CONFIG_KGDB_8250)
-	/* Add any 8250-like ports we find later. */
-	if (port->type <= PORT_MAX_8250)
-		kgdb8250_add_port(port->line, port);
-#endif
-
  out:
 	mutex_unlock(&state->mutex);
 	mutex_unlock(&port_mutex);
Index: b/include/linux/kgdb.h
===================================================================
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -271,7 +271,6 @@ extern struct kgdb_arch arch_kgdb_ops;
 int kgdb_register_io_module(struct kgdb_io *local_kgdb_io_ops);
 void kgdb_unregister_io_module(struct kgdb_io *local_kgdb_io_ops);
 
-void kgdb8250_add_port(int i, struct uart_port *serial_req);
 void __init kgdb8250_add_platform_port(int i,
 	struct plat_serial8250_port *serial_req);
 
Index: b/lib/Kconfig.kgdb
===================================================================
--- a/lib/Kconfig.kgdb
+++ b/lib/Kconfig.kgdb
@@ -36,6 +36,14 @@ config KGDB_SIMPLE_SERIAL
 	  or on the command line, the type (I/O or MMIO), IRQ and
 	  address to use.  If in doubt, say Y.
 
+config KGDB_PORT_NUM
+	int "Serial port number for KGDB"
+	range 0 SERIAL_8250_NR_UARTS
+	depends on KGDB_SIMPLE_SERIAL
+	default "1"
+	help
+	  Pick the port number (0 based) for KGDB to use.
+
 config KGDB_BAUDRATE
 	int "Debug serial port baud rate"
 	depends on KGDB_SIMPLE_SERIAL
@@ -45,14 +53,6 @@ config KGDB_BAUDRATE
 	  used.  Standard rates from 9600 to 115200 are allowed, and this
 	  may be overridden via the commandline.
 
-config KGDB_PORT_NUM
-	int "Serial port number for KGDB"
-	range 0 3
-	depends on KGDB_SIMPLE_SERIAL
-	default "1"
-	help
-	  Pick the port number (0 based) for KGDB to use.
-
 config KGDB_8250_CONF_STRING
 	string "Configuration string for KGDB"
 	depends on (KGDB_8250 = y) && !KGDB_SIMPLE_SERIAL

Re: [Kgdb-bugreport] [PATCH 4/5] KGDB-8250: refactor configuration

[Sergei Shtylyov]

Hello.

Jan Kiszka wrote:

> Sorry, previous version was missing some __init[data] attributes which
> were dropped in an intermediate stage. Here comes an updated patch:

> <---snip--->

> This major refactoring of the quite complex kgdb8250 configuration does
> the following:

>  - ensures that static configurations according to SERIAL_PORT_DFNS are
>    always loaded first
>  - tries to pull more accurate configuration via serial8250_get_port_def
>    if simple-config is used
>  - detects empty/invalid simple-configs
>  - enforces KGDB_PORT_NUM <= SERIAL_8250_NR_UARTS at kconfig level
>  - removes kgdb8250_add_port and its hook in serial_core (calling
>    serial8250_get_port_def in demand should provide us the same
>    information)

    You left powerpc-lite.patch broken with this change as it has multiple 
calls to kgdb8250_add_port()...

> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>

> Index: b/drivers/serial/serial_core.c
> ===================================================================
> --- a/drivers/serial/serial_core.c
> +++ b/drivers/serial/serial_core.c
[...]
> @@ -2370,12 +2369,6 @@ int uart_add_one_port(struct uart_driver
>  	 */
>  	port->flags &= ~UPF_DEAD;
>  
> -#if defined(CONFIG_KGDB_8250)
> -	/* Add any 8250-like ports we find later. */
> -	if (port->type <= PORT_MAX_8250)
> -		kgdb8250_add_port(port->line, port);
> -#endif
> -

    I'm afraid this wasn't correct from the very start since this can add 
ports with .iotype that 8250_kgdb.c does not support. So, nothing to regret 
here...

WBR, Sergei

Re: [Kgdb-bugreport] [PATCH 4/5] KGDB-8250: refactor configuration

[Jan Kiszka]

Sergei Shtylyov wrote:
> Hello.
> 
> Jan Kiszka wrote:
> 
>> Sorry, previous version was missing some __init[data] attributes which
>> were dropped in an intermediate stage. Here comes an updated patch:
> 
>> <---snip--->
> 
>> This major refactoring of the quite complex kgdb8250 configuration does
>> the following:
> 
>>  - ensures that static configurations according to SERIAL_PORT_DFNS are
>>    always loaded first
>>  - tries to pull more accurate configuration via serial8250_get_port_def
>>    if simple-config is used
>>  - detects empty/invalid simple-configs
>>  - enforces KGDB_PORT_NUM <= SERIAL_8250_NR_UARTS at kconfig level
>>  - removes kgdb8250_add_port and its hook in serial_core (calling
>>    serial8250_get_port_def in demand should provide us the same
>>    information)
> 
>    You left powerpc-lite.patch broken with this change as it has
> multiple calls to kgdb8250_add_port()...

I see. But I wonder if there ever was a real need for these hooks (in
2.4 times?): If I look at bamboo_early_serial_map() e.g., I find it
calling into early_serial_setup() which fills serial8250_ports[] - and
that content is now retrieved via serial8250_get_port_def() when we
parse the runtime or build-time provided parameters (port number &
baudrate).

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> 
>> Index: b/drivers/serial/serial_core.c
>> ===================================================================
>> --- a/drivers/serial/serial_core.c
>> +++ b/drivers/serial/serial_core.c
> [...]
>> @@ -2370,12 +2369,6 @@ int uart_add_one_port(struct uart_driver
>>       */
>>      port->flags &= ~UPF_DEAD;
>>  
>> -#if defined(CONFIG_KGDB_8250)
>> -    /* Add any 8250-like ports we find later. */
>> -    if (port->type <= PORT_MAX_8250)
>> -        kgdb8250_add_port(port->line, port);
>> -#endif
>> -
> 
>    I'm afraid this wasn't correct from the very start since this can add
> ports with .iotype that 8250_kgdb.c does not support. So, nothing to
> regret here...

I think a lot of cruft piled up in the kgdb patches over their long life. :)

Thanks for your feedback!

Jan

Re: [Kgdb-bugreport] [PATCH 4/5] KGDB-8250: refactor configuration

[Sergei Shtylyov]

Hello.

Jan Kiszka wrote:

>>>Sorry, previous version was missing some __init[data] attributes which
>>>were dropped in an intermediate stage. Here comes an updated patch:

>>><---snip--->

>>>This major refactoring of the quite complex kgdb8250 configuration does
>>>the following:

>>> - ensures that static configurations according to SERIAL_PORT_DFNS are
>>>   always loaded first
>>> - tries to pull more accurate configuration via serial8250_get_port_def
>>>   if simple-config is used
>>> - detects empty/invalid simple-configs
>>> - enforces KGDB_PORT_NUM <= SERIAL_8250_NR_UARTS at kconfig level
>>> - removes kgdb8250_add_port and its hook in serial_core (calling
>>>   serial8250_get_port_def in demand should provide us the same
>>>   information)

>>   You left powerpc-lite.patch broken with this change as it has
>>multiple calls to kgdb8250_add_port()...

> I see. But I wonder if there ever was a real need for these hooks (in
> 2.4 times?): If I look at bamboo_early_serial_map() e.g., I find it
> calling into early_serial_setup() which fills serial8250_ports[] - and
> that content is now retrieved via serial8250_get_port_def() when we
> parse the runtime or build-time provided parameters (port number &
> baudrate).

    Of course. But now the kgdb8250_add_port() calls need to be removed.

>>>Signed-off-by: Jan Kiszka <jan.kiszka@web.de>

WBR, Sergei

Re: [Kgdb-bugreport] [PATCH 4/5] KGDB-8250: refactor configuration

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

Sergei Shtylyov wrote:
>>>   You left powerpc-lite.patch broken with this change as it has
>>> multiple calls to kgdb8250_add_port()...
> 
>> I see. But I wonder if there ever was a real need for these hooks (in
>> 2.4 times?): If I look at bamboo_early_serial_map() e.g., I find it
>> calling into early_serial_setup() which fills serial8250_ports[] - and
>> that content is now retrieved via serial8250_get_port_def() when we
>> parse the runtime or build-time provided parameters (port number &
>> baudrate).
> 
>    Of course. But now the kgdb8250_add_port() calls need to be removed.

For sure. Most arch patches need to go through some refactoring anyway
when preparing them for upstream. Cleaning up no longer required hooks
should be no problem at this chance.

If you want to accelerate this process, please check out Jason's
linux-2.6-kgdb.git for 2.6.25 and start rebasing the powerpc patch. He
just recently said that support around kgdb for non-x86 would be highly
welcome. And if you stumble over ppc-related issues that cannot be
solved with latest kgdb design, please let us know. The sooner, the better.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]