> Hi Prasad,
I'm responding because Preasend's current patch was mostly rewritten by me.
First of all, thank you for making specific alternate suggestions!
I'll try to implement them.
> What exactly is the use case here? I ask because
The one that occurred to me was two machines with cross-connected serial
conoles logging to each other. You want each to capture 100% of the
data from the other.
If one outputs an important printk longer than its Tx FIFO, and the
other happens to be talking at the same time, the Rx FIFO will overrun
during transmission.
For standard 16-byte FIFOs, that's not much.
> 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.
1a. It doesn't matter, because the serial console is taking
input from a machine, and SysRq is normally for human use.
1b. The patch should work for both, right? It accepts imput
during any polled output, including SysRq output.
> 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.
Suppose you're debugging one or both machines over serial console to catch
a crash/lockup crashing condition. Then both are logging reasonably
frequently.
> Why is the line speed so slow?
I wonder, too, but the problem is the same at any line speed.
> 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).
A more aggressive refactoring, but I can try that.
>> /*
>> + * 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).
Er, but the first is waiting for the LSR, while the second is waiting
for the MSR. What kind of parameter should I pass in to select the
termination conditions?
I guess the LSR bits, and if they're 0, that means return if msr &
UART_MSR_CTS.
I can do it, and I can't exactly picture it yet, but I worry it'll
be a bit ugly, too.
> 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.
I don't understand that.
First of all, wait_for_xmitr polls the transmit condition first,
so the Rx FIFO is checked only if the TX FIFO is full. If both
FIFOs are enormous, the latter will never happen, so the former never
even gets checked.
Second, serial8250_rx_chars() has a 256-char maximum (see the "max_count"
local variable). Is that really terrible?
If the objective is not to drop incoming data, then the data has to be
read *sometime*, and now is not greatly worse than in an ISR later.
> 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.
I was indeed just avoiding more invasive refactoring when I didn't feel I
understaood the larger implications. I'll happily do more if encouraged.
How about this alternative, which seems even simpler:
Rather than a "no_sysrq" flag, make serial8250_rx_chars take the maximum
number of characters to process as a parameter. Then most code paths
can call it with "256", but wait_for_xmitr can call it with 1.
Alternatively, refactor it to handle multiple normal characters, but
have errors and break and so on return to a caller, and have wait_for_xmitr
stop callin it in that case. Then you could use a larger number, line 16.
(But is 255 really so bad?)
>> @@ -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.
Um, what does UPF_CONS_FLOW have to do with this?
This is the heart of the entre patch: adding Rx handling while
wait_for_xmitr is waiting. Dropping this hunk would render the
entire patch useless.
> 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.
Oh, is *that* what GregKH was talking about? I was assuming he didn't
like bool parameters in the called function; I didn't think about the
call site. Yes, random integer or bool parameters can be confusing.
> 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)?
Neither am I; I think Prasad is just plain wrong about that. Only the
ratio of Rx and Tx speeds matters, and given that it can be a problem
at the common ratio of 1:1...
>> @@ -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.
Um, huh? serial8250_console_putchar only waits for THRE (Tx FIFO not
full), so it could have filled the Tx FIFO. If the Rx and Tx FIFOs are
equal size, then by the time TEMT is set the Rx GFIFO could have overrun.
>> 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.
Do you mean "people don't need reminding of that", or the comment is
stating a requirement that's not real? My general habit is to add
somments when I have to study code carefully to understand it.
And I think that it actually is necessary, although for subtle reasons.
--
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
