| Message ID | 20250323060603.388621-1-ryotkkr98@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | serial: sifive: Switch to nbcon console | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | fail | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | fail | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | fail | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
Hi Ryo, On 2025-03-23, Ryo Takakura <ryotkkr98@gmail.com> wrote: > Add the necessary callbacks(write_atomic, write_thread, device_lock > and device_unlock) and CON_NBCON flag to switch the sifive console > driver to perform as nbcon console. > > Both ->write_atomic() and ->write_thread() will check for console > ownership whenever they are accessing registers. > > The ->device_lock()/unlock() will provide the additional serilization > necessary for ->write_thread() which is called from dedicated printing > thread. > > Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com> This driver has the same issue that the 8250 previously had. The ->startup() and ->shutdown() callbacks are called without the port lock. However, the sifive driver is accessing SIFIVE_SERIAL_IE_OFFS in these callbacks and this register is also accessed by the ->write() callback. This needs to be synchronized. The related 8250 patches fixing this are startup [0] and shutdown [1]. I am assuming the following change would be sufficient: diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c index d032de6199af..1de1b2a5833d 100644 --- a/drivers/tty/serial/sifive.c +++ b/drivers/tty/serial/sifive.c @@ -564,8 +564,11 @@ static void sifive_serial_break_ctl(struct uart_port *port, int break_state) static int sifive_serial_startup(struct uart_port *port) { struct sifive_serial_port *ssp = port_to_sifive_serial_port(port); + unsigned long flags; + uart_port_lock_irqsave(&ssp->port, &flags); __ssp_enable_rxwm(ssp); + uart_port_unlock_irqrestore(&ssp->port, flags); return 0; } @@ -573,9 +576,12 @@ static int sifive_serial_startup(struct uart_port *port) static void sifive_serial_shutdown(struct uart_port *port) { struct sifive_serial_port *ssp = port_to_sifive_serial_port(port); + unsigned long flags; + uart_port_lock_irqsave(&ssp->port, &flags); __ssp_disable_rxwm(ssp); __ssp_disable_txwm(ssp); + uart_port_unlock_irqrestore(&ssp->port, flags); } /** The fix should be applied first (and likely Cc stable) since it is fixing an existing mainline problem. Your patch also needs the synchronization. The ->write_atomic() callback does not use the port lock. However, the uart_port_*() functions also take the nbcon console ownership, so they synchronize against ->write_atomic() callbacks. Otherwise, this patch looks good. If the ->startup() and ->shutdown() callbacks are fixed in a previous patch, feel free to add: Reviewed-by: John Ogness <john.ogness@linutronix.de> to this patch. John Ogness [0] https://lore.kernel.org/lkml/20230525093159.223817-2-john.ogness@linutronix.de [1] https://lore.kernel.org/lkml/20230525093159.223817-9-john.ogness@linutronix.de
Hi John, On Mon, 24 Mar 2025 16:30:20 +0106, John Ogness wrote: >On 2025-03-23, Ryo Takakura <ryotkkr98@gmail.com> wrote: >> Add the necessary callbacks(write_atomic, write_thread, device_lock >> and device_unlock) and CON_NBCON flag to switch the sifive console >> driver to perform as nbcon console. >> >> Both ->write_atomic() and ->write_thread() will check for console >> ownership whenever they are accessing registers. >> >> The ->device_lock()/unlock() will provide the additional serilization >> necessary for ->write_thread() which is called from dedicated printing >> thread. >> >> Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com> > >This driver has the same issue that the 8250 previously had. The >->startup() and ->shutdown() callbacks are called without the port >lock. However, the sifive driver is accessing SIFIVE_SERIAL_IE_OFFS in >these callbacks and this register is also accessed by the ->write() >callback. This needs to be synchronized. I see. Thanks for pointing out. (I didn't know about console_on_rootfs() and what it does. Interesting!) >The related 8250 patches fixing this are startup [0] and shutdown [1]. I >am assuming the following change would be sufficient: > >diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c >index d032de6199af..1de1b2a5833d 100644 >--- a/drivers/tty/serial/sifive.c >+++ b/drivers/tty/serial/sifive.c >@@ -564,8 +564,11 @@ static void sifive_serial_break_ctl(struct uart_port *port, int break_state) > static int sifive_serial_startup(struct uart_port *port) > { > struct sifive_serial_port *ssp = port_to_sifive_serial_port(port); >+ unsigned long flags; > >+ uart_port_lock_irqsave(&ssp->port, &flags); > __ssp_enable_rxwm(ssp); >+ uart_port_unlock_irqrestore(&ssp->port, flags); > > return 0; > } >@@ -573,9 +576,12 @@ static int sifive_serial_startup(struct uart_port *port) > static void sifive_serial_shutdown(struct uart_port *port) > { > struct sifive_serial_port *ssp = port_to_sifive_serial_port(port); >+ unsigned long flags; > >+ uart_port_lock_irqsave(&ssp->port, &flags); > __ssp_disable_rxwm(ssp); > __ssp_disable_txwm(ssp); >+ uart_port_unlock_irqrestore(&ssp->port, flags); > } > > /** > >The fix should be applied first (and likely Cc stable) since it is >fixing an existing mainline problem. Ok, I will add a patch in the next version and cc stable. >Your patch also needs the synchronization. The ->write_atomic() callback >does not use the port lock. However, the uart_port_*() functions also >take the nbcon console ownership, so they synchronize against >->write_atomic() callbacks. I see, nice! >Otherwise, this patch looks good. If the ->startup() and ->shutdown() >callbacks are fixed in a previous patch, feel free to add: > >Reviewed-by: John Ogness <john.ogness@linutronix.de> > >to this patch. I'll add in the next verison. Thanks for reviewing! Sincerely, Ryo Takakura >John Ogness > >[0] https://lore.kernel.org/lkml/20230525093159.223817-2-john.ogness@linutronix.de >[1] https://lore.kernel.org/lkml/20230525093159.223817-9-john.ogness@linutronix.de
diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c index 5904a2d4c..d032de619 100644 --- a/drivers/tty/serial/sifive.c +++ b/drivers/tty/serial/sifive.c @@ -151,6 +151,7 @@ struct sifive_serial_port { unsigned long baud_rate; struct clk *clk; struct notifier_block clk_notifier; + bool console_line_ended; }; /* @@ -779,33 +780,88 @@ static void sifive_serial_console_putchar(struct uart_port *port, unsigned char __ssp_wait_for_xmitr(ssp); __ssp_transmit_char(ssp, ch); + + ssp->console_line_ended = (ch == '\n'); +} + +static void sifive_serial_device_lock(struct console *co, unsigned long *flags) +{ + struct uart_port *up = &sifive_serial_console_ports[co->index]->port; + + return __uart_port_lock_irqsave(up, flags); +} + +static void sifive_serial_device_unlock(struct console *co, unsigned long flags) +{ + struct uart_port *up = &sifive_serial_console_ports[co->index]->port; + + return __uart_port_unlock_irqrestore(up, flags); } -static void sifive_serial_console_write(struct console *co, const char *s, - unsigned int count) +static void sifive_serial_console_write_atomic(struct console *co, + struct nbcon_write_context *wctxt) { struct sifive_serial_port *ssp = sifive_serial_console_ports[co->index]; - unsigned long flags; + struct uart_port *port = &ssp->port; unsigned int ier; - int locked = 1; if (!ssp) return; - if (oops_in_progress) - locked = uart_port_trylock_irqsave(&ssp->port, &flags); - else - uart_port_lock_irqsave(&ssp->port, &flags); + if (!nbcon_enter_unsafe(wctxt)) + return; ier = __ssp_readl(ssp, SIFIVE_SERIAL_IE_OFFS); __ssp_writel(0, SIFIVE_SERIAL_IE_OFFS, ssp); - uart_console_write(&ssp->port, s, count, sifive_serial_console_putchar); + if (!ssp->console_line_ended) + uart_console_write(port, "\n", 1, sifive_serial_console_putchar); + uart_console_write(port, wctxt->outbuf, wctxt->len, + sifive_serial_console_putchar); __ssp_writel(ier, SIFIVE_SERIAL_IE_OFFS, ssp); - if (locked) - uart_port_unlock_irqrestore(&ssp->port, flags); + nbcon_exit_unsafe(wctxt); +} + +static void sifive_serial_console_write_thread(struct console *co, + struct nbcon_write_context *wctxt) +{ + struct sifive_serial_port *ssp = sifive_serial_console_ports[co->index]; + struct uart_port *port = &ssp->port; + unsigned int ier; + + if (!ssp) + return; + + if (!nbcon_enter_unsafe(wctxt)) + return; + + ier = __ssp_readl(ssp, SIFIVE_SERIAL_IE_OFFS); + __ssp_writel(0, SIFIVE_SERIAL_IE_OFFS, ssp); + + if (nbcon_exit_unsafe(wctxt)) { + int len = READ_ONCE(wctxt->len); + int i; + + for (i = 0; i < len; i++) { + if (!nbcon_enter_unsafe(wctxt)) + break; + + uart_console_write(port, wctxt->outbuf + i, 1, + sifive_serial_console_putchar); + + if (!nbcon_exit_unsafe(wctxt)) + break; + } + } + + while (!nbcon_enter_unsafe(wctxt)) + nbcon_reacquire_nobuf(wctxt); + + __ssp_writel(ier, SIFIVE_SERIAL_IE_OFFS, ssp); + + nbcon_exit_unsafe(wctxt); } static int sifive_serial_console_setup(struct console *co, char *options) @@ -823,6 +879,8 @@ static int sifive_serial_console_setup(struct console *co, char *options) if (!ssp) return -ENODEV; + ssp->console_line_ended = true; + if (options) uart_parse_options(options, &baud, &parity, &bits, &flow); @@ -833,10 +891,13 @@ static struct uart_driver sifive_serial_uart_driver; static struct console sifive_serial_console = { .name = SIFIVE_TTY_PREFIX, - .write = sifive_serial_console_write, + .write_atomic = sifive_serial_console_write_atomic, + .write_thread = sifive_serial_console_write_thread, + .device_lock = sifive_serial_device_lock, + .device_unlock = sifive_serial_device_unlock, .device = uart_console_device, .setup = sifive_serial_console_setup, - .flags = CON_PRINTBUFFER, + .flags = CON_PRINTBUFFER | CON_NBCON, .index = -1, .data = &sifive_serial_uart_driver, };
Add the necessary callbacks(write_atomic, write_thread, device_lock and device_unlock) and CON_NBCON flag to switch the sifive console driver to perform as nbcon console. Both ->write_atomic() and ->write_thread() will check for console ownership whenever they are accessing registers. The ->device_lock()/unlock() will provide the additional serilization necessary for ->write_thread() which is called from dedicated printing thread. Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com> --- Hi! I referred to the already converted console drivers 8250 [0], imx [1] and amba-pl011 [2]. I tested the patch for each of the NBCON priority (PANIC, EMERGENCY and NORMAL) on riscv qemu. Hope to get feedbacks, thanks! Sincerely, Ryo Takakura [0] https://lore.kernel.org/lkml/20250107212702.169493-1-john.ogness@linutronix.de/ [1] https://lore.kernel.org/linux-arm-kernel/20240913-serial-imx-nbcon-v3-1-4c627302335b@geanix.com/ [2] https://lore.kernel.org/linux-serial/20250204044428.2191983-1-fj6611ie@aa.jp.fujitsu.com/ --- drivers/tty/serial/sifive.c | 87 +++++++++++++++++++++++++++++++------ 1 file changed, 74 insertions(+), 13 deletions(-)