| Message ID | 20241025105728.602310-1-john.ogness@linutronix.de |
|---|---|
| Headers | show |
| Series | convert 8250 to nbcon | expand |
On Fri, Oct 25, 2024 at 01:03:23PM +0206, John Ogness wrote: > After a console has fed a line into TX, it uses wait_for_xmitr() > to wait until the data has been sent out before returning to the > printk code. However, wait_for_xmitr() will timeout after 10ms, printk here is a function reference or module? For the latter I would use the filename to be sure it's clear, like printk.c. For the former (and it seems you know that) we may use printk(). > regardless if the data has been transmitted or not. > > For single bytes, this timeout is sufficient even at very slow > baud rates, such as 1200bps. However, when FIFO mode is used, > there may be 64 bytes pushed into the FIFO at once. At a baud > rate of 115200bps, the 10ms timeout is still sufficient. > However, when using lower baud rates (such as 57600bps), the > timeout is _not_ sufficient. This causes longer lines to be cut > off, resulting in lost and horribly misformatted output on the > console. > > When using FIFO mode, take the number of bytes into account to > determine an appropriate max timeout. Increasing the timeout maximum (in order not to mix with max() function) > does not affect performance since ideally the timeout never > occurs. ... > /* > * Wait for transmitter & holding register to empty > + * with timeout Can you fix the style while at it? > */ /* Wait for transmitter & holding register to empty with timeout */ ... > static void serial8250_console_fifo_write(struct uart_8250_port *up, > const char *s, unsigned int count) > { > - int i; > const char *end = s + count; > unsigned int fifosize = up->tx_loadsz; > + unsigned int tx_count = 0; > bool cr_sent = false; > + unsigned int i; > > while (s != end) { > - wait_for_lsr(up, UART_LSR_THRE); > + /* Allow timeout for each byte of a possibly full FIFO. */ Does the one-line comment style in this file use periods? If not, drop, otherwise apply it to the above proposal. > + for (i = 0; i < fifosize; i++) { > + if (wait_for_lsr(up, UART_LSR_THRE)) > + break; > + } > + } > + > + /* Allow timeout for each byte written. */ > + for (i = 0; i < tx_count; i++) { > + if (wait_for_lsr(up, UART_LSR_THRE)) > + break; This effectively repeats the above. Even for the fix case I would still add a new helper to deduplicate. > } > }
On Fri, Oct 25, 2024 at 04:45:02PM +0300, Andy Shevchenko wrote: > On Fri, Oct 25, 2024 at 01:03:23PM +0206, John Ogness wrote: > > After a console has fed a line into TX, it uses wait_for_xmitr() > > to wait until the data has been sent out before returning to the > > printk code. However, wait_for_xmitr() will timeout after 10ms, > > printk here is a function reference or module? > For the latter I would use the filename to be sure it's clear, > like printk.c. For the former (and it seems you know that) > we may use printk(). > > > regardless if the data has been transmitted or not. > > > > For single bytes, this timeout is sufficient even at very slow > > baud rates, such as 1200bps. However, when FIFO mode is used, > > there may be 64 bytes pushed into the FIFO at once. At a baud > > rate of 115200bps, the 10ms timeout is still sufficient. > > However, when using lower baud rates (such as 57600bps), the > > timeout is _not_ sufficient. This causes longer lines to be cut > > off, resulting in lost and horribly misformatted output on the > > console. > > > > When using FIFO mode, take the number of bytes into account to > > determine an appropriate max timeout. Increasing the timeout > > maximum > (in order not to mix with max() function) > > > does not affect performance since ideally the timeout never > > occurs. > > ... > > > /* > > * Wait for transmitter & holding register to empty > > + * with timeout > > Can you fix the style while at it? > > > */ > > /* Wait for transmitter & holding register to empty with timeout */ > > ... > > > static void serial8250_console_fifo_write(struct uart_8250_port *up, > > const char *s, unsigned int count) > > { > > - int i; > > const char *end = s + count; > > unsigned int fifosize = up->tx_loadsz; > > + unsigned int tx_count = 0; > > bool cr_sent = false; > > + unsigned int i; > > > > while (s != end) { > > - wait_for_lsr(up, UART_LSR_THRE); > > + /* Allow timeout for each byte of a possibly full FIFO. */ > > Does the one-line comment style in this file use periods? If not, drop, > otherwise apply it to the above proposal. > > > + for (i = 0; i < fifosize; i++) { > > + if (wait_for_lsr(up, UART_LSR_THRE)) > > + break; > > + } > > > + } > > + > > + /* Allow timeout for each byte written. */ > > + for (i = 0; i < tx_count; i++) { > > + if (wait_for_lsr(up, UART_LSR_THRE)) > > + break; > > This effectively repeats the above. Even for the fix case I would still add > a new helper to deduplicate. > > > } > > } Forgot to add, with the above being addressed, feel free to add Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Fri, Oct 25, 2024 at 01:03:26PM +0206, John Ogness wrote: > For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the > console write callback needs to enable/disable TX. It does this > by calling the rs485_start/stop_tx() callbacks. However, these It would be nice if you be consistent across the commit messages and also provide the names of the callbacks in full, because I dunno if we have a local stop_tx() or rs485_start() or whatever the above means. If it is "the rs485_start_tx() / rs485_stop_tx() callbacks.", it's much clearer for the reader. > callbacks will disable/enable interrupts, which is a problem toggle? > for console write, as it must be responsible for > disabling/enabling interrupts. toggling ? > Add an argument @in_con to the rs485_start/stop_tx() callbacks As per above. > to specify if they are being called from console write. If so, > the callbacks will not handle interrupt disabling/enabling. toggling ? > For all call sites other than console write, there is no > functional change. So, why not call the parameter better to emphasize that it's about IRQ toggling? bool toggle_irq ?
On Fri, Oct 25, 2024 at 01:03:27PM +0206, 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. Again, use consistent style for the references to the callbacks. it may be .func, or ->func(), or something else, but make it consistent. > All register access in the callbacks are within unsafe sections. > The write callbacks allow safe handover/takeover per byte and add > a preceding newline if they take 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. ... > +/* > + * 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); > serial_out(up, UART_IER, 0); > } > > +static inline void serial8250_clear_IER(struct uart_8250_port *up) > +{ > + __serial8250_clear_IER(up); Shouldn't this have a lockdep annotation to differentiate with the above? > +} ... > +static void serial8250_console_byte_write(struct uart_8250_port *up, > + struct nbcon_write_context *wctxt) > +{ > + const char *s = READ_ONCE(wctxt->outbuf); > + const char *end = s + READ_ONCE(wctxt->len); Is there any possibility that outbuf value be changed before we get the len and at the end we get the wrong pointer? > + struct uart_port *port = &up->port; > + > + /* > + * 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. > + */ > + while (s != end) { > + if (!nbcon_enter_unsafe(wctxt)) > + return; > + > + uart_console_write(port, s++, 1, serial8250_console_wait_putchar); > + > + if (!nbcon_exit_unsafe(wctxt)) > + return; > } > } ... > +/* > + * irq_work handler to perform modem control. Only triggered via > + * write_atomic() callback because it may be in a scheduler or NMI Also make same style for the callback reference in the comments. > + * context, unable to wake tasks. > + */ ... > 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; Btw, have you run `pahole` on this? Perhaps there are better places for new members?
On Fri, Oct 25, 2024 at 04:31:05PM +0206, John Ogness wrote: > On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> Add an argument @in_con to the rs485_start/stop_tx() callbacks > >> to specify if they are being called from console write. If so, > >> the callbacks will not handle interrupt disabling/enabling. > > > > toggling ? > > > >> For all call sites other than console write, there is no > >> functional change. > > > > So, why not call the parameter better to emphasize that it's about IRQ > > toggling? bool toggle_irq ? > > Currently there are only 2 users: > > serial8250_em485_stop_tx() > bcm2835aux_rs485_stop_tx() > > The first one toggles the IER bits, the second one does not. I figured > it would make more sense to specify the context rather than what needs > to be done and let the 8250-variant decide what it should do. > > But I have no problems renaming it to toggle_irq. It is an 8250-specific > callback with few users. And really the IER bits is the only reason that > the argument even needs to exist. Maybe toggle_ier will be better than? I haven't looked deeply into the implementations, so choose whichever describes better what's behind it.
On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> +/* >> + * 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); > >> serial_out(up, UART_IER, 0); >> } >> >> +static inline void serial8250_clear_IER(struct uart_8250_port *up) >> +{ >> + __serial8250_clear_IER(up); > > Shouldn't this have a lockdep annotation to differentiate with the > above? Yes, but the follow-up patch adds the annotation as a clean "revert patch". I can add a line about that in the commit message. >> +static void serial8250_console_byte_write(struct uart_8250_port *up, >> + struct nbcon_write_context *wctxt) >> +{ >> + const char *s = READ_ONCE(wctxt->outbuf); >> + const char *end = s + READ_ONCE(wctxt->len); > > Is there any possibility that outbuf value be changed before we get > the len and at the end we get the wrong pointer? No. I was concerned about compiler optimization, since @outbuf can become NULL. However, it can only become NULL if ownership was transferred, and that is properly checked anyway. I will remove the READ_ONCE() usage for v4. >> 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; > > Btw, have you run `pahole` on this? Perhaps there are better places > for new members? Indeed there are. Placing it above the MSR_SAVE_FLAGS macro will reduce an existing 3-byte hole to 2-bytes and avoid creating a new 7-byte hole. Thanks. John
On Fri, Oct 25, 2024 at 04:45:02PM +0300, Andy Shevchenko wrote: > On Fri, Oct 25, 2024 at 01:03:23PM +0206, John Ogness wrote: > > After a console has fed a line into TX, it uses wait_for_xmitr() > > to wait until the data has been sent out before returning to the > > printk code. However, wait_for_xmitr() will timeout after 10ms, > > printk here is a function reference or module? > For the latter I would use the filename to be sure it's clear, > like printk.c. For the former (and it seems you know that) > we may use printk(). > > > regardless if the data has been transmitted or not. > > > > For single bytes, this timeout is sufficient even at very slow > > baud rates, such as 1200bps. However, when FIFO mode is used, > > there may be 64 bytes pushed into the FIFO at once. At a baud > > rate of 115200bps, the 10ms timeout is still sufficient. > > However, when using lower baud rates (such as 57600bps), the > > timeout is _not_ sufficient. This causes longer lines to be cut > > off, resulting in lost and horribly misformatted output on the > > console. > > > > When using FIFO mode, take the number of bytes into account to > > determine an appropriate max timeout. Increasing the timeout > > maximum > (in order not to mix with max() function) > > > does not affect performance since ideally the timeout never > > occurs. > > ... > > > /* > > * Wait for transmitter & holding register to empty > > + * with timeout > > Can you fix the style while at it? > > > */ > > /* Wait for transmitter & holding register to empty with timeout */ > > ... > > > static void serial8250_console_fifo_write(struct uart_8250_port *up, > > const char *s, unsigned int count) > > { > > - int i; > > const char *end = s + count; > > unsigned int fifosize = up->tx_loadsz; > > + unsigned int tx_count = 0; > > bool cr_sent = false; > > + unsigned int i; > > > > while (s != end) { > > - wait_for_lsr(up, UART_LSR_THRE); > > + /* Allow timeout for each byte of a possibly full FIFO. */ > > Does the one-line comment style in this file use periods? If not, drop, > otherwise apply it to the above proposal. > > > + for (i = 0; i < fifosize; i++) { > > + if (wait_for_lsr(up, UART_LSR_THRE)) > > + break; > > + } > > > + } > > + > > + /* Allow timeout for each byte written. */ > > + for (i = 0; i < tx_count; i++) { > > + if (wait_for_lsr(up, UART_LSR_THRE)) > > + break; > > This effectively repeats the above. Even for the fix case I would still add > a new helper to deduplicate. +1 With this fixed, Reviewed-by: Wander Lairson Costa <wander@redhat.com> > > > } > > } > > -- > With Best Regards, > Andy Shevchenko > >
This is v3 of a series to convert the 8250 driver to an NBCON console, providing both threaded and atomic printing implementations. v2 of this series is here [0], which also contains additional background information about NBCON consoles in general in the cover letter. To test this version I acquired real hardware (TI AM3358 BeagleBone Black) and tested the following modes: RS232 - no flow control - software flow control (UPF_SOFT_FLOW, UPSTAT_AUTOXOFF) - hardware flow control (UPF_HARD_FLOW, UPSTAT_AUTOCTS, UPSTAT_AUTORTS) - software emulated hardware flow control (UPF_CONS_FLOW, UPSTAT_CTS_ENABLE) RS485 - with SER_RS485_RX_DURING_TX - without SER_RS485_RX_DURING_TX The tests focussed on kernel logging in various combinations of normal, warning, and panic situations. Although not related to the console printing code changes, the tests also included using a getty/login session on the console. Note that this UART (TI16750) supports a 64-byte TX-FIFO, which is used in all console printing modes except for the software emulated hardware flow control. Here are the changes since v2: - For RS485 start/stop TX, specify if called in console context. - For RS485 start/stop TX, when in console context, do not disable/enable interrupts. - Relocate modem_status_handler() to avoid unused static function for some configs. - Move LSR_THRE waiting into a new serial8250_console_wait_putchar() function. - For serial8250_console_fifo_write(), use serial8250_console_putchar() for writing. This allows newline tracking for FIFO mode as well. - For serial8250_console_fifo_write(), allow 10ms timeout for each byte written. - Use FIFO mode for thread and atomic modes when available. - Introduce serial8250_console_byte_write() to handle writing when not using the FIFO mode. - Consolidate thread and atomic callbacks. Now the only difference is modem control: For atomic, called as irq_work. For thread, called direct. John Ogness [0] https://lore.kernel.org/lkml/20240913140538.221708-1-john.ogness@linutronix.de John Ogness (6): serial: 8250: Adjust the timeout for FIFO mode serial: 8250: Use high-level write function for FIFO serial: 8250: Split out rx stop/start code into helpers serial: 8250: Specify console context for rs485_start/stop_tx serial: 8250: Switch to nbcon console serial: 8250: Revert "drop lockdep annotation from serial8250_clear_IER()" drivers/tty/serial/8250/8250.h | 4 +- drivers/tty/serial/8250/8250_bcm2835aux.c | 4 +- drivers/tty/serial/8250/8250_core.c | 35 ++- drivers/tty/serial/8250/8250_omap.c | 2 +- drivers/tty/serial/8250/8250_port.c | 267 +++++++++++++++++----- include/linux/serial_8250.h | 11 +- 6 files changed, 251 insertions(+), 72 deletions(-) base-commit: 44059790a5cb9258ae6137387e4c39b717fd2ced