| Message ID | 20240416182741.22514-1-mcpratt@pm.me |
|---|---|
| Headers | show |
| Series | serial: 8250: Set fifo timeout with uart_fifo_timeout() | expand |
On Tue, Apr 16, 2024 at 06:29:28PM +0000, Michael Pratt wrote: > This is a partial revert of Commit f9008285bb69 > ("serial: Drop timeout from uart_port"). > > In order to prevent having to calculate a timeout > for the fifo device during a write operation, if enabled, > calculate it ahead of time and store the value of the timeout > in a struct member of uart_port. ... > + if (port->fifosize > 1) > + port->timeout = uart_fifo_timeout(port); else port->timeout = port->frame_time; ? > }
Hi Andy, On Tuesday, April 16th, 2024 at 14:57, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > 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->fifo_enable && up->port.timeout) > > + tmout = jiffies_to_usecs(up->port.timeout); > > > Why do we still use that default? Can't we calculate timeout even for\ > FIFO-less / FIFO-disabled devices? Maybe it's possible that there is some kind of rare case where the LSR register is not working or not configured properly for a device in which support is being worked on...without a timeout, that would result in an infinite loop. AFAIK, when everything is working properly, there is no such thing as needing a timeout for a uart device without fifo, as every single byte written would trigger an interrupt anyway. > -- > With Best Regards, > Andy Shevchenko -- MCP
Hi Andy, On Tuesday, April 16th, 2024 at 14:58, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > ... > > > + if (port->fifosize > 1) > > + port->timeout = uart_fifo_timeout(port); > > > else > port->timeout = port->frame_time; > Consistent with what I said in the other reply, the only reason that I have an if statement here, is to avoid doing extra math for devices without a fifo, as a specifically calculated timeout value would be useless in those cases. However, if you don't like the 10 ms default timeout, perhaps port->frame_time could actually be a more reasonable default value? That is, provided that we have a process for calculating the proper value already in place... > > -- > With Best Regards, > Andy Shevchenko Thanks for taking a look :D -- MCP
On Tue, 16 Apr 2024, 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 until flushing > the fifo device before writing data again when partially filled, > using the now stored frame_time value and size of the buffer. > > Fix this by using the stored timeout value made with this new function > to calculate the timeout for the fifo device, when enabled, and when > the buffer is larger than 1 byte (unknown port default). > > The previous 2 commits add the struct members used here > in order to store the values, so that the calculations > are offloaded from the functions that are called > during a write operation for better performance. > > Tested on a MIPS device (ar934x) at baud rates 625, 9600, 115200. Did you actually see some perfomance issue with 115000 bps? Or did you just make the "better performance" claim up? > Fixes: 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver") > Signed-off-by: Michael Pratt <mcpratt@pm.me> > --- > V1 -> V2: > Use stored values instead of calling uart_fifo_timeout() > or checking capability flags. > The existence of the timeout value satisfies fifosize > 1. > > drivers/tty/serial/8250/8250_port.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index 5b0cfe6bc98c..cf67911a74f5 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -2066,7 +2066,10 @@ static void wait_for_lsr(struct uart_8250_port *up, int bits) > { > 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->fifo_enable && up->port.timeout) > + tmout = jiffies_to_usecs(up->port.timeout); > + > for (;;) { > status = serial_lsr_in(up); Hi, Is this the only reason for adding the timeout field? I think you should just refactor the code such that you don't need to recalculate the timeout for each characted when writing n characters?
On Tue, 16 Apr 2024, Michael Pratt wrote: > On Tuesday, April 16th, 2024 at 14:58, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > + if (port->fifosize > 1) > > > + port->timeout = uart_fifo_timeout(port); > > > > > > else > > port->timeout = port->frame_time; > > > > > Consistent with what I said in the other reply, the only reason that > I have an if statement here, is to avoid doing extra math for devices > without a fifo, as a specifically calculated timeout value would be useless > in those cases. Please benchmark to show this actually matters if want to make this claim. Otherwise just do the math always. > However, if you don't like the 10 ms default timeout, perhaps port->frame_time > could actually be a more reasonable default value? That is, provided > that we have a process > for calculating the proper value already in place... While it would be a step toward the correct direction, you'd still need to add the safety there which is already done by uart_fifo_timeout(). So no, I don't think there's advantage of using port->frame_time over just calling uart_fifo_timeout() and ensuring uart_fifo_timeout() is always using at least 1 as the FIFO size when it does the calculations.