tl;dr: I'm sorry to have needed remedial 16550 education. I was being thick, and you're absolutely right. Will fix. Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> wrote: > On 02/18/2015 12:31 AM, George Spelvin wrote: > 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. Oh, right! I forgot about that 16550 misfeature^Wquirk. I knew that, just forgot it last night. And you're absolutely right it's a problem. Mea culpa. Yes. THRE means "FIFO not empty" and to make use of the Tx FIFO you have to magically know the FIFO size to know how many characters it's safe to put in. Which in turn is why the polling output has to drain the FIFO before handing the port back, because the regular transmitter may be in the middle of a "stuff up->tx_loadsz bytes into the FIFO" loop. As I said, mea culpa, thank you, will fix. >>> 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. Oh! You meant "drop RTS". I thought you meant "drop this hunk". >> 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. I'm not seeing this right this moment, but I'm going to shut up and study it before running my mouth off again. >> 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. Yes, as I said above I had repressed that horrible memory. :-) Now this all makes sense; I apologize for wasting your time re-explaining it. > 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. I think the idea is more "I'm polling anyway, *and* I have to wait for the FIFO to drain, so I don't need the FIFO for interrupt mitigation". The emergency transmit could use the FIFO, but it wouldn't shrink the interrupts-disabled window unless we let it return with a non-empty FIFO, and that would require some interlocking with the regular serial8250_tx_chars path. I'm not I'd survive the torches & pitchforks if I seriously proposed that. > 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 I understand what you mean by "the actual shifter", i.e. the difference between THRE and TEMT, then that's always 8 bits (+ start + stop). (Except for UART_CAP_HFIFO, which is just weird.) >> 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". Okay. -- 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
