KGDB: 8250_kgdb warnings

KGDB: 8250_kgdb warnings

[Jan Kiszka]

Hi Jason,

so far I ignored this because it worked, but I know my customer will
complain later anyway: What is the deeper meaning of this warning which
shows up once per registered UART port on my (x86) boxes?

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

When I look at kgdb8250_add_platform_port, it starts with a call to
kgdb8250_copy_rs_table, and I'm wondering now if that wouldn't be more
appropriate here.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[PATCH][KGDB] Re: [Kgdb-bugreport] KGDB: 8250_kgdb warnings

[Jason Wessel]

Jan Kiszka wrote:
> Hi Jason,
> 
> so far I ignored this because it worked, but I know my customer will
> complain later anyway: What is the deeper meaning of this warning which
> shows up once per registered UART port on my (x86) boxes?
> 
> 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
> ...
> 
> When I look at kgdb8250_add_platform_port, it starts with a call to
> kgdb8250_copy_rs_table, and I'm wondering now if that wouldn't be more
> appropriate here.
> 
> Jan
> 


This was the result of a race condition between the init code of the
platform vs the init code in kgdb.  The init code in the arch platform
could register serial ports prior to the kgdb module being configured
by the kernel while the kernel is processing all the __init functions.
It would have been easy to fix this with another call to
kgdb8250_copy_rs_table() but you cannot do that because a non-__init
function cannot call an __init function.


We might as well go ahead and fix the problem by adding in some checks
so as not to overwrite the dynamic registrations, because eventually
the SERIAL_PORT_DFNS will be gone.  Below is the patch with the fix to
add some saftey checks as well as to remove the warning.

--------Cut here-----------


Fix the initialization of the kgdb port structure such that
dynamically registered ports will not be later overwritten by the
SERIAL_PORT_DFNS table.  With this problem fixed, the printk about the
overwriting of the kgdb serial definitions at init time can be
removed.

Also add in additional runtime safety checks to make sure UART_NR was
statically allocated by the kernel at compile time to be large enough
for all the dynamic registered ports.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 drivers/serial/8250_kgdb.c |   27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

--- a/drivers/serial/8250_kgdb.c
+++ b/drivers/serial/8250_kgdb.c
@@ -53,7 +53,7 @@ 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 int __initdata should_copy_rs_table = 1;
 static struct serial_state old_rs_table[] __initdata = {
 #ifdef SERIAL_PORT_DFNS
 	SERIAL_PORT_DFNS
@@ -260,7 +260,10 @@ static void __init kgdb8250_copy_rs_tabl
 	if (!should_copy_rs_table)
 		return;
 
-	for (i = 0; i < ARRAY_SIZE(old_rs_table); i++) {
+	for (i = 0; i < ARRAY_SIZE(old_rs_table) && i < UART_NR; i++) {
+		if (kgdb8250_ports[i].iobase || kgdb8250_ports[i].irq ||
+			kgdb8250_ports[i].membase)
+			continue;
 		kgdb8250_ports[i].iobase = old_rs_table[i].port;
 		kgdb8250_ports[i].irq = irq_canonicalize(old_rs_table[i].irq);
 		kgdb8250_ports[i].uartclk = old_rs_table[i].baud_base * 16;
@@ -281,7 +284,7 @@ static void __init kgdb8250_copy_rs_tabl
  */
 static void __init kgdb8250_late_init(void)
 {
-	/* Try and copy the old_rs_table. */
+	/* Setup the KGDB uart table if not already initialized */
 	kgdb8250_copy_rs_table();
 
 #if defined(CONFIG_SERIAL_8250) || defined(CONFIG_SERIAL_8250_MODULE)
@@ -303,7 +306,7 @@ static void __init kgdb8250_late_init(vo
 
 static __init int kgdb_init_io(void)
 {
-	/* Give us the basic table of uarts. */
+	/* Setup the KGDB uart table if not already initialized */
 	kgdb8250_copy_rs_table();
 
 	/* We're either a module and parse a config string, or we have a
@@ -401,11 +404,11 @@ struct kgdb_io kgdb_io_ops = {
  */
 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
+	if (i >= UART_NR) {
+		printk(KERN_ERR "KGDB dynamic uart registration failed"
+		   "NR_UARTS is too small");
+		return;
+	}
 
 	/* Copy the whole thing over. */
 	if (current_port != &kgdb8250_ports[i])
@@ -427,6 +430,12 @@ void __init kgdb8250_add_platform_port(i
 	/* Make sure we've got the built-in data before we override. */
 	kgdb8250_copy_rs_table();
 
+	if (i >= UART_NR) {
+		printk(KERN_ERR "KGDB dynamic uart registration failed"
+		   "NR_UARTS is too small");
+		return;
+	}
+
 	kgdb8250_ports[i].iobase = p->iobase;
 	kgdb8250_ports[i].membase = p->membase;
 	kgdb8250_ports[i].irq = p->irq;

Re: [PATCH][KGDB] Re: KGDB: 8250_kgdb warnings

[Jan Kiszka]

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

Jason Wessel wrote:
> Jan Kiszka wrote:
>> Hi Jason,
>>
>> so far I ignored this because it worked, but I know my customer will
>> complain later anyway: What is the deeper meaning of this warning which
>> shows up once per registered UART port on my (x86) boxes?
>>
>> 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
>> ...
>>
>> When I look at kgdb8250_add_platform_port, it starts with a call to
>> kgdb8250_copy_rs_table, and I'm wondering now if that wouldn't be more
>> appropriate here.
>>
>> Jan
>>
> 
> 
> This was the result of a race condition between the init code of the
> platform vs the init code in kgdb.  The init code in the arch platform
> could register serial ports prior to the kgdb module being configured
> by the kernel while the kernel is processing all the __init functions.
> It would have been easy to fix this with another call to
> kgdb8250_copy_rs_table() but you cannot do that because a non-__init
> function cannot call an __init function.
> 
> 
> We might as well go ahead and fix the problem by adding in some checks
> so as not to overwrite the dynamic registrations, because eventually
> the SERIAL_PORT_DFNS will be gone.  Below is the patch with the fix to
> add some saftey checks as well as to remove the warning.
> 
> --------Cut here-----------
> 
> 
> Fix the initialization of the kgdb port structure such that
> dynamically registered ports will not be later overwritten by the
> SERIAL_PORT_DFNS table.  With this problem fixed, the printk about the
> overwriting of the kgdb serial definitions at init time can be
> removed.
> 
> Also add in additional runtime safety checks to make sure UART_NR was
> statically allocated by the kernel at compile time to be large enough
> for all the dynamic registered ports.

I'm currently preparing to post a larger patch series for KGDB which
also addresses this problem - but quite differently. Therefore this
question in advance:

What use case could not be covered by polling the (8250) config via
serial8250_get_port_def() when there were no kgdb8250_add_port()? That's
in fact what I did in my patch because I found none and the result
appears much more consistent to me. But I'm not claiming to be a UART
expert across all those various embedded platforms.

Jan


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