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.

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);
 
-- 
2.30.2

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?

-- 
 i.

On Tue, Apr 16, 2024 at 06:34:10PM +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 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.

...

>  	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?

-- 
With Best Regards,
Andy Shevchenko

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

On Tue, 16 Apr 2024, Michael Pratt wrote:
> 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?

Yes we definitely should be able to. Unfortunately these patches just keep 
coming back not in the form that follows the review feedback, but they 
come up their own way of doing things which is way worse and ignores the 
given feedback.

> 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.

"without a timeout" is not what Andy said. He said you should always have 
a timeout, regardless of there being FIFO or not. And that timeout should 
be derived in the same manner from baudrate and FIFO size (to address the
cases w/o FIFO, the fifosize should be lower bounded to 1 while 
calculating the FIFO timeout).

> 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.

While I agree the general principle, that this is backup that should not 
even be needed, the statement is partly incorrect. We don't get interrupts 
during console write because they're disabled. But LSR should still change 
and allow progress without the backup timeout.

-- 
 i.