| Message ID | 20240913140538.221708-4-john.ogness@linutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | convert 8250 to nbcon | expand |
On Fri 2024-09-13 16:11:37, John Ogness wrote: > Implement the necessary callbacks to switch the 8250 console driver > to perform as an nbcon console. > > Add implementations for the nbcon console callbacks (write_atomic, > write_thread, device_lock, device_unlock) and add CON_NBCON to the > initial flags. > > All register access in the callbacks are within unsafe sections. > The write_thread() callback allows safe handover/takeover per byte. > The write_atomic() callback allows safe handover/takeover per > printk record and adds a preceding newline if it took over mid-line. > > For the write_atomic() case, a new irq_work is used to defer modem > control since it may be a context that does not allow waking up > tasks. It would be fair to mention that it does not longer support fifo in the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5 ("serial/8250: Use fifo in 8250 console driver"). It is not usable in write_thread() because it would not allow a safe takeover between emitting particular characters. It might still be used in write_atomic() but it is probably not worth it. This callback is used "only" in emergency and panic situations. > Signed-off-by: John Ogness <john.ogness@linutronix.de> Otherwise, it looks good to me. And it even works fine. With an updated commit message: Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On Wed, Sep 18, 2024 at 02:26:25PM +0200, Petr Mladek wrote: > On Fri 2024-09-13 16:11:37, John Ogness wrote: > > Implement the necessary callbacks to switch the 8250 console driver > > to perform as an nbcon console. > > > > Add implementations for the nbcon console callbacks (write_atomic, > > write_thread, device_lock, device_unlock) and add CON_NBCON to the > > initial flags. > > > > All register access in the callbacks are within unsafe sections. > > The write_thread() callback allows safe handover/takeover per byte. > > The write_atomic() callback allows safe handover/takeover per > > printk record and adds a preceding newline if it took over mid-line. > > > > For the write_atomic() case, a new irq_work is used to defer modem > > control since it may be a context that does not allow waking up > > tasks. > > It would be fair to mention that it does not longer support fifo in > the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5 > ("serial/8250: Use fifo in 8250 console driver"). > > It is not usable in write_thread() because it would not allow > a safe takeover between emitting particular characters. > > It might still be used in write_atomic() but it is probably not > worth it. This callback is used "only" in emergency and panic > situations. This is unfortunate. It will drop down the efficiency of printing. I think it should be done differently, i.e. the takeover the code has to drop FIFO (IIRC it's easy to achieve by disabling it or so) and switch to printing the panic/emergency message. But still at some baud rate speeds draining the FIFO to the other end may be not a bad idea as it takes a few dozens of microseconds.
On Wed 2024-09-18 17:01:27, Andy Shevchenko wrote: > On Wed, Sep 18, 2024 at 02:26:25PM +0200, Petr Mladek wrote: > > On Fri 2024-09-13 16:11:37, John Ogness wrote: > > > Implement the necessary callbacks to switch the 8250 console driver > > > to perform as an nbcon console. > > > > > > Add implementations for the nbcon console callbacks (write_atomic, > > > write_thread, device_lock, device_unlock) and add CON_NBCON to the > > > initial flags. > > > > > > All register access in the callbacks are within unsafe sections. > > > The write_thread() callback allows safe handover/takeover per byte. > > > The write_atomic() callback allows safe handover/takeover per > > > printk record and adds a preceding newline if it took over mid-line. > > > > > > For the write_atomic() case, a new irq_work is used to defer modem > > > control since it may be a context that does not allow waking up > > > tasks. > > > > It would be fair to mention that it does not longer support fifo in > > the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5 > > ("serial/8250: Use fifo in 8250 console driver"). > > > > It is not usable in write_thread() because it would not allow > > a safe takeover between emitting particular characters. > > > > It might still be used in write_atomic() but it is probably not > > worth it. This callback is used "only" in emergency and panic > > situations. > > This is unfortunate. It will drop down the efficiency of printing. The FIFO mode has been added by the commit 8f3631f0f6eb42e5 ("serial/8250: Use fifo in 8250 console driver"). The interesting parts are: <paste> While investigating a bug in the RHEL kernel, I noticed that the serial console throughput is way below the configured speed of 115200 bps in a HP Proliant DL380 Gen9. I was expecting something above 10KB/s, but I got 2.5KB/s. In another machine, I measured a throughput of 11.5KB/s, with the serial controller taking between 80-90us to send each byte. That matches the expected throughput for a configuration of 115200 bps. This patch changes the serial8250_console_write to use the 16550 fifo if available. In my benchmarks I got around 25% improvement in the slow machine, and no performance penalty in the fast machine. </paste> I would translate it: The FIFO mode helped with some buggy serial console. But it helped to gain only small portion of the expected speed. The commit message does not mention any gain with the normally working system. It has been added in 2022. It was considered only because of a "broken" system. Nobody cared enough before. > I think it should be done differently, i.e. the takeover the code > has to drop FIFO (IIRC it's easy to achieve by disabling it or so) > and switch to printing the panic/emergency message. But still at > some baud rate speeds draining the FIFO to the other end may be > not a bad idea as it takes a few dozens of microseconds. Sure. it is doable. But I am not convinced that it is really worth it. Best Regards, Petr
On 2024-09-13, John Ogness <john.ogness@linutronix.de> wrote: > +/* > + * irq_work handler to perform modem control. Only triggered via > + * write_atomic() callback because it may be in a scheduler or NMI > + * context, unable to wake tasks. > + */ > +static void modem_status_handler(struct irq_work *iwp) > +{ > + struct uart_8250_port *up = container_of(iwp, struct uart_8250_port, modem_status_work); > + struct uart_port *port = &up->port; > + > + uart_port_lock(port); > + serial8250_modem_status(up); > + uart_port_unlock(port); > +} As reported [0] by the kernel test robot, I need to move modem_status_handler() down into the "#ifdef CONFIG_SERIAL_8250_CONSOLE" block. John Ogness [0] https://lore.kernel.org/oe-kbuild-all/202409140437.EP0Ryw3u-lkp@intel.com
On 2024-09-18, Petr Mladek <pmladek@suse.com> wrote: > It would be fair to mention that it does not longer support fifo in > the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5 > ("serial/8250: Use fifo in 8250 console driver"). Agreed. > It is not usable in write_thread() because it would not allow > a safe takeover between emitting particular characters. If write_thread could exit_unsafe()/enter_unsafe() while busy-waiting, then emergency/panic could still take over at any time. Even if it means that atomic_write() would need to first wait for the FIFO to drain (which it will). The important thing is that emergency/panic is able to take over. I dropped the optimization to keep things simple for now, but I agree with Andy that it would be unfortunate. I will take a look at what such an implementation could look like. John
On Wed, Sep 18, 2024 at 04:35:06PM +0200, Petr Mladek wrote: > On Wed 2024-09-18 17:01:27, Andy Shevchenko wrote: > > On Wed, Sep 18, 2024 at 02:26:25PM +0200, Petr Mladek wrote: > > > On Fri 2024-09-13 16:11:37, John Ogness wrote: > > > > Implement the necessary callbacks to switch the 8250 console driver > > > > to perform as an nbcon console. > > > > > > > > Add implementations for the nbcon console callbacks (write_atomic, > > > > write_thread, device_lock, device_unlock) and add CON_NBCON to the > > > > initial flags. > > > > > > > > All register access in the callbacks are within unsafe sections. > > > > The write_thread() callback allows safe handover/takeover per byte. > > > > The write_atomic() callback allows safe handover/takeover per > > > > printk record and adds a preceding newline if it took over mid-line. > > > > > > > > For the write_atomic() case, a new irq_work is used to defer modem > > > > control since it may be a context that does not allow waking up > > > > tasks. > > > > > > It would be fair to mention that it does not longer support fifo in > > > the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5 > > > ("serial/8250: Use fifo in 8250 console driver"). > > > > > > It is not usable in write_thread() because it would not allow > > > a safe takeover between emitting particular characters. > > > > > > It might still be used in write_atomic() but it is probably not > > > worth it. This callback is used "only" in emergency and panic > > > situations. > > > > This is unfortunate. It will drop down the efficiency of printing. > > The FIFO mode has been added by the commit 8f3631f0f6eb42e5 > ("serial/8250: Use fifo in 8250 console driver"). The interesting > parts are: > > <paste> > While investigating a bug in the RHEL kernel, I noticed that the serial > console throughput is way below the configured speed of 115200 bps in > a HP Proliant DL380 Gen9. I was expecting something above 10KB/s, but > I got 2.5KB/s. > > In another machine, I measured a throughput of 11.5KB/s, with the serial > controller taking between 80-90us to send each byte. That matches the > expected throughput for a configuration of 115200 bps. > > This patch changes the serial8250_console_write to use the 16550 fifo > if available. In my benchmarks I got around 25% improvement in the slow > machine, and no performance penalty in the fast machine. > </paste> > > I would translate it: > > The FIFO mode helped with some buggy serial console. But it helped to gain > only small portion of the expected speed. The commit message does not > mention any gain with the normally working system. > > It has been added in 2022. It was considered only because of a > "broken" system. Nobody cared enough before. > > > I think it should be done differently, i.e. the takeover the code > > has to drop FIFO (IIRC it's easy to achieve by disabling it or so) > > and switch to printing the panic/emergency message. But still at > > some baud rate speeds draining the FIFO to the other end may be > > not a bad idea as it takes a few dozens of microseconds. > > Sure. it is doable. But I am not convinced that it is really worth it. Fair enough. But perhaps Cc to the author to at least notify them about this change?
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index 5f9f06911795..2d690ff32a32 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -388,12 +388,34 @@ void __init serial8250_register_ports(struct uart_driver *drv, struct device *de #ifdef CONFIG_SERIAL_8250_CONSOLE -static void univ8250_console_write(struct console *co, const char *s, - unsigned int count) +static void univ8250_console_write_atomic(struct console *co, + struct nbcon_write_context *wctxt) { struct uart_8250_port *up = &serial8250_ports[co->index]; - serial8250_console_write(up, s, count); + serial8250_console_write_atomic(up, wctxt); +} + +static void univ8250_console_write_thread(struct console *co, + struct nbcon_write_context *wctxt) +{ + struct uart_8250_port *up = &serial8250_ports[co->index]; + + serial8250_console_write_thread(up, wctxt); +} + +static void univ8250_console_device_lock(struct console *con, unsigned long *flags) +{ + struct uart_port *up = &serial8250_ports[con->index].port; + + __uart_port_lock_irqsave(up, flags); +} + +static void univ8250_console_device_unlock(struct console *con, unsigned long flags) +{ + struct uart_port *up = &serial8250_ports[con->index].port; + + __uart_port_unlock_irqrestore(up, flags); } static int univ8250_console_setup(struct console *co, char *options) @@ -494,12 +516,15 @@ static int univ8250_console_match(struct console *co, char *name, int idx, static struct console univ8250_console = { .name = "ttyS", - .write = univ8250_console_write, + .write_atomic = univ8250_console_write_atomic, + .write_thread = univ8250_console_write_thread, + .device_lock = univ8250_console_device_lock, + .device_unlock = univ8250_console_device_unlock, .device = uart_console_device, .setup = univ8250_console_setup, .exit = univ8250_console_exit, .match = univ8250_console_match, - .flags = CON_PRINTBUFFER | CON_ANYTIME, + .flags = CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON, .index = -1, .data = &serial8250_reg, }; diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 7ee74ec944d2..d58a0fa95e3b 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -691,7 +691,12 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep) serial8250_rpm_put(p); } -static void serial8250_clear_IER(struct uart_8250_port *up) +/* + * Only to be used directly by the console write callbacks, which may not + * require the port lock. Use serial8250_clear_IER() instead for all other + * cases. + */ +static void __serial8250_clear_IER(struct uart_8250_port *up) { if (up->capabilities & UART_CAP_UUE) serial_out(up, UART_IER, UART_IER_UUE); @@ -699,6 +704,11 @@ static void serial8250_clear_IER(struct uart_8250_port *up) serial_out(up, UART_IER, 0); } +static inline void serial8250_clear_IER(struct uart_8250_port *up) +{ + __serial8250_clear_IER(up); +} + #ifdef CONFIG_SERIAL_8250_RSA /* * Attempts to turn on the RSA FIFO. Returns zero on failure. @@ -1864,6 +1874,8 @@ unsigned int serial8250_modem_status(struct uart_8250_port *up) struct uart_port *port = &up->port; unsigned int status = serial_in(up, UART_MSR); + lockdep_assert_held_once(&port->lock); + status |= up->msr_saved_flags; up->msr_saved_flags = 0; if (status & UART_MSR_ANY_DELTA && up->ier & UART_IER_MSI && @@ -1884,6 +1896,21 @@ unsigned int serial8250_modem_status(struct uart_8250_port *up) } EXPORT_SYMBOL_GPL(serial8250_modem_status); +/* + * irq_work handler to perform modem control. Only triggered via + * write_atomic() callback because it may be in a scheduler or NMI + * context, unable to wake tasks. + */ +static void modem_status_handler(struct irq_work *iwp) +{ + struct uart_8250_port *up = container_of(iwp, struct uart_8250_port, modem_status_work); + struct uart_port *port = &up->port; + + uart_port_lock(port); + serial8250_modem_status(up); + uart_port_unlock(port); +} + static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir) { switch (iir & 0x3f) { @@ -3296,6 +3323,11 @@ static void serial8250_console_putchar(struct uart_port *port, unsigned char ch) wait_for_xmitr(up, UART_LSR_THRE); serial_port_out(port, UART_TX, ch); + + if (ch == '\n') + up->console_line_ended = true; + else + up->console_line_ended = false; } /* @@ -3324,65 +3356,68 @@ static void serial8250_console_restore(struct uart_8250_port *up) serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS); } -/* - * Print a string to the serial port using the device FIFO - * - * It sends fifosize bytes and then waits for the fifo - * to get empty. - */ -static void serial8250_console_fifo_write(struct uart_8250_port *up, - const char *s, unsigned int count) +static void __serial8250_console_write_thread(struct uart_8250_port *up, + struct nbcon_write_context *wctxt) { - int i; - const char *end = s + count; - unsigned int fifosize = up->tx_loadsz; - bool cr_sent = false; - - while (s != end) { - wait_for_lsr(up, UART_LSR_THRE); - - for (i = 0; i < fifosize && s != end; ++i) { - if (*s == '\n' && !cr_sent) { - serial_out(up, UART_TX, '\r'); - cr_sent = true; - } else { - serial_out(up, UART_TX, *s++); - cr_sent = false; - } + unsigned int len = READ_ONCE(wctxt->len); + struct uart_port *port = &up->port; + unsigned int i; + + if (nbcon_exit_unsafe(wctxt)) { + /* + * Write out the message. Toggle unsafe for each byte in order + * to give another (higher priority) context the opportunity + * for a friendly takeover. If such a takeover occurs, this + * must abort writing since wctxt->outbuf and wctxt->len are + * no longer valid. + */ + for (i = 0; i < len; i++) { + if (!nbcon_enter_unsafe(wctxt)) + break; + + uart_console_write(port, wctxt->outbuf + i, 1, serial8250_console_putchar); + + if (!nbcon_exit_unsafe(wctxt)) + break; } } + + /* + * If ownership was lost, this context must reacquire ownership in + * order to perform final actions (such as re-enabling interrupts). + */ + while (!nbcon_enter_unsafe(wctxt)) + nbcon_reacquire_nobuf(wctxt); } -/* - * Print a string to the serial port trying not to disturb - * any possible real use of the port... - * - * The console_lock must be held when we get here. - * - * Doing runtime PM is really a bad idea for the kernel console. - * Thus, we assume the function is called when device is powered up. - */ -void serial8250_console_write(struct uart_8250_port *up, const char *s, - unsigned int count) +static void __serial8250_console_write_atomic(struct uart_8250_port *up, + struct nbcon_write_context *wctxt) { - struct uart_8250_em485 *em485 = up->em485; struct uart_port *port = &up->port; - unsigned long flags; - unsigned int ier, use_fifo; - int locked = 1; - touch_nmi_watchdog(); + if (!up->console_line_ended) + uart_console_write(port, "\n", 1, serial8250_console_putchar); + uart_console_write(port, wctxt->outbuf, wctxt->len, serial8250_console_putchar); +} - if (oops_in_progress) - locked = uart_port_trylock_irqsave(port, &flags); - else - uart_port_lock_irqsave(port, &flags); +static void serial8250_console_write(struct uart_8250_port *up, + struct nbcon_write_context *wctxt, + bool is_atomic) +{ + struct uart_8250_em485 *em485 = up->em485; + struct uart_port *port = &up->port; + unsigned int ier; + + if (!nbcon_enter_unsafe(wctxt)) + return; /* - * First save the IER then disable the interrupts + * First save IER then disable the interrupts. The special variant + * to clear IER is used because console printing may occur without + * holding the port lock. */ ier = serial_port_in(port, UART_IER); - serial8250_clear_IER(up); + __serial8250_clear_IER(up); /* check scratch reg to see if port powered off during system sleep */ if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) { @@ -3396,30 +3431,10 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s, mdelay(port->rs485.delay_rts_before_send); } - use_fifo = (up->capabilities & UART_CAP_FIFO) && - /* - * BCM283x requires to check the fifo - * after each byte. - */ - !(up->capabilities & UART_CAP_MINI) && - /* - * tx_loadsz contains the transmit fifo size - */ - up->tx_loadsz > 1 && - (up->fcr & UART_FCR_ENABLE_FIFO) && - port->state && - test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) && - /* - * After we put a data in the fifo, the controller will send - * it regardless of the CTS state. Therefore, only use fifo - * if we don't use control flow. - */ - !(up->port.flags & UPF_CONS_FLOW); - - if (likely(use_fifo)) - serial8250_console_fifo_write(up, s, count); + if (is_atomic) + __serial8250_console_write_atomic(up, wctxt); else - uart_console_write(port, s, count, serial8250_console_putchar); + __serial8250_console_write_thread(up, wctxt); /* * Finally, wait for transmitter to become empty @@ -3442,11 +3457,32 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s, * call it if we have saved something in the saved flags * while processing with interrupts off. */ - if (up->msr_saved_flags) - serial8250_modem_status(up); + if (up->msr_saved_flags) { + /* + * For atomic, it must be deferred to irq_work because this + * may be a context that does not permit waking up tasks. + */ + if (is_atomic) + irq_work_queue(&up->modem_status_work); + else + serial8250_modem_status(up); + } - if (locked) - uart_port_unlock_irqrestore(port, flags); + nbcon_exit_unsafe(wctxt); +} + +void serial8250_console_write_thread(struct uart_8250_port *up, + struct nbcon_write_context *wctxt) +{ + serial8250_console_write(up, wctxt, false); +} + +void serial8250_console_write_atomic(struct uart_8250_port *up, + struct nbcon_write_context *wctxt) +{ + touch_nmi_watchdog(); + + serial8250_console_write(up, wctxt, true); } static unsigned int probe_baud(struct uart_port *port) @@ -3466,6 +3502,7 @@ static unsigned int probe_baud(struct uart_port *port) int serial8250_console_setup(struct uart_port *port, char *options, bool probe) { + struct uart_8250_port *up = up_to_u8250p(port); int baud = 9600; int bits = 8; int parity = 'n'; @@ -3475,6 +3512,9 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe) if (!port->iobase && !port->membase) return -ENODEV; + up->console_line_ended = true; + up->modem_status_work = IRQ_WORK_INIT(modem_status_handler); + if (options) uart_parse_options(options, &baud, &parity, &bits, &flow); else if (probe) diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h index fd59ed2cca53..75c1f93fec73 100644 --- a/include/linux/serial_8250.h +++ b/include/linux/serial_8250.h @@ -152,6 +152,9 @@ struct uart_8250_port { u16 lsr_save_mask; #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA unsigned char msr_saved_flags; + struct irq_work modem_status_work; + + bool console_line_ended; /* line fully output */ struct uart_8250_dma *dma; const struct uart_8250_ops *ops; @@ -202,8 +205,10 @@ void serial8250_tx_chars(struct uart_8250_port *up); unsigned int serial8250_modem_status(struct uart_8250_port *up); void serial8250_init_port(struct uart_8250_port *up); void serial8250_set_defaults(struct uart_8250_port *up); -void serial8250_console_write(struct uart_8250_port *up, const char *s, - unsigned int count); +void serial8250_console_write_atomic(struct uart_8250_port *up, + struct nbcon_write_context *wctxt); +void serial8250_console_write_thread(struct uart_8250_port *up, + struct nbcon_write_context *wctxt); int serial8250_console_setup(struct uart_port *port, char *options, bool probe); int serial8250_console_exit(struct uart_port *port);
Implement the necessary callbacks to switch the 8250 console driver to perform as an nbcon console. Add implementations for the nbcon console callbacks (write_atomic, write_thread, device_lock, device_unlock) and add CON_NBCON to the initial flags. All register access in the callbacks are within unsafe sections. The write_thread() callback allows safe handover/takeover per byte. The write_atomic() callback allows safe handover/takeover per printk record and adds a preceding newline if it took over mid-line. For the write_atomic() case, a new irq_work is used to defer modem control since it may be a context that does not allow waking up tasks. Signed-off-by: John Ogness <john.ogness@linutronix.de> --- drivers/tty/serial/8250/8250_core.c | 35 +++++- drivers/tty/serial/8250/8250_port.c | 188 +++++++++++++++++----------- include/linux/serial_8250.h | 9 +- 3 files changed, 151 insertions(+), 81 deletions(-)