On Wed, Feb 18, 2015 at 12:35 AM, Prasad Koya <prasad@xxxxxxxxxx> wrote:
> Hi Peter,
>
> Thanks for your thorough review/comments. My replies are inlined.
>
> On Tue, Feb 17, 2015 at 7:34 PM, Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
> wrote:
>>
>> 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:
>>
Yes, its been quite some my first patch or problem was posted..then holiday
season etc.
>
>>
>> > 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
>
>
In our testing infrastructure we have scripts shooting commands to the
server (ethernet switch) via its console. Bunch of such servers are
connected to a terminal server. These could be commands like "ip route add
to 192.168.0.0/16 dev ma1 via 192.168.0.1" which are over 16 chars in
length. If server happens to write something out to console in the middle of
receiving above command, we are seeing receive fifo overrun.
>
>
>>
>> 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.
>
>
The sysrq-key combinations are manual entry and we'd never see this in that
case.
>
>> 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.
>>
>
When we first ran into this issue it was soon after boot where script was
configuring the server and one of our modules printed something on console.
There could be other cases where one could run into such situation.
>>
>> Do you have a test case that I can use to reproduce this?
>>
>> Why is the line speed so slow?
>>
>
Maybe this low speed requirement was something that came from our customers.
>>
>>
>> > 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.
>>
>
What I thought after your first comments was that, since it seems to take
same amount of time for serial_in() compared to udelay(1), why not read one
char from rx fifo instead of waiting with udelay().
>
>>
>> > 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.
>
No. not related to flow control.
>>
>>
>>
>> > 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)?
>>
>
not sure if i understood your question. we are doing tx and rx is done
only if lsr has DR and BI
bits set. not unconditional. the issue may be seen even at higher speeds.
>>
>> What other port types and line speeds has this been tested on besides
>> your 16550A design?
>>
>
I can try at higher speeds as well and see if the issue gets reproduced. I
don't have other port types. From what i read online, 16550s are quite
popular commercially but maybe people use it at higher speeds.
>>
>> > 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.
>>
Yes, rx here is similar to the rx in serial8250_console_putchar().
>>
>> > 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
