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

[Sergei Shtylylov <sshtylyov@ru.mvista.com>]

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