RE: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"

[Sherry Sun]

> -----Original Message-----
> From: Lukas Wunner <lukas@xxxxxxxxx>
> Sent: 2022年10月10日 17:10
> To: Sherry Sun <sherry.sun@xxxxxxx>
> Cc: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby <jirislaby@xxxxxxxxxx>; linux-serial
> <linux-serial@xxxxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; dl-
> linux-imx <linux-imx@xxxxxxx>
> Subject: Re: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
> 
> On Sun, Oct 09, 2022 at 10:23:13AM +0000, Sherry Sun wrote:
> > I am not familiar with 8250 serial, but at least for imx uart driver
> > and lpuart driver, the following behavior is same.
> > For the "real" consoles (everything which is not a bootconsole), the
> > (port)->cons and (port)->cons->index are initialized through
> > uart_add_one_port()->uart_configure_port()->register_console()->
> > try_enable_new_console(), here the console index is assigned by the
> > console cmdline parameters.
> 
> Hm, uart_add_one_port() does the following *before* calling
> uart_configure_port():
> 
> 	/*
> 	 * If this port is in use as a console then the spinlock is already
> 	 * initialised.
> 	 */
> 	if (!uart_console_enabled(uport))
> 		uart_port_spin_lock_init(uport);
> 
> It sounds like in the case of fsl_lpuart.c, the spin lock is *always* initialized,
> even though a concurrent lpuart_console_write() may be holding it.  That's
> not solved by moving lpuart_global_reset() around.

Hi Lukas, 

For the "real" consoles that registered through uart_add_one_port() during uart driver probe, yes, the spin lock is *always* initialized.
And don't worry about the lpuart_console_write() holding the spin lock, as the lpuart_console_write() will only be used after the lpuart_console_setup(), the spin lock has been initialized.
[    1.824177] Call trace:
[    1.826627]  uart_port_spin_lock_init+0x20/0x38
[    1.831177]  uart_add_one_port+0x150/0x5d0
[    1.835290]  lpuart_probe+0x26c/0x484
[    1.838966]  platform_probe+0x68/0xe0

Now the issue is, for the lpuart console port, the uart_console() will only be set correctly in uart_add_one_port(), so if move lpuart_global_reset() before uart_add_one_port(), we cannot recognize the console port, which will cause the console port also been global reset to break the console print.

> 
> The problem with performing lpuart_global_reset() after UART registration is
> that as soon as uart_add_one_port() returns, the port is available for user
> space to use.  So resetting it is a no-go.
> 

Per my understanding, even we don’t do the global reset for the non-console ports after uart_add_one_port(), we still need to initialize the tty ports in lpuart32_startup() when open tty device, it should be safe to add a software reset before initializing the tty ports.

Best Regards
Sherry