| 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:
<TY4PR01MB137779148D6C8C537E7108071D7132@TY4PR01MB13777.jpnprd01.prod.outlook.com>
Date: Thu, 9 Jan 2025 01:04:36 +0000
From: "Toshiyuki Sato (Fujitsu)" <fj6611ie@...itsu.com>
To: 'Greg Kroah-Hartman' <gregkh@...uxfoundation.org>, Russell King
<linux@...linux.org.uk>, Jiri Slaby <jirislaby@...nel.org>,
"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>
Subject: RE: [PATCH] serial: amba-pl011: Implement nbcon console
Hi, Greg. Thanks for the comment.
> On Wed, Jan 08, 2025 at 12:47:30AM +0000, Toshiyuki Sato wrote:
> > Implement the callbacks required for an NBCON console [0] on the
> > amba-pl011 console driver.
> > The codes for legacy console are retained, and the module
> > parameter (use_nbcon) allows switching between legacy and NBCON.
> > The default is off (use legacy console) for now.
> >
> > 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).
> >
> > [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/
> >
> > Signed-off-by: Toshiyuki Sato <fj6611ie@...jp.fujitsu.com>
> > ---
> > drivers/tty/serial/amba-pl011.c | 113
> ++++++++++++++++++++++++++++++++
> > 1 file changed, 113 insertions(+)
> >
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 69b7a3e1e..52fab3170 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -41,6 +41,7 @@
> > #include <linux/sizes.h>
> > #include <linux/io.h>
> > #include <linux/acpi.h>
> > +#include <linux/moduleparam.h>
> >
> > #define UART_NR 14
> >
> > @@ -263,6 +264,7 @@ struct uart_amba_port {
> > char type[12];
> > bool rs485_tx_started;
> > unsigned int rs485_tx_drain_interval; /* usecs */
> > + bool console_line_ended;
> > #ifdef CONFIG_DMA_ENGINE
> > /* DMA stuff */
> > unsigned int dmacr; /* dma control reg */
> > @@ -274,6 +276,10 @@ struct uart_amba_port {
> > #endif
> > };
> >
> > +/* if non-zero, the console is nbcon. */
> > +static int use_nbcon;
> > +module_param(use_nbcon, int, 0444);
>
> Why is a module parameter needed here? That feels wrong and not
> scalable at all. What happens if you have multiple devices, which one
> is nbcon and which isn't?
This module parameter is for pl011 driver.
With this patch implemented, only one type will be usable, depending
on the value of use_nbcon.
I thought it would be more user-friendly if legacy operation could be
selected via boot parameter.
Would it be better to make the patch purely nbcon-compatible, without
retaining the legacy functionality?
>
>
> > +
> > static unsigned int pl011_tx_empty(struct uart_port *port);
> >
> > static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap,
> > @@ -2305,6 +2311,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);
> > + uap->console_line_ended = (ch == '\n');
> > }
> >
> > static void
> > @@ -2411,6 +2418,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;
> >
> > @@ -2494,6 +2503,106 @@ 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 void
> > +pl011_console_switch_to_nbcon(struct console *co)
> > +{
> > + co->write = NULL;
> > + co->write_atomic = pl011_console_write_atomic;
> > + co->write_thread = pl011_console_write_thread;
> > + co->device_lock = pl011_console_device_lock;
> > + co->device_unlock = pl011_console_device_unlock;
> > + co->flags = CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON;
> > + pr_info("Serial: switched to nbcon\n");
>
> dev_info()?
If the "use_nbcon" switching function is to be retained, I would like
to change pr_info to dev_info.
>
> thanks,
>
> greg k-h
Regards,
Toshiyuki Sato
Powered by blists - more mailing lists