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

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

[Sergei Shtylylov]

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

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 <sshtylyov@ru.mvista.com>



[-- Attachment #2: GOKU-console-spinlock-fix.patch --]
[-- Type: text/plain, Size: 685 bytes --]

diff --git a/drivers/serial/serial_txx9.c b/drivers/serial/serial_txx9.c
index f10c86d..3f8dc41 100644
--- a/drivers/serial/serial_txx9.c
+++ b/drivers/serial/serial_txx9.c
@@ -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;

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Atsushi Nemoto]

>>>>> On Tue, 27 Dec 2005 01:24:52 +0300, Sergei Shtylylov <sshtylyov@ru.mvista.com> said:
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 <anemo@mba.ocn.ne.jp>


diff --git a/drivers/serial/serial_txx9.c b/drivers/serial/serial_txx9.c
index f10c86d..c24e0c3 100644
--- a/drivers/serial/serial_txx9.c
+++ b/drivers/serial/serial_txx9.c
@@ -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;

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Sergei Shtylylov]

Hello.

Atsushi Nemoto wrote:
>>>>>>On Tue, 27 Dec 2005 01:24:52 +0300, Sergei Shtylylov <sshtylyov@ru.mvista.com> said:
> 
> 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.

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

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

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

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

>  * Call register_console again for PCI ports.

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

> This is a patch against linux-mips GIT.

> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> 
> 
> diff --git a/drivers/serial/serial_txx9.c b/drivers/serial/serial_txx9.c
> index f10c86d..c24e0c3 100644
> --- a/drivers/serial/serial_txx9.c
> +++ b/drivers/serial/serial_txx9.c
> @@ -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.
>  	 */

    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.

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

WBR, Sergei

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Atsushi Nemoto]

Thanks for your comment.

>>>>> On Tue, 27 Dec 2005 16:38:54 +0300, Sergei Shtylylov <sshtylyov@ru.mvista.com> said:

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

--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -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);


>> --- a/drivers/serial/serial_txx9.c
>> +++ b/drivers/serial/serial_txx9.c
>> @@ -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.
>> */

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 <anemo@mba.ocn.ne.jp>

diff --git a/drivers/serial/serial_txx9.c b/drivers/serial/serial_txx9.c
index f10c86d..7f484ad 100644
--- a/drivers/serial/serial_txx9.c
+++ b/drivers/serial/serial_txx9.c
@@ -33,6 +33,9 @@
  *	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.
  */
 #include <linux/config.h>
 
@@ -56,7 +59,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 +212,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 +223,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 +261,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 +289,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 +307,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 +330,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 +360,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 +462,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 +793,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 +941,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.
 	 */
@@ -1090,7 +1089,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 +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;

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Sergei Shtylylov]

Hello.

Atsushi Nemoto wrote:

>>>>>>On Tue, 27 Dec 2005 16:38:54 +0300, Sergei Shtylylov <sshtylyov@ru.mvista.com> said:

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

> --- a/drivers/serial/serial_core.c
> +++ b/drivers/serial/serial_core.c
> @@ -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.

>>>--- a/drivers/serial/serial_txx9.c
>>>+++ b/drivers/serial/serial_txx9.c
>>>@@ -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.
>>>*/

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

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

WBR, Sergei

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Russell King]

On Wed, Dec 28, 2005 at 12:34:57AM +0900, Atsushi Nemoto wrote:
> Thanks for your comment.
> 
> >>>>> On Tue, 27 Dec 2005 16:38:54 +0300, Sergei Shtylylov <sshtylyov@ru.mvista.com> said:
> 
> >> 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?

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.

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

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Sergei Shtylylov]

Hello.

Russell King wrote:

> On Wed, Dec 28, 2005 at 12:34:57AM +0900, Atsushi Nemoto wrote:
> 
>>Thanks for your comment.
>>
>>
>>>>>>>On Tue, 27 Dec 2005 16:38:54 +0300, Sergei Shtylylov <sshtylyov@ru.mvista.com> said:
>>
>>>>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?

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

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

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

> 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

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Sergei Shtylyov]

Hello.

Sergei Shtylylov wrote:

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

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

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

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Atsushi Nemoto]

>>>>> On Tue, 27 Dec 2005 18:41:52 +0000, Russell King <rmk@arm.linux.org.uk> said:
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.

>>>>> On Tue, 27 Dec 2005 22:31:53 +0300, Sergei Shtylyov <sshtylyov@dev.rtsoft.ru> said:
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 <anemo@mba.ocn.ne.jp>

diff --git a/drivers/serial/serial_txx9.c b/drivers/serial/serial_txx9.c
index f10c86d..37fa0aa 100644
--- a/drivers/serial/serial_txx9.c
+++ b/drivers/serial/serial_txx9.c
@@ -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);
 }

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Atsushi Nemoto]

>>>>> On Wed, 28 Dec 2005 13:25:44 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:

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

Re: [PATCH] serial_txx9: forcibly init the spinlock for PCI UART used as a console

[Atsushi Nemoto]

>>>>> On Tue, 27 Dec 2005 18:41:52 +0000, Russell King <rmk@arm.linux.org.uk> said:

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