Hi all,
On Thu, Jan 11, 2024 I submitted a single patch solution
for this issue of data overrun because of the default timeout
value which is insufficient for very low baud rates
with the internal fifo device enabled.
Following the advice of Vamshi Gajjela in the v1 thread,
I tried to find a way to limit the amount of math
being done within the write operation, and this is a possibility.
In this series, I'm proposing we bring back the
"timeout" member of struct uart_port, and also that we
add a new member of struct uart_8250_port in order
to store whether the fifo device has been enabled.
This way, in the function wait_for_lsr()
all we need to do is convert the value from jiffies to usecs
in order to have the appropriate timeout value.
I'm hoping to get this in during the RC phase of v6.9
and I'm trying to keep this fix as simple as possible,
or at least I think this is the appropriate time to
bring this issue up again...
Thanks in advance for your time.
--
MCP
Michael Pratt (3):
serial: core: Store fifo timeout again
serial: 8250: Store whether fifo device is enabled
serial: 8250: Set fifo timeout using uart_fifo_timeout()
drivers/tty/serial/8250/8250_port.c | 7 ++++++-
drivers/tty/serial/serial_core.c | 3 +++
include/linux/serial_8250.h | 1 +
include/linux/serial_core.h | 1 +
4 files changed, 11 insertions(+), 1 deletion(-)
base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680
--
2.30.2
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.
Signed-off-by: Michael Pratt <[email protected]>
---
V1 -> V2: new commit
drivers/tty/serial/serial_core.c | 3 +++
include/linux/serial_core.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ff85ebd3a007..9b3176d684a4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -414,6 +414,9 @@ uart_update_timeout(struct uart_port *port, unsigned int cflag,
temp *= NSEC_PER_SEC;
port->frame_time = (unsigned int)DIV64_U64_ROUND_UP(temp, baud);
+
+ if (port->fifosize > 1)
+ port->timeout = uart_fifo_timeout(port);
}
EXPORT_SYMBOL(uart_update_timeout);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 0a0f6e21d40e..c6422021152f 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -561,6 +561,7 @@ struct uart_port {
bool hw_stopped; /* sw-assisted CTS flow state */
unsigned int mctrl; /* current modem ctrl settings */
+ unsigned long timeout; /* character-based timeout */
unsigned int frame_time; /* frame timing in ns */
unsigned int type; /* port type */
const struct uart_ops *ops;
--
2.30.2
Currently, there are 7 checks for whether to enable
the internal fifo device of a 8250/16550 type uart.
Instead of checking all 7 values again whenever
we need to know whether we have the fifo device enabled,
store the result as a struct member of uart_8250_port.
This can, for example, lessen the amount
of calculations done during a write operation.
Signed-off-by: Michael Pratt <[email protected]>
---
V1 -> V2: new commit
drivers/tty/serial/8250/8250_port.c | 2 ++
include/linux/serial_8250.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index fc9dd5d45295..5b0cfe6bc98c 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
*/
!(up->port.flags & UPF_CONS_FLOW);
+ up->fifo_enable = use_fifo;
+
if (likely(use_fifo))
serial8250_console_fifo_write(up, s, count);
else
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index fd59ed2cca53..017429f0e743 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -127,6 +127,7 @@ struct uart_8250_port {
struct list_head list; /* ports on this IRQ */
u32 capabilities; /* port capabilities */
u16 bugs; /* port bugs */
+ unsigned int fifo_enable; /* fifo enabled if capable */
unsigned int tx_loadsz; /* transmit fifo load size */
unsigned char acr;
unsigned char fcr;
--
2.30.2
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 <[email protected]>
---
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, Apr 16, 2024 at 06:29:56PM +0000, Michael Pratt wrote:
> Currently, there are 7 checks for whether to enable
> the internal fifo device of a 8250/16550 type uart.
>
> Instead of checking all 7 values again whenever
> we need to know whether we have the fifo device enabled,
> store the result as a struct member of uart_8250_port.
>
> This can, for example, lessen the amount
> of calculations done during a write operation.
..
> @@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
> + up->fifo_enable = use_fifo;
This seems incorrect / not the only one place to assign this. What if the
console not enabled at compile time? What if it's not enabled at boot time?
--
With Best Regards,
Andy Shevchenko
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
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;
?
> }
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Tuesday, April 16th, 2024 at 14:55, Andy Shevchenko <[email protected]> wrote:
>
> > @@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>
> > + up->fifo_enable = use_fifo;
>
>
> This seems incorrect / not the only one place to assign this. What if the
> console not enabled at compile time? What if it's not enabled at boot time?
>
This is 8250 specific, and currently, it's the only place there
where it's decided whether or not to use the fifo device
by checking a bunch of flags and values.
If you're suggesting that these checks are moved out of this function somewhere else,
I would probably agree with that, but let's save that idea for the future...
If you're suggesting that there could be a null pointer, I don't think that's possible
in this function... (the name of the pointer being "up" might be confusing?)
Sorry if I'm misunderstanding what you mean.
> --
> With Best Regards,
> Andy Shevchenko
--
MCP
Hi Andy,
On Tuesday, April 16th, 2024 at 14:57, Andy Shevchenko <[email protected]> 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, Apr 16, 2024 at 07:09:52PM +0000, Michael Pratt wrote:
> On Tuesday, April 16th, 2024 at 14:55, Andy Shevchenko <[email protected]> wrote:
> > > @@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
> >
> > > + up->fifo_enable = use_fifo;
> >
> > This seems incorrect / not the only one place to assign this. What if the
> > console not enabled at compile time? What if it's not enabled at boot time?
>
> This is 8250 specific, and currently, it's the only place there
> where it's decided whether or not to use the fifo device
> by checking a bunch of flags and values.
Exactly, as initial commit is related to the kernel console _only_.
While your code, IIUC (correct me, if I'm wrong) is for any use of the port.
> If you're suggesting that these checks are moved out of this function somewhere else,
> I would probably agree with that, but let's save that idea for the future...
Not really (again, IIUC above), as console can be not enabled, and hence
serial8250_console_write() never been called and you will have false impression
that there is no FIFO in use.
> If you're suggesting that there could be a null pointer, I don't think that's possible
> in this function... (the name of the pointer being "up" might be confusing?)
>
> Sorry if I'm misunderstanding what you mean.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Tuesday, April 16th, 2024 at 14:58, Andy Shevchenko <[email protected]> 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
Hi Andy,
On Tuesday, April 16th, 2024 at 15:18, Andy Shevchenko <[email protected]> wrote:
>
>
> On Tue, Apr 16, 2024 at 07:09:52PM +0000, Michael Pratt wrote:
>
> > On Tuesday, April 16th, 2024 at 14:55, Andy Shevchenko [email protected] wrote:
>
> > > > @@ -3392,6 +3392,8 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
> > >
> > > > + up->fifo_enable = use_fifo;
> > >
> > > This seems incorrect / not the only one place to assign this. What if the
> > > console not enabled at compile time? What if it's not enabled at boot time?
> >
> > This is 8250 specific, and currently, it's the only place there
> > where it's decided whether or not to use the fifo device
> > by checking a bunch of flags and values.
>
>
> Exactly, as initial commit is related to the kernel console only.
> While your code, IIUC (correct me, if I'm wrong) is for any use of the port.
>
> > If you're suggesting that these checks are moved out of this function somewhere else,
> > I would probably agree with that, but let's save that idea for the future...
>
>
> Not really (again, IIUC above), as console can be not enabled, and hence
> serial8250_console_write() never been called and you will have false impression
> that there is no FIFO in use.
Ah ok, I understand now...
So there are cases where the function with the checks will never be called,
yet the device itself will be configured the same way and the struct member I am adding
will still be instantiated with value of 0 and never be set elsewhere... and because
it is declared in a major struct "uart_8250_port", it appears to apply to a larger scope
compared to the way it is actually being used...
(or at the very least, the name "fifo_enable" would be misleading).
Thanks for pointing that out, I'll take a deeper dive into the file...
>
> --
> With Best Regards,
> Andy Shevchenko
--
MCP
On Tue, 16 Apr 2024, Michael Pratt wrote:
> On Tuesday, April 16th, 2024 at 14:57, Andy Shevchenko <[email protected]> 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.
On Tue, 16 Apr 2024, Michael Pratt wrote:
> On Tuesday, April 16th, 2024 at 14:58, Andy Shevchenko <[email protected]> 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.
--
i.
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 <[email protected]>
> ---
> 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, 16 Apr 2024, 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.
Hi,
Why is calculating during write bad/wrong, you don't give any real
justification for this change? You're talking about "low rates" in cover
letter, which makes it even more confusing because writes occur very
infrequently so a few math operations are nothing to be concerned of (in
timescales UARTs operate in, no math penalty is really worth even
discussing IMO).
--
i.
> Signed-off-by: Michael Pratt <[email protected]>
> ---
> V1 -> V2: new commit
>
> drivers/tty/serial/serial_core.c | 3 +++
> include/linux/serial_core.h | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index ff85ebd3a007..9b3176d684a4 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -414,6 +414,9 @@ uart_update_timeout(struct uart_port *port, unsigned int cflag,
>
> temp *= NSEC_PER_SEC;
> port->frame_time = (unsigned int)DIV64_U64_ROUND_UP(temp, baud);
> +
> + if (port->fifosize > 1)
> + port->timeout = uart_fifo_timeout(port);
> }
> EXPORT_SYMBOL(uart_update_timeout);
>
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 0a0f6e21d40e..c6422021152f 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -561,6 +561,7 @@ struct uart_port {
>
> bool hw_stopped; /* sw-assisted CTS flow state */
> unsigned int mctrl; /* current modem ctrl settings */
> + unsigned long timeout; /* character-based timeout */
> unsigned int frame_time; /* frame timing in ns */
> unsigned int type; /* port type */
> const struct uart_ops *ops;
>
On Tue, Apr 16, 2024 at 07:20:18PM +0000, Michael Pratt wrote:
> Hi Andy,
>
> On Tuesday, April 16th, 2024 at 14:58, Andy Shevchenko <[email protected]> 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.
>
As we are talking about a device with a scale of KB/s, I am attempted to
suggest to call uart_fifo_timeout() unconditionally.
> 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
>