On 02/18/2015 03:40 AM, Prasad Koya wrote:
> On Wed, Feb 18, 2015 at 12:35 AM, Prasad Koya <prasad@xxxxxxxxxx> wrote:
>> On Tue, Feb 17, 2015 at 7:34 PM, Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
>> wrote:
>>> 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.
Thanks for explaining that.
I just want to be honest right here: I'm reluctant to recommend such an
invasive and consequential change for an automated test harness that
can be worked around at the sender end.
I'm not implying yours is not a valid use-case; it is. But there are a
number of factors which weigh against it:
1. It makes serial console output less reliable. This is already a problem
with printk().
2. It imposes a significant maintenance burden by linking areas of code
that were separate before.
3. It's had really limited testing.
4. There are other workarounds available.
At a minimum this patch should have a CONFIG_ knob which is by default disabled.
FWIW, I carry a number of patchsets outside mainline because they're
simply not suitable, either because they're really only useful to me
or because they're too ugly or whatever.
>>> 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.
That's what I thought; I just wanted to be sure.
>>> 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.
Ok, this makes sense. The console goes online way earlier than the network
stack.
>>> 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.
So it is an actual requirement and not simply because the default console
speed is 9600?
If possible, I'd rather fix this problem by setting the console line speed
on the kernel command line from your bootloader.
[...]
>>>> +/*
>>>> * 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().
I did think about that too, but I'm not sure it matters much.
The line status is being heavily oversampled as it is.
>>
>>>
>>>> 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.
Ok.
[...]
>>>> +
>>>> #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.
I meant unconditionally with respect to the observed problem:
the commit log indicates that the problem is limited to slow line speeds
on uarts with small fifos, but yet the patch applies to all uarts at
all line speeds.
If the problem can happen at any line speed then the commit log shouldn't
say it only happens at slow line 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.
I have many and they do not all behave similarly, that's why I'm asking
which platforms and conditions this has been tested under.
>>>
>>>> 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().
I think you may misunderstand what I mean here.
I don't think rx could overflow while waiting for the transmitter to empty
here:
1. the last output char has just been written to the tx fifo by
serial8250_console_char()
2. this means the tx fifo can have at most 1 char in it right now
3. the rx fifo was just read at most 1us before the tx fifo was written
4. so this call to wait_for_xmitr() is only waiting for 1 char to be output
plus how ever much data the shifter can hold, which probably isn't much
How could the rx fifo overflow in the time it takes the xmitr to send
1-2 chars, especially since the rx fifo was being polled at 1000x oversample
(every 1us for each 1ms tx char)?
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
