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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AS8PR04MB8404CB3E77A112C0F44E2EB392239@AS8PR04MB8404.eurprd04.prod.outlook.com>
Date:   Tue, 11 Oct 2022 08:07:28 +0000
From:   Sherry Sun <sherry.sun@....com>
To:     Lukas Wunner <lukas@...ner.de>
CC:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-serial <linux-serial@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"

> -----Original Message-----
> From: Lukas Wunner <lukas@...ner.de>
> Sent: 2022年10月10日 17:10
> To: Sherry Sun <sherry.sun@....com>
> Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>; Greg Kroah-Hartman
> <gregkh@...uxfoundation.org>; Jiri Slaby <jirislaby@...nel.org>; linux-serial
> <linux-serial@...r.kernel.org>; LKML <linux-kernel@...r.kernel.org>; dl-
> linux-imx <linux-imx@....com>
> 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