Hi George,
On 02/18/2015 12:31 AM, George Spelvin wrote:
[...]
>>> /*
>>> + * 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.
I think you misunderstand my suggestion. For example, if
serial8250_rx_chars() is refactored (without functional changes) into
static void serial8250_rx_one_char(struct uart_8250_port *up, unsigned char lsr)
{
if (lsr & UART_LSR_DR)
chr = serial_in()
...
uart_insert_char(port, lsr, UART_LSR_OE, ch, flag);
}
unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
{
do {
serial8250_rx_one_char(up, lsr);
lsr = serial_in(up, UART_LSR);
} while (....)
....
}
Then,
#ifdef CONFIG_SERIAL_8250_CONSOLE
static void rx_while_wait_for_xmitr(struct uart_8250_port *up, unsigned char lsr)
{
if (lsr & UART_LSR_DR && !port->sysrq && !oops_in_progress &&
!(lsr & UART_LSR_BRK_ERROR_BITS))
serial8250_rx_one_char(up, lsr);
}
#else
#define rx_while_wait_for_xmitr(up, lsr) do { } while (0)
#endif
Then, in __wait_for_xmitr()
for (;;) {
status = serial_in(up, UART_LSR);
...
if (--tmout == 0)
break;
if (rx)
rx_while_wait_for_xmitr(up, status);
udelay(1);
}
>> 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.
No. The rx fifo is checked if the tx fifo is _not empty_, and we may
have just missed the tx fifo going empty.
Conversely, the rx fifo could be nearly full.
> 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.
Now is _considerably_ worse than an ISR later;
Rx overrun is not the worst thing that could be happening on the
machine right now which the console may be trying to communicate.
>> 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?
UPF_CONS_FLOW usage is very limited, and I don't think rx is appropriate;
why not drop RTS? Not that I'm suggested this patch should do that, but
rather that this patch should focus on fixing only the observed problem.
> 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.
This hunk isn't even getting tested.
>> 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'm not sure; I think he was actually objecting to the idea of
combining conditional tests into a standalone function.
> 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.
[...]
>>> @@ -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.
No. THRE = Tx fifo Empty, so at most there is 1 char in the tx fifo.
The console output waits for the tx fifo to be _empty_ and then only writes
1 char. It behaves this way because it has no idea how big the tx fifo is
and how much data might be in the tx fifo when console output started.
If the actual shifter is sufficiently small (which I would think would be
the case on a low-cost design with a small fifo), then the rx fifo cannot
overflow in the time it takes for 1 char plus the remainder in the shifter
to be transmitted.
> 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?
The comment doesn't really convey useful information; anyone needing to
understand the behavior of sysrq is still going to need to analyze the
code. IOW, no one will see that comment and think to themselves, "ah,
that's why that was done".
> My general habit is to add
> somments when I have to study code carefully to understand it.
I do that too, but I don't always submit that, because often it only
makes sense to me.
> And I think that it actually is necessary, although for subtle reasons.
I'd prefer a separate patch with a kernel-doc comment header for
uart_handle_sysrq_char() and uart_handle_break(), explaining the
behavior of port->sysrq and the "subtle reasons".
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
