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

[PATCH] 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>
---
 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] serial/8250: Set fifo timeout with uart_fifo_timeout()

[Andy Shevchenko]

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?

...

>  	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 (;;) {

>  	for (;;) {
>  		status = serial_lsr_in(up);

-- 
With Best Regards,
Andy Shevchenko

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

[mcpratt]

On Monday, November 27th, 2023 at 11:07, Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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

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

[Ilpo Järvinen]

+ Cc Vamshi

On Tue, 28 Nov 2023, mcpratt@pm.me wrote:
> On Monday, November 27th, 2023 at 11:07, Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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.

Sometimes the proper way is then backported too to stables, but it needs
to be checked how complicated that seems.

So basically you'd need 31f6bd7fad3b and at least a part of f9008285bb69?

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

I wonder if fifosize is always >= 1?

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

Would you happen to have a pointer for that 20 msecs is for HW 
workaround information bit

I'd kind of want to lower it and base the extra delay on frame time 
rather than, like you say, on arbitrary number.

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

-- 
 i.

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

[Michael C. Pratt]

Hi Ilpo,


On Tuesday, November 28th, 2023 at 07:22, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Tue, 28 Nov 2023, mcpratt@pm.me wrote:
>
> > On Monday, November 27th, 2023 at 11:07, Andy Shevchenko andriy.shevchenko@linux.intel.com 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.
>
>
> Sometimes the proper way is then backported too to stables, but it needs
> to be checked how complicated that seems.
>
> So basically you'd need 31f6bd7fad3b and at least a part of f9008285bb69?

Correct, since frame_time is from 31f6bd7fad3b and the function is from f9008285bb69,
we would need both.


> > > > 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));
>
>
> I wonder if fifosize is always >= 1?


From the snippets in 8250_port.c that I have looked at,
I think it's safe to assume that it is >= 1 as long as it is defined,
and there are several spots that check whether or not it has been defined yet.

If not defined it takes the value from the constant fifo_size which is defined
for each port type to be at least 1, except for one, PORT_8250_CIR,
which most likely will never use this function anyway,
it causes many steps to be skipped in serial8250_register_8250_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...
>
>
> Would you happen to have a pointer for that 20 msecs is for HW
> workaround information bit
>
> I'd kind of want to lower it and base the extra delay on frame time
> rather than, like you say, on arbitrary number.
>
> --
> i.


When I said "some hardware-specific workaround" that was just a guess.

I decided to take a deep dive into the rabbit hole to find the source:

With the pre-git-history git repo from archive.org, I have traced the 
first instance of the comment ".02 seconds of slop" to Linux 2.1.15.

However, it is not the first instance of adding an arbitrary `HZ/50`
to a delay to make "something" work, that would be in Linux 1.3.88
for the file net/ipv4/tcp_input.c for use in tcp_send_delayed_ack().

In the 90's, each version came in the form of a single bulk patch of patches,
so I went hunting for the original patch.

27 years ago, Theodore T'so sent this patch to Linus and the mailing list:
(almost as old as me...)
(I can't find this one on lkml.org !!!)
https://lkml.iu.edu/hypermail/linux/kernel/9612.0/0501.html

Unfortunately, there's not much comment about the "slop",
but some talk of POSIX conformance and timing becoming stricter,
especially the comment which is still in master:

  * Note: we have to use pretty tight timings here to satisfy
  * the NIST-PCTS.

and

  Wait for the characters to drain based on
  info->timeout. At low baud rates (50bps), it may take a
  long time for the FIFO to completely drain out!

This makes me believe that it was added just for extra room in the timeout
to make sure that an _exact_ value for a timeout would not be too close
to a real-life absolute minimum that could cause data loss.

I noted that the `HZ/50` also appears in the same patch for defining char_time,
the timeout adds the extra time, and then char_time uses the timeout without it.

I went looking for the most recent change to char_time and found your commit 31f6bd7fad3b
(so I really went full circle this time...)
And since port->timeout has been fully replaced and the calculations separated,
there's no issue with removing the extra time from char_time but not the timeout.

However, it's clear from the inline comments that these lines were added
at a time when HZ was a constant, whereas now in modern Linux it can be variable,
like in the cases in version 2.1.15:

$ git grep 'HZ/50'
drivers/char/serial.c:		info->timeout += HZ/50;		/* Add .02 seconds of slop */
drivers/char/serial.c:		char_time = (info->timeout - HZ/50) / info->xmit_fifo_size;
drivers/net/shaper.h:#define SHAPER_BURST	(HZ/50)		/* Good for >128K then */
include/net/route.h:#define RT_REDIRECT_LOAD		(HZ/50) /* 20 msec */
net/ipv4/tcp_input.c:		if (tp->ato < HZ/50)
net/ipv4/tcp_input.c:			tp->ato = HZ/50;
net/ipv4/tcp_timer.c:		when=HZ/50;


And in master there are still a few of these...

$ git grep 'seconds of slop'
drivers/tty/amiserial.c:	info->timeout += HZ/50;		/* Add .02 seconds of slop */
drivers/tty/mxser.c:	info->timeout = timeout + HZ / 50; /* Add .02 seconds of slop */
drivers/tty/synclink_gt.c:	info->timeout += HZ/50;		/* Add .02 seconds of slop */
include/linux/serial_core.h:	/* Add .02 seconds of slop */

but less of these...

$ git grep 'HZ/50' net drivers/net include/net
drivers/net/ethernet/dec/tulip/interrupt.c:				mod_timer(&tp->timer, RUN_AT(HZ/50));
drivers/net/ethernet/dec/tulip/interrupt.c:			mod_timer(&tp->timer, RUN_AT(HZ/50));
drivers/net/wireless/virtual/mac80211_hwsim.c:	ieee80211_queue_delayed_work(hw, &hwsim->roc_start, HZ/50);


On a side note: in file drivers/tty/amiserial.c
The "NIST-PCTS" comment is there,
and char_time is defined using port->timeout and the `HZ/50` still.
This is harder to find because the function name differs from serial_core.c:
uart_wait_until_sent() was originally also named rs_wait_until_sent()
and that was also changed before the mainline git history started...

So in summary, I do believe the `HZ/50` or similar should be replaced
in all of the cases in drivers/tty, maybe `frame_time * (fifosize + 1)`
for FIFO, and in the other cases, it might be safe to remove altogether,
as long as very low baud rates are tested. But also, it might not be a bad idea
for someone to take a look at the usage of `HZ` throughout the kernel,
since it is no longer constant and these lines that were originally meant to be
an equivalence to seconds still seem to be lingering around in some areas...

And if you do decide to change/remove it, you can try to CC Theodore
and maybe he can explain it to us himself. :D

--
All the best,
MCP

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

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 09:52:19AM +0000, Michael C. Pratt wrote:
> On Tuesday, November 28th, 2023 at 07:22, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

...

> With the pre-git-history git repo from archive.org, I have traced the 
> first instance of the comment ".02 seconds of slop" to Linux 2.1.15.

Side note: Add a remote to the history.git by History group from kernel.org
(it's tag based, but allow you to browse deep to Linux 0.01):

history git://git.kernel.org/pub/scm/linux/kernel/git/history/history.git (fetch)

$ git log --oneline --no-merges 0.10
fa1ec1000cf9 (tag: 0.10) Linux 0.10 (November 11, 1991 ???)
bb441db1a90a Linux-0.01 (September 17, 1991)

-- 
With Best Regards,
Andy Shevchenko