On Monday, November 27th, 2023 at 11:07, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> On Sat, Nov 25, 2023 at 06:36:32AM +0000, Michael Pratt wrote:
>
> > Commit 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
> > reworked functions for basic 8250 and 16550 type serial devices
> > in order to enable and use the internal FIFO device for buffering,
> > however the default timeout of 10 ms remained, which is proving
> > to be insufficient for low baud rates like 9600, causing data overrun.
> >
> > Unforunately, that commit was written and accepted just before commit
> > 31f6bd7fad3b ("serial: Store character timing information to uart_port")
> > which introduced the frame_time member of the uart_port struct
> > in order to store the amount of time it takes to send one UART frame
> > relative to the baud rate and other serial port configuration,
> > and commit f9008285bb69 ("serial: Drop timeout from uart_port")
> > which established function uart_fifo_timeout() in order to
> > calculate a reasonable timeout to wait for all frames
> > in the FIFO device to flush before writing data again
> > using the now stored frame_time value and size of the buffer.
> >
> > Fix this by using the new function to calculate the timeout
> > whenever the buffer is larger than 1 byte (unknown port default).
> >
> > Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200.
>
>
> Do we need a Fixed tag?
>
> ...
Hi Andy,
I'm not sure whether this qualifies as a "bug fix" or not,
since the proper way to handle it was introduced after the "bad" commit,
and the "bad" commit happens to still work fine for anyone running the
standard 115200 baud or higher.
For that matter, I'm not even sure if this affects other hardware,
I'm only able to test this on a MIPS SOC, and I wonder if anyone can
reproduce it on something else.
If that level of accuracy doesn't matter for tags, then yes I suppose
it should be tagged as "Fixes".
>
> > unsigned int status, tmout = 10000;
> >
> > - /* Wait up to 10ms for the character(s) to be sent. /
> > + / Wait for a time relative to buffer size and baud */
> > + if (up->port.fifosize > 1)
> > + tmout = jiffies_to_usecs(uart_fifo_timeout(&up->port));
>
>
> Why can't we simply use this one?
>
> unsigned int status, tmout;
>
> tmout = jiffies_to_usecs(uart_fifo_timeout(&up->port));
>
> > for (;;) {
> > status = serial_lsr_in(up);
Again, I'm not sure which is better for performance, between adding
a conditional check or doing the math for every case.
The 10 ms timeout has been there since the beginning of the git history,
so clearly it is enough for single-frame transfers at any baud.
The new function uart_fifo_timeout() provides a variable timeout, but starting out
with an arbitrary 20 ms as a minimum, which I think can be traced back
to some hardware-specific workaround... but definitely much more
than what's needed for most cases (115200 baud needs at least ~1.5 ms).
I'd let you all decide, and I can adjust the patch if needed.
--
Thanks for your time
MCP
