Hi Prasad,
On 02/17/2015 06:22 PM, prasad@xxxxxxxxxx wrote:
> From: Prasad Koya <prasad@xxxxxxxxxx>
>
> At slow baud rates, say 9600, it is easy to run into tty buffer overrun
> as interrupts could get disabled for a long time. To avoid that, if
> UART_LSR_DR is set, pick up bytes from rx fifo while waiting for tx
> buffer to be empty. Stop buffering if port->sysrq is true or
> oops_in_progress is set.
Some of the original information you supplied has been lost in this commit
log. I think it would be helpful if you detailed how the overrun error can
happen, like you did before:
> On 09/30/2014 01:30 PM, Prasad Koya wrote:
>> At 9600 baud, we are seeing buffer overruns on our 16550A UARTs. If a
>> command, say 40 chars long, is sent to console and if printk to
>> console happens at same time, we running into receive buffer overrun.
>> Thats because, at 9600 baud, it takes about a millisecond to xmit 1
>> char and with receive FIFO only 16 chars deep, it is very easy to
>> cause receive buffer overrun.
What exactly is the use case here? I ask because
1. the patch is trying to fix overrun during a random printk but
makes no effort to fix overrun during the potentially much, much
longer sysrq printk.
2. I'm struggling to envision the circumstances where a printk is
happening frequently enough to collide with large tty input. Heaviest
printk usage is during boot but that is also typically the lightest
tty input load.
Do you have a test case that I can use to reproduce this?
Why is the line speed so slow?
> George Spelvin reworked original patch quite a bit.
>
> Tested on 16550 UART with 16byte FIFO
>
> Signed-off-by: Prasad Koya <prasad@xxxxxxxxxx>
> Signed-off-by: George Spelvin <linux@xxxxxxxxxxx>
> ---
> drivers/tty/serial/8250/8250_core.c | 65 +++++++++++++++++++++++++++++++++----
> drivers/tty/serial/8250/8250_fsl.c | 2 +-
> include/linux/serial_8250.h | 3 +-
> include/linux/serial_core.h | 2 +-
> 4 files changed, 63 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 11c6685..a9adf9d 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1434,9 +1434,12 @@ static void serial8250_enable_ms(struct uart_port *port)
> * serial8250_rx_chars: processes according to the passed in LSR
> * value, and returns the remaining LSR bits not handled
> * by this Rx routine.
> + * no_sysrq is normally false, but when set it indicates we should stop
> + * processing input after receiving a break, rather than passing the
> + * next character to uart_handle_sysrq_char.
> */
> unsigned char
> -serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
> +serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr, bool no_sysrq)
> {
> struct uart_port *port = &up->port;
> unsigned char ch;
> @@ -1472,8 +1475,11 @@ serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
> * may get masked by ignore_status_mask
> * or read_status_mask.
> */
> - if (uart_handle_break(port))
> + if (uart_handle_break(port)) {
> + if (no_sysrq)
> + max_count = 0;
> goto ignore_char;
> + }
None of this should be necessary because error and break handling should not
be performed from the console write; if LSR & UART_LSR_BRK_ERROR_BITS then
rx should cease (a non-zero lsr_saved_flags in wait_for_xmitr() is equivalent).
> } else if (lsr & UART_LSR_PE)
> port->icount.parity++;
> else if (lsr & UART_LSR_FE)
> @@ -1609,7 +1615,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
> dma_err = up->dma->rx_dma(up, iir);
>
> if (!up->dma || dma_err)
> - status = serial8250_rx_chars(up, status);
> + status = serial8250_rx_chars(up, status, false);
> }
> serial8250_modem_status(up);
> if ((!up->dma || (up->dma && up->dma->tx_err)) &&
> @@ -1962,9 +1968,30 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state)
> }
>
> /*
> + * In some special situations, we allow console output without necessarily
> + * locking the port. The same situations also suppress processing input
> + * during polling output, because we aren't ready to deal with it.
> + * So just hope the hardware FIFO can hold until interrupts are
> + * re-enabled.
> + */
> +static bool emergency_situation(const struct uart_port *port)
> +{
> +#ifdef SUPPORT_SYSRQ
> + return port->sysrq != 0 || oops_in_progress;
> +#else
> + (void)port;
> + return oops_in_progress;
> +#endif
> +}
This should just go in the single function that reads 1 char while
waiting for xmitr (and that function should only compile for
CONFIG_SERIAL_8250_CONSOLE so the SUPPORT_SYSRQ can be dropped).
> +/*
> * Wait for transmitter & holding register to empty
> + * If rx is true, then also poll for received characters if it's
> + * safe to process them. This is only used in the SUPPORT_SYSRQ
> + * case, and hopefully the compiler can figure out it's dead code
> + * if that's not the case.
> */
> -static void wait_for_xmitr(struct uart_8250_port *up, int bits)
> +static void __wait_for_xmitr(struct uart_8250_port *up, int bits, bool rx)
> {
> unsigned int status, tmout = 10000;
>
> @@ -1978,6 +2005,11 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
> break;
> if (--tmout == 0)
> break;
> +
> + if (rx && status & (UART_LSR_DR | UART_LSR_BI) &&
> + !emergency_situation(&up->port))
> + serial8250_rx_chars(up, status, true);
> +
serial8250_rx_chars() should not be used in this context because it loops
as long as there is data to be read. That is unacceptably long with
large fifos and fast line speeds.
serial8250_rx_chars() will have to be refactored so that just a single char
can be read from here. And what I said about not trying to handle anything
other than data ready.
> udelay(1);
> }
>
> @@ -1989,12 +2021,29 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
> up->msr_saved_flags |= msr & MSR_SAVE_FLAGS;
> if (msr & UART_MSR_CTS)
> break;
> +
> + if (rx && !emergency_situation(&up->port)) {
> + status = serial_in(up, UART_LSR);
> +
> + up->lsr_saved_flags |= status & LSR_SAVE_FLAGS;
> + if (status & (UART_LSR_DR | UART_LSR_BI))
> + serial8250_rx_chars(up, status, true);
> + }
> +
Is UPF_CONS_FLOW part of the use case? If not, please drop.
> udelay(1);
> touch_nmi_watchdog();
> }
> }
> }
>
> +/*
> + * Wait for transmitter & holding register to empty
> + */
Comment not necessary; this is obviously only a wrapper function
that performs no work other than what it wraps.
> +static void wait_for_xmitr(struct uart_8250_port *up, int bits)
> +{
> + __wait_for_xmitr(up, bits, false);
> +}
bool parameters convey no information at the call site; alternative
solutions are:
1. Pass UART_LSR_DR in the 'bits' parameter
2. use function names to differentiate behavior and the leading double
underscore flavor to perform the actual work.
> +
> #ifdef CONFIG_CONSOLE_POLL
> /*
> * Console polling routines for writing and reading from the uart while
> @@ -3168,7 +3217,7 @@ static void serial8250_console_putchar(struct uart_port *port, int ch)
> {
> struct uart_8250_port *up = up_to_u8250p(port);
>
> - wait_for_xmitr(up, UART_LSR_THRE);
> + __wait_for_xmitr(up, UART_LSR_THRE, true);
Why is rx performed unconditionally if the problem set is limited
to slow line speeds (I'm not convinced it is limited to slow
line speeds though)?
What other port types and line speeds has this been tested on besides
your 16550A design?
> serial_port_out(port, UART_TX, ch);
> }
>
> @@ -3176,6 +3225,10 @@ static void serial8250_console_putchar(struct uart_port *port, int ch)
> * Print a string to the serial port trying not to disturb
> * any possible real use of the port...
> *
> + * In order to avoid dropped input if the output is longer than
> + * the available hardware FIFO space, we try to handle incoming
> + * characters during this polling loop.
> + *
> * The console_lock must be held when we get here.
> */
> static void
> @@ -3214,7 +3267,7 @@ serial8250_console_write(struct console *co, const char *s, unsigned int count)
> * Finally, wait for transmitter to become empty
> * and restore the IER
> */
> - wait_for_xmitr(up, BOTH_EMPTY);
> + __wait_for_xmitr(up, BOTH_EMPTY, true);
Is rx actually necessary here? How big is the tx shifter? Because the tx fifo
can have at most 1 char right now and any pending rx was done just before that.
> serial_port_out(port, UART_IER, ier);
>
> /*
> diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
> index c0533a5..25f0aa3 100644
> --- a/drivers/tty/serial/8250/8250_fsl.c
> +++ b/drivers/tty/serial/8250/8250_fsl.c
> @@ -49,7 +49,7 @@ int fsl8250_handle_irq(struct uart_port *port)
> lsr = orig_lsr = up->port.serial_in(&up->port, UART_LSR);
>
> if (lsr & (UART_LSR_DR | UART_LSR_BI))
> - lsr = serial8250_rx_chars(up, lsr);
> + lsr = serial8250_rx_chars(up, lsr, false);
>
> serial8250_modem_status(up);
>
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index e02acf0..8544bc5 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -128,7 +128,8 @@ extern void serial8250_do_pm(struct uart_port *port, unsigned int state,
> unsigned int oldstate);
> extern int fsl8250_handle_irq(struct uart_port *port);
> int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
> -unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr);
> +unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr,
> + bool no_sysrq);
> void serial8250_tx_chars(struct uart_8250_port *up);
> unsigned int serial8250_modem_status(struct uart_8250_port *up);
>
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 057038c..c93c5aa 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -410,7 +410,7 @@ uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
> if (port->sysrq) {
> if (ch && time_before(jiffies, port->sysrq)) {
> handle_sysrq(ch);
> - port->sysrq = 0;
> + port->sysrq = 0; /* Must be AFTER handle_sysrq */
Not necessary.
> return 1;
> }
> port->sysrq = 0;
>
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
