Hi. Could someone review this patch?
Thank you.
On Tue, Dec 9, 2014 at 11:34 PM, Prasad Koya <prasad.koya@xxxxxxxxx> wrote:
> Hi
>
> Below version is tested with 3.18 kernel on 16550 UART.
>
> - Added a new variable 'writing_to_con' to 'struct uart_8250_port'.
> This would be set to 1 inside serial8250_console_write() only if
> port_sysrq is 0 and oops_in_progress is 0 and "receive interrupts" are
> enabled.
>
> - uart_console_write() is called to send out whole printk message as
> earlier. wait_for_xmitr() invokes serial8250_rx_chars() only if
> writing_to_con is set and if UART_LSR_DR is set. so instead of waiting
> with usleep(1) it now buffers chars from receive queue.
>
> - if serial_8250_chars() is called with writing_to_con=1,
> serial8250_rx_chars() would read one char from UART rx buffer.
>
> - as long as serial8250_rx_chars() does not see brk i.e., sysrq is not
> set, it could buffer. if we split this functionality, driver would have
> to buffer each char and flags associated with it. with this approach,
> serial8250_rx_chars() will buffer "regular" chars but anything after
> sysrq is left to be processed after serial8250_console_write()
> returns.
>
> Thanks.
>
>
> From 46d5da6069296132d3e9f505715ab7f30701b2a5 Mon Sep 17 00:00:00 2001
> From: Prasad Koya <prasad@xxxxxxxxxx>
> Date: Tue, 9 Dec 2014 22:53:31 -0800
> Subject: [PATCH 1/1] tty, serial, 8250: to avoid recv fifo overrun, read rx
> fifo during xmit
>
> 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, buffer char from rx fifo. Stop buffering if
> port->sysrq is true. wait_for_xmitr() could pick up a byte from FIFO
> (if UART_LSR_DR is set) rather than calling udelay(1).
>
> Signed-off-by: Prasad Koya <prasad@xxxxxxxxxx>
> ---
> drivers/tty/serial/8250/8250_core.c | 26 ++++++++++++++++++++++++--
> include/linux/serial_8250.h | 2 ++
> 2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_core.c
> b/drivers/tty/serial/8250/8250_core.c
> index ca5cfdc..b3d3eda 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1422,7 +1422,7 @@ serial8250_rx_chars(struct uart_8250_port *up,
> unsigned char lsr)
> {
> struct uart_port *port = &up->port;
> unsigned char ch;
> - int max_count = 256;
> + int max_count = up->writing_to_con ? 1 : 256;
> char flag;
>
> do {
> @@ -1482,6 +1482,12 @@ serial8250_rx_chars(struct uart_8250_port *up,
> unsigned char lsr)
> uart_insert_char(port, lsr, UART_LSR_OE, ch, flag);
>
> ignore_char:
> + /*
> + * We see BRK while buffering rx bytes. Stop buffering and
> + * let SysRq or SAK handling be done after console_write
> + */
> + if (port->sysrq && up->writing_to_con)
> + break;
> lsr = serial_in(up, UART_LSR);
> } while ((lsr & (UART_LSR_DR | UART_LSR_BI)) && (max_count-- > 0));
> spin_unlock(&port->lock);
> @@ -1959,7 +1965,15 @@ static void wait_for_xmitr(struct
> uart_8250_port *up, int bits)
> break;
> if (--tmout == 0)
> break;
> - udelay(1);
> + /*
> + * We are writing to console with interrupts disabled.
> + * read recv FIFO to avoid buffer overrun
> + */
> + if (up->writing_to_con && (status & UART_LSR_DR))
> + serial8250_rx_chars(up, status);
> + else
> + udelay(1);
> +
> }
>
> /* Wait up to 1s for flow control if necessary */
> @@ -3102,6 +3116,7 @@ static void __init serial8250_isa_init_ports(void)
> init_timer(&up->timer);
> up->timer.function = serial8250_timeout;
> up->cur_iotype = 0xFF;
> + up->writing_to_con = 0;
>
> /*
> * ALPHA_KLUDGE_MCR needs to be killed.
> @@ -3213,8 +3228,15 @@ serial8250_console_write(struct console *co,
> const char *s, unsigned int count)
> else
> serial_port_out(port, UART_IER, 0);
>
> + /* Is UART configured to send interrupts for received data */
> + if (((ier & UART_IER_RDI) == UART_IER_RDI) && !oops_in_progress &&
> + !port->sysrq)
> + up->writing_to_con = 1;
> +
> uart_console_write(port, s, count, serial8250_console_putchar);
>
> + if (up->writing_to_con)
> + up->writing_to_con = 0;
> /*
> * Finally, wait for transmitter to become empty
> * and restore the IER
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index 3df10d5..1ad79e2 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -96,6 +96,8 @@ struct uart_8250_port {
> #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
> unsigned char msr_saved_flags;
>
> + /* Write to console on this port now */
> + unsigned char writing_to_con;
> struct uart_8250_dma *dma;
> struct serial_rs485 rs485;
>
> On Fri, Oct 3, 2014 at 5:57 AM, Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> wrote:
>> Hi Prasad,
>>
>> On 09/30/2014 01:30 PM, Prasad Koya wrote:
>>> Hi Ted
>>>
>>> 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.
>>>
>>> To get around that, I am trying below patch on 3.17 kernel. This is
>>> tested and working on 3.4 kernel. I wanted to get some comments from
>>> linux-serial mailing list.
>>>
>>> If UART_IER_RDI is enabled, for every quarter of fifo size bytes sent,
>>> console_write() checks to see if there is data in receive fifo. If so,
>>> it lets serial8250_rx_chars() pick those bytes and buffer them in tty
>>> layer.
>>
>> The issue here occurs irrespective of whether a UART has a FIFO; it's
>> even easier to get an overrun without a FIFO.
>>
>>> --- linux-3.17rc6-orig/linux-3.17-rc6/drivers/tty/serial/8250/8250_core.c
>>> 2014-09-21 15:43:02.000000000 -0700
>>> +++ linux-3.17-rc6/drivers/tty/serial/8250/8250_core.c 2014-09-30
>>> 10:08:06.401242782 -0700
>>> @@ -3004,6 +3004,7 @@
>>> unsigned long flags;
>>> unsigned int ier;
>>> int locked = 1;
>>> + int to_send, offset, fifo = count;
>>>
>>> touch_nmi_watchdog();
>>>
>>> @@ -3022,8 +3023,27 @@
>>> else
>>> serial_port_out(port, UART_IER, 0);
>>>
>>> - uart_console_write(port, s, count, serial8250_console_putchar);
>>> + offset = 0;
>>> + if (uart_config[up->port.type].fcr & UART_FCR_ENABLE_FIFO) {
>>> + if (!oops_in_progress && ((ier & UART_IER_RDI) == UART_IER_RDI))
>>> + fifo = port->fifosize >> 2;
>>> + }
>>> +
>>> +send_rest:
>>> + to_send = count > fifo ? fifo : count;
>>>
>>> + uart_console_write(port, s + offset, to_send,
>>> serial8250_console_putchar);
>>> +
>>> + count -= to_send;
>>> + if (count > 0) {
>>> + unsigned int status;
>>> + status = serial_in(up, UART_LSR);
>>> + if (status & (UART_LSR_DR | UART_LSR_BI))
>>> + serial8250_rx_chars(up, status);
>>
>> This will potentially take up to 256 chars in a loop, all with interrupts
>> still off, and then drops the port lock which isn't ok in the context of
>> a console message (with the way the driver works now).
>>
>> serial8250_rx_chars() doesn't need to drop the port lock anymore; so that
>> solves that problem.
>>
>> Also, serial8250_rx_chars() is where sysrq and SAK handling is done;
>> this would allow it to be re-entered, which would be bad. So rx cannot/
>> should not be happening if either oops_in_progress or port->sysrq is true.
>>
>> I think a better way to do this would be to train wait_for_xmitr() to
>> receive data to a temp buffer (if bits is set to the requisite LSR bits and
>> not if oops_in_progress or port->sysrq). serial8250_rx_chars() would
>> need refactoring to split out the per-char handling so it could work on
>> the temp buffer once the console message was written.
>>
>> 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
