[PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[Michael Pratt]

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.

Signed-off-by: Michael Pratt <mcpratt@pm.me>
---
v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/

 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 8ca061d3bbb9..777b61a79c5e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2076,7 +2076,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->port.fifosize > 1)
+		tmout = jiffies_to_usecs(uart_fifo_timeout(&up->port));
+
 	for (;;) {
 		status = serial_lsr_in(up);
 
-- 
2.30.2

Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[Greg Kroah-Hartman]

On Thu, Jan 11, 2024 at 12:27:07AM +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.
> 
> Signed-off-by: Michael Pratt <mcpratt@pm.me>
> ---
> v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/

What commit id does this fix?

thanks,

greg k-h

Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[Michael Pratt]

On Thursday, January 11th, 2024 at 01:52, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jan 11, 2024 at 12:27:07AM +0000, Michael Pratt wrote:
> 
> > Commit 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")

This is the commit that made the issue present itself.

I'm not sure whether it's right to say that this "fixes" that commit
since it only caused the issue indirectly, and the diff for this patch
doesn't touch any lines that the other commit touched, and the method
I'm using to fix the issue was not available at the time, and also that
for high baud rates like 115200 everything is still fine...
(the 10 ms timeout is as old as the tree)

If that's enough for a "Fixes" tag then go ahead (or tell me to add it),
but maybe a "Ref" tag would be enough?

You can see the other thread linked for more discussion on that point if you like...

> > 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.
> > 
> > Signed-off-by: Michael Pratt mcpratt@pm.me
> > ---
> > v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
> 
> 
> What commit id does this fix?
> 
> thanks,
> 
> greg k-h

--
MCP

Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[Ilpo Järvinen]

On Thu, 11 Jan 2024, Michael Pratt wrote:

> 
> 
> On Thursday, January 11th, 2024 at 01:52, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Jan 11, 2024 at 12:27:07AM +0000, Michael Pratt wrote:
> > 
> > > Commit 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
> 
> This is the commit that made the issue present itself.
> 
> I'm not sure whether it's right to say that this "fixes" that commit
> since it only caused the issue indirectly, and the diff for this patch
> doesn't touch any lines that the other commit touched, and the method
> I'm using to fix the issue was not available at the time, and also that
> for high baud rates like 115200 everything is still fine...
> (the 10 ms timeout is as old as the tree)
> 
> If that's enough for a "Fixes" tag then go ahead (or tell me to add it),
> but maybe a "Ref" tag would be enough?
> 
> You can see the other thread linked for more discussion on that point if you like...

Besides Fixes tags, you can add dependency information for stable folks 
with Cc tag using the format described in 
Documentation/process/stable-kernel-rules.rst

-- 
 i.


> > > 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.
> > > 
> > > Signed-off-by: Michael Pratt mcpratt@pm.me
> > > ---
> > > v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
> > 
> > 
> > What commit id does this fix?
> > 
> > thanks,
> > 
> > greg k-h
> 
> --
> MCP
> 

Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[VAMSHI GAJJELA]

On Thu, Jan 11, 2024 at 5:57 AM Michael Pratt <mcpratt@pm.me> 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.
>
> Signed-off-by: Michael Pratt <mcpratt@pm.me>
> ---
> v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
>
>  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 8ca061d3bbb9..777b61a79c5e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2076,7 +2076,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->port.fifosize > 1)
> +               tmout = jiffies_to_usecs(uart_fifo_timeout(&up->port));
> +
Michael, I have copied you onto a patch addressing the similar issue, where
it was considered to avoid the computation of timeout on call to wait_for_lsr.
>         for (;;) {
>                 status = serial_lsr_in(up);
>
> --
> 2.30.2
>
>
>

Re: [PATCH v1 RESEND] serial: 8250: Set fifo timeout with uart_fifo_timeout()

[Ilpo Järvinen]

On Thu, 11 Jan 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

Unfortunately,

-- 
 i.

> 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.
> 
> Signed-off-by: Michael Pratt <mcpratt@pm.me>
> ---
> v1 thread: https://lore.kernel.org/linux-serial/20231125063552.517-1-mcpratt@pm.me/
>  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 8ca061d3bbb9..777b61a79c5e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2076,7 +2076,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->port.fifosize > 1)
> +		tmout = jiffies_to_usecs(uart_fifo_timeout(&up->port));
> +
>  	for (;;) {
>  		status = serial_lsr_in(up);
>  
>