Hi, Thanks for reviewing the patch and reworking it. It looks good. It makes sense to change wait_for_xmitr(). And the logic to call serial8250_rx_chars() everytime only if !emergency_situation() is good. I can give this patch a try. In your previous comment (http://marc.info/?l=linux-serial&m=141900724429124&w=2), since I initialized writing_to_console to 0, and is set to 1 only if certain condition is true, I therefore did below code to reset that: if (up->writing_to_con) up->writing_to_con = 0; so as long as writing_to_con is not 0, it should go into that loop. What is wrong with that? Another way would be to do "if (up->writing_to_con != 0)" which may make compiler use 'cmp' and 'jne' instructions. >>>>>>>>>>>>>>>>>>>> Two other, deeper issues with your patch: 1. It doesn't compile if SUPPORT_SYSRQ is not set. 2. The spin_unlock(&port->lock); tty_flip_buffer_push(&port->state->port); spin_lock(&port->lock); at the end of serial8250_rx_chars is risky during polled console output. To start with, serial8250_console_write doesn't necessarily lock the port in the first place! Fortunately, the condition under which it might not (port->sysrq || oops_in_progress) is exactly that during which input isn't processed. Should the code instead just buffer to the flip_buffer and flip it when returning from serial8250_console_write? Looking at it, I'm not sure the spin_unlock is even required. tty_flip_buffer_push(port) -> tty_schedule_flip(port) -> schedule_work(work) -> queue_work(system_wq, work) -> queue_work_on(WORK_CPU_UNBOUND, system_wq, work) -> __queue_work() and I don't see anything that needs the port unlocked. Anyway, my suggestion to pass the flag down is illustrated in the following (untested) patch. Also note the emergency_situation() function, which ensures the logic is the same in the various locations. diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index ca5cfdc..3a7af36 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -1416,9 +1416,13 @@ 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 if set 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; @@ -1454,8 +1458,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; + } } else if (lsr & UART_LSR_PE) port->icount.parity++; else if (lsr & UART_LSR_FE) @@ -1591,7 +1598,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir) dma_err = serial8250_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 && (status & UART_LSR_THRE)) @@ -1943,9 +1950,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 up->port.sysrq != 0 || oops_in_progress; +#else + (void)port; + return oops_in_progress; +#endif +} + +/* * 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; @@ -1959,6 +1987,9 @@ 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); udelay(1); } @@ -1970,12 +2001,24 @@ 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); + } udelay(1); touch_nmi_watchdog(); } } } +static void wait_for_xmitr(struct uart_8250_port *up, int bits) +{ + __wait_for_xmitr(up, bits, false); +} + #ifdef CONFIG_CONSOLE_POLL /* * Console polling routines for writing and reading from the uart while @@ -3175,7 +3218,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); serial_port_out(port, UART_TX, ch); } @@ -3183,6 +3226,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 @@ -3198,7 +3245,7 @@ serial8250_console_write(struct console *co, const char *s, unsigned int count) serial8250_rpm_get(up); - if (port->sysrq || oops_in_progress) + if (emergency_situation(port)) locked = spin_trylock_irqsave(&port->lock, flags); else spin_lock_irqsave(&port->lock, flags); @@ -3219,7 +3266,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); 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 3df10d5..8735c9c 100644 --- a/include/linux/serial_8250.h +++ b/include/linux/serial_8250.h @@ -131,7 +131,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 21c2e05..2739fac 100644 --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -389,7 +389,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 */ return 1; } port->sysrq = 0; -- 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 [prev in list] [next in list] [prev in thread] [next in thread] Configure | About | News | Add a list | Sponsored by KoreLogic On Fri, Jan 9, 2015 at 2:37 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > On Wed, Jan 07, 2015 at 03:30:48PM -0800, Prasad Koya wrote: >> Hi Greg >> >> Did you get any chance to look at below patch? >> >> [PATCH 1/1] tty, serial, 8250: to avoid recv fifo overrun, read rx >> fifo during xmit > > I have processed all tty/serial patches in my queue now, if I missed > something, please resend. > > thanks, > > greg k-h -- 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
