| 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
| ||
|
Message-ID:
<TY4PR01MB13777DB55F2231F7399E42609D7D62@TY4PR01MB13777.jpnprd01.prod.outlook.com>
Date: Mon, 10 Mar 2025 02:23:35 +0000
From: "Toshiyuki Sato (Fujitsu)" <fj6611ie@...itsu.com>
To: Russell King <linux@...linux.org.uk>, Greg Kroah-Hartman
<gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "Toshiyuki Sato (Fujitsu)"
<fj6611ie@...itsu.com>
Subject: RE: [PATCH v3] serial: amba-pl011: Implement nbcon console
Dear Reviewers,
It has been about a month since this patch was merged into tty-next -> linux-next.
I would appreciate any comments or feedback on this patch.
Regards.
Toshiyuki Sato
> Implement the callbacks required for an NBCON console [0] on the
> amba-pl011 console driver.
>
> Referred to the NBCON implementation work for 8250 [1] and imx [2].
>
> The normal-priority write_thread checks for console ownership each time a
> character is printed.
> write_atomic holds the console ownership until the entire string is printed.
>
> UART register operations are protected from other contexts by uart_port_lock,
> except for a final flush(nbcon_atomic_flush_unsafe) on panic.
>
> The patch has been verified to correctly handle the output and competition of
> messages with different priorities and flushing panic message to console after
> nmi panic using ARM64 QEMU and a physical machine(A64FX).
>
> Stress testing was conducted on a physical machine(A64FX).
> The results are as follows:
>
> - The output speed and volume of messages using the NBCON console were
> comparable to the legacy console (data suggests a slight improvement).
>
> - When inducing a panic (sysrq-triggered or NMI) under heavy contention
> on the serial console output,
> the legacy console resulted in the loss of some or all crash messages.
> However, using the NBCON console, no message loss occurred.
>
> This testing referenced the NBCON console work for 8250 [3].
>
> [0] https://lore.kernel.org/all/ZuRRTbapH0DCj334@pathway.suse.cz/
> [1]
> https://lore.kernel.org/all/20240913140538.221708-1-john.ogness@linutronix.d
> e/T/
> [2]
> https://lore.kernel.org/linux-arm-kernel/20240913-serial-imx-nbcon-v3-1-4c62
> 7302335b@...nix.com/T/
> [3]
> https://lore.kernel.org/lkml/ZsdoD6PomBRsB-ow@debarbos-thinkpadt14sgen2
> i.remote.csb/#t
>
> Signed-off-by: Toshiyuki Sato <fj6611ie@...jp.fujitsu.com>
> ---
> This patch removes the legacy console code.
> Please comment if you have any concerns.
>
> Changes in v3:
> - Add stress test results to patch comments.
> - based on tty-next branch
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
> v2:
> https://lore.kernel.org/all/20250115052749.3205675-1-fj6611ie@aa.jp.fujitsu.c
> om/
>
> Changes in v2:
> - Remove the module parameter used to switch between legacy and NBCON.
> - Remove codes for legacy console.
> - Fix build errors detected by the test robot.
> - based on 6.13-rc7
> v1:
> https://lore.kernel.org/all/20250108004730.2302996-1-fj6611ie@aa.jp.fujitsu.c
> om/
>
> Thanks Greg for the comment.
> ---
> drivers/tty/serial/amba-pl011.c | 143
> ++++++++++++++++++++++----------
> 1 file changed, 97 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 04212c823..9a9a1d630 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -272,6 +272,7 @@ struct uart_amba_port {
> enum pl011_rs485_tx_state rs485_tx_state;
> struct hrtimer trigger_start_tx;
> struct hrtimer trigger_stop_tx;
> + bool console_line_ended;
> #ifdef CONFIG_DMA_ENGINE
> /* DMA stuff */
> unsigned int dmacr; /* dma control reg */
> @@ -2366,50 +2367,7 @@ static void pl011_console_putchar(struct uart_port
> *port, unsigned char ch)
> while (pl011_read(uap, REG_FR) & UART01x_FR_TXFF)
> cpu_relax();
> pl011_write(ch, uap, REG_DR);
> -}
> -
> -static void
> -pl011_console_write(struct console *co, const char *s, unsigned int count) -{
> - struct uart_amba_port *uap = amba_ports[co->index];
> - unsigned int old_cr = 0, new_cr;
> - unsigned long flags;
> - int locked = 1;
> -
> - clk_enable(uap->clk);
> -
> - if (oops_in_progress)
> - locked = uart_port_trylock_irqsave(&uap->port, &flags);
> - else
> - uart_port_lock_irqsave(&uap->port, &flags);
> -
> - /*
> - * First save the CR then disable the interrupts
> - */
> - if (!uap->vendor->always_enabled) {
> - old_cr = pl011_read(uap, REG_CR);
> - new_cr = old_cr & ~UART011_CR_CTSEN;
> - new_cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
> - pl011_write(new_cr, uap, REG_CR);
> - }
> -
> - uart_console_write(&uap->port, s, count, pl011_console_putchar);
> -
> - /*
> - * Finally, wait for transmitter to become empty and restore the
> - * TCR. Allow feature register bits to be inverted to work around
> - * errata.
> - */
> - while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr)
> - & uap->vendor->fr_busy)
> - cpu_relax();
> - if (!uap->vendor->always_enabled)
> - pl011_write(old_cr, uap, REG_CR);
> -
> - if (locked)
> - uart_port_unlock_irqrestore(&uap->port, flags);
> -
> - clk_disable(uap->clk);
> + uap->console_line_ended = (ch == '\n');
> }
>
> static void pl011_console_get_options(struct uart_amba_port *uap, int *baud,
> @@ -2472,6 +2430,8 @@ static int pl011_console_setup(struct console *co, char
> *options)
> if (ret)
> return ret;
>
> + uap->console_line_ended = true;
> +
> if (dev_get_platdata(uap->port.dev)) {
> struct amba_pl011_data *plat;
>
> @@ -2555,14 +2515,105 @@ static int pl011_console_match(struct console *co,
> char *name, int idx,
> return -ENODEV;
> }
>
> +static void
> +pl011_console_write_atomic(struct console *co, struct
> +nbcon_write_context *wctxt) {
> + struct uart_amba_port *uap = amba_ports[co->index];
> + unsigned int old_cr = 0;
> +
> + if (!nbcon_enter_unsafe(wctxt))
> + return;
> +
> + clk_enable(uap->clk);
> +
> + if (!uap->vendor->always_enabled) {
> + old_cr = pl011_read(uap, REG_CR);
> + pl011_write((old_cr & ~UART011_CR_CTSEN) |
> (UART01x_CR_UARTEN | UART011_CR_TXE),
> + uap, REG_CR);
> + }
> +
> + if (!uap->console_line_ended)
> + uart_console_write(&uap->port, "\n", 1,
> pl011_console_putchar);
> + uart_console_write(&uap->port, wctxt->outbuf, wctxt->len,
> +pl011_console_putchar);
> +
> + while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) &
> uap->vendor->fr_busy)
> + cpu_relax();
> +
> + if (!uap->vendor->always_enabled)
> + pl011_write(old_cr, uap, REG_CR);
> +
> + clk_disable(uap->clk);
> +
> + nbcon_exit_unsafe(wctxt);
> +}
> +
> +static void
> +pl011_console_write_thread(struct console *co, struct
> +nbcon_write_context *wctxt) {
> + struct uart_amba_port *uap = amba_ports[co->index];
> + unsigned int old_cr = 0;
> +
> + if (!nbcon_enter_unsafe(wctxt))
> + return;
> +
> + clk_enable(uap->clk);
> +
> + if (!uap->vendor->always_enabled) {
> + old_cr = pl011_read(uap, REG_CR);
> + pl011_write((old_cr & ~UART011_CR_CTSEN) |
> (UART01x_CR_UARTEN | UART011_CR_TXE),
> + uap, REG_CR);
> + }
> +
> + if (nbcon_exit_unsafe(wctxt)) {
> + int i;
> + unsigned int len = READ_ONCE(wctxt->len);
> +
> + for (i = 0; i < len; i++) {
> + if (!nbcon_enter_unsafe(wctxt))
> + break;
> + uart_console_write(&uap->port, wctxt->outbuf + i, 1,
> pl011_console_putchar);
> + if (!nbcon_exit_unsafe(wctxt))
> + break;
> + }
> + }
> +
> + while (!nbcon_enter_unsafe(wctxt))
> + nbcon_reacquire_nobuf(wctxt);
> +
> + while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) &
> uap->vendor->fr_busy)
> + cpu_relax();
> +
> + if (!uap->vendor->always_enabled)
> + pl011_write(old_cr, uap, REG_CR);
> +
> + clk_disable(uap->clk);
> +
> + nbcon_exit_unsafe(wctxt);
> +}
> +
> +static void
> +pl011_console_device_lock(struct console *co, unsigned long *flags) {
> + __uart_port_lock_irqsave(&amba_ports[co->index]->port, flags); }
> +
> +static void
> +pl011_console_device_unlock(struct console *co, unsigned long flags) {
> + __uart_port_unlock_irqrestore(&amba_ports[co->index]->port, flags); }
> +
> static struct uart_driver amba_reg;
> static struct console amba_console = {
> .name = "ttyAMA",
> - .write = pl011_console_write,
> .device = uart_console_device,
> .setup = pl011_console_setup,
> .match = pl011_console_match,
> - .flags = CON_PRINTBUFFER | CON_ANYTIME,
> + .write_atomic = pl011_console_write_atomic,
> + .write_thread = pl011_console_write_thread,
> + .device_lock = pl011_console_device_lock,
> + .device_unlock = pl011_console_device_unlock,
> + .flags = CON_PRINTBUFFER | CON_ANYTIME |
> CON_NBCON,
> .index = -1,
> .data = &amba_reg,
> };
>
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> --
> 2.34.1
Powered by blists - more mailing lists