serial_txx9: forcibly init the spinlock for PCI UART used as a console

PATCH In review Sergei Shtylylov 4 participants 11 messages Dec 26, 2005 – Jan 5, 2006 MIPS

lore

Share & tools

lore

Sergei Shtylylov

author

2005-12-26 22:21 UTC #0 lore raw

PATCH 1/1 serial_txx9: forcibly init the spinlock for PCI UART used as a console

Hello.

When a system console gets assigned to the UART located on the Toshiba
GOKU-S PCI card, the port spinlock is not initialized at all -- uart_add_one_port() thinks it's been initialized by the console setup code which is called too early for being able to do that, before the PCI card is even detected by the driver, and therefore fails. That unitialized spinlock causes 3 BUG messages in the boot log with Ingo Molnar's RT preemption patch as uart_add_one_port() called to register PCI UART with the serial core calls uart_configure_port() which makes use of the port spinlock.

WBR, Sergei

Signed-off-by Sergei Shtylyov <[email protected]>

drivers/serial/serial_txx9.c +8 -0

index f10c86d..3f8dc41 100644
@@ -1065,6 +1065,14 @@ static int __devinit serial_txx9_registe
 		uart->port.mapbase  = port->mapbase;
 		if (port->dev)
 			uart->port.dev = port->dev;
+
+		/*
+		 * If this port is a console, its spinlock couldn't have been
+		 * initialized by serial_txx9_console_setup() and it won't be
+		 * initialized by uart_add_one_port(), so have to do it here...
+		 */
+		spin_lock_init(&uart->port.lock);
+
 		ret = uart_add_one_port(&serial_txx9_reg, &uart->port);
 		if (ret == 0)
 			ret = uart->port.line;

Atsushi Nemoto

2005-12-27 05:44 UTC #1 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Sergei Shtylylov

sshtylyov> When a system console gets assigned to the UART
sshtylyov> located on the Toshiba GOKU-S PCI card, the port spinlock sshtylyov> is not initialized at all -- uart_add_one_port() thinks sshtylyov> it's been initialized by the console setup code which is sshtylyov> called too early for being able to do that, before the PCI sshtylyov> card is even detected by the driver, and therefore sshtylyov> fails.

The problem is not just only spin_lock_init. The parameters of "console=" option (baudrate, etc.) are not passed for PCI UART. Also, if console setup failed, the console never enabled. So the console can not be used anyway.

I have an another fix. Call register_console() again for PCI UART if the console was not enabled. This fixes spin_lock_init issue and makes PCI UART really usable as console.

Also, I have some other pending changes for this driver.

* More strict check in verify_port. Cleanup.
* Do not insert a char caused previous overrun.
* Fix some spin_locks.
* Call register_console again for PCI ports.

This is a patch against linux-mips GIT.

Signed-off-by Atsushi Nemoto <[email protected]>

drivers/serial/serial_txx9.c +39 ~~-31~~

index f10c86d..c24e0c3 100644
@@ -33,6 +33,10 @@
  *	1.02	Cleanup. (import 8250.c changes)
  *	1.03	Fix low-latency mode. (import 8250.c changes)
  *	1.04	Remove usage of deprecated functions, cleanup.
+ *	1.05	More strict check in verify_port.  Cleanup.
+ *	1.06	Do not insert a char caused previous overrun.
+ *		Fix some spin_locks.
+ *		Call register_console again for PCI ports.
  */
 #include <linux/config.h>

@@ -56,7 +60,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-static char *serial_version = "1.04";
+static char *serial_version = "1.06";
 static char *serial_name = "TX39/49 Serial driver";
 
 #define PASS_LIMIT	256

@@ -209,7 +213,7 @@ static inline unsigned int sio_in(struct
 {
 	switch (up->port.iotype) {
 	default:
-		return *(volatile u32 *)(up->port.membase + offset);
+		return __raw_readl(up->port.membase + offset);
 	case UPIO_PORT:
 		return inl(up->port.iobase + offset);
 	}

@@ -220,7 +224,7 @@ sio_out(struct uart_txx9_port *up, int o
 {
 	switch (up->port.iotype) {
 	default:
-		*(volatile u32 *)(up->port.membase + offset) = value;
+		__raw_writel(value, up->port.membase + offset);
 		break;
 	case UPIO_PORT:
 		outl(value, up->port.iobase + offset);

@@ -258,34 +262,19 @@ sio_quot_set(struct uart_txx9_port *up, 
 static void serial_txx9_stop_tx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	sio_mask(up, TXX9_SIDICR, TXX9_SIDICR_TIE);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_start_tx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	sio_set(up, TXX9_SIDICR, TXX9_SIDICR_TIE);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_stop_rx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	up->port.read_status_mask &= ~TXX9_SIDISR_RDIS;
-#if 0
-	sio_mask(up, TXX9_SIDICR, TXX9_SIDICR_RIE);
-#endif
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_enable_ms(struct uart_port *port)

@@ -301,6 +290,7 @@ receive_chars(struct uart_txx9_port *up,
 	unsigned int disr = *status;
 	int max_count = 256;
 	char flag;
+	unsigned int next_ignore_status_mask;
 
 	do {
 		/* The following is not allowed by the tty layer and

@@ -318,6 +308,9 @@ receive_chars(struct uart_txx9_port *up,
 		flag = TTY_NORMAL;
 		up->port.icount.rx++;
 
+		/* mask out RFDN_MASK bit added by previous overrun */
+		next_ignore_status_mask =
+			up->port.ignore_status_mask & ~TXX9_SIDISR_RFDN_MASK;
 		if (unlikely(disr & (TXX9_SIDISR_UBRK | TXX9_SIDISR_UPER |
 				     TXX9_SIDISR_UFER | TXX9_SIDISR_UOER))) {
 			/*

@@ -338,8 +331,17 @@ receive_chars(struct uart_txx9_port *up,
 				up->port.icount.parity++;
 			else if (disr & TXX9_SIDISR_UFER)
 				up->port.icount.frame++;
-			if (disr & TXX9_SIDISR_UOER)
+			if (disr & TXX9_SIDISR_UOER) {
 				up->port.icount.overrun++;
+				/*
+				 * The receiver read buffer still hold
+				 * a char which caused overrun.
+				 * Ignore next char by adding RFDN_MASK
+				 * to ignore_status_mask temporarily.
+				 */
+				next_ignore_status_mask |=
+					TXX9_SIDISR_RFDN_MASK;
+			}
 
 			/*
 			 * Mask off conditions which should be ingored.

@@ -359,6 +361,7 @@ receive_chars(struct uart_txx9_port *up,
 		uart_insert_char(&up->port, disr, TXX9_SIDISR_UOER, ch, flag);
 
 	ignore_char:
+		up->port.ignore_status_mask = next_ignore_status_mask;
 		disr = sio_in(up, TXX9_SIDISR);
 	} while (!(disr & TXX9_SIDISR_UVALID) && (max_count-- > 0));
 	spin_unlock(&up->port.lock);

@@ -460,14 +463,11 @@ static unsigned int serial_txx9_get_mctr
 static void serial_txx9_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
 
-	spin_lock_irqsave(&up->port.lock, flags);
 	if (mctrl & TIOCM_RTS)
 		sio_mask(up, TXX9_SIFLCR, TXX9_SIFLCR_RTSSC);
 	else
 		sio_set(up, TXX9_SIFLCR, TXX9_SIFLCR_RTSSC);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_break_ctl(struct uart_port *port, int break_state)

@@ -794,8 +794,13 @@ static void serial_txx9_config_port(stru
 static int
 serial_txx9_verify_port(struct uart_port *port, struct serial_struct *ser)
 {
-	if (ser->irq < 0 ||
-	    ser->baud_base < 9600 || ser->type != PORT_TXX9)
+	unsigned long new_port = (unsigned long)ser->port +
+		((unsigned long)ser->port_high << ((sizeof(long) - sizeof(int)) * 8));
+	if (ser->type != port->type ||
+	    ser->irq != port->irq ||
+	    ser->io_type != port->iotype ||
+	    new_port != port->iobase ||
+	    (unsigned long)ser->iomem_base != port->mapbase)
 		return -EINVAL;
 	return 0;
 }

@@ -937,11 +942,6 @@ static int serial_txx9_console_setup(str
 		return -ENODEV;
 
 	/*
-	 * Temporary fix.
-	 */
-	spin_lock_init(&port->lock);
-
-	/*
 	 *	Disable UART interrupts, set DTR and RTS high
 	 *	and set speed.
 	 */

@@ -1065,6 +1065,14 @@ static int __devinit serial_txx9_registe
 		uart->port.mapbase  = port->mapbase;
 		if (port->dev)
 			uart->port.dev = port->dev;
+#ifdef CONFIG_SERIAL_TXX9_CONSOLE
+		/*
+		 * The 'early' register_console fails for PCI ports.
+		 * Do it again.
+		 */
+		if (!(serial_txx9_console.flags & CON_ENABLED))
+			register_console(&serial_txx9_console);
+#endif
 		ret = uart_add_one_port(&serial_txx9_reg, &uart->port);
 		if (ret == 0)
 			ret = uart->port.line;

@@ -1090,7 +1098,7 @@ static void __devexit serial_txx9_unregi
 	uart->port.type = PORT_UNKNOWN;
 	uart->port.iobase = 0;
 	uart->port.mapbase = 0;
-	uart->port.membase = 0;
+	uart->port.membase = NULL;
 	uart->port.dev = NULL;
 	uart_add_one_port(&serial_txx9_reg, &uart->port);
 	up(&serial_txx9_sem);

@@ -1195,7 +1203,7 @@ static int __init serial_txx9_init(void)
 		serial_txx9_register_ports(&serial_txx9_reg);
 
 #ifdef ENABLE_SERIAL_TXX9_PCI
-		ret = pci_module_init(&serial_txx9_pci_driver);
+		ret = pci_register_driver(&serial_txx9_pci_driver);
 #endif
 	}
 	return ret;

Sergei Shtylylov

author

2005-12-27 13:35 UTC #2 lore raw

Hello.

Quoted 10 lines · Atsushi Nemoto

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Sergei Shtylylov
sshtylyov> When a system console gets assigned to the UART
sshtylyov> located on the Toshiba GOKU-S PCI card, the port spinlock sshtylyov> is not initialized at all -- uart_add_one_port() thinks sshtylyov> it's been initialized by the console setup code which is sshtylyov> called too early for being able to do that, before the PCI sshtylyov> card is even detected by the driver, and therefore sshtylyov> fails.

Quoted 2 lines

The problem is not just only spin_lock_init. The parameters of "console=" option (baudrate, etc.) are not passed for PCI UART.

They are -- uart_add_one_port() calls console setup once more when
registering PCI UART with serial code.

Quoted 3 lines

Also,
if console setup failed, the console never enabled. So the console can not be used anyway.

I'm able to use it with ths fix in 2.6.10 with 1.04 driver version backported.

Quoted 2 lines

I have an another fix. Call register_console() again for PCI UART if the console was not enabled.

I disagree. Look at what uart_add_one_port() does closely.

Quoted 8 lines

This fixes spin_lock_init issue and
makes PCI UART really usable as console.
Also, I have some other pending changes for this driver.
* More strict check in verify_port. Cleanup.
* Do not insert a char caused previous overrun.
* Fix some spin_locks.

Yeah, I was about to send the patch that deasl with the nested spinlocks
as well... :-)

Quoted 1 line

* Call register_console again for PCI ports.

This change doesn't look correct to me...

Quoted 11 lines

This is a patch against linux-mips GIT.
Signed-off-by Atsushi Nemoto <[email protected]>
drivers/serial/serial_txx9.c +0 -0
index f10c86d..c24e0c3 100644
@@ -937,11 +942,6 @@ static int serial_txx9_console_setup(str
 		return -ENODEV;

drivers/serial/serial_txx9.c static int serial_txx9_console_setup(str · line 947

Quoted 10 lines

drivers/serial/serial_txx9.c +0 -5

 
 	/*
-	 * Temporary fix.
-	 */
-	spin_lock_init(&port->lock);
-
-	/*
 	 *	Disable UART interrupts, set DTR and RTS high
 	 *	and set speed.
 	 */

Can you tell me, how this is supposed to work with TX49xx SOC UARTs? When
that spinlock will be init'ed for the console port? uart_add_one_port() won't do it, and your added code below won't do it either, so I disagree with this change (though with "empty" spinlock it will no doubt work) since there's nothing to init.

Quoted 5 lines

drivers/serial/serial_txx9.c +1 -0

@@ -1065,6 +1065,14 @@ static int __devinit serial_txx9_registe
 		uart->port.mapbase  = port->mapbase;
 		if (port->dev)
 			uart->port.dev = port->dev;
+#ifdef CONFIG_SERIAL_TXX9_CONSOLE

drivers/serial/serial_txx9.c static int __devinit serial_txx9_registe · line 1078

Quoted 10 lines

drivers/serial/serial_txx9.c +7 -0

+		/*
+		 * The 'early' register_console fails for PCI ports.
+		 * Do it again.
+		 */
+		if (!(serial_txx9_console.flags & CON_ENABLED))
+			register_console(&serial_txx9_console);
+#endif
 		ret = uart_add_one_port(&serial_txx9_reg, &uart->port);
 		if (ret == 0)
 			ret = uart->port.line;

WBR, Sergei

Atsushi Nemoto

2005-12-27 15:34 UTC #3 lore raw

Thanks for your comment.

Quoted 4 lines

Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line · Sergei Shtylylov
The problem is not just only spin_lock_init. The parameters of "console=" option (baudrate, etc.) are not passed for PCI UART.

sshtylyov> They are -- uart_add_one_port() calls console setup
sshtylyov> once more when registering PCI UART with serial code.

Yes, you are right. I missed the register_console call in uart_add_one_port(). So your patch will fix the problem. But I suppose the spinlock should be initialized in serial_core. How about this?

drivers/serial/serial_core.c +1 -1

@@ -2233,7 +2233,7 @@ int uart_add_one_port(struct uart_driver
 	 * If this port is a console, then the spinlock is already
 	 * initialised.
 	 */
-	if (!uart_console(port))
+	if (!(uart_console(port) && (port->cons->flags & CON_ENABLED)))
 		spin_lock_init(&port->lock);
 
 	uart_configure_port(drv, state, port);

Quoted 5 lines

Quoted 5 lines
drivers/serial/serial_txx9.c +0 -0
@@ -937,11 +942,6 @@ static int serial_txx9_console_setup(str
 return -ENODEV;
 

Quoted 9 lines

drivers/serial/serial_txx9.c static int serial_txx9_console_setup(str · line 947
Quoted 9 lines
drivers/serial/serial_txx9.c +0 -5
 /*
-	 * Temporary fix.
-	 */
-	spin_lock_init(&port->lock);
-
-	/*
 *	Disable UART interrupts, set DTR and RTS high
 *	and set speed.
 */

sshtylyov> Can you tell me, how this is supposed to work with TX49xx sshtylyov> SOC UARTs? When that spinlock will be init'ed for the sshtylyov> console port? uart_add_one_port() won't do it, and your sshtylyov> added code below won't do it either, so I disagree with sshtylyov> this change (though with "empty" spinlock it will no doubt sshtylyov> work) since there's nothing to init.

The spinlock is initialized in uart_set_options() which is called from console setup function.

And this is a revised patch.

* More strict check in verify_port. Cleanup.
* Do not insert a char caused previous overrun.
* Fix some spin_locks.

Signed-off-by Atsushi Nemoto <[email protected]>

drivers/serial/serial_txx9.c +30 ~~-31~~

164 lines not on this page · raw message

Sergei Shtylylov

author

2005-12-27 16:25 UTC #4 lore raw

Hello.

Quoted 16 lines · Atsushi Nemoto

Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line · Sergei Shtylylov
The problem is not just only spin_lock_init. The parameters of "console=" option (baudrate, etc.) are not passed for PCI UART.
sshtylyov> They are -- uart_add_one_port() calls console setup
sshtylyov> once more when registering PCI UART with serial code.
Yes, you are right. I missed the register_console call in uart_add_one_port(). So your patch will fix the problem. But I suppose the spinlock should be initialized in serial_core. How about this?

drivers/serial/serial_core.c int uart_add_one_port(struct uart_driver · line 2239

Quoted 8 lines

drivers/serial/serial_core.c +1 -1

@@ -2233,7 +2233,7 @@ int uart_add_one_port(struct uart_driver
 	 * If this port is a console, then the spinlock is already
 	 * initialised.
 	 */
-	if (!uart_console(port))
+	if (!(uart_console(port) && (port->cons->flags & CON_ENABLED)))
 		spin_lock_init(&port->lock);
 
 	uart_configure_port(drv, state, port);

I wouldn't object.

Quoted 22 lines

Quoted 5 lines
Quoted 5 lines
drivers/serial/serial_txx9.c +0 -0
@@ -937,11 +942,6 @@ static int serial_txx9_console_setup(str
 return -ENODEV;
 
Quoted 9 lines
drivers/serial/serial_txx9.c static int serial_txx9_console_setup(str · line 946
Quoted 9 lines
drivers/serial/serial_txx9.c +0 -5
 /*
-	 * Temporary fix.
-	 */
-	spin_lock_init(&port->lock);
-
-	/*
 *	Disable UART interrupts, set DTR and RTS high
 *	and set speed.
 */
sshtylyov> Can you tell me, how this is supposed to work with TX49xx sshtylyov> SOC UARTs? When that spinlock will be init'ed for the sshtylyov> console port? uart_add_one_port() won't do it, and your sshtylyov> added code below won't do it either, so I disagree with sshtylyov> this change (though with "empty" spinlock it will no doubt sshtylyov> work) since there's nothing to init.

Quoted 2 lines

The spinlock is initialized in uart_set_options() which is called from console setup function.

I'm sorry, haven't dug that deep. :-)
Thought explicit spinlock init was really necessary there...

WBR, Sergei

Russell King

2005-12-27 18:40 UTC #5 lore raw

In reply to Atsushi Nemoto

Quoted 7 lines · Atsushi Nemoto

Thanks for your comment.
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line · Sergei Shtylylov
The problem is not just only spin_lock_init. The parameters of "console=" option (baudrate, etc.) are not passed for PCI UART.

Quoted 7 lines

sshtylyov> They are -- uart_add_one_port() calls console setup
sshtylyov> once more when registering PCI UART with serial code.
Yes, you are right. I missed the register_console call in uart_add_one_port(). So your patch will fix the problem. But I suppose the spinlock should be initialized in serial_core. How about this?

I think you're layering work-around on top of work-around on top of work-around here.

I think the first thing you need to resolve is the way you're registering your ports. Firstly, if you're solely PCI-based, there's no need to pre-register all the uart ports at driver initialisation time. Consequently, there's no need to remove them all when you remove the module.

Secondly, the upshot of this is that you only call uart_add_one_port() when you initialise a PCI card.

This should result in a cleaner implementation, and the console will not be started until you detect the PCI card. Which is the behaviour you desire in any case.

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Sergei Shtylylov

author

2005-12-27 18:50 UTC #6 lore raw

Hello.

Quoted 18 lines · Russell King

Quoted 8 lines · Atsushi Nemoto
Thanks for your comment.
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line · Sergei Shtylylov
The problem is not just only spin_lock_init. The parameters of "console=" option (baudrate, etc.) are not passed for PCI UART.
Quoted 7 lines
sshtylyov> They are -- uart_add_one_port() calls console setup
sshtylyov> once more when registering PCI UART with serial code.
Yes, you are right. I missed the register_console call in uart_add_one_port(). So your patch will fix the problem. But I suppose the spinlock should be initialized in serial_core. How about this?

Quoted 5 lines

I think you're layering work-around on top of work-around on top of work-around here.
I think the first thing you need to resolve is the way you're registering your ports. Firstly, if you're solely PCI-based, there's

No, this is not the case. The driver serves Toshiba TX39xx/49xx SOC UARTs
as well as the compatible PCI UART (GOKU-S).

Quoted 3 lines

no need to pre-register all the uart ports at driver initialisation time. Consequently, there's no need to remove them all when you remove the module.

Hm, then the driver would need to keep track of which ports it has
registered and which it has not...

Quoted 2 lines

Secondly, the upshot of this is that you only call uart_add_one_port() when you initialise a PCI card.

Not the case as I've said; uart_add_one_port() should be called on driver
startup anyway...

Quoted 2 lines

This should result in a cleaner implementation, and the console will not be started until you detect the PCI card.

It will still be started with the console_initcall() in this driver, if
that code is not also deleted...

WBR, Sergei

Sergei Shtylyov

2005-12-27 19:28 UTC #7 lore raw

Hello.

Quoted 6 lines · Sergei Shtylylov

Quoted 3 lines
no need to pre-register all the uart ports at driver initialisation time. Consequently, there's no need to remove them all when you remove the module.
Hm, then the driver would need to keep track of which ports it has
registered and which it has not...

However, it does this already in a way, via port type.

Quoted 11 lines

Quoted 2 lines
Secondly, the upshot of this is that you only call uart_add_one_port() when you initialise a PCI card.
Not the case as I've said; uart_add_one_port() should be called on
driver startup anyway...
Quoted 2 lines
This should result in a cleaner implementation, and the console will not be started until you detect the PCI card.
It will still be started with the console_initcall() in this driver,
if that code is not also deleted...

That doesn't matter. Driver init time uart_add_one_port() calls to
register SOC UARTs will result in register_console() being called multiple times unsuccesfully anyway before we get to the PCI UART to which the console is assigned.
All in all, the problem of uninit'ed PCI port spinlock will remain unless
serial core is fixed (ot at least the driver). Proposed driver re-design wouldn't help as this is actually chicken-before-egg type problem -- uart_add_one_port() assumes that the console is setup beforehand, and this just can't happen in case of a PCI card.

WBR, Sergei

Atsushi Nemoto

2005-12-28 04:24 UTC #8 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Russell King

rmk> This should result in a cleaner implementation, and the console rmk> will not be started until you detect the PCI card. Which is the rmk> behaviour you desire in any case.

Thanks for your advise. Though this is not fix the problem as Sergei said, I would refine the driver.

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Sergei Shtylyov

sshtylyov> All in all, the problem of uninit'ed PCI port spinlock
sshtylyov> will remain unless serial core is fixed (ot at least the sshtylyov> driver). Proposed driver re-design wouldn't help as this is sshtylyov> actually chicken-before-egg type problem -- sshtylyov> uart_add_one_port() assumes that the console is setup sshtylyov> beforehand, and this just can't happen in case of a PCI sshtylyov> card.

I agree. Let me clarify a situation.

* The serial_txx9 driver handles both SOC UARTs and PCI UARTs.

* We want to use console_initcall to register console for SOC UARTs.

* PCI UARTs were not detected when console_initcall function was called.

* For PCI UARTs, uart_add_one_port() calls register_console (again) _after_ configuring the port. The spinlock must be initialized before configure the port.

* By first register_console, port->cons->index has been initialized. (even if console setup function failed)

* The uart_console() returns 1 even if the console was not successfully configured (CON_ENABLED was not set and spinlock did not initialized). So uart_add_one_port() does not initialize the spinlock for the console.

There would be 3 solutions:

1. uart_add_one_port() initialize the spinlock for console port without CON_ENABLED.

2. lowlevel driver initialize the spinlock somewhere.

3. register_console() revert console->index to -1 when console->setup function failed. (But register_console is already too complex for me ...)

I would prefer (1), but if not acceptable, I do not object to (2).

This is a refined patch following Russell's advise. This does not include Sergei's fix.

* More strict check in verify_port. Cleanup.
* Do not insert a char caused previous overrun.
* Fix some spin_locks.
* Do not call uart_add_one_port for absent ports.

Signed-off-by Atsushi Nemoto <[email protected]>

drivers/serial/serial_txx9.c +39 ~~-37~~

index f10c86d..37fa0aa 100644
@@ -33,6 +33,10 @@
  *	1.02	Cleanup. (import 8250.c changes)
  *	1.03	Fix low-latency mode. (import 8250.c changes)
  *	1.04	Remove usage of deprecated functions, cleanup.
+ *	1.05	More strict check in verify_port.  Cleanup.
+ *	1.06	Do not insert a char caused previous overrun.
+ *		Fix some spin_locks.
+ *		Do not call uart_add_one_port for absent ports.
  */
 #include <linux/config.h>

@@ -56,7 +60,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-static char *serial_version = "1.04";
+static char *serial_version = "1.06";
 static char *serial_name = "TX39/49 Serial driver";
 
 #define PASS_LIMIT	256

@@ -209,7 +213,7 @@ static inline unsigned int sio_in(struct
 {
 	switch (up->port.iotype) {
 	default:
-		return *(volatile u32 *)(up->port.membase + offset);
+		return __raw_readl(up->port.membase + offset);
 	case UPIO_PORT:
 		return inl(up->port.iobase + offset);
 	}

@@ -220,7 +224,7 @@ sio_out(struct uart_txx9_port *up, int o
 {
 	switch (up->port.iotype) {
 	default:
-		*(volatile u32 *)(up->port.membase + offset) = value;
+		__raw_writel(value, up->port.membase + offset);
 		break;
 	case UPIO_PORT:
 		outl(value, up->port.iobase + offset);

@@ -258,34 +262,19 @@ sio_quot_set(struct uart_txx9_port *up, 
 static void serial_txx9_stop_tx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	sio_mask(up, TXX9_SIDICR, TXX9_SIDICR_TIE);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_start_tx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	sio_set(up, TXX9_SIDICR, TXX9_SIDICR_TIE);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_stop_rx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	up->port.read_status_mask &= ~TXX9_SIDISR_RDIS;
-#if 0
-	sio_mask(up, TXX9_SIDICR, TXX9_SIDICR_RIE);
-#endif
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_enable_ms(struct uart_port *port)

@@ -301,6 +290,7 @@ receive_chars(struct uart_txx9_port *up,
 	unsigned int disr = *status;
 	int max_count = 256;
 	char flag;
+	unsigned int next_ignore_status_mask;
 
 	do {
 		/* The following is not allowed by the tty layer and

@@ -318,6 +308,9 @@ receive_chars(struct uart_txx9_port *up,
 		flag = TTY_NORMAL;
 		up->port.icount.rx++;
 
+		/* mask out RFDN_MASK bit added by previous overrun */
+		next_ignore_status_mask =
+			up->port.ignore_status_mask & ~TXX9_SIDISR_RFDN_MASK;
 		if (unlikely(disr & (TXX9_SIDISR_UBRK | TXX9_SIDISR_UPER |
 				     TXX9_SIDISR_UFER | TXX9_SIDISR_UOER))) {
 			/*

@@ -338,8 +331,17 @@ receive_chars(struct uart_txx9_port *up,
 				up->port.icount.parity++;
 			else if (disr & TXX9_SIDISR_UFER)
 				up->port.icount.frame++;
-			if (disr & TXX9_SIDISR_UOER)
+			if (disr & TXX9_SIDISR_UOER) {
 				up->port.icount.overrun++;
+				/*
+				 * The receiver read buffer still hold
+				 * a char which caused overrun.
+				 * Ignore next char by adding RFDN_MASK
+				 * to ignore_status_mask temporarily.
+				 */
+				next_ignore_status_mask |=
+					TXX9_SIDISR_RFDN_MASK;
+			}
 
 			/*
 			 * Mask off conditions which should be ingored.

@@ -359,6 +361,7 @@ receive_chars(struct uart_txx9_port *up,
 		uart_insert_char(&up->port, disr, TXX9_SIDISR_UOER, ch, flag);
 
 	ignore_char:
+		up->port.ignore_status_mask = next_ignore_status_mask;
 		disr = sio_in(up, TXX9_SIDISR);
 	} while (!(disr & TXX9_SIDISR_UVALID) && (max_count-- > 0));
 	spin_unlock(&up->port.lock);

@@ -460,14 +463,11 @@ static unsigned int serial_txx9_get_mctr
 static void serial_txx9_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
 
-	spin_lock_irqsave(&up->port.lock, flags);
 	if (mctrl & TIOCM_RTS)
 		sio_mask(up, TXX9_SIFLCR, TXX9_SIFLCR_RTSSC);
 	else
 		sio_set(up, TXX9_SIFLCR, TXX9_SIFLCR_RTSSC);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_break_ctl(struct uart_port *port, int break_state)

@@ -794,8 +794,13 @@ static void serial_txx9_config_port(stru
 static int
 serial_txx9_verify_port(struct uart_port *port, struct serial_struct *ser)
 {
-	if (ser->irq < 0 ||
-	    ser->baud_base < 9600 || ser->type != PORT_TXX9)
+	unsigned long new_port = (unsigned long)ser->port +
+		((unsigned long)ser->port_high << ((sizeof(long) - sizeof(int)) * 8));
+	if (ser->type != port->type ||
+	    ser->irq != port->irq ||
+	    ser->io_type != port->iotype ||
+	    new_port != port->iobase ||
+	    (unsigned long)ser->iomem_base != port->mapbase)
 		return -EINVAL;
 	return 0;
 }

@@ -837,7 +842,8 @@ static void __init serial_txx9_register_
 
 		up->port.line = i;
 		up->port.ops = &serial_txx9_pops;
-		uart_add_one_port(drv, &up->port);
+		if (up->port.iobase || up->port.mapbase)
+			uart_add_one_port(drv, &up->port);
 	}
 }

@@ -937,11 +943,6 @@ static int serial_txx9_console_setup(str
 		return -ENODEV;
 
 	/*
-	 * Temporary fix.
-	 */
-	spin_lock_init(&port->lock);
-
-	/*
 	 *	Disable UART interrupts, set DTR and RTS high
 	 *	and set speed.
 	 */

@@ -1051,11 +1052,10 @@ static int __devinit serial_txx9_registe
 	down(&serial_txx9_sem);
 	for (i = 0; i < UART_NR; i++) {
 		uart = &serial_txx9_ports[i];
-		if (uart->port.type == PORT_UNKNOWN)
+		if (!(uart->port.iobase || uart->port.mapbase))
 			break;
 	}
 	if (i < UART_NR) {
-		uart_remove_one_port(&serial_txx9_reg, &uart->port);
 		uart->port.iobase = port->iobase;
 		uart->port.membase = port->membase;
 		uart->port.irq      = port->irq;

@@ -1090,9 +1090,8 @@ static void __devexit serial_txx9_unregi
 	uart->port.type = PORT_UNKNOWN;
 	uart->port.iobase = 0;
 	uart->port.mapbase = 0;
-	uart->port.membase = 0;
+	uart->port.membase = NULL;
 	uart->port.dev = NULL;
-	uart_add_one_port(&serial_txx9_reg, &uart->port);
 	up(&serial_txx9_sem);
 }

@@ -1195,7 +1194,7 @@ static int __init serial_txx9_init(void)
 		serial_txx9_register_ports(&serial_txx9_reg);
 
 #ifdef ENABLE_SERIAL_TXX9_PCI
-		ret = pci_module_init(&serial_txx9_pci_driver);
+		ret = pci_register_driver(&serial_txx9_pci_driver);
 #endif
 	}
 	return ret;

@@ -1208,8 +1207,11 @@ static void __exit serial_txx9_exit(void
 #ifdef ENABLE_SERIAL_TXX9_PCI
 	pci_unregister_driver(&serial_txx9_pci_driver);
 #endif
-	for (i = 0; i < UART_NR; i++)
-		uart_remove_one_port(&serial_txx9_reg, &serial_txx9_ports[i].port);
+	for (i = 0; i < UART_NR; i++) {
+		struct uart_txx9_port *up = &serial_txx9_ports[i];
+		if (up->port.iobase || up->port.mapbase)
+			uart_remove_one_port(&serial_txx9_reg, &up->port);
+	}
 
 	uart_unregister_driver(&serial_txx9_reg);
 }

Atsushi Nemoto

2005-12-29 16:31 UTC #9 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Atsushi Nemoto

anemo> * The uart_console() returns 1 even if the console was not anemo> successfully configured (CON_ENABLED was not set and spinlock anemo> did not initialized). So uart_add_one_port() does not anemo> initialize the spinlock for the console.

This is same for the 8250 driver which also can handle PCI and legacy ports.

The 8250 driver is working correctly just because serial8250_isa_init_ports() initializes all spinlocks. If this way a right way, spin_lock_init() in uart_add_one_port() seems redundant...

---
Atsushi Nemoto

Atsushi Nemoto

2006-01-05 15:12 UTC #10 lore raw

In reply to Russell King

Quoted 6 lines

Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line · Russell King
Yes, you are right. I missed the register_console call in uart_add_one_port(). So your patch will fix the problem. But I suppose the spinlock should be initialized in serial_core. How about this?

rmk> I think you're layering work-around on top of work-around on top rmk> of work-around here.

Russell, could you tell me where is the right place to initialize port's spinlock ?

Currently, serial_core do initialize the spinlock in uart_set_options (for console) and uart_add_one_port (for other ports). And some low level drivers also initialize it in some place. (serial8250_isa_init_ports, etc.)

A. The initialization in serial_core is just for failsafe and low
level drivers should initialize the spinlock.

B. Those initializations in low level drivers are there just by
historical (or any other) reason and now redundant. They can be omitted because serial_core is responsible to initialize the spinlock.

Which is right ? (if both are wrong, could you correct me ?)

I will update and send patch(es) based on your answer. Thank you.

---
Atsushi Nemoto